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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250426134958.GB13806@nxa18884-linux>
Date: Sat, 26 Apr 2025 21:49:58 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Hiago De Franco <hiagofranco@...il.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 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.

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.


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