From: "J. Neuschäfer" <j.ne@posteo.net>

Convert the Freescale localbus controller bindings from text form to
YAML. The updated list of compatible strings reflects current usage
in arch/powerpc/boot/dts/, except that many existing device trees
erroneously specify "simple-bus" in addition to fsl,*elbc.

Changes compared to the txt version:
 - removed the board-control (fsl,mpc8272ads-bcsr) node because it only
   appears in this example and nowhere else
 - added a new example with NAND flash
 - updated list of compatible strings

Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
---

V2:
- fix order of properties in examples, according to dts coding style
- move to Documentation/devicetree/bindings/memory-controllers
- clarify the commit message a tiny bit
- remove unnecessary multiline markers (|)
- define address format in patternProperties
- trim subject line (remove "binding")
- remove use of "simple-bus", because it's technically incorrect
---
 .../bindings/memory-controllers/fsl,elbc.yaml      | 146 +++++++++++++++++++++
 .../devicetree/bindings/powerpc/fsl/lbc.txt        |  43 ------
 2 files changed, 146 insertions(+), 43 deletions(-)

diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl,elbc.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl,elbc.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..7bc05e3b9ac74125e5786748df57f6cc1255a62d
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/fsl,elbc.yaml
@@ -0,0 +1,146 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/fsl,elbc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale Enhanced Local Bus Controller
+
+maintainers:
+  - J. Neuschäfer <j.ne@posteo.net>
+
+properties:
+  $nodename:
+    pattern: "^localbus@[0-9a-f]+$"
+
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - fsl,mpc8313-elbc
+              - fsl,mpc8315-elbc
+              - fsl,mpc8377-elbc
+              - fsl,mpc8378-elbc
+              - fsl,mpc8379-elbc
+              - fsl,mpc8536-elbc
+              - fsl,mpc8569-elbc
+              - fsl,mpc8572-elbc
+              - fsl,p1020-elbc
+              - fsl,p1021-elbc
+              - fsl,p1023-elbc
+              - fsl,p2020-elbc
+              - fsl,p2041-elbc
+              - fsl,p3041-elbc
+              - fsl,p4080-elbc
+              - fsl,p5020-elbc
+              - fsl,p5040-elbc
+          - const: fsl,elbc
+
+      - items:
+          - const: fsl,mpc8272-localbus
+          - const: fsl,pq2-localbus
+
+      - items:
+          - enum:
+              - fsl,mpc8247-localbus
+              - fsl,mpc8248-localbus
+              - fsl,mpc8360-localbus
+          - const: fsl,pq2pro-localbus
+
+      - items:
+          - enum:
+              - fsl,mpc8540-localbus
+              - fsl,mpc8544-lbc
+              - fsl,mpc8544-localbus
+              - fsl,mpc8548-lbc
+              - fsl,mpc8548-localbus
+              - fsl,mpc8560-localbus
+              - fsl,mpc8568-localbus
+          - const: fsl,pq3-localbus
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#address-cells":
+    enum: [2, 3]
+    description:
+      The first cell is the chipselect number, and the remaining cells are the
+      offset into the chipselect.
+
+  "#size-cells":
+    enum: [1, 2]
+    description:
+      Either one or two, depending on how large each chipselect can be.
+
+  ranges:
+    description:
+      Each range corresponds to a single chipselect, and covers the entire
+      access window as configured.
+
+patternProperties:
+  # format: name@chipselect,address
+  "^.*@[0-9a-f]+,[0-9a-f]+$":
+    type: object
+
+additionalProperties: false
+
+examples:
+  - |
+    localbus@f0010100 {
+        compatible = "fsl,mpc8272-localbus",
+                     "fsl,pq2-localbus";
+        reg = <0xf0010100 0x40>;
+        ranges = <0x0 0x0 0xfe000000 0x02000000
+                  0x1 0x0 0xf4500000 0x00008000
+                  0x2 0x0 0xfd810000 0x00010000>;
+        #address-cells = <2>;
+        #size-cells = <1>;
+
+        flash@0,0 {
+            compatible = "jedec-flash";
+            reg = <0x0 0x0 0x2000000>;
+            bank-width = <4>;
+            device-width = <1>;
+        };
+
+        simple-periph@2,0 {
+            compatible = "fsl,elbc-gpcm-uio";
+            reg = <0x2 0x0 0x10000>;
+            elbc-gpcm-br = <0xfd810800>;
+            elbc-gpcm-or = <0xffff09f7>;
+        };
+    };
+
+  - |
+    localbus@e0005000 {
+        compatible = "fsl,mpc8315-elbc", "fsl,elbc";
+        reg = <0xe0005000 0x1000>;
+        ranges = <0x0 0x0 0xfe000000 0x00800000
+                  0x1 0x0 0xe0600000 0x00002000
+                  0x2 0x0 0xf0000000 0x00020000
+                  0x3 0x0 0xfa000000 0x00008000>;
+        #address-cells = <2>;
+        #size-cells = <1>;
+        interrupts = <77 0x8>;
+        interrupt-parent = <&ipic>;
+
+        flash@0,0 {
+            compatible = "cfi-flash";
+            reg = <0x0 0x0 0x800000>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+            bank-width = <2>;
+            device-width = <1>;
+        };
+
+        nand@1,0 {
+            compatible = "fsl,mpc8315-fcm-nand",
+                         "fsl,elbc-fcm-nand";
+            reg = <0x1 0x0 0x2000>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/powerpc/fsl/lbc.txt b/Documentation/devicetree/bindings/powerpc/fsl/lbc.txt
deleted file mode 100644
index 1c80fcedebb52049721fbd61c4dd4c57133bd47c..0000000000000000000000000000000000000000
--- a/Documentation/devicetree/bindings/powerpc/fsl/lbc.txt
+++ /dev/null
@@ -1,43 +0,0 @@
-* Chipselect/Local Bus
-
-Properties:
-- name : Should be localbus
-- #address-cells : Should be either two or three.  The first cell is the
-                   chipselect number, and the remaining cells are the
-                   offset into the chipselect.
-- #size-cells : Either one or two, depending on how large each chipselect
-                can be.
-- ranges : Each range corresponds to a single chipselect, and cover
-           the entire access window as configured.
-
-Example:
-	localbus@f0010100 {
-		compatible = "fsl,mpc8272-localbus",
-			   "fsl,pq2-localbus";
-		#address-cells = <2>;
-		#size-cells = <1>;
-		reg = <0xf0010100 0x40>;
-
-		ranges = <0x0 0x0 0xfe000000 0x02000000
-			  0x1 0x0 0xf4500000 0x00008000
-			  0x2 0x0 0xfd810000 0x00010000>;
-
-		flash@0,0 {
-			compatible = "jedec-flash";
-			reg = <0x0 0x0 0x2000000>;
-			bank-width = <4>;
-			device-width = <1>;
-		};
-
-		board-control@1,0 {
-			reg = <0x1 0x0 0x20>;
-			compatible = "fsl,mpc8272ads-bcsr";
-		};
-
-		simple-periph@2,0 {
-			compatible = "fsl,elbc-gpcm-uio";
-			reg = <0x2 0x0 0x10000>;
-			elbc-gpcm-br = <0xfd810800>;
-			elbc-gpcm-or = <0xffff09f7>;
-		};
-	};

-- 
2.48.0.rc1.219.gb6b6757d772

On Fri, Feb 07, 2025 at 10:30:26PM +0100, J. Neuschäfer via B4 Relay wrote:
> From: "J. Neuschäfer" <j.ne@posteo.net>
> 
> Convert the Freescale localbus controller bindings from text form to
> YAML. The updated list of compatible strings reflects current usage
> in arch/powerpc/boot/dts/, except that many existing device trees
> erroneously specify "simple-bus" in addition to fsl,*elbc.
> 
> Changes compared to the txt version:
>  - removed the board-control (fsl,mpc8272ads-bcsr) node because it only
>    appears in this example and nowhere else
>  - added a new example with NAND flash
>  - updated list of compatible strings
> 
> Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> ---
> 
> V2:
> - fix order of properties in examples, according to dts coding style
> - move to Documentation/devicetree/bindings/memory-controllers
> - clarify the commit message a tiny bit
> - remove unnecessary multiline markers (|)
> - define address format in patternProperties
> - trim subject line (remove "binding")
> - remove use of "simple-bus", because it's technically incorrect

While I admit I haven't been following recent developments in this area,
as someone who was involved when "simple-bus" was created (and was on the
ePAPR committee that standardized it) I'm surprised to hear simple-bus
being called "erroneous" or "technically incorrect" here.

For non-NAND devices this bus generally meets the definition of "an
internal I/O bus that cannot be probed for devices" where "devices on the
bus can be accessed directly without additional configuration
required".  NAND flash is an exception, but those devices have
compatibles that are specific to the bus controller.

The fact that the address encoding is non-linear is irrelevant; the
addresses can still be translated using the standard "ranges" mechanism. 
This seems to be a disconnect between the schema verification and the way
the compatible has previously been defined and used.

And as a practical matter, unless I'm missing something (which I might be
since I haven't been in devicetree-land for nearly a decade), Linux is
relying on simple-bus to probe these devices.  There is a driver that
binds to the bus itself but that is just for error interrupts and NAND.

You'd probably need something like commit 3e25f800afb82bd9e5f8 ("memory:
fsl_ifc: populate child devices without relying on simple-bus") and the 
subsequent fix in dd8adc713b1656 ("memory: fsl_ifc: populate child
nodes of buses and mfd devices")...

I'm curious what the reasoning was for removing simple-bus from IFC.  It
seems that the schema verification also played a role in that:
https://www.spinics.net/lists/devicetree/msg220418.html

...but there's also the comment in 985ede63a045eabf3f9d ("dt-bindings:
memory: fsl: convert ifc binding to yaml schema") that "this will help to
enforce the correct probe order between parent device and child devices",
but was that really not already guaranteed by the parent/child
relationship (and again, it should only really matter for NAND except for
the possibility of missing error reports during early boot)?

> +  compatible:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - fsl,mpc8313-elbc
> +              - fsl,mpc8315-elbc
> +              - fsl,mpc8377-elbc
> +              - fsl,mpc8378-elbc
> +              - fsl,mpc8379-elbc
> +              - fsl,mpc8536-elbc
> +              - fsl,mpc8569-elbc
> +              - fsl,mpc8572-elbc
> +              - fsl,p1020-elbc
> +              - fsl,p1021-elbc
> +              - fsl,p1023-elbc
> +              - fsl,p2020-elbc
> +              - fsl,p2041-elbc
> +              - fsl,p3041-elbc
> +              - fsl,p4080-elbc
> +              - fsl,p5020-elbc
> +              - fsl,p5040-elbc
> +          - const: fsl,elbc

Is it really necessary to list every single chip?

And then it would need to be updated when new ones came out?  I know this
particular line of chips is not going to see any new members at this
point, but as far as the general approach goes...

Does the schema validation complain if it sees an extra compatible it
doesn't recognize?  If so that's obnoxious.

> +examples:
> +  - |
> +    localbus@f0010100 {
> +        compatible = "fsl,mpc8272-localbus",
> +                     "fsl,pq2-localbus";
> +        reg = <0xf0010100 0x40>;
> +        ranges = <0x0 0x0 0xfe000000 0x02000000
> +                  0x1 0x0 0xf4500000 0x00008000
> +                  0x2 0x0 0xfd810000 0x00010000>;
> +        #address-cells = <2>;
> +        #size-cells = <1>;
> +
> +        flash@0,0 {
> +            compatible = "jedec-flash";
> +            reg = <0x0 0x0 0x2000000>;
> +            bank-width = <4>;
> +            device-width = <1>;
> +        };
> +
> +        simple-periph@2,0 {
> +            compatible = "fsl,elbc-gpcm-uio";
> +            reg = <0x2 0x0 0x10000>;
> +            elbc-gpcm-br = <0xfd810800>;
> +            elbc-gpcm-or = <0xffff09f7>;
> +        };

I know this isn't new, but... since we're using this as an example,
where is the documentation for this fsl,elbc-gpcm-uio and
elbc-gpcm-br/or?  What exactly is a simple-periph?

There are no in-tree device trees that use this either.  The bcsr
node was actually a much more normal example, despite that particular
platform having been removed.  There are other bcsr nodes that still
exist that could be used instead.

-Crystal

On Sun, Feb 09, 2025 at 02:31:34PM -0600, Crystal Wood wrote:
> On Fri, Feb 07, 2025 at 10:30:26PM +0100, J. Neuschäfer via B4 Relay wrote:
> > From: "J. Neuschäfer" <j.ne@posteo.net>
> > 
> > Convert the Freescale localbus controller bindings from text form to
> > YAML. The updated list of compatible strings reflects current usage
> > in arch/powerpc/boot/dts/, except that many existing device trees
> > erroneously specify "simple-bus" in addition to fsl,*elbc.
> > 
> > Changes compared to the txt version:
> >  - removed the board-control (fsl,mpc8272ads-bcsr) node because it only
> >    appears in this example and nowhere else
> >  - added a new example with NAND flash
> >  - updated list of compatible strings
> > 
> > Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> > ---
> > 
> > V2:
> > - fix order of properties in examples, according to dts coding style
> > - move to Documentation/devicetree/bindings/memory-controllers
> > - clarify the commit message a tiny bit
> > - remove unnecessary multiline markers (|)
> > - define address format in patternProperties
> > - trim subject line (remove "binding")
> > - remove use of "simple-bus", because it's technically incorrect
> 
> While I admit I haven't been following recent developments in this area,
> as someone who was involved when "simple-bus" was created (and was on the
> ePAPR committee that standardized it) I'm surprised to hear simple-bus
> being called "erroneous" or "technically incorrect" here.

Erroneous because the binding did not say "simple-bus" was used. Not 
uncommon with the old .txt bindings.

Generally, if a bus has control registers or resources like clocks, then 
we tend not to call them 'simple-bus'. And '"specific-bus", 
"simple-bus"' gives some problems around what driver if any do you 
bind to. 

If you have chip selects, then you have config registers for those. 
Not really "simple" if you ask me. That being said, you could keep 
'simple-bus' here. I would tend to err on making the schema match the 
actual .dts rather than updating the .dts files on older platforms like 
these.

> For non-NAND devices this bus generally meets the definition of "an
> internal I/O bus that cannot be probed for devices" where "devices on the
> bus can be accessed directly without additional configuration
> required".  NAND flash is an exception, but those devices have
> compatibles that are specific to the bus controller.

NAND bindings have evolved quite a bit if you haven't been paying 
attention.

> The fact that the address encoding is non-linear is irrelevant; the
> addresses can still be translated using the standard "ranges" mechanism. 
> This seems to be a disconnect between the schema verification and the way
> the compatible has previously been defined and used.
> 
> And as a practical matter, unless I'm missing something (which I might be
> since I haven't been in devicetree-land for nearly a decade), Linux is
> relying on simple-bus to probe these devices.  There is a driver that
> binds to the bus itself but that is just for error interrupts and NAND.
> 
> You'd probably need something like commit 3e25f800afb82bd9e5f8 ("memory:
> fsl_ifc: populate child devices without relying on simple-bus") and the 
> subsequent fix in dd8adc713b1656 ("memory: fsl_ifc: populate child
> nodes of buses and mfd devices")...
> 
> I'm curious what the reasoning was for removing simple-bus from IFC.  It
> seems that the schema verification also played a role in that:
> https://www.spinics.net/lists/devicetree/msg220418.html

If a kernel change is needed to support changed .dts files, then we 
shouldn't be doing that here (being mature platforms). That would mean 
new DTB will not work with existing kernels.

> 
> ...but there's also the comment in 985ede63a045eabf3f9d ("dt-bindings:
> memory: fsl: convert ifc binding to yaml schema") that "this will help to
> enforce the correct probe order between parent device and child devices",
> but was that really not already guaranteed by the parent/child
> relationship (and again, it should only really matter for NAND except for
> the possibility of missing error reports during early boot)?
> 
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - fsl,mpc8313-elbc
> > +              - fsl,mpc8315-elbc
> > +              - fsl,mpc8377-elbc
> > +              - fsl,mpc8378-elbc
> > +              - fsl,mpc8379-elbc
> > +              - fsl,mpc8536-elbc
> > +              - fsl,mpc8569-elbc
> > +              - fsl,mpc8572-elbc
> > +              - fsl,p1020-elbc
> > +              - fsl,p1021-elbc
> > +              - fsl,p1023-elbc
> > +              - fsl,p2020-elbc
> > +              - fsl,p2041-elbc
> > +              - fsl,p3041-elbc
> > +              - fsl,p4080-elbc
> > +              - fsl,p5020-elbc
> > +              - fsl,p5040-elbc
> > +          - const: fsl,elbc
> 
> Is it really necessary to list every single chip?

Yes. If they exist, they have to be documented.

> 
> And then it would need to be updated when new ones came out?  I know this
> particular line of chips is not going to see any new members at this
> point, but as far as the general approach goes...
> 
> Does the schema validation complain if it sees an extra compatible it
> doesn't recognize?  If so that's obnoxious.

Yes.

More annoying is having to boot and debug typos:

compatible = "foo,bar", "simplebus";

Rob

On Mon, Feb 10, 2025 at 03:53:24PM -0600, Rob Herring wrote:
> Generally, if a bus has control registers or resources like clocks, then 
> we tend not to call them 'simple-bus'. And '"specific-bus", 
> "simple-bus"' gives some problems around what driver if any do you 
> bind to. 

Isn't the general idea that you bind to the first one in the list that
you have a driver for, since it goes from most to least specific?

> If you have chip selects, then you have config registers for those. 
> Not really "simple" if you ask me. That being said, you could keep 
> 'simple-bus' here. I would tend to err on making the schema match the 
> actual .dts rather than updating the .dts files on older platforms like 
> these.

By that definition I wonder how much truly qualifies.  Even with
IMMR/CCSR, firmware needs to at least set the base register (which is
itself inside CCSR, so there's no way to avoid relying on knowledge of
what the firmware did, except on 8xx).  Though I acknowledge that eLBC is
a stretch, with FCM and UPM being exceptions.  FCM didn't exist in the
original LBC, and UPM was... kind of considered a fringe use case
until someone hooked NAND up to it.  :-P

The point back then wasn't that such registers don't exist, but that the
OS can use the devices without having to care.  But of course, there's
subjectivity there about what the OS might care about (e.g. UPM).

FWIW, on these chips (especially the later ones) there were all sorts of
things (in general, not specifically LBC-related) that firmware had to
set up to present a coherent system to the OS.  Not all the choices made
there were great, but if we tried to describe all the gory details from
the start I'm sure we would have made an even bigger mess of it.

> > For non-NAND devices this bus generally meets the definition of "an
> > internal I/O bus that cannot be probed for devices" where "devices on the
> > bus can be accessed directly without additional configuration
> > required".  NAND flash is an exception, but those devices have
> > compatibles that are specific to the bus controller.
> 
> NAND bindings have evolved quite a bit if you haven't been paying 
> attention.

I haven't, as I acknowledged... but I was describing how eLBC does it,
and just meant that we're not binding to drivers that don't know about
the bus in that case.  The NAND control registers are part of eLBC/IFC,
not a separate block (the reg in the NAND node itself is just the SRAM
used as a buffer).  I'm not sure what that would be expected to look like
these days.

-Crystal

On Mon, Feb 10, 2025 at 03:53:24PM -0600, Rob Herring wrote:
> On Sun, Feb 09, 2025 at 02:31:34PM -0600, Crystal Wood wrote:
> > On Fri, Feb 07, 2025 at 10:30:26PM +0100, J. Neuschäfer via B4 Relay wrote:
> > > From: "J. Neuschäfer" <j.ne@posteo.net>
> > > 
> > > Convert the Freescale localbus controller bindings from text form to
> > > YAML. The updated list of compatible strings reflects current usage
> > > in arch/powerpc/boot/dts/, except that many existing device trees
> > > erroneously specify "simple-bus" in addition to fsl,*elbc.
> > > 
> > > Changes compared to the txt version:
> > >  - removed the board-control (fsl,mpc8272ads-bcsr) node because it only
> > >    appears in this example and nowhere else
> > >  - added a new example with NAND flash
> > >  - updated list of compatible strings
> > > 
> > > Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> > > ---
> > > 
> > > V2:
> > > - fix order of properties in examples, according to dts coding style
> > > - move to Documentation/devicetree/bindings/memory-controllers
> > > - clarify the commit message a tiny bit
> > > - remove unnecessary multiline markers (|)
> > > - define address format in patternProperties
> > > - trim subject line (remove "binding")
> > > - remove use of "simple-bus", because it's technically incorrect
> > 
> > While I admit I haven't been following recent developments in this area,
> > as someone who was involved when "simple-bus" was created (and was on the
> > ePAPR committee that standardized it) I'm surprised to hear simple-bus
> > being called "erroneous" or "technically incorrect" here.
> 
> Erroneous because the binding did not say "simple-bus" was used. Not 
> uncommon with the old .txt bindings.
> 
> Generally, if a bus has control registers or resources like clocks, then 
> we tend not to call them 'simple-bus'. And '"specific-bus", 
> "simple-bus"' gives some problems around what driver if any do you 
> bind to. 
[...]
> > You'd probably need something like commit 3e25f800afb82bd9e5f8 ("memory:
> > fsl_ifc: populate child devices without relying on simple-bus") and the 
> > subsequent fix in dd8adc713b1656 ("memory: fsl_ifc: populate child
> > nodes of buses and mfd devices")...
> > 
> > I'm curious what the reasoning was for removing simple-bus from IFC.  It
> > seems that the schema verification also played a role in that:
> > https://www.spinics.net/lists/devicetree/msg220418.html
> 
> If a kernel change is needed to support changed .dts files, then we 
> shouldn't be doing that here (being mature platforms). That would mean 
> new DTB will not work with existing kernels.

Alright, I'll keep simple-bus in the eLBC binding for historical
compatibility.


Thank you both for your discussion.


J. Neuschäfer

On Sun, Feb 09, 2025 at 02:31:34PM -0600, Crystal Wood wrote:
> On Fri, Feb 07, 2025 at 10:30:26PM +0100, J. Neuschäfer via B4 Relay wrote:
> > From: "J. Neuschäfer" <j.ne@posteo.net>
> > 
> > Convert the Freescale localbus controller bindings from text form to
> > YAML. The updated list of compatible strings reflects current usage
> > in arch/powerpc/boot/dts/, except that many existing device trees
> > erroneously specify "simple-bus" in addition to fsl,*elbc.
> > 
> > Changes compared to the txt version:
> >  - removed the board-control (fsl,mpc8272ads-bcsr) node because it only
> >    appears in this example and nowhere else
> >  - added a new example with NAND flash
> >  - updated list of compatible strings
> > 
> > Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> > ---
> > 
> > V2:
> > - fix order of properties in examples, according to dts coding style
> > - move to Documentation/devicetree/bindings/memory-controllers
> > - clarify the commit message a tiny bit
> > - remove unnecessary multiline markers (|)
> > - define address format in patternProperties
> > - trim subject line (remove "binding")
> > - remove use of "simple-bus", because it's technically incorrect
> 
> While I admit I haven't been following recent developments in this area,
> as someone who was involved when "simple-bus" was created (and was on the
> ePAPR committee that standardized it) I'm surprised to hear simple-bus
> being called "erroneous" or "technically incorrect" here.

It is quite possible that my understanding of it is incomplete or wrong.

> 
> For non-NAND devices this bus generally meets the definition of "an
> internal I/O bus that cannot be probed for devices" where "devices on the
> bus can be accessed directly without additional configuration
> required".  NAND flash is an exception, but those devices have
> compatibles that are specific to the bus controller.
> 
> The fact that the address encoding is non-linear is irrelevant; the
> addresses can still be translated using the standard "ranges" mechanism. 
> This seems to be a disconnect between the schema verification and the way
> the compatible has previously been defined and used.

This is what led me to my assumptions: The simple-bus validation logic
in dtc complains about unit addresses such as nand@1,0 which are quite
appropriate for the eLBC.

> 
> And as a practical matter, unless I'm missing something (which I might be
> since I haven't been in devicetree-land for nearly a decade), Linux is
> relying on simple-bus to probe these devices.  There is a driver that
> binds to the bus itself but that is just for error interrupts and NAND.

As of now, yes, that's correct. Without simple-bus, a current Linux
kernel doesn't find the device nodes inside such a localbus.

> 
> You'd probably need something like commit 3e25f800afb82bd9e5f8 ("memory:
> fsl_ifc: populate child devices without relying on simple-bus") and the 
> subsequent fix in dd8adc713b1656 ("memory: fsl_ifc: populate child
> nodes of buses and mfd devices")...

I have prepared such a patch, based on the same assumptions:

  [PATCH] powerpc/fsl_lbc: Explicitly populate bus
  https://lore.kernel.org/lkml/20250209-localbus-v1-1-efcd780153a0@posteo.net/

> 
> I'm curious what the reasoning was for removing simple-bus from IFC.  It
> seems that the schema verification also played a role in that:
> https://www.spinics.net/lists/devicetree/msg220418.html

Yes, that's the same as my reasoning.

> 
> ...but there's also the comment in 985ede63a045eabf3f9d ("dt-bindings:
> memory: fsl: convert ifc binding to yaml schema") that "this will help to
> enforce the correct probe order between parent device and child devices",
> but was that really not already guaranteed by the parent/child
> relationship (and again, it should only really matter for NAND except for
> the possibility of missing error reports during early boot)?

I'm inclined to agree with you, but it's somewhat beyond my skill level.

I'll let Li Yang or Rob Herring comment on that.

> 
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - fsl,mpc8313-elbc
> > +              - fsl,mpc8315-elbc
> > +              - fsl,mpc8377-elbc
> > +              - fsl,mpc8378-elbc
> > +              - fsl,mpc8379-elbc
> > +              - fsl,mpc8536-elbc
> > +              - fsl,mpc8569-elbc
> > +              - fsl,mpc8572-elbc
> > +              - fsl,p1020-elbc
> > +              - fsl,p1021-elbc
> > +              - fsl,p1023-elbc
> > +              - fsl,p2020-elbc
> > +              - fsl,p2041-elbc
> > +              - fsl,p3041-elbc
> > +              - fsl,p4080-elbc
> > +              - fsl,p5020-elbc
> > +              - fsl,p5040-elbc
> > +          - const: fsl,elbc
> 
> Is it really necessary to list every single chip?
> 
> And then it would need to be updated when new ones came out?  I know this
> particular line of chips is not going to see any new members at this
> point, but as far as the general approach goes...

As far as I'm aware, this reflects common practice today.

> 
> Does the schema validation complain if it sees an extra compatible it
> doesn't recognize?  If so that's obnoxious.

Yes.

> 
> > +examples:
> > +  - |
> > +    localbus@f0010100 {
> > +        compatible = "fsl,mpc8272-localbus",
> > +                     "fsl,pq2-localbus";
> > +        reg = <0xf0010100 0x40>;
> > +        ranges = <0x0 0x0 0xfe000000 0x02000000
> > +                  0x1 0x0 0xf4500000 0x00008000
> > +                  0x2 0x0 0xfd810000 0x00010000>;
> > +        #address-cells = <2>;
> > +        #size-cells = <1>;
> > +
> > +        flash@0,0 {
> > +            compatible = "jedec-flash";
> > +            reg = <0x0 0x0 0x2000000>;
> > +            bank-width = <4>;
> > +            device-width = <1>;
> > +        };
> > +
> > +        simple-periph@2,0 {
> > +            compatible = "fsl,elbc-gpcm-uio";
> > +            reg = <0x2 0x0 0x10000>;
> > +            elbc-gpcm-br = <0xfd810800>;
> > +            elbc-gpcm-or = <0xffff09f7>;
> > +        };
> 
> I know this isn't new, but... since we're using this as an example,
> where is the documentation for this fsl,elbc-gpcm-uio and
> elbc-gpcm-br/or?  What exactly is a simple-periph?

fsl,elbc-gpcm-uio is handled in the following patch
(dt-bindings: memory-controllers: Add fsl,elbc-gpcm-uio).

simple-periph is something I haven't thought about, because this whole
example comes from the old txt-format binding. The whole purpose of
fsl,elbc-gpcm-uio is to allow userspace drivers to interact with
localbus devices, so that doesn't make the intention any clearer, either.

> 
> There are no in-tree device trees that use this either.  The bcsr
> node was actually a much more normal example, despite that particular
> platform having been removed.  There are other bcsr nodes that still
> exist that could be used instead.

Ah, fsl,mpc8568mds-bcsr for example, good point. I'll add it back.

> 
> -Crystal

Thank you for reaching out!

Best regards,
J. Neuschäfer

On Sun, Feb 09, 2025 at 02:31:35PM -0600, Crystal Wood wrote:
> On Fri, Feb 07, 2025 at 10:30:26PM +0100, J. Neuschäfer via B4 Relay wrote:
> > +        simple-periph@2,0 {
> > +            compatible = "fsl,elbc-gpcm-uio";
> > +            reg = <0x2 0x0 0x10000>;
> > +            elbc-gpcm-br = <0xfd810800>;
> > +            elbc-gpcm-or = <0xffff09f7>;
> > +        };
> 
> I know this isn't new, but... since we're using this as an example,
> where is the documentation for this fsl,elbc-gpcm-uio and
> elbc-gpcm-br/or?  What exactly is a simple-periph?
> 
> There are no in-tree device trees that use this either.  The bcsr
> node was actually a much more normal example, despite that particular
> platform having been removed.  There are other bcsr nodes that still
> exist that could be used instead.

OK, I noticed patch 10 after I sent this :-P

Seems I didn't like it too much when it was new either:
https://lkml.org/lkml/2014/12/9/530

And it's still a bad example for how GPCM devices on this bus should
normally be represented.

-Crystal

On Fri, 07 Feb 2025 22:30:26 +0100, J. Neuschäfer wrote:
> Convert the Freescale localbus controller bindings from text form to
> YAML. The updated list of compatible strings reflects current usage
> in arch/powerpc/boot/dts/, except that many existing device trees
> erroneously specify "simple-bus" in addition to fsl,*elbc.
> 
> Changes compared to the txt version:
>  - removed the board-control (fsl,mpc8272ads-bcsr) node because it only
>    appears in this example and nowhere else
>  - added a new example with NAND flash
>  - updated list of compatible strings
> 
> Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> ---
> 
> V2:
> - fix order of properties in examples, according to dts coding style
> - move to Documentation/devicetree/bindings/memory-controllers
> - clarify the commit message a tiny bit
> - remove unnecessary multiline markers (|)
> - define address format in patternProperties
> - trim subject line (remove "binding")
> - remove use of "simple-bus", because it's technically incorrect
> ---
>  .../bindings/memory-controllers/fsl,elbc.yaml      | 146 +++++++++++++++++++++
>  .../devicetree/bindings/powerpc/fsl/lbc.txt        |  43 ------
>  2 files changed, 146 insertions(+), 43 deletions(-)
> 

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-0/localbus@f0010100/simple-periph@2,0: failed to match any schema with compatible: ['fsl,elbc-gpcm-uio']
Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']
Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']

doc reference errors (make refcheckdocs):
Warning: Documentation/devicetree/bindings/display/ssd1289fb.txt references a file that doesn't exist: Documentation/devicetree/bindings/powerpc/fsl/lbc.txt
Documentation/devicetree/bindings/display/ssd1289fb.txt: Documentation/devicetree/bindings/powerpc/fsl/lbc.txt

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250207-ppcyaml-v2-9-8137b0c42526@posteo.net

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

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

pip3 install dtschema --upgrade

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

On Fri, Feb 07, 2025 at 05:44:59PM -0600, Rob Herring (Arm) wrote:
> On Fri, 07 Feb 2025 22:30:26 +0100, J. Neuschäfer wrote:
[...]
> >  .../bindings/memory-controllers/fsl,elbc.yaml      | 146 +++++++++++++++++++++
> >  .../devicetree/bindings/powerpc/fsl/lbc.txt        |  43 ------
> >  2 files changed, 146 insertions(+), 43 deletions(-)
[...]
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-0/localbus@f0010100/simple-periph@2,0: failed to match any schema with compatible: ['fsl,elbc-gpcm-uio']
> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']
> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']

I think this is due to how the patches are ordered in the series.
This patch uses fsl,elbc-gpcm-uio and fsl,elbc-fcm-nand in examples, but
comes before the patches that define the corresponding bindings.

On 09/02/2025 18:28, J. Neuschäfer wrote:
> On Fri, Feb 07, 2025 at 05:44:59PM -0600, Rob Herring (Arm) wrote:
>> On Fri, 07 Feb 2025 22:30:26 +0100, J. Neuschäfer wrote:
> [...]
>>>  .../bindings/memory-controllers/fsl,elbc.yaml      | 146 +++++++++++++++++++++
>>>  .../devicetree/bindings/powerpc/fsl/lbc.txt        |  43 ------
>>>  2 files changed, 146 insertions(+), 43 deletions(-)
> [...]
>> dtschema/dtc warnings/errors:
>> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-0/localbus@f0010100/simple-periph@2,0: failed to match any schema with compatible: ['fsl,elbc-gpcm-uio']
>> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']
>> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']
> 
> I think this is due to how the patches are ordered in the series.

If that's possible, this should be fixed, e.g.  by re-ordering the patches.

> This patch uses fsl,elbc-gpcm-uio and fsl,elbc-fcm-nand in examples, but
> comes before the patches that define the corresponding bindings.


Best regards,
Krzysztof

On Sun, Feb 09, 2025 at 06:30:44PM +0100, Krzysztof Kozlowski wrote:
> On 09/02/2025 18:28, J. Neuschäfer wrote:
> > On Fri, Feb 07, 2025 at 05:44:59PM -0600, Rob Herring (Arm) wrote:
> >> On Fri, 07 Feb 2025 22:30:26 +0100, J. Neuschäfer wrote:
> > [...]
> >>>  .../bindings/memory-controllers/fsl,elbc.yaml      | 146 +++++++++++++++++++++
> >>>  .../devicetree/bindings/powerpc/fsl/lbc.txt        |  43 ------
> >>>  2 files changed, 146 insertions(+), 43 deletions(-)
> > [...]
> >> dtschema/dtc warnings/errors:
> >> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-0/localbus@f0010100/simple-periph@2,0: failed to match any schema with compatible: ['fsl,elbc-gpcm-uio']
> >> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']
> >> Documentation/devicetree/bindings/memory-controllers/fsl,elbc.example.dtb: /example-1/localbus@e0005000/nand@1,0: failed to match any schema with compatible: ['fsl,mpc8315-fcm-nand', 'fsl,elbc-fcm-nand']
> > 
> > I think this is due to how the patches are ordered in the series.
> 
> If that's possible, this should be fixed, e.g.  by re-ordering the patches.

Yes, I'll do that for the next iteration


Best regards,
J. Neuschäfer