Received-SPF: pass (google.com: domain of linux-kernel+bounces-207185-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ea39242d-9b2d-4ea0-aa24-8a959e4f875e@mail.de>
Date: Sat, 8 Jun 2024 21:58:36 +0200
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v2 2/2] arm64: dts: rockchip: Add FriendlyElec CM3588 NAS
 board
To: Space Meyer <me@the-space.agency>, Heiko Stuebner <heiko@sntech.de>,
 linux-rockchip@lists.infradead.org,
 Sebastian Reichel <sebastian.reichel@collabora.com>
Cc: Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,
 Conor Dooley <conor+dt@kernel.org>, Jonas Karlman <jonas@kwiboo.se>,
 Dragan Simic <dsimic@manjaro.org>, devicetree@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20240602211901.237769-1-seb-dev@mail.de>
 <20240602202132.2012-1-seb-dev@mail.de>
 <20240602202132.2012-2-seb-dev@mail.de>
 <c4087311-cbd2-415e-a582-3565f2f62e81@the-space.agency>
From: Sebastian Kropatsch <seb-dev@mail.de>
In-Reply-To: <c4087311-cbd2-415e-a582-3565f2f62e81@the-space.agency>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hello Space,

Am 06.06.2024 um 15:13 schrieb Space Meyer:
> + Sebastian Reichel regarding pcie3x4 BAR 1 overlap
> [...]
> 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)

The audio driver is pretty barebones, at least by looking at the
dt bindings (search for "realtek,rt5616"). I couldn't find any other
device apart from the NanoPC T6 and one old rk device that uses it.
Per dt bindings, properties like clocks and so on are invalid for
the current rt5616 driver.

Just look at FriendlyElec's rt5616 node:

	rt5616: rt5616@1b {
		status = "okay";
		#sound-dai-cells = <0>;
		compatible = "rt5616";
		reg = <0x1b>;
		clocks = <&mclkout_i2s0>;
		clock-names = "mclk";
		assigned-clocks = <&mclkout_i2s0>;
		assigned-clock-rates = <12288000>;
		pinctrl-names = "default";
		pinctrl-0 = <&i2s0_mclk>;
	};

Versus this one:

	rt5616: audio-codec@1b {
		compatible = "realtek,rt5616";
		reg = <0x1b>;
		#sound-dai-cells = <0>;
	};

I'll try to test the 3.5mm headphones output, but that's pretty
low priority currently.

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

At least some of your errors are because the features simply haven't
been upstreamed yet :)
For example the thermal sensors are still waiting for this patch series:
https://lore.kernel.org/linux-rockchip/20240506-rk-dts-additions-v4-0-271023ddfd40@gmail.com/

Thanks for your testing and feedback! I'll send a v3 soon. Feel free
to test this as well and if you like, send me a tested-by.

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

Yeah, many of the rk devices use "Volume Up", even though I'm sure
this likely is not the label on the board :D

> 
>> +            press-threshold-microvolt = <17000>;
>> +        };
>> +    };
> 
> While at it you could also add the button labeled "Mask", which is at 
> `io-channels = <&saradc 0>;`.

I didn't include this button since it's used to enter MASKROM mode
before booting (some hard-wired stuff iirc). It could be added, yes,
but I'm not sure if there's any use or if there's a best practice to
not include it in the DT.

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

You are correect, thanks! I trusted FriendlyElec too much on getting
this right in their own devicetree.

Following this mistake, I was just checking if the u2phy devices have
the correct phy-supply, but it's giving me headache.

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

I have not tested USB-C power delivery. But I also don't know exactly
how the schematic should look if it had PD in both directions.
Probably more complicated like on the NanoPi R6C. So if you're pretty
sure about this from reading the schematic, I'm removing it for v3.

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

Yes! I can confirm that the kernel will hang when trying to access
a NVMe SSD with aspm powersave enabled. Thought it was just something
with my configurations. Thanks for confirming this issue!

>> +&u2phy0_otg {
> 
> Missing `phy-supply = <&vbus_5v0_typec>;`?

Yes, thanks.

>  > +    usb-role-switch;
> 
> Were you actually able to use the typec in gadget mode?
> I think this might only work in dr_mode = "host";

Not tested. Vendor devicetree has a dwc3 role switch, but I
don't think we can necessarily count on that being true :)

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

Comparing it side by side with a really red LED, it looks more
like amber (dark orange) to me. But it may also be just some kind
of red.

>> +
>> +        led_usr: led-1 {
>> +            color = <LED_COLOR_ID_AMBER>;
> 
> And this one is LED_COLOR_ID_GREEN.

True, copy-paste mistake. Thanks!


Cheers, Sebastian