Re: [PATCH v2 2/2] arm64: dts: rockchip: Add FriendlyElec CM3588 NAS board
From: Space Meyer
Date: Thu Jun 06 2024 - 09:14:28 EST
+ Sebastian Reichel regarding pcie3x4 BAR 1 overlap
Hi Sebastian (Kropatsch),
I was working on the same device, alas you were faster then me :^)
I tested your device tree and confirmed:
- SD card works (I'm booting from it)
- Ethernet works
- My NVME is detected in all 4 ports and I can read from it.
- OHCI is working for all three USB-A ports (I assume EHCI as well)
- XHCI is working for both USB3-A ports
- 'User' button presses end up in
/dev/input/by-path/platform-gpio-keys-event (though I have no idea how
to use / decode it)
However there are some issues:
- Type-C: No PD negotiated in or out
- Type-C: No USB 1/2/3 devices recognised (I don't have any device to
test DP mode switching)
- Audio: No audio (might just be my userspace)
- I didn't test mmc, hdmi, db, gpu, fan, npu raspi header...
- Your regulators are not always following the naming in the schematic
very closely.
Dmesg also has some concerning boot logs:
- Missing phy-supply for usbdp_phy1, combphy0_ps, combphy1_ps,
combphy2_psu, pcie30phy
- Missing vmmc-supply and vqmmc-supply for sdhci
- PCIE: pcie3x4 BAR 1 fails to assign (probably overlapping region due
to untested 1x1x1x1 bifurcation in rk3588.dtsi)
- PCIE: a bunch of `bridge configuration invalid` during boot, no idea
whether they having something todo with your DTS though
- Sensors: rockchip-thermal fec00000.tsadc fails initializing.
lm-sensors shows me no sensors at all. Maybe I'm just missing the driver?
- `rockchip-drm display-subsystem` fails initializing
Regarding the dts I'll leave some comments below, but please note I also
have very little device tree experience so take my input with a truck
load of salt.
On 02.06.2024 22:20, Sebastian Kropatsch wrote:
Some RK3588 boards are still using this property, the following quote
is from rk3588-tiger-haikou.dts for example:
&sdmmc {
/* while the same pin, sdmmc_det does not detect card changes */
cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
I am unsure as to whether this comment from the quote might apply for
the CM3588 as well. Please let me know if you are able to tell :-)
I don't quite understand this. However GPIO0_A4 *is* routed to the micro
sd CD according to the NAS schematic, page 16 around A5.
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588-nas.dts
+ adc_keys: adc-keys {
AFAICT this board uses only 1 button per ADC input. Hence I think we
need seperate ADC defs per button. The usual plural "adc-keys" does not
apply.
+ compatible = "adc-keys";
+ io-channels = <&saradc 1>;
+ io-channel-names = "buttons";
+ keyup-threshold-microvolt = <1800000>;
+ poll-interval = <100>;
+
+ button-vol-up {
+ label = "Volume Up";
+ linux,code = <KEY_VOLUMEUP>;
I believe this should be `label = "Recovery"`, as it's printed like that
on the silk screen. Maybe also give it a generic function like BTN_1.
+ press-threshold-microvolt = <17000>;
+ };
+ };
While at it you could also add the button labeled "Mask", which is at
`io-channels = <&saradc 0>;`.
+ analog-sound {
+ compatible = "simple-audio-card";
+ pinctrl-names = "default";
+ pinctrl-0 = <&headphone_detect>;
+
+ simple-audio-card,name = "realtek,rt5616-codec";
+ simple-audio-card,format = "i2s";
+ simple-audio-card,mclk-fs = <256>;
+
+ simple-audio-card,hp-det-gpio = <&gpio1 RK_PC4 GPIO_ACTIVE_LOW>;
+
+ simple-audio-card,routing =
+ "Headphones", "HPOL",
+ "Headphones", "HPOR",
+ "MIC1", "Microphone Jack",
+ "Microphone Jack", "micbias1";
+ simple-audio-card,widgets =
+ "Headphone", "Headphones",
+ "Microphone", "Microphone Jack";
+
+ simple-audio-card,cpu {
+ sound-dai = <&i2s0_8ch>;
+ };
+
+ simple-audio-card,codec {
+ sound-dai = <&rt5616>;
+ };
+ };
The rt5616 is on the SoM according to the schematic. Maybe move it all
there and then only define the hp-det-gpio here?
+ vcc_3v3_host_32: regulator-vcc-3v3-host-32 {
+ compatible = "regulator-fixed";
+ enable-active-high;
+ gpios = <&gpio3 RK_PA5 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&vcc_3v3_host32_en>;
+ regulator-name = "vcc_3v3_host_32";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <&vcc_5v0_sys>;
+ };
I think this is a 5v0 regulator?
+ vcc_3v3_pcie30: regulator-vcc-3v3-pcie30 {
+ compatible = "regulator-fixed";
+ regulator-name = "vcc_3v3_pcie30";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ vin-supply = <&vcc_5v0_sys>;
+ };
These are 4 seperate regulators according to the schematic. However, as
they are all fixed, idk if they should be split or kept like this.
+&combphy0_ps {
+ status = "okay";
+};
Dupplicate definition, already present in dtsi (where it belongs imho).
Also maybe add a comment, that this is used for pcie2x1l2.
+&combphy1_ps {
+ status = "okay";
+};
Maybe add a comment, that this is used for pcie2x1l0, connected to M.2
Slot #2.
+&combphy2_psu {
+ status = "okay";
+};
Maybe add a comment, that this is used for USB30 HOST2.
+ fusb302: typec-portc@22 {
+ compatible = "fcs,fusb302";
+ reg = <0x22>;
+ interrupt-parent = <&gpio0>;
+ interrupts = <RK_PD3 IRQ_TYPE_LEVEL_LOW>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&usbc0_int>;
+ vbus-supply = <&vbus_5v0_typec>;
Isn't this missing a `status = "okay";`?
+
+ usb_con: connector {
+ compatible = "usb-c-connector";
+ data-role = "dual";
+ label = "USB-C";
+ op-sink-microwatt = <1000000>;
+ power-role = "dual";
Looking at the schematic, I don't think this is dual role power. I think
it's only a source. Have you tested this working in both directions?
+&pcie30phy {
Not really a review comment, but a note for others: ASPM implementation
seems buggy. Setting CONFIG_PCIEASPM_POWERSAVE to certain values breaks
PCIe completely.
+&pinctrl {
+ audio {
+ headphone_detect: headphone-detect {
+ rockchip,pins = <1 RK_PC4 RK_FUNC_GPIO &pcfg_pull_none>;
You could use &gpio1 instead of 1. Same for every entry in &pinctrl.
+&u2phy0 {
You could add a comment about the usage like:
USB20 OTG0 in CM3588 USB Controller Configure Table
USB 2.0 phy for the 'USBC1' Port in Nas Schematic
+&u2phy0_otg {
Missing `phy-supply = <&vbus_5v0_typec>;`?
+&u2phy1 {
You could add a comment about the usage like:
USB20 OTG1 in CM3588 USB Controller Configure Table
USB 2.0 phy for the 'USB 3.0 Type A x2 Up' Port in Nas Schematic
+&u2phy2 {
You could add a comment about the usage like:
USB20 HOST0 in CM3588 USB Controller Configure Table
Phy for the 'USB 2.0 A' Port in Nas Schematic
> +&usb_host0_ehci {
> +&usb_host0_ohci {
Maybe add a comment, that this is using
`phys = <&u2phy2_host>;`
> +&usb_host0_xhci {
Maybe add a comment, that this is using
`phys = <&u2phy0_otg>, <&usbdp_phy0 PHY_TYPE_USB3>;`
> + usb-role-switch;
Were you actually able to use the typec in gadget mode?
I think this might only work in dr_mode = "host";
+&usb_host1_ehci {
> +&usb_host1_ohci {
Maybe add a comment, that these are using `phys = <&u2phy3_host>`.
+/* Upper USB 3.0 port */
+&usb_host1_xhci {
Maybe add a comment, that this is using
`phys = <&u2phy1_otg>, <&usbdp_phy1 PHY_TYPE_USB3>;`
+/* Lower USB 3.0 port */
+&usb_host2_xhci {
Maybe add a comment, that this is using
phys = <&combphy2_psu PHY_TYPE_USB3>;
+&usbdp_phy0 {
You could add a comment about the usage like:
USB30 OTG0 in CM3588 USB Controller Configure Table
USB 3.0 phy for the USBC1 Port in Nas Schematic
+&usbdp_phy1 {
You could add a comment about the usage like:
USB30 OTG1 in CM3588 USB Controller Configure Table
USB 3.0 phy for the USB 3.0 Type A x2 Up Port in Nas Schematic
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588-friendlyelec-cm3588.dtsi
+ led_sys: led-0 {
+ color = <LED_COLOR_ID_AMBER>;
This one is LED_COLOR_ID_RED.
+
+ led_usr: led-1 {
+ color = <LED_COLOR_ID_AMBER>;
And this one is LED_COLOR_ID_GREEN.
+&combphy0_ps {
For pcie2x1l2, connected to RTL8125 ethernet
+&combphy1_ps {
> +&combphy2_psu {
Duplicate definitions, also in nas dts (where they belong imho).
+&pinctrl {
+ gpio-leds {
+ led_sys_pin: led-sys-pin {
+ rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
You could use &gpio2 instead of 2. Same for every entry in &pinctrl.
Kind regards,
Space