Message-ID: <20250427020825.GC13806@nxa18884-linux>
Date: Sun, 27 Apr 2025 10:08:25 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Hiago De Franco <hiagofranco@...il.com>, daniel.baluta@....com,
	iuliana.prodan@....nxp.com, linux-remoteproc@...r.kernel.org,
	Bjorn Andersson <andersson@...nel.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Hiago De Franco <hiago.franco@...adex.com>
Subject: Re: [PATCH] remoteproc: imx_rproc: replace devm_clk_get() with
 devm_clk_get_optional()

On Sat, Apr 26, 2025 at 03:47:50PM -0600, Mathieu Poirier wrote:
>On Sat, 26 Apr 2025 at 06:41, Peng Fan <peng.fan@....nxp.com> wrote:
>>
>> On Wed, Apr 23, 2025 at 04:21:56PM -0300, Hiago De Franco wrote:
>> >Hi Mathieu,
>> >
>> >On Wed, Apr 23, 2025 at 11:14:17AM -0600, Mathieu Poirier wrote:
>> >> Good morning,
>> >>
>> >> On Wed, Apr 23, 2025 at 12:51:31PM -0300, Hiago De Franco wrote:
>> >> > From: Hiago De Franco <hiago.franco@...adex.com>
>> >> >
>> >> > The "clocks" device tree property is not mandatory, and if not provided
>> >> > Linux will shut down the remote processor power domain during boot if it
>> >> > is not present, even if it is running (e.g. it was started by U-Boot's
>> >> > bootaux command).
>> >>
>> >> If a clock is not present imx_rproc_probe() will fail, the clock will remain
>> >> unused and Linux will switch it off.  I think that is description of what is
>> >> happening.
>> >>
>> >> >
>> >> > Use the optional devm_clk_get instead.
>> >> >
>> >> > Signed-off-by: Hiago De Franco <hiago.franco@...adex.com>
>> >> > ---
>> >> >  drivers/remoteproc/imx_rproc.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>> >> > index 74299af1d7f1..45b5b23980ec 100644
>> >> > --- a/drivers/remoteproc/imx_rproc.c
>> >> > +++ b/drivers/remoteproc/imx_rproc.c
>> >> > @@ -1033,7 +1033,7 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
>> >> >    if (dcfg->method == IMX_RPROC_NONE)
>> >> >            return 0;
>> >> >
>> >> > -  priv->clk = devm_clk_get(dev, NULL);
>> >> > +  priv->clk = devm_clk_get_optional(dev, NULL);
>> >>
>> >> If my understanding of the problem is correct (see above), I think the real fix
>> >> for this is to make the "clocks" property mandatory in the bindings.
>> >
>> >Thanks for the information, from my understanding this was coming from
>> >the power domain, I had a small discussion about this with Peng [1],
>> >where I was able to bisect the issue into a scu-pd commit. But I see
>> >your point for this commit, I can update the commit description.
>> >
>> >About the change itself, I was not able to find a defined clock to use
>> >into the device tree node for the i.MX8QXP/DX, maybe I am missing
>> >something? I saw some downstream device trees from NXP using a dummy
>> >clock, which I tested and it works, however this would not be the
>> >correct solution.
>>
>> The clock should be "clocks = <&clk IMX_SC_R_M4_0_PID0 IMX_SC_PM_CLK_CPU>;" for
>> i.MX8QX. This should be added into device tree to reflect the hardware truth.
>>
>> But there are several working configurations regarding M4 on i.MX8QM/QX/DX/DXL.
>>
>> 1. M4 in a separate SCFW partition, linux has no permission to configure
>>   anything except building rpmsg connection.
>> 2. M4 in same SCFW partition with Linux, Linux has permission to start/stop M4
>>    In this scenario, there are two more items:
>>    -(2.1) M4 is started by bootloader
>>    -(2.2) M4 is started by Linux remoteproc.
>>
>>
>> Current imx_rproc.c only supports 1 and 2.2,
>> Your case is 2.1.
>
>Remoteproc operations .attach() and .detach() are implemented in
>imx_rproc.c and as such, 2.1 _is_ supported.

For i.MX8QM/QXP/DX/DXL, attach/detach is for case 1.

To support case 2.1, more code needs to be added in imx_rproc_detect_mode,

Something as below(no test, no build, just write example):
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 09d02f7d9e42..eeb1cd19314c 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -1019,6 +1019,9 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
                        if (of_property_read_u32(dev->of_node, "fsl,entry-address", &priv->entry))
                                return -EINVAL;

+                       if (imx_sc_cpu_is_started(M4X))
+                               priv->rproc->state = RPROC_DETACHED;
+
                        return imx_rproc_attach_pd(priv);
                }


When we let uboot to start M4(case 1), we(NXP) only wanna to add some test
code in U-Boot. Not intended to make it for remoteproc

But if there are users wanna case 1 in their product, we could support it,
1. adding cpu state detection in drivers/firmware/imx/
2. Use the cpu state API in imx_rproc.c to detect cpu is started by bootloader
   when the cpu is owned by linux.

>
>>
>> There is a clk_prepare_enable which not work for case 1 if adding a real
>> clock entry.
>>
>> So need move clk_prepare_enable to imx_rproc_start, not leaving it in probe?`
>> But for case 2.1, without clk_prepare_enable, kernel clk disable unused will
>> turn off the clk and hang M4. But even leaving clk_prepare_enable in probe,
>> if imx_rproc.c is built as module, clk_disable_unused will still turn
>> off the clk and hang M4.
>>
>> So for case 2.1, there is no good way to keep M4 clk not being turned off,
>> unless pass "clk_ignore_unused" in bootargs.
>>
>
>Isn't there something like an "always on" property for clocks?

There is CLK_IS_CRITICAL flag that could be added in clk driver, but this
is harcoded in clk driver. Using this flag means for case 2.2, there is no
chance to disable the clock when stop M4.

There is no device tree property to indicate a clk is always on as I know.

Regards,
Peng
>
>>
>> For case 2.2, you could use the clock entry to enable the clock, but actually
>> SCFW will handle the clock automatically when power on M4.
>>
>> If you have concern on the clk here, you may considering the various cases
>> and choose which to touch the clk, which to ignore the clk, but not
>> "clk get and clk prepare" for all cases in current imx_rproc.c implementation.
>>
>> Regards,
>> Peng
>>
>>
>> >
>> >[1] https://lore.kernel.org/lkml/20250404141713.ac2ntcsjsf7epdfa@hiago-nb/
>> >
>> >Cheers,
>> >Hiago.
>> >
>> >>
>> >> Daniel and Iuliana, I'd like to have your opinions on this.
>> >>
>> >> Thanks,
>> >> Mathieu
>> >>
>> >> >    if (IS_ERR(priv->clk)) {
>> >> >            dev_err(dev, "Failed to get clock\n");
>> >> >            return PTR_ERR(priv->clk);
>> >> > --
>> >> > 2.39.5
>> >> >

Powered by blists - more mailing lists