| Message ID | 20190104180809.7071-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 734882a8bf984c2ac8a57d8ac3ee53230bd0bed8 |
| Headers | show |
| Series | spi: cadence: Correct initialisation of runtime PM | expand |
Hi Charles, On Fri, Jan 4, 2019 at 11:40 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > Currently the driver calls pm_runtime_put_autosuspend but without ever > having done a pm_runtime_get, this causes the reference count in the pm Before every transfer there is a prepare and then the get_sync gets called as we have the auto_runtime_pm as true. let me know if I am missing something?
On Mon, Jan 07, 2019 at 11:26:36AM +0530, Shubhrajyoti Datta wrote: > Hi Charles, > > On Fri, Jan 4, 2019 at 11:40 PM Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > > Currently the driver calls pm_runtime_put_autosuspend but without ever > > having done a pm_runtime_get, this causes the reference count in the pm > > Before every transfer there is a prepare and then the get_sync gets > called as we > have the auto_runtime_pm as true. > > let me know if I am missing something? When the code makes the call to: pm_runtime_set_active(&pdev->dev); That informs the PM runtime core that the device is currently active, however it does not increment the PM runtimes reference count. So the reference count is still 0 and the device will not be prevented from suspending. The driver requires the device to be powered up for the call to: cdns_spi_init_hw(xspi); Currently there is nothing that prevents it from suspending before then, other than it normally running through the code before SPI_AUTOSUSPEND_TIMEOUT expires. Additionally though the code then calls: pm_runtime_mark_last_busy(&pdev->dev); pm_runtime_put_autosuspend(&pdev->dev); Which will decrease the PM reference count to -1. This is very bad as the first PM runtime get will only increase it to zero, which still allows the device to suspend. It is an unbalanced put and the net effect is it cancels out the first get. Now what appears to happen on my system is that historically there has always been enough lag in the system that a second PM runtime get happens before the device actually suspends, allowing things to still work. However this patch to update the timers in the PM runtime core: 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers") Tightened up the response time and now sometimes I get the device suspending before a transaction is full complete, causing failures. Thanks, Charles
Hi Charles, On Mon, Jan 7, 2019 at 2:52 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > > On Mon, Jan 07, 2019 at 11:26:36AM +0530, Shubhrajyoti Datta wrote: > > Hi Charles, > > > > On Fri, Jan 4, 2019 at 11:40 PM Charles Keepax > > <ckeepax@opensource.cirrus.com> wrote: > > > > > > Currently the driver calls pm_runtime_put_autosuspend but without ever > > > having done a pm_runtime_get, this causes the reference count in the pm > > > > Before every transfer there is a prepare and then the get_sync gets > > called as we > > have the auto_runtime_pm as true. > > > > let me know if I am missing something? > > When the code makes the call to: > > pm_runtime_set_active(&pdev->dev); > > That informs the PM runtime core that the device is currently > active, however it does not increment the PM runtimes reference > count. So the reference count is still 0 and the device will not > be prevented from suspending. The driver requires the device to > be powered up for the call to: > > cdns_spi_init_hw(xspi); > > Currently there is nothing that prevents it from suspending > before then, other than it normally running through the code > before SPI_AUTOSUSPEND_TIMEOUT expires. Additionally though the > code then calls: > > pm_runtime_mark_last_busy(&pdev->dev); > pm_runtime_put_autosuspend(&pdev->dev); > > Which will decrease the PM reference count to -1. This is very > bad as the first PM runtime get will only increase it to zero, which > still allows the device to suspend. It is an unbalanced put and > the net effect is it cancels out the first get. > > Now what appears to happen on my system is that historically > there has always been enough lag in the system that a second PM > runtime get happens before the device actually suspends, allowing > things to still work. However this patch to update the timers in > the PM runtime core: > > 8234f6734c5d ("PM-runtime: Switch autosuspend over to using hrtimers") > > Tightened up the response time and now sometimes I get the device > suspending before a transaction is full complete, causing > failures. > BTW after the probe the clocks are left enabled ? > Thanks, > Charles
On Mon, Jan 07, 2019 at 03:07:23PM +0530, Shubhrajyoti Datta wrote: > Hi Charles, > BTW after the probe the clocks are left enabled ? > Assuming you mean after my patch, the clocks are left enabled but once the auto suspend timeout triggers it will turn them off. So they should be abled for about 3mS after probe. Thanks, Charles
> -----Original Message----- > From: Charles Keepax [mailto:ckeepax@opensource.cirrus.com] > Sent: Monday, January 7, 2019 3:27 PM > To: Shubhrajyoti Datta <shubhrajyoti.datta@gmail.com> > Cc: Mark Brown <broonie@kernel.org>; Shubhrajyoti Datta > <shubhraj@xilinx.com>; linux-spi@vger.kernel.org > Subject: Re: [PATCH] spi: cadence: Correct initialisation of runtime PM > > On Mon, Jan 07, 2019 at 03:07:23PM +0530, Shubhrajyoti Datta wrote: > > Hi Charles, > > BTW after the probe the clocks are left enabled ? > > > > Assuming you mean after my patch, the clocks are left enabled but once the > auto suspend timeout triggers it will turn them off. So they should be abled > for about 3mS after probe. Yes can can you check if there are no transactions if the clock are off? > > Thanks, > Charles
On Mon, Jan 07, 2019 at 10:23:17AM +0000, Shubhrajyoti Datta wrote: > > -----Original Message----- > > From: Charles Keepax [mailto:ckeepax@opensource.cirrus.com] > > Sent: Monday, January 7, 2019 3:27 PM > > To: Shubhrajyoti Datta <shubhrajyoti.datta@gmail.com> > > Cc: Mark Brown <broonie@kernel.org>; Shubhrajyoti Datta > > <shubhraj@xilinx.com>; linux-spi@vger.kernel.org > > Subject: Re: [PATCH] spi: cadence: Correct initialisation of runtime PM > > > > On Mon, Jan 07, 2019 at 03:07:23PM +0530, Shubhrajyoti Datta wrote: > > > Hi Charles, > > > BTW after the probe the clocks are left enabled ? > > > > > > > Assuming you mean after my patch, the clocks are left enabled but once the > > auto suspend timeout triggers it will turn them off. So they should be abled > > for about 3mS after probe. > > Yes can can you check if there are no transactions if the clock are off? Already tested before I sent the patch I can see both cnds_runtime_resume and cnds_runtime_suspend being called as SPI transactions happen. Thanks, Charles
diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c index 7c88f74f7f470..94cc0a152449f 100644 --- a/drivers/spi/spi-cadence.c +++ b/drivers/spi/spi-cadence.c @@ -584,11 +584,6 @@ static int cdns_spi_probe(struct platform_device *pdev) goto clk_dis_apb; } - pm_runtime_use_autosuspend(&pdev->dev); - pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT); - pm_runtime_set_active(&pdev->dev); - pm_runtime_enable(&pdev->dev); - ret = of_property_read_u32(pdev->dev.of_node, "num-cs", &num_cs); if (ret < 0) master->num_chipselect = CDNS_SPI_DEFAULT_NUM_CS; @@ -603,8 +598,10 @@ static int cdns_spi_probe(struct platform_device *pdev) /* SPI controller initializations */ cdns_spi_init_hw(xspi); - pm_runtime_mark_last_busy(&pdev->dev); - pm_runtime_put_autosuspend(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT); irq = platform_get_irq(pdev, 0); if (irq <= 0) {
Currently the driver calls pm_runtime_put_autosuspend but without ever having done a pm_runtime_get, this causes the reference count in the pm runtime core to become -1. The bad reference count causes the core to sometimes suspend whilst an active SPI transfer is in progress. arizona spi0.1: SPI transfer timed out spi_master spi0: failed to transfer one message from queue The correct proceedure is to do all the initialisation that requires the hardware to be powered up before enabling the PM runtime, then enable the PM runtime having called pm_runtime_set_active to inform it that the hardware is currently powered up. The core will then power it down at it's leisure and no explicit pm_runtime_put is required. Fixes: d36ccd9f7ea4 ("spi: cadence: Runtime pm adaptation") Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/spi/spi-cadence.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)