Bug in drivers/serial/of_serial.c?

Bug in drivers/serial/of_serial.c?

[Alon Ziv]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

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

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, },
+	{ .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.

[-- Attachment #2: 03-of_serial-16550A.patch --]
[-- Type: application/octet-stream, Size: 1018 bytes --]

commit 79119cf800c51f43f7f5415250b99664bc57641d
Author: Alon Ziv <alonz@discretix.com>
Date:   Sun Nov 15 09:45:04 2009 +0200

    Xilinx 16550 UART is actually 16550A-compatible

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, },
+	{ .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

Re: Bug in drivers/serial/of_serial.c?

[Arnd Bergmann]

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 <><

RE: Bug in drivers/serial/of_serial.c?

[Alon Ziv]

Hi,

On Monday, November 16, 2009, Arnd wrote:
> > -       { .type =3D "serial", .compatible =3D "ns16550",  .data =3D (=
void
*)PORT_16550, },
> > +       { .type =3D "serial", .compatible =3D "ns16550",  .data =3D (=
void
*)PORT_16550A, },
>=20
> 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=20
>=20

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 confidentia=
l. They are intended for the=20
named recipient(s) only.
If you have received this email in error, please notify the system manage=
r or the sender immediately and do=20
not disclose the contents to anyone or make copies thereof.

Re: Bug in drivers/serial/of_serial.c?

[Arnd Bergmann]

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 <><

RE: Bug in drivers/serial/of_serial.c?

[Alon Ziv]

Hi,

On Thursday, November 19, 2009, Arnd wrote:
> 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.
>=20

Is the following better?

---
[PATCH] Xilinx 16550 UART is actually 16550A-compatible

diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 02406ba..40bf8f4 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -161,6 +161,7 @@ static int of_platform_serial_remove(struct
of_device *ofdev)
 static struct of_device_id __devinitdata of_platform_serial_table[] =3D =
{
 	{ .type =3D "serial", .compatible =3D "ns8250",   .data =3D (void
*)PORT_8250, },
 	{ .type =3D "serial", .compatible =3D "ns16450",  .data =3D (void
*)PORT_16450, },
+	{ .type =3D "serial", .compatible =3D "xlnx,xps-uart16550-2.00.b",
=2Edata =3D (void *)PORT_16550A, },
 	{ .type =3D "serial", .compatible =3D "ns16550",  .data =3D (void
*)PORT_16550, },
 	{ .type =3D "serial", .compatible =3D "ns16750",  .data =3D (void
*)PORT_16750, },
 	{ .type =3D "serial", .compatible =3D "ns16850",  .data =3D (void
*)PORT_16850, },
*************************************************************************=
*********************
IMPORTANT: The contents of this email and any attachments are confidentia=
l. They are intended for the=20
named recipient(s) only.
If you have received this email in error, please notify the system manage=
r or the sender immediately and do=20
not disclose the contents to anyone or make copies thereof.

Re: Bug in drivers/serial/of_serial.c?

[Arnd Bergmann]

On Thursday 19 November 2009, Alon Ziv wrote:
> Is the following better?
> 
> ---
> [PATCH] Xilinx 16550 UART is actually 16550A-compatible
> 

Yes, that's better because it's guaranteed not to break any
other system, while fixing yours.

I'd still add support for the compatible="ns16550a" property
so that we do the right thing for future systems.

	Arnd <><

RE: Bug in drivers/serial/of_serial.c?

[Alon Ziv]

On Thursday, November 19, 2009, Arnd Bergmann wrote:
> I'd still add support for the compatible=3D"ns16550a" property
> so that we do the right thing for future systems.
>=20

OK...
---
Xilinx 16550 UART is actually 16550A-compatible

Signed-off-by: Alon Ziv <alonz@discretix.com>

diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
index 02406ba..241be77 100644
--- a/drivers/serial/of_serial.c
+++ b/drivers/serial/of_serial.c
@@ -161,7 +161,9 @@ static int of_platform_serial_remove(struct
of_device *ofdev)
 static struct of_device_id __devinitdata of_platform_serial_table[] =3D =
{
 	{ .type =3D "serial", .compatible =3D "ns8250",   .data =3D (void
*)PORT_8250, },
 	{ .type =3D "serial", .compatible =3D "ns16450",  .data =3D (void
*)PORT_16450, },
+	{ .type =3D "serial", .compatible =3D "xlnx,xps-uart16550-2.00.b",
=2Edata =3D (void *)PORT_16550A, },
 	{ .type =3D "serial", .compatible =3D "ns16550",  .data =3D (void
*)PORT_16550, },
+	{ .type =3D "serial", .compatible =3D "ns16550a", .data =3D (void
*)PORT_16550A, },
 	{ .type =3D "serial", .compatible =3D "ns16750",  .data =3D (void
*)PORT_16750, },
 	{ .type =3D "serial", .compatible =3D "ns16850",  .data =3D (void
*)PORT_16850, },
 #ifdef CONFIG_SERIAL_OF_PLATFORM_NWPSERIAL
*************************************************************************=
*********************
IMPORTANT: The contents of this email and any attachments are confidentia=
l. They are intended for the=20
named recipient(s) only.
If you have received this email in error, please notify the system manage=
r or the sender immediately and do=20
not disclose the contents to anyone or make copies thereof.

Re: Bug in drivers/serial/of_serial.c?

[Arnd Bergmann]

On Thursday 19 November 2009, Alon Ziv wrote:
> On Thursday, November 19, 2009, Arnd Bergmann wrote:
> > I'd still add support for the compatible="ns16550a" property
> > so that we do the right thing for future systems.
> > 
> 
> OK...
> ---
> Xilinx 16550 UART is actually 16550A-compatible
> 
> Signed-off-by: Alon Ziv <alonz@discretix.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Does this go through the powerpc or the tty tree?
I'd be happy if either Ben or Greg could pick this up.

I'm happy to keep maintaining the driver itself but it
would be nice to know a definite subsystem maintainer
responsible for it.

Greg, if you want to take patches for of_serial.c generally,
I'll start forwarding them to you as they come in and make
sure they apply to your tree.

	Arnd <><

> diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
> index 02406ba..241be77 100644
> --- a/drivers/serial/of_serial.c
> +++ b/drivers/serial/of_serial.c
> @@ -161,7 +161,9 @@ 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 = "xlnx,xps-uart16550-2.00.b",
> .data = (void *)PORT_16550A, },
>  	{ .type = "serial", .compatible = "ns16550",  .data = (void
> *)PORT_16550, },
> +	{ .type = "serial", .compatible = "ns16550a", .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

Re: Bug in drivers/serial/of_serial.c?

[Greg KH]

On Thu, Nov 19, 2009 at 03:09:31PM +0100, Arnd Bergmann wrote:
> On Thursday 19 November 2009, Alon Ziv wrote:
> > On Thursday, November 19, 2009, Arnd Bergmann wrote:
> > > I'd still add support for the compatible="ns16550a" property
> > > so that we do the right thing for future systems.
> > > 
> > 
> > OK...
> > ---
> > Xilinx 16550 UART is actually 16550A-compatible
> > 
> > Signed-off-by: Alon Ziv <alonz@discretix.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> Does this go through the powerpc or the tty tree?
> I'd be happy if either Ben or Greg could pick this up.
> 
> I'm happy to keep maintaining the driver itself but it
> would be nice to know a definite subsystem maintainer
> responsible for it.
> 
> Greg, if you want to take patches for of_serial.c generally,
> I'll start forwarding them to you as they come in and make
> sure they apply to your tree.

Sure, I would be glad to do so, send them on.

thanks,

greg k-h

RE: Bug in drivers/serial/of_serial.c?

[Stephen Neuendorffer]

> -----Original Message-----
> From: linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen=3Dneuendorffer.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 =3D "serial", .compatible =3D "ns16550",  .data =3D
(void
> *)PORT_16550, },
> > > +       { .type =3D "serial", .compatible =3D "ns16550",  .data =3D
(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 r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

RE: Bug in drivers/serial/of_serial.c?

[Stephen Neuendorffer]

NAK.

If the problem is in the device trees that are being generated, we
should fix the issue there.
We've been trying to avoid putting the fully specified IP versions in
the kernel like this, since
the IP changes so often.

Steve

> -----Original Message-----
> From: linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org] On Behalf Of Alon
Ziv
> Sent: Thursday, November 19, 2009 5:49 AM
> To: Arnd Bergmann; linuxppc-dev@lists.ozlabs.org
> Subject: RE: Bug in drivers/serial/of_serial.c?
> =

> On Thursday, November 19, 2009, Arnd Bergmann wrote:
> > I'd still add support for the compatible=3D"ns16550a" property
> > so that we do the right thing for future systems.
> >
> =

> OK...
> ---
> Xilinx 16550 UART is actually 16550A-compatible
> =

> Signed-off-by: Alon Ziv <alonz@discretix.com>
> =

> diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
> index 02406ba..241be77 100644
> --- a/drivers/serial/of_serial.c
> +++ b/drivers/serial/of_serial.c
> @@ -161,7 +161,9 @@ static int of_platform_serial_remove(struct
> of_device *ofdev)
>  static struct of_device_id __devinitdata of_platform_serial_table[] =3D
{
>  	{ .type =3D "serial", .compatible =3D "ns8250",   .data =3D (void
> *)PORT_8250, },
>  	{ .type =3D "serial", .compatible =3D "ns16450",  .data =3D (void
> *)PORT_16450, },
> +	{ .type =3D "serial", .compatible =3D "xlnx,xps-uart16550-2.00.b",
> .data =3D (void *)PORT_16550A, },
>  	{ .type =3D "serial", .compatible =3D "ns16550",  .data =3D (void
> *)PORT_16550, },
> +	{ .type =3D "serial", .compatible =3D "ns16550a", .data =3D (void
> *)PORT_16550A, },
>  	{ .type =3D "serial", .compatible =3D "ns16750",  .data =3D (void
> *)PORT_16750, },
>  	{ .type =3D "serial", .compatible =3D "ns16850",  .data =3D (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.
> =

> =

> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev


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

Re: Bug in drivers/serial/of_serial.c?

[Arnd Bergmann]

On Thursday 19 November 2009, Stephen Neuendorffer wrote:
> If the problem is in the device trees that are being generated, we
> should fix the issue there.
> We've been trying to avoid putting the fully specified IP versions in
> the kernel like this, since
> the IP changes so often.

No, the problem that Alon has is that the firmware currently has no
way whatsoever to give a correct device tree, because of-serial.c
does not even know about ns16550a.

The patch adds both a special-case for the specific uart he
is using so that one is grandfathered in and a new compatible
value so future boards can specify both ns16550a and ns16550.

	Arnd <><

RE: Bug in drivers/serial/of_serial.c?

[John Linn]

NAK also.

Yes we can generate a different device tree to fix this issue.

Thanks,
John

> -----Original Message-----
> From: Stephen Neuendorffer
> Sent: Thursday, November 19, 2009 10:23 AM
> To: Alon Ziv; Arnd Bergmann; linuxppc-dev@lists.ozlabs.org
> Cc: John Linn; grant.likely@secretlab.ca
> Subject: RE: Bug in drivers/serial/of_serial.c?
> =

> =

> NAK.
> =

> If the problem is in the device trees that are being generated, we
should fix the issue there.
> We've been trying to avoid putting the fully specified IP versions in
the kernel like this, since
> the IP changes so often.
> =

> Steve
> =

> > -----Original Message-----
> > From:
linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> > bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org] On Behalf Of
Alon Ziv
> > Sent: Thursday, November 19, 2009 5:49 AM
> > To: Arnd Bergmann; linuxppc-dev@lists.ozlabs.org
> > Subject: RE: Bug in drivers/serial/of_serial.c?
> >
> > On Thursday, November 19, 2009, Arnd Bergmann wrote:
> > > I'd still add support for the compatible=3D"ns16550a" property
> > > so that we do the right thing for future systems.
> > >
> >
> > OK...
> > ---
> > Xilinx 16550 UART is actually 16550A-compatible
> >
> > Signed-off-by: Alon Ziv <alonz@discretix.com>
> >
> > diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
> > index 02406ba..241be77 100644
> > --- a/drivers/serial/of_serial.c
> > +++ b/drivers/serial/of_serial.c
> > @@ -161,7 +161,9 @@ static int of_platform_serial_remove(struct
> > of_device *ofdev)
> >  static struct of_device_id __devinitdata of_platform_serial_table[]
=3D {
> >  	{ .type =3D "serial", .compatible =3D "ns8250",   .data =3D (void
> > *)PORT_8250, },
> >  	{ .type =3D "serial", .compatible =3D "ns16450",  .data =3D (void
> > *)PORT_16450, },
> > +	{ .type =3D "serial", .compatible =3D "xlnx,xps-uart16550-2.00.b",
> > .data =3D (void *)PORT_16550A, },
> >  	{ .type =3D "serial", .compatible =3D "ns16550",  .data =3D (void
> > *)PORT_16550, },
> > +	{ .type =3D "serial", .compatible =3D "ns16550a", .data =3D (void
> > *)PORT_16550A, },
> >  	{ .type =3D "serial", .compatible =3D "ns16750",  .data =3D (void
> > *)PORT_16750, },
> >  	{ .type =3D "serial", .compatible =3D "ns16850",  .data =3D (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.
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev


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

RE: Bug in drivers/serial/of_serial.c?

[Stephen Neuendorffer]

> -----Original Message-----
> From: linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org
[mailto:linuxppc-dev-
> bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org] On Behalf Of Arnd
Bergmann
> Sent: Thursday, November 19, 2009 9:33 AM
> To: Stephen Neuendorffer
> Cc: John Linn; Alon Ziv; linuxppc-dev@lists.ozlabs.org
> Subject: Re: Bug in drivers/serial/of_serial.c?
> =

> On Thursday 19 November 2009, Stephen Neuendorffer wrote:
> > If the problem is in the device trees that are being generated, we
> > should fix the issue there.
> > We've been trying to avoid putting the fully specified IP versions
in
> > the kernel like this, since
> > the IP changes so often.
> =

> No, the problem that Alon has is that the firmware currently has no
> way whatsoever to give a correct device tree, because of-serial.c
> does not even know about ns16550a.
> =

> The patch adds both a special-case for the specific uart he
> is using so that one is grandfathered in and a new compatible
> value so future boards can specify both ns16550a and ns16550.

That's true...  The 16550a line still needs to get added, but not the
xlnx-
specific line.

Steve


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

Re: Bug in drivers/serial/of_serial.c?

[Grant Likely]

[fixed top-posting nonsense  :-) ]

On Thu, Nov 19, 2009 at 10:22 AM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
>> diff --git a/drivers/serial/of_serial.c b/drivers/serial/of_serial.c
>> index 02406ba..241be77 100644
>> --- a/drivers/serial/of_serial.c
>> +++ b/drivers/serial/of_serial.c
>> @@ -161,7 +161,9 @@ 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 = "xlnx,xps-uart16550-2.00.b",
>> .data = (void *)PORT_16550A, },
>>       { .type = "serial", .compatible = "ns16550",  .data = (void
>> *)PORT_16550, },
>> +     { .type = "serial", .compatible = "ns16550a", .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
>
> NAK.

No, I wouldn't NAK this.  It is an appropriate fix to still support
older trees that use the ns16550 value.  Arnd, I've got no problem
with this being merged.

g.

>
> If the problem is in the device trees that are being generated, we
> should fix the issue there.
> We've been trying to avoid putting the fully specified IP versions in
> the kernel like this, since
> the IP changes so often.

Yes, the error is in the device tree generator... which is *exactly*
why the fully specified IP version is in the device tree; so that a
driver can work around bugs like this.  You're right that we avoid
specifying it to eliminate an explosion of xlnx,<device><version>
strings in the kernel; but it is perfectly safe to use in situations
like this.

As you also know, but it is worth stating here for the record, this is
also exactly why newer versions of the IP claim compatibility with
fully specified versions of older IP.  So that the list of strings
doesn't explode, but also so that it claims compatibility with a real
version of the device (not a made up string whose meaning can change
over time).

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Bug in drivers/serial/of_serial.c?

[Grant Likely]

On Thu, Nov 19, 2009 at 10:42 AM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
>
>
>> -----Original Message-----
>> From: linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org
> [mailto:linuxppc-dev-
>> bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org] On Behalf Of Arnd
> Bergmann
>> Sent: Thursday, November 19, 2009 9:33 AM
>> To: Stephen Neuendorffer
>> Cc: John Linn; Alon Ziv; linuxppc-dev@lists.ozlabs.org
>> Subject: Re: Bug in drivers/serial/of_serial.c?
>>
>> On Thursday 19 November 2009, Stephen Neuendorffer wrote:
>> > If the problem is in the device trees that are being generated, we
>> > should fix the issue there.
>> > We've been trying to avoid putting the fully specified IP versions
> in
>> > the kernel like this, since
>> > the IP changes so often.
>>
>> No, the problem that Alon has is that the firmware currently has no
>> way whatsoever to give a correct device tree, because of-serial.c
>> does not even know about ns16550a.
>>
>> The patch adds both a special-case for the specific uart he
>> is using so that one is grandfathered in and a new compatible
>> value so future boards can specify both ns16550a and ns16550.
>
> That's true... =A0The 16550a line still needs to get added, but not the
> xlnx-
> specific line.

The xlnx- line should be added and is entirely appropriate for exactly
the reason Arnd stated.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

RE: Bug in drivers/serial/of_serial.c?

[John Linn]

> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Gra=
nt Likely
> Sent: Friday, November 20, 2009 2:58 PM
> To: Stephen Neuendorffer
> Cc: Arnd Bergmann; John Linn; Alon Ziv; linuxppc-dev@lists.ozlabs.org
> Subject: Re: Bug in drivers/serial/of_serial.c?
> =

> On Thu, Nov 19, 2009 at 10:42 AM, Stephen Neuendorffer
> <stephen.neuendorffer@xilinx.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.or=
g
> > [mailto:linuxppc-dev-
> >> bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org] On Behalf Of Arn=
d
> > Bergmann
> >> Sent: Thursday, November 19, 2009 9:33 AM
> >> To: Stephen Neuendorffer
> >> Cc: John Linn; Alon Ziv; linuxppc-dev@lists.ozlabs.org
> >> Subject: Re: Bug in drivers/serial/of_serial.c?
> >>
> >> On Thursday 19 November 2009, Stephen Neuendorffer wrote:
> >> > If the problem is in the device trees that are being generated, we
> >> > should fix the issue there.
> >> > We've been trying to avoid putting the fully specified IP versions
> > in
> >> > the kernel like this, since
> >> > the IP changes so often.
> >>
> >> No, the problem that Alon has is that the firmware currently has no
> >> way whatsoever to give a correct device tree, because of-serial.c
> >> does not even know about ns16550a.
> >>
> >> The patch adds both a special-case for the specific uart he
> >> is using so that one is grandfathered in and a new compatible
> >> value so future boards can specify both ns16550a and ns16550.
> >
> > That's true... =A0The 16550a line still needs to get added, but not the=

> > xlnx-
> > specific line.
> =

> The xlnx- line should be added and is entirely appropriate for exactly
> the reason Arnd stated.

I'm just wanting to make sure I'm on the same page...

It seems like you're saying that adding this line fixes the system before a=
ny device tree generator fix, for existing systems, true?

If that's true, how do you know that the 16550 in the xilinx system is not =
a 16450 as that's the default?

Right now we're not generating the ns16450 as we should be when there are n=
o FIFOs.  Since we've been using 16550 and not 16550A it probably hasn't be=
en a problem.

-- John


> =

> g.
> =

> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.


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

Re: Bug in drivers/serial/of_serial.c?

[Grant Likely]

On Fri, Nov 20, 2009 at 3:11 PM, John Linn <John.Linn@xilinx.com> wrote:
>
>> -----Original Message-----
>> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Gr=
ant Likely
>> Sent: Friday, November 20, 2009 2:58 PM
>> To: Stephen Neuendorffer
>> Cc: Arnd Bergmann; John Linn; Alon Ziv; linuxppc-dev@lists.ozlabs.org
>> Subject: Re: Bug in drivers/serial/of_serial.c?
>>
>> On Thu, Nov 19, 2009 at 10:42 AM, Stephen Neuendorffer
>> <stephen.neuendorffer@xilinx.com> wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: linuxppc-dev-bounces+stephen=3Dneuendorffer.name@lists.ozlabs.o=
rg
>> > [mailto:linuxppc-dev-
>> >> bounces+stephen=3Dneuendorffer.name@lists.ozlabs.org] On Behalf Of Ar=
nd
>> > Bergmann
>> >> Sent: Thursday, November 19, 2009 9:33 AM
>> >> To: Stephen Neuendorffer
>> >> Cc: John Linn; Alon Ziv; linuxppc-dev@lists.ozlabs.org
>> >> Subject: Re: Bug in drivers/serial/of_serial.c?
>> >>
>> >> On Thursday 19 November 2009, Stephen Neuendorffer wrote:
>> >> > If the problem is in the device trees that are being generated, we
>> >> > should fix the issue there.
>> >> > We've been trying to avoid putting the fully specified IP versions
>> > in
>> >> > the kernel like this, since
>> >> > the IP changes so often.
>> >>
>> >> No, the problem that Alon has is that the firmware currently has no
>> >> way whatsoever to give a correct device tree, because of-serial.c
>> >> does not even know about ns16550a.
>> >>
>> >> The patch adds both a special-case for the specific uart he
>> >> is using so that one is grandfathered in and a new compatible
>> >> value so future boards can specify both ns16550a and ns16550.
>> >
>> > That's true... =A0The 16550a line still needs to get added, but not th=
e
>> > xlnx-
>> > specific line.
>>
>> The xlnx- line should be added and is entirely appropriate for exactly
>> the reason Arnd stated.
>
> I'm just wanting to make sure I'm on the same page...
>
> It seems like you're saying that adding this line fixes the system before=
 any device tree generator fix, for existing systems, true?
>
> If that's true, how do you know that the 16550 in the xilinx system is no=
t a 16450 as that's the default?
>
> Right now we're not generating the ns16450 as we should be when there are=
 no FIFOs. =A0Since we've been using 16550 and not 16550A it probably hasn'=
t been a problem.

Hmmm, right.  I forgot that we talked about that on the phone.  Yes,
in that case it is not a good idea to add the
xlnx,xps-uart16550-2.00.b because we don't know if it is configured as
a 16450 or a 16550.  At least, if the xlnx,... match is added, then
the driver *also* needs to look for the value of the "xlnx,is-a-16550"
property.

BTW, while looking at that file... Arnd, does the .type =3D "serial"
stuff really need to be there?

Cheers,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Bug in drivers/serial/of_serial.c?

[Arnd Bergmann]

On Saturday 21 November 2009, Grant Likely wrote:
> BTW, while looking at that file... Arnd, does the .type = "serial"
> stuff really need to be there?

Well, serial is one of the few that actually has some preexisting
binding with a well defined device-type, so I guess we should use
it.

	Arnd <><

Re: Bug in drivers/serial/of_serial.c?

[Grant Likely]

On Sat, Nov 21, 2009 at 12:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Saturday 21 November 2009, Grant Likely wrote:
>> BTW, while looking at that file... Arnd, does the .type = "serial"
>> stuff really need to be there?
>
> Well, serial is one of the few that actually has some preexisting
> binding with a well defined device-type, so I guess we should use
> it.

But device-type describes a OpenFirmware API, not a device register
interface.  Once we're in the kernel and talking to the hardware
directly, device-type has no real meaning because we're not using the
OpenFirmware interface to talk to serial devices in a generic manner.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Bug in drivers/serial/of_serial.c?

[Grant Likely]

On Sun, Nov 22, 2009 at 3:42 PM, Grant Likely <grant.likely@secretlab.ca> w=
rote:
> On Sat, Nov 21, 2009 at 12:45 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Saturday 21 November 2009, Grant Likely wrote:
>>> BTW, while looking at that file... Arnd, does the .type =3D "serial"
>>> stuff really need to be there?
>>
>> Well, serial is one of the few that actually has some preexisting
>> binding with a well defined device-type, so I guess we should use
>> it.
>
> But device-type describes a OpenFirmware API, not a device register
> interface. =A0Once we're in the kernel and talking to the hardware
> directly, device-type has no real meaning because we're not using the
> OpenFirmware interface to talk to serial devices in a generic manner.

And looking for it in the driver forces us to put meaningless
"device_type" properties in the flat trees, where they make no sense
whatsoever.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.