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

Chukun Pan
2024-04-17T13:31:08Z

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?

>> + 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.

>> + 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