Re: [PATCH net-next 2/3] net: stmmac: qcom-ethqos: add rgmii set/clear functions

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Nov 20, 2025 at 10:42:04AM +0100, Konrad Dybcio wrote:
> On 11/19/25 12:34 PM, Russell King (Oracle) wrote:
> > The driver has a lot of bit manipulation of the RGMII registers. Add
> > a pair of helpers to set bits and clear bits, converting the various
> > calls to rgmii_updatel() as appropriate.
> > 
> > Most of the change was done via this sed script:
> > 
> > /rgmii_updatel/ {
> > 	N
> > 	/,$/N
> > 	/mask, / ! {
> > 		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)\2,\s+|rgmii_setmask(\1|
> > 		s|rgmii_updatel\(([^,]*,\s+([^,]*),\s+)0,\s+|rgmii_clrmask(\1|
> > 		s|^\s+$||
> > 	}
> > }
> > 
> > and then formatting tweaked where necessary.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 187 +++++++++---------
> >  1 file changed, 89 insertions(+), 98 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > index ae3cf163005b..cdaf02471d3a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > @@ -137,6 +137,18 @@ static void rgmii_updatel(struct qcom_ethqos *ethqos, u32 mask, u32 val,
> >  	rgmii_writel(ethqos, temp, offset);
> >  }
> >  
> > +static void rgmii_setmask(struct qcom_ethqos *ethqos, u32 mask,
> > +			  unsigned int offset)
> > +{
> > +	rgmii_updatel(ethqos, mask, mask, offset);
> > +}
> 
> It's almost unbelieveable there's no set/clr/rmw generics for
> readl and friends

Consider what that would mean - such operations can not be atomic, but
users would likely not realise, which means we get a load of new
potential bugs. Not having these means that driver authors get to
code this up, and because they realise they have to do separate read
and write operations, it's more obvious that there may be races.

The phy_* accessors are different - these take the bus lock while they
operate, and thus are atomic.

> 
> [...]
> >  	/* Set DLL_EN */
> > -	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_EN,
> > -		      SDCC_DLL_CONFIG_DLL_EN, SDCC_HC_REG_DLL_CONFIG);
> > +	rgmii_setmask(ethqos, SDCC_DLL_CONFIG_DLL_EN,  SDCC_HC_REG_DLL_CONFIG);
> 
> double space
> 
> [...]
> 
> >  	/* Select RGMII, write 0 to interface select */
> > -	rgmii_updatel(ethqos, RGMII_CONFIG_INTF_SEL,
> > -		      0, RGMII_IO_MACRO_CONFIG);
> > +	rgmii_clrmask(ethqos, RGMII_CONFIG_INTF_SEL,  RGMII_IO_MACRO_CONFIG);
> 
> and here
> 
> Everything else looks in tact

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!