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

From: Soeren Moch <smoch@web.de>
To: Robin Murphy <robin.murphy@arm.com>,
	Jonas Karlman <jonas@kwiboo.se>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	Heiko Stuebner <heiko@sntech.de>
Cc: "linux-rockchip@lists.infradead.org" 
	<linux-rockchip@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 3/3] arm64: dts: rockchip: fix RockPro64 sdmmc settings
Date: Fri, 11 Oct 2019 16:16:12 +0200	[thread overview]
Message-ID: <6c2e6523-dc0a-1ad6-ffd3-7ef63c6f7df9@web.de> (raw)
In-Reply-To: <64a7d056-28d0-b6d8-6148-b98b58265c08@arm.com>

On 11.10.19 15:00, Robin Murphy wrote:
> On 11/10/2019 12:40, Soeren Moch wrote:
>> 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.
>
> Hmm, that makes me wonder if the problem might be not so much that the
> level of SDMMC0_VDD itself stays at 1.8V, but that at some point after
> the bootrom the GRF_IO_VSEL bit gets reset so the controller just
> stops being able to read anything as logic-high.
Would be great if someone from Rockchip could give some insights whether
and when during reboot GRF_IO_VSEL is reset (ATF before reboot, some SoC
soft-reset, ROM bootloader, rkminiloader, something different), Shawn?
Or gets this VSEL changed only when switching the voltage regulator (via
io_domains,sdmmc-supply)?

Soeren
>
> Robin.
>
>> 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.
>>>>>>
>>
>>