Message-ID: <20250307175309.od3grjzr63dxp6ec@pengutronix.de>
Date: Fri, 7 Mar 2025 18:53:09 +0100
From: Marco Felsch <m.felsch@...gutronix.de>
To: Laurentiu Mihalcea <laurentiumihalcea111@...il.com>
Cc: Mark Brown <broonie@...nel.org>,
	Daniel Baluta <daniel.baluta@...il.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
	Sascha Hauer <s.hauer@...gutronix.de>, devicetree@...r.kernel.org,
	imx@...ts.linux.dev, linux-kernel@...r.kernel.org,
	linux-sound@...r.kernel.org,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 1/3] ASoC: dt-bindings: support imx95's CM7 core

On 25-03-04, Laurentiu Mihalcea wrote:
> On 2/13/2025 8:47 AM, Marco Felsch wrote:
> > Hi Laurentiu, Daniel,
> >
> > On 25-02-12, Laurentiu Mihalcea wrote:
> >> On 2/12/2025 2:38 PM, Mark Brown wrote:
> >>> On Wed, Feb 12, 2025 at 12:11:49PM +0200, Daniel Baluta wrote:
> >>>> On Wed, Feb 12, 2025 at 11:38 AM Marco Felsch <m.felsch@...gutronix.de> wrote:
> >>>>> On 25-02-11, Laurentiu Mihalcea wrote:
> >>>>>> +    const: fsl,imx95-cm7-sof
> >>>>> Albeit Krzysztof already add his Reviewed-by, can I ask why we need to
> >>>>> add the -sof suffix instead of -audio or so? SOF is a software project
> >>>>> but you can clearly run different software on the audio-copro as well.
> >>>> Sure you can run a different software project on the audio DSP but
> >>>> you will need a way to distinguish between the different projects.
> >>>> There might be different mailbox, memory configurations. So you will  need
> >>>> to invent another suffix specific to the new project.
> >>>> We can make  const: fsl,imx95-cm7-audio as the one used with SOF
> >>>> and think about a different name later for when another project will
> >>>> want to use the DSP.
> >>> I think the point here was that the DT should stay the same even if the
> >>> DSP firwmare changes, just as how changing the main OS shouldn't affect
> >>> the DT.
> >> It's rather unfortunate but based on the experience from the 8 series
> >> (imx8qm, imx8qxp, imx8mp), the programming model can differ quite
> >> a bit (e.g: remoteproc vs SOF) even if the core is the same (i.e: DSP core).
> >>
> >> The different programming models also required different DT configurations
> >> (e.g: dif. mboxes as Daniel mentioned, some extra properties (i.e: reg-names), etc...)
> >>
> >> The "-sof" suffix was chosen here instead of the more generic "-audio" (or whatever else
> >> alternative) because the DT configuration is specific to SOF's programming model. Other
> >> audio applications running on the same core may have dif. configurations (e.g: use
> >> DTCM/ITCM for memory instead of DDR, dif. mbox count, etc...). I suppose this kind of thing
> >> is bound to happen to some degree since the DT node doesn't just describe the CM7 core
> >> (but, rather, it also encompasses information on the memory, mboxes, etc. used)
> >> but perhaps I'm wrong?
> > Time will tell if there will be any other user except for SOF for the
> > DSP but and this is what I wanted to point out: the DTS should abstract
> > the HW. IMHO The CM7-Audio node should contain all properties required
> > to turn power and reset the core (e.g. clocks, reset, pm-domains). I get
> > your point regarding different configs but please have a look at
> > mt8183-kukui.dtsi. Here the rpmsg config is a subnode of the actual
> > system-control-proc. This makes much more sense to me since the HW part
> > is part of the generic core-node and all the software config goes into a
> > separate subnode.
> >
> > Regards,
> >   Marco
> 
> I understand your point but we're dealing with 2 different programming
> models here: SOF and remoteproc as opposed to just remoteproc as it's
> the case for Mediatek.

The idea is exactly to decouple the CM7 HW config from the
software/firmware running on it and this is what MediaTek did.

Of course they are only interested in teh RPMsg part but this approach
could be used to decouple the HW and the FW part which is your use-case
(different FWs running on the CM7).

> Going for a similar approach would also mean quite a bit of software
> changes as we'd need to factor out the common bits (most importantly here:
> core operations like start/stop) and placing them in a common driver. 

Exactly but shouldn't be to hard to implement. Of course it is more work
than just adapting the compatible.

> This is not trivial and I'm not sure it's worth the effort right now
> given that:
> 
>     1) The current way we model the hardware inside the DT is not exactly inaccurate.
>     The core does physically use those memory regions, mailboxes, etc.
> 
>     2) Whatever we do, the information in the DT will still depend on the programming model.
>     It's just that you're placing it in a child node, instead of the parent, which is arguably not that
>     big of an improvement?

The benefit is to have different configs in place and a board can choose
between them, e.g.:

	sof_cpu: cm7-cpu@...00000 {
		compatible = "fsl,imx95-cm7";
		reg = <0x0 0x80000000 0x0 0x6100000>;
		reg-names = "sram";
		clocks = <...>;
		resets = <...>;
		access-controllers = <...>;

		firmware-name = "nxp/foo";
		firmware-config = <&cm7_sof>;

		cm7_sof: cm7-sof {
			compatible = "fsl,imx95-sof";
			memory-region = <&adma_res>;
			memory-region-names = "dma";
			mboxes = <&mu7 2 0>, <&mu7 2 1>, <&mu7 3 0>, <&mu7 3 1>;
			mbox-names = "txdb0", "txdb1", "rxdb0", "rxdb1";

			cpu: port {
				cpu_ep: endpoint {
					remote-endpoint = <&codec_ep>;
				};
			};
		};

		cm7_rpmsg: cm7-rpmsg {
			compatible = "fsl,imx95-rpmsg";
		};

		cm7_foo: cm7-foo {
			compatible = "fsl,imx95-foo";
		};
	};

I get your point and I'm not against your patchset. Just IMHO the above
example would make more sense to me since the CM7 hardware handling
(en-/disable/access/reset/clock) can be done by a common driver and must
not be done by each subsystem driver: sof, rpmsg, etc. Also the since
the driver would be common it could be reused by other CoProcessors
within the system as well since there are now more and more CoProcessors
on a SoC.

Regards,
  Marco

Powered by blists - more mailing lists