[4/4,RFC,for,Juno,2/2] tty: SBSA compatible UART

From: Grame Gregory <graeme.gregory@linaro.org>

This is a subset of pl011 UART which does not supprt DMA or baud rate
changing.

It is specified in the Server Base System Architecture document from
ARM.

Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
---
 drivers/tty/Kconfig    |   6 +
 drivers/tty/Makefile   |   1 +
 drivers/tty/sbsauart.c | 328 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 335 insertions(+)
 create mode 100644 drivers/tty/sbsauart.c

Firstly provide some useful information about the hardware. It's no good
wavng your arms at a document that requires agreeing to a giant ARM T&C
to get access to. Most of don't work for ARM and we'd have to get our own
corporate legal to approve the legal garbage involved.

Secondly explain why you can't use PL011 given that it already supports
non DMA accesses. What would it take to tweak it further for this ?


> +static void sbsa_tty_do_write(const char *buf, unsigned count)
> +{
> +	unsigned long irq_flags;
> +	struct sbsa_tty *qtty = sbsa_tty;
> +	void __iomem *base = qtty->base;
> +	unsigned n;
> +
> +	spin_lock_irqsave(&qtty->lock, irq_flags);
> +	for (n = 0; n < count; n++) {
> +		while (readw(base + UART01x_FR) & UART01x_FR_TXFF)
> +			mdelay(10);
> +		writew(buf[n], base + UART01x_DR);

serious - you are going to sit and spin in kernel space with interrupts
off for an indefinite period ?

No. Even if your hardware is so completely brain dead and broken that it
hasn't got an interrupt for 'write room' that's not acceptable. You need
error handling and some kind of sensible timer based solution.

To put it simply. You have a queue (or you should - your driver is broken
in that respect too), you have a baud rate, you have a bit time. From
that you can compute sensible wakeup points to try and refill the
hardware FIFO. Assuming the hardware fifo is not tiny you don't even have
to be that good an aim.

It's acceptable for the printk console code to spin, if you think getting
the message out on a failure or error outweighs the pain. It's not
acceptable for the tty layer.

> +static void sbsauart_fifo_to_tty(struct sbsa_tty *qtty)
> +{
> +	void __iomem *base = qtty->base;
> +	unsigned int flag, max_count = 32;
> +	u16 status, ch;
> +
> +	while (max_count--) {
> +		status = readw(base + UART01x_FR);
> +		if (status & UART01x_FR_RXFE)
> +			break;
> +
> +		/* Take chars from the FIFO and update status */
> +		ch = readw(base + UART01x_DR);
> +		flag = TTY_NORMAL;
> +
> +		if (ch & UART011_DR_BE)
> +			flag = TTY_BREAK;
> +		else if (ch & UART011_DR_PE)
> +			flag = TTY_PARITY;
> +		else if (ch & UART011_DR_FE)
> +			flag = TTY_FRAME;
> +		else if (ch & UART011_DR_OE)
> +			flag = TTY_OVERRUN;
> +
> +		ch &= SBSAUART_CHAR_MASK;
> +
> +		tty_insert_flip_char(&qtty->port, ch, flag);

If its a console you ought to support the sysrq interfaces.

> +static int sbsa_tty_write_room(struct tty_struct *tty)
> +{
> +	return 32;
> +}

You can't do this. You need a proper queue and queueing mechanism or
you'll break in some situations (aside from sitting spinning in your
write code trashing your system performance entirely). We have a kfifo
object in the kernel which is really trivial to use and should do what
you need without any effort.

> +
> +static void sbsa_tty_console_write(struct console *co, const char *b,
> +								unsigned count)
> +{
> +	sbsa_tty_do_write(b, count);
> +
> +	if(b[count - 1] == '\n');
> +		sbsa_tty_do_write("\r", 1);

I would expect \r\n to be the order ?

> +static struct tty_port_operations sbsa_port_ops = {
> +};

No power management ?

> +
> +static const struct tty_operations sbsa_tty_ops = {
> +	.open = sbsa_tty_open,
> +	.close = sbsa_tty_close,
> +	.hangup = sbsa_tty_hangup,
> +	.write = sbsa_tty_write,
> +	.write_room = sbsa_tty_write_room,

No termios handling ?

> +static int sbsa_tty_probe(struct platform_device *pdev)
> +{
> +	struct sbsa_tty *qtty;
> +	int ret = -EINVAL;
> +	int i;
> +	struct resource *r;
> +	struct device *ttydev;
> +	void __iomem *base;
> +	u32 irq;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (r == NULL)
> +		return -EINVAL;
> +
> +	base = ioremap(r->start, r->end - r->start);
> +	if (base == NULL)
> +		pr_err("sbsa_tty: unable to remap base\n");

So you are then going to continue and randomly crash  ???

Also you've got a device so use dev_err() and friends on it

> +	if (pdev->id > 0)
> +		goto err_unmap;

Why not test this before you do all the mapping ??


It's clean code, it's easy to understand it just doesn't seem to be very
complete ?

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

On Wed, Sep 17, 2014 at 11:40:29AM +0100, One Thousand Gnomes wrote:
> 
> Firstly provide some useful information about the hardware. It's no good
> wavng your arms at a document that requires agreeing to a giant ARM T&C
> to get access to. Most of don't work for ARM and we'd have to get our own
> corporate legal to approve the legal garbage involved.
> 
> Secondly explain why you can't use PL011 given that it already supports
> non DMA accesses. What would it take to tweak it further for this ?
> 
> 

As the original author of this driver it is only meant for internal use
inside Linaro. It only ever reached the level of "good enough" for
internal testing.

There is a discussion on a more complete driver in this thread.

http://www.spinics.net/lists/arm-kernel/msg358083.html

Graeme

> > +static void sbsa_tty_do_write(const char *buf, unsigned count)
> > +{
> > +	unsigned long irq_flags;
> > +	struct sbsa_tty *qtty = sbsa_tty;
> > +	void __iomem *base = qtty->base;
> > +	unsigned n;
> > +
> > +	spin_lock_irqsave(&qtty->lock, irq_flags);
> > +	for (n = 0; n < count; n++) {
> > +		while (readw(base + UART01x_FR) & UART01x_FR_TXFF)
> > +			mdelay(10);
> > +		writew(buf[n], base + UART01x_DR);
> 
> serious - you are going to sit and spin in kernel space with interrupts
> off for an indefinite period ?
> 
> No. Even if your hardware is so completely brain dead and broken that it
> hasn't got an interrupt for 'write room' that's not acceptable. You need
> error handling and some kind of sensible timer based solution.
> 
> To put it simply. You have a queue (or you should - your driver is broken
> in that respect too), you have a baud rate, you have a bit time. From
> that you can compute sensible wakeup points to try and refill the
> hardware FIFO. Assuming the hardware fifo is not tiny you don't even have
> to be that good an aim.
> 
> It's acceptable for the printk console code to spin, if you think getting
> the message out on a failure or error outweighs the pain. It's not
> acceptable for the tty layer.
> 
> > +static void sbsauart_fifo_to_tty(struct sbsa_tty *qtty)
> > +{
> > +	void __iomem *base = qtty->base;
> > +	unsigned int flag, max_count = 32;
> > +	u16 status, ch;
> > +
> > +	while (max_count--) {
> > +		status = readw(base + UART01x_FR);
> > +		if (status & UART01x_FR_RXFE)
> > +			break;
> > +
> > +		/* Take chars from the FIFO and update status */
> > +		ch = readw(base + UART01x_DR);
> > +		flag = TTY_NORMAL;
> > +
> > +		if (ch & UART011_DR_BE)
> > +			flag = TTY_BREAK;
> > +		else if (ch & UART011_DR_PE)
> > +			flag = TTY_PARITY;
> > +		else if (ch & UART011_DR_FE)
> > +			flag = TTY_FRAME;
> > +		else if (ch & UART011_DR_OE)
> > +			flag = TTY_OVERRUN;
> > +
> > +		ch &= SBSAUART_CHAR_MASK;
> > +
> > +		tty_insert_flip_char(&qtty->port, ch, flag);
> 
> If its a console you ought to support the sysrq interfaces.
> 
> > +static int sbsa_tty_write_room(struct tty_struct *tty)
> > +{
> > +	return 32;
> > +}
> 
> You can't do this. You need a proper queue and queueing mechanism or
> you'll break in some situations (aside from sitting spinning in your
> write code trashing your system performance entirely). We have a kfifo
> object in the kernel which is really trivial to use and should do what
> you need without any effort.
> 
> > +
> > +static void sbsa_tty_console_write(struct console *co, const char *b,
> > +								unsigned count)
> > +{
> > +	sbsa_tty_do_write(b, count);
> > +
> > +	if(b[count - 1] == '\n');
> > +		sbsa_tty_do_write("\r", 1);
> 
> I would expect \r\n to be the order ?
> 
> > +static struct tty_port_operations sbsa_port_ops = {
> > +};
> 
> No power management ?
> 
> > +
> > +static const struct tty_operations sbsa_tty_ops = {
> > +	.open = sbsa_tty_open,
> > +	.close = sbsa_tty_close,
> > +	.hangup = sbsa_tty_hangup,
> > +	.write = sbsa_tty_write,
> > +	.write_room = sbsa_tty_write_room,
> 
> No termios handling ?
> 
> > +static int sbsa_tty_probe(struct platform_device *pdev)
> > +{
> > +	struct sbsa_tty *qtty;
> > +	int ret = -EINVAL;
> > +	int i;
> > +	struct resource *r;
> > +	struct device *ttydev;
> > +	void __iomem *base;
> > +	u32 irq;
> > +
> > +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (r == NULL)
> > +		return -EINVAL;
> > +
> > +	base = ioremap(r->start, r->end - r->start);
> > +	if (base == NULL)
> > +		pr_err("sbsa_tty: unable to remap base\n");
> 
> So you are then going to continue and randomly crash  ???
> 
> Also you've got a device so use dev_err() and friends on it
> 
> > +	if (pdev->id > 0)
> > +		goto err_unmap;
> 
> Why not test this before you do all the mapping ??
> 
> 
> It's clean code, it's easy to understand it just doesn't seem to be very
> complete ?
> 
> Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/