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: <aOz1SzpzGLDQjiYQ@shell.armlinux.org.uk>
Date: Mon, 13 Oct 2025 13:49:15 +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 Fri, Oct 10, 2025 at 11:20:08AM +0800, Liangbin Lian wrote:
> Russell King (Oracle) <linux@...linux.org.uk> 于2025年10月9日周四 21:59写道:
> >
> > 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:
...
> >   clock_in_out:
> >     description:
> >       For RGMII, it must be "input"
> 
> This description does not match the actual situation,
> there are many dts using 'output':
> https://elixir.bootlin.com/linux/v6.17/source/arch/arm64/boot/dts/rockchip/rk3568-bpi-r2-pro.dts#L235
> https://elixir.bootlin.com/linux/v6.17/source/arch/arm64/boot/dts/rockchip/rk3568-evb1-v10.dts#L241
> https://elixir.bootlin.com/linux/v6.17/source/arch/arm64/boot/dts/rockchip/rk3568-fastrhino-r68s.dts#L33
> https://elixir.bootlin.com/linux/v6.17/source/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts#L78

For all of the above, whether it is "input" or "output" has no effect
as these are all rk3568, and rk3568 does not implement the
set_clock_selection() method.

static const struct rk_gmac_ops rk3568_ops = {
        .set_phy_intf_sel = rk3568_set_phy_intf_sel,
        .set_to_rgmii = rk3568_set_to_rgmii,
        .set_to_rmii = rk3568_set_to_rmii,
        .set_speed = rk_set_clk_mac_speed,
        .regs_valid = true,
        .regs = {
                0xfe2a0000, /* gmac0 */
                0xfe010000, /* gmac1 */
                0x0, /* sentinel */
        },
};

I'm going to propose:

1) that the driver should only print an error if "clock_in_out" is
missing _and_ the SoC implements the required function.

2) that the driver should print a non-fatal error if this property is
specified in DT _and_ the SoC does not implement the function to
discourage its use.

3) [tr]x_delay should not print an error for non-RGMII phy interface
modes.

I consider it a bug that a driver prints errors for properties that
have not been specified that it does not actually require. By doing
so, it encourages people to add useless properties to their DT
description that will never ever be used (e.g. because they are not
relevant for hardware the operating mode that the board is setup
for.)

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

Powered by Openwall GNU/*/Linux Powered by OpenVZ