Re: [PATCH] ARM: add serial.h and set BASE_BAUD to 0
On 2018-09-21 15:59, Russell King - ARM Linux wrote:
On Thu, Sep 20, 2018 at 10:13:57PM +0200, Rafał Miłecki wrote:
From: Rafał Miłecki
For years arm has been using serial.h from asm-generic which sets
BASE_BAUD value to the (1843200 / 16). This is incorrect as:
1) This value obviously isn't correct for all devices
2) There are no device specific serial.h with
CONFIG_ARCH_MULTIPLATFORM
This goes back a long way. The BASE_BAUD definition is the base baud
for 8250 compatible UARTs _only_, no others.
However, as of 182ead3e418a ("earlycon: Remove hardcoded port->uartclk
initialization in of_setup_earlycon"), port->uartclk is no longer
initialised using BASE_BAUD. As acknowledged in 814453adea7d
("earlycon: Initialize port->uartclk based on clock-frequency
property")
the initialisation using BASE_BAUD was bogus, and there is now the
clock-frequency DT property which should be present to set this up.
Thanks a lot for looking at that. I was not aware of the commit
182ead3e418a ("earlycon: Remove hardcoded port->uartclk initialization
in of_setup_earlycon"), too bad it wasn't marked for stable.
I simply assumed that
port->uartclk = BASE_BAUD * 16;
is correct and that has poisoned all my further reasoning.
I've just confirmed that backporting 182ead3e418a to the 4.14 fixes the
problem for me (it results in the same 8250_early.c behavior as setting
BASE_BAUD to 0).
I'm planning to backport both:
182ead3e418a ("earlycon: Remove hardcoded port->uartclk initialization
in of_setup_earlycon")
814453adea7d ("earlycon: Initialize port->uartclk based on
clock-frequency property")
to 4.14+ as they fix regression triggered (not to say "caused") by the
commit 31cb9a8575ca0 ("earlycon: initialise baud field of earlycon
device structure").
Please DROP my serial.h patch and thanks again.
Now, setting BASE_BAUD to zero as per your patch will break the
8250 serial driver - this relies on BASE_BAUD being set to the
current value, and yes, ARM hardware that uses this will break. So
this is not a solution.
The only solution is that BASE_BAUD must not be abused - it must not
be used by non-8250 hardware, and thankfully there are already
solutions in the kernel (such as clock-frequency) to allow the clock
rate to be specified.
Somehow my serial console kept working with BASE_BAUD set to 0. I'm not
sure why.
Re: [PATCH] ARM: add serial.h and set BASE_BAUD to 0
On Thu, Sep 20, 2018 at 10:13:57PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> For years arm has been using serial.h from asm-generic which sets
> BASE_BAUD value to the (1843200 / 16). This is incorrect as:
> 1) This value obviously isn't correct for all devices
> 2) There are no device specific serial.h with CONFIG_ARCH_MULTIPLATFORM
This goes back a long way. The BASE_BAUD definition is the base baud
for 8250 compatible UARTs _only_, no others.
However, as of 182ead3e418a ("earlycon: Remove hardcoded port->uartclk
initialization in of_setup_earlycon"), port->uartclk is no longer
initialised using BASE_BAUD. As acknowledged in 814453adea7d
("earlycon: Initialize port->uartclk based on clock-frequency property")
the initialisation using BASE_BAUD was bogus, and there is now the
clock-frequency DT property which should be present to set this up.
Now, setting BASE_BAUD to zero as per your patch will break the
8250 serial driver - this relies on BASE_BAUD being set to the
current value, and yes, ARM hardware that uses this will break. So
this is not a solution.
The only solution is that BASE_BAUD must not be abused - it must not
be used by non-8250 hardware, and thankfully there are already
solutions in the kernel (such as clock-frequency) to allow the clock
rate to be specified.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
[PATCH] ARM: add serial.h and set BASE_BAUD to 0
From: Rafał Miłecki
For years arm has been using serial.h from asm-generic which sets
BASE_BAUD value to the (1843200 / 16). This is incorrect as:
1) This value obviously isn't correct for all devices
2) There are no device specific serial.h with CONFIG_ARCH_MULTIPLATFORM
That value breaks early serial console e.g. on Broadcom Northstar boards
(CONFIG_ARCH_BCM_5301X) which need BASE_BAUD = (99532800 / 16).
The problem remained unnoticed until 4.14 and the commit 31cb9a8575ca0
("earlycon: initialise baud field of earlycon device structure").
Above commit started filling "baud" in the "struct earlycon_device" (it
seems to be a correct behavior) and changed behavior of the 8250/16550
early console driver. Starting with 4.14 kernel's 8250_early.c sets UART
divisor if both: "uartclk" and "baud" are set. Having wrong value for
BASE_BAUD (and so "uartclk") results in broken console setup.
Solution to that has been introduced in commit 0ff3ab701963 ("serial:
8250_early: Only set divisor if valid clk & baud") and requires setting
BASE_BAUD (and so "uartclk") to 0.
A change similar to this action has been pushed for MIPS in the commit
c8ec2041f549 ("MIPS: Add custom serial.h with BASE_BAUD override for
generic kernel"). Another platform using 0 for BASE_BAUD is s390.
Fixes: 31cb9a8575ca ("earlycon: initialise baud field of earlycon device
structure")
Cc: sta...@vger.kernel.org # v4.14+
Signed-off-by: Rafał Miłecki
---
This problem has been originally reported by me in the e-mail:
ns16550 earlycon baud broken on BCM5301X since commit 31cb9a8575ca0 ("earlycon:
initialise baud field of earlycon device structure")
https://www.spinics.net/lists/arm-kernel/msg644295.html
---
arch/arm/include/asm/serial.h | 8
1 file changed, 8 insertions(+)
create mode 100644 arch/arm/include/asm/serial.h
diff --git a/arch/arm/include/asm/serial.h b/arch/arm/include/asm/serial.h
new file mode 100644
index ..e12f262290ad
--- /dev/null
+++ b/arch/arm/include/asm/serial.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_ARM_SERIAL_H
+#define _ASM_ARM_SERIAL_H
+
+/* ARM kernels support multiple devices so there isn't a single valid value. */
+#define BASE_BAUD 0
+
+#endif /* _ASM_ARM_SERIAL_H */
--
2.13.7
