Re: [PATCH v2 2/2] arm64: dts: rockchip: Add FriendlyElec CM3588 NAS board

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



+ 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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux