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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Oct 10, 2025 at 11:20:08AM +0800, Liangbin Lian wrote:
> Russell King (Oracle) <linux@armlinux.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!