Re: [PATCH v2 07/10] net: pcs: xpcs: a dd Rockchip RK3568 platform glue driver

On August 2, 2026 10:39:46 PM GMT+08:00, Andrew Lunn <andrew@lunn=2Ech> wro=
te:
>On Sun, Aug 02, 2026 at 11:20:27AM +0800, Coia Prant wrote:
>> > On Sat, Aug 01, 2026 at 10:22:31PM +0800, Coia Prant wrote:
>> > > The XPCS block contains four MII ports (0=2E=2E3), each of which ca=
n be
>> > > routed to GMAC0 or GMAC1 via the pcs-handle property in the MAC nod=
e=2E
>> > > The hardware maps these ports to different MMDs:
>> > >   - port 0: MMD 7 (ROCKCHIP_MMD_MII)
>> > >   - port 1: MMD 2 (ROCKCHIP_MMD_MII1)
>> > >   - port 2: MMD 3 (ROCKCHIP_MMD_MII2)
>> > >   - port 3: MMD 4 (ROCKCHIP_MMD_MII3)
>> >
>> > Why is port 0 called ROCKCHIP_MMD_MII not ROCKCHIP_MMD_MII0 ?
>>=20
>> Hi Andrew,
>>=20
>> The naming follows the Rockchip TRM=2E The hardware documentation refer=
s to
>> the MMD for the first port as simply "MII" without a numeric suffix, wh=
ile
>> the other ports are named "MII1", "MII2", "MII3"=2E I kept the naming
>> consistent with the TRM to make it easier to cross-reference=2E
>>=20
>> As I understand it, this is probably because the MII controls not only
>> Port 0, but
>> also the entire PCS (these registers are read-only in Ports 1-3 and ref=
lected
>> back to the MII)=2E
>>=20
>> If you prefer, I can rename it to ROCKCHIP_MMD_MII0 for consistency=2E =
Let me
>> know and I'll update it in v3=2E
>
>If the TRM gives it this name, then O=2EK=2E It just makes the code look
>odd, unbalanced=2E
>
>> > > +static int xpcs_rk_read_c22(struct mii_bus *bus, int addr, int reg=
)
>> > > +{
>> > > +     struct dw_xpcs_rk *pxpcs =3D bus->priv;
>> > > +     int dev;
>> > > +
>> > > +     if (!xpcs_rk_mdio_addr_validate(addr))
>> > > +             return -ENODEV;
>> > > +
>> > > +     dev =3D xpcs_rk_mdio_read_remapping(addr, MDIO_MMD_VEND2, reg=
);
>> >
>> > Does this mean C22 registers are mapped into the first 32 of C45
>> > MDIO_MMD_VEND2?
>>=20
>> Yes, exactly=2E The C22 register space (reg 0-31) is mapped into the fi=
rst
>> 32 registers of the VEND2 MMD (MMD 7)=2E This is how the hardware is de=
signed
>> and matches the standard C22 to C45 address mapping=2E
>
>Does the xpcs code actually perform any C22 access? A quick look
>suggests it is C45 only=2E xpcs_read() calls mdiodev_c45_read()=2E If C22
>is not needed, i would not provide these functions=2E
>
>>=20
>> >
>> > > +static int xpcs_rk_read_c45(struct mii_bus *bus, int addr, int dev=
, int reg)
>> > > +{
>> > > +     struct dw_xpcs_rk *pxpcs =3D bus->priv;
>> > > +
>> > > +     if (!xpcs_rk_mdio_addr_validate(addr))
>> > > +             return -ENODEV;
>> > > +
>> > > +     dev =3D xpcs_rk_mdio_read_remapping(addr, dev, reg);
>> >
>> > Should it be returning an error for dev =3D=3D MDIO_MMD_VEND2? Or at =
least
>> > if reg < 32?
>>=20
>> No, we cannot simply return an error here=2E
>
>Thanks for the explanation=2E
>
>       Andrew

Hi Andrew,

You're right that the xpcs core uses C45 exclusively, and modern kernels
no longer require C22 callbacks for mdiobus_register()=2E

However, I'd prefer to keep them for two reasons:
1=2E Debugging tools (mdio-tools, ethtool, etc=2E) often use C22 reads to
   inspect PHY/PCS registers=2E Having these callbacks makes debugging
   much easier without having to patch the driver=2E
2=2E It keeps the driver consistent with pcs-xpcs-plat=2Ec, which also
   provides both C22 and C45 callbacks even though the xpcs core
   only uses C45=2E

If you strongly prefer removing them to keep the code minimal, I can do
that in v3=2E But I think the debug benefit justifies keeping them=2E

Thanks,
Coia