Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello.
Marc St-Jean wrote:
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 3d91bfc..bfaacc5 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -308,6 +308,7 @@ static unsigned int serial_in(struct uar
> return inb(up->port.iobase + 1);
>
> case UPIO_MEM:
> + case UPIO_DWAPB:
> return readb(up->port.membase + offset);
>
> case UPIO_MEM32:
> @@ -333,6 +334,8 @@ #endif
> static void
> serial_out(struct uart_8250_port *up, int offset, int value)
> {
> + /* Save the offset before it's remapped */
> + int save_offset = offset;
> offset = map_8250_out_reg(up, offset) << up->port.regshift;
>
> switch (up->port.iotype) {
I've just got an idea how to "beautify" this code -- rename the
'offset'
arg to something like reg, and then declare/init 'offset' as local
variable.
Would certainly look better (and worth doing in all serial_* accessros).
I agree but am having enough of a hard time getting the bare minimum
accepted that I don't dare touch any unnecessary lines.
Well, I can try pushing this simple cleanup myself... seems worth doing
for alike future cases anyway.
> @@ -359,6 +362,18 @@ #endif
> writeb(value, up->port.membase + offset);
> break;
>
> + case UPIO_DWAPB:
> + /* Save the LCR value so it can be re-written when a
> + * Busy Detect interrupt occurs. */
> + if (save_offset == UART_LCR)
> + up->lcr = value;
> + writeb(value, up->port.membase + offset);
> + /* Read the IER to ensure any interrupt is cleared before
> + * returning from ISR. */
> + if (save_offset == UART_TX || save_offset == UART_IER)
> + value = serial_in(up, UART_IER);
> + break;
> +
> default:
> outb(value, up->port.iobase + offset);
> }
> @@ -2454,8 +2485,8 @@ int __init serial8250_start_console(stru
>
> add_preferred_console("ttyS", line, options);
> printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
> - line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
> - port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
> + line, port->iotype >= UPIO_MEM ? "MMIO" : "I/O port",
> + port->iotype >= UPIO_MEM ? (unsigned long) port->mapbase :
> (unsigned long) port->iobase, options);
> if (!(serial8250_console.flags & CON_ENABLED)) {
> serial8250_console.flags &= ~CON_PRINTBUFFER;
I've remembered why I decided not to fix it: this code only gets called
from 8250__eraly.c which can't handle anything beyond UPIO_MEM anyway.
We don't use 8250_early and I've tested it works without, so I'll drop this
change.
No need I guess since this is the Right Thing (TM) anyway.
Thanks,
Marc
WBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
> Hello.
>
> Marc St-Jean wrote:
>
> > There are three different fixes:
> > 1. Fix for DesignWare APB THRE errata:
> > In brief, this is a non-standard 16550 in that the THRE interrupt
> > will not re-assert itself simply by disabling and re-enabling the
> > THRI bit in the IER, it is only re-enabled if a character is actually
> > sent out.
> > It appears that the "8250-uart-backup-timer.patch" in the "mm" tree also
> > fixes it so we have dropped our initial workaround.
> > This patch now needs to be applied on top of that "mm" patch.
>
> This patch has hit the mainline kernel already.
I see, I'll drop the reference to the "mm" patch.
> > 3. Workaround for interrupt/data concurrency issue:
> > The SoC needs to ensure that writes that can cause interrupts to be
> > cleared reach the UART before returning from the ISR. This fix reads
> > a non-destructive register on the UART so the read transaction
> > completion ensures the previously queued write transaction has
> > also completed.
>
> > The differences from the last attempt is dropping the call to
> > in_irq() and including more detailed description of the fixes.
>
> The difference part would better be under the "---" cutoff line, along
> with diffstat.
> This way it gets auto-removed by quilt/git when applying the patch.
I'll add to the next posting.
> > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> > index 3d91bfc..bfaacc5 100644
> > --- a/drivers/serial/8250.c
> > +++ b/drivers/serial/8250.c
> > @@ -308,6 +308,7 @@ static unsigned int serial_in(struct uar
> > return inb(up->port.iobase + 1);
> >
> > case UPIO_MEM:
> > + case UPIO_DWAPB:
> > return readb(up->port.membase + offset);
> >
> > case UPIO_MEM32:
> > @@ -333,6 +334,8 @@ #endif
> > static void
> > serial_out(struct uart_8250_port *up, int offset, int value)
> > {
> > + /* Save the offset before it's remapped */
> > + int save_offset = offset;
> > offset = map_8250_out_reg(up, offset) << up->port.regshift;
> >
> > switch (up->port.iotype) {
>
> I've just got an idea how to "beautify" this code -- rename the
> 'offset'
> arg to something like reg, and then declare/init 'offset' as local
> variable.
> Would certainly look better (and worth doing in all serial_* accessros).
I agree but am having enough of a hard time getting the bare minimum
accepted that I don't dare touch any unnecessary lines.
> > @@ -359,6 +362,18 @@ #endif
> > writeb(value, up->port.membase + offset);
> > break;
> >
> > + case UPIO_DWAPB:
> > + /* Save the LCR value so it can be re-written when a
> > + * Busy Detect interrupt occurs. */
> > + if (save_offset == UART_LCR)
> > + up->lcr = value;
> > + writeb(value, up->port.membase + offset);
> > + /* Read the IER to ensure any interrupt is cleared before
> > + * returning from ISR. */
> > + if (save_offset == UART_TX || save_offset == UART_IER)
> > + value = serial_in(up, UART_IER);
> > + break;
> > +
> > default:
> > outb(value, up->port.iobase + offset);
> > }
> > @@ -2454,8 +2485,8 @@ int __init serial8250_start_console(stru
> >
> > add_preferred_console("ttyS", line, options);
> > printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
> > - line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
> > - port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
> > + line, port->iotype >= UPIO_MEM ? "MMIO" : "I/O port",
> > + port->iotype >= UPIO_MEM ? (unsigned long) port->mapbase :
> > (unsigned long) port->iobase, options);
> > if (!(serial8250_console.flags & CON_ENABLED)) {
> > serial8250_console.flags &= ~CON_PRINTBUFFER;
>
> I've remembered why I decided not to fix it: this code only gets called
> from 8250__eraly.c which can't handle anything beyond UPIO_MEM anyway.
We don't use 8250_early and I've tested it works without, so I'll drop this
change.
Thanks,
Marc
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Andrew Morton wrote: > On Thu, 15 Feb 2007 13:26:29 -0600 > Marc St-Jean <[EMAIL PROTECTED]> wrote: > > > + status = *(volatile u32 *)up->port.private_data; > > It distresses me that this patch uses a variable which this patch > doesn't initialise anywhere. It isn't complete. > > The sub-driver code whch sets up this field shuld be included in the > patch, no? > OK, I'll re-post with the platform file which initializes the variable. That file however will reference other files in the platform which has not been posted yet. I thought it was better to post the driver-only code. Marc - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Andrew Morton wrote: On Thu, 15 Feb 2007 13:26:29 -0600 Marc St-Jean [EMAIL PROTECTED] wrote: + status = *(volatile u32 *)up-port.private_data; It distresses me that this patch uses a variable which this patch doesn't initialise anywhere. It isn't complete. The sub-driver code whch sets up this field shuld be included in the patch, no? OK, I'll re-post with the platform file which initializes the variable. That file however will reference other files in the platform which has not been posted yet. I thought it was better to post the driver-only code. Marc - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
Hello.
Marc St-Jean wrote:
There are three different fixes:
1. Fix for DesignWare APB THRE errata:
In brief, this is a non-standard 16550 in that the THRE interrupt
will not re-assert itself simply by disabling and re-enabling the
THRI bit in the IER, it is only re-enabled if a character is actually
sent out.
It appears that the 8250-uart-backup-timer.patch in the mm tree also
fixes it so we have dropped our initial workaround.
This patch now needs to be applied on top of that mm patch.
This patch has hit the mainline kernel already.
I see, I'll drop the reference to the mm patch.
3. Workaround for interrupt/data concurrency issue:
The SoC needs to ensure that writes that can cause interrupts to be
cleared reach the UART before returning from the ISR. This fix reads
a non-destructive register on the UART so the read transaction
completion ensures the previously queued write transaction has
also completed.
The differences from the last attempt is dropping the call to
in_irq() and including more detailed description of the fixes.
The difference part would better be under the --- cutoff line, along
with diffstat.
This way it gets auto-removed by quilt/git when applying the patch.
I'll add to the next posting.
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 3d91bfc..bfaacc5 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -308,6 +308,7 @@ static unsigned int serial_in(struct uar
return inb(up-port.iobase + 1);
case UPIO_MEM:
+ case UPIO_DWAPB:
return readb(up-port.membase + offset);
case UPIO_MEM32:
@@ -333,6 +334,8 @@ #endif
static void
serial_out(struct uart_8250_port *up, int offset, int value)
{
+ /* Save the offset before it's remapped */
+ int save_offset = offset;
offset = map_8250_out_reg(up, offset) up-port.regshift;
switch (up-port.iotype) {
I've just got an idea how to beautify this code -- rename the
'offset'
arg to something like reg, and then declare/init 'offset' as local
variable.
Would certainly look better (and worth doing in all serial_* accessros).
I agree but am having enough of a hard time getting the bare minimum
accepted that I don't dare touch any unnecessary lines.
@@ -359,6 +362,18 @@ #endif
writeb(value, up-port.membase + offset);
break;
+ case UPIO_DWAPB:
+ /* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+ if (save_offset == UART_LCR)
+ up-lcr = value;
+ writeb(value, up-port.membase + offset);
+ /* Read the IER to ensure any interrupt is cleared before
+ * returning from ISR. */
+ if (save_offset == UART_TX || save_offset == UART_IER)
+ value = serial_in(up, UART_IER);
+ break;
+
default:
outb(value, up-port.iobase + offset);
}
@@ -2454,8 +2485,8 @@ int __init serial8250_start_console(stru
add_preferred_console(ttyS, line, options);
printk(Adding console on ttyS%d at %s 0x%lx (options '%s')\n,
- line, port-iotype == UPIO_MEM ? MMIO : I/O port,
- port-iotype == UPIO_MEM ? (unsigned long) port-mapbase :
+ line, port-iotype = UPIO_MEM ? MMIO : I/O port,
+ port-iotype = UPIO_MEM ? (unsigned long) port-mapbase :
(unsigned long) port-iobase, options);
if (!(serial8250_console.flags CON_ENABLED)) {
serial8250_console.flags = ~CON_PRINTBUFFER;
I've remembered why I decided not to fix it: this code only gets called
from 8250__eraly.c which can't handle anything beyond UPIO_MEM anyway.
We don't use 8250_early and I've tested it works without, so I'll drop this
change.
Thanks,
Marc
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello.
Marc St-Jean wrote:
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 3d91bfc..bfaacc5 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -308,6 +308,7 @@ static unsigned int serial_in(struct uar
return inb(up-port.iobase + 1);
case UPIO_MEM:
+ case UPIO_DWAPB:
return readb(up-port.membase + offset);
case UPIO_MEM32:
@@ -333,6 +334,8 @@ #endif
static void
serial_out(struct uart_8250_port *up, int offset, int value)
{
+ /* Save the offset before it's remapped */
+ int save_offset = offset;
offset = map_8250_out_reg(up, offset) up-port.regshift;
switch (up-port.iotype) {
I've just got an idea how to beautify this code -- rename the
'offset'
arg to something like reg, and then declare/init 'offset' as local
variable.
Would certainly look better (and worth doing in all serial_* accessros).
I agree but am having enough of a hard time getting the bare minimum
accepted that I don't dare touch any unnecessary lines.
Well, I can try pushing this simple cleanup myself... seems worth doing
for alike future cases anyway.
@@ -359,6 +362,18 @@ #endif
writeb(value, up-port.membase + offset);
break;
+ case UPIO_DWAPB:
+ /* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+ if (save_offset == UART_LCR)
+ up-lcr = value;
+ writeb(value, up-port.membase + offset);
+ /* Read the IER to ensure any interrupt is cleared before
+ * returning from ISR. */
+ if (save_offset == UART_TX || save_offset == UART_IER)
+ value = serial_in(up, UART_IER);
+ break;
+
default:
outb(value, up-port.iobase + offset);
}
@@ -2454,8 +2485,8 @@ int __init serial8250_start_console(stru
add_preferred_console(ttyS, line, options);
printk(Adding console on ttyS%d at %s 0x%lx (options '%s')\n,
- line, port-iotype == UPIO_MEM ? MMIO : I/O port,
- port-iotype == UPIO_MEM ? (unsigned long) port-mapbase :
+ line, port-iotype = UPIO_MEM ? MMIO : I/O port,
+ port-iotype = UPIO_MEM ? (unsigned long) port-mapbase :
(unsigned long) port-iobase, options);
if (!(serial8250_console.flags CON_ENABLED)) {
serial8250_console.flags = ~CON_PRINTBUFFER;
I've remembered why I decided not to fix it: this code only gets called
from 8250__eraly.c which can't handle anything beyond UPIO_MEM anyway.
We don't use 8250_early and I've tested it works without, so I'll drop this
change.
No need I guess since this is the Right Thing (TM) anyway.
Thanks,
Marc
WBR, Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Andrew Morton wrote:
> On Mon, 12 Feb 2007 12:04:08 -0600 Marc St-Jean
> <[EMAIL PROTECTED]> wrote:
>
> > There are three different fixes:
> > 1. Fix for DesignWare THRE errata
> > - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
> > tree also fixes it. This patch needs to be applied on top of "mm" patch.
> >
> > 2. Fix for Busy Detect on LCR write
> > - No changes since last patch.
> >
> > 3. Workaround for interrupt/data concurrency issue
> > - No changes since last patch.
>
> A couple of things.
>
> - When preparing a changelog, please just tell us what the patch does.
> The information about how this patch differes from a previous version of
> the patch is irrelevant when the patch hits the mainline repository hence
> I must edit it all.
>
> It's OK to add the what-i-changed-since-last-time details, but please
> make
> that separate and remember that it will be removed for when the patch
> goes
> upstream.
>
> - When fixing a bug, please tell us in detail what that bug *is*. None
> of the above three items tell us any of this, which is essential
> information for those who are to review the change.
Understood, I'm adding the original description here and I'll add to the
next patch update.
1. Fix for DesignWare APB THRE errata:
In brief, this is a non-standard 16550 in that the THRE interrupt
will not re-assert itself simply by disabling and re-enabling the
THRI bit in the IER, it is only re-enabled if a character is actually
sent out.
It appears that the "8250-uart-backup-timer.patch" in the "mm" tree also
fixes it so we have dropped our initial workaround.
This patch now needs to be applied on top of that "mm" patch.
2. Fix for Busy Detect on LCR write:
The DesignWare APB UART has a feature which causes a new Busy Detect
interrupt to be generated if it's busy when the LCR is written. This
fix saves the value of the LCR and rewrites it after clearing the
interrupt.
3. Workaround for interrupt/data concurrency issue:
The SoC needs to ensure that writes that can cause interrupts to be
cleared reach the UART before returning from the ISR. This fix reads
a non-destructive register on the UART so the read transaction
completion ensures the previously queued write transaction has
also completed.
> >
> > + case UPIO_DWAPB:
> > + /* Save the LCR value so it can be re-written when a
> > + * Busy Detect interrupt occurs. */
> > + if (save_offset == UART_LCR)
> > + up->lcr = value;
> > + writeb(value, up->port.membase + offset);
> > + /* Read the IER to ensure any interrupt is cleared before
> > + * returning from ISR. */
> > + if ((save_offset == UART_TX || save_offset == UART_IER)
> && in_irq())
>
> The in_irq() is a worry. If the serial driver is used as the system
> console, then it can be called from _any_ interrupt handler. eg a printk()
> in the sata code.
>
> What happens then?
The in_irq() is there to improve performance. The logic being that
writing to registers outside the interrupt context will not require
the fix for issue 3.
There should be no issues if called from other interrupt context
as the read is non-destructive. I can remove the call to in_irq() if
you prefer.
> > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
> > handled = 1;
> >
> > end = NULL;
> > + } else if (up->port.iotype == UPIO_DWAPB &&
> > + (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
> > + /* The DesignWare APB UART has an Busy Detect
> (0x07)
> > + * interrupt meaning an LCR write attempt
> occured while the
> > + * UART was busy. The interrupt must be cleared
> by reading
> > + * the UART status register (USR) and the LCR
> re-written. */
> > + unsigned int status;
> > + status = *(volatile u32 *)up->port.private_data;
>
> Are you sure this is right? The use of volatile is generally discouraged
> and is a sign that something is wrong. What is the driver attempting to
> read here? Should it be using readl()?
The driver is reading the UART's USR (UART Status Register) which is
a non-standard register at a non-fixed offset, hence the need for
private_data. This was discussed in detail in the patch thread and the
goal was to avoid using platform specific #ifdefs as part of the fix
The register is accessed in kseg1 (unmapped on the mips architecture) so
readl is not needed. The register definitions in this space are all marked
volatile but since it's now accessed through private_data, the read had
to be made explicitly volatile.
> Also, the newly-added private_data field is not being initialised to
> anything anywhere in this patch.
private_data is initialized in the platform specific initialization code
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Andrew Morton wrote:
On Mon, 12 Feb 2007 12:04:08 -0600 Marc St-Jean
[EMAIL PROTECTED] wrote:
There are three different fixes:
1. Fix for DesignWare THRE errata
- Dropped our fix since the 8250-uart-backup-timer.patch in the mm
tree also fixes it. This patch needs to be applied on top of mm patch.
2. Fix for Busy Detect on LCR write
- No changes since last patch.
3. Workaround for interrupt/data concurrency issue
- No changes since last patch.
A couple of things.
- When preparing a changelog, please just tell us what the patch does.
The information about how this patch differes from a previous version of
the patch is irrelevant when the patch hits the mainline repository hence
I must edit it all.
It's OK to add the what-i-changed-since-last-time details, but please
make
that separate and remember that it will be removed for when the patch
goes
upstream.
- When fixing a bug, please tell us in detail what that bug *is*. None
of the above three items tell us any of this, which is essential
information for those who are to review the change.
Understood, I'm adding the original description here and I'll add to the
next patch update.
1. Fix for DesignWare APB THRE errata:
In brief, this is a non-standard 16550 in that the THRE interrupt
will not re-assert itself simply by disabling and re-enabling the
THRI bit in the IER, it is only re-enabled if a character is actually
sent out.
It appears that the 8250-uart-backup-timer.patch in the mm tree also
fixes it so we have dropped our initial workaround.
This patch now needs to be applied on top of that mm patch.
2. Fix for Busy Detect on LCR write:
The DesignWare APB UART has a feature which causes a new Busy Detect
interrupt to be generated if it's busy when the LCR is written. This
fix saves the value of the LCR and rewrites it after clearing the
interrupt.
3. Workaround for interrupt/data concurrency issue:
The SoC needs to ensure that writes that can cause interrupts to be
cleared reach the UART before returning from the ISR. This fix reads
a non-destructive register on the UART so the read transaction
completion ensures the previously queued write transaction has
also completed.
+ case UPIO_DWAPB:
+ /* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+ if (save_offset == UART_LCR)
+ up-lcr = value;
+ writeb(value, up-port.membase + offset);
+ /* Read the IER to ensure any interrupt is cleared before
+ * returning from ISR. */
+ if ((save_offset == UART_TX || save_offset == UART_IER)
in_irq())
The in_irq() is a worry. If the serial driver is used as the system
console, then it can be called from _any_ interrupt handler. eg a printk()
in the sata code.
What happens then?
The in_irq() is there to improve performance. The logic being that
writing to registers outside the interrupt context will not require
the fix for issue 3.
There should be no issues if called from other interrupt context
as the read is non-destructive. I can remove the call to in_irq() if
you prefer.
@@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
handled = 1;
end = NULL;
+ } else if (up-port.iotype == UPIO_DWAPB
+ (iir UART_IIR_BUSY) == UART_IIR_BUSY) {
+ /* The DesignWare APB UART has an Busy Detect
(0x07)
+ * interrupt meaning an LCR write attempt
occured while the
+ * UART was busy. The interrupt must be cleared
by reading
+ * the UART status register (USR) and the LCR
re-written. */
+ unsigned int status;
+ status = *(volatile u32 *)up-port.private_data;
Are you sure this is right? The use of volatile is generally discouraged
and is a sign that something is wrong. What is the driver attempting to
read here? Should it be using readl()?
The driver is reading the UART's USR (UART Status Register) which is
a non-standard register at a non-fixed offset, hence the need for
private_data. This was discussed in detail in the patch thread and the
goal was to avoid using platform specific #ifdefs as part of the fix
The register is accessed in kseg1 (unmapped on the mips architecture) so
readl is not needed. The register definitions in this space are all marked
volatile but since it's now accessed through private_data, the read had
to be made explicitly volatile.
Also, the newly-added private_data field is not being initialised to
anything anywhere in this patch.
private_data is initialized in the platform specific initialization code
which will be submitted to the l-m.o That patch contains only code which
lives in arch/mips and include/asm-mips so I was not
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: >> > Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx >>device. I think you need to submit your patch to Andrew Morton since it requires a patch from his tree. OK, will do. In fact, since the serial drivers are not maintained anymore, this seems the only option. >> > + /* The DesignWare APB UART has an Busy Detect >>(0x07) >> > + * interrupt meaning an LCR write attempt >>occured while the >> > + * UART was busy. The interrupt must be cleared >>by reading >> > + * the UART status register (USR) and the LCR >>re-written. */ >> > + unsigned int status; >> > + status = *(volatile u32 *)up->port.private_data; >> > + serial_out(up, UART_LCR, up->lcr); >> > + >> > + handled = 1; >> > + >> > + end = NULL; >> > } else if (end == NULL) >> > end = l; >> > return 0; >>Still, shouldn't you be doing this in serial8250_timeout() > No, the serial8250_timeout is for issue 1 at the top, this is for > issue 2. It's for lost interrupts, IIUC. They use anothe timeout handler for the workaround... This issue (2) is a completely new type of interrupt generated but the DesignWare APB uart, it has nothing to do with lost interrupts. Yeah, I just thought that the lost interrupts might be a "generic" issue. >>also? >>What IRQ numbers this UART is using, BTW? > For the ports on the device they are 27 and 42. Is there any significance > that I'm not aware of? Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls back to using serial8250_timeout() to handle "interrupts". Good to know. It won't be affecting us then. This may be overriden anyway... >>Oops, your mailer went and did it again. :-) > I'm completely giving up on Thunderbird,unless someone can point out Ypu should have long ago. :-) > the specific internal configuration items which needs a kick! Only the attachments will work in the Mozilla kind mailer, AFAIK. The last patch looked OK at last. :-) The attachemnents appear to be MIME which is a no-no according the The text/plain type attachments seem to be acceptable for the most maintainers. This Mozilla can do, at least. :-) linux FAQ at kernel.org. I guess I'll stick with /bin/mail. Thanks, Marc WBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
> Hello.
>
> Marc St-Jean wrote:
>
> >> > Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
> >>device.
>
> I think you need to submit your patch to Andrew Morton since it
> requires a patch from his tree.
OK, will do.
> >> > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
> >> > handled = 1;
> >> >
> >> > end = NULL;
> >> > + } else if (up->port.iotype == UPIO_DWAPB &&
> >> > + (iir & UART_IIR_BUSY) ==
> UART_IIR_BUSY) {
>
> >> Worth aligning this line with the opening paren of if... but's
> that's
> >>nitpicking. :-)
>
> > No problem I'll change it. I just usually align to the closest tab
> stop to
> > the opening parenthesis.
>
> It haven't really changed in the last patch. :-)
I will respin with 8 char tabs.
> >> > + /* The DesignWare APB UART has an Busy Detect
> >>(0x07)
> >> > + * interrupt meaning an LCR write attempt
> >>occured while the
> >> > + * UART was busy. The interrupt must be cleared
> >>by reading
> >> > + * the UART status register (USR) and the LCR
> >>re-written. */
> >> > + unsigned int status;
> >> > + status = *(volatile u32
> *)up->port.private_data;
> >> > + serial_out(up, UART_LCR, up->lcr);
> >> > +
> >> > + handled = 1;
> >> > +
> >> > + end = NULL;
> >> > } else if (end == NULL)
> >> > end = l;
> >> >
> >> > return 0;
>
> >>Still, shouldn't you be doing this in serial8250_timeout()
>
> > No, the serial8250_timeout is for issue 1 at the top, this is for
> > issue 2.
>
> It's for lost interrupts, IIUC. They use anothe timeout handler for the
> workaround...
This issue (2) is a completely new type of interrupt generated but the
DesignWare APB uart, it has nothing to do with lost interrupts.
> >>also?
> >>What IRQ numbers this UART is using, BTW?
>
> > For the ports on the device they are 27 and 42. Is there any
> significance
> > that I'm not aware of?
>
> Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls
> back to
> using serial8250_timeout() to handle "interrupts".
Good to know. It won't be affecting us then.
>
> >>Oops, your mailer went and did it again. :-)
>
> > I'm completely giving up on Thunderbird,unless someone can point out
>
> Ypu should have long ago. :-)
>
> > the specific internal configuration items which needs a kick!
>
> Only the attachments will work in the Mozilla kind mailer, AFAIK.
> The last patch looked OK at last. :-)
The attachemnents appear to be MIME which is a no-no according the
linux FAQ at kernel.org. I guess I'll stick with /bin/mail.
Thanks,
Marc
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
Hello.
Marc St-Jean wrote:
Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
device.
I think you need to submit your patch to Andrew Morton since it
requires a patch from his tree.
OK, will do.
@@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
handled = 1;
end = NULL;
+ } else if (up-port.iotype == UPIO_DWAPB
+ (iir UART_IIR_BUSY) ==
UART_IIR_BUSY) {
Worth aligning this line with the opening paren of if... but's
that's
nitpicking. :-)
No problem I'll change it. I just usually align to the closest tab
stop to
the opening parenthesis.
It haven't really changed in the last patch. :-)
I will respin with 8 char tabs.
+ /* The DesignWare APB UART has an Busy Detect
(0x07)
+ * interrupt meaning an LCR write attempt
occured while the
+ * UART was busy. The interrupt must be cleared
by reading
+ * the UART status register (USR) and the LCR
re-written. */
+ unsigned int status;
+ status = *(volatile u32
*)up-port.private_data;
+ serial_out(up, UART_LCR, up-lcr);
+
+ handled = 1;
+
+ end = NULL;
} else if (end == NULL)
end = l;
return 0;
Still, shouldn't you be doing this in serial8250_timeout()
No, the serial8250_timeout is for issue 1 at the top, this is for
issue 2.
It's for lost interrupts, IIUC. They use anothe timeout handler for the
workaround...
This issue (2) is a completely new type of interrupt generated but the
DesignWare APB uart, it has nothing to do with lost interrupts.
also?
What IRQ numbers this UART is using, BTW?
For the ports on the device they are 27 and 42. Is there any
significance
that I'm not aware of?
Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls
back to
using serial8250_timeout() to handle interrupts.
Good to know. It won't be affecting us then.
Oops, your mailer went and did it again. :-)
I'm completely giving up on Thunderbird,unless someone can point out
Ypu should have long ago. :-)
the specific internal configuration items which needs a kick!
Only the attachments will work in the Mozilla kind mailer, AFAIK.
The last patch looked OK at last. :-)
The attachemnents appear to be MIME which is a no-no according the
linux FAQ at kernel.org. I guess I'll stick with /bin/mail.
Thanks,
Marc
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx device. I think you need to submit your patch to Andrew Morton since it requires a patch from his tree. OK, will do. In fact, since the serial drivers are not maintained anymore, this seems the only option. + /* The DesignWare APB UART has an Busy Detect (0x07) + * interrupt meaning an LCR write attempt occured while the + * UART was busy. The interrupt must be cleared by reading + * the UART status register (USR) and the LCR re-written. */ + unsigned int status; + status = *(volatile u32 *)up-port.private_data; + serial_out(up, UART_LCR, up-lcr); + + handled = 1; + + end = NULL; } else if (end == NULL) end = l; return 0; Still, shouldn't you be doing this in serial8250_timeout() No, the serial8250_timeout is for issue 1 at the top, this is for issue 2. It's for lost interrupts, IIUC. They use anothe timeout handler for the workaround... This issue (2) is a completely new type of interrupt generated but the DesignWare APB uart, it has nothing to do with lost interrupts. Yeah, I just thought that the lost interrupts might be a generic issue. also? What IRQ numbers this UART is using, BTW? For the ports on the device they are 27 and 42. Is there any significance that I'm not aware of? Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls back to using serial8250_timeout() to handle interrupts. Good to know. It won't be affecting us then. This may be overriden anyway... Oops, your mailer went and did it again. :-) I'm completely giving up on Thunderbird,unless someone can point out Ypu should have long ago. :-) the specific internal configuration items which needs a kick! Only the attachments will work in the Mozilla kind mailer, AFAIK. The last patch looked OK at last. :-) The attachemnents appear to be MIME which is a no-no according the The text/plain type attachments seem to be acceptable for the most maintainers. This Mozilla can do, at least. :-) linux FAQ at kernel.org. I guess I'll stick with /bin/mail. Thanks, Marc WBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello.
Marc St-Jean wrote:
> Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
device.
> There are three different fixes:
> 1. Fix for DesignWare THRE errata
> - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
> tree also fixes it. This patch needs to be applied on top of "mm" patch.
I think you need to submit your patch to Andrew Morton since it requires a
patch from his tree.
> @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
> handled = 1;
>
> end = NULL;
> + } else if (up->port.iotype == UPIO_DWAPB &&
> + (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
Worth aligning this line with the opening paren of if... but's that's
nitpicking. :-)
No problem I'll change it. I just usually align to the closest tab stop to
the opening parenthesis.
It haven't really changed in the last patch. :-)
> + /* The DesignWare APB UART has an Busy Detect
(0x07)
> + * interrupt meaning an LCR write attempt
occured while the
> + * UART was busy. The interrupt must be cleared
by reading
> + * the UART status register (USR) and the LCR
re-written. */
> + unsigned int status;
> + status = *(volatile u32 *)up->port.private_data;
> + serial_out(up, UART_LCR, up->lcr);
> +
> + handled = 1;
> +
> + end = NULL;
> } else if (end == NULL)
> end = l;
>
> return 0;
Still, shouldn't you be doing this in serial8250_timeout()
No, the serial8250_timeout is for issue 1 at the top, this is for
issue 2.
It's for lost interrupts, IIUC. They use anothe timeout handler for the
workaround...
also?
What IRQ numbers this UART is using, BTW?
For the ports on the device they are 27 and 42. Is there any significance
that I'm not aware of?
Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls back to
using serial8250_timeout() to handle "interrupts".
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index cf23813..bd9711a 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -276,6 +277,7 @@ #define UPF_USR_MASK ((__force upf_t) (
> struct device *dev; /* parent
device */
> unsigned char hub6; /* this should
be in the 8250 driver */
> unsigned char unused[3];
> + void*private_data; /*
generic platform data pointer */
One tab to many before *private_data...
In my editor using 4 columns per tab it lines up with everything. Is there
some convention that patches should be made assuming 8 columns?
Documentation/CodingStyle, chapter 1. :-)
Oops, your mailer went and did it again. :-)
I'm completely giving up on Thunderbird,unless someone can point out
Ypu should have long ago. :-)
the specific internal configuration items which needs a kick!
Only the attachments will work in the Mozilla kind mailer, AFAIK.
The last patch looked OK at last. :-)
Thanks,
Marc
WBR, Sergei
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello.
Marc St-Jean wrote:
Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
device.
There are three different fixes:
1. Fix for DesignWare THRE errata
- Dropped our fix since the 8250-uart-backup-timer.patch in the mm
tree also fixes it. This patch needs to be applied on top of mm patch.
I think you need to submit your patch to Andrew Morton since it requires a
patch from his tree.
@@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
handled = 1;
end = NULL;
+ } else if (up-port.iotype == UPIO_DWAPB
+ (iir UART_IIR_BUSY) == UART_IIR_BUSY) {
Worth aligning this line with the opening paren of if... but's that's
nitpicking. :-)
No problem I'll change it. I just usually align to the closest tab stop to
the opening parenthesis.
It haven't really changed in the last patch. :-)
+ /* The DesignWare APB UART has an Busy Detect
(0x07)
+ * interrupt meaning an LCR write attempt
occured while the
+ * UART was busy. The interrupt must be cleared
by reading
+ * the UART status register (USR) and the LCR
re-written. */
+ unsigned int status;
+ status = *(volatile u32 *)up-port.private_data;
+ serial_out(up, UART_LCR, up-lcr);
+
+ handled = 1;
+
+ end = NULL;
} else if (end == NULL)
end = l;
return 0;
Still, shouldn't you be doing this in serial8250_timeout()
No, the serial8250_timeout is for issue 1 at the top, this is for
issue 2.
It's for lost interrupts, IIUC. They use anothe timeout handler for the
workaround...
also?
What IRQ numbers this UART is using, BTW?
For the ports on the device they are 27 and 42. Is there any significance
that I'm not aware of?
Yeah, IRQ0 is treated as no IRQ by 8250, and in this case it falls back to
using serial8250_timeout() to handle interrupts.
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index cf23813..bd9711a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -276,6 +277,7 @@ #define UPF_USR_MASK ((__force upf_t) (
struct device *dev; /* parent
device */
unsigned char hub6; /* this should
be in the 8250 driver */
unsigned char unused[3];
+ void*private_data; /*
generic platform data pointer */
One tab to many before *private_data...
In my editor using 4 columns per tab it lines up with everything. Is there
some convention that patches should be made assuming 8 columns?
Documentation/CodingStyle, chapter 1. :-)
Oops, your mailer went and did it again. :-)
I'm completely giving up on Thunderbird,unless someone can point out
Ypu should have long ago. :-)
the specific internal configuration items which needs a kick!
Only the attachments will work in the Mozilla kind mailer, AFAIK.
The last patch looked OK at last. :-)
Thanks,
Marc
WBR, Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
> Marc St-Jean wrote:
> > Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
> device.
> >
> > There are three different fixes:
> > 1. Fix for DesignWare THRE errata
> > - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
> > tree also fixes it. This patch needs to be applied on top of "mm" patch.
> >
> > 2. Fix for Busy Detect on LCR write
> > - Changed the ordering of test in serial8250_interrupt().
> > - Combined test for UPIO_DWAPB with UPIO_MEM in
> serial8250_start_console().
> > - Renamed new uart_8250_port member to private_data.
> >
> > 3. Workaround for interrupt/data concurrency issue
> > - No changes since last patch.
>
> > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
> > handled = 1;
> >
> > end = NULL;
> > + } else if (up->port.iotype == UPIO_DWAPB &&
> > + (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
>
> Worth aligning this line with the opening paren of if... but's that's
> nitpicking. :-)
No problem I'll change it. I just usually align to the closest tab stop to
the opening parenthesis.
> > + /* The DesignWare APB UART has an Busy Detect
> (0x07)
> > + * interrupt meaning an LCR write attempt
> occured while the
> > + * UART was busy. The interrupt must be cleared
> by reading
> > + * the UART status register (USR) and the LCR
> re-written. */
> > + unsigned int status;
> > + status = *(volatile u32 *)up->port.private_data;
> > + serial_out(up, UART_LCR, up->lcr);
> > +
> > + handled = 1;
> > +
> > + end = NULL;
> > } else if (end == NULL)
> > end = l;
> >
> > return 0;
>
> Still, shouldn't you be doing this in serial8250_timeout()
No, the serial8250_timeout is for issue 1 at the top, this is for
issue 2.
> also?
> What IRQ numbers this UART is using, BTW?
For the ports on the device they are 27 and 42. Is there any significance
that I'm not aware of?
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index cf23813..bd9711a 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -276,6 +277,7 @@ #define UPF_USR_MASK ((__force upf_t) (
> > struct device *dev; /* parent
> device */
> > unsigned char hub6; /* this should
> be in the 8250 driver */
> > unsigned char unused[3];
> > + void*private_data; /*
> generic platform data pointer */
>
> One tab to many before *private_data...
In my editor using 4 columns per tab it lines up with everything. Is there
some convention that patches should be made assuming 8 columns?
> > diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
> > index 3c8a6aa..1c5ed7d 100644
> > --- a/include/linux/serial_reg.h
> > +++ b/include/linux/serial_reg.h
> > @@ -38,6 +38,8 @@ #define UART_IIR_THRI 0x02 /* Transmitt
> > #define UART_IIR_RDI0x04 /* Receiver data interrupt */
> > #define UART_IIR_RLSI 0x06 /* Receiver line status
> interrupt */
> >
> > +#define UART_IIR_BUSY0x07 /* DesignWare APB Busy
> Detect */
> > +
> > #define UART_FCR2 /* Out: FIFO Control Register */
> > #define UART_FCR_ENABLE_FIFO0x01 /* Enable the FIFO */
> > #define UART_FCR_CLEAR_RCVR 0x02 /* Clear the RCVR FIFO */
>
> Oops, your mailer went and did it again. :-)
I'm completely giving up on Thunderbird,unless someone can point out
the specific internal configuration items which needs a kick!
Thanks,
Marc
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
Marc St-Jean wrote:
Fourth attempt at the serial driver patch for the PMC-Sierra MSP71xx
device.
There are three different fixes:
1. Fix for DesignWare THRE errata
- Dropped our fix since the 8250-uart-backup-timer.patch in the mm
tree also fixes it. This patch needs to be applied on top of mm patch.
2. Fix for Busy Detect on LCR write
- Changed the ordering of test in serial8250_interrupt().
- Combined test for UPIO_DWAPB with UPIO_MEM in
serial8250_start_console().
- Renamed new uart_8250_port member to private_data.
3. Workaround for interrupt/data concurrency issue
- No changes since last patch.
@@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
handled = 1;
end = NULL;
+ } else if (up-port.iotype == UPIO_DWAPB
+ (iir UART_IIR_BUSY) == UART_IIR_BUSY) {
Worth aligning this line with the opening paren of if... but's that's
nitpicking. :-)
No problem I'll change it. I just usually align to the closest tab stop to
the opening parenthesis.
+ /* The DesignWare APB UART has an Busy Detect
(0x07)
+ * interrupt meaning an LCR write attempt
occured while the
+ * UART was busy. The interrupt must be cleared
by reading
+ * the UART status register (USR) and the LCR
re-written. */
+ unsigned int status;
+ status = *(volatile u32 *)up-port.private_data;
+ serial_out(up, UART_LCR, up-lcr);
+
+ handled = 1;
+
+ end = NULL;
} else if (end == NULL)
end = l;
return 0;
Still, shouldn't you be doing this in serial8250_timeout()
No, the serial8250_timeout is for issue 1 at the top, this is for
issue 2.
also?
What IRQ numbers this UART is using, BTW?
For the ports on the device they are 27 and 42. Is there any significance
that I'm not aware of?
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index cf23813..bd9711a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -276,6 +277,7 @@ #define UPF_USR_MASK ((__force upf_t) (
struct device *dev; /* parent
device */
unsigned char hub6; /* this should
be in the 8250 driver */
unsigned char unused[3];
+ void*private_data; /*
generic platform data pointer */
One tab to many before *private_data...
In my editor using 4 columns per tab it lines up with everything. Is there
some convention that patches should be made assuming 8 columns?
diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
index 3c8a6aa..1c5ed7d 100644
--- a/include/linux/serial_reg.h
+++ b/include/linux/serial_reg.h
@@ -38,6 +38,8 @@ #define UART_IIR_THRI 0x02 /* Transmitt
#define UART_IIR_RDI0x04 /* Receiver data interrupt */
#define UART_IIR_RLSI 0x06 /* Receiver line status
interrupt */
+#define UART_IIR_BUSY0x07 /* DesignWare APB Busy
Detect */
+
#define UART_FCR2 /* Out: FIFO Control Register */
#define UART_FCR_ENABLE_FIFO0x01 /* Enable the FIFO */
#define UART_FCR_CLEAR_RCVR 0x02 /* Clear the RCVR FIFO */
Oops, your mailer went and did it again. :-)
I'm completely giving up on Thunderbird,unless someone can point out
the specific internal configuration items which needs a kick!
Thanks,
Marc
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
> Marc St-Jean wrote:
> > Third attempt at the serial driver patch for the PMC-Sierra MSP71xx
> device.
> >
> > There are three different fixes:
> > 1. Fix for DesignWare THRE errata
> > - Dropped our fix since the "8250-uart-backup-timer.patch" in the "mm"
> > tree also fixes it. This patch needs to be applied on top of it.
> >
> > 2. Fix for Busy Detect on LCR write
> > - Dropped the addition of UPIO_DWAPB iotype to 8250_early.c as Sergei
> > pointed out the fix wasn't complete and we don't require it.
> >
> > 3. Workaround for interrupt/data concurrency issue
> > - Fix must remain serial8250_interrupt() in order to mark interrupt as
> > handled.
> >
> > Thanks,
> > Marc
> >
> > Signed-off-by: Marc St-Jean <[EMAIL PROTECTED]>
> >
> > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
...
> > break;
> > @@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
> > handled = 1;
> >
> > end = NULL;
> > + } else if ((iir & UART_IIR_BUSY) == UART_IIR_BUSY &&
> > + up->port.iotype == UPIO_DWAPB) {
>
> Makes sense to swap the checks, i.e. to only test for UART_IIR_BUSY is
> this is UPIO_DWAPB.
I had left in the other order for readability with the previous test.
I agree this will save a few cycles so I've reordered.
> [...]
>
> > @@ -2454,9 +2485,12 @@ int __init serial8250_start_console(stru
> >
> > add_preferred_console("ttyS", line, options);
> > printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
> > - line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
> > - port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
> > - (unsigned long) port->iobase, options);
> > + line,
> > + (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
> > + ? "MMIO" : "I/O port",
> > + (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
> > + ? (unsigned long) port->mapbase : (unsigned
> long) port->iobase,
> > + options);
>
> Please turn this check into port->iotype >= UPIO_MEM, since this
> would be
> the Right Thing (RM). All iotypes beyond UPIO_MEM are memory mapped.
> And I
> thought I fixed that -- was wrong, obviously... :-/
This is news to me, I thought the numbering was of no particular importance.
I've made the change.
> > diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
> > index 3c8a6aa..b3550cc 100644
> > --- a/include/linux/serial_reg.h
> > +++ b/include/linux/serial_reg.h
> > @@ -37,6 +37,7 @@ #define UART_IIR_MSI0x00 /* Modem stat
> > #define UART_IIR_THRI 0x02 /* Transmitter holding
> register empty */
> > #define UART_IIR_RDI0x04 /* Receiver data interrupt */
> > #define UART_IIR_RLSI 0x06 /* Receiver line status
> interrupt */
> > +#define UART_IIR_BUSY0x07 /* DesignWare APB Busy
> Detect */
>
> Alan already said about this one... :-)
Yes, changed.
> BTW, your patches are still corrupt by your mailer (space added to
> lines
> starting with space)
Argh! There were no spaces in the message window before sending, I swear!
I've solved the problem for the next patch.
Marc
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
Marc St-Jean wrote:
Third attempt at the serial driver patch for the PMC-Sierra MSP71xx
device.
There are three different fixes:
1. Fix for DesignWare THRE errata
- Dropped our fix since the 8250-uart-backup-timer.patch in the mm
tree also fixes it. This patch needs to be applied on top of it.
2. Fix for Busy Detect on LCR write
- Dropped the addition of UPIO_DWAPB iotype to 8250_early.c as Sergei
pointed out the fix wasn't complete and we don't require it.
3. Workaround for interrupt/data concurrency issue
- Fix must remain serial8250_interrupt() in order to mark interrupt as
handled.
Thanks,
Marc
Signed-off-by: Marc St-Jean [EMAIL PROTECTED]
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
...
break;
@@ -1383,6 +1399,19 @@ static irqreturn_t serial8250_interrupt(
handled = 1;
end = NULL;
+ } else if ((iir UART_IIR_BUSY) == UART_IIR_BUSY
+ up-port.iotype == UPIO_DWAPB) {
Makes sense to swap the checks, i.e. to only test for UART_IIR_BUSY is
this is UPIO_DWAPB.
I had left in the other order for readability with the previous test.
I agree this will save a few cycles so I've reordered.
[...]
@@ -2454,9 +2485,12 @@ int __init serial8250_start_console(stru
add_preferred_console(ttyS, line, options);
printk(Adding console on ttyS%d at %s 0x%lx (options '%s')\n,
- line, port-iotype == UPIO_MEM ? MMIO : I/O port,
- port-iotype == UPIO_MEM ? (unsigned long) port-mapbase :
- (unsigned long) port-iobase, options);
+ line,
+ (port-iotype == UPIO_MEM || port-iotype == UPIO_DWAPB)
+ ? MMIO : I/O port,
+ (port-iotype == UPIO_MEM || port-iotype == UPIO_DWAPB)
+ ? (unsigned long) port-mapbase : (unsigned
long) port-iobase,
+ options);
Please turn this check into port-iotype = UPIO_MEM, since this
would be
the Right Thing (RM). All iotypes beyond UPIO_MEM are memory mapped.
And I
thought I fixed that -- was wrong, obviously... :-/
This is news to me, I thought the numbering was of no particular importance.
I've made the change.
diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h
index 3c8a6aa..b3550cc 100644
--- a/include/linux/serial_reg.h
+++ b/include/linux/serial_reg.h
@@ -37,6 +37,7 @@ #define UART_IIR_MSI0x00 /* Modem stat
#define UART_IIR_THRI 0x02 /* Transmitter holding
register empty */
#define UART_IIR_RDI0x04 /* Receiver data interrupt */
#define UART_IIR_RLSI 0x06 /* Receiver line status
interrupt */
+#define UART_IIR_BUSY0x07 /* DesignWare APB Busy
Detect */
Alan already said about this one... :-)
Yes, changed.
BTW, your patches are still corrupt by your mailer (space added to
lines
starting with space)
Argh! There were no spaces in the message window before sending, I swear!
I've solved the problem for the next patch.
Marc
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Thank Alan. I made the changes yesterday but I'll wait another day before reposting, in case other interested people have more comments. Marc Alan wrote: > > unsigned char hub6; /* this should > be in the 8250 driver */ > > unsigned char unused[3]; > > + void*data; /* > generic platform data pointer */ > > }; > > Convention is "private_data" > > > > > /* > > diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h > > index 3c8a6aa..b3550cc 100644 > > --- a/include/linux/serial_reg.h > > +++ b/include/linux/serial_reg.h > > @@ -37,6 +37,7 @@ #define UART_IIR_MSI0x00 /* Modem stat > > #define UART_IIR_THRI 0x02 /* Transmitter holding > register empty */ > > #define UART_IIR_RDI0x04 /* Receiver data interrupt */ > > #define UART_IIR_RLSI 0x06 /* Receiver line status > interrupt */ > > +#define UART_IIR_BUSY0x07 /* DesignWare APB Busy > Detect */ > > Please move this down a line to break it from "official" values and call > it DESIGNWARE_UART_IIR_BUSY, so it is obviously designware specific. > > Otherwise looks much less invasive and messy > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Thank Alan. I made the changes yesterday but I'll wait another day before reposting, in case other interested people have more comments. Marc Alan wrote: unsigned char hub6; /* this should be in the 8250 driver */ unsigned char unused[3]; + void*data; /* generic platform data pointer */ }; Convention is private_data /* diff --git a/include/linux/serial_reg.h b/include/linux/serial_reg.h index 3c8a6aa..b3550cc 100644 --- a/include/linux/serial_reg.h +++ b/include/linux/serial_reg.h @@ -37,6 +37,7 @@ #define UART_IIR_MSI0x00 /* Modem stat #define UART_IIR_THRI 0x02 /* Transmitter holding register empty */ #define UART_IIR_RDI0x04 /* Receiver data interrupt */ #define UART_IIR_RLSI 0x06 /* Receiver line status interrupt */ +#define UART_IIR_BUSY0x07 /* DesignWare APB Busy Detect */ Please move this down a line to break it from official values and call it DESIGNWARE_UART_IIR_BUSY, so it is obviously designware specific. Otherwise looks much less invasive and messy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
> Hello.
>
> Marc St-Jean wrote:
>
> >> > Index: linux_2_6/drivers/serial/8250.c
> >> > ===
> >> > RCS file: linux_2_6/drivers/serial/8250.c,v
> >> > retrieving revision 1.1.1.7
> >> > diff -u -r1.1.1.7 8250.c
> >> > --- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58 -
> >>1.1.1.7
> >> > +++ linux_2_6/drivers/serial/8250.c 24 Jan 2007 23:55:27 -
> >>[...]
> >> > @@ -333,6 +334,8 @@
> >> > static void
> >> > serial_out(struct uart_8250_port *up, int offset, int value)
>
> >> Your patch is clearly garbled again, something added an extra space
> >>to all
> >>lines stating with space... :-/
>
> > I see that but was under the impression cvs diff did that so all lines
> > lineup correctly whether they have a +, -, or neither.
>
> Yes. And your mailer has probably added another space to lines already
> begginign with space. Some mailers tend to do it, don't know sure why...
> :-/
I'll look into it for the next patch.
> > It doesn't affect applying the patches for me, did you try?
>
> I hadn't but I had no doubts it'll fail... just tried, not a single
> hunk
> applied. :-]
>
> >> > {
> >> > + /* Save the offset before it's remapped */
> >> > + int save_offset = offset;
>
> >>Is there real need to save this? What regshift equals for this UART?
>
> > Yes, we have a regshift of 2 on this SoC and it could be different on
> other
> > SoCs which reuse the UART IP.
>
> Agreed then (though still seems avoidable).
>
> >> > offset = map_8250_out_reg(up, offset) << up->port.regshift;
>
> >> > switch (up->port.iotype) {
> >> > @@ -359,6 +362,19 @@
> >> > writeb(value, up->port.membase + offset);
> >> > break;
> >> >
> >> > + case UPIO_DWAPB:
> >> > + /* Save the LCR value so it can be re-written when a
> >> > + * Busy Detect interrupt occurs. */
> >> > + if (save_offset == UART_LCR)
> >> > + up->lcr = value;
> >> > + writeb(value, up->port.membase + offset);
> >> > + /* Read the IER to ensure any interrupt is cleared
> before
> >> > + * returning from ISR. */
> >> > + if ((save_offset == UART_TX || save_offset ==
> UART_IER) &&
>
> >>Not sure how an IER read ensures that...
>
> > It's just a dummy read to ensure that any writes which clear
> interrupts have
> > reached the device before returning from the IRQ.
>
> Hm, isn't it CPU write buffer flush? Shouldn't this be handled more
> generically?
No because this peripheral is across an internal SoC bus. Only a read
accessing it will ensure the write is complete. We must insure the
interrupt source is cleared to avoid spurious interrupts.
> [...]
> >> > Index: linux_2_6/drivers/serial/8250_early.c
> >> > ===
> >> > RCS file: linux_2_6/drivers/serial/8250_early.c,v
> >> > retrieving revision 1.1.1.3
> >> > diff -u -r1.1.1.3 8250_early.c
> >> > --- linux_2_6/drivers/serial/8250_early.c 19 Oct 2006 20:08:20
> >>- 1.1.1.3
> >> > +++ linux_2_6/drivers/serial/8250_early.c 24 Jan 2007 23:55:27
> -
> >> > @@ -46,7 +46,7 @@
> >> >
> >> > static unsigned int __init serial_in(struct uart_port *port, int
> >>offset)
> >> > {
> >> > - if (port->iotype == UPIO_MEM)
> >> > + if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
> >> > return readb(port->membase + offset);
> >> > else
> >> > return inb(port->iobase + offset);
> >> > @@ -54,7 +54,7 @@
> >> >
> >> > static void __init serial_out(struct uart_port *port, int offset,
> >>int value)
> >> > {
> >> > - if (port->iotype == UPIO_MEM)
> >> > + if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
> >> > writeb(value, port->membase + offset);
> >> > else
> >> > outb(value, port->iobase + offset);
> >> > @@ -233,7 +233,7 @@
> >> > return 0;
> >> >
> >> > /* Try to start the normal driver on a matching line. */
> >> > - mmio = (port->iotype == UPIO_MEM);
> >> > + mmio = (port->iotype == UPIO_MEM || port->iotype ==
> UPIO_DWAPB);
> >> > line = serial8250_start_console(port, device->options);
> >> > if (line < 0)
> >> > printk("No ttyS device at %s 0x%lx for console\n",
>
> >>From your 8250_eraly.c changes I can conclude regshift == 1 (it
> doesn't
> >>currently support other cases). So, serial_pot() doesn't need
> >>save_offset. :-)
>
> > Our regshift is definitely 2 on this SoC and we don't have any
> problems with
> > console output before the serial driver opens. So assuming it's using
> > 8250_early.c for initial console output I can only conclude that it
> works
>
>
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello.
Marc St-Jean wrote:
> Here is my second attempt at the serial driver patch for the
> PMC-Sierra MSP71xx device.
> There are three different fixes:
> 1. Fix for THRE errata
> - I verified the UART_BUG_TXEN fix does not help with this erratum.
> - I left our current fix in until I get our platform booting on
> 2.6.20-rc4 to try the mm tree "8250-uart-backup-timer.patch".
> Feel free to ignore for now.
> 2. Fix for Busy Detect on LCR write
> - Moved to new UPIO_DWAPB iotype. Because the new type is a memory
> mapped device and there are several tests for UPIO_MEM, this involved
> updating serial_core.c and 8250_early.c in addition to 8250.c.
> - I tried implementing this totally in serial_in as suggested, but
> it can't be done because of bit overlap between UART_IIR_NO_INT and
> UART_IIR_BUSY. Also there is no way to set the interrupt "handled = 1"
> from serial_in.
> 3. Workaround for interrupt/data concurrency issue
> - Moved to new UPIO_DWAPB iotype.
> Index: linux_2_6/drivers/serial/8250.c
> ===
> RCS file: linux_2_6/drivers/serial/8250.c,v
> retrieving revision 1.1.1.7
> diff -u -r1.1.1.7 8250.c
> --- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58 -
1.1.1.7
> +++ linux_2_6/drivers/serial/8250.c 24 Jan 2007 23:55:27 -
[...]
> @@ -333,6 +334,8 @@
> static void
> serial_out(struct uart_8250_port *up, int offset, int value)
Your patch is clearly garbled again, something added an extra space
to all
lines stating with space... :-/
I see that but was under the impression cvs diff did that so all lines
lineup correctly whether they have a +, -, or neither.
Yes. And your mailer has probably added another space to lines already
begginign with space. Some mailers tend to do it, don't know sure why... :-/
It doesn't affect applying the patches for me, did you try?
I hadn't but I had no doubts it'll fail... just tried, not a single hunk
applied. :-]
> {
> + /* Save the offset before it's remapped */
> + int save_offset = offset;
Is there real need to save this? What regshift equals for this UART?
Yes, we have a regshift of 2 on this SoC and it could be different on other
SoCs which reuse the UART IP.
Agreed then (though still seems avoidable).
> offset = map_8250_out_reg(up, offset) << up->port.regshift;
> switch (up->port.iotype) {
> @@ -359,6 +362,19 @@
> writeb(value, up->port.membase + offset);
> break;
>
> + case UPIO_DWAPB:
> + /* Save the LCR value so it can be re-written when a
> + * Busy Detect interrupt occurs. */
> + if (save_offset == UART_LCR)
> + up->lcr = value;
> + writeb(value, up->port.membase + offset);
> + /* Read the IER to ensure any interrupt is cleared before
> + * returning from ISR. */
> + if ((save_offset == UART_TX || save_offset == UART_IER) &&
Not sure how an IER read ensures that...
It's just a dummy read to ensure that any writes which clear interrupts have
reached the device before returning from the IRQ.
Hm, isn't it CPU write buffer flush? Shouldn't this be handled more
generically?
[...]
> Index: linux_2_6/drivers/serial/8250_early.c
> ===
> RCS file: linux_2_6/drivers/serial/8250_early.c,v
> retrieving revision 1.1.1.3
> diff -u -r1.1.1.3 8250_early.c
> --- linux_2_6/drivers/serial/8250_early.c 19 Oct 2006 20:08:20
- 1.1.1.3
> +++ linux_2_6/drivers/serial/8250_early.c 24 Jan 2007 23:55:27 -
> @@ -46,7 +46,7 @@
>
> static unsigned int __init serial_in(struct uart_port *port, int
offset)
> {
> - if (port->iotype == UPIO_MEM)
> + if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
> return readb(port->membase + offset);
> else
> return inb(port->iobase + offset);
> @@ -54,7 +54,7 @@
>
> static void __init serial_out(struct uart_port *port, int offset,
int value)
> {
> - if (port->iotype == UPIO_MEM)
> + if (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB)
> writeb(value, port->membase + offset);
> else
> outb(value, port->iobase + offset);
> @@ -233,7 +233,7 @@
> return 0;
>
> /* Try to start the normal driver on a matching line. */
> - mmio = (port->iotype == UPIO_MEM);
> + mmio = (port->iotype == UPIO_MEM || port->iotype == UPIO_DWAPB);
> line = serial8250_start_console(port, device->options);
> if (line < 0)
> printk("No ttyS device at %s 0x%lx for console\n",
From your 8250_eraly.c changes I can conclude regshift == 1 (it doesn't
currently support other cases). So, serial_pot() doesn't need
save_offset. :-)
Our regshift is definitely 2 on this SoC and we don't have any problems
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello.
Marc St-Jean wrote:
Here is my second attempt at the serial driver patch for the
PMC-Sierra MSP71xx device.
There are three different fixes:
1. Fix for THRE errata
- I verified the UART_BUG_TXEN fix does not help with this erratum.
- I left our current fix in until I get our platform booting on
2.6.20-rc4 to try the mm tree 8250-uart-backup-timer.patch.
Feel free to ignore for now.
2. Fix for Busy Detect on LCR write
- Moved to new UPIO_DWAPB iotype. Because the new type is a memory
mapped device and there are several tests for UPIO_MEM, this involved
updating serial_core.c and 8250_early.c in addition to 8250.c.
- I tried implementing this totally in serial_in as suggested, but
it can't be done because of bit overlap between UART_IIR_NO_INT and
UART_IIR_BUSY. Also there is no way to set the interrupt handled = 1
from serial_in.
3. Workaround for interrupt/data concurrency issue
- Moved to new UPIO_DWAPB iotype.
Index: linux_2_6/drivers/serial/8250.c
===
RCS file: linux_2_6/drivers/serial/8250.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 8250.c
--- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58 -
1.1.1.7
+++ linux_2_6/drivers/serial/8250.c 24 Jan 2007 23:55:27 -
[...]
@@ -333,6 +334,8 @@
static void
serial_out(struct uart_8250_port *up, int offset, int value)
Your patch is clearly garbled again, something added an extra space
to all
lines stating with space... :-/
I see that but was under the impression cvs diff did that so all lines
lineup correctly whether they have a +, -, or neither.
Yes. And your mailer has probably added another space to lines already
begginign with space. Some mailers tend to do it, don't know sure why... :-/
It doesn't affect applying the patches for me, did you try?
I hadn't but I had no doubts it'll fail... just tried, not a single hunk
applied. :-]
{
+ /* Save the offset before it's remapped */
+ int save_offset = offset;
Is there real need to save this? What regshift equals for this UART?
Yes, we have a regshift of 2 on this SoC and it could be different on other
SoCs which reuse the UART IP.
Agreed then (though still seems avoidable).
offset = map_8250_out_reg(up, offset) up-port.regshift;
switch (up-port.iotype) {
@@ -359,6 +362,19 @@
writeb(value, up-port.membase + offset);
break;
+ case UPIO_DWAPB:
+ /* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+ if (save_offset == UART_LCR)
+ up-lcr = value;
+ writeb(value, up-port.membase + offset);
+ /* Read the IER to ensure any interrupt is cleared before
+ * returning from ISR. */
+ if ((save_offset == UART_TX || save_offset == UART_IER)
Not sure how an IER read ensures that...
It's just a dummy read to ensure that any writes which clear interrupts have
reached the device before returning from the IRQ.
Hm, isn't it CPU write buffer flush? Shouldn't this be handled more
generically?
[...]
Index: linux_2_6/drivers/serial/8250_early.c
===
RCS file: linux_2_6/drivers/serial/8250_early.c,v
retrieving revision 1.1.1.3
diff -u -r1.1.1.3 8250_early.c
--- linux_2_6/drivers/serial/8250_early.c 19 Oct 2006 20:08:20
- 1.1.1.3
+++ linux_2_6/drivers/serial/8250_early.c 24 Jan 2007 23:55:27 -
@@ -46,7 +46,7 @@
static unsigned int __init serial_in(struct uart_port *port, int
offset)
{
- if (port-iotype == UPIO_MEM)
+ if (port-iotype == UPIO_MEM || port-iotype == UPIO_DWAPB)
return readb(port-membase + offset);
else
return inb(port-iobase + offset);
@@ -54,7 +54,7 @@
static void __init serial_out(struct uart_port *port, int offset,
int value)
{
- if (port-iotype == UPIO_MEM)
+ if (port-iotype == UPIO_MEM || port-iotype == UPIO_DWAPB)
writeb(value, port-membase + offset);
else
outb(value, port-iobase + offset);
@@ -233,7 +233,7 @@
return 0;
/* Try to start the normal driver on a matching line. */
- mmio = (port-iotype == UPIO_MEM);
+ mmio = (port-iotype == UPIO_MEM || port-iotype == UPIO_DWAPB);
line = serial8250_start_console(port, device-options);
if (line 0)
printk(No ttyS device at %s 0x%lx for console\n,
From your 8250_eraly.c changes I can conclude regshift == 1 (it doesn't
currently support other cases). So, serial_pot() doesn't need
save_offset. :-)
Our regshift is definitely 2 on this SoC and we don't have any problems with
console output before the serial driver opens. So assuming it's using
8250_early.c for initial console
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
Hello.
Marc St-Jean wrote:
Index: linux_2_6/drivers/serial/8250.c
===
RCS file: linux_2_6/drivers/serial/8250.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 8250.c
--- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58 -
1.1.1.7
+++ linux_2_6/drivers/serial/8250.c 24 Jan 2007 23:55:27 -
[...]
@@ -333,6 +334,8 @@
static void
serial_out(struct uart_8250_port *up, int offset, int value)
Your patch is clearly garbled again, something added an extra space
to all
lines stating with space... :-/
I see that but was under the impression cvs diff did that so all lines
lineup correctly whether they have a +, -, or neither.
Yes. And your mailer has probably added another space to lines already
begginign with space. Some mailers tend to do it, don't know sure why...
:-/
I'll look into it for the next patch.
It doesn't affect applying the patches for me, did you try?
I hadn't but I had no doubts it'll fail... just tried, not a single
hunk
applied. :-]
{
+ /* Save the offset before it's remapped */
+ int save_offset = offset;
Is there real need to save this? What regshift equals for this UART?
Yes, we have a regshift of 2 on this SoC and it could be different on
other
SoCs which reuse the UART IP.
Agreed then (though still seems avoidable).
offset = map_8250_out_reg(up, offset) up-port.regshift;
switch (up-port.iotype) {
@@ -359,6 +362,19 @@
writeb(value, up-port.membase + offset);
break;
+ case UPIO_DWAPB:
+ /* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+ if (save_offset == UART_LCR)
+ up-lcr = value;
+ writeb(value, up-port.membase + offset);
+ /* Read the IER to ensure any interrupt is cleared
before
+ * returning from ISR. */
+ if ((save_offset == UART_TX || save_offset ==
UART_IER)
Not sure how an IER read ensures that...
It's just a dummy read to ensure that any writes which clear
interrupts have
reached the device before returning from the IRQ.
Hm, isn't it CPU write buffer flush? Shouldn't this be handled more
generically?
No because this peripheral is across an internal SoC bus. Only a read
accessing it will ensure the write is complete. We must insure the
interrupt source is cleared to avoid spurious interrupts.
[...]
Index: linux_2_6/drivers/serial/8250_early.c
===
RCS file: linux_2_6/drivers/serial/8250_early.c,v
retrieving revision 1.1.1.3
diff -u -r1.1.1.3 8250_early.c
--- linux_2_6/drivers/serial/8250_early.c 19 Oct 2006 20:08:20
- 1.1.1.3
+++ linux_2_6/drivers/serial/8250_early.c 24 Jan 2007 23:55:27
-
@@ -46,7 +46,7 @@
static unsigned int __init serial_in(struct uart_port *port, int
offset)
{
- if (port-iotype == UPIO_MEM)
+ if (port-iotype == UPIO_MEM || port-iotype == UPIO_DWAPB)
return readb(port-membase + offset);
else
return inb(port-iobase + offset);
@@ -54,7 +54,7 @@
static void __init serial_out(struct uart_port *port, int offset,
int value)
{
- if (port-iotype == UPIO_MEM)
+ if (port-iotype == UPIO_MEM || port-iotype == UPIO_DWAPB)
writeb(value, port-membase + offset);
else
outb(value, port-iobase + offset);
@@ -233,7 +233,7 @@
return 0;
/* Try to start the normal driver on a matching line. */
- mmio = (port-iotype == UPIO_MEM);
+ mmio = (port-iotype == UPIO_MEM || port-iotype ==
UPIO_DWAPB);
line = serial8250_start_console(port, device-options);
if (line 0)
printk(No ttyS device at %s 0x%lx for console\n,
From your 8250_eraly.c changes I can conclude regshift == 1 (it
doesn't
currently support other cases). So, serial_pot() doesn't need
save_offset. :-)
Our regshift is definitely 2 on this SoC and we don't have any
problems with
console output before the serial driver opens. So assuming it's using
8250_early.c for initial console output I can only conclude that it
works
It comes into action only if you specify console=uart,... kernel option
for the eraly console support. The plain 8250 console driver is
containded
within 8250.c itself.
because UART_TX is offset 0 and the port was left configured from our
ROM monitor.
Well, this part of the patch can't be considered complete then (how LSR
polling is going to work?),
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
> Marc St-Jean wrote:
> > Here is my second attempt at the serial driver patch for the
> > PMC-Sierra MSP71xx device.
> >
> > There are three different fixes:
> > 1. Fix for THRE errata
> > - I verified the UART_BUG_TXEN fix does not help with this erratum.
> > - I left our current fix in until I get our platform booting on
> > 2.6.20-rc4 to try the mm tree "8250-uart-backup-timer.patch".
> > Feel free to ignore for now.
> >
> > 2. Fix for Busy Detect on LCR write
> > - Moved to new UPIO_DWAPB iotype. Because the new type is a memory
> > mapped device and there are several tests for UPIO_MEM, this involved
> > updating serial_core.c and 8250_early.c in addition to 8250.c.
> > - I tried implementing this totally in serial_in as suggested, but
> > it can't be done because of bit overlap between UART_IIR_NO_INT and
> > UART_IIR_BUSY. Also there is no way to set the interrupt "handled = 1"
> > from serial_in.
> >
> > 3. Workaround for interrupt/data concurrency issue
> > - Moved to new UPIO_DWAPB iotype.
>
> > Index: linux_2_6/drivers/serial/8250.c
> > ===
> > RCS file: linux_2_6/drivers/serial/8250.c,v
> > retrieving revision 1.1.1.7
> > diff -u -r1.1.1.7 8250.c
> > --- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58 -
> 1.1.1.7
> > +++ linux_2_6/drivers/serial/8250.c 24 Jan 2007 23:55:27 -
> [...]
> > @@ -333,6 +334,8 @@
> > static void
> > serial_out(struct uart_8250_port *up, int offset, int value)
>
>Your patch is clearly garbled again, something added an extra space
> to all
> lines stating with space... :-/
I see that but was under the impression cvs diff did that so all lines
lineup correctly whether they have a +, -, or neither. It doesn't affect
applying the patches for me, did you try?
> > {
> > + /* Save the offset before it's remapped */
> > + int save_offset = offset;
>
> Is there real need to save this? What regshift equals for this UART?
Yes, we have a regshift of 2 on this SoC and it could be different on other
SoCs which reuse the UART IP.
> > offset = map_8250_out_reg(up, offset) << up->port.regshift;
>
> > switch (up->port.iotype) {
> > @@ -359,6 +362,19 @@
> > writeb(value, up->port.membase + offset);
> > break;
> >
> > + case UPIO_DWAPB:
> > + /* Save the LCR value so it can be re-written when a
> > + * Busy Detect interrupt occurs. */
> > + if (save_offset == UART_LCR)
> > + up->lcr = value;
> > + writeb(value, up->port.membase + offset);
> > + /* Read the IER to ensure any interrupt is cleared before
> > + * returning from ISR. */
> > + if ((save_offset == UART_TX || save_offset == UART_IER) &&
>
> Not sure how an IER read ensures that...
It's just a dummy read to ensure that any writes which clear interrupts have
reached the device before returning from the IRQ.
> > + in_irq())
>
> I'd suggest to either indent this line right (start below 2ns paren
> of if
> stmt) or keep on the same line.
OK, moved onto same line.
> > + value = serial_in(up, UART_IER);
> > + break;
> > +
> > default:
> > outb(value, up->port.iobase + offset);
> > }
> > @@ -1016,6 +1032,17 @@
> > up->bugs |= UART_BUG_NOMSR;
> > #endif
> >
> > + /* Workaround:
> > + * The DesignWare SoC UART part has a bug for all
> > + * versions before 3.03a (2005-07-18)
> > + * In brief, this is a non-standard 16550 in that the THRE
> interrupt
> > + * will not re-assert itself simply by disabling and
> re-enabling the
> > + * THRI bit in the IER, it is only re-enabled if a character is
> actually
> > + * sent out.
> > + */
> > + if( up->port.flags & UPF_DW_THRE_BUG )
> > + up->bugs |= UART_BUG_DWTHRE;
> > +
> > serial_outp(up, UART_LCR, save_lcr);
> >
> > if (up->capabilities != uart_config[up->port.type].flags) {
> > @@ -1141,6 +1168,12 @@
> > iir = serial_in(up, UART_IIR);
> > if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
> > transmit_chars(up);
> > + } else if (up->bugs & UART_BUG_DWTHRE) {
> > + unsigned char lsr, iir;
> > + lsr = serial_in(up, UART_LSR);
> > + iir = serial_in(up, UART_IIR);
> > + if (lsr & UART_LSR_THRE)
>
> Why read IIR if you don't check it?
Good point. I didn't write that one and assumed reading the IIR was part of the
fix.
It just tested fine without it so it's gone.
> > @@ -2352,9 +2402,12 @@
> >
> > add_preferred_console("ttyS", line, options);
>
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: > I could use both iotype and type with a test on each for the appropriate > bug, what do you recommend? I think iotype would be enough. You can't pass type for platform devices anyway, IIRC (the thing I don't quite like). I just found that out the hard way, it get's overwritten during autoconfig* and ends up back at PORT_16550A. I'm now trying to use my own iotype = UPIO_DWAPB and I've added it to all cases that check for UPIO_MEM. However at boot time I'm getting: "serial8250: ttyS0 at *unknown* (irq = 27) is a 16550A". It looks like something outside of 8250.c must be checking for UPIO_MEM, I'm looking into it. Yeah, be sure to change serial_core.c as well (whereever you'll see UPIO_AU/TSI there)... Quite ugly. :-/ WBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: I could use both iotype and type with a test on each for the appropriate bug, what do you recommend? I think iotype would be enough. You can't pass type for platform devices anyway, IIRC (the thing I don't quite like). I just found that out the hard way, it get's overwritten during autoconfig* and ends up back at PORT_16550A. I'm now trying to use my own iotype = UPIO_DWAPB and I've added it to all cases that check for UPIO_MEM. However at boot time I'm getting: serial8250: ttyS0 at *unknown* (irq = 27) is a 16550A. It looks like something outside of 8250.c must be checking for UPIO_MEM, I'm looking into it. Yeah, be sure to change serial_core.c as well (whereever you'll see UPIO_AU/TSI there)... Quite ugly. :-/ WBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote:
Marc St-Jean wrote:
Here is my second attempt at the serial driver patch for the
PMC-Sierra MSP71xx device.
There are three different fixes:
1. Fix for THRE errata
- I verified the UART_BUG_TXEN fix does not help with this erratum.
- I left our current fix in until I get our platform booting on
2.6.20-rc4 to try the mm tree 8250-uart-backup-timer.patch.
Feel free to ignore for now.
2. Fix for Busy Detect on LCR write
- Moved to new UPIO_DWAPB iotype. Because the new type is a memory
mapped device and there are several tests for UPIO_MEM, this involved
updating serial_core.c and 8250_early.c in addition to 8250.c.
- I tried implementing this totally in serial_in as suggested, but
it can't be done because of bit overlap between UART_IIR_NO_INT and
UART_IIR_BUSY. Also there is no way to set the interrupt handled = 1
from serial_in.
3. Workaround for interrupt/data concurrency issue
- Moved to new UPIO_DWAPB iotype.
Index: linux_2_6/drivers/serial/8250.c
===
RCS file: linux_2_6/drivers/serial/8250.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 8250.c
--- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58 -
1.1.1.7
+++ linux_2_6/drivers/serial/8250.c 24 Jan 2007 23:55:27 -
[...]
@@ -333,6 +334,8 @@
static void
serial_out(struct uart_8250_port *up, int offset, int value)
Your patch is clearly garbled again, something added an extra space
to all
lines stating with space... :-/
I see that but was under the impression cvs diff did that so all lines
lineup correctly whether they have a +, -, or neither. It doesn't affect
applying the patches for me, did you try?
{
+ /* Save the offset before it's remapped */
+ int save_offset = offset;
Is there real need to save this? What regshift equals for this UART?
Yes, we have a regshift of 2 on this SoC and it could be different on other
SoCs which reuse the UART IP.
offset = map_8250_out_reg(up, offset) up-port.regshift;
switch (up-port.iotype) {
@@ -359,6 +362,19 @@
writeb(value, up-port.membase + offset);
break;
+ case UPIO_DWAPB:
+ /* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+ if (save_offset == UART_LCR)
+ up-lcr = value;
+ writeb(value, up-port.membase + offset);
+ /* Read the IER to ensure any interrupt is cleared before
+ * returning from ISR. */
+ if ((save_offset == UART_TX || save_offset == UART_IER)
Not sure how an IER read ensures that...
It's just a dummy read to ensure that any writes which clear interrupts have
reached the device before returning from the IRQ.
+ in_irq())
I'd suggest to either indent this line right (start below 2ns paren
of if
stmt) or keep on the same line.
OK, moved onto same line.
+ value = serial_in(up, UART_IER);
+ break;
+
default:
outb(value, up-port.iobase + offset);
}
@@ -1016,6 +1032,17 @@
up-bugs |= UART_BUG_NOMSR;
#endif
+ /* Workaround:
+ * The DesignWare SoC UART part has a bug for all
+ * versions before 3.03a (2005-07-18)
+ * In brief, this is a non-standard 16550 in that the THRE
interrupt
+ * will not re-assert itself simply by disabling and
re-enabling the
+ * THRI bit in the IER, it is only re-enabled if a character is
actually
+ * sent out.
+ */
+ if( up-port.flags UPF_DW_THRE_BUG )
+ up-bugs |= UART_BUG_DWTHRE;
+
serial_outp(up, UART_LCR, save_lcr);
if (up-capabilities != uart_config[up-port.type].flags) {
@@ -1141,6 +1168,12 @@
iir = serial_in(up, UART_IIR);
if (lsr UART_LSR_TEMT iir UART_IIR_NO_INT)
transmit_chars(up);
+ } else if (up-bugs UART_BUG_DWTHRE) {
+ unsigned char lsr, iir;
+ lsr = serial_in(up, UART_LSR);
+ iir = serial_in(up, UART_IIR);
+ if (lsr UART_LSR_THRE)
Why read IIR if you don't check it?
Good point. I didn't write that one and assumed reading the IIR was part of the
fix.
It just tested fine without it so it's gone.
@@ -2352,9 +2402,12 @@
add_preferred_console(ttyS, line, options);
printk(Adding console on ttyS%d at %s 0x%lx (options '%s')\n,
- line, port-iotype == UPIO_MEM ? MMIO : I/O port,
- port-iotype == UPIO_MEM ? (unsigned long) port-mapbase :
-
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote: > Hello. > > Marc St-Jean wrote: > > >> >>This I would hope you can hide in the platform specific > >> >>serial_in/serial_out functions. If you write the UART_LCR save it in > >> >>serial_out(), if you read IER etc. > > >> > I couldn't find hooks for platform specific serial_in/out functions. > > >>It's because there are none. :-) > > >> > Do you mean using the up->port.iotype's in serial_in/out from 8250.c? > > >>Not sure what Alan meant, but this seems the only option for now. > > > That's the conclusion I came to. I've rewritten the patch to use > port.type > > instead of iotype since one of the fix is SoC and not UART specific. > I guess > > I failed to folkow your logic. :-) No longer matters as I can't use port.type. See next comment. > > I could use both iotype and type with a test on each for the appropriate > > bug, what do you recommend? > > I think iotype would be enough. You can't pass type for platform > devices > anyway, IIRC (the thing I don't quite like). I just found that out the hard way, it get's overwritten during autoconfig* and ends up back at PORT_16550A. I'm now trying to use my own iotype = UPIO_DWAPB and I've added it to all cases that check for UPIO_MEM. However at boot time I'm getting: "serial8250: ttyS0 at *unknown* (irq = 27) is a 16550A". It looks like something outside of 8250.c must be checking for UPIO_MEM, I'm looking into it. > > >> >>And we might want to add a void * for board specific insanity to > the 8250 > >> >>structures if we really have to so you can hang your brain damage > >> >>privately off that ? > > >> > Sounds good to me, it would give us a location to store the > address of the > >> > UART_STATUS_REG required by this UART variant. > > >>I doubt we really need to *store* it somewhere. Isn't it an fixed > offset > >>from UART's base (I haven't seen the header)? > > > Unfortunately it's not a constant offset from the UART in the SoC > register > > Hm... > > > space. I've used Alan suggestion and added a classic, on some other > OSes %-|, > > void "user" pointer. > > Only do not do it under #ifdef. Understood, getting rid of them is why I started this thread. Marc - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: >>This I would hope you can hide in the platform specific >>serial_in/serial_out functions. If you write the UART_LCR save it in >>serial_out(), if you read IER etc. > I couldn't find hooks for platform specific serial_in/out functions. It's because there are none. :-) > Do you mean using the up->port.iotype's in serial_in/out from 8250.c? Not sure what Alan meant, but this seems the only option for now. That's the conclusion I came to. I've rewritten the patch to use port.type instead of iotype since one of the fix is SoC and not UART specific. I guess I failed to folkow your logic. :-) I could use both iotype and type with a test on each for the appropriate bug, what do you recommend? I think iotype would be enough. You can't pass type for platform devices anyway, IIRC (the thing I don't quite like). >>And we might want to add a void * for board specific insanity to the 8250 >>structures if we really have to so you can hang your brain damage >>privately off that ? > Sounds good to me, it would give us a location to store the address of the > UART_STATUS_REG required by this UART variant. I doubt we really need to *store* it somewhere. Isn't it an fixed offset from UART's base (I haven't seen the header)? Unfortunately it's not a constant offset from the UART in the SoC register Hm... space. I've used Alan suggestion and added a classic, on some other OSes %-|, void "user" pointer. Only do not do it under #ifdef. Marc WBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-uart-backup-timer.patch This second patch failure description is identical to what we are seeing without the THRE work-around. This must be the timer patch Alan mentioned but it's not in the linux.git at l-m.o. Yeah, it must still be considered "experimental" as it resides only within the -mm tree. Could you please explain what you mean by and where I can find the "-mm tree"? http://kernel.org/patchtypes/mm.html As the serial drivers have no maintainer now, you probably have to CC the final patch to Andrew Morton ([EMAIL PROTECTED]), so it'd be better to be applicable against the recent -mm patch. Marc MBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote: > > >>This I would hope you can hide in the platform specific > >>serial_in/serial_out functions. If you write the UART_LCR save it in > >>serial_out(), if you read IER etc. > > > I couldn't find hooks for platform specific serial_in/out functions. > > It's because there are none. :-) > > > Do you mean using the up->port.iotype's in serial_in/out from 8250.c? > > Not sure what Alan meant, but this seems the only option for now. That's the conclusion I came to. I've rewritten the patch to use port.type instead of iotype since one of the fix is SoC and not UART specific. I guess I could use both iotype and type with a test on each for the appropriate bug, what do you recommend? > >>And we might want to add a void * for board specific insanity to the > 8250 > >>structures if we really have to so you can hang your brain damage > >>privately off that ? > > > Sounds good to me, it would give us a location to store the address > of the > > UART_STATUS_REG required by this UART variant. > > I doubt we really need to *store* it somewhere. Isn't it an fixed > offset > from UART's base (I haven't seen the header)? Unfortunately it's not a constant offset from the UART in the SoC register space. I've used Alan suggestion and added a classic, on some other OSes %-|, void "user" pointer. I'll repost as soon I complete testing and try the new timer patch. Marc - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote: > Hello, I wrote: > > Here is a serial driver patch for the PMC-Sierra MSP71xx device. > > There are three different fixes: > 1. Fix for THRE errata > 2. Fix for Busy Detect on LCR write > 3. Workaround for interrupt/data concurrency issue > > The first fix is handled cleanly using a UART_BUG_* flag. > > >>>Hm, I wouldn't call it clean... > > >> Relative to the other two. Any recommended improvements on this one? > > >I already told: runtime check. > > BTW, I've just came accross 2 patches in the -mm tree related to the > transmitter bug: > > http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-make-probing-for-txen-bug-a-config-option.patch Thanks. The first of these is not at all what we are seeing. However I guess there could be a remote chance it's triggering our THRE errata as it did for the pnx4008 boards. > http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-uart-backup-timer.patch This second patch failure description is identical to what we are seeing without the THRE work-around. This must be the timer patch Alan mentioned but it's not in the linux.git at l-m.o. Could you please explain what you mean by and where I can find the "-mm tree"? Marc - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: > 2. Fix for Busy Detect on LCR write > 3. Workaround for interrupt/data concurrency issue > case UPIO_MEM: > +#ifdef CONFIG_PMC_MSP > + /* Save the LCR value so it can be re-written when a > + * Busy Detect interrupt occurs. */ > + if (dwapb_offset == UART_LCR) > + up->dwapb_lcr = value; > +#endif > writeb(value, up->port.membase + offset); > +#ifdef CONFIG_PMC_MSP > + /* Re-read the IER to ensure any interrupt disabling has > + * completed before proceeding with ISR. */ > + if (dwapb_offset == UART_IER) > + value = serial_in(up, dwapb_offset); > +#endif > break; This I would hope you can hide in the platform specific serial_in/serial_out functions. If you write the UART_LCR save it in serial_out(), if you read IER etc. I couldn't find hooks for platform specific serial_in/out functions. It's because there are none. :-) Do you mean using the up->port.iotype's in serial_in/out from 8250.c? Not sure what Alan meant, but this seems the only option for now. >>And we might want to add a void * for board specific insanity to the 8250 structures if we really have to so you can hang your brain damage privately off that ? Sounds good to me, it would give us a location to store the address of the UART_STATUS_REG required by this UART variant. I doubt we really need to *store* it somewhere. Isn't it an fixed offset from UART's base (I haven't seen the header)? Marc MBR, Sergei - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: 2. Fix for Busy Detect on LCR write 3. Workaround for interrupt/data concurrency issue case UPIO_MEM: +#ifdef CONFIG_PMC_MSP + /* Save the LCR value so it can be re-written when a + * Busy Detect interrupt occurs. */ + if (dwapb_offset == UART_LCR) + up-dwapb_lcr = value; +#endif writeb(value, up-port.membase + offset); +#ifdef CONFIG_PMC_MSP + /* Re-read the IER to ensure any interrupt disabling has + * completed before proceeding with ISR. */ + if (dwapb_offset == UART_IER) + value = serial_in(up, dwapb_offset); +#endif break; This I would hope you can hide in the platform specific serial_in/serial_out functions. If you write the UART_LCR save it in serial_out(), if you read IER etc. I couldn't find hooks for platform specific serial_in/out functions. It's because there are none. :-) Do you mean using the up-port.iotype's in serial_in/out from 8250.c? Not sure what Alan meant, but this seems the only option for now. And we might want to add a void * for board specific insanity to the 8250 structures if we really have to so you can hang your brain damage privately off that ? Sounds good to me, it would give us a location to store the address of the UART_STATUS_REG required by this UART variant. I doubt we really need to *store* it somewhere. Isn't it an fixed offset from UART's base (I haven't seen the header)? Marc MBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote: Hello, I wrote: Here is a serial driver patch for the PMC-Sierra MSP71xx device. There are three different fixes: 1. Fix for THRE errata 2. Fix for Busy Detect on LCR write 3. Workaround for interrupt/data concurrency issue The first fix is handled cleanly using a UART_BUG_* flag. Hm, I wouldn't call it clean... Relative to the other two. Any recommended improvements on this one? I already told: runtime check. BTW, I've just came accross 2 patches in the -mm tree related to the transmitter bug: http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-make-probing-for-txen-bug-a-config-option.patch Thanks. The first of these is not at all what we are seeing. However I guess there could be a remote chance it's triggering our THRE errata as it did for the pnx4008 boards. http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-uart-backup-timer.patch This second patch failure description is identical to what we are seeing without the THRE work-around. This must be the timer patch Alan mentioned but it's not in the linux.git at l-m.o. Could you please explain what you mean by and where I can find the -mm tree? Marc - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote: This I would hope you can hide in the platform specific serial_in/serial_out functions. If you write the UART_LCR save it in serial_out(), if you read IER etc. I couldn't find hooks for platform specific serial_in/out functions. It's because there are none. :-) Do you mean using the up-port.iotype's in serial_in/out from 8250.c? Not sure what Alan meant, but this seems the only option for now. That's the conclusion I came to. I've rewritten the patch to use port.type instead of iotype since one of the fix is SoC and not UART specific. I guess I could use both iotype and type with a test on each for the appropriate bug, what do you recommend? And we might want to add a void * for board specific insanity to the 8250 structures if we really have to so you can hang your brain damage privately off that ? Sounds good to me, it would give us a location to store the address of the UART_STATUS_REG required by this UART variant. I doubt we really need to *store* it somewhere. Isn't it an fixed offset from UART's base (I haven't seen the header)? Unfortunately it's not a constant offset from the UART in the SoC register space. I've used Alan suggestion and added a classic, on some other OSes %-|, void user pointer. I'll repost as soon I complete testing and try the new timer patch. Marc - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.20-rc4/2.6.20-rc4-mm1/broken-out/8250-uart-backup-timer.patch This second patch failure description is identical to what we are seeing without the THRE work-around. This must be the timer patch Alan mentioned but it's not in the linux.git at l-m.o. Yeah, it must still be considered experimental as it resides only within the -mm tree. Could you please explain what you mean by and where I can find the -mm tree? http://kernel.org/patchtypes/mm.html As the serial drivers have no maintainer now, you probably have to CC the final patch to Andrew Morton ([EMAIL PROTECTED]), so it'd be better to be applicable against the recent -mm patch. Marc MBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Hello. Marc St-Jean wrote: This I would hope you can hide in the platform specific serial_in/serial_out functions. If you write the UART_LCR save it in serial_out(), if you read IER etc. I couldn't find hooks for platform specific serial_in/out functions. It's because there are none. :-) Do you mean using the up-port.iotype's in serial_in/out from 8250.c? Not sure what Alan meant, but this seems the only option for now. That's the conclusion I came to. I've rewritten the patch to use port.type instead of iotype since one of the fix is SoC and not UART specific. I guess I failed to folkow your logic. :-) I could use both iotype and type with a test on each for the appropriate bug, what do you recommend? I think iotype would be enough. You can't pass type for platform devices anyway, IIRC (the thing I don't quite like). And we might want to add a void * for board specific insanity to the 8250 structures if we really have to so you can hang your brain damage privately off that ? Sounds good to me, it would give us a location to store the address of the UART_STATUS_REG required by this UART variant. I doubt we really need to *store* it somewhere. Isn't it an fixed offset from UART's base (I haven't seen the header)? Unfortunately it's not a constant offset from the UART in the SoC register Hm... space. I've used Alan suggestion and added a classic, on some other OSes %-|, void user pointer. Only do not do it under #ifdef. Marc WBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Sergei Shtylyov wrote: Hello. Marc St-Jean wrote: This I would hope you can hide in the platform specific serial_in/serial_out functions. If you write the UART_LCR save it in serial_out(), if you read IER etc. I couldn't find hooks for platform specific serial_in/out functions. It's because there are none. :-) Do you mean using the up-port.iotype's in serial_in/out from 8250.c? Not sure what Alan meant, but this seems the only option for now. That's the conclusion I came to. I've rewritten the patch to use port.type instead of iotype since one of the fix is SoC and not UART specific. I guess I failed to folkow your logic. :-) No longer matters as I can't use port.type. See next comment. I could use both iotype and type with a test on each for the appropriate bug, what do you recommend? I think iotype would be enough. You can't pass type for platform devices anyway, IIRC (the thing I don't quite like). I just found that out the hard way, it get's overwritten during autoconfig* and ends up back at PORT_16550A. I'm now trying to use my own iotype = UPIO_DWAPB and I've added it to all cases that check for UPIO_MEM. However at boot time I'm getting: serial8250: ttyS0 at *unknown* (irq = 27) is a 16550A. It looks like something outside of 8250.c must be checking for UPIO_MEM, I'm looking into it. And we might want to add a void * for board specific insanity to the 8250 structures if we really have to so you can hang your brain damage privately off that ? Sounds good to me, it would give us a location to store the address of the UART_STATUS_REG required by this UART variant. I doubt we really need to *store* it somewhere. Isn't it an fixed offset from UART's base (I haven't seen the header)? Unfortunately it's not a constant offset from the UART in the SoC register Hm... space. I've used Alan suggestion and added a classic, on some other OSes %-|, void user pointer. Only do not do it under #ifdef. Understood, getting rid of them is why I started this thread. Marc - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
On Tue, 23 Jan 2007 14:37:23 -0800 Marc St-Jean <[EMAIL PROTECTED]> wrote: > > This I would hope you can hide in the platform specific > > serial_in/serial_out functions. If you write the UART_LCR save it in > > serial_out(), if you read IER etc. > > I couldn't find hooks for platform specific serial_in/out functions. > Do you mean using the up->port.iotype's in serial_in/out from 8250.c? If you can have other uarts as well then yes. Basically I want all the ugliness to belong to you not to the 8250 driver (and ditto for any other half baked uart or demented piece of broken glue logic). If we don't do that then 8250.c will end up unmanagable. So long as you crap in your own pond everyone else is fine ;) > A serial_in(up, UART_IIR) calls occur in more places that just the interrupt > handler (i.e. autoconfig*, serial8250_start_tx, etc). We will need to check > if we are in an interrupt on each IIR read, hopefully that won't be too much > overhead! Or if need be we make that hint generally available as we can do that bit cleanly. > > > > And we might want to add a void * for board specific insanity to the 8250 > > structures if we really have to so you can hang your brain damage > > privately off that ? > > Sounds good to me, it would give us a location to store the address of the > UART_STATUS_REG required by this UART variant. and for anything harder people can hang a struct off it. Alan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Alan wrote:
> > There are three different fixes:
> > 1. Fix for THRE errata
>
> That should be handled anyway. The current code actually spots this and
> uses a backup timer for dodgy UARTS
Thanks, I'll retest without this fix on the current l-m.o git master and see
if it still solves our errata.
> > 2. Fix for Busy Detect on LCR write
> > 3. Workaround for interrupt/data concurrency issue
>
> > case UPIO_MEM:
> > +#ifdef CONFIG_PMC_MSP
> > + /* Save the LCR value so it can be re-written when a
> > + * Busy Detect interrupt occurs. */
> > + if (dwapb_offset == UART_LCR)
> > + up->dwapb_lcr = value;
> > +#endif
> > writeb(value, up->port.membase + offset);
> > +#ifdef CONFIG_PMC_MSP
> > + /* Re-read the IER to ensure any interrupt disabling has
> > + * completed before proceeding with ISR. */
> > + if (dwapb_offset == UART_IER)
> > + value = serial_in(up, dwapb_offset);
> > +#endif
> > break;
>
> This I would hope you can hide in the platform specific
> serial_in/serial_out functions. If you write the UART_LCR save it in
> serial_out(), if you read IER etc.
I couldn't find hooks for platform specific serial_in/out functions.
Do you mean using the up->port.iotype's in serial_in/out from 8250.c?
>
> > +#ifdef CONFIG_PMC_MSP
> > + } else if ((iir & UART_IER_BUSY) == UART_IER_BUSY) {
> > + /*
> > + * The MSP (DesignWare APB UART) serial
> subsystem has a
> > + * non-standard interrupt condition (0x7) which
> means
> > + * that the LCR was written while the UART was
> busy, so
> > + * the LCR was not actually written. It is
> cleared by
> > + * reading the special non-standard extended
> UART status
> > + * register.
>
> Ditto... spot this case and whack it in your serial methods.
A serial_in(up, UART_IIR) calls occur in more places that just the interrupt
handler (i.e. autoconfig*, serial8250_start_tx, etc). We will need to check
if we are in an interrupt on each IIR read, hopefully that won't be too much
overhead!
>
> And we might want to add a void * for board specific insanity to the 8250
> structures if we really have to so you can hang your brain damage
> privately off that ?
Sounds good to me, it would give us a location to store the address of the
UART_STATUS_REG required by this UART variant.
Marc
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
Alan wrote:
There are three different fixes:
1. Fix for THRE errata
That should be handled anyway. The current code actually spots this and
uses a backup timer for dodgy UARTS
Thanks, I'll retest without this fix on the current l-m.o git master and see
if it still solves our errata.
2. Fix for Busy Detect on LCR write
3. Workaround for interrupt/data concurrency issue
case UPIO_MEM:
+#ifdef CONFIG_PMC_MSP
+ /* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+ if (dwapb_offset == UART_LCR)
+ up-dwapb_lcr = value;
+#endif
writeb(value, up-port.membase + offset);
+#ifdef CONFIG_PMC_MSP
+ /* Re-read the IER to ensure any interrupt disabling has
+ * completed before proceeding with ISR. */
+ if (dwapb_offset == UART_IER)
+ value = serial_in(up, dwapb_offset);
+#endif
break;
This I would hope you can hide in the platform specific
serial_in/serial_out functions. If you write the UART_LCR save it in
serial_out(), if you read IER etc.
I couldn't find hooks for platform specific serial_in/out functions.
Do you mean using the up-port.iotype's in serial_in/out from 8250.c?
+#ifdef CONFIG_PMC_MSP
+ } else if ((iir UART_IER_BUSY) == UART_IER_BUSY) {
+ /*
+ * The MSP (DesignWare APB UART) serial
subsystem has a
+ * non-standard interrupt condition (0x7) which
means
+ * that the LCR was written while the UART was
busy, so
+ * the LCR was not actually written. It is
cleared by
+ * reading the special non-standard extended
UART status
+ * register.
Ditto... spot this case and whack it in your serial methods.
A serial_in(up, UART_IIR) calls occur in more places that just the interrupt
handler (i.e. autoconfig*, serial8250_start_tx, etc). We will need to check
if we are in an interrupt on each IIR read, hopefully that won't be too much
overhead!
And we might want to add a void * for board specific insanity to the 8250
structures if we really have to so you can hang your brain damage
privately off that ?
Sounds good to me, it would give us a location to store the address of the
UART_STATUS_REG required by this UART variant.
Marc
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
On Tue, 23 Jan 2007 14:37:23 -0800 Marc St-Jean [EMAIL PROTECTED] wrote: This I would hope you can hide in the platform specific serial_in/serial_out functions. If you write the UART_LCR save it in serial_out(), if you read IER etc. I couldn't find hooks for platform specific serial_in/out functions. Do you mean using the up-port.iotype's in serial_in/out from 8250.c? If you can have other uarts as well then yes. Basically I want all the ugliness to belong to you not to the 8250 driver (and ditto for any other half baked uart or demented piece of broken glue logic). If we don't do that then 8250.c will end up unmanagable. So long as you crap in your own pond everyone else is fine ;) A serial_in(up, UART_IIR) calls occur in more places that just the interrupt handler (i.e. autoconfig*, serial8250_start_tx, etc). We will need to check if we are in an interrupt on each IIR read, hopefully that won't be too much overhead! Or if need be we make that hint generally available as we can do that bit cleanly. And we might want to add a void * for board specific insanity to the 8250 structures if we really have to so you can hang your brain damage privately off that ? Sounds good to me, it would give us a location to store the address of the UART_STATUS_REG required by this UART variant. and for anything harder people can hang a struct off it. Alan - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
CCing to linux-kernel as per AC's suggestion...
Original Message
Subject: RE: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git master
Date: Mon, 22 Jan 2007 10:11:04 -0800
From: Marc St-Jean
To: Sergei Shtylyov
CC: <[EMAIL PROTECTED]>,
> -Original Message-
> From: Sergei Shtylyov [mailto:[EMAIL PROTECTED]
> Sent: Friday, January 19, 2007 9:05 AM
> To: Marc St-Jean
> Cc: [EMAIL PROTECTED]; linux-serial@vger.kernel.org
> Subject: Re: [PATCH] serial driver PMC MSP71xx, kernel
> linux-mips.git master
>
> Hello.
>
> Marc St-Jean wrote:
> > Here is a serial driver patch for the PMC-Sierra MSP71xx device.
>
> > There are three different fixes:
> > 1. Fix for THRE errata
> > 2. Fix for Busy Detect on LCR write
> > 3. Workaround for interrupt/data concurrency issue
>
> > The first fix is handled cleanly using a UART_BUG_* flag.
>
> Hm, I wouldn't call it clean...
Relative to the other two. Any recommended improvements on this one?
> > The second and third fixes use platform tests. I couldn't
> think of a
> > good way to implement them without using tests and not
> increase code
> > and structure sizes for platforms not requiring them.
>
> > Does anyone have any suggestions on implementing these without the
> > platform flag?
>
> > Thanks,
> > Marc
>
> > Signed-off-by: Marc St-Jean <[EMAIL PROTECTED]>
>
> > Index: linux_2_6/drivers/serial/8250.c
> > ===
> > RCS file: linux_2_6/drivers/serial/8250.c,v retrieving revision
> > 1.1.1.7 retrieving revision 1.9 diff -u -r1.1.1.7 -r1.9
> > --- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58
> - 1.1.1.7
> > +++ linux_2_6/drivers/serial/8250.c 19 Oct 2006 22:08:15
> - 1.9
> > @@ -44,6 +44,10 @@
> > #include
> > #include
> >
> > +#ifdef CONFIG_PMC_MSP
> > +#include
> > +#endif
> > +
> > #include "8250.h"
> >
> > /*
> > @@ -130,6 +134,9 @@
> > unsigned char mcr_mask; /* mask of user bits */
> > unsigned char mcr_force; /* mask of
> forced bits */
> > unsigned char lsr_break_flag;
> > +#ifdef CONFIG_PMC_MSP
> > + int dwapb_lcr;
> /* saved LCR for DW APB WAR */
> > +#endif
>
> There was already 'lcr' field there, couldn't it be used?
Possibly but the current driver doesn't always save the LCR value before
trying to write it. For example see serial8250_set_termios()
serial_outp(up, UART_LCR, cval);/* reset DLAB */
up->lcr = cval; /* Save LCR */
It's impossible to ensure that going foward the driver will always save
the value before writing it. We need to have it saved before since the
write will cause an interrupt, hence the save in serial_out.
If that's not acceptable I can audit the driver and ensure all LCR
writes are first saved. Maybe comments would reduce the chance of future
updates breaking this?
> > @@ -333,6 +340,10 @@
> > static void
> > serial_out(struct uart_8250_port *up, int offset, int value)
> > {
> > +#ifdef CONFIG_PMC_MSP
> > + /* Save the offset before it's remapped */
> > + int dwapb_offset = offset;
> > +#endif
> > offset = map_8250_out_reg(up, offset) << up->port.regshift;
> >
> > switch (up->port.iotype) {
> > @@ -342,7 +353,19 @@
> > break;
> >
> > case UPIO_MEM:
> > +#ifdef CONFIG_PMC_MSP
> > + /* Save the LCR value so it can be re-written when a
> > +* Busy Detect interrupt occurs. */
> > + if (dwapb_offset == UART_LCR)
> > + up->dwapb_lcr = value;
> > +#endif
> > writeb(value, up->port.membase + offset);
> > +#ifdef CONFIG_PMC_MSP
> > + /* Re-read the IER to ensure any interrupt disabling has
> > +* completed before proceeding with ISR. */
> > + if (dwapb_offset == UART_IER)
> > + value = serial_in(up, dwapb_offset); #endif
> > break;
>
> Hm, was there really a need for #ifdef mess here?
> I'd vote for introducing new UPIO_* here, like was done
> for TSi10x UARTs just for the same reason.
I'm willing to do this, however IIRC there was a thread in late Sept.
which came to the conclusion that UPIO were to be used for different
access methods and not UART types. Do you see this as an exception?
In the second #ifdef CONFIG_PMC_MSP above the workaround is required
because of SoC interrupt design which is out-of-band with respect to r/w
of UART registers, it's not specific to the DesignWare UART.
>
> > @@ -1016,6 +1039,17 @@
> > up->bugs |= UART_BUG_NOMSR;
> > #endif
> >
> > + /* Workaround:
> > +* The DesignWare SoC UART part has a bug for all
> > +* versions before 3.03a (2005-07-18)
> > +* In brief, this is a non-standard 16550 in that the
> THRE interrupt
> > +* will not re-assert itself simply by disabling and
>
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
CCing to linux-kernel as per AC's suggestion...
Original Message
Subject:Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git
mast er
Date: Mon, 22 Jan 2007 12:23:56 -0800
From: Sergei Shtylyov
Organization: MontaVista Software Inc.
To: Marc St-Jean
CC: <[EMAIL PROTECTED]>,
References:
<[EMAIL PROTECTED]>
Hello.
Marc St-Jean wrote:
Your maile is strange: it line-wraps quotes but not your own text...
:-/
>>>Here is a serial driver patch for the PMC-Sierra MSP71xx device.
>>>There are three different fixes:
>>>1. Fix for THRE errata
>>>2. Fix for Busy Detect on LCR write
>>>3. Workaround for interrupt/data concurrency issue
>>>The first fix is handled cleanly using a UART_BUG_* flag.
>>Hm, I wouldn't call it clean...
> Relative to the other two. Any recommended improvements on this one?
I already told: runtime check.
>>>Thanks,
>>>Marc
>>>Signed-off-by: Marc St-Jean <[EMAIL PROTECTED]>
>>>Index: linux_2_6/drivers/serial/8250.c
>>>===
>>>RCS file: linux_2_6/drivers/serial/8250.c,v retrieving revision
>>>1.1.1.7 retrieving revision 1.9 diff -u -r1.1.1.7 -r1.9
>>>--- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58
>>
>>- 1.1.1.7
>>
>>>+++ linux_2_6/drivers/serial/8250.c 19 Oct 2006 22:08:15
>>
>>- 1.9
>>
>>>@@ -44,6 +44,10 @@
>>> #include
>>> #include
>>>
>>>+#ifdef CONFIG_PMC_MSP
>>>+#include
>>>+#endif
>>>+
>>> #include "8250.h"
>>> /*
>>>@@ -130,6 +134,9 @@
>>> unsigned char mcr_mask; /* mask of user bits */
>>> unsigned char mcr_force; /* mask of forced bits */
>>> unsigned char lsr_break_flag;
>>>+#ifdef CONFIG_PMC_MSP
>>>+int dwapb_lcr;
>>/* saved LCR for DW APB WAR */
>>>+#endif
>>There was already 'lcr' field there, couldn't it be used?
> Possibly but the current driver doesn't always save the LCR value
before trying to write it. For example see serial8250_set_termios()
> serial_outp(up, UART_LCR, cval);/* reset DLAB */
> up->lcr = cval; /* Save LCR */
This certainly may be fixed.
> It's impossible to ensure that going foward the driver will always
save the value before writing it.
> We need to have it saved before since the write will cause an
interrupt, hence the save in serial_out.
You may rework driver to always save it on write to 'lcr' field (in
case
of your UART). I don't think it's going to spoil something there...
> If that's not acceptable I can audit the driver and ensure all LCR
writes are first saved.
What's *certainly* undesirable is #ifdef mess. :-)
>>>@@ -333,6 +340,10 @@
>>> static void
>>> serial_out(struct uart_8250_port *up, int offset, int value)
>>> {
>>>+#ifdef CONFIG_PMC_MSP
>>>+/* Save the offset before it's remapped */
>>>+int dwapb_offset = offset;
>>>+#endif
>>> offset = map_8250_out_reg(up, offset) << up->port.regshift;
>>>
>>> switch (up->port.iotype) {
>>>@@ -342,7 +353,19 @@
>>> break;
>>>
>>> case UPIO_MEM:
>>>+#ifdef CONFIG_PMC_MSP
>>>+/* Save the LCR value so it can be re-written when a
>>>+ * Busy Detect interrupt occurs. */
>>>+if (dwapb_offset == UART_LCR)
>>>+up->dwapb_lcr = value;
>>>+#endif
>>> writeb(value, up->port.membase + offset);
>>>+#ifdef CONFIG_PMC_MSP
>>>+/* Re-read the IER to ensure any interrupt disabling has
>>>+ * completed before proceeding with ISR. */
>>>+if (dwapb_offset == UART_IER)
>>>+value = serial_in(up, dwapb_offset); #endif
>>> break;
>>Hm, was there really a need for #ifdef mess here?
>>I'd vote for introducing new UPIO_* here, like was done
>>for TSi10x UARTs just for the same reason.
> I'm willing to do this, however IIRC there was a thread in late Sept.
which came
> to the conclusion that UPIO were to be used for different access
methods and not UART types.
AFAIR, almost e
RE: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
CCing to linux-kernel as per AC's suggestion...
Original Message
Subject: RE: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git master
Date: Mon, 22 Jan 2007 10:11:04 -0800
From: Marc St-Jean
To: Sergei Shtylyov
CC: [EMAIL PROTECTED],linux-serial@vger.kernel.org
-Original Message-
From: Sergei Shtylyov [mailto:[EMAIL PROTECTED]
Sent: Friday, January 19, 2007 9:05 AM
To: Marc St-Jean
Cc: [EMAIL PROTECTED]; linux-serial@vger.kernel.org
Subject: Re: [PATCH] serial driver PMC MSP71xx, kernel
linux-mips.git master
Hello.
Marc St-Jean wrote:
Here is a serial driver patch for the PMC-Sierra MSP71xx device.
There are three different fixes:
1. Fix for THRE errata
2. Fix for Busy Detect on LCR write
3. Workaround for interrupt/data concurrency issue
The first fix is handled cleanly using a UART_BUG_* flag.
Hm, I wouldn't call it clean...
Relative to the other two. Any recommended improvements on this one?
The second and third fixes use platform tests. I couldn't
think of a
good way to implement them without using tests and not
increase code
and structure sizes for platforms not requiring them.
Does anyone have any suggestions on implementing these without the
platform flag?
Thanks,
Marc
Signed-off-by: Marc St-Jean [EMAIL PROTECTED]
Index: linux_2_6/drivers/serial/8250.c
===
RCS file: linux_2_6/drivers/serial/8250.c,v retrieving revision
1.1.1.7 retrieving revision 1.9 diff -u -r1.1.1.7 -r1.9
--- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58
- 1.1.1.7
+++ linux_2_6/drivers/serial/8250.c 19 Oct 2006 22:08:15
- 1.9
@@ -44,6 +44,10 @@
#include asm/io.h
#include asm/irq.h
+#ifdef CONFIG_PMC_MSP
+#include msp_regs.h
+#endif
+
#include 8250.h
/*
@@ -130,6 +134,9 @@
unsigned char mcr_mask; /* mask of user bits */
unsigned char mcr_force; /* mask of
forced bits */
unsigned char lsr_break_flag;
+#ifdef CONFIG_PMC_MSP
+ int dwapb_lcr;
/* saved LCR for DW APB WAR */
+#endif
There was already 'lcr' field there, couldn't it be used?
Possibly but the current driver doesn't always save the LCR value before
trying to write it. For example see serial8250_set_termios()
serial_outp(up, UART_LCR, cval);/* reset DLAB */
up-lcr = cval; /* Save LCR */
It's impossible to ensure that going foward the driver will always save
the value before writing it. We need to have it saved before since the
write will cause an interrupt, hence the save in serial_out.
If that's not acceptable I can audit the driver and ensure all LCR
writes are first saved. Maybe comments would reduce the chance of future
updates breaking this?
@@ -333,6 +340,10 @@
static void
serial_out(struct uart_8250_port *up, int offset, int value)
{
+#ifdef CONFIG_PMC_MSP
+ /* Save the offset before it's remapped */
+ int dwapb_offset = offset;
+#endif
offset = map_8250_out_reg(up, offset) up-port.regshift;
switch (up-port.iotype) {
@@ -342,7 +353,19 @@
break;
case UPIO_MEM:
+#ifdef CONFIG_PMC_MSP
+ /* Save the LCR value so it can be re-written when a
+* Busy Detect interrupt occurs. */
+ if (dwapb_offset == UART_LCR)
+ up-dwapb_lcr = value;
+#endif
writeb(value, up-port.membase + offset);
+#ifdef CONFIG_PMC_MSP
+ /* Re-read the IER to ensure any interrupt disabling has
+* completed before proceeding with ISR. */
+ if (dwapb_offset == UART_IER)
+ value = serial_in(up, dwapb_offset); #endif
break;
Hm, was there really a need for #ifdef mess here?
I'd vote for introducing new UPIO_* here, like was done
for TSi10x UARTs just for the same reason.
I'm willing to do this, however IIRC there was a thread in late Sept.
which came to the conclusion that UPIO were to be used for different
access methods and not UART types. Do you see this as an exception?
In the second #ifdef CONFIG_PMC_MSP above the workaround is required
because of SoC interrupt design which is out-of-band with respect to r/w
of UART registers, it's not specific to the DesignWare UART.
@@ -1016,6 +1039,17 @@
up-bugs |= UART_BUG_NOMSR;
#endif
+ /* Workaround:
+* The DesignWare SoC UART part has a bug for all
+* versions before 3.03a (2005-07-18)
+* In brief, this is a non-standard 16550 in that the
THRE interrupt
+* will not re-assert itself simply by disabling and
re-enabling the
+* THRI bit in the IER, it is only re-enabled if a
character is actually
+* sent out.
+*/
+ if( up-port.flags
Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git mast er
CCing to linux-kernel as per AC's suggestion...
Original Message
Subject:Re: [PATCH] serial driver PMC MSP71xx, kernel linux-mips.git
mast er
Date: Mon, 22 Jan 2007 12:23:56 -0800
From: Sergei Shtylyov
Organization: MontaVista Software Inc.
To: Marc St-Jean
CC: [EMAIL PROTECTED], linux-serial@vger.kernel.org
References:
[EMAIL PROTECTED]
Hello.
Marc St-Jean wrote:
Your maile is strange: it line-wraps quotes but not your own text...
:-/
Here is a serial driver patch for the PMC-Sierra MSP71xx device.
There are three different fixes:
1. Fix for THRE errata
2. Fix for Busy Detect on LCR write
3. Workaround for interrupt/data concurrency issue
The first fix is handled cleanly using a UART_BUG_* flag.
Hm, I wouldn't call it clean...
Relative to the other two. Any recommended improvements on this one?
I already told: runtime check.
Thanks,
Marc
Signed-off-by: Marc St-Jean [EMAIL PROTECTED]
Index: linux_2_6/drivers/serial/8250.c
===
RCS file: linux_2_6/drivers/serial/8250.c,v retrieving revision
1.1.1.7 retrieving revision 1.9 diff -u -r1.1.1.7 -r1.9
--- linux_2_6/drivers/serial/8250.c 19 Oct 2006 21:00:58
- 1.1.1.7
+++ linux_2_6/drivers/serial/8250.c 19 Oct 2006 22:08:15
- 1.9
@@ -44,6 +44,10 @@
#include asm/io.h
#include asm/irq.h
+#ifdef CONFIG_PMC_MSP
+#include msp_regs.h
+#endif
+
#include 8250.h
/*
@@ -130,6 +134,9 @@
unsigned char mcr_mask; /* mask of user bits */
unsigned char mcr_force; /* mask of forced bits */
unsigned char lsr_break_flag;
+#ifdef CONFIG_PMC_MSP
+int dwapb_lcr;
/* saved LCR for DW APB WAR */
+#endif
There was already 'lcr' field there, couldn't it be used?
Possibly but the current driver doesn't always save the LCR value
before trying to write it. For example see serial8250_set_termios()
serial_outp(up, UART_LCR, cval);/* reset DLAB */
up-lcr = cval; /* Save LCR */
This certainly may be fixed.
It's impossible to ensure that going foward the driver will always
save the value before writing it.
We need to have it saved before since the write will cause an
interrupt, hence the save in serial_out.
You may rework driver to always save it on write to 'lcr' field (in
case
of your UART). I don't think it's going to spoil something there...
If that's not acceptable I can audit the driver and ensure all LCR
writes are first saved.
What's *certainly* undesirable is #ifdef mess. :-)
@@ -333,6 +340,10 @@
static void
serial_out(struct uart_8250_port *up, int offset, int value)
{
+#ifdef CONFIG_PMC_MSP
+/* Save the offset before it's remapped */
+int dwapb_offset = offset;
+#endif
offset = map_8250_out_reg(up, offset) up-port.regshift;
switch (up-port.iotype) {
@@ -342,7 +353,19 @@
break;
case UPIO_MEM:
+#ifdef CONFIG_PMC_MSP
+/* Save the LCR value so it can be re-written when a
+ * Busy Detect interrupt occurs. */
+if (dwapb_offset == UART_LCR)
+up-dwapb_lcr = value;
+#endif
writeb(value, up-port.membase + offset);
+#ifdef CONFIG_PMC_MSP
+/* Re-read the IER to ensure any interrupt disabling has
+ * completed before proceeding with ISR. */
+if (dwapb_offset == UART_IER)
+value = serial_in(up, dwapb_offset); #endif
break;
Hm, was there really a need for #ifdef mess here?
I'd vote for introducing new UPIO_* here, like was done
for TSi10x UARTs just for the same reason.
I'm willing to do this, however IIRC there was a thread in late Sept.
which came
to the conclusion that UPIO were to be used for different access
methods and not UART types.
AFAIR, almost every participant was left with his own opinion. I myself
changed it twice during the discussion, to finalyl mostly agree with
Russell. :-D
Do you see this as an exception?
Well, there's already an incident of using this to work around the
register access errata (that UPIO_TSI I mentioned) -- and I must note that
this is certainly cleaner than #ifdef's, which should be avoided at all
costs. :-)
In the second #ifdef CONFIG_PMC_MSP above the workaround is required
because of SoC
interrupt design which is out-of-band with respect to r/w of UART
registers, it's not
specific to the DesignWare UART.
Erm... so why is this under #ifdef?
-1141,6 +1175,12 @@
iir = serial_in(up, UART_IIR);
if (lsr UART_LSR_TEMT iir
UART_IIR_NO_INT)
transmit_chars
