[PATCH 0/8] mfd: ocelot: add support for MDIO managed switch

[PATCH 0/8] mfd: ocelot: add support for MDIO managed switch

[Rasmus Villemoes]

The primary purpose of this series is to add support for vsc751x
switches that are wired up to be managed via MDIO. The current MFD
driver only has support for management over SPI.

While reworking the spi and core files to allow hooking up another
underlying bus, I found what I think might be a bug in the SPI
implementation, for the case where the SPI bus is set higher than
500kHz. But as I don't have such hardware, I don't know if the bug is
real, nor am I able to test my changes.

If desired, I can drop those changes, at the expense of having to
duplicate some logic around setting up the gcb_regmap and calling
reset in the mdio backend, but since I spotted it anyway I thought I'd
include it for the first iteration.

Also, if desired, I can put the mdio and spi backends behind separate
config options, with the spi option defaulting to y to preserve
behaviour of existing .configs setting CONFIG_MFD_OCELOT.

Rasmus Villemoes (8):
  mfd: ocelot: refactor bus-specific regmap initialization
  mfd: ocelot: move SPI specific macros to ocelot-spi.c
  mfd: ocelot: rework SPI (re-)initialization after chip reset
  mfd: ocelot: lift chip reset logic to ocelot-core.c
  mfd: ocelot: make ocelot_chip_init() static
  mfd: ocelot: correct Kconfig dependency
  mfd: ocelot: enable support for mdio management
  dt-bindings: mfd: ocelot: mention MDIO management and add example

 .../devicetree/bindings/mfd/mscc,ocelot.yaml  | 121 ++++++++++++-
 drivers/mfd/Kconfig                           |   5 +-
 drivers/mfd/Makefile                          |   2 +-
 drivers/mfd/ocelot-core.c                     |  44 ++++-
 drivers/mfd/ocelot-mdio.c                     | 161 ++++++++++++++++++
 drivers/mfd/ocelot-spi.c                      |  53 ++----
 drivers/mfd/ocelot.h                          |  18 +-
 7 files changed, 339 insertions(+), 65 deletions(-)
 create mode 100644 drivers/mfd/ocelot-mdio.c

-- 
2.49.0

[PATCH 1/8] mfd: ocelot: refactor bus-specific regmap initialization

[Rasmus Villemoes]

Make ocelot-core truly bus-agnostic by letting the bus-specific part
set an ->init_regmap callback in struct ocelot_ddata, instead of
relying on the bus being spi.

With this, the only symbol in the MFD_OCELOT_SPI namespace vanishes,
and hence ocelot-core should no longer import that.

This is preparation for adding support for mdio-based management of
the Ocelot chip.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 drivers/mfd/ocelot-core.c | 5 +++--
 drivers/mfd/ocelot-spi.c  | 4 ++--
 drivers/mfd/ocelot.h      | 6 ++----
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 41aff27088548..78b5fe15efdd2 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -200,10 +200,12 @@ static const struct mfd_cell vsc7512_devs[] = {
 static void ocelot_core_try_add_regmap(struct device *dev,
 				       const struct resource *res)
 {
+	struct ocelot_ddata *ddata = dev_get_drvdata(dev);
+
 	if (dev_get_regmap(dev, res->name))
 		return;
 
-	ocelot_spi_init_regmap(dev, res);
+	ddata->init_regmap(dev, res);
 }
 
 static void ocelot_core_try_add_regmaps(struct device *dev,
@@ -231,4 +233,3 @@ EXPORT_SYMBOL_NS(ocelot_core_init, "MFD_OCELOT");
 MODULE_DESCRIPTION("Externally Controlled Ocelot Chip Driver");
 MODULE_AUTHOR("Colin Foster <colin.foster@in-advantage.com>");
 MODULE_LICENSE("GPL");
-MODULE_IMPORT_NS("MFD_OCELOT_SPI");
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 1fed9878c3231..a320a613d00e1 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -181,7 +181,7 @@ static const struct regmap_bus ocelot_spi_regmap_bus = {
 	.read = ocelot_spi_regmap_bus_read,
 };
 
-struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource *res)
+static struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource *res)
 {
 	struct regmap_config regmap_config;
 
@@ -193,7 +193,6 @@ struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource
 
 	return devm_regmap_init(dev, &ocelot_spi_regmap_bus, dev, &regmap_config);
 }
-EXPORT_SYMBOL_NS(ocelot_spi_init_regmap, "MFD_OCELOT_SPI");
 
 static int ocelot_spi_probe(struct spi_device *spi)
 {
@@ -207,6 +206,7 @@ static int ocelot_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	spi_set_drvdata(spi, ddata);
+	ddata->init_regmap = ocelot_spi_init_regmap;
 
 	if (spi->max_speed_hz <= 500000) {
 		ddata->spi_padding_bytes = 0;
diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
index b8bc2f1486e24..4305e7a55cb1a 100644
--- a/drivers/mfd/ocelot.h
+++ b/drivers/mfd/ocelot.h
@@ -12,6 +12,7 @@ struct resource;
 
 /**
  * struct ocelot_ddata - Private data for an external Ocelot chip
+ * @init_regmap:	Bus-specific callback for initializing regmap.
  * @gcb_regmap:		General Configuration Block regmap. Used for
  *			operations like chip reset.
  * @cpuorg_regmap:	CPU Device Origin Block regmap. Used for operations
@@ -24,6 +25,7 @@ struct resource;
  *			data of a SPI read operation.
  */
 struct ocelot_ddata {
+	struct regmap * (*init_regmap)(struct device *dev, const struct resource *res);
 	struct regmap *gcb_regmap;
 	struct regmap *cpuorg_regmap;
 	int spi_padding_bytes;
@@ -33,10 +35,6 @@ struct ocelot_ddata {
 int ocelot_chip_reset(struct device *dev);
 int ocelot_core_init(struct device *dev);
 
-/* SPI-specific routines that won't be necessary for other interfaces */
-struct regmap *ocelot_spi_init_regmap(struct device *dev,
-				      const struct resource *res);
-
 #define OCELOT_SPI_BYTE_ORDER_LE 0x00000000
 #define OCELOT_SPI_BYTE_ORDER_BE 0x81818181
 
-- 
2.49.0

Re: [PATCH 1/8] mfd: ocelot: refactor bus-specific regmap initialization

[Colin Foster]

On Wed, Mar 19, 2025 at 01:30:51PM +0100, Rasmus Villemoes wrote:
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index 41aff27088548..78b5fe15efdd2 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -200,10 +200,12 @@ static const struct mfd_cell vsc7512_devs[] = {
>  static void ocelot_core_try_add_regmap(struct device *dev,
>                                        const struct resource *res)
>  {
> +       struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> +
>         if (dev_get_regmap(dev, res->name))
>                 return;
> 
> -       ocelot_spi_init_regmap(dev, res);
> +       ddata->init_regmap(dev, res);

I remember changing this from function pointers to the direct function
call during initial development, per Lee's suggestion. I like it though,
and I'm glad to see multiple users now.

Reviewed-by: Colin Foster <colin.foster@in-advantage.com>
Tested-by: Colin Foster <colin.foster@in-advantage.com>

Re: [PATCH 1/8] mfd: ocelot: refactor bus-specific regmap initialization

[Lee Jones]

On Wed, 19 Mar 2025, Colin Foster wrote:

> On Wed, Mar 19, 2025 at 01:30:51PM +0100, Rasmus Villemoes wrote:
> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> > index 41aff27088548..78b5fe15efdd2 100644
> > --- a/drivers/mfd/ocelot-core.c
> > +++ b/drivers/mfd/ocelot-core.c
> > @@ -200,10 +200,12 @@ static const struct mfd_cell vsc7512_devs[] = {
> >  static void ocelot_core_try_add_regmap(struct device *dev,
> >                                        const struct resource *res)
> >  {
> > +       struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> > +
> >         if (dev_get_regmap(dev, res->name))
> >                 return;
> > 
> > -       ocelot_spi_init_regmap(dev, res);
> > +       ddata->init_regmap(dev, res);
> 
> I remember changing this from function pointers to the direct function
> call during initial development, per Lee's suggestion. I like it though,
> and I'm glad to see multiple users now.

Yeah, we're still not going to be putting call-backs into device data.

Either pass the differentiating config through to the core driver or
handle the differentiation inside the *-i2c.c / *-spi.c files.

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/8] mfd: ocelot: refactor bus-specific regmap initialization

[Rasmus Villemoes]

On Fri, Mar 21 2025, Lee Jones <lee@kernel.org> wrote:

> On Wed, 19 Mar 2025, Colin Foster wrote:
>
>> On Wed, Mar 19, 2025 at 01:30:51PM +0100, Rasmus Villemoes wrote:
>> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
>> > index 41aff27088548..78b5fe15efdd2 100644
>> > --- a/drivers/mfd/ocelot-core.c
>> > +++ b/drivers/mfd/ocelot-core.c
>> > @@ -200,10 +200,12 @@ static const struct mfd_cell vsc7512_devs[] = {
>> >  static void ocelot_core_try_add_regmap(struct device *dev,
>> >                                        const struct resource *res)
>> >  {
>> > +       struct ocelot_ddata *ddata = dev_get_drvdata(dev);
>> > +
>> >         if (dev_get_regmap(dev, res->name))
>> >                 return;
>> > 
>> > -       ocelot_spi_init_regmap(dev, res);
>> > +       ddata->init_regmap(dev, res);
>> 
>> I remember changing this from function pointers to the direct function
>> call during initial development, per Lee's suggestion. I like it though,
>> and I'm glad to see multiple users now.
>
> Yeah, we're still not going to be putting call-backs into device data.

OK. Can you explain why that is such a bad design?

> Either pass the differentiating config through to the core driver

So you mean something like defining a new struct ocelot_backend_ops { } with
those function pointers, and pass an instance of that to
ocelot_core_init (and from there down to the static helper functions)?
That should be doable.

> or handle the differentiation inside the *-i2c.c / *-spi.c files.

I really fail to see how that could be done. Currently, the core file
has a hard-coded call of ocelot_spi_init_regmap(). I don't suppose you
mean to teach that function to realize "hey, this struct device is not
really a struct spi_device, let's delegate to ocelot_mdio_init_regmap()
instead". So _somehow_ the core will need to know to call one or the
other init_regmap implementation. I could add some "enum ocelot_type"
and switch on that and then call the appropriate bus-specific function,
but that's morally equivalent to having the function pointers.

Thanks,
Rasmus

Re: [PATCH 1/8] mfd: ocelot: refactor bus-specific regmap initialization

[Lee Jones]

On Fri, 21 Mar 2025, Rasmus Villemoes wrote:

> On Fri, Mar 21 2025, Lee Jones <lee@kernel.org> wrote:
> 
> > On Wed, 19 Mar 2025, Colin Foster wrote:
> >
> >> On Wed, Mar 19, 2025 at 01:30:51PM +0100, Rasmus Villemoes wrote:
> >> > diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> >> > index 41aff27088548..78b5fe15efdd2 100644
> >> > --- a/drivers/mfd/ocelot-core.c
> >> > +++ b/drivers/mfd/ocelot-core.c
> >> > @@ -200,10 +200,12 @@ static const struct mfd_cell vsc7512_devs[] = {
> >> >  static void ocelot_core_try_add_regmap(struct device *dev,
> >> >                                        const struct resource *res)
> >> >  {
> >> > +       struct ocelot_ddata *ddata = dev_get_drvdata(dev);
> >> > +
> >> >         if (dev_get_regmap(dev, res->name))
> >> >                 return;
> >> > 
> >> > -       ocelot_spi_init_regmap(dev, res);
> >> > +       ddata->init_regmap(dev, res);
> >> 
> >> I remember changing this from function pointers to the direct function
> >> call during initial development, per Lee's suggestion. I like it though,
> >> and I'm glad to see multiple users now.
> >
> > Yeah, we're still not going to be putting call-backs into device data.
> 
> OK. Can you explain why that is such a bad design?

It opens things up for all kinds of other 'cleverness' (a.k.a. hackery).

Save call-backs for things like class-level ops, not driver level hacks.

> > Either pass the differentiating config through to the core driver
> 
> So you mean something like defining a new struct ocelot_backend_ops { } with
> those function pointers, and pass an instance of that to
> ocelot_core_init (and from there down to the static helper functions)?
> That should be doable.

No call-backs at all.  Pass a pointer to the Regmap data.

> > or handle the differentiation inside the *-i2c.c / *-spi.c files.
> 
> I really fail to see how that could be done. Currently, the core file
> has a hard-coded call of ocelot_spi_init_regmap(). I don't suppose you
> mean to teach that function to realize "hey, this struct device is not
> really a struct spi_device, let's delegate to ocelot_mdio_init_regmap()
> instead". So _somehow_ the core will need to know to call one or the
> other init_regmap implementation. I could add some "enum ocelot_type"
> and switch on that and then call the appropriate bus-specific function,
> but that's morally equivalent to having the function pointers.

Enums are acceptable for this use-case.  Opaque function pointers that
are troublesome to debug / read without; running the code, printing
pointers then conducting a system table look-up, are not.

It's not that function pointers wouldn't work perfectly well in this
scenario.  The issue is the precedent it (or any of the previous
attempts to do this) would set and the chaos that would follow.

-- 
Lee Jones [李琼斯]

[PATCH 2/8] mfd: ocelot: move SPI specific macros to ocelot-spi.c

[Rasmus Villemoes]

These are only used in and relevant to the SPI backend.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 drivers/mfd/ocelot-spi.c | 9 +++++++++
 drivers/mfd/ocelot.h     | 9 ---------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index a320a613d00e1..97e4061e3dff7 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -41,6 +41,15 @@
 #define VSC7512_CHIP_REGS_RES_START	0x71070000
 #define VSC7512_CHIP_REGS_RES_SIZE	0x14
 
+#define OCELOT_SPI_BYTE_ORDER_LE 0x00000000
+#define OCELOT_SPI_BYTE_ORDER_BE 0x81818181
+
+#ifdef __LITTLE_ENDIAN
+#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_LE
+#else
+#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_BE
+#endif
+
 static const struct resource vsc7512_dev_cpuorg_resource =
 	DEFINE_RES_REG_NAMED(VSC7512_DEVCPU_ORG_RES_START,
 			     VSC7512_DEVCPU_ORG_RES_SIZE,
diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
index 4305e7a55cb1a..4f602611a5a86 100644
--- a/drivers/mfd/ocelot.h
+++ b/drivers/mfd/ocelot.h
@@ -35,13 +35,4 @@ struct ocelot_ddata {
 int ocelot_chip_reset(struct device *dev);
 int ocelot_core_init(struct device *dev);
 
-#define OCELOT_SPI_BYTE_ORDER_LE 0x00000000
-#define OCELOT_SPI_BYTE_ORDER_BE 0x81818181
-
-#ifdef __LITTLE_ENDIAN
-#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_LE
-#else
-#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_BE
-#endif
-
 #endif
-- 
2.49.0

Re: [PATCH 2/8] mfd: ocelot: move SPI specific macros to ocelot-spi.c

[Colin Foster]

On Wed, Mar 19, 2025 at 01:30:52PM +0100, Rasmus Villemoes wrote:
> [You don't often get email from ravi@prevas.dk. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> These are only used in and relevant to the SPI backend.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Colin Foster <colin.foster@in-advantage.com>

[PATCH 3/8] mfd: ocelot: rework SPI (re-)initialization after chip reset

[Rasmus Villemoes]

As the comments in ocelot-spi.c explain, after a chip reset, the
CFGSTAT register must be written again setting the appropriate number
of padding bytes; otherwise reads are not reliable.

However, the way the code is currently structured violates that: After
the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
enters a readx_poll_timeout().

Since the code in ocelot-spi.c implementing those reads still insert
the padding bytes in the spi transfer, any read done after the reset
is effected, including the "final" read that should confirm the
self-clearing has happened, is most likely garbage:

Say we are using 1 padding byte; after the
reset is completed, the device doesn't wait for 8 cycles before
sending data (because CFGSTAT.IF_CFG has been reset to 0), so the
result is going to be something like 24 bits from the actual register,
shifted 8 bits, followed by 8 bits of whatever state the MISO line is
in when not being driven explicitly.

Now, some of the registers blocks (DEVCPU_QS and DEVCPU_ORG) are
documented to have access times of 0.1us, but the reset register is
located in DEVCPU_GCB, thus has access time 1us, so it seems there is
no way around doing the reads of the SOFT_RST register with the
correct value set in CFGSTAT.IF_CFG.

So rework the code to let the underlying bus define an ->init_bus
callback that the core can call after setting the soft reset bit, and
before polling that bit for being clear.

Note that we do have somewhat of a catch-22: We cannot read back the
REG_GCB_SOFT_RST register to know if reset is completed until
CFGSTAT.IF_CFG is reinitialized, but even if we are always allowed to
write to that register, if it is possible to write it "too soon",
before reset is completed, that re-initialization might be in
vain. The data sheet is unfortunately silent on how much time a soft
reset might take, and I simply assume that the re-initialization can
be done after 100us.

This also serves as preparation for implementing mdio management,
lifting some of the initialization steps from ocelot-spi.c to
ocelot-core.c.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 drivers/mfd/ocelot-core.c | 8 ++++++++
 drivers/mfd/ocelot-spi.c  | 9 +--------
 drivers/mfd/ocelot.h      | 2 ++
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 78b5fe15efdd2..9caab83138e59 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -110,6 +110,14 @@ int ocelot_chip_reset(struct device *dev)
 	if (ret)
 		return ret;
 
+	if (ddata->init_bus) {
+		fsleep(VSC7512_GCB_RST_SLEEP_US);
+		ret = ddata->init_bus(dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Error initializing bus after reset\n");
+	}
+
 	return readx_poll_timeout(ocelot_gcb_chip_rst_status, ddata, val, !val,
 				  VSC7512_GCB_RST_SLEEP_US, VSC7512_GCB_RST_TIMEOUT_US);
 }
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 97e4061e3dff7..37828dd3ee95e 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -215,6 +215,7 @@ static int ocelot_spi_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	spi_set_drvdata(spi, ddata);
+	ddata->init_bus = ocelot_spi_initialize;
 	ddata->init_regmap = ocelot_spi_init_regmap;
 
 	if (spi->max_speed_hz <= 500000) {
@@ -264,14 +265,6 @@ static int ocelot_spi_probe(struct spi_device *spi)
 	if (err)
 		return dev_err_probe(dev, err, "Error resetting device\n");
 
-	/*
-	 * A chip reset will clear the SPI configuration, so it needs to be done
-	 * again before we can access any registers.
-	 */
-	err = ocelot_spi_initialize(dev);
-	if (err)
-		return dev_err_probe(dev, err, "Error initializing SPI bus after reset\n");
-
 	err = ocelot_core_init(dev);
 	if (err)
 		return dev_err_probe(dev, err, "Error initializing Ocelot core\n");
diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
index 4f602611a5a86..5aa6589b9038e 100644
--- a/drivers/mfd/ocelot.h
+++ b/drivers/mfd/ocelot.h
@@ -12,6 +12,7 @@ struct resource;
 
 /**
  * struct ocelot_ddata - Private data for an external Ocelot chip
+ * @init_bus:		Bus-specific initialization callback (optional).
  * @init_regmap:	Bus-specific callback for initializing regmap.
  * @gcb_regmap:		General Configuration Block regmap. Used for
  *			operations like chip reset.
@@ -25,6 +26,7 @@ struct resource;
  *			data of a SPI read operation.
  */
 struct ocelot_ddata {
+	int (*init_bus)(struct device *dev);
 	struct regmap * (*init_regmap)(struct device *dev, const struct resource *res);
 	struct regmap *gcb_regmap;
 	struct regmap *cpuorg_regmap;
-- 
2.49.0

Re: [PATCH 3/8] mfd: ocelot: rework SPI (re-)initialization after chip reset

[Colin Foster]

Hi Rasmus,

On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote:
> As the comments in ocelot-spi.c explain, after a chip reset, the
> CFGSTAT register must be written again setting the appropriate number
> of padding bytes; otherwise reads are not reliable.
> 
> However, the way the code is currently structured violates that: After
> the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
> enters a readx_poll_timeout().

I ran this new version and everything worked - and I've not seen an
issue in previous versions. I'm looking for guidance as to whether this
should include a Fixes tag and be backported.

Great find, by the way! Is there any information you would like from my
setup? I'm happy to add a reviewed tag, and my "tested" would be that I
ran it.

Colin Foster

Re: [PATCH 3/8] mfd: ocelot: rework SPI (re-)initialization after chip reset

[Rasmus Villemoes]

Hi Colin

On Wed, Mar 19 2025, Colin Foster <colin.foster@in-advantage.com> wrote:

> On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote:
>> As the comments in ocelot-spi.c explain, after a chip reset, the
>> CFGSTAT register must be written again setting the appropriate number
>> of padding bytes; otherwise reads are not reliable.
>> 
>> However, the way the code is currently structured violates that: After
>> the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
>> enters a readx_poll_timeout().
>
> I ran this new version and everything worked - and I've not seen an
> issue in previous versions. I'm looking for guidance as to whether this
> should include a Fixes tag and be backported.

Thanks a lot for testing and reviewing! As for backporting, IDK, I think
we'd at least first have to know that it really fixes a bug for somebody.

> Great find, by the way! Is there any information you would like from my
> setup?

Certainly I'd like to know if you do in fact use a SPI clock > 500 kHz?

And if so, could you try inserting a read and printk of e.g. CHIP_REGS.CHIP_ID
immediately after the fsleep(), but before the re-initialization, just
so we can see if my theory that the values are off-by-8-bits plus 8 bits
of MISO "garbage" is correct? Because that register should have a fairly
easily recognizable value.

Thanks,
Rasmus

Re: [PATCH 3/8] mfd: ocelot: rework SPI (re-)initialization after chip reset

[Colin Foster]

On Thu, Mar 20, 2025 at 12:17:37PM +0100, Rasmus Villemoes wrote:
> Hi Colin
> 
> On Wed, Mar 19 2025, Colin Foster <colin.foster@in-advantage.com> wrote:
> 
> > On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote:
> >> As the comments in ocelot-spi.c explain, after a chip reset, the
> >> CFGSTAT register must be written again setting the appropriate number
> >> of padding bytes; otherwise reads are not reliable.
> >> 
> >> However, the way the code is currently structured violates that: After
> >> the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
> >> enters a readx_poll_timeout().
> >
> > I ran this new version and everything worked - and I've not seen an
> > issue in previous versions. I'm looking for guidance as to whether this
> > should include a Fixes tag and be backported.
> 
> Thanks a lot for testing and reviewing! As for backporting, IDK, I think
> we'd at least first have to know that it really fixes a bug for somebody.
> 
> > Great find, by the way! Is there any information you would like from my
> > setup?
> 
> Certainly I'd like to know if you do in fact use a SPI clock > 500 kHz?

Yep, looks like 2.5MHz

&spi0 {
        #address-cells = <1>;
        #size-cells = <0>;
        status = "okay";

        soc@0 {
                compatible = "mscc,vsc7512";
                spi-max-frequency = <2500000>;

> 
> And if so, could you try inserting a read and printk of e.g. CHIP_REGS.CHIP_ID
> immediately after the fsleep(), but before the re-initialization, just
> so we can see if my theory that the values are off-by-8-bits plus 8 bits
> of MISO "garbage" is correct? Because that register should have a fairly
> easily recognizable value.

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index c00d30dbfca8..5a2762b6ecac 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -115,6 +115,8 @@ static int ocelot_chip_reset(struct device *dev)

        if (ddata->init_bus) {
                fsleep(VSC7512_GCB_RST_SLEEP_US);
+               regmap_read(ddata->gcb_regmap, 0, &val);
+               printk("7512 Chip ID after sleep: 0x%08x\n", val);
                ret = ddata->init_bus(dev);
                if (ret)
                        return dev_err_probe(dev, ret,


Prints out this:

[    3.360986] 7512 Chip ID after sleep: 0xf0e94051

That doesn't seem right. I added a print after init and it makes more sense.

[    3.351656] 7512 Chip ID after sleep: 0xf0e94051
[    3.356828] 7512 Chip ID after init: 0x175140e9

That looks better. Good find!


Colin Foster

Re: [PATCH 3/8] mfd: ocelot: rework SPI (re-)initialization after chip reset

[Rasmus Villemoes]

On Sat, Mar 22 2025, Colin Foster <colin.foster@in-advantage.com> wrote:

> On Thu, Mar 20, 2025 at 12:17:37PM +0100, Rasmus Villemoes wrote:
>> Hi Colin
>> 
>> On Wed, Mar 19 2025, Colin Foster <colin.foster@in-advantage.com> wrote:
>> 
>> > On Wed, Mar 19, 2025 at 01:30:53PM +0100, Rasmus Villemoes wrote:
>> >> As the comments in ocelot-spi.c explain, after a chip reset, the
>> >> CFGSTAT register must be written again setting the appropriate number
>> >> of padding bytes; otherwise reads are not reliable.
>> >> 
>> >> However, the way the code is currently structured violates that: After
>> >> the BIT_SOFT_CHIP_RST is written, ocelot_chip_reset() immediately
>> >> enters a readx_poll_timeout().
>> >
>> > I ran this new version and everything worked - and I've not seen an
>> > issue in previous versions. I'm looking for guidance as to whether this
>> > should include a Fixes tag and be backported.
>> 
>> Thanks a lot for testing and reviewing! As for backporting, IDK, I think
>> we'd at least first have to know that it really fixes a bug for somebody.
>> 
>> > Great find, by the way! Is there any information you would like from my
>> > setup?
>> 
>> Certainly I'd like to know if you do in fact use a SPI clock > 500 kHz?
>
> Yep, looks like 2.5MHz
>
> &spi0 {
>         #address-cells = <1>;
>         #size-cells = <0>;
>         status = "okay";
>
>         soc@0 {
>                 compatible = "mscc,vsc7512";
>                 spi-max-frequency = <2500000>;
>
>> 
>> And if so, could you try inserting a read and printk of e.g. CHIP_REGS.CHIP_ID
>> immediately after the fsleep(), but before the re-initialization, just
>> so we can see if my theory that the values are off-by-8-bits plus 8 bits
>> of MISO "garbage" is correct? Because that register should have a fairly
>> easily recognizable value.
>
> diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
> index c00d30dbfca8..5a2762b6ecac 100644
> --- a/drivers/mfd/ocelot-core.c
> +++ b/drivers/mfd/ocelot-core.c
> @@ -115,6 +115,8 @@ static int ocelot_chip_reset(struct device *dev)
>
>         if (ddata->init_bus) {
>                 fsleep(VSC7512_GCB_RST_SLEEP_US);
> +               regmap_read(ddata->gcb_regmap, 0, &val);
> +               printk("7512 Chip ID after sleep: 0x%08x\n", val);
>                 ret = ddata->init_bus(dev);
>                 if (ret)
>                         return dev_err_probe(dev, ret,
>
>
> Prints out this:
>
> [    3.360986] 7512 Chip ID after sleep: 0xf0e94051
>
> That doesn't seem right. I added a print after init and it makes more sense.
>
> [    3.351656] 7512 Chip ID after sleep: 0xf0e94051
> [    3.356828] 7512 Chip ID after init: 0x175140e9

Thanks for testing. I hadn't realized that another thing the spi bus init
does is setting the endianness, but this clearly shows both the
off-by-one-byte and that the bytes are sent in the wrong order.

It's hard to know how you end up with that f0 garbage byte, I'd assume
either all-1s or all-0s when MISO is no longer driven explicitly. A wild
guess could be that it's leftover capacitance (the last actually-driven
bit is 1), which could explain why you haven't had a problem when
reading the reset register and expected all zeroes, because in that case
the device only sends 0s, and thus the garbage byte ends up also being a
0x00.

So yes, it does seem like this warrants a backport. I'll add a Fixes tag
for the next iteration, plus a link to this thread which demonstrates
the problem. I suppose this goes back to f3e89362.

Thanks,
Rasmus

[PATCH 4/8] mfd: ocelot: lift chip reset logic to ocelot-core.c

[Rasmus Villemoes]

As further preparation for implementing support for mdio management,
lift the initialization of the ->gcb_regmap, the initial (and
optional) bus initialization callback, and the call of
ocelot_chip_reset() to ocelot_core_init().

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 drivers/mfd/ocelot-core.c | 28 +++++++++++++++++++++++++++-
 drivers/mfd/ocelot-spi.c  | 33 +--------------------------------
 2 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index 9caab83138e59..c0ab5492c83f9 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -28,6 +28,9 @@
 
 #include "ocelot.h"
 
+#define VSC7512_CHIP_REGS_RES_START	0x71070000
+#define VSC7512_CHIP_REGS_RES_SIZE	0x14
+
 #define REG_GCB_SOFT_RST		0x0008
 
 #define BIT_SOFT_CHIP_RST		BIT(0)
@@ -123,6 +126,11 @@ int ocelot_chip_reset(struct device *dev)
 }
 EXPORT_SYMBOL_NS(ocelot_chip_reset, "MFD_OCELOT");
 
+static const struct resource vsc7512_gcb_resource =
+	DEFINE_RES_REG_NAMED(VSC7512_CHIP_REGS_RES_START,
+			     VSC7512_CHIP_REGS_RES_SIZE,
+			     "devcpu_gcb_chip_regs");
+
 static const struct resource vsc7512_miim0_resources[] = {
 	DEFINE_RES_REG_NAMED(VSC7512_MIIM0_RES_START, VSC7512_MIIM_RES_SIZE, "gcb_miim0"),
 	DEFINE_RES_REG_NAMED(VSC7512_PHY_RES_START, VSC7512_PHY_RES_SIZE, "gcb_phy"),
@@ -227,7 +235,25 @@ static void ocelot_core_try_add_regmaps(struct device *dev,
 
 int ocelot_core_init(struct device *dev)
 {
-	int i, ndevs;
+	struct ocelot_ddata *ddata = dev_get_drvdata(dev);
+	struct regmap *r;
+	int i, ndevs, err;
+
+	r = ddata->init_regmap(dev, &vsc7512_gcb_resource);
+	if (IS_ERR(r))
+		return PTR_ERR(r);
+
+	ddata->gcb_regmap = r;
+
+	if (ddata->init_bus) {
+		err = ddata->init_bus(dev);
+		if (err)
+			return dev_err_probe(dev, err, "Error initializing bus\n");
+	}
+
+	err = ocelot_chip_reset(dev);
+	if (err)
+		return dev_err_probe(dev, err, "Error resetting device\n");
 
 	ndevs = ARRAY_SIZE(vsc7512_devs);
 
diff --git a/drivers/mfd/ocelot-spi.c b/drivers/mfd/ocelot-spi.c
index 37828dd3ee95e..1844b8451e8e7 100644
--- a/drivers/mfd/ocelot-spi.c
+++ b/drivers/mfd/ocelot-spi.c
@@ -38,9 +38,6 @@
 #define VSC7512_DEVCPU_ORG_RES_START	0x71000000
 #define VSC7512_DEVCPU_ORG_RES_SIZE	0x38
 
-#define VSC7512_CHIP_REGS_RES_START	0x71070000
-#define VSC7512_CHIP_REGS_RES_SIZE	0x14
-
 #define OCELOT_SPI_BYTE_ORDER_LE 0x00000000
 #define OCELOT_SPI_BYTE_ORDER_BE 0x81818181
 
@@ -55,11 +52,6 @@ static const struct resource vsc7512_dev_cpuorg_resource =
 			     VSC7512_DEVCPU_ORG_RES_SIZE,
 			     "devcpu_org");
 
-static const struct resource vsc7512_gcb_resource =
-	DEFINE_RES_REG_NAMED(VSC7512_CHIP_REGS_RES_START,
-			     VSC7512_CHIP_REGS_RES_SIZE,
-			     "devcpu_gcb_chip_regs");
-
 static int ocelot_spi_initialize(struct device *dev)
 {
 	struct ocelot_ddata *ddata = dev_get_drvdata(dev);
@@ -246,30 +238,7 @@ static int ocelot_spi_probe(struct spi_device *spi)
 
 	ddata->cpuorg_regmap = r;
 
-	r = ocelot_spi_init_regmap(dev, &vsc7512_gcb_resource);
-	if (IS_ERR(r))
-		return PTR_ERR(r);
-
-	ddata->gcb_regmap = r;
-
-	/*
-	 * The chip must be set up for SPI before it gets initialized and reset.
-	 * This must be done before calling init, and after a chip reset is
-	 * performed.
-	 */
-	err = ocelot_spi_initialize(dev);
-	if (err)
-		return dev_err_probe(dev, err, "Error initializing SPI bus\n");
-
-	err = ocelot_chip_reset(dev);
-	if (err)
-		return dev_err_probe(dev, err, "Error resetting device\n");
-
-	err = ocelot_core_init(dev);
-	if (err)
-		return dev_err_probe(dev, err, "Error initializing Ocelot core\n");
-
-	return 0;
+	return ocelot_core_init(dev);
 }
 
 static const struct spi_device_id ocelot_spi_ids[] = {
-- 
2.49.0

Re: [PATCH 4/8] mfd: ocelot: lift chip reset logic to ocelot-core.c

[Colin Foster]

On Wed, Mar 19, 2025 at 01:30:54PM +0100, Rasmus Villemoes wrote:
> As further preparation for implementing support for mdio management,
> lift the initialization of the ->gcb_regmap, the initial (and
> optional) bus initialization callback, and the call of
> ocelot_chip_reset() to ocelot_core_init().
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Colin Foster <colin.foster@in-advantage.com>

[PATCH 5/8] mfd: ocelot: make ocelot_chip_init() static

[Rasmus Villemoes]

Now only called from within ocelot-core.c.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 drivers/mfd/ocelot-core.c | 3 +--
 drivers/mfd/ocelot.h      | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mfd/ocelot-core.c b/drivers/mfd/ocelot-core.c
index c0ab5492c83f9..c00d30dbfca82 100644
--- a/drivers/mfd/ocelot-core.c
+++ b/drivers/mfd/ocelot-core.c
@@ -98,7 +98,7 @@ static int ocelot_gcb_chip_rst_status(struct ocelot_ddata *ddata)
 	return val;
 }
 
-int ocelot_chip_reset(struct device *dev)
+static int ocelot_chip_reset(struct device *dev)
 {
 	struct ocelot_ddata *ddata = dev_get_drvdata(dev);
 	int ret, val;
@@ -124,7 +124,6 @@ int ocelot_chip_reset(struct device *dev)
 	return readx_poll_timeout(ocelot_gcb_chip_rst_status, ddata, val, !val,
 				  VSC7512_GCB_RST_SLEEP_US, VSC7512_GCB_RST_TIMEOUT_US);
 }
-EXPORT_SYMBOL_NS(ocelot_chip_reset, "MFD_OCELOT");
 
 static const struct resource vsc7512_gcb_resource =
 	DEFINE_RES_REG_NAMED(VSC7512_CHIP_REGS_RES_START,
diff --git a/drivers/mfd/ocelot.h b/drivers/mfd/ocelot.h
index 5aa6589b9038e..f1272a359ef47 100644
--- a/drivers/mfd/ocelot.h
+++ b/drivers/mfd/ocelot.h
@@ -34,7 +34,6 @@ struct ocelot_ddata {
 	void *dummy_buf;
 };
 
-int ocelot_chip_reset(struct device *dev);
 int ocelot_core_init(struct device *dev);
 
 #endif
-- 
2.49.0

Re: [PATCH 5/8] mfd: ocelot: make ocelot_chip_init() static

[Colin Foster]

On Wed, Mar 19, 2025 at 01:30:55PM +0100, Rasmus Villemoes wrote:
> Now only called from within ocelot-core.c.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

Reviewed-by: Colin Foster <colin.foster@in-advantage.com>

[PATCH 6/8] mfd: ocelot: correct Kconfig dependency

[Rasmus Villemoes]

The ocelot-spi.c file does not actually use the generic spi regmap
support (i.e., it does not call any variant of regmap_init_spi), but
instead implements its own specialized regmap (though of course with
spi as the underlying bus). So it should simply 'select REGMAP'.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 drivers/mfd/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6b0682af6e32b..4dc894061b62e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1049,7 +1049,7 @@ config MFD_OCELOT
 	tristate "Microsemi Ocelot External Control Support"
 	depends on SPI_MASTER
 	select MFD_CORE
-	select REGMAP_SPI
+	select REGMAP
 	help
 	  Ocelot is a family of networking chips that support multiple ethernet
 	  and fibre interfaces. In addition to networking, they contain several
-- 
2.49.0

Re: [PATCH 6/8] mfd: ocelot: correct Kconfig dependency

[Colin Foster]

On Wed, Mar 19, 2025 at 01:30:56PM +0100, Rasmus Villemoes wrote:
> The ocelot-spi.c file does not actually use the generic spi regmap
> support (i.e., it does not call any variant of regmap_init_spi), but
> instead implements its own specialized regmap (though of course with
> spi as the underlying bus). So it should simply 'select REGMAP'.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>

This might have gone back and forth a few times during development. Good
find!

Reviewed-by: Colin Foster <colin.foster@in-advantage.com>

[PATCH 7/8] mfd: ocelot: enable support for mdio management

[Rasmus Villemoes]

The implementation is rather straight-forward, following section
3.5.3 (MIIM interface in slave mode) in the data sheet for the
vsc7514.

Since each register access requires multiple MDIO accesses, keep the
parent mii_bus locked for the whole read/write in order that accesses
by different sub-devices do not end up corrupting each other. Since
the MFD among other things exposes an mdio bus to the switch's
internal PHYs, use MDIO_MUTEX_NESTED.

Looking through the data sheets of all of VSC7511, VSC7512, VSC7513,
VSC7514, I haven't seen any indication that they can be controlled
over I2C, so drop that mention from the Kconfig help text and add MDIO
in its place.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 drivers/mfd/Kconfig       |   3 +-
 drivers/mfd/Makefile      |   2 +-
 drivers/mfd/ocelot-mdio.c | 161 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mfd/ocelot-mdio.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 4dc894061b62e..c062563794d9e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1048,6 +1048,7 @@ config MFD_MENF21BMC
 config MFD_OCELOT
 	tristate "Microsemi Ocelot External Control Support"
 	depends on SPI_MASTER
+	select PHYLIB
 	select MFD_CORE
 	select REGMAP
 	help
@@ -1056,7 +1057,7 @@ config MFD_OCELOT
 	  other functions, including pinctrl, MDIO, and communication with
 	  external chips. While some chips have an internal processor capable of
 	  running an OS, others don't. All chips can be controlled externally
-	  through different interfaces, including SPI, I2C, and PCIe.
+	  through different interfaces, including SPI, MDIO, and PCIe.
 
 	  Say yes here to add support for Ocelot chips (VSC7511, VSC7512,
 	  VSC7513, VSC7514) controlled externally.
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 9220eaf7cf125..fc675ddd59f17 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -123,7 +123,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
 
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
 
-ocelot-soc-objs			:= ocelot-core.o ocelot-spi.o
+ocelot-soc-objs			:= ocelot-core.o ocelot-spi.o ocelot-mdio.o
 obj-$(CONFIG_MFD_OCELOT)	+= ocelot-soc.o
 
 obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
diff --git a/drivers/mfd/ocelot-mdio.c b/drivers/mfd/ocelot-mdio.c
new file mode 100644
index 0000000000000..7ac232a1ad6ad
--- /dev/null
+++ b/drivers/mfd/ocelot-mdio.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * MIIM core driver for the Ocelot chip family.
+ */
+
+/*
+ * Each register access requires multiple MDIO accesses.
+ */
+
+#include <linux/device.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
+#include <linux/regmap.h>
+
+#include "ocelot.h"
+
+#define ADDR_REG0 0
+#define ADDR_REG1 1
+#define DATA_REG0 2
+#define DATA_REG1 3
+
+static int
+ocelot_mdio_write_addr(struct mdio_device *mdiodev, unsigned int addr)
+{
+	int ret;
+
+	addr &= 0x00ffffff;
+	addr >>= 2;
+
+	ret = __mdiodev_write(mdiodev, ADDR_REG0, addr & 0xffff);
+	if (ret)
+		return ret;
+
+	return __mdiodev_write(mdiodev, ADDR_REG1, addr >> 16);
+}
+
+static int
+__ocelot_mdio_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct mdio_device *mdiodev = context;
+	int ret;
+
+	ret = ocelot_mdio_write_addr(mdiodev, reg);
+	if (ret)
+		return ret;
+
+	ret = __mdiodev_write(mdiodev, DATA_REG0, val & 0xffff);
+	if (ret)
+		return ret;
+
+	return __mdiodev_write(mdiodev, DATA_REG1, val >> 16);
+}
+
+static int
+__ocelot_mdio_read(struct mdio_device *mdiodev, unsigned int reg, unsigned int *val)
+{
+	int ret, lo, hi, i;
+
+	ret = ocelot_mdio_write_addr(mdiodev, reg);
+	if (ret)
+		return ret;
+
+	/*
+	 * The data registers must be read twice. Only after the first
+	 * read is the value of the register whose address was written
+	 * into the address registers latched into the data registers.
+	 */
+	for (i = 0; i < 2; ++i) {
+		lo = __mdiodev_read(mdiodev, DATA_REG0);
+		if (lo < 0)
+			return lo;
+		hi = __mdiodev_read(mdiodev, DATA_REG1);
+		if (hi < 0)
+			return hi;
+	}
+
+	*val = (hi << 16) | (lo & 0xffff);
+
+	return 0;
+}
+
+static int
+ocelot_mdio_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct mdio_device *mdiodev = context;
+	int ret;
+
+	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
+	ret = __ocelot_mdio_write(mdiodev, reg, val);
+	mutex_unlock(&mdiodev->bus->mdio_lock);
+
+	return ret;
+}
+
+static int
+ocelot_mdio_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct mdio_device *mdiodev = context;
+	int ret;
+
+	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
+	ret = __ocelot_mdio_read(mdiodev, reg, val);
+	mutex_unlock(&mdiodev->bus->mdio_lock);
+
+	return ret;
+}
+
+static const struct regmap_bus ocelot_mdio_regmap_bus = {
+	.reg_write = ocelot_mdio_write,
+	.reg_read = ocelot_mdio_read,
+};
+
+static struct regmap *ocelot_mdio_init_regmap(struct device *dev, const struct resource *res)
+{
+	struct mdio_device *mdiodev = to_mdio_device(dev);
+	struct regmap_config regmap_config = {};
+
+	regmap_config.reg_bits = 32;
+	regmap_config.reg_stride = 4;
+	regmap_config.val_bits = 32;
+	regmap_config.name = res->name;
+	regmap_config.max_register = resource_size(res) - 1;
+	regmap_config.reg_base = res->start;
+
+	return devm_regmap_init(dev, &ocelot_mdio_regmap_bus, mdiodev, &regmap_config);
+}
+
+static int ocelot_mdio_probe(struct mdio_device *mdiodev)
+{
+	struct device *dev = &mdiodev->dev;
+	struct ocelot_ddata *ddata;
+
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+	if (!ddata)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, ddata);
+	ddata->init_regmap = ocelot_mdio_init_regmap;
+
+	return ocelot_core_init(dev);
+}
+
+static const struct of_device_id ocelot_mdio_of_match[] = {
+	{ .compatible = "mscc,vsc7512" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ocelot_mdio_of_match);
+
+static struct mdio_driver ocelot_mdio_driver = {
+	.probe = ocelot_mdio_probe,
+	.mdiodrv.driver = {
+		.name = "ocelot-soc",
+		.of_match_table = ocelot_mdio_of_match,
+	},
+};
+mdio_module_driver(ocelot_mdio_driver);
+
+MODULE_DESCRIPTION("MDIO Controlled Ocelot Chip Driver");
+MODULE_AUTHOR("Rasmus Villemoes <ravi@prevas.dk>");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_IMPORT_NS("MFD_OCELOT");
-- 
2.49.0

[PATCH 8/8] dt-bindings: mfd: ocelot: mention MDIO management and add example

[Rasmus Villemoes]

The ocelot switches can also be strapped so that they can be
controlled via an MDIO bus (on either address 0 or 31). Mention that
and add an example.

Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
---
 .../devicetree/bindings/mfd/mscc,ocelot.yaml  | 121 +++++++++++++++++-
 1 file changed, 119 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml b/Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml
index 8bd1abfc44d99..bd2787a613e16 100644
--- a/Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml
+++ b/Documentation/devicetree/bindings/mfd/mscc,ocelot.yaml
@@ -12,8 +12,8 @@ maintainers:
 description: |
   The Ocelot ethernet switch family contains chips that have an internal CPU
   (VSC7513, VSC7514) and chips that don't (VSC7511, VSC7512). All switches have
-  the option to be controlled externally via external interfaces like SPI or
-  PCIe.
+  the option to be controlled externally via external interfaces like SPI, MDIO
+  or PCIe.
 
   The switch family is a multi-port networking switch that supports many
   interfaces. Additionally, the device can perform pin control, MDIO buses, and
@@ -164,6 +164,123 @@ examples:
             };
         };
     };
+  - |
+    #include <dt-bindings/phy/phy-ocelot-serdes.h>
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        soc@0 {
+            compatible = "mscc,vsc7512";
+            reg = <0x0>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+
+            mdio@7107009c {
+                compatible = "mscc,ocelot-miim";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <0x7107009c 0x24>;
+
+                sw_phy1: ethernet-phy@1 {
+                    reg = <0x1>;
+                };
+                sw_phy2: ethernet-phy@2 {
+                    reg = <0x2>;
+                };
+            };
+
+            mdio@710700c0 {
+                compatible = "mscc,ocelot-miim";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <0x710700c0 0x24>;
+                status = "disabled";
+            };
+
+            ocelot_gpio: pinctrl@71070034 {
+                compatible = "mscc,ocelot-pinctrl";
+                gpio-controller;
+                #gpio-cells = <2>;
+                gpio-ranges = <&ocelot_gpio 0 0 22>;
+                reg = <0x71070034 0x6c>;
+            };
+
+            gpio@710700f8 {
+                compatible = "mscc,ocelot-sgpio";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                reg = <0x710700f8 0x100>;
+                status = "disabled";
+            };
+
+            ocelot_serdes: serdes@710d0000 {
+                compatible = "mscc,vsc7514-serdes";
+                reg = <0x710d0000 0x10000>;
+                #phy-cells = <2>;
+            };
+
+            ethernet-switch@71010000 {
+                compatible = "mscc,vsc7512-switch";
+                reg = <0x71010000 0x10000>,
+                    <0x71030000 0x10000>,
+                    <0x71080000 0x100>,
+                    <0x710e0000 0x10000>,
+                    <0x711e0000 0x100>,
+                    <0x711f0000 0x100>,
+                    <0x71200000 0x100>,
+                    <0x71210000 0x100>,
+                    <0x71220000 0x100>,
+                    <0x71230000 0x100>,
+                    <0x71240000 0x100>,
+                    <0x71250000 0x100>,
+                    <0x71260000 0x100>,
+                    <0x71270000 0x100>,
+                    <0x71280000 0x100>,
+                    <0x71800000 0x80000>,
+                    <0x71880000 0x10000>,
+                    <0x71040000 0x10000>,
+                    <0x71050000 0x10000>,
+                    <0x71060000 0x10000>;
+                reg-names = "sys", "rew", "qs", "ptp", "port0", "port1",
+                    "port2", "port3", "port4", "port5", "port6",
+                    "port7", "port8", "port9", "port10", "qsys",
+                    "ana", "s0", "s1", "s2";
+
+                ethernet-ports {
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+
+                    port@1 {
+                        reg = <1>;
+                        label = "swp1";
+                        phy-handle = <&sw_phy1>;
+                        phy-mode = "internal";
+                    };
+
+                    port@2 {
+                        reg = <2>;
+                        label = "swp2";
+                        phy-handle = <&sw_phy2>;
+                        phy-mode = "internal";
+                    };
+
+                    port@a {
+                        reg = <10>;
+                        label = "cpu";
+                        ethernet = <&enetc_port0>;
+                        phy-connection-type = "sgmii";
+                        phys = <&ocelot_serdes 10 SERDES6G(2)>;
+
+                        fixed-link {
+                            speed = <1000>;
+                            full-duplex;
+                        };
+                    };
+                };
+            };
+        };
+    };
 
 ...
 
-- 
2.49.0

Re: [PATCH 8/8] dt-bindings: mfd: ocelot: mention MDIO management and add example

[Rob Herring (Arm)]

On Wed, 19 Mar 2025 13:30:58 +0100, Rasmus Villemoes wrote:
> The ocelot switches can also be strapped so that they can be
> controlled via an MDIO bus (on either address 0 or 31). Mention that
> and add an example.
> 
> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
> ---
>  .../devicetree/bindings/mfd/mscc,ocelot.yaml  | 121 +++++++++++++++++-
>  1 file changed, 119 insertions(+), 2 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: soc@0: ethernet-switch@71010000:ethernet-ports:port@a: 'phy-mode' is a required property
	from schema $id: http://devicetree.org/schemas/mfd/mscc,ocelot.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: soc@0: ethernet-switch@71010000:ethernet-ports:port@a: Unevaluated properties are not allowed ('phys' was unexpected)
	from schema $id: http://devicetree.org/schemas/mfd/mscc,ocelot.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: soc@0: ethernet-switch@71010000: Unevaluated properties are not allowed ('ethernet-ports' was unexpected)
	from schema $id: http://devicetree.org/schemas/mfd/mscc,ocelot.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: soc@0: ethernet-switch@71010000: Unevaluated properties are not allowed ('ethernet-ports' was unexpected)
	from schema $id: http://devicetree.org/schemas/mfd/mscc,ocelot.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: soc@0: 'serdes@710d0000' does not match any of the regexes: '^ethernet-switch@[0-9a-f]+$', '^gpio@[0-9a-f]+$', '^mdio@[0-9a-f]+$', '^pinctrl@[0-9a-f]+$', 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/mfd/mscc,ocelot.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: serdes@710d0000: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+'
	from schema $id: http://devicetree.org/schemas/phy/mscc,vsc7514-serdes.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: ethernet-switch@71010000: ethernet-ports:port@a: 'phy-mode' is a required property
	from schema $id: http://devicetree.org/schemas/net/mscc,vsc7514-switch.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: ethernet-switch@71010000: ethernet-ports:port@a: Unevaluated properties are not allowed ('phys' was unexpected)
	from schema $id: http://devicetree.org/schemas/net/mscc,vsc7514-switch.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: ethernet-switch@71010000: Unevaluated properties are not allowed ('ethernet-ports' was unexpected)
	from schema $id: http://devicetree.org/schemas/net/mscc,vsc7514-switch.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250319123058.452202-9-ravi@prevas.dk

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 8/8] dt-bindings: mfd: ocelot: mention MDIO management and add example

[Rasmus Villemoes]

On Wed, Mar 19 2025, "Rob Herring (Arm)" <robh@kernel.org> wrote:

> On Wed, 19 Mar 2025 13:30:58 +0100, Rasmus Villemoes wrote:
>> The ocelot switches can also be strapped so that they can be
>> controlled via an MDIO bus (on either address 0 or 31). Mention that
>> and add an example.
>> 
>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>> ---
>>  .../devicetree/bindings/mfd/mscc,ocelot.yaml  | 121 +++++++++++++++++-
>>  1 file changed, 119 insertions(+), 2 deletions(-)
>> 
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb: soc@0: ethernet-switch@71010000:ethernet-ports:port@a: 'phy-mode' is a required property
> 	from schema $id:
> http://devicetree.org/schemas/mfd/mscc,ocelot.yaml#

So I thought phy-mode and phy-connection-type were interchangeable, but
apparently not wrt. dt bindings. I also see that I wasn't even
consistent (I used phy-mode for the user ports), so that one is easy to
fix.

> Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb:
> soc@0: ethernet-switch@71010000:ethernet-ports:port@a: Unevaluated
> properties are not allowed ('phys' was unexpected)
> 	from schema $id:
> http://devicetree.org/schemas/mfd/mscc,ocelot.yaml#


Well, I do need to specify that phys property for the cpu-facing port,
so I'm gonna need some help to extend the schema to allow that. The
definition of what is allowed here isn't in the mscc,vsc7514-switch.yaml
file itself but is "inherited" through several levels of $ref, but I
don't think it would be appropriate to add to dsa-port.yaml as this is
somewhat special to this switch.

> Documentation/devicetree/bindings/mfd/mscc,ocelot.example.dtb:
> soc@0: 'serdes@710d0000' does not match any of the regexes:
> '^ethernet-switch@[0-9a-f]+$', '^gpio@[0-9a-f]+$', '^mdio@[0-9a-f]+$',
> '^pinctrl@[0-9a-f]+$', 'pinctrl-[0-9]+'

Hm. I have probably cheated somewhat, but I think there are a few things
that need fixing here.

First, I think the existing binding should allow for a "syscon" subnode,
which in turn can then contain that serdes subnode (see
arch/mips/boot/dts/mscc/ocelot.dtsi). But when I then put my serdes node
inside that new mscc,ocelot-hsio node, I think the MFD driver is going
to warn, because that expects a subnode with the mscc,vsc7514-serdes
compatible. So that should be updated to expect a mscc,ocelot-hsio
subnode, and the naming changed s/serdes/hsio/. I'll have to test this
first, to see if the various drivers still find the regmap(s) they need.

Rasmus

Re: [PATCH 0/8] mfd: ocelot: add support for MDIO managed switch

[Colin Foster]

Hi Rasmus,

On Wed, Mar 19, 2025 at 01:30:50PM +0100, Rasmus Villemoes wrote:
> While reworking the spi and core files to allow hooking up another
> underlying bus, I found what I think might be a bug in the SPI
> implementation, for the case where the SPI bus is set higher than
> 500kHz. But as I don't have such hardware, I don't know if the bug is
> real, nor am I able to test my changes.
>
> If desired, I can drop those changes

I ran the patch set on my setup and everything worked. I was hopeful
that it would solve a power sequencing issue that I came across a while
back. It didn't, but the logic works and seems sound, so I say keep it!

I'll review my parts and add the relevant testing tags.

Thanks for the update, and I'm glad it is being used!

Colin Foster

Re: [PATCH 0/8] mfd: ocelot: add support for MDIO managed switch

[Colin Foster]

Hi Rasmus,

On Wed, Mar 19, 2025 at 01:30:50PM +0100, Rasmus Villemoes wrote:
> The primary purpose of this series is to add support for vsc751x
> switches that are wired up to be managed via MDIO. The current MFD
> driver only has support for management over SPI.

Are you still intending to work on this? I understand if not, but maybe
the bugfix / documentation patches could be broken out, as I think
they'd be accepted pretty easily.

>   mfd: ocelot: rework SPI (re-)initialization after chip reset
>   mfd: ocelot: correct Kconfig dependency
>   dt-bindings: mfd: ocelot: mention MDIO management and add example

Is there anything you need from me?

Thanks,

Colin Foster

Re: [PATCH 0/8] mfd: ocelot: add support for MDIO managed switch

[Rasmus Villemoes]

On Mon, Jun 30 2025, Colin Foster <colin.foster@in-advantage.com> wrote:

> Hi Rasmus,
>
> On Wed, Mar 19, 2025 at 01:30:50PM +0100, Rasmus Villemoes wrote:
>> The primary purpose of this series is to add support for vsc751x
>> switches that are wired up to be managed via MDIO. The current MFD
>> driver only has support for management over SPI.
>
> Are you still intending to work on this?

Sorry for dropping the ball, but the customer didn't want to fund
finishing the upstreaming, and I was assigned to other
projects/customers. I also no longer have their hardware, so couldn't
test a new version myself. So no, I don't expect to work on this
anymore.

> I understand if not, but maybe the bugfix / documentation patches
> could be broken out, as I think they'd be accepted pretty easily.

>>   mfd: ocelot: rework SPI (re-)initialization after chip reset
>>   mfd: ocelot: correct Kconfig dependency
>>   dt-bindings: mfd: ocelot: mention MDIO management and add example

Probably the first two, but I'm not sure the last one makes sense
without the actual support landing. And the first one, AFAIR, would need
to be reworked as it relied on the "mfd: ocelot: refactor bus-specific
regmap initialization" which Lee rejected.

There's also "mfd: ocelot: move SPI specific macros to ocelot-spi.c"
which is just a trivial cleanup.

Rasmus