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

[Ryan Wanner]

On 3/3/26 15:44, Conor Dooley wrote:
> On Tue, Mar 03, 2026 at 03:04:19PM -0700, Ryan Wanner wrote:
>> On 3/3/26 11:01, Conor Dooley wrote:
>>> 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.
>>
>> Yes that is correct, I am not sure why this was left out in the initial
>> implementation of the gems maybe others can give some insight. The gems
>> can, similar to the emacs, take an external or internal ref clock that
>> is decided by the usrio registers. The difference with the gems and the
>> emacs is the gems can have 125MHz refclk for RGMII (50Mhz for RMII)
>> either internal or external where as the emacs can only switch refclk
>> for RMII.
>>
>>>
>>>> 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?
>>
>> Since a dt string would need to be placed either way could the default
>> be setting be 1 like how the emacs is set then make a
>> "microchip,refclk-internal" flag, this seems to have the minimal amount
>> of conflicts with legacy implementation.
> 
> Unless I misunderstand what you're saying, you're suggesting changing
> the default for the non-emacs? I don't think this can be done, for the
> same reason that your emac patch got reverted - it breaks existing
> devices. With my suggestion, the driver support for refclk-ext would
> remain, so as not to break any existing setups, but it would be removed
> from the binding or marked deprecated.

I understand what you are saying now about the gem and emcas being flipped.

> 
>>> 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?
>>
>> So to leave the "USRIO_HAS_CLKEN" caps flag in the existing configs for
>> emacs, then have this be a toggle for the gem configs, and the
>> "microchip/cdns,refclk-source" would be parsed to either set this bit to
>> 1 or 0? The USRIO refclk bit will be set in the macb_init() based on if
>> the dt property is there rather than how it is currently implemented now
>> if I understand correctly?
> 
> No, I don't think this is what I am suggesting. I am suggesting adding
> USRIO_HAS_CLKEN to the gem configs, and then modifying the usrio struct
> to have a member called something like "refclk_default_enable" and setting
> it to true for emacs and false for gems. Then in init code somewhere, we
> would do:
> 
> 	if (property_present(refclk-source))
> 		if (property_read_string(refclk-source) == "external"
> 			refclk_enable = true
> 		else
> 			refclk_enable = false
> 	else if (property_present(refclk-ext))
> 		refclk_enable = true
> 	else
> 		refclk_enable = usrio.refclk_default_enable
> 
> Then later on in init, when configuring the caps, we would do:
> 
> 	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
> 
> 		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
> 			val |= refclk_enable;
> 
> 		macb_or_gem_writel(bp, USRIO, val);
> 	}
> 
> Or maybe we just roll all that up together inside the !USRIO_DISABLED
> code. 
> 
> The code currently in macb_configure_caps() about refclks would be
> deleted.
> 
> This would produce one property that works for both emacs and gems, not
> change any defaults for existing devices and retain support for whatever
> devicetrees made it into the wild with your property.
> 
> Does that make sense?

If we are set to have an external and internal clock dt property than I
think we should just keep those if statements inside the !USRIO_DISABLED
code block just to keep it more organized and clean and keeping the
current HAS_CLKEN if statement be the fallback default case to set the
register bit value. Some thing like this:

if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
		if (property_present(refclk-source)) {
			if (property_read_string == "external")
				val |= true;
			else
				val |= false;
		}
		else if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
			val |= bp->usrio->refclk;

		macb_or_gem_writel(bp, USRIO, val);
}

Would this be a good middle ground?

Ryan