diff mbox series

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

Message ID 20210609080112.1753221-10-miquel.raynal@bootlin.com
State New
Headers show
Series ARM Primecell PL35x support | expand

Commit Message

Miquel Raynal June 9, 2021, 8:01 a.m. UTC
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

Comments

Krzysztof Kozlowski June 9, 2021, 12:12 p.m. UTC | #1
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.

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

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.

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

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


> +    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
Miquel Raynal June 9, 2021, 1:34 p.m. UTC | #2
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
Krzysztof Kozlowski June 9, 2021, 1:54 p.m. UTC | #3
On 09/06/2021 15:34, Miquel Raynal wrote:
> 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.

Which seems to be result of unusual compatible match, so once you
convert to regular match, this select is not needed.

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

That's the common, easiest and actually expected.

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

The explanation of all four areas could have sense, but now it states
the obvious - these are configuration registers :)

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

I was just wondering... your explanation is fine to me, thanks!


Best regards,
Krzysztof
Miquel Raynal June 9, 2021, 2:11 p.m. UTC | #4
Hi Krzysztof, Rob,

Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote on Wed, 9
Jun 2021 15:54:19 +0200:

> On 09/06/2021 15:34, Miquel Raynal wrote:
> > 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.  
> 
> Which seems to be result of unusual compatible match, so once you
> convert to regular match, this select is not needed.

I don't think so, I received a hint from Rob some time ago, he told
me to add this additional select line as in all other arm,primecell
binding.

Rob, any additional info regarding this?


> >>> +
> >>> +  "#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.  
> 
> The explanation of all four areas could have sense, but now it states
> the obvious - these are configuration registers :)

Well, that's true :)

Thanks,
Miquèl
Krzysztof Kozlowski June 9, 2021, 3:26 p.m. UTC | #5
On 09/06/2021 16:11, Miquel Raynal wrote:
> Hi Krzysztof, Rob,
> 
> Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote on Wed, 9
> Jun 2021 15:54:19 +0200:
> 
>> On 09/06/2021 15:34, Miquel Raynal wrote:
>>> 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.  
>>
>> Which seems to be result of unusual compatible match, so once you
>> convert to regular match, this select is not needed.
> 
> I don't think so, I received a hint from Rob some time ago, he told
> me to add this additional select line as in all other arm,primecell
> binding.
> 
> Rob, any additional info regarding this?

Hmm, I think you' are right. Since arm,primecell is used in many other
compatibles (including ones without schema yet), the select is needed.

In such case the select can be only:

select:
  properties:
    compatible:
      contains:
        const: arm,pl353-smc-r2p1


Best regards,
Krzysztof
Rob Herring June 9, 2021, 4:16 p.m. UTC | #6
On Wed, 09 Jun 2021 10:01:03 +0200, 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
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/memory-controllers/arm,pl353-smc.example.dt.yaml:0:0: /example-0/memory-controller@e000e000/nand-controller@0,0: failed to match any schema with compatible: ['arm,pl353-nand-r2p1']
\ndoc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1489728

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.
Rob Herring June 9, 2021, 7:26 p.m. UTC | #7
On Wed, Jun 9, 2021 at 10:26 AM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 09/06/2021 16:11, Miquel Raynal wrote:
> > Hi Krzysztof, Rob,
> >
> > Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote on Wed, 9
> > Jun 2021 15:54:19 +0200:
> >
> >> On 09/06/2021 15:34, Miquel Raynal wrote:
> >>> 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.
> >>
> >> Which seems to be result of unusual compatible match, so once you
> >> convert to regular match, this select is not needed.
> >
> > I don't think so, I received a hint from Rob some time ago, he told
> > me to add this additional select line as in all other arm,primecell
> > binding.
> >
> > Rob, any additional info regarding this?
>
> Hmm, I think you' are right. Since arm,primecell is used in many other
> compatibles (including ones without schema yet), the select is needed.
>
> In such case the select can be only:
>
> select:
>   properties:
>     compatible:
>       contains:
>         const: arm,pl353-smc-r2p1

The above is true if there's no 'compatible'. So you need 'required: [
compatible ]' as well.

Rob
Rob Herring June 9, 2021, 7:35 p.m. UTC | #8
On Wed, Jun 9, 2021 at 8:34 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> 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

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
diff mbox series

Patch

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
+  required:
+    - compatible
+
+properties:
+  $nodename:
+    pattern: "^memory-controller@[0-9a-f]+$"
+
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - arm,pl353-smc-r2p1
+          - enum:
+              - arm,primecell
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 1
+
+  reg:
+    items:
+      - description: configuration registers for the host and sub-controllers
+
+  clocks:
+    items:
+      - description: the clock for the memory device bus
+      - description: the main clock of the controller
+
+  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]+$":
+    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.
+
+    properties:
+      compatible:
+        description:
+          Compatible of memory controller.
+
+      reg:
+        items:
+          - items:
+              - description: |
+                  Chip-select ID, as in the parent range property.
+                minimum: 0
+                maximum: 2
+              - description: |
+                  Offset of the memory region requested by the device.
+              - description: |
+                  Length of the memory region requested by the device.
+
+    required:
+      - compatible
+      - reg
+
+required:
+  - compatible
+  - reg
+  - clock-names
+  - clocks
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    smcc: memory-controller@e000e000 {
+      compatible = "arm,pl353-smc-r2p1", "arm,primecell";
+      reg = <0xe000e000 0x0001000>;
+      clock-names = "memclk", "apb_pclk";
+      clocks = <&clkc 11>, <&clkc 44>;
+      ranges = <0x0 0x0 0xe1000000 0x1000000 /* Nand CS region */
+                0x1 0x0 0xe2000000 0x2000000 /* SRAM/NOR CS0 region */
+                0x2 0x0 0xe4000000 0x2000000>; /* SRAM/NOR CS1 region */
+      #address-cells = <2>;
+      #size-cells = <1>;
+
+      nfc0: nand-controller@0,0 {
+        compatible = "arm,pl353-nand-r2p1";
+        reg = <0 0 0x1000000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/memory-controllers/pl353-smc.txt b/Documentation/devicetree/bindings/memory-controllers/pl353-smc.txt
deleted file mode 100644
index ba6a5426f62b..000000000000
--- a/Documentation/devicetree/bindings/memory-controllers/pl353-smc.txt
+++ /dev/null
@@ -1,45 +0,0 @@ 
-Device tree bindings for ARM PL353 static memory controller
-
-PL353 Static Memory Controller is a bus where you can connect two kinds
-of memory interfaces: NAND and memory mapped interfaces (such as SRAM or NOR).
-
-Required properties:
-- compatible		: Should be "arm,pl353-smc-r2p1", "arm,primecell".
-- reg			: SMC controller and sub-controllers configuration
-			  registers.
-- clock-names		: List of input clock names - "memclk", "apb_pclk"
-			  (See clock bindings for details).
-- clocks		: Clock phandles (see clock bindings for details).
-- address-cells		: Must be 2.
-- size-cells		: Must be 1.
-- ranges		: Memory bus areas for interacting with the devices.
-			  Encodes CS to memory region association.
-
-The child device node represents the controller connected to the SMC
-bus. Only one between: NAND controller, NOR controller and SRAM controller
-is allowed in a single system.
-
-Required device node properties:
-
-- reg:			Contains the chip-select id, the offset and the length
-			of the memory region requested by the device.
-
-Example:
-	smcc: memory-controller@e000e000 {
-		compatible = "arm,pl353-smc-r2p1", "arm,primecell";
-		clock-names = "memclk", "apb_pclk";
-		clocks = <&clkc 11>, <&clkc 44>;
-		reg = <0xe000e000 0x1000>;
-		#address-cells = <2>;
-		#size-cells = <1>;
-		ranges = <0x0 0x0 0xe1000000 0x1000000 /* Nand CS region */
-			  0x1 0x0 0xe2000000 0x2000000 /* SRAM/NOR CS0 region */
-			  0x2 0x0 0xe4000000 0x2000000>; /* SRAM/NOR CS1 region */
-
-		nfc0: nand-controller@0,0 {
-			compatible = "arm,pl353-nand-r2p1";
-			reg = <0 0 0x1000000>;
-			#address-cells = <1>;
-			#size-cells = <0>;
-		};
-	};