Re: [PATCH 3/3] arm64: dts: rockchip: fix RockPro64 sdmmc settings

[Soeren Moch]

On 11.10.19 10:22, Jonas Karlman wrote:
> On 2019-10-04 19:24, SÃren Moch wrote:
>> On 04.10.19 17:33, Shawn Lin wrote:
>>> On 2019/10/4 22:20, Robin Murphy wrote:
>>>> On 04/10/2019 04:39, Soeren Moch wrote:
>>>>> On 04.10.19 04:13, Shawn Lin wrote:
>>>>>> On 2019/10/4 8:53, Soeren Moch wrote:
>>>>>>> On 04.10.19 02:01, Robin Murphy wrote:
>>>>>>>> On 2019-10-03 10:50 pm, Soeren Moch wrote:
>>>>>>>>> According to the RockPro64 schematic [1] the rk3399 sdmmc
>>>>>>>>> controller is
>>>>>>>>> connected to a microSD (TF card) slot, which cannot be switched to
>>>>>>>>> 1.8V.
>>>>>>>> Really? AFAICS the SDMMC0 wiring looks pretty much identical to the
>>>>>>>> NanoPC-T4 schematic (it's the same reference design, after all),
>>>>>>>> and I
>>>>>>>> know that board can happily drive a UHS-I microSD card with 1.8v
>>>>>>>> I/Os,
>>>>>>>> because mine's doing so right now.
>>>>>>>>
>>>>>>>> Robin.
>>>>>>> OK, the RockPro64 does not allow a card reset (power cycle) since
>>>>>>> VCC3V0_SD is directly connected to VCC3V3_SYS (via R89555), the
>>>>>>> SDMMC0_PWH_H signal is not connected. So the card fails to identify
>>>>>>> itself after suspend or reboot when switched to 1.8V operation.
>>>> Ah, thanks for clarifying - I did overlook the subtlety that U12 and
>>>> friends have "NC" as alternative part numbers, even though they
>>>> aren't actually marked as DNP. So it's still not so much "cannot be
>>>> switched" as "switching can lead to other problems".
>>>>
>>>>>> I believe we addressed this issue long time ago, please check:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6a11fc47f175c8d87018e89cb58e2d36c66534cb
>>>>>>
>>>>>>
>>>>> Thanks for the pointer.
>>>>> In this case I guess I should use following patch instead:
>>>>>
>>>>> --- rk3399-rockpro64.dts.bak ÂÂ 2019-10-03 22:14:00.067745799 +0200
>>>>> +++ rk3399-rockpro64.dtsÂÂÂ 2019-10-04 00:02:50.047892366 +0200
>>>>> @@ -619,6 +619,8 @@
>>>>> ÂÂÂÂÂ max-frequency = <150000000>;
>>>>> ÂÂÂÂÂ pinctrl-names = "default";
>>>>> ÂÂÂÂÂ pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_bus4>;
>>>>> +ÂÂÂ sd-uhs-sdr104;
>>>>> +ÂÂÂ vqmmc-supply = <&vcc_sdio>;
>>>>> ÂÂÂÂÂ status = "okay";
>>>>> ÂÂ};
>>>>> When I do so, the sd card is detected as SDR104, but a reboot hangs:
>>>>>
>>>>> Boot1: 2018-06-26, version: 1.14
>>>>> CPUId = 0x0
>>>>> ChipType = 0x10, 286
>>>>> Spi_ChipId = c84018
>>>>> no find rkpartition
>>>>> SpiBootInit:ffffffff
>>>>> mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
>>>>> mmc: ERROR: Card did not respond to voltage select!
>>>>> emmc reinit
>>>>> mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
>>>>> mmc: ERROR: Card did not respond to voltage select!
>>>>> emmc reinit
>>>>> mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
>>>>> mmc: ERROR: Card did not respond to voltage select!
>>>>> SdmmcInit=2 1
>>>>> mmc0:cmd5,32
>>>>> mmc0:cmd7,32
>>>>> mmc0:cmd5,32
>>>>> mmc0:cmd7,32
>>>>> mmc0:cmd5,32
>>>>> mmc0:cmd7,32
>>>>> SdmmcInit=0 1
>>>>>
>>>>> So I guess I should use a different miniloader for this reboot to
>>>>> work!?
>>>>> Or what else could be wrong here?
>>>> Hmm, I guess this is "the Tinkerboard problem" again - the patch
>>>> above would be OK if we could get as far as the kernel, but can't
>>>> help if the 
>>> I didn't realize that SD was used as boot medium for RockPro64, but I
>>> did patch the vendor tree to solve the issue for Tinkerboard, see
>>> https://github.com/rockchip-linux/kernel/commit/a4ccde21f5a9f04f996fb02479cb9f16d3dc8dc0
>>>
>>>
>>> My initial plan was to patching upstream kernel by adding ->shutdown,but
>>> never finish it.
>>>
>>>> offending card is itself the boot medium. There was a proposal here:
>>>>
>>>> https://patchwork.kernel.org/patch/10817217/
>>> This RFC also looks good to me, but seems it needs volunteers
>>> to push it again.
>> Oh, I think this is a totally wrong way.
>>
>> While this might work for some cards, setting the controller's i/o
>> voltage to 3.3V while leaving the card at 1.8V configuration is totally
>> against the specification, can lead to all kinds of strange behaviour
>> and even cause hardware damage. It also would actively defend the
>> purpose of the above mentioned patch (6a11fc4) where the kernel guesses
>> the i/o voltage from the card configuration and switches the controller
>> accordingly. We would end up with a 1.8V card and controller
>> configuration and a regulator voltage of 3.3V. This would only work with
>> good luck. Even if the kernel driver would switch the regulator back to
>> 1.8V in this case, the voltage mismatch remains in the bootloader when
>> this card contains the boot image.
>>
>> The only sane way I see to handle this is implementing the same
>> workaround (mode guessing) also in the bootloader (rockchip miniloader
>> and u-boot SPL since both bootloader chains are supported for this board).
>>
>> Or maybe I miss something?
> Thanks for your input, I have made a new series [1] with a similar approach but is limited to dw_mmc-rockchip
> and only changes the regulator at power_off after the regulator has been disabled (the vqmmc regulator in affected devices is always-on).
Thanks for your work on this. Unfortunately I still think that this is
the wrong approach.
I see several problems in your patch series:
- You introduced GPIO0_PA1 as regulator enable for RockPro64.
Unfortunately Pine64 decided to disable this regulator on Board Version
2.1 (real product version), see above. I have no idea why they did this.
- You changed the i/o voltage from 3.0V to 3.3V. This is not allowed on
RK3399 I/O bank F.

Changing the i/o voltage to 3.0V/3.3V while the sd card is configured
for 1.8V is against the specification and dangerous. While experimenting
with different images (ayufan, armbian) for my newly bought RockPro64 I
killed a SD card (32GB Samsung UHS-I). I cannot reconstruct the exact
circumstances, but I'm pretty sure this happened due to the voltage
mismatch. Of course I'm not keen to experiment further with this and
kill more sd cards. This is not just an theoretical issue.
> To my knowledge the problem is not with the rockchip miniloader or u-boot SPL, it is the initial boot rom that tries to load
> the miniloader/SPL from a SD-card, so nothing that can be updated.
What I observed on my RockPro64:
The ROM bootloader always was able to load the next stage, maybe due to
the low initial clock of 400kHz? Also the ROM bootloader cannot change
voltage regulator settings. So if the i/o voltage still is at 1.8V and
matching the sd card setting, there is no problem for the ROM bootloader.
So I think the normal reboot handling should be:
If the sd card can be switched off (preferred solution), do so and reset
the controller i/o voltage to 3.0V/3.3V.
If the sd card can not be switched off, make sure to leave the
controller i/o voltage at the current setting. Make sure in later boot
stages to not change the controller i/o voltage to 3.0V/3.3V when the
card is configured for 1.8V. According to the patch mentioned above this
behaviour already is implemented in linux, we also need this for the
bootloaders.

Regards,
Soeren
>
> I have observed this issue on the following devices, and the patches at [1] makes it possible to reboot from SD-card after UHS has been enabled.
> - Asus Tinker Board (RK3288)
> - Rockchip Sapphire Board (RK3399)
> - Radxa Rock Pi 4 (RK3399)
> - Pine64 RockPro64 (RK3399)
> All of the above seem to use the RK808 regulator for sd io voltage.
>
> The ROC-RK3328-CC did not have this issue and seem to automatically reset to 3.3v.
>
> [1] https://github.com/Kwiboo/linux-rockchip/compare/next-20191011...next-20191011-mmc
>
> Regards,
> Jonas
>
>> Soeren
>>
>>
>>>> although I'm not sure what if any progress has been made since then.
>>>>
>>>> Robin.
>>>>