ip3106_uart oddity

ip3106_uart oddity

[Vitaly Wool]

Hello Russell,

it looks like drivers/serial/ip3106_uart.c was dropped from the mainline at some point I couldn't identify. Can you please confirm that?
I'd like to take the burden of restoring the UART functionality for PNX8550 boards in the mainline. This very UART HW is very weird and doesn't fit well into 8250 model, even with fixups like those that were introduced for Alchemy. It also differs from the IP_3106-based UARTs used on Philips ARM targets in registers layout so I'm not sure it's correct to call it ip3106_uart.
So, given the above, does it make sense to try make it fir into standard 8250 driver model or restore/rework the custom driver?

Thanks,
   Vitaly

Re: ip3106_uart oddity

[Russell King]

On Thu, Aug 17, 2006 at 06:29:48PM +0400, Vitaly Wool wrote:
> it looks like drivers/serial/ip3106_uart.c was dropped from the
> mainline at some point I couldn't identify. Can you please confirm
> that?

I am not aware of its addition nor removal of this file.  There was
au1x00_uart.c at one time.

> I'd like to take the burden of restoring the UART functionality for
> PNX8550 boards in the mainline. This very UART HW is very weird and
> doesn't fit well into 8250 model, even with fixups like those that
> were introduced for Alchemy. It also differs from the IP_3106-based
> UARTs used on Philips ARM targets in registers layout so I'm not
> sure it's correct to call it ip3106_uart.
> So, given the above, does it make sense to try make it fir into
> standard 8250 driver model or restore/rework the custom driver?

No real clue.  Is it similar to any other drivers?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: ip3106_uart oddity

[Jean-Paul Saman]

linux-kernel-owner@vger.kernel.org wrote on 17-08-2006 22:29:54:

> On Thu, Aug 17, 2006 at 06:29:48PM +0400, Vitaly Wool wrote:
> > it looks like drivers/serial/ip3106_uart.c was dropped from the
> > mainline at some point I couldn't identify. Can you please confirm
> > that?
> 
Looks like someone wanted to rename it, but forgot to include the new 
file.

> I am not aware of its addition nor removal of this file.  There was
> au1x00_uart.c at one time.
> 
> > I'd like to take the burden of restoring the UART functionality for
> > PNX8550 boards in the mainline. This very UART HW is very weird and
> > doesn't fit well into 8250 model, even with fixups like those that
> > were introduced for Alchemy. It also differs from the IP_3106-based
> > UARTs used on Philips ARM targets in registers layout so I'm not
> > sure it's correct to call it ip3106_uart.
> > So, given the above, does it make sense to try make it fir into
> > standard 8250 driver model or restore/rework the custom driver?
> 
> No real clue.  Is it similar to any other drivers?

The ip3106_uart.c file that was used in the PNX8550 boards is wrongly 
named. The uart just isn't an ip3106, because those are used in philips 
ARM based devices.

If you restore the ip3106T_uart.c, then please rename it pnx8550_uart.c 
(or pnx8xxxx_uart.c).

Kind greetings,

Jean-Paul Saman

Philips Semiconductors

Re: ip3106_uart oddity

[Russell King]

On Fri, Aug 18, 2006 at 09:46:01AM +0200, Jean-Paul Saman wrote:
> linux-kernel-owner@vger.kernel.org wrote on 17-08-2006 22:29:54:
> > On Thu, Aug 17, 2006 at 06:29:48PM +0400, Vitaly Wool wrote:
> > > I'd like to take the burden of restoring the UART functionality for
> > > PNX8550 boards in the mainline. This very UART HW is very weird and
> > > doesn't fit well into 8250 model, even with fixups like those that
> > > were introduced for Alchemy. It also differs from the IP_3106-based
> > > UARTs used on Philips ARM targets in registers layout so I'm not
> > > sure it's correct to call it ip3106_uart.
> > > So, given the above, does it make sense to try make it fir into
> > > standard 8250 driver model or restore/rework the custom driver?
> > 
> > No real clue.  Is it similar to any other drivers?
> 
> The ip3106_uart.c file that was used in the PNX8550 boards is wrongly 
> named. The uart just isn't an ip3106, because those are used in philips 
> ARM based devices.
> 
> If you restore the ip3106T_uart.c, then please rename it pnx8550_uart.c 
> (or pnx8xxxx_uart.c).

There are no plans what so ever to "restore" what was never even there.
Searching around, there was a pnx0105 driver submitted but needed some
additional work which was never done.

The same situation seems to apply to this driver.  Ralf submitted a
driver called ip3106_uart.c which claimed to be a rewrite of
pnx8550_uart.c.  Comments were given at that time, no real feedback
came of that.

I would suggest you read the "Serial driver for the Philips PNX8550
SOC." thread, but it was a private one between Ralf, me and an
embeddedalley.com employee (who allegedly was the author of the driver
and who volunteered to do the work to fix the comments, but seemingly
never actually did anything.)

I suggest you ask embeddedalley.com...

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: ip3106_uart oddity

[Vitaly Wool]

On Fri, 18 Aug 2006 09:27:56 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:
 
> There are no plans what so ever to "restore" what was never even there.
> Searching around, there was a pnx0105 driver submitted but needed some
> additional work which was never done.

pnx0105's uart fits well into 8250 concept, so I don't think it will ever show up.
 
> The same situation seems to apply to this driver.  Ralf submitted a
> driver called ip3106_uart.c which claimed to be a rewrite of
> pnx8550_uart.c.  Comments were given at that time, no real feedback
> came of that.

Is it possbile that you include comments in the reply to this message? TIA! :)

Vitaly

Re: ip3106_uart oddity

[Russell King]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On Fri, Aug 18, 2006 at 03:23:46PM +0400, Vitaly Wool wrote:
> On Fri, 18 Aug 2006 09:27:56 +0100
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > There are no plans what so ever to "restore" what was never even there.
> > Searching around, there was a pnx0105 driver submitted but needed some
> > additional work which was never done.
> 
> pnx0105's uart fits well into 8250 concept, so I don't think it will ever show up.
>  
> > The same situation seems to apply to this driver.  Ralf submitted a
> > driver called ip3106_uart.c which claimed to be a rewrite of
> > pnx8550_uart.c.  Comments were given at that time, no real feedback
> > came of that.
> 
> Is it possbile that you include comments in the reply to this message? TIA! :)

Attached.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

[-- Attachment #2: Type: message/rfc822, Size: 4726 bytes --]

Subject: Re: [PATCH] Serial driver for the Philips PNX8550 SOC.
Date: Fri, 28 Oct 2005 21:48:01 +0100

On Sat, Oct 15, 2005 at 02:59:57AM +0100, Ralf Baechle wrote:
> Serial driver for the Philips PNX8550 SOC.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

> +static void
> +ip3106_rx_chars(struct ip3106_port *sport, struct pt_regs *regs)
> +{
> +	struct tty_struct *tty = sport->port.info->tty;
> +	unsigned int status, ch, flg, ignored = 0;
> +
> +	status = FIFO_TO_SM(serial_in(sport, IP3106_FIFO)) |
> +		 ISTAT_TO_SM(serial_in(sport, IP3106_ISTAT));
> +	while (status & FIFO_TO_SM(IP3106_UART_FIFO_RXFIFO)) {
> +		ch = serial_in(sport, IP3106_FIFO);
> +
> +		if (tty->flip.count >= TTY_FLIPBUF_SIZE)
> +			goto ignore_char;
> +		sport->port.icount.rx++;
> +
> +		flg = TTY_NORMAL;
> +
> +		/*
> +		 * note that the error handling code is
> +		 * out of the main execution path
> +		 */
> +		if (status & FIFO_TO_SM(IP3106_UART_FIFO_RXFE |
> +					IP3106_UART_FIFO_RXPAR))
> +			goto handle_error;
> +
> +		if (uart_handle_sysrq_char(&sport->port, ch, regs))
> +			goto ignore_char;
> +
> +	error_return:
> +		tty_insert_flip_char(tty, ch, flg);
> +	ignore_char:
> +		serial_out(sport, IP3106_LCR, serial_in(sport, IP3106_LCR) |
> +				IP3106_UART_LCR_RX_NEXT);
> +		status = FIFO_TO_SM(serial_in(sport, IP3106_FIFO)) |
> +			 ISTAT_TO_SM(serial_in(sport, IP3106_ISTAT));
> +	}
> + out:
> +	tty_flip_buffer_push(tty);
> +	return;
> +
> + handle_error:
> +	if (status & FIFO_TO_SM(IP3106_UART_FIFO_RXPAR))
> +		sport->port.icount.parity++;
> +	else if (status & FIFO_TO_SM(IP3106_UART_FIFO_RXFE))
> +		sport->port.icount.frame++;
> +	if (status & ISTAT_TO_SM(IP3106_UART_INT_RXOVRN))
> +		sport->port.icount.overrun++;
> +
> +	if (status & sport->port.ignore_status_mask) {
> +		if (++ignored > 100)
> +			goto out;
> +		goto ignore_char;
> +	}
> +
> +//	status &= sport->port.read_status_mask;
> +
> +	if (status & FIFO_TO_SM(IP3106_UART_FIFO_RXPAR))
> +		flg = TTY_PARITY;
> +	else if (status & FIFO_TO_SM(IP3106_UART_FIFO_RXFE))
> +		flg = TTY_FRAME;
> +
> +	if (status & ISTAT_TO_SM(IP3106_UART_INT_RXOVRN)) {
> +		/*
> +		 * overrun does *not* affect the character
> +		 * we read from the FIFO
> +		 */
> +		tty_insert_flip_char(tty, ch, flg);
> +		ch = 0;
> +		flg = TTY_OVERRUN;
> +	}
> +#ifdef SUPPORT_SYSRQ
> +	sport->port.sysrq = 0;
> +#endif
> +	goto error_return;
> +}

I removed these kinds of gotos in the other drivers to clean them up -
they don't actually buy anything.  I also added uart_insert_char() to
ensure that the overflow and ignoring handling was correct - it's
actually not correct above.  Please see current drivers/serial/sa1100.c
for the correct method.

> +#if	0	/* REVISIT */
> +	sport->port.read_status_mask &= UTSR0_TO_SM(UTSR0_TFS);
> +	sport->port.read_status_mask |= UTSR1_TO_SM(UTSR1_ROR);
> +	if (termios->c_iflag & INPCK)
> +		sport->port.read_status_mask |=
> +				UTSR1_TO_SM(UTSR1_FRE | UTSR1_PRE);
> +	if (termios->c_iflag & (BRKINT | PARMRK))
> +		sport->port.read_status_mask |=
> +				UTSR0_TO_SM(UTSR0_RBB | UTSR0_REB);
> +
> +	/*
> +	 * Characters to ignore
> +	 */
> +	sport->port.ignore_status_mask = 0;
> +	if (termios->c_iflag & IGNPAR)
> +		sport->port.ignore_status_mask |=
> +				UTSR1_TO_SM(UTSR1_FRE | UTSR1_PRE);
> +	if (termios->c_iflag & IGNBRK) {
> +		sport->port.ignore_status_mask |=
> +				UTSR0_TO_SM(UTSR0_RBB | UTSR0_REB);
> +		/*
> +		 * If we're ignoring parity and break indicators,
> +		 * ignore overruns too (for real raw support).
> +		 */
> +		if (termios->c_iflag & IGNPAR)
> +			sport->port.ignore_status_mask |=
> +				UTSR1_TO_SM(UTSR1_ROR);
> +	}
> +#endif

I'm slightly worried about this - it means that you're not capable of
handing some of the termios modes that programs may request.  If that
isn't a problem, you should force the termios settings to reflect that
(eg) you can't ignore parity.

I don't actually see any reason why you can't support any of these
settings though.

> +static int ip3106_serial_suspend(struct device *_dev, u32 state, u32 level)

This requires fixing up - u32 state is now pm_message_t state, and
u32 level has completely gone.

> +{
> +	struct ip3106_port *sport = dev_get_drvdata(_dev);
> +
> +	if (sport && level == SUSPEND_DISABLE)
> +		uart_suspend_port(&ip3106_reg, &sport->port);
> +
> +	return 0;
> +}
> +
> +static int ip3106_serial_resume(struct device *_dev, u32 level)

u32 level has completely gone.


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core