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
