diff mbox series

mmc: dw_mmc: rockchip: Keep controller working for card detect

Message ID 20240912152538.1.I858c2a0bf83606c8b59ba1ab6944978a398d2ac5@changeid
State New
Headers show
Series mmc: dw_mmc: rockchip: Keep controller working for card detect | expand

Commit Message

Kever Yang Sept. 12, 2024, 7:26 a.m. UTC
In order to make the SD card hotplug working we need the card detect
function logic inside the controller always working. The runtime PM will
gate the clock and the power domain, which stops controller working when
no data transfer happen.

So lets skip enable runtime PM when the card needs to detected by the
controller and the card is removable.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/mmc/host/dw_mmc-rockchip.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

Comments

FUKAUMI Naoki Sept. 14, 2024, 11:20 a.m. UTC | #1
hi

On 9/12/24 16:26, Kever Yang wrote:
> In order to make the SD card hotplug working we need the card detect
> function logic inside the controller always working. The runtime PM will
> gate the clock and the power domain, which stops controller working when
> no data transfer happen.
> 
> So lets skip enable runtime PM when the card needs to detected by the
> controller and the card is removable.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>

following RK3588(s) boards work fine without cd-gpios,

- Radxa E54C
- Radxa ROCK 5A
- Radxa ROCK 5B
- Radxa ROCK 5C

thank you very much!

Tested-by: FUKAUMI Naoki <naoki@radxa.com>

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> ---
> 
>   drivers/mmc/host/dw_mmc-rockchip.c | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index b07190ba4b7a..df91205f9cd3 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -345,28 +345,39 @@ static int dw_mci_rockchip_probe(struct platform_device *pdev)
>   	const struct dw_mci_drv_data *drv_data;
>   	const struct of_device_id *match;
>   	int ret;
> +	bool use_rpm = true;
>   
>   	if (!pdev->dev.of_node)
>   		return -ENODEV;
>   
> +	if (!device_property_read_bool(&pdev->dev, "non-removable") &&
> +	     !device_property_read_bool(&pdev->dev, "cd-gpios"))
> +		use_rpm = false;
> +
>   	match = of_match_node(dw_mci_rockchip_match, pdev->dev.of_node);
>   	drv_data = match->data;
>   
>   	pm_runtime_get_noresume(&pdev->dev);
>   	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> -	pm_runtime_use_autosuspend(&pdev->dev);
> +
> +	if (use_rpm) {
> +		pm_runtime_enable(&pdev->dev);
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +		pm_runtime_use_autosuspend(&pdev->dev);
> +	}
>   
>   	ret = dw_mci_pltfm_register(pdev, drv_data);
>   	if (ret) {
> -		pm_runtime_disable(&pdev->dev);
> -		pm_runtime_set_suspended(&pdev->dev);
> +		if (use_rpm) {
> +			pm_runtime_disable(&pdev->dev);
> +			pm_runtime_set_suspended(&pdev->dev);
> +		}
>   		pm_runtime_put_noidle(&pdev->dev);
>   		return ret;
>   	}
>   
> -	pm_runtime_put_autosuspend(&pdev->dev);
> +	if (use_rpm)
> +		pm_runtime_put_autosuspend(&pdev->dev);
>   
>   	return 0;
>   }
Heiko Stuebner Sept. 14, 2024, 11:52 a.m. UTC | #2
Am Donnerstag, 12. September 2024, 09:26:14 CEST schrieb Kever Yang:
> In order to make the SD card hotplug working we need the card detect
> function logic inside the controller always working. The runtime PM will
> gate the clock and the power domain, which stops controller working when
> no data transfer happen.
> 
> So lets skip enable runtime PM when the card needs to detected by the
> controller and the card is removable.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>

So for the change itself this looks good, i.e. it fixes an issue for baords relying
on the on-chip-card-detect.


But for boards doing that, the controller will be running _all the time_
even if there is never any card inserted.

So relying on the on-soc card-detect will effectively increase the power-
consumption of the board - even it it'll never use any sd-card?

> ---
> 
>  drivers/mmc/host/dw_mmc-rockchip.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
> index b07190ba4b7a..df91205f9cd3 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -345,28 +345,39 @@ static int dw_mci_rockchip_probe(struct platform_device *pdev)
>  	const struct dw_mci_drv_data *drv_data;
>  	const struct of_device_id *match;
>  	int ret;
> +	bool use_rpm = true;
>  
>  	if (!pdev->dev.of_node)
>  		return -ENODEV;
>  
> +	if (!device_property_read_bool(&pdev->dev, "non-removable") &&

It would be nice to add a comment here about the fact that this will
disable power-management for the controller.

Also shouldn't non-removable already work, making the case above not
necessary?


Thanks
Heiko

> +	     !device_property_read_bool(&pdev->dev, "cd-gpios"))
> +		use_rpm = false;
> +
>  	match = of_match_node(dw_mci_rockchip_match, pdev->dev.of_node);
>  	drv_data = match->data;
>  
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> -	pm_runtime_use_autosuspend(&pdev->dev);
> +
> +	if (use_rpm) {
> +		pm_runtime_enable(&pdev->dev);
> +		pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
> +		pm_runtime_use_autosuspend(&pdev->dev);
> +	}
>  
>  	ret = dw_mci_pltfm_register(pdev, drv_data);
>  	if (ret) {
> -		pm_runtime_disable(&pdev->dev);
> -		pm_runtime_set_suspended(&pdev->dev);
> +		if (use_rpm) {
> +			pm_runtime_disable(&pdev->dev);
> +			pm_runtime_set_suspended(&pdev->dev);
> +		}
>  		pm_runtime_put_noidle(&pdev->dev);
>  		return ret;
>  	}
>  
> -	pm_runtime_put_autosuspend(&pdev->dev);
> +	if (use_rpm)
> +		pm_runtime_put_autosuspend(&pdev->dev);
>  
>  	return 0;
>  }
>
diff mbox series

Patch

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index b07190ba4b7a..df91205f9cd3 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -345,28 +345,39 @@  static int dw_mci_rockchip_probe(struct platform_device *pdev)
 	const struct dw_mci_drv_data *drv_data;
 	const struct of_device_id *match;
 	int ret;
+	bool use_rpm = true;
 
 	if (!pdev->dev.of_node)
 		return -ENODEV;
 
+	if (!device_property_read_bool(&pdev->dev, "non-removable") &&
+	     !device_property_read_bool(&pdev->dev, "cd-gpios"))
+		use_rpm = false;
+
 	match = of_match_node(dw_mci_rockchip_match, pdev->dev.of_node);
 	drv_data = match->data;
 
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
-	pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
-	pm_runtime_use_autosuspend(&pdev->dev);
+
+	if (use_rpm) {
+		pm_runtime_enable(&pdev->dev);
+		pm_runtime_set_autosuspend_delay(&pdev->dev, 50);
+		pm_runtime_use_autosuspend(&pdev->dev);
+	}
 
 	ret = dw_mci_pltfm_register(pdev, drv_data);
 	if (ret) {
-		pm_runtime_disable(&pdev->dev);
-		pm_runtime_set_suspended(&pdev->dev);
+		if (use_rpm) {
+			pm_runtime_disable(&pdev->dev);
+			pm_runtime_set_suspended(&pdev->dev);
+		}
 		pm_runtime_put_noidle(&pdev->dev);
 		return ret;
 	}
 
-	pm_runtime_put_autosuspend(&pdev->dev);
+	if (use_rpm)
+		pm_runtime_put_autosuspend(&pdev->dev);
 
 	return 0;
 }