clk: imx8mq: Mark AHB clock as critical
diff mbox series

Message ID 1561453316-11481-1-git-send-email-abel.vesa@nxp.com
State New
Headers show
Series
  • clk: imx8mq: Mark AHB clock as critical
Related show

Commit Message

Abel Vesa June 25, 2019, 9:01 a.m. UTC
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.

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

Comments

Stephen Boyd June 25, 2019, 10:32 p.m. UTC | #1
Quoting Abel Vesa (2019-06-25 02:01:56)
> 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.
> 
> Signed-off-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 5fbc2a7..b48268b 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -398,7 +398,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
>         clks[IMX8MQ_CLK_NOC_APB] = imx8m_clk_composite_critical("noc_apb", imx8mq_noc_apb_sels, base + 0x8d80);
>  
>         /* AHB */
> -       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite("ahb", imx8mq_ahb_sels, base + 0x9000);
> +       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite_critical("ahb", imx8mq_ahb_sels, base + 0x9000);

Please add a comment into the code why it's critical.

>         clks[IMX8MQ_CLK_AUDIO_AHB] = imx8m_clk_composite("audio_ahb", imx8mq_audio_ahb_sels, base + 0x9100);
>  
>         /* IPG */
> -- 
> 2.7.4
>
Abel Vesa July 5, 2019, 8:52 a.m. UTC | #2
On 19-06-25 15:32:22, Stephen Boyd wrote:
> Quoting Abel Vesa (2019-06-25 02:01:56)
> > 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.
> > 
> > Signed-off-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 5fbc2a7..b48268b 100644
> > --- a/drivers/clk/imx/clk-imx8mq.c
> > +++ b/drivers/clk/imx/clk-imx8mq.c
> > @@ -398,7 +398,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> >         clks[IMX8MQ_CLK_NOC_APB] = imx8m_clk_composite_critical("noc_apb", imx8mq_noc_apb_sels, base + 0x8d80);
> >  
> >         /* AHB */
> > -       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite("ahb", imx8mq_ahb_sels, base + 0x9000);
> > +       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite_critical("ahb", imx8mq_ahb_sels, base + 0x9000);
> 
> Please add a comment into the code why it's critical.

Comment explaining why the AHB bus clock is critical ?
Isn't that self-explanatory ?

> 
> >         clks[IMX8MQ_CLK_AUDIO_AHB] = imx8m_clk_composite("audio_ahb", imx8mq_audio_ahb_sels, base + 0x9100);
> >  
> >         /* IPG */
> > -- 
> > 2.7.4
> >
Stephen Boyd July 22, 2019, 9:25 p.m. UTC | #3
Quoting Abel Vesa (2019-07-05 01:52:19)
> On 19-06-25 15:32:22, Stephen Boyd wrote:
> > Quoting Abel Vesa (2019-06-25 02:01:56)
> > > 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.
> > > 
> > > Signed-off-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 5fbc2a7..b48268b 100644
> > > --- a/drivers/clk/imx/clk-imx8mq.c
> > > +++ b/drivers/clk/imx/clk-imx8mq.c
> > > @@ -398,7 +398,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> > >         clks[IMX8MQ_CLK_NOC_APB] = imx8m_clk_composite_critical("noc_apb", imx8mq_noc_apb_sels, base + 0x8d80);
> > >  
> > >         /* AHB */
> > > -       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite("ahb", imx8mq_ahb_sels, base + 0x9000);
> > > +       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite_critical("ahb", imx8mq_ahb_sels, base + 0x9000);
> > 
> > Please add a comment into the code why it's critical.
> 
> Comment explaining why the AHB bus clock is critical ?
> Isn't that self-explanatory ?

Nope, it isn't self-explanatory, because nothing on this line says "bus"
and it could be that someone reading this code isn't well versed in the
concepts of ARM world AHB to connect the two.

> 
> > 
> > >         clks[IMX8MQ_CLK_AUDIO_AHB] = imx8m_clk_composite("audio_ahb", imx8mq_audio_ahb_sels, base + 0x9100);
Daniel Baluta July 27, 2019, 6:30 a.m. UTC | #4
On Tue, Jul 23, 2019 at 6:17 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Abel Vesa (2019-07-05 01:52:19)
> > On 19-06-25 15:32:22, Stephen Boyd wrote:
> > > Quoting Abel Vesa (2019-06-25 02:01:56)
> > > > 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.
> > > >
> > > > Signed-off-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 5fbc2a7..b48268b 100644
> > > > --- a/drivers/clk/imx/clk-imx8mq.c
> > > > +++ b/drivers/clk/imx/clk-imx8mq.c
> > > > @@ -398,7 +398,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> > > >         clks[IMX8MQ_CLK_NOC_APB] = imx8m_clk_composite_critical("noc_apb", imx8mq_noc_apb_sels, base + 0x8d80);
> > > >
> > > >         /* AHB */
> > > > -       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite("ahb", imx8mq_ahb_sels, base + 0x9000);
> > > > +       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite_critical("ahb", imx8mq_ahb_sels, base + 0x9000);
> > >
> > > Please add a comment into the code why it's critical.
> >
> > Comment explaining why the AHB bus clock is critical ?
> > Isn't that self-explanatory ?
>
> Nope, it isn't self-explanatory, because nothing on this line says "bus"
> and it could be that someone reading this code isn't well versed in the
> concepts of ARM world AHB to connect the two.

Agree with Stephen. Commit message should try to give as much details
as possible
also maybe educate the readers who might not have that much knowledge.

Abel, I understand that for someone who works daily with this part of the kernel
this change might look trivial.

Also, without this patch linux-next hangs on imx8mq.

With the explanation added you can add my:

Tested-by: Daniel Baluta <daniel.baluta@nxp.com>

thanks,
Daniel.
Shawn Guo Aug. 3, 2019, 7:27 a.m. UTC | #5
On Sat, Jul 27, 2019 at 09:30:57AM +0300, Daniel Baluta wrote:
> On Tue, Jul 23, 2019 at 6:17 AM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Abel Vesa (2019-07-05 01:52:19)
> > > On 19-06-25 15:32:22, Stephen Boyd wrote:
> > > > Quoting Abel Vesa (2019-06-25 02:01:56)
> > > > > 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.
> > > > >
> > > > > Signed-off-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 5fbc2a7..b48268b 100644
> > > > > --- a/drivers/clk/imx/clk-imx8mq.c
> > > > > +++ b/drivers/clk/imx/clk-imx8mq.c
> > > > > @@ -398,7 +398,7 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> > > > >         clks[IMX8MQ_CLK_NOC_APB] = imx8m_clk_composite_critical("noc_apb", imx8mq_noc_apb_sels, base + 0x8d80);
> > > > >
> > > > >         /* AHB */
> > > > > -       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite("ahb", imx8mq_ahb_sels, base + 0x9000);
> > > > > +       clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite_critical("ahb", imx8mq_ahb_sels, base + 0x9000);
> > > >
> > > > Please add a comment into the code why it's critical.
> > >
> > > Comment explaining why the AHB bus clock is critical ?
> > > Isn't that self-explanatory ?
> >
> > Nope, it isn't self-explanatory, because nothing on this line says "bus"
> > and it could be that someone reading this code isn't well versed in the
> > concepts of ARM world AHB to connect the two.
> 
> Agree with Stephen. Commit message should try to give as much details
> as possible
> also maybe educate the readers who might not have that much knowledge.
> 
> Abel, I understand that for someone who works daily with this part of the kernel
> this change might look trivial.
> 
> Also, without this patch linux-next hangs on imx8mq.

How does that happen?  Mainline is fine there?

Shawn
Shawn Guo Aug. 3, 2019, 8 a.m. UTC | #6
On Sat, Aug 03, 2019 at 09:27:25AM +0200, Shawn Guo wrote:
> > Also, without this patch linux-next hangs on imx8mq.
> 
> How does that happen?  Mainline is fine there?

Okay, understood it by reading more threads.

Shawn

Patch
diff mbox series

diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index 5fbc2a7..b48268b 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -398,7 +398,7 @@  static int imx8mq_clocks_probe(struct platform_device *pdev)
 	clks[IMX8MQ_CLK_NOC_APB] = imx8m_clk_composite_critical("noc_apb", imx8mq_noc_apb_sels, base + 0x8d80);
 
 	/* AHB */
-	clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite("ahb", imx8mq_ahb_sels, base + 0x9000);
+	clks[IMX8MQ_CLK_AHB] = imx8m_clk_composite_critical("ahb", imx8mq_ahb_sels, base + 0x9000);
 	clks[IMX8MQ_CLK_AUDIO_AHB] = imx8m_clk_composite("audio_ahb", imx8mq_audio_ahb_sels, base + 0x9100);
 
 	/* IPG */