Re: [PATCH] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

Re: [PATCH] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Robin Murphy]

On 2022-07-14 13:14, Christian Kohlschütter wrote:
>> Am 14.07.2022 um 13:41 schrieb Robin Murphy <robin.murphy@arm.com>:
>>
>> On 2022-07-14 00:41, Heiko Stübner wrote:
>>> Hi Christian,
>>> Am Donnerstag, 14. Juli 2022, 00:22:23 CEST schrieb Christian Kohlschütter:
>>>> mmc/SD-card initialization may sometimes fail on NanoPi r4s with
>>>> "mmc1: problem reading SD Status register" /
>>>> "mmc1: error -110 whilst initialising SD card"
>>>>
>>>> Moreover, rebooting would also sometimes hang.
>>>>
>>> Nit: here the commit message should continue with something like:
>>> -----
>>> This is caused by the vcc3v0-sd regulator referencing the wrong gpio.
>>> Fix the regulator to use the correct pin and drop the always-on property.
>>> -----
>>>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>>>> ---
>>>>   arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>>> index 8c0ff6c96e03..91789801ab03 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>>> @@ -67,10 +67,10 @@ vcc1v8_s3: vcc1v8-s3 {
>>>>   	vcc3v0_sd: vcc3v0-sd {
>>>>   		compatible = "regulator-fixed";
>>>>   		enable-active-high;
>>>> -		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
>>>> +		gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_HIGH>;
>>> The interesting question would be how nano-pi-specific that gpio is.
>>> I.e. this is the rk3399-nanopi4.dtsi that is shared by multiple board types,
>>> so can you check in schematics if gpio0-d6 is always used on all of them?
>>
>> On the R4S schematic, this is GPIO0_A1 same as the others. GPIO0 doesn't even have a bank D on RK3399, it only goes up to B5 :/
>>
>>> Thanks
>>> Heiko
>>>>   		pinctrl-names = "default";
>>>>   		pinctrl-0 = <&sdmmc0_pwr_h>;
>>>> -		regulator-always-on;
>>>> +		regulator-boot-on;
>>>>   		regulator-min-microvolt = <3000000>;
>>>>   		regulator-max-microvolt = <3000000>;
>>>>   		regulator-name = "vcc3v0_sd";
>>>> @@ -580,7 +580,7 @@ wifi_reg_on_h: wifi-reg_on-h {
>>>>     	sdmmc {
>>>>   		sdmmc0_det_l: sdmmc0-det-l {
>>>> -			rockchip,pins = <0 RK_PA7 RK_FUNC_GPIO &pcfg_pull_up>;
>>>> +			rockchip,pins = <0 RK_PD6 RK_FUNC_GPIO &pcfg_pull_up>;
>>
>> ...and claiming the card detect is on the same non-existent pin as the regulator enable is clearly even more wrong.
>>
>> Is this another case where a UHS card is involved, such that VCC_SDIO gets left at 1.8V after a reboot, so subsequent attempts to do the initial handshake where the card is expecting 3V logic levels fail? (AKA the Tinkerboard problem). Hobbling the regulator such that Linux can never actually turn VCC3V0_SD off, thus the card never gets reset, might appear to "work", but isn't the right thing to do.
>>
>> Robin.
> 
> Right, this is all very strange.
> 
> Indeed, I have a UHS card and the problem you describe.
> I've actually looked into the other RK3399 dts files to come up with that patch (e.g. rk3399-roc-pc.dtsi, which does this; also see [1])

That patch is simply adding a description of the VCC3V0_SD regulator 
which is correct for that board (at least according to the 
roc-rk3399-pc-plus schematic that I could find); very different from 
breaking an existing already-correct description.

> I understand that we should do the right thing, but I am by no means sure what the "right" thing for this problem is.
> That said, I wished someone with expertise and authority in the mmc/rockchip community would fix this for good.
> 
> There are several patches around this 1.8V/3V voltage dance that do work, get used by several distributions but didn't get merged to mainline because, well, there could be a better right thing to do (like [2], which is also still needed when using mainline u-boot).
> 
> Given the state of this issue, at least aligning the code to match another board's fix — which already is in the mainline kernel — seems a sensible approach to me. It unblocks users like me who would perhaps otherwise just give up on using these devices.

Indeed it's an unfortunate situation, and I'd like to see the "proper" 
solution too, but my point is more that this patch isn't even the 
correct way to do what this patch actually achieves. Tricking Linux into 
toggling a non-existent pin prevents it from turning this regulator off 
(or on, so we're already hoping that someone else has done that); 
however that's what the "regulator-always-on" property should already 
imply, so the patch should at least explain why we're taking this more 
drastic measure instead of trying to fix whatever causes the always-on 
property to apparently not be honoured. Furthermore, if it is justified 
to remove Linux's ability to control the regulator at all, then making 
up a bogus GPIO is still nonsensical (just remove it completely), and 
changing regulator-always-on to regulator-boot-on (saying that Linux 
*is* permitted to try to do the thing you're actively preventing it from 
doing) even more so.

The other concern I have is whether this is really a sufficiently robust 
workaround anyway. I'm not familiar enough to know whether a soft-reset 
at potentially the wrong I/O voltage is something that all cards are 
going to handle reliably and without risk of damage. Furthermore, if a 
card reset happens anyway for any other reason, e.g. the user physically 
swaps cards during the reboot, then it's still not going to work.

Thanks,
Robin.

> [1] https://lore.kernel.org/linux-mmc/3364813.APbW32NlgJ@phil/T/#m1b23cd2c43b600b5a6f084461ae53e482ad65316
> [2] https://patchwork.kernel.org/project/linux-mmc/patch/AM3PR03MB09664161A7FA2BD68B2800A7AC620@AM3PR03MB0966.eurprd03.prod.outlook.com/
> 

Re: [PATCH] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

> Am 14.07.2022 um 15:50 schrieb Robin Murphy <robin.murphy@arm.com>:
> 
> On 2022-07-14 13:14, Christian Kohlschütter wrote:
>>> Am 14.07.2022 um 13:41 schrieb Robin Murphy <robin.murphy@arm.com>:
>>> 
>>> On 2022-07-14 00:41, Heiko Stübner wrote:
>>>> Hi Christian,
>>>> Am Donnerstag, 14. Juli 2022, 00:22:23 CEST schrieb Christian Kohlschütter:
>>>>> mmc/SD-card initialization may sometimes fail on NanoPi r4s with
>>>>> "mmc1: problem reading SD Status register" /
>>>>> "mmc1: error -110 whilst initialising SD card"
>>>>> 
>>>>> Moreover, rebooting would also sometimes hang.
>>>>> 
>>>> Nit: here the commit message should continue with something like:
>>>> -----
>>>> This is caused by the vcc3v0-sd regulator referencing the wrong gpio.
>>>> Fix the regulator to use the correct pin and drop the always-on property.
>>>> -----
>>>>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>>>> index 8c0ff6c96e03..91789801ab03 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>>>> @@ -67,10 +67,10 @@ vcc1v8_s3: vcc1v8-s3 {
>>>>>  	vcc3v0_sd: vcc3v0-sd {
>>>>>  		compatible = "regulator-fixed";
>>>>>  		enable-active-high;
>>>>> -		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
>>>>> +		gpio = <&gpio0 RK_PD6 GPIO_ACTIVE_HIGH>;
>>>> The interesting question would be how nano-pi-specific that gpio is.
>>>> I.e. this is the rk3399-nanopi4.dtsi that is shared by multiple board types,
>>>> so can you check in schematics if gpio0-d6 is always used on all of them?
>>> 
>>> On the R4S schematic, this is GPIO0_A1 same as the others. GPIO0 doesn't even have a bank D on RK3399, it only goes up to B5 :/
>>> 
>>>> Thanks
>>>> Heiko
>>>>>  		pinctrl-names = "default";
>>>>>  		pinctrl-0 = <&sdmmc0_pwr_h>;
>>>>> -		regulator-always-on;
>>>>> +		regulator-boot-on;
>>>>>  		regulator-min-microvolt = <3000000>;
>>>>>  		regulator-max-microvolt = <3000000>;
>>>>>  		regulator-name = "vcc3v0_sd";
>>>>> @@ -580,7 +580,7 @@ wifi_reg_on_h: wifi-reg_on-h {
>>>>>    	sdmmc {
>>>>>  		sdmmc0_det_l: sdmmc0-det-l {
>>>>> -			rockchip,pins = <0 RK_PA7 RK_FUNC_GPIO &pcfg_pull_up>;
>>>>> +			rockchip,pins = <0 RK_PD6 RK_FUNC_GPIO &pcfg_pull_up>;
>>> 
>>> ...and claiming the card detect is on the same non-existent pin as the regulator enable is clearly even more wrong.
>>> 
>>> Is this another case where a UHS card is involved, such that VCC_SDIO gets left at 1.8V after a reboot, so subsequent attempts to do the initial handshake where the card is expecting 3V logic levels fail? (AKA the Tinkerboard problem). Hobbling the regulator such that Linux can never actually turn VCC3V0_SD off, thus the card never gets reset, might appear to "work", but isn't the right thing to do.
>>> 
>>> Robin.
>> Right, this is all very strange.
>> Indeed, I have a UHS card and the problem you describe.
>> I've actually looked into the other RK3399 dts files to come up with that patch (e.g. rk3399-roc-pc.dtsi, which does this; also see [1])
> 
> That patch is simply adding a description of the VCC3V0_SD regulator which is correct for that board (at least according to the roc-rk3399-pc-plus schematic that I could find); very different from breaking an existing already-correct description.
> 
>> I understand that we should do the right thing, but I am by no means sure what the "right" thing for this problem is.
>> That said, I wished someone with expertise and authority in the mmc/rockchip community would fix this for good.
>> There are several patches around this 1.8V/3V voltage dance that do work, get used by several distributions but didn't get merged to mainline because, well, there could be a better right thing to do (like [2], which is also still needed when using mainline u-boot).
>> Given the state of this issue, at least aligning the code to match another board's fix — which already is in the mainline kernel — seems a sensible approach to me. It unblocks users like me who would perhaps otherwise just give up on using these devices.
> 
> Indeed it's an unfortunate situation, and I'd like to see the "proper" solution too, but my point is more that this patch isn't even the correct way to do what this patch actually achieves. Tricking Linux into toggling a non-existent pin prevents it from turning this regulator off (or on, so we're already hoping that someone else has done that); however that's what the "regulator-always-on" property should already imply, so the patch should at least explain why we're taking this more drastic measure instead of trying to fix whatever causes the always-on property to apparently not be honoured. Furthermore, if it is justified to remove Linux's ability to control the regulator at all, then making up a bogus GPIO is still nonsensical (just remove it completely), and changing regulator-always-on to regulator-boot-on (saying that Linux *is* permitted to try to do the thing you're actively preventing it from doing) even more so.
> 
> The other concern I have is whether this is really a sufficiently robust workaround anyway. I'm not familiar enough to know whether a soft-reset at potentially the wrong I/O voltage is something that all cards are going to handle reliably and without risk of damage. Furthermore, if a card reset happens anyway for any other reason, e.g. the user physically swaps cards during the reboot, then it's still not going to work.
> 
> Thanks,
> Robin.

Thanks for the clarification, Robin! That makes a lot of sense to me now.

In a separate reply, Markus Reichl mentioned
> rk3399-roc-pc uses GPIO4-D6 for SDMMC0_PWR to enable VCC3V0_SD.
and that is what's reflected in rk3399-roc-pc.dtsi. Admittedly I copied that only partially — I accidentally used GPIO0 D6, which, as you correctly said, doesn't exist).

GPIO4-D6 is indeed SDMMC_PWREN_d, but for RK3308 [1], not RK3399 (where, for example, ROCK960 drives the blue LED [6]).
I can't find a reference to GPIO4_D6 in the ROC-RK3399-PC specification [7].
I can find a reference to GPIO4_D6 in the RK3399 Datasheet [8], but it doesn't mention SDMMC / power.

From what I understand when I look at the schematics of the nanopi4 family [1,2,3,4] (R4S, T4, M4, NEO4), VCC3V0_SD should be controllable via SDMMC0_PWR_H (which is GPIO0_A1, as in mainline).

So, my understanding is that GPIO0_A1 would indeed be the correct gpio binding, but something interferes with the power regulator when this configuration is set.

When I keep that gpio binding, the SD card would not be detected (problem reading SD Status register) or hang upon reboot (my original problem).
Here's the dmesg output with linux mainline (tested with 5.8.10)
> [    2.189133] dwmmc_rockchip fe320000.mmc: IDMAC supports 32-bit address mode.
> [    2.189852] dwmmc_rockchip fe320000.mmc: Using internal DMA controller.
> [    2.190451] dwmmc_rockchip fe320000.mmc: Version ID is 270a
> [    2.191075] dwmmc_rockchip fe320000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo
> [    2.193011] dwmmc_rockchip fe320000.mmc: Got CD GPIO
> [    2.206165] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [    2.380276] mmc1: problem reading SD Status register
> [    2.380832] mmc_host mmc1: Bus speed (slot 0) = 25000000Hz (slot req 25000000Hz, actual 25000000HZ div = 0)
> [    2.382181] mmc1: error -110 whilst initialising SD card
> [    2.409380] mmc_host mmc1: Bus speed (slot 0) = 300000Hz (slot req 300000Hz, actual 300000HZ div = 0)
> [    2.558118] mmc1: problem reading SD Status register
> [    2.558673] mmc_host mmc1: Bus speed (slot 0) = 25000000Hz (slot req 25000000Hz, actual 25000000HZ div = 0)
> [    2.560067] mmc1: error -110 whilst initialising SD card
> [    2.587388] mmc_host mmc1: Bus speed (slot 0) = 200000Hz (slot req 200000Hz, actual 200000HZ div = 0)
> [    2.743852] mmc1: problem reading SD Status register
> [    2.744407] mmc_host mmc1: Bus speed (slot 0) = 25000000Hz (slot req 25000000Hz, actual 25000000HZ div = 0)
> [    2.745633] mmc1: error -110 whilst initialising SD card


Therefore, changing this to some other GPIO, existing (GPIO4_D6) or non-existing (GPIO0_D6), might have just fixed it by chance (no one else uses GPIO4_D6) or because a non-existing gpio binding is equivalent to a missing binding.

rk3399-rock960.dtsi, for example, doesn't have a gpio binding for vcc3v0_sd, and I think that's a better approach for an always-on-regulator than assigning a non-existing gpio.

So, with no gpio binding or a different one, I get the satisfying output of a properly detected SDHC card, all the time (cold boot, warm boot after reset). I still need the patch from [9] so u-boot wouldn't hang due to finding an unexpected voltage (1.8V vs 3.0V)
> [    2.182438] dwmmc_rockchip fe320000.mmc: IDMAC supports 32-bit address mode.
> [    2.183136] dwmmc_rockchip fe320000.mmc: Usi internal DMA controller.
> [    2.183730] dwmmc_rockchip fe320000.mmc: Version ID is 270a
> [    2.184293] dwmmc_rockchip fe320000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo
> [    2.185824] dwmmc_rockchip fe320000.mmc: Got CD GPIO
> [    2.199454] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [    2.266425] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [    2.700022] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 212
> [    2.700656] mmc1: new ultra high speed SDR104 SDHC card at address 0001

Thankfully, now we have a better fix. Is that the right approach? I don't know. After reading through all these RK3399 DTBs, I have the feeling that neither is fully correct, and some were lucky enough to use a different GPIO.

Heiko Stübner asked,
> The interesting question would be how nano-pi-specific that gpio is.
> 
> I.e. this is the rk3399-nanopi4.dtsi that is shared by multiple board types,
> so can you check in schematics if gpio0-d6 is always used on all of them?

While my analysis suggests that removing the reference to GPIO0_A1 may fix potential SD card issues for any affected RK3399 boards, I can only verify with the board I have: It works for my R4S, and according to the board specifications, I can be quite certain that it applies to all rk3399-nanopi4 variants.

All in all, I hope you like this approach (removing the gpio binding, a one-line fix) better than the first take. A revised patch will follow after this email.

Best,
Christian

PS: This is my first attempt at fixing a DTS. I learned a lot since yesterday but definitely not enough. My apologies if I missed anything obvious.

[1] https://wiki.friendlyelec.com/wiki/images/c/c2/NanoPi-R4S-4GB-2008-Schematic.pdf
[2] https://wiki.friendlyelec.com/wiki/images/e/e0/NanoPC-T4-1902-Schematic.pdf
[3] https://esys.ir/images/img_Item/1492/Files/NanoPi-M4-2GB-1807-Schematic.pdf
[4] https://wiki.friendlyelec.com/wiki/images/5/5c/NanoPi-NEO4-1808-Schematic.pdf
[5] https://dl.radxa.com/rockpis/docs/hw/ROCK-PI-S_v13_sch_200910.pdf
[6] https://www.96boards.org/documentation/consumer/rock/rock960c/hardware-docs/hardware-user-manual.md.html
[7] https://drive.google.com/drive/folders/1zwrXegvh0q0u5Ru5pM2u5nJWS8-KbTdz
[8] https://www.rockchip.fr/RK3399%20datasheet%20V1.8.pdf
[9] https://patchwork.kernel.org/project/linux-mmc/patch/AM3PR03MB09664161A7FA2BD68B2800A7AC620@AM3PR03MB0966.eurprd03.prod.outlook.com/

[PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

mmc/SD-card initialization may fail on NanoPi r4s with
"mmc1: problem reading SD Status register" /
"mmc1: error -110 whilst initialising SD card"

Moreover, rebooting would also sometimes hang.

This is caused by the gpio entry for the vcc3v0-sd regulator;
even though it appears to be the correct GPIO pin, the presence
of the binding causes these errors.

Fix the regulator to drop the gpio binding and add a comment
to prevent accidental reintroduction of that entry.

Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index 8c0ff6c96e03..d5f8a62e01be 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -67,7 +67,7 @@ vcc1v8_s3: vcc1v8-s3 {
 	vcc3v0_sd: vcc3v0-sd {
 		compatible = "regulator-fixed";
 		enable-active-high;
-		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
+		// gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>; // breaks SDHC card support
 		pinctrl-names = "default";
 		pinctrl-0 = <&sdmmc0_pwr_h>;
 		regulator-always-on;
-- 
2.36.1

RE: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Loehle]

I only briefly skimmed the discussion, but does this mean that a soft-reset (CMD0) of a UHS (post-voltage-switch) will not work?
(As the card/spec requires a power-cycle by the host which will not come, right?)
Can you try this real quick? I can give you a mmc-utils snippet if you have trouble issuing one.
If that does indeed not work I think the general approach is to disable uhs in the dts or at least document that.
Regards,
Christian

-----Original Message-----
From: Christian Kohlschütter <christian@kohlschutter.com> 
Sent: Donnerstag, 14. Juli 2022 18:27
To: Robin Murphy <robin.murphy@arm.com>; Markus Reichl <m.reichl@fivetechno.de>; Heiko Stübner <heiko@sntech.de>; linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org; linux-kernel@vger.kernel.org; Linux MMC List <linux-mmc@vger.kernel.org>
Subject: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

mmc/SD-card initialization may fail on NanoPi r4s with
"mmc1: problem reading SD Status register" /
"mmc1: error -110 whilst initialising SD card"

Moreover, rebooting would also sometimes hang.

This is caused by the gpio entry for the vcc3v0-sd regulator;
even though it appears to be the correct GPIO pin, the presence
of the binding causes these errors.

Fix the regulator to drop the gpio binding and add a comment
to prevent accidental reintroduction of that entry.

Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index 8c0ff6c96e03..d5f8a62e01be 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -67,7 +67,7 @@ vcc1v8_s3: vcc1v8-s3 {
 	vcc3v0_sd: vcc3v0-sd {
 		compatible = "regulator-fixed";
 		enable-active-high;
-		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
+		// gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>; // breaks SDHC card support
 		pinctrl-names = "default";
 		pinctrl-0 = <&sdmmc0_pwr_h>;
 		regulator-always-on;
-- 
2.36.1



Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

Re: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

> Am 14.07.2022 um 18:44 schrieb Christian Loehle <CLoehle@hyperstone.com>:
> 
> I only briefly skimmed the discussion, but does this mean that a soft-reset (CMD0) of a UHS (post-voltage-switch) will not work?
> (As the card/spec requires a power-cycle by the host which will not come, right?)
> Can you try this real quick? I can give you a mmc-utils snippet if you have trouble issuing one.
> If that does indeed not work I think the general approach is to disable uhs in the dts or at least document that.
> Regards,
> Christian

I tried disabling UHS in the DTS, but that would still cause mmc detection issues.

"rmmod dw_mmc_rockchip" followed by "modprobe dw_mmc_rockchip" still detects the card:
> [ 4481.141764] mmc1: card 0001 removed
> [ 4488.133398] dwmmc_rockchip fe320000.mmc: IDMAC supports 32-bit address mode.
> [ 4488.133462] dwmmc_rockchip fe320000.mmc: Using internal DMA controller.
> [ 4488.133484] dwmmc_rockchip fe320000.mmc: Version ID is 270a
> [ 4488.133541] dwmmc_rockchip fe320000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo
> [ 4488.134320] dwmmc_rockchip fe320000.mmc: Got CD GPIO
> [ 4488.147329] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 4488.218364] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 4488.678181] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 214
> [ 4488.678239] mmc1: new ultra high speed SDR104 SDHC card at address 0001
> [ 4488.680315] mmcblk1: mmc1:0001 ASTC 14.6 GiB 
> [ 4488.684871]  mmcblk1: p1 p2

Ejecting/re-inserting the card also works:
> [ 4607.521119] mmc1: card 0001 removed
> [ 4608.517343] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 4608.632987] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 4609.065445] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 213
> [ 4609.065535] mmc1: new ultra high speed SDR104 SDHC card at address 0001
> [ 4609.067942] mmcblk1: mmc1:0001 ASTC 14.6 GiB 
> [ 4609.073521]  mmcblk1: p1 p2

and so is changing the clock back and forth:
> echo 400000 > /sys/kernel/debug/mmc1/clock; echo 150000000 > kernel/debug/mmc1/clock; fdisk -l /dev/mmcblk1

> [ 4817.829078] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 4820.063457] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 4835.305419] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 213
> [ 4836.346928]  mmcblk1: p1 p2

Swapping with a "highspeed" (non-UHS) card also seems to work
> [ 5733.702083] mmc1: card 0001 removed
> [ 5738.858439] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 5739.378487] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 50000000Hz, actual 50000000HZ div = 0)
> [ 5739.378627] mmc1: new high speed SD card at address 21bb
> [ 5739.380491] mmcblk1: mmc1:21bb APPSD 480 MiB 
> [ 5739.382795] debugfs: Directory 'mmcblk1' with parent 'block' already present!
> [ 5739.385096]  mmcblk1: p1
> [ 5774.386536] FAT-fs (mmcblk1p1): utf8 is not a recommended IO charset for FAT filesystems, filesystem will be case sensitive!
> [ 5795.486365] mmc1: card 21bb removed
> [ 5801.302688] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 5801.447128] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 5801.880374] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 211
> [ 5801.880440] mmc1: new ultra high speed SDR104 SDHC card at address 0001
> [ 5801.882253] mmcblk1: mmc1:0001 ASTC 14.6 GiB 
> [ 5801.884145] debugfs: Directory 'mmcblk1' with parent 'block' already present!
> [ 5801.886558]  mmcblk1: p1 p2
> 

Some debug output: cat /sys/kernel/debug/mmc1/ios
UHC
> clock:		150000000 Hz
> actual clock:	148500000 Hz
> vdd:		18 (3.0 ~ 3.1 V)
> bus mode:	2 (push-pull)
> chip select:	0 (don't care)
> power mode:	2 (on)
> bus width:	2 (4 bits)
> timing spec:	6 (sd uhs SDR104)
> signal voltage:	1 (1.80 V)
> driver type:	0 (driver type B)
non-UHC
> cat /sys/kernel/debug/mmc1/ios
> clock:		50000000 Hz
> vdd:		18 (3.0 ~ 3.1 V)
> bus mode:	2 (push-pull)
> chip select:	0 (don't care)
> power mode:	2 (on)
> bus width:	2 (4 bits)
> timing spec:	2 (sd high-speed)
> signal voltage:	0 (3.30 V)
> driver type:	0 (driver type B)
> 

How do I make sure I specifically send the soft-reset command? I'm happy to help but I'm really a novice here.

Best,
Christian

RE: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Loehle]

Please try my softreset patch for mmc-utils.

If some UHS cards have trouble coming up again then we have a problem on that hardware.

-----Original Message-----
From: Christian Kohlschütter <christian@kohlschutter.com> 
Sent: Donnerstag, 14. Juli 2022 19:21
To: Christian Loehle <CLoehle@hyperstone.com>
Cc: Robin Murphy <robin.murphy@arm.com>; Markus Reichl <m.reichl@fivetechno.de>; Heiko Stübner <heiko@sntech.de>; linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org; linux-kernel@vger.kernel.org; Linux MMC List <linux-mmc@vger.kernel.org>
Subject: Re: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

> Am 14.07.2022 um 18:44 schrieb Christian Loehle <CLoehle@hyperstone.com>:
> 
> I only briefly skimmed the discussion, but does this mean that a soft-reset (CMD0) of a UHS (post-voltage-switch) will not work?
> (As the card/spec requires a power-cycle by the host which will not come, right?)
> Can you try this real quick? I can give you a mmc-utils snippet if you have trouble issuing one.
> If that does indeed not work I think the general approach is to disable uhs in the dts or at least document that.
> Regards,
> Christian

I tried disabling UHS in the DTS, but that would still cause mmc detection issues.

"rmmod dw_mmc_rockchip" followed by "modprobe dw_mmc_rockchip" still detects the card:
> [ 4481.141764] mmc1: card 0001 removed
> [ 4488.133398] dwmmc_rockchip fe320000.mmc: IDMAC supports 32-bit address mode.
> [ 4488.133462] dwmmc_rockchip fe320000.mmc: Using internal DMA controller.
> [ 4488.133484] dwmmc_rockchip fe320000.mmc: Version ID is 270a
> [ 4488.133541] dwmmc_rockchip fe320000.mmc: DW MMC controller at irq 32,32 bit host data width,256 deep fifo
> [ 4488.134320] dwmmc_rockchip fe320000.mmc: Got CD GPIO
> [ 4488.147329] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 4488.218364] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 4488.678181] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 214
> [ 4488.678239] mmc1: new ultra high speed SDR104 SDHC card at address 0001
> [ 4488.680315] mmcblk1: mmc1:0001 ASTC 14.6 GiB 
> [ 4488.684871]  mmcblk1: p1 p2

Ejecting/re-inserting the card also works:
> [ 4607.521119] mmc1: card 0001 removed
> [ 4608.517343] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 4608.632987] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 4609.065445] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 213
> [ 4609.065535] mmc1: new ultra high speed SDR104 SDHC card at address 0001
> [ 4609.067942] mmcblk1: mmc1:0001 ASTC 14.6 GiB 
> [ 4609.073521]  mmcblk1: p1 p2

and so is changing the clock back and forth:
> echo 400000 > /sys/kernel/debug/mmc1/clock; echo 150000000 > kernel/debug/mmc1/clock; fdisk -l /dev/mmcblk1

> [ 4817.829078] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 4820.063457] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 4835.305419] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 213
> [ 4836.346928]  mmcblk1: p1 p2

Swapping with a "highspeed" (non-UHS) card also seems to work
> [ 5733.702083] mmc1: card 0001 removed
> [ 5738.858439] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 5739.378487] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 50000000Hz, actual 50000000HZ div = 0)
> [ 5739.378627] mmc1: new high speed SD card at address 21bb
> [ 5739.380491] mmcblk1: mmc1:21bb APPSD 480 MiB 
> [ 5739.382795] debugfs: Directory 'mmcblk1' with parent 'block' already present!
> [ 5739.385096]  mmcblk1: p1
> [ 5774.386536] FAT-fs (mmcblk1p1): utf8 is not a recommended IO charset for FAT filesystems, filesystem will be case sensitive!
> [ 5795.486365] mmc1: card 21bb removed
> [ 5801.302688] mmc_host mmc1: Bus speed (slot 0) = 400000Hz (slot req 400000Hz, actual 400000HZ div = 0)
> [ 5801.447128] mmc_host mmc1: Bus speed (slot 0) = 148500000Hz (slot req 150000000Hz, actual 148500000HZ div = 0)
> [ 5801.880374] dwmmc_rockchip fe320000.mmc: Successfully tuned phase to 211
> [ 5801.880440] mmc1: new ultra high speed SDR104 SDHC card at address 0001
> [ 5801.882253] mmcblk1: mmc1:0001 ASTC 14.6 GiB 
> [ 5801.884145] debugfs: Directory 'mmcblk1' with parent 'block' already present!
> [ 5801.886558]  mmcblk1: p1 p2
> 

Some debug output: cat /sys/kernel/debug/mmc1/ios
UHC
> clock:		150000000 Hz
> actual clock:	148500000 Hz
> vdd:		18 (3.0 ~ 3.1 V)
> bus mode:	2 (push-pull)
> chip select:	0 (don't care)
> power mode:	2 (on)
> bus width:	2 (4 bits)
> timing spec:	6 (sd uhs SDR104)
> signal voltage:	1 (1.80 V)
> driver type:	0 (driver type B)
non-UHC
> cat /sys/kernel/debug/mmc1/ios
> clock:		50000000 Hz
> vdd:		18 (3.0 ~ 3.1 V)
> bus mode:	2 (push-pull)
> chip select:	0 (don't care)
> power mode:	2 (on)
> bus width:	2 (4 bits)
> timing spec:	2 (sd high-speed)
> signal voltage:	0 (3.30 V)
> driver type:	0 (driver type B)
> 

How do I make sure I specifically send the soft-reset command? I'm happy to help but I'm really a novice here.

Best,
Christian

Hyperstone GmbH | Reichenaustr. 39a  | 78467 Konstanz
Managing Director: Dr. Jan Peter Berns.
Commercial register of local courts: Freiburg HRB381782

Re: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Chen-Yu Tsai]

On Fri, Jul 15, 2022 at 12:27 AM Christian Kohlschütter
<christian@kohlschutter.com> wrote:
>
> mmc/SD-card initialization may fail on NanoPi r4s with
> "mmc1: problem reading SD Status register" /
> "mmc1: error -110 whilst initialising SD card"
>
> Moreover, rebooting would also sometimes hang.
>
> This is caused by the gpio entry for the vcc3v0-sd regulator;
> even though it appears to be the correct GPIO pin, the presence
> of the binding causes these errors.
>
> Fix the regulator to drop the gpio binding and add a comment
> to prevent accidental reintroduction of that entry.
>
> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> index 8c0ff6c96e03..d5f8a62e01be 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> @@ -67,7 +67,7 @@ vcc1v8_s3: vcc1v8-s3 {
>         vcc3v0_sd: vcc3v0-sd {
>                 compatible = "regulator-fixed";
>                 enable-active-high;
> -               gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
> +               // gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>; // breaks SDHC card support

This change only means that the regulator no longer gets cycled when
it probes. It's not a proper fix. You're leaving the kernel without
any control over SD card power, and with whatever state the bootloader
left the GPIO in. If the bootloader left the GPIO low, then you don't
get to use the SD card, ever.

It cycles because of the lack of regulator-boot-on, so the driver
requests the GPIO with initial low state, and then drives it
high to enable the regulator.

>                 pinctrl-names = "default";
>                 pinctrl-0 = <&sdmmc0_pwr_h>;
>                 regulator-always-on;

I think dropping "regulator-always-on" so that Linux can cycle power
and properly reset the SD card is the proper fix to the card being
stuck in UHS and not responding.

Also, the regulator used is RT9193, according to the schematics. That
chip has an enable delay under 50 micro-seconds. If that needs to be
modeled, then add regulator-enable-ramp-delay.


Regards
ChenYu

> --
> 2.36.1
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Robin Murphy]

On 14/07/2022 6:02 pm, Chen-Yu Tsai wrote:
> On Fri, Jul 15, 2022 at 12:27 AM Christian Kohlschütter
> <christian@kohlschutter.com> wrote:
>>
>> mmc/SD-card initialization may fail on NanoPi r4s with
>> "mmc1: problem reading SD Status register" /
>> "mmc1: error -110 whilst initialising SD card"
>>
>> Moreover, rebooting would also sometimes hang.
>>
>> This is caused by the gpio entry for the vcc3v0-sd regulator;
>> even though it appears to be the correct GPIO pin, the presence
>> of the binding causes these errors.
>>
>> Fix the regulator to drop the gpio binding and add a comment
>> to prevent accidental reintroduction of that entry.
>>
>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>> ---
>>   arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>> index 8c0ff6c96e03..d5f8a62e01be 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>> @@ -67,7 +67,7 @@ vcc1v8_s3: vcc1v8-s3 {
>>          vcc3v0_sd: vcc3v0-sd {
>>                  compatible = "regulator-fixed";
>>                  enable-active-high;
>> -               gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
>> +               // gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>; // breaks SDHC card support
> 
> This change only means that the regulator no longer gets cycled when
> it probes. It's not a proper fix. You're leaving the kernel without
> any control over SD card power, and with whatever state the bootloader
> left the GPIO in. If the bootloader left the GPIO low, then you don't
> get to use the SD card, ever.
> 
> It cycles because of the lack of regulator-boot-on, so the driver
> requests the GPIO with initial low state, and then drives it
> high to enable the regulator.

Hmm, that's a good point - by the time we get to Linux, we should have 
control over the VCC_SDIO regulator and the I/O domain as well, so a 
full clean reset should really be no problem :/

The "Tinkerboard problem" I was thinking of is when the SD card is the 
boot medium, VCC_SDIO stays at 1.8V over a reboot but the I/O domain 
gets reset back to 3.3V mode, thus cannot see a logic high on any of the 
I/O lines, thus the boot ROM gives up after failing to detect the card. 
If we're still able to boot as far as Linux, it's probably a different 
thing. Apologies for the confusion.

>>                  pinctrl-names = "default";
>>                  pinctrl-0 = <&sdmmc0_pwr_h>;
>>                  regulator-always-on;
> 
> I think dropping "regulator-always-on" so that Linux can cycle power
> and properly reset the SD card is the proper fix to the card being
> stuck in UHS and not responding.
> 
> Also, the regulator used is RT9193, according to the schematics. That
> chip has an enable delay under 50 micro-seconds. If that needs to be
> modeled, then add regulator-enable-ramp-delay.

Indeed, if it's a slow regulator and we're simply trying to probe the 
card too soon before its supply voltage has actually stabilised, that 
sounds entirely plausible, particularly if the failure is only intermittent.

Thanks,
Robin.

Re: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

Am 14.07.2022 um 19:35 schrieb Robin Murphy <robin.murphy@arm.com>:
> 
> On 14/07/2022 6:02 pm, Chen-Yu Tsai wrote:
>> On Fri, Jul 15, 2022 at 12:27 AM Christian Kohlschütter
>> <christian@kohlschutter.com> wrote:
>>> 
>>> mmc/SD-card initialization may fail on NanoPi r4s with
>>> "mmc1: problem reading SD Status register" /
>>> "mmc1: error -110 whilst initialising SD card"
>>> 
>>> Moreover, rebooting would also sometimes hang.
>>> 
>>> This is caused by the gpio entry for the vcc3v0-sd regulator;
>>> even though it appears to be the correct GPIO pin, the presence
>>> of the binding causes these errors.
>>> 
>>> Fix the regulator to drop the gpio binding and add a comment
>>> to prevent accidental reintroduction of that entry.
>>> 
>>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> index 8c0ff6c96e03..d5f8a62e01be 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> @@ -67,7 +67,7 @@ vcc1v8_s3: vcc1v8-s3 {
>>>         vcc3v0_sd: vcc3v0-sd {
>>>                 compatible = "regulator-fixed";
>>>                 enable-active-high;
>>> -               gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
>>> +               // gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>; // breaks SDHC card support
>> This change only means that the regulator no longer gets cycled when
>> it probes. It's not a proper fix. You're leaving the kernel without
>> any control over SD card power, and with whatever state the bootloader
>> left the GPIO in. If the bootloader left the GPIO low, then you don't
>> get to use the SD card, ever.

This is the current situation in the mainline kernel (regulator-always-on is there and unchanged), but yes, you are correct that's a bug.

See below for my results with your proposed changes.

>> It cycles because of the lack of regulator-boot-on, so the driver
>> requests the GPIO with initial low state, and then drives it
>> high to enable the regulator.
> 
> Hmm, that's a good point - by the time we get to Linux, we should have control over the VCC_SDIO regulator and the I/O domain as well, so a full clean reset should really be no problem :/
> 
> The "Tinkerboard problem" I was thinking of is when the SD card is the boot medium, VCC_SDIO stays at 1.8V over a reboot but the I/O domain gets reset back to 3.3V mode, thus cannot see a logic high on any of the I/O lines, thus the boot ROM gives up after failing to detect the card. If we're still able to boot as far as Linux, it's probably a different thing. Apologies for the confusion.

I believe it's that problem you describe.

In my case, I boot off the mmc card with u-boot mainline v2022.07, which invokes IPXE that boots the kernel (5.18.10 mainline) + rk3399-nanopi-r4s dtb over the network. 
This lets me quickly test kernel changes without juggling sd-cards.

> 
>>>                 pinctrl-names = "default";
>>>                 pinctrl-0 = <&sdmmc0_pwr_h>;
>>>                 regulator-always-on;
>> I think dropping "regulator-always-on" so that Linux can cycle power
>> and properly reset the SD card is the proper fix to the card being
>> stuck in UHS and not responding.
>> Also, the regulator used is RT9193, according to the schematics. That
>> chip has an enable delay under 50 micro-seconds. If that needs to be
>> modeled, then add regulator-enable-ramp-delay.
> 
> Indeed, if it's a slow regulator and we're simply trying to probe the card too soon before its supply voltage has actually stabilised, that sounds entirely plausible, particularly if the failure is only intermittent.

With
> gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
> // regulator-always-on;
> regulator-enable-ramp-delay = <50>;
I'm still not getting the reset going. It looks the MMC gets detected reliably upon cold start, but I cannot test reboot/warm start.

It looks like there's one issue upon start (which MAY be fixed by these changes), but there's another issue upon restart ("suspend" in device tree lingo?).
I tried both

> regulator-state-mem {
>         regulator-off-in-suspend;
> };    
and
> regulator-state-mem {
>         regulator-on-in-suspend;
> };
to no avail (no difference; reboot still hangs).

What are we missing?

Best,
Christian

Re: [PATCH v2] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Robin Murphy]

[-- Attachment #1: Type: text/plain, Size: 4389 bytes --]

On 2022-07-14 18:35, Robin Murphy wrote:
> On 14/07/2022 6:02 pm, Chen-Yu Tsai wrote:
>> On Fri, Jul 15, 2022 at 12:27 AM Christian Kohlschütter
>> <christian@kohlschutter.com> wrote:
>>>
>>> mmc/SD-card initialization may fail on NanoPi r4s with
>>> "mmc1: problem reading SD Status register" /
>>> "mmc1: error -110 whilst initialising SD card"
>>>
>>> Moreover, rebooting would also sometimes hang.
>>>
>>> This is caused by the gpio entry for the vcc3v0-sd regulator;
>>> even though it appears to be the correct GPIO pin, the presence
>>> of the binding causes these errors.
>>>
>>> Fix the regulator to drop the gpio binding and add a comment
>>> to prevent accidental reintroduction of that entry.
>>>
>>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>>> ---
>>>   arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
>>> b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> index 8c0ff6c96e03..d5f8a62e01be 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> @@ -67,7 +67,7 @@ vcc1v8_s3: vcc1v8-s3 {
>>>          vcc3v0_sd: vcc3v0-sd {
>>>                  compatible = "regulator-fixed";
>>>                  enable-active-high;
>>> -               gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
>>> +               // gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>; // breaks 
>>> SDHC card support
>>
>> This change only means that the regulator no longer gets cycled when
>> it probes. It's not a proper fix. You're leaving the kernel without
>> any control over SD card power, and with whatever state the bootloader
>> left the GPIO in. If the bootloader left the GPIO low, then you don't
>> get to use the SD card, ever.
>>
>> It cycles because of the lack of regulator-boot-on, so the driver
>> requests the GPIO with initial low state, and then drives it
>> high to enable the regulator.
> 
> Hmm, that's a good point - by the time we get to Linux, we should have 
> control over the VCC_SDIO regulator and the I/O domain as well, so a 
> full clean reset should really be no problem :/
> 
> The "Tinkerboard problem" I was thinking of is when the SD card is the 
> boot medium, VCC_SDIO stays at 1.8V over a reboot but the I/O domain 
> gets reset back to 3.3V mode, thus cannot see a logic high on any of the 
> I/O lines, thus the boot ROM gives up after failing to detect the card. 
> If we're still able to boot as far as Linux, it's probably a different 
> thing. Apologies for the confusion.
> 
>>>                  pinctrl-names = "default";
>>>                  pinctrl-0 = <&sdmmc0_pwr_h>;
>>>                  regulator-always-on;
>>
>> I think dropping "regulator-always-on" so that Linux can cycle power
>> and properly reset the SD card is the proper fix to the card being
>> stuck in UHS and not responding.
>>
>> Also, the regulator used is RT9193, according to the schematics. That
>> chip has an enable delay under 50 micro-seconds. If that needs to be
>> modeled, then add regulator-enable-ramp-delay.
> 
> Indeed, if it's a slow regulator and we're simply trying to probe the 
> card too soon before its supply voltage has actually stabilised, that 
> sounds entirely plausible, particularly if the failure is only 
> intermittent.

For giggles, I scoped it on my NanoPC-T4. It actually takes about 60 
microseconds from asserting the enable to reach 3V, but then seems to 
take about another 100 after that to truly stabilise (the load in that 
case was a no-name 8GB high speed micro-SDHC). Thus if I wanted to test 
further I'd probably first try a significantly longer delay in the order 
of milliseconds to see whether that has any effect.

However I also gave it a bit of a stress test by removing 
regulator-always-on then repeatedly unbinding and rebinding the MMC 
driver to cycle the regulator and re-detect the card, and none of the 
cards I have to hand (from a fancy Sandisk Extreme Plus to a full-size 
128MB Lexar from about 2005 hanging off a comedy 60cm adapter cable) 
seemed to mind. So if something is up with R4S, I think it's fair to say 
that it doesn't affect all nanopi4 boards.

Cheers,
Robin.

[-- Attachment #2: SDS00001.png --]
[-- Type: image/png, Size: 26741 bytes --]

[PATCH v3] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

mmc/SD-card initialization may fail on NanoPi R4S with
"mmc1: problem reading SD Status register" /
"mmc1: error -110 whilst initialising SD card"
either on cold boot or after a reboot.

Moreover, the system would also sometimes hang upon reboot.

This is prevented by setting an explicit undervoltage protection limit
for the SD-card-specific vcc3v0_sd voltage regulator.

While using a limit of 3V seems to work, an additional safety buffer
should prevent accidental tripping, preventing a system hang.

Set the undervoltage protection limit to 2.7V, which is the minimum
permissible SD card operating voltage.

Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 4 ++++
 1 file changed, 4 insertions(+)
 mode change 100644 => 100755 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
old mode 100644
new mode 100755
index 8c0ff6c96e03..669c74ce4d13
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -73,6 +73,10 @@ vcc3v0_sd: vcc3v0-sd {
 		regulator-always-on;
 		regulator-min-microvolt = <3000000>;
 		regulator-max-microvolt = <3000000>;
+
+		// must be initialized or SD card may fail to initialize / system may hang
+		regulator-uv-protection-microvolt = <2700000>;
+
 		regulator-name = "vcc3v0_sd";
 		vin-supply = <&vcc3v3_sys>;
 	};
-- 
2.36.1

[PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

mmc/SD-card initialization may fail on NanoPi R4S with
"mmc1: problem reading SD Status register" /
"mmc1: error -110 whilst initialising SD card"
either on cold boot or after a reboot.

Moreover, the system would also sometimes hang upon reboot.

This is prevented by setting an explicit undervoltage protection limit
for the SD-card-specific vcc3v0_sd voltage regulator.

Set the undervoltage protection limit to 2.7V, which is the minimum
permissible SD card operating voltage.

Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
---
arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 4 ++++
1 file changed, 4 insertions(+)
mode change 100644 => 100755 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
old mode 100644
new mode 100755
index 8c0ff6c96e03..669c74ce4d13
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -73,6 +73,10 @@ vcc3v0_sd: vcc3v0-sd {
		regulator-always-on;
		regulator-min-microvolt = <3000000>;
		regulator-max-microvolt = <3000000>;
+
+		// must be configured or SD card may fail to initialize occasionally
+		regulator-uv-protection-microvolt = <2700000>;
+
		regulator-name = "vcc3v0_sd";
		vin-supply = <&vcc3v3_sys>;
	};
-- 
2.36.1

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

OK, this took me a while to figure out.

When no undervoltage limit is configured, I can reliably trigger the initialization bug upon boot.
When the limit is set to 3.0V, it rarely occurs, but just after I send the v3 patch, I was able to reproduce...

> Am 15.07.2022 um 19:12 schrieb Christian Kohlschütter <christian@kohlschutter.com>:
> 
> mmc/SD-card initialization may fail on NanoPi R4S with
> "mmc1: problem reading SD Status register" /
> "mmc1: error -110 whilst initialising SD card"
> either on cold boot or after a reboot.
> 
> Moreover, the system would also sometimes hang upon reboot.
> 
> This is prevented by setting an explicit undervoltage protection limit
> for the SD-card-specific vcc3v0_sd voltage regulator.
> 
> Set the undervoltage protection limit to 2.7V, which is the minimum
> permissible SD card operating voltage.
> 
> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
> ---
> arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 4 ++++
> 1 file changed, 4 insertions(+)
> mode change 100644 => 100755 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> old mode 100644
> new mode 100755
> index 8c0ff6c96e03..669c74ce4d13
> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
> @@ -73,6 +73,10 @@ vcc3v0_sd: vcc3v0-sd {
> 		regulator-always-on;
> 		regulator-min-microvolt = <3000000>;
> 		regulator-max-microvolt = <3000000>;
> +
> +		// must be configured or SD card may fail to initialize occasionally
> +		regulator-uv-protection-microvolt = <2700000>;
> +
> 		regulator-name = "vcc3v0_sd";
> 		vin-supply = <&vcc3v3_sys>;
> 	};
> -- 
> 2.36.1

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Robin Murphy]

On 2022-07-15 18:16, Christian Kohlschütter wrote:
> OK, this took me a while to figure out.
> 
> When no undervoltage limit is configured, I can reliably trigger the initialization bug upon boot.
> When the limit is set to 3.0V, it rarely occurs, but just after I send the v3 patch, I was able to reproduce...

Well this has to be in the running for "weirdest placebo ever"... :/

All it actually seems to achieve is printing an error[1] (this is after 
all a tiny 5-pin fixed-voltage LDO regulator, not an intelligent PMIC), 
and if that makes an appreciable difference then there has to be some 
kind of weird timing condition at play. Maybe regulator_register() ends 
up turning it off and on again rapidly enough that the card sees a 
voltage brownout and glitches, and adding more delay by printing to the 
console somewhere in the middle gives it enough time to act as a proper 
power cycle with no ill effect?

If you just whack something like an mdelay(500) at around that point in 
set_machine_constraints(), without the DT property, does it have the 
same effect?

Robin.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c#n1521

>> Am 15.07.2022 um 19:12 schrieb Christian Kohlschütter <christian@kohlschutter.com>:
>>
>> mmc/SD-card initialization may fail on NanoPi R4S with
>> "mmc1: problem reading SD Status register" /
>> "mmc1: error -110 whilst initialising SD card"
>> either on cold boot or after a reboot.
>>
>> Moreover, the system would also sometimes hang upon reboot.
>>
>> This is prevented by setting an explicit undervoltage protection limit
>> for the SD-card-specific vcc3v0_sd voltage regulator.
>>
>> Set the undervoltage protection limit to 2.7V, which is the minimum
>> permissible SD card operating voltage.
>>
>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>> ---
>> arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 4 ++++
>> 1 file changed, 4 insertions(+)
>> mode change 100644 => 100755 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>> old mode 100644
>> new mode 100755
>> index 8c0ff6c96e03..669c74ce4d13
>> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>> @@ -73,6 +73,10 @@ vcc3v0_sd: vcc3v0-sd {
>> 		regulator-always-on;
>> 		regulator-min-microvolt = <3000000>;
>> 		regulator-max-microvolt = <3000000>;
>> +
>> +		// must be configured or SD card may fail to initialize occasionally
>> +		regulator-uv-protection-microvolt = <2700000>;
>> +
>> 		regulator-name = "vcc3v0_sd";
>> 		vin-supply = <&vcc3v3_sys>;
>> 	};
>> -- 
>> 2.36.1
> 

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

> Am 15.07.2022 um 20:11 schrieb Robin Murphy <robin.murphy@arm.com>:
> 
> On 2022-07-15 18:16, Christian Kohlschütter wrote:
>> OK, this took me a while to figure out.
>> When no undervoltage limit is configured, I can reliably trigger the initialization bug upon boot.
>> When the limit is set to 3.0V, it rarely occurs, but just after I send the v3 patch, I was able to reproduce...
> 
> Well this has to be in the running for "weirdest placebo ever"... :/
> 
> All it actually seems to achieve is printing an error[1] (this is after all a tiny 5-pin fixed-voltage LDO regulator, not an intelligent PMIC), and if that makes an appreciable difference then there has to be some kind of weird timing condition at play. Maybe regulator_register() ends up turning it off and on again rapidly enough that the card sees a voltage brownout and glitches, and adding more delay by printing to the console somewhere in the middle gives it enough time to act as a proper power cycle with no ill effect?

That's definitely something between placebo and homeopathy :-)

I can confirm that setting a limit higher than 3.0V still works, which means that the one time incident where it still crashed means that there's indeed a timing issue at play, and adding that undervoltage statement (unlike the ramp-delay configs that I also tried) added just enough of a delay that made it work 99 out of 100 times.

> If you just whack something like an mdelay(500) at around that point in set_machine_constraints(), without the DT property, does it have the same effect?
Adding a delay for vcc3v0_sd works, which is great! (patch below)

Is there an existing path from device-tree parser to regular/core.c that we can use to specify this delay specifically for this regulator?
Also, what delay should we choose to make sure it works all the time and not just 99 out of 100 times?

Best,
Christian

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c4d844ffad7a..0e15ec2548f4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1483,6 +1483,11 @@ static int set_machine_constraints(struct regulator_dev *rdev)
                          "IC does not support requested over voltage limits\n");
        }
 
+if(!strncmp(rdev_get_name(rdev),"vcc3v0_sd",sizeof("vcc3v0_sd"))) {
+       rdev_err(rdev, "DELAY: %s\n", rdev_get_name(rdev));
+       mdelay(500);
+}
+
        if (rdev->constraints->under_voltage_detection)
                ret = handle_notify_limits(rdev,
                                           ops->set_under_voltage_protection,

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Robin Murphy]

[-- Attachment #1: Type: text/plain, Size: 3771 bytes --]

On 2022-07-15 19:11, Robin Murphy wrote:
> On 2022-07-15 18:16, Christian Kohlschütter wrote:
>> OK, this took me a while to figure out.
>>
>> When no undervoltage limit is configured, I can reliably trigger the 
>> initialization bug upon boot.
>> When the limit is set to 3.0V, it rarely occurs, but just after I send 
>> the v3 patch, I was able to reproduce...
> 
> Well this has to be in the running for "weirdest placebo ever"... :/
> 
> All it actually seems to achieve is printing an error[1] (this is after 
> all a tiny 5-pin fixed-voltage LDO regulator, not an intelligent PMIC), 
> and if that makes an appreciable difference then there has to be some 
> kind of weird timing condition at play. Maybe regulator_register() ends 
> up turning it off and on again rapidly enough that the card sees a 
> voltage brownout and glitches, and adding more delay by printing to the 
> console somewhere in the middle gives it enough time to act as a proper 
> power cycle with no ill effect?

...and apparently the answer is yes, it seems to be doing exactly that 
(see attached). But seemingly my SD cards don't mind, or maybe my T4 
board happens to have more capacitance than Christian's R4S so my 
voltage dip isn't as bad, or both.

So it seems like the solution here might indeed simply be to remove the 
regulator-always-on which doesn't seem to have any reason to be here 
anyway. Without that, the enable stays low until the MMC driver probes 
and claims it, which is then massively longer than the time it takes for 
VCC3V0_SD to ramp down completely.

Robin.

> 
> If you just whack something like an mdelay(500) at around that point in 
> set_machine_constraints(), without the DT property, does it have the 
> same effect?
> 
> Robin.
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/core.c#n1521 
> 
> 
>>> Am 15.07.2022 um 19:12 schrieb Christian Kohlschütter 
>>> <christian@kohlschutter.com>:
>>>
>>> mmc/SD-card initialization may fail on NanoPi R4S with
>>> "mmc1: problem reading SD Status register" /
>>> "mmc1: error -110 whilst initialising SD card"
>>> either on cold boot or after a reboot.
>>>
>>> Moreover, the system would also sometimes hang upon reboot.
>>>
>>> This is prevented by setting an explicit undervoltage protection limit
>>> for the SD-card-specific vcc3v0_sd voltage regulator.
>>>
>>> Set the undervoltage protection limit to 2.7V, which is the minimum
>>> permissible SD card operating voltage.
>>>
>>> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
>>> ---
>>> arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>> mode change 100644 => 100755 
>>> arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
>>> b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> old mode 100644
>>> new mode 100755
>>> index 8c0ff6c96e03..669c74ce4d13
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
>>> @@ -73,6 +73,10 @@ vcc3v0_sd: vcc3v0-sd {
>>>         regulator-always-on;
>>>         regulator-min-microvolt = <3000000>;
>>>         regulator-max-microvolt = <3000000>;
>>> +
>>> +        // must be configured or SD card may fail to initialize 
>>> occasionally
>>> +        regulator-uv-protection-microvolt = <2700000>;
>>> +
>>>         regulator-name = "vcc3v0_sd";
>>>         vin-supply = <&vcc3v3_sys>;
>>>     };
>>> -- 
>>> 2.36.1
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[-- Attachment #2: SDS00002.png --]
[-- Type: image/png, Size: 21500 bytes --]

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

Am 15.07.2022 um 20:57 schrieb Robin Murphy <robin.murphy@arm.com>:
> 
> On 2022-07-15 19:11, Robin Murphy wrote:
>> On 2022-07-15 18:16, Christian Kohlschütter wrote:
>>> OK, this took me a while to figure out.
>>> 
>>> When no undervoltage limit is configured, I can reliably trigger the initialization bug upon boot.
>>> When the limit is set to 3.0V, it rarely occurs, but just after I send the v3 patch, I was able to reproduce...
>> Well this has to be in the running for "weirdest placebo ever"... :/
>> All it actually seems to achieve is printing an error[1] (this is after all a tiny 5-pin fixed-voltage LDO regulator, not an intelligent PMIC), and if that makes an appreciable difference then there has to be some kind of weird timing condition at play. Maybe regulator_register() ends up turning it off and on again rapidly enough that the card sees a voltage brownout and glitches, and adding more delay by printing to the console somewhere in the middle gives it enough time to act as a proper power cycle with no ill effect?
> 
> ...and apparently the answer is yes, it seems to be doing exactly that (see attached). But seemingly my SD cards don't mind, or maybe my T4 board happens to have more capacitance than Christian's R4S so my voltage dip isn't as bad, or both.
> 
> So it seems like the solution here might indeed simply be to remove the regulator-always-on which doesn't seem to have any reason to be here anyway. Without that, the enable stays low until the MMC driver probes and claims it, which is then massively longer than the time it takes for VCC3V0_SD to ramp down completely.
> 
> Robin.

Removing "regulator-always-on" has the effect that the system freezes upon reboot. There may well be another bug slumbering in the codebase that is circumvented by 1. adding a delay in the code and 2. not turning the regulator off upon shutdown.

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Robin Murphy]

[-- Attachment #1: Type: text/plain, Size: 2574 bytes --]

On 2022-07-15 20:04, Christian Kohlschütter wrote:
> Am 15.07.2022 um 20:57 schrieb Robin Murphy <robin.murphy@arm.com>:
>>
>> On 2022-07-15 19:11, Robin Murphy wrote:
>>> On 2022-07-15 18:16, Christian Kohlschütter wrote:
>>>> OK, this took me a while to figure out.
>>>>
>>>> When no undervoltage limit is configured, I can reliably trigger the initialization bug upon boot.
>>>> When the limit is set to 3.0V, it rarely occurs, but just after I send the v3 patch, I was able to reproduce...
>>> Well this has to be in the running for "weirdest placebo ever"... :/
>>> All it actually seems to achieve is printing an error[1] (this is after all a tiny 5-pin fixed-voltage LDO regulator, not an intelligent PMIC), and if that makes an appreciable difference then there has to be some kind of weird timing condition at play. Maybe regulator_register() ends up turning it off and on again rapidly enough that the card sees a voltage brownout and glitches, and adding more delay by printing to the console somewhere in the middle gives it enough time to act as a proper power cycle with no ill effect?
>>
>> ...and apparently the answer is yes, it seems to be doing exactly that (see attached). But seemingly my SD cards don't mind, or maybe my T4 board happens to have more capacitance than Christian's R4S so my voltage dip isn't as bad, or both.
>>
>> So it seems like the solution here might indeed simply be to remove the regulator-always-on which doesn't seem to have any reason to be here anyway. Without that, the enable stays low until the MMC driver probes and claims it, which is then massively longer than the time it takes for VCC3V0_SD to ramp down completely.
>>
>> Robin.
> 
> Removing "regulator-always-on" has the effect that the system freezes upon reboot.

Ah, right (can we fast-forward to a world where everyone has a reliable 
bootloader in SPI flash or similar?). Is that more glitching, or a 
firmware bug not resetting the GPIOs to their default state on warm 
reset, I wonder.

> There may well be another bug slumbering in the codebase that is circumvented by 1. adding a delay in the code and 2. not turning the regulator off upon shutdown.

Yes, it seems suboptimal that the regulator core allows this glitch 
where an always-on regulator which is already on gets turned off at all, 
but I guess that's its own problem. In the meantime, off-on-delay-us 
sounds like the most likely property to bandage this locally. I'm seeing 
a fall time in the order of milliseconds (attached), so we'd probably 
want a fair chunk of that to be safe.

Robin.

[-- Attachment #2: SDS00003.png --]
[-- Type: image/png, Size: 22276 bytes --]

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

> Am 15.07.2022 um 21:38 schrieb Robin Murphy <robin.murphy@arm.com>:
> 
> On 2022-07-15 20:04, Christian Kohlschütter wrote:
>> Am 15.07.2022 um 20:57 schrieb Robin Murphy <robin.murphy@arm.com>:
>>> 
>>> On 2022-07-15 19:11, Robin Murphy wrote:
>>>> On 2022-07-15 18:16, Christian Kohlschütter wrote:
>>>>> OK, this took me a while to figure out.
>>>>> 
>>>>> When no undervoltage limit is configured, I can reliably trigger the initialization bug upon boot.
>>>>> When the limit is set to 3.0V, it rarely occurs, but just after I send the v3 patch, I was able to reproduce...
>>>> Well this has to be in the running for "weirdest placebo ever"... :/
>>>> All it actually seems to achieve is printing an error[1] (this is after all a tiny 5-pin fixed-voltage LDO regulator, not an intelligent PMIC), and if that makes an appreciable difference then there has to be some kind of weird timing condition at play. Maybe regulator_register() ends up turning it off and on again rapidly enough that the card sees a voltage brownout and glitches, and adding more delay by printing to the console somewhere in the middle gives it enough time to act as a proper power cycle with no ill effect?
>>> 
>>> ...and apparently the answer is yes, it seems to be doing exactly that (see attached). But seemingly my SD cards don't mind, or maybe my T4 board happens to have more capacitance than Christian's R4S so my voltage dip isn't as bad, or both.
>>> 
>>> So it seems like the solution here might indeed simply be to remove the regulator-always-on which doesn't seem to have any reason to be here anyway. Without that, the enable stays low until the MMC driver probes and claims it, which is then massively longer than the time it takes for VCC3V0_SD to ramp down completely.
>>> 
>>> Robin.
>> Removing "regulator-always-on" has the effect that the system freezes upon reboot.
> 
> Ah, right (can we fast-forward to a world where everyone has a reliable bootloader in SPI flash or similar?). Is that more glitching, or a firmware bug not resetting the GPIOs to their default state on warm reset, I wonder.
> 
>> There may well be another bug slumbering in the codebase that is circumvented by 1. adding a delay in the code and 2. not turning the regulator off upon shutdown.
> 
> Yes, it seems suboptimal that the regulator core allows this glitch where an always-on regulator which is already on gets turned off at all, but I guess that's its own problem. In the meantime, off-on-delay-us sounds like the most likely property to bandage this locally. I'm seeing a fall time in the order of milliseconds (attached), so we'd probably want a fair chunk of that to be safe.
> 
> Robin.<SDS00003.png>

I think we have a way where there's no need to pick a delay value that may ultimately not work in all cases.
Following up with "[PATCH] regulator: core: Resolve supply name earlier to prevent double-init" [1]

Thank you so much for helping me getting that far! It would be great if you'd keep following the thread.

Best,
Christian

[1] https://www.spinics.net/lists/kernel/msg4440365.html

Re: [PATCH v4] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

>> 
>>> There may well be another bug slumbering in the codebase that is circumvented by 1. adding a delay in the code and 2. not turning the regulator off upon shutdown.
>> 
>> Yes, it seems suboptimal that the regulator core allows this glitch where an always-on regulator which is already on gets turned off at all, but I guess that's its own problem. In the meantime, off-on-delay-us sounds like the most likely property to bandage this locally. I'm seeing a fall time in the order of milliseconds (attached), so we'd probably want a fair chunk of that to be safe.
>> 
>> Robin.<SDS00003.png>
> 
> I think we have a way where there's no need to pick a delay value that may ultimately not work in all cases.
> Following up with "[PATCH] regulator: core: Resolve supply name earlier to prevent double-init" [1]
> 
> Thank you so much for helping me getting that far! It would be great if you'd keep following the thread.
> 
> Best,
> Christian
> 
> [1] https://www.spinics.net/lists/kernel/msg4440365.html

@Robin,

oddly enough, setting off-on-delay-us with values of up to a second (1000000 us) still results in failed inits.
I hope we can find another bandage until the regular-core patch gets merged.

[PATCH v5] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

mmc/SD-card initialization may fail on NanoPi R4S with
"mmc1: problem reading SD Status register" /
"mmc1: error -110 whilst initialising SD card"
either on cold boot or after a reboot.

Moreover, the system would also sometimes hang upon reboot.

This is caused by vcc3v0-sd's "regulator-always-on", which triggers
an erroneous double-initialization of the regulator. This causes
voltage fluctuations that can, depending on timing, prevent the
SD card from initializing correctly.

Adding some liberal delay via "off-on-delay-us" is ineffective since
that codepath is skipped as long "regulator-always-on" is set.

Removing "regulator-always-on" alone is not sufficient because that
would allow the system to set GPIO0_A1 to LOW upon reboot, which may
cause the system to hang.

In order to allow the system to set GPIO0_A1 to HIGH upon initialization
but prevent it from changing it back to LOW, this patch increases the
usage count of vcc3v0-sd from 1 to 2, whereas the additional reference,
"vcc1v8_s3", is marked as "always-on", causing permanent retention.

As a welcome side-effect, this change allows the SD card voltage to be
set back to 3.0V upon reboot, allowing bootloaders to use the card right
away, obsoleting further patching.

Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
---
 arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index 8c0ff6c96e03..38507a6e3046 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -61,7 +61,17 @@ vcc1v8_s3: vcc1v8-s3 {
 		regulator-min-microvolt = <1800000>;
 		regulator-max-microvolt = <1800000>;
 		regulator-name = "vcc1v8_s3";
-		vin-supply = <&vcc_1v8>;
+
+		/*
+		 * Workaround to skip setting gpio0 RK_PA1 to LOW upon reboot,
+		 * which may freeze the system.
+		 *
+		 * Adding a reference to vcc3v0_sd increases its num_users
+		 * count to 2, preventing deactivation since this regulator is
+		 * marked "always-on".
+		 */
+		// vin-supply = <&vcc_1v8>; // actual supply
+		vin-supply = <&vcc3v0_sd>;
 	};
 
 	vcc3v0_sd: vcc3v0-sd {
@@ -70,7 +80,6 @@ vcc3v0_sd: vcc3v0-sd {
 		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&sdmmc0_pwr_h>;
-		regulator-always-on;
 		regulator-min-microvolt = <3000000>;
 		regulator-max-microvolt = <3000000>;
 		regulator-name = "vcc3v0_sd";
-- 
2.36.1

[PATCH v6] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

mmc/SD-card initialization may fail on NanoPi R4S with
"mmc1: problem reading SD Status register" /
"mmc1: error -110 whilst initialising SD card"
either on cold boot or after a reboot.

Moreover, the system would also sometimes hang upon reboot.

This is caused by vcc3v0-sd's "regulator-always-on", which triggers
an erroneous double-initialization of the regulator. This causes
voltage fluctuations that can, depending on timing, prevent the
SD card from initializing correctly.

Adding some liberal delay via "off-on-delay-us" is ineffective since
that codepath is skipped as long "regulator-always-on" is set.

Removing "regulator-always-on" alone is not sufficient because that
would allow the system to set GPIO0_A1 to LOW upon reboot, which may
cause the system to hang.

In order to allow the system to set GPIO0_A1 to HIGH upon initialization
but prevent it from changing it back to LOW, this patch increases the
usage count of vcc3v0-sd from 1 to 2, whereas the additional reference,
"vcc1v8_s3", is marked as "always-on", causing permanent retention.

Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>
---
arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
index 8c0ff6c96e03..38507a6e3046 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -61,7 +61,17 @@ vcc1v8_s3: vcc1v8-s3 {
		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
		regulator-name = "vcc1v8_s3";
-		vin-supply = <&vcc_1v8>;
+
+		/*
+		 * Workaround to skip setting gpio0 RK_PA1 to LOW upon reboot,
+		 * which may freeze the system.
+		 *
+		 * Adding a reference to vcc3v0_sd increases its num_users
+		 * count to 2, preventing deactivation since this regulator is
+		 * marked "always-on".
+		 */
+		// vin-supply = <&vcc_1v8>; // actual supply
+		vin-supply = <&vcc3v0_sd>;
	};

	vcc3v0_sd: vcc3v0-sd {
@@ -70,7 +80,6 @@ vcc3v0_sd: vcc3v0-sd {
		gpio = <&gpio0 RK_PA1 GPIO_ACTIVE_HIGH>;
		pinctrl-names = "default";
		pinctrl-0 = <&sdmmc0_pwr_h>;
-		regulator-always-on;
		regulator-min-microvolt = <3000000>;
		regulator-max-microvolt = <3000000>;
		regulator-name = "vcc3v0_sd";
-- 
2.36.1

Re: [PATCH v6] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

> Am 18.07.2022 um 14:04 schrieb Christian Kohlschütter <christian@kohlschutter.com>:
> 
> mmc/SD-card initialization may fail on NanoPi R4S with
> "mmc1: problem reading SD Status register" /
> "mmc1: error -110 whilst initialising SD card"
> either on cold boot or after a reboot.
...
Walking back on my claim in the commit message that no further patches are needed for the u-boot integration to work.
Other than that, the actual patch is unchanged.

Re: [PATCH v6] arm64: dts: rockchip: Fix SD card init on rk3399-nanopi4

[Christian Kohlschütter]

Am 18.07.2022 um 14:05 schrieb Christian Kohlschütter <christian@kohlschutter.com>:
> 
>> Am 18.07.2022 um 14:04 schrieb Christian Kohlschütter <christian@kohlschutter.com>:
>> 
>> mmc/SD-card initialization may fail on NanoPi R4S with
>> "mmc1: problem reading SD Status register" /
>> "mmc1: error -110 whilst initialising SD card"
>> either on cold boot or after a reboot.
> ...
> Walking back on my claim in the commit message that no further patches are needed for the u-boot integration to work.
> Other than that, the actual patch is unchanged.

I can no longer verify that the v6 patch actually increases use_count to 2.

At this point, I don't have a purely device tree-based fix that works reliably, therefore necessitating the code changes in regulator/core.c.
If anyone of you knows a way to reliably increase vcc3v0_sd's use_count ("cat /sys/kernel/debug/regulator/vcc3v0_sd/use_count") to at least 2, then we can remove the regulator-always-on statement.