ARM: add serial.h and set BASE_BAUD to 0

Commit Message

Rafał Miłecki Sept. 20, 2018, 8:13 p.m. UTC

From: Rafał Miłecki <[e-mail]>

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: [e-mail] # v4.14+
Signed-off-by: Rafał Miłecki <[e-mail]>
---
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

Comments

Russell King (Oracle) Sept. 21, 2018, 1:59 p.m. UTC | #1

On Thu, Sep 20, 2018 at 10:13:57PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <[e-mail]>
> 
> 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.

Rafał Miłecki Sept. 21, 2018, 2:36 p.m. UTC | #2

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 <[e-mail]>
>> 
>> 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.

Patch

diff --git a/arch/arm/include/asm/serial.h b/arch/arm/include/asm/serial.h
new file mode 100644
index 000000000000..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 */