[5/6] clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
diff mbox series

Message ID 20190705045612.27665-5-Anson.Huang@nxp.com
State Superseded, archived
Delegated to: Eduardo Valentin
Headers show
Series
  • [1/6] thermal: qoriq: Use devm_platform_ioremap_resource() instead of of_iomap()
Related show

Commit Message

Anson Huang July 5, 2019, 4:56 a.m. UTC
From: Anson Huang <Anson.Huang@nxp.com>

IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
should manage this clock, so no need to have CLK_IS_CRITICAL flag
set.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/clk/imx/clk-imx8mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Abel Vesa July 5, 2019, 7:01 a.m. UTC | #1
On 19-07-05 12:56:11, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
> should manage this clock, so no need to have CLK_IS_CRITICAL flag
> set.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> ---
>  drivers/clk/imx/clk-imx8mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index d407a07..91de69a 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -539,7 +539,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>  	clks[IMX8MQ_CLK_DISP_AXI_ROOT]  = imx_clk_gate2_shared2("disp_axi_root_clk", "disp_axi", base + 0x45d0, 0, &share_count_dcss);
>  	clks[IMX8MQ_CLK_DISP_APB_ROOT]  = imx_clk_gate2_shared2("disp_apb_root_clk", "disp_apb", base + 0x45d0, 0, &share_count_dcss);
>  	clks[IMX8MQ_CLK_DISP_RTRM_ROOT] = imx_clk_gate2_shared2("disp_rtrm_root_clk", "disp_rtrm", base + 0x45d0, 0, &share_count_dcss);
> -	clks[IMX8MQ_CLK_TMU_ROOT] = imx_clk_gate4_flags("tmu_root_clk", "ipg_root", base + 0x4620, 0, CLK_IS_CRITICAL);
> +	clks[IMX8MQ_CLK_TMU_ROOT] = imx_clk_gate4("tmu_root_clk", "ipg_root", base + 0x4620, 0);
>  	clks[IMX8MQ_CLK_VPU_DEC_ROOT] = imx_clk_gate2_flags("vpu_dec_root_clk", "vpu_bus", base + 0x4630, 0, CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
>  	clks[IMX8MQ_CLK_CSI1_ROOT] = imx_clk_gate4("csi1_root_clk", "csi1_core", base + 0x4650, 0);
>  	clks[IMX8MQ_CLK_CSI2_ROOT] = imx_clk_gate4("csi2_root_clk", "csi2_core", base + 0x4660, 0);
> -- 
> 2.7.4
>
Stephen Boyd July 22, 2019, 9:31 p.m. UTC | #2
Quoting Anson.Huang@nxp.com (2019-07-04 21:56:11)
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
> should manage this clock, so no need to have CLK_IS_CRITICAL flag
> set.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>
Shawn Guo July 23, 2019, 2:50 a.m. UTC | #3
On Fri, Jul 05, 2019 at 12:56:11PM +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
> should manage this clock, so no need to have CLK_IS_CRITICAL flag
> set.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Applied, thanks.
Daniel Baluta July 26, 2019, 10:26 p.m. UTC | #4
Hi all,

latest linux-next hangs at boot.

commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD -> master, tag:
next-20190726, origin/master, origin/HEAD)
Author: Stephen Rothwell <sfr@canb.auug.org.au>
Date:   Fri Jul 26 15:18:02 2019 +1000

    Add linux-next specific files for 20190726

    Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>


I know this is crazy but reverting commit:

commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef (refs/bisect/bad)
Author: Anson Huang <Anson.Huang@nxp.com>
Date:   Fri Jul 5 12:56:11 2019 +0800

    clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT

    IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
    should manage this clock, so no need to have CLK_IS_CRITICAL flag
    set.



makes the boot work again.

Any idea?

On Fri, Jul 5, 2019 at 8:07 AM <Anson.Huang@nxp.com> wrote:
>
> From: Anson Huang <Anson.Huang@nxp.com>
>
> IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
> should manage this clock, so no need to have CLK_IS_CRITICAL flag
> set.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/clk/imx/clk-imx8mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index d407a07..91de69a 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -539,7 +539,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>         clks[IMX8MQ_CLK_DISP_AXI_ROOT]  = imx_clk_gate2_shared2("disp_axi_root_clk", "disp_axi", base + 0x45d0, 0, &share_count_dcss);
>         clks[IMX8MQ_CLK_DISP_APB_ROOT]  = imx_clk_gate2_shared2("disp_apb_root_clk", "disp_apb", base + 0x45d0, 0, &share_count_dcss);
>         clks[IMX8MQ_CLK_DISP_RTRM_ROOT] = imx_clk_gate2_shared2("disp_rtrm_root_clk", "disp_rtrm", base + 0x45d0, 0, &share_count_dcss);
> -       clks[IMX8MQ_CLK_TMU_ROOT] = imx_clk_gate4_flags("tmu_root_clk", "ipg_root", base + 0x4620, 0, CLK_IS_CRITICAL);
> +       clks[IMX8MQ_CLK_TMU_ROOT] = imx_clk_gate4("tmu_root_clk", "ipg_root", base + 0x4620, 0);
>         clks[IMX8MQ_CLK_VPU_DEC_ROOT] = imx_clk_gate2_flags("vpu_dec_root_clk", "vpu_bus", base + 0x4630, 0, CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
>         clks[IMX8MQ_CLK_CSI1_ROOT] = imx_clk_gate4("csi1_root_clk", "csi1_core", base + 0x4650, 0);
>         clks[IMX8MQ_CLK_CSI2_ROOT] = imx_clk_gate4("csi2_root_clk", "csi2_core", base + 0x4660, 0);
> --
> 2.7.4
>
Anson Huang July 27, 2019, 6:19 a.m. UTC | #5
Hi, Daniel

> Subject: Re: [PATCH 5/6] clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> IMX8MQ_CLK_TMU_ROOT
> 
> Hi all,
> 
> latest linux-next hangs at boot.
> 
> commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD -> master, tag:
> next-20190726, origin/master, origin/HEAD)
> Author: Stephen Rothwell <sfr@canb.auug.org.au>
> Date:   Fri Jul 26 15:18:02 2019 +1000
> 
>     Add linux-next specific files for 20190726
> 
>     Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> 
> 
> I know this is crazy but reverting commit:
> 
> commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef (refs/bisect/bad)
> Author: Anson Huang <Anson.Huang@nxp.com>
> Date:   Fri Jul 5 12:56:11 2019 +0800
> 
>     clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
> 
>     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
>     should manage this clock, so no need to have CLK_IS_CRITICAL flag
>     set.
> 
> 
> 
> makes the boot work again.
> 
> Any idea?

I just found if disabling SDMA1, then kernel can boot up, it does NOT make sense
TMU clock is related to SDMA1, I will check with design and get back to you soon.

Anson
Daniel Baluta July 27, 2019, 6:33 a.m. UTC | #6
On Sat, Jul 27, 2019 at 9:19 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> Hi, Daniel
>
> > Subject: Re: [PATCH 5/6] clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > IMX8MQ_CLK_TMU_ROOT
> >
> > Hi all,
> >
> > latest linux-next hangs at boot.
> >
> > commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD -> master, tag:
> > next-20190726, origin/master, origin/HEAD)
> > Author: Stephen Rothwell <sfr@canb.auug.org.au>
> > Date:   Fri Jul 26 15:18:02 2019 +1000
> >
> >     Add linux-next specific files for 20190726
> >
> >     Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> >
> >
> > I know this is crazy but reverting commit:
> >
> > commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef (refs/bisect/bad)
> > Author: Anson Huang <Anson.Huang@nxp.com>
> > Date:   Fri Jul 5 12:56:11 2019 +0800
> >
> >     clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
> >
> >     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
> >     should manage this clock, so no need to have CLK_IS_CRITICAL flag
> >     set.
> >
> >
> >
> > makes the boot work again.
> >
> > Any idea?
>
> I just found if disabling SDMA1, then kernel can boot up, it does NOT make sense
> TMU clock is related to SDMA1, I will check with design and get back to you soon.
>

Hi Anson,

Applying Abel's patch:

commit 8816c47db6a82f55bb4d64f62fd9dd3af680f0e4 (HEAD -> master)
Author: Abel Vesa <abel.vesa@nxp.com>
Date:   Tue Jun 25 12:01:56 2019 +0300

    clk: imx8mq: Mark AHB clock as critical

    Keep the AHB clock always on since there is no driver to control it and
    all the other clocks that use it as parent rely on it being always enabled.



The kernel boots up again.

It make some sense. I don't understand though why having
IMX8MQ_CLK_TMU_ROOT as critical also "unhangs" the kernel.

thanks,
Daniel.
Abel Vesa July 27, 2019, 4:17 p.m. UTC | #7
On 19-07-27 09:33:10, Daniel Baluta wrote:
> On Sat, Jul 27, 2019 at 9:19 AM Anson Huang <anson.huang@nxp.com> wrote:
> >
> > Hi, Daniel
> >
> > > Subject: Re: [PATCH 5/6] clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > IMX8MQ_CLK_TMU_ROOT
> > >
> > > Hi all,
> > >
> > > latest linux-next hangs at boot.
> > >
> > > commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD -> master, tag:
> > > next-20190726, origin/master, origin/HEAD)
> > > Author: Stephen Rothwell <sfr@canb.auug.org.au>
> > > Date:   Fri Jul 26 15:18:02 2019 +1000
> > >
> > >     Add linux-next specific files for 20190726
> > >
> > >     Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > >
> > >
> > > I know this is crazy but reverting commit:
> > >
> > > commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef (refs/bisect/bad)
> > > Author: Anson Huang <Anson.Huang@nxp.com>
> > > Date:   Fri Jul 5 12:56:11 2019 +0800
> > >
> > >     clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
> > >
> > >     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
> > >     should manage this clock, so no need to have CLK_IS_CRITICAL flag
> > >     set.
> > >
> > >
> > >
> > > makes the boot work again.
> > >
> > > Any idea?
> >
> > I just found if disabling SDMA1, then kernel can boot up, it does NOT make sense
> > TMU clock is related to SDMA1, I will check with design and get back to you soon.
> >
> 
> Hi Anson,
> 
> Applying Abel's patch:
> 
> commit 8816c47db6a82f55bb4d64f62fd9dd3af680f0e4 (HEAD -> master)
> Author: Abel Vesa <abel.vesa@nxp.com>
> Date:   Tue Jun 25 12:01:56 2019 +0300
> 
>     clk: imx8mq: Mark AHB clock as critical
> 
>     Keep the AHB clock always on since there is no driver to control it and
>     all the other clocks that use it as parent rely on it being always enabled.
> 
> 
> 
> The kernel boots up again.
> 
> It make some sense. I don't understand though why having
> IMX8MQ_CLK_TMU_ROOT as critical also "unhangs" the kernel.
> 

OK, so this is how it works.

By removing the critical flag from TMU, the AHB doesn't stay always on.
With my patch the AHB is marked as critical and therefore stays on.

The sdma1_clk has as parent the ipg_root which in turn has as parent the
ahb clock. And I think what happens is some read from the sdma registers hangs
because, for whatever reason, enabling the sdma1_clk doesn't propagate to enable
the ahb clock. I might be wrong though.

> thanks,
> Daniel.
Guido Günther July 27, 2019, 6:26 p.m. UTC | #8
Hi Daniel,
On Sat, Jul 27, 2019 at 01:26:50AM +0300, Daniel Baluta wrote:
> Hi all,
> 
> latest linux-next hangs at boot.
> 
> commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD -> master, tag:
> next-20190726, origin/master, origin/HEAD)
> Author: Stephen Rothwell <sfr@canb.auug.org.au>
> Date:   Fri Jul 26 15:18:02 2019 +1000
> 
>     Add linux-next specific files for 20190726
> 
>     Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> 
> 
> I know this is crazy but reverting commit:
> 
> commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef (refs/bisect/bad)
> Author: Anson Huang <Anson.Huang@nxp.com>
> Date:   Fri Jul 5 12:56:11 2019 +0800
> 
>     clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
> 
>     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
>     should manage this clock, so no need to have CLK_IS_CRITICAL flag
>     set.
> 
> 
> 
> makes the boot work again.

I noticed a boot hang yesterday on next-20190726 when loading the
qoriq_thermal which I worked around by blacklisting it. The
fsl,imx8mq-tmu node specifies a clock (IMX8MQ_CLK_TMU_ROOT) but does not
seem to enable, shouldn't it do so?

Cheers,
 -- Guido
Fabio Estevam July 27, 2019, 8:17 p.m. UTC | #9
Hi Guido,

On Sat, Jul 27, 2019 at 3:26 PM Guido Günther <agx@sigxcpu.org> wrote:

> I noticed a boot hang yesterday on next-20190726 when loading the
> qoriq_thermal which I worked around by blacklisting it. The
> fsl,imx8mq-tmu node specifies a clock (IMX8MQ_CLK_TMU_ROOT) but does not
> seem to enable, shouldn't it do so?

Yes, I think you are right.

I don't have access to a imx8mq board at the moment, but something
like below would probably help:
http://code.bulix.org/pd88jp-812381

If it helps, I can send it as a formal patch.

Regards,

Fabio Estevam
Guido Günther July 28, 2019, 7:58 a.m. UTC | #10
Hi Fabio,
On Sat, Jul 27, 2019 at 05:17:50PM -0300, Fabio Estevam wrote:
> Hi Guido,
> 
> On Sat, Jul 27, 2019 at 3:26 PM Guido Günther <agx@sigxcpu.org> wrote:
> 
> > I noticed a boot hang yesterday on next-20190726 when loading the
> > qoriq_thermal which I worked around by blacklisting it. The
> > fsl,imx8mq-tmu node specifies a clock (IMX8MQ_CLK_TMU_ROOT) but does not
> > seem to enable, shouldn't it do so?
> 
> Yes, I think you are right.
> 
> I don't have access to a imx8mq board at the moment, but something
> like below would probably help:
> http://code.bulix.org/pd88jp-812381
> 
> If it helps, I can send it as a formal patch.

Yes, this fixes it for me, thanks!
 -- Guido
Fabio Estevam July 28, 2019, 3:24 p.m. UTC | #11
Hi Guido,

On Sun, Jul 28, 2019 at 4:59 AM Guido Günther <agx@sigxcpu.org> wrote:

> Yes, this fixes it for me, thanks!

Thanks for testing it.

I will send a formal patch tomorrow.

Regards,

Fabio Estevam
Anson Huang July 29, 2019, 1:29 a.m. UTC | #12
Hi, Abel/Daniel

> On 19-07-27 09:33:10, Daniel Baluta wrote:
> > On Sat, Jul 27, 2019 at 9:19 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> > >
> > > Hi, Daniel
> > >
> > > > Subject: Re: [PATCH 5/6] clk: imx8mq: Remove CLK_IS_CRITICAL flag
> > > > for IMX8MQ_CLK_TMU_ROOT
> > > >
> > > > Hi all,
> > > >
> > > > latest linux-next hangs at boot.
> > > >
> > > > commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD ->
> master, tag:
> > > > next-20190726, origin/master, origin/HEAD)
> > > > Author: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > Date:   Fri Jul 26 15:18:02 2019 +1000
> > > >
> > > >     Add linux-next specific files for 20190726
> > > >
> > > >     Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > >
> > > >
> > > > I know this is crazy but reverting commit:
> > > >
> > > > commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef
> (refs/bisect/bad)
> > > > Author: Anson Huang <Anson.Huang@nxp.com>
> > > > Date:   Fri Jul 5 12:56:11 2019 +0800
> > > >
> > > >     clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > IMX8MQ_CLK_TMU_ROOT
> > > >
> > > >     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the
> driver
> > > >     should manage this clock, so no need to have CLK_IS_CRITICAL flag
> > > >     set.
> > > >
> > > >
> > > >
> > > > makes the boot work again.
> > > >
> > > > Any idea?
> > >
> > > I just found if disabling SDMA1, then kernel can boot up, it does
> > > NOT make sense TMU clock is related to SDMA1, I will check with design
> and get back to you soon.
> > >
> >
> > Hi Anson,
> >
> > Applying Abel's patch:
> >
> > commit 8816c47db6a82f55bb4d64f62fd9dd3af680f0e4 (HEAD -> master)
> > Author: Abel Vesa <abel.vesa@nxp.com>
> > Date:   Tue Jun 25 12:01:56 2019 +0300
> >
> >     clk: imx8mq: Mark AHB clock as critical
> >
> >     Keep the AHB clock always on since there is no driver to control it and
> >     all the other clocks that use it as parent rely on it being always enabled.
> >
> >
> >
> > The kernel boots up again.
> >
> > It make some sense. I don't understand though why having
> > IMX8MQ_CLK_TMU_ROOT as critical also "unhangs" the kernel.
> >
> 
> OK, so this is how it works.
> 
> By removing the critical flag from TMU, the AHB doesn't stay always on.
> With my patch the AHB is marked as critical and therefore stays on.
> 
> The sdma1_clk has as parent the ipg_root which in turn has as parent the
> ahb clock. And I think what happens is some read from the sdma registers
> hangs because, for whatever reason, enabling the sdma1_clk doesn't
> propagate to enable the ahb clock. I might be wrong though.
> 

I can explain why Abel's patch can fix this issue, the AHB clock is a MUST
always ON for system bus, while it does NOT have CLK_IS_CRITICAL flag set for now,
when SDMA1 is probed, it will enable its clock, and the clk path is sdma1_clk->ipg_root->ahb,
after SDMA1 probed done, it will disable its clock since no one use it, and it will trigger
the ahb clock to be OFF, as its usecount is added by 1 when probe and decreased by 1 after
SDMA1 probe done, so usecount=0 will trigger AHB clock to be OFF.

So I think the best solution should be applying Abel's patch as you mentioned upper, the TMU
clock patch is NOT the root cause, it just triggers this issue accidently☹

But I saw Abel's AHB patch is still under discussion, so I think we need to speed it up and make 
kernel boot up work for development. AHB/IPG are always critical for i.MX SoCs.

Thanks,
Anson.
Daniel Baluta July 29, 2019, 6:51 a.m. UTC | #13
On Mon, Jul 29, 2019 at 4:29 AM Anson Huang <anson.huang@nxp.com> wrote:
>
> Hi, Abel/Daniel
>
> > On 19-07-27 09:33:10, Daniel Baluta wrote:
> > > On Sat, Jul 27, 2019 at 9:19 AM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > >
> > > > Hi, Daniel
> > > >
> > > > > Subject: Re: [PATCH 5/6] clk: imx8mq: Remove CLK_IS_CRITICAL flag
> > > > > for IMX8MQ_CLK_TMU_ROOT
> > > > >
> > > > > Hi all,
> > > > >
> > > > > latest linux-next hangs at boot.
> > > > >
> > > > > commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD ->
> > master, tag:
> > > > > next-20190726, origin/master, origin/HEAD)
> > > > > Author: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > Date:   Fri Jul 26 15:18:02 2019 +1000
> > > > >
> > > > >     Add linux-next specific files for 20190726
> > > > >
> > > > >     Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > >
> > > > >
> > > > > I know this is crazy but reverting commit:
> > > > >
> > > > > commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef
> > (refs/bisect/bad)
> > > > > Author: Anson Huang <Anson.Huang@nxp.com>
> > > > > Date:   Fri Jul 5 12:56:11 2019 +0800
> > > > >
> > > > >     clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > IMX8MQ_CLK_TMU_ROOT
> > > > >
> > > > >     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the
> > driver
> > > > >     should manage this clock, so no need to have CLK_IS_CRITICAL flag
> > > > >     set.
> > > > >
> > > > >
> > > > >
> > > > > makes the boot work again.
> > > > >
> > > > > Any idea?
> > > >
> > > > I just found if disabling SDMA1, then kernel can boot up, it does
> > > > NOT make sense TMU clock is related to SDMA1, I will check with design
> > and get back to you soon.
> > > >
> > >
> > > Hi Anson,
> > >
> > > Applying Abel's patch:
> > >
> > > commit 8816c47db6a82f55bb4d64f62fd9dd3af680f0e4 (HEAD -> master)
> > > Author: Abel Vesa <abel.vesa@nxp.com>
> > > Date:   Tue Jun 25 12:01:56 2019 +0300
> > >
> > >     clk: imx8mq: Mark AHB clock as critical
> > >
> > >     Keep the AHB clock always on since there is no driver to control it and
> > >     all the other clocks that use it as parent rely on it being always enabled.
> > >
> > >
> > >
> > > The kernel boots up again.
> > >
> > > It make some sense. I don't understand though why having
> > > IMX8MQ_CLK_TMU_ROOT as critical also "unhangs" the kernel.
> > >
> >
> > OK, so this is how it works.
> >
> > By removing the critical flag from TMU, the AHB doesn't stay always on.
> > With my patch the AHB is marked as critical and therefore stays on.
> >
> > The sdma1_clk has as parent the ipg_root which in turn has as parent the
> > ahb clock. And I think what happens is some read from the sdma registers
> > hangs because, for whatever reason, enabling the sdma1_clk doesn't
> > propagate to enable the ahb clock. I might be wrong though.
> >
>
> I can explain why Abel's patch can fix this issue, the AHB clock is a MUST
> always ON for system bus, while it does NOT have CLK_IS_CRITICAL flag set for now,
> when SDMA1 is probed, it will enable its clock, and the clk path is sdma1_clk->ipg_root->ahb,
> after SDMA1 probed done, it will disable its clock since no one use it, and it will trigger
> the ahb clock to be OFF, as its usecount is added by 1 when probe and decreased by 1 after
> SDMA1 probe done, so usecount=0 will trigger AHB clock to be OFF.
>
> So I think the best solution should be applying Abel's patch as you mentioned upper, the TMU
> clock patch is NOT the root cause, it just triggers this issue accidently☹
>
> But I saw Abel's AHB patch is still under discussion, so I think we need to speed it up and make
> kernel boot up work for development. AHB/IPG are always critical for i.MX SoCs.

Thanks Anson,

Your explanation makes a lot of sense. We will take care today of Abel's patch.
What do you think about Fabio's patch? I also think this is a valid patch:

http://code.bulix.org/pd88jp-812381
Anson Huang July 29, 2019, 7:14 a.m. UTC | #14
Hi, Daniel

> On Mon, Jul 29, 2019 at 4:29 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> >
> > Hi, Abel/Daniel
> >
> > > On 19-07-27 09:33:10, Daniel Baluta wrote:
> > > > On Sat, Jul 27, 2019 at 9:19 AM Anson Huang <anson.huang@nxp.com>
> > > wrote:
> > > > >
> > > > > Hi, Daniel
> > > > >
> > > > > > Subject: Re: [PATCH 5/6] clk: imx8mq: Remove CLK_IS_CRITICAL
> > > > > > flag for IMX8MQ_CLK_TMU_ROOT
> > > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > latest linux-next hangs at boot.
> > > > > >
> > > > > > commit fde50b96be821ac9673a7e00847cc4605bd88f34 (HEAD ->
> > > master, tag:
> > > > > > next-20190726, origin/master, origin/HEAD)
> > > > > > Author: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > > Date:   Fri Jul 26 15:18:02 2019 +1000
> > > > > >
> > > > > >     Add linux-next specific files for 20190726
> > > > > >
> > > > > >     Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > >
> > > > > >
> > > > > > I know this is crazy but reverting commit:
> > > > > >
> > > > > > commit 431bdd1df48ee2896ea9980d9153e3aeaf0c81ef
> > > (refs/bisect/bad)
> > > > > > Author: Anson Huang <Anson.Huang@nxp.com>
> > > > > > Date:   Fri Jul 5 12:56:11 2019 +0800
> > > > > >
> > > > > >     clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > > IMX8MQ_CLK_TMU_ROOT
> > > > > >
> > > > > >     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the
> > > driver
> > > > > >     should manage this clock, so no need to have CLK_IS_CRITICAL
> flag
> > > > > >     set.
> > > > > >
> > > > > >
> > > > > >
> > > > > > makes the boot work again.
> > > > > >
> > > > > > Any idea?
> > > > >
> > > > > I just found if disabling SDMA1, then kernel can boot up, it
> > > > > does NOT make sense TMU clock is related to SDMA1, I will check
> > > > > with design
> > > and get back to you soon.
> > > > >
> > > >
> > > > Hi Anson,
> > > >
> > > > Applying Abel's patch:
> > > >
> > > > commit 8816c47db6a82f55bb4d64f62fd9dd3af680f0e4 (HEAD -> master)
> > > > Author: Abel Vesa <abel.vesa@nxp.com>
> > > > Date:   Tue Jun 25 12:01:56 2019 +0300
> > > >
> > > >     clk: imx8mq: Mark AHB clock as critical
> > > >
> > > >     Keep the AHB clock always on since there is no driver to control it and
> > > >     all the other clocks that use it as parent rely on it being always
> enabled.
> > > >
> > > >
> > > >
> > > > The kernel boots up again.
> > > >
> > > > It make some sense. I don't understand though why having
> > > > IMX8MQ_CLK_TMU_ROOT as critical also "unhangs" the kernel.
> > > >
> > >
> > > OK, so this is how it works.
> > >
> > > By removing the critical flag from TMU, the AHB doesn't stay always on.
> > > With my patch the AHB is marked as critical and therefore stays on.
> > >
> > > The sdma1_clk has as parent the ipg_root which in turn has as parent
> > > the ahb clock. And I think what happens is some read from the sdma
> > > registers hangs because, for whatever reason, enabling the sdma1_clk
> > > doesn't propagate to enable the ahb clock. I might be wrong though.
> > >
> >
> > I can explain why Abel's patch can fix this issue, the AHB clock is a
> > MUST always ON for system bus, while it does NOT have CLK_IS_CRITICAL
> > flag set for now, when SDMA1 is probed, it will enable its clock, and
> > the clk path is sdma1_clk->ipg_root->ahb, after SDMA1 probed done, it
> > will disable its clock since no one use it, and it will trigger the
> > ahb clock to be OFF, as its usecount is added by 1 when probe and
> > decreased by 1 after
> > SDMA1 probe done, so usecount=0 will trigger AHB clock to be OFF.
> >
> > So I think the best solution should be applying Abel's patch as you
> > mentioned upper, the TMU clock patch is NOT the root cause, it just
> > triggers this issue accidently☹
> >
> > But I saw Abel's AHB patch is still under discussion, so I think we
> > need to speed it up and make kernel boot up work for development.
> AHB/IPG are always critical for i.MX SoCs.
> 
> Thanks Anson,
> 
> Your explanation makes a lot of sense. We will take care today of Abel's
> patch.
> What do you think about Fabio's patch? I also think this is a valid patch:
> 
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fcode.b
> ulix.org%2Fpd88jp-
> 812381&amp;data=02%7C01%7Canson.huang%40nxp.com%7C23b4c21e3cbc
> 4fcf2a3c08d713f131a8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C636999798949622961&amp;sdata=Jx9B40rJKpQvakOCfx%2B3v80NTxPqUw
> D4pdGojQxVIoY%3D&amp;reserved=0

Hmm, when did Fabio sent out this patch? I can NOT find it...
I also have a patch in this series (#4/6) doing same thing on July 5th...

https://patchwork.kernel.org/patch/11032783/

Thanks.
Anson
Daniel Baluta July 29, 2019, 7:20 a.m. UTC | #15
<snip>
> > Your explanation makes a lot of sense. We will take care today of Abel's
> > patch.
> > What do you think about Fabio's patch? I also think this is a valid patch:
> >
<snip>

>
> Hmm, when did Fabio sent out this patch? I can NOT find it...
> I also have a patch in this series (#4/6) doing same thing on July 5th...
>
> https://patchwork.kernel.org/patch/11032783/

He didn't send the patch yet. It was just a request for test here:

http://code.bulix.org/pd88jp-812381

But, now I see is exactly like your patch here:
Daniel Baluta July 29, 2019, 7:21 a.m. UTC | #16
On Mon, Jul 29, 2019 at 10:20 AM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> <snip>
> > > Your explanation makes a lot of sense. We will take care today of Abel's
> > > patch.
> > > What do you think about Fabio's patch? I also think this is a valid patch:
> > >
> <snip>
>
> >
> > Hmm, when did Fabio sent out this patch? I can NOT find it...
> > I also have a patch in this series (#4/6) doing same thing on July 5th...
> >
> > https://patchwork.kernel.org/patch/11032783/
>
> He didn't send the patch yet. It was just a request for test here:
>
> http://code.bulix.org/pd88jp-812381
>
> But, now I see is exactly like your patch here:

... pressed send to early.

https://lkml.org/lkml/2019/7/5/19

We are all set then. Thanks Anson for clarifications!
Anson Huang July 29, 2019, 7:49 a.m. UTC | #17
Hi, Daniel

> On Mon, Jul 29, 2019 at 10:20 AM Daniel Baluta <daniel.baluta@gmail.com>
> wrote:
> >
> > <snip>
> > > > Your explanation makes a lot of sense. We will take care today of
> > > > Abel's patch.
> > > > What do you think about Fabio's patch? I also think this is a valid patch:
> > > >
> > <snip>
> >
> > >
> > > Hmm, when did Fabio sent out this patch? I can NOT find it...
> > > I also have a patch in this series (#4/6) doing same thing on July 5th...
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > >
> tchwork.kernel.org%2Fpatch%2F11032783%2F&amp;data=02%7C01%7Canso
> n.hu
> > >
> ang%40nxp.com%7C048d8695b3dc4ceef6bd08d713f55e8a%7C686ea1d3bc2
> b4c6fa
> > >
> 92cd99c5c301635%7C0%7C0%7C636999816880118674&amp;sdata=1HIMQ0l
> iKpEFS
> > > 6P2WSG%2FH9evspxIdxAvFmaklH1woDk%3D&amp;reserved=0
> >
> > He didn't send the patch yet. It was just a request for test here:
> >
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fcode.
> > bulix.org%2Fpd88jp-
> 812381&amp;data=02%7C01%7Canson.huang%40nxp.com%7C0
> >
> 48d8695b3dc4ceef6bd08d713f55e8a%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0
> > %7C0%7C636999816880118674&amp;sdata=p70mgCDucCgLJ8TTRMn3a%2
> Fk68FXGQeiR
> > FR0fVSV7Jlo%3D&amp;reserved=0
> >
> > But, now I see is exactly like your patch here:
> 
> ... pressed send to early.
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.o
> rg%2Flkml%2F2019%2F7%2F5%2F19&amp;data=02%7C01%7Canson.huang%
> 40nxp.com%7C048d8695b3dc4ceef6bd08d713f55e8a%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C636999816880118674&amp;sdata=ciWj8y
> WPvkIZ5ni%2BohZuaqAXf9Pb2FCWhp9GQhpMwAY%3D&amp;reserved=0
> 
> We are all set then. Thanks Anson for clarifications!

Thanks, so we are all clear about this issue, need to wait thermal maintainer to review
the rest patch in this series, but I did NOT receive any response from thermal sub-system
maintainer for really long time, NOT sure when the thermal patches can be accepted.

Anson
Daniel Baluta July 29, 2019, 8:13 a.m. UTC | #18
On Mon, Jul 29, 2019 at 10:49 AM Anson Huang <anson.huang@nxp.com> wrote:

> > We are all set then. Thanks Anson for clarifications!
>
> Thanks, so we are all clear about this issue, need to wait thermal maintainer to review
> the rest patch in this series, but I did NOT receive any response from thermal sub-system
> maintainer for really long time, NOT sure when the thermal patches can be accepted.

This is really unfortunate. I think it is safe to do a RESEND of the
patches as it has
been at least 3 weeks since your first send them.

Pick any reviewed-by you got and do a resend.

Patch
diff mbox series

diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index d407a07..91de69a 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -539,7 +539,7 @@  static int imx8mq_clocks_probe(struct platform_device *pdev)
 	clks[IMX8MQ_CLK_DISP_AXI_ROOT]  = imx_clk_gate2_shared2("disp_axi_root_clk", "disp_axi", base + 0x45d0, 0, &share_count_dcss);
 	clks[IMX8MQ_CLK_DISP_APB_ROOT]  = imx_clk_gate2_shared2("disp_apb_root_clk", "disp_apb", base + 0x45d0, 0, &share_count_dcss);
 	clks[IMX8MQ_CLK_DISP_RTRM_ROOT] = imx_clk_gate2_shared2("disp_rtrm_root_clk", "disp_rtrm", base + 0x45d0, 0, &share_count_dcss);
-	clks[IMX8MQ_CLK_TMU_ROOT] = imx_clk_gate4_flags("tmu_root_clk", "ipg_root", base + 0x4620, 0, CLK_IS_CRITICAL);
+	clks[IMX8MQ_CLK_TMU_ROOT] = imx_clk_gate4("tmu_root_clk", "ipg_root", base + 0x4620, 0);
 	clks[IMX8MQ_CLK_VPU_DEC_ROOT] = imx_clk_gate2_flags("vpu_dec_root_clk", "vpu_bus", base + 0x4630, 0, CLK_SET_RATE_PARENT | CLK_OPS_PARENT_ENABLE);
 	clks[IMX8MQ_CLK_CSI1_ROOT] = imx_clk_gate4("csi1_root_clk", "csi1_core", base + 0x4650, 0);
 	clks[IMX8MQ_CLK_CSI2_ROOT] = imx_clk_gate4("csi2_root_clk", "csi2_core", base + 0x4660, 0);