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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Jul 2022 00:44:42 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     wens@...nel.org,
        Christian Kohlschütter 
        <christian@...lschutter.com>
Cc:     Markus Reichl <m.reichl@...etechno.de>,
        Heiko Stübner <heiko@...ech.de>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Linux MMC List <linux-mmc@...r.kernel.org>
Subject: Re: [PATCH v2] arm64: dts: rockchip: Fix SD card init on
 rk3399-nanopi4

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@...lschutter.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@...lschutter.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.
Download attachment "SDS00001.png" of type "image/png" (26741 bytes)