Patchwork Bug in drivers/serial/of_serial.c?

login
register
mail settings
Submitter Alon Ziv
Date Nov. 15, 2009, 9:30 a.m.
Message ID <8B957E110B62714A84290A01A597805F05CA0AA0@Exchange.discretix.com>
Download mbox | patch
Permalink /patch/38446/
State Superseded
Headers show

Comments

Alon Ziv - Nov. 15, 2009, 9:30 a.m.
As of commit eedacbf0, drivers/serial/of_serial.c forces the serial core
to recognize the xps-uart16550-2.00.b as an NS16550 (which had no FIFO).
Prior to this commit, the kernel's auto-configuration logic would
correctly recognize the UART as NS16550A (with 16-byte FIFO).

The following patch corrects the issue in what I believe to be the
correct way:
---
Xilinx 16550 UART is actually 16550A-compatible

+	{ .type = "serial", .compatible = "ns16550",  .data = (void
*)PORT_16550A, },
 	{ .type = "serial", .compatible = "ns16750",  .data = (void
*)PORT_16750, },
 	{ .type = "serial", .compatible = "ns16850",  .data = (void
*)PORT_16850, },
 #ifdef CONFIG_SERIAL_OF_PLATFORM_NWPSERIAL
**********************************************************************************************
IMPORTANT: The contents of this email and any attachments are confidential. They are intended for the 
named recipient(s) only.
If you have received this email in error, please notify the system manager or the sender immediately and do 
not disclose the contents to anyone or make copies thereof.
Arnd Bergmann - Nov. 16, 2009, 8:09 a.m.
On Sunday 15 November 2009, Alon Ziv wrote:
> *)PORT_16450, },
> -       { .type = "serial", .compatible = "ns16550",  .data = (void *)PORT_16550, },
> +       { .type = "serial", .compatible = "ns16550",  .data = (void *)PORT_16550A, },

Does not seem logical. If the device claims compatibility with ns16550, we should
not automatically assume it's an ns16550a. Why not add another line for 

{ .type = "serial", .compatible = "ns16550a",  .data = (void *)PORT_16550A, },

	Arnd <><
Alon Ziv - Nov. 19, 2009, 12:47 p.m.
Hi,

On Monday, November 16, 2009, Arnd wrote:
> > -       { .type = "serial", .compatible = "ns16550",  .data = (void
*)PORT_16550, },
> > +       { .type = "serial", .compatible = "ns16550",  .data = (void
*)PORT_16550A, },
> 
> Does not seem logical. If the device claims compatibility with
ns16550, we should
> not automatically assume it's an ns16550a. Why not add another line
for 
> 

Unfortunately, there is no way to change what the device claims--it's
encoded into the OpenFirmware tree by the EDK tools.
And, in any case, the device is actually not lying: it _is_ compatible
with NS16550--just with a non-buggy one.  Unfortunately the kernel
driver for 8250-class UARTs makes the conservative choice to assume any
16550 is one of the (early, buggy) revisions where the FIFO was
non-functional; any 16550 with working UART is classed as a 16550A.

-az
**********************************************************************************************
IMPORTANT: The contents of this email and any attachments are confidential. They are intended for the 
named recipient(s) only.
If you have received this email in error, please notify the system manager or the sender immediately and do 
not disclose the contents to anyone or make copies thereof.
Arnd Bergmann - Nov. 19, 2009, 1:01 p.m.
On Thursday 19 November 2009, Alon Ziv wrote:
> On Monday, November 16, 2009, Arnd wrote:
> > > -       { .type = "serial", .compatible = "ns16550",  .data = (void
> *)PORT_16550, },
> > > +       { .type = "serial", .compatible = "ns16550",  .data = (void
> *)PORT_16550A, },
> > 
> > Does not seem logical. If the device claims compatibility with
> ns16550, we should
> > not automatically assume it's an ns16550a. Why not add another line
> for 
> > 
> 
> Unfortunately, there is no way to change what the device claims--it's
> encoded into the OpenFirmware tree by the EDK tools.
> And, in any case, the device is actually not lying: it is compatible
> with NS16550--just with a non-buggy one.  Unfortunately the kernel
> driver for 8250-class UARTs makes the conservative choice to assume any
> 16550 is one of the (early, buggy) revisions where the FIFO was
> non-functional; any 16550 with working UART is classed as a 16550A.

In that case, add another entry for the device encoded in the firmware
itself. The ns16550 entry should be the second one after a more specific
one telling which device it is exactly.

	Arnd <><
Stephen Neuendorffer - Nov. 19, 2009, 5:20 p.m.
> -----Original Message-----
> From: linuxppc-dev-bounces+stephen=neuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen=neuendorffer.name@lists.ozlabs.org] On Behalf Of Alon
Ziv
> Sent: Thursday, November 19, 2009 4:47 AM
> To: Arnd Bergmann; linuxppc-dev@lists.ozlabs.org
> Subject: RE: Bug in drivers/serial/of_serial.c?
> 
> Hi,
> 
> On Monday, November 16, 2009, Arnd wrote:
> > > -       { .type = "serial", .compatible = "ns16550",  .data =
(void
> *)PORT_16550, },
> > > +       { .type = "serial", .compatible = "ns16550",  .data =
(void
> *)PORT_16550A, },
> >
> > Does not seem logical. If the device claims compatibility with
> ns16550, we should
> > not automatically assume it's an ns16550a. Why not add another line
> for
> >
> 
> Unfortunately, there is no way to change what the device claims--it's
> encoded into the OpenFirmware tree by the EDK tools.
> And, in any case, the device is actually not lying: it _is_ compatible
> with NS16550--just with a non-buggy one.  Unfortunately the kernel
> driver for 8250-class UARTs makes the conservative choice to assume
any
> 16550 is one of the (early, buggy) revisions where the FIFO was
> non-functional; any 16550 with working UART is classed as a 16550A.

Definitely changing what is generated by EDK seems to make sense here...

Steve


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Patch

diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 02406ba..78267af 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -161,7 +161,7 @@  static int of_platform_serial_remove(struct
of_device *ofdev)
 static struct of_device_id __devinitdata of_platform_serial_table[] = {
 	{ .type = "serial", .compatible = "ns8250",   .data = (void
*)PORT_8250, },
 	{ .type = "serial", .compatible = "ns16450",  .data = (void
*)PORT_16450, },
-	{ .type = "serial", .compatible = "ns16550",  .data = (void
*)PORT_16550, },