linux-kernel - Re: [PATCH v5 3/3] arm64: dts: rockchip: add LinkEase EasePi R1

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