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 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.

Attachment: signature.asc
Description: PGP signature