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] [day] [month] [year] [list]

Message-ID: <CAP_9mL6x=p169y026nNycZsHK6UMHqJ+o-M_k0MqnvpB2fPzJg@mail.gmail.com>
Date: Mon, 13 Oct 2025 22:07:02 +0800
From: Liangbin Lian <jjm2473@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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

Russell King (Oracle) <linux@...linux.org.uk> 于2025年10月13日周一 20:49写道：
>
> 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!

I will remove clock_in_out, its default value is 'input'.
After removing it, there will only be an error log, which will not
affect the function.