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

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..3), each of which can be
> > > routed to GMAC0 or GMAC1 via the pcs-handle property in the MAC node.
> > > 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 ?
> 
> Hi Andrew,
> 
> The naming follows the Rockchip TRM. The hardware documentation refers to
> the MMD for the first port as simply "MII" without a numeric suffix, while
> the other ports are named "MII1", "MII2", "MII3". I kept the naming
> consistent with the TRM to make it easier to cross-reference.
> 
> 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 reflected
> back to the MII).
> 
> If you prefer, I can rename it to ROCKCHIP_MMD_MII0 for consistency. Let me
> know and I'll update it in v3.

If the TRM gives it this name, then O.K. It just makes the code look
odd, unbalanced.

> > > +static int xpcs_rk_read_c22(struct mii_bus *bus, int addr, int reg)
> > > +{
> > > +     struct dw_xpcs_rk *pxpcs = bus->priv;
> > > +     int dev;
> > > +
> > > +     if (!xpcs_rk_mdio_addr_validate(addr))
> > > +             return -ENODEV;
> > > +
> > > +     dev = 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?
> 
> Yes, exactly. The C22 register space (reg 0-31) is mapped into the first
> 32 registers of the VEND2 MMD (MMD 7). This is how the hardware is designed
> and matches the standard C22 to C45 address mapping.

Does the xpcs code actually perform any C22 access? A quick look
suggests it is C45 only. xpcs_read() calls mdiodev_c45_read(). If C22
is not needed, i would not provide these functions.

> 
> >
> > > +static int xpcs_rk_read_c45(struct mii_bus *bus, int addr, int dev, int reg)
> > > +{
> > > +     struct dw_xpcs_rk *pxpcs = bus->priv;
> > > +
> > > +     if (!xpcs_rk_mdio_addr_validate(addr))
> > > +             return -ENODEV;
> > > +
> > > +     dev = xpcs_rk_mdio_read_remapping(addr, dev, reg);
> >
> > Should it be returning an error for dev == MDIO_MMD_VEND2? Or at least
> > if reg < 32?
> 
> No, we cannot simply return an error here.

Thanks for the explanation.

       Andrew