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