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

[One Thousand Gnomes]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/