[PATCH] ARM: dts: imx6sl: correct PWM ipg clock source

[PATCH] ARM: dts: imx6sl: correct PWM ipg clock source

[Anson Huang]

From i.MX6SL Reference Manual, the PWMx's ipg clock
for registers access is from perclk, correct them.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm/boot/dts/imx6sl.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index e7524e7..4b4813f 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -338,7 +338,7 @@
 				compatible = "fsl,imx6sl-pwm", "fsl,imx27-pwm";
 				reg = <0x02080000 0x4000>;
 				interrupts = <0 83 IRQ_TYPE_LEVEL_HIGH>;
-				clocks = <&clks IMX6SL_CLK_PWM1>,
+				clocks = <&clks IMX6SL_CLK_PERCLK>,
 					 <&clks IMX6SL_CLK_PWM1>;
 				clock-names = "ipg", "per";
 			};
@@ -348,7 +348,7 @@
 				compatible = "fsl,imx6sl-pwm", "fsl,imx27-pwm";
 				reg = <0x02084000 0x4000>;
 				interrupts = <0 84 IRQ_TYPE_LEVEL_HIGH>;
-				clocks = <&clks IMX6SL_CLK_PWM2>,
+				clocks = <&clks IMX6SL_CLK_PERCLK>,
 					 <&clks IMX6SL_CLK_PWM2>;
 				clock-names = "ipg", "per";
 			};
@@ -358,7 +358,7 @@
 				compatible = "fsl,imx6sl-pwm", "fsl,imx27-pwm";
 				reg = <0x02088000 0x4000>;
 				interrupts = <0 85 IRQ_TYPE_LEVEL_HIGH>;
-				clocks = <&clks IMX6SL_CLK_PWM3>,
+				clocks = <&clks IMX6SL_CLK_PERCLK>,
 					 <&clks IMX6SL_CLK_PWM3>;
 				clock-names = "ipg", "per";
 			};
@@ -368,7 +368,7 @@
 				compatible = "fsl,imx6sl-pwm", "fsl,imx27-pwm";
 				reg = <0x0208c000 0x4000>;
 				interrupts = <0 86 IRQ_TYPE_LEVEL_HIGH>;
-				clocks = <&clks IMX6SL_CLK_PWM4>,
+				clocks = <&clks IMX6SL_CLK_PERCLK>,
 					 <&clks IMX6SL_CLK_PWM4>;
 				clock-names = "ipg", "per";
 			};
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: imx6sl: correct PWM ipg clock source

[Uwe Kleine-König]

[Cc: += linux-pwm]

Hello,

On Wed, Dec 19, 2018 at 05:41:31AM +0000, Anson Huang wrote:
> From i.MX6SL Reference Manual, the PWMx's ipg clock
> for registers access is from perclk, correct them.

I assume this is related to the patch "pwm: imx: add ipg clock
operation"? This patch doesn't fix a real problem because in practise
perclk is already enabled, right? (I don't question the patch, just
wonder how urgent it is.)

> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm/boot/dts/imx6sl.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
> index e7524e7..4b4813f 100644
> --- a/arch/arm/boot/dts/imx6sl.dtsi
> +++ b/arch/arm/boot/dts/imx6sl.dtsi
> @@ -338,7 +338,7 @@
>  				compatible = "fsl,imx6sl-pwm", "fsl,imx27-pwm";
>  				reg = <0x02080000 0x4000>;
>  				interrupts = <0 83 IRQ_TYPE_LEVEL_HIGH>;
> -				clocks = <&clks IMX6SL_CLK_PWM1>,
> +				clocks = <&clks IMX6SL_CLK_PERCLK>,
>  					 <&clks IMX6SL_CLK_PWM1>;
>  				clock-names = "ipg", "per";

It's a bit irritating that IMX6SL_CLK_PERCLK is used for the "ipg" clk,
not the "per" clk. Is this correct?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] ARM: dts: imx6sl: correct PWM ipg clock source

[Anson Huang]

Best Regards!
Anson Huang

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: 2018年12月19日 16:37
> To: Anson Huang <anson.huang@nxp.com>
> Cc: shawnguo@kernel.org; s.hauer@pengutronix.de; kernel@pengutronix.de;
> Fabio Estevam <fabio.estevam@nxp.com>; robh+dt@kernel.org;
> mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; linux-pwm@vger.kernel.org
> Subject: Re: [PATCH] ARM: dts: imx6sl: correct PWM ipg clock source
> 
> [Cc: += linux-pwm]
> 
> Hello,
> 
> On Wed, Dec 19, 2018 at 05:41:31AM +0000, Anson Huang wrote:
> > From i.MX6SL Reference Manual, the PWMx's ipg clock for registers
> > access is from perclk, correct them.
> 
> I assume this is related to the patch "pwm: imx: add ipg clock operation"? This
> patch doesn't fix a real problem because in practise perclk is already enabled,
> right? (I don't question the patch, just wonder how urgent it is.)

Yes, I met the kernel boot up hang on i.MX7D and look into the PWM module and
found that the ipg_clk_s is for register access, but different i.MX SoC could have
different ipg_clk_s clock source, such as on i.MX6QDL, it is from ipg clock, the rest
are from perclk, they are always ON, so no issue, but on i.MX7D, the ipg_clk_s of
PWM module is controllable in CCM CCGR, so we have to add the ipg clock operation
as well as perclk.

i.MX6SL current setting has no issue, because the ipg_clk_s is from perclk which is always
ON, but it does NOT match the clock info in reference manual, while other i.MX6 SoCs
are having correct clock settings for PWM IPG clock. 

It is just an improvement, NOT fix a real problem.

> 
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  arch/arm/boot/dts/imx6sl.dtsi | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/imx6sl.dtsi
> > b/arch/arm/boot/dts/imx6sl.dtsi index e7524e7..4b4813f 100644
> > --- a/arch/arm/boot/dts/imx6sl.dtsi
> > +++ b/arch/arm/boot/dts/imx6sl.dtsi
> > @@ -338,7 +338,7 @@
> >  				compatible = "fsl,imx6sl-pwm", "fsl,imx27-pwm";
> >  				reg = <0x02080000 0x4000>;
> >  				interrupts = <0 83 IRQ_TYPE_LEVEL_HIGH>;
> > -				clocks = <&clks IMX6SL_CLK_PWM1>,
> > +				clocks = <&clks IMX6SL_CLK_PERCLK>,
> >  					 <&clks IMX6SL_CLK_PWM1>;
> >  				clock-names = "ipg", "per";
> 
> It's a bit irritating that IMX6SL_CLK_PERCLK is used for the "ipg" clk, not the
> "per" clk. Is this correct?

Yes, you can check the i.MX6SL CCM chapter for PWM clocks, the ipg_clk_s
is from perclk and its enabled control is VCC which is always ON. The "per"
clock is for PWM function, NOT register access, I tried disabling the PWM per clock,
PWM registers still can be accessed.

Anson.

> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König
> |
> Industrial Linux Solutions                 |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C7bf
> fc7bad61545fb595508d6658d1d0f%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C636808054141733417&amp;sdata=dDv2spIwbQmkpu7mhEm
> 8bRHyKKviSO%2BQ33ZU7nD1bJQ%3D&amp;reserved=0  |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: imx6sl: correct PWM ipg clock source

[Uwe Kleine-König]

Hello,

On Wed, Dec 19, 2018 at 08:44:13AM +0000, Anson Huang wrote:
> i.MX6SL current setting has no issue, because the ipg_clk_s is from perclk which is always
> ON, but it does NOT match the clock info in reference manual, while other i.MX6 SoCs
> are having correct clock settings for PWM IPG clock. 
> 
> It is just an improvement, NOT fix a real problem.

OK, so it's merge window material as I expected.
 
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >  arch/arm/boot/dts/imx6sl.dtsi | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/imx6sl.dtsi
> > > b/arch/arm/boot/dts/imx6sl.dtsi index e7524e7..4b4813f 100644
> > > --- a/arch/arm/boot/dts/imx6sl.dtsi
> > > +++ b/arch/arm/boot/dts/imx6sl.dtsi
> > > @@ -338,7 +338,7 @@
> > >  				compatible = "fsl,imx6sl-pwm", "fsl,imx27-pwm";
> > >  				reg = <0x02080000 0x4000>;
> > >  				interrupts = <0 83 IRQ_TYPE_LEVEL_HIGH>;
> > > -				clocks = <&clks IMX6SL_CLK_PWM1>,
> > > +				clocks = <&clks IMX6SL_CLK_PERCLK>,
> > >  					 <&clks IMX6SL_CLK_PWM1>;
> > >  				clock-names = "ipg", "per";
> > 
> > It's a bit irritating that IMX6SL_CLK_PERCLK is used for the "ipg" clk, not the
> > "per" clk. Is this correct?
> 
> Yes, you can check the i.MX6SL CCM chapter for PWM clocks, the ipg_clk_s
> is from perclk and its enabled control is VCC which is always ON. The "per"
> clock is for PWM function, NOT register access, I tried disabling the PWM per clock,
> PWM registers still can be accessed.

OK, then Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

> > Industrial Linux Solutions                 |
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> > pengutronix.de%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7C7bf
> > fc7bad61545fb595508d6658d1d0f%7C686ea1d3bc2b4c6fa92cd99c5c301635
> > %7C0%7C0%7C636808054141733417&amp;sdata=dDv2spIwbQmkpu7mhEm
> > 8bRHyKKviSO%2BQ33ZU7nD1bJQ%3D&amp;reserved=0  |

haha, something between you and me mangles links. I bet it's on your
side.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: dts: imx6sl: correct PWM ipg clock source

[Shawn Guo]

On Wed, Dec 19, 2018 at 05:41:31AM +0000, Anson Huang wrote:
> From i.MX6SL Reference Manual, the PWMx's ipg clock
> for registers access is from perclk, correct them.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Applied, thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel