Re: [PATCH 1/4] net: stmmac: s32: use the syscon interface PHY_INTF_SEL_RGMII

[Dan Carpenter <dan.carpenter@linaro.org>]

On Mon, Dec 01, 2025 at 04:48:12PM +0000, Russell King (Oracle) wrote:
> On Mon, Dec 01, 2025 at 04:08:20PM +0300, Dan Carpenter wrote:
> > On the s32 chipset the GMAC_0_CTRL_STS register is in GPR region.
> > Originally, accessing this register was done in a sort of ad-hoc way,
> > but we want to use the syscon interface to do it.
> > 
> > This is a little bit uglier because we to maintain backwards compatibility
> > to the old device trees so we have to support both ways to access this
> > register.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  .../net/ethernet/stmicro/stmmac/dwmac-s32.c   | 23 +++++++++++++++----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> > index 5a485ee98fa7..20de761b7d28 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-s32.c
> > @@ -11,12 +11,14 @@
> >  #include <linux/device.h>
> >  #include <linux/ethtool.h>
> >  #include <linux/io.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/module.h>
> >  #include <linux/of_mdio.h>
> >  #include <linux/of_address.h>
> >  #include <linux/phy.h>
> >  #include <linux/phylink.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> >  #include <linux/stmmac.h>
> >  
> >  #include "stmmac_platform.h"
> > @@ -32,6 +34,8 @@
> >  struct s32_priv_data {
> >  	void __iomem *ioaddr;
> >  	void __iomem *ctrl_sts;
> > +	struct regmap *sts_regmap;
> > +	unsigned int sts_offset;
> >  	struct device *dev;
> >  	phy_interface_t *intf_mode;
> >  	struct clk *tx_clk;
> > @@ -40,7 +44,10 @@ struct s32_priv_data {
> >  
> >  static int s32_gmac_write_phy_intf_select(struct s32_priv_data *gmac)
> >  {
> > -	writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> > +	if (gmac->ctrl_sts)
> > +		writel(S32_PHY_INTF_SEL_RGMII, gmac->ctrl_sts);
> > +	else
> > +		regmap_write(gmac->sts_regmap, gmac->sts_offset, PHY_INTF_SEL_RGMII);
> 
> Sorry, but even if that regmap_write() is targetting the exact same
> register, these are not identical.
> 
> S32_PHY_INTF_SEL_RGMII, which is a S32-specific value, takes the value 2.
> PHY_INTF_SEL_RGMII is the dwmac specific value, and takes the value 1.
> 
> If this targets the same register, then by writing PHY_INTF_SEL_RGMII,
> you are in effect writing the equivalent of S32_PHY_INTF_SEL_SGMII to
> it. This seems like a bug.
> 

Yeah.  Sorry, I forward ported this, then back ported it, then forward
ported this again and I messed up.  :(

regards,
dan carpenter