Message-ID: <db72b7f7-0771-442c-91bf-507a3e08cfdc@ti.com>
Date: Thu, 6 Mar 2025 19:35:04 -0600
From: Judith Mendez <jm@...com>
To: Devarsh Thakkar <devarsht@...com>, Andrew Davis <afd@...com>,
        Nishanth
 Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>
CC: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Hari Nagalla
	<hnagalla@...com>,
        Soumya <s-tripathy@...com>,
        "'Krishnamoorthy, Venkatesan'"
	<v-krishnamoorthy@...com>,
        "Khasim, Syed Mohammed" <khasim@...com>,
        "Bajjuri,
 Praneeth" <praneeth@...com>
Subject: Re: [PATCH v5 06/10] arm64: dts: ti: k3-am62p5-sk: Enable IPC with
 remote processors

Hi Devarsh,

On 2/27/25 6:05 AM, Devarsh Thakkar wrote:
> Hi Judith,
> 
> Thanks for the patch.
> 
> On 18/02/25 23:21, Judith Mendez wrote:
>> Hi Andrew,
>>
>>
>> On 2/18/25 10:38 AM, Andrew Davis wrote:
>>> On 2/10/25 4:15 PM, Judith Mendez wrote:
>>>> From: Devarsh Thakkar <devarsht@...com>
>>>>
>>>> For each remote proc, reserve memory for IPC and bind the mailbox
>>>> assignments. Two memory regions are reserved for each remote processor.
>>>> The first region of 1MB of memory is used for Vring shared buffers
>>>> and the second region is used as external memory to the remote processor
>>>> for the resource table and for tracebuffer allocations.
>>>>
>>>> Signed-off-by: Devarsh Thakkar <devarsht@...com>
>>>> Signed-off-by: Hari Nagalla <hnagalla@...com>
>>>> Signed-off-by: Judith Mendez <jm@...com>
>>>> ---
>>>> Changes since v4:
>>>> - Drop SRAM node for am62px MCU R5fSS0 core0
>>>> ---
>>>>    arch/arm64/boot/dts/ti/k3-am62p5-sk.dts | 50 ++++++++++++++++++++++---
>>>>    1 file changed, 44 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>>>> b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>>>> index ad71d2f27f538..9609727d042d3 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>>>> @@ -48,6 +48,30 @@ reserved-memory {
>>>>            #size-cells = <2>;
>>>>            ranges;
>>>> +        mcu_r5fss0_core0_dma_memory_region:
>>>> mcu-r5fss-dma-memory-region@...00000 {
>>>> +            compatible = "shared-dma-pool";
>>>> +            reg = <0x00 0x9b800000 0x00 0x100000>;
>>>> +            no-map;
>>>> +        };
>>>> +
> 
> I believe you are testing these carveouts against the default firmwares
> shipped with AM62P SDK (compiled from meta-arago), With the same firmwares,
> each remote core also does inter-processor communication with each other
> (RTOS<->RTOS) on bootup, so you need to reserve the regions for the same too
> as done here [1].

This is how I originally had the patch Devarsh, if you see earlier
review, we removed the SRAM nodes and the rtos-to-rtos memory carveouts.


> 
>>>> +        mcu_r5fss0_core0_memory_region: mcu-r5fss-memory-region@...00000 {
>>>> +            compatible = "shared-dma-pool";
>>>> +            reg = <0x00 0x9b900000 0x00 0xf00000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        wkup_r5fss0_core0_dma_memory_region: r5f-dma-memory@...00000 {
>>>> +            compatible = "shared-dma-pool";
>>>> +            reg = <0x00 0x9c800000 0x00 0x100000>;
>>>> +            no-map;
>>>> +        };
>>>> +
>>>> +        wkup_r5fss0_core0_memory_region: r5f-memory@...00000 {
>>>> +            compatible = "shared-dma-pool";
>>>> +            reg = <0x00 0x9c900000 0x00 0x1e00000>;
>>>
>>> 0x1e00000?
>>>
>>> Yes I know you didn't add this and are just coping it from below, but it
>>> is still an issue. I see the same problem for the next patch, the R5F memory
>>> size is 0xc00000??
>>>
>>> Every remote core gets 15MB (0xf00000), this has been true for all K3, and
>>> all cores, DSP, R5F, M4, etc.. You even do it correct for the MCU R5F above,
>>> but the WKUP R5F on AM62P and AM62 are just randomly given 30M and 12MB?
>>
>> Not sure why FW requires 30MB here, I have reached out to FW team to
>> investigate this, will respond back here soon.
>>
> 
> You will need an alignment with the firmware team to make sure that it doesn't
> break with the default firmwares shipped with the AM62Px SDK. Also just FYI,
> this will leave a gap of 14 MiB between the wakeup R5 and the next component
> i.e. ATF, ideally we should have avoided this gap but seems like ATF nodes are
> already upstream [2], so probably can't do much, nevertheless I hope that 14
> MiB will be claimed/used by Linux in some manner.

I did a sanity boot test with the default firmwares shipped for am62px
SDK, no error with am62px boot so far. Changes are: removed SRAM node, 
reduced wkup r5 memory carveout, no rtos-to-rtos memory carveout).

But I realize this is not a complete test. I believe there may be
potentially memory corruption with these changes if all implemented.

Andrew, I am not sure we are going in a good direction here, unless we
have a different reduced/fixed FW in the am62px SDK, we may have memory
corruption issues on our hands.

~ Judith


> 
> Soumya,
> Please provide an ACK for this, if the DM R5 firmware is exceeding 15 MiB,
> then you will need to update your linker scripts and regenerate the ipc echo
> test firmwares to make sure the wakeup R5 code/data does not exceed to what is
> being proposed here (15 MiB).
> 
> [1]:
> https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts?h=11.00.05#n72
> 
> [2]:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts?h=next-20250227#n51
> 
> Regards
> Devarsh

Powered by blists - more mailing lists