| Message ID | 8B957E110B62714A84290A01A597805F05D2AE47@Exchange.discretix.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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=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 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="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[] = { > { .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 > ************************************************************************ ********************** > 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 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.
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 <><
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=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 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="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[] = { > > { .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 > > ************************************************************************ ********************** > > 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 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.
> -----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 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 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.
[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.
On Thu, Nov 19, 2009 at 10:42 AM, Stephen Neuendorffer <stephen.neuendorffer@xilinx.com> wrote: > > >> -----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 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. The xlnx- line should be added and is entirely appropriate for exactly the reason Arnd stated. g.
> -----Original Message----- > From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant 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=neuendorffer.name@lists.ozlabs.org > > [mailto:linuxppc-dev- > >> bounces+stephen=neuendorffer.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. > > 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 not a 16450 as that's the default? Right now we're not generating the ns16450 as we should be when there are no FIFOs. Since we've been using 16550 and not 16550A it probably hasn't been 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 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.
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 Grant 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=neuendorffer.name@lists.ozlabs.org >> > [mailto:linuxppc-dev- >> >> bounces+stephen=neuendorffer.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. >> >> 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 not a 16450 as that's the default? > > Right now we're not generating the ns16450 as we should be when there are no FIFOs. Since 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 = "serial" stuff really need to be there? Cheers, g.
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 <><
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.
On Sun, Nov 22, 2009 at 3:42 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > 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. 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.
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