Message-ID: <20250421214924.a52pakut7nzrbufc@bryanbrattlof.com>
Date: Mon, 21 Apr 2025 16:49:24 -0500
From: Bryan Brattlof <bb@...com>
To: Andrew Davis <afd@...com>
CC: Judith Mendez <jm@...com>, Nishanth Menon <nm@...com>,
        Vignesh Raghavendra
	<vigneshr@...com>,
        Tero Kristo <kristo@...nel.org>, 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>,
        Beleswar Prasad <b-padhi@...com>,
        Markus
 Schneider-Pargmann <msp@...libre.com>,
        Devarsh Thakkar <devarsht@...com>
Subject: Re: [PATCH v7 06/11] arm64: dts: ti: k3-am62a7-sk: Enable IPC with
 remote processors

On April 21, 2025 thus sayeth Andrew Davis:
> On 4/21/25 2:05 PM, Judith Mendez wrote:
> > Hi Bryan,
> > 
> > On 4/21/25 11:26 AM, Bryan Brattlof wrote:
> > > On April 21, 2025 thus sayeth Nishanth Menon:
> > > > On 10:04-20250419, Bryan Brattlof wrote:
> > > > > On April 15, 2025 thus sayeth Judith Mendez:
> > > > > > 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>
> > > > > > Acked-by: Andrew Davis <afd@...com>
> > > > > > Reviewed-by: Beleswar Padhi <b-padhi@...com>
> > > > > > Reviewed-by: Jai Luthra <jai.luthra@...asonboard.com>
> > > > > > ---
> > > > > >   arch/arm64/boot/dts/ti/k3-am62a7-sk.dts | 96 +++++++++++++++++++++++--
> > > > > >   1 file changed, 90 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts b/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
> > > > > > index 1c9d95696c839..7d817b447c1d0 100644
> > > > > > --- a/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
> > > > > > +++ b/arch/arm64/boot/dts/ti/k3-am62a7-sk.dts
> > > > > > @@ -52,6 +52,42 @@ linux,cma {
> > > > > >               linux,cma-default;
> > > > > >           };
> > > > > > +        c7x_0_dma_memory_region: c7x-dma-memory@...00000 {
> > > > > > +            compatible = "shared-dma-pool";
> > > > > > +            reg = <0x00 0x99800000 0x00 0x100000>;
> > > > > > +            no-map;
> > > > > > +        };
> > > > > > +
> > > > > > +        c7x_0_memory_region: c7x-memory@...00000 {
> > > > > > +            compatible = "shared-dma-pool";
> > > > > > +            reg = <0x00 0x99900000 0x00 0xf00000>;
> > > > > > +            no-map;
> > > > > > +        };
> > > > > > +
> > > > > 
> > > > > I know this has been a push for our IPC and MCU+ teams for a couple
> > > > > windows now, though I do want to point out that some AM62A devices
> > > > > (AM62A12AQMSIAMBRQ1) will not even have a C7x.
> > > > > 
> > > > > It's relatively easy to cut nodes out that describe the hardware in the
> > > > > bootloaders, but once we start configuring them to demo something it
> > > > > becomes impossible to unwind that during boot.
> > > > > 
> > > > > We can clam we only support the superset devices but I just wanted to
> > > > > make this email so I could point people to it when they inevitably ask
> > > > > why their parts do not work out of the box with Linux.
> > > > > 
> > > > > Naked-by: Bryan Brattlof <bb@...com>
> > > > 
> > > > 
> > > > I am confused. I do not see support for AM62A1 in upstream. We have
> > > > AM62A7-SK in upstream. I am not sure what direction you are suggesting
> > > > here.
> > > 
> > > All I'm trying to point out is for every part we upstream there are >10
> > > times the number of parts that for one reason or another will not make
> > > it to these upstream repositories.
> > > 
> > > Most of these parts will have trivial changes like having lower CPU
> > > counts, some will not have a GPU, MCU, PRU, or display, or maybe it's
> > > just a package change and the thermal zones are different, or it's just
> > > the speeds the IP can confidently run at, or it could be as simple as
> > > DDR part changes. Each variant will be mostly the superset device with
> > > one or two nodes disabled or modified in some way.
> > > 
> > > For a while now, without configuring the remote cores to demo anything,
> > > it's been relatively seamless to support these variants in the
> > > bootloaders by disabling or modifying the nodes that do not exist so
> > > Linux can at least boot to a shell and provides a great foundation for
> > > others to start their development
> > > 
> > > If we want to use these boards to demo a advanced usecases we can do
> > > that but I worry it will come at the cost of supporting all the part
> > > variants.
> > > 
> > > My hope was we could define the board as minimally as possible here so
> > > we can maximize their flexibility with what timers, mailboxes and memory
> > > carve-outs each remote processor uses.
> > > 
> > 
> > Is it not agreed upon to support the superset device upstream? It seems
> > like what we need is a detailed whitepaper on board bring-up for each
> > part variant instead of NOT adding support for the superset device
> > upstream approach.
> > 
> 
> We would still support the superset device upstream, Bryan is simply
> suggesting (correct me if I'm wrong) a different way of supporting
> the superset device such that later adding support for the cutdowns
> is less painful.
> 
> To this I 100% agree, and I've suggested the same before in
> cases where we know a subset device is in the works. Sometimes
> plans for a cutdown happen after we have added support and that
> makes a bit of a mess (see J78424 / J742s2). Even when we know
> this is going to happen we still make a mess of DT which we end
> up having to clean up later (see AM62p / J722s).
> 

Exactly. We will typically have the AM62A7 superset plan well in advance 
then as the platform ages we start defining cutdowns that may or may not 
function depending on the behavior of the drivers which creates the 
weird DT layout and even worse some fraction of the subset devices that 
may or may not boot and no indication if it even should.

Separating the core description of the hardware, so Zephyr, U-Boot, 
Coreboot, Barebox and whoever else can use and modify it, from the 
allocation/configuration of the hardware while Linux is running is the 
only way (in my eyes) this can possibly scale.

>
> Easy manipulation of the DT in this way was one of the promises
> of "System Device Trees". Until we have that, the next best thing
> is DT overlays. The solution here then is to factor out the components
> we know will be conditional into a device tree overlays. These
> overlays will be applied by default to form the superset DT as the
> out-of-box supported device, while also allowing easy removal without
> having the bootloader perform complex DT surgery. Win-Win
> 
> The plan we agreed on for this series (back in v2 IIRC) is to add
> these items as part of the base board DT to match what is already
> done for all our other devices. *then* we factor it out from all
> devices in one go to make that series more coherent.
> 
> So if that sounds good, Bryan feel free to un-NAK this and we
> we will be sure to make your task of supporting all these subset
> devices much easier in the follow up series :)

I can get behind that plan but I do still worry about how this scales. 
Many distributions are starting to look at UKIs as the solution to the 
exploding number of DTBs. Adding overlays will just add plutonium to 
this compressed DTB blob best-fit walk.

Un-Naked-by: Bryan Brattlof <bb@...com>

~Bryan

Powered by blists - more mailing lists