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

[Coia Prant <coiaprant@gmail.com>]

On August 2, 2026 10:39:46 PM GMT+08:00, Andrew Lunn <andrew@lunn.ch> wrote:
>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

Hi Andrew,

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

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

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

Thanks,
Coia