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:

quoted

On 05/07/2023 19:27, Gatien Chevallier wrote:

quoted

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

quoted

quoted

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

quoted

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

quoted

quoted

+
+maintainers:
+  - Gatien Chevallier [off-list ref]
+
+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

quoted

quoted

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

quoted

+        };
+    };

Best regards,
Krzysztof

Best regards,
Gatien