Message-ID: <20211126195832.GA3778508@euler>
Date:   Fri, 26 Nov 2021 11:58:32 -0800
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH v2 net-next 3/3] net: dsa: ocelot: felix: utilize shared
 mscc-miim driver for indirect MDIO access

Hi Vladimir,

On Fri, Nov 26, 2021 at 12:58:25AM +0000, Vladimir Oltean wrote:
> On Thu, Nov 25, 2021 at 12:13:01PM -0800, Colin Foster wrote:
> > Switch to a shared MDIO access implementation by way of the mdio-mscc-miim
> > driver.
> > 
> > Signed-off-by: Colin Foster <colin.foster@...advantage.com>
> > ---
> 
> I'm sorry, I still wasn't able to boot the T1040RDB to test these, it
> looks like it's bricked or something. I'll try to do more debugging
> tomorrow.

No rush - I clearly have a couple things yet to work out. I appreciate
your time!

> >  
> > -	ret = regmap_read(miim->regs, MSCC_MIIM_REG_DATA, &val);
> > -
> > +	ret = regmap_read(miim->regs,
> > +			  MSCC_MIIM_REG_DATA + miim->mii_status_offset, &val);
> 
> I'd be tempted to create one separate regmap for DEVCPU_MIIM which
> starts precisely at 0x8700AC, and therefore does not need adjustment
> with an offset here. What do you think?

I've gone back and forth on this. 

My current decision is to bring around those offset variables. I
understand it is clunky - and ends up bleeding into several drivers
(pinctrl, miim, possibly some others I haven't gotten to yet...) I'll be
the first to say I don't like this architecture.

The benefit of this is we don't have several "micro-regmaps" running
around, overlapping. 

On the other hand, maybe smaller regmaps wouldn't be the worst thing. It
might make debugging pinctrl easier if I have
sys/kernel/debug/regmap/spi0.0-ocelot_spi-devcpu-gcb-gpio insetead of
just sys/kernel/debug/regmap/spi0.0-ocelot_spi-devcpu-gcb.


So while my initial thought was "don't make extra regmaps when they
aren't needed" I'm now thinking "make extra regmaps for drivers when
they make sense." It would also make behavior consistent with how the
full VSC7514 driver acts.


The last option I haven't put much consideration toward would be to
move some of the decision making to the device tree. The main ocelot
driver appears to leave a lot of these addresses out. For instance
Documentation/devicetree/bindings/pinctrl/mscc,ocelot-pinctrl.txt. 
That added DT complexity could remove needs for lines like this:
> > +			     ocelot->map[GCB][GCB_MIIM_MII_STATUS & REG_MASK],
But that would probably impose DT changes on Seville and Felix, which is
the last thing I want to do.



So at the end of the day, I'm now leaning toward creating a new, smaller
regmap space. It will be a proper subset of the GCB regmap. This would be 
applied here to mdio-mscc-miim, but also the pinctrl-ocelot (GCB:GPIO) and 
pinctrl-microchip-sgpio (GCB:SIO_CTRL) drivers as well for the 7512_spi
driver. I don't know of a better way to get the base address than the
code I referenced above. But I think that is probably the design I
dislike the least.


> 
> >  	if (ret < 0) {
> >  		WARN_ONCE(1, "mscc miim read data reg error %d\n", ret);
> >  		goto out;
> > @@ -134,7 +140,9 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
> >  	if (ret < 0)
> >  		goto out;
> >  
> > -	ret = regmap_write(miim->regs, MSCC_MIIM_REG_CMD, MSCC_MIIM_CMD_VLD |
> > +	ret = regmap_write(miim->regs,
> > +			   MSCC_MIIM_REG_CMD + miim->mii_status_offset,
> > +			   MSCC_MIIM_CMD_VLD |
> >  			   (mii_id << MSCC_MIIM_CMD_PHYAD_SHIFT) |
> >  			   (regnum << MSCC_MIIM_CMD_REGAD_SHIFT) |
> >  			   (value << MSCC_MIIM_CMD_WRDATA_SHIFT) |
> > @@ -149,16 +157,19 @@ static int mscc_miim_write(struct mii_bus *bus, int mii_id,
> >  static int mscc_miim_reset(struct mii_bus *bus)
> >  {
> >  	struct mscc_miim_dev *miim = bus->priv;
> > +	int offset = miim->phy_reset_offset;
> >  	int ret;
> >  
> >  	if (miim->phy_regs) {
> > -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0);
> > +		ret = regmap_write(miim->phy_regs,
> > +				   MSCC_PHY_REG_PHY_CFG + offset, 0);
> >  		if (ret < 0) {
> >  			WARN_ONCE(1, "mscc reset set error %d\n", ret);
> >  			return ret;
> >  		}
> >  
> > -		ret = regmap_write(miim->phy_regs, MSCC_PHY_REG_PHY_CFG, 0x1ff);
> > +		ret = regmap_write(miim->phy_regs,
> > +				   MSCC_PHY_REG_PHY_CFG + offset, 0x1ff);
> >  		if (ret < 0) {
> >  			WARN_ONCE(1, "mscc reset clear error %d\n", ret);
> >  			return ret;
> > @@ -176,8 +187,9 @@ static const struct regmap_config mscc_miim_regmap_config = {
> >  	.reg_stride	= 4,
> >  };
> >  
> > -static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> > -			   struct regmap *mii_regmap, struct regmap *phy_regmap)
> > +int mscc_miim_setup(struct device *dev, struct mii_bus **pbus, const char *name,
> > +		    struct regmap *mii_regmap, int status_offset,
> > +		    struct regmap *phy_regmap, int reset_offset)
> >  {
> >  	struct mscc_miim_dev *miim;
> >  	struct mii_bus *bus;
> > @@ -186,7 +198,7 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> >  	if (!bus)
> >  		return -ENOMEM;
> >  
> > -	bus->name = "mscc_miim";
> > +	bus->name = name;
> >  	bus->read = mscc_miim_read;
> >  	bus->write = mscc_miim_write;
> >  	bus->reset = mscc_miim_reset;
> > @@ -198,10 +210,15 @@ static int mscc_miim_setup(struct device *dev, struct mii_bus **pbus,
> >  	*pbus = bus;
> >  
> >  	miim->regs = mii_regmap;
> > +	miim->mii_status_offset = status_offset;
> >  	miim->phy_regs = phy_regmap;
> > +	miim->phy_reset_offset = reset_offset;
> 
> The reset_offset is unused. Will vsc7514_spi need it?

Yes, the SPI driver currently uses the phy_regs regmap to reset the phys
when registering the bus. I suppose it isn't necessary to expose that
for Seville right now, since Seville didn't do resetting of the phys at
this point.

> > +	GCB_PHY_PHY_CFG,
> 
> This appears extraneous, you are still using MSCC_PHY_REG_PHY_CFG.

This is related to the comment above. They're both artifacts of the
vsc7512_spi driver and aren't currently used in Seville. For the 7512
this would get defined as 0x00f0 inside vsc7512_gcb_regmap. As suggested
way above, it sounds like the direction (for vsc7512_spi) is to create
two additional regmaps. 
One that would be GCB:MIIM. Then mdio-mscc-miim.c could refer to
GCB:MIIM:MII_CMD by way of the internal MSCC_MIIM_REG_CMD macro, as an
example. 

The same would go for MSCC_PHY_REG_PHY_CFG. If the driver is to reset
the phys during initialization, a regmap at GCB:PHY could be passed in.
Then the offsets MSCC_PHY_REG_PHY_CFG and MSCC_PHY_REG_PHY_STATUS could
be referenced.


So to summarize these changes for v3:
* Create new regmaps instead of offset variables
* Don't expose phy_regmap in mscc_miim_setup yet?
* Don't create GCB_PHY_PHY_CFG yet?

Powered by blists - more mailing lists