diff mbox series

[v5,01/11] dt-bindings: document generic access controller

Message ID 20230929142852.578394-2-gatien.chevallier@foss.st.com (mailing list archive)
State Superseded
Headers show
Series Introduce STM32 Firewall framework | expand

Commit Message

Gatien CHEVALLIER Sept. 29, 2023, 2:28 p.m. UTC
From: Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>

Introducing of the generic access controller bindings for the
access controller provider and consumer devices. Those bindings are
intended to allow a better handling of accesses to resources in a
hardware architecture supporting several compartments.

This patch is based on [1]. It is integrated in this patchset as it
provides a use-case for it.

Diffs with [1]:
	- Rename feature-domain* properties to access-control* to narrow
	  down the scope of the binding
	- YAML errors and typos corrected.
	- Example updated
	- Some rephrasing in the binding description

[1]: https://lore.kernel.org/lkml/0c0a82bb-18ae-d057-562b

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>

---
Changes in V5:
	- Diffs with [1]
	- Discarded the [IGNORE] tag as the patch is now part of the
	  patchset

 .../access-controllers/access-controller.yaml | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/access-controllers/access-controller.yaml

Comments

Rob Herring Sept. 29, 2023, 3:35 p.m. UTC | #1
On Fri, 29 Sep 2023 16:28:42 +0200, Gatien Chevallier wrote:
> From: Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>
> 
> Introducing of the generic access controller bindings for the
> access controller provider and consumer devices. Those bindings are
> intended to allow a better handling of accesses to resources in a
> hardware architecture supporting several compartments.
> 
> This patch is based on [1]. It is integrated in this patchset as it
> provides a use-case for it.
> 
> Diffs with [1]:
> 	- Rename feature-domain* properties to access-control* to narrow
> 	  down the scope of the binding
> 	- YAML errors and typos corrected.
> 	- Example updated
> 	- Some rephrasing in the binding description
> 
> [1]: https://lore.kernel.org/lkml/0c0a82bb-18ae-d057-562b
> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> 
> ---
> Changes in V5:
> 	- Diffs with [1]
> 	- Discarded the [IGNORE] tag as the patch is now part of the
> 	  patchset
> 
>  .../access-controllers/access-controller.yaml | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/access-controllers/access-controller.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/access-controllers/access-controller.yaml: access-control-provider: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230929142852.578394-2-gatien.chevallier@foss.st.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Gatien CHEVALLIER Oct. 2, 2023, 9:10 a.m. UTC | #2
On 9/29/23 17:35, Rob Herring wrote:
> 
> On Fri, 29 Sep 2023 16:28:42 +0200, Gatien Chevallier wrote:
>> From: Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>
>>
>> Introducing of the generic access controller bindings for the
>> access controller provider and consumer devices. Those bindings are
>> intended to allow a better handling of accesses to resources in a
>> hardware architecture supporting several compartments.
>>
>> This patch is based on [1]. It is integrated in this patchset as it
>> provides a use-case for it.
>>
>> Diffs with [1]:
>> 	- Rename feature-domain* properties to access-control* to narrow
>> 	  down the scope of the binding
>> 	- YAML errors and typos corrected.
>> 	- Example updated
>> 	- Some rephrasing in the binding description
>>
>> [1]: https://lore.kernel.org/lkml/0c0a82bb-18ae-d057-562b
>>
>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>
>> ---
>> Changes in V5:
>> 	- Diffs with [1]
>> 	- Discarded the [IGNORE] tag as the patch is now part of the
>> 	  patchset
>>
>>   .../access-controllers/access-controller.yaml | 90 +++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/access-controllers/access-controller.yaml
>>
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/access-controllers/access-controller.yaml: access-control-provider: missing type definition
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230929142852.578394-2-gatien.chevallier@foss.st.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

Hi Rob,

Running:
1- make dt_binding_check | grep access-control
2- make dt_binding_check 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/access-controllers/access-controller.yaml
from Krzysztof's slideset

with

pip3 show dtschema
Name: dtschema
Version: 2023.9

and

pip3 show yamllint
Name: yamllint
Version: 1.32.0

I don't see any of the errors reported by the robot. I have to clone
your repository to reproduce it.

Should I resubmit with a clean dt-check using the latest dtschema?

***********
However, I get:
warning: ignoring duplicate '$id' value 
'http://devicetree.org/schemas/reserved-memory/framebuffer.yaml#
warning: ignoring duplicate '$id' value 
'http://devicetree.org/schemas/reserved-memory/memory-region.yaml#
warning: ignoring duplicate '$id' value 
'http://devicetree.org/schemas/reserved-memory/shared-dma-pool.yaml#
warning: ignoring duplicate '$id' value 
'http://devicetree.org/schemas/reserved-memory/reserved-memory.yaml

Above warnings disappears when switching to:
pip3 show dtschema
Name: dtschema
Version: 2023.7

The above YAMLs seem to be duplicated in dtschema's latest version.
I guess it's a synchro that needs to be done since:
https://lore.kernel.org/all/20230830231758.2561402-2-sjg@chromium.org/
***********

Best regards,
Gatien
Rob Herring Oct. 2, 2023, 5:30 p.m. UTC | #3
On Fri, Sep 29, 2023 at 04:28:42PM +0200, Gatien Chevallier wrote:
> From: Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>
> 
> Introducing of the generic access controller bindings for the
> access controller provider and consumer devices. Those bindings are
> intended to allow a better handling of accesses to resources in a
> hardware architecture supporting several compartments.
> 
> This patch is based on [1]. It is integrated in this patchset as it
> provides a use-case for it.
> 
> Diffs with [1]:
> 	- Rename feature-domain* properties to access-control* to narrow
> 	  down the scope of the binding
> 	- YAML errors and typos corrected.
> 	- Example updated
> 	- Some rephrasing in the binding description
> 
> [1]: https://lore.kernel.org/lkml/0c0a82bb-18ae-d057-562b
> 
> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> 
> ---
> Changes in V5:
> 	- Diffs with [1]
> 	- Discarded the [IGNORE] tag as the patch is now part of the
> 	  patchset
> 
>  .../access-controllers/access-controller.yaml | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/access-controllers/access-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/access-controllers/access-controller.yaml b/Documentation/devicetree/bindings/access-controllers/access-controller.yaml
> new file mode 100644
> index 000000000000..9d305fccc333
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/access-controllers/access-controller.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/access-controllers/access-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic Domain Access Controller
> +
> +maintainers:
> +  - Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> +
> +description: |+
> +  Common access controllers properties
> +
> +  Access controllers are in charge of stating which of the hardware blocks under
> +  their responsibility (their domain) can be accesssed by which compartment. A
> +  compartment can be a cluster of CPUs (or coprocessors), a range of addresses
> +  or a group of hardware blocks. An access controller's domain is the set of
> +  resources covered by the access controller.
> +
> +  This device tree bindings can be used to bind devices to their access
> +  controller provided by access-controller property. In this case, the device is
> +  a consumer and the access controller is the provider.
> +
> +  An access controller can be represented by any node in the device tree and
> +  can provide one or more configuration parameters, needed to control parameters
> +  of the consumer device. A consumer node can refer to the provider by phandle
> +  and a set of phandle arguments, specified by '#access-controller-cells'
> +  property in the access controller node.
> +
> +  Access controllers are typically used to set/read the permissions of a
> +  hardware block and grant access to it. Any of which depends on the access
> +  controller. The capabilities of each access controller are defined by the
> +  binding of the access controller device.
> +
> +  Each node can be a consumer for the several access controllers.
> +
> +# always select the core schema
> +select: true
> +
> +properties:
> +  "#access-controller-cells":
> +    $ref: /schemas/types.yaml#/definitions/uint32

Drop. "#.*-cells" already defines the type.

> +    description:
> +      Number of cells in a access-controller specifier;
> +      Can be any value as specified by device tree binding documentation
> +      of a particular provider.
> +
> +  access-control-provider:
> +    description:
> +      Indicates that the node is an access controller.

Drop. The presence of "#access-controller-cells" is enough to do that.

> +
> +  access-controller-names:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description:
> +      A list of access-controller names, sorted in the same order as
> +      access-controller entries. Consumer drivers will use
> +      access-controller-names to match with existing access-controller entries.
> +
> +  access-controller:

For consistency with other provider bindings: access-controllers

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      A list of access controller specifiers, as defined by the
> +      bindings of the access-controller provider.
> +
> +additionalProperties: true
> +
> +examples:
> +  - |
> +    uart_controller: access-controller@50000 {
> +        reg = <0x50000 0x10>;
> +        access-control-provider;
> +        #access-controller-cells = <2>;
> +    };
> +
> +    bus_controller: bus@60000 {
> +        reg = <0x60000 0x10000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        ranges;
> +        access-control-provider;
> +        #access-controller-cells = <3>;
> +
> +        uart4: serial@60100 {
> +            reg = <0x60100 0x400>;
> +            access-controller = <&uart_controller 1 2>,
> +                                <&bus_controller 1 3 5>;
> +            access-controller-names = "controller", "bus-controller";

Not great names. It should indicate what access is being controlled 
locally. Perhaps "reg" for register access, "dma" or "bus" for bus 
master access. (Not sure what your uart_controller is controlling access 
to.)

Rob
Gatien CHEVALLIER Oct. 3, 2023, 7:45 a.m. UTC | #4
Hi Rob,

On 10/2/23 19:30, Rob Herring wrote:
> On Fri, Sep 29, 2023 at 04:28:42PM +0200, Gatien Chevallier wrote:
>> From: Oleksii Moisieiev <Oleksii_Moisieiev@epam.com>
>>
>> Introducing of the generic access controller bindings for the
>> access controller provider and consumer devices. Those bindings are
>> intended to allow a better handling of accesses to resources in a
>> hardware architecture supporting several compartments.
>>
>> This patch is based on [1]. It is integrated in this patchset as it
>> provides a use-case for it.
>>
>> Diffs with [1]:
>> 	- Rename feature-domain* properties to access-control* to narrow
>> 	  down the scope of the binding
>> 	- YAML errors and typos corrected.
>> 	- Example updated
>> 	- Some rephrasing in the binding description
>>
>> [1]: https://lore.kernel.org/lkml/0c0a82bb-18ae-d057-562b
>>
>> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>
>> ---
>> Changes in V5:
>> 	- Diffs with [1]
>> 	- Discarded the [IGNORE] tag as the patch is now part of the
>> 	  patchset
>>
>>   .../access-controllers/access-controller.yaml | 90 +++++++++++++++++++
>>   1 file changed, 90 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/access-controllers/access-controller.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/access-controllers/access-controller.yaml b/Documentation/devicetree/bindings/access-controllers/access-controller.yaml
>> new file mode 100644
>> index 000000000000..9d305fccc333
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/access-controllers/access-controller.yaml
>> @@ -0,0 +1,90 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/access-controllers/access-controller.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Generic Domain Access Controller
>> +
>> +maintainers:
>> +  - Oleksii Moisieiev <oleksii_moisieiev@epam.com>
>> +
>> +description: |+
>> +  Common access controllers properties
>> +
>> +  Access controllers are in charge of stating which of the hardware blocks under
>> +  their responsibility (their domain) can be accesssed by which compartment. A
>> +  compartment can be a cluster of CPUs (or coprocessors), a range of addresses
>> +  or a group of hardware blocks. An access controller's domain is the set of
>> +  resources covered by the access controller.
>> +
>> +  This device tree bindings can be used to bind devices to their access
>> +  controller provided by access-controller property. In this case, the device is
>> +  a consumer and the access controller is the provider.
>> +
>> +  An access controller can be represented by any node in the device tree and
>> +  can provide one or more configuration parameters, needed to control parameters
>> +  of the consumer device. A consumer node can refer to the provider by phandle
>> +  and a set of phandle arguments, specified by '#access-controller-cells'
>> +  property in the access controller node.
>> +
>> +  Access controllers are typically used to set/read the permissions of a
>> +  hardware block and grant access to it. Any of which depends on the access
>> +  controller. The capabilities of each access controller are defined by the
>> +  binding of the access controller device.
>> +
>> +  Each node can be a consumer for the several access controllers.
>> +
>> +# always select the core schema
>> +select: true
>> +
>> +properties:
>> +  "#access-controller-cells":
>> +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> Drop. "#.*-cells" already defines the type.
> 

Ok, I will drop it for V6

>> +    description:
>> +      Number of cells in a access-controller specifier;
>> +      Can be any value as specified by device tree binding documentation
>> +      of a particular provider.
>> +
>> +  access-control-provider:
>> +    description:
>> +      Indicates that the node is an access controller.
> 
> Drop. The presence of "#access-controller-cells" is enough to do that.
> 

Ok, I wasn't sure. I'll will drop it for V6

>> +
>> +  access-controller-names:
>> +    $ref: /schemas/types.yaml#/definitions/string-array
>> +    description:
>> +      A list of access-controller names, sorted in the same order as
>> +      access-controller entries. Consumer drivers will use
>> +      access-controller-names to match with existing access-controller entries.
>> +
>> +  access-controller:
> 
> For consistency with other provider bindings: access-controllers
> 

Ack

>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      A list of access controller specifiers, as defined by the
>> +      bindings of the access-controller provider.
>> +
>> +additionalProperties: true
>> +
>> +examples:
>> +  - |
>> +    uart_controller: access-controller@50000 {
>> +        reg = <0x50000 0x10>;
>> +        access-control-provider;
>> +        #access-controller-cells = <2>;
>> +    };
>> +
>> +    bus_controller: bus@60000 {
>> +        reg = <0x60000 0x10000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +        ranges;
>> +        access-control-provider;
>> +        #access-controller-cells = <3>;
>> +
>> +        uart4: serial@60100 {
>> +            reg = <0x60100 0x400>;
>> +            access-controller = <&uart_controller 1 2>,
>> +                                <&bus_controller 1 3 5>;
>> +            access-controller-names = "controller", "bus-controller";
> 
> Not great names. It should indicate what access is being controlled
> locally. Perhaps "reg" for register access, "dma" or "bus" for bus
> master access. (Not sure what your uart_controller is controlling access
> to.)
> 
> Rob

Yes, I agree it's poor naming. I'll come up with something more
adequate. Thank you for the input.

Best regards,
Gatien
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/access-controllers/access-controller.yaml b/Documentation/devicetree/bindings/access-controllers/access-controller.yaml
new file mode 100644
index 000000000000..9d305fccc333
--- /dev/null
+++ b/Documentation/devicetree/bindings/access-controllers/access-controller.yaml
@@ -0,0 +1,90 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/access-controllers/access-controller.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic Domain Access Controller
+
+maintainers:
+  - Oleksii Moisieiev <oleksii_moisieiev@epam.com>
+
+description: |+
+  Common access controllers properties
+
+  Access controllers are in charge of stating which of the hardware blocks under
+  their responsibility (their domain) can be accesssed by which compartment. A
+  compartment can be a cluster of CPUs (or coprocessors), a range of addresses
+  or a group of hardware blocks. An access controller's domain is the set of
+  resources covered by the access controller.
+
+  This device tree bindings can be used to bind devices to their access
+  controller provided by access-controller property. In this case, the device is
+  a consumer and the access controller is the provider.
+
+  An access controller can be represented by any node in the device tree and
+  can provide one or more configuration parameters, needed to control parameters
+  of the consumer device. A consumer node can refer to the provider by phandle
+  and a set of phandle arguments, specified by '#access-controller-cells'
+  property in the access controller node.
+
+  Access controllers are typically used to set/read the permissions of a
+  hardware block and grant access to it. Any of which depends on the access
+  controller. The capabilities of each access controller are defined by the
+  binding of the access controller device.
+
+  Each node can be a consumer for the several access controllers.
+
+# always select the core schema
+select: true
+
+properties:
+  "#access-controller-cells":
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Number of cells in a access-controller specifier;
+      Can be any value as specified by device tree binding documentation
+      of a particular provider.
+
+  access-control-provider:
+    description:
+      Indicates that the node is an access controller.
+
+  access-controller-names:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description:
+      A list of access-controller names, sorted in the same order as
+      access-controller entries. Consumer drivers will use
+      access-controller-names to match with existing access-controller entries.
+
+  access-controller:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      A list of access controller specifiers, as defined by the
+      bindings of the access-controller provider.
+
+additionalProperties: true
+
+examples:
+  - |
+    uart_controller: access-controller@50000 {
+        reg = <0x50000 0x10>;
+        access-control-provider;
+        #access-controller-cells = <2>;
+    };
+
+    bus_controller: bus@60000 {
+        reg = <0x60000 0x10000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+        ranges;
+        access-control-provider;
+        #access-controller-cells = <3>;
+
+        uart4: serial@60100 {
+            reg = <0x60100 0x400>;
+            access-controller = <&uart_controller 1 2>,
+                                <&bus_controller 1 3 5>;
+            access-controller-names = "controller", "bus-controller";
+        };
+    };