linux-kernel - Re: [PATCH 3/3] arm64: dts: rockchip: Add Radxa E24C

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e2fd11db-543a-43eb-b118-9f246ff149b5@kwiboo.se>
Date: Mon, 28 Jul 2025 19:09:10 +0200
From: Jonas Karlman <jonas@...boo.se>
To: Chukun Pan <amadeus@....edu.cn>
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org, heiko@...ech.de,
 krzk+dt@...nel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
 robh@...nel.org, ziyao@...root.org
Subject: Re: [PATCH 3/3] arm64: dts: rockchip: Add Radxa E24C

Hi Chukun,

On 7/28/2025 2:50 PM, Chukun Pan wrote:
> Hi,
> 
>> +	avddl_1v1: avddh_3v3: avdd_rtl8367rb: regulator-avdd-rtl8367rb {
>> +		compatible = "regulator-fixed";
>> +		enable-active-high;
>> +		gpios = <&gpio1 RK_PC3 GPIO_ACTIVE_HIGH>;
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&gpio_8367_en>;
>> +		regulator-name = "avdd_rtl8367rb";
> 
> I don't see the avdd_rtl8367rb regulator in the schematics. It looks like
> DVDDIO (RTL8367RB power) is connected to AVDDH_3V3 via a magnetic bead.

Both avddl_1v1 and avddh_3v3 are controlled by the same gpio, I do not
remember if using two regulators with same gpios is supported, can only
remember it being an issue in the past, so I opted to just describe it
as a single regulator and gave it a new name and added labels for the
name used in schematic.

Would calling it vdd_8367 (after gpio_8367_en) be better or do you have
any other suggestion on how to describe these?

I will at least add a comment related to this regulator for v2.

> 
>> +&gmac1 {
>> +	clock_in_out = "output";
>> +	phy-mode = "rgmii-id";
>> +	phy-supply = <&avdd_rtl8367rb>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&rgmii_miim>, <&rgmii_tx_bus2>, <&rgmii_rx_bus2>,
>> +		    <&rgmii_rgmii_clk>, <&rgmii_rgmii_bus>, <&gmac1_rstn_l>;
> 
> Should the pinctrl of gmac1_rstn_l be written together with the
> reset-gpios of the rtl8367rb switch?

When defining pinctrl to the mdio1 node they are not applied, and there
was issues probing the switch when using reset-gpios of the switch.
So I opted to describe the switch reset as the mdio bus reset.

I guess we should describe the HW and not work around SW issues, will
change in v2.

> 
> ```
> reset-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_LOW>;
> pinctrl-0 = <&gmac1_rstn_l>;
> ```
> 
>> +&i2c0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&i2c0m0_xfer>;
>> +	status = "okay";
>> +
>> +	rk805: pmic@18 {
>> +		compatible = "rockchip,rk805";
>> +		reg = <0x18>;
>> +		interrupt-parent = <&gpio4>;
>> +		interrupts = <RK_PB2 IRQ_TYPE_LEVEL_LOW>;
>> +		#clock-cells = <1>;
>> +		clock-output-names = "rk805-clkout1", "rk805-clkout2";
> 
> The clkout pin is not connected, but the dt-bindings require it.
> Maybe clock-output-names could be made optional?

Seem the using just #clock-cells = <0> is valid without
clock-output-names, will use that in v2, thanks.

> 
> +&mdio1 {
> +	reset-delay-us = <25000>;
> +	reset-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_LOW>;
> +	reset-post-delay-us = <100000>;
> +};
> 
> I don't think this is correct, reset-gpios should be written on the
> rtl8365mb switch node. The switch driver has defined the reset time.

See above, I had issues using the reset-gpios of the switch, because the
switch was probed twice, once deferred by gmac, and by the second probe
failed with -BUSY because of the reset-gpios still being claimd by the
first probe.

I can change to describe the reset pin in the switch, however that will
likely mean Ethernet is unusable until the issue in devres/gpiolib is
tracked down and fixed by someone.

> 
> ```
> &mdio1 {
> 	switch@29 {
> 		compatible = "realtek,rtl8365mb";
> 		reg = <29>;
> 		reset-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_LOW>;
> ```

As mentioned above, this caused probe issues and an unusable switch.
I would rather skip describing this reset pin as it does not seem to be
needed it self reset when the regulator is powered on.

Any thoughts on what is better of the two? Skip describing the reset pin
or describe it and leave the switch possible unusable? Probing gmac
before the switch should leave it in a working state.

Regards,
Jonas

> 
> Thanks,
> Chukun
> 
> --
> 2.25.1
> 
>