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

[Vladimir Zapolskiy <vz@mleia.com>]

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