Message-ID: <20071218001240.5cb42aae@siona>
Date:	Tue, 18 Dec 2007 00:12:40 +0100
From:	Haavard Skinnemoen <hskinnemoen@...el.com>
To:	"Remy Bohmer" <linux@...mer.net>
Cc:	"Andrew Victor" <linux@...im.org.za>,
	RT <linux-rt-users@...r.kernel.org>,
	"Steven Rostedt" <rostedt@...dmis.org>,
	"ARM Linux Mailing List" <linux-arm-kernel@...ts.arm.linux.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH]: Atmel Serial Console interrupt handler splitup

On Mon, 17 Dec 2007 21:56:30 +0100
"Remy Bohmer" <linux@...mer.net> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists