Message-ID: <20250507153811.ukkficut2f3jm2hg@skbuf>
Date: Wed, 7 May 2025 18:38:11 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Ioana Ciornei <ioana.ciornei@....com>, Lee Jones <lee@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 5/6] arm64: dts: ls1028a-qds: make the QIXIS CPLD use the
 simple-mfd-i2c.c driver

Hi Krzysztof,

On Wed, May 07, 2025 at 03:56:20PM +0200, Krzysztof Kozlowski wrote:
> On 07/05/2025 14:28, Ioana Ciornei wrote:
> > On Wed, May 07, 2025 at 06:54:38AM +0200, Krzysztof Kozlowski wrote:
> >> On 06/05/2025 16:21, Ioana Ciornei wrote:
> >>> On Fri, May 02, 2025 at 09:04:03AM +0200, Krzysztof Kozlowski wrote:
> >>>> On Wed, Apr 30, 2025 at 06:36:33PM GMT, Ioana Ciornei wrote:
> >>>>> From: Vladimir Oltean <vladimir.oltean@....com>
> >>>>>
> >>>>> The MDIO mux on the LS1028A-QDS never worked in mainline. The device
> >>>>> tree was submitted as-is, and there is a downstream driver for the QIXIS
> >>>>> FPGA:
> >>>>>
> >>>>> https://github.com/nxp-qoriq/linux/blob/lf-6.12.y/drivers/soc/fsl/qixis_ctrl.c
> >>>>>
> >>>>> That driver is very similar to the already existing drivers/mfd/simple-mfd-i2c.c,
> >>>>> and the hardware works with the simple-mfd-i2c driver, so there isn't
> >>>>> any reason to upstream the other one.
> >>>>>
> >>>>> Adapt the compatible string and child node format of the FPGA node, so
> >>>>> that the simple-mfd-i2c driver accepts it.
> >>>>
> >>>> Why do you break the users based on some driver differences? Fix the
> >>>> drivers, not the DTS.
> >>>>
> >>>>>
> >>>>> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> >>>>> Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> >>>>> ---
> >>>>>  arch/arm64/boot/dts/freescale/fsl-ls1028a-qds.dts | 9 +++++----
> >>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds.dts
> >>>>> index 0bb2f28a0441..58b54d521d75 100644
> >>>>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds.dts
> >>>>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-qds.dts
> >>>>> @@ -338,17 +338,18 @@ sgtl5000: audio-codec@a {
> >>>>>  	};
> >>>>>  
> >>>>>  	fpga@66 {
> >>>>> -		compatible = "fsl,ls1028aqds-fpga", "fsl,fpga-qixis-i2c",
> >>>>> -			     "simple-mfd";
> >>>>> +		compatible = "fsl,ls1028a-qds-qixis-i2c";
> >>>>
> >>>> This breaks all the existing users. NAK.
> >>>
> >>> Using a mainline kernel, this DT node was never used or probed by a
> >>> driver since that driver was never submitted. I am not breaking any user
> >>> of the mainline kernel.
> >> 1. Users of DTS is plural, so what about all other projects and out of
> >> tree users?
> 
> You still can have users in all possible projects. Dropping simple-mfd,
> even though it is Linux thingy, is affecting users, potentially breaking
> them. Exactly what we talked on last plumbers - don't do such changes.
> 
> I recall even warning from Rob for people adding simple-mfd: be careful
> adding it, because dropping it creates compatibility issue.
> 
> This is not a fresh platform, where you do not care about users. It is
> published to all users since ~2019.
> 
> 
> >> 2. Did you remove simple-mfd from kernel or what? How can you claim
> >> there is no driver for simple-mfd?
> > 
> > No, I did not remove simple-mfd from the kernel.
> > 
> 
> 
> ...
> 
> > If, as you say, this works just by having the simple-mfd, I should have
> > been able to see the enetc_port1 functional and the RGMII PHY be
> > accesible on the MDIO bus which is behind the reg-mux. But this is not
> > happening.
> > 
> > Instead, I get this:
> > 
> > 	[   17.635216] platform mdio-mux: deferred probe pending: mdio-mux-multiplexer: Failed to get mux
> > 
> > 	root@...alhost:~# ip link set dev eno1 up
> > 	[ 1155.190391] net eno1: could not attach to PHY
> > 	root@...alhost:~# uname -a
> > 	Linux localhost 6.15.0-rc5-next-20250507 #112 SMP PREEMPT Wed May  7 15:21:14 EEST 2025 aarch64 aarch64 aarch64 GNU/Linux
> > 
> > From what I understand, even though the fpga@66 has the simple-mfd
> > compatible, no entity initializes an I2C regmap (since this is an I2C
> > device) for it so that it can be used by any child device.
> > 
> This sounds reasonable, thanks for providing context. Most of this (so a
> summary) should be in the commit msg as the rationale for changing the
> ABI, so please grow a bit the commit msg part:
> "The MDIO mux on the LS1028A-QDS never worked in mainline because ...".
> 
> With all this I still do not get why do you need to affect the
> compatibles. What is wrong with the actual compatibles?
> 
> Best regards,
> Krzysztof

I really care about not breaking compatibility with device trees too,
and that's why I occasionally report issues such as
https://lore.kernel.org/all/20250412001703.qbbfhtb6koofvner@skbuf/,
but in this case the compatibility breakage is not something to worry
about.

The QDS (QorIQ Development System) boards are not made to deploy any
production software on them, they are more fully-featured variants of
our RDB (Reference Design Boards) which sit in labs and are used
exclusively by engineers to test/prototype SoC features in order to
develop for other (production) platforms. Most if not all engineers use
TFTP to load the kernel and device tree at the same time. I think it's
going to be a case of a tree falling in a forest with no one to hear it.

In terms of other projects using the Linux device tree bindings - in
this case that would be U-Boot, which unfortunately has yet another
schema for the QIXIS CPLD:
https://github.com/u-boot/u-boot/blob/master/arch/arm/dts/fsl-ls1028a-qds.dtsi#L134
So not really a concern right now, but we will take it as an action item
to resync U-Boot with the upstream device trees for the LS1028A-QDS too,
like was done for the LS1028A-RDB.

In this case, why would changing the compatible string be preferable to
using the node as is? It would be nice for the QIXIS CPLD to have child
nodes with "reg". The current format lacks that, so we thought it would
be a cleaner break if we just introduced a new compatible string, to
make it easier to distinguish:
- "fsl,ls1028aqds-fpga", "fsl,fpga-qixis-i2c", "simple-mfd" doesn't
  expect children with "reg"
- "fsl,ls1028a-qds-qixis-i2c" does.

Powered by blists - more mailing lists