| Message ID | 20250221191909.31874-5-laurentiumihalcea111@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | imx8mp: add support for the IMX AIPSTZ bridge | expand |
On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote: > From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > AIPS5 is actually AIPSTZ5 as it offers some security-related > configurations. Since these configurations need to be applied before > accessing any of the peripherals on the bus, it's better to make AIPSTZ5 > be their parent instead of keeping AIPS5 and adding a child node for > AIPSTZ5. Also, because of the security configurations, the address space > of the bus has to be changed to that of the configuration registers. The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only config address part in your drivers. Frank > > Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the > missing 'power-domains' property. The domain needs to be powered on before > attempting to configure the security-related registers. > > The DT node name is not changed to avoid potential issues with DTs in > which this node is referenced. > > 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> > --- > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > index e0d3b8cba221..a1d9b834d2da 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { > }; > }; > > - aips5: bus@30c00000 { > - compatible = "fsl,aips-bus", "simple-bus"; > - reg = <0x30c00000 0x400000>; > + aips5: bus@30df0000 { > + compatible = "fsl,imx8mp-aipstz", "simple-bus"; > + reg = <0x30df0000 0x10000>; > + power-domains = <&pgc_audio>; > #address-cells = <1>; > #size-cells = <1>; > + #access-controller-cells = <0>; > ranges; > > spba-bus@30c00000 { > -- > 2.34.1 >
On 21.02.2025 21:56, Frank Li wrote: > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote: >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> >> >> AIPS5 is actually AIPSTZ5 as it offers some security-related >> configurations. Since these configurations need to be applied before >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5 >> be their parent instead of keeping AIPS5 and adding a child node for >> AIPSTZ5. Also, because of the security configurations, the address space >> of the bus has to be changed to that of the configuration registers. > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only > config address part in your drivers. > > Frank Any concerns/anything wrong with current approach? I find it a bit awkward to have the whole bus address space in the DT given that we're only interested in using the access controller register space. I'm fine with the approach you suggested but I don't see a reason for using it? > >> Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the >> missing 'power-domains' property. The domain needs to be powered on before >> attempting to configure the security-related registers. >> >> The DT node name is not changed to avoid potential issues with DTs in >> which this node is referenced. >> >> 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> >> --- >> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi >> index e0d3b8cba221..a1d9b834d2da 100644 >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { >> }; >> }; >> >> - aips5: bus@30c00000 { >> - compatible = "fsl,aips-bus", "simple-bus"; >> - reg = <0x30c00000 0x400000>; >> + aips5: bus@30df0000 { >> + compatible = "fsl,imx8mp-aipstz", "simple-bus"; >> + reg = <0x30df0000 0x10000>; >> + power-domains = <&pgc_audio>; >> #address-cells = <1>; >> #size-cells = <1>; >> + #access-controller-cells = <0>; >> ranges; >> >> spba-bus@30c00000 { >> -- >> 2.34.1 >>
On Tue, Feb 25, 2025 at 04:14:34PM +0200, Mihalcea Laurentiu wrote: > > On 21.02.2025 21:56, Frank Li wrote: > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote: > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > >> > >> AIPS5 is actually AIPSTZ5 as it offers some security-related > >> configurations. Since these configurations need to be applied before > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5 > >> be their parent instead of keeping AIPS5 and adding a child node for > >> AIPSTZ5. Also, because of the security configurations, the address space > >> of the bus has to be changed to that of the configuration registers. > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only > > config address part in your drivers. > > > > Frank > > > Any concerns/anything wrong with current approach? > > > I find it a bit awkward to have the whole bus address space > > in the DT given that we're only interested in using the access > > controller register space. > > > I'm fine with the approach you suggested but I don't see a > > reason for using it? After second through, reg should indicate only used space. The current method is good enough. Just need figure out KK's comment about 'simple-bus' Frank > > > > > >> Finally, since AIPSTZ5 belongs to the AUDIOMIX power domain, add the > >> missing 'power-domains' property. The domain needs to be powered on before > >> attempting to configure the security-related registers. > >> > >> The DT node name is not changed to avoid potential issues with DTs in > >> which this node is referenced. > >> > >> 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> > >> --- > >> arch/arm64/boot/dts/freescale/imx8mp.dtsi | 8 +++++--- > >> 1 file changed, 5 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >> index e0d3b8cba221..a1d9b834d2da 100644 > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { > >> }; > >> }; > >> > >> - aips5: bus@30c00000 { > >> - compatible = "fsl,aips-bus", "simple-bus"; > >> - reg = <0x30c00000 0x400000>; > >> + aips5: bus@30df0000 { > >> + compatible = "fsl,imx8mp-aipstz", "simple-bus"; > >> + reg = <0x30df0000 0x10000>; > >> + power-domains = <&pgc_audio>; > >> #address-cells = <1>; > >> #size-cells = <1>; > >> + #access-controller-cells = <0>; > >> ranges; > >> > >> spba-bus@30c00000 { > >> -- > >> 2.34.1 > >>
On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote: > > On 21.02.2025 21:56, Frank Li wrote: > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote: > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > >> > >> AIPS5 is actually AIPSTZ5 as it offers some security-related > >> configurations. Since these configurations need to be applied before > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5 > >> be their parent instead of keeping AIPS5 and adding a child node for > >> AIPSTZ5. Also, because of the security configurations, the address space > >> of the bus has to be changed to that of the configuration registers. > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only > > config address part in your drivers. > > > > Frank > > > Any concerns/anything wrong with current approach? > > > I find it a bit awkward to have the whole bus address space > in the DT given that we're only interested in using the access > controller register space. > > > I'm fine with the approach you suggested but I don't see a > reason for using it? Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus Applications Processor Reference Manual, Rev. 3, 08/2024), the AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something different than the bus configuration. Why not model it as part of the bus? > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >> index e0d3b8cba221..a1d9b834d2da 100644 > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { > >> }; > >> }; > >> > >> - aips5: bus@30c00000 { > >> - compatible = "fsl,aips-bus", "simple-bus"; > >> - reg = <0x30c00000 0x400000>; > >> + aips5: bus@30df0000 { ^^^^^^^^^^^^ > >> + compatible = "fsl,imx8mp-aipstz", "simple-bus"; > >> + reg = <0x30df0000 0x10000>; > >> + power-domains = <&pgc_audio>; > >> #address-cells = <1>; > >> #size-cells = <1>; > >> + #access-controller-cells = <0>; > >> ranges; > >> > >> spba-bus@30c00000 { ^^^^^^^^^^^^^^^^^ This looks very strange: The aips5 bus starts at 0x30df0000 and has a child bus starting at 0x30c00000? regards, Marc
On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote: > On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote: > > > > On 21.02.2025 21:56, Frank Li wrote: > > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote: > > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > >> > > >> AIPS5 is actually AIPSTZ5 as it offers some security-related > > >> configurations. Since these configurations need to be applied before > > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5 > > >> be their parent instead of keeping AIPS5 and adding a child node for > > >> AIPSTZ5. Also, because of the security configurations, the address space > > >> of the bus has to be changed to that of the configuration registers. > > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only > > > config address part in your drivers. > > > > > > Frank > > > > > > Any concerns/anything wrong with current approach? > > > > > > I find it a bit awkward to have the whole bus address space > > in the DT given that we're only interested in using the access > > controller register space. > > > > > > I'm fine with the approach you suggested but I don't see a > > reason for using it? > > Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus > Applications Processor Reference Manual, Rev. 3, 08/2024), the > AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something > different than the bus configuration. Why not model it as part of the > bus? > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > >> index e0d3b8cba221..a1d9b834d2da 100644 > > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { > > >> }; > > >> }; > > >> > > >> - aips5: bus@30c00000 { > > >> - compatible = "fsl,aips-bus", "simple-bus"; > > >> - reg = <0x30c00000 0x400000>; > > >> + aips5: bus@30df0000 { > ^^^^^^^^^^^^ > > >> + compatible = "fsl,imx8mp-aipstz", "simple-bus"; > > >> + reg = <0x30df0000 0x10000>; > > >> + power-domains = <&pgc_audio>; > > >> #address-cells = <1>; > > >> #size-cells = <1>; > > >> + #access-controller-cells = <0>; > > >> ranges; > > >> > > >> spba-bus@30c00000 { > ^^^^^^^^^^^^^^^^^ > > This looks very strange: The aips5 bus starts at 0x30df0000 and has a > child bus starting at 0x30c00000? @30df0000 should match controller reg's address. subnode address 0x30c00000, should be descript in "ranges", which 1:1 map. So it should be reasonable. another example: i2c@1000 { device@1c <- which use difference address space. } The similar case also happen at pcie. Frank > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 27.02.2025 11:45:36, Frank Li wrote: > On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote: > > On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote: > > > > > > On 21.02.2025 21:56, Frank Li wrote: > > > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote: > > > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > > >> > > > >> AIPS5 is actually AIPSTZ5 as it offers some security-related > > > >> configurations. Since these configurations need to be applied before > > > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5 > > > >> be their parent instead of keeping AIPS5 and adding a child node for > > > >> AIPSTZ5. Also, because of the security configurations, the address space > > > >> of the bus has to be changed to that of the configuration registers. > > > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only > > > > config address part in your drivers. > > > > > > > > Frank > > > > > > > > > Any concerns/anything wrong with current approach? > > > > > > > > > I find it a bit awkward to have the whole bus address space > > > in the DT given that we're only interested in using the access > > > controller register space. > > > > > > > > > I'm fine with the approach you suggested but I don't see a > > > reason for using it? > > > > Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus > > Applications Processor Reference Manual, Rev. 3, 08/2024), the > > AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something > > different than the bus configuration. Why not model it as part of the > > bus? > > > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > >> index e0d3b8cba221..a1d9b834d2da 100644 > > > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { > > > >> }; > > > >> }; > > > >> > > > >> - aips5: bus@30c00000 { > > > >> - compatible = "fsl,aips-bus", "simple-bus"; > > > >> - reg = <0x30c00000 0x400000>; > > > >> + aips5: bus@30df0000 { > > ^^^^^^^^^^^^ > > > >> + compatible = "fsl,imx8mp-aipstz", "simple-bus"; > > > >> + reg = <0x30df0000 0x10000>; > > > >> + power-domains = <&pgc_audio>; > > > >> #address-cells = <1>; > > > >> #size-cells = <1>; > > > >> + #access-controller-cells = <0>; > > > >> ranges; > > > >> > > > >> spba-bus@30c00000 { > > ^^^^^^^^^^^^^^^^^ > > > > This looks very strange: The aips5 bus starts at 0x30df0000 and has a > > child bus starting at 0x30c00000? > > @30df0000 should match controller reg's address. > > subnode address 0x30c00000, should be descript in "ranges", which 1:1 map. Ok. What about aips1...4? Should the be changed as well in this patch? Marc
Hi, On 25-02-27, Frank Li wrote: > On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote: > > On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote: > > > > > > On 21.02.2025 21:56, Frank Li wrote: > > > > On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote: > > > >> From: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com> > > > >> > > > >> AIPS5 is actually AIPSTZ5 as it offers some security-related > > > >> configurations. Since these configurations need to be applied before > > > >> accessing any of the peripherals on the bus, it's better to make AIPSTZ5 > > > >> be their parent instead of keeping AIPS5 and adding a child node for > > > >> AIPSTZ5. Also, because of the security configurations, the address space > > > >> of the bus has to be changed to that of the configuration registers. > > > > The orginal 0x30c0_0000..0x31200000 include 0x30df0000, why not map only > > > > config address part in your drivers. > > > > > > > > Frank > > > > > > Any concerns/anything wrong with current approach? > > > > > > > > > I find it a bit awkward to have the whole bus address space > > > in the DT given that we're only interested in using the access > > > controller register space. > > > > > > > > > I'm fine with the approach you suggested but I don't see a > > > reason for using it? > > > > Looking at the "AIPS5 Memory Map" (page 34/35 in i.MX 8M Plus > > Applications Processor Reference Manual, Rev. 3, 08/2024), the > > AIPS5_Configuration is part of the AIPS5 bus. IMHO the bus is something > > different than the bus configuration. Why not model it as part of the > > bus? > > > > > >> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > >> index e0d3b8cba221..a1d9b834d2da 100644 > > > >> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > >> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi > > > >> @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { > > > >> }; > > > >> }; > > > >> > > > >> - aips5: bus@30c00000 { > > > >> - compatible = "fsl,aips-bus", "simple-bus"; > > > >> - reg = <0x30c00000 0x400000>; > > > >> + aips5: bus@30df0000 { > > ^^^^^^^^^^^^ > > > >> + compatible = "fsl,imx8mp-aipstz", "simple-bus"; > > > >> + reg = <0x30df0000 0x10000>; > > > >> + power-domains = <&pgc_audio>; > > > >> #address-cells = <1>; > > > >> #size-cells = <1>; > > > >> + #access-controller-cells = <0>; > > > >> ranges; > > > >> > > > >> spba-bus@30c00000 { > > ^^^^^^^^^^^^^^^^^ > > > > This looks very strange: The aips5 bus starts at 0x30df0000 and has a > > child bus starting at 0x30c00000? > > @30df0000 should match controller reg's address. > > subnode address 0x30c00000, should be descript in "ranges", which 1:1 map. > > So it should be reasonable. another example: > i2c@1000 { > > device@1c <- which use difference address space. > } > > The similar case also happen at pcie. I'm not really convinced that pcie and i2c are good examples here. I2C does have an other addressing scheme by nature and the hotplug-able pcie is dependeds on the pcie device memory map of course. Here we're talking about an access control IP core on a bus which is static. But.. it looks like from DT abstraction it's fine because STM did something similiar with their st,stm32mp25-rifsc or st,stm32-etzpc albeit it does look strange and I don't know why we have to limit the address space since it was already mapped but used by the fsl,aips-bus driver. Regards, Marco > > Frank > > > > > regards, > > Marc > > > > -- > > Pengutronix e.K. | Marc Kleine-Budde | > > Embedded Linux | https://www.pengutronix.de | > > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | > > > >
diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi index e0d3b8cba221..a1d9b834d2da 100644 --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi @@ -1399,11 +1399,13 @@ eqos: ethernet@30bf0000 { }; }; - aips5: bus@30c00000 { - compatible = "fsl,aips-bus", "simple-bus"; - reg = <0x30c00000 0x400000>; + aips5: bus@30df0000 { + compatible = "fsl,imx8mp-aipstz", "simple-bus"; + reg = <0x30df0000 0x10000>; + power-domains = <&pgc_audio>; #address-cells = <1>; #size-cells = <1>; + #access-controller-cells = <0>; ranges; spba-bus@30c00000 {