by Jonas Karlman

[permalink] [raw]

Subject: Re: [PATCH 2/2] arm64: dts: rockchip: Add Radxa ROCK3 Model C

Hi Chukun,

Sorry for late reply, I will test and review your v2 later today :-)

On 2024-04-17 15:30, Chukun Pan wrote:
> Hi Jonas,
>>> + model = "Radxa ROCK3 Model C";
>>
>> The marketing name seems to be "Radxa ROCK 3C" according to the product
>> page at [1].
>>
>> [1] https://radxa.com/products/rock3/3c
>
> According to https://wiki.radxa.com/Rock3/3c , it should be called
> "Radxa ROCK 3 Model C". I copied rock3a here without paying attention.
>
>>> + compatible = "radxa,rock3c", "rockchip,rk3566";
>>
>> A personal preference would be to match the product name and the dtb
>> filename, e.g. "radxa,rock-3c".
>
> I thought so too, here is also copied from rock3a.
> I think rock3a needs fixing too?
>
>>> + led_user: led-0 {
>> This is called user_led2 in the schematic, in case we want the symbol
>> to match the schematic.
>
>>> + regulator-name = "vcc5v0_usb_host";
>> This regulator is named vcc5v0_usb30_host in schematic.
>
> Thanks, I will fix it.
>
>>> + #clock-cells = <1>;
>>> + clock-names = "mclk";
>>> + clocks = <&cru I2S1_MCLKOUT_TX>;
>>
>> I think clock-output-names may be missing? Something like:
>>
>> clock-output-names = "rk809-clkout1", "rk809-clkout2";
>
> Thanks, I'll add it in the next patch.
>
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pmic_int_l>, <&i2s1m0_mclk>;
>>> + rockchip,system-power-controller;
>
>> I think this prop is deprecated and system-power-controller
>> should be used instead.
>
>>> + regulator-state-mem {
>>> + regulator-on-in-suspend;
>>> + regulator-suspend-microvolt = <900000>;
>
>> Not sure we need this in suspend to ram and this can probably use:
>>
>> regulator-off-in-suspend
>>
>> That is also what vendor kernel does.
>
> Will be corrected in the next patch.
>
>>> + vcc3v3_sd: SWITCH_REG2 {
>>> + regulator-name = "vcc3v3_sd";
>>> + regulator-always-on;
>>> + regulator-boot-on;
>>
>> If I am reading the schematic correctly this is not connected.
>
> Yes, I didn't notice it was NC, thanks.
>
>>> +&sdhci {
>>> + bus-width = <8>;
>>> + max-frequency = <200000000>;
>> This board support HS200, please add:
>>
>> mmc-hs200-1_8v
>
> Will be added in the next patch.
>
>>> +&sdmmc0 {
>>> + bus-width = <4>;
>>> + cap-sd-highspeed;
>>> + cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
>>
>> Please drop the cd-gpios prop, sdmmc0_det is used below and
>> works fine on this board.
>
> Do other rk356x boards also need this change?

Yes, I think in general on Rockchip platform it is preferred to
configure the pin for sd-card detect function instead of using the
cd-gpios prop (pin configured to use gpio function) when the default
pin is used and correctly wired to sd-slot.

The RK3566 HW design guide also mention following:

"""
RK3566 reuses JTAG function and SDMMC function together to reduce IO pin
count and take into account the convenience of the complete machine
debugging. The SDMMC0_DET pin is used to switch the output function.
Therefore, this pin must be configured before power-on. Otherwise, the
debugging during the boot period will be affected if JTAG has no output,
and SDMMC0 has no output will affects SDMMC boot.
- SDMMC0_DET pin detects that the level is high, the IO switches to the
JTAG function;
- SDMMC0_DET detects that the level is low (normal state of SD card
insertion, and the PIN is pulled down by SD card slot), the IO
function is switched to SDMMC;
"""

And similar was changed on RK3588 boards in:

arm64: dts: rockchip: rk3588: remove redundant cd-gpios in sdmmc node
https://lore.kernel.org/linux-rockchip/[email protected]/

>
>>> + sd-uhs-sdr50;
>>
>> Why limit to sdr50? and not use sd-uhs-sdr104?
>
> The sdr104 mode is not stable on the rk356x platform.
> This problem has been reported on both rock3a and e25 boards.

Do you have any references for these issues?

When I tested on Radxa ZERO 3W/3E there was an issue executing tuning
that resulted in a not working sdr104 mode. However, after the card is
removed and re-inserted the tuning works and the card can use sdr104
mode.

That issue seems to be related to the io-domain driver not being notified
about the voltage change when mmc driver is probed during boot. When the
card is removed and re-inserted the io-domain driver gets notified and
re-configure the io-domain.

Testing on a RK3328 board (Rock64) the io-domain driver gets notified
about the voltage change during boot and the tuning is successful for
sdr104 mode.

Will re-run tests on my ROCK 3C boards to validate is that is the same
issue, guessing it may affect multiple RK356x boards.

Regards,
Jonas

>
>>> + vmmc-supply = <&vcc3v3_sd>;
>>
>> If I read the scematics correctly this is using the
>> vcc3v3_sys regulator and not the vcc3v3_sd.
>
> Yes, you are right. I didn't notice it, thanks.
>
>>> +&sfc {
>>
>> This is missing pinctrl:
>>
>> pinctrl-names = "default";
>> pinctrl-0 = <&fspi_pins>;
>
> This is already defined on rk356x.dtsi:
>
> sfc: spi@fe300000 {
> compatible = "rockchip,sfc";
> ......
> pinctrl-0 = <&fspi_pins>;
> pinctrl-names = "default";
> status = "disabled";
> };
>
>>> + spi-max-frequency = <104000000>;
>>
>> My board is using a GD25LQ128EWIGR same as mentioned in the schematic,
>> and datasheet for this flash chip menion 120 mhz and not 104 mhz.
>
> Will be corrected in the next patch, thanks.
>
> Thanks,
> Chukun