by Colin Foster

[permalink] [raw]

Subject: Re: [RFC 4/7] mfd: ocelot-spi: Change the regmap stride to reflect the real one

On Fri, Mar 24, 2023 at 08:48:18AM -0700, Colin Foster wrote:
> Hi Maxime,
>
> On Fri, Mar 24, 2023 at 01:48:17PM +0100, Maxime Chevallier wrote:
> > Hello Andrew,
> >
> > On Fri, 24 Mar 2023 13:11:07 +0100
> > Andrew Lunn <[email protected]> wrote:
> >
> > > > .reg_bits = 24,
> > > > - .reg_stride = 4,
> > > > + .reg_stride = 1,
> > > > .reg_shift = REGMAP_DOWNSHIFT(2),
> > > > .val_bits = 32,
> > >
> > > This does not look like a bisectable change? Or did it never work
> > > before?
> >
> > Actually this works in all cases because of "regmap: check for alignment
> > on translated register addresses" in this series. Before this series,
> > I think using a stride of 1 would have worked too, as any 4-byte-aligned
> > accesses are also 1-byte aligned.
> >
> > But that's also why I need review on this, my understanding is that
> > reg_stride is used just as a check for alignment, and I couldn't test
> > this ocelot-related patch on the real HW, so please take it with a
> > grain of salt :(
>
> You're exactly right. reg_stride wasn't used anywhere in the
> ocelot-spi path before this patch series. When I build against patch 3
> ("regmap: allow upshifting register addresses before performing
> operations") ocelot-spi breaks.
>
> [ 3.207711] ocelot-soc spi0.0: error -EINVAL: Error initializing SPI bus
>
> When I build against the whole series, or even just up to patch 4 ("mfd:
> ocelot-spi: Change the regmap stride to reflect the real one")
> functionality returns.
>
> If you keep patch 4 and apply it before patch 2, everything should
> work.

I replied too soon, before looking more into patch 2.

Some context from that patch:

--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2016,7 +2016,7 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
{
int ret;

- if (!IS_ALIGNED(reg, map->reg_stride))
+ if (!IS_ALIGNED(regmap_reg_addr(map, reg), map->reg_stride))
return -EINVAL;

map->lock(map->lock_arg);

I don't know whether checking IS_ALIGNED before or after the shift is
the right thing to do. My initial intention was to perform the shift at
the last possible moment before calling into the read / write routines.
That way it wouldn't interfere with any underlying regcache mechanisms
(which aren't used by ocelot-spi)

But to me it seems like patch 2 changes this expected behavior, so the
two patches should be squashed.

... Thinking more about it ...

In ocelot-spi, at the driver layer, we're accessing two registers.
They'd be at address 0x71070000 and 0x71070004. The driver uses those
addresses, so there's a stride of 4. I can't access 0x71070001.

The fact that the translation from "address" to "bits that go out the
SPI bus" shifts out the last two bits and hacks off a couple of the MSBs
doesn't seem like it should affect the 'reg_stride'.

So maybe patches 2 and 4 should be dropped, and your patch 6
alterra_tse_main should use a reg_stride of 1? That has a subtle benefit
of not needing an additional operation or two from regmap_reg_addr().

Would that cause any issues? Hopefully there isn't something I'm
missing.

(Aside: I'm now curious how the compiler will optimize
regmap_reg_addr())

Colin

2023-03-24 18:54:52

On Thu, Mar 30, 2023 at 11:45:46AM +0200, Maxime Chevallier wrote:
> Mark Brown <[email protected]> wrote:

> > It is not at all clear to me that the combination of stride and
> > downshift particularly makes sense, and especially not that the
> > stride should be applied after downshifting rather than to what
> > the user is passing in.

> I agree on the part where the ordering of "adding and offset, then
> down/upshifting" isn't natural. This is the order in which operations
> are done today, and from what I could gather, only the ocelot-spi MFD
> driver uses both of these operations.

Right. I don't think the ordering is particularly intentional,
like I say it's not clear that combinding the two makes sense so
I think it just wasn't considered at all.

Attachments:

(No filename) (783.00 B)
signature.asc (499.00 B)
Download all attachments

2023-03-30 16:40:35

by Colin Foster

[permalink] [raw]

Subject: Re: [RFC 2/7] regmap: check for alignment on translated register addresses

Hi Maxime,

On Thu, Mar 30, 2023 at 11:45:46AM +0200, Maxime Chevallier wrote:
> Hello Mark,
>
> On Fri, 24 Mar 2023 18:51:19 +0000
> Mark Brown <[email protected]> wrote:
>
> > On Fri, Mar 24, 2023 at 10:36:39AM +0100, Maxime Chevallier wrote:
> > > With regmap->reg_base and regmap->reg_downshift, the actual register
> > > address that is going to be used for the next operation might not
> > > be the same as the one we have as an input. Addresses can be offset
> > > with reg_base and shifted, which will affect alignment.
> > >
> > > Check for alignment on the real register address.
> >
> > It is not at all clear to me that the combination of stride and
> > downshift particularly makes sense, and especially not that the
> > stride should be applied after downshifting rather than to what
> > the user is passing in.
>
> I agree on the part where the ordering of "adding and offset, then
> down/upshifting" isn't natural. This is the order in which operations
> are done today, and from what I could gather, only the ocelot-spi MFD
> driver uses both of these operations.
>
> It would indeed make sense to first shift the register to have the
> proper spacing between register addresses, then adding the offset.
>
> So maybe we should address that in ocelot-spi in the next iteration,
> Colin, would you agree ?

I'm curious what you mean by "proper spacing". The proper spacing of the
VSC7512 (ocelot-spi) is 4, and that's what the reg_stride is. All
registers can be accessed in many ways, as I described.

Consider the case I brought up, where we're trying to access a register
at 0x71070004. If you're on the processor internal to ocelot, this would
be

uint32 val = *(uint32 *)0x71070004;

Accesses to 0x71070001, 0x71070002, and 0x71070003 are not possible.
(Well maybe they are _possible_ on some architectures... you get the
point though)

Translate this to accessing the same register via SPI, where everything
is shifted down two bits. We're still accessing 0x71070004, but the way
this gets sent on the bus is 0x71070004 >> 2 = 0x1c41c001.

This patch set would have allowed accesses to 0x71070001 in the
ocelot-spi scenario, but not in the MMIO scenario. That wouldn't be
desired.

The result of all this is a consistent driver interface. "Give me
register 0x4 from a base address of 0x71070000" works in both MMIO and
SPI. Everything in /sys/kernel/debug/regmap should be the same in both
scenarios as well. All of these scenarios were the motivation behind
reg_shift.

Colin Foster

2023-04-01 14:06:19

by Vladimir Oltean

[permalink] [raw]

Subject: Re: [RFC 5/7] net: mdio: Introduce a regmap-based mdio driver

Hi Maxime,

On Fri, Mar 24, 2023 at 10:36:42AM +0100, Maxime Chevallier wrote:
> There exists several examples today of devices that embed an ethernet
> PHY or PCS directly inside an SoC. In this situation, either the device
> is controlled through a vendor-specific register set, or sometimes
> exposes the standard 802.3 registers that are typically accessed over
> MDIO.
>
> As phylib and phylink are designed to use mdiodevices, this driver
> allows creating a virtual MDIO bus, that translates mdiodev register
> accesses to regmap accesses.
>
> The reason we use regmap is because there are at least 3 such devices
> known today, 2 of them are Altera TSE PCS's, memory-mapped, exposed
> with a 4-byte stride in stmmac's dwmac-socfpga variant, and a 2-byte
> stride in altera-tse. The other one (nxp,sja1110-base-tx-mdio) is
> exposed over SPI.
>
> Signed-off-by: Maxime Chevallier <[email protected]>
> ---
> MAINTAINERS | 7 +++
> drivers/net/mdio/Kconfig | 11 +++++
> drivers/net/mdio/Makefile | 1 +
> drivers/net/mdio/mdio-regmap.c | 85 ++++++++++++++++++++++++++++++++
> include/linux/mdio/mdio-regmap.h | 25 ++++++++++
> 5 files changed, 129 insertions(+)
> create mode 100644 drivers/net/mdio/mdio-regmap.c
> create mode 100644 include/linux/mdio/mdio-regmap.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d5bc223f305..10b3a1800e0d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12751,6 +12751,13 @@ F: Documentation/devicetree/bindings/net/ieee802154/mcr20a.txt
> F: drivers/net/ieee802154/mcr20a.c
> F: drivers/net/ieee802154/mcr20a.h
>
> +MDIO REGMAP DRIVER
> +M: Maxime Chevallier <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: drivers/net/mdio/mdio-regmap.c
> +F: include/linux/mdio/mdio-regmap.h
> +
> MEASUREMENT COMPUTING CIO-DAC IIO DRIVER
> M: William Breathitt Gray <[email protected]>
> L: [email protected]
> diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
> index 90309980686e..671e4bb82e3e 100644
> --- a/drivers/net/mdio/Kconfig
> +++ b/drivers/net/mdio/Kconfig
> @@ -182,6 +182,17 @@ config MDIO_IPQ8064
> This driver supports the MDIO interface found in the network
> interface units of the IPQ8064 SoC
>
> +config MDIO_REGMAP
> + tristate "Regmap-based virtual MDIO bus driver"

IMO this shouldn't have the text visible to the Kconfig user (just
"tristate"); it should only be selectable by the driver which makes use
of the exported symbol.

AFAIK, Kconfig options which are selectable should not depend on
anything. The dependency should transfer to the options which selects
it (here REGMAP). Optionally there can be a comment specifying that the

> + depends on REGMAP
> + help
> + This driver allows using MDIO devices that are not sitting on a
> + regular MDIO bus, but still exposes the standard 802.3 register
> + layout. It's regmap-based so that it can be used on integrated,
> + memory-mapped PHYs, SPI PHYs and so on. A new virtual MDIO bus is
> + created, and its read/write operations are mapped to the underlying
> + regmap.
> +
> config MDIO_THUNDER
> tristate "ThunderX SOCs MDIO buses"
> depends on 64BIT
> diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
> index 7d4cb4c11e4e..1015f0db4531 100644
> --- a/drivers/net/mdio/Makefile
> +++ b/drivers/net/mdio/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o
> obj-$(CONFIG_MDIO_MSCC_MIIM) += mdio-mscc-miim.o
> obj-$(CONFIG_MDIO_MVUSB) += mdio-mvusb.o
> obj-$(CONFIG_MDIO_OCTEON) += mdio-octeon.o
> +obj-$(CONFIG_MDIO_REGMAP) += mdio-regmap.o
> obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o
> obj-$(CONFIG_MDIO_THUNDER) += mdio-thunder.o
> obj-$(CONFIG_MDIO_XGENE) += mdio-xgene.o
> diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
> new file mode 100644
> index 000000000000..c85d62c2f55c
> --- /dev/null
> +++ b/drivers/net/mdio/mdio-regmap.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Driver for MMIO-Mapped MDIO devices. Some IPs expose internal PHYs or PCS
> + * within the MMIO-mapped area
> + *
> + * Copyright (C) 2023 Maxime Chevallier <[email protected]>
> + */
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/mdio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_mdio.h>
> +#include <linux/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mdio/mdio-regmap.h>
> +
> +#define DRV_NAME "mdio-regmap"
> +
> +static int mdio_regmap_read_c22(struct mii_bus *bus, int addr, int regnum)
> +{
> + struct mdio_regmap_config *ctx = bus->priv;
> + unsigned int val;
> + int ret;
> +
> + if (!(ctx->valid_addr & BIT(addr)))
> + return -ENODEV;
> +
> + ret = regmap_read(ctx->regmap, regnum, &val);
> + if (ret < 0)
> + return ret;
> +
> + return val;
> +}
> +
> +static int mdio_regmap_write_c22(struct mii_bus *bus, int addr, int regnum,
> + u16 val)
> +{
> + struct mdio_regmap_config *ctx = bus->priv;
> +
> + if (!(ctx->valid_addr & BIT(addr)))
> + return -ENODEV;
> +
> + return regmap_write(ctx->regmap, regnum, val);
> +}
> +
> +struct mii_bus *devm_mdio_regmap_register(struct device *dev,
> + const struct mdio_regmap_config *config)
> +{
> + struct mdio_regmap_config *mrc;
> + struct mii_bus *mii;
> + int rc;
> +
> + if (!config->parent)
> + return ERR_PTR(-EINVAL);
> +
> + if (!config->valid_addr)
> + return ERR_PTR(-EINVAL);
> +
> + mii = devm_mdiobus_alloc_size(config->parent, sizeof(*mrc));
> + if (!mii)
> + return ERR_PTR(-ENOMEM);
> +
> + mrc = mii->priv;
> + memcpy(mrc, config, sizeof(*mrc));
> +
> + mrc->regmap = config->regmap;
> + mrc->parent = config->parent;

mrc->parent doesn't seem to be used

> + mrc->valid_addr = config->valid_addr;
> +
> + mii->name = DRV_NAME;
> + strncpy(mii->id, config->name, MII_BUS_ID_SIZE);
> + mii->parent = config->parent;
> + mii->read = mdio_regmap_read_c22;
> + mii->write = mdio_regmap_write_c22;
> +
> + rc = devm_mdiobus_register(dev, mii);
> + if (rc) {
> + dev_err(config->parent, "Cannot register MDIO bus![%s] (%d)\n", mii->id, rc);
> + return ERR_PTR(rc);
> + }
> +
> + return mii;
> +}
> +
> diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h
> new file mode 100644
> index 000000000000..ea428e5a2913
> --- /dev/null
> +++ b/include/linux/mdio/mdio-regmap.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Driver for MMIO-Mapped MDIO devices. Some IPs expose internal PHYs or PCS
> + * within the MMIO-mapped area
> + *
> + * Copyright (C) 2023 Maxime Chevallier <[email protected]>
> + */
> +#ifndef MDIO_REGMAP_H
> +#define MDIO_REGMAP_H
> +
> +#define MDIO_REGMAP_NAME 63

hmm.. NAME_LEN would be less confusing for a name. Although... why not
MII_BUS_ID_SIZE directly, seeing that this is how it is actually used?

> +
> +struct device;
> +struct regmap;
> +
> +struct mdio_regmap_config {
> + struct device *parent;
> + struct regmap *regmap;
> + char name[MDIO_REGMAP_NAME];
> + u32 valid_addr;

Would it be better to name this valid_addr_mask, since that is how it is
used? Or is the "ctx->valid_addr & BIT(addr)" check wrong (should have
been "ctx->valid_addr == addr")? How is the driver supposed to work with
multiple valid addresses?

> +};
> +
> +struct mii_bus *devm_mdio_regmap_register(struct device *dev,
> + const struct mdio_regmap_config *config);
> +
> +#endif
> --
> 2.39.2
>

2023-05-15 12:10:26

by Vladimir Oltean

[permalink] [raw]

Subject: Re: [RFC 7/7] net: pcs: Drop the TSE PCS driver

Hi Maxime,

On Fri, Mar 24, 2023 at 10:36:44AM +0100, Maxime Chevallier wrote:
> Now that we can easily create a mdio-device that represents a
> memory-mapped device that exposes an MDIO-like register layout, we don't
> need the Altera TSE PCS anymore, since we can use the Lynx PCS instead.
>
> Signed-off-by: Maxime Chevallier <[email protected]>
> ---
> -static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
> -{
> - u16 bmcr;
> -
> - /* Reset PCS block */
> - bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
> - bmcr |= BMCR_RESET;
> - tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
> -
> - return read_poll_timeout(tse_pcs_read, bmcr, (bmcr & BMCR_RESET),
> - 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
> - tse_pcs, MII_BMCR);
> -}

I just noticed this difference between the Lynx PCS and Altera TSE PCS
drivers. The Lynx driver doesn't reset the PCS, and if it did, it would
wait until the opposite condition from yours would be true: BMCR_RESET
should clear: "!(bmcr & BMCR_RESET)".

Is your reset procedure correct, I wonder?

2023-05-22 06:35:59

by Maxime Chevallier

[permalink] [raw]

Subject: Re: [RFC 7/7] net: pcs: Drop the TSE PCS driver

Hello Vlad,

On Mon, 15 May 2023 14:48:09 +0300
Vladimir Oltean <[email protected]> wrote:

> Hi Maxime,
>
> On Fri, Mar 24, 2023 at 10:36:44AM +0100, Maxime Chevallier wrote:
> > Now that we can easily create a mdio-device that represents a
> > memory-mapped device that exposes an MDIO-like register layout, we
> > don't need the Altera TSE PCS anymore, since we can use the Lynx
> > PCS instead.
> >
> > Signed-off-by: Maxime Chevallier <[email protected]>
> > ---
> > -static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
> > -{
> > - u16 bmcr;
> > -
> > - /* Reset PCS block */
> > - bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
> > - bmcr |= BMCR_RESET;
> > - tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
> > -
> > - return read_poll_timeout(tse_pcs_read, bmcr, (bmcr &
> > BMCR_RESET),
> > - 10, SGMII_PCS_SW_RESET_TIMEOUT, 1,
> > - tse_pcs, MII_BMCR);
> > -}
>
> I just noticed this difference between the Lynx PCS and Altera TSE PCS
> drivers. The Lynx driver doesn't reset the PCS, and if it did, it
> would wait until the opposite condition from yours would be true:
> BMCR_RESET should clear: "!(bmcr & BMCR_RESET)".
>
> Is your reset procedure correct, I wonder?

I also wonder... My understanding from the documentation I had is that
the PCS needed to be reset every time a major configuration was
changed, as the reset would put the internal state machines back to
their init condition, but without resetting the whole IP.
When performing my initial tests, this was indeed necessary. However
when porting to Lynx and running tests, it pretty much worked out of the
box, and I'm quite happy with it being that way. So I didn't dig much
deeper on why the reset was needed with the TSE PCS in the first place.

On a side note, the final dependencies for the final removal of TSE
were merged into the regmap tree, I'll therefore send a new version of
that series shortly.

Thanks for the review Vlad,

Best Regards,

Maxime