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 <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 8067 bytes --]

On Tue, Mar 03, 2026 at 10:35:05AM -0700, Ryan Wanner wrote:
> On 2/28/26 17:06, Conor Dooley wrote:
> > 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.
> 
> So even with the revert the patch still does what it is intended to do,
> mainly for the sama7g5_gem and the newer SAM devices, sam9x75 and sama7d65.

Oh, the gems actually have this, but with the default the other way to
the emacs? I had figured that only the emacs actually had it, given they
were all added prior to your change. Obviously the property then cannot
be removed if the gems need it.

> Currently with the removal of the change to the emac there is no
> functional difference. With the newer SAM devices that need this usrio
> flexibility this patch still works.
> 
> How this works now is for sama7g5_gmac configs, the default is to use
> the internal clock for refclk then adding that dt property will set that
> bit to 1 and refclk will be from an external source. For the
> sama7g5_emac configs this has no effect due to ABI.
> > 
> > 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?
> 
> I think this could be the better way to go as this keeps the existing
> behavior for the legacy devices while still allowing the initial goal of
> the patch series. Seems it would be more strait forward in the ABI sense
> to have a "microchip,refclk-internal" property than an "refclk-ext"
> property and having the risk of breaking older devices.

By the sounds of things, you actually need both to be functional. The
gems default to internal, so need refclk-external and the emacs default
to external so need refclk-internal. Maybe it'd be better to have
"microchip/cdns,refclk-source" that can be set to a string to deal with
both cases?
I'd also like to decouple setting the USRIO_HAS_CLKEN cap from what
direction it is set in. That way, all devices with the bit in their
USRIO would set the cap, but the value would come from either a default
in match data or from the dt property if set. I think that's more
natural behaviour for the capability, since the bit is in the usrio
register regardless of which refclk source is used.

Does that seem reasonable?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]