linux-kernel - Re: [PATCH] tty/serial: add support for Xilinx PS UART

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110419221530.7370d013@lxorguk.ukuu.org.uk>
Date:	Tue, 19 Apr 2011 22:15:30 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	John Linn <john.linn@...inx.com>
Cc:	<linux-kernel@...r.kernel.org>, <linux-serial@...r.kernel.org>
Subject: Re: [PATCH] tty/serial: add support for Xilinx PS UART

On Tue, 19 Apr 2011 14:14:52 -0600
John Linn <john.linn@...inx.com> wrote:

> The Xilinx PS Uart is used on the new ARM based SoC. This
> UART is not compatible with others such that a seperate
> driver is required.

Joyous. I wish people would standardise.

> +		 213 = /dev/ttyPS0		Xilinx PS serial port 0
> +		 214 = /dev/ttyPS1		Xilinx PS serial port 1
> +		 215 = /dev/ttyPS2		Xilinx PS serial port 2
> +		 216 = /dev/ttyPS3		Xilinx PS serial port 3

Is there a specific reason you need fixed minor numbers ? If not please
use a dynamic range and keep Linus happy.

> +/**
> + * xuartps_isr - Interrupt handler
> + * @irq: Irq number
> + * @dev_id: Id of the port
> + *
> + * Returns IRQHANDLED
> + **/
> +static irqreturn_t xuartps_isr(int irq, void *dev_id)
> +{
> +	struct uart_port *port = (struct uart_port *)dev_id;
> +	struct tty_struct *tty = port->state->port.tty;
> +	unsigned long flags;
> +	unsigned int isrstatus, numbytes;
> +	unsigned int data;
> +	char status = TTY_NORMAL;
> +
> +	spin_lock_irqsave(&port->lock, flags);

The ttys are refcounting now and your locking subtly wrong if you want to
avoid it (you lookup port-> stuff before you lock it)

The best way to do this is

	tty = tty_port_tty_get(&port->state->port);

	/* Returns a tty with reference or NULL */

	Do stuff

	tty_kref_put(tty);



> +static void xuartps_set_termios(struct uart_port *port,
> +				struct ktermios *termios, struct ktermios *old)
> +{

> +	/* Min baud rate = 6bps and Max Baud Rate is 10Mbps for 100Mhz clk */
> +	baud = uart_get_baud_rate(port, termios, old, 0, 460800);
> +	xuartps_set_baud_rate(port, baud);

So why pass 460800 ?

> +	if (termios->c_iflag & INPCK)
> +		port->read_status_mask |= XUARTPS_IXR_PARITY |
> +		XUARTPS_IXR_FRAMING;
> +
> +	if (termios->c_iflag & IGNPAR)
> +		port->ignore_status_mask |= XUARTPS_IXR_PARITY |
> +			XUARTPS_IXR_FRAMING | XUARTPS_IXR_OVERRUN;
> +
> +	/* ignore all characters if CREAD is not set */
> +	if ((termios->c_cflag & CREAD) == 0)
> +		port->ignore_status_mask |= XUARTPS_IXR_RXTRIG |
> +			XUARTPS_IXR_TOUT | XUARTPS_IXR_PARITY |
> +			XUARTPS_IXR_FRAMING | XUARTPS_IXR_OVERRUN;
> +
> +	mode_reg = xuartps_readl(XUARTPS_MR_OFFSET);
> +
> +	/* Handling Data Size */
> +	switch (termios->c_cflag & CSIZE) {
> +	case CS6:
> +		cval |= XUARTPS_MR_CHARLEN_6_BIT;
> +		break;
> +	case CS7:
> +		cval |= XUARTPS_MR_CHARLEN_7_BIT;
> +		break;
> +	default:
> +	case CS8:
> +		cval |= XUARTPS_MR_CHARLEN_8_BIT;
> +		break;

You need to clear/set appropriately any flags for hardware requests you
cannot meet. So your default should be

	termios->c_cflag &= ~CSIZE;
	termios->c_cflag |= CS8;

to rewrite CS5

And set the baud rate (see 8250.c for an example). Note that the helper
functions know about mapping slight errors so if you are asked for 9600
and the hardware does 9575 it will report B9600 as you'd expect not do
something crazy.

> +	xuartps_set_baud_rate(port, 9600);

This seems a bit odd in the startup or is there a hardware need to do
this and then fix the rate ? Ditto some of the others


Otherwise looks fine.

Alan
--
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/