[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6edb1d1e-ae6b-486a-9548-4b2e0353f3dc@foss.st.com>
Date: Thu, 20 Jul 2023 16:58:35 +0200
From: Gatien CHEVALLIER <gatien.chevallier@...s.st.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
<Oleksii_Moisieiev@...m.com>, <gregkh@...uxfoundation.org>,
<herbert@...dor.apana.org.au>, <davem@...emloft.net>,
<robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
<conor+dt@...nel.org>, <alexandre.torgue@...s.st.com>,
<vkoul@...nel.org>, <jic23@...nel.org>,
<olivier.moysan@...s.st.com>, <arnaud.pouliquen@...s.st.com>,
<mchehab@...nel.org>, <fabrice.gasnier@...s.st.com>,
<andi.shyti@...nel.org>, <ulf.hansson@...aro.org>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<hugues.fruchet@...s.st.com>, <lee@...nel.org>, <will@...nel.org>,
<catalin.marinas@....com>, <arnd@...nel.org>,
<richardcochran@...il.com>
CC: <linux-crypto@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <dmaengine@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, <linux-iio@...r.kernel.org>,
<alsa-devel@...a-project.org>, <linux-media@...r.kernel.org>,
<linux-mmc@...r.kernel.org>, <netdev@...r.kernel.org>,
<linux-phy@...ts.infradead.org>, <linux-serial@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <linux-usb@...r.kernel.org>
Subject: Re: [PATCH 02/10] dt-bindings: bus: add device tree bindings for
RIFSC
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@...s.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@...s.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@...80000 {
>>> + compatible = "st,stm32mp25-rifsc";
>>> + reg = <0x42080000 0x1000>;
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> + ranges;
>>> + feature-domain-controller;
>>> + #feature-domain-cells = <1>;
>>> +
>>> + usart2: serial@...e0000 {
>>> + 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
Powered by blists - more mailing lists