On 25-03-10, Laurentiu Mihalcea wrote:

On 3/7/2025 5:22 PM, Marco Felsch wrote:

quoted

On 25-03-04, Laurentiu Mihalcea wrote:

quoted

On 2/28/2025 12:19 PM, Marco Felsch wrote:

quoted

Hi,

On 25-02-27, Frank Li wrote:

quoted

On Thu, Feb 27, 2025 at 11:57:54AM +0100, Marc Kleine-Budde wrote:

quoted

On 25.02.2025 16:14:34, Mihalcea Laurentiu wrote:

quoted

On 21.02.2025 21:56, Frank Li wrote:

quoted

On Fri, Feb 21, 2025 at 02:19:08PM -0500, Laurentiu Mihalcea wrote:

quoted

From: Laurentiu Mihalcea <redacted>

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?

quoted

quoted

quoted

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 {

                       ^^^^^^^^^^^^

quoted

quoted

quoted

+			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

The address space of the bridge was changed to that of the bridge's
configuration space because I think it's very awkward from the
software's point of view to have to hardcode the offset and size of
the configuration space inside the driver. 

You mean the access-controller IP core. I could also arguee that it's
akward to put the bridge access-controller IP core into the middle of
the bridge address-space instead of placing it at the very beginning of
the bridge. But this doesn't help here :)

I see what you mean but from DT abstraction POV it seems more reasonable
to keep it as it is and just adapt the compatible. The current driver
maps the whole address space too, so I don't see why we need to change
it if we change it to the aipstz driver. If you see the
access-controller IP core as part of the bus I don't see any problem and
would argue that the offset detail needs to be handled within the
driver.

quoted

I also looked at what STM did with "st,stm32-etzpc" so I thought this
would be acceptable from the DT's POV.

Regarding why I chose not to model the access controller part as a subnode of the
bus:

    1) The access controller is part of the bridge itself (not a separate module accessible
    via the bridge like it's the case for its children) so I think the current approach
    should also make sense if we take the hardware into consideration.

I don't like this approach if you see the controller as part of the
bridge because the offset could be handled within the bridge driver.
I also that the register offset needs to be supplied else we can't reuse
the driver and we don't want to adapt the driver for each SoC.

What came into my mind is the following:

	spba-bus@30c00000 {
		compatible = "nxp,imx8mp-aiptz-bus", "nxp,aiptz-bus";
		reg = <0x30c00000 0x400000>, <0x30df0000 0x10000>;
		reg-names = "bus", "aipstz";

		child-nodes {};
		child-nodes {};
		child-nodes {};
	}

This way we can abstract the access-controller register space and the
whole bus register space and a generic driver could be written just by
making use two reg fields.

by changing the compatible, we've also effectively changed the
programming model.

You need to keep the backward compatible which means, a new kernel have
to boot an old dts (firmware), which is stil the case since the
"simple-bus" driver sill matches on the old dts files.

But this doesn't mean that a new dts (firmware) have to be compatible
with an old kernel.

I don't really see why we need to stick to the old way of configuring
the bus node (i.e: specify the whole address space of the bus as well)
when all we really care about is the AC configuration region?

I see and as I said it's just my oppinion that we should abstract it
this way since upstream already accepted the approach you implemented
for the STM32 case.

If NXP would have placed the AC at the beginning of each AIPS bus you
wouldn't need to deal with this problem too since you would have the
base register already.

anyhow, I'm not going to insist on this. I think the proposed approach
will work just fine. If there's no other comments on this then I'll
just switch to it in V3.

That would be nice but please wait for maintainers feedback e.g Shawn.

Regards,
  Marco