Re: [PATCH v2 net-next 3/3] net: dsa: ocelot: felix: utilize shared mscc-miim driver for indirect MDIO access

[Vladimir Oltean]

On Fri, Nov 26, 2021 at 11:58:32AM -0800, Colin Foster wrote:
> 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@xxxxxxxxxxxxxxxx>
> > > ---
> >
> > 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!

The T1040RDB is now unbricked. Now it hangs during early kernel boot here:

[    0.010255] clocksource: timebase mult[1aaaaaab] shift[24] registered
[    0.019577] Console: colour dummy device 80x25
[    0.024016] pid_max: default: 32768 minimum: 301
[    0.028796] Mount-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.036163] Mountpoint-cache hash table entries: 8192 (order: 4, 65536 bytes, linear)
[    0.045211] e500 family performance monitor hardware support registered
[    0.051891] rcu: Hierarchical SRCU implementation.
[    0.057791] smp: Bringing up secondary CPUs ...

At this pace, I hope we'll have something by the end of the year, because
then I'll need the space in the living room for the Christmas tree :D

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

Yeah, micro-regmaps aren't the worst thing. That prize would probably go
to the T1040RDB for the pains it's putting me through.

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

The thing with putting the targets in the device tree is that you're
inflicting yourself unnecessary pain. Take a look at
Documentation/devicetree/bindings/net/mscc-ocelot.txt, and notice that
they mark the "ptp" target as optional because it wasn't needed when
they first published the device tree, and now they need to maintain
compatibility with those old blobs. To me that is one of the sillier
reasons why you would not support PTP, because you don't know where your
registers are. And that document is not even up to date, it hasn't been
updated when VCAP ES0, IS1, IS2 were added. I don't think that Horatiu
even bothered to maintain backwards compatibility when he initially
added tc-flower offload for VCAP IS2, and as a result, I did not bother
either when extending it for the S0 and S1 targets. At some point
afterwards, the Microchip people even stopped complaining and just went
along with it. (the story is pretty much told from memory, I'm sorry if
I mixed up some facts). It's pretty messy, and that's what you get for
creating these micro-maps of registers spread through the guts of the
SoC and then a separate reg-name for each. When we worked on the device
tree for LS1028A and then T1040, it was very much a conscious decision
for the driver to have a single, big register map and split it up pretty
much in whichever way it wants to. In fact I think we wouldn't be
having the discussion about how to split things right now if we didn't
have that flexibility.

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

An offset is not all that bad TBH. I just felt like I should bring it up.
It's up to you.

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

Correct. One at a time.

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

This could work.

> So to summarize these changes for v3:
> * Create new regmaps instead of offset variables

Optional.

> * Don't expose phy_regmap in mscc_miim_setup yet?
> * Don't create GCB_PHY_PHY_CFG yet?

Yes please.