| Message ID | 20170924105924.23923-6-vigneshr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/24/2017 12:59 PM, Vignesh R wrote: > Add pm_runtime* calls to cadence-quadspi driver. This is required to > switch on QSPI power domain on TI 66AK2G SoC during probe. > > Signed-off-by: Vignesh R <vigneshr@ti.com> Are you planning to add some more fine-grained PM control later? > --- > drivers/mtd/spi-nor/cadence-quadspi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c > index d9629e8f4798..2c8e6226d267 100644 > --- a/drivers/mtd/spi-nor/cadence-quadspi.c > +++ b/drivers/mtd/spi-nor/cadence-quadspi.c > @@ -31,6 +31,7 @@ > #include <linux/of_device.h> > #include <linux/of.h> > #include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > #include <linux/sched.h> > #include <linux/spi/spi.h> > #include <linux/timer.h> > @@ -1224,6 +1225,13 @@ static int cqspi_probe(struct platform_device *pdev) > return -ENXIO; > } > > + pm_runtime_enable(&pdev->dev); > + ret = pm_runtime_get_sync(&pdev->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&pdev->dev); > + return ret; > + } > + > ret = clk_prepare_enable(cqspi->clk); > if (ret) { > dev_err(dev, "Cannot enable QSPI clock.\n"); > @@ -1275,6 +1283,9 @@ static int cqspi_remove(struct platform_device *pdev) > > clk_disable_unprepare(cqspi->clk); > > + pm_runtime_put_sync(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > + > return 0; > } > >
On 9/24/2017 5:31 PM, Marek Vasut wrote: > On 09/24/2017 12:59 PM, Vignesh R wrote: >> Add pm_runtime* calls to cadence-quadspi driver. This is required to >> switch on QSPI power domain on TI 66AK2G SoC during probe. >> >> Signed-off-by: Vignesh R <vigneshr@ti.com> > > Are you planning to add some more fine-grained PM control later? Yes, I will need to add fine-grained PM control at some point. But, for now SoC does not really support low power mode or runtime power saving option. The fact that driver still uses clk_prepare_*() calls to enable/disable clocks instead of pm_*() calls makes it a bit tricky though. Just figured out I forgot to add cleanup code in error handling path of probe(). Will fix that and send a v4. > >> --- >> drivers/mtd/spi-nor/cadence-quadspi.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c >> index d9629e8f4798..2c8e6226d267 100644 >> --- a/drivers/mtd/spi-nor/cadence-quadspi.c >> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c >> @@ -31,6 +31,7 @@ >> #include <linux/of_device.h> >> #include <linux/of.h> >> #include <linux/platform_device.h> >> +#include <linux/pm_runtime.h> >> #include <linux/sched.h> >> #include <linux/spi/spi.h> >> #include <linux/timer.h> >> @@ -1224,6 +1225,13 @@ static int cqspi_probe(struct platform_device *pdev) >> return -ENXIO; >> } >> >> + pm_runtime_enable(&pdev->dev); >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (ret < 0) { >> + pm_runtime_put_noidle(&pdev->dev); >> + return ret; >> + } >> + >> ret = clk_prepare_enable(cqspi->clk); >> if (ret) { >> dev_err(dev, "Cannot enable QSPI clock.\n"); >> @@ -1275,6 +1283,9 @@ static int cqspi_remove(struct platform_device *pdev) >> >> clk_disable_unprepare(cqspi->clk); >> >> + pm_runtime_put_sync(&pdev->dev); >> + pm_runtime_disable(&pdev->dev); >> + >> return 0; >> } >> >> > >
On 09/24/2017 03:08 PM, Vignesh R wrote: > > > On 9/24/2017 5:31 PM, Marek Vasut wrote: >> On 09/24/2017 12:59 PM, Vignesh R wrote: >>> Add pm_runtime* calls to cadence-quadspi driver. This is required to >>> switch on QSPI power domain on TI 66AK2G SoC during probe. >>> >>> Signed-off-by: Vignesh R <vigneshr@ti.com> >> >> Are you planning to add some more fine-grained PM control later? > > Yes, I will need to add fine-grained PM control at some point. But, for > now SoC does not really support low power mode or runtime power saving > option. > The fact that driver still uses clk_prepare_*() calls to enable/disable > clocks instead of pm_*() calls makes it a bit tricky though. > > Just figured out I forgot to add cleanup code in error handling path of > probe(). Will fix that and send a v4. OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM either, so it's fine for now.
On 9/24/2017 6:42 PM, Marek Vasut wrote: > On 09/24/2017 03:08 PM, Vignesh R wrote: >> >> >> On 9/24/2017 5:31 PM, Marek Vasut wrote: >>> On 09/24/2017 12:59 PM, Vignesh R wrote: >>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to >>>> switch on QSPI power domain on TI 66AK2G SoC during probe. >>>> >>>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>> >>> Are you planning to add some more fine-grained PM control later? >> >> Yes, I will need to add fine-grained PM control at some point. But, for >> now SoC does not really support low power mode or runtime power saving >> option. >> The fact that driver still uses clk_prepare_*() calls to enable/disable >> clocks instead of pm_*() calls makes it a bit tricky though. >> >> Just figured out I forgot to add cleanup code in error handling path of >> probe(). Will fix that and send a v4. > > OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM > either, so it's fine for now. > Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see if its possible to get rid of clk_*() calls in favor of pm_*() calls.
On 09/24/2017 03:27 PM, Vignesh R wrote: > > > On 9/24/2017 6:42 PM, Marek Vasut wrote: >> On 09/24/2017 03:08 PM, Vignesh R wrote: >>> >>> >>> On 9/24/2017 5:31 PM, Marek Vasut wrote: >>>> On 09/24/2017 12:59 PM, Vignesh R wrote: >>>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to >>>>> switch on QSPI power domain on TI 66AK2G SoC during probe. >>>>> >>>>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>>> >>>> Are you planning to add some more fine-grained PM control later? >>> >>> Yes, I will need to add fine-grained PM control at some point. But, for >>> now SoC does not really support low power mode or runtime power saving >>> option. >>> The fact that driver still uses clk_prepare_*() calls to enable/disable >>> clocks instead of pm_*() calls makes it a bit tricky though. >>> >>> Just figured out I forgot to add cleanup code in error handling path of >>> probe(). Will fix that and send a v4. >> >> OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM >> either, so it's fine for now. >> > > Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for > QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see > if its possible to get rid of clk_*() calls in favor of pm_*() calls. Not of the top of my head, sorry. +CC Matthew, he should know.
On Sun, 24 Sep 2017, Marek Vasut wrote: > On 09/24/2017 03:27 PM, Vignesh R wrote: >> >> >> On 9/24/2017 6:42 PM, Marek Vasut wrote: >>> On 09/24/2017 03:08 PM, Vignesh R wrote: >>>> >>>> >>>> On 9/24/2017 5:31 PM, Marek Vasut wrote: >>>>> On 09/24/2017 12:59 PM, Vignesh R wrote: >>>>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to >>>>>> switch on QSPI power domain on TI 66AK2G SoC during probe. >>>>>> >>>>>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>>>> >>>>> Are you planning to add some more fine-grained PM control later? >>>> >>>> Yes, I will need to add fine-grained PM control at some point. But, for >>>> now SoC does not really support low power mode or runtime power saving >>>> option. >>>> The fact that driver still uses clk_prepare_*() calls to enable/disable >>>> clocks instead of pm_*() calls makes it a bit tricky though. >>>> >>>> Just figured out I forgot to add cleanup code in error handling path of >>>> probe(). Will fix that and send a v4. >>> >>> OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM >>> either, so it's fine for now. >>> >> >> Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for >> QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see >> if its possible to get rid of clk_*() calls in favor of pm_*() calls. > > Not of the top of my head, sorry. +CC Matthew, he should know. I am not an expert at the clock framework nor the power management, but I did ask around a bit. No one I asked was planning to change the clk_*() calls to pm_*() call, but the feedback was that it would be a good idea. Matthew Gerlach > > -- > Best regards, > Marek Vasut >
On 09/26/2017 12:41 AM, matthew.gerlach@linux.intel.com wrote: > > > On Sun, 24 Sep 2017, Marek Vasut wrote: > >> On 09/24/2017 03:27 PM, Vignesh R wrote: >>> >>> >>> On 9/24/2017 6:42 PM, Marek Vasut wrote: >>>> On 09/24/2017 03:08 PM, Vignesh R wrote: >>>>> >>>>> >>>>> On 9/24/2017 5:31 PM, Marek Vasut wrote: >>>>>> On 09/24/2017 12:59 PM, Vignesh R wrote: >>>>>>> Add pm_runtime* calls to cadence-quadspi driver. This is required to >>>>>>> switch on QSPI power domain on TI 66AK2G SoC during probe. >>>>>>> >>>>>>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>>>>> >>>>>> Are you planning to add some more fine-grained PM control later? >>>>> >>>>> Yes, I will need to add fine-grained PM control at some point. But, >>>>> for >>>>> now SoC does not really support low power mode or runtime power saving >>>>> option. >>>>> The fact that driver still uses clk_prepare_*() calls to >>>>> enable/disable >>>>> clocks instead of pm_*() calls makes it a bit tricky though. >>>>> >>>>> Just figured out I forgot to add cleanup code in error handling >>>>> path of >>>>> probe(). Will fix that and send a v4. >>>> >>>> OK, fine. Cleanups are welcome. The SoCFPGA doesn't do much runtime PM >>>> either, so it's fine for now. >>>> >>> >>> Ok thanks! Do you know if pm_runtime_get_sync() can enable clocks for >>> QSPI on SoCFPGA or if clk_prepare_enable() is needed? Just trying to see >>> if its possible to get rid of clk_*() calls in favor of pm_*() calls. >> >> Not of the top of my head, sorry. +CC Matthew, he should know. > > I am not an expert at the clock framework nor the power management, but I > did ask around a bit. No one I asked was planning to change the clk_*() > calls to pm_*() call, but the feedback was that it would be a good idea. The question is, if we do the replacement, will it break on socfpga ? A quick test might be useful.
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c index d9629e8f4798..2c8e6226d267 100644 --- a/drivers/mtd/spi-nor/cadence-quadspi.c +++ b/drivers/mtd/spi-nor/cadence-quadspi.c @@ -31,6 +31,7 @@ #include <linux/of_device.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/sched.h> #include <linux/spi/spi.h> #include <linux/timer.h> @@ -1224,6 +1225,13 @@ static int cqspi_probe(struct platform_device *pdev) return -ENXIO; } + pm_runtime_enable(&pdev->dev); + ret = pm_runtime_get_sync(&pdev->dev); + if (ret < 0) { + pm_runtime_put_noidle(&pdev->dev); + return ret; + } + ret = clk_prepare_enable(cqspi->clk); if (ret) { dev_err(dev, "Cannot enable QSPI clock.\n"); @@ -1275,6 +1283,9 @@ static int cqspi_remove(struct platform_device *pdev) clk_disable_unprepare(cqspi->clk); + pm_runtime_put_sync(&pdev->dev); + pm_runtime_disable(&pdev->dev); + return 0; }
Add pm_runtime* calls to cadence-quadspi driver. This is required to switch on QSPI power domain on TI 66AK2G SoC during probe. Signed-off-by: Vignesh R <vigneshr@ti.com> --- drivers/mtd/spi-nor/cadence-quadspi.c | 11 +++++++++++ 1 file changed, 11 insertions(+)