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] [day] [month] [year] [list] 
Message-ID: <20250428171257.276bqhaupe4ksu5l@hiago-nb>
Date: Mon, 28 Apr 2025 14:12:57 -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 Sat, Apr 26, 2025 at 09:49:58PM +0800, Peng Fan 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.

Is this correct? I added this clock entry and also updated the clk
drivers to handle this option:

diff --git a/drivers/clk/imx/clk-imx8qxp-rsrc.c b/drivers/clk/imx/clk-imx8qxp-rsrc.c
index 585c425524a4..69c6f1711667 100644
--- a/drivers/clk/imx/clk-imx8qxp-rsrc.c
+++ b/drivers/clk/imx/clk-imx8qxp-rsrc.c
@@ -58,6 +58,7 @@ static const u32 imx8qxp_clk_scu_rsrc_table[] = {
        IMX_SC_R_NAND,
        IMX_SC_R_LVDS_0,
        IMX_SC_R_LVDS_1,
+       IMX_SC_R_M4_0_PID0,
        IMX_SC_R_M4_0_UART,
        IMX_SC_R_M4_0_I2C,
        IMX_SC_R_ELCDIF_PLL,
diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
index 3ae162625bb1..be6dfe0a5b97 100644
--- a/drivers/clk/imx/clk-imx8qxp.c
+++ b/drivers/clk/imx/clk-imx8qxp.c
@@ -142,6 +142,7 @@ static int imx8qxp_clk_probe(struct platform_device *pdev)
        imx_clk_scu("a35_clk", IMX_SC_R_A35, IMX_SC_PM_CLK_CPU);
        imx_clk_scu("a53_clk", IMX_SC_R_A53, IMX_SC_PM_CLK_CPU);
        imx_clk_scu("a72_clk", IMX_SC_R_A72, IMX_SC_PM_CLK_CPU);
+       imx_clk_scu("cm40_clk", IMX_SC_R_M4_0_PID0, IMX_SC_PM_CLK_CPU);

        /* LSIO SS */
        imx_clk_scu("pwm0_clk", IMX_SC_R_PWM_0, IMX_SC_PM_CLK_PER);


However I am seeing a permission denied (-13) from the imx_rproc:

root@...ibri-imx8x-07308754:~# dmesg | grep rproc
[    0.489113] imx-rproc imx8x-cm4: Failed to enable clock
[    0.489644] imx-rproc imx8x-cm4: probe with driver imx-rproc failed with error -13
[    0.489708] remoteproc remoteproc0: releasing imx-rproc

	imx8x-cm4 {
		compatible = "fsl,imx8qxp-cm4";
		clocks = <&clk IMX_SC_R_M4_0_PID0 IMX_SC_PM_CLK_CPU>;
		mbox-names = "tx", "rx", "rxdb";
		mboxes = <&lsio_mu5 0 1
			  &lsio_mu5 1 1
			  &lsio_mu5 3 1>;
		memory-region = <&vdev0buffer>, <&vdev0vring0>, <&vdev0vring1>,
				<&vdev1vring0>, <&vdev1vring1>, <&rsc_table>;
		power-domains = <&pd IMX_SC_R_M4_0_PID0>,
				<&pd IMX_SC_R_M4_0_MU_1A>;
		fsl,entry-address = <0x34fe0000>;
		fsl,resource-id = <IMX_SC_R_M4_0_PID0>;
	};

Am I missing something?

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ