Re: [PATCH 02/10] dt-bindings: bus: add device tree bindings for RIFSC

[Gatien CHEVALLIER <gatien.chevallier@foss.st.com>]

Hello Krzysztof,

On 7/6/23 11:29, Gatien CHEVALLIER wrote:
> Hello Krzysztof,
> 
> Firstly, I will correct the bindings error pointed by Rob's robot.
> Obviously, I did not pass the bindings check the proper way or maybe I'm 
> running an old version.
> 
> On 7/6/23 08:28, Krzysztof Kozlowski wrote:
>> On 05/07/2023 19:27, Gatien Chevallier wrote:
>>> Document RIFSC (RIF security controller). RIFSC is a firewall controller
>>> composed of different kinds of hardware resources.
>>>
>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>
>> A nit, subject: drop second/last, redundant "device tree bindings for".
>> The "dt-bindings" prefix is already stating that these are bindings. 4
>> words of your 6 word subject is meaningless...
> 
> Ack, I will rephrase, it is indeed redundant
> 
>>
>>> ---
>>>   .../bindings/bus/st,stm32-rifsc.yaml          | 101 ++++++++++++++++++
>>>   1 file changed, 101 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml 
>>> b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>>> new file mode 100644
>>> index 000000000000..68d585ed369c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/st,stm32-rifsc.yaml
>>
>> Filename like compatible, unless you know list of compatibles will
>> grow... but then add them.
>>
>>> @@ -0,0 +1,101 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/bus/st,stm32-rifsc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: STM32 Resource isolation framework security controller bindings
>>
>> Drop bindings
> 
> Ack
> 
>>
>>> +
>>> +maintainers:
>>> +  - Gatien Chevallier <gatien.chevallier@foss.st.com>
>>> +
>>> +description: |
>>> +  Resource isolation framework (RIF) is a comprehensive set of 
>>> hardware blocks
>>> +  designed to enforce and manage isolation of STM32 hardware 
>>> resources like
>>> +  memory and peripherals.
>>> +
>>> +  The RIFSC (RIF security controller) is composed of three sets of 
>>> registers,
>>> +  each managing a specific set of hardware resources:
>>> +    - RISC registers associated with RISUP logic (resource isolation 
>>> device unit
>>> +      for peripherals), assign all non-RIF aware peripherals to 
>>> zero, one or
>>> +      any security domains (secure, privilege, compartment).
>>> +    - RIMC registers: associated with RIMU logic (resource isolation 
>>> master
>>> +      unit), assign all non RIF-aware bus master to one security 
>>> domain by
>>> +      setting secure, privileged and compartment information on the 
>>> system bus.
>>> +      Alternatively, the RISUP logic controlling the device port 
>>> access to a
>>> +      peripheral can assign target bus attributes to this peripheral 
>>> master port
>>> +      (supported attribute: CID).
>>> +    - RISC registers associated with RISAL logic (resource isolation 
>>> device unit
>>> +      for address space - Lite version), assign address space 
>>> subregions to one
>>> +      security domains (secure, privilege, compartment).
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: st,stm32mp25-rifsc
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  "#address-cells":
>>> +    const: 1
>>> +
>>> +  "#size-cells":
>>> +    const: 1
>>> +
>>> +  "#feature-domain-cells":
>>> +    const: 1
>>> +
>>> +  ranges: true
>>> +
>>> +  feature-domain-controller: true
>>> +
>>> +patternProperties:
>>> +  "^.*@[0-9a-f]+$":
>>> +    description: Peripherals
>>> +    type: object
>>> +    properties:
>>> +      feature-domains:
>>> +        minItems: 1
>>> +        maxItems: 2
>>> +        description:
>>> +          The first argument must always be a phandle that 
>>> references to the
>>> +          firewall controller of the peripheral. The second can 
>>> contain the
>>> +          platform specific firewall ID of the peripheral.
>>
>> It does not make much sense to me to have hierarchy parent-child and via
>> phandle at the same time. You express the similar relationship twice
> Thank you for pointing this out.
> 
> About the parent-child relation:
> 
> The bus-like device tree architecture allows a bus-probe mechanism with 
> which we want to check accesses of peripherals before probing their 
> driver. This has several advantages:
> -This bus architecture provides a clearer view of the hardware.
> -No peripheral driver modifications as it is fully handled by the 
> firewall drivers.
> -Drivers for devices that aren't accessible will not even be probed => 
> no probe fail.
> 
> It would be possible to manage this mechanism another way by handling 
> probe deferrals in drivers. But it would mean modifying every driver 
> with a check on ST firewall that we probe and some of them aren't from 
> STMicroelectronics.
> 
> About the phandle relation:
> 
> I agree on the fact that this double expression of the relationship is 
> redundant.
> 
> I've done it this way because there will be other nodes outside the 
> RIFSC node that will need to reference it as their feature-domain 
> controller. I kept the same information in the property to be coherent 
> between all.
> 
> For nodes under the RIFSC, the phandle is indeed useless and could be 
> removed, just to leave the firewall ID. And I'm inclined to do so. I 
> just have one worry on the YAML binding files where I will have a 
> pattern property in the RIFSC that will state something and maybe 
> another description in the peripheral YAML files. What is your take on 
> that?
> 

Looking back at it, feature-domains is a phandle-array. I guess I can't
derogate to the following architecture:

items:
   - items:
       - description: A phandle
       - description: 1st arg cell
       - description: 2nd arg cell

can I?

Some devices' nodes that are not subnodes of the firewall controllers
will need the phandle reference. Should I keep the redundant information
then?

Best regards,
Gatien

>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - "#address-cells"
>>> +  - "#size-cells"
>>> +  - feature-domain-controller
>>> +  - "#feature-domain-cells"
>>> +  - ranges
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    // In this example, the usart2 device refers to rifsc as its domain
>>> +    // controller.
>>> +    // Access rights are verified before creating devices.
>>> +
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +
>>> +    rifsc: rifsc-bus@42080000 {
>>> +        compatible = "st,stm32mp25-rifsc";
>>> +        reg = <0x42080000 0x1000>;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +        ranges;
>>> +        feature-domain-controller;
>>> +        #feature-domain-cells = <1>;
>>> +
>>> +        usart2: serial@400e0000 {
>>> +            compatible = "st,stm32h7-uart";
>>> +            reg = <0x400e0000 0x400>;
>>> +            interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
>>> +            clocks = <&ck_flexgen_08>;
>>> +            feature-domains = <&rifsc 32>;
>>> +            status = "disabled";
>>
>> No status in the examples.
>>
>>> +        };
>>> +    };
>>
>> Best regards,
>> Krzysztof
>>
> 
> Best regards,
> Gatien