[PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[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>
---

 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;
 }
-- 
2.25.1

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[FUKAUMI Naoki]

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;
>   }

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[Heiko Stübner]

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;
>  }
> 

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[Ulf Hansson]

On Sat, 14 Sept 2024 at 13:52, Heiko Stübner <heiko@sntech.de> wrote:
>
> 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?

Good point! A better option is to use a polling based mechanism - and
we have MMC_CAP_NEEDS_POLL for exactly that.

Moreover, on DT based platforms one can even use the "broken-cd"
property to indicate this.

[...]

Kind regards
Uffe

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[Robin Murphy]

On 02/10/2024 10:55 pm, Ulf Hansson wrote:
> On Sat, 14 Sept 2024 at 13:52, Heiko Stübner <heiko@sntech.de> wrote:
>>
>> 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?
> 
> Good point! A better option is to use a polling based mechanism - and
> we have MMC_CAP_NEEDS_POLL for exactly that.
> 
> Moreover, on DT based platforms one can even use the "broken-cd"
> property to indicate this.

Except that goes further than is needed here, since it would fall back 
entirely to software-based polling for card presence. In this case the 
CD function is not broken in terms of actually detecting a card, it just 
doesn't work to wake the controller up from suspend because it can't 
fire its own interrupt while powered off. In principle all we should 
require here is to periodically resume/suspend the device, to provide a 
window for the interrupt to work and normal operation to take over if 
appropriate.

Of course the really clever way would be for suspend to switch the pin 
into GPIO mode, and set the GPIO interrupt as a wakeup to trigger resume 
and switch it back again, but perhaps that's a bit tricky without 
explicit pinctrl states in the DT :/

Thanks,
Robin.

> 
> [...]
> 
> Kind regards
> Uffe
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[Heiko Stübner]

Am Freitag, 4. Oktober 2024, 19:34:33 CEST schrieb Robin Murphy:
> On 02/10/2024 10:55 pm, Ulf Hansson wrote:
> > On Sat, 14 Sept 2024 at 13:52, Heiko Stübner <heiko@sntech.de> wrote:
> >>
> >> 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?
> > 
> > Good point! A better option is to use a polling based mechanism - and
> > we have MMC_CAP_NEEDS_POLL for exactly that.
> > 
> > Moreover, on DT based platforms one can even use the "broken-cd"
> > property to indicate this.
> 
> Except that goes further than is needed here, since it would fall back 
> entirely to software-based polling for card presence. In this case the 
> CD function is not broken in terms of actually detecting a card, it just 
> doesn't work to wake the controller up from suspend because it can't 
> fire its own interrupt while powered off. In principle all we should 
> require here is to periodically resume/suspend the device, to provide a 
> window for the interrupt to work and normal operation to take over if 
> appropriate.
> 
> Of course the really clever way would be for suspend to switch the pin 
> into GPIO mode, and set the GPIO interrupt as a wakeup to trigger resume 
> and switch it back again, but perhaps that's a bit tricky without 
> explicit pinctrl states in the DT :/

and then the question really becomes, why move away from cd-gpios at all.

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[Ulf Hansson]

On Fri, 4 Oct 2024 at 19:34, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 02/10/2024 10:55 pm, Ulf Hansson wrote:
> > On Sat, 14 Sept 2024 at 13:52, Heiko Stübner <heiko@sntech.de> wrote:
> >>
> >> 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?
> >
> > Good point! A better option is to use a polling based mechanism - and
> > we have MMC_CAP_NEEDS_POLL for exactly that.
> >
> > Moreover, on DT based platforms one can even use the "broken-cd"
> > property to indicate this.
>
> Except that goes further than is needed here, since it would fall back
> entirely to software-based polling for card presence. In this case the
> CD function is not broken in terms of actually detecting a card, it just
> doesn't work to wake the controller up from suspend because it can't
> fire its own interrupt while powered off. In principle all we should
> require here is to periodically resume/suspend the device, to provide a
> window for the interrupt to work and normal operation to take over if
> appropriate.

Well, I would not object if "broken-cd" would be used for this case
too. I believe it already is.

Another option would be to look at a compatible string and set
MMC_CAP_NEEDS_POLL based on that.

>
> Of course the really clever way would be for suspend to switch the pin
> into GPIO mode, and set the GPIO interrupt as a wakeup to trigger resume
> and switch it back again, but perhaps that's a bit tricky without
> explicit pinctrl states in the DT :/

Right. A dedicated GPIO pin for the card detect is certainly the
preferred method, if you care about not wasting power.

Kind regards
Uffe

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[Kever Yang]

Hi Ulf, Robin, Heiko,

On 2024/10/7 17:49, Ulf Hansson wrote:
> On Fri, 4 Oct 2024 at 19:34, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 02/10/2024 10:55 pm, Ulf Hansson wrote:
>>> On Sat, 14 Sept 2024 at 13:52, Heiko Stübner <heiko@sntech.de> wrote:
>>>> 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?
Yes, this is how the controller works, the controller needs the clock to 
make the detect logic work.
If we use gpio to implement this card-detect, it works because the GPIO 
controller/clock keeps working.
For the dw_mmc driver support, we should support both kind of implement 
due to the controller has this function,
so this patch is for the card-detect implement by the dwmmc controller, 
the controller need to keep working
- only for sd-card (so not include the "non-removable " device)
- also not disable rpm when "cd-gpios" is used.

For the power consumption, I believe it will increase, but very very 
small, we can't even monitor the change
if we use the normal equipment. The driver should make function works 
first, and then consider the power.

This patch is to make the dwmmc function works without gpio's help in 
dwmmc driver,
  and has no affect to the gpio option, people still able to use gpio to 
do the cd.


Thanks,
- Kever
>>> Good point! A better option is to use a polling based mechanism - and
>>> we have MMC_CAP_NEEDS_POLL for exactly that.
>>>
>>> Moreover, on DT based platforms one can even use the "broken-cd"
>>> property to indicate this.
>> Except that goes further than is needed here, since it would fall back
>> entirely to software-based polling for card presence. In this case the
>> CD function is not broken in terms of actually detecting a card, it just
>> doesn't work to wake the controller up from suspend because it can't
>> fire its own interrupt while powered off. In principle all we should
>> require here is to periodically resume/suspend the device, to provide a
>> window for the interrupt to work and normal operation to take over if
>> appropriate.
> Well, I would not object if "broken-cd" would be used for this case
> too. I believe it already is.
>
> Another option would be to look at a compatible string and set
> MMC_CAP_NEEDS_POLL based on that.
>
>> Of course the really clever way would be for suspend to switch the pin
>> into GPIO mode, and set the GPIO interrupt as a wakeup to trigger resume
>> and switch it back again, but perhaps that's a bit tricky without
>> explicit pinctrl states in the DT :/
> Right. A dedicated GPIO pin for the card detect is certainly the
> preferred method, if you care about not wasting power.
>
> Kind regards
> Uffe

Re: [PATCH] mmc: dw_mmc: rockchip: Keep controller working for card detect

[Ulf Hansson]

On Fri, 1 Nov 2024 at 08:59, Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Hi Ulf, Robin, Heiko,
>
> On 2024/10/7 17:49, Ulf Hansson wrote:
> > On Fri, 4 Oct 2024 at 19:34, Robin Murphy <robin.murphy@arm.com> wrote:
> >> On 02/10/2024 10:55 pm, Ulf Hansson wrote:
> >>> On Sat, 14 Sept 2024 at 13:52, Heiko Stübner <heiko@sntech.de> wrote:
> >>>> 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?
> Yes, this is how the controller works, the controller needs the clock to
> make the detect logic work.
> If we use gpio to implement this card-detect, it works because the GPIO
> controller/clock keeps working.

Right. On embedded battery driven platforms it's quite common that
there is some always-on logic (maybe via a PMIC) that helps to take
care of these GPIO irqs.

> For the dw_mmc driver support, we should support both kind of implement
> due to the controller has this function,
> so this patch is for the card-detect implement by the dwmmc controller,
> the controller need to keep working
> - only for sd-card (so not include the "non-removable " device)
> - also not disable rpm when "cd-gpios" is used.
>
> For the power consumption, I believe it will increase, but very very
> small, we can't even monitor the change

Right, that may be perfectly correct on the platform you are using. On
others it may not.

In general, it's a bad idea to keep devices runtime resumed, unless we
really have too. In particular when the are shared power-rails being
managed by a power-domain, for example.

> if we use the normal equipment. The driver should make function works
> first, and then consider the power.
>
> This patch is to make the dwmmc function works without gpio's help in
> dwmmc driver,
>   and has no affect to the gpio option, people still able to use gpio to
> do the cd.
>

I understand and you certainly have a point.

However, it sounds like you think there is a drawback involved to use
MMC_CAP_NEEDS_POLL in this case? In that case, can you please
elaborate why preventing runtime suspend would be better?

[...]

Kind regards
Uffe