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]

On Wed, Mar 04, 2026 at 06:52:58PM +0000, Conor Dooley wrote:
> On Wed, Mar 04, 2026 at 09:13:39AM -0700, Ryan Wanner wrote:
> > > 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?
> 
> I explicitly would like to make HAS_CLKEN represent the USRIO having the
> bit, and not ascribe behaviour to it, so that it is actually a
> capability. This is why I want to make the default a member of the usrio
> struct in match data.
> Also, the bit position depends on the usrio struct, which we both
> omitted from our pseudo code, which means that even knowing how to
> modify val depends on having HAS_CLKEN/usrio.refclk being set, so your
> fallback doesn't sit right with me - I am trying to avoid things writing
> to USRIO without specifically being told what it is writing to is valid.

Ryan and I have discussed this a bit more, and it seems like the
USRIO_HAS_CLKEN capability flag is also being used to represent two
different "features" entirely (turning on a xceiver clock and choosing
the refclk source), depending on which device the driver has been bound
to. The next version of this series will also split this into two
different capability flags, so that each actually does what it says on
the tin.

Attachment: signature.asc
Description: PGP signature