linux-kernel - Re: [PATCH v2 2/2] ARM: dts: aspeed: add device tree for ASRock Rack ALTRAD8 BMC

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <8b5625a5-56bf-49bc-b4cf-82c60460db65@lunn.ch>
Date: Wed, 17 Sep 2025 23:04:45 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Rebecca Cran <rebecca@...io.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Joel Stanley <joel@....id.au>,
	Andrew Jeffery <andrew@...econstruct.com.au>,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-aspeed@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] ARM: dts: aspeed: add device tree for ASRock Rack
 ALTRAD8 BMC

Hi Rebecca

> +// Connected to host Intel X550 (ALTRAD8UD-1L2T) or
> +// Broadcom BCM57414 (ALTRAD8UD2-1L2Q) interface
> +&mac0 {

Thanks for the comments.

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_rmii1_default>;
> +
> +	clocks = <&syscon ASPEED_CLK_GATE_MAC1CLK>,
> +		 <&syscon ASPEED_CLK_MAC1RCLK>;
> +	clock-names = "MACCLK", "RCLK";
> +
> +	use-ncsi;
> +
> +	nvmem-cells = <&eth0_macaddress>;
> +	nvmem-cell-names = "mac-address";
> +};
> +
> +// Connected to Realtek RTL8211E
> +&mac1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_rgmii2_default &pinctrl_mdio2_default>;
> +
> +	nvmem-cells = <&eth1_macaddress>;
> +	nvmem-cell-names = "mac-address";
> +};

I see you did not manage to get phy-handle to work.

The problem i have is the lack of phy-mode = "rgmii-id".

Aspeed has a long history have getting phy-mode wrong. This has mainly
affected 2600, but as far as i understand, this is a 2500?

I've been pushing back on any 2600 DT using phy-mode = "rgmii". This
is 99% of the time wrong, since it indicates that the PCB has extra
long clock lines.

https://elixir.bootlin.com/linux/v6.15/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

What aspeed get wrong is that they configure the MAC to insert 2ns
delay. And this is hidden way, in the bootloader. As a result,
PHY_INTERFACE_MODE_RGMII gets passed to phy_connect(), when
PHY_INTERFACE_MODE_RGMII_ID should be passed. Its a case of two wrongs
combine to give a working system.

I've been pushing aspeed to clean up this mess. And to do that, i've
been rejecting DT with phy-mode = "rgmii". Most submissions has ended
up dropping ethernet support, and are now twiddling their thumbs
waiting for aspeed to cleanup the mess. Or anybody actually, there is
enough public information anybody could fix it, and probably end up
with a crate of beer/box of wine, etc.

Now, it is slightly different here. You don't have a phy-mode at
all. And all the other 2500 i looked at also don't have phy-mode
properties. You are relying on

        /* Default to RGMII. It's a gigabit part after all */
        err = of_get_phy_mode(np, &phy_intf);
        if (err)
                phy_intf = PHY_INTERFACE_MODE_RGMII;

This is equally wrong :-(

Now, there was somebody who tried getting around my push back on 2600
by simply deleting the phy-mode, and relying on this fallback code. I
said don't do that. Its just going to cause more problems when the code
gets cleaned up, and we have to remember there is an oddball platform.

At the time, i did not know 2500 was different, and none of the DTs
use phy-mode. So, i'm humming and harring. I set a precedent, no
phy-mode should get a patch rejected. But this is 2500, not 2600.

O.K. I will accept it, for 2500. Since none of the 2500 boards have
it, this is not an oddball platform. If the cleanup breaks it, it will
break all 2500 and that should get noticed pretty quickly.

So, for these two nodes only:

Reviewed-by: Andrew Lunn <andrew@...n.ch>

    Andrew