Re: [PATCH] remoteproc: imx_rproc: replace devm_clk_get() with devm_clk_get_optional()

[Hiago De Franco <hiagofranco@gmail.com>]

On Wed, Apr 30, 2025 at 02:08:35PM +0800, Peng Fan wrote:
> On Mon, Apr 28, 2025 at 02:12:57PM -0300, Hiago De Franco wrote:
> >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@toradex.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@toradex.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@colibri-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>;
> 
> From hardware perspective, this is correct. But i.MX8QXP has
> SCFW which manages the system resources. For this clock, when
> M4_0_PID0 is powered up, SCFW will not allow clk_prepare_enable to
> enable the clock, the error return will be LOCKED if user continue
> to send the enable request. When SCFW powers up M4, it will automatically
> configure the clock as I said before.
> 
> Set rate is still allowed by SCFW, even enable API return failure, but I think
> there is no such requirement.
> 
> So,
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 74299af1d7f1..627e57a88db2 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -1029,8 +1029,8 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
>         struct device *dev = priv->dev;
>         int ret;
> 
> -       /* Remote core is not under control of Linux */
> -       if (dcfg->method == IMX_RPROC_NONE)
> +       /* Remote core is not under control of Linux or it is managed by SCU API */
> +       if (dcfg->method == IMX_RPROC_NONE || dcfg->method == IMX_RPROC_SCU_API)
>                 return 0;
> 
>         priv->clk = devm_clk_get(dev, NULL);

Got it, thanks for the information. I will prepare the V2 with this
change.

> 
> 
> 
> Regards,
> Peng
> 
> >		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
> >> >> > 

Cheers,
Hiago.