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

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

[Hiago De Franco]

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

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 (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to get clock\n");
 		return PTR_ERR(priv->clk);
-- 
2.39.5

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

[Mathieu Poirier]

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.

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
> 

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

[Hiago De Franco]

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.

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

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

[Mathieu Poirier]

On Wed, 23 Apr 2025 at 13:22, Hiago De Franco <hiagofranco@gmail.com> 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.
>

If commit 4f6c9832613b breaks on your platform then it should be reverted.

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

If we make the clock optional, what guarantees do we have that the
clock the M4 is sharing with other devices won't be switched off?  To
me a clock needs to be defined in the device tree and referenced in
the remote processor bindings.

Thanks,
Mathieu

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

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

[Peng Fan]

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.

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

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

[Mathieu Poirier]

On Sat, 26 Apr 2025 at 06:41, Peng Fan <peng.fan@oss.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@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.
>
> 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
> >> >

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

[Peng Fan]

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@oss.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@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.
>>
>> 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.

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

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

[Hiago De Franco]

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@oss.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@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.
> >>
> >> 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.

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

[Hiago De Franco]

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

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

[Peng Fan]

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



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

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

[Hiago De Franco]

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.