Re: [U-Boot] [PATCHv1] mx53ppd: Change UART clock divider for high baudrate
Hi Fabio,
First a disclaimer. I no longer work for the client that had me make this change. I also no longer have any iMX53 hardware to do any kind of tests. But I'm happy to help out with this email.
Hi Rick and Sebastian,
On Thu, Feb 15, 2018 at 2:15 PM, Sebastian Reichel sebastian.reichel@collabora.co.uk wrote:
From: Rick Bronson rick@efn.org
Allow higher UART baud rates for communications to ACHC at 13.5 MBaud
What does ACHC mean?
ACHC is an product acronym, probably should change that sentence to:
Allow higher UART baud rates for communications up to 13.5 MBaud
Done in u-boot since it changes a divider common to all UARTs, including the console. If it were done in Linux we would get garbled characters during the handoff from u-boot to the kernel.
It would be nice to explain a bit how you achieve the higher baud rates.
Background: PLL2 runs at 400 MHz, PLL3 runs at 216 MHz. One of these PLL's are fed to a UART divider, the output of which sets the maximum UART baud rates that are based on these rates divided by 16.
Before patch: PLL3 (400 MHz) for UART's, divide by 6, get 66.66 MHz from it. Max baud rate is 4.166 MBaud.
After patch: PLL2 (216 MHz), divide by 1, get 216 MHz from it. Max baud rate is 13.5 MBaud.
The last I saw this code was 2014 so it's a little fuzzy. Not sure why I didn't keep PLL3 and just change the divider from 6 to 3, that would have allowed a max of 8.33 MBaud and at the time, I was just looking for at least 8 MBaud.
Looks like you don't use PLL2 as the UART clock anyomore.
That's correct, see above.
Please expland a bit further on the new UART clock sheme.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.co.uk
arch/arm/mach-imx/mx5/lowlevel_init.S | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm/mach-imx/mx5/lowlevel_init.S
b/arch/arm/mach-imx/mx5/lowlevel_init.S
index f5bc6728b7c2..ee4c3afd2fed 100644 --- a/arch/arm/mach-imx/mx5/lowlevel_init.S +++ b/arch/arm/mach-imx/mx5/lowlevel_init.S @@ -319,11 +319,13 @@ setup_pll_func: ldr r1, =0x00016154 str r1, [r0, #CLKCTL_CBCMR]
+#if !defined(CONFIG_TARGET_MX53PPD)
It would be better if this option could be a generic one, something like CONFIG_MX53_SERIAL_HIGHBAUD, so that other boads could use it if needed.
Yes it probably would.
Cheers,
Rick
Hi Rick,
On Sat, Feb 17, 2018 at 9:24 PM, Rick Bronson rick@efn.org wrote:
It would be nice to explain a bit how you achieve the higher baud rates.
Background: PLL2 runs at 400 MHz, PLL3 runs at 216 MHz. One of these PLL's are fed to a UART divider, the output of which sets the maximum UART baud rates that are based on these rates divided by 16.
Before patch: PLL3 (400 MHz) for UART's, divide by 6, get 66.66 MHz from it. Max baud rate is 4.166 MBaud.
After patch: PLL2 (216 MHz), divide by 1, get 216 MHz from it. Max baud rate is 13.5 MBaud.
Thanks for the clarification.
Maybe Sebastian can send a v2 with this explanation incorporated.
Regards,
Fabio Estevam
Hi Rick,
On 18/02/2018 00:24, Rick Bronson wrote:
Hi Fabio,
First a disclaimer. I no longer work for the client that had me make this change. I also no longer have any iMX53 hardware to do any kind of tests. But I'm happy to help out with this email.
Thanks for your support !
Hi Rick and Sebastian,
On Thu, Feb 15, 2018 at 2:15 PM, Sebastian Reichel sebastian.reichel@collabora.co.uk wrote:
From: Rick Bronson rick@efn.org
Allow higher UART baud rates for communications to ACHC at 13.5 MBaud
What does ACHC mean?
ACHC is an product acronym, probably should change that sentence to:
Allow higher UART baud rates for communications up to 13.5 MBaud
ok, understood. The whole commit message is misleading, and it is not clear what the patch is supposed to address.
Done in u-boot since it changes a divider common to all UARTs, including the console. If it were done in Linux we would get garbled characters during the handoff from u-boot to the kernel.
It would be nice to explain a bit how you achieve the higher baud rates.
Background: PLL2 runs at 400 MHz, PLL3 runs at 216 MHz. One of these PLL's are fed to a UART divider, the output of which sets the maximum UART baud rates that are based on these rates divided by 16.
ok
Before patch: PLL3 (400 MHz) for UART's, divide by 6, get 66.66 MHz from it. Max baud rate is 4.166 MBaud.
Generally enough for a console....
After patch: PLL2 (216 MHz), divide by 1, get 216 MHz from it. Max baud rate is 13.5 MBaud.
The last I saw this code was 2014 so it's a little fuzzy. Not sure why I didn't keep PLL3 and just change the divider from 6 to 3, that would have allowed a max of 8.33 MBaud and at the time, I was just looking for at least 8 MBaud.
ok, understood. In any case, it looks to me that this requirement is very specific to this target (even if I have not got why we cannot set the parent clock in the DT in linux, it would be my first choice). It should be implemented into board code, and not in lowlevelinit with some #ifdef. We already spent a lot of effort to reduce nasty compiler switch in common code.
Looks like you don't use PLL2 as the UART clock anyomore.
That's correct, see above.
Please expland a bit further on the new UART clock sheme.
Signed-off-by: Sebastian Reichel sebastian.reichel@collabora.co.uk
arch/arm/mach-imx/mx5/lowlevel_init.S | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm/mach-imx/mx5/lowlevel_init.S
b/arch/arm/mach-imx/mx5/lowlevel_init.S
index f5bc6728b7c2..ee4c3afd2fed 100644 --- a/arch/arm/mach-imx/mx5/lowlevel_init.S +++ b/arch/arm/mach-imx/mx5/lowlevel_init.S @@ -319,11 +319,13 @@ setup_pll_func: ldr r1, =0x00016154 str r1, [r0, #CLKCTL_CBCMR]
+#if !defined(CONFIG_TARGET_MX53PPD)
It would be better if this option could be a generic one, something like CONFIG_MX53_SERIAL_HIGHBAUD, so that other boads could use it if needed.
Yes it probably would.
Anyway, I have doubt this is useful. U-Boot is just a bootloader, and the high baud protocol is running in Linux. It is not used in U-Boot, because U-Boot needs just a console. The well known approach in kernel is to not rely to the bootloader (I know there are some exceptions..). Kernel sets all peripherals and does not bother what the bootloader did or didn't. This is really a good approach and makes no sense to add some kind of dependency here.
Best regards, Stefano Babic
participants (3)
-
Fabio Estevam -
Rick Bronson -
Stefano Babic