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

[Krzysztof Kozlowski]

On 09/06/2021 15:34, Miquel Raynal 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.

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