[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aOe_0EufouURu7R2@shell.armlinux.org.uk>
Date: Thu, 9 Oct 2025 14:59:44 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Liangbin Lian <jjm2473@...il.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
heiko@...ech.de, quentin.schulz@...rry.de,
kever.yang@...k-chips.com, naoki@...xa.com, honyuenkwun@...il.com,
inindev@...il.com, ivan8215145640@...il.com,
neil.armstrong@...aro.org, mani@...nel.org, dsimic@...jaro.org,
pbrobinson@...il.com, alchark@...il.com, didi.debian@...ow.org,
jbx6244@...il.com, andrew@...n.ch, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] arm64: dts: rockchip: add LinkEase EasePi R1
On Thu, Oct 09, 2025 at 04:44:16PM +0800, Liangbin Lian wrote:
> +&gmac0 {
> + phy-mode = "rgmii-id";
> + clock_in_out = "input";
...
> +&gmac1 {
> + phy-mode = "rgmii-id";
> + clock_in_out = "input";
I am fine with what is being proposed here, but I think this
clock_in_out property needs fixing. The description for it is thus:
clock_in_out:
description:
For RGMII, it must be "input", means main clock(125MHz)
is not sourced from SoC's PLL, but input from PHY.
For RMII, "input" means PHY provides the reference clock(50MHz),
"output" means GMAC provides the reference clock.
$ref: /schemas/types.yaml#/definitions/string
enum: [input, output]
default: input
The problems that I have here are:
1) the description states that the only possible value for this when in
RGMII mode is "input" which is reasonable, because it's due to the
RGMII specification. The driver code is perfectly able to determine
whether RGMII has been specified, and set bsp_priv->clock_input
itself, relieving DT of this need.
2) bsp_priv->clock_input is only used in gmac_clk_enable() when calling
the SoC specific set_clock_selection() method. Only RK3528, RK3576,
and RK3588 populate this method. Every other SoC supported by this
driver still requires the property:
ret = of_property_read_string(dev->of_node, "clock_in_out", &strings);
if (ret) {
dev_err(dev, "Can not read property: clock_in_out.\n");
bsp_priv->clock_input = true;
} ...
If one doesn't provide it, one gets an error print, which is not nice
I note that the DT binding doesn't list this property as required, so
the code is at odds with the binding.
(I note that your Rockchip SoC is RK3568, which doesn't implement this
method.)
So, taking both these points together, the code should not be printing
an error if "clock_in_out" is missing when either:
a) RGMII is being used (or maybe only when RMII is being used?)
b) the set_clock_selection() method is not present for the SoC variant.
With the driver fixed as indicated above, I then think clock_in_out in
your descriptions becomes unnecessary, and should be removed.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists