| Message ID | 20170924105924.23923-6-vigneshr@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
| Series | K2G: Add QSPI support | expand |
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.
Hi Matthew, On Tuesday 26 September 2017 05:19 AM, Marek Vasut wrote: [...] >>>> 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. > yes, a quick qspi test with clk_prepare_enable() replaced by pm_*() calls like below patch would be helpful: diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c index 53c7d8e0327a..7ad3e176cc88 100644 --- a/drivers/mtd/spi-nor/cadence-quadspi.c +++ b/drivers/mtd/spi-nor/cadence-quadspi.c @@ -34,6 +34,7 @@ #include <linux/sched.h> #include <linux/spi/spi.h> #include <linux/timer.h> +#include <linux/pm_runtime.h> #define CQSPI_NAME "cadence-qspi" #define CQSPI_MAX_CHIPSELECT 16 @@ -1206,11 +1207,8 @@ static int cqspi_probe(struct platform_device *pdev) return -ENXIO; } - ret = clk_prepare_enable(cqspi->clk); - if (ret) { - dev_err(dev, "Cannot enable QSPI clock.\n"); - return ret; - } + pm_runtime_enable(dev); + pm_runtime_get_sync(dev); cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
Hi Vignesh, I tried this patch on an Arria10 SOCFPGA devkit against the 4.1.33-ltsi kernel, and it did not go well. Commands to the flash chip timedout resulting in the probe function failing. I ran into other problems, not related to cadence-quadspi, that prevented me from testing against 4.9 and 4.12 kernels, but I suspect similar behavior. Matthew Gerlach On Wed, 27 Sep 2017, Vignesh R wrote: > Hi Matthew, > > On Tuesday 26 September 2017 05:19 AM, Marek Vasut wrote: > [...] >>>>> 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. >> > > yes, a quick qspi test with clk_prepare_enable() replaced by pm_*() calls > like below patch would be helpful: > > > diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c > index 53c7d8e0327a..7ad3e176cc88 100644 > --- a/drivers/mtd/spi-nor/cadence-quadspi.c > +++ b/drivers/mtd/spi-nor/cadence-quadspi.c > @@ -34,6 +34,7 @@ > #include <linux/sched.h> > #include <linux/spi/spi.h> > #include <linux/timer.h> > +#include <linux/pm_runtime.h> > > #define CQSPI_NAME "cadence-qspi" > #define CQSPI_MAX_CHIPSELECT 16 > @@ -1206,11 +1207,8 @@ static int cqspi_probe(struct platform_device *pdev) > return -ENXIO; > } > > - ret = clk_prepare_enable(cqspi->clk); > - if (ret) { > - dev_err(dev, "Cannot enable QSPI clock.\n"); > - return ret; > - } > + pm_runtime_enable(dev); > + pm_runtime_get_sync(dev); > > cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk); > > > > > > -- > Regards > Vignesh >
Hi, On 9/28/2017 8:31 PM, matthew.gerlach@linux.intel.com wrote: > > Hi Vignesh, > > I tried this patch on an Arria10 SOCFPGA devkit against the 4.1.33-ltsi > kernel, and it did not go well. Commands to the flash chip timedout > resulting in the probe function failing. I ran into other problems, not > related to cadence-quadspi, that prevented me from testing against 4.9 and > 4.12 kernels, but I suspect similar behavior. > Ok, thanks! I will keep the clk_*() calls for now. Regards Vignesh > Matthew Gerlach > > On Wed, 27 Sep 2017, Vignesh R wrote: > >> Hi Matthew, >> >> On Tuesday 26 September 2017 05:19 AM, Marek Vasut wrote: >> [...] >>>>>> 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. >>> >> >> yes, a quick qspi test with clk_prepare_enable() replaced by pm_*() calls >> like below patch would be helpful: >> >> >> diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c >> index 53c7d8e0327a..7ad3e176cc88 100644 >> --- a/drivers/mtd/spi-nor/cadence-quadspi.c >> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c >> @@ -34,6 +34,7 @@ >> #include <linux/sched.h> >> #include <linux/spi/spi.h> >> #include <linux/timer.h> >> +#include <linux/pm_runtime.h> >> >> #define CQSPI_NAME "cadence-qspi" >> #define CQSPI_MAX_CHIPSELECT 16 >> @@ -1206,11 +1207,8 @@ static int cqspi_probe(struct platform_device *pdev) >> return -ENXIO; >> } >> >> - ret = clk_prepare_enable(cqspi->clk); >> - if (ret) { >> - dev_err(dev, "Cannot enable QSPI clock.\n"); >> - return ret; >> - } >> + pm_runtime_enable(dev); >> + pm_runtime_get_sync(dev); >> >> cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk); >> >> >> >> >> >> -- >> Regards >> Vignesh >> >
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(+)