Message-ID: <20251216002955.bgjy52s4stn2eo4r@skbuf>
Date: Tue, 16 Dec 2025 02:29:55 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Lee Jones <lee@...nel.org>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 07/15] mfd: core: add ability for cells to probe
 on a custom parent OF node

Hello Lee,

On Mon, Dec 15, 2025 at 03:50:28PM +0000, Lee Jones wrote:
> Thanks for drafting all of this.  It's not an ideal level of verboseness
> for a busy maintainer with 50+ of reviews to do, but I appreciate your
> depth of knowledge and the eloquence of the writing.

Thanks for coming back and I hope your time at LPC was spent usefully.
I will try to reorder your messages to group the replies in the way that
is most efficient.

> Side note: The implementation is also janky.

Yes, this is why it's up for review, so I can learn why it's janky and
fix it.

> There does appear to be at least some level of misunderstanding between
> us.  I'm not for one moment suggesting that a switch can't be an MFD. If
> it contains probe-able components that need to be split-up across
> multiple different subsystems, then by all means, move the core driver
> into drivers/mfd/ and register child devices 'till your heart's content.

Are you still speaking generically here, or have you actually looked at
any "nxp,sja1105q" or "nxp,sja1110a" device trees to see what it would
mean for these compatible strings to be probed by a driver in drivers/mfd?
What OF node would remain for the DSA switch (child) device driver? The same?
Or are you suggesting that the entire drivers/net/dsa/sja1105/ would
move to drivers/mfd/? Or?

> What I am saying, however, is that from what I can see in front of me,
> there doesn't appear to be any evidence that this device belongs there.
> 
> Unless there's something I'm missing, it looks awfully like you're
> simply trying to register a couple of platform deices devices and you've
> chosen to use the MFD API as a convenient way to do so.  That is not
> what MFD is for.

I may be just uneducated here, but I'm genuinely perplexed. Isn't the
MFD API a convenient way to instantiate resources of a device into
platform sub-devices for _everyone_ who uses it? Is it more than that?
I don't know how to defend this.

> > Fact of the matter is, we will always clash with the MFD maintainer in
> > this process, and it simply doesn't scale for us to keep repeating the
> > same stuff over and over. It is just too much friction. We went through
> > this once, with Colin Foster who added the Microchip VSC7512 as MFD
> > through your tree, and that marked the first time when a DSA driver over
> > a SPI device concerned itself with just the switching IP, using MFD as
> > the abstraction layer.
> 
> I don't recall those discussions from 3 years ago, but the Ocelot
> platform, whatever it may be, seems to have quite a lot more
> cross-subsystem device support requirements going on than I see here:
> 
> drivers/i2c/busses/i2c-designware-platdrv.c
> drivers/irqchip/irq-mscc-ocelot.c
> drivers/mfd/ocelot-*
> drivers/net/dsa/ocelot/*
> drivers/net/ethernet/mscc/ocelot*
> drivers/net/mdio/mdio-mscc-miim.c
> drivers/phy/mscc/phy-ocelot-serdes.c
> drivers/pinctrl/pinctrl-microchip-sgpio.c
> drivers/pinctrl/pinctrl-ocelot.c
> drivers/power/reset/ocelot-reset.c
> drivers/spi/spi-dw-mmio.c
> net/dsa/tag_ocelot_8021q.c

This is a natural effect of Ocelot being "whatever it may be". It is a
family of networking SoCs, of which VSC7514 has a MIPS CPU and Linux
port, where the above drivers are used. The VSC7512 is then a simplified
variant with the MIPS CPU removed, and the internal components controlled
externally over SPI. Hence MFD to reuse the same drivers as Linux on
MIPS (using MMIO) did. This is all that matters, not the quantity.

> > - We've had the exact same discussions with Colin Foster's VSC7512
> >   work, which you ended up accepting
> 
> Quick update: After doing a little searching for Colin's original
> patch-set, I've managed to find as far back as v5 (v16 was merged),
> which I believe was the first version that proposed using MFD.  There
> were lots of review comments and an insistence to add more than one
> device (rather than adding them subsequently) to make it a true MFD,
> however, I don't see any suggestion that MFD wasn't the right place for
> it.
> 
> > Then you start to want to develop these further. You want to avoid
> > polling PHYs for link status every second.. well, you find there's an
> > interrupt controller in that chip too, that you should be using with
> > irqchip. You want to read the chip's temperature to prevent it from
> > overheating - you find temperature sensors too, for which you register
> > with hwmon. You find reset blocks, clock generation blocks, power
> > management blocks, GPIO controllers, what have you.
> 
> Absolutely!  MFD would be perfect for that.
> 
> My point is, you don't seem to have have any of that here.

What do you want to see exactly which is not here?

I have converted three classes of sub-devices on the NXP SJA1110 to MFD
children in this patch set. Two MDIO buses and an Ethernet PCS for SGMII.

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

I need to remind you that my purpose here is not to add drivers in
breadth for all SJA1110 sub-devices now.

But rather, a concrete use case (SGMII polarity inversion) has appeared
which requires the SGMII1..SGMII4 blocks to appear in the device tree
(so far, the SJA1110 driver has happily programmed these blocks based on
hardcoded SPI addresses in the driver, and there hasn't existed a reason
to describe them in the DT).

The SGMII blocks are highly reusable IPs licensed from Synopsys, and
Linux already has DT bindings and a corresponding platform driver for
the case where their registers are viewed using MMIO.

So my proposal in patch 14/15 is to create the following DT sub-nodes of
the DSA switch:
- regs/ethernet-pcs@...000 for SGMII1
- regs/ethernet-pcs@...000 for SGMII2
- regs/ethernet-pcs@...000 for SGMII3
- regs/ethernet-pcs@...000 for SGMII4

and to use MFD so that the xpcs-plat driver currently used for the MMIO
case "just works" for the "register view over SPI" case, and the SPI DT
node inherits the same bindings. In this sense, it is exactly the same
problem and solution as Colin Foster's ocelot set, at a smaller scale
(just one sub-device of this switch already had an established MMIO driver).
https://lore.kernel.org/netdev/20251118190530.580267-15-vladimir.oltean@nxp.com/

Furthermore, I also finalized the split of region handling that started
with the aforementioned SGMII blocks, by making the DSA driver stop
accessing the 100BASE-T1 and 100BASE-TX regions directly, and use MFD to
probe separate drivers for these resources.

I did not _have_ to do this for 100BASE-T1 and 100BASE-TX, but the
intention behind doing it was to solidify the argument that this device
has multiple regions for which the MFD model is suitable, rather than
impair it.

In the upstream DT bindings of the switch, the 100BASE-T1 region has a
corresponding mdios/mdio@0 child node, and the resource is hardcoded in
the driver. Similarly, the 100BASE-TX region is described as the
mdios/mdio@1 child. This is what I need this patch (07/15) for.

The intention is for all future sub-devices of the switch to live under
the "regs" sub-node, with the exception of "mdios/mdio@0" and "mdios/mdio@1"
which are already established somewhere else via a stable ABI. This
makes the SJA1110 a hybrid DSA+MFD driver, due to the impossibility of
getting rid of current DT bindings (this, plus the fact that I don't
necessarily see a problem with them).

In my opinion I do not need to add handling for any other sub-device,
for the support to be more "cross-system" like for Ocelot. What is here
is enough for you to decide if this is adequate for MFD or not.

The driver for SJA1110 needs a path forward from point A (where it
handles some resources internally which are outside the SWITCH region)
to point B (where those resources are handled by their correct reusable
drivers with specific DT bindings which we need). At the very least, I
expect you to clarify what are the problems you perceive in MFD being
part of this transition. I'd rather not speculate, and your previous
response is not sufficiently applied to the problem at hand.

Powered by blists - more mailing lists