From: Alan Cox on
> +static void altera_jtaguart_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + struct ktermios *old)
> +{
> +}

Erm no .. it may be wrong in the code you are copying but please don't
further that. The requirement is that the termios handed back reflects
the actual settings not the requested ones.

Use tty_termios_copy_hw() to copy the old termios hardware settings back
so that the caller sees it cannot set them.

> +
> +static void altera_jtaguart_rx_chars(struct altera_jtaguart *pp)
> +{
> + struct uart_port *port = &pp->port;
> + unsigned char ch, flag;
> + unsigned long status;
> +
> + while ((status = readl(port->membase + ALTERA_JTAGUART_DATA_REG)) &
> + ALTERA_JTAGUART_DATA_RVALID_MSK) {
> + ch = status & ALTERA_JTAGUART_DATA_DATA_MSK;
> + flag = TTY_NORMAL;
> + port->icount.rx++;
> +
> + if (uart_handle_sysrq_char(port, ch))
> + continue;
> + uart_insert_char(port, 0, 0, ch, flag);
> + }
> +
> + tty_flip_buffer_push(port->state->port.tty);

port.tty needs to be protected here - you might race an unplug/hangup as
far as I can see - I think you need to take the lock over a bigger area

[I'm working on switching this to krefs in all the drivers currently so
that will end up cleaner]

Basically fine other than that. The proposed UART number clashes with
some other pending patches but that will get sorted out when they get
merged
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter Korsgaard on
>>>>> "Tobias" == Tobias Klauser <tklauser(a)distanz.ch> writes:

Tobias> Add an UART driver for the JTAG UART component available as a SOPC
Tobias> (System on Programmable Chip) component for Altera FPGAs.

Tobias> Signed-off-by: Tobias Klauser <tklauser(a)distanz.ch>

Tobias> +config SERIAL_ALTERA_JTAGUART
Tobias> + bool "Altera JTAG UART support"
Tobias> + depends on EMBEDDED

Don't depend on EMBEDDED - There's embedded devices not using
CONFIG_EMBEDDED and kernel configs for PCs using it.

--
Bye, Peter Korsgaard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tobias Klauser on
On 2010-02-25 at 16:26:22 +0100, Alan Cox <alan(a)lxorguk.ukuu.org.uk> wrote:
> > +static void altera_jtaguart_set_termios(struct uart_port *port,
> > + struct ktermios *termios,
> > + struct ktermios *old)
> > +{
> > +}
>
> Erm no .. it may be wrong in the code you are copying but please don't
> further that. The requirement is that the termios handed back reflects
> the actual settings not the requested ones.
>
> Use tty_termios_copy_hw() to copy the old termios hardware settings back
> so that the caller sees it cannot set them.

Would the following be sufficient?

if (old)
tty_termios_copy_hw(termios, old);

> > +static void altera_jtaguart_rx_chars(struct altera_jtaguart *pp)
> > +{
> > + struct uart_port *port = &pp->port;
> > + unsigned char ch, flag;
> > + unsigned long status;
> > +
> > + while ((status = readl(port->membase + ALTERA_JTAGUART_DATA_REG)) &
> > + ALTERA_JTAGUART_DATA_RVALID_MSK) {
> > + ch = status & ALTERA_JTAGUART_DATA_DATA_MSK;
> > + flag = TTY_NORMAL;
> > + port->icount.rx++;
> > +
> > + if (uart_handle_sysrq_char(port, ch))
> > + continue;
> > + uart_insert_char(port, 0, 0, ch, flag);
> > + }
> > +
> > + tty_flip_buffer_push(port->state->port.tty);
>
> port.tty needs to be protected here - you might race an unplug/hangup as
> far as I can see - I think you need to take the lock over a bigger area

So should we take port->lock in altera_jtaguart_interrupt already,
something like the following?

spin_lock(&port->lock);

if (isr & ALTERA_JTAGUART_CONTROL_RE_MSK)
altera_jtaguart_rx_chars(pp);
if (isr & ALTERA_JTAGUART_CONTROL_WE_MSK)
altera_jtaguart_tx_chars(pp);

spin_unlock(&port->lock);

And of course remove the lock/unlock from altera_jtaguart_tx_chars.

> [I'm working on switching this to krefs in all the drivers currently so
> that will end up cleaner]
>
> Basically fine other than that. The proposed UART number clashes with
> some other pending patches but that will get sorted out when they get
> merged

Thanks a lot for your review.

Tobias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on
> > Use tty_termios_copy_hw() to copy the old termios hardware settings back
> > so that the caller sees it cannot set them.
>
> Would the following be sufficient?
>
> if (old)
> tty_termios_copy_hw(termios, old);

That will do the trick yes.

> spin_lock(&port->lock);
>
> if (isr & ALTERA_JTAGUART_CONTROL_RE_MSK)
> altera_jtaguart_rx_chars(pp);
> if (isr & ALTERA_JTAGUART_CONTROL_WE_MSK)
> altera_jtaguart_tx_chars(pp);
>
> spin_unlock(&port->lock);

Yes - that stops ->tty becoming NULL while your handler is running

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on
On Fri, 5 Mar 2010 17:52:22 +0100
Tobias Klauser <tklauser(a)distanz.ch> wrote:

> Add an UART driver for the JTAG UART component available as a SOPC
> (System on Programmable Chip) component for Altera FPGAs.
>
> ...
>
> --- a/drivers/serial/Kconfig
> +++ b/drivers/serial/Kconfig
> @@ -1490,4 +1490,25 @@ config SERIAL_GRLIB_GAISLER_APBUART_CONSOLE
> help
> Support for running a console on the GRLIB APBUART
>
> +config SERIAL_ALTERA_JTAGUART
> + bool "Altera JTAG UART support"
> + select SERIAL_CORE
> + help
> + This driver supports the Altera JTAG UART port.
> +
> +config SERIAL_ALTERA_JTAGUART_CONSOLE
> + bool "Altera JTAG UART console support"
> + depends on SERIAL_ALTERA_JTAGUART
> + select SERIAL_CORE_CONSOLE
> + help
> + Enable a Altera JTAG UART port to be the system console.
> +
> +config SERIAL_ALTERA_JTAGUART_CONSOLE_BYPASS
> + bool "Bypass output when no connection"
> + depends on SERIAL_ALTERA_JTAGUART_CONSOLE
> + select SERIAL_CORE_CONSOLE
> + help
> + Bypass console output and keep going even if there is no
> + JTAG terminal connection with the host.

So this driver will be available on all CPU architectures.

I'm guessing that the hardware _isn't_ available on all CPU
architectures? Maybe that's wrong.

If this hardware is only available on certain CPUs then we face
tradeoffs. By making it universally available, the code gets better
compile tesst coverage and that can detect problems (usually minor
ones). otoh, it can increase your maintenance load a bit, and adds a
risk that people will ship unusable kernel modules and will see
increased build times. I have no particular opinion either way.


> endmenu
> diff --git a/drivers/serial/Makefile b/drivers/serial/Makefile
> index 5548fe7..6228b6e 100644
> --- a/drivers/serial/Makefile
> +++ b/drivers/serial/Makefile
> @@ -82,3 +82,4 @@ obj-$(CONFIG_KGDB_SERIAL_CONSOLE) += kgdboc.o
> obj-$(CONFIG_SERIAL_QE) += ucc_uart.o
> obj-$(CONFIG_SERIAL_TIMBERDALE) += timbuart.o
> obj-$(CONFIG_SERIAL_GRLIB_GAISLER_APBUART) += apbuart.o
> +obj-$(CONFIG_SERIAL_ALTERA_JTAGUART) += altera_jtaguart.o
> diff --git a/drivers/serial/altera_jtaguart.c b/drivers/serial/altera_jtaguart.c
> new file mode 100644
> index 0000000..f9b49b5
> --- /dev/null
> +++ b/drivers/serial/altera_jtaguart.c
> @@ -0,0 +1,504 @@
> +/*
> + * altera_jtaguart.c -- Altera JTAG UART driver
> + *
> + * Based on mcf.c -- Freescale ColdFire UART driver
> + *
> + * (C) Copyright 2003-2007, Greg Ungerer <gerg(a)snapgear.com>
> + * (C) Copyright 2008, Thomas Chou <thomas(a)wytron.com.tw>
> + * (C) Copyright 2010, Tobias Klauser <tklauser(a)distanz.ch>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/console.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/altera_jtaguart.h>

Does it make sense to put altera_jtaguart.h into include/linux? Could
we put it in drivers/serial/?

>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/