[PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin]

Fix WARN_ON:
[   22.869352][ T1885] clk_spi0 already unprepared
[   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
[   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
[   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
[   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
[   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
[   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214

Fixes:  ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index e1ecd96c7858..043a7739c330 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -940,33 +940,24 @@ static void rockchip_spi_remove(struct platform_device *pdev)
 	spi_controller_put(ctlr);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int rockchip_spi_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int rockchip_spi_runtime_suspend(struct device *dev)
 {
-	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = spi_controller_suspend(ctlr);
-	if (ret < 0)
-		return ret;
-
 	clk_disable_unprepare(rs->spiclk);
 	clk_disable_unprepare(rs->apb_pclk);
 
-	pinctrl_pm_select_sleep_state(dev);
-
 	return 0;
 }
 
-static int rockchip_spi_resume(struct device *dev)
+static int rockchip_spi_runtime_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	pinctrl_pm_select_default_state(dev);
-
 	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret < 0)
 		return ret;
@@ -975,41 +966,45 @@ static int rockchip_spi_resume(struct device *dev)
 	if (ret < 0)
 		clk_disable_unprepare(rs->apb_pclk);
 
-	ret = spi_controller_resume(ctlr);
-	if (ret < 0) {
-		clk_disable_unprepare(rs->spiclk);
-		clk_disable_unprepare(rs->apb_pclk);
-	}
-
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
 
-#ifdef CONFIG_PM
-static int rockchip_spi_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_spi_suspend(struct device *dev)
 {
+	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	clk_disable_unprepare(rs->spiclk);
-	clk_disable_unprepare(rs->apb_pclk);
+	ret = spi_controller_suspend(ctlr);
+	if (ret < 0)
+		return ret;
+
+	/* Avoid redundant clock disable */
+	if (!pm_runtime_status_suspended(dev))
+		rockchip_spi_runtime_suspend(dev);
+
+	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
 }
 
-static int rockchip_spi_runtime_resume(struct device *dev)
+static int rockchip_spi_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = clk_prepare_enable(rs->apb_pclk);
-	if (ret < 0)
-		return ret;
+	pinctrl_pm_select_default_state(dev);
 
-	ret = clk_prepare_enable(rs->spiclk);
+	if (!pm_runtime_status_suspended(dev)) {
+		ret = rockchip_spi_runtime_resume(dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = spi_controller_resume(ctlr);
 	if (ret < 0)
-		clk_disable_unprepare(rs->apb_pclk);
+		rockchip_spi_runtime_suspend(dev);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Kuan-Wei Chiu]

Hi Jon,

On Sat, Aug 24, 2024 at 12:57:02PM +0800, Jon Lin wrote:
> Fix WARN_ON:
> [   22.869352][ T1885] clk_spi0 already unprepared
> [   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
> [   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
> [   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
> [   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
> [   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
> [   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
> [   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214
> 
> Fixes:  ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")

Thanks for the patch. However, the fixes tag should include the 12
characters of SHA-1 ID. the corrected Fixes tag should be:

Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")

Regards,
Kuan-Wei

> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> ---
> 
>  drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index e1ecd96c7858..043a7739c330 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -940,33 +940,24 @@ static void rockchip_spi_remove(struct platform_device *pdev)
>  	spi_controller_put(ctlr);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int rockchip_spi_suspend(struct device *dev)
> +#ifdef CONFIG_PM
> +static int rockchip_spi_runtime_suspend(struct device *dev)
>  {
> -	int ret;
>  	struct spi_controller *ctlr = dev_get_drvdata(dev);
>  	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>  
> -	ret = spi_controller_suspend(ctlr);
> -	if (ret < 0)
> -		return ret;
> -
>  	clk_disable_unprepare(rs->spiclk);
>  	clk_disable_unprepare(rs->apb_pclk);
>  
> -	pinctrl_pm_select_sleep_state(dev);
> -
>  	return 0;
>  }
>  
> -static int rockchip_spi_resume(struct device *dev)
> +static int rockchip_spi_runtime_resume(struct device *dev)
>  {
>  	int ret;
>  	struct spi_controller *ctlr = dev_get_drvdata(dev);
>  	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>  
> -	pinctrl_pm_select_default_state(dev);
> -
>  	ret = clk_prepare_enable(rs->apb_pclk);
>  	if (ret < 0)
>  		return ret;
> @@ -975,41 +966,45 @@ static int rockchip_spi_resume(struct device *dev)
>  	if (ret < 0)
>  		clk_disable_unprepare(rs->apb_pclk);
>  
> -	ret = spi_controller_resume(ctlr);
> -	if (ret < 0) {
> -		clk_disable_unprepare(rs->spiclk);
> -		clk_disable_unprepare(rs->apb_pclk);
> -	}
> -
>  	return 0;
>  }
> -#endif /* CONFIG_PM_SLEEP */
> +#endif /* CONFIG_PM */
>  
> -#ifdef CONFIG_PM
> -static int rockchip_spi_runtime_suspend(struct device *dev)
> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_spi_suspend(struct device *dev)
>  {
> +	int ret;
>  	struct spi_controller *ctlr = dev_get_drvdata(dev);
> -	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>  
> -	clk_disable_unprepare(rs->spiclk);
> -	clk_disable_unprepare(rs->apb_pclk);
> +	ret = spi_controller_suspend(ctlr);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Avoid redundant clock disable */
> +	if (!pm_runtime_status_suspended(dev))
> +		rockchip_spi_runtime_suspend(dev);
> +
> +	pinctrl_pm_select_sleep_state(dev);
>  
>  	return 0;
>  }
>  
> -static int rockchip_spi_runtime_resume(struct device *dev)
> +static int rockchip_spi_resume(struct device *dev)
>  {
>  	int ret;
>  	struct spi_controller *ctlr = dev_get_drvdata(dev);
> -	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>  
> -	ret = clk_prepare_enable(rs->apb_pclk);
> -	if (ret < 0)
> -		return ret;
> +	pinctrl_pm_select_default_state(dev);
>  
> -	ret = clk_prepare_enable(rs->spiclk);
> +	if (!pm_runtime_status_suspended(dev)) {
> +		ret = rockchip_spi_runtime_resume(dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = spi_controller_resume(ctlr);
>  	if (ret < 0)
> -		clk_disable_unprepare(rs->apb_pclk);
> +		rockchip_spi_runtime_suspend(dev);
>  
>  	return 0;
>  }
> -- 
> 2.34.1
> 
> 

[PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin]

Fix WARN_ON:
[   22.869352][ T1885] clk_spi0 already unprepared
[   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
[   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
[   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
[   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
[   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
[   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214

Fixes:  ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index e1ecd96c7858..043a7739c330 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -940,33 +940,24 @@ static void rockchip_spi_remove(struct platform_device *pdev)
 	spi_controller_put(ctlr);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int rockchip_spi_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int rockchip_spi_runtime_suspend(struct device *dev)
 {
-	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = spi_controller_suspend(ctlr);
-	if (ret < 0)
-		return ret;
-
 	clk_disable_unprepare(rs->spiclk);
 	clk_disable_unprepare(rs->apb_pclk);
 
-	pinctrl_pm_select_sleep_state(dev);
-
 	return 0;
 }
 
-static int rockchip_spi_resume(struct device *dev)
+static int rockchip_spi_runtime_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	pinctrl_pm_select_default_state(dev);
-
 	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret < 0)
 		return ret;
@@ -975,41 +966,45 @@ static int rockchip_spi_resume(struct device *dev)
 	if (ret < 0)
 		clk_disable_unprepare(rs->apb_pclk);
 
-	ret = spi_controller_resume(ctlr);
-	if (ret < 0) {
-		clk_disable_unprepare(rs->spiclk);
-		clk_disable_unprepare(rs->apb_pclk);
-	}
-
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
 
-#ifdef CONFIG_PM
-static int rockchip_spi_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_spi_suspend(struct device *dev)
 {
+	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	clk_disable_unprepare(rs->spiclk);
-	clk_disable_unprepare(rs->apb_pclk);
+	ret = spi_controller_suspend(ctlr);
+	if (ret < 0)
+		return ret;
+
+	/* Avoid redundant clock disable */
+	if (!pm_runtime_status_suspended(dev))
+		rockchip_spi_runtime_suspend(dev);
+
+	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
 }
 
-static int rockchip_spi_runtime_resume(struct device *dev)
+static int rockchip_spi_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = clk_prepare_enable(rs->apb_pclk);
-	if (ret < 0)
-		return ret;
+	pinctrl_pm_select_default_state(dev);
 
-	ret = clk_prepare_enable(rs->spiclk);
+	if (!pm_runtime_status_suspended(dev)) {
+		ret = rockchip_spi_runtime_resume(dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = spi_controller_resume(ctlr);
 	if (ret < 0)
-		clk_disable_unprepare(rs->apb_pclk);
+		rockchip_spi_runtime_suspend(dev);
 
 	return 0;
 }
-- 
2.34.1

[PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin]

Fix WARN_ON:
[   22.869352][ T1885] clk_spi0 already unprepared
[   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
[   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
[   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
[   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
[   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
[   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214

Fixes:  ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index e1ecd96c7858..043a7739c330 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -940,33 +940,24 @@ static void rockchip_spi_remove(struct platform_device *pdev)
 	spi_controller_put(ctlr);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int rockchip_spi_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int rockchip_spi_runtime_suspend(struct device *dev)
 {
-	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = spi_controller_suspend(ctlr);
-	if (ret < 0)
-		return ret;
-
 	clk_disable_unprepare(rs->spiclk);
 	clk_disable_unprepare(rs->apb_pclk);
 
-	pinctrl_pm_select_sleep_state(dev);
-
 	return 0;
 }
 
-static int rockchip_spi_resume(struct device *dev)
+static int rockchip_spi_runtime_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	pinctrl_pm_select_default_state(dev);
-
 	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret < 0)
 		return ret;
@@ -975,41 +966,45 @@ static int rockchip_spi_resume(struct device *dev)
 	if (ret < 0)
 		clk_disable_unprepare(rs->apb_pclk);
 
-	ret = spi_controller_resume(ctlr);
-	if (ret < 0) {
-		clk_disable_unprepare(rs->spiclk);
-		clk_disable_unprepare(rs->apb_pclk);
-	}
-
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
 
-#ifdef CONFIG_PM
-static int rockchip_spi_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_spi_suspend(struct device *dev)
 {
+	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	clk_disable_unprepare(rs->spiclk);
-	clk_disable_unprepare(rs->apb_pclk);
+	ret = spi_controller_suspend(ctlr);
+	if (ret < 0)
+		return ret;
+
+	/* Avoid redundant clock disable */
+	if (!pm_runtime_status_suspended(dev))
+		rockchip_spi_runtime_suspend(dev);
+
+	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
 }
 
-static int rockchip_spi_runtime_resume(struct device *dev)
+static int rockchip_spi_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = clk_prepare_enable(rs->apb_pclk);
-	if (ret < 0)
-		return ret;
+	pinctrl_pm_select_default_state(dev);
 
-	ret = clk_prepare_enable(rs->spiclk);
+	if (!pm_runtime_status_suspended(dev)) {
+		ret = rockchip_spi_runtime_resume(dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = spi_controller_resume(ctlr);
 	if (ret < 0)
-		clk_disable_unprepare(rs->apb_pclk);
+		rockchip_spi_runtime_suspend(dev);
 
 	return 0;
 }
-- 
2.34.1

[PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin]

Fix WARN_ON:
[   22.869352][ T1885] clk_spi0 already unprepared
[   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
[   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
[   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
[   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
[   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
[   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214

Fixes:  ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index e1ecd96c7858..043a7739c330 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -940,33 +940,24 @@ static void rockchip_spi_remove(struct platform_device *pdev)
 	spi_controller_put(ctlr);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int rockchip_spi_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int rockchip_spi_runtime_suspend(struct device *dev)
 {
-	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = spi_controller_suspend(ctlr);
-	if (ret < 0)
-		return ret;
-
 	clk_disable_unprepare(rs->spiclk);
 	clk_disable_unprepare(rs->apb_pclk);
 
-	pinctrl_pm_select_sleep_state(dev);
-
 	return 0;
 }
 
-static int rockchip_spi_resume(struct device *dev)
+static int rockchip_spi_runtime_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	pinctrl_pm_select_default_state(dev);
-
 	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret < 0)
 		return ret;
@@ -975,41 +966,45 @@ static int rockchip_spi_resume(struct device *dev)
 	if (ret < 0)
 		clk_disable_unprepare(rs->apb_pclk);
 
-	ret = spi_controller_resume(ctlr);
-	if (ret < 0) {
-		clk_disable_unprepare(rs->spiclk);
-		clk_disable_unprepare(rs->apb_pclk);
-	}
-
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
 
-#ifdef CONFIG_PM
-static int rockchip_spi_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_spi_suspend(struct device *dev)
 {
+	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	clk_disable_unprepare(rs->spiclk);
-	clk_disable_unprepare(rs->apb_pclk);
+	ret = spi_controller_suspend(ctlr);
+	if (ret < 0)
+		return ret;
+
+	/* Avoid redundant clock disable */
+	if (!pm_runtime_status_suspended(dev))
+		rockchip_spi_runtime_suspend(dev);
+
+	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
 }
 
-static int rockchip_spi_runtime_resume(struct device *dev)
+static int rockchip_spi_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = clk_prepare_enable(rs->apb_pclk);
-	if (ret < 0)
-		return ret;
+	pinctrl_pm_select_default_state(dev);
 
-	ret = clk_prepare_enable(rs->spiclk);
+	if (!pm_runtime_status_suspended(dev)) {
+		ret = rockchip_spi_runtime_resume(dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = spi_controller_resume(ctlr);
 	if (ret < 0)
-		clk_disable_unprepare(rs->apb_pclk);
+		rockchip_spi_runtime_suspend(dev);
 
 	return 0;
 }
-- 
2.34.1

[PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin]

Fix WARN_ON:
[   22.869352][ T1885] clk_spi0 already unprepared
[   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
[   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
[   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
[   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
[   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
[   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
[   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214

Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index e1ecd96c7858..043a7739c330 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -940,33 +940,24 @@ static void rockchip_spi_remove(struct platform_device *pdev)
 	spi_controller_put(ctlr);
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int rockchip_spi_suspend(struct device *dev)
+#ifdef CONFIG_PM
+static int rockchip_spi_runtime_suspend(struct device *dev)
 {
-	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = spi_controller_suspend(ctlr);
-	if (ret < 0)
-		return ret;
-
 	clk_disable_unprepare(rs->spiclk);
 	clk_disable_unprepare(rs->apb_pclk);
 
-	pinctrl_pm_select_sleep_state(dev);
-
 	return 0;
 }
 
-static int rockchip_spi_resume(struct device *dev)
+static int rockchip_spi_runtime_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	pinctrl_pm_select_default_state(dev);
-
 	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret < 0)
 		return ret;
@@ -975,41 +966,45 @@ static int rockchip_spi_resume(struct device *dev)
 	if (ret < 0)
 		clk_disable_unprepare(rs->apb_pclk);
 
-	ret = spi_controller_resume(ctlr);
-	if (ret < 0) {
-		clk_disable_unprepare(rs->spiclk);
-		clk_disable_unprepare(rs->apb_pclk);
-	}
-
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
 
-#ifdef CONFIG_PM
-static int rockchip_spi_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_spi_suspend(struct device *dev)
 {
+	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	clk_disable_unprepare(rs->spiclk);
-	clk_disable_unprepare(rs->apb_pclk);
+	ret = spi_controller_suspend(ctlr);
+	if (ret < 0)
+		return ret;
+
+	/* Avoid redundant clock disable */
+	if (!pm_runtime_status_suspended(dev))
+		rockchip_spi_runtime_suspend(dev);
+
+	pinctrl_pm_select_sleep_state(dev);
 
 	return 0;
 }
 
-static int rockchip_spi_runtime_resume(struct device *dev)
+static int rockchip_spi_resume(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
-	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
-	ret = clk_prepare_enable(rs->apb_pclk);
-	if (ret < 0)
-		return ret;
+	pinctrl_pm_select_default_state(dev);
 
-	ret = clk_prepare_enable(rs->spiclk);
+	if (!pm_runtime_status_suspended(dev)) {
+		ret = rockchip_spi_runtime_resume(dev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = spi_controller_resume(ctlr);
 	if (ret < 0)
-		clk_disable_unprepare(rs->apb_pclk);
+		rockchip_spi_runtime_suspend(dev);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Brian Norris]

(NB: I have several nearly identical copies of this email. I'm replying
to the latest one I see.)

Hi Jon,

On Sun, Aug 25, 2024 at 11:54:22AM +0800, Jon Lin wrote:
> Fix WARN_ON:
> [   22.869352][ T1885] clk_spi0 already unprepared
> [   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
> [   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
> [   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
> [   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
> [   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
> [   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
> [   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214

I appreciate the snippet of a WARNING trace, but I'd also appreciate
some actual explanation of what the problem is, and why you're solving
it this way.

> Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> ---
> 
>  drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index e1ecd96c7858..043a7739c330 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c

> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_spi_suspend(struct device *dev)
>  {
> +	int ret;
>  	struct spi_controller *ctlr = dev_get_drvdata(dev);
> -	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>  
> -	clk_disable_unprepare(rs->spiclk);
> -	clk_disable_unprepare(rs->apb_pclk);
> +	ret = spi_controller_suspend(ctlr);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Avoid redundant clock disable */
> +	if (!pm_runtime_status_suspended(dev))
> +		rockchip_spi_runtime_suspend(dev);

It seems like you'd really be served well by
pm_runtime_force_{suspend,resume}() here, and in fact, that's what this
driver used to use before the breaking change (commit
e882575efc77). Why aren't you just going back to using it? (This is the
kind of thing I might expect in your commit message -- reasoning as to
why you're doing what you're doing.)

And in fact, I already submitted a patch that resolves the above problem
and does exactly that:

https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/
[PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling

Do you see any problem with it?

Thanks,
Brian

> +	pinctrl_pm_select_sleep_state(dev);
>  
>  	return 0;
>  }

Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin]

On 2024/8/27 6:27, Brian Norris wrote:
> (NB: I have several nearly identical copies of this email. I'm replying
> to the latest one I see.)
> 
> Hi Jon,
> 
> On Sun, Aug 25, 2024 at 11:54:22AM +0800, Jon Lin wrote:
>> Fix WARN_ON:
>> [   22.869352][ T1885] clk_spi0 already unprepared
>> [   22.869379][ T1885] WARNING: CPU: 3 PID: 1885 at drivers/clk/clk.c:813 clk_core_unprepare+0xbc4
>> [   22.869380][ T1885] Modules linked in: bcmdhd dhd_static_buf
>> [   22.869391][ T1885] CPU: 3 PID: 1885 Comm: Binder:355_2 Tainted: G        W         5.10.66 #59
>> [   22.869393][ T1885] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
>> [   22.869397][ T1885] pstate: 60400009 (nZCv daif +PAN -UAO -TCO BTYPE=--)
>> [   22.869401][ T1885] pc : clk_core_unprepare+0xbc/0x214
>> [   22.869404][ T1885] lr : clk_core_unprepare+0xbc/0x214
> 
> I appreciate the snippet of a WARNING trace, but I'd also appreciate
> some actual explanation of what the problem is, and why you're solving
> it this way.
> 

Thank you for the reminder.

>> Fixes: e882575efc77 ("spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops")
>> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
>> ---
>>
>>   drivers/spi/spi-rockchip.c | 57 +++++++++++++++++---------------------
>>   1 file changed, 26 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
>> index e1ecd96c7858..043a7739c330 100644
>> --- a/drivers/spi/spi-rockchip.c
>> +++ b/drivers/spi/spi-rockchip.c
> 
>> +#ifdef CONFIG_PM_SLEEP
>> +static int rockchip_spi_suspend(struct device *dev)
>>   {
>> +	int ret;
>>   	struct spi_controller *ctlr = dev_get_drvdata(dev);
>> -	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>>   
>> -	clk_disable_unprepare(rs->spiclk);
>> -	clk_disable_unprepare(rs->apb_pclk);
>> +	ret = spi_controller_suspend(ctlr);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Avoid redundant clock disable */
>> +	if (!pm_runtime_status_suspended(dev))
>> +		rockchip_spi_runtime_suspend(dev);
> 
> It seems like you'd really be served well by
> pm_runtime_force_{suspend,resume}() here, and in fact, that's what this
> driver used to use before the breaking change (commit
> e882575efc77). Why aren't you just going back to using it? (This is the
> kind of thing I might expect in your commit message -- reasoning as to
> why you're doing what you're doing.)
> 
> And in fact, I already submitted a patch that resolves the above problem
> and does exactly that:
> 
> https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/
> [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling
> 
> Do you see any problem with it?
> 

I have reviewed your submission and although the code has been 
simplified, the execution efficiency has decreased. So although it is a 
commonly used processing solution for SPI Upstream, I still hope to 
retain a more efficiency approach as I submitted.

> Thanks,
> Brian
> 
>> +	pinctrl_pm_select_sleep_state(dev);
>>   
>>   	return 0;
>>   }
> 

Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Brian Norris]

On Mon, Aug 26, 2024 at 6:33 PM Jon Lin <jon.lin@rock-chips.com> wrote:
> On 2024/8/27 6:27, Brian Norris wrote:
> > It seems like you'd really be served well by
> > pm_runtime_force_{suspend,resume}() here, and in fact, that's what this
> > driver used to use before the breaking change (commit
> > e882575efc77). Why aren't you just going back to using it? (This is the
> > kind of thing I might expect in your commit message -- reasoning as to
> > why you're doing what you're doing.)
> >
> > And in fact, I already submitted a patch that resolves the above problem
> > and does exactly that:
> >
> > https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/
> > [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling
> >
> > Do you see any problem with it?
> >
>
> I have reviewed your submission and although the code has been
> simplified, the execution efficiency has decreased. So although it is a
> commonly used processing solution for SPI Upstream, I still hope to
> retain a more efficiency approach as I submitted.

What do you mean by "efficiency"? You mean because there's
indirection, via the PM runtime framework? If so, I doubt that's a
priority for this piece of functionality -- simplicity is more
important than a function call or two when talking about system
suspend.

Additionally, simplicity has additional benefits -- it heads off
questions that your more complex code doesn't address. For example,
are runtime PM and system PM mutually exclusive? Do we have to
coordinate with any pending autosuspend? (Reading through
https://docs.kernel.org/power/runtime_pm.html, I believe these are not
actually concerns, but it's really not obvious and takes a bit of
reading.) But your patch makes it more likely that runtime and system
PM states get out of sync.

Anyway, if the patches really are equivalent, I suppose it can be the
maintainer's choice as to which is preferable.

Brian

Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Jon Lin]

On 2024/8/27 10:59, Brian Norris wrote:
> On Mon, Aug 26, 2024 at 6:33 PM Jon Lin <jon.lin@rock-chips.com> wrote:
>> On 2024/8/27 6:27, Brian Norris wrote:
>>> It seems like you'd really be served well by
>>> pm_runtime_force_{suspend,resume}() here, and in fact, that's what this
>>> driver used to use before the breaking change (commit
>>> e882575efc77). Why aren't you just going back to using it? (This is the
>>> kind of thing I might expect in your commit message -- reasoning as to
>>> why you're doing what you're doing.)
>>>
>>> And in fact, I already submitted a patch that resolves the above problem
>>> and does exactly that:
>>>
>>> https://lore.kernel.org/all/20240823214235.1718769-1-briannorris@chromium.org/
>>> [PATCH] spi: rockchip: Resolve unbalanced runtime PM / system PM handling
>>>
>>> Do you see any problem with it?
>>>
>>
>> I have reviewed your submission and although the code has been
>> simplified, the execution efficiency has decreased. So although it is a
>> commonly used processing solution for SPI Upstream, I still hope to
>> retain a more efficiency approach as I submitted.
> 
> What do you mean by "efficiency"? You mean because there's
> indirection, via the PM runtime framework? If so, I doubt that's a
> priority for this piece of functionality -- simplicity is more
> important than a function call or two when talking about system
> suspend.
> 

Your code is fine, and I have tested it locally. The interface 
simplification is indeed in line with the direction of community 
development, and your solution can be used as a solution.

> Additionally, simplicity has additional benefits -- it heads off
> questions that your more complex code doesn't address. For example,
> are runtime PM and system PM mutually exclusive? Do we have to
> coordinate with any pending autosuspend? (Reading through
> https://docs.kernel.org/power/runtime_pm.html, I believe these are not
> actually concerns, but it's really not obvious and takes a bit of
> reading.) But your patch makes it more likely that runtime and system
> PM states get out of sync.
> 
> Anyway, if the patches really are equivalent, I suppose it can be the
> maintainer's choice as to which is preferable.
> 
> Brian
> 

Re: [PATCH] spi: rockchip: Avoid redundant clock disable in pm operation

[Brian Norris]

Hi Jon,

On Sat, Aug 24, 2024 at 8:55 PM Jon Lin <jon.lin@rock-chips.com> wrote:
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_spi_suspend(struct device *dev)
>  {
> +       int ret;
>         struct spi_controller *ctlr = dev_get_drvdata(dev);
> -       struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>
> -       clk_disable_unprepare(rs->spiclk);
> -       clk_disable_unprepare(rs->apb_pclk);
> +       ret = spi_controller_suspend(ctlr);
> +       if (ret < 0)
> +               return ret;
> +
> +       /* Avoid redundant clock disable */
> +       if (!pm_runtime_status_suspended(dev))
> +               rockchip_spi_runtime_suspend(dev);

rockchip_spi_runtime_suspend() returns an error code. I understand
that it doesn't actually fail in practice, but you should probably
check it anyway.

> +
> +       pinctrl_pm_select_sleep_state(dev);
>
>         return 0;
>  }
>
> -static int rockchip_spi_runtime_resume(struct device *dev)
> +static int rockchip_spi_resume(struct device *dev)
>  {
>         int ret;
>         struct spi_controller *ctlr = dev_get_drvdata(dev);
> -       struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
>
> -       ret = clk_prepare_enable(rs->apb_pclk);
> -       if (ret < 0)
> -               return ret;
> +       pinctrl_pm_select_default_state(dev);
>
> -       ret = clk_prepare_enable(rs->spiclk);
> +       if (!pm_runtime_status_suspended(dev)) {
> +               ret = rockchip_spi_runtime_resume(dev);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       ret = spi_controller_resume(ctlr);
>         if (ret < 0)
> -               clk_disable_unprepare(rs->apb_pclk);
> +               rockchip_spi_runtime_suspend(dev);

I don't think this is valid error handling. AFAIK, failing the
resume() function doesn't actually "disable" the device in any way
(it's just informative at best), so we probably shouldn't disable
clocks here. Otherwise, you might leave the clock disabled while the
runtime PM framework thinks it's enabled.

Brian

>
>         return 0;
>  }
> --
> 2.34.1
>