| Message ID | 20250324162556.30972-2-laurentiumihalcea111@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | imx8mp: add support for the IMX AIPSTZ bridge | expand |
On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote: > From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > 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 <laurentiu.mihalcea@nxp.com> > --- > .../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. > + > +maintainers: > + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > + > +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? > + > + 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>; > + reg-names = "bus", "ac"; > + power-domains = <&pgc_audio>; > + #address-cells = <1>; > + #size-cells = <1>; > + #access-controller-cells = <0>; > + ranges; > + > + dma-controller@30e00000 { > + compatible = "fsl,imx8mp-sdma", "fsl,imx8mq-sdma"; > + reg = <0x30e00000 0x10000>; > + #dma-cells = <3>; > + clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SDMA3_ROOT>, > + <&clk IMX8MP_CLK_AUDIO_ROOT>; > + clock-names = "ipg", "ahb"; > + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>; > + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin"; No 'access-controllers' here? > + }; > + }; > -- > 2.34.1 >
Hi Laurentiu, On 25-03-24, Rob Herring wrote: > On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote: > > From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > > > 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 <laurentiu.mihalcea@nxp.com> > > --- > > .../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. > > > + > > +maintainers: > > + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > + > > +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? > > > + > > + 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>; > > > > + reg-names = "bus", "ac"; Thanks for picking up my suggestion :) IMHO it does look more logical now. I wasn't aware of the 'ranges' property else I would have suggested you to use this property instead of having two regs, sorry. Once you changed it to ranges we can drop the 'reg-names' as well since you only need to supply the 'ac' register space. Regards, Marco > > + power-domains = <&pgc_audio>; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + #access-controller-cells = <0>; > > + ranges; > > + > > + dma-controller@30e00000 { > > + compatible = "fsl,imx8mp-sdma", "fsl,imx8mq-sdma"; > > + reg = <0x30e00000 0x10000>; > > + #dma-cells = <3>; > > + clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SDMA3_ROOT>, > > + <&clk IMX8MP_CLK_AUDIO_ROOT>; > > + clock-names = "ipg", "ahb"; > > + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>; > > + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin"; > > No 'access-controllers' here? > > > + }; > > + }; > > -- > > 2.34.1 > > >
On 25-03-26, Marco Felsch wrote: > Hi Laurentiu, > > On 25-03-24, Rob Herring wrote: > > On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote: > > > From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > > > > > 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 <laurentiu.mihalcea@nxp.com> > > > --- > > > .../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. > > > > > + > > > +maintainers: > > > + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > > + > > > +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? > > > > > + > > > + 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>; > > > > > > > + reg-names = "bus", "ac"; > > Thanks for picking up my suggestion :) IMHO it does look more logical > now. I wasn't aware of the 'ranges' property else I would have suggested > you to use this property instead of having two regs, sorry. Once you > changed it to ranges we can drop the 'reg-names' as well since you only > need to supply the 'ac' register space. > > Regards, > Marco > > > > + power-domains = <&pgc_audio>; > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > + #access-controller-cells = <0>; > > > + ranges; I didn't noticed that we already do have the ranges 1:1 mapping, sorry! @Rob A "ranges = <0x0 0x30c00000 0x400000>;" would make writing/syncing the .dtsi harder since NXP decided to use use global addresses in their technical reference manual. @Laurentiu Could you please add a "ranges = <0x30c00000 0x30c00000 0x400000>;" for the bus? Of course it is still a 1:1 mapping but limits the bus size which can be helpful if someone add a device on the wrong bus. Sorry for my previous suggestion on your V2 which seems more reasonable now. Regards, Marco > > > + dma-controller@30e00000 { > > > + compatible = "fsl,imx8mp-sdma", "fsl,imx8mq-sdma"; > > > + reg = <0x30e00000 0x10000>; > > > + #dma-cells = <3>; > > > + clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SDMA3_ROOT>, > > > + <&clk IMX8MP_CLK_AUDIO_ROOT>; > > > + clock-names = "ipg", "ahb"; > > > + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>; > > > + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin"; > > > > No 'access-controllers' here? > > > > > + }; > > > + }; > > > -- > > > 2.34.1 > > > > > > >
On 25.03.2025 05:23, Rob Herring wrote: > On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote: >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> >> >> 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 <laurentiu.mihalcea@nxp.com> >> --- >> .../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 > >> + >> +maintainers: >> + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> >> + >> +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. 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. > >> + >> + 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? > > >> + reg-names = "bus", "ac"; >> + power-domains = <&pgc_audio>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + #access-controller-cells = <0>; >> + ranges; >> + >> + dma-controller@30e00000 { >> + compatible = "fsl,imx8mp-sdma", "fsl,imx8mq-sdma"; >> + reg = <0x30e00000 0x10000>; >> + #dma-cells = <3>; >> + clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SDMA3_ROOT>, >> + <&clk IMX8MP_CLK_AUDIO_ROOT>; >> + clock-names = "ipg", "ahb"; >> + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>; >> + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin"; > No 'access-controllers' here? no need for that unless the child wants to request a specific AC configuration for itself. > >> + }; >> + }; >> -- >> 2.34.1 >>
On 25-03-28, Mihalcea Laurentiu wrote: > > On 25.03.2025 05:23, Rob Herring wrote: > > On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote: > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > >> > >> 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 <laurentiu.mihalcea@nxp.com> > >> --- > >> .../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 > > >> +maintainers: > >> + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > >> + > >> +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. I think Rob is speaking from DT API pov. What the driver is doing with additional information is up to the driver. > 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. Which is? According the TRM it's a bit tricky to define the API since you need to describe two different types: - master configuration - peripheral configuration One which came up in my mind is: <&phandle TYPE ID VALUE>; e.g. <&aipstz AIPSTZ_MASTER 0 0xf>; <&aipstz AIPSTZ_PERI 0 0xf>; One could use a defien for the magic value of 0xf of course. > >> + 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? Switching from a global addressing to a local one is not favourable IMHO since NXP i.MX8M SoC TRMs are mention documenting all IPs with the global addressing scheme. So yes either your v2 scheme or the one with the limiting site but keeping the 1:1 mapping. Sorry again for the ping-pong, wasn't that clear to me until now. Regards, Marco > >> + reg-names = "bus", "ac"; > >> + power-domains = <&pgc_audio>; > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + #access-controller-cells = <0>; > >> + ranges; > >> + > >> + dma-controller@30e00000 { > >> + compatible = "fsl,imx8mp-sdma", "fsl,imx8mq-sdma"; > >> + reg = <0x30e00000 0x10000>; > >> + #dma-cells = <3>; > >> + clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SDMA3_ROOT>, > >> + <&clk IMX8MP_CLK_AUDIO_ROOT>; > >> + clock-names = "ipg", "ahb"; > >> + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>; > >> + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin"; > > No 'access-controllers' here? > > > no need for that unless the child wants to request a specific AC > configuration for itself. > > > > > >> + }; > >> + }; > >> -- > >> 2.34.1 > >> >
On 31.03.2025 09:41, Marco Felsch wrote: > On 25-03-28, Mihalcea Laurentiu wrote: >> On 25.03.2025 05:23, Rob Herring wrote: >>> On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote: >>>> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> >>>> >>>> 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 <laurentiu.mihalcea@nxp.com> >>>> --- >>>> .../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 >> >>>> +maintainers: >>>> + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> >>>> + >>>> +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. > I think Rob is speaking from DT API pov. What the driver is doing with > additional information is up to the driver. > >> 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. > Which is? > > According the TRM it's a bit tricky to define the API since you need to > describe two different types: > - master configuration > - peripheral configuration > > One which came up in my mind is: > > <&phandle TYPE ID VALUE>; > > e.g. > > <&aipstz AIPSTZ_MASTER 0 0xf>; > <&aipstz AIPSTZ_PERI 0 0xf>; > > One could use a defien for the magic value of 0xf of course. so, my original idea was to use 2 cells: <&phandle ID VALUE>, where bit 0 of ID is used to identify the IP type (master or slave/peripheral) and the rest of the bits are used to encode the ID itself. I think I like your idea a bit more though (i.e: have the TYPE as a separate cell) because I think it's easier to deal with/understand from the DTS user's perspective.
On Fri, Mar 28, 2025 at 02:34:11PM +0200, Mihalcea Laurentiu wrote: > > On 25.03.2025 05:23, Rob Herring wrote: > > On Mon, Mar 24, 2025 at 12:25:52PM -0400, Laurentiu Mihalcea wrote: > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > >> > >> 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 <laurentiu.mihalcea@nxp.com> > >> --- > >> .../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 > > > > > >> + > >> +maintainers: > >> + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > >> + > >> +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. > > > > > >> + > >> + 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
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. + +maintainers: + - Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> + +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 + + 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>; + reg-names = "bus", "ac"; + power-domains = <&pgc_audio>; + #address-cells = <1>; + #size-cells = <1>; + #access-controller-cells = <0>; + ranges; + + dma-controller@30e00000 { + compatible = "fsl,imx8mp-sdma", "fsl,imx8mq-sdma"; + reg = <0x30e00000 0x10000>; + #dma-cells = <3>; + clocks = <&audio_blk_ctrl IMX8MP_CLK_AUDIOMIX_SDMA3_ROOT>, + <&clk IMX8MP_CLK_AUDIO_ROOT>; + clock-names = "ipg", "ahb"; + interrupts = <GIC_SPI 34 IRQ_TYPE_LEVEL_HIGH>; + fsl,sdma-ram-script-name = "imx/sdma/sdma-imx7d.bin"; + }; + };