[PATCH 1/2] serial: 8250_early: Only set divisor if valid clk & baud

[PATCH 1/2] serial: 8250_early: Only set divisor if valid clk & baud

[Matt Redfearn]

If either uartclk or baud are 0, avoid calculating and setting a divisor
based on them since the output will almost certainly be garbage.

This also allows platforms such as the MIPS generic kernel, which has no
way to know a valid BASE_BASE for the board it is actually booted on at
compile time, to set BASE_BAUD to 0 and avoid early_8250 setting a bad
divisor.

This fixes a regression caused by commit 31cb9a8575ca ("earlycon:
initialise baud field of earlycon device structure"), which changed the
behavior of of_setup_earlycon such that it sets a baud rate in the
earlycon structure where previously it was left as 0. All boards
supported by the MIPS generic kernel started outputting garbage from the
boot console due to an incorrect divisor being set.

Fixes: 31cb9a8575ca ("earlycon: initialise baud field of earlycon device structure")
Cc: stable <stable@vger.kernel.org> # 4.14
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
---

 drivers/tty/serial/8250/8250_early.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_early.c b/drivers/tty/serial/8250/8250_early.c
index af72ec32e404..f135c1846477 100644
--- a/drivers/tty/serial/8250/8250_early.c
+++ b/drivers/tty/serial/8250/8250_early.c
@@ -125,12 +125,14 @@ static void __init init_port(struct earlycon_device *device)
 	serial8250_early_out(port, UART_FCR, 0);	/* no fifo */
 	serial8250_early_out(port, UART_MCR, 0x3);	/* DTR + RTS */
 
-	divisor = DIV_ROUND_CLOSEST(port->uartclk, 16 * device->baud);
-	c = serial8250_early_in(port, UART_LCR);
-	serial8250_early_out(port, UART_LCR, c | UART_LCR_DLAB);
-	serial8250_early_out(port, UART_DLL, divisor & 0xff);
-	serial8250_early_out(port, UART_DLM, (divisor >> 8) & 0xff);
-	serial8250_early_out(port, UART_LCR, c & ~UART_LCR_DLAB);
+	if (port->uartclk && device->baud) {
+		divisor = DIV_ROUND_CLOSEST(port->uartclk, 16 * device->baud);
+		c = serial8250_early_in(port, UART_LCR);
+		serial8250_early_out(port, UART_LCR, c | UART_LCR_DLAB);
+		serial8250_early_out(port, UART_DLL, divisor & 0xff);
+		serial8250_early_out(port, UART_DLM, (divisor >> 8) & 0xff);
+		serial8250_early_out(port, UART_LCR, c & ~UART_LCR_DLAB);
+	}
 }
 
 int __init early_serial8250_setup(struct earlycon_device *device,
-- 
2.7.4

[PATCH 2/2] MIPS: Add custom serial.h with BASE_BAUD override for generic kernel

[Matt Redfearn]

Add a custom serial.h header for MIPS, allowing platforms to override
the asm-generic version if required.

The generic platform uses this header to set BASE_BAUD to 0. The
generic platform supports multiple boards, which may have different
UART clocks. Also one of the boards supported is the Boston FPGA board,
where the UART clock depends on the loaded FPGA bitfile. As such there
is no way that the generic kernel can set a compile time default
BASE_BAUD.

Commit 31cb9a8575ca ("earlycon: initialise baud field of earlycon device
structure") changed the behavior of of_setup_earlycon such that any baud
rate set in the device tree is now set in the earlycon structure. The
UART driver will then calculate a divisor based on BASE_BAUD and set it.
With MIPS generic kernels this resulted in garbage output due to the
incorrect uart clock rate being used to calculate a divisor. This
commit, combined with "serial: 8250_early: Only set divisor if valid clk
& baud" prevents the earlycon code setting a bad divisor and restores
earlycon output.

Fixes: 31cb9a8575ca ("earlycon: initialise baud field of earlycon device structure")
Cc: stable <stable@vger.kernel.org> # 4.14
Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>

---

 arch/mips/include/asm/Kbuild   |  1 -
 arch/mips/include/asm/serial.h | 21 +++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100644 arch/mips/include/asm/serial.h

diff --git a/arch/mips/include/asm/Kbuild b/arch/mips/include/asm/Kbuild
index 7c8aab23bce8..b1f66699677d 100644
--- a/arch/mips/include/asm/Kbuild
+++ b/arch/mips/include/asm/Kbuild
@@ -16,7 +16,6 @@ generic-y += qrwlock.h
 generic-y += qspinlock.h
 generic-y += sections.h
 generic-y += segment.h
-generic-y += serial.h
 generic-y += trace_clock.h
 generic-y += unaligned.h
 generic-y += user.h
diff --git a/arch/mips/include/asm/serial.h b/arch/mips/include/asm/serial.h
new file mode 100644
index 000000000000..30be5cd8efdb
--- /dev/null
+++ b/arch/mips/include/asm/serial.h
@@ -0,0 +1,21 @@
+/*
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (C) 2017 MIPS Tech, LLC
+ */
+#ifndef __ASM__SERIAL_H
+#define __ASM__SERIAL_H
+
+#ifdef CONFIG_MIPS_GENERIC
+/*
+ * Generic kernels cannot know a correct value for all platforms at
+ * compile time. Set it to 0 to prevent 8250_early using it
+ */
+#define BASE_BAUD 0
+#else
+#include <asm-generic/serial.h>
+#endif
+
+#endif /* __ASM__SERIAL_H */
-- 
2.7.4

Re: [PATCH 2/2] MIPS: Add custom serial.h with BASE_BAUD override for generic kernel

[Greg Kroah-Hartman]

On Wed, Nov 22, 2017 at 09:57:29AM +0000, Matt Redfearn wrote:
> Add a custom serial.h header for MIPS, allowing platforms to override
> the asm-generic version if required.
> 
> The generic platform uses this header to set BASE_BAUD to 0. The
> generic platform supports multiple boards, which may have different
> UART clocks. Also one of the boards supported is the Boston FPGA board,
> where the UART clock depends on the loaded FPGA bitfile. As such there
> is no way that the generic kernel can set a compile time default
> BASE_BAUD.
> 
> Commit 31cb9a8575ca ("earlycon: initialise baud field of earlycon device
> structure") changed the behavior of of_setup_earlycon such that any baud
> rate set in the device tree is now set in the earlycon structure. The
> UART driver will then calculate a divisor based on BASE_BAUD and set it.
> With MIPS generic kernels this resulted in garbage output due to the
> incorrect uart clock rate being used to calculate a divisor. This
> commit, combined with "serial: 8250_early: Only set divisor if valid clk
> & baud" prevents the earlycon code setting a bad divisor and restores
> earlycon output.
> 
> Fixes: 31cb9a8575ca ("earlycon: initialise baud field of earlycon device structure")
> Cc: stable <stable@vger.kernel.org> # 4.14
> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
> 
> ---
> 
>  arch/mips/include/asm/Kbuild   |  1 -
>  arch/mips/include/asm/serial.h | 21 +++++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>  create mode 100644 arch/mips/include/asm/serial.h
> 
> diff --git a/arch/mips/include/asm/Kbuild b/arch/mips/include/asm/Kbuild
> index 7c8aab23bce8..b1f66699677d 100644
> --- a/arch/mips/include/asm/Kbuild
> +++ b/arch/mips/include/asm/Kbuild
> @@ -16,7 +16,6 @@ generic-y += qrwlock.h
>  generic-y += qspinlock.h
>  generic-y += sections.h
>  generic-y += segment.h
> -generic-y += serial.h
>  generic-y += trace_clock.h
>  generic-y += unaligned.h
>  generic-y += user.h
> diff --git a/arch/mips/include/asm/serial.h b/arch/mips/include/asm/serial.h
> new file mode 100644
> index 000000000000..30be5cd8efdb
> --- /dev/null
> +++ b/arch/mips/include/asm/serial.h
> @@ -0,0 +1,21 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.

Which version of the GPL?  As it is, this means "GPL v1 and all others".

I doubt you want that :)

thanks,

greg k-h

Re: [PATCH 2/2] MIPS: Add custom serial.h with BASE_BAUD override for generic kernel

[Matt Redfearn]

On 28/11/17 14:35, Greg Kroah-Hartman wrote:
> On Wed, Nov 22, 2017 at 09:57:29AM +0000, Matt Redfearn wrote:
>> Add a custom serial.h header for MIPS, allowing platforms to override
>> the asm-generic version if required.
>>
>> The generic platform uses this header to set BASE_BAUD to 0. The
>> generic platform supports multiple boards, which may have different
>> UART clocks. Also one of the boards supported is the Boston FPGA board,
>> where the UART clock depends on the loaded FPGA bitfile. As such there
>> is no way that the generic kernel can set a compile time default
>> BASE_BAUD.
>>
>> Commit 31cb9a8575ca ("earlycon: initialise baud field of earlycon device
>> structure") changed the behavior of of_setup_earlycon such that any baud
>> rate set in the device tree is now set in the earlycon structure. The
>> UART driver will then calculate a divisor based on BASE_BAUD and set it.
>> With MIPS generic kernels this resulted in garbage output due to the
>> incorrect uart clock rate being used to calculate a divisor. This
>> commit, combined with "serial: 8250_early: Only set divisor if valid clk
>> & baud" prevents the earlycon code setting a bad divisor and restores
>> earlycon output.
>>
>> Fixes: 31cb9a8575ca ("earlycon: initialise baud field of earlycon device structure")
>> Cc: stable <stable@vger.kernel.org> # 4.14
>> Signed-off-by: Matt Redfearn <matt.redfearn@mips.com>
>>
>> ---
>>
>>   arch/mips/include/asm/Kbuild   |  1 -
>>   arch/mips/include/asm/serial.h | 21 +++++++++++++++++++++
>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>   create mode 100644 arch/mips/include/asm/serial.h
>>
>> diff --git a/arch/mips/include/asm/Kbuild b/arch/mips/include/asm/Kbuild
>> index 7c8aab23bce8..b1f66699677d 100644
>> --- a/arch/mips/include/asm/Kbuild
>> +++ b/arch/mips/include/asm/Kbuild
>> @@ -16,7 +16,6 @@ generic-y += qrwlock.h
>>   generic-y += qspinlock.h
>>   generic-y += sections.h
>>   generic-y += segment.h
>> -generic-y += serial.h
>>   generic-y += trace_clock.h
>>   generic-y += unaligned.h
>>   generic-y += user.h
>> diff --git a/arch/mips/include/asm/serial.h b/arch/mips/include/asm/serial.h
>> new file mode 100644
>> index 000000000000..30be5cd8efdb
>> --- /dev/null
>> +++ b/arch/mips/include/asm/serial.h
>> @@ -0,0 +1,21 @@
>> +/*
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License.  See the file "COPYING" in the main directory of this archive
>> + * for more details.
> 
> Which version of the GPL?  As it is, this means "GPL v1 and all others".
> 
> I doubt you want that :)

Good point - thanks!

Matt

> 
> thanks,
> 
> greg k-h
>