Re: [PATCH net-next] net: macb: add support for 1000BASE-X autonegotiation to PCS

[Charles Perry <charles.perry@microchip.com>]

On Wed, Jul 15, 2026 at 12:35:48PM -0400, Nathan Whitehorn wrote:
> 
> 
> On 7/15/26 12:29, Charles Perry wrote:
> > On Tue, Jul 14, 2026 at 04:09:04PM -0400, Nathan Whitehorn wrote:
> > > The current PCS code unconditionally uses SGMII autonegotiation, though
> > > the hardware supports both SGMII and 1000BASE-X modes. Decouple the
> > > choice of PCS enablement from use of the SGMII mode when running at
> > > gigabit rates and announce to phylink that 1000BASE-X is a supported
> > > operating mode. This enables direct attachment of the PCS to e.g. an
> > > SFP.
> > > 
> > > Tested and developed on Microchip Polarfire SoC hardware.
> > > 
> > > Signed-off-by: Nathan Whitehorn <nwhitehorn@pa.msu.edu>
> > > ---
> > >   drivers/net/ethernet/cadence/macb_main.c | 31 ++++++++++++++++++------
> > >   1 file changed, 24 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > > index d394f1f43b68..284a3b03f8c7 100644
> > > --- a/drivers/net/ethernet/cadence/macb_main.c
> > > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > > @@ -583,7 +583,13 @@ static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> > >   static void macb_pcs_an_restart(struct phylink_pcs *pcs)
> > >   {
> > > -	/* Not supported */
> > > +	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
> > > +	u32 old, new;
> > > +
> > > +	old = gem_readl(bp, PCSCNTRL);
> > > +	new = old | BMCR_ANRESTART;
> > > +	if (old != new)
> > > +		gem_writel(bp, PCSCNTRL, new);
> > This bit is self-clearing so I don't think the old != new check is
> > required.
> 
> Ah, OK, good to know.
> 
> > Also, can you comment on why AN restart is needed for this patch? Is it a
> > requirement for 1000base-x of is it because we now have sgmii and
> > 1000base-x?
> 
> Our equipment works well enough without this, and it is just here for
> completeness rather than an actual need. I am happy to drop it from the
> patch if you prefer.
> 

Well it does look like the ->pcs_an_restart() callback was made
specifically for 1000base-x because phylink_pcs_an_restart() in phylink.c
checks for phy_interface_mode_is_8023z():

https://elixir.bootlin.com/linux/v7.1.3/source/drivers/net/phy/phylink.c#L1028

Maybe you actually need this.

Thanks,
Charles