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

[Rob Herring]

On Wed, Jun 9, 2021 at 8:34 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hi Krzysztof,
>
> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > ---
> > >  .../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@xxxxxxxxxxx>
> > > +  - Naga Sureshkumar Relli <naga.sureshkumar.relli@xxxxxxxxxx>
> > > +
> > > +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

Ah, required is there already. So only change is using 'const' for single entry.

> > > +
> > > +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

Yes, please.

> > 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.

I think it is worthwhile to state what region this is AND the chip
select regions are in 'ranges'. Without the latter part, I agree it
seems like a genericish description.

> >
> > > +
> > > +  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]+$"

+1