Message ID | 20230705172759.1610753-1-gatien.chevallier@foss.st.com |
---|---|
Headers | show |
Series | Introduce STM32 Firewall framework | expand |
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