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 <[email protected]> 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 <[email protected]> > >> >> > > >> >> > 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 <[email protected]> > >> >> > --- > >> >> > 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.
