linux-kernel - Re: [PATCH] remoteproc: imx_rproc: replace devm_clk_get() with devm_clk_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANLsYkzLZKHpwv+Zz7YqtU4NCy7ZmapuzpgtfxsRfoV=Ve8rVg@mail.gmail.com>
Date: Sat, 26 Apr 2025 15:47:50 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Peng Fan <peng.fan@....nxp.com>
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, 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.

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

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