| Message ID | 20240416130010.379705-1-amadeus@jmu.edu.cn |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/2] dt-bindings: arm: rockchip: add Radxa ROCK3 Model C | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
Hi Chukun, On 2024-04-16 15:00, Chukun Pan wrote: > The Radxa ROCK3 Model C is a development board with the > Rockchip RK3566 SoC. It has the following features: > > - 1/2/4GB LPDDR4 > - 1x HDMI Type A > - 1x PCIE 2.0 slot > - 1x FAN connector > - 3.5mm jack with mic > - 1GbE RTL8211F Ethernet > - 1x USB 3.0, 3x USB 2.0 > - 40-pin expansion header > - MicroSD card/eMMC socket > - 16MB SPI NOR (gd25lq128d) Great, I was about to send out a similar device tree patch for this board, below you find a few remarks. > > Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn> > --- > arch/arm64/boot/dts/rockchip/Makefile | 1 + > .../boot/dts/rockchip/rk3566-rock-3c.dts | 751 ++++++++++++++++++ > 2 files changed, 752 insertions(+) > create mode 100644 arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts > > diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile > index 0192980ef37f..53efff815bf3 100644 > --- a/arch/arm64/boot/dts/rockchip/Makefile > +++ b/arch/arm64/boot/dts/rockchip/Makefile > @@ -90,6 +90,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-a.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-quartz64-b.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-radxa-cm3-io.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-roc-pc.dtb > +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-rock-3c.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-soquartz-blade.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-soquartz-cm4.dtb > dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3566-soquartz-model-a.dtb > diff --git a/arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts b/arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts > new file mode 100644 > index 000000000000..96d24069aba2 > --- /dev/null > +++ b/arch/arm64/boot/dts/rockchip/rk3566-rock-3c.dts > @@ -0,0 +1,751 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > + > +/dts-v1/; > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/leds/common.h> > +#include <dt-bindings/pinctrl/rockchip.h> > +#include <dt-bindings/soc/rockchip,vop2.h> > +#include "rk3566.dtsi" > + > +/ { > + 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 > + compatible = "radxa,rock3c", "rockchip,rk3566"; A personal preference would be to match the product name and the dtb filename, e.g. "radxa,rock-3c". > + > + aliases { > + ethernet0 = &gmac1; > + mmc0 = &sdhci; > + mmc1 = &sdmmc0; > + mmc2 = &sdmmc1; > + }; > + > + chosen: chosen { > + stdout-path = "serial2:1500000n8"; > + }; > + > + hdmi-con { > + compatible = "hdmi-connector"; > + type = "a"; > + > + port { > + hdmi_con_in: endpoint { > + remote-endpoint = <&hdmi_out_con>; > + }; > + }; > + }; > + > + gmac1_clkin: external-gmac1-clock { > + compatible = "fixed-clock"; > + clock-frequency = <125000000>; > + clock-output-names = "gmac1_clkin"; > + #clock-cells = <0>; > + }; > + > + leds { > + compatible = "gpio-leds"; > + > + led_user: led-0 { This is called user_led2 in the schematic, in case we want the symbol to match the schematic. > + gpios = <&gpio0 RK_PA0 GPIO_ACTIVE_HIGH>; > + function = LED_FUNCTION_HEARTBEAT; > + color = <LED_COLOR_ID_BLUE>; > + linux,default-trigger = "heartbeat"; > + pinctrl-names = "default"; > + pinctrl-0 = <&led_user_en>; > + }; > + }; > + > + rk809-sound { > + compatible = "simple-audio-card"; > + simple-audio-card,format = "i2s"; > + simple-audio-card,name = "Analog RK809"; > + simple-audio-card,mclk-fs = <256>; > + > + simple-audio-card,cpu { > + sound-dai = <&i2s1_8ch>; > + }; > + > + simple-audio-card,codec { > + sound-dai = <&rk809>; > + }; > + }; > + > + sdio_pwrseq: sdio-pwrseq { > + compatible = "mmc-pwrseq-simple"; > + clocks = <&rk809 1>; > + clock-names = "ext_clock"; > + pinctrl-names = "default"; > + pinctrl-0 = <&wifi_reg_on_h>; > + post-power-on-delay-ms = <100>; > + power-off-delay-us = <5000000>; > + reset-gpios = <&gpio0 RK_PC0 GPIO_ACTIVE_LOW>; > + }; > + > + vcc5v_dcin: vcc5v-dcin-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "vcc5v_dcin"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + }; > + > + vcc3v3_pcie: vcc3v3-pcie-regulator { > + compatible = "regulator-fixed"; > + enable-active-high; > + gpios = <&gpio0 RK_PA6 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pcie_pwr_en>; > + regulator-name = "vcc3v3_pcie"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + vin-supply = <&vcc3v3_sys>; > + }; > + > + vcc3v3_sys: vcc3v3-sys-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "vcc3v3_sys"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + vin-supply = <&vcc5v0_sys>; > + }; > + > + vcc5v0_sys: vcc5v0-sys-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "vcc5v0_sys"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + vin-supply = <&vcc5v_dcin>; > + }; > + > + vcc5v0_usb_host: vcc5v0-usb-host-regulator { > + compatible = "regulator-fixed"; > + enable-active-high; > + gpio = <&gpio0 RK_PC5 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vcc5v0_usb_host_en>; > + regulator-name = "vcc5v0_usb_host"; This regulator is named vcc5v0_usb30_host in schematic. > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + vin-supply = <&vcc5v0_sys>; > + }; > + > + vcc5v0_usb_otg: vcc5v0-usb-otg-regulator { > + compatible = "regulator-fixed"; > + enable-active-high; > + gpio = <&gpio0 RK_PC6 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vcc5v0_usb_otg_en>; > + regulator-name = "vcc5v0_usb_otg"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + vin-supply = <&vcc5v0_sys>; > + }; > + > + vcc_cam: vcc-cam-regulator { > + compatible = "regulator-fixed"; > + enable-active-high; > + gpio = <&gpio0 RK_PC4 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vcc_cam_en>; > + regulator-name = "vcc_cam"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + vin-supply = <&vcc3v3_sys>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vcc_mipi: vcc-mipi-regulator { > + compatible = "regulator-fixed"; > + enable-active-high; > + gpio = <&gpio0 RK_PC7 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default"; > + pinctrl-0 = <&vcc_mipi_en>; > + regulator-name = "vcc_mipi"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + vin-supply = <&vcc3v3_sys>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > +}; > + > +&combphy1 { > + status = "okay"; > +}; > + > +&combphy2 { > + status = "okay"; > +}; > + > +&cpu0 { > + cpu-supply = <&vdd_cpu>; > +}; > + > +&cpu1 { > + cpu-supply = <&vdd_cpu>; > +}; > + > +&cpu2 { > + cpu-supply = <&vdd_cpu>; > +}; > + > +&cpu3 { > + cpu-supply = <&vdd_cpu>; > +}; > + > +&gmac1 { > + assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>; > + assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&gmac1_clkin>; > + clock_in_out = "input"; > + phy-handle = <&rgmii_phy1>; > + phy-mode = "rgmii-id"; > + phy-supply = <&vcc_3v3>; > + pinctrl-names = "default"; > + pinctrl-0 = <&gmac1m1_miim > + &gmac1m1_tx_bus2 > + &gmac1m1_rx_bus2 > + &gmac1m1_rgmii_clk > + &gmac1m1_clkinout > + &gmac1m1_rgmii_bus>; > + status = "okay"; > +}; > + > +&gpu { > + mali-supply = <&vdd_gpu>; > + status = "okay"; > +}; > + > +&hdmi { > + avdd-0v9-supply = <&vdda0v9_image>; > + avdd-1v8-supply = <&vcca1v8_image>; > + status = "okay"; > +}; > + > +&hdmi_in { > + hdmi_in_vp0: endpoint { > + remote-endpoint = <&vp0_out_hdmi>; > + }; > +}; > + > +&hdmi_out { > + hdmi_out_con: endpoint { > + remote-endpoint = <&hdmi_con_in>; > + }; > +}; > + > +&hdmi_sound { > + status = "okay"; > +}; > + > +&i2c0 { > + status = "okay"; > + > + vdd_cpu: regulator@1c { > + compatible = "tcs,tcs4525"; > + reg = <0x1c>; > + fcs,suspend-voltage-selector = <1>; > + regulator-name = "vdd_cpu"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <800000>; > + regulator-max-microvolt = <1150000>; > + regulator-ramp-delay = <2300>; > + vin-supply = <&vcc5v0_sys>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + rk809: pmic@20 { > + compatible = "rockchip,rk809"; > + reg = <0x20>; > + interrupt-parent = <&gpio0>; > + interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>; > + assigned-clocks = <&cru I2S1_MCLKOUT_TX>; > + assigned-clock-parents = <&cru CLK_I2S1_8CH_TX>; > + #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"; > + 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. > + #sound-dai-cells = <0>; > + vcc1-supply = <&vcc3v3_sys>; > + vcc2-supply = <&vcc3v3_sys>; > + vcc3-supply = <&vcc3v3_sys>; > + vcc4-supply = <&vcc3v3_sys>; > + vcc5-supply = <&vcc3v3_sys>; > + vcc6-supply = <&vcc3v3_sys>; > + vcc7-supply = <&vcc3v3_sys>; > + vcc8-supply = <&vcc3v3_sys>; > + vcc9-supply = <&vcc3v3_sys>; > + wakeup-source; > + > + regulators { > + vdd_logic: DCDC_REG1 { > + regulator-name = "vdd_logic"; > + regulator-always-on; > + regulator-boot-on; > + regulator-initial-mode = <0x2>; > + regulator-min-microvolt = <500000>; > + regulator-max-microvolt = <1350000>; > + regulator-ramp-delay = <6001>; > + > + 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. > + }; > + }; > + > + vdd_gpu: DCDC_REG2 { > + regulator-name = "vdd_gpu"; > + regulator-always-on; > + regulator-boot-on; > + regulator-initial-mode = <0x2>; > + regulator-min-microvolt = <500000>; > + regulator-max-microvolt = <1350000>; > + regulator-ramp-delay = <6001>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + regulator-suspend-microvolt = <900000>; > + }; > + }; > + > + vcc_ddr: DCDC_REG3 { > + regulator-name = "vcc_ddr"; > + regulator-always-on; > + regulator-boot-on; > + regulator-initial-mode = <0x2>; > + > + regulator-state-mem { > + regulator-on-in-suspend; > + }; > + }; > + > + vdd_npu: DCDC_REG4 { > + regulator-name = "vdd_npu"; > + regulator-initial-mode = <0x2>; > + regulator-min-microvolt = <500000>; > + regulator-max-microvolt = <1350000>; > + regulator-ramp-delay = <6001>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vcc_1v8: DCDC_REG5 { > + regulator-name = "vcc_1v8"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vdda0v9_image: LDO_REG1 { > + regulator-name = "vdda0v9_image"; > + regulator-min-microvolt = <900000>; > + regulator-max-microvolt = <900000>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vdda_0v9: LDO_REG2 { > + regulator-name = "vdda_0v9"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <900000>; > + regulator-max-microvolt = <900000>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vdda0v9_pmu: LDO_REG3 { > + regulator-name = "vdda0v9_pmu"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <900000>; > + regulator-max-microvolt = <900000>; > + > + regulator-state-mem { > + regulator-on-in-suspend; > + regulator-suspend-microvolt = <900000>; > + }; > + }; > + > + vccio_acodec: LDO_REG4 { > + regulator-name = "vccio_acodec"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vccio_sd: LDO_REG5 { > + regulator-name = "vccio_sd"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <3300000>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vcc3v3_pmu: LDO_REG6 { > + regulator-name = "vcc3v3_pmu"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + > + regulator-state-mem { > + regulator-on-in-suspend; > + regulator-suspend-microvolt = <3300000>; > + }; > + }; > + > + vcca_1v8: LDO_REG7 { > + regulator-name = "vcca_1v8"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vcca1v8_pmu: LDO_REG8 { > + regulator-name = "vcca1v8_pmu"; > + regulator-always-on; > + regulator-boot-on; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + > + regulator-state-mem { > + regulator-on-in-suspend; > + regulator-suspend-microvolt = <1800000>; > + }; > + }; > + > + vcca1v8_image: LDO_REG9 { > + regulator-name = "vcca1v8_image"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + vcc_3v3: SWITCH_REG1 { > + regulator-name = "vcc_3v3"; > + regulator-always-on; > + regulator-boot-on; > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + > + 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. > + > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > + }; > + }; > + > + codec { > + mic-in-differential; > + }; > + }; > + > + eeprom: eeprom@50 { > + compatible = "belling,bl24c16a", "atmel,24c16"; > + reg = <0x50>; > + pagesize = <16>; > + }; > +}; > + > +&i2s0_8ch { > + status = "okay"; > +}; > + > +&i2s1_8ch { > + pinctrl-names = "default"; > + pinctrl-0 = <&i2s1m0_sclktx &i2s1m0_lrcktx &i2s1m0_sdi0 &i2s1m0_sdo0>; > + rockchip,trcm-sync-tx-only; > + status = "okay"; > +}; > + > +&mdio1 { > + rgmii_phy1: ethernet-phy@1 { > + compatible = "ethernet-phy-ieee802.3-c22"; > + reg = <0x1>; > + reset-assert-us = <20000>; > + reset-deassert-us = <100000>; > + reset-gpios = <&gpio3 RK_PC0 GPIO_ACTIVE_LOW>; > + }; > +}; > + > +&pcie2x1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pcie_reset_h>; > + reset-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>; > + vpcie3v3-supply = <&vcc3v3_pcie>; > + status = "okay"; > +}; > + > +&pinctrl { > + bluetooth { > + bt_reg_on_h: bt-reg-on-h { > + rockchip,pins = <0 RK_PC1 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + bt_wake_host_h: bt-wake-host-h { > + rockchip,pins = <0 RK_PB3 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + bt_host_wake_h: bt-host-wake-h { > + rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + }; > + > + cam { > + vcc_cam_en: vcc_cam_en { > + rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + }; > + > + display { > + vcc_mipi_en: vcc_mipi_en { > + rockchip,pins = <0 RK_PC7 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + }; > + > + leds { > + led_user_en: led_user_en { > + rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + }; > + > + pcie { > + pcie_pwr_en: pcie-pwr-en { > + rockchip,pins = <0 RK_PA6 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + pcie_reset_h: pcie-reset-h { > + rockchip,pins = <1 RK_PB2 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + }; > + > + pmic { > + pmic_int_l: pmic-int-l { > + rockchip,pins = <0 RK_PA3 RK_FUNC_GPIO &pcfg_pull_up>; This should probably use a space instead of a tab after =. > + }; > + }; > + > + usb { > + vcc5v0_usb_host_en: vcc5v0-usb-host-en { > + rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + vcc5v0_usb_otg_en: vcc5v0-usb-otg-en { > + rockchip,pins = <0 RK_PC6 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + }; > + > + wifi { > + wifi_host_wake_h: wifi-host-wake-h { > + rockchip,pins = <0 RK_PB7 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + > + wifi_reg_on_h: wifi-reg-on-h { > + rockchip,pins = <0 RK_PC0 RK_FUNC_GPIO &pcfg_pull_none>; > + }; > + }; > +}; > + > +&pmu_io_domains { > + pmuio1-supply = <&vcc3v3_pmu>; > + pmuio2-supply = <&vcca1v8_pmu>; > + vccio1-supply = <&vccio_acodec>; > + vccio2-supply = <&vcc_1v8>; > + vccio3-supply = <&vccio_sd>; > + vccio4-supply = <&vcca1v8_pmu>; > + vccio5-supply = <&vcc_3v3>; > + vccio6-supply = <&vcc_3v3>; > + vccio7-supply = <&vcc_3v3>; > + status = "okay"; > +}; > + > +&saradc { > + vref-supply = <&vcca_1v8>; > + status = "okay"; > +}; > + > +&sdhci { > + bus-width = <8>; > + max-frequency = <200000000>; This board support HS200, please add: mmc-hs200-1_8v > + non-removable; > + pinctrl-names = "default"; > + pinctrl-0 = <&emmc_bus8 &emmc_clk &emmc_cmd &emmc_datastrobe>; > + vmmc-supply = <&vcc_3v3>; > + vqmmc-supply = <&vcc_1v8>; > + status = "okay"; > +}; > + > +&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. > + disable-wp; > + pinctrl-names = "default"; > + pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>; > + sd-uhs-sdr50; Why limit to sdr50? and not use sd-uhs-sdr104? > + vmmc-supply = <&vcc3v3_sd>; If I read the scematics correctly this is using the vcc3v3_sys regulator and not the vcc3v3_sd. R90724 is not connected however R90723 is and this also matches visual inspection of the board. > + vqmmc-supply = <&vccio_sd>; > + status = "okay"; > +}; > + > +&sdmmc1 { > + bus-width = <4>; > + cap-sd-highspeed; > + cap-sdio-irq; > + keep-power-in-suspend; > + mmc-pwrseq = <&sdio_pwrseq>; > + non-removable; > + pinctrl-names = "default"; > + pinctrl-0 = <&sdmmc1_bus4 &sdmmc1_clk &sdmmc1_cmd>; > + sd-uhs-sdr104; > + vmmc-supply = <&vcc3v3_sys>; > + vqmmc-supply = <&vcca1v8_pmu>; > + status = "okay"; > +}; > + > +&sfc { This is missing pinctrl: pinctrl-names = "default"; pinctrl-0 = <&fspi_pins>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "okay"; > + > + flash@0 { > + compatible = "jedec,spi-nor"; > + reg = <0x0>; > + 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. Regards, Jonas > + spi-rx-bus-width = <4>; > + spi-tx-bus-width = <1>; > + }; > +}; > + > +&tsadc { > + rockchip,hw-tshut-mode = <1>; > + rockchip,hw-tshut-polarity = <0>; > + status = "okay"; > +}; > + > +&uart1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&uart1m0_ctsn &uart1m0_rtsn &uart1m0_xfer>; > + status = "okay"; > +}; > + > +&uart2 { > + status = "okay"; > +}; > + > +&usb_host0_ehci { > + status = "okay"; > +}; > + > +&usb_host0_ohci { > + status = "okay"; > +}; > + > +&usb_host0_xhci { > + dr_mode = "host"; > + status = "okay"; > +}; > + > +&usb_host1_ehci { > + status = "okay"; > +}; > + > +&usb_host1_ohci { > + status = "okay"; > +}; > + > +&usb_host1_xhci { > + status = "okay"; > +}; > + > +&usb2phy0 { > + status = "okay"; > +}; > + > +&usb2phy0_host { > + phy-supply = <&vcc5v0_usb_host>; > + status = "okay"; > +}; > + > +&usb2phy0_otg { > + phy-supply = <&vcc5v0_usb_otg>; > + status = "okay"; > +}; > + > +&usb2phy1 { > + status = "okay"; > +}; > + > +&usb2phy1_host { > + phy-supply = <&vcc5v0_usb_host>; > + status = "okay"; > +}; > + > +&usb2phy1_otg { > + phy-supply = <&vcc5v0_usb_host>; > + status = "okay"; > +}; > + > +&vop { > + assigned-clocks = <&cru DCLK_VOP0>, <&cru DCLK_VOP1>; > + assigned-clock-parents = <&pmucru PLL_HPLL>, <&cru PLL_VPLL>; > + status = "okay"; > +}; > + > +&vop_mmu { > + status = "okay"; > +}; > + > +&vp0 { > + vp0_out_hdmi: endpoint@ROCKCHIP_VOP2_EP_HDMI0 { > + reg = <ROCKCHIP_VOP2_EP_HDMI0>; > + remote-endpoint = <&hdmi_in_vp0>; > + }; > +};
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
On 16/04/2024 15:00, Chukun Pan wrote: > The Radxa ROCK3 Model C is a similar board to > the Radxa ROCK3 Model A with the RK3566 SoC. > Add devicetree binding documentation for it. > > Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn> > --- Acked-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
Hello Chukun, 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. If I may interject, here's the result of a quick qrep: ./rk3399pro-rock-pi-n10.dts: model = "Radxa ROCK Pi N10"; ./rk3399-rock-pi-4a-plus.dts: model = "Radxa ROCK Pi 4A+"; ./rk3588s-rock-5a.dts: model = "Radxa ROCK 5 Model A"; ./rk3588-rock-5b.dts: model = "Radxa ROCK 5 Model B"; ./rk3399-rock-4c-plus.dts: model = "Radxa ROCK 4C+"; ./rk3399-rock-pi-4b-plus.dts: model = "Radxa ROCK Pi 4B+"; ./rk3399-rock-pi-4b.dts: model = "Radxa ROCK Pi 4B"; ./rk3399-rock-pi-4c.dts: model = "Radxa ROCK Pi 4C"; ./rk3308-rock-pi-s.dts: model = "Radxa ROCK Pi S"; ./rk3399-rock-pi-4a.dts: model = "Radxa ROCK Pi 4A"; ./rk3399-rock-4se.dts: model = "Radxa ROCK 4SE"; ./rk3328-rock-pi-e.dts: model = "Radxa ROCK Pi E"; ./rk3568-rock-3a.dts: model = "Radxa ROCK3 Model A"; Based on that, I think that "Radxa ROCK 3 Model C" would actually be the preferred name... Perhaps? If we end up following that approach, the last board dts on the list above should also be fixed to read "Radxa ROCK 3 Model A". Either that, or all "Model " strings should be stripped out from all board dts files that currently contain it.
Hi, this is Naoki @ Radxa. thank you for your great work! On 4/18/24 02:02, Dragan Simic wrote: > Hello Chukun, > > 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. sorry, wiki.radxa.com is outdated. docs.radxa.com is correct one and maintained. (but it still has some errors, sorry) > If I may interject, here's the result of a quick qrep: > > ./rk3399pro-rock-pi-n10.dts: model = "Radxa ROCK Pi N10"; > ./rk3399-rock-pi-4a-plus.dts: model = "Radxa ROCK Pi 4A+"; > ./rk3588s-rock-5a.dts: model = "Radxa ROCK 5 Model A"; > ./rk3588-rock-5b.dts: model = "Radxa ROCK 5 Model B"; > ./rk3399-rock-4c-plus.dts: model = "Radxa ROCK 4C+"; > ./rk3399-rock-pi-4b-plus.dts: model = "Radxa ROCK Pi 4B+"; > ./rk3399-rock-pi-4b.dts: model = "Radxa ROCK Pi 4B"; > ./rk3399-rock-pi-4c.dts: model = "Radxa ROCK Pi 4C"; > ./rk3308-rock-pi-s.dts: model = "Radxa ROCK Pi S"; > ./rk3399-rock-pi-4a.dts: model = "Radxa ROCK Pi 4A"; > ./rk3399-rock-4se.dts: model = "Radxa ROCK 4SE"; > ./rk3328-rock-pi-e.dts: model = "Radxa ROCK Pi E"; > ./rk3568-rock-3a.dts: model = "Radxa ROCK3 Model A"; > > Based on that, I think that "Radxa ROCK 3 Model C" would actually > be the preferred name... Perhaps? > > If we end up following that approach, the last board dts on the list > above should also be fixed to read "Radxa ROCK 3 Model A". > > Either that, or all "Model " strings should be stripped out from > all board dts files that currently contain it. we have a document named "Radxa Product Naming Convention". there are "full name" and "short name". "Model" is used in "full name", but it's not used in "short name". both are correct. but, we preferred to use "short name" for "model = " in mainline linux/u-boot. for ROCK 3C, please use model = "Radxa ROCK 3C"; for ROCK 5A/5B and ROCK 3A, model = "Radxa ROCK 5A"; model = "Radxa ROCK 5B"; model = "Radxa ROCK 3A"; are preferred. we need to fix this inconsistency. (include our docs) Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd.
Hello Naoki, On 2024-04-18 07:52, FUKAUMI Naoki wrote: > this is Naoki @ Radxa. > > thank you for your great work! > > On 4/18/24 02:02, Dragan Simic wrote: >> Hello Chukun, >> >> 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. > > sorry, wiki.radxa.com is outdated. > > docs.radxa.com is correct one and maintained. > (but it still has some errors, sorry) > >> If I may interject, here's the result of a quick qrep: >> >> ./rk3399pro-rock-pi-n10.dts: model = "Radxa ROCK Pi N10"; >> ./rk3399-rock-pi-4a-plus.dts: model = "Radxa ROCK Pi 4A+"; >> ./rk3588s-rock-5a.dts: model = "Radxa ROCK 5 Model A"; >> ./rk3588-rock-5b.dts: model = "Radxa ROCK 5 Model B"; >> ./rk3399-rock-4c-plus.dts: model = "Radxa ROCK 4C+"; >> ./rk3399-rock-pi-4b-plus.dts: model = "Radxa ROCK Pi 4B+"; >> ./rk3399-rock-pi-4b.dts: model = "Radxa ROCK Pi 4B"; >> ./rk3399-rock-pi-4c.dts: model = "Radxa ROCK Pi 4C"; >> ./rk3308-rock-pi-s.dts: model = "Radxa ROCK Pi S"; >> ./rk3399-rock-pi-4a.dts: model = "Radxa ROCK Pi 4A"; >> ./rk3399-rock-4se.dts: model = "Radxa ROCK 4SE"; >> ./rk3328-rock-pi-e.dts: model = "Radxa ROCK Pi E"; >> ./rk3568-rock-3a.dts: model = "Radxa ROCK3 Model A"; >> >> Based on that, I think that "Radxa ROCK 3 Model C" would actually >> be the preferred name... Perhaps? >> >> If we end up following that approach, the last board dts on the list >> above should also be fixed to read "Radxa ROCK 3 Model A". >> >> Either that, or all "Model " strings should be stripped out from >> all board dts files that currently contain it. > > we have a document named "Radxa Product Naming Convention". > there are "full name" and "short name". "Model" is used in "full > name", but it's not used in "short name". both are correct. > > but, we preferred to use "short name" for "model = " in mainline > linux/u-boot. > > for ROCK 3C, please use > > model = "Radxa ROCK 3C"; > > for ROCK 5A/5B and ROCK 3A, > > model = "Radxa ROCK 5A"; > model = "Radxa ROCK 5B"; > model = "Radxa ROCK 3A"; > > are preferred. > > we need to fix this inconsistency. (include our docs) Great, thanks for this detailed clarification!
Hi, On 4/16/24 22:00, Chukun Pan wrote: > The Radxa ROCK3 Model C is a similar board to > the Radxa ROCK3 Model A with the RK3566 SoC. > Add devicetree binding documentation for it. > > Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn> > --- > Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml > index 99bd5e2c76a0..dc3847bd67fe 100644 > --- a/Documentation/devicetree/bindings/arm/rockchip.yaml > +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml > @@ -777,6 +777,11 @@ properties: > - const: radxa,rock3a > - const: rockchip,rk3568 > > + - description: Radxa ROCK3 Model C please use "Radxa ROCK 3C" here too. > + items: > + - const: radxa,rock3c and "rock-3c" here. > + - const: rockchip,rk3566 > + > - description: Radxa ROCK 5 Model A > items: > - const: radxa,rock-5a Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd.
Hello all, On 2024-04-18 07:55, Dragan Simic wrote: > On 2024-04-18 07:52, FUKAUMI Naoki wrote: >> this is Naoki @ Radxa. >> >> thank you for your great work! >> >> On 4/18/24 02:02, Dragan Simic wrote: >>> Hello Chukun, >>> >>> 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. >> >> sorry, wiki.radxa.com is outdated. >> >> docs.radxa.com is correct one and maintained. >> (but it still has some errors, sorry) >> >>> If I may interject, here's the result of a quick qrep: >>> >>> ./rk3399pro-rock-pi-n10.dts: model = "Radxa ROCK Pi N10"; >>> ./rk3399-rock-pi-4a-plus.dts: model = "Radxa ROCK Pi 4A+"; >>> ./rk3588s-rock-5a.dts: model = "Radxa ROCK 5 Model A"; >>> ./rk3588-rock-5b.dts: model = "Radxa ROCK 5 Model B"; >>> ./rk3399-rock-4c-plus.dts: model = "Radxa ROCK 4C+"; >>> ./rk3399-rock-pi-4b-plus.dts: model = "Radxa ROCK Pi 4B+"; >>> ./rk3399-rock-pi-4b.dts: model = "Radxa ROCK Pi 4B"; >>> ./rk3399-rock-pi-4c.dts: model = "Radxa ROCK Pi 4C"; >>> ./rk3308-rock-pi-s.dts: model = "Radxa ROCK Pi S"; >>> ./rk3399-rock-pi-4a.dts: model = "Radxa ROCK Pi 4A"; >>> ./rk3399-rock-4se.dts: model = "Radxa ROCK 4SE"; >>> ./rk3328-rock-pi-e.dts: model = "Radxa ROCK Pi E"; >>> ./rk3568-rock-3a.dts: model = "Radxa ROCK3 Model A"; >>> >>> Based on that, I think that "Radxa ROCK 3 Model C" would actually >>> be the preferred name... Perhaps? >>> >>> If we end up following that approach, the last board dts on the list >>> above should also be fixed to read "Radxa ROCK 3 Model A". >>> >>> Either that, or all "Model " strings should be stripped out from >>> all board dts files that currently contain it. >> >> we have a document named "Radxa Product Naming Convention". >> there are "full name" and "short name". "Model" is used in "full >> name", but it's not used in "short name". both are correct. >> >> but, we preferred to use "short name" for "model = " in mainline >> linux/u-boot. >> >> for ROCK 3C, please use >> >> model = "Radxa ROCK 3C"; >> >> for ROCK 5A/5B and ROCK 3A, >> >> model = "Radxa ROCK 5A"; >> model = "Radxa ROCK 5B"; >> model = "Radxa ROCK 3A"; >> >> are preferred. >> >> we need to fix this inconsistency. (include our docs) > > Great, thanks for this detailed clarification! I went ahead and prepared a patch that cleans up currently present naming inconsistencies for the Radxa boards. AFAICT, there should be no issues arising from the changes in the "model" and "compatible" strings, because they aren't used anywhere in the drivers. There are no board-specific ALSA UCM2 profiles, which could be affected by those changes. U-Boot is also fine. I'll think a bit more about it, and I'll send the patch over a bit later. If it goes well, I'll prepare and send one more similar patch, for some other boards.
On 2024-04-18 08:33, Dragan Simic wrote: > On 2024-04-18 07:55, Dragan Simic wrote: >> On 2024-04-18 07:52, FUKAUMI Naoki wrote: >>> this is Naoki @ Radxa. >>> >>> thank you for your great work! >>> >>> On 4/18/24 02:02, Dragan Simic wrote: >>>> Hello Chukun, >>>> >>>> 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. >>> >>> sorry, wiki.radxa.com is outdated. >>> >>> docs.radxa.com is correct one and maintained. >>> (but it still has some errors, sorry) >>> >>>> If I may interject, here's the result of a quick qrep: >>>> >>>> ./rk3399pro-rock-pi-n10.dts: model = "Radxa ROCK Pi N10"; >>>> ./rk3399-rock-pi-4a-plus.dts: model = "Radxa ROCK Pi 4A+"; >>>> ./rk3588s-rock-5a.dts: model = "Radxa ROCK 5 Model A"; >>>> ./rk3588-rock-5b.dts: model = "Radxa ROCK 5 Model B"; >>>> ./rk3399-rock-4c-plus.dts: model = "Radxa ROCK 4C+"; >>>> ./rk3399-rock-pi-4b-plus.dts: model = "Radxa ROCK Pi 4B+"; >>>> ./rk3399-rock-pi-4b.dts: model = "Radxa ROCK Pi 4B"; >>>> ./rk3399-rock-pi-4c.dts: model = "Radxa ROCK Pi 4C"; >>>> ./rk3308-rock-pi-s.dts: model = "Radxa ROCK Pi S"; >>>> ./rk3399-rock-pi-4a.dts: model = "Radxa ROCK Pi 4A"; >>>> ./rk3399-rock-4se.dts: model = "Radxa ROCK 4SE"; >>>> ./rk3328-rock-pi-e.dts: model = "Radxa ROCK Pi E"; >>>> ./rk3568-rock-3a.dts: model = "Radxa ROCK3 Model A"; >>>> >>>> Based on that, I think that "Radxa ROCK 3 Model C" would actually >>>> be the preferred name... Perhaps? >>>> >>>> If we end up following that approach, the last board dts on the list >>>> above should also be fixed to read "Radxa ROCK 3 Model A". >>>> >>>> Either that, or all "Model " strings should be stripped out from >>>> all board dts files that currently contain it. >>> >>> we have a document named "Radxa Product Naming Convention". >>> there are "full name" and "short name". "Model" is used in "full >>> name", but it's not used in "short name". both are correct. >>> >>> but, we preferred to use "short name" for "model = " in mainline >>> linux/u-boot. >>> >>> for ROCK 3C, please use >>> >>> model = "Radxa ROCK 3C"; >>> >>> for ROCK 5A/5B and ROCK 3A, >>> >>> model = "Radxa ROCK 5A"; >>> model = "Radxa ROCK 5B"; >>> model = "Radxa ROCK 3A"; >>> >>> are preferred. >>> >>> we need to fix this inconsistency. (include our docs) >> >> Great, thanks for this detailed clarification! > > I went ahead and prepared a patch that cleans up currently present > naming inconsistencies for the Radxa boards. AFAICT, there should > be no issues arising from the changes in the "model" and "compatible" > strings, because they aren't used anywhere in the drivers. There > are no board-specific ALSA UCM2 profiles, which could be affected > by those changes. U-Boot is also fine. > > I'll think a bit more about it, and I'll send the patch over a bit > later. If it goes well, I'll prepare and send one more similar patch, > for some other boards. Actually, a couple of board dts(i) files, listed below, need to be renamed to add dashes into their names. That, together with the other naming cleanups I've already prepared, would make the naming cleanup of the Radxa boards complete. - arch/arm/boot/dts/rockchip/rk3288-rock2-som.dtsi (-> arch/arm/boot/dts/rockchip/rk3288-rock-2-som.dtsi) - arch/arm/boot/dts/rockchip/rk3288-rock2-square.dts (-> arch/arm/boot/dts/rockchip/rk3288-rock-2-square.dts) I see no possible issues in the kernel arising from these two file renames, with the other related changes applied, of course, but there are some issues in U-Boot. Luckily, overcoming those issues shouldn't be too hard, and I've already submitted a small preparatory patch to the U-Boot mailing list. [1] We'll see how it goes. [1] https://lore.kernel.org/u-boot/369151a2b8d73d733ca79eca98d4d5958f90d491.1713423687.git.dsimic@manjaro.org/
Hi, On 4/18/24 16:18, Dragan Simic wrote: > Actually, a couple of board dts(i) files, listed below, need to > be renamed to add dashes into their names. That, together with > the other naming cleanups I've already prepared, would make the > naming cleanup of the Radxa boards complete. > > - arch/arm/boot/dts/rockchip/rk3288-rock2-som.dtsi > (-> arch/arm/boot/dts/rockchip/rk3288-rock-2-som.dtsi) > - arch/arm/boot/dts/rockchip/rk3288-rock2-square.dts > (-> arch/arm/boot/dts/rockchip/rk3288-rock-2-square.dts) uh... they are prehistoric relics... there was no strict rule... (FYI, there is rk3188-radxarock.dts too) I don't feel a strong need for them... -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd.
On 2024-04-18 10:31, FUKAUMI Naoki wrote: > On 4/18/24 16:18, Dragan Simic wrote: >> Actually, a couple of board dts(i) files, listed below, need to >> be renamed to add dashes into their names. That, together with >> the other naming cleanups I've already prepared, would make the >> naming cleanup of the Radxa boards complete. >> >> - arch/arm/boot/dts/rockchip/rk3288-rock2-som.dtsi >> (-> arch/arm/boot/dts/rockchip/rk3288-rock-2-som.dtsi) >> - arch/arm/boot/dts/rockchip/rk3288-rock2-square.dts >> (-> arch/arm/boot/dts/rockchip/rk3288-rock-2-square.dts) > > uh... they are prehistoric relics... there was no strict rule... > (FYI, there is rk3188-radxarock.dts too) > > I don't feel a strong need for them... In that case, it's perhaps better not to do any file renaming, which will also keep us on the extra safe side. The U-Boot patch I already submitted won't hurt anyway. :)
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/20240201034621.1970279-1-kever.yang@rock-chips.com/ > >>> + 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
diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml index 99bd5e2c76a0..dc3847bd67fe 100644 --- a/Documentation/devicetree/bindings/arm/rockchip.yaml +++ b/Documentation/devicetree/bindings/arm/rockchip.yaml @@ -777,6 +777,11 @@ properties: - const: radxa,rock3a - const: rockchip,rk3568 + - description: Radxa ROCK3 Model C + items: + - const: radxa,rock3c + - const: rockchip,rk3566 + - description: Radxa ROCK 5 Model A items: - const: radxa,rock-5a
The Radxa ROCK3 Model C is a similar board to the Radxa ROCK3 Model A with the RK3566 SoC. Add devicetree binding documentation for it. Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn> --- Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++ 1 file changed, 5 insertions(+)