Message-ID: <ec38d41d-f2ce-4a98-bb02-d1ae9beb0b3b@gmail.com>
Date: Mon, 10 Mar 2025 22:24:44 +0200
From: Laurentiu Mihalcea <laurentiumihalcea111@...il.com>
To: Marco Felsch <m.felsch@...gutronix.de>
Cc: Frank Li <Frank.li@....com>, Marc Kleine-Budde <mkl@...gutronix.de>,
 Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 devicetree@...r.kernel.org, Daniel Baluta <daniel.baluta@....com>,
 Sascha Hauer <s.hauer@...gutronix.de>, linux-kernel@...r.kernel.org,
 imx@...ts.linux.dev, Pengutronix Kernel Team <kernel@...gutronix.de>,
 Shawn Guo <shawnguo@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Fabio Estevam <festevam@...il.com>, Shengjiu Wang <shengjiu.wang@....com>,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 4/5] arm64: dts: imx8mp: convert 'aips5' to 'aipstz5'



On 3/7/2025 5:22 PM, Marco Felsch wrote:
> On 25-03-04, Laurentiu Mihalcea wrote:
>> On 2/28/2025 12:19 PM, Marco Felsch wrote:
>>> 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@....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@...f0000 {
>>>>>>>>  			};
>>>>>>>>  		};
>>>>>>>>
>>>>>>>> -		aips5: bus@...00000 {
>>>>>>>> -			compatible = "fsl,aips-bus", "simple-bus";
>>>>>>>> -			reg = <0x30c00000 0x400000>;
>>>>>>>> +		aips5: bus@...f0000 {
>>>>>                        ^^^^^^^^^^^^
>>>>>>>> +			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@...00000 {
>>>>>                         ^^^^^^^^^^^^^^^^^
>>>>>
>>>>> 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@...0 {
>>>>
>>>> 	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.
>
>> 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@...00000 {
> 		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.
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?
 
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.


>
>>     2) The access controller configuration also impacts the AP. As such, we needed a way to
>>     enforce a dependency between the children of the aips5 bus and the access controller
>>     (we could have used the 'access-controllers' property like we did with the DSP but having
>>     to do that for all children of the bus is not ideal I'd say.
>>
>> Of course, argument no. 2 is somewhat brittle in the context of i.MX8MP as the reset
>> values of the AC's registers already allow the AP to access said peripherals. Despite this,
>> I think the current approach would be more scalable given that the IP offers some more
>> configuration bits which we might want to toggle. For that to work, we need to make sure
>> the bits are toggled before the AP accesses the peripherals on the bridge.
> Please have a look on my suggestion above.
>
> Regards,
>   Marco
>
>> Note that we don't do this for aips1-aips4 because it's really not needed. If I'm not mistaken,
>> they're not really attached to a PD so we don't need to re-configure them each time the domain
>> is power cycled (which is why we can just do it once from ATF during the boot process)

Powered by blists - more mailing lists