Re: [PATCH v3 1/5] dt-bindings: bus: add documentation for the IMX AIPSTZ bridge

On Fri, Mar 28, 2025 at 02:34:11PM +0200, Mihalcea Laurentiu wrote:

On 25.03.2025 05:23, Rob Herring wrote:

quoted

On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote:

quoted

From: Laurentiu Mihalcea <redacted>

Add documentation for IMX AIPSTZ bridge.

Co-developed-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
Signed-off-by: Laurentiu Mihalcea <redacted>
---
 .../bindings/bus/fsl,imx8mp-aipstz.yaml       | 107 ++++++++++++++++++
 1 file changed, 107 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml

diff --git a/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml
new file mode 100644
index 000000000000..c0427dfcdaca
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/fsl,imx8mp-aipstz.yaml

@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bus/fsl,imx8mp-aipstz.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Secure AHB to IP Slave bus (AIPSTZ) bridge
+
+description:
+  The secure AIPS bridge (AIPSTZ) acts as a bridge for AHB masters
+  issuing transactions to IP Slave peripherals. Additionally, this module
+  offers access control configurations meant to restrict which peripherals
+  a master can access.

Wrap at 80 chars.

fix in v4, thx

quoted

quoted

+
+maintainers:
+  - Laurentiu Mihalcea [off-list ref]
+
+properties:
+  compatible:
+    const: fsl,imx8mp-aipstz
+
+  reg:
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: bus
+      - const: ac
+
+  power-domains:
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 1
+
+  "#access-controller-cells":
+    const: 0

With 0 cells, how do you identify which device it is?

we don't atm. We're relying on the default configuration.

Then you don't really need the property at all. However, if you ever 
need the non-default and configure it, adding it or changing it later is 
an ABI issue. So better to define it correctly sooner rather than later.

we don't have any APIs for AC configuration so I left the

cell number to 0 thinking that the cell number might depend

on the API.


if need be, I can set it to the value I was initially thinking of in v4.

quoted

quoted

+
+  ranges: true
+
+# borrowed from simple-bus.yaml, no additional requirements for children
+patternProperties:
+  "@(0|[1-9a-f][0-9a-f]*)$":
+    type: object
+    additionalProperties: true
+    properties:
+      reg:
+        items:
+          minItems: 2
+          maxItems: 4
+        minItems: 1
+        maxItems: 1024
+      ranges:
+        oneOf:
+          - items:
+              minItems: 3
+              maxItems: 7
+            minItems: 1
+            maxItems: 1024
+          - $ref: /schemas/types.yaml#/definitions/flag
+    anyOf:
+      - required:
+          - reg
+      - required:
+          - ranges
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - power-domains
+  - "#address-cells"
+  - "#size-cells"
+  - "#access-controller-cells"
+  - ranges
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8mp-clock.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    bus@30c00000 {
+        compatible = "fsl,imx8mp-aipstz";
+        reg = <0x30c00000 0x400000>, <0x30df0000 0x10000>;

It doesn't look like you have any registers in the 1st entry, but they 
are child devices? Then you should use ranges and drop it here:

ranges = <0x0 0x30c00000 0x400000>;

I guess this would mean switching from global addresses (current way) to

bus-relative addresses for the child devices. This wasn't my intent.


I wonder if we could just switch to V2 in which we just use the bridge's AC

configuration space and an empty 'ranges':


aips5: bus@30df0000 {

    compatible = "fsl,imx8mp-aipstz";

    reg = <0x30df0000 0x10000>;

    /* some more properties here */

    ranges;

};


or as Marco just suggested use


ranges = <0x30c00000 0x30c00000 0x400000>;


instead of an empty 'ranges' to restrict the bus size.


Personally, I'm fine with both approaches but what's your opinion on this?

The latter restricting the size is my preference.

Rob