* [PATCH]serial: 8250: Fix detect XScale port wrong
@ 2013-03-01 5:56 Wang YanQing
2013-03-01 19:17 ` Paul Gortmaker
0 siblings, 1 reply; 3+ messages in thread
From: Wang YanQing @ 2013-03-01 5:56 UTC (permalink / raw)
To: gregkh
Cc: swarren, jslaby, alan, arnd, paul.gortmaker, damm, linux-serial,
linux-kernel
Some UARTs add enhanced functions with unused bit in
16550 standard, like UART_IER_UUE bit, it cause XScale
detect wrong. Now detect UART_IER_UUE and UART_IER_RTOIE
to reduce the annoying wrong result which cause UARTs don't
work.
Serial controller: Device 4348:3253(CH352 PCI based Multi-I/O Controller)
is a example. It use UART_IER_UUE as the LOWPOWER function,
you can get the datasheet from below urls:
http://wch-ic.com/download/list.asp?id=116
CH352DS1.PDF
http://wch-ic.com/download/list.asp?id=117
CH352DS2.PDF.
I choice UART_IER_RTOIE as another test bit, because
choice it is harmless for current code, we will set
UART_CAP_RTOIE if it is XScale port.
Signed-off-by: Wang YanQing <udknight@gmail.com>
---
drivers/tty/serial/8250/8250.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 0efc815..2c1f9c9 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -841,6 +841,7 @@ static void autoconfig_16550a(struct uart_8250_port *up)
{
unsigned char status1, status2;
unsigned int iersave;
+ unsigned int iertest;
up->port.type = PORT_16550A;
up->capabilities |= UART_CAP_FIFO;
@@ -966,16 +967,25 @@ static void autoconfig_16550a(struct uart_8250_port *up)
* We're going to explicitly set the UUE bit to 0 before
* trying to write and read a 1 just to make sure it's not
* already a 1 and maybe locked there before we even start start.
+ *
+ * 01/03/2013
+ * Some UARTs add enhanced functions with unused bit in
+ * 16550 standard, like UART_IER_UUE bit, it cause XScale
+ * detect wrong. Now detect UART_IER_UUE and UART_IER_RTOIE
+ * to reduce the annoying wrong result which cause UART don't
+ * work.
*/
iersave = serial_in(up, UART_IER);
- serial_out(up, UART_IER, iersave & ~UART_IER_UUE);
- if (!(serial_in(up, UART_IER) & UART_IER_UUE)) {
+ iertest = UART_IER_UUE | UART_IER_RTOIE;
+
+ serial_out(up, UART_IER, iersave & ~iertest);
+ if (!(serial_in(up, UART_IER) & iertest)) {
/*
* OK it's in a known zero state, try writing and reading
* without disturbing the current state of the other bits.
*/
- serial_out(up, UART_IER, iersave | UART_IER_UUE);
- if (serial_in(up, UART_IER) & UART_IER_UUE) {
+ serial_out(up, UART_IER, iersave | iertest);
+ if ((serial_in(up, UART_IER) & iertest) == iertest) {
/*
* It's an Xscale.
* We'll leave the UART_IER_UUE bit set to 1 (enabled).
--
1.7.11.1.116.g8228a23
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH]serial: 8250: Fix detect XScale port wrong
2013-03-01 5:56 [PATCH]serial: 8250: Fix detect XScale port wrong Wang YanQing
@ 2013-03-01 19:17 ` Paul Gortmaker
2013-03-02 8:54 ` Wang YanQing
0 siblings, 1 reply; 3+ messages in thread
From: Paul Gortmaker @ 2013-03-01 19:17 UTC (permalink / raw)
To: Wang YanQing, gregkh, swarren, jslaby, alan, arnd, damm,
linux-serial, linux-kernel
On 13-03-01 12:56 AM, Wang YanQing wrote:
> Some UARTs add enhanced functions with unused bit in
> 16550 standard, like UART_IER_UUE bit, it cause XScale
Which xscale platform? It would be nice to know the specifics.
> detect wrong. Now detect UART_IER_UUE and UART_IER_RTOIE
> to reduce the annoying wrong result which cause UARTs don't
> work.
You should ideally identify the original commit which caused
the regression. Assuming you have fully understood the problem
you should be able to do this with "git blame" and not need to
do the full bisect.
In addition, you should also describe how they fail, since just
saying "don't work" does not convey what the end user symptom
looked like at all.
>
> Serial controller: Device 4348:3253(CH352 PCI based Multi-I/O Controller)
> is a example. It use UART_IER_UUE as the LOWPOWER function,
> you can get the datasheet from below urls:
>
> http://wch-ic.com/download/list.asp?id=116
> CH352DS1.PDF
>
> http://wch-ic.com/download/list.asp?id=117
> CH352DS2.PDF.
Rather than quote links that will expire and no longer be
valid in six months, it would be better if you described
exactly how and why these ports are different directly in
your commit long log.
> I choice UART_IER_RTOIE as another test bit, because
> choice it is harmless for current code, we will set
> UART_CAP_RTOIE if it is XScale port.
There are a lot of 8250/16550 variations out there, and you
really need to be careful when claiming that you can make
"harmless" changes. It might look harmless but at the same
time break some 8250 clone in some SoC.
>
> Signed-off-by: Wang YanQing <udknight@gmail.com>
> ---
> drivers/tty/serial/8250/8250.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 0efc815..2c1f9c9 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -841,6 +841,7 @@ static void autoconfig_16550a(struct uart_8250_port *up)
> {
> unsigned char status1, status2;
> unsigned int iersave;
> + unsigned int iertest;
It looks to me like this is never written to, aside from the 1st
assignment and hence could just as easily be a #define or similar.
>
> up->port.type = PORT_16550A;
> up->capabilities |= UART_CAP_FIFO;
> @@ -966,16 +967,25 @@ static void autoconfig_16550a(struct uart_8250_port *up)
> * We're going to explicitly set the UUE bit to 0 before
> * trying to write and read a 1 just to make sure it's not
> * already a 1 and maybe locked there before we even start start.
> + *
> + * 01/03/2013
> + * Some UARTs add enhanced functions with unused bit in
> + * 16550 standard, like UART_IER_UUE bit, it cause XScale
> + * detect wrong. Now detect UART_IER_UUE and UART_IER_RTOIE
> + * to reduce the annoying wrong result which cause UART don't
> + * work.
> */
> iersave = serial_in(up, UART_IER);
> - serial_out(up, UART_IER, iersave & ~UART_IER_UUE);
> - if (!(serial_in(up, UART_IER) & UART_IER_UUE)) {
> + iertest = UART_IER_UUE | UART_IER_RTOIE;
> +
> + serial_out(up, UART_IER, iersave & ~iertest);
> + if (!(serial_in(up, UART_IER) & iertest)) {
> /*
> * OK it's in a known zero state, try writing and reading
> * without disturbing the current state of the other bits.
> */
> - serial_out(up, UART_IER, iersave | UART_IER_UUE);
> - if (serial_in(up, UART_IER) & UART_IER_UUE) {
> + serial_out(up, UART_IER, iersave | iertest);
> + if ((serial_in(up, UART_IER) & iertest) == iertest) {
> /*
> * It's an Xscale.
> * We'll leave the UART_IER_UUE bit set to 1 (enabled).
What does your change do to Xscale that do not do UART_IER_RTOIE?
Paul.
--
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH]serial: 8250: Fix detect XScale port wrong
2013-03-01 19:17 ` Paul Gortmaker
@ 2013-03-02 8:54 ` Wang YanQing
0 siblings, 0 replies; 3+ messages in thread
From: Wang YanQing @ 2013-03-02 8:54 UTC (permalink / raw)
To: Paul Gortmaker
Cc: gregkh, swarren, jslaby, alan, arnd, damm, linux-serial,
linux-kernel
On Fri, Mar 01, 2013 at 02:17:16PM -0500, Paul Gortmaker wrote:
> On 13-03-01 12:56 AM, Wang YanQing wrote:
> > Some UARTs add enhanced functions with unused bit in
> > 16550 standard, like UART_IER_UUE bit, it cause XScale
>
> Which xscale platform? It would be nice to know the specifics.
The "XScale" mean the XScale port detect code in autoconfig_16550a,
I meet the problem in a normal pc motherboard, I should rephrase
the subject use "Fix detect 16550A ports as XScale ports wrong"
>
> > detect wrong. Now detect UART_IER_UUE and UART_IER_RTOIE
> > to reduce the annoying wrong result which cause UARTs don't
> > work.
>
> You should ideally identify the original commit which caused
> the regression. Assuming you have fully understood the problem
> you should be able to do this with "git blame" and not need to
> do the full bisect.
>
No it is not a regression, it is caused just like you say
there are many 8250/16550 variations, and I meet one, that is it.
>
> >
> > Serial controller: Device 4348:3253(CH352 PCI based Multi-I/O Controller)
> > is a example. It use UART_IER_UUE as the LOWPOWER function,
> > you can get the datasheet from below urls:
> >
> > http://wch-ic.com/download/list.asp?id=116
> > CH352DS1.PDF
> >
> > http://wch-ic.com/download/list.asp?id=117
> > CH352DS2.PDF.
>
> Rather than quote links that will expire and no longer be
> valid in six months, it would be better if you described
> exactly how and why these ports are different directly in
> your commit long log.
Device 4348:3253 use the UART_IER_UUE
as the LOWPOWER function , so it is readable and writable,
quote out the origin words in datasheet.
"
LOWPOWER:When the bit is 1, close the internal benchmark clock
of serial port to set into low-power status.
"
That is the convict cause our XScale detect code work wrong.
> What does your change do to Xscale that do not do UART_IER_RTOIE?
Yes, you are right, I don't have experience with Xscale, so I will
fix the problem in another way, set it fix type in 8250_pci code
maybe is good.
Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-03-02 8:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-01 5:56 [PATCH]serial: 8250: Fix detect XScale port wrong Wang YanQing
2013-03-01 19:17 ` Paul Gortmaker
2013-03-02 8:54 ` Wang YanQing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).