Re: [PATCH v22 09/18] dt-binding: memory: pl353-smc: Convert to yaml

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

Hi Krzysztof,

Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote on Wed, 9
Jun 2021 14:12:40 +0200:

> On 09/06/2021 10:01, Miquel Raynal wrote:
> > Convert this binding file to yaml schema.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../memory-controllers/arm,pl353-smc.yaml     | 133 ++++++++++++++++++
> >  .../bindings/memory-controllers/pl353-smc.txt |  45 ------
> >  2 files changed, 133 insertions(+), 45 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml
> >  delete mode 100644 Documentation/devicetree/bindings/memory-controllers/pl353-smc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml b/Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml
> > new file mode 100644
> > index 000000000000..1de6f87d4986
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.yaml
> > @@ -0,0 +1,133 @@
> > +# 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#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM PL353 Static Memory Controller (SMC) device-tree bindings
> > +
> > +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
> > +  of memory interfaces, which are NAND and memory mapped interfaces (such as
> > +  SRAM or NOR).
> > +
> > +# We need a select here so we don't match all nodes with 'arm,primecell'
> > +select:
> > +  properties:
> > +    compatible:
> > +      contains:
> > +        enum:
> > +          - arm,pl353-smc-r2p1  
> 
> That's a const... but also I don't get the need for select.

I think this is needed to ensure this binding is not enforced against
arm,primecell compatible nodes which are not featuring the
arm,pl353-smc-r2p1 compatible.

> 
> > +  required:
> > +    - compatible
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^memory-controller@[0-9a-f]+$"
> > +
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - arm,pl353-smc-r2p1
> > +          - enum:
> > +              - arm,primecell  
> 
> This looks unusual. Basically you change the bindings, because before
> they required "arm,pl353-smc-r2p1", "arm,primecell".

That is precisely what I try to match and I think it works. Perhaps
this version is easier to extend when a new compatible comes in.
However, I am fine using an alternative formula, like below if you
think it's better:

compatible:
  items:
    - const: arm,pl353-smc-r2p1
    - const: arm,primecell

> Don't you want here items:
>  - const: ...
>  - const: ...
> ?
> 
> > +
> > +  "#address-cells":
> > +    const: 2
> > +
> > +  "#size-cells":
> > +    const: 1
> > +
> > +  reg:
> > +    items:
> > +      - description: configuration registers for the host and sub-controllers  
> 
> Just maxItems. Description is obvious.

I don't think it is that obvious because there are actually 4 areas
and, because of the yaml language, we only describe one in the reg
property while the others and defined in the ranges property, but
that's fine by me, I'll drop the description and stick to a
maxItems entry.

> 
> > +
> > +  clocks:
> > +    items:
> > +      - description: the clock for the memory device bus
> > +      - description: the main clock of the controller  
> 
> Isn't apb_pclk the bus clock (so second item below)?

The SMC has two clock domains referred as aclk and mclk. In the TRM,
aclk is described as "Clock for the AXI domain". The AXI interface is
used to trigger CMD/ADDR/DATA cycles on the memory bus. There is also
an APB interface used to reach the SMC registers. I *think* that
both APB and AXI domains are fed by the same apb_pclk source but I
might be wrong. Hence memclk would just feed the memory bus that bonds
the memory device (eg. the NAND flash) to the host controller.

This is my current understanding but if you think it works differently
I'm all ears because this part is not 100% clear to me.

> > +
> > +  clock-names:
> > +    items:
> > +      - const: memclk
> > +      - const: apb_pclk  
> 
> 
> > +
> > +  ranges:
> > +    minItems: 1
> > +    maxItems: 3
> > +    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
> > +
> > +  interrupts: true
> > +
> > +patternProperties:
> > +  ".*@[0-9]+,[0-9]+$":  
> 
> Match with start ^. I think you cannot have 9 nodes and hex can appear
> in address so maybe:
> "^.*@[0-3],[a-f0-9]+$":

I think Rob even now prefers to drop the ^.* prefix, but you're right on
the two other points so I'll stick to:

  "@[0-3],[a-f0-9]+$"

> 
> 
> > +    type: object
> > +    description: |
> > +      The child device node represents the controller connected to the SMC
> > +      bus. The controller can be a NAND controller or a pair of any memory
> > +      mapped controllers such as NOR and SRAM controllers.
> > +  
> 
> Best regards,
> Krzysztof

Thanks,
Miquèl