[PATCH v2 net-next 00/15] Probe SJA1105 DSA children as platform sub-devices

[PATCH v2 net-next 00/15] Probe SJA1105 DSA children as platform sub-devices

[Vladimir Oltean]

The NXP SJA1105 and SJA1110 switches have a linear address space thanks
to their SPI-to-AHB bridge. This address space contains not only the
Ethernet switch, but (depending on hw generation) SGMII PCS blocks,
MDIO controllers, interrupt controllers, GPIO controllers. Of these, the
SGMII PCS blocks and the MDIO controllers are supported today by the DSA
driver, through internal programming.

This series proposes registering platform devices for each non-switch
weakly coupled component, all such devices being backed by an OF node
(and reachable through phandles to it), one or more resources and the
switch global regmap. For backward compatibility and not only (DT author
convenience), if these OF nodes do not exist, they are created using the
dynamic of_changeset API.

The motivation must be explained top-down:

  The high-level goal is to customize the XPCS (SGMII PCS block) with
  board-specific device tree that describe the required differential
  lane polarity.

  But the XPCS is not presently described in the device tree in
  SJA1105/SJA1110, hence the desire to do that.

  However, instances of XPCS blocks described in the device tree already
  exist for the case where it is MMIO-mapped. Those bindings dictate how
  the XPCS in SJA1105/SJA1110 should be described.

v1 at:
https://lore.kernel.org/netdev/20251118190530.580267-1-vladimir.oltean@nxp.com/

Major changes since v1:
- Stop using MFD API due to pushback from Lee Jones. Create replacement
  of_subdevs_populate() API localized to sja1105 (can be moved if need
  be)
- Exclude first patch "net: dsa: sja1105: let phylink help with the
  replay of link callbacks", upstreamed separately.
- Include XPCS lane polarity parsing.
Minor changes listed in individual patches.

Note that the current of_changeset API is "sufficient" for this use
case, but if its use is acceptable for XPCS, I will need to look at
expanding it with phandle support, for the interrupt controller
sub-device. I was able to "wing it" here by manually keeping dynamic
fwnode_handle references rather than reaching them through phandles,
but that has limits.

Vladimir Oltean (15):
  net: mdio-regmap: permit working with non-MMIO regmaps
  net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs
  net: mdio: add generic driver for NXP SJA1110 100BASE-TX embedded PHYs
  net: dsa: sja1105: prepare regmap for passing to child devices
  net: dsa: sja1105: include spi.h from sja1105.h
  net: dsa: sja1105: transition OF-based MDIO controllers to standalone
    sub-devices
  net: pcs: xpcs: introduce xpcs_create_pcs_fwnode()
  net: pcs: xpcs-plat: convert to regmap
  dt-bindings: net: dsa: sja1105: document the PCS nodes
  net: pcs: xpcs-plat: add NXP SJA1105/SJA1110 support
  net: dsa: sja1105: fill device tree with ethernet-pcs sub-devices
    under "regs" node
  net: dsa: sja1105: replace mdiobus-pcs with xpcs-plat driver
  net: dsa: sja1105: permit finding the XPCS via pcs-handle
  dt-bindings: net: xpcs: allow properties from phy-common-props.yaml
  net: pcs: xpcs: allow generic polarity inversion

 .../bindings/net/dsa/nxp,sja1105.yaml         |  27 +
 .../bindings/net/pcs/snps,dw-xpcs.yaml        |  13 +-
 MAINTAINERS                                   |   2 +
 drivers/net/dsa/sja1105/Kconfig               |   1 +
 drivers/net/dsa/sja1105/Makefile              |   2 +-
 drivers/net/dsa/sja1105/sja1105.h             |  39 +-
 drivers/net/dsa/sja1105/sja1105_main.c        |  86 ++-
 drivers/net/dsa/sja1105/sja1105_mdio.c        | 507 -----------------
 drivers/net/dsa/sja1105/sja1105_spi.c         | 113 +++-
 drivers/net/dsa/sja1105/sja1105_subdev.c      | 513 ++++++++++++++++++
 drivers/net/dsa/sja1105/sja1105_subdev.h      |  10 +
 drivers/net/mdio/Kconfig                      |  22 +-
 drivers/net/mdio/Makefile                     |   2 +
 drivers/net/mdio/mdio-regmap-simple.c         |  80 +++
 drivers/net/mdio/mdio-regmap.c                |   7 +-
 drivers/net/mdio/mdio-sja1110-cbt1.c          | 179 ++++++
 drivers/net/pcs/Kconfig                       |   1 +
 drivers/net/pcs/pcs-xpcs-nxp.c                |  11 -
 drivers/net/pcs/pcs-xpcs-plat.c               | 146 +++--
 drivers/net/pcs/pcs-xpcs.c                    |  58 +-
 drivers/net/pcs/pcs-xpcs.h                    |   1 -
 include/linux/mdio/mdio-regmap.h              |   2 +
 include/linux/pcs/pcs-xpcs.h                  |   1 +
 23 files changed, 1185 insertions(+), 638 deletions(-)
 delete mode 100644 drivers/net/dsa/sja1105/sja1105_mdio.c
 create mode 100644 drivers/net/dsa/sja1105/sja1105_subdev.c
 create mode 100644 drivers/net/dsa/sja1105/sja1105_subdev.h
 create mode 100644 drivers/net/mdio/mdio-regmap-simple.c
 create mode 100644 drivers/net/mdio/mdio-sja1110-cbt1.c

-- 
2.34.1

[PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

The regmap world is seemingly split into two groups which attempt to
solve different problems. Effectively, this means that not all regmap
providers are compatible with all regmap consumers.

First, we have the group where the current mdio-regmap users fit:
altera_tse_main.c and dwmac-socfpga.c use devm_regmap_init_mmio() to
ioremap their pcs_base and obtain a regmap where address zero is the
first PCS register.

Second, we have the group where MFD parent drivers call
mfd_add_devices(), having previously initialized a non-MMIO (SPI, I2C)
regmap and added it to their devres list, and MFD child drivers use
dev_get_regmap(dev->parent, NULL) in their probe function, to find the
first (and single) regmap of the MFD parent. The address zero of this
regmap is global to the entire parent, so the children need to be
parent-aware and add their own offsets for the registers that they
should manage. This is essentially because MFD is seemingly coming from
a world where peripheral registers are all entangled with each other.

What I'm trying to support are potentially multiple instances of the
same kind of device, at well separated address space regions.

To provide isolated regmaps for each child device would essentially mean
solving the problem of how would each child device needs to find the
correct regmap. This further means that "dev_get_regmap(dev->parent,
NULL)" transforms either in:
- dev_get_regmap(dev, NULL): search in the child device's devres list,
  not in the parent's. This means adding the regmap in between
  platform_device_alloc() and platform_device_add(), but is
  structurally impossible because &dev->devres_head is initialized way
  too late, in device_initialize().
- dev_get_regmap(dev->parent, "unique-regmap-name"): now the child
  device needs to know, in case there are multiple instances of it,
  which one is it, to ask for the right one. I've seen
  drivers/mfd/ocelot-core.c work around this rather elegantly, providing
  a resource to the child, and then the child uses resource->name to
  find the regmap of the same name in the parent. But then I also
  stumbled upon drivers/net/pcs/pcs-xpcs-plat.c which I need to support
  as a child platform device, and that superimposes its own naming
  scheme for the resources: "direct" or "indirect" - scheme which is
  obviously incompatible with namespacing per instance.

So a parent device needs to decide whether it is in the boat that
provides one isolated regmap for each child, or one big regmap for all.
The "one big regmap" is the lowest common denominator when considering
children like pcs-xpcs-plat.c.

This means that from mdio-regmap's perspective, it needs to deal with
regmaps coming from both kinds of providers, as neither of them is going
away.

Users who provide a big regmap but want to access only a window into it
should provide as a struct mdio_regmap_config field a resource that
describes the start and end of that window. Currently we only use the
start as an offset into the regmap, and hope that MDIO reads and writes
won't go past the end.

Cc: Mark Brown <broonie@kernel.org>
Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
v1->v2: add Maxime's review tag

 drivers/net/mdio/mdio-regmap.c   | 7 +++++--
 include/linux/mdio/mdio-regmap.h | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
index 8a742a8d6387..2a0e9c519fa3 100644
--- a/drivers/net/mdio/mdio-regmap.c
+++ b/drivers/net/mdio/mdio-regmap.c
@@ -19,6 +19,7 @@
 
 struct mdio_regmap_priv {
 	struct regmap *regmap;
+	unsigned int base;
 	u8 valid_addr;
 };
 
@@ -31,7 +32,7 @@ static int mdio_regmap_read_c22(struct mii_bus *bus, int addr, int regnum)
 	if (ctx->valid_addr != addr)
 		return -ENODEV;
 
-	ret = regmap_read(ctx->regmap, regnum, &val);
+	ret = regmap_read(ctx->regmap, ctx->base + regnum, &val);
 	if (ret < 0)
 		return ret;
 
@@ -46,7 +47,7 @@ static int mdio_regmap_write_c22(struct mii_bus *bus, int addr, int regnum,
 	if (ctx->valid_addr != addr)
 		return -ENODEV;
 
-	return regmap_write(ctx->regmap, regnum, val);
+	return regmap_write(ctx->regmap, ctx->base + regnum, val);
 }
 
 struct mii_bus *devm_mdio_regmap_register(struct device *dev,
@@ -66,6 +67,8 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
 	mr = mii->priv;
 	mr->regmap = config->regmap;
 	mr->valid_addr = config->valid_addr;
+	if (config->resource)
+		mr->base = config->resource->start;
 
 	mii->name = DRV_NAME;
 	strscpy(mii->id, config->name, MII_BUS_ID_SIZE);
diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h
index 679d9069846b..441cead97936 100644
--- a/include/linux/mdio/mdio-regmap.h
+++ b/include/linux/mdio/mdio-regmap.h
@@ -11,10 +11,12 @@
 
 struct device;
 struct regmap;
+struct resource;
 
 struct mdio_regmap_config {
 	struct device *parent;
 	struct regmap *regmap;
+	const struct resource *resource;
 	char name[MII_BUS_ID_SIZE];
 	u8 valid_addr;
 	bool autoscan;
-- 
2.34.1

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 12:56:40PM +0200, Vladimir Oltean wrote:
> The regmap world is seemingly split into two groups which attempt to
> solve different problems. Effectively, this means that not all regmap
> providers are compatible with all regmap consumers.
> 
> First, we have the group where the current mdio-regmap users fit:
> altera_tse_main.c and dwmac-socfpga.c use devm_regmap_init_mmio() to
> ioremap their pcs_base and obtain a regmap where address zero is the
> first PCS register.
> 
> Second, we have the group where MFD parent drivers call
> mfd_add_devices(), having previously initialized a non-MMIO (SPI, I2C)
> regmap and added it to their devres list, and MFD child drivers use
> dev_get_regmap(dev->parent, NULL) in their probe function, to find the
> first (and single) regmap of the MFD parent. The address zero of this
> regmap is global to the entire parent, so the children need to be
> parent-aware and add their own offsets for the registers that they
> should manage. This is essentially because MFD is seemingly coming from
> a world where peripheral registers are all entangled with each other.
> 
> What I'm trying to support are potentially multiple instances of the
> same kind of device, at well separated address space regions.
> 
> To provide isolated regmaps for each child device would essentially mean
> solving the problem of how would each child device needs to find the
> correct regmap. This further means that "dev_get_regmap(dev->parent,
> NULL)" transforms either in:
> - dev_get_regmap(dev, NULL): search in the child device's devres list,
>   not in the parent's. This means adding the regmap in between
>   platform_device_alloc() and platform_device_add(), but is
>   structurally impossible because &dev->devres_head is initialized way
>   too late, in device_initialize().
> - dev_get_regmap(dev->parent, "unique-regmap-name"): now the child
>   device needs to know, in case there are multiple instances of it,
>   which one is it, to ask for the right one. I've seen
>   drivers/mfd/ocelot-core.c work around this rather elegantly, providing
>   a resource to the child, and then the child uses resource->name to
>   find the regmap of the same name in the parent. But then I also
>   stumbled upon drivers/net/pcs/pcs-xpcs-plat.c which I need to support
>   as a child platform device, and that superimposes its own naming
>   scheme for the resources: "direct" or "indirect" - scheme which is
>   obviously incompatible with namespacing per instance.
> 
> So a parent device needs to decide whether it is in the boat that
> provides one isolated regmap for each child, or one big regmap for all.
> The "one big regmap" is the lowest common denominator when considering
> children like pcs-xpcs-plat.c.
> 
> This means that from mdio-regmap's perspective, it needs to deal with
> regmaps coming from both kinds of providers, as neither of them is going
> away.
> 
> Users who provide a big regmap but want to access only a window into it
> should provide as a struct mdio_regmap_config field a resource that
> describes the start and end of that window. Currently we only use the
> start as an offset into the regmap, and hope that MDIO reads and writes
> won't go past the end.

> Cc: Mark Brown <broonie@kernel.org>
> Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>

FWIW, Cc list may be located after --- line. It will have the same effect on
emails (as regular tooling will parse and put them into email headers), but
will reduce unneeded noise in the commit message. List will be still available
on lore.kernel.org in the mail archives.

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---

  Cc: ...
  ...

...

>  struct mdio_regmap_priv {
>  	struct regmap *regmap;
> +	unsigned int base;

Hmm... resource_size_t ?

>  	u8 valid_addr;
>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote:
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> FWIW, Cc list may be located after --- line. It will have the same effect on
> emails (as regular tooling will parse and put them into email headers), but
> will reduce unneeded noise in the commit message. List will be still available
> on lore.kernel.org in the mail archives.

Thanks for the comment. I know it may be located after ---, but for me,
doing that implies an extra step which I find unnecessary (moving them
there after the git format-patch stage). I keep the Cc: in the commit
message in git so that it's preserved across revisions.

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> 
>   Cc: ...
>   ...
> 
> ...
> 
> >  struct mdio_regmap_priv {
> >  	struct regmap *regmap;
> > +	unsigned int base;
> 
> Hmm... resource_size_t ?

Well, regmap_read() takes "unsigned int reg".
https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
So in practice, a truncation will be done somewhere if the register base
exceeds unsigned int storage capacity. But I didn't feel that it's worth
handling that.

> >  	u8 valid_addr;
> >  };
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Russell King (Oracle)]

On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote:
> > > Cc: Mark Brown <broonie@kernel.org>
> > > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > 
> > FWIW, Cc list may be located after --- line. It will have the same effect on
> > emails (as regular tooling will parse and put them into email headers), but
> > will reduce unneeded noise in the commit message. List will be still available
> > on lore.kernel.org in the mail archives.
> 
> Thanks for the comment. I know it may be located after ---, but for me,
> doing that implies an extra step which I find unnecessary (moving them
> there after the git format-patch stage). I keep the Cc: in the commit
> message in git so that it's preserved across revisions.

What I do with my individual patch versioning is in the editor, after
my sign-off, add the "---" and put the version changes below. This means
git format-patch will do what it normally does, and as the commit
message is reproduced verbatum, you get all that included too.

You do get an extra "---" line between the versioning changes and the
diffstat, but that's fine.

So in the commit message:

Signed-off-by: ...
---
Cc: ...

Changes v2:
 ...

will work fine.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 12:16:35PM +0000, Russell King (Oracle) wrote:
> On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> > On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote:
> > > > Cc: Mark Brown <broonie@kernel.org>
> > > > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > 
> > > FWIW, Cc list may be located after --- line. It will have the same effect on
> > > emails (as regular tooling will parse and put them into email headers), but
> > > will reduce unneeded noise in the commit message. List will be still available
> > > on lore.kernel.org in the mail archives.
> > 
> > Thanks for the comment. I know it may be located after ---, but for me,
> > doing that implies an extra step which I find unnecessary (moving them
> > there after the git format-patch stage). I keep the Cc: in the commit
> > message in git so that it's preserved across revisions.
> 
> What I do with my individual patch versioning is in the editor, after
> my sign-off, add the "---" and put the version changes below. This means
> git format-patch will do what it normally does, and as the commit
> message is reproduced verbatum, you get all that included too.
> 
> You do get an extra "---" line between the versioning changes and the
> diffstat, but that's fine.
> 
> So in the commit message:
> 
> Signed-off-by: ...
> ---
> Cc: ...
> 
> Changes v2:
>  ...
> 
> will work fine.

Oh, that's quite a clever use. I can now keep my changelog in git too...
Thanks for the hint!

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> > >  struct mdio_regmap_priv {
> > >  	struct regmap *regmap;
> > > +	unsigned int base;
> > 
> > Hmm... resource_size_t ?
> 
> Well, regmap_read() takes "unsigned int reg".
> https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> So in practice, a truncation will be done somewhere if the register base
> exceeds unsigned int storage capacity. But I didn't feel that it's worth
> handling that.

Would this address your feedback?

diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
index 2a0e9c519fa3..416ff4e13e8f 100644
--- a/drivers/net/mdio/mdio-regmap.c
+++ b/drivers/net/mdio/mdio-regmap.c
@@ -67,8 +67,15 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
 	mr = mii->priv;
 	mr->regmap = config->regmap;
 	mr->valid_addr = config->valid_addr;
-	if (config->resource)
+	if (config->resource) {
+		if (config->resource->start > U32_MAX ||
+		    config->resource->end > U32_MAX) {
+			dev_err(config->parent,
+				"Resource exceeds regmap API addressing possibilities\n");
+			return ERR_PTR(-EINVAL);
+		}
 		mr->base = config->resource->start;
+	}
 
 	mii->name = DRV_NAME;
 	strscpy(mii->id, config->name, MII_BUS_ID_SIZE);

(needs to be replicated in a bunch of other places)

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:

...

> > > > +	unsigned int base;
> > > 
> > > Hmm... resource_size_t ?

> > Well, regmap_read() takes "unsigned int reg".
> > https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> > So in practice, a truncation will be done somewhere if the register base
> > exceeds unsigned int storage capacity. But I didn't feel that it's worth
> > handling that.
> 
> Would this address your feedback?

Yes and no. See my remarks below.

> diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
> index 2a0e9c519fa3..416ff4e13e8f 100644
> --- a/drivers/net/mdio/mdio-regmap.c
> +++ b/drivers/net/mdio/mdio-regmap.c
> @@ -67,8 +67,15 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
>  	mr = mii->priv;
>  	mr->regmap = config->regmap;
>  	mr->valid_addr = config->valid_addr;
> -	if (config->resource)
> +	if (config->resource) {

Btw, this might be not enough, one should check size and flags as well
before use. There was a discussion about this recently. Maybe we should
just move to a simple unsigned int in the config for now? Because handling
resources maybe considered as over engineering in this case.

> +		if (config->resource->start > U32_MAX ||
> +		    config->resource->end > U32_MAX) {

Ideally it should be resource_overlaps() check. But see above.

> +			dev_err(config->parent,
> +				"Resource exceeds regmap API addressing possibilities\n");
> +			return ERR_PTR(-EINVAL);
> +		}
>  		mr->base = config->resource->start;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 04:38:37PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:
> > On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> 
> ...
> 
> > > > > +	unsigned int base;
> > > > 
> > > > Hmm... resource_size_t ?
> 
> > > Well, regmap_read() takes "unsigned int reg".
> > > https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> > > So in practice, a truncation will be done somewhere if the register base
> > > exceeds unsigned int storage capacity. But I didn't feel that it's worth
> > > handling that.
> > 
> > Would this address your feedback?
> 
> Yes and no. See my remarks below.
> 
> > diff --git a/drivers/net/mdio/mdio-regmap.c b/drivers/net/mdio/mdio-regmap.c
> > index 2a0e9c519fa3..416ff4e13e8f 100644
> > --- a/drivers/net/mdio/mdio-regmap.c
> > +++ b/drivers/net/mdio/mdio-regmap.c
> > @@ -67,8 +67,15 @@ struct mii_bus *devm_mdio_regmap_register(struct device *dev,
> >  	mr = mii->priv;
> >  	mr->regmap = config->regmap;
> >  	mr->valid_addr = config->valid_addr;
> > -	if (config->resource)
> > +	if (config->resource) {
> 
> Btw, this might be not enough, one should check size and flags as well
> before use. There was a discussion about this recently. Maybe we should
> just move to a simple unsigned int in the config for now? Because handling
> resources maybe considered as over engineering in this case.

The resource flags are never taken into consideration, but I can for
sure replace the resource in struct mdio_regmap_config with just an
unsigned int start and an end, but that doesn't get rid of the resource
usage. The dev_get_resource(dev->parent, NULL) call is how we learn of
where our register window is located in the "one big regmap" provided by
the parent (SJA1105). So we still need this check somewhere else if we
wanted to not fail silently in case of address bits truncation.

> > +		if (config->resource->start > U32_MAX ||
> > +		    config->resource->end > U32_MAX) {
> 
> Ideally it should be resource_overlaps() check. But see above.

resource_overlaps_with_what? The only problem is that the resource can
exceed the 32 bit representation that regmap works with.

> > +			dev_err(config->parent,
> > +				"Resource exceeds regmap API addressing possibilities\n");
> > +			return ERR_PTR(-EINVAL);
> > +		}
> >  		mr->base = config->resource->start;
> > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 12:18:48AM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 04:38:37PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:
> > > On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:

...

> > > > > > +	unsigned int base;
> > > > > 
> > > > > Hmm... resource_size_t ?
> > 
> > > > Well, regmap_read() takes "unsigned int reg".
> > > > https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> > > > So in practice, a truncation will be done somewhere if the register base
> > > > exceeds unsigned int storage capacity. But I didn't feel that it's worth
> > > > handling that.
> > > 
> > > Would this address your feedback?
> > 
> > Yes and no. See my remarks below.

...

> > > -	if (config->resource)
> > > +	if (config->resource) {
> > 
> > Btw, this might be not enough, one should check size and flags as well
> > before use. There was a discussion about this recently. Maybe we should
> > just move to a simple unsigned int in the config for now? Because handling
> > resources maybe considered as over engineering in this case.
> 
> The resource flags are never taken into consideration, but I can for
> sure replace the resource in struct mdio_regmap_config with just an
> unsigned int start and an end, but that doesn't get rid of the resource
> usage. The dev_get_resource(dev->parent, NULL) call is how we learn of
> where our register window is located in the "one big regmap" provided by
> the parent (SJA1105). So we still need this check somewhere else if we
> wanted to not fail silently in case of address bits truncation.

Hmm... Bu why we can't embed the full struct resource in such a case?
Because resource should have a flag check, otherwise it's a wrong check.

Discussion I mentioned is this:
https://lore.kernel.org/lkml/20251207215359.28895-1-ansuelsmth@gmail.com/

Fixes due to that finding:
https://lore.kernel.org/lkml/20251208200437.14199-1-ansuelsmth@gmail.com/
https://lore.kernel.org/lkml/20251208145654.5294-1-ilpo.jarvinen@linux.intel.com/

> > > +		if (config->resource->start > U32_MAX ||
> > > +		    config->resource->end > U32_MAX) {
> > 
> > Ideally it should be resource_overlaps() check. But see above.
> 
> resource_overlaps_with_what? The only problem is that the resource can
> exceed the 32 bit representation that regmap works with.

Obviously with the 4G address space :-)

	struct resource r4g = DEFINE_RESOURCE...(..., 0, SZ_4G...);

	if (resource_overlaps(&r4g, config->resource))
		aiaiai! // using %pR to print the content

> > > +			dev_err(config->parent,
> > > +				"Resource exceeds regmap API addressing possibilities\n");
> > > +			return ERR_PTR(-EINVAL);
> > > +		}
> > >  		mr->base = config->resource->start;
> > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

On Fri, Jan 23, 2026 at 09:20:58AM +0200, Andy Shevchenko wrote:
> On Fri, Jan 23, 2026 at 12:18:48AM +0200, Vladimir Oltean wrote:
> > On Thu, Jan 22, 2026 at 04:38:37PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:
> > > > On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> 
> ...
> 
> > > > > > > +	unsigned int base;
> > > > > > 
> > > > > > Hmm... resource_size_t ?
> > > 
> > > > > Well, regmap_read() takes "unsigned int reg".
> > > > > https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> > > > > So in practice, a truncation will be done somewhere if the register base
> > > > > exceeds unsigned int storage capacity. But I didn't feel that it's worth
> > > > > handling that.
> > > > 
> > > > Would this address your feedback?
> > > 
> > > Yes and no. See my remarks below.
> 
> ...
> 
> > > > -	if (config->resource)
> > > > +	if (config->resource) {
> > > 
> > > Btw, this might be not enough, one should check size and flags as well
> > > before use. There was a discussion about this recently. Maybe we should
> > > just move to a simple unsigned int in the config for now? Because handling
> > > resources maybe considered as over engineering in this case.
> > 
> > The resource flags are never taken into consideration, but I can for
> > sure replace the resource in struct mdio_regmap_config with just an
> > unsigned int start and an end, but that doesn't get rid of the resource
> > usage. The dev_get_resource(dev->parent, NULL) call is how we learn of
> > where our register window is located in the "one big regmap" provided by
> > the parent (SJA1105). So we still need this check somewhere else if we
> > wanted to not fail silently in case of address bits truncation.
> 
> Hmm... Bu why we can't embed the full struct resource in such a case?

We can also embed the full struct resource, I never said we can't...

> Because resource should have a flag check, otherwise it's a wrong check.
> 
> Discussion I mentioned is this:
> https://lore.kernel.org/lkml/20251207215359.28895-1-ansuelsmth@gmail.com/
> 
> Fixes due to that finding:
> https://lore.kernel.org/lkml/20251208200437.14199-1-ansuelsmth@gmail.com/
> https://lore.kernel.org/lkml/20251208145654.5294-1-ilpo.jarvinen@linux.intel.com/

The linked issues seem unrelated; they are caused by the assumption that
resource_size() can be zero. But I'm not using the resource_size()
helper, and even if I were, I'm not testing it against zero.

As opposed to the PCI BAR case, we don't keep around in an altered form
the resources exceeding 4G. Just need to reject them once and be done
with them.

Also, what else to even check about the resource flags? We get the
resource using "platform_get_resource(pdev, IORESOURCE_REG, 0)", so we
know they're of that type. I don't think IORESOURCE_REG resources have
any other valid bits in flags except for IORESOURCE_TYPE_BITS.

> > > > +		if (config->resource->start > U32_MAX ||
> > > > +		    config->resource->end > U32_MAX) {
> > > 
> > > Ideally it should be resource_overlaps() check. But see above.
> > 
> > resource_overlaps_with_what? The only problem is that the resource can
> > exceed the 32 bit representation that regmap works with.
> 
> Obviously with the 4G address space :-)
> 
> 	struct resource r4g = DEFINE_RESOURCE...(..., 0, SZ_4G...);
> 
> 	if (resource_overlaps(&r4g, config->resource))
> 		aiaiai! // using %pR to print the content

This is a buggy replacement of my intention. I need to sanity check that
my IORESOURCE_REG resource is entirely within the 0-4G region.

The correct way to express this using helpers:

	if (!resource_contains(&r4g, config->resource))
		nazad!

but... you see my point? In trying to make use of "standard" helpers, we
overcomplicate simple things and introduce bugs.

My initially proposed test can be written even simpler:

	if (config->resource->end > U32_MAX) {
		...

because end >= start, so also testing resource->start is redundant.

> > > > +			dev_err(config->parent,
> > > > +				"Resource exceeds regmap API addressing possibilities\n");
> > > > +			return ERR_PTR(-EINVAL);
> > > > +		}
> > > >  		mr->base = config->resource->start;
> > > > +	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:
> > > > > > > > +	unsigned int base;
> > > > > > > 
> > > > > > > Hmm... resource_size_t ?
> > > > 
> > > > > > Well, regmap_read() takes "unsigned int reg".
> > > > > > https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> > > > > > So in practice, a truncation will be done somewhere if the register base
> > > > > > exceeds unsigned int storage capacity. But I didn't feel that it's worth
> > > > > > handling that.
> > > > > 
> > > > > Would this address your feedback?
> > > > 
> > > > Yes and no. See my remarks below.
> > 
> > ...
> > 
> > > > > -	if (config->resource)
> > > > > +	if (config->resource) {
> > > > 
> > > > Btw, this might be not enough, one should check size and flags as well
> > > > before use. There was a discussion about this recently. Maybe we should
> > > > just move to a simple unsigned int in the config for now? Because handling
> > > > resources maybe considered as over engineering in this case.
> > > 
> > > The resource flags are never taken into consideration, but I can for
> > > sure replace the resource in struct mdio_regmap_config with just an
> > > unsigned int start and an end, but that doesn't get rid of the resource
> > > usage. The dev_get_resource(dev->parent, NULL) call is how we learn of
> > > where our register window is located in the "one big regmap" provided by
> > > the parent (SJA1105). So we still need this check somewhere else if we
> > > wanted to not fail silently in case of address bits truncation.
> > 
> > Hmm... Bu why we can't embed the full struct resource in such a case?
> 
> We can also embed the full struct resource, I never said we can't...
> 
> > Because resource should have a flag check, otherwise it's a wrong check.
> > 
> > Discussion I mentioned is this:
> > https://lore.kernel.org/lkml/20251207215359.28895-1-ansuelsmth@gmail.com/
> > 
> > Fixes due to that finding:
> > https://lore.kernel.org/lkml/20251208200437.14199-1-ansuelsmth@gmail.com/
> > https://lore.kernel.org/lkml/20251208145654.5294-1-ilpo.jarvinen@linux.intel.com/
> 
> The linked issues seem unrelated; they are caused by the assumption that
> resource_size() can be zero. But I'm not using the resource_size()
> helper, and even if I were, I'm not testing it against zero.
> 
> As opposed to the PCI BAR case, we don't keep around in an altered form
> the resources exceeding 4G. Just need to reject them once and be done
> with them.
> 
> Also, what else to even check about the resource flags? We get the
> resource using "platform_get_resource(pdev, IORESOURCE_REG, 0)", so we
> know they're of that type. I don't think IORESOURCE_REG resources have
> any other valid bits in flags except for IORESOURCE_TYPE_BITS.
> 
> > > > > +		if (config->resource->start > U32_MAX ||
> > > > > +		    config->resource->end > U32_MAX) {
> > > > 
> > > > Ideally it should be resource_overlaps() check. But see above.
> > > 
> > > resource_overlaps_with_what? The only problem is that the resource can
> > > exceed the 32 bit representation that regmap works with.
> > 
> > Obviously with the 4G address space :-)
> > 
> > 	struct resource r4g = DEFINE_RESOURCE...(..., 0, SZ_4G...);
> > 
> > 	if (resource_overlaps(&r4g, config->resource))
> > 		aiaiai! // using %pR to print the content
> 
> This is a buggy replacement of my intention. I need to sanity check that
> my IORESOURCE_REG resource is entirely within the 0-4G region.
> 
> The correct way to express this using helpers:
> 
> 	if (!resource_contains(&r4g, config->resource))
> 		nazad!
> 
> but... you see my point? In trying to make use of "standard" helpers, we
> overcomplicate simple things and introduce bugs.
> 
> My initially proposed test can be written even simpler:
> 
> 	if (config->resource->end > U32_MAX) {
> 		...
> 
> because end >= start, so also testing resource->start is redundant.
> 
> > > > > +			dev_err(config->parent,
> > > > > +				"Resource exceeds regmap API addressing possibilities\n");
> > > > > +			return ERR_PTR(-EINVAL);
> > > > > +		}
> > > > >  		mr->base = config->resource->start;
> > > > > +	}

A data structure which I find a bit under-utilized in the kernel is

/**
 * struct regmap_range - A register range, used for access related checks
 *                       (readable/writeable/volatile/precious checks)
 *
 * @range_min: address of first register
 * @range_max: address of last register
 */
struct regmap_range {
	unsigned int range_min;
	unsigned int range_max;
};

I could imagine a helper like:

/* Type adaptation between phy_addr_t and unsigned int */
static inline int __must_check regmap_range_from_resource(const struct resource *res,
							  struct regmap_range *range)
{
	struct resource r4g = DEFINE_RES(0, SZ_4G, res->flags);

	if (res->flags != IORESOURCE_REG) {
		pr_err("%s should be used only with IORESOURCE_REG resources\n");
		return -EINVAL;
	}

	if (!resource_contains(&r4g, res)) {
		pr_err("Resource exceeds regmap API addressing possibilities\n");
		return -EINVAL;
	}

	range->range_min = res->start;
	range->range_max = res->end;

	return 0;
}

and then proceed to use the simpler and validated regmap_range structure in the driver.
Too bad such use is not an established coding pattern...

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 03:55:01PM +0200, Vladimir Oltean wrote:
> On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:

...

> A data structure which I find a bit under-utilized in the kernel is
> 
> /**
>  * struct regmap_range - A register range, used for access related checks
>  *                       (readable/writeable/volatile/precious checks)
>  *
>  * @range_min: address of first register
>  * @range_max: address of last register
>  */
> struct regmap_range {
> 	unsigned int range_min;
> 	unsigned int range_max;
> };

Not sure. See below.

> I could imagine a helper like:
> 
> /* Type adaptation between phy_addr_t and unsigned int */
> static inline int __must_check regmap_range_from_resource(const struct resource *res,
> 							  struct regmap_range *range)
> {
> 	struct resource r4g = DEFINE_RES(0, SZ_4G, res->flags);
> 
> 	if (res->flags != IORESOURCE_REG) {
> 		pr_err("%s should be used only with IORESOURCE_REG resources\n");
> 		return -EINVAL;
> 	}
> 
> 	if (!resource_contains(&r4g, res)) {
> 		pr_err("Resource exceeds regmap API addressing possibilities\n");

%pR

> 		return -EINVAL;
> 	}
> 
> 	range->range_min = res->start;
> 	range->range_max = res->end;
> 
> 	return 0;
> }
> 
> and then proceed to use the simpler and validated regmap_range structure in the driver.
> Too bad such use is not an established coding pattern...

Dunno about semantics, as I only saw the use of that in regard to the special
slices of regmap.

Also we have struct range in range.h. Maybe that one suits better? It has also
some interesting APIs.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Vladimir Oltean]

On Fri, Jan 23, 2026 at 04:31:25PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 23, 2026 at 03:55:01PM +0200, Vladimir Oltean wrote:
> > On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:
> 
> ...
> 
> > A data structure which I find a bit under-utilized in the kernel is
> > 
> > /**
> >  * struct regmap_range - A register range, used for access related checks
> >  *                       (readable/writeable/volatile/precious checks)
> >  *
> >  * @range_min: address of first register
> >  * @range_max: address of last register
> >  */
> > struct regmap_range {
> > 	unsigned int range_min;
> > 	unsigned int range_max;
> > };
> 
> Not sure. See below.
> 
> > I could imagine a helper like:
> > 
> > /* Type adaptation between phy_addr_t and unsigned int */
> > static inline int __must_check regmap_range_from_resource(const struct resource *res,
> > 							  struct regmap_range *range)
> > {
> > 	struct resource r4g = DEFINE_RES(0, SZ_4G, res->flags);
> > 
> > 	if (res->flags != IORESOURCE_REG) {
> > 		pr_err("%s should be used only with IORESOURCE_REG resources\n");
> > 		return -EINVAL;
> > 	}
> > 
> > 	if (!resource_contains(&r4g, res)) {
> > 		pr_err("Resource exceeds regmap API addressing possibilities\n");
> 
> %pR
> 
> > 		return -EINVAL;
> > 	}
> > 
> > 	range->range_min = res->start;
> > 	range->range_max = res->end;
> > 
> > 	return 0;
> > }
> > 
> > and then proceed to use the simpler and validated regmap_range structure in the driver.
> > Too bad such use is not an established coding pattern...
> 
> Dunno about semantics, as I only saw the use of that in regard to the special
> slices of regmap.
> 
> Also we have struct range in range.h. Maybe that one suits better? It has also
> some interesting APIs.

It is defined as

struct range {
	u64   start;
	u64   end;
};

So it could be used, but it still doesn't properly express the fact that
regmap takes unsigned int register offsets (which struct regmap_range does).
Furthermore, by using struct range you are coupling unrelated data types,
whereas by using struct regmap_range you are not (if the regmap_read()
prototype changes, the regmap_range field data types immediately follow
suit).

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 05:10:49PM +0200, Vladimir Oltean wrote:
> On Fri, Jan 23, 2026 at 04:31:25PM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 23, 2026 at 03:55:01PM +0200, Vladimir Oltean wrote:
> > > On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:

...

> > > A data structure which I find a bit under-utilized in the kernel is
> > > 
> > > /**
> > >  * struct regmap_range - A register range, used for access related checks
> > >  *                       (readable/writeable/volatile/precious checks)
> > >  *
> > >  * @range_min: address of first register
> > >  * @range_max: address of last register
> > >  */
> > > struct regmap_range {
> > > 	unsigned int range_min;
> > > 	unsigned int range_max;
> > > };
> > 
> > Not sure. See below.
> > 
> > > I could imagine a helper like:
> > > 
> > > /* Type adaptation between phy_addr_t and unsigned int */
> > > static inline int __must_check regmap_range_from_resource(const struct resource *res,
> > > 							  struct regmap_range *range)
> > > {
> > > 	struct resource r4g = DEFINE_RES(0, SZ_4G, res->flags);
> > > 
> > > 	if (res->flags != IORESOURCE_REG) {
> > > 		pr_err("%s should be used only with IORESOURCE_REG resources\n");
> > > 		return -EINVAL;
> > > 	}
> > > 
> > > 	if (!resource_contains(&r4g, res)) {
> > > 		pr_err("Resource exceeds regmap API addressing possibilities\n");
> > 
> > %pR
> > 
> > > 		return -EINVAL;
> > > 	}
> > > 
> > > 	range->range_min = res->start;
> > > 	range->range_max = res->end;
> > > 
> > > 	return 0;
> > > }
> > > 
> > > and then proceed to use the simpler and validated regmap_range structure in the driver.
> > > Too bad such use is not an established coding pattern...
> > 
> > Dunno about semantics, as I only saw the use of that in regard to the special
> > slices of regmap.
> > 
> > Also we have struct range in range.h. Maybe that one suits better? It has also
> > some interesting APIs.
> 
> It is defined as
> 
> struct range {
> 	u64   start;
> 	u64   end;
> };
> 
> So it could be used, but it still doesn't properly express the fact that
> regmap takes unsigned int register offsets (which struct regmap_range does).
> Furthermore, by using struct range you are coupling unrelated data types,
> whereas by using struct regmap_range you are not (if the regmap_read()
> prototype changes, the regmap_range field data types immediately follow
> suit).

I'm fine with regmap_range, but I'm not a regmap maintainer.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 05:39:13PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 23, 2026 at 05:10:49PM +0200, Vladimir Oltean wrote:

...

> I'm fine with regmap_range, but I'm not a regmap maintainer.

Also TIL the range_*() APIs in overflow.h.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 02:15:29PM +0200, Vladimir Oltean wrote:
> On Fri, Jan 23, 2026 at 09:20:58AM +0200, Andy Shevchenko wrote:
> > On Fri, Jan 23, 2026 at 12:18:48AM +0200, Vladimir Oltean wrote:
> > > On Thu, Jan 22, 2026 at 04:38:37PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 22, 2026 at 03:47:04PM +0200, Vladimir Oltean wrote:
> > > > > On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:

...

> > > > > -	if (config->resource)
> > > > > +	if (config->resource) {
> > > > 
> > > > Btw, this might be not enough, one should check size and flags as well
> > > > before use. There was a discussion about this recently. Maybe we should
> > > > just move to a simple unsigned int in the config for now? Because handling
> > > > resources maybe considered as over engineering in this case.
> > > 
> > > The resource flags are never taken into consideration, but I can for
> > > sure replace the resource in struct mdio_regmap_config with just an
> > > unsigned int start and an end, but that doesn't get rid of the resource
> > > usage. The dev_get_resource(dev->parent, NULL) call is how we learn of
> > > where our register window is located in the "one big regmap" provided by
> > > the parent (SJA1105). So we still need this check somewhere else if we
> > > wanted to not fail silently in case of address bits truncation.
> > 
> > Hmm... Bu why we can't embed the full struct resource in such a case?
> 
> We can also embed the full struct resource, I never said we can't...
> 
> > Because resource should have a flag check, otherwise it's a wrong check.
> > 
> > Discussion I mentioned is this:
> > https://lore.kernel.org/lkml/20251207215359.28895-1-ansuelsmth@gmail.com/
> > 
> > Fixes due to that finding:
> > https://lore.kernel.org/lkml/20251208200437.14199-1-ansuelsmth@gmail.com/
> > https://lore.kernel.org/lkml/20251208145654.5294-1-ilpo.jarvinen@linux.intel.com/
> 
> The linked issues seem unrelated; they are caused by the assumption that
> resource_size() can be zero. But I'm not using the resource_size()
> helper, and even if I were, I'm not testing it against zero.

I referred to the full discussion, and not just to the OP message.
During discussion it was explicitly said that:
1) doing

	struct resource foo = {};

is wrong, and

2) checking the resource parameters (start, end), a.k.a. size is wrong
without checking flags.

So it is related.

> As opposed to the PCI BAR case, we don't keep around in an altered form
> the resources exceeding 4G.
> Just need to reject them once and be done with them.

I'm not sure I follow here. The PCI case is much more complicated (it has
resources even in 64-bit space that can be resplit, remerged, etc. It's
a dynamic living thing due to hotplug and bridges and USB4/Thunderbolt.
I am definitely not talking about all of this.

> Also, what else to even check about the resource flags? We get the
> resource using "platform_get_resource(pdev, IORESOURCE_REG, 0)", so we
> know they're of that type. I don't think IORESOURCE_REG resources have
> any other valid bits in flags except for IORESOURCE_TYPE_BITS.

They can (not sure that is possible with current code, but in general)
be disabled, or size can be 0. Maybe even more, I haven't checked that.

> > > > > +		if (config->resource->start > U32_MAX ||
> > > > > +		    config->resource->end > U32_MAX) {
> > > > 
> > > > Ideally it should be resource_overlaps() check. But see above.
> > > 
> > > resource_overlaps_with_what? The only problem is that the resource can
> > > exceed the 32 bit representation that regmap works with.
> > 
> > Obviously with the 4G address space :-)
> > 
> > 	struct resource r4g = DEFINE_RESOURCE...(..., 0, SZ_4G...);
> > 
> > 	if (resource_overlaps(&r4g, config->resource))
> > 		aiaiai! // using %pR to print the content
> 
> This is a buggy replacement of my intention.

Sorry for that, I haven't given enough time to think about it.

> I need to sanity check that
> my IORESOURCE_REG resource is entirely within the 0-4G region.
> 
> The correct way to express this using helpers:
> 
> 	if (!resource_contains(&r4g, config->resource))
> 		nazad!
> 
> but... you see my point? In trying to make use of "standard" helpers, we
> overcomplicate simple things and introduce bugs.

I see, but do you see my points? I may have made a mistake, no doubts,
the point of using helpers is to avoid other, more subtle bugs.

> My initially proposed test can be written even simpler:
> 
> 	if (config->resource->end > U32_MAX) {
> 		...
> 
> because end >= start, so also testing resource->start is redundant.

Not at all, both needs to be checked and flags.

> > > > > +			dev_err(config->parent,
> > > > > +				"Resource exceeds regmap API addressing possibilities\n");
> > > > > +			return ERR_PTR(-EINVAL);
> > > > > +		}
> > > > >  		mr->base = config->resource->start;
> > > > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 01/15] net: mdio-regmap: permit working with non-MMIO regmaps

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 02:13:01PM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 02:06:23PM +0200, Andy Shevchenko wrote:
> > > Cc: Mark Brown <broonie@kernel.org>
> > > Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > 
> > FWIW, Cc list may be located after --- line. It will have the same effect on
> > emails (as regular tooling will parse and put them into email headers), but
> > will reduce unneeded noise in the commit message. List will be still available
> > on lore.kernel.org in the mail archives.
> 
> Thanks for the comment. I know it may be located after ---, but for me,
> doing that implies an extra step which I find unnecessary (moving them
> there after the git format-patch stage). I keep the Cc: in the commit
> message in git so that it's preserved across revisions.

You can keep them in your commit message. Many --- lines are also allowed!

> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > > ---
> > 
> >   Cc: ...
> >   ...

...

> > > +	unsigned int base;
> > 
> > Hmm... resource_size_t ?
> 
> Well, regmap_read() takes "unsigned int reg".
> https://elixir.bootlin.com/linux/v6.18.6/source/include/linux/regmap.h#L1297
> So in practice, a truncation will be done somewhere if the register base
> exceeds unsigned int storage capacity. But I didn't feel that it's worth
> handling that.

Maybe a comment near to the assignment?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 net-next 02/15] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs

[Vladimir Oltean]

This driver is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c.
In terms of differences:

- this one uses regmaps provided by the parent as a method to abstract
  away the sja1105_xfer_u32() calls for register access
- the driver prefix has been changed from sja1105 to sja1110 (this MDIO
  controller is not present on the older SJA1105 family)
- in the sja1105 driver, each memory word has 32 bits, so addresses as
  seen by regmap need to be multiplied by 4. This affects what
  sja1110_base_t1_encode_addr() returns, and is different compared to
  sja1105_base_t1_encode_addr().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: use FIELD_PREP()

 MAINTAINERS                          |   1 +
 drivers/net/mdio/Kconfig             |   7 ++
 drivers/net/mdio/Makefile            |   1 +
 drivers/net/mdio/mdio-sja1110-cbt1.c | 179 +++++++++++++++++++++++++++
 4 files changed, 188 insertions(+)
 create mode 100644 drivers/net/mdio/mdio-sja1110-cbt1.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 92768bceb929..d3fec699c577 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18948,6 +18948,7 @@ M:	Vladimir Oltean <olteanv@gmail.com>
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 F:	drivers/net/dsa/sja1105
+F:	drivers/net/mdio/mdio-sja1110-cbt1.c
 F:	drivers/net/pcs/pcs-xpcs-nxp.c
 
 NXP TDA998X DRM DRIVER
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 44380378911b..9819d1dc18de 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -136,6 +136,13 @@ config MDIO_MOXART
 	  This driver supports the MDIO interface found in the network
 	  interface units of the MOXA ART SoC
 
+config MDIO_SJA1110_CBT1
+	tristate "NXP SJA1110 100BASE-T1 MDIO bus"
+	help
+	  This driver supports the MDIO controller embedded in the NXP SJA1110
+	  automotive Ethernet switches, which is used to access the internal
+	  100BASE-T1 PHYs over SPI.
+
 config MDIO_OCTEON
 	tristate "Octeon and some ThunderX SOCs MDIO buses"
 	depends on (64BIT && OF_MDIO) || COMPILE_TEST
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index fbec636700e7..9abf20d1b030 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
 obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
 obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
+obj-$(CONFIG_MDIO_SJA1110_CBT1)		+= mdio-sja1110-cbt1.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-sja1110-cbt1.c b/drivers/net/mdio/mdio-sja1110-cbt1.c
new file mode 100644
index 000000000000..f170b63c7f69
--- /dev/null
+++ b/drivers/net/mdio/mdio-sja1110-cbt1.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2022-2026 NXP
+ *
+ * NXP SJA1110 100BASE-T1 MDIO bus driver
+ */
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct sja1110_base_t1_private {
+	struct regmap *regmap;
+	struct mii_bus *bus;
+	unsigned int base;
+};
+
+enum sja1110_mdio_opcode {
+	SJA1110_C45_ADDR = 0,
+	SJA1110_C22 = 1,
+	SJA1110_C45_DATA = 2,
+	SJA1110_C45_DATA_AUTOINC = 3,
+};
+
+#define SJA1110_PHYADDR		GENMASK(11, 9)
+#define SJA1110_OPCODE		GENMASK(8, 7)
+#define SJA1110_XAD		GENMASK(6, 2)
+
+static unsigned int sja1110_base_t1_encode_addr(unsigned int phy,
+						enum sja1110_mdio_opcode op,
+						unsigned int xad)
+{
+	return FIELD_PREP(SJA1110_PHYADDR, phy) |
+	       FIELD_PREP(SJA1110_OPCODE, op) |
+	       FIELD_PREP(SJA1110_XAD, xad);
+}
+
+static int sja1110_base_t1_mdio_read_c22(struct mii_bus *bus, int phy, int reg)
+{
+	struct sja1110_base_t1_private *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	unsigned int addr, val;
+	int err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
+
+	err = regmap_read(regmap, priv->base + addr, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int sja1110_base_t1_mdio_read_c45(struct mii_bus *bus, int phy,
+					 int mmd, int reg)
+{
+	struct sja1110_base_t1_private *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	unsigned int addr, val;
+	int err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_ADDR, mmd);
+	err = regmap_write(regmap, priv->base + addr, reg);
+	if (err)
+		return err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
+	err = regmap_read(regmap, priv->base + addr, &val);
+	if (err)
+		return err;
+
+	return val & 0xffff;
+}
+
+static int sja1110_base_t1_mdio_write_c22(struct mii_bus *bus, int phy, int reg,
+					  u16 val)
+{
+	struct sja1110_base_t1_private *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	unsigned int addr;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
+	return regmap_write(regmap, priv->base + addr, val & 0xffff);
+}
+
+static int sja1110_base_t1_mdio_write_c45(struct mii_bus *bus, int phy,
+					  int mmd, int reg, u16 val)
+{
+	struct sja1110_base_t1_private *priv = bus->priv;
+	struct regmap *regmap = priv->regmap;
+	unsigned int addr;
+	int err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_ADDR, mmd);
+	err = regmap_write(regmap, priv->base + addr, reg);
+	if (err)
+		return err;
+
+	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
+	return regmap_write(regmap, priv->base + addr, val & 0xffff);
+}
+
+static int sja1110_base_t1_mdio_probe(struct platform_device *pdev)
+{
+	struct sja1110_base_t1_private *priv;
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	struct resource *res;
+	struct mii_bus *bus;
+	int err;
+
+	if (!dev->of_node || !dev->parent)
+		return -ENODEV;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	bus = mdiobus_alloc_size(sizeof(*priv));
+	if (!bus)
+		return -ENOMEM;
+
+	bus->name = "SJA1110 100base-T1 MDIO bus";
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
+	bus->read = sja1110_base_t1_mdio_read_c22;
+	bus->write = sja1110_base_t1_mdio_write_c22;
+	bus->read_c45 = sja1110_base_t1_mdio_read_c45;
+	bus->write_c45 = sja1110_base_t1_mdio_write_c45;
+	bus->parent = dev;
+	priv = bus->priv;
+	priv->regmap = regmap;
+
+	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (res)
+		priv->base = res->start;
+
+	err = of_mdiobus_register(bus, dev->of_node);
+	if (err)
+		goto err_free_bus;
+
+	priv->bus = bus;
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+
+err_free_bus:
+	mdiobus_free(bus);
+
+	return err;
+}
+
+static void sja1110_base_t1_mdio_remove(struct platform_device *pdev)
+{
+	struct sja1110_base_t1_private *priv = platform_get_drvdata(pdev);
+
+	mdiobus_unregister(priv->bus);
+	mdiobus_free(priv->bus);
+}
+
+static const struct of_device_id sja1110_base_t1_mdio_match[] = {
+	{ .compatible = "nxp,sja1110-base-t1-mdio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sja1110_base_t1_mdio_match);
+
+static struct platform_driver sja1110_base_t1_mdio_driver = {
+	.probe = sja1110_base_t1_mdio_probe,
+	.remove = sja1110_base_t1_mdio_remove,
+	.driver = {
+		.name = "sja1110-base-t1-mdio",
+		.of_match_table = sja1110_base_t1_mdio_match,
+	},
+};
+
+module_platform_driver(sja1110_base_t1_mdio_driver);
+
+MODULE_DESCRIPTION("NXP SJA1110 100BASE-T1 MDIO bus driver");
+MODULE_AUTHOR("Vladimir Oltean <vladimir.oltean@nxp.com>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v2 net-next 02/15] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote:
> This driver is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c.
> In terms of differences:
> 
> - this one uses regmaps provided by the parent as a method to abstract
>   away the sja1105_xfer_u32() calls for register access
> - the driver prefix has been changed from sja1105 to sja1110 (this MDIO
>   controller is not present on the older SJA1105 family)
> - in the sja1105 driver, each memory word has 32 bits, so addresses as
>   seen by regmap need to be multiplied by 4. This affects what
>   sja1110_base_t1_encode_addr() returns, and is different compared to
>   sja1105_base_t1_encode_addr().

...

> +static int sja1110_base_t1_mdio_read_c22(struct mii_bus *bus, int phy, int reg)
> +{
> +	struct sja1110_base_t1_private *priv = bus->priv;
> +	struct regmap *regmap = priv->regmap;
> +	unsigned int addr, val;
> +	int err;
> +
> +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);

GENMASK() ? Or do you have already a defined mask for this?

> +	err = regmap_read(regmap, priv->base + addr, &val);
> +	if (err)
> +		return err;
> +
> +	return val & 0xffff;

lower_16_bits() from wordpart.h?

> +}

...

> +static int sja1110_base_t1_mdio_read_c45(struct mii_bus *bus, int phy,
> +					 int mmd, int reg)
> +{
> +	struct sja1110_base_t1_private *priv = bus->priv;
> +	struct regmap *regmap = priv->regmap;
> +	unsigned int addr, val;
> +	int err;
> +
> +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_ADDR, mmd);
> +	err = regmap_write(regmap, priv->base + addr, reg);
> +	if (err)
> +		return err;
> +
> +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
> +	err = regmap_read(regmap, priv->base + addr, &val);
> +	if (err)
> +		return err;
> +
> +	return val & 0xffff;

Ditto.

> +}

...

> +static int sja1110_base_t1_mdio_write_c22(struct mii_bus *bus, int phy, int reg,
> +					  u16 val)
> +{
> +	struct sja1110_base_t1_private *priv = bus->priv;
> +	struct regmap *regmap = priv->regmap;
> +	unsigned int addr;
> +
> +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
> +	return regmap_write(regmap, priv->base + addr, val & 0xffff);

val is already u16.

> +}

...

> +static int sja1110_base_t1_mdio_write_c45(struct mii_bus *bus, int phy,
> +					  int mmd, int reg, u16 val)
> +{
> +	struct sja1110_base_t1_private *priv = bus->priv;
> +	struct regmap *regmap = priv->regmap;
> +	unsigned int addr;
> +	int err;
> +
> +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_ADDR, mmd);
> +	err = regmap_write(regmap, priv->base + addr, reg);
> +	if (err)
> +		return err;
> +
> +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
> +	return regmap_write(regmap, priv->base + addr, val & 0xffff);

Ditto.

> +}

...

> +static int sja1110_base_t1_mdio_probe(struct platform_device *pdev)
> +{
> +	struct sja1110_base_t1_private *priv;
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	struct mii_bus *bus;
> +	int err;

> +	if (!dev->of_node || !dev->parent)

Can we avoid dereferencing? And perhaps dev_fwnode(dev)?

> +		return -ENODEV;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return -ENODEV;
> +
> +	bus = mdiobus_alloc_size(sizeof(*priv));
> +	if (!bus)
> +		return -ENOMEM;
> +
> +	bus->name = "SJA1110 100base-T1 MDIO bus";
> +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +	bus->read = sja1110_base_t1_mdio_read_c22;
> +	bus->write = sja1110_base_t1_mdio_write_c22;
> +	bus->read_c45 = sja1110_base_t1_mdio_read_c45;
> +	bus->write_c45 = sja1110_base_t1_mdio_write_c45;
> +	bus->parent = dev;
> +	priv = bus->priv;
> +	priv->regmap = regmap;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> +	if (res)
> +		priv->base = res->start;
> +
> +	err = of_mdiobus_register(bus, dev->of_node);
> +	if (err)
> +		goto err_free_bus;
> +
> +	priv->bus = bus;
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +
> +err_free_bus:
> +	mdiobus_free(bus);
> +
> +	return err;
> +}

...

> +static const struct of_device_id sja1110_base_t1_mdio_match[] = {
> +	{ .compatible = "nxp,sja1110-base-t1-mdio", },

Inner comma is redundant.

> +	{},

Terminator is terminator, trailing comma is confusing here.

> +};

...

> +static struct platform_driver sja1110_base_t1_mdio_driver = {
> +	.probe = sja1110_base_t1_mdio_probe,
> +	.remove = sja1110_base_t1_mdio_remove,
> +	.driver = {
> +		.name = "sja1110-base-t1-mdio",
> +		.of_match_table = sja1110_base_t1_mdio_match,
> +	},
> +};

> +

Redundant blank line.

> +module_platform_driver(sja1110_base_t1_mdio_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 02/15] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 02:12:21PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote:
> > This driver is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c.
> > In terms of differences:
> > 
> > - this one uses regmaps provided by the parent as a method to abstract
> >   away the sja1105_xfer_u32() calls for register access
> > - the driver prefix has been changed from sja1105 to sja1110 (this MDIO
> >   controller is not present on the older SJA1105 family)
> > - in the sja1105 driver, each memory word has 32 bits, so addresses as
> >   seen by regmap need to be multiplied by 4. This affects what
> >   sja1110_base_t1_encode_addr() returns, and is different compared to
> >   sja1105_base_t1_encode_addr().
> 
> ...
> 
> > +static int sja1110_base_t1_mdio_read_c22(struct mii_bus *bus, int phy, int reg)
> > +{
> > +	struct sja1110_base_t1_private *priv = bus->priv;
> > +	struct regmap *regmap = priv->regmap;
> > +	unsigned int addr, val;
> > +	int err;
> > +
> > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
> 
> GENMASK() ? Or do you have already a defined mask for this?

Hmm, I can't find a definition for this. In the MDIO world it is
"well known" that clause 22 offers a 5-bit register address space.
So the 0x1f number doesn't seem too magical to me.

But I think my assumptions date since before the MDIO bus API was split
between separate clause 22 and clause 45 reads/writes. I don't know
whether masking reg & 0x1f is the best practice. I'm surprised that
__mdiobus_read() doesn't enforce a limit on "regnum", and I don't see
other MDIO bus drivers explicitly C22 registers >= 32. I really don't
know what is the best practice.

> > +	err = regmap_read(regmap, priv->base + addr, &val);
> > +	if (err)
> > +		return err;
> > +
> > +	return val & 0xffff;
> 
> lower_16_bits() from wordpart.h?

Ok, let's say so.

> > +}
> 
> ...
> 
> > +static int sja1110_base_t1_mdio_read_c45(struct mii_bus *bus, int phy,
> > +					 int mmd, int reg)
> > +{
> > +	struct sja1110_base_t1_private *priv = bus->priv;
> > +	struct regmap *regmap = priv->regmap;
> > +	unsigned int addr, val;
> > +	int err;
> > +
> > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_ADDR, mmd);
> > +	err = regmap_write(regmap, priv->base + addr, reg);
> > +	if (err)
> > +		return err;
> > +
> > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
> > +	err = regmap_read(regmap, priv->base + addr, &val);
> > +	if (err)
> > +		return err;
> > +
> > +	return val & 0xffff;
> 
> Ditto.
> 
> > +}
> 
> ...
> 
> > +static int sja1110_base_t1_mdio_write_c22(struct mii_bus *bus, int phy, int reg,
> > +					  u16 val)
> > +{
> > +	struct sja1110_base_t1_private *priv = bus->priv;
> > +	struct regmap *regmap = priv->regmap;
> > +	unsigned int addr;
> > +
> > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
> > +	return regmap_write(regmap, priv->base + addr, val & 0xffff);
> 
> val is already u16.

Ok.

> > +}
> 
> ...
> 
> > +static int sja1110_base_t1_mdio_write_c45(struct mii_bus *bus, int phy,
> > +					  int mmd, int reg, u16 val)
> > +{
> > +	struct sja1110_base_t1_private *priv = bus->priv;
> > +	struct regmap *regmap = priv->regmap;
> > +	unsigned int addr;
> > +	int err;
> > +
> > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_ADDR, mmd);
> > +	err = regmap_write(regmap, priv->base + addr, reg);
> > +	if (err)
> > +		return err;
> > +
> > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C45_DATA, mmd);
> > +	return regmap_write(regmap, priv->base + addr, val & 0xffff);
> 
> Ditto.
> 
> > +}
> 
> ...
> 
> > +static int sja1110_base_t1_mdio_probe(struct platform_device *pdev)
> > +{
> > +	struct sja1110_base_t1_private *priv;
> > +	struct device *dev = &pdev->dev;
> > +	struct regmap *regmap;
> > +	struct resource *res;
> > +	struct mii_bus *bus;
> > +	int err;
> 
> > +	if (!dev->of_node || !dev->parent)
> 
> Can we avoid dereferencing? And perhaps dev_fwnode(dev)?

Avoid dereferencing what?

> > +		return -ENODEV;
> > +
> > +	regmap = dev_get_regmap(dev->parent, NULL);
> > +	if (!regmap)
> > +		return -ENODEV;
> > +
> > +	bus = mdiobus_alloc_size(sizeof(*priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->name = "SJA1110 100base-T1 MDIO bus";
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > +	bus->read = sja1110_base_t1_mdio_read_c22;
> > +	bus->write = sja1110_base_t1_mdio_write_c22;
> > +	bus->read_c45 = sja1110_base_t1_mdio_read_c45;
> > +	bus->write_c45 = sja1110_base_t1_mdio_write_c45;
> > +	bus->parent = dev;
> > +	priv = bus->priv;
> > +	priv->regmap = regmap;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> > +	if (res)
> > +		priv->base = res->start;
> > +
> > +	err = of_mdiobus_register(bus, dev->of_node);

Why would I use dev_fwnode() if I need to pass it as OF to
of_mdiobus_register() here?

> > +	if (err)
> > +		goto err_free_bus;
> > +
> > +	priv->bus = bus;
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	return 0;
> > +
> > +err_free_bus:
> > +	mdiobus_free(bus);
> > +
> > +	return err;
> > +}
> 
> ...
> 
> > +static const struct of_device_id sja1110_base_t1_mdio_match[] = {
> > +	{ .compatible = "nxp,sja1110-base-t1-mdio", },
> 
> Inner comma is redundant.

Ok.

> > +	{},
> 
> Terminator is terminator, trailing comma is confusing here.

Ok.

> > +};
> 
> ...
> 
> > +static struct platform_driver sja1110_base_t1_mdio_driver = {
> > +	.probe = sja1110_base_t1_mdio_probe,
> > +	.remove = sja1110_base_t1_mdio_remove,
> > +	.driver = {
> > +		.name = "sja1110-base-t1-mdio",
> > +		.of_match_table = sja1110_base_t1_mdio_match,
> > +	},
> > +};
> 
> > +
> 
> Redundant blank line.

Ok.

> > +module_platform_driver(sja1110_base_t1_mdio_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Re: [PATCH v2 net-next 02/15] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 02:47:08PM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 02:12:21PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote:

...

> > > +static int sja1110_base_t1_mdio_read_c22(struct mii_bus *bus, int phy, int reg)
> > > +{
> > > +	struct sja1110_base_t1_private *priv = bus->priv;
> > > +	struct regmap *regmap = priv->regmap;
> > > +	unsigned int addr, val;
> > > +	int err;
> > > +
> > > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
> > 
> > GENMASK() ? Or do you have already a defined mask for this?
> 
> Hmm, I can't find a definition for this. In the MDIO world it is
> "well known" that clause 22 offers a 5-bit register address space.
> So the 0x1f number doesn't seem too magical to me.
> 
> But I think my assumptions date since before the MDIO bus API was split
> between separate clause 22 and clause 45 reads/writes. I don't know
> whether masking reg & 0x1f is the best practice. I'm surprised that
> __mdiobus_read() doesn't enforce a limit on "regnum", and I don't see
> other MDIO bus drivers explicitly C22 registers >= 32. I really don't
> know what is the best practice.

Me neither. At bare minimum to check / perform two things:
- make sure this approach is consistent across the kernel
- define the magic with meaningful name

Maybe (assuming second one is done) fix the rest in the future
via some helper function?

...

> > > +static int sja1110_base_t1_mdio_probe(struct platform_device *pdev)
> > > +{
> > > +	struct sja1110_base_t1_private *priv;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct regmap *regmap;
> > > +	struct resource *res;
> > > +	struct mii_bus *bus;
> > > +	int err;
> > 
> > > +	if (!dev->of_node || !dev->parent)
> > 
> > Can we avoid dereferencing? And perhaps dev_fwnode(dev)?
> 
> Avoid dereferencing what?

of_node

> > > +		return -ENODEV;
> > > +
> > > +	regmap = dev_get_regmap(dev->parent, NULL);
> > > +	if (!regmap)
> > > +		return -ENODEV;
> > > +
> > > +	bus = mdiobus_alloc_size(sizeof(*priv));
> > > +	if (!bus)
> > > +		return -ENOMEM;
> > > +
> > > +	bus->name = "SJA1110 100base-T1 MDIO bus";
> > > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > > +	bus->read = sja1110_base_t1_mdio_read_c22;
> > > +	bus->write = sja1110_base_t1_mdio_write_c22;
> > > +	bus->read_c45 = sja1110_base_t1_mdio_read_c45;
> > > +	bus->write_c45 = sja1110_base_t1_mdio_write_c45;
> > > +	bus->parent = dev;
> > > +	priv = bus->priv;
> > > +	priv->regmap = regmap;
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> > > +	if (res)
> > > +		priv->base = res->start;
> > > +
> > > +	err = of_mdiobus_register(bus, dev->of_node);
> 
> Why would I use dev_fwnode() if I need to pass it as OF to
> of_mdiobus_register() here?

dev_of_node() then. Wondering if we can use fwnode_mdiobus_register_phy() here
(I remember that OF/fwnode code in MDIO/PHY is not trivial, but I don't know
 all the details).

> > > +	if (err)
> > > +		goto err_free_bus;
> > > +
> > > +	priv->bus = bus;
> > > +	platform_set_drvdata(pdev, priv);
> > > +
> > > +	return 0;
> > > +
> > > +err_free_bus:
> > > +	mdiobus_free(bus);
> > > +
> > > +	return err;
> > > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 02/15] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 04:44:47PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 02:47:08PM +0200, Vladimir Oltean wrote:
> > On Thu, Jan 22, 2026 at 02:12:21PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote:
> 
> ...
> 
> > > > +static int sja1110_base_t1_mdio_read_c22(struct mii_bus *bus, int phy, int reg)
> > > > +{
> > > > +	struct sja1110_base_t1_private *priv = bus->priv;
> > > > +	struct regmap *regmap = priv->regmap;
> > > > +	unsigned int addr, val;
> > > > +	int err;
> > > > +
> > > > +	addr = sja1110_base_t1_encode_addr(phy, SJA1110_C22, reg & 0x1f);
> > > 
> > > GENMASK() ? Or do you have already a defined mask for this?
> > 
> > Hmm, I can't find a definition for this. In the MDIO world it is
> > "well known" that clause 22 offers a 5-bit register address space.
> > So the 0x1f number doesn't seem too magical to me.
> > 
> > But I think my assumptions date since before the MDIO bus API was split
> > between separate clause 22 and clause 45 reads/writes. I don't know
> > whether masking reg & 0x1f is the best practice. I'm surprised that
> > __mdiobus_read() doesn't enforce a limit on "regnum", and I don't see
> > other MDIO bus drivers explicitly C22 registers >= 32. I really don't
> > know what is the best practice.
> 
> Me neither. At bare minimum to check / perform two things:
> - make sure this approach is consistent across the kernel
> - define the magic with meaningful name
> 
> Maybe (assuming second one is done) fix the rest in the future
> via some helper function?

I wasn't prepared to go down this rabbit hole, but it turns out that the
__mdiobus_read() and __mdiobus_write() functions do support regnum >= 32.

I don't have time to investigate why that is, plus the fact that the
majority of drivers don't reject such register addresses but truncate
them to 5 bits. In the next version I will:
- add a "#define MII_BUS_MAX_C22_REGNUM	0x1f" in include/linux/phy.h and
  use it to reject registers out of range in mdio-sja1110-cbt1.c.
- allow registers >= 32 in mdio-regmap.c and just disallow what exceeds
  the passed resource->end. Although standard MDIO framing has 5-bit
  register addresses, mdio-regmap.c represents a non-standard linear
  mapping of those registers inside an address space, with no such
  inherent limitation. Besides, as mentioned in commit f3b766d98131
  ("net: phy: add basic driver for NXP CBTX PHY"), the NXP CBTX PHY
  register map extends well beyond the standard 32 registers, and I was
  wondering how to expose the rest. Turns out there isn't any problem as
  long as the PHY and its MDIO controller driver are paired together.

> ...
> 
> > > > +static int sja1110_base_t1_mdio_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct sja1110_base_t1_private *priv;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct regmap *regmap;
> > > > +	struct resource *res;
> > > > +	struct mii_bus *bus;
> > > > +	int err;
> > > 
> > > > +	if (!dev->of_node || !dev->parent)
> > > 
> > > Can we avoid dereferencing? And perhaps dev_fwnode(dev)?
> > 
> > Avoid dereferencing what?
> 
> of_node

Why? The driver is useless when bound to a device without an of_node.
of_mdiobus_register() will fall back gracefully to __mdiobus_register(),
and still technically get registered, but its child PHYs will be
inaccessible through phandles.

> > > > +		return -ENODEV;
> > > > +
> > > > +	regmap = dev_get_regmap(dev->parent, NULL);
> > > > +	if (!regmap)
> > > > +		return -ENODEV;
> > > > +
> > > > +	bus = mdiobus_alloc_size(sizeof(*priv));
> > > > +	if (!bus)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	bus->name = "SJA1110 100base-T1 MDIO bus";
> > > > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > > > +	bus->read = sja1110_base_t1_mdio_read_c22;
> > > > +	bus->write = sja1110_base_t1_mdio_write_c22;
> > > > +	bus->read_c45 = sja1110_base_t1_mdio_read_c45;
> > > > +	bus->write_c45 = sja1110_base_t1_mdio_write_c45;
> > > > +	bus->parent = dev;
> > > > +	priv = bus->priv;
> > > > +	priv->regmap = regmap;
> > > > +
> > > > +	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> > > > +	if (res)
> > > > +		priv->base = res->start;
> > > > +
> > > > +	err = of_mdiobus_register(bus, dev->of_node);
> > 
> > Why would I use dev_fwnode() if I need to pass it as OF to
> > of_mdiobus_register() here?
> 
> dev_of_node() then. Wondering if we can use fwnode_mdiobus_register_phy() here
> (I remember that OF/fwnode code in MDIO/PHY is not trivial, but I don't know
>  all the details).

fwnode_mdiobus_register_phy() shall be read as: "hey MDIO bus, please
register a PHY for this fwnode!"

of_mdiobus_register() shall be read as: "I have this mii_bus structure
and I want it registered as an active MDIO bus, associated with this OF
node".

So the two do not serve the same purpose; one is not the more generic
variant of the other.

There is no fwnode variant of of_mdiobus_register(). Perhaps this
snippet from drivers/net/ethernet/marvell/mvmdio.c can clarify:

	/* For the platforms not supporting DT/ACPI fall-back
	 * to mdiobus_register via of_mdiobus_register.
	 */
	if (is_acpi_node(pdev->dev.fwnode))
		ret = acpi_mdiobus_register(bus, pdev->dev.fwnode);
	else
		ret = of_mdiobus_register(bus, pdev->dev.of_node);

Out of the two API functions, I used OF because that's what I need
to support.

> > > > +	if (err)
> > > > +		goto err_free_bus;
> > > > +
> > > > +	priv->bus = bus;
> > > > +	platform_set_drvdata(pdev, priv);
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_free_bus:
> > > > +	mdiobus_free(bus);
> > > > +
> > > > +	return err;
> > > > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Re: [PATCH v2 net-next 02/15] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs

[Andrew Lunn]

> I wasn't prepared to go down this rabbit hole, but it turns out that the
> __mdiobus_read() and __mdiobus_write() functions do support regnum >= 32.

It could be historical, from before their were C22 and C45 operations.

Previously, both transaction types were passed through one call. The
MSB indicated if C45 should be performed, and there were some macros
to split the number part into MMD and register.

With the current implementation, it should be O.K. to add a range
change.

	Andrew

Re: [PATCH v2 net-next 02/15] net: mdio: add driver for NXP SJA1110 100BASE-T1 embedded PHYs

[Andy Shevchenko]

On Fri, Jan 23, 2026 at 12:10:03AM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 04:44:47PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 22, 2026 at 02:47:08PM +0200, Vladimir Oltean wrote:
> > > On Thu, Jan 22, 2026 at 02:12:21PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 22, 2026 at 12:56:41PM +0200, Vladimir Oltean wrote:

...

> > > > > +	if (!dev->of_node || !dev->parent)
> > > > 
> > > > Can we avoid dereferencing? And perhaps dev_fwnode(dev)?
> > > 
> > > Avoid dereferencing what?
> > 
> > of_node
> 
> Why? The driver is useless when bound to a device without an of_node.
> of_mdiobus_register() will fall back gracefully to __mdiobus_register(),
> and still technically get registered, but its child PHYs will be
> inaccessible through phandles.

dereferencing != use

> > > > > +		return -ENODEV;

What I meant is to avoid accessing of_node directly, use APIs: dev_of_node().

...

> > > > > +	err = of_mdiobus_register(bus, dev->of_node);
> > > 
> > > Why would I use dev_fwnode() if I need to pass it as OF to
> > > of_mdiobus_register() here?
> > 
> > dev_of_node() then. Wondering if we can use fwnode_mdiobus_register_phy() here
> > (I remember that OF/fwnode code in MDIO/PHY is not trivial, but I don't know
> >  all the details).
> 
> fwnode_mdiobus_register_phy() shall be read as: "hey MDIO bus, please
> register a PHY for this fwnode!"
> 
> of_mdiobus_register() shall be read as: "I have this mii_bus structure
> and I want it registered as an active MDIO bus, associated with this OF
> node".
> 
> So the two do not serve the same purpose; one is not the more generic
> variant of the other.
> 
> There is no fwnode variant of of_mdiobus_register(). Perhaps this
> snippet from drivers/net/ethernet/marvell/mvmdio.c can clarify:
> 
> 	/* For the platforms not supporting DT/ACPI fall-back
> 	 * to mdiobus_register via of_mdiobus_register.
> 	 */
> 	if (is_acpi_node(pdev->dev.fwnode))
> 		ret = acpi_mdiobus_register(bus, pdev->dev.fwnode);
> 	else
> 		ret = of_mdiobus_register(bus, pdev->dev.of_node);
> 
> Out of the two API functions, I used OF because that's what I need
> to support.

I see, so perhaps in the future we will see this snipped to be converted to
fwnode_mdiobus_register() then. Thank you for clarification.

> > > > > +	if (err)
> > > > > +		goto err_free_bus;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 net-next 03/15] net: mdio: add generic driver for NXP SJA1110 100BASE-TX embedded PHYs

[Vladimir Oltean]

This is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c.
Same kind of differences between this driver and the embedded DSA one
apply: regmap is being used for register access, and addresses are
multiplied by 4 with regmap.

In fact this is so generic that there is nothing NXP SJA1110 specific
about it at all, and just instantiates mdio-regmap. I decided to name it
mdio-regmap-simple.c in the style of drivers/mfd/simple-mfd-i2c.c which
has support for various vendor compatible strings.

Cc: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- reorder MAINTAINERS entry to be alphabetic
- clarify that platform_get_resource() is optional (thanks to Maxime)
- update copyright to 2026

 MAINTAINERS                           |  1 +
 drivers/net/mdio/Kconfig              | 15 ++++-
 drivers/net/mdio/Makefile             |  1 +
 drivers/net/mdio/mdio-regmap-simple.c | 80 +++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/mdio/mdio-regmap-simple.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d3fec699c577..2b910fdd1122 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15671,6 +15671,7 @@ MDIO REGMAP DRIVER
 M:	Maxime Chevallier <maxime.chevallier@bootlin.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	drivers/net/mdio/mdio-regmap-simple.c
 F:	drivers/net/mdio/mdio-regmap.c
 F:	include/linux/mdio/mdio-regmap.h
 
diff --git a/drivers/net/mdio/Kconfig b/drivers/net/mdio/Kconfig
index 9819d1dc18de..c6e824baf228 100644
--- a/drivers/net/mdio/Kconfig
+++ b/drivers/net/mdio/Kconfig
@@ -179,14 +179,23 @@ config MDIO_REALTEK_RTL9300
 config MDIO_REGMAP
 	tristate
 	help
-	  This driver allows using MDIO devices that are not sitting on a
-	  regular MDIO bus, but still exposes the standard 802.3 register
+	  This support module 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. Users willing to use this driver must explicitly select
+	  regmap. Users willing to use this module must explicitly select
 	  REGMAP.
 
+config MDIO_REGMAP_SIMPLE
+	tristate
+	select MDIO_REGMAP
+	help
+	  Generic platform driver for MDIO buses with a linear address space
+	  that can be directly accessed using the MDIO_REGMAP support code and
+	  need no special handling. The regmap is provided by the parent
+	  device.
+
 config MDIO_THUNDER
 	tristate "ThunderX SOCs MDIO buses"
 	depends on 64BIT
diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 9abf20d1b030..95f201b73a7d 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_MDIO_MVUSB)		+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)		+= mdio-octeon.o
 obj-$(CONFIG_MDIO_REALTEK_RTL9300)	+= mdio-realtek-rtl9300.o
 obj-$(CONFIG_MDIO_REGMAP)		+= mdio-regmap.o
+obj-$(CONFIG_MDIO_REGMAP_SIMPLE)	+= mdio-regmap-simple.o
 obj-$(CONFIG_MDIO_SJA1110_CBT1)		+= mdio-sja1110-cbt1.o
 obj-$(CONFIG_MDIO_SUN4I)		+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_THUNDER)		+= mdio-thunder.o
diff --git a/drivers/net/mdio/mdio-regmap-simple.c b/drivers/net/mdio/mdio-regmap-simple.c
new file mode 100644
index 000000000000..5db1ca50c374
--- /dev/null
+++ b/drivers/net/mdio/mdio-regmap-simple.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2025-2026 NXP
+ *
+ * Generic MDIO bus driver for simple regmap-based MDIO devices
+ *
+ * This driver creates MDIO buses for devices that expose their internal
+ * PHYs or PCS through a regmap interface. It's intended to be a simple,
+ * generic driver similar to simple-mfd-i2c.c.
+ */
+#include <linux/module.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>
+
+struct mdio_regmap_simple_data {
+	u8 valid_addr;
+	bool autoscan;
+};
+
+static const struct mdio_regmap_simple_data nxp_sja1110_base_tx = {
+	.valid_addr = 0,
+	.autoscan = false,
+};
+
+static int mdio_regmap_simple_probe(struct platform_device *pdev)
+{
+	const struct mdio_regmap_simple_data *data;
+	struct mdio_regmap_config config = {};
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	struct mii_bus *bus;
+
+	if (!dev->of_node || !dev->parent)
+		return -ENODEV;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	data = device_get_match_data(dev);
+
+	config.regmap = regmap;
+	config.parent = dev;
+	config.name = dev_name(dev);
+	/* The resource is optional, provided for finding the registers
+	 * within a device-wide non-MMIO regmap
+	 */
+	config.resource = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (data) {
+		config.valid_addr = data->valid_addr;
+		config.autoscan = data->autoscan;
+	}
+
+	return PTR_ERR_OR_ZERO(devm_mdio_regmap_register(dev, &config));
+}
+
+static const struct of_device_id mdio_regmap_simple_match[] = {
+	{
+		.compatible = "nxp,sja1110-base-tx-mdio",
+		.data = &nxp_sja1110_base_tx,
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, mdio_regmap_simple_match);
+
+static struct platform_driver mdio_regmap_simple_driver = {
+	.probe = mdio_regmap_simple_probe,
+	.driver = {
+		.name = "mdio-regmap-simple",
+		.of_match_table = mdio_regmap_simple_match,
+	},
+};
+
+module_platform_driver(mdio_regmap_simple_driver);
+
+MODULE_DESCRIPTION("Generic MDIO bus driver for simple regmap-based devices");
+MODULE_AUTHOR("Vladimir Oltean <vladimir.oltean@nxp.com>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v2 net-next 03/15] net: mdio: add generic driver for NXP SJA1110 100BASE-TX embedded PHYs

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 12:56:42PM +0200, Vladimir Oltean wrote:
> This is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c.
> Same kind of differences between this driver and the embedded DSA one
> apply: regmap is being used for register access, and addresses are
> multiplied by 4 with regmap.
> 
> In fact this is so generic that there is nothing NXP SJA1110 specific
> about it at all, and just instantiates mdio-regmap. I decided to name it
> mdio-regmap-simple.c in the style of drivers/mfd/simple-mfd-i2c.c which
> has support for various vendor compatible strings.

...

> +#include <linux/module.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>

...

> +static const struct mdio_regmap_simple_data nxp_sja1110_base_tx = {
> +	.valid_addr = 0,
> +	.autoscan = false,
> +};

Actually the  { } is enough to initialise that. But if you want to be super
explicit... :-)

...

> +static int mdio_regmap_simple_probe(struct platform_device *pdev)
> +{
> +	const struct mdio_regmap_simple_data *data;
> +	struct mdio_regmap_config config = {};
> +	struct device *dev = &pdev->dev;
> +	struct regmap *regmap;
> +	struct mii_bus *bus;
> +
> +	if (!dev->of_node || !dev->parent)

dev->of_node check is not needed, see below.

> +		return -ENODEV;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		return -ENODEV;
> +
> +	data = device_get_match_data(dev);
> +
> +	config.regmap = regmap;
> +	config.parent = dev;
> +	config.name = dev_name(dev);
> +	/* The resource is optional, provided for finding the registers
> +	 * within a device-wide non-MMIO regmap
> +	 */
> +	config.resource = platform_get_resource(pdev, IORESOURCE_REG, 0);

> +	if (data) {

We may always require data to be present. As you use a default one anyway.

> +		config.valid_addr = data->valid_addr;
> +		config.autoscan = data->autoscan;
> +	}

And if it is not provided we will have a crash which is fine. It will just
point that the code was not ever been run on real HW.

> +	return PTR_ERR_OR_ZERO(devm_mdio_regmap_register(dev, &config));
> +}

...

> +static struct platform_driver mdio_regmap_simple_driver = {
> +	.probe = mdio_regmap_simple_probe,
> +	.driver = {
> +		.name = "mdio-regmap-simple",
> +		.of_match_table = mdio_regmap_simple_match,
> +	},
> +};

> +

Unneeded blank line.

> +module_platform_driver(mdio_regmap_simple_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 03/15] net: mdio: add generic driver for NXP SJA1110 100BASE-TX embedded PHYs

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 02:20:22PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 12:56:42PM +0200, Vladimir Oltean wrote:
> > This is the standalone variant of drivers/net/dsa/sja1105/sja1105_mdio.c.
> > Same kind of differences between this driver and the embedded DSA one
> > apply: regmap is being used for register access, and addresses are
> > multiplied by 4 with regmap.
> > 
> > In fact this is so generic that there is nothing NXP SJA1110 specific
> > about it at all, and just instantiates mdio-regmap. I decided to name it
> > mdio-regmap-simple.c in the style of drivers/mfd/simple-mfd-i2c.c which
> > has support for various vendor compatible strings.
> 
> ...
> 
> > +#include <linux/module.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>
> 
> ...
> 
> > +static const struct mdio_regmap_simple_data nxp_sja1110_base_tx = {
> > +	.valid_addr = 0,
> > +	.autoscan = false,
> > +};
> 
> Actually the  { } is enough to initialise that. But if you want to be super
> explicit... :-)
> ...

Yes, I guess I do.

> > +static int mdio_regmap_simple_probe(struct platform_device *pdev)
> > +{
> > +	const struct mdio_regmap_simple_data *data;
> > +	struct mdio_regmap_config config = {};
> > +	struct device *dev = &pdev->dev;
> > +	struct regmap *regmap;
> > +	struct mii_bus *bus;
> > +
> > +	if (!dev->of_node || !dev->parent)
> 
> dev->of_node check is not needed, see below.

Oh.... this is a bug. dev->of_node should have been propagated to
devm_mdio_regmap_register() -> devm_mdiobus_register(), turning it into
devm_of_mdiobus_register().

It shows that my SJA1110 testing platform (Bluebox 3) doesn't have the
CBTX PHY routed to pinout, since I didn't catch this... I'll fix this
for v3.

> > +		return -ENODEV;
> > +
> > +	regmap = dev_get_regmap(dev->parent, NULL);
> > +	if (!regmap)
> > +		return -ENODEV;
> > +
> > +	data = device_get_match_data(dev);
> > +
> > +	config.regmap = regmap;
> > +	config.parent = dev;
> > +	config.name = dev_name(dev);
> > +	/* The resource is optional, provided for finding the registers
> > +	 * within a device-wide non-MMIO regmap
> > +	 */
> > +	config.resource = platform_get_resource(pdev, IORESOURCE_REG, 0);
> 
> > +	if (data) {
> 
> We may always require data to be present. As you use a default one anyway.
> 
> > +		config.valid_addr = data->valid_addr;
> > +		config.autoscan = data->autoscan;
> > +	}
> 
> And if it is not provided we will have a crash which is fine. It will just
> point that the code was not ever been run on real HW.

Hmm. This patch is super old, so I'm revisiting it with foreign eyes,
same as you.

I think the case with .valid_addr = 0 and .autoscan = false will
constitute the vast majority of instantiations of this driver.
I would like to avoid the proliferation of the same basic config with
100 different names (nxp_sja1110_base_tx, etc).

So for v3 I'm planning to:
- rename nxp_sja1110_base_tx to mdio_regmap_simple_default_data
- delete the "if (data)" conditional and directly assign from
  device_get_match_data() to the config structure

Thanks for taking a look.

> > +	return PTR_ERR_OR_ZERO(devm_mdio_regmap_register(dev, &config));
> > +}
> 
> ...
> 
> > +static struct platform_driver mdio_regmap_simple_driver = {
> > +	.probe = mdio_regmap_simple_probe,
> > +	.driver = {
> > +		.name = "mdio-regmap-simple",
> > +		.of_match_table = mdio_regmap_simple_match,
> > +	},
> > +};
> 
> > +
> 
> Unneeded blank line.

Ok.

> > +module_platform_driver(mdio_regmap_simple_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Re: [PATCH v2 net-next 03/15] net: mdio: add generic driver for NXP SJA1110 100BASE-TX embedded PHYs

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 03:31:23PM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 02:20:22PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 22, 2026 at 12:56:42PM +0200, Vladimir Oltean wrote:

...

> > > +static int mdio_regmap_simple_probe(struct platform_device *pdev)
> > > +{
> > > +	const struct mdio_regmap_simple_data *data;
> > > +	struct mdio_regmap_config config = {};
> > > +	struct device *dev = &pdev->dev;
> > > +	struct regmap *regmap;
> > > +	struct mii_bus *bus;
> > > +
> > > +	if (!dev->of_node || !dev->parent)
> > 
> > dev->of_node check is not needed, see below.
> 
> Oh.... this is a bug. dev->of_node should have been propagated to
> devm_mdio_regmap_register() -> devm_mdiobus_register(), turning it into
> devm_of_mdiobus_register().
> 
> It shows that my SJA1110 testing platform (Bluebox 3) doesn't have the
> CBTX PHY routed to pinout, since I didn't catch this... I'll fix this
> for v3.

Same Q, why not using fwnode APIs to begin with?

> > > +		return -ENODEV;
> > > +
> > > +	regmap = dev_get_regmap(dev->parent, NULL);
> > > +	if (!regmap)
> > > +		return -ENODEV;
> > > +
> > > +	data = device_get_match_data(dev);
> > > +
> > > +	config.regmap = regmap;
> > > +	config.parent = dev;
> > > +	config.name = dev_name(dev);
> > > +	/* The resource is optional, provided for finding the registers
> > > +	 * within a device-wide non-MMIO regmap
> > > +	 */
> > > +	config.resource = platform_get_resource(pdev, IORESOURCE_REG, 0);
> > 
> > > +	if (data) {
> > 
> > We may always require data to be present. As you use a default one anyway.
> > 
> > > +		config.valid_addr = data->valid_addr;
> > > +		config.autoscan = data->autoscan;
> > > +	}
> > 
> > And if it is not provided we will have a crash which is fine. It will just
> > point that the code was not ever been run on real HW.
> 
> Hmm. This patch is super old, so I'm revisiting it with foreign eyes,
> same as you.
> 
> I think the case with .valid_addr = 0 and .autoscan = false will
> constitute the vast majority of instantiations of this driver.
> I would like to avoid the proliferation of the same basic config with
> 100 different names (nxp_sja1110_base_tx, etc).

But you name it as default_blablalbla. That will be just assigned to each
currently "NULL" driver_data. We have examples in the kernel that do this.
IIRC 8250_dw cases, stmmac driver (PCI glue part of it?), et cetera...

> So for v3 I'm planning to:
> - rename nxp_sja1110_base_tx to mdio_regmap_simple_default_data
> - delete the "if (data)" conditional and directly assign from
>   device_get_match_data() to the config structure
> 
> Thanks for taking a look.

You're welcome!

> > > +	return PTR_ERR_OR_ZERO(devm_mdio_regmap_register(dev, &config));
> > > +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 net-next 04/15] net: dsa: sja1105: prepare regmap for passing to child devices

[Vladimir Oltean]

Prepare a single regmap covering the entire SPI address space of the
SJA1105 and SJA1110 switches which can be given to MDIO buses, XPCS,
irqchip drivers etc.

This regmap is address-zero-based (can access the entire switch address
space) and child devices are supposed to access their respective memory
region with the help of struct resource (IORESOURCE_REG, to be precise).

Nothing is currently done with the regmap, it is just allocated and
added to the device's devres list, so it doesn't need to be freed.

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- s/sja1105_create_regmap()/devm_sja1105_create_regmap()/ for clarity in
  sja1105_probe() that teardown is not needed.
- drop unnecessary priv->regmap

 drivers/net/dsa/sja1105/sja1105.h      |  3 ++
 drivers/net/dsa/sja1105/sja1105_main.c |  6 +++
 drivers/net/dsa/sja1105/sja1105_spi.c  | 55 ++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index dceb96ae9c83..8d4c0c8df326 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -11,6 +11,8 @@
 #include <linux/dsa/8021q.h>
 #include <net/dsa.h>
 #include <linux/mutex.h>
+#include <linux/regmap.h>
+
 #include "sja1105_static_config.h"
 
 #define SJA1105ET_FDB_BIN_SIZE		4
@@ -338,6 +340,7 @@ int static_config_buf_prepare_for_upload(struct sja1105_private *priv,
 int sja1105_static_config_upload(struct sja1105_private *priv);
 int sja1105_inhibit_tx(const struct sja1105_private *priv,
 		       unsigned long port_bitmap, bool tx_inhibited);
+int devm_sja1105_create_regmap(struct sja1105_private *priv);
 
 extern const struct sja1105_info sja1105e_info;
 extern const struct sja1105_info sja1105t_info;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 2a4a0fe20dae..e9e091cf8998 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3291,6 +3291,12 @@ static int sja1105_probe(struct spi_device *spi)
 
 	priv->info = of_device_get_match_data(dev);
 
+	rc = devm_sja1105_create_regmap(priv);
+	if (rc < 0) {
+		dev_err(dev, "Failed to create regmap: %pe\n", ERR_PTR(rc));
+		return rc;
+	}
+
 	/* Detect hardware device */
 	rc = sja1105_check_device_id(priv);
 	if (rc < 0) {
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index 834b5c1b4db0..856a751de53a 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -408,6 +408,61 @@ int sja1105_static_config_upload(struct sja1105_private *priv)
 	return rc;
 }
 
+static int sja1105_regmap_bus_reg_read(void *ctx, unsigned int reg,
+				       unsigned int *val)
+{
+	struct sja1105_private *priv = ctx;
+	u32 tmp;
+	int rc;
+
+	rc = sja1105_xfer_u32(priv, SPI_READ, SJA1110_SPI_ADDR(reg), &tmp,
+			      NULL);
+	if (rc)
+		return rc;
+
+	*val = tmp;
+
+	return 0;
+}
+
+static int sja1105_regmap_bus_reg_write(void *ctx, unsigned int reg,
+					unsigned int val)
+{
+	struct sja1105_private *priv = ctx;
+	u32 tmp = val;
+
+	return sja1105_xfer_u32(priv, SPI_WRITE, SJA1110_SPI_ADDR(reg), &tmp,
+				NULL);
+}
+
+/* The primary purpose of this is to pass it to child devices,
+ * not to abstract SPI access for the main driver.
+ */
+int devm_sja1105_create_regmap(struct sja1105_private *priv)
+{
+	static const struct regmap_bus sja1105_regmap_bus = {
+		.reg_read = sja1105_regmap_bus_reg_read,
+		.reg_write = sja1105_regmap_bus_reg_write,
+		.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+		.val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+	};
+	static const struct regmap_config regmap_config = {
+		.name = "regs",
+		.reg_bits = 32,
+		.val_bits = 32,
+		.reg_stride = 4,
+	};
+	struct device *dev = &priv->spidev->dev;
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init(dev, &sja1105_regmap_bus, priv,
+				  &regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return 0;
+}
+
 static const struct sja1105_regs sja1105et_regs = {
 	.device_id = 0x0,
 	.prod_id = 0x100BC3,
-- 
2.34.1

Re: [PATCH v2 net-next 04/15] net: dsa: sja1105: prepare regmap for passing to child devices

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 12:56:43PM +0200, Vladimir Oltean wrote:
> Prepare a single regmap covering the entire SPI address space of the
> SJA1105 and SJA1110 switches which can be given to MDIO buses, XPCS,
> irqchip drivers etc.
> 
> This regmap is address-zero-based (can access the entire switch address
> space) and child devices are supposed to access their respective memory
> region with the help of struct resource (IORESOURCE_REG, to be precise).
> 
> Nothing is currently done with the regmap, it is just allocated and
> added to the device's devres list, so it doesn't need to be freed.

...

> --- a/drivers/net/dsa/sja1105/sja1105.h
> +++ b/drivers/net/dsa/sja1105/sja1105.h

>  #include <linux/dsa/8021q.h>
>  #include <net/dsa.h>
>  #include <linux/mutex.h>
> +#include <linux/regmap.h>
> +
>  #include "sja1105_static_config.h"

It looks to me that somebody missed grouping above (not this change) and it's
better to have it done:

#include <linux/dsa/8021q.h>
#include <linux/mutex.h>
#include <linux/regmap.h>

#include <net/dsa.h>

#include "sja1105_static_config.h"

...

> +	rc = devm_sja1105_create_regmap(priv);
> +	if (rc < 0) {
> +		dev_err(dev, "Failed to create regmap: %pe\n", ERR_PTR(rc));
> +		return rc;
> +	}

Hmm... Perhaps return dev_err_probe(...); ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 04/15] net: dsa: sja1105: prepare regmap for passing to child devices

[Vladimir Oltean]

On Thu, Jan 22, 2026 at 02:23:18PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 12:56:43PM +0200, Vladimir Oltean wrote:
> > Prepare a single regmap covering the entire SPI address space of the
> > SJA1105 and SJA1110 switches which can be given to MDIO buses, XPCS,
> > irqchip drivers etc.
> > 
> > This regmap is address-zero-based (can access the entire switch address
> > space) and child devices are supposed to access their respective memory
> > region with the help of struct resource (IORESOURCE_REG, to be precise).
> > 
> > Nothing is currently done with the regmap, it is just allocated and
> > added to the device's devres list, so it doesn't need to be freed.
> 
> ...
> 
> > --- a/drivers/net/dsa/sja1105/sja1105.h
> > +++ b/drivers/net/dsa/sja1105/sja1105.h
> 
> >  #include <linux/dsa/8021q.h>
> >  #include <net/dsa.h>
> >  #include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +
> >  #include "sja1105_static_config.h"
> 
> It looks to me that somebody missed grouping above (not this change) and it's
> better to have it done:
> 
> #include <linux/dsa/8021q.h>
> #include <linux/mutex.h>
> #include <linux/regmap.h>
> 
> #include <net/dsa.h>
> 
> #include "sja1105_static_config.h"

That somebody would be me, but I don't consider this a relevant change
for this patch set.

> ...
> 
> > +	rc = devm_sja1105_create_regmap(priv);
> > +	if (rc < 0) {
> > +		dev_err(dev, "Failed to create regmap: %pe\n", ERR_PTR(rc));
> > +		return rc;
> > +	}
> 
> Hmm... Perhaps return dev_err_probe(...); ?

I never understood the point of dev_err_probe() when you know the return
code can never be -EPROBE_DEFER.

> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

Re: [PATCH v2 net-next 04/15] net: dsa: sja1105: prepare regmap for passing to child devices

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 03:42:45PM +0200, Vladimir Oltean wrote:
> On Thu, Jan 22, 2026 at 02:23:18PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 22, 2026 at 12:56:43PM +0200, Vladimir Oltean wrote:

...

> > > --- a/drivers/net/dsa/sja1105/sja1105.h
> > > +++ b/drivers/net/dsa/sja1105/sja1105.h
> > 
> > >  #include <linux/dsa/8021q.h>
> > >  #include <net/dsa.h>
> > >  #include <linux/mutex.h>
> > > +#include <linux/regmap.h>
> > > +
> > >  #include "sja1105_static_config.h"
> > 
> > It looks to me that somebody missed grouping above (not this change) and it's
> > better to have it done:
> > 
> > #include <linux/dsa/8021q.h>
> > #include <linux/mutex.h>
> > #include <linux/regmap.h>
> > 
> > #include <net/dsa.h>
> > 
> > #include "sja1105_static_config.h"
> 
> That somebody would be me, but I don't consider this a relevant change
> for this patch set.

While it's true, this patch already adds blank line which suggests that the
author (would be you :-) has something like grouping in mind. Otherwise just
place a new one before net/* which might be more logical in accordance with
your justification (of not doing grouping here).

But I'm not going to fight to death for this :-) Your choice!

...

> > > +	rc = devm_sja1105_create_regmap(priv);
> > > +	if (rc < 0) {
> > > +		dev_err(dev, "Failed to create regmap: %pe\n", ERR_PTR(rc));
> > > +		return rc;
> > > +	}
> > 
> > Hmm... Perhaps return dev_err_probe(...); ?
> 
> I never understood the point of dev_err_probe() when you know the return
> code can never be -EPROBE_DEFER.

i) Smaller code; ii) no need to care about: a) deferred probe cases;
b) -ENOMEM cases.  I see only benefits here by using it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 net-next 04/15] net: dsa: sja1105: prepare regmap for passing to child devices

[Russell King (Oracle)]

On Thu, Jan 22, 2026 at 04:54:11PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 22, 2026 at 03:42:45PM +0200, Vladimir Oltean wrote:
> > I never understood the point of dev_err_probe() when you know the return
> > code can never be -EPROBE_DEFER.
> 
> i) Smaller code; ii) no need to care about: a) deferred probe cases;
> b) -ENOMEM cases.  I see only benefits here by using it.

Isn't it also used for /sys/kernel/debug/devices_deferred to report to
the user ?

E.g.

supply-voltage-monitor  iio_hwmon: Failed to get channels

produced by drivers/hwmon/iio_hwmon.c::iio_hwmon_probe():

        channels = devm_iio_channel_get_all(dev);
        if (IS_ERR(channels)) {
                ret = PTR_ERR(channels);
                if (ret == -ENODEV)
                        ret = -EPROBE_DEFER;
                return dev_err_probe(dev, ret,
                                     "Failed to get channels\n");
        }


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 net-next 04/15] net: dsa: sja1105: prepare regmap for passing to child devices

[Andy Shevchenko]

On Thu, Jan 22, 2026 at 04:17:59PM +0000, Russell King (Oracle) wrote:
> On Thu, Jan 22, 2026 at 04:54:11PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 22, 2026 at 03:42:45PM +0200, Vladimir Oltean wrote:
> > > I never understood the point of dev_err_probe() when you know the return
> > > code can never be -EPROBE_DEFER.
> > 
> > i) Smaller code; ii) no need to care about: a) deferred probe cases;
> > b) -ENOMEM cases.  I see only benefits here by using it.
> 
> Isn't it also used for /sys/kernel/debug/devices_deferred to report to
> the user ?
> 
> E.g.
> 
> supply-voltage-monitor  iio_hwmon: Failed to get channels
> 
> produced by drivers/hwmon/iio_hwmon.c::iio_hwmon_probe():
> 
>         channels = devm_iio_channel_get_all(dev);
>         if (IS_ERR(channels)) {
>                 ret = PTR_ERR(channels);
>                 if (ret == -ENODEV)
>                         ret = -EPROBE_DEFER;
>                 return dev_err_probe(dev, ret,
>                                      "Failed to get channels\n");
>         }

Yes, that's what I imply by "deferred probe cases".

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 net-next 05/15] net: dsa: sja1105: include spi.h from sja1105.h

[Vladimir Oltean]

We have a reference to struct spi_device, but users of sja1105.h cannot
dereference it if they need to. One such example will come in the next
change, where sja1105_mdio.c does not include <linux/spi.h>, so it
cannot dereference priv->spidev->dev.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 drivers/net/dsa/sja1105/sja1105.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 8d4c0c8df326..30903fb62302 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -9,6 +9,7 @@
 #include <linux/timecounter.h>
 #include <linux/dsa/sja1105.h>
 #include <linux/dsa/8021q.h>
+#include <linux/spi/spi.h>
 #include <net/dsa.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
-- 
2.34.1

[PATCH v2 net-next 06/15] net: dsa: sja1105: transition OF-based MDIO controllers to standalone sub-devices

[Vladimir Oltean]

The switch driver never interacts directly with the PHYs from its
internal 100Base-T1 or 100Base-TX MDIO buses, but rather, uses the
generic phylink library to follow "phy-handle" fwnode references to
them.

These MDIO buses are currently created by the DSA driver for historical
reasons, but they have well-defined address space regions for their
registers, which do not collide with the DSA switch registers. In the
SJA1110 memory map, the important resources look something like this:

Name         Description                                         Start      End
SWITCH       Ethernet Switch Subsystem                           0x000000   0x3ffffc
100BASE-T1   Internal MDIO bus for 100BASE-T1 PHY (port 5 - 10)  0x704000   0x704ffc
SGMII1       SGMII Port 1                                        0x705000   0x705ffc
SGMII2       SGMII Port 2                                        0x706000   0x706ffc
SGMII3       SGMII Port 3                                        0x707000   0x707ffc
SGMII4       SGMII Port 4                                        0x708000   0x708ffc
100BASE-TX   Internal MDIO bus for 100BASE-TX PHY                0x709000   0x709ffc
ACU          Auxiliary Control Unit                              0x711000   0x711ffc
GPIO         General Purpose Input/Output                        0x712000   0x712ffc

OTOH, dedicated standalone platform_device drivers now exist for the two
controllers found in the 100BASE-T1 and 100BASE-TX regions. If we could
leverage them, we could simplify the DSA driver code by removing the
weakly coupled components.

There was an initial attempt to use mfd_add_devices() to probe these
children, but:
- Using mfd_add_devices() liberally outside of drivers/mfd/ causes
  "minor chaos" in the words of the maintainer
- The SJA1110 dt-bindings, where there exists one more hierarchical
  level between the OF node of the parent and the OF nodes of the
  children (aka the "mdios" container node), is too unconventional for
  MFD:
  https://lore.kernel.org/netdev/20251118190530.580267-8-vladimir.oltean@nxp.com/

So I turned to something custom based on platform_device_register_full()
instead. The OF nodes of the known MDIO controllers are searched for,
platform devices are created for them, and they are given hardcoded
resources which correspond to the above 100BASE-T1 and 100BASE-TX regions.
The platform drivers will use these regions to segment the SPI device's
regmap in order to access just their own registers.

Delete the duplicated drivers for 100base-T1 and 100base-TX from the DSA
driver, which are now unnecessary.

Cc: Lee Jones <lee@kernel.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- replace mfd_add_devices() with devm_sja1105_add_subdevs()

 drivers/net/dsa/sja1105/Makefile         |   1 +
 drivers/net/dsa/sja1105/sja1105.h        |   4 -
 drivers/net/dsa/sja1105/sja1105_main.c   |   8 +
 drivers/net/dsa/sja1105/sja1105_mdio.c   | 270 +----------------------
 drivers/net/dsa/sja1105/sja1105_spi.c    |   6 -
 drivers/net/dsa/sja1105/sja1105_subdev.c | 154 +++++++++++++
 drivers/net/dsa/sja1105/sja1105_subdev.h |   9 +
 7 files changed, 173 insertions(+), 279 deletions(-)
 create mode 100644 drivers/net/dsa/sja1105/sja1105_subdev.c
 create mode 100644 drivers/net/dsa/sja1105/sja1105_subdev.h

diff --git a/drivers/net/dsa/sja1105/Makefile b/drivers/net/dsa/sja1105/Makefile
index 40d69e6c0bae..7b5537d67072 100644
--- a/drivers/net/dsa/sja1105/Makefile
+++ b/drivers/net/dsa/sja1105/Makefile
@@ -5,6 +5,7 @@ sja1105-objs := \
     sja1105_spi.o \
     sja1105_main.o \
     sja1105_mdio.o \
+    sja1105_subdev.o \
     sja1105_flower.o \
     sja1105_ethtool.o \
     sja1105_devlink.o \
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 30903fb62302..cf718e7c2b7b 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -91,8 +91,6 @@ struct sja1105_regs {
 	u64 rmii_ref_clk[SJA1105_MAX_NUM_PORTS];
 	u64 rmii_ext_tx_clk[SJA1105_MAX_NUM_PORTS];
 	u64 stats[__MAX_SJA1105_STATS_AREA][SJA1105_MAX_NUM_PORTS];
-	u64 mdio_100base_tx;
-	u64 mdio_100base_t1;
 	u64 pcs_base[SJA1105_MAX_NUM_PORTS];
 };
 
@@ -278,8 +276,6 @@ struct sja1105_private {
 	struct mutex dynamic_config_lock;
 	struct devlink_region **regions;
 	struct sja1105_cbs_entry *cbs;
-	struct mii_bus *mdio_base_t1;
-	struct mii_bus *mdio_base_tx;
 	struct mii_bus *mdio_pcs;
 	struct phylink_pcs *pcs[SJA1105_MAX_NUM_PORTS];
 	struct sja1105_ptp_data ptp_data;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index e9e091cf8998..d3fb42772071 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -23,6 +23,7 @@
 #include <linux/units.h>
 
 #include "sja1105.h"
+#include "sja1105_subdev.h"
 #include "sja1105_tas.h"
 
 #define SJA1105_UNKNOWN_MULTICAST	0x010000000000ull
@@ -3329,6 +3330,13 @@ static int sja1105_probe(struct spi_device *spi)
 		return rc;
 	}
 
+	rc = devm_sja1105_add_subdevs(ds);
+	if (rc) {
+		dev_err(ds->dev, "Failed to create child devices: %pe\n",
+			ERR_PTR(rc));
+		return rc;
+	}
+
 	if (IS_ENABLED(CONFIG_NET_SCH_CBS)) {
 		priv->cbs = devm_kcalloc(dev, priv->info->num_cbs_shapers,
 					 sizeof(struct sja1105_cbs_entry),
diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
index 8d535c033cef..b803ce71f5cc 100644
--- a/drivers/net/dsa/sja1105/sja1105_mdio.c
+++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
@@ -133,238 +133,6 @@ int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg,
 				&tmp, NULL);
 }
 
-enum sja1105_mdio_opcode {
-	SJA1105_C45_ADDR = 0,
-	SJA1105_C22 = 1,
-	SJA1105_C45_DATA = 2,
-	SJA1105_C45_DATA_AUTOINC = 3,
-};
-
-static u64 sja1105_base_t1_encode_addr(struct sja1105_private *priv,
-				       int phy, enum sja1105_mdio_opcode op,
-				       int xad)
-{
-	const struct sja1105_regs *regs = priv->info->regs;
-
-	return regs->mdio_100base_t1 | (phy << 7) | (op << 5) | (xad << 0);
-}
-
-static int sja1105_base_t1_mdio_read_c22(struct mii_bus *bus, int phy, int reg)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	u64 addr;
-	u32 tmp;
-	int rc;
-
-	addr = sja1105_base_t1_encode_addr(priv, phy, SJA1105_C22, reg & 0x1f);
-
-	rc = sja1105_xfer_u32(priv, SPI_READ, addr, &tmp, NULL);
-	if (rc < 0)
-		return rc;
-
-	return tmp & 0xffff;
-}
-
-static int sja1105_base_t1_mdio_read_c45(struct mii_bus *bus, int phy,
-					 int mmd, int reg)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	u64 addr;
-	u32 tmp;
-	int rc;
-
-	addr = sja1105_base_t1_encode_addr(priv, phy, SJA1105_C45_ADDR, mmd);
-
-	rc = sja1105_xfer_u32(priv, SPI_WRITE, addr, &reg, NULL);
-	if (rc < 0)
-		return rc;
-
-	addr = sja1105_base_t1_encode_addr(priv, phy, SJA1105_C45_DATA, mmd);
-
-	rc = sja1105_xfer_u32(priv, SPI_READ, addr, &tmp, NULL);
-	if (rc < 0)
-		return rc;
-
-	return tmp & 0xffff;
-}
-
-static int sja1105_base_t1_mdio_write_c22(struct mii_bus *bus, int phy, int reg,
-					  u16 val)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	u64 addr;
-	u32 tmp;
-
-	addr = sja1105_base_t1_encode_addr(priv, phy, SJA1105_C22, reg & 0x1f);
-
-	tmp = val & 0xffff;
-
-	return sja1105_xfer_u32(priv, SPI_WRITE, addr, &tmp, NULL);
-}
-
-static int sja1105_base_t1_mdio_write_c45(struct mii_bus *bus, int phy,
-					  int mmd, int reg, u16 val)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	u64 addr;
-	u32 tmp;
-	int rc;
-
-	addr = sja1105_base_t1_encode_addr(priv, phy, SJA1105_C45_ADDR, mmd);
-
-	rc = sja1105_xfer_u32(priv, SPI_WRITE, addr, &reg, NULL);
-	if (rc < 0)
-		return rc;
-
-	addr = sja1105_base_t1_encode_addr(priv, phy, SJA1105_C45_DATA, mmd);
-
-	tmp = val & 0xffff;
-
-	return sja1105_xfer_u32(priv, SPI_WRITE, addr, &tmp, NULL);
-}
-
-static int sja1105_base_tx_mdio_read(struct mii_bus *bus, int phy, int reg)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	const struct sja1105_regs *regs = priv->info->regs;
-	u32 tmp;
-	int rc;
-
-	rc = sja1105_xfer_u32(priv, SPI_READ, regs->mdio_100base_tx + reg,
-			      &tmp, NULL);
-	if (rc < 0)
-		return rc;
-
-	return tmp & 0xffff;
-}
-
-static int sja1105_base_tx_mdio_write(struct mii_bus *bus, int phy, int reg,
-				      u16 val)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	const struct sja1105_regs *regs = priv->info->regs;
-	u32 tmp = val;
-
-	return sja1105_xfer_u32(priv, SPI_WRITE, regs->mdio_100base_tx + reg,
-				&tmp, NULL);
-}
-
-static int sja1105_mdiobus_base_tx_register(struct sja1105_private *priv,
-					    struct device_node *mdio_node)
-{
-	struct sja1105_mdio_private *mdio_priv;
-	struct device_node *np;
-	struct mii_bus *bus;
-	int rc = 0;
-
-	np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-tx-mdio");
-	if (!np)
-		return 0;
-
-	if (!of_device_is_available(np))
-		goto out_put_np;
-
-	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
-	if (!bus) {
-		rc = -ENOMEM;
-		goto out_put_np;
-	}
-
-	bus->name = "SJA1110 100base-TX MDIO bus";
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-base-tx",
-		 dev_name(priv->ds->dev));
-	bus->read = sja1105_base_tx_mdio_read;
-	bus->write = sja1105_base_tx_mdio_write;
-	bus->parent = priv->ds->dev;
-	mdio_priv = bus->priv;
-	mdio_priv->priv = priv;
-
-	rc = of_mdiobus_register(bus, np);
-	if (rc) {
-		mdiobus_free(bus);
-		goto out_put_np;
-	}
-
-	priv->mdio_base_tx = bus;
-
-out_put_np:
-	of_node_put(np);
-
-	return rc;
-}
-
-static void sja1105_mdiobus_base_tx_unregister(struct sja1105_private *priv)
-{
-	if (!priv->mdio_base_tx)
-		return;
-
-	mdiobus_unregister(priv->mdio_base_tx);
-	mdiobus_free(priv->mdio_base_tx);
-	priv->mdio_base_tx = NULL;
-}
-
-static int sja1105_mdiobus_base_t1_register(struct sja1105_private *priv,
-					    struct device_node *mdio_node)
-{
-	struct sja1105_mdio_private *mdio_priv;
-	struct device_node *np;
-	struct mii_bus *bus;
-	int rc = 0;
-
-	np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-t1-mdio");
-	if (!np)
-		return 0;
-
-	if (!of_device_is_available(np))
-		goto out_put_np;
-
-	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
-	if (!bus) {
-		rc = -ENOMEM;
-		goto out_put_np;
-	}
-
-	bus->name = "SJA1110 100base-T1 MDIO bus";
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-base-t1",
-		 dev_name(priv->ds->dev));
-	bus->read = sja1105_base_t1_mdio_read_c22;
-	bus->write = sja1105_base_t1_mdio_write_c22;
-	bus->read_c45 = sja1105_base_t1_mdio_read_c45;
-	bus->write_c45 = sja1105_base_t1_mdio_write_c45;
-	bus->parent = priv->ds->dev;
-	mdio_priv = bus->priv;
-	mdio_priv->priv = priv;
-
-	rc = of_mdiobus_register(bus, np);
-	if (rc) {
-		mdiobus_free(bus);
-		goto out_put_np;
-	}
-
-	priv->mdio_base_t1 = bus;
-
-out_put_np:
-	of_node_put(np);
-
-	return rc;
-}
-
-static void sja1105_mdiobus_base_t1_unregister(struct sja1105_private *priv)
-{
-	if (!priv->mdio_base_t1)
-		return;
-
-	mdiobus_unregister(priv->mdio_base_t1);
-	mdiobus_free(priv->mdio_base_t1);
-	priv->mdio_base_t1 = NULL;
-}
-
 static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 {
 	struct sja1105_mdio_private *mdio_priv;
@@ -459,49 +227,13 @@ static void sja1105_mdiobus_pcs_unregister(struct sja1105_private *priv)
 int sja1105_mdiobus_register(struct dsa_switch *ds)
 {
 	struct sja1105_private *priv = ds->priv;
-	const struct sja1105_regs *regs = priv->info->regs;
-	struct device_node *switch_node = ds->dev->of_node;
-	struct device_node *mdio_node;
-	int rc;
-
-	rc = sja1105_mdiobus_pcs_register(priv);
-	if (rc)
-		return rc;
-
-	mdio_node = of_get_available_child_by_name(switch_node, "mdios");
-	if (!mdio_node)
-		return 0;
 
-	if (regs->mdio_100base_tx != SJA1105_RSV_ADDR) {
-		rc = sja1105_mdiobus_base_tx_register(priv, mdio_node);
-		if (rc)
-			goto err_put_mdio_node;
-	}
-
-	if (regs->mdio_100base_t1 != SJA1105_RSV_ADDR) {
-		rc = sja1105_mdiobus_base_t1_register(priv, mdio_node);
-		if (rc)
-			goto err_free_base_tx_mdiobus;
-	}
-
-	of_node_put(mdio_node);
-
-	return 0;
-
-err_free_base_tx_mdiobus:
-	sja1105_mdiobus_base_tx_unregister(priv);
-err_put_mdio_node:
-	of_node_put(mdio_node);
-	sja1105_mdiobus_pcs_unregister(priv);
-
-	return rc;
+	return sja1105_mdiobus_pcs_register(priv);
 }
 
 void sja1105_mdiobus_unregister(struct dsa_switch *ds)
 {
 	struct sja1105_private *priv = ds->priv;
 
-	sja1105_mdiobus_base_t1_unregister(priv);
-	sja1105_mdiobus_base_tx_unregister(priv);
 	sja1105_mdiobus_pcs_unregister(priv);
 }
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index 856a751de53a..20757e166b08 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -495,8 +495,6 @@ static const struct sja1105_regs sja1105et_regs = {
 	.ptpclkval = 0x18, /* Spans 0x18 to 0x19 */
 	.ptpclkrate = 0x1A,
 	.ptpclkcorp = 0x1D,
-	.mdio_100base_tx = SJA1105_RSV_ADDR,
-	.mdio_100base_t1 = SJA1105_RSV_ADDR,
 };
 
 static const struct sja1105_regs sja1105pqrs_regs = {
@@ -534,8 +532,6 @@ static const struct sja1105_regs sja1105pqrs_regs = {
 	.ptpclkrate = 0x1B,
 	.ptpclkcorp = 0x1E,
 	.ptpsyncts = 0x1F,
-	.mdio_100base_tx = SJA1105_RSV_ADDR,
-	.mdio_100base_t1 = SJA1105_RSV_ADDR,
 };
 
 static const struct sja1105_regs sja1110_regs = {
@@ -618,8 +614,6 @@ static const struct sja1105_regs sja1110_regs = {
 	.ptpclkrate = SJA1110_SPI_ADDR(0x74),
 	.ptpclkcorp = SJA1110_SPI_ADDR(0x80),
 	.ptpsyncts = SJA1110_SPI_ADDR(0x84),
-	.mdio_100base_tx = 0x1c2400,
-	.mdio_100base_t1 = 0x1c1000,
 	.pcs_base = {SJA1105_RSV_ADDR, 0x1c1400, 0x1c1800, 0x1c1c00, 0x1c2000,
 		     SJA1105_RSV_ADDR, SJA1105_RSV_ADDR, SJA1105_RSV_ADDR,
 		     SJA1105_RSV_ADDR, SJA1105_RSV_ADDR, SJA1105_RSV_ADDR},
diff --git a/drivers/net/dsa/sja1105/sja1105_subdev.c b/drivers/net/dsa/sja1105/sja1105_subdev.c
new file mode 100644
index 000000000000..06957d44f084
--- /dev/null
+++ b/drivers/net/dsa/sja1105/sja1105_subdev.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2025 NXP
+ */
+#include <linux/device/devres.h>
+#include <linux/ioport.h>
+#include <linux/platform_device.h>
+
+#include "sja1105.h"
+#include "sja1105_subdev.h"
+
+static const struct resource sja1110_mdio_cbt1_res =
+	DEFINE_RES_REG_NAMED(0x704000, 0x4000, "mdio_cbt1");
+
+static const struct resource sja1110_mdio_cbtx_res =
+	DEFINE_RES_REG_NAMED(0x709000, 0x1000, "mdio_cbtx");
+
+static bool fwnode_is_hierarchical_child(struct fwnode_handle *child,
+					 struct fwnode_handle *parent)
+{
+	struct fwnode_handle *next = child;
+
+	do {
+		if (next == parent)
+			return true;
+		next = fwnode_get_parent(next);
+	} while (next);
+
+	return false;
+}
+
+static void of_subdev_del(void *data)
+{
+	struct platform_device *pdev = data;
+
+	platform_device_unregister(pdev);
+}
+
+/**
+ * devm_of_subdev_add() - Register an OF sub-device as a managed platform device
+ * @pdevinfo: Platform device information structure containing parent, fwnode,
+ *	name, resources, etc.
+ *
+ * This function registers a platform device as a sub-device of
+ * @pdevinfo.parent using the information provided in @pdevinfo. The sub-device
+ * will be automatically unregistered when @pdevinfo.parent is removed, thanks
+ * to devres management.
+ *
+ * If the fwnode specified in @pdevinfo is not available (disabled in device
+ * tree), this function returns success without creating the device.
+ *
+ * For the sub-device drivers to access their registers, a form of
+ * devm_regmap_init(parent) should have been called prior to this, which
+ * makes the parent regmap visible via dev_get_regmap(&pdev->dev.parent)
+ * in the sub-device driver. The entire address space is made available through
+ * this regmap to all sub-devices, although they are expected to segment it
+ * according to the given resources.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int devm_of_subdev_add(const struct platform_device_info *pdevinfo)
+{
+	struct device *parent = pdevinfo->parent;
+	struct platform_device *pdev;
+
+	if (!fwnode_device_is_available(pdevinfo->fwnode))
+		return 0;
+
+	/* To avoid API abuse, ensure that the sub-device fwnode is,
+	 * in fact, related to the parent.
+	 */
+	if (!fwnode_is_hierarchical_child(pdevinfo->fwnode, dev_fwnode(parent)))
+		return -EINVAL;
+
+	pdev = platform_device_register_full(pdevinfo);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	return devm_add_action_or_reset(parent, of_subdev_del, pdev);
+}
+
+static int devm_sja1105_add_mdio_subdev(struct device *parent,
+					struct device_node *np,
+					const struct resource *res,
+					size_t num_res)
+{
+	struct platform_device_info subdev;
+	char name[64];
+	u32 reg;
+	int err;
+
+	err = of_property_read_u32(np, "reg", &reg);
+	if (err)
+		return err;
+
+	snprintf(name, sizeof(name), "%s.%pOFn", dev_name(parent), np);
+	subdev = (struct platform_device_info) {
+		.parent = parent,
+		.fwnode = of_fwnode_handle(np),
+		.name = name,
+		.id = reg,
+		.res = res,
+		.num_res = num_res,
+	};
+
+	return devm_of_subdev_add(&subdev);
+}
+
+/* Legacy nodes which lack a proper resource description in the device tree,
+ * so we need to specify it manually.
+ */
+static int devm_sja1105_add_mdio_subdevs(struct dsa_switch *ds,
+					 struct device_node *mdio_node)
+{
+	struct device *parent = ds->dev;
+	struct device_node *np;
+	int err;
+
+	np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-tx-mdio");
+	if (np) {
+		err = devm_sja1105_add_mdio_subdev(parent, np,
+						   &sja1110_mdio_cbtx_res, 1);
+		of_node_put(np);
+		if (err)
+			return err;
+	}
+
+	np = of_get_compatible_child(mdio_node, "nxp,sja1110-base-t1-mdio");
+	if (np) {
+		err = devm_sja1105_add_mdio_subdev(parent, np,
+						   &sja1110_mdio_cbt1_res, 1);
+		of_node_put(np);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+int devm_sja1105_add_subdevs(struct dsa_switch *ds)
+{
+	struct device_node *switch_node = dev_of_node(ds->dev);
+	struct device_node *mdio_node;
+	int rc = 0;
+
+	mdio_node = of_get_available_child_by_name(switch_node, "mdios");
+	if (mdio_node) {
+		rc = devm_sja1105_add_mdio_subdevs(ds, mdio_node);
+		of_node_put(mdio_node);
+		if (rc)
+			return rc;
+	}
+
+	return 0;
+}
diff --git a/drivers/net/dsa/sja1105/sja1105_subdev.h b/drivers/net/dsa/sja1105/sja1105_subdev.h
new file mode 100644
index 000000000000..9b5a02401399
--- /dev/null
+++ b/drivers/net/dsa/sja1105/sja1105_subdev.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2025-2026 NXP
+ */
+#ifndef _SJA1105_SUBDEV_H
+#define _SJA1105_SUBDEV_H
+
+int devm_sja1105_add_subdevs(struct dsa_switch *ds);
+
+#endif
-- 
2.34.1

[PATCH v2 net-next 07/15] net: pcs: xpcs: introduce xpcs_create_pcs_fwnode()

[Vladimir Oltean]

Introduce a wrapper over xpcs_create_fwnode() that doesn't return the
specific dw_xpcs pointer type, but the generic phylink_pcs pointer type.
For example, the NXP SJA1105 driver might use this if it has a
pcs-handle - it is already a user of xpcs_create_pcs_mdiodev().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 drivers/net/pcs/pcs-xpcs.c   | 12 ++++++++++++
 include/linux/pcs/pcs-xpcs.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 9679f2b35a44..910fd8b23d41 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1707,6 +1707,18 @@ struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(xpcs_create_fwnode);
 
+struct phylink_pcs *xpcs_create_pcs_fwnode(struct fwnode_handle *fwnode)
+{
+	struct dw_xpcs *xpcs;
+
+	xpcs = xpcs_create_fwnode(fwnode);
+	if (IS_ERR(xpcs))
+		return ERR_CAST(xpcs);
+
+	return &xpcs->pcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_create_pcs_fwnode);
+
 void xpcs_destroy(struct dw_xpcs *xpcs)
 {
 	if (!xpcs)
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 36073f7b6bb4..9e450a356737 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -58,6 +58,7 @@ struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode);
 void xpcs_destroy(struct dw_xpcs *xpcs);
 
 struct phylink_pcs *xpcs_create_pcs_mdiodev(struct mii_bus *bus, int addr);
+struct phylink_pcs *xpcs_create_pcs_fwnode(struct fwnode_handle *fwnode);
 void xpcs_destroy_pcs(struct phylink_pcs *pcs);
 
 #endif /* __LINUX_PCS_XPCS_H */
-- 
2.34.1

[PATCH v2 net-next 08/15] net: pcs: xpcs-plat: convert to regmap

[Vladimir Oltean]

Generalize the MDIO buses for accessing memory-mapped XPCS devices
(through direct or indirect I/O) to also cover the case where the CSR is
behind an SPI bus. This is the case when accessing the embedded XPCS
from the NXP SJA1105/SJA1110 DSA switches.

Cc: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 drivers/net/pcs/pcs-xpcs-plat.c | 142 +++++++++++++++++++++-----------
 1 file changed, 95 insertions(+), 47 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs-plat.c b/drivers/net/pcs/pcs-xpcs-plat.c
index b8c48f9effbf..2bc0afe2bbb4 100644
--- a/drivers/net/pcs/pcs-xpcs-plat.c
+++ b/drivers/net/pcs/pcs-xpcs-plat.c
@@ -17,6 +17,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include <linux/regmap.h>
 #include <linux/sizes.h>
 
 #include "pcs-xpcs.h"
@@ -29,7 +30,8 @@ struct dw_xpcs_plat {
 	struct mii_bus *bus;
 	bool reg_indir;
 	int reg_width;
-	void __iomem *reg_base;
+	unsigned int base;
+	struct regmap *regmap;
 	struct clk *cclk;
 };
 
@@ -52,7 +54,9 @@ static int xpcs_mmio_read_reg_indirect(struct dw_xpcs_plat *pxpcs,
 				       int dev, int reg)
 {
 	ptrdiff_t csr, ofs;
+	unsigned int addr;
 	u16 page;
+	u32 val;
 	int ret;
 
 	csr = xpcs_mmio_addr_format(dev, reg);
@@ -63,19 +67,21 @@ static int xpcs_mmio_read_reg_indirect(struct dw_xpcs_plat *pxpcs,
 	if (ret)
 		return ret;
 
-	switch (pxpcs->reg_width) {
-	case 4:
-		writel(page, pxpcs->reg_base + (DW_VR_CSR_VIEWPORT << 2));
-		ret = readl(pxpcs->reg_base + (ofs << 2)) & 0xffff;
-		break;
-	default:
-		writew(page, pxpcs->reg_base + (DW_VR_CSR_VIEWPORT << 1));
-		ret = readw(pxpcs->reg_base + (ofs << 1));
-		break;
-	}
+	addr = pxpcs->base + (DW_VR_CSR_VIEWPORT * pxpcs->reg_width);
+	ret = regmap_write(pxpcs->regmap, addr, page);
+	if (ret)
+		goto err_put;
+
+	addr = pxpcs->base + (ofs * pxpcs->reg_width);
+	ret = regmap_read(pxpcs->regmap, addr, &val);
+	if (ret)
+		goto err_put;
 
 	pm_runtime_put(&pxpcs->pdev->dev);
+	return val & 0xffff;
 
+err_put:
+	pm_runtime_put(&pxpcs->pdev->dev);
 	return ret;
 }
 
@@ -83,6 +89,7 @@ static int xpcs_mmio_write_reg_indirect(struct dw_xpcs_plat *pxpcs,
 					int dev, int reg, u16 val)
 {
 	ptrdiff_t csr, ofs;
+	unsigned int addr;
 	u16 page;
 	int ret;
 
@@ -94,26 +101,25 @@ static int xpcs_mmio_write_reg_indirect(struct dw_xpcs_plat *pxpcs,
 	if (ret)
 		return ret;
 
-	switch (pxpcs->reg_width) {
-	case 4:
-		writel(page, pxpcs->reg_base + (DW_VR_CSR_VIEWPORT << 2));
-		writel(val, pxpcs->reg_base + (ofs << 2));
-		break;
-	default:
-		writew(page, pxpcs->reg_base + (DW_VR_CSR_VIEWPORT << 1));
-		writew(val, pxpcs->reg_base + (ofs << 1));
-		break;
-	}
+	addr = pxpcs->base + (DW_VR_CSR_VIEWPORT * pxpcs->reg_width);
+	ret = regmap_write(pxpcs->regmap, addr, page);
+	if (ret)
+		goto err_put;
 
-	pm_runtime_put(&pxpcs->pdev->dev);
+	addr = pxpcs->base + (ofs * pxpcs->reg_width);
+	ret = regmap_write(pxpcs->regmap, addr, val);
 
-	return 0;
+err_put:
+	pm_runtime_put(&pxpcs->pdev->dev);
+	return ret;
 }
 
 static int xpcs_mmio_read_reg_direct(struct dw_xpcs_plat *pxpcs,
 				     int dev, int reg)
 {
+	unsigned int addr;
 	ptrdiff_t csr;
+	u32 val;
 	int ret;
 
 	csr = xpcs_mmio_addr_format(dev, reg);
@@ -122,23 +128,23 @@ static int xpcs_mmio_read_reg_direct(struct dw_xpcs_plat *pxpcs,
 	if (ret)
 		return ret;
 
-	switch (pxpcs->reg_width) {
-	case 4:
-		ret = readl(pxpcs->reg_base + (csr << 2)) & 0xffff;
-		break;
-	default:
-		ret = readw(pxpcs->reg_base + (csr << 1));
-		break;
-	}
+	addr = pxpcs->base + (csr * pxpcs->reg_width);
+	ret = regmap_read(pxpcs->regmap, addr, &val);
+	if (ret)
+		goto err_put;
 
 	pm_runtime_put(&pxpcs->pdev->dev);
+	return val & 0xffff;
 
+err_put:
+	pm_runtime_put(&pxpcs->pdev->dev);
 	return ret;
 }
 
 static int xpcs_mmio_write_reg_direct(struct dw_xpcs_plat *pxpcs,
 				      int dev, int reg, u16 val)
 {
+	unsigned int addr;
 	ptrdiff_t csr;
 	int ret;
 
@@ -148,18 +154,11 @@ static int xpcs_mmio_write_reg_direct(struct dw_xpcs_plat *pxpcs,
 	if (ret)
 		return ret;
 
-	switch (pxpcs->reg_width) {
-	case 4:
-		writel(val, pxpcs->reg_base + (csr << 2));
-		break;
-	default:
-		writew(val, pxpcs->reg_base + (csr << 1));
-		break;
-	}
+	addr = pxpcs->base + (csr * pxpcs->reg_width);
+	ret = regmap_write(pxpcs->regmap, addr, val);
 
 	pm_runtime_put(&pxpcs->pdev->dev);
-
-	return 0;
+	return ret;
 }
 
 static int xpcs_mmio_read_c22(struct mii_bus *bus, int addr, int reg)
@@ -230,11 +229,48 @@ static struct dw_xpcs_plat *xpcs_plat_create_data(struct platform_device *pdev)
 	return pxpcs;
 }
 
+static struct regmap *xpcs_plat_create_regmap(struct dw_xpcs_plat *pxpcs,
+					      const struct resource *res)
+{
+	struct platform_device *pdev = pxpcs->pdev;
+	struct regmap_config config = {};
+	struct device *dev = &pdev->dev;
+	void __iomem *reg_base;
+
+	reg_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg_base)) {
+		dev_err(dev, "Failed to map reg-space\n");
+		return ERR_CAST(reg_base);
+	}
+
+	if (pxpcs->reg_width == 2) {
+		config.reg_bits = 16;
+		config.val_bits = 16;
+		config.reg_stride = 2;
+	} else {
+		config.reg_bits = 32;
+		config.val_bits = 32;
+		config.reg_stride = 4;
+	}
+
+	if (pxpcs->reg_indir)
+		config.max_register = 0xff * pxpcs->reg_width;
+	else
+		config.max_register = 0x1fffff * pxpcs->reg_width;
+
+	config.reg_format_endian = REGMAP_ENDIAN_NATIVE;
+	config.val_format_endian = REGMAP_ENDIAN_NATIVE;
+
+	return devm_regmap_init_mmio(dev, reg_base, &config);
+}
+
 static int xpcs_plat_init_res(struct dw_xpcs_plat *pxpcs)
 {
 	struct platform_device *pdev = pxpcs->pdev;
 	struct device *dev = &pdev->dev;
+	bool have_reg_resource = false;
 	resource_size_t spc_size;
+	struct regmap *regmap;
 	struct resource *res;
 
 	if (!device_property_read_u32(dev, "reg-io-width", &pxpcs->reg_width)) {
@@ -246,8 +282,14 @@ static int xpcs_plat_init_res(struct dw_xpcs_plat *pxpcs)
 		pxpcs->reg_width = 2;
 	}
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "direct") ?:
-	      platform_get_resource_byname(pdev, IORESOURCE_MEM, "indirect");
+	res = platform_get_resource_byname(pdev, IORESOURCE_REG, "direct") ?:
+	      platform_get_resource_byname(pdev, IORESOURCE_REG, "indirect");
+	if (res) {
+		have_reg_resource = true;
+	} else {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "direct") ?:
+		      platform_get_resource_byname(pdev, IORESOURCE_MEM, "indirect");
+	}
 	if (!res) {
 		dev_err(dev, "No reg-space found\n");
 		return -EINVAL;
@@ -266,10 +308,16 @@ static int xpcs_plat_init_res(struct dw_xpcs_plat *pxpcs)
 		return -EINVAL;
 	}
 
-	pxpcs->reg_base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(pxpcs->reg_base)) {
-		dev_err(dev, "Failed to map reg-space\n");
-		return PTR_ERR(pxpcs->reg_base);
+	if (have_reg_resource) {
+		regmap = dev_get_regmap(dev->parent, NULL);
+		pxpcs->base = res->start;
+	} else {
+		regmap = xpcs_plat_create_regmap(pxpcs, res);
+	}
+	pxpcs->regmap = regmap;
+	if (!pxpcs->regmap) {
+		dev_err(dev, "No regmap available\n");
+		return -ENODEV;
 	}
 
 	return 0;
-- 
2.34.1

[PATCH v2 net-next 09/15] dt-bindings: net: dsa: sja1105: document the PCS nodes

[Vladimir Oltean]

Some (not all) use cases are in dire need of describing the XPCS blocks
embedded in the NXP SJA1105 and SJA1110 switches in the device tree.
The use case driving this effort is specifying custom 'rx-polarity' or
'tx-polarity' property values.

These PCS blocks follow the same bindings as the other instances which
are memory-mapped using an APB3 or MCI interface.

Since the SJA1105 applies the
Documentation/devicetree/bindings/net/ethernet-switch.yaml schema
directly on the SPI device OF node, its bindings are incompatible with
describing address space regions where sub-devices like the XPCS exist.
Namely, ethernet-switch.yaml wants #address-cells and #size-cells = <0>
to satisfy the unit-address-less '^(ethernet-)?ports$' child node.
But the XPCS sub-devices want their unit address to be the start
of their "reg" region in the switch address space, and that
requires #address-cells and #size-cells = <1>.

If the SPI device OF node had an MFD-style schema, i.e. "(1)" from here:
https://lore.kernel.org/netdev/20260109121432.lu2o22iijd4i57qq@skbuf/
things would have been simpler. But that ship has sailed and we need to
continue supporting the direction in which the SJA1105 bindings have
started already.

The retrofit-ready compromise solution is for the ethernet-switch to
define a custom "regs" sub-node with #address-cells and #size-cells = <1>,
and this will hold any memory-mapped sub-devices, like the XPCS in this
case.

This solution could have been used for the "nxp,sja1110-base-t1-mdio"
and "nxp,sja1110-base-tx-mdio" sub-devices too (although that ship has
also sailed), and is further extensible for other SJA1110 sub-devices
not yet supported (GPIO controller, cascaded IRQ controller).

Document the XPCS integration-specific compatible string, positioning in
the switch's "regs" subnode, and the pcs-handle to them.

The "type: object" addition in the ethernet-port node is to suppress
a dt_binding_check warning that states "node schemas must have a type
or $ref". Rob Herring explains why this started being required just now:
https://lore.kernel.org/netdev/20251120173012.GA1563834-robh@kernel.org/

Because the regs and ethernet-pcs nodes are optional, I don't want to
pollute the example with them. However, I think I can add them to the
commit message:

compatible = "nxp,sja1105s";
...
regs {
	#address-cells = <0x01>;
	#size-cells = <0x01>;

	ethernet-pcs@0 {
		compatible = "nxp,sja1105-pcs";
		reg = <0x00 0x800000>;
		reg-names = "direct";
		reg-io-width = <0x04>;
		tx-polarity = <PHY_POL_INVERTED>;
	};
};

compatible = "nxp,sja1110a";
...
regs {
	#address-cells = <0x01>;
	#size-cells = <0x01>;

	ethernet-pcs@705000 {
		compatible = "nxp,sja1110-pcs";
		reg = <0x705000 0x1000>;
		reg-names = "indirect";
		reg-io-width = <0x04>;
		tx-polarity = <PHY_POL_NORMAL>;
	};

	ethernet-pcs@706000 {
		compatible = "nxp,sja1110-pcs";
		reg = <0x706000 0x1000>;
		reg-names = "indirect";
		reg-io-width = <0x04>;
		tx-polarity = <PHY_POL_NORMAL>;
	};

	ethernet-pcs@707000 {
		compatible = "nxp,sja1110-pcs";
		reg = <0x707000 0x1000>;
		reg-names = "indirect";
		reg-io-width = <0x04>;
		tx-polarity = <PHY_POL_NORMAL>;
	};

	ethernet-pcs@708000 {
		compatible = "nxp,sja1110-pcs";
		reg = <0x708000 0x1000>;
		reg-names = "indirect";
		reg-io-width = <0x04>;
		tx-polarity = <PHY_POL_NORMAL>;
	};
};

Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: rewrite commit message

 .../bindings/net/dsa/nxp,sja1105.yaml         | 27 +++++++++++++++++++
 .../bindings/net/pcs/snps,dw-xpcs.yaml        |  8 ++++++
 2 files changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
index 607b7fe8d28e..af86e425cd9a 100644
--- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
@@ -85,11 +85,31 @@ properties:
           - compatible
           - reg
 
+  regs:
+    type: object
+    description:
+      Optional container node for peripherals in the switch address space other
+      than the switching IP itself. This node and its children only need to be
+      described if board-specific properties need to be specified, like SerDes
+      lane polarity inversion. If absent, default descriptions are used.
+    additionalProperties: false
+
+    properties:
+      '#address-cells':
+        const: 1
+      '#size-cells':
+        const: 1
+
+    patternProperties:
+      "^ethernet-pcs@[0-9a-f]+$":
+        $ref: /schemas/net/pcs/snps,dw-xpcs.yaml#
+
 patternProperties:
   "^(ethernet-)?ports$":
     additionalProperties: true
     patternProperties:
       "^(ethernet-)?port@[0-9]$":
+        type: object
         allOf:
           - if:
               properties:
@@ -107,6 +127,13 @@ patternProperties:
                 tx-internal-delay-ps:
                   $ref: "#/$defs/internal-delay-ps"
 
+        properties:
+          pcs-handle:
+            description:
+              Phandle to a PCS device node from the "regs" container.
+              Can be skipped if the PCS description is missing - in that case,
+              the connection is implicit.
+
 required:
   - compatible
   - reg
diff --git a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
index e77eec9ac9ee..46e4f611f714 100644
--- a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
+++ b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
@@ -25,6 +25,14 @@ description:
 properties:
   compatible:
     oneOf:
+      - description:
+          Synopsys DesignWare XPCS in NXP SJA1105 switch (direct APB3 access
+          via SPI) with custom PMA
+        const: nxp,sja1105-pcs
+      - description:
+          Synopsys DesignWare XPCS in NXP SJA1110 switch (indirect APB3 access
+          via SPI) with custom PMA
+        const: nxp,sja1110-pcs
       - description: Synopsys DesignWare XPCS with none or unknown PMA
         const: snps,dw-xpcs
       - description: Synopsys DesignWare XPCS with Consumer Gen1 3G PMA
-- 
2.34.1

Re: [PATCH v2 net-next 09/15] dt-bindings: net: dsa: sja1105: document the PCS nodes

[Rob Herring (Arm)]

On Thu, 22 Jan 2026 12:56:48 +0200, Vladimir Oltean wrote:
> Some (not all) use cases are in dire need of describing the XPCS blocks
> embedded in the NXP SJA1105 and SJA1110 switches in the device tree.
> The use case driving this effort is specifying custom 'rx-polarity' or
> 'tx-polarity' property values.
> 
> These PCS blocks follow the same bindings as the other instances which
> are memory-mapped using an APB3 or MCI interface.
> 
> Since the SJA1105 applies the
> Documentation/devicetree/bindings/net/ethernet-switch.yaml schema
> directly on the SPI device OF node, its bindings are incompatible with
> describing address space regions where sub-devices like the XPCS exist.
> Namely, ethernet-switch.yaml wants #address-cells and #size-cells = <0>
> to satisfy the unit-address-less '^(ethernet-)?ports$' child node.
> But the XPCS sub-devices want their unit address to be the start
> of their "reg" region in the switch address space, and that
> requires #address-cells and #size-cells = <1>.
> 
> If the SPI device OF node had an MFD-style schema, i.e. "(1)" from here:
> https://lore.kernel.org/netdev/20260109121432.lu2o22iijd4i57qq@skbuf/
> things would have been simpler. But that ship has sailed and we need to
> continue supporting the direction in which the SJA1105 bindings have
> started already.
> 
> The retrofit-ready compromise solution is for the ethernet-switch to
> define a custom "regs" sub-node with #address-cells and #size-cells = <1>,
> and this will hold any memory-mapped sub-devices, like the XPCS in this
> case.
> 
> This solution could have been used for the "nxp,sja1110-base-t1-mdio"
> and "nxp,sja1110-base-tx-mdio" sub-devices too (although that ship has
> also sailed), and is further extensible for other SJA1110 sub-devices
> not yet supported (GPIO controller, cascaded IRQ controller).
> 
> Document the XPCS integration-specific compatible string, positioning in
> the switch's "regs" subnode, and the pcs-handle to them.
> 
> The "type: object" addition in the ethernet-port node is to suppress
> a dt_binding_check warning that states "node schemas must have a type
> or $ref". Rob Herring explains why this started being required just now:
> https://lore.kernel.org/netdev/20251120173012.GA1563834-robh@kernel.org/
> 
> Because the regs and ethernet-pcs nodes are optional, I don't want to
> pollute the example with them. However, I think I can add them to the
> commit message:
> 
> compatible = "nxp,sja1105s";
> ...
> regs {
> 	#address-cells = <0x01>;
> 	#size-cells = <0x01>;
> 
> 	ethernet-pcs@0 {
> 		compatible = "nxp,sja1105-pcs";
> 		reg = <0x00 0x800000>;
> 		reg-names = "direct";
> 		reg-io-width = <0x04>;
> 		tx-polarity = <PHY_POL_INVERTED>;
> 	};
> };
> 
> compatible = "nxp,sja1110a";
> ...
> regs {
> 	#address-cells = <0x01>;
> 	#size-cells = <0x01>;
> 
> 	ethernet-pcs@705000 {
> 		compatible = "nxp,sja1110-pcs";
> 		reg = <0x705000 0x1000>;
> 		reg-names = "indirect";
> 		reg-io-width = <0x04>;
> 		tx-polarity = <PHY_POL_NORMAL>;
> 	};
> 
> 	ethernet-pcs@706000 {
> 		compatible = "nxp,sja1110-pcs";
> 		reg = <0x706000 0x1000>;
> 		reg-names = "indirect";
> 		reg-io-width = <0x04>;
> 		tx-polarity = <PHY_POL_NORMAL>;
> 	};
> 
> 	ethernet-pcs@707000 {
> 		compatible = "nxp,sja1110-pcs";
> 		reg = <0x707000 0x1000>;
> 		reg-names = "indirect";
> 		reg-io-width = <0x04>;
> 		tx-polarity = <PHY_POL_NORMAL>;
> 	};
> 
> 	ethernet-pcs@708000 {
> 		compatible = "nxp,sja1110-pcs";
> 		reg = <0x708000 0x1000>;
> 		reg-names = "indirect";
> 		reg-io-width = <0x04>;
> 		tx-polarity = <PHY_POL_NORMAL>;
> 	};
> };
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: rewrite commit message
> 
>  .../bindings/net/dsa/nxp,sja1105.yaml         | 27 +++++++++++++++++++
>  .../bindings/net/pcs/snps,dw-xpcs.yaml        |  8 ++++++
>  2 files changed, 35 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH v2 net-next 10/15] net: pcs: xpcs-plat: add NXP SJA1105/SJA1110 support

[Vladimir Oltean]

The XPCS in these NXP switches returns 0 when reading the ID registers,
and is integrated with a custom PMA.

The current way to support it is with hijacked PHYID register reads
in sja1105_pcs_mdio_read_c45(), to fake that it returns
NXP_SJA1105_XPCS_ID.

The new way to support it is with a specific compatible string. This
makes the platform XPCS driver use a specific struct dw_xpcs_info which
it assigns to mdiodev->dev.platform_data, and from there, xpcs_init_id()
picks it up and uses it. Later, xpcs_identify() doesn't overwrite the
xpcs->info.pcs and xpcs->info.pma unless they are set to
DW_XPCS_ID_NATIVE and DW_XPCS_PMA_ID_NATIVE, aka zeroes.

Since what is custom is the PMA and not the PCS, a later patch will
probably have to move the NXP constants around. But that should be done
only after this becomes strictly XPCS internal business.

Cc: Serge Semin <fancer.lancer@gmail.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: none

 drivers/net/pcs/pcs-xpcs-plat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs-plat.c b/drivers/net/pcs/pcs-xpcs-plat.c
index 2bc0afe2bbb4..a256ae40c757 100644
--- a/drivers/net/pcs/pcs-xpcs-plat.c
+++ b/drivers/net/pcs/pcs-xpcs-plat.c
@@ -476,6 +476,8 @@ DW_XPCS_INFO_DECLARE(xpcs_pma_gen4_3g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN4_3G_ID
 DW_XPCS_INFO_DECLARE(xpcs_pma_gen4_6g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN4_6G_ID);
 DW_XPCS_INFO_DECLARE(xpcs_pma_gen5_10g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN5_10G_ID);
 DW_XPCS_INFO_DECLARE(xpcs_pma_gen5_12g, DW_XPCS_ID_NATIVE, DW_XPCS_PMA_GEN5_12G_ID);
+DW_XPCS_INFO_DECLARE(xpcs_sja1105, NXP_SJA1105_XPCS_ID, DW_XPCS_PMA_ID_NATIVE);
+DW_XPCS_INFO_DECLARE(xpcs_sja1110, NXP_SJA1110_XPCS_ID, DW_XPCS_PMA_ID_NATIVE);
 
 static const struct of_device_id xpcs_of_ids[] = {
 	{ .compatible = "snps,dw-xpcs", .data = &xpcs_generic },
@@ -486,6 +488,8 @@ static const struct of_device_id xpcs_of_ids[] = {
 	{ .compatible = "snps,dw-xpcs-gen4-6g", .data = &xpcs_pma_gen4_6g },
 	{ .compatible = "snps,dw-xpcs-gen5-10g", .data = &xpcs_pma_gen5_10g },
 	{ .compatible = "snps,dw-xpcs-gen5-12g", .data = &xpcs_pma_gen5_12g },
+	{ .compatible = "nxp,sja1105-pcs", .data = &xpcs_sja1105 },
+	{ .compatible = "nxp,sja1110-pcs", .data = &xpcs_sja1110 },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, xpcs_of_ids);
-- 
2.34.1

[PATCH v2 net-next 11/15] net: dsa: sja1105: fill device tree with ethernet-pcs sub-devices under "regs" node

[Vladimir Oltean]

The code in sja1105_mdio.c does the same thing as the one added by Serge
Semin in drivers/net/pcs/pcs-xpcs-plat.c (implements a virtual MDIO bus,
backed by either a direct or an indirect register access method), except
the latter is generic after the conversion to regmap.

The SJA1105 binding now has a way of specifying sub-devices in the
switch's address space, using the 'regs' container node. However,
specifying the XPCS in the device tree is optional, yet it is a critical
component for the SGMII protocol (which is supported as of today). So we
must continue to instantiate the pcs-xpcs-plat.c driver somehow.

I've tried various ways of using that driver while avoiding major DT
bindings changes for this switch, like fwnode_create_software_node() and
custom platform data. Platform data was ugly and software nodes didn't
work at all, for reasons explained here:
https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/

I have to give huge credits to Andy Shevchenko, who after more than one
year remembered the discussion and referenced Hervé Codina's work on PCI
DT overlays, as well as a presentation from Lizhi Hou and Rob Herring.

I think I found the compromise solution that allows me to make progress,
which is to create a dynamic OF changeset that attaches the PCS node to
the live device tree, if it wasn't described already in the DTS, or use
the one from the DTS if it's already there. With a proper OF node, the
xpcs-plat driver probes just fine.

There also exists a use case where the XPCS is manually described
in the device tree, and that is when the board author needs to describe
SGMII lane polarity inversion via 'rx-polarity' or 'tx-polarity'. In
that case, sja1105_fill_device_tree() detects which PCS nodes are
present and fills in default descriptions only for the rest.

Nobody probes these ethernet-pcs devices just yet, because the custom
bus code is missing. SGMII continues to be supported through the
sja1105_mdiobus_pcs_register() and sja1105_mdiobus_pcs_unregister() code
path.

Cc: Serge Semin <fancer.lancer@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Herve Codina <herve.codina@bootlin.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- use devres for sja1105_fill_device_tree()
- fill correct default tx-polarity in ethernet-pcs node rather than
  expect XPCS driver to have SJA1105-specific correct default value
- drop cell_name from struct sja1105_pcs_resource after no longer making
  use of MFD
- rewrite of_child_node_exists() using of_node_full_name()
- print phys_addr_t using unsigned long long and %llx

 drivers/net/dsa/sja1105/Kconfig          |   1 +
 drivers/net/dsa/sja1105/sja1105.h        |  11 ++
 drivers/net/dsa/sja1105/sja1105_main.c   |   7 +
 drivers/net/dsa/sja1105/sja1105_spi.c    |  37 ++++-
 drivers/net/dsa/sja1105/sja1105_subdev.c | 190 +++++++++++++++++++++++
 drivers/net/dsa/sja1105/sja1105_subdev.h |   1 +
 6 files changed, 246 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/Kconfig b/drivers/net/dsa/sja1105/Kconfig
index 1291bba3f3b6..55ce8f6a2758 100644
--- a/drivers/net/dsa/sja1105/Kconfig
+++ b/drivers/net/dsa/sja1105/Kconfig
@@ -7,6 +7,7 @@ tristate "NXP SJA1105 Ethernet switch family support"
 	select PCS_XPCS
 	select PACKING
 	select CRC32
+	select OF_DYNAMIC
 	help
 	  This is the driver for the NXP SJA1105 (5-port) and SJA1110 (10-port)
 	  automotive Ethernet switch family. These are managed over an SPI
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index cf718e7c2b7b..1b52beba62d4 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -113,6 +113,13 @@ enum sja1105_internal_phy_t {
 	SJA1105_PHY_BASE_T1,
 };
 
+struct sja1105_pcs_resource {
+	struct resource res;
+	int port;
+	u32 tx_polarity;
+	const char *compatible;
+};
+
 struct sja1105_info {
 	u64 device_id;
 	/* Needed for distinction between P and R, and between Q and S
@@ -165,6 +172,8 @@ struct sja1105_info {
 	bool supports_2500basex[SJA1105_MAX_NUM_PORTS];
 	enum sja1105_internal_phy_t internal_phy[SJA1105_MAX_NUM_PORTS];
 	const u64 port_speed[SJA1105_SPEED_MAX];
+	const struct sja1105_pcs_resource *pcs_resources;
+	size_t num_pcs_resources;
 };
 
 enum sja1105_key_type {
@@ -278,6 +287,8 @@ struct sja1105_private {
 	struct sja1105_cbs_entry *cbs;
 	struct mii_bus *mdio_pcs;
 	struct phylink_pcs *pcs[SJA1105_MAX_NUM_PORTS];
+	struct fwnode_handle *pcs_fwnode[SJA1105_MAX_NUM_PORTS];
+	struct of_changeset of_cs;
 	struct sja1105_ptp_data ptp_data;
 	struct sja1105_tas_data tas_data;
 };
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index d3fb42772071..84c0f7c676e2 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3330,6 +3330,13 @@ static int sja1105_probe(struct spi_device *spi)
 		return rc;
 	}
 
+	rc = devm_sja1105_fill_device_tree(ds);
+	if (rc) {
+		dev_err(ds->dev, "Failed to fill device tree: %pe\n",
+			ERR_PTR(rc));
+		return rc;
+	}
+
 	rc = devm_sja1105_add_subdevs(ds);
 	if (rc) {
 		dev_err(ds->dev, "Failed to create child devices: %pe\n",
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index 20757e166b08..4d4da69b3c30 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: BSD-3-Clause
-/* Copyright 2016-2018 NXP
+/* Copyright 2016-2018, 2026 NXP
  * Copyright (c) 2018, Sensor-Technik Wiedemann GmbH
  * Copyright (c) 2018-2019, Vladimir Oltean <olteanv@gmail.com>
  */
+#include <linux/phy/phy-common-props.h>
 #include <linux/spi/spi.h>
 #include <linux/packing.h>
 #include "sja1105.h"
@@ -619,6 +620,28 @@ static const struct sja1105_regs sja1110_regs = {
 		     SJA1105_RSV_ADDR, SJA1105_RSV_ADDR, SJA1105_RSV_ADDR},
 };
 
+/* See port compatibility matrix in Documentation/networking/dsa/sja1105.rst */
+static const struct sja1105_pcs_resource sja1105rs_pcs_resources[] = {
+	{ DEFINE_RES_REG_NAMED(0x0, 0x800000, "direct"), 4,
+	  PHY_POL_INVERT, "nxp,sja1105-pcs"
+	},
+};
+
+static const struct sja1105_pcs_resource sja1110_pcs_resources[] = {
+	{ DEFINE_RES_REG_NAMED(0x705000, 0x1000, "indirect"), 1,
+	  PHY_POL_NORMAL, "nxp,sja1110-pcs"
+	},
+	{ DEFINE_RES_REG_NAMED(0x706000, 0x1000, "indirect"), 2,
+	  PHY_POL_NORMAL, "nxp,sja1110-pcs"
+	},
+	{ DEFINE_RES_REG_NAMED(0x707000, 0x1000, "indirect"), 3,
+	  PHY_POL_NORMAL, "nxp,sja1110-pcs"
+	},
+	{ DEFINE_RES_REG_NAMED(0x708000, 0x1000, "indirect"), 4,
+	  PHY_POL_NORMAL, "nxp,sja1110-pcs"
+	},
+};
+
 const struct sja1105_info sja1105e_info = {
 	.device_id		= SJA1105E_DEVICE_ID,
 	.part_no		= SJA1105ET_PART_NO,
@@ -782,6 +805,8 @@ const struct sja1105_info sja1105r_info = {
 	.supports_rmii		= {true, true, true, true, true},
 	.supports_rgmii		= {true, true, true, true, true},
 	.supports_sgmii		= {false, false, false, false, true},
+	.pcs_resources		= sja1105rs_pcs_resources,
+	.num_pcs_resources	= ARRAY_SIZE(sja1105rs_pcs_resources),
 	.name			= "SJA1105R",
 };
 
@@ -818,6 +843,8 @@ const struct sja1105_info sja1105s_info = {
 	.supports_rmii		= {true, true, true, true, true},
 	.supports_rgmii		= {true, true, true, true, true},
 	.supports_sgmii		= {false, false, false, false, true},
+	.pcs_resources		= sja1105rs_pcs_resources,
+	.num_pcs_resources	= ARRAY_SIZE(sja1105rs_pcs_resources),
 	.name			= "SJA1105S",
 };
 
@@ -869,6 +896,8 @@ const struct sja1105_info sja1110a_info = {
 				   SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
 				   SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
 				   SJA1105_PHY_BASE_T1},
+	.pcs_resources		= sja1110_pcs_resources,
+	.num_pcs_resources	= ARRAY_SIZE(sja1110_pcs_resources),
 	.name			= "SJA1110A",
 };
 
@@ -920,6 +949,8 @@ const struct sja1105_info sja1110b_info = {
 				   SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
 				   SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
 				   SJA1105_NO_PHY},
+	.pcs_resources		= &sja1110_pcs_resources[2], /* ports 3 and 4 */
+	.num_pcs_resources	= ARRAY_SIZE(sja1110_pcs_resources) - 2,
 	.name			= "SJA1110B",
 };
 
@@ -971,6 +1002,8 @@ const struct sja1105_info sja1110c_info = {
 				   SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
 				   SJA1105_NO_PHY, SJA1105_NO_PHY,
 				   SJA1105_NO_PHY},
+	.pcs_resources		= &sja1110_pcs_resources[3], /* port 4 */
+	.num_pcs_resources	= ARRAY_SIZE(sja1110_pcs_resources) - 3,
 	.name			= "SJA1110C",
 };
 
@@ -1022,5 +1055,7 @@ const struct sja1105_info sja1110d_info = {
 				   SJA1105_PHY_BASE_T1, SJA1105_PHY_BASE_T1,
 				   SJA1105_NO_PHY, SJA1105_NO_PHY,
 				   SJA1105_NO_PHY},
+	.pcs_resources		= sja1110_pcs_resources,
+	.num_pcs_resources	= ARRAY_SIZE(sja1110_pcs_resources),
 	.name			= "SJA1110D",
 };
diff --git a/drivers/net/dsa/sja1105/sja1105_subdev.c b/drivers/net/dsa/sja1105/sja1105_subdev.c
index 06957d44f084..085d77947dc3 100644
--- a/drivers/net/dsa/sja1105/sja1105_subdev.c
+++ b/drivers/net/dsa/sja1105/sja1105_subdev.c
@@ -152,3 +152,193 @@ int devm_sja1105_add_subdevs(struct dsa_switch *ds)
 
 	return 0;
 }
+
+static bool of_child_node_exists(struct device_node *np, const char *name)
+{
+	for_each_child_of_node_scoped(np, child)
+		if (!strcmp(of_node_full_name(child), name))
+			return true;
+
+	return false;
+}
+
+static int sja1105_create_pcs_nodes(struct sja1105_private *priv,
+				    struct device_node *regs_node)
+{
+	struct dsa_switch *ds = priv->ds;
+	struct device *dev = ds->dev;
+	struct device_node *pcs_node;
+	char node_name[32];
+	u32 reg_props[2];
+	int rc;
+
+	for (int i = 0; i < priv->info->num_pcs_resources; i++) {
+		const struct sja1105_pcs_resource *pcs_res;
+
+		pcs_res = &priv->info->pcs_resources[i];
+
+		/* phys_addr_t has variable size depending on the value of
+		 * CONFIG_PHYS_ADDR_T_64BIT, cast to the larger unsigned long
+		 * long type for printf.
+		 */
+		snprintf(node_name, sizeof(node_name), "ethernet-pcs@%llx",
+			 (unsigned long long)pcs_res->res.start);
+
+		if (of_child_node_exists(regs_node, node_name))
+			continue;
+
+		pcs_node = of_changeset_create_node(&priv->of_cs, regs_node,
+						    node_name);
+		if (!pcs_node) {
+			dev_err(dev, "Failed to create PCS node %s\n", node_name);
+			return -ENOMEM;
+		}
+
+		rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
+						  "compatible",
+						  pcs_res->compatible);
+		if (rc) {
+			dev_err(dev, "Failed to add compatible property to %s: %pe\n",
+				node_name, ERR_PTR(rc));
+			return rc;
+		}
+
+		reg_props[0] = pcs_res->res.start;
+		reg_props[1] = resource_size(&pcs_res->res);
+		rc = of_changeset_add_prop_u32_array(&priv->of_cs, pcs_node,
+						     "reg", reg_props, 2);
+		if (rc) {
+			dev_err(dev, "Failed to add reg property to %s: %pe\n",
+				node_name, ERR_PTR(rc));
+			return rc;
+		}
+
+		rc = of_changeset_add_prop_string(&priv->of_cs, pcs_node,
+						  "reg-names",
+						  pcs_res->res.name);
+		if (rc) {
+			dev_err(dev, "Failed to add reg-names property to %s: %pe\n",
+				node_name, ERR_PTR(rc));
+			return rc;
+		}
+
+		rc = of_changeset_add_prop_u32(&priv->of_cs, pcs_node,
+					       "reg-io-width", 4);
+		if (rc) {
+			dev_err(dev, "Failed to add reg-io-width property to %s: %pe\n",
+				node_name, ERR_PTR(rc));
+			return rc;
+		}
+
+		/* The SJA1105 XPCS is integrated with a TX-inverting custom
+		 * PMA. We need to invert the polarity in the PCS to obtain a
+		 * non-inverted signal at the pins.
+		 */
+		rc = of_changeset_add_prop_u32(&priv->of_cs, pcs_node, "tx-polarity",
+					       pcs_res->tx_polarity);
+		if (rc) {
+			dev_err(dev, "Failed to add tx-polarity property to %s: %pe\n",
+				node_name, ERR_PTR(rc));
+			return rc;
+		}
+
+		dev_dbg(dev, "Created OF node %pOF\n", pcs_node);
+		priv->pcs_fwnode[pcs_res->port] = of_fwnode_handle(pcs_node);
+	}
+
+	return 0;
+}
+
+static struct device_node *sja1105_create_regs_node(struct sja1105_private *priv,
+						    struct device_node *switch_node)
+{
+	struct device *dev = priv->ds->dev;
+	struct device_node *regs_node;
+	int rc;
+
+	regs_node = of_changeset_create_node(&priv->of_cs, switch_node, "regs");
+	if (!regs_node) {
+		dev_err(dev, "Failed to create 'regs' device tree node\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	rc = of_changeset_add_prop_u32(&priv->of_cs, regs_node, "#address-cells", 1);
+	if (rc) {
+		dev_err(dev, "Failed to add #address-cells property: %pe\n",
+			ERR_PTR(rc));
+		return ERR_PTR(rc);
+	}
+
+	rc = of_changeset_add_prop_u32(&priv->of_cs, regs_node, "#size-cells", 1);
+	if (rc) {
+		dev_err(dev, "Failed to add #size-cells property: %pe\n",
+			ERR_PTR(rc));
+		return ERR_PTR(rc);
+	}
+
+	return regs_node;
+}
+
+static void sja1105_restore_device_tree(void *data)
+{
+	struct sja1105_private *priv = data;
+	struct device *dev = priv->ds->dev;
+	int rc;
+
+	rc = of_changeset_revert(&priv->of_cs);
+	if (rc) {
+		dev_err(dev, "Failed to revert device tree changeset: %pe\n",
+			ERR_PTR(rc));
+	}
+
+	of_changeset_destroy(&priv->of_cs);
+}
+
+int devm_sja1105_fill_device_tree(struct dsa_switch *ds)
+{
+	struct device_node *switch_node, *regs_node;
+	struct sja1105_private *priv = ds->priv;
+	bool regs_node_created = false;
+	struct device *dev = ds->dev;
+	int rc;
+
+	if (!priv->info->num_pcs_resources)
+		return 0;
+
+	switch_node = dev_of_node(dev);
+	of_changeset_init(&priv->of_cs);
+
+	regs_node = of_get_child_by_name(switch_node, "regs");
+	if (!regs_node) {
+		regs_node = sja1105_create_regs_node(priv, switch_node);
+		if (IS_ERR(regs_node)) {
+			rc = PTR_ERR(regs_node);
+			goto out_destroy_changeset;
+		}
+
+		regs_node_created = true;
+		dev_dbg(dev, "Created OF node %pOF\n", regs_node);
+	}
+
+	rc = sja1105_create_pcs_nodes(priv, regs_node);
+	if (rc)
+		goto out_destroy_changeset;
+
+	rc = of_changeset_apply(&priv->of_cs);
+	if (rc) {
+		dev_err(dev, "Failed to apply device tree changeset: %pe\n",
+			ERR_PTR(rc));
+		goto out_destroy_changeset;
+	}
+
+	rc = devm_add_action_or_reset(dev, sja1105_restore_device_tree, priv);
+	goto out_put_regs_node;
+
+out_destroy_changeset:
+	of_changeset_destroy(&priv->of_cs);
+out_put_regs_node:
+	if (!regs_node_created)
+		of_node_put(regs_node);
+
+	return rc;
+}
diff --git a/drivers/net/dsa/sja1105/sja1105_subdev.h b/drivers/net/dsa/sja1105/sja1105_subdev.h
index 9b5a02401399..1507ff3c44d1 100644
--- a/drivers/net/dsa/sja1105/sja1105_subdev.h
+++ b/drivers/net/dsa/sja1105/sja1105_subdev.h
@@ -5,5 +5,6 @@
 #define _SJA1105_SUBDEV_H
 
 int devm_sja1105_add_subdevs(struct dsa_switch *ds);
+int devm_sja1105_fill_device_tree(struct dsa_switch *ds);
 
 #endif
-- 
2.34.1

Re: [PATCH v2 net-next 11/15] net: dsa: sja1105: fill device tree with ethernet-pcs sub-devices under "regs" node

[kernel test robot]

Hi Vladimir,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Vladimir-Oltean/net-mdio-regmap-permit-working-with-non-MMIO-regmaps/20260122-192934
base:   net-next/main
patch link:    https://lore.kernel.org/r/20260122105654.105600-12-vladimir.oltean%40nxp.com
patch subject: [PATCH v2 net-next 11/15] net: dsa: sja1105: fill device tree with ethernet-pcs sub-devices under "regs" node
config: mips-randconfig-r112-20260123 (https://download.01.org/0day-ci/archive/20260124/202601240325.L4Ejl6qH-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260124/202601240325.L4Ejl6qH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601240325.L4Ejl6qH-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from arch/mips/kernel/asm-offsets.c:26:
   In file included from include/linux/kvm_host.h:5:
   In file included from include/linux/entry-virt.h:10:
   In file included from include/linux/tick.h:8:
   In file included from include/linux/clockchips.h:14:
   In file included from include/linux/clocksource.h:19:
>> include/linux/of.h:1645:34: error: use of undeclared identifier 'OF_RECONFIG_ATTACH_NODE'; did you mean 'OF_RECONFIG_NO_CHANGE'?
    1645 |         return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL);
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~
         |                                         OF_RECONFIG_NO_CHANGE
   include/linux/of.h:1620:2: note: 'OF_RECONFIG_NO_CHANGE' declared here
    1620 |         OF_RECONFIG_NO_CHANGE = 0,
         |         ^
>> include/linux/of.h:1651:34: error: use of undeclared identifier 'OF_RECONFIG_DETACH_NODE'; did you mean 'OF_RECONFIG_NO_CHANGE'?
    1651 |         return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL);
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~
         |                                         OF_RECONFIG_NO_CHANGE
   include/linux/of.h:1620:2: note: 'OF_RECONFIG_NO_CHANGE' declared here
    1620 |         OF_RECONFIG_NO_CHANGE = 0,
         |         ^
>> include/linux/of.h:1657:34: error: use of undeclared identifier 'OF_RECONFIG_ADD_PROPERTY'
    1657 |         return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop);
         |                                         ^
>> include/linux/of.h:1663:34: error: use of undeclared identifier 'OF_RECONFIG_REMOVE_PROPERTY'
    1663 |         return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop);
         |                                         ^
>> include/linux/of.h:1669:34: error: use of undeclared identifier 'OF_RECONFIG_UPDATE_PROPERTY'
    1669 |         return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
         |                                         ^
   5 errors generated.
   make[3]: *** [scripts/Makefile.build:182: arch/mips/kernel/asm-offsets.s] Error 1
   make[3]: Target 'prepare' not remade because of errors.
   make[2]: *** [Makefile:1314: prepare0] Error 2
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:248: __sub-make] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:248: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for OF_DYNAMIC
   Depends on [n]: OF [=n]
   Selected by [m]:
   - NET_DSA_SJA1105 [=m] && NETDEVICES [=y] && NET_DSA [=m] && SPI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]


vim +1645 include/linux/of.h

201c910bd6898d Pantelis Antoniou 2014-07-04  1618  
b53a2340d0d304 Pantelis Antoniou 2014-10-28  1619  enum of_reconfig_change {
b53a2340d0d304 Pantelis Antoniou 2014-10-28 @1620  	OF_RECONFIG_NO_CHANGE = 0,
b53a2340d0d304 Pantelis Antoniou 2014-10-28  1621  	OF_RECONFIG_CHANGE_ADD,
b53a2340d0d304 Pantelis Antoniou 2014-10-28  1622  	OF_RECONFIG_CHANGE_REMOVE,
b53a2340d0d304 Pantelis Antoniou 2014-10-28  1623  };
b53a2340d0d304 Pantelis Antoniou 2014-10-28  1624  
2e8fff668dc14e Rob Herring       2023-03-29  1625  struct notifier_block;
2e8fff668dc14e Rob Herring       2023-03-29  1626  
201c910bd6898d Pantelis Antoniou 2014-07-04  1627  #ifdef CONFIG_OF_DYNAMIC
f6892d193fb9d6 Grant Likely      2014-11-21  1628  extern int of_reconfig_notifier_register(struct notifier_block *);
f6892d193fb9d6 Grant Likely      2014-11-21  1629  extern int of_reconfig_notifier_unregister(struct notifier_block *);
f5242e5a883bf1 Grant Likely      2014-11-24  1630  extern int of_reconfig_notify(unsigned long, struct of_reconfig_data *rd);
f5242e5a883bf1 Grant Likely      2014-11-24  1631  extern int of_reconfig_get_state_change(unsigned long action,
f5242e5a883bf1 Grant Likely      2014-11-24  1632  					struct of_reconfig_data *arg);
f6892d193fb9d6 Grant Likely      2014-11-21  1633  
201c910bd6898d Pantelis Antoniou 2014-07-04  1634  extern void of_changeset_init(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04  1635  extern void of_changeset_destroy(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04  1636  extern int of_changeset_apply(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04  1637  extern int of_changeset_revert(struct of_changeset *ocs);
201c910bd6898d Pantelis Antoniou 2014-07-04  1638  extern int of_changeset_action(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04  1639  		unsigned long action, struct device_node *np,
201c910bd6898d Pantelis Antoniou 2014-07-04  1640  		struct property *prop);
201c910bd6898d Pantelis Antoniou 2014-07-04  1641  
201c910bd6898d Pantelis Antoniou 2014-07-04  1642  static inline int of_changeset_attach_node(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04  1643  		struct device_node *np)
201c910bd6898d Pantelis Antoniou 2014-07-04  1644  {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1645  	return of_changeset_action(ocs, OF_RECONFIG_ATTACH_NODE, np, NULL);
201c910bd6898d Pantelis Antoniou 2014-07-04  1646  }
201c910bd6898d Pantelis Antoniou 2014-07-04  1647  
201c910bd6898d Pantelis Antoniou 2014-07-04  1648  static inline int of_changeset_detach_node(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04  1649  		struct device_node *np)
201c910bd6898d Pantelis Antoniou 2014-07-04  1650  {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1651  	return of_changeset_action(ocs, OF_RECONFIG_DETACH_NODE, np, NULL);
201c910bd6898d Pantelis Antoniou 2014-07-04  1652  }
201c910bd6898d Pantelis Antoniou 2014-07-04  1653  
201c910bd6898d Pantelis Antoniou 2014-07-04  1654  static inline int of_changeset_add_property(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04  1655  		struct device_node *np, struct property *prop)
201c910bd6898d Pantelis Antoniou 2014-07-04  1656  {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1657  	return of_changeset_action(ocs, OF_RECONFIG_ADD_PROPERTY, np, prop);
201c910bd6898d Pantelis Antoniou 2014-07-04  1658  }
201c910bd6898d Pantelis Antoniou 2014-07-04  1659  
201c910bd6898d Pantelis Antoniou 2014-07-04  1660  static inline int of_changeset_remove_property(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04  1661  		struct device_node *np, struct property *prop)
201c910bd6898d Pantelis Antoniou 2014-07-04  1662  {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1663  	return of_changeset_action(ocs, OF_RECONFIG_REMOVE_PROPERTY, np, prop);
201c910bd6898d Pantelis Antoniou 2014-07-04  1664  }
201c910bd6898d Pantelis Antoniou 2014-07-04  1665  
201c910bd6898d Pantelis Antoniou 2014-07-04  1666  static inline int of_changeset_update_property(struct of_changeset *ocs,
201c910bd6898d Pantelis Antoniou 2014-07-04  1667  		struct device_node *np, struct property *prop)
201c910bd6898d Pantelis Antoniou 2014-07-04  1668  {
201c910bd6898d Pantelis Antoniou 2014-07-04 @1669  	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
201c910bd6898d Pantelis Antoniou 2014-07-04  1670  }
b544fc2b8606d7 Lizhi Hou         2023-08-15  1671  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2 net-next 12/15] net: dsa: sja1105: replace mdiobus-pcs with xpcs-plat driver

[Vladimir Oltean]

The following switches supported by the driver have at least one XPCS
sub-device:
- "nxp,sja1105r"
- "nxp,sja1105s"
- "nxp,sja1110a"
- "nxp,sja1110b"
- "nxp,sja1110c"
- "nxp,sja1110d"

For these switches, it is guaranteed that the XPCS blocks is described
in the "regs" OF subnode of the switch, either manually by the board DT
author or by sja1105_fill_device_tree().

So we can write some custom "bus" code to probe platform devices for
each child OF node of "regs", and that completely replaces the need for
the code in sja1105_mdio.c.

There were discussions about how to instantiate the XPCS sub-devices and
the MFD maintainer doesn't consider mfd_add_devices() to be used
canonically for this use case:
https://lore.kernel.org/netdev/20260116132345.GA882947@google.com/

So I am rolling my own custom code on top of devm_of_subdev_add() ->
platform_device_register_full() that is also used for the MDIO
sub-devices, but this time, instead of manually picking the MDIO nodes
and registering them one by one, the PCS nodes (as well as anything
else under "regs") will be automatically be picked up by the new
devm_of_subdevs_populate() method. The critical difference is that the
XPCS device tree binding is sufficiently detailed to be able to extract
the address space resources from its "reg" properties, whereas the MDIO
nodes were not (and had to be manually associated with resources).

This of_subdev_* "bus" code lives in the sja1105 driver for lack of a
better home, but can easily be moved to a more generic location, and is
written to permit that.

A small implementation note: priv->pcs_fwnode[] exists because
currently, the of_changeset API doesn't support creating phandles.
Thus, instead of also filling in 'pcs-handle' properties from ports to
the dynamic 'ethernet-pcs' OF nodes, the driver just saves them for
later use in sja1105_create_pcs().

This implies that when the PCS nodes do exist in DT, priv->pcs_fwnode[]
will be NULL. This is fine - xpcs_create_fwnode() is NULL-tolerant via
fwnode_device_is_available(), and this case currently returns -ENODEV
and will be handled by the next change.

Cc: Serge Semin <fancer.lancer@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Herve Codina <herve.codina@bootlin.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- mfd_add_devices() has been replaced with a completely new approach in
  the form of devm_of_subdevs_populate()

 drivers/net/dsa/sja1105/Makefile         |   1 -
 drivers/net/dsa/sja1105/sja1105.h        |  20 --
 drivers/net/dsa/sja1105/sja1105_main.c   |  53 +++--
 drivers/net/dsa/sja1105/sja1105_mdio.c   | 239 -----------------------
 drivers/net/dsa/sja1105/sja1105_spi.c    |  15 --
 drivers/net/dsa/sja1105/sja1105_subdev.c | 171 +++++++++++++++-
 6 files changed, 212 insertions(+), 287 deletions(-)
 delete mode 100644 drivers/net/dsa/sja1105/sja1105_mdio.c

diff --git a/drivers/net/dsa/sja1105/Makefile b/drivers/net/dsa/sja1105/Makefile
index 7b5537d67072..049d198f681e 100644
--- a/drivers/net/dsa/sja1105/Makefile
+++ b/drivers/net/dsa/sja1105/Makefile
@@ -4,7 +4,6 @@ obj-$(CONFIG_NET_DSA_SJA1105) += sja1105.o
 sja1105-objs := \
     sja1105_spi.o \
     sja1105_main.o \
-    sja1105_mdio.o \
     sja1105_subdev.o \
     sja1105_flower.o \
     sja1105_ethtool.o \
diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 1b52beba62d4..7fce1507eb12 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -91,11 +91,6 @@ struct sja1105_regs {
 	u64 rmii_ref_clk[SJA1105_MAX_NUM_PORTS];
 	u64 rmii_ext_tx_clk[SJA1105_MAX_NUM_PORTS];
 	u64 stats[__MAX_SJA1105_STATS_AREA][SJA1105_MAX_NUM_PORTS];
-	u64 pcs_base[SJA1105_MAX_NUM_PORTS];
-};
-
-struct sja1105_mdio_private {
-	struct sja1105_private *priv;
 };
 
 enum {
@@ -159,10 +154,6 @@ struct sja1105_info {
 	bool (*rxtstamp)(struct dsa_switch *ds, int port, struct sk_buff *skb);
 	void (*txtstamp)(struct dsa_switch *ds, int port, struct sk_buff *skb);
 	int (*clocking_setup)(struct sja1105_private *priv);
-	int (*pcs_mdio_read_c45)(struct mii_bus *bus, int phy, int mmd,
-				 int reg);
-	int (*pcs_mdio_write_c45)(struct mii_bus *bus, int phy, int mmd,
-				  int reg, u16 val);
 	int (*disable_microcontroller)(struct sja1105_private *priv);
 	const char *name;
 	bool supports_mii[SJA1105_MAX_NUM_PORTS];
@@ -285,7 +276,6 @@ struct sja1105_private {
 	struct mutex dynamic_config_lock;
 	struct devlink_region **regions;
 	struct sja1105_cbs_entry *cbs;
-	struct mii_bus *mdio_pcs;
 	struct phylink_pcs *pcs[SJA1105_MAX_NUM_PORTS];
 	struct fwnode_handle *pcs_fwnode[SJA1105_MAX_NUM_PORTS];
 	struct of_changeset of_cs;
@@ -316,16 +306,6 @@ int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled,
 			   struct netlink_ext_ack *extack);
 void sja1105_frame_memory_partitioning(struct sja1105_private *priv);
 
-/* From sja1105_mdio.c */
-int sja1105_mdiobus_register(struct dsa_switch *ds);
-void sja1105_mdiobus_unregister(struct dsa_switch *ds);
-int sja1105_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg);
-int sja1105_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg,
-			       u16 val);
-int sja1110_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg);
-int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg,
-			       u16 val);
-
 /* From sja1105_devlink.c */
 int sja1105_devlink_setup(struct dsa_switch *ds);
 void sja1105_devlink_teardown(struct dsa_switch *ds);
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 84c0f7c676e2..b60a890ba416 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -15,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/of_net.h>
 #include <linux/of_mdio.h>
+#include <linux/pcs/pcs-xpcs.h>
 #include <linux/netdev_features.h>
 #include <linux/netdevice.h>
 #include <linux/if_bridge.h>
@@ -3005,6 +3006,44 @@ static int sja1105_port_bridge_flags(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int sja1105_create_pcs(struct dsa_switch *ds, int port)
+{
+	struct sja1105_private *priv = ds->priv;
+	struct phylink_pcs *pcs;
+
+	if (priv->phy_mode[port] != PHY_INTERFACE_MODE_SGMII &&
+	    priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
+		return 0;
+
+	pcs = xpcs_create_pcs_fwnode(priv->pcs_fwnode[port]);
+	if (IS_ERR(pcs))
+		return PTR_ERR(pcs);
+
+	priv->pcs[port] = pcs;
+
+	return 0;
+}
+
+static void sja1105_destroy_pcs(struct dsa_switch *ds, int port)
+{
+	struct sja1105_private *priv = ds->priv;
+
+	if (priv->pcs[port]) {
+		xpcs_destroy_pcs(priv->pcs[port]);
+		priv->pcs[port] = NULL;
+	}
+}
+
+static int sja1105_port_setup(struct dsa_switch *ds, int port)
+{
+	return sja1105_create_pcs(ds, port);
+}
+
+static void sja1105_port_teardown(struct dsa_switch *ds, int port)
+{
+	sja1105_destroy_pcs(ds, port);
+}
+
 /* The programming model for the SJA1105 switch is "all-at-once" via static
  * configuration tables. Some of these can be dynamically modified at runtime,
  * but not the xMII mode parameters table.
@@ -3059,16 +3098,9 @@ static int sja1105_setup(struct dsa_switch *ds)
 		goto out_flower_teardown;
 	}
 
-	rc = sja1105_mdiobus_register(ds);
-	if (rc < 0) {
-		dev_err(ds->dev, "Failed to register MDIO bus: %pe\n",
-			ERR_PTR(rc));
-		goto out_ptp_clock_unregister;
-	}
-
 	rc = sja1105_devlink_setup(ds);
 	if (rc < 0)
-		goto out_mdiobus_unregister;
+		goto out_ptp_clock_unregister;
 
 	rtnl_lock();
 	rc = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
@@ -3098,8 +3130,6 @@ static int sja1105_setup(struct dsa_switch *ds)
 
 out_devlink_teardown:
 	sja1105_devlink_teardown(ds);
-out_mdiobus_unregister:
-	sja1105_mdiobus_unregister(ds);
 out_ptp_clock_unregister:
 	sja1105_ptp_clock_unregister(ds);
 out_flower_teardown:
@@ -3120,7 +3150,6 @@ static void sja1105_teardown(struct dsa_switch *ds)
 	rtnl_unlock();
 
 	sja1105_devlink_teardown(ds);
-	sja1105_mdiobus_unregister(ds);
 	sja1105_ptp_clock_unregister(ds);
 	sja1105_flower_teardown(ds);
 	sja1105_tas_teardown(ds);
@@ -3139,6 +3168,8 @@ static const struct dsa_switch_ops sja1105_switch_ops = {
 	.connect_tag_protocol	= sja1105_connect_tag_protocol,
 	.setup			= sja1105_setup,
 	.teardown		= sja1105_teardown,
+	.port_setup		= sja1105_port_setup,
+	.port_teardown		= sja1105_port_teardown,
 	.set_ageing_time	= sja1105_set_ageing_time,
 	.port_change_mtu	= sja1105_change_mtu,
 	.port_max_mtu		= sja1105_get_max_mtu,
diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
deleted file mode 100644
index b803ce71f5cc..000000000000
--- a/drivers/net/dsa/sja1105/sja1105_mdio.c
+++ /dev/null
@@ -1,239 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Copyright 2021 NXP
- */
-#include <linux/pcs/pcs-xpcs.h>
-#include <linux/of_mdio.h>
-#include "sja1105.h"
-
-#define SJA1110_PCS_BANK_REG		SJA1110_SPI_ADDR(0x3fc)
-
-int sja1105_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	u64 addr;
-	u32 tmp;
-	int rc;
-
-	addr = (mmd << 16) | reg;
-
-	if (mmd != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
-		return 0xffff;
-
-	if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID1)
-		return NXP_SJA1105_XPCS_ID >> 16;
-	if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID2)
-		return NXP_SJA1105_XPCS_ID & GENMASK(15, 0);
-
-	rc = sja1105_xfer_u32(priv, SPI_READ, addr, &tmp, NULL);
-	if (rc < 0)
-		return rc;
-
-	return tmp & 0xffff;
-}
-
-int sja1105_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd,
-			       int reg, u16 val)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	u64 addr;
-	u32 tmp;
-
-	addr = (mmd << 16) | reg;
-	tmp = val;
-
-	if (mmd != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
-		return -EINVAL;
-
-	return sja1105_xfer_u32(priv, SPI_WRITE, addr, &tmp, NULL);
-}
-
-int sja1110_pcs_mdio_read_c45(struct mii_bus *bus, int phy, int mmd, int reg)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	const struct sja1105_regs *regs = priv->info->regs;
-	int offset, bank;
-	u64 addr;
-	u32 tmp;
-	int rc;
-
-	if (regs->pcs_base[phy] == SJA1105_RSV_ADDR)
-		return -ENODEV;
-
-	addr = (mmd << 16) | reg;
-
-	if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID1)
-		return NXP_SJA1110_XPCS_ID >> 16;
-	if (mmd == MDIO_MMD_VEND2 && (reg & GENMASK(15, 0)) == MII_PHYSID2)
-		return NXP_SJA1110_XPCS_ID & GENMASK(15, 0);
-
-	bank = addr >> 8;
-	offset = addr & GENMASK(7, 0);
-
-	/* This addressing scheme reserves register 0xff for the bank address
-	 * register, so that can never be addressed.
-	 */
-	if (WARN_ON(offset == 0xff))
-		return -ENODEV;
-
-	tmp = bank;
-
-	rc = sja1105_xfer_u32(priv, SPI_WRITE,
-			      regs->pcs_base[phy] + SJA1110_PCS_BANK_REG,
-			      &tmp, NULL);
-	if (rc < 0)
-		return rc;
-
-	rc = sja1105_xfer_u32(priv, SPI_READ, regs->pcs_base[phy] + offset,
-			      &tmp, NULL);
-	if (rc < 0)
-		return rc;
-
-	return tmp & 0xffff;
-}
-
-int sja1110_pcs_mdio_write_c45(struct mii_bus *bus, int phy, int mmd, int reg,
-			       u16 val)
-{
-	struct sja1105_mdio_private *mdio_priv = bus->priv;
-	struct sja1105_private *priv = mdio_priv->priv;
-	const struct sja1105_regs *regs = priv->info->regs;
-	int offset, bank;
-	u64 addr;
-	u32 tmp;
-	int rc;
-
-	if (regs->pcs_base[phy] == SJA1105_RSV_ADDR)
-		return -ENODEV;
-
-	addr = (mmd << 16) | reg;
-
-	bank = addr >> 8;
-	offset = addr & GENMASK(7, 0);
-
-	/* This addressing scheme reserves register 0xff for the bank address
-	 * register, so that can never be addressed.
-	 */
-	if (WARN_ON(offset == 0xff))
-		return -ENODEV;
-
-	tmp = bank;
-
-	rc = sja1105_xfer_u32(priv, SPI_WRITE,
-			      regs->pcs_base[phy] + SJA1110_PCS_BANK_REG,
-			      &tmp, NULL);
-	if (rc < 0)
-		return rc;
-
-	tmp = val;
-
-	return sja1105_xfer_u32(priv, SPI_WRITE, regs->pcs_base[phy] + offset,
-				&tmp, NULL);
-}
-
-static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
-{
-	struct sja1105_mdio_private *mdio_priv;
-	struct dsa_switch *ds = priv->ds;
-	struct mii_bus *bus;
-	int rc = 0;
-	int port;
-
-	if (!priv->info->pcs_mdio_read_c45 || !priv->info->pcs_mdio_write_c45)
-		return 0;
-
-	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
-	if (!bus)
-		return -ENOMEM;
-
-	bus->name = "SJA1105 PCS MDIO bus";
-	snprintf(bus->id, MII_BUS_ID_SIZE, "%s-pcs",
-		 dev_name(ds->dev));
-	bus->read_c45 = priv->info->pcs_mdio_read_c45;
-	bus->write_c45 = priv->info->pcs_mdio_write_c45;
-	bus->parent = ds->dev;
-	/* There is no PHY on this MDIO bus => mask out all PHY addresses
-	 * from auto probing.
-	 */
-	bus->phy_mask = ~0;
-	mdio_priv = bus->priv;
-	mdio_priv->priv = priv;
-
-	rc = mdiobus_register(bus);
-	if (rc) {
-		mdiobus_free(bus);
-		return rc;
-	}
-
-	for (port = 0; port < ds->num_ports; port++) {
-		struct phylink_pcs *pcs;
-
-		if (dsa_is_unused_port(ds, port))
-			continue;
-
-		if (priv->phy_mode[port] != PHY_INTERFACE_MODE_SGMII &&
-		    priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
-			continue;
-
-		pcs = xpcs_create_pcs_mdiodev(bus, port);
-		if (IS_ERR(pcs)) {
-			rc = PTR_ERR(pcs);
-			goto out_pcs_free;
-		}
-
-		priv->pcs[port] = pcs;
-	}
-
-	priv->mdio_pcs = bus;
-
-	return 0;
-
-out_pcs_free:
-	for (port = 0; port < ds->num_ports; port++) {
-		if (priv->pcs[port]) {
-			xpcs_destroy_pcs(priv->pcs[port]);
-			priv->pcs[port] = NULL;
-		}
-	}
-
-	mdiobus_unregister(bus);
-	mdiobus_free(bus);
-
-	return rc;
-}
-
-static void sja1105_mdiobus_pcs_unregister(struct sja1105_private *priv)
-{
-	struct dsa_switch *ds = priv->ds;
-	int port;
-
-	if (!priv->mdio_pcs)
-		return;
-
-	for (port = 0; port < ds->num_ports; port++) {
-		if (priv->pcs[port]) {
-			xpcs_destroy_pcs(priv->pcs[port]);
-			priv->pcs[port] = NULL;
-		}
-	}
-
-	mdiobus_unregister(priv->mdio_pcs);
-	mdiobus_free(priv->mdio_pcs);
-	priv->mdio_pcs = NULL;
-}
-
-int sja1105_mdiobus_register(struct dsa_switch *ds)
-{
-	struct sja1105_private *priv = ds->priv;
-
-	return sja1105_mdiobus_pcs_register(priv);
-}
-
-void sja1105_mdiobus_unregister(struct dsa_switch *ds)
-{
-	struct sja1105_private *priv = ds->priv;
-
-	sja1105_mdiobus_pcs_unregister(priv);
-}
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index 4d4da69b3c30..27cac00eba32 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -615,9 +615,6 @@ static const struct sja1105_regs sja1110_regs = {
 	.ptpclkrate = SJA1110_SPI_ADDR(0x74),
 	.ptpclkcorp = SJA1110_SPI_ADDR(0x80),
 	.ptpsyncts = SJA1110_SPI_ADDR(0x84),
-	.pcs_base = {SJA1105_RSV_ADDR, 0x1c1400, 0x1c1800, 0x1c1c00, 0x1c2000,
-		     SJA1105_RSV_ADDR, SJA1105_RSV_ADDR, SJA1105_RSV_ADDR,
-		     SJA1105_RSV_ADDR, SJA1105_RSV_ADDR, SJA1105_RSV_ADDR},
 };
 
 /* See port compatibility matrix in Documentation/networking/dsa/sja1105.rst */
@@ -791,8 +788,6 @@ const struct sja1105_info sja1105r_info = {
 	.ptp_cmd_packing	= sja1105pqrs_ptp_cmd_packing,
 	.rxtstamp		= sja1105_rxtstamp,
 	.clocking_setup		= sja1105_clocking_setup,
-	.pcs_mdio_read_c45	= sja1105_pcs_mdio_read_c45,
-	.pcs_mdio_write_c45	= sja1105_pcs_mdio_write_c45,
 	.regs			= &sja1105pqrs_regs,
 	.port_speed		= {
 		[SJA1105_SPEED_AUTO] = 0,
@@ -830,8 +825,6 @@ const struct sja1105_info sja1105s_info = {
 	.ptp_cmd_packing	= sja1105pqrs_ptp_cmd_packing,
 	.rxtstamp		= sja1105_rxtstamp,
 	.clocking_setup		= sja1105_clocking_setup,
-	.pcs_mdio_read_c45	= sja1105_pcs_mdio_read_c45,
-	.pcs_mdio_write_c45	= sja1105_pcs_mdio_write_c45,
 	.port_speed		= {
 		[SJA1105_SPEED_AUTO] = 0,
 		[SJA1105_SPEED_10MBPS] = 3,
@@ -871,8 +864,6 @@ const struct sja1105_info sja1110a_info = {
 	.rxtstamp		= sja1110_rxtstamp,
 	.txtstamp		= sja1110_txtstamp,
 	.disable_microcontroller = sja1110_disable_microcontroller,
-	.pcs_mdio_read_c45	= sja1110_pcs_mdio_read_c45,
-	.pcs_mdio_write_c45	= sja1110_pcs_mdio_write_c45,
 	.port_speed		= {
 		[SJA1105_SPEED_AUTO] = 0,
 		[SJA1105_SPEED_10MBPS] = 4,
@@ -924,8 +915,6 @@ const struct sja1105_info sja1110b_info = {
 	.rxtstamp		= sja1110_rxtstamp,
 	.txtstamp		= sja1110_txtstamp,
 	.disable_microcontroller = sja1110_disable_microcontroller,
-	.pcs_mdio_read_c45	= sja1110_pcs_mdio_read_c45,
-	.pcs_mdio_write_c45	= sja1110_pcs_mdio_write_c45,
 	.port_speed		= {
 		[SJA1105_SPEED_AUTO] = 0,
 		[SJA1105_SPEED_10MBPS] = 4,
@@ -977,8 +966,6 @@ const struct sja1105_info sja1110c_info = {
 	.rxtstamp		= sja1110_rxtstamp,
 	.txtstamp		= sja1110_txtstamp,
 	.disable_microcontroller = sja1110_disable_microcontroller,
-	.pcs_mdio_read_c45	= sja1110_pcs_mdio_read_c45,
-	.pcs_mdio_write_c45	= sja1110_pcs_mdio_write_c45,
 	.port_speed		= {
 		[SJA1105_SPEED_AUTO] = 0,
 		[SJA1105_SPEED_10MBPS] = 4,
@@ -1030,8 +1017,6 @@ const struct sja1105_info sja1110d_info = {
 	.rxtstamp		= sja1110_rxtstamp,
 	.txtstamp		= sja1110_txtstamp,
 	.disable_microcontroller = sja1110_disable_microcontroller,
-	.pcs_mdio_read_c45	= sja1110_pcs_mdio_read_c45,
-	.pcs_mdio_write_c45	= sja1110_pcs_mdio_write_c45,
 	.port_speed		= {
 		[SJA1105_SPEED_AUTO] = 0,
 		[SJA1105_SPEED_10MBPS] = 4,
diff --git a/drivers/net/dsa/sja1105/sja1105_subdev.c b/drivers/net/dsa/sja1105/sja1105_subdev.c
index 085d77947dc3..5ca613a4549d 100644
--- a/drivers/net/dsa/sja1105/sja1105_subdev.c
+++ b/drivers/net/dsa/sja1105/sja1105_subdev.c
@@ -78,6 +78,167 @@ static int devm_of_subdev_add(const struct platform_device_info *pdevinfo)
 	return devm_add_action_or_reset(parent, of_subdev_del, pdev);
 }
 
+static int of_subdev_collect_resources(struct device *parent, struct device_node *np,
+				       size_t address_cells, size_t size_cells,
+				       struct resource **res, size_t *num_res)
+{
+	size_t reg_len;
+	u32 *reg;
+	int err;
+
+	err = of_property_count_u32_elems(np, "reg");
+	if (!err)
+		err = -EINVAL;
+	if (err < 0) {
+		dev_err(parent, "Failed to read subdev %pOF \"reg\" property: %pe\n",
+			np, ERR_PTR(err));
+		return err;
+	}
+	reg_len = err;
+
+	if (reg_len % (address_cells + size_cells)) {
+		dev_err(parent, "Invalid \"reg\" specifier for %pOF\n", np);
+		return -EINVAL;
+	}
+
+	*num_res = reg_len / (address_cells + size_cells);
+	*res = kcalloc(*num_res, sizeof(**res), GFP_KERNEL);
+	if (!*res)
+		return -ENOMEM;
+
+	reg = kcalloc(reg_len, sizeof(*reg), GFP_KERNEL);
+	if (!reg) {
+		kfree(*res);
+		return -ENOMEM;
+	}
+
+	err = of_property_read_u32_array(np, "reg", reg, reg_len);
+	if (err) {
+		kfree(reg);
+		kfree(*res);
+		return err;
+	}
+
+	for (int cur_res = 0; cur_res < *num_res; cur_res++) {
+		int idx, address_cell, size_cell;
+		phys_addr_t start = 0, size = 0;
+
+		for (address_cell = 0; address_cell < address_cells; address_cell++) {
+			idx = cur_res * (address_cells + size_cells) + address_cell;
+			start = (unsigned long long)start << 32 | reg[idx];
+		}
+		for (size_cell = 0; size_cell < size_cells; size_cell++) {
+			idx = cur_res * (address_cells + size_cells) + address_cells + size_cell;
+			size = (unsigned long long)size << 32 | reg[idx];
+		}
+
+		(*res)[cur_res].start = start;
+		(*res)[cur_res].end = start + size - 1;
+		(*res)[cur_res].flags = IORESOURCE_REG;
+		of_property_read_string_index(np, "reg-names", cur_res, &(*res)[cur_res].name);
+	}
+
+	return 0;
+}
+
+/* Custom version of of_device_make_bus_id() which derives the name from the
+ * parent device plus the subdev name and untranslatable address.
+ * We don't set the resource address in the platform ID because that would
+ * print it as decimal rather than hex.
+ */
+static void of_subdev_make_bus_id(char *name, size_t name_len,
+				  const struct device *parent,
+				  struct device_node *child,
+				  const struct resource *res)
+{
+	if (res)
+		snprintf(name, name_len, "%s.%llx.%pOFn", dev_name(parent),
+			 (unsigned long long)res->start, child);
+	else
+		snprintf(name, name_len, "%s.%pOFn", dev_name(parent), child);
+}
+
+/**
+ * devm_of_subdevs_populate() - Populate platform sub-devices from device tree
+ * @parent: Parent device for all created sub-devices
+ * @np: Device tree node containing child nodes to be converted to sub-devices
+ *
+ * The device tree node @np describes the (MMIO-like but untranslatable) linear
+ * address space of device @parent, as can sometimes be found when such device
+ * is accessed through a SPI-to-AHB bridge.
+ *
+ * This function parses the device tree node @np and creates platform devices
+ * for each available child node. It reads the #address-cells and #size-cells
+ * properties to properly parse the "reg" properties of child nodes.
+ *
+ * For each child node, the function:
+ * - Creates a platform device with a name based on the parent and child node
+ * - Auto-detects and attaches resources to sub-devices based on parsed device
+ *   tree "reg" and "reg-names" properties
+ * - Uses the first resource's start address as the platform device ID
+ * - Registers the device with automatic cleanup via devres
+ *
+ * This is similar to of_platform_populate() except it expects to find
+ * IORESOURCE_REG resources rather than IORESOURCE_MEM/IORESOURCE_IO.
+ * It is also similar to mfd_add_devices() except we don't have to specify the
+ * mfd_cells[], but rather, the resources are embedded into the device tree.
+ * More importantly, this allows for the parent to have a hybrid function
+ * (MFD parent + the main function of the device) and a custom device tree
+ * binding, whereas MFD does not.
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int devm_of_subdevs_populate(struct device *parent, struct device_node *np)
+{
+	u32 address_cells, size_cells;
+	int err;
+
+	err = of_property_read_u32(np, "#address-cells", &address_cells);
+	if (err)
+		return err;
+
+	err = of_property_read_u32(np, "#size-cells", &size_cells);
+	if (err)
+		return err;
+
+	if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) ?
+	    (address_cells > 2 || size_cells > 2) :
+	    (address_cells > 1 || size_cells > 1)) {
+		dev_err(parent, "Subdev address space exceeds phys_addr_t possibilities\n");
+		return -EINVAL;
+	}
+
+	for_each_available_child_of_node_scoped(np, child) {
+		struct platform_device_info subdev;
+		struct resource *res;
+		size_t num_res;
+		char name[64];
+
+		err = of_subdev_collect_resources(parent, child, address_cells,
+						  size_cells, &res, &num_res);
+		if (err)
+			return err;
+
+		of_subdev_make_bus_id(name, sizeof(name), parent, child,
+				      num_res ? &res[0] : NULL);
+		subdev = (struct platform_device_info) {
+			.parent = parent,
+			.fwnode = of_fwnode_handle(child),
+			.name = name,
+			.id = PLATFORM_DEVID_NONE,
+			.res = res,
+			.num_res = num_res,
+		};
+
+		err = devm_of_subdev_add(&subdev);
+		kfree(res);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int devm_sja1105_add_mdio_subdev(struct device *parent,
 					struct device_node *np,
 					const struct resource *res,
@@ -139,9 +300,17 @@ static int devm_sja1105_add_mdio_subdevs(struct dsa_switch *ds,
 int devm_sja1105_add_subdevs(struct dsa_switch *ds)
 {
 	struct device_node *switch_node = dev_of_node(ds->dev);
-	struct device_node *mdio_node;
+	struct device_node *regs_node, *mdio_node;
 	int rc = 0;
 
+	regs_node = of_get_available_child_by_name(switch_node, "regs");
+	if (regs_node) {
+		rc = devm_of_subdevs_populate(ds->dev, regs_node);
+		of_node_put(regs_node);
+		if (rc)
+			return rc;
+	}
+
 	mdio_node = of_get_available_child_by_name(switch_node, "mdios");
 	if (mdio_node) {
 		rc = devm_sja1105_add_mdio_subdevs(ds, mdio_node);
-- 
2.34.1

[PATCH v2 net-next 13/15] net: dsa: sja1105: permit finding the XPCS via pcs-handle

[Vladimir Oltean]

This completes support for describing the XPCS in the device tree,
rather than just the case where sja1105_fill_device_tree() populates it.
Having it in the device tree is necessary when configuring lane polarity.

Note: I would have added a pcs-handle during sja1105_fill_device_tree()
too (for more unified handling), but this doesn't seem to be supported
with the of_changeset API.

Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: rewrite commit message

 drivers/net/dsa/sja1105/sja1105_main.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index b60a890ba416..3c2030e8fce5 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3008,14 +3008,26 @@ static int sja1105_port_bridge_flags(struct dsa_switch *ds, int port,
 
 static int sja1105_create_pcs(struct dsa_switch *ds, int port)
 {
+	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct sja1105_private *priv = ds->priv;
+	struct fwnode_handle *pcs_fwnode;
 	struct phylink_pcs *pcs;
 
 	if (priv->phy_mode[port] != PHY_INTERFACE_MODE_SGMII &&
 	    priv->phy_mode[port] != PHY_INTERFACE_MODE_2500BASEX)
 		return 0;
 
-	pcs = xpcs_create_pcs_fwnode(priv->pcs_fwnode[port]);
+	pcs_fwnode = fwnode_handle_get(priv->pcs_fwnode[port]);
+	/* priv->pcs_fwnode[port] is only set if the PCS is absent
+	 * from the device tree source. If present, there needs to
+	 * be a pcs-handle to it.
+	 */
+	if (!pcs_fwnode)
+		pcs_fwnode = fwnode_find_reference(of_fwnode_handle(dp->dn),
+						   "pcs-handle", 0);
+
+	pcs = xpcs_create_pcs_fwnode(pcs_fwnode);
+	fwnode_handle_put(pcs_fwnode);
 	if (IS_ERR(pcs))
 		return PTR_ERR(pcs);
 
-- 
2.34.1

[PATCH v2 net-next 14/15] dt-bindings: net: xpcs: allow properties from phy-common-props.yaml

[Vladimir Oltean]

Now XPCS device tree nodes can specify properties to configure transmit
amplitude, receiver polarity inversion, and transmitter polarity
inversion for different PHY protocols.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: add Rob's review tag

 Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
index 46e4f611f714..e44253deeeb4 100644
--- a/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
+++ b/Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml
@@ -22,6 +22,9 @@ description:
   by means of the APB3/MCI interfaces. In the later case the XPCS can be mapped
   right to the system IO memory space.
 
+allOf:
+  - $ref: /schemas/phy/phy-common-props.yaml#
+
 properties:
   compatible:
     oneOf:
@@ -110,7 +113,7 @@ required:
   - compatible
   - reg
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
-- 
2.34.1

[PATCH v2 net-next 15/15] net: pcs: xpcs: allow generic polarity inversion

[Vladimir Oltean]

Using the linux/phy/phy-common-props.h helpers, get the 'rx-polarity'
and 'tx-polarity' device tree properties, and apply them to hardware in
the newly introduced xpcs_pma_config(), called from phylink_pcs_ops ::
pcs_config().

This is the right place to do it, as the generic PHY helpers require
knowing the phy_interface_t for which we want the polarity known, and
that comes from phylink.

By using the "manual" helpers, we default to PHY_POL_NORMAL, and support
normal and inverted polarities in the RX and TX directions.

Note that for NXP SJA1105, to get a functional data path with
non-inverted signals at the device pins, we have to apply TX polarity
inversion in the PCS, due to its integration with a custom PMA.

Since the SJA1105 was only recently made to describe the XPCS in the
device tree, we can require that this hardware quirk is described there,
and this avoids having custom handling for it in the driver.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2:
- use phy_get_manual_rx_polarity() and phy_get_manual_tx_polarity(),
  which are simpler helpers that assume the default polarity is
  PHY_POL_NORMAL. There is a slight regression risk if some XPCS
  integrations rely on custom polarity set in the bootloader.
- drop custom handling for SJA1105.

 drivers/net/pcs/Kconfig        |  1 +
 drivers/net/pcs/pcs-xpcs-nxp.c | 11 --------
 drivers/net/pcs/pcs-xpcs.c     | 46 ++++++++++++++++++++++++++++------
 drivers/net/pcs/pcs-xpcs.h     |  1 -
 4 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index e417fd66f660..c95e24c895df 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -8,6 +8,7 @@ menu "PCS device drivers"
 config PCS_XPCS
 	tristate "Synopsys DesignWare Ethernet XPCS"
 	select PHYLINK
+	select PHY_COMMON_PROPS
 	help
 	  This module provides a driver and helper functions for Synopsys
 	  DesignWare XPCS controllers.
diff --git a/drivers/net/pcs/pcs-xpcs-nxp.c b/drivers/net/pcs/pcs-xpcs-nxp.c
index e8efe94cf4ec..37708b28a7aa 100644
--- a/drivers/net/pcs/pcs-xpcs-nxp.c
+++ b/drivers/net/pcs/pcs-xpcs-nxp.c
@@ -64,17 +64,6 @@
 /* RX_CDR_CTLE register */
 #define SJA1110_RX_CDR_CTLE		0x8042
 
-/* In NXP SJA1105, the PCS is integrated with a PMA that has the TX lane
- * polarity inverted by default (PLUS is MINUS, MINUS is PLUS). To obtain
- * normal non-inverted behavior, the TX lane polarity must be inverted in the
- * PCS, via the DIGITAL_CONTROL_2 register.
- */
-int nxp_sja1105_sgmii_pma_config(struct dw_xpcs *xpcs)
-{
-	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL2,
-			  DW_VR_MII_DIG_CTRL2_TX_POL_INV);
-}
-
 static int nxp_sja1110_pma_config(struct dw_xpcs *xpcs,
 				  u16 txpll_fbdiv, u16 txpll_refdiv,
 				  u16 rxpll_fbdiv, u16 rxpll_refdiv,
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 910fd8b23d41..eeddd0b1f3da 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -11,6 +11,7 @@
 #include <linux/pcs/pcs-xpcs.h>
 #include <linux/mdio.h>
 #include <linux/phy.h>
+#include <linux/phy/phy-common-props.h>
 #include <linux/phylink.h>
 #include <linux/property.h>
 
@@ -908,6 +909,42 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
 			   BMCR_SPEED1000);
 }
 
+static int xpcs_pma_config(struct dw_xpcs *xpcs, const struct dw_xpcs_compat *compat)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(&xpcs->mdiodev->dev);
+	u32 val = 0, mask;
+	unsigned int pol;
+	int ret;
+
+	mask = DW_VR_MII_DIG_CTRL2_TX_POL_INV | DW_VR_MII_DIG_CTRL2_RX_POL_INV;
+
+	ret = phy_get_manual_rx_polarity(fwnode, phy_modes(compat->interface),
+					 &pol);
+	if (ret)
+		return ret;
+	if (pol == PHY_POL_INVERT)
+		val |= DW_VR_MII_DIG_CTRL2_RX_POL_INV;
+
+	ret = phy_get_manual_tx_polarity(fwnode, phy_modes(compat->interface),
+					 &pol);
+	if (ret)
+		return ret;
+	if (pol == PHY_POL_INVERT)
+		val |= DW_VR_MII_DIG_CTRL2_TX_POL_INV;
+
+	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL2, mask, val);
+	if (ret < 0)
+		return ret;
+
+	if (compat->pma_config) {
+		ret = compat->pma_config(xpcs);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 			  const unsigned long *advertising,
 			  unsigned int neg_mode)
@@ -959,13 +996,7 @@ static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 		return -EINVAL;
 	}
 
-	if (compat->pma_config) {
-		ret = compat->pma_config(xpcs);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return xpcs_pma_config(xpcs, compat);
 }
 
 static int xpcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
@@ -1456,7 +1487,6 @@ static const struct dw_xpcs_compat nxp_sja1105_xpcs_compat[] = {
 		.interface = PHY_INTERFACE_MODE_SGMII,
 		.supported = xpcs_sgmii_features,
 		.an_mode = DW_AN_C37_SGMII,
-		.pma_config = nxp_sja1105_sgmii_pma_config,
 	}, {
 	}
 };
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 929fa238445e..56fd170a8736 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -121,7 +121,6 @@ int xpcs_write(struct dw_xpcs *xpcs, int dev, u32 reg, u16 val);
 int xpcs_modify(struct dw_xpcs *xpcs, int dev, u32 reg, u16 mask, u16 set);
 int xpcs_read_vpcs(struct dw_xpcs *xpcs, int reg);
 int xpcs_write_vpcs(struct dw_xpcs *xpcs, int reg, u16 val);
-int nxp_sja1105_sgmii_pma_config(struct dw_xpcs *xpcs);
 int nxp_sja1110_sgmii_pma_config(struct dw_xpcs *xpcs);
 int nxp_sja1110_2500basex_pma_config(struct dw_xpcs *xpcs);
 int txgbe_xpcs_switch_mode(struct dw_xpcs *xpcs, phy_interface_t interface);
-- 
2.34.1