On Fri, 14 Dec 2007 12:46:09 +0100
"Remy Bohmer" <[email protected]> wrote:
> Hello Andrew,
>
> So, to come to a conclusion about this complex patch series, I
> attached all the latest versions to this mail. The latest patches from
> yesterday including inline are also included to make the set complete.
>
> So, this is the order in which the patches should be installed:
> 1) atmel_serial_cleanup.patch
> 2) atmel_serial_irq_splitup.patch
> 3) NEW: optional: add-atmel-serial-dma.patch, this merged the DMA
> code (from Chip Coldwell) in your 2.6.23 patch back on top of this
> series. Because the AT32 bug is not been fixed for a very long time, I
> do not expect it to be fixed soon, so I think a reordering is better
> to make preempt-RT work on AT91.
I'll give it a shot, but first I have some comments on your other
patches.
Btw, it would be nice if patches that affect more or less
architecture-independent drivers were posted to linux-kernel (added to
Cc.)
Also, it would be easier to review if you posted just one patch per
e-mail. I'm going to cut & paste a bit from your attachments.
> 4) For RT only: atmel_serial_irqf_nodelay.patch can be applied
> anywhere after 1 and 2
I'll ignore this for now.
> +#define lread(port) __raw_readl(port)
> +#define lwrite(v, port) __raw_writel(v, port)
Why is this necessary, and what does 'l' stand for?
> - struct uart_port uart; /* uart */
> - struct clk *clk; /* uart clock */
> - unsigned short suspended; /* is port suspended? */
> - int break_active; /* break being received */
> + struct uart_port uart; /* uart */
> + struct clk *clk; /* uart clock */
> + unsigned short suspended; /* is port suspended? */
> + int break_active; /* break being received */
Looks like you're adding one or more spaces before each TAB here. Why
is that an improvement?
> static void atmel_stop_tx(struct uart_port *port)
> {
> - struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
> -
> UART_PUT_IDR(port, ATMEL_US_TXRDY);
> }
>
> @@ -214,8 +216,6 @@ static void atmel_stop_tx(struct uart_po
> */
> static void atmel_start_tx(struct uart_port *port)
> {
> - struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
> -
> UART_PUT_IER(port, ATMEL_US_TXRDY);
> }
>
> @@ -224,8 +224,6 @@ static void atmel_start_tx(struct uart_p
> */
> static void atmel_stop_rx(struct uart_port *port)
> {
> - struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
> -
> UART_PUT_IDR(port, ATMEL_US_RXRDY);
> }
These conflict with David Brownell's "atmel_serial build warnings
begone" patch which was merged into mainline a few weeks ago.
> static void atmel_enable_ms(struct uart_port *port)
> {
> - UART_PUT_IER(port, ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC | ATMEL_US_CTSIC);
> + UART_PUT_IER(port,
> + ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC |
> + ATMEL_US_CTSIC);
This looks a bit funny...IMO it would be better to keep a few of them
on the first line and indent the next line with an extra tab. But I'm
not going to be difficult about it if it conforms with the existing
style in the driver.
> /*
> + * receive interrupt handler.
> + */
> +static inline void
> +atmel_handle_receive(struct uart_port *port, unsigned int pending)
Please drop "inline" here. The compiler will do it automatically if it
has only one caller, and if it at some point gets several callers, we
might not want to inline it after all.
> +{
> + struct atmel_uart_port *atmel_port = (struct atmel_uart_port *)port;
> +
> + /* Interrupt receive */
> + if (pending & ATMEL_US_RXRDY)
> + atmel_rx_chars(port);
> + else if (pending & ATMEL_US_RXBRK) {
> + /*
> + * End of break detected. If it came along
> + * with a character, atmel_rx_chars will
> + * handle it.
> + */
Looks like the indentation got messed up here.
> + UART_PUT_CR(port, ATMEL_US_RSTSTA);
> + UART_PUT_IDR(port, ATMEL_US_RXBRK);
> + atmel_port->break_active = 0;
> + }
> +}
> +
> +/*
> + * transmit interrupt handler.
> + */
> +static inline void
> +atmel_handle_transmit(struct uart_port *port, unsigned int pending)
> +{
> + /* Interrupt transmit */
> + if (pending & ATMEL_US_TXRDY)
> + atmel_tx_chars(port);
> +}
> +
> +/*
> + * status flags interrupt handler.
> + */
> +static inline void
> +atmel_handle_status(struct uart_port *port, unsigned int pending,
> + unsigned int status)
> +{
> + /* TODO: All reads to CSR will clear these interrupts! */
> + if (pending & ATMEL_US_RIIC)
> + port->icount.rng++;
> + if (pending & ATMEL_US_DSRIC)
> + port->icount.dsr++;
> + if (pending & ATMEL_US_DCDIC)
> + uart_handle_dcd_change(port, !(status & ATMEL_US_DCD));
> + if (pending & ATMEL_US_CTSIC)
> + uart_handle_cts_change(port, !(status & ATMEL_US_CTS));
> + if (pending &
> + (ATMEL_US_RIIC | ATMEL_US_DSRIC | ATMEL_US_DCDIC |
> + ATMEL_US_CTSIC))
> + wake_up_interruptible(&port->info->delta_msr_wait);
Perhaps indent the big OR expression with another TAB to separate it
from the body?
> @@ -422,7 +454,9 @@ static int atmel_startup(struct uart_por
> /*
> * Allocate the IRQ
> */
> - retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED, "atmel_serial", port);
> + retval =
> + request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
> + "atmel_serial", port);
I think request_irq() belongs on the same line as "retval =".
> static void atmel_shutdown(struct uart_port *port)
> {
> - struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
> -
Already in mainline.
> /* disable interrupts and drain transmitter */
> - imr = UART_GET_IMR(port); /* get interrupt mask */
> - UART_PUT_IDR(port, -1); /* disable all interrupts */
> - while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY)) { barrier(); }
> + imr = UART_GET_IMR(port); /* get interrupt mask */
> + UART_PUT_IDR(port, -1); /* disable all interrupts */
Please use TABs, not spaces. Might as well remove those comments...they
don't seem all that useful.
> + while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
> + barrier();
Should probably use cpu_relax(), but it's probably out of scope for a
codingstyle cleanup patch (and I don't think it matters on AVR32 or
ARM.)
> /*
> - * First, save IMR and then disable interrupts
> + * First, save IMR and then disable interrupts
> */
> imr = UART_GET_IMR(port); /* get interrupt mask */
> UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
> @@ -790,30 +828,32 @@ static void atmel_console_write(struct c
> uart_console_write(port, s, count, atmel_console_putchar);
>
> /*
> - * Finally, wait for transmitter to become empty
> - * and restore IMR
> + * Finally, wait for transmitter to become empty
> + * and restore IMR
> */
Looks like you're replacing TABs with spaces. Why?
> -// TODO: CR is a write-only register
> -// unsigned int cr;
> +/* TODO: CR is a write-only register
> +// unsigned int cr;
> //
> -// cr = UART_GET_CR(port) & (ATMEL_US_RXEN | ATMEL_US_TXEN);
> -// if (cr == (ATMEL_US_RXEN | ATMEL_US_TXEN)) {
> -// /* ok, the port was enabled */
> -// }
> +// cr = UART_GET_CR(port) & (ATMEL_US_RXEN | ATMEL_US_TXEN);
> +// if (cr == (ATMEL_US_RXEN | ATMEL_US_TXEN)) {
> +// / * ok, the port was enabled * /
> +// }*/
That's a funny mix of C and C++ comments. Why not simply use #if 0 and
kill all the C++ comments?
That said, I don't understand what "TODO" means in this context. CR
isn't going to be readable any time soon.
> @@ -845,10 +885,10 @@ static int __init atmel_console_setup(st
> int parity = 'n';
> int flow = 'n';
>
> - if (port->membase == 0) /* Port not initialized yet - delay setup */
> + if (port->membase == 0) /* Port not initialized yet - delay setup */
> return -ENODEV;
Looks better if you move the comment inside the body IMO.
> - UART_PUT_IDR(port, -1); /* disable interrupts */
> + UART_PUT_IDR(port, -1); /* disable interrupts */
Just kill that useless comment.
> @@ -880,13 +920,17 @@ static struct console atmel_console = {
> static int __init atmel_console_init(void)
> {
> if (atmel_default_console_device) {
> - add_preferred_console(ATMEL_DEVICENAME, atmel_default_console_device->id, NULL);
> - atmel_init_port(&(atmel_ports[atmel_default_console_device->id]), atmel_default_console_device);
> + add_preferred_console(ATMEL_DEVICENAME,
> + atmel_default_console_device->id, NULL);
> + atmel_init_port(
> + &(atmel_ports[atmel_default_console_device->id]),
> + atmel_default_console_device);
That looks a bit funny. If you're having trouble moving
&(atmel_ports... onto the same line as atmel_init_port, please consider
removing the redundant parentheses.
Moving on to the next patch...
> This patch splits up the interrupt handler of the serial port
> into a interrupt top-half and some tasklets.
>
> The goal is to get the interrupt top-half as short as possible to
> minimize latencies on interrupts. But the old code also does some
> calls in the interrupt handler that are not allowed on preempt-RT
> in IRQF_NODELAY context. This handler is executed in this context
> because of the interrupt sharing with the timer interrupt.
> The timer interrupt on Preempt-RT runs in IRQF_NODELAY context.
What calls are we talking about here? uart_insert_char() and
tty_flip_buffer_push()?
> 2 tasklets are used:
> * one for handling the error statuses
> * one for pushing the incoming characters into the tty-layer.
Why is it necessary with two tasklets?
> +struct atmel_uart_char {
> + unsigned int status;
> + unsigned int overrun;
> + unsigned int ch;
> + unsigned int flg;
> +};
Hmm. 16 bytes per char is a bit excessive, isn't it? How about
struct atmel_uart_char {
u16 ch;
u16 status;
};
where ch is the received character (up to 9 bits) and status is the
lowest 16 bits of the status register?
> +#define ATMEL_SERIAL_RINGSIZE 1024
> +
> +struct atmel_uart_ring {
> + unsigned int head;
> + unsigned int tail;
> + unsigned int count;
> + struct atmel_uart_char data[ATMEL_SERIAL_RINGSIZE];
> +};
Why not use struct circ_buf? Why do you need "count"?
Ok, that's all for now. I'm going to take a closer look at the DMA
patch and hopefully figure out why it isn't working on AVR32.
Thanks for your efforts in cleaning this stuff up.
Haavard
On Mon, 17 Dec 2007 13:17:01 +0100
Haavard Skinnemoen <[email protected]> wrote:
> > 3) NEW: optional: add-atmel-serial-dma.patch, this merged the DMA
> > code (from Chip Coldwell) in your 2.6.23 patch back on top of this
> > series. Because the AT32 bug is not been fixed for a very long time, I
> > do not expect it to be fixed soon, so I think a reordering is better
> > to make preempt-RT work on AT91.
>
> I'll give it a shot, but first I have some comments on your other
> patches.
I found a bug. The RX DMA buffer isn't invalidated before it's handed
over to the hardware. With the below patch, it seems to behave a lot
better on AVR32, but it still does funny things now and then (it seems
to dump a good chunk of stale TX data before rebooting), and magic sysrq
doesn't work.
I think I've spotted other potential races in the DMA code as well, but
they look a bit more difficult to hunt down. I'll give it another try
tomorrow.
Haavard
From: Haavard Skinnemoen <[email protected]>
Subject: [PATCH] atmel_serial: Sync DMA RX buffer after copying from it
Calling dma_sync_single_for_cpu() before reading the buffer isn't
enough. We need to call dma_sync_single_for_device() after we're done
reading as well.
Signed-off-by: Haavard Skinnemoen <[email protected]>
---
drivers/serial/atmel_serial.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 0610ad9..e23a3ba 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -391,6 +391,8 @@ static void atmel_pdc_endrx(struct uart_port *port)
tty_insert_flip_string(tty, pdc->buf + pdc->ofs, count);
tty_flip_buffer_push(tty);
+ dma_sync_single_for_device(port->dev, pdc->dma_addr,
+ pdc->dma_size, DMA_FROM_DEVICE);
port->icount.rx += count;
}
@@ -425,6 +427,8 @@ static void atmel_pdc_timeout(struct uart_port *port)
tty_insert_flip_string(tty, pdc->buf + pdc->ofs, count);
tty_flip_buffer_push(tty);
+ dma_sync_single_for_device(port->dev, pdc->dma_addr,
+ pdc->dma_size, DMA_FROM_DEVICE);
pdc->ofs = ofs;
port->icount.rx += count;
}
--
1.5.3.4
Hello Haavard,
> I'll give it a shot, but first I have some comments on your other
> patches.
Good news someone is working on this bug again. Also good news you
already found a bug in there.
> Btw, it would be nice if patches that affect more or less
> architecture-independent drivers were posted to linux-kernel (added to
> Cc.)
Not really architecture independant, I believe, because thos are
drivers for peripherals _inside_ Atmel Cores ;-)
> Also, it would be easier to review if you posted just one patch per
> e-mail. I'm going to cut & paste a bit from your attachments.
I know, but I have some troubles to get 'quilt mail' to work from
behind a proxy server, and attaching to a mail, at least it does not
corrupt the contents of the patches.
> > 4) For RT only: atmel_serial_irqf_nodelay.patch can be applied
> > anywhere after 1 and 2
>
> I'll ignore this for now.
OK.
> > +#define lread(port) __raw_readl(port)
> > +#define lwrite(v, port) __raw_writel(v, port)
>
> Why is this necessary, and what does 'l' stand for?
There is a huge list of macros below these definitions. By doing it
this way, the macros still fit on 80 characters wide, while without
them, I had split up the macros over several lines, which does not
make it more readable. That's all.
'l' refers at the last letter of __raw_readl, and writel. Nothing special.
>
> > - struct uart_port uart; /* uart */
> > - struct clk *clk; /* uart clock */
> > - unsigned short suspended; /* is port suspended? */
> > - int break_active; /* break being received */
> > + struct uart_port uart; /* uart */
> > + struct clk *clk; /* uart clock */
> > + unsigned short suspended; /* is port suspended? */
> > + int break_active; /* break being received */
>
> Looks like you're adding one or more spaces before each TAB here. Why
> is that an improvement?
I used scripts/Lindent to reformat the file, and then I removed the
quirks Lindent put in the file. Apparantly I missed that one.
> These conflict with David Brownell's "atmel_serial build warnings
> begone" patch which was merged into mainline a few weeks ago.
Hmm, I seem to have missed that one. Why is it not there in a
big-AT91-patch from Andrew?
> > /*
> > + * receive interrupt handler.
> > + */
> > +static inline void
> > +atmel_handle_receive(struct uart_port *port, unsigned int pending)
>
> Please drop "inline" here. The compiler will do it automatically if it
> has only one caller, and if it at some point gets several callers, we
> might not want to inline it after all.
Funny, This was the first thing that Andrew started complaining about.
He suggested to put an inline there which I had not. I already
mentioned that this was against the CodingStyle, but I also mentioned
that I did not wanted to start a fight about this :-)
So, to prevent a discussion, I added the inline...
> > @@ -422,7 +454,9 @@ static int atmel_startup(struct uart_por
> > /*
> > * Allocate the IRQ
> > */
> > - retval = request_irq(port->irq, atmel_interrupt, IRQF_SHARED, "atmel_serial", port);
> > + retval =
> > + request_irq(port->irq, atmel_interrupt, IRQF_SHARED,
> > + "atmel_serial", port);
>
> I think request_irq() belongs on the same line as "retval =".
I blame scripts/Lindent ;-)
> Please use TABs, not spaces. Might as well remove those comments...they
> don't seem all that useful.
Go ahead...
I did not remove any comment, even if they appear useless to me. I am
not the maintainer of this driver, and just wanted to improve it, so
that it was able of running on Preempt-RT.
Before being able to submit the change that really mattered to me, I
had to make the driver pass the scripts/checkpatch.pl check, otherwise
the patch-that-matters would be completely unreadable.
>
> > + while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
> > + barrier();
>
> Should probably use cpu_relax(), but it's probably out of scope for a
> codingstyle cleanup patch (and I don't think it matters on AVR32 or
> ARM.)
Agree.
> > /*
> > - * First, save IMR and then disable interrupts
> > + * First, save IMR and then disable interrupts
> > */
> > imr = UART_GET_IMR(port); /* get interrupt mask */
> > UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
> > @@ -790,30 +828,32 @@ static void atmel_console_write(struct c
> > uart_console_write(port, s, count, atmel_console_putchar);
> >
> > /*
> > - * Finally, wait for transmitter to become empty
> > - * and restore IMR
> > + * Finally, wait for transmitter to become empty
> > + * and restore IMR
> > */
>
> Looks like you're replacing TABs with spaces. Why?
????
> > -// TODO: CR is a write-only register
> > -// unsigned int cr;
> > +/* TODO: CR is a write-only register
> > +// unsigned int cr;
> > //
> > -// cr = UART_GET_CR(port) & (ATMEL_US_RXEN | ATMEL_US_TXEN);
> > -// if (cr == (ATMEL_US_RXEN | ATMEL_US_TXEN)) {
> > -// /* ok, the port was enabled */
> > -// }
> > +// cr = UART_GET_CR(port) & (ATMEL_US_RXEN | ATMEL_US_TXEN);
> > +// if (cr == (ATMEL_US_RXEN | ATMEL_US_TXEN)) {
> > +// / * ok, the port was enabled * /
> > +// }*/
>
> That's a funny mix of C and C++ comments. Why not simply use #if 0 and
> kill all the C++ comments?
As mentioned, did not want to change it that much.
> That said, I don't understand what "TODO" means in this context. CR
> isn't going to be readable any time soon.
I also did not understand what was meant here.
> > @@ -845,10 +885,10 @@ static int __init atmel_console_setup(st
> > int parity = 'n';
> > int flow = 'n';
> >
> > - if (port->membase == 0) /* Port not initialized yet - delay setup */
> > + if (port->membase == 0) /* Port not initialized yet - delay setup */
> > return -ENODEV;
>
> Looks better if you move the comment inside the body IMO.
And would add some extra brackets etc.
I did not try to make it perfect ;-)
>
> > - UART_PUT_IDR(port, -1); /* disable interrupts */
> > + UART_PUT_IDR(port, -1); /* disable interrupts */
>
> Just kill that useless comment.
Agree
> > @@ -880,13 +920,17 @@ static struct console atmel_console = {
> > static int __init atmel_console_init(void)
> > {
> > if (atmel_default_console_device) {
> > - add_preferred_console(ATMEL_DEVICENAME, atmel_default_console_device->id, NULL);
> > - atmel_init_port(&(atmel_ports[atmel_default_console_device->id]), atmel_default_console_device);
> > + add_preferred_console(ATMEL_DEVICENAME,
> > + atmel_default_console_device->id, NULL);
> > + atmel_init_port(
> > + &(atmel_ports[atmel_default_console_device->id]),
> > + atmel_default_console_device);
>
> That looks a bit funny. If you're having trouble moving
> &(atmel_ports... onto the same line as atmel_init_port, please consider
> removing the redundant parentheses.
Guess, that wouldn't fit either?
> Moving on to the next patch...
>
> > This patch splits up the interrupt handler of the serial port
> > into a interrupt top-half and some tasklets.
> >
> > The goal is to get the interrupt top-half as short as possible to
> > minimize latencies on interrupts. But the old code also does some
> > calls in the interrupt handler that are not allowed on preempt-RT
> > in IRQF_NODELAY context. This handler is executed in this context
> > because of the interrupt sharing with the timer interrupt.
> > The timer interrupt on Preempt-RT runs in IRQF_NODELAY context.
>
> What calls are we talking about here? uart_insert_char() and
> tty_flip_buffer_push()?
Yep. All calls that block on a Mutex somehow on Preempt-RT. (such as
spinlocks, wakeup_interruptible() and many of its friends.)
> > 2 tasklets are used:
> > * one for handling the error statuses
> > * one for pushing the incoming characters into the tty-layer.
>
> Why is it necessary with two tasklets?
To prevent rewriting the code completely. 1 change was in a read
routine, the other at a different location.
> > +struct atmel_uart_char {
> > + unsigned int status;
> > + unsigned int overrun;
> > + unsigned int ch;
> > + unsigned int flg;
> > +};
>
> Hmm. 16 bytes per char is a bit excessive, isn't it? How about
>
> struct atmel_uart_char {
> u16 ch;
> u16 status;
> };
>
> where ch is the received character (up to 9 bits) and status is the
> lowest 16 bits of the status register?
I used the NODELAY patch for the 8250 serial port as reference, it
contains similar code, and I tried to make both look the same.
>
> > +#define ATMEL_SERIAL_RINGSIZE 1024
> > +
> > +struct atmel_uart_ring {
> > + unsigned int head;
> > + unsigned int tail;
> > + unsigned int count;
> > + struct atmel_uart_char data[ATMEL_SERIAL_RINGSIZE];
> > +};
>
> Why not use struct circ_buf? Why do you need "count"?
See previous remark.
> Ok, that's all for now. I'm going to take a closer look at the DMA
> patch and hopefully figure out why it isn't working on AVR32.
>
> Thanks for your efforts in cleaning this stuff up.
>
> Haavard
>
Thank you for reviewing it.
I will have a look at it in again later this week.
Have you any idea how we can integrate both patch series (yours and
mine) without interfering each other? This patchset was quite
annoying, because through multiple channels patches are submitted, and
even stacked up...
Kind Regards,
Remy
On Mon, 17 Dec 2007 21:56:30 +0100
"Remy Bohmer" <[email protected]> wrote:
> > Btw, it would be nice if patches that affect more or less
> > architecture-independent drivers were posted to linux-kernel (added
> > to Cc.)
>
> Not really architecture independant, I believe, because thos are
> drivers for peripherals _inside_ Atmel Cores ;-)
Well, those Atmel cores cover two different architectures: ARM and
AVR32. But yeah, "architecture independent" is perhaps a bit of a
stretch ;-)
> > Also, it would be easier to review if you posted just one patch per
> > e-mail. I'm going to cut & paste a bit from your attachments.
>
> I know, but I have some troubles to get 'quilt mail' to work from
> behind a proxy server, and attaching to a mail, at least it does not
> corrupt the contents of the patches.
Ok, attachments are a lot better than corrupted patches.
> > > +#define lread(port) __raw_readl(port)
> > > +#define lwrite(v, port) __raw_writel(v, port)
> >
> > Why is this necessary, and what does 'l' stand for?
>
> There is a huge list of macros below these definitions. By doing it
> this way, the macros still fit on 80 characters wide, while without
> them, I had split up the macros over several lines, which does not
> make it more readable. That's all.
> 'l' refers at the last letter of __raw_readl, and writel. Nothing
> special.
Ok, makes sense.
> > Looks like you're adding one or more spaces before each TAB here.
> > Why is that an improvement?
>
> I used scripts/Lindent to reformat the file, and then I removed the
> quirks Lindent put in the file. Apparantly I missed that one.
Hmm. Lindent does such things? That's not very helpful...
> > These conflict with David Brownell's "atmel_serial build warnings
> > begone" patch which was merged into mainline a few weeks ago.
>
> Hmm, I seem to have missed that one. Why is it not there in a
> big-AT91-patch from Andrew?
I think it went in via Andrew Morton.
But it's fine by me -- it all worked out automatically when I applied it
to 2.6.23 and rebased.
> > > /*
> > > + * receive interrupt handler.
> > > + */
> > > +static inline void
> > > +atmel_handle_receive(struct uart_port *port, unsigned int
> > > pending)
> >
> > Please drop "inline" here. The compiler will do it automatically if
> > it has only one caller, and if it at some point gets several
> > callers, we might not want to inline it after all.
>
> Funny, This was the first thing that Andrew started complaining about.
> He suggested to put an inline there which I had not. I already
> mentioned that this was against the CodingStyle, but I also mentioned
> that I did not wanted to start a fight about this :-)
> So, to prevent a discussion, I added the inline...
Hmm, ok. Andrew, could you elaborate on why you want it to be inline?
> I did not remove any comment, even if they appear useless to me. I am
> not the maintainer of this driver, and just wanted to improve it, so
> that it was able of running on Preempt-RT.
> Before being able to submit the change that really mattered to me, I
> had to make the driver pass the scripts/checkpatch.pl check, otherwise
> the patch-that-matters would be completely unreadable.
I understand that.
> > > /*
> > > - * First, save IMR and then disable interrupts
> > > + * First, save IMR and then disable interrupts
> > > */
> > > imr = UART_GET_IMR(port); /* get interrupt mask */
> > > UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
> > > @@ -790,30 +828,32 @@ static void atmel_console_write(struct c
> > > uart_console_write(port, s, count, atmel_console_putchar);
> > >
> > > /*
> > > - * Finally, wait for transmitter to become empty
> > > - * and restore IMR
> > > + * Finally, wait for transmitter to become empty
> > > + * and restore IMR
> > > */
> >
> > Looks like you're replacing TABs with spaces. Why?
>
> ????
Originally, there's a TAB between '*' and First/Finally. The patch
replaces it with spaces.
That said, it's probably better to just replace the tab with a single
space...
> > > -// TODO: CR is a write-only register
> > > -// unsigned int cr;
> > > +/* TODO: CR is a write-only register
> > > +// unsigned int cr;
> > > //
> > > -// cr = UART_GET_CR(port) & (ATMEL_US_RXEN | ATMEL_US_TXEN);
> > > -// if (cr == (ATMEL_US_RXEN | ATMEL_US_TXEN)) {
> > > -// /* ok, the port was enabled */
> > > -// }
> > > +// cr = UART_GET_CR(port) & (ATMEL_US_RXEN | ATMEL_US_TXEN);
> > > +// if (cr == (ATMEL_US_RXEN | ATMEL_US_TXEN)) {
> > > +// / * ok, the port was enabled * /
> > > +// }*/
> >
> > That's a funny mix of C and C++ comments. Why not simply use #if 0
> > and kill all the C++ comments?
>
> As mentioned, did not want to change it that much.
Understandably. However, I think we should just kill that code
altogether and check BRGR instead. If BRGR is 0, the baud rate
generator doesn't run, so we can assume the port hasn't been
initialized.
I'll cook up a separate patch for that. Until then, I think we should
just leave it as it was.
> > That said, I don't understand what "TODO" means in this context. CR
> > isn't going to be readable any time soon.
>
> I also did not understand what was meant here.
I think I do. We want to check if the port is already enabled, but we
can't read it from CR. So I think we should use BRGR instead.
> > > @@ -845,10 +885,10 @@ static int __init atmel_console_setup(st
> > > int parity = 'n';
> > > int flow = 'n';
> > >
> > > - if (port->membase == 0) /* Port not initialized yet
> > > - delay setup */
> > > + if (port->membase == 0) /* Port not initialized yet - delay
> > > setup */ return -ENODEV;
> >
> > Looks better if you move the comment inside the body IMO.
>
> And would add some extra brackets etc.
> I did not try to make it perfect ;-)
No extra brackets are needed. Comments don't count.
> > > @@ -880,13 +920,17 @@ static struct console atmel_console = {
> > > static int __init atmel_console_init(void)
> > > {
> > > if (atmel_default_console_device) {
> > > - add_preferred_console(ATMEL_DEVICENAME,
> > > atmel_default_console_device->id, NULL);
> > > -
> > > atmel_init_port(&(atmel_ports[atmel_default_console_device->id]),
> > > atmel_default_console_device);
> > > + add_preferred_console(ATMEL_DEVICENAME,
> > > +
> > > atmel_default_console_device->id, NULL);
> > > + atmel_init_port(
> > > +
> > > &(atmel_ports[atmel_default_console_device->id]),
> > > + atmel_default_console_device);
> >
> > That looks a bit funny. If you're having trouble moving
> > &(atmel_ports... onto the same line as atmel_init_port, please
> > consider removing the redundant parentheses.
>
> Guess, that wouldn't fit either?
It does. Barely.
> > Moving on to the next patch...
> >
> > > This patch splits up the interrupt handler of the serial port
> > > into a interrupt top-half and some tasklets.
> > >
> > > The goal is to get the interrupt top-half as short as possible to
> > > minimize latencies on interrupts. But the old code also does some
> > > calls in the interrupt handler that are not allowed on preempt-RT
> > > in IRQF_NODELAY context. This handler is executed in this context
> > > because of the interrupt sharing with the timer interrupt.
> > > The timer interrupt on Preempt-RT runs in IRQF_NODELAY context.
> >
> > What calls are we talking about here? uart_insert_char() and
> > tty_flip_buffer_push()?
>
> Yep. All calls that block on a Mutex somehow on Preempt-RT. (such as
> spinlocks, wakeup_interruptible() and many of its friends.)
Right. Looks like the DMA patch call these functions from irq context
too...I guess it'll need the same treatment?
> > > 2 tasklets are used:
> > > * one for handling the error statuses
> > > * one for pushing the incoming characters into the tty-layer.
> >
> > Why is it necessary with two tasklets?
>
> To prevent rewriting the code completely. 1 change was in a read
> routine, the other at a different location.
Ok, but both originate from the interrupt handler...
> > > +struct atmel_uart_char {
> > > + unsigned int status;
> > > + unsigned int overrun;
> > > + unsigned int ch;
> > > + unsigned int flg;
> > > +};
> >
> > Hmm. 16 bytes per char is a bit excessive, isn't it? How about
> >
> > struct atmel_uart_char {
> > u16 ch;
> > u16 status;
> > };
> >
> > where ch is the received character (up to 9 bits) and status is the
> > lowest 16 bits of the status register?
>
> I used the NODELAY patch for the 8250 serial port as reference, it
> contains similar code, and I tried to make both look the same.
Ok, I see. But...
> > > +#define ATMEL_SERIAL_RINGSIZE 1024
This means that the buffer ends up at 16K. Since the buffer itself is
kept in a struct along with a few other pieces of data, we end up
kmalloc()ing 32KB of memory...
> > > +struct atmel_uart_ring {
> > > + unsigned int head;
> > > + unsigned int tail;
> > > + unsigned int count;
> > > + struct atmel_uart_char data[ATMEL_SERIAL_RINGSIZE];
> > > +};
> >
> > Why not use struct circ_buf? Why do you need "count"?
>
> See previous remark.
Ok.
> > Ok, that's all for now. I'm going to take a closer look at the DMA
> > patch and hopefully figure out why it isn't working on AVR32.
> >
> > Thanks for your efforts in cleaning this stuff up.
> >
> > Haavard
> >
>
> Thank you for reviewing it.
>
> I will have a look at it in again later this week.
>
> Have you any idea how we can integrate both patch series (yours and
> mine) without interfering each other? This patchset was quite
> annoying, because through multiple channels patches are submitted, and
> even stacked up...
I'm a bit tempted to just go ahead and do the changes we agree on and
post the result. Then I'll see if I can make the DMA stuff behave. I've
got a different driver lying around which is DMA-only and has a few
problems on its own. Integrating the good bits into the atmel_serial
driver will hopefully result in something that is better than either of
them.
Also, I think the DMA patch needs to be more integrated with your
"interrupt handler splitup" patch. No point in keeping two RX buffers
around, and the DMA code needs to obey the same rules as the rest of
the driver if it's going to work in -rt.
And I'd really like to have working DMA support on avr32 before
christmas ;-)
Haavard
On Mon, Dec 17, 2007 at 09:56:30PM +0100, Remy Bohmer wrote:
> > > +#define lread(port) __raw_readl(port)
> > > +#define lwrite(v, port) __raw_writel(v, port)
> >
> > Why is this necessary, and what does 'l' stand for?
>
> There is a huge list of macros below these definitions. By doing it
> this way, the macros still fit on 80 characters wide, while without
> them, I had split up the macros over several lines, which does not
> make it more readable. That's all.
> 'l' refers at the last letter of __raw_readl, and writel. Nothing special.
So why not keep to the Linux convention? How about at_readl() and
at_writel() ?
> > > /*
> > > + * receive interrupt handler.
> > > + */
> > > +static inline void
> > > +atmel_handle_receive(struct uart_port *port, unsigned int pending)
> >
> > Please drop "inline" here. The compiler will do it automatically if it
> > has only one caller, and if it at some point gets several callers, we
> > might not want to inline it after all.
>
> Funny, This was the first thing that Andrew started complaining about.
> He suggested to put an inline there which I had not. I already
> mentioned that this was against the CodingStyle, but I also mentioned
> that I did not wanted to start a fight about this :-)
> So, to prevent a discussion, I added the inline...
There's two schools of thought - those who want to add 'inline' keywords
all over the place and those who don't. It's quite correct that if a
static function will be inlined by the compiler as it sees fit. It
_can_ be that the compiler will chose not to inline it and that may
result in better register allocation in the caller, resulting in overall
faster code.
> >
> > > + while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
> > > + barrier();
> >
> > Should probably use cpu_relax(), but it's probably out of scope for a
> > codingstyle cleanup patch (and I don't think it matters on AVR32 or
> > ARM.)
>
> Agree.
Even though it doesn't matter at the moment, I rather like to think a
bit about the future. If the kernel has a simple and cheap mechanism
there's no reason to avoid using it.
>
> > > /*
> > > - * First, save IMR and then disable interrupts
> > > + * First, save IMR and then disable interrupts
> > > */
> > > imr = UART_GET_IMR(port); /* get interrupt mask */
> > > UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
> > > @@ -790,30 +828,32 @@ static void atmel_console_write(struct c
> > > uart_console_write(port, s, count, atmel_console_putchar);
> > >
> > > /*
> > > - * Finally, wait for transmitter to become empty
> > > - * and restore IMR
> > > + * Finally, wait for transmitter to become empty
> > > + * and restore IMR
> > > */
> >
> > Looks like you're replacing TABs with spaces. Why?
>
> ????
I think someone's mailer might be messing with the patches. The above
removed and added lines appear to be identical.
Hello Haavard,
> > Yep. All calls that block on a Mutex somehow on Preempt-RT. (such as
> > spinlocks, wakeup_interruptible() and many of its friends.)
> Right. Looks like the DMA patch call these functions from irq context
> too...I guess it'll need the same treatment?
That is correct. DMA code does do that, but the DMA code is not used
for DBGU, and _only_ the interrupt handler of DBGU runs in
IRQF_NODELAY context (because it is part of the System-IRQ and thus
shared with the timer-irq). The DMA code always runs in IRQ-thread
context, where it is safe to block on a mutex.
So, it is not mandatory to change that also. (It may be nice to do it anyway)
> > > > +struct atmel_uart_char {
> > > > + unsigned int status;
> > > > + unsigned int overrun;
> > > > + unsigned int ch;
> > > > + unsigned int flg;
> > > > +};
> > >
> > > Hmm. 16 bytes per char is a bit excessive, isn't it? How about
> > >
> > > struct atmel_uart_char {
> > > u16 ch;
> > > u16 status;
> > > };
> > >
> > > where ch is the received character (up to 9 bits) and status is the
> > > lowest 16 bits of the status register?
> > I used the NODELAY patch for the 8250 serial port as reference, it
> > contains similar code, and I tried to make both look the same.
> Ok, I see. But...
> > > > +#define ATMEL_SERIAL_RINGSIZE 1024
> This means that the buffer ends up at 16K. Since the buffer itself is
> kept in a struct along with a few other pieces of data, we end up
> kmalloc()ing 32KB of memory...
Oops... 32kB on X86 is not much, relatively, on ARM it is somewhat different.
We can also decrease the total RingSize, 128 would be good enough also.
I can look at it later this week.
> I'm a bit tempted to just go ahead and do the changes we agree on and
> post the result. Then I'll see if I can make the DMA stuff behave. I've
> got a different driver lying around which is DMA-only and has a few
> problems on its own.
Beware that, according to several older discussions, DBGU cannot make
use of DMA... (If I understood it correctly, it was because the buffer
first have to be full, before an interupt is generated. That would be
very annoying while typing 1 character at the time ;-)
> Also, I think the DMA patch needs to be more integrated with your
> "interrupt handler splitup" patch. No point in keeping two RX buffers
> around, and the DMA code needs to obey the same rules as the rest of
> the driver if it's going to work in -rt.
As mentioned, not necessarily...
> And I'd really like to have working DMA support on avr32 before
> christmas ;-)
Me too ;-)
Kind Regards,
Remy
On Mon, 17 Dec 2007 23:49:32 +0000
Russell King - ARM Linux <[email protected]> wrote:
> On Mon, Dec 17, 2007 at 09:56:30PM +0100, Remy Bohmer wrote:
> > > > +#define lread(port) __raw_readl(port)
> > > > +#define lwrite(v, port) __raw_writel(v, port)
> > >
> > > Why is this necessary, and what does 'l' stand for?
> >
> > There is a huge list of macros below these definitions. By doing it
> > this way, the macros still fit on 80 characters wide, while without
> > them, I had split up the macros over several lines, which does not
> > make it more readable. That's all.
> > 'l' refers at the last letter of __raw_readl, and writel. Nothing special.
>
> So why not keep to the Linux convention? How about at_readl() and
> at_writel() ?
Something like this perhaps?
#define at_readl(port, off) __raw_readl((port)->membase + (off))
#define at_writel(v, port, off) __raw_writel(v, (port)->membase + (off))
#define UART_PUT_CR(port, v) at_writel(v, port, ATMEL_US_CR)
#define UART_PUT_MR(port, v) at_writel(v, port, ATMEL_US_MR)
#define UART_PUT_IER(port, v) at_writel(v, port, ATMEL_US_IER)
#define UART_PUT_IDR(port, v) at_writel(v, port, ATMEL_US_IDR)
#define UART_PUT_CHAR(port, v) at_writel(v, port, ATMEL_US_THR)
#define UART_PUT_BRGR(port, v) at_writel(v, port, ATMEL_US_BRGR)
#define UART_PUT_RTOR(port, v) at_writel(v, port, ATMEL_US_RTOR)
That said, I wonder if it may actually be nicer to just use
at_writel()/at_readl() throughout the driver...but that's for a later
cleanup.
> > >
> > > > + while (!(UART_GET_CSR(port) & ATMEL_US_TXEMPTY))
> > > > + barrier();
> > >
> > > Should probably use cpu_relax(), but it's probably out of scope for a
> > > codingstyle cleanup patch (and I don't think it matters on AVR32 or
> > > ARM.)
> >
> > Agree.
>
> Even though it doesn't matter at the moment, I rather like to think a
> bit about the future. If the kernel has a simple and cheap mechanism
> there's no reason to avoid using it.
I can do it in a separate patch.
> >
> > > > /*
> > > > - * First, save IMR and then disable interrupts
> > > > + * First, save IMR and then disable interrupts
> > > > */
> > > > imr = UART_GET_IMR(port); /* get interrupt mask */
> > > > UART_PUT_IDR(port, ATMEL_US_RXRDY | ATMEL_US_TXRDY);
> > > > @@ -790,30 +828,32 @@ static void atmel_console_write(struct c
> > > > uart_console_write(port, s, count, atmel_console_putchar);
> > > >
> > > > /*
> > > > - * Finally, wait for transmitter to become empty
> > > > - * and restore IMR
> > > > + * Finally, wait for transmitter to become empty
> > > > + * and restore IMR
> > > > */
> > >
> > > Looks like you're replacing TABs with spaces. Why?
> >
> > ????
>
> I think someone's mailer might be messing with the patches. The above
> removed and added lines appear to be identical.
Yes, the difference was wiped out after a few times back and forth. It
was visible (or...not actually visible, but you know how to detect
these things) in the first couple of e-mails.
Haavard