[PATCH] dt-bindings: memory-controllers: arm,pl353-smc: Extend to support 'arm,pl354' SMC

[PATCH] dt-bindings: memory-controllers: arm,pl353-smc: Extend to support 'arm,pl354' SMC

[Rob Herring]

Add support for the Arm PL354 static memory controller to the existing
Arm PL353 binding. Both are different configurations of the same IP with
support for different types of memory interfaces.

The 'arm,pl354' binding has already been in use upstream for a long time
in Arm development boards. The existing users have only the controller
without any child devices, so drop the required address properties
(ranges, #address-cells, #size-cells). The schema for 'ranges' is too
constrained as the order is not important and the PL354 has 8
chipselects (And the PL353 actually has up to 8 too).

The clocks aren't really correct in either case. There's 1 bus clock and
then a clock for each of the 2 memory interfaces.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 ...{arm,pl353-smc.yaml => arm,pl35x-smc.yaml} | 80 ++++++++++++-------
 1 file changed, 53 insertions(+), 27 deletions(-)
 rename Documentation/devicetree/bindings/memory-controllers/{arm,pl353-smc.yaml => arm,pl35x-smc.yaml} (65%)

diff --git a/Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml b/Documentation/devicetree/bindings/memory-controllers/arm,pl35x-smc.yaml
similarity index 65%
rename from Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml
rename to Documentation/devicetree/bindings/memory-controllers/arm,pl35x-smc.yaml
index 01c9acf9275d..bd23257fe021 100644
--- a/Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/arm,pl35x-smc.yaml
@@ -1,26 +1,31 @@
 # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/memory-controllers/arm,pl353-smc.yaml#
+$id: http://devicetree.org/schemas/memory-controllers/arm,pl35x-smc.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: ARM PL353 Static Memory Controller (SMC) device-tree bindings
+title: Arm PL35x Series Static Memory Controller (SMC)
 
 maintainers:
   - Miquel Raynal <miquel.raynal@bootlin.com>
   - Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
 
-description:
-  The PL353 Static Memory Controller is a bus where you can connect two kinds
+description: |
+  The PL35x Static Memory Controller is a bus where you can connect two kinds
   of memory interfaces, which are NAND and memory mapped interfaces (such as
-  SRAM or NOR).
+  SRAM or NOR) depending on the specific configuration.
+
+  The TRM is available here:
+  https://documentation-service.arm.com/static/5e8e2524fd977155116a58aa
 
 # We need a select here so we don't match all nodes with 'arm,primecell'
 select:
   properties:
     compatible:
       contains:
-        const: arm,pl353-smc-r2p1
+        enum:
+          - arm,pl353-smc-r2p1
+          - arm,pl354
   required:
     - compatible
 
@@ -30,7 +35,9 @@ properties:
 
   compatible:
     items:
-      - const: arm,pl353-smc-r2p1
+      - enum:
+          - arm,pl353-smc-r2p1
+          - arm,pl354
       - const: arm,primecell
 
   "#address-cells":
@@ -46,30 +53,25 @@ properties:
           The three chip select regions are defined in 'ranges'.
 
   clocks:
-    items:
-      - description: clock for the memory device bus
-      - description: main clock of the SMC
+    minItems: 1
+    maxItems: 2
 
   clock-names:
-    items:
-      - const: memclk
-      - const: apb_pclk
+    minItems: 1
+    maxItems: 2
 
   ranges:
     minItems: 1
-    description: |
-      Memory bus areas for interacting with the devices. Reflects
-      the memory layout with four integer values following:
-      <cs-number> 0 <offset> <size>
-    items:
-      - description: NAND bank 0
-      - description: NOR/SRAM bank 0
-      - description: NOR/SRAM bank 1
+    maxItems: 8
 
-  interrupts: true
+  interrupts:
+    minItems: 1
+    items:
+      - description: Combined or Memory interface 0 IRQ
+      - description: Memory interface 1 IRQ
 
 patternProperties:
-  "@[0-3],[a-f0-9]+$":
+  "@[0-7],[a-f0-9]+$":
     type: object
     description: |
       The child device node represents the controller connected to the SMC
@@ -87,7 +89,7 @@ patternProperties:
               - description: |
                   Chip-select ID, as in the parent range property.
                 minimum: 0
-                maximum: 2
+                maximum: 7
               - description: |
                   Offset of the memory region requested by the device.
               - description: |
@@ -102,12 +104,36 @@ required:
   - reg
   - clock-names
   - clocks
-  - "#address-cells"
-  - "#size-cells"
-  - ranges
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: arm,pl354
+    then:
+      properties:
+        clocks:
+          # According to TRM, really should be 3 clocks
+          maxItems: 1
+
+        clock-names:
+          const: apb_pclk
+
+    else:
+      properties:
+        clocks:
+          items:
+            - description: clock for the memory device bus
+            - description: main clock of the SMC
+
+        clock-names:
+          items:
+            - const: memclk
+            - const: apb_pclk
+
 examples:
   - |
     smcc: memory-controller@e000e000 {
-- 
2.35.1

Re: [PATCH] dt-bindings: memory-controllers: arm,pl353-smc: Extend to support 'arm,pl354' SMC

[Miquel Raynal]

Hi Rob,

robh@kernel.org wrote on Fri, 21 Oct 2022 15:39:28 -0500:

> Add support for the Arm PL354 static memory controller to the existing
> Arm PL353 binding. Both are different configurations of the same IP with
> support for different types of memory interfaces.
> 
> The 'arm,pl354' binding has already been in use upstream for a long time
> in Arm development boards. The existing users have only the controller
> without any child devices, so drop the required address properties
> (ranges, #address-cells, #size-cells). The schema for 'ranges' is too
> constrained as the order is not important and the PL354 has 8
> chipselects (And the PL353 actually has up to 8 too).

I'm not convinced the ranges constraint should be soften. For me
the order was important (and the description in the yaml useful, but
that's a personal opinion). What makes you think the ranges order is
not relevant on PL353?

Thanks,
Miquèl

> The clocks aren't really correct in either case. There's 1 bus clock and
> then a clock for each of the 2 memory interfaces.
> 
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  ...{arm,pl353-smc.yaml => arm,pl35x-smc.yaml} | 80 ++++++++++++-------
>  1 file changed, 53 insertions(+), 27 deletions(-)
>  rename Documentation/devicetree/bindings/memory-controllers/{arm,pl353-smc.yaml => arm,pl35x-smc.yaml} (65%)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml b/Documentation/devicetree/bindings/memory-controllers/arm,pl35x-smc.yaml
> similarity index 65%
> rename from Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml
> rename to Documentation/devicetree/bindings/memory-controllers/arm,pl35x-smc.yaml
> index 01c9acf9275d..bd23257fe021 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/arm,pl35x-smc.yaml
> @@ -1,26 +1,31 @@
>  # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/memory-controllers/arm,pl353-smc.yaml#
> +$id: http://devicetree.org/schemas/memory-controllers/arm,pl35x-smc.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: ARM PL353 Static Memory Controller (SMC) device-tree bindings
> +title: Arm PL35x Series Static Memory Controller (SMC)
>  
>  maintainers:
>    - Miquel Raynal <miquel.raynal@bootlin.com>
>    - Naga Sureshkumar Relli <naga.sureshkumar.relli@xilinx.com>
>  
> -description:
> -  The PL353 Static Memory Controller is a bus where you can connect two kinds
> +description: |
> +  The PL35x Static Memory Controller is a bus where you can connect two kinds
>    of memory interfaces, which are NAND and memory mapped interfaces (such as
> -  SRAM or NOR).
> +  SRAM or NOR) depending on the specific configuration.
> +
> +  The TRM is available here:
> +  https://documentation-service.arm.com/static/5e8e2524fd977155116a58aa
>  
>  # We need a select here so we don't match all nodes with 'arm,primecell'
>  select:
>    properties:
>      compatible:
>        contains:
> -        const: arm,pl353-smc-r2p1
> +        enum:
> +          - arm,pl353-smc-r2p1
> +          - arm,pl354
>    required:
>      - compatible
>  
> @@ -30,7 +35,9 @@ properties:
>  
>    compatible:
>      items:
> -      - const: arm,pl353-smc-r2p1
> +      - enum:
> +          - arm,pl353-smc-r2p1
> +          - arm,pl354
>        - const: arm,primecell
>  
>    "#address-cells":
> @@ -46,30 +53,25 @@ properties:
>            The three chip select regions are defined in 'ranges'.
>  
>    clocks:
> -    items:
> -      - description: clock for the memory device bus
> -      - description: main clock of the SMC
> +    minItems: 1
> +    maxItems: 2
>  
>    clock-names:
> -    items:
> -      - const: memclk
> -      - const: apb_pclk
> +    minItems: 1
> +    maxItems: 2
>  
>    ranges:
>      minItems: 1
> -    description: |
> -      Memory bus areas for interacting with the devices. Reflects
> -      the memory layout with four integer values following:
> -      <cs-number> 0 <offset> <size>
> -    items:
> -      - description: NAND bank 0
> -      - description: NOR/SRAM bank 0
> -      - description: NOR/SRAM bank 1
> +    maxItems: 8
>  
> -  interrupts: true
> +  interrupts:
> +    minItems: 1
> +    items:
> +      - description: Combined or Memory interface 0 IRQ
> +      - description: Memory interface 1 IRQ
>  
>  patternProperties:
> -  "@[0-3],[a-f0-9]+$":
> +  "@[0-7],[a-f0-9]+$":
>      type: object
>      description: |
>        The child device node represents the controller connected to the SMC
> @@ -87,7 +89,7 @@ patternProperties:
>                - description: |
>                    Chip-select ID, as in the parent range property.
>                  minimum: 0
> -                maximum: 2
> +                maximum: 7
>                - description: |
>                    Offset of the memory region requested by the device.
>                - description: |
> @@ -102,12 +104,36 @@ required:
>    - reg
>    - clock-names
>    - clocks
> -  - "#address-cells"
> -  - "#size-cells"
> -  - ranges
>  
>  additionalProperties: false
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: arm,pl354
> +    then:
> +      properties:
> +        clocks:
> +          # According to TRM, really should be 3 clocks
> +          maxItems: 1
> +
> +        clock-names:
> +          const: apb_pclk
> +
> +    else:
> +      properties:
> +        clocks:
> +          items:
> +            - description: clock for the memory device bus
> +            - description: main clock of the SMC
> +
> +        clock-names:
> +          items:
> +            - const: memclk
> +            - const: apb_pclk
> +
>  examples:
>    - |
>      smcc: memory-controller@e000e000 {

Re: [PATCH] dt-bindings: memory-controllers: arm,pl353-smc: Extend to support 'arm,pl354' SMC

[Rob Herring]

On Mon, Oct 24, 2022 at 3:14 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> robh@kernel.org wrote on Fri, 21 Oct 2022 15:39:28 -0500:
>
> > Add support for the Arm PL354 static memory controller to the existing
> > Arm PL353 binding. Both are different configurations of the same IP with
> > support for different types of memory interfaces.
> >
> > The 'arm,pl354' binding has already been in use upstream for a long time
> > in Arm development boards. The existing users have only the controller
> > without any child devices, so drop the required address properties
> > (ranges, #address-cells, #size-cells). The schema for 'ranges' is too
> > constrained as the order is not important and the PL354 has 8
> > chipselects (And the PL353 actually has up to 8 too).
>
> I'm not convinced the ranges constraint should be soften. For me
> the order was important (and the description in the yaml useful, but
> that's a personal opinion). What makes you think the ranges order is
> not relevant on PL353?

Address translation looks for a matching entry, so order doesn't
matter. However, we have seen cases in PCI hosts where the driver
populates registers based on the order of ranges. That's a driver
problem IMO. For PCI, it was multiple entries of the same type. For
this, we have the chip select number in the entry, so we shouldn't
have the same sort of problem. Except there is another issue that the
SRAM interface chipselects are numbered 1 and 2. The PL353 can have 4
NAND chipselects, I don't think the host addresses are necessarily in
order or contiguous either, so you could need 4 entries for NAND. The
existing description doesn't handle that, and the chipselects for the
SRAM interface should have been numbered 4-7. I don't mind saying the
entries should be in order by chipselect, but we can't define indices
of the entries as was done. It's all kind of academic because we don't
have any h/w needing anything else though the Arm boards would if the
child nodes actually got defined (not likely at this point).

Rob

Re: [PATCH] dt-bindings: memory-controllers: arm,pl353-smc: Extend to support 'arm,pl354' SMC

[Miquel Raynal]

Hi Rob,

robh@kernel.org wrote on Mon, 24 Oct 2022 09:31:41 -0500:

> On Mon, Oct 24, 2022 at 3:14 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Rob,
> >
> > robh@kernel.org wrote on Fri, 21 Oct 2022 15:39:28 -0500:
> >  
> > > Add support for the Arm PL354 static memory controller to the existing
> > > Arm PL353 binding. Both are different configurations of the same IP with
> > > support for different types of memory interfaces.
> > >
> > > The 'arm,pl354' binding has already been in use upstream for a long time
> > > in Arm development boards. The existing users have only the controller
> > > without any child devices, so drop the required address properties
> > > (ranges, #address-cells, #size-cells). The schema for 'ranges' is too
> > > constrained as the order is not important and the PL354 has 8
> > > chipselects (And the PL353 actually has up to 8 too).  
> >
> > I'm not convinced the ranges constraint should be soften. For me
> > the order was important (and the description in the yaml useful, but
> > that's a personal opinion). What makes you think the ranges order is
> > not relevant on PL353?  
> 
> Address translation looks for a matching entry, so order doesn't
> matter. However, we have seen cases in PCI hosts where the driver
> populates registers based on the order of ranges. That's a driver
> problem IMO. For PCI, it was multiple entries of the same type. For
> this, we have the chip select number in the entry, so we shouldn't
> have the same sort of problem. Except there is another issue that the
> SRAM interface chipselects are numbered 1 and 2. The PL353 can have 4
> NAND chipselects, I don't think the host addresses are necessarily in
> order or contiguous either, so you could need 4 entries for NAND. The
> existing description doesn't handle that, and the chipselects for the
> SRAM interface should have been numbered 4-7. I don't mind saying the
> entries should be in order by chipselect, but we can't define indices
> of the entries as was done. It's all kind of academic because we don't
> have any h/w needing anything else though the Arm boards would if the
> child nodes actually got defined (not likely at this point).

Alright, thanks for the feedback.

Cheers,
Miquèl

Re: [PATCH] dt-bindings: memory-controllers: arm,pl353-smc: Extend to support 'arm,pl354' SMC

[Krzysztof Kozlowski]

On Fri, 21 Oct 2022 15:39:28 -0500, Rob Herring wrote:
> Add support for the Arm PL354 static memory controller to the existing
> Arm PL353 binding. Both are different configurations of the same IP with
> support for different types of memory interfaces.
> 
> The 'arm,pl354' binding has already been in use upstream for a long time
> in Arm development boards. The existing users have only the controller
> without any child devices, so drop the required address properties
> (ranges, #address-cells, #size-cells). The schema for 'ranges' is too
> constrained as the order is not important and the PL354 has 8
> chipselects (And the PL353 actually has up to 8 too).
> 
> [...]

Applied, thanks!

[1/1] dt-bindings: memory-controllers: arm,pl353-smc: Extend to support 'arm,pl354' SMC
      https://git.kernel.org/krzk/linux-mem-ctrl/c/de67fa80c66992b13dd018ec18e8c91156522c18

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>