[PATCH v2] clk: lpc32xx: add HCLK PLL output configuration

[PATCH v2] clk: lpc32xx: add HCLK PLL output configuration

[slemieux.tyco]

From: Sylvain Lemieux <slemieux@tycoint.com>

This patch add the support to setup the HCLK PLL output
using the "assigned-clock-rates" parameter in the device tree.

If the option is not use, the clock setup by the kickstart
and/or bootloader remain unchanged.

The previous kernel version did not change the clock frequency
output setup by the kickstart and/or bootloader;
this version always setup the clock frequency output to 208MHz.

Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
---
Changes from v1 to v2:
- Rename patch title;
  was "clk: lpc32xx: add clock frequency output configuration"
- Update the patch as per the feedback received from:
  Stephen: http://permalink.gmane.org/gmane.linux.kernel.clk/3913
  Vladimir: http://permalink.gmane.org/gmane.linux.kernel.clk/3921 

Note:
- There is currently an issue in the current driver;
  if the HCLK PLL output, configured by the kickstart and/or
  bootloader, is change by the kernel (ex. 266.5MHz to 208MHz),
  the serial console is no longer outputing properly.

 drivers/clk/nxp/clk-lpc32xx.c             | 6 +-----
 include/dt-bindings/clock/lpc32xx-clock.h | 1 +
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/nxp/clk-lpc32xx.c b/drivers/clk/nxp/clk-lpc32xx.c
index 981ff0d..48b3a11 100644
--- a/drivers/clk/nxp/clk-lpc32xx.c
+++ b/drivers/clk/nxp/clk-lpc32xx.c
@@ -87,7 +87,7 @@ enum {
 
 enum {
 	/* Start from the last defined clock in dt bindings */
-	LPC32XX_CLK_ADC_DIV = LPC32XX_CLK_ADC + 1,
+	LPC32XX_CLK_ADC_DIV = LPC32XX_CLK_HCLK_PLL + 1,
 	LPC32XX_CLK_ADC_RTC,
 	LPC32XX_CLK_TEST1,
 	LPC32XX_CLK_TEST2,
@@ -96,7 +96,6 @@ enum {
 	LPC32XX_CLK_OSC,
 	LPC32XX_CLK_SYS,
 	LPC32XX_CLK_PLL397X,
-	LPC32XX_CLK_HCLK_PLL,
 	LPC32XX_CLK_HCLK_DIV_PERIPH,
 	LPC32XX_CLK_HCLK_DIV,
 	LPC32XX_CLK_HCLK,
@@ -1526,9 +1525,6 @@ static void __init lpc32xx_clk_init(struct device_node *np)
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
 
-	/* For 13MHz osc valid output range of PLL is from 156MHz to 266.5MHz */
-	clk_set_rate(clk[LPC32XX_CLK_HCLK_PLL], 208000000);
-
 	/* Set 48MHz rate of USB PLL clock */
 	clk_set_rate(clk[LPC32XX_CLK_USB_PLL], 48000000);
 
diff --git a/include/dt-bindings/clock/lpc32xx-clock.h b/include/dt-bindings/clock/lpc32xx-clock.h
index bcb1c9a..d41b6fe 100644
--- a/include/dt-bindings/clock/lpc32xx-clock.h
+++ b/include/dt-bindings/clock/lpc32xx-clock.h
@@ -47,6 +47,7 @@
 #define LPC32XX_CLK_PWM1	32
 #define LPC32XX_CLK_PWM2	33
 #define LPC32XX_CLK_ADC		34
+#define LPC32XX_CLK_HCLK_PLL	35
 
 /* LPC32XX USB clocks */
 #define LPC32XX_USB_CLK_I2C	1
-- 
1.8.3.1

Re: [PATCH v2] clk: lpc32xx: add HCLK PLL output configuration

[Stephen Boyd]

On 02/10, slemieux.tyco-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Sylvain Lemieux <slemieux-1xCVI8+nB4ZBDgjK7y7TUQ@public.gmane.org>
> 
> This patch add the support to setup the HCLK PLL output
> using the "assigned-clock-rates" parameter in the device tree.
> 
> If the option is not use, the clock setup by the kickstart
> and/or bootloader remain unchanged.
> 
> The previous kernel version did not change the clock frequency
> output setup by the kickstart and/or bootloader;
> this version always setup the clock frequency output to 208MHz.
> 
> Signed-off-by: Sylvain Lemieux <slemieux-1xCVI8+nB4ZBDgjK7y7TUQ@public.gmane.org>
> ---

I couldn't find any usage of this driver in the tree so I just
applied this to clk-next. If it needs to go into some immutable
branch please let me know.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clk: lpc32xx: add HCLK PLL output configuration

[Vladimir Zapolskiy]

Hi Sylvain,

On 10.02.2016 20:52, slemieux.tyco@gmail.com wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>
> 
> This patch add the support to setup the HCLK PLL output
> using the "assigned-clock-rates" parameter in the device tree.
> 
> If the option is not use, the clock setup by the kickstart
> and/or bootloader remain unchanged.
> 
> The previous kernel version did not change the clock frequency
> output setup by the kickstart and/or bootloader;
> this version always setup the clock frequency output to 208MHz.
> 
> Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>

I've found enough time to test the change and it looks good, so


Acked-by: Vladimir Zapolskiy <vz@mleia.com>



Do you have any objections, if 208MHz clock is set by default
in the shared DTSI file i.e.

diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
index c58d8da..ecbace8 100644
--- a/arch/arm/boot/dts/lpc32xx.dtsi
+++ b/arch/arm/boot/dts/lpc32xx.dtsi
@@ -294,6 +294,9 @@
 
 					clocks = <&xtal_32k>, <&xtal>;
 					clock-names = "xtal_32k", "xtal";
+
+					assigned-clocks = <&clk LPC32XX_CLK_HCLK_PLL>;
+					assigned-clock-rates = <208000000>;
 				};
 			};
 
?

In particular board files the value can be overwritten, however I think
it is important to have some well defined clock value here to mitigate
a risk of possible clock rate change done by a bootloader, for example
the rate may be set to a lower value by a bootloader.

> ---
> Changes from v1 to v2:
> - Rename patch title;
>   was "clk: lpc32xx: add clock frequency output configuration"
> - Update the patch as per the feedback received from:
>   Stephen: http://permalink.gmane.org/gmane.linux.kernel.clk/3913
>   Vladimir: http://permalink.gmane.org/gmane.linux.kernel.clk/3921 
> 
> Note:
> - There is currently an issue in the current driver;
>   if the HCLK PLL output, configured by the kickstart and/or
>   bootloader, is change by the kernel (ex. 266.5MHz to 208MHz),
>   the serial console is no longer outputing properly.

yep, I'm aware of this issue too, however it is not obvious that
the rootcause is in the clock driver, so we can discuss this topic
somewhere else, but let me present a short intro about what I
see on my end.

In case if PCLK is 266 MHz / 16 ~ 16520833 Hz, UART pre-divider is
bypassed (X = 1, Y = 1), baudrate is 115200, then I observe corrupted
I/O but correctly set divisor latch registers (DLM = 0, DLL = 9):

  16520833 / 16 / 9 ~ 114728

But if I change DLL to 7 as I should do in assumption
that PCLK is 13 MHz, then UART I/O is *not* corrupted.

The same happens if I modify UART pre-divider, for example UART
clock rate is 16520833, baudrate is 115200, X = 27, Y = 242,
DLM = 0, DLL = 1:

  16520833 / 16 * 27 / 242 ~ 115202 -- but I/O is corrupted

And if I change to X = 19, Y = 134 as I should do in case of 
PCLK = 13 MHz, then console UART is working fine.

It looks like UART clock is always pinned to 13 MHz (how is it
possible technically?), but this contradicts to the datasheet IMHO.

--
With best wishes,
Vladimir

Re: [PATCH v2] clk: lpc32xx: add HCLK PLL output configuration

[Sylvain Lemieux]

Hi Vladimir,

On Wed, 2016-03-02 at 08:19 +0200, Vladimir Zapolskiy wrote:
> Hi Sylvain,
> 
> On 10.02.2016 20:52, slemieux.tyco@gmail.com wrote:
> > From: Sylvain Lemieux <slemieux@tycoint.com>
> > 
> > This patch add the support to setup the HCLK PLL output
> > using the "assigned-clock-rates" parameter in the device tree.
> > 
> > If the option is not use, the clock setup by the kickstart
> > and/or bootloader remain unchanged.
> > 
> > The previous kernel version did not change the clock frequency
> > output setup by the kickstart and/or bootloader;
> > this version always setup the clock frequency output to 208MHz.
> > 
> > Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> 
> I've found enough time to test the change and it looks good, so
> 
> 
> Acked-by: Vladimir Zapolskiy <vz@mleia.com>
> 
> 
> 
> Do you have any objections, if 208MHz clock is set by default
> in the shared DTSI file i.e.
> 
I do not have any objection;
you can add my Acked-by when you submit the patch.

> diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
> index c58d8da..ecbace8 100644
> --- a/arch/arm/boot/dts/lpc32xx.dtsi
> +++ b/arch/arm/boot/dts/lpc32xx.dtsi
> @@ -294,6 +294,9 @@
>  
>  					clocks = <&xtal_32k>, <&xtal>;
>  					clock-names = "xtal_32k", "xtal";
> +
> +					assigned-clocks = <&clk LPC32XX_CLK_HCLK_PLL>;
> +					assigned-clock-rates = <208000000>;
>  				};
>  			};
>  
> ?
> 
> In particular board files the value can be overwritten, however I think
> it is important to have some well defined clock value here to mitigate
> a risk of possible clock rate change done by a bootloader, for example
> the rate may be set to a lower value by a bootloader.
> 
> > ---
> > Changes from v1 to v2:
> > - Rename patch title;
> >   was "clk: lpc32xx: add clock frequency output configuration"
> > - Update the patch as per the feedback received from:
> >   Stephen: http://permalink.gmane.org/gmane.linux.kernel.clk/3913
> >   Vladimir: http://permalink.gmane.org/gmane.linux.kernel.clk/3921 
> > 
> > Note:
> > - There is currently an issue in the current driver;
> >   if the HCLK PLL output, configured by the kickstart and/or
> >   bootloader, is change by the kernel (ex. 266.5MHz to 208MHz),
> >   the serial console is no longer outputing properly.
> 
> yep, I'm aware of this issue too, however it is not obvious that
> the rootcause is in the clock driver, so we can discuss this topic
> somewhere else, but let me present a short intro about what I
> see on my end.
> 
> In case if PCLK is 266 MHz / 16 ~ 16520833 Hz, UART pre-divider is
> bypassed (X = 1, Y = 1), baudrate is 115200, then I observe corrupted
> I/O but correctly set divisor latch registers (DLM = 0, DLL = 9):
> 
>   16520833 / 16 / 9 ~ 114728
> 
> But if I change DLL to 7 as I should do in assumption
> that PCLK is 13 MHz, then UART I/O is *not* corrupted.
> 
> The same happens if I modify UART pre-divider, for example UART
> clock rate is 16520833, baudrate is 115200, X = 27, Y = 242,
> DLM = 0, DLL = 1:
> 
>   16520833 / 16 * 27 / 242 ~ 115202 -- but I/O is corrupted
> 
> And if I change to X = 19, Y = 134 as I should do in case of 
> PCLK = 13 MHz, then console UART is working fine.
> 
> It looks like UART clock is always pinned to 13 MHz (how is it
> possible technically?), but this contradicts to the datasheet IMHO.
> 
> --
> With best wishes,
> Vladimir