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

Krzysztof Kozlowski
2021-06-09T13:54:26Z

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