Re: [net-next,v2,2/8] net: macb: rename macb_default_usrio to at91_default_usrio as not all platforms have mii mode control in usrio

[Conor Dooley]

Yo,

On Sat, Feb 28, 2026 at 03:26:00PM -0800, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> 
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> net: macb: rename macb_default_usrio to at91_default_usrio as not all
> platforms have mii mode control in usrio
> 
> This patch renames macb_default_usrio to at91_default_usrio to better
> reflect that the usrio configuration is platform-specific, and
> introduces a new capability flag (MACB_CAPS_USRIO_HAS_MII) to gate MII
> mode configuration code for platforms that actually support it.
> 
> > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> > --- a/drivers/net/ethernet/cadence/macb.h
> > +++ b/drivers/net/ethernet/cadence/macb.h
> > @@ -779,6 +779,7 @@ #define MACB_CAPS_DMA_64B			BIT(21)
> >  #define MACB_CAPS_DMA_PTP			BIT(22)
> >  #define MACB_CAPS_RSC				BIT(23)
> >  #define MACB_CAPS_NO_LSO			BIT(24)
> > +#define MACB_CAPS_USRIO_HAS_MII			BIT(25)
> >
> >  /* LSO settings */
> >  #define MACB_LSO_UFO_ENABLE			0x01
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -4614,13 +4614,15 @@
> >
> >  	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> >  		val = 0;
> > -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
> > -			val = bp->usrio->rgmii;
> > -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> > -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > -			val = bp->usrio->rmii;
> > -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > -			val = bp->usrio->mii;
> > +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
> > +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
> > +				val = bp->usrio->rgmii;
> > +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> > +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > +				val = bp->usrio->rmii;
> > +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> > +				val = bp->usrio->mii;
> > +		}
> >
> >  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> >  			val |= bp->usrio->refclk;
> 
> Can bp->usrio be NULL here? The new MACB_CAPS_USRIO_HAS_MII capability
> gates the MII-related dereferences of bp->usrio, but bp->usrio is also
> dereferenced when MACB_CAPS_USRIO_HAS_CLKEN is set.
> 
> With the change to default_gem_config later in this patch:
> 
> static const struct macb_config default_gem_config = {
> 	...
> 	.usrio = NULL,
> 	...
> };
> 
> A device that matches the generic 'cdns,macb' compatible string (without
> specific .data) will get bp->usrio = NULL during probe. If the device
> tree then has 'cdns,refclk-ext', macb_configure_caps() adds
> MACB_CAPS_USRIO_HAS_CLKEN without checking if bp->usrio is valid:
> 
> macb_configure_caps() {
> 	...
> 	if (of_property_read_bool(np, "cdns,refclk-ext"))
> 		bp->caps |= MACB_CAPS_USRIO_HAS_CLKEN;
> 	...
> }
> 
> Then in macb_init(), the code would dereference bp->usrio->refclk with
> bp->usrio being NULL, causing a crash.
> 
> The MACB_CAPS_USRIO_HAS_MII gate only protects the MII-related
> dereferences above, not the CLKEN dereference.

AI makes a good point here, but in one way this isn't even a problem
with the patchset, it's actually another example of the sort of thing
that I am trying to get rid of from the driver. As far as I can tell,
this was added very recently, for the emac on sama7g5. The sama7g5-emac
sets this cap in its match data, as do several other devices, so this
code that sets the cap based on the dt property isn't needed.

I would like to get rid of setting the cap based on the dt property,
because it is inherently tied to how at91 have their USRIO set up.
Other platforms that want to use this external refclk might use a
different mechanism for the selection, or only support an external
source. What USRIO does is very platform specific, so it should be
something that is opted in to explicitly. I didn't ask for the property
to be bound to only the at91 devices that use it when I reviewed the
binding, because I figured other platforms might be able to reuse the
property and that's also why I didn't ask for a microchip prefix on the
property. For the driver to reflect that general use, it shouldn't then
do at91-specific things when the property is present.

Ryan, how does this property actually work now, given your removal
of the cap from match data was reverted? I feel like it just doesn't do
what you want it to do anymore? Before the revert, having the property
meant that the value in sama7g5_usrio.refclk would be written to the
hardware and not having the property meant that the bit would remain
unset. You then had to revert to avoid breaking old devices, because
your property changed the default behaviour for the emac, so now having
the property means that the value in sama7g5_usrio.refclk is written to
the hardware, but that also happens without the property because the cap
is set in the match data.

Unless I am missing something, should we not actually revert
dce32ece3bb8f ("net: cadence: macb: Expose REFCLK as a device tree
property") and 1b7531c094c88 ("dt-bindings: net: cdns,macb: Add external
REFCLK property"), and instead add a property like
"microchip,refclk-internal", because that would actually let you
override the default behaviour of the driver?

Cheers,
Conor.

> 
> > @@ -4638,13 +4640,6 @@
> >  	return 0;
> >  }
> >
> > -static const struct macb_usrio_config macb_default_usrio = {
> > -	.mii = MACB_BIT(MII),
> > -	.rmii = MACB_BIT(RMII),
> > -	.rgmii = GEM_BIT(RGMII),
> > -	.refclk = MACB_BIT(CLKEN),
> > -};
> > -
> 
> [ ... ]
> 
> > @@ -5219,6 +5214,13 @@ static int eyeq5_init(struct platform_device *pdev)
> >  	return ret;
> >  }
> >
> > +static const struct macb_usrio_config at91_default_usrio = {
> > +	.mii = MACB_BIT(MII),
> > +	.rmii = MACB_BIT(RMII),
> > +	.rgmii = GEM_BIT(RGMII),
> > +	.refclk = MACB_BIT(CLKEN),
> > +};
> > +
> 
> [ ... ]
> 
> > @@ -5432,7 +5448,7 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
> >  	.dma_burst_length = 16,
> >  	.clk_init = macb_clk_init,
> >  	.init = macb_init,
> > -	.usrio = &macb_default_usrio,
> > +	.usrio = NULL,
> >  	.jumbo_max_len = 10240,
> >  };
> >
> -- 
> pw-bot: cr

Attachment: signature.asc
Description: PGP signature