Message-ID: <20250428141026.pvbk5qrgflv4wkak@hiago-nb>
Date: Mon, 28 Apr 2025 11:10:26 -0300
From: Hiago De Franco <hiagofranco@...il.com>
To: Peng Fan <peng.fan@....nxp.com>
Cc: Mathieu Poirier <mathieu.poirier@...aro.org>, 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 Sun, Apr 27, 2025 at 10:08:25AM +0800, Peng Fan wrote:
> 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.

Thanks for the information Peng. I think the way forward is clear now, I
will prepare the patches to address the 2.1 use case (bootaux).

> 
> >
> >>
> >> 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
> >> >> >

Cheers,
Hiago.

Powered by blists - more mailing lists