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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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 {