Message-ID: <f785a9f9-a931-a4b0-5d97-d9e8ce24065a@foss.st.com>
Date:   Thu, 22 Dec 2022 14:39:28 +0100
From:   Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>,
        <alexandre.torgue@...s.st.com>, <robh+dt@...nel.org>,
        <Oleksii_Moisieiev@...m.com>, <linus.walleij@...aro.org>,
        <gregkh@...uxfoundation.org>
CC:     <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <loic.pallardy@...com>,
        <devicetree@...r.kernel.org>, <mark.rutland@....com>,
        <arnd@...db.de>
Subject: Re: [RFC PATCH 2/7] dt-bindings: bus: add STM32 System Bus

Hello,

On 12/22/22 11:24, Krzysztof Kozlowski wrote:
> On 21/12/2022 18:30, Gatien Chevallier wrote:
>> Document STM32 System Bus. This bus is intended to control firewall
>> access for the peripherals connected to it.
>>
>> Signed-off-by: Loic PALLARDY <loic.pallardy@...com>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@...s.st.com>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel
As it is based on Oleksii's patchset and older threads:
[1]: 
https://lore.kernel.org/all/20190318100605.29120-1-benjamin.gaignard@st.com/
[2]: 
https://lore.kernel.org/all/20200701132523.32533-1-benjamin.gaignard@st.com/

I wanted to include people that have already been included or 
participated in these.

I'm sorry I did miss/added some (extra) people. I will double-check for 
next patchset version.

> 
>> ---
>>   .../devicetree/bindings/bus/st,sys-bus.yaml   | 88 +++++++++++++++++++
>>   1 file changed, 88 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/bus/st,sys-bus.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/bus/st,sys-bus.yaml b/Documentation/devicetree/bindings/bus/st,sys-bus.yaml
>> new file mode 100644
>> index 000000000000..9c0e86612695
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/bus/st,sys-bus.yaml
>> @@ -0,0 +1,88 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/bus/stm32,sys-bus.yaml
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title:  STM32 System Bus
> 
> Only one space.
> 

Ack. I already pushed a V2, that is now outdated with your review, where 
this error is fixed.

>> +
>> +description: |
>> +  The STM32 System Bus is an internal bus to which some internal peripherals
>> +  are connected. STM32 System Bus integrates a firewall controlling access to each
>> +  device. This bus prevents non-accessible devices to be probed.
>> +
>> +  To see which peripherals are securable, please check the SoC reference manual.
>> +
>> +maintainers:
>> +  - Gatien Chevallier <gatien.chevallier@...s.st.com>
>> +
>> +allOf:
>> +  - $ref: /schemas/feature-controllers/feature-domain-controller.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - st,stm32mp13-sys-bus
>> +      - st,stm32mp15-sys-bus
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 1
>> +
>> +  '#feature-domain-cells':
> 
> Use consistent quotes, either ' or "

Ack, will change in V3.

> 
>> +    minItems: 1
> 
> No. Cells must be const. This does not match cells at all...
> 

Ack, will change to const in V3. What do imply by saying it does not 
match? Note that I've changed it to "minimum" in V2.

>> +
>> +  ranges: true
>> +
>> +  feature-domain-controller: true
>> +
>> +patternProperties:
>> +  "^.*@[0-9a-f]+$":
>> +    description: Devices attached to system bus
>> +    type: object
>> +    properties:
>> +      feature-domains:
>> +        $ref: /schemas/feature-controllers/feature-domain-controller.yaml#/properties/feature-domains
> 
> maxItems

I don't think setting a max here is relevant as there can be numerous 
feature-domains referenced.

Maybe a min?

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +  - feature-domain-controller
>> +  - '#feature-domain-cells'
>> +  - ranges
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    // In this example,
>> +    // - the foo1 device refers to etzpc as his domain controller.
>> +    // - same goes for foo2.
>> +    // Access rights are verified before creating devices.
>> +
>> +    etzpc: etzpc@...07000 {
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 

Ack, will change to "etzpc: bus@...07000" in V3

>> +        compatible = "st,stm32mp15-sys-bus";
>> +        reg = <0x5c007000 0x400>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges;
>> +        feature-domain-controller;
>> +        #feature-domain-cells = <1>;
>> +
>> +        foo1: foo@...0000 {
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Here, if I use real peripherals, I have an issue with the dependency to 
YAML files. The feature-domains property is not defined in their 
bindings. Therefore, the dt_binding_check fails on peripherals whose 
YAML declare "additionalProperties: false" because the link to the 
feature domain controller bindings does not exist.

What would be your recommandation here as declaring:

patternProperties:
   "^.*@[0-9a-f]+$":
     description: Devices attached to system bus
     type: object
     properties:
       feature-domains:
         $ref: 
/schemas/feature-controllers/feature-domain-controller.yaml#/properties/feature-domains

does not solve the issue?



> 
>> +          reg = <0x0 0x1000000>;
> 
> Missing compatible, missing proper device name. Don't use fake names,
> but describe real case.

Linked to above issue.

> 
>> +          feature-domains = <&etzpc 0>;
>> +        };
>> +
>> +        foo2: foo@...0000 {
>> +          reg = <0x0 0x2000000>;
>> +          feature-domains = <&etzpc 0>;
>> +        };
>> +    };
> 
> Best regards,
> Krzysztof
> 

Best regards,
Gatien

Powered by blists - more mailing lists