| Message ID | 1513949488-13026-4-git-send-email-faiz_abbas@ti.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | None | expand |
On 12/22/2017 02:31 PM, Faiz Abbas wrote: > From: Franklin S Cooper Jr <fcooper@ti.com> > > Add support for PM Runtime which is the new way to handle managing clocks. > However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk > management approach in place. There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate CONFIG_PM_RUNTIME") Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : >> Well, I admit it would be nicer if drivers didn't have to worry about >> whether or not CONFIG_PM was enabled. A slightly cleaner approach >> from the one outlined above would have the probe routine do this: >> >> my_power_up(dev); >> pm_runtime_set_active(dev); >> pm_runtime_get_noresume(dev); >> pm_runtime_enable(dev); > PM_RUNTIME is required by OMAP based devices to handle clock management. > Therefore, this allows future Texas Instruments SoCs that have the MCAN IP > to work with this driver. Who will set the SET_RUNTIME_PM_OPS in this case? > Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> > [nsekhar@ti.com: handle pm_runtime_get_sync() failure, fix some bugs] > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > --- > drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c > index f72116e..53e764f 100644 > --- a/drivers/net/can/m_can/m_can.c > +++ b/drivers/net/can/m_can/m_can.c > @@ -23,6 +23,7 @@ > #include <linux/of.h> > #include <linux/of_device.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #include <linux/iopoll.h> > #include <linux/can/dev.h> > > @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv) > { > int err; > > + err = pm_runtime_get_sync(priv->device); > + if (err) { > + pm_runtime_put_noidle(priv->device); Why do you call this in case of an error? > + return err; > + } > + > err = clk_prepare_enable(priv->hclk); > if (err) > - return err; > + goto pm_runtime_put; > > err = clk_prepare_enable(priv->cclk); > if (err) > - clk_disable_unprepare(priv->hclk); > + goto disable_hclk; > > return err; > + > +disable_hclk: > + clk_disable_unprepare(priv->hclk); > +pm_runtime_put: > + pm_runtime_put_sync(priv->device); > + return err; > } > > static void m_can_clk_stop(struct m_can_priv *priv) > { > + pm_runtime_put_sync(priv->device); > + > clk_disable_unprepare(priv->cclk); > clk_disable_unprepare(priv->hclk); > } > @@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev) > /* Enable clocks. Necessary to read Core Release in order to determine > * M_CAN version > */ > + pm_runtime_enable(&pdev->dev); > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret) { > + pm_runtime_put_noidle(&pdev->dev); Why do you call this in case of error? > + goto pm_runtime_fail; > + } > + > ret = clk_prepare_enable(hclk); > if (ret) > - goto disable_hclk_ret; > + goto pm_runtime_put; > > ret = clk_prepare_enable(cclk); > if (ret) > - goto disable_cclk_ret; > + goto disable_hclk_ret; > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can"); > addr = devm_ioremap_resource(&pdev->dev, res); > @@ -1666,6 +1688,11 @@ static int m_can_plat_probe(struct platform_device *pdev) > clk_disable_unprepare(cclk); > disable_hclk_ret: > clk_disable_unprepare(hclk); > +pm_runtime_put: > + pm_runtime_put_sync(&pdev->dev); > +pm_runtime_fail: > + if (ret) > + pm_runtime_disable(&pdev->dev); > failed_ret: > return ret; > } > @@ -1723,6 +1750,9 @@ static int m_can_plat_remove(struct platform_device *pdev) > struct net_device *dev = platform_get_drvdata(pdev); > > unregister_m_can_dev(dev); > + > + pm_runtime_disable(&pdev->dev); > + > platform_set_drvdata(pdev, NULL); > > free_m_can_dev(dev); > regards, Marc
Hi, On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: > On 12/22/2017 02:31 PM, Faiz Abbas wrote: >> From: Franklin S Cooper Jr <fcooper@ti.com> >> >> Add support for PM Runtime which is the new way to handle managing clocks. >> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >> management approach in place. > > There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate > CONFIG_PM_RUNTIME") Ok. Will change the commit message. > > Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : > >>> Well, I admit it would be nicer if drivers didn't have to worry about >>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>> from the one outlined above would have the probe routine do this: >>> >>> my_power_up(dev); >>> pm_runtime_set_active(dev); >>> pm_runtime_get_noresume(dev); >>> pm_runtime_enable(dev); This discussion seems to be about cases in which CONFIG_PM is not enabled. CONFIG_PM is always selected in the case of omap devices. > >> PM_RUNTIME is required by OMAP based devices to handle clock management. >> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP >> to work with this driver. > > Who will set the SET_RUNTIME_PM_OPS in this case? It is set with a common SET_RUNTIME_PM_OPS in the case of omap at arch/arm/mach-omap2/omap_device.c:632 struct dev_pm_domain omap_device_pm_domain = { .ops = { SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume, NULL) USE_PLATFORM_PM_SLEEP_OPS SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq, _od_resume_noirq) } }; > >> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> >> [nsekhar@ti.com: handle pm_runtime_get_sync() failure, fix some bugs] >> Signed-off-by: Sekhar Nori <nsekhar@ti.com> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >> --- >> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c >> index f72116e..53e764f 100644 >> --- a/drivers/net/can/m_can/m_can.c >> +++ b/drivers/net/can/m_can/m_can.c >> @@ -23,6 +23,7 @@ >> #include <linux/of.h> >> #include <linux/of_device.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> #include <linux/iopoll.h> >> #include <linux/can/dev.h> >> >> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv) >> { >> int err; >> >> + err = pm_runtime_get_sync(priv->device); >> + if (err) { >> + pm_runtime_put_noidle(priv->device); > > Why do you call this in case of an error? pm_runtime_get_sync() increments the usage count of the device before any error is returned. This needs to be decremented using pm_runtime_put_noidle(). > >> + return err; >> + } >> + >> err = clk_prepare_enable(priv->hclk); >> if (err) >> - return err; >> + goto pm_runtime_put; >> >> err = clk_prepare_enable(priv->cclk); >> if (err) >> - clk_disable_unprepare(priv->hclk); >> + goto disable_hclk; >> >> return err; >> + >> +disable_hclk: >> + clk_disable_unprepare(priv->hclk); >> +pm_runtime_put: >> + pm_runtime_put_sync(priv->device); >> + return err; >> } >> >> static void m_can_clk_stop(struct m_can_priv *priv) >> { >> + pm_runtime_put_sync(priv->device); >> + >> clk_disable_unprepare(priv->cclk); >> clk_disable_unprepare(priv->hclk); >> } >> @@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev) >> /* Enable clocks. Necessary to read Core Release in order to determine >> * M_CAN version >> */ >> + pm_runtime_enable(&pdev->dev); >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (ret) { >> + pm_runtime_put_noidle(&pdev->dev); > > Why do you call this in case of error? Same here. Thanks, Faiz
On 01/03/2018 01:39 PM, Faiz Abbas wrote: > On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: >> On 12/22/2017 02:31 PM, Faiz Abbas wrote: >>> From: Franklin S Cooper Jr <fcooper@ti.com> >>> >>> Add support for PM Runtime which is the new way to handle managing clocks. >>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >>> management approach in place. >> >> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate >> CONFIG_PM_RUNTIME") > > Ok. Will change the commit message. > >> >> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : >> >>>> Well, I admit it would be nicer if drivers didn't have to worry about >>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>>> from the one outlined above would have the probe routine do this: >>>> >>>> my_power_up(dev); >>>> pm_runtime_set_active(dev); >>>> pm_runtime_get_noresume(dev); >>>> pm_runtime_enable(dev); > > This discussion seems to be about cases in which CONFIG_PM is not > enabled. CONFIG_PM is always selected in the case of omap devices. Yes, but in the commit message you state that you need to support systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select CONFIG_PM, then we can make the driver much simpler. >>> PM_RUNTIME is required by OMAP based devices to handle clock management. >>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP >>> to work with this driver. >> >> Who will set the SET_RUNTIME_PM_OPS in this case? > > It is set with a common SET_RUNTIME_PM_OPS in the case of omap at > arch/arm/mach-omap2/omap_device.c:632 > > struct dev_pm_domain omap_device_pm_domain = { > .ops = { > SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume, > NULL) > USE_PLATFORM_PM_SLEEP_OPS > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq, > _od_resume_noirq) > } > }; > > >> >>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> >>> [nsekhar@ti.com: handle pm_runtime_get_sync() failure, fix some bugs] >>> Signed-off-by: Sekhar Nori <nsekhar@ti.com> >>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>> --- >>> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 34 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c >>> index f72116e..53e764f 100644 >>> --- a/drivers/net/can/m_can/m_can.c >>> +++ b/drivers/net/can/m_can/m_can.c >>> @@ -23,6 +23,7 @@ >>> #include <linux/of.h> >>> #include <linux/of_device.h> >>> #include <linux/platform_device.h> >>> +#include <linux/pm_runtime.h> >>> #include <linux/iopoll.h> >>> #include <linux/can/dev.h> >>> >>> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv) >>> { >>> int err; >>> >>> + err = pm_runtime_get_sync(priv->device); >>> + if (err) { >>> + pm_runtime_put_noidle(priv->device); >> >> Why do you call this in case of an error? > > pm_runtime_get_sync() increments the usage count of the device before > any error is returned. This needs to be decremented using > pm_runtime_put_noidle(). Oh, I'm curious how many drivers don't get this right. Marc
Hi, On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote: > On 01/03/2018 01:39 PM, Faiz Abbas wrote: >> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: >>> On 12/22/2017 02:31 PM, Faiz Abbas wrote: >>>> From: Franklin S Cooper Jr <fcooper@ti.com> >>>> >>>> Add support for PM Runtime which is the new way to handle managing clocks. >>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >>>> management approach in place. >>> >>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate >>> CONFIG_PM_RUNTIME") >> >> Ok. Will change the commit message. >> >>> >>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : >>> >>>>> Well, I admit it would be nicer if drivers didn't have to worry about >>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>>>> from the one outlined above would have the probe routine do this: >>>>> >>>>> my_power_up(dev); >>>>> pm_runtime_set_active(dev); >>>>> pm_runtime_get_noresume(dev); >>>>> pm_runtime_enable(dev); >> >> This discussion seems to be about cases in which CONFIG_PM is not >> enabled. CONFIG_PM is always selected in the case of omap devices. > > Yes, but in the commit message you state that you need to support > systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see > is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select > CONFIG_PM, then we can make the driver much simpler. Actually the old clock management (for hclk which is the interface clock) is still required as mentioned in the cover letter. Will change the rather misleading description. Thanks, Faiz > >>>> PM_RUNTIME is required by OMAP based devices to handle clock management. >>>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP >>>> to work with this driver. >>> >>> Who will set the SET_RUNTIME_PM_OPS in this case? >> >> It is set with a common SET_RUNTIME_PM_OPS in the case of omap at >> arch/arm/mach-omap2/omap_device.c:632 >> >> struct dev_pm_domain omap_device_pm_domain = { >> .ops = { >> SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume, >> NULL) >> USE_PLATFORM_PM_SLEEP_OPS >> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq, >> _od_resume_noirq) >> } >> }; >> >> >>> >>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com> >>>> [nsekhar@ti.com: handle pm_runtime_get_sync() failure, fix some bugs] >>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com> >>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >>>> --- >>>> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++---- >>>> 1 file changed, 34 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c >>>> index f72116e..53e764f 100644 >>>> --- a/drivers/net/can/m_can/m_can.c >>>> +++ b/drivers/net/can/m_can/m_can.c >>>> @@ -23,6 +23,7 @@ >>>> #include <linux/of.h> >>>> #include <linux/of_device.h> >>>> #include <linux/platform_device.h> >>>> +#include <linux/pm_runtime.h> >>>> #include <linux/iopoll.h> >>>> #include <linux/can/dev.h> >>>> >>>> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv) >>>> { >>>> int err; >>>> >>>> + err = pm_runtime_get_sync(priv->device); >>>> + if (err) { >>>> + pm_runtime_put_noidle(priv->device); >>> >>> Why do you call this in case of an error? >> >> pm_runtime_get_sync() increments the usage count of the device before >> any error is returned. This needs to be decremented using >> pm_runtime_put_noidle(). > > Oh, I'm curious how many drivers don't get this right. > > Marc >
On 01/03/2018 04:06 PM, Faiz Abbas wrote: > Hi, > > On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote: >> On 01/03/2018 01:39 PM, Faiz Abbas wrote: >>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: >>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote: >>>>> From: Franklin S Cooper Jr <fcooper@ti.com> >>>>> >>>>> Add support for PM Runtime which is the new way to handle managing clocks. >>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >>>>> management approach in place. >>>> >>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate >>>> CONFIG_PM_RUNTIME") >>> >>> Ok. Will change the commit message. >>> >>>> >>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : >>>> >>>>>> Well, I admit it would be nicer if drivers didn't have to worry about >>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>>>>> from the one outlined above would have the probe routine do this: >>>>>> >>>>>> my_power_up(dev); >>>>>> pm_runtime_set_active(dev); >>>>>> pm_runtime_get_noresume(dev); >>>>>> pm_runtime_enable(dev); >>> >>> This discussion seems to be about cases in which CONFIG_PM is not >>> enabled. CONFIG_PM is always selected in the case of omap devices. >> >> Yes, but in the commit message you state that you need to support >> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see >> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select >> CONFIG_PM, then we can make the driver much simpler. > > Actually the old clock management (for hclk which is the interface > clock) is still required as mentioned in the cover letter. Will change > the rather misleading description. Ok. So you can use the code as discussed on https://patchwork.kernel.org/patch/9436507/ ? Marc
Hi, On Wednesday 03 January 2018 08:47 PM, Marc Kleine-Budde wrote: > On 01/03/2018 04:06 PM, Faiz Abbas wrote: >> Hi, >> >> On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote: >>> On 01/03/2018 01:39 PM, Faiz Abbas wrote: >>>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: >>>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote: >>>>>> From: Franklin S Cooper Jr <fcooper@ti.com> >>>>>> >>>>>> Add support for PM Runtime which is the new way to handle managing clocks. >>>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >>>>>> management approach in place. >>>>> >>>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate >>>>> CONFIG_PM_RUNTIME") >>>> >>>> Ok. Will change the commit message. >>>> >>>>> >>>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : >>>>> >>>>>>> Well, I admit it would be nicer if drivers didn't have to worry about >>>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>>>>>> from the one outlined above would have the probe routine do this: >>>>>>> >>>>>>> my_power_up(dev); >>>>>>> pm_runtime_set_active(dev); >>>>>>> pm_runtime_get_noresume(dev); >>>>>>> pm_runtime_enable(dev); >>>> >>>> This discussion seems to be about cases in which CONFIG_PM is not >>>> enabled. CONFIG_PM is always selected in the case of omap devices. >>> >>> Yes, but in the commit message you state that you need to support >>> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see >>> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select >>> CONFIG_PM, then we can make the driver much simpler. >> >> Actually the old clock management (for hclk which is the interface >> clock) is still required as mentioned in the cover letter. Will change >> the rather misleading description. > > Ok. So you can use the code as discussed on > https://patchwork.kernel.org/patch/9436507/ ? Looking at the kernel configuration, it seems like SAMA5D2 platform selects CONFIG_PM (Wenyou, please confirm). So, it seems like the only users of this driver always have CONFIG_PM enabled. So I guess the best way is to maintain the current code for pm_runtime_* and move the clock enable/disable to pm_runtime callbacks. Something like this: m_can_runtime_resume() { clk_prepare_enable(cclk); clk_prepare_enable(hclk); } m_can_runtime_suspend() { clk_disable_unprepare(cclk); clk_disable_unprepare(hclk); } SET_RUNTIME_PM_OPS(m_can_runtime_suspend, m_can_runtime_resume, NULL) static void m_can_start(struct net_device *dev) { pm_runtime_get_sync(dev) ... } static void m_can_stop(struct net_device *dev) { ... pm_runtime_put_sync(dev) } Does that sound okay? If yes, I will go work on the implementation. Thanks, Faiz
On 01/04/2018 04:17 PM, Faiz Abbas wrote: > Hi, > > On Wednesday 03 January 2018 08:47 PM, Marc Kleine-Budde wrote: >> On 01/03/2018 04:06 PM, Faiz Abbas wrote: >>> Hi, >>> >>> On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote: >>>> On 01/03/2018 01:39 PM, Faiz Abbas wrote: >>>>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: >>>>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote: >>>>>>> From: Franklin S Cooper Jr <fcooper@ti.com> >>>>>>> >>>>>>> Add support for PM Runtime which is the new way to handle managing clocks. >>>>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >>>>>>> management approach in place. >>>>>> >>>>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate >>>>>> CONFIG_PM_RUNTIME") >>>>> >>>>> Ok. Will change the commit message. >>>>> >>>>>> >>>>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : >>>>>> >>>>>>>> Well, I admit it would be nicer if drivers didn't have to worry about >>>>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>>>>>>> from the one outlined above would have the probe routine do this: >>>>>>>> >>>>>>>> my_power_up(dev); >>>>>>>> pm_runtime_set_active(dev); >>>>>>>> pm_runtime_get_noresume(dev); >>>>>>>> pm_runtime_enable(dev); >>>>> >>>>> This discussion seems to be about cases in which CONFIG_PM is not >>>>> enabled. CONFIG_PM is always selected in the case of omap devices. >>>> >>>> Yes, but in the commit message you state that you need to support >>>> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see >>>> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select >>>> CONFIG_PM, then we can make the driver much simpler. >>> >>> Actually the old clock management (for hclk which is the interface >>> clock) is still required as mentioned in the cover letter. Will change >>> the rather misleading description. >> >> Ok. So you can use the code as discussed on >> https://patchwork.kernel.org/patch/9436507/ ? > > Looking at the kernel configuration, it seems like SAMA5D2 platform > selects CONFIG_PM (Wenyou, please confirm). So, it seems like the only > users of this driver always have CONFIG_PM enabled. > > So I guess the best way is to maintain the current code for pm_runtime_* > and move the clock enable/disable to pm_runtime callbacks. > > Something like this: > > m_can_runtime_resume() > { > clk_prepare_enable(cclk); > clk_prepare_enable(hclk); > } > > m_can_runtime_suspend() > { > clk_disable_unprepare(cclk); > clk_disable_unprepare(hclk); > } > > SET_RUNTIME_PM_OPS(m_can_runtime_suspend, m_can_runtime_resume, NULL) > > static void m_can_start(struct net_device *dev) > { > pm_runtime_get_sync(dev) > ... > } > > static void m_can_stop(struct net_device *dev) > { > ... > pm_runtime_put_sync(dev) > } > > Does that sound okay? If yes, I will go work on the implementation. ACK + error checking. Marc
On 2018/1/4 23:17, Faiz Abbas wrote: > Hi, > > On Wednesday 03 January 2018 08:47 PM, Marc Kleine-Budde wrote: >> On 01/03/2018 04:06 PM, Faiz Abbas wrote: >>> Hi, >>> >>> On Wednesday 03 January 2018 07:55 PM, Marc Kleine-Budde wrote: >>>> On 01/03/2018 01:39 PM, Faiz Abbas wrote: >>>>> On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: >>>>>> On 12/22/2017 02:31 PM, Faiz Abbas wrote: >>>>>>> From: Franklin S Cooper Jr <fcooper@ti.com> >>>>>>> >>>>>>> Add support for PM Runtime which is the new way to handle managing clocks. >>>>>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >>>>>>> management approach in place. >>>>>> There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate >>>>>> CONFIG_PM_RUNTIME") >>>>> Ok. Will change the commit message. >>>>> >>>>>> Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : >>>>>> >>>>>>>> Well, I admit it would be nicer if drivers didn't have to worry about >>>>>>>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>>>>>>> from the one outlined above would have the probe routine do this: >>>>>>>> >>>>>>>> my_power_up(dev); >>>>>>>> pm_runtime_set_active(dev); >>>>>>>> pm_runtime_get_noresume(dev); >>>>>>>> pm_runtime_enable(dev); >>>>> This discussion seems to be about cases in which CONFIG_PM is not >>>>> enabled. CONFIG_PM is always selected in the case of omap devices. >>>> Yes, but in the commit message you state that you need to support >>>> systems that don't have PM_RUNTIME enabled. The only mainline SoCs I see >>>> is "arch/arm/boot/dts/sama5d2.dtsi" so far. Please check if they select >>>> CONFIG_PM, then we can make the driver much simpler. >>> Actually the old clock management (for hclk which is the interface >>> clock) is still required as mentioned in the cover letter. Will change >>> the rather misleading description. >> Ok. So you can use the code as discussed on >> https://patchwork.kernel.org/patch/9436507/ ? > Looking at the kernel configuration, it seems like SAMA5D2 platform > selects CONFIG_PM (Wenyou, please confirm). Confirmed. The CONFIG_PM is selected. > So, it seems like the only > users of this driver always have CONFIG_PM enabled. > > So I guess the best way is to maintain the current code for pm_runtime_* > and move the clock enable/disable to pm_runtime callbacks. > > Something like this: > > m_can_runtime_resume() > { > clk_prepare_enable(cclk); > clk_prepare_enable(hclk); > } > > m_can_runtime_suspend() > { > clk_disable_unprepare(cclk); > clk_disable_unprepare(hclk); > } > > SET_RUNTIME_PM_OPS(m_can_runtime_suspend, m_can_runtime_resume, NULL) > > static void m_can_start(struct net_device *dev) > { > pm_runtime_get_sync(dev) > ... > } > > static void m_can_stop(struct net_device *dev) > { > ... > pm_runtime_put_sync(dev) > } > > Does that sound okay? If yes, I will go work on the implementation. > > Thanks, > Faiz Best Regards, Wenyou Yang
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index f72116e..53e764f 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -23,6 +23,7 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/iopoll.h> #include <linux/can/dev.h> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv) { int err; + err = pm_runtime_get_sync(priv->device); + if (err) { + pm_runtime_put_noidle(priv->device); + return err; + } + err = clk_prepare_enable(priv->hclk); if (err) - return err; + goto pm_runtime_put; err = clk_prepare_enable(priv->cclk); if (err) - clk_disable_unprepare(priv->hclk); + goto disable_hclk; return err; + +disable_hclk: + clk_disable_unprepare(priv->hclk); +pm_runtime_put: + pm_runtime_put_sync(priv->device); + return err; } static void m_can_clk_stop(struct m_can_priv *priv) { + pm_runtime_put_sync(priv->device); + clk_disable_unprepare(priv->cclk); clk_disable_unprepare(priv->hclk); } @@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev) /* Enable clocks. Necessary to read Core Release in order to determine * M_CAN version */ + pm_runtime_enable(&pdev->dev); + ret = pm_runtime_get_sync(&pdev->dev); + if (ret) { + pm_runtime_put_noidle(&pdev->dev); + goto pm_runtime_fail; + } + ret = clk_prepare_enable(hclk); if (ret) - goto disable_hclk_ret; + goto pm_runtime_put; ret = clk_prepare_enable(cclk); if (ret) - goto disable_cclk_ret; + goto disable_hclk_ret; res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can"); addr = devm_ioremap_resource(&pdev->dev, res); @@ -1666,6 +1688,11 @@ static int m_can_plat_probe(struct platform_device *pdev) clk_disable_unprepare(cclk); disable_hclk_ret: clk_disable_unprepare(hclk); +pm_runtime_put: + pm_runtime_put_sync(&pdev->dev); +pm_runtime_fail: + if (ret) + pm_runtime_disable(&pdev->dev); failed_ret: return ret; } @@ -1723,6 +1750,9 @@ static int m_can_plat_remove(struct platform_device *pdev) struct net_device *dev = platform_get_drvdata(pdev); unregister_m_can_dev(dev); + + pm_runtime_disable(&pdev->dev); + platform_set_drvdata(pdev, NULL); free_m_can_dev(dev);