spi: s3c64xx: fix runtime PM handling
Subsystems (3)
PATCH 2/3 spi: s3c64xx: fix runtime PM handling
The patch fixes / extends the runtime PM handling.
- Currently the driver instructs the SPI core to use auto-suspend but doesn't configure it. Add the necessary calls and let it autosuspend after probe.
- In case the driver registration fails runtime PM needs to be properly disabled.
--- drivers/spi/spi-s3c64xx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/spi/spi-s3c64xx.c +10 -1
@@ -32,6 +32,7 @@#define MAX_SPI_PORTS 6#define S3C64XX_SPI_QUIRK_POLL (1 << 0)#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) +#define AUTOSUSPEND_TIMEOUT 2000/* Registers and bit-fields */
@@ -1180,13 +1181,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)S3C64XX_SPI_INT_TX_OVERRUN_EN |S3C64XX_SPI_INT_TX_UNDERRUN_EN , sdd->regs + S3C64XX_SPI_INT_EN); +pm_runtime_set_autosuspend_delay (dev,AUTOSUSPEND_TIMEOUT ); +pm_runtime_use_autosuspend (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev); ret =devm_spi_register_master (dev, master);if (ret !=0 ) {dev_err (dev,"cannot register SPI master: %d\n" , ret); - goto err3; + goto err4; } dev_dbg(dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",
@@ -1195,8 +1198,14 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) mem_res, (FIFO_LVL_MASK (sdd) >>1 ) +1 , sdd->rx_dma.dmach, sdd->tx_dma.dmach); +pm_runtime_mark_last_busy (dev); +pm_runtime_autosuspend (dev); +return 0 ; +err4: +pm_runtime_disable (dev); +pm_runtime_set_suspended (dev); err3:clk_disable_unprepare (sdd->src_clk); err2:
Signature · 6 lines
2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · Heiner Kallweit
The patch fixes / extends the runtime PM handling.
- Currently the driver instructs the SPI core to use auto-suspend but doesn't configure it. Add the necessary calls and let it autosuspend after probe.
After quick look at the driver I think it does not use autosuspend fully. It just sets master->auto_runtime_pm (so the core code will use autosuspend) but its own calls to pm_runtime_put() are regular ones. Shouldn't this be enhanced as well?
Quoted 38 lines
- In case the driver registration fails runtime PM needs to be properly disabled.
Signed-off-by Heiner Kallweit <[email protected]>--- drivers/spi/spi-s3c64xx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/spi/spi-s3c64xx.c +4
-1@@ -32,6 +32,7 @@#define MAX_SPI_PORTS 6#define S3C64XX_SPI_QUIRK_POLL (1 << 0)#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) +#define AUTOSUSPEND_TIMEOUT 2000/* Registers and bit-fields */ @@ -1180,13 +1181,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)S3C64XX_SPI_INT_TX_OVERRUN_EN |S3C64XX_SPI_INT_TX_UNDERRUN_EN , sdd->regs + S3C64XX_SPI_INT_EN); +pm_runtime_set_autosuspend_delay (dev,AUTOSUSPEND_TIMEOUT ); +pm_runtime_use_autosuspend (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev); ret =devm_spi_register_master (dev, master);if (ret !=0 ) {dev_err (dev,"cannot register SPI master: %d\n" , ret); - goto err3; + goto err4; } dev_dbg(dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1202
Quoted 5 lines
drivers/spi/spi-s3c64xx.c +2
-0@@ -1195,8 +1198,14 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) mem_res, (FIFO_LVL_MASK (sdd) >>1 ) +1 , sdd->rx_dma.dmach, sdd->tx_dma.dmach); +pm_runtime_mark_last_busy (dev); +pm_runtime_autosuspend (dev);
There is a high chance that device will be suspended already at this point because pm_runtime_put() in setup. In most cases (when SPI is started) this won't do anything so we are back at first of my questions
- shouldn't you update pm_runtime_put() calls?
Additionally (which is not related to your patch) shouldn't we pm_runtime_get() at some point in probe() to be sure that it won't be suspended to early?
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1208
Quoted 6 lines
drivers/spi/spi-s3c64xx.c +4
-0+return 0 ; +err4: +pm_runtime_disable (dev); +pm_runtime_set_suspended (dev);
Please separate fixes from enhancements into different patches.
Best regards,
Krzysztof
Quoted 4 lines
drivers/spi/spi-s3c64xx.c +0
-0err3:clk_disable_unprepare (sdd->src_clk); err2:
Am 19.08.2015 um 02:35 schrieb Krzysztof Kozlowski:
Quoted 10 lines · Heiner Kallweit
Quoted 4 lines
The patch fixes / extends the runtime PM handling.
- Currently the driver instructs the SPI core to use auto-suspend but doesn't configure it. Add the necessary calls and let it autosuspend after probe.After quick look at the driver I think it does not use autosuspend fully. It just sets
master->auto_runtime_pm(so the core code will use autosuspend) but its own calls topm_runtime_put()are regular ones. Shouldn't this be enhanced as well?
Right, the pm_runtime_put calls in s3c64xx_spi_setup should be replaced as well. Will provide an updated patch.
Quoted 42 lines
Quoted 42 lines
- In case the driver registration fails runtime PM needs to be properly disabled.
Signed-off-by Heiner Kallweit <[email protected]>--- drivers/spi/spi-s3c64xx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/spi/spi-s3c64xx.c +4
-1@@ -32,6 +32,7 @@#define MAX_SPI_PORTS 6#define S3C64XX_SPI_QUIRK_POLL (1 << 0)#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) +#define AUTOSUSPEND_TIMEOUT 2000/* Registers and bit-fields */ @@ -1180,13 +1181,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)S3C64XX_SPI_INT_TX_OVERRUN_EN |S3C64XX_SPI_INT_TX_UNDERRUN_EN , sdd->regs + S3C64XX_SPI_INT_EN); +pm_runtime_set_autosuspend_delay (dev,AUTOSUSPEND_TIMEOUT ); +pm_runtime_use_autosuspend (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev); ret =devm_spi_register_master (dev, master);if (ret !=0 ) {dev_err (dev,"cannot register SPI master: %d\n" , ret); - goto err3; + goto err4; } dev_dbg(dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",@@ -1195,8 +1198,14 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) mem_res, (FIFO_LVL_MASK (sdd) >>1 ) +1 , sdd->rx_dma.dmach, sdd->tx_dma.dmach);
Quoted 7 lines
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1202
Quoted 2 lines
drivers/spi/spi-s3c64xx.c +2
-0+pm_runtime_mark_last_busy (dev); +pm_runtime_autosuspend (dev);There is a high chance that device will be suspended already at this point because
pm_runtime_put()in setup. In most cases (when SPI is started) this won't do anything so we are back at first of my questions
- shouldn't you updatepm_runtime_put()calls?
Yes, these calls should be updated.
Quoted 4 lines
Additionally (which is not related to your patch) shouldn't we
pm_runtime_get()at some point inprobe()to be sure that it won't be suspended to early?
Not 100% sure at this moment, I will check.
Quoted 9 lines
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1208
Quoted 7 lines
drivers/spi/spi-s3c64xx.c +4
-0+return 0 ; +err4: +pm_runtime_disable (dev); +pm_runtime_set_suspended (dev);Please separate fixes from enhancements into different patches.
OK, will do so.
Quoted 10 lines
Best regards,
KrzysztofQuoted 4 lines
drivers/spi/spi-s3c64xx.c +0
-0err3:clk_disable_unprepare (sdd->src_clk); err2:
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 19.08.2015 um 07:54 schrieb Heiner Kallweit:
Quoted 62 lines
Am 19.08.2015 um 02:35 schrieb Krzysztof Kozlowski:
Quoted 10 lines · Heiner Kallweit
Quoted 4 lines
The patch fixes / extends the runtime PM handling.
- Currently the driver instructs the SPI core to use auto-suspend but doesn't configure it. Add the necessary calls and let it autosuspend after probe.After quick look at the driver I think it does not use autosuspend fully. It just sets
master->auto_runtime_pm(so the core code will use autosuspend) but its own calls topm_runtime_put()are regular ones. Shouldn't this be enhanced as well?Right, the
pm_runtime_putcalls ins3c64xx_spi_setupshould be replaced as well. Will provide an updated patch.Quoted 42 lines
Quoted 42 lines
- In case the driver registration fails runtime PM needs to be properly disabled.
Signed-off-by Heiner Kallweit <[email protected]>--- drivers/spi/spi-s3c64xx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/spi/spi-s3c64xx.c +4
-1@@ -32,6 +32,7 @@#define MAX_SPI_PORTS 6#define S3C64XX_SPI_QUIRK_POLL (1 << 0)#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) +#define AUTOSUSPEND_TIMEOUT 2000/* Registers and bit-fields */ @@ -1180,13 +1181,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)S3C64XX_SPI_INT_TX_OVERRUN_EN |S3C64XX_SPI_INT_TX_UNDERRUN_EN , sdd->regs + S3C64XX_SPI_INT_EN); +pm_runtime_set_autosuspend_delay (dev,AUTOSUSPEND_TIMEOUT ); +pm_runtime_use_autosuspend (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev); ret =devm_spi_register_master (dev, master);if (ret !=0 ) {dev_err (dev,"cannot register SPI master: %d\n" , ret); - goto err3; + goto err4; } dev_dbg(dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",@@ -1195,8 +1198,14 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) mem_res, (FIFO_LVL_MASK (sdd) >>1 ) +1 , sdd->rx_dma.dmach, sdd->tx_dma.dmach);Quoted 7 lines
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1202
Quoted 2 lines
drivers/spi/spi-s3c64xx.c +2
-0+pm_runtime_mark_last_busy (dev); +pm_runtime_autosuspend (dev);There is a high chance that device will be suspended already at this point because
pm_runtime_put()in setup. In most cases (when SPI is started) this won't do anything so we are back at first of my questions
- shouldn't you updatepm_runtime_put()calls?
Quoted 6 lines
Yes, these calls should be updated.
Quoted 4 lines
Additionally (which is not related to your patch) shouldn't we
pm_runtime_get()at some point inprobe()to be sure that it won't be suspended to early?Not 100% sure at this moment, I will check.
Kernel documentation says:
"Note, if the device may execute pm_runtime calls during the probe (such as if it is registers with a subsystem that may call back in) then the pm_runtime_get_sync() call paired with a pm_runtime_put() call will be appropriate to ensure that the device is not put back to sleep during the probe. This can happen with systems such as the network device layer."
I don't think this applies to SPI but I'm not expert enough to guarantee. At least no other SPI driver using runtime PM protects the probe function this way. Therefore I would dare to leave it as it is, especially as there don't seem to be any related known problems with SPI drivers.
Regards, Heiner
Quoted 21 lines
Quoted 9 lines
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1208
Quoted 7 lines
drivers/spi/spi-s3c64xx.c +4
-0+return 0 ; +err4: +pm_runtime_disable (dev); +pm_runtime_set_suspended (dev);Please separate fixes from enhancements into different patches.
OK, will do so.
Quoted 10 lines
Best regards,
KrzysztofQuoted 4 lines
drivers/spi/spi-s3c64xx.c +0
-0err3:clk_disable_unprepare (sdd->src_clk); err2:
Quoted 76 lines · Heiner Kallweit
Am 19.08.2015 um 07:54 schrieb Heiner Kallweit:
Quoted 62 lines
Am 19.08.2015 um 02:35 schrieb Krzysztof Kozlowski:
Quoted 10 lines · Heiner Kallweit
Quoted 4 lines
The patch fixes / extends the runtime PM handling.
- Currently the driver instructs the SPI core to use auto-suspend but doesn't configure it. Add the necessary calls and let it autosuspend after probe.After quick look at the driver I think it does not use autosuspend fully. It just sets
master->auto_runtime_pm(so the core code will use autosuspend) but its own calls topm_runtime_put()are regular ones. Shouldn't this be enhanced as well?Right, the
pm_runtime_putcalls ins3c64xx_spi_setupshould be replaced as well. Will provide an updated patch.Quoted 42 lines
Quoted 42 lines
- In case the driver registration fails runtime PM needs to be properly disabled.
Signed-off-by Heiner Kallweit <[email protected]>--- drivers/spi/spi-s3c64xx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/spi/spi-s3c64xx.c +4
-1@@ -32,6 +32,7 @@#define MAX_SPI_PORTS 6#define S3C64XX_SPI_QUIRK_POLL (1 << 0)#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) +#define AUTOSUSPEND_TIMEOUT 2000/* Registers and bit-fields */ @@ -1180,13 +1181,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)S3C64XX_SPI_INT_TX_OVERRUN_EN |S3C64XX_SPI_INT_TX_UNDERRUN_EN , sdd->regs + S3C64XX_SPI_INT_EN); +pm_runtime_set_autosuspend_delay (dev,AUTOSUSPEND_TIMEOUT ); +pm_runtime_use_autosuspend (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev); ret =devm_spi_register_master (dev, master);if (ret !=0 ) {dev_err (dev,"cannot register SPI master: %d\n" , ret); - goto err3; + goto err4; } dev_dbg(dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",@@ -1195,8 +1198,14 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) mem_res, (FIFO_LVL_MASK (sdd) >>1 ) +1 , sdd->rx_dma.dmach, sdd->tx_dma.dmach);Quoted 7 lines
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1202
Quoted 2 lines
drivers/spi/spi-s3c64xx.c +2
-0+pm_runtime_mark_last_busy (dev); +pm_runtime_autosuspend (dev);There is a high chance that device will be suspended already at this point because
pm_runtime_put()in setup. In most cases (when SPI is started) this won't do anything so we are back at first of my questions
- shouldn't you updatepm_runtime_put()calls?Quoted 6 lines
Yes, these calls should be updated.
Quoted 4 lines
Additionally (which is not related to your patch) shouldn't we
pm_runtime_get()at some point inprobe()to be sure that it won't be suspended to early?Not 100% sure at this moment, I will check.
Kernel documentation says:
"Note, if the device may executepm_runtimecalls during the probe (such as if it is registers with a subsystem that may call back in) then thepm_runtime_get_sync()call paired with apm_runtime_put()call will be appropriate to ensure that the device is not put back to sleep during the probe. This can happen with systems such as the network device layer."
Quoted 4 lines
I don't think this applies to SPI but I'm not expert enough to guarantee. At least no other SPI driver using runtime PM protects the probe function this way. Therefore I would dare to leave it as it is, especially as there don't seem to be any related known problems with SPI drivers.
But you know that these runtime callbacks will be executed anyway during probe because what I mentioned earlier - setup calls get and put. Additionally pm_runtime_enable() is called in the middle of probe, not at the end. This means that already it is assumed probe in certain time can handle runtime callbacks.
I think it is good opportunity to fix any remaining issues with runtime PM in this driver.
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 20.08.2015 um 01:41 schrieb Krzysztof Kozlowski:
Quoted 88 lines · Heiner Kallweit
Quoted 76 lines
Am 19.08.2015 um 07:54 schrieb Heiner Kallweit:
Quoted 62 lines
Am 19.08.2015 um 02:35 schrieb Krzysztof Kozlowski:
Quoted 10 lines · Heiner Kallweit
Quoted 4 lines
The patch fixes / extends the runtime PM handling.
- Currently the driver instructs the SPI core to use auto-suspend but doesn't configure it. Add the necessary calls and let it autosuspend after probe.After quick look at the driver I think it does not use autosuspend fully. It just sets
master->auto_runtime_pm(so the core code will use autosuspend) but its own calls topm_runtime_put()are regular ones. Shouldn't this be enhanced as well?Right, the
pm_runtime_putcalls ins3c64xx_spi_setupshould be replaced as well. Will provide an updated patch.Quoted 42 lines
Quoted 42 lines
- In case the driver registration fails runtime PM needs to be properly disabled.
Signed-off-by Heiner Kallweit <[email protected]>--- drivers/spi/spi-s3c64xx.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)drivers/spi/spi-s3c64xx.c +4
-1@@ -32,6 +32,7 @@#define MAX_SPI_PORTS 6#define S3C64XX_SPI_QUIRK_POLL (1 << 0)#define S3C64XX_SPI_QUIRK_CS_AUTO (1 << 1) +#define AUTOSUSPEND_TIMEOUT 2000/* Registers and bit-fields */ @@ -1180,13 +1181,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)S3C64XX_SPI_INT_TX_OVERRUN_EN |S3C64XX_SPI_INT_TX_UNDERRUN_EN , sdd->regs + S3C64XX_SPI_INT_EN); +pm_runtime_set_autosuspend_delay (dev,AUTOSUSPEND_TIMEOUT ); +pm_runtime_use_autosuspend (dev);pm_runtime_set_active (dev);pm_runtime_enable (dev); ret =devm_spi_register_master (dev, master);if (ret !=0 ) {dev_err (dev,"cannot register SPI master: %d\n" , ret); - goto err3; + goto err4; } dev_dbg(dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",@@ -1195,8 +1198,14 @@ static int s3c64xx_spi_probe(struct platform_device *pdev) mem_res, (FIFO_LVL_MASK (sdd) >>1 ) +1 , sdd->rx_dma.dmach, sdd->tx_dma.dmach);Quoted 7 lines
drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_probe(struct platform_device *pdev) · line 1202
Quoted 2 lines
drivers/spi/spi-s3c64xx.c +2
-0+pm_runtime_mark_last_busy (dev); +pm_runtime_autosuspend (dev);There is a high chance that device will be suspended already at this point because
pm_runtime_put()in setup. In most cases (when SPI is started) this won't do anything so we are back at first of my questions
- shouldn't you updatepm_runtime_put()calls?Quoted 6 lines
Yes, these calls should be updated.
Quoted 4 lines
Additionally (which is not related to your patch) shouldn't we
pm_runtime_get()at some point inprobe()to be sure that it won't be suspended to early?Not 100% sure at this moment, I will check.
Kernel documentation says:
"Note, if the device may executepm_runtimecalls during the probe (such as if it is registers with a subsystem that may call back in) then thepm_runtime_get_sync()call paired with apm_runtime_put()call will be appropriate to ensure that the device is not put back to sleep during the probe. This can happen with systems such as the network device layer."Quoted 4 lines
I don't think this applies to SPI but I'm not expert enough to guarantee. At least no other SPI driver using runtime PM protects the probe function this way. Therefore I would dare to leave it as it is, especially as there don't seem to be any related known problems with SPI drivers.
But you know that these runtime callbacks will be executed anyway during probe because what I mentioned earlier - setup calls get and put. Additionally
pm_runtime_enable()is called in the middle of probe, not at the end. This means that already it is assumed probe in certain time can handle runtime callbacks.
Quoted 5 lines
I think it is good opportunity to fix any remaining issues with runtime PM in this driver.
Best regards,
Krzysztof
OK. Will include this and resend. Thanks for the review.
Regards, Heiner
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html