Message-ID: <20121026135452.GB27141@arwen.pp.htv.fi>
Date:	Fri, 26 Oct 2012 16:54:52 +0300
From:	Felipe Balbi <balbi@...com>
To:	Vineet Gupta <Vineet.Gupta1@...opsys.com>
CC:	<balbi@...com>, <gregkh@...uxfoundation.org>,
	<alan@...ux.intel.com>, <arc-linux-dev@...opsys.com>,
	<linux-serial@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] serial/arc-uart: Add new driver

hi,

On Fri, Oct 26, 2012 at 06:17:26PM +0530, Vineet Gupta wrote:
> >> +#endif
> >> +};
> >> +
> >> +static void arc_serial_stop_rx(struct uart_port *port)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> > I would suggest using container_of() here. It's very unlikely to happen,
> > but if another field is added before struct uart_port member in your
> > structure, this will break.
> 
> I agree  that container_of() would make it future safe - but I don't
> foresee any significant changes to driver specially the arc_uart_port
> structure.

I would still do it. The way code is right now, container_of() will be
optmized into the same cast you have today, so no impacts there.

> >> +static void arc_serial_rx_chars(struct arc_uart_port *uart)
> >> +{
> >> +	struct tty_struct *tty = tty_port_tty_get(&uart->port.state->port);
> >> +	unsigned int status, ch, flg = 0;
> >> +
> >> +	if (!tty)
> >> +		return;
> > can this really happen ? why would you receive characters while tty is
> > NULL ?
> 
> Since we are getting a ref to tty - it makes sense to check if the
> pointer is not NULL. Alan had pointed to a possible hangup race which
> could yield a NULL tty. But I'm not really an expert in serial core to
> be sure if at all this will happen - so added the check.

fair enough...

> >> +	/*
> >> +	 * UART has 4 deep RX-FIFO. Driver's recongnition of this fact
> >> +	 * is very subtle. Here's how ...
> >> +	 * Upon getting a RX-Intr, such that RX-EMPTY=0, meaning data available,
> >> +	 * driver reads the DATA Reg and keeps doing that in a loop, until
> >> +	 * RX-EMPTY=1. Multiple chars being avail, with a single Interrupt,
> >> +	 * before RX-EMPTY=0, implies some sort of buffering going on in the
> >> +	 * controller, which is indeed the Rx-FIFO.
> >> +	 */
> >> +	while (!((status = UART_GET_STATUS(uart)) & RXEMPTY)) {
> >> +
> >> +		ch = UART_GET_DATA(uart);
> >> +		uart->port.icount.rx++;
> >> +
> >> +		if (unlikely(status & (RXOERR | RXFERR))) {
> >> +			if (status & RXOERR) {
> >> +				uart->port.icount.overrun++;
> >> +				flg = TTY_OVERRUN;
> >> +				UART_CLR_STATUS(uart, RXOERR);
> >> +			}
> >> +
> >> +			if (status & RXFERR) {
> >> +				uart->port.icount.frame++;
> >> +				flg = TTY_FRAME;
> >> +				UART_CLR_STATUS(uart, RXFERR);
> >> +			}
> >> +		} else
> >> +			flg = TTY_NORMAL;
> >> +
> >> +		if (unlikely(uart_handle_sysrq_char(&uart->port, ch)))
> >> +			goto done;
> >> +
> >> +		uart_insert_char(&uart->port, status, RXOERR, ch, flg);
> >> +
> >> +done:
> >> +		tty_flip_buffer_push(tty);
> >> +	}
> >> +
> >> +	tty_kref_put(tty);
> >> +}
> >> +
> >> +/*
> >> + * A note on the Interrupt handling state machine of this driver
> >> + *
> >> + * kernel printk writes funnel thru the console driver framework and in order
> >> + * to keep things simple as well as efficient, it writes to UART in polled
> >> + * mode, in one shot, and exits.
> >> + *
> >> + * OTOH, Userland output (via tty layer), uses interrupt based writes as there
> >> + * can be undeterministic delay between char writes.
> >> + *
> >> + * Thus Rx-interrupts are always enabled, while tx-interrupts are by default
> >> + * disabled.
> >> + *
> >> + * When tty has some data to send out, serial core calls driver's start_tx
> >> + * which
> >> + *   -checks-if-tty-buffer-has-char-to-send
> >> + *   -writes-data-to-uart
> >> + *   -enable-tx-intr
> >> + *
> >> + * Once data bits are pushed out, controller raises the Tx-room-avail-Interrupt.
> >> + * The first thing Tx ISR does is disable further Tx interrupts (as this could
> >> + * be the last char to send, before settling down into the quiet polled mode).
> >> + * It then calls the exact routine used by tty layer write to send out any
> >> + * more char in tty buffer. In case of sending, it re-enables Tx-intr. In case
> >> + * of no data, it remains disabled.
> >> + * This is how the transmit state machine is dynamically switched on/off
> >> + */
> >> +
> >> +static irqreturn_t arc_serial_isr(int irq, void *dev_id)
> >> +{
> >> +	struct arc_uart_port *uart = dev_id;
> >> +	unsigned int status;
> >> +
> >> +	status = UART_GET_STATUS(uart);
> >> +
> >> +	/*
> >> +	 * Single IRQ for both Rx (data available) Tx (room available) Interrupt
> >> +	 * notifications from the UART Controller.
> >> +	 * To demultiplex between the two, we check the relevant bits
> >> +	 */
> >> +	if ((status & RXIENB) && !(status & RXEMPTY)) {
> >> +
> >> +		/* already in ISR, no need of xx_irqsave */
> >> +		spin_lock(&uart->port.lock);
> >> +		arc_serial_rx_chars(uart);
> >> +		spin_unlock(&uart->port.lock);
> >> +	}
> >> +
> >> +	if ((status & TXIENB) && (status & TXEMPTY)) {
> >> +
> >> +		/* Unconditionally disable further Tx-Interrupts.
> >> +		 * will be enabled by tx_chars() if needed.
> >> +		 */
> >> +		UART_TX_IRQ_DISABLE(uart);
> >> +
> >> +		spin_lock(&uart->port.lock);
> >> +
> >> +		if (!uart_tx_stopped(&uart->port))
> >> +			arc_serial_tx_chars(uart);
> >> +
> >> +		spin_unlock(&uart->port.lock);
> >> +	}
> >> +
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static unsigned int arc_serial_get_mctrl(struct uart_port *port)
> >> +{
> >> +	/*
> >> +	 * Pretend we have a Modem status reg and following bits are
> >> +	 *  always set, to satify the serial core state machine
> >> +	 *  (DSR) Data Set Ready
> >> +	 *  (CTS) Clear To Send
> >> +	 *  (CAR) Carrier Detect
> >> +	 */
> >> +	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
> >> +}
> >> +
> >> +static void arc_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
> >> +{
> >> +	/* MCR not present */
> >> +}
> >> +
> >> +/* Enable Modem Status Interrupts */
> >> +
> >> +static void arc_serial_enable_ms(struct uart_port *port)
> >> +{
> >> +	/* MSR not present */
> >> +}
> >> +
> >> +static void arc_serial_break_ctl(struct uart_port *port, int break_state)
> >> +{
> >> +	/* ARC UART doesn't support sending Break signal */
> >> +}
> >> +
> >> +static int arc_serial_startup(struct uart_port *port)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> >> +
> >> +	/* Before we hook up the ISR, Disable all UART Interrupts */
> >> +	UART_ALL_IRQ_DISABLE(uart);
> >> +
> >> +	if (request_irq(uart->port.irq, arc_serial_isr, 0, "arc uart rx-tx",
> >> +			uart)) {
> >> +		pr_warn("Unable to attach ARC UART interrupt\n");
> >> +		return -EBUSY;
> >> +	}
> >> +
> >> +	UART_RX_IRQ_ENABLE(uart); /* Only Rx IRQ enabled to begin with */
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/* This is not really needed */
> >> +static void arc_serial_shutdown(struct uart_port *port)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> >> +	free_irq(uart->port.irq, uart);
> >> +}
> >> +
> >> +static void
> >> +arc_serial_set_termios(struct uart_port *port, struct ktermios *new,
> >> +		       struct ktermios *old)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> >> +	unsigned int baud, uartl, uarth, hw_val;
> >> +	unsigned long flags;
> >> +
> >> +	/*
> >> +	 * Use the generic handler so that any specially encoded baud rates
> >> +	 * such as SPD_xx flags or "%B0" can be handled
> >> +	 * Max Baud I suppose will not be more than current 115K * 4
> >> +	 * Formula for ARC UART is: hw-val = ((CLK/(BAUD*4)) -1)
> >> +	 * spread over two 8-bit registers
> >> +	 */
> >> +	baud = uart_get_baud_rate(port, new, old, 0, 460800);
> >> +
> >> +	hw_val = port->uartclk / (uart->baud * 4) - 1;
> >> +	uartl = hw_val & 0xFF;
> >> +	uarth = (hw_val >> 8) & 0xFF;
> >> +
> >> +	/*
> >> +	 * UART ISS(Instruction Set simulator) emulation has a subtle bug:
> >> +	 * A existing value of Baudh = 0 is used as a indication to startup
> >> +	 * it's internal state machine.
> >> +	 * Thus if baudh is set to 0, 2 times, it chokes.
> >> +	 * This happens with BAUD=115200 and the formaula above
> >> +	 * Until that is fixed, when running on ISS, we will set baudh to !0
> >> +	 */
> >> +	if (uart->is_emulated)
> >> +		uarth = 1;
> >> +
> >> +	spin_lock_irqsave(&port->lock, flags);
> >> +
> >> +	UART_ALL_IRQ_DISABLE(uart);
> >> +
> >> +	UART_SET_BAUDL(uart, uartl);
> >> +	UART_SET_BAUDH(uart, uarth);
> >> +
> >> +	UART_RX_IRQ_ENABLE(uart);
> >> +
> >> +	/*
> >> +	 * UART doesn't support Parity/Hardware Flow Control;
> >> +	 * Only supports 8N1 character size
> >> +	 */
> >> +	new->c_cflag &= ~(CMSPAR|CRTSCTS|CSIZE);
> >> +	new->c_cflag |= CS8;
> >> +
> >> +	if (old)
> >> +		tty_termios_copy_hw(new, old);
> >> +
> >> +	/* Don't rewrite B0 */
> >> +	if (tty_termios_baud_rate(new))
> >> +		tty_termios_encode_baud_rate(new, baud, baud);
> >> +
> >> +	uart_update_timeout(port, new->c_cflag, baud);
> >> +
> >> +	spin_unlock_irqrestore(&port->lock, flags);
> >> +}
> >> +
> >> +static const char *arc_serial_type(struct uart_port *port)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> >> +
> >> +	return uart->port.type == PORT_ARC ? DRIVER_NAME : NULL;
> >> +}
> >> +
> >> +/*
> >> + * Release the memory region(s) being used by 'port'.
> >> + */
> >> +static void arc_serial_release_port(struct uart_port *port)
> >> +{
> >> +}
> >> +
> >> +/*
> >> + * Request the memory region(s) being used by 'port'.
> >> + */
> >> +static int arc_serial_request_port(struct uart_port *port)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +/*
> >> + * Verify the new serial_struct (for TIOCSSERIAL).
> >> + */
> >> +static int
> >> +arc_serial_verify_port(struct uart_port *port, struct serial_struct *ser)
> >> +{
> >> +	return 0;
> >> +}
> > why all these empty functions with wrong comments above them ??
> 
> Copy/paste cruft. Empty functions deleted in next ver !
> Regarding verify_port, I'm not sure whether it needs to elaborately
> check for PORT_UNKNOWN -> PORT_ARC or can we simply continue to return
> 0. But IMHO the comment in there is right. No ?

nope, you say that you should verify the new serial_struct, but you
don't verify anything, just return.

> >> +/*
> >> + * Configure/autoconfigure the port.
> >> + */
> >> +static void arc_serial_config_port(struct uart_port *port, int flags)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> >> +
> >> +	if (flags & UART_CONFIG_TYPE &&
> >> +	    arc_serial_request_port(&uart->port) == 0)
> >> +		uart->port.type = PORT_ARC;
> >> +}
> >> +
> >> +#if defined(CONFIG_CONSOLE_POLL) || defined(CONFIG_SERIAL_ARC_CONSOLE)
> >> +
> >> +static void arc_serial_poll_putchar(struct uart_port *port, unsigned char chr)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> >> +
> >> +	while (!(UART_GET_STATUS(uart) & TXEMPTY))
> >> +		cpu_relax();
> >> +
> >> +	UART_SET_DATA(uart, chr);
> >> +}
> >> +#endif
> >> +
> >> +#ifdef CONFIG_CONSOLE_POLL
> >> +static int arc_serial_poll_getchar(struct uart_port *port)
> >> +{
> >> +	struct arc_uart_port *uart = (struct arc_uart_port *)port;
> >> +	unsigned char chr;
> >> +
> >> +	while (!(UART_GET_STATUS(uart) & RXEMPTY))
> >> +		cpu_relax();
> >> +
> >> +	chr = UART_GET_DATA(uart);
> >> +	return chr;
> >> +}
> >> +#endif
> >> +
> >> +static struct uart_ops arc_serial_pops = {
> >> +	.tx_empty	= arc_serial_tx_empty,
> >> +	.set_mctrl	= arc_serial_set_mctrl,
> >> +	.get_mctrl	= arc_serial_get_mctrl,
> >> +	.stop_tx	= arc_serial_stop_tx,
> >> +	.start_tx	= arc_serial_start_tx,
> >> +	.stop_rx	= arc_serial_stop_rx,
> >> +	.enable_ms	= arc_serial_enable_ms,
> >> +	.break_ctl	= arc_serial_break_ctl,
> >> +	.startup	= arc_serial_startup,
> >> +	.shutdown	= arc_serial_shutdown,
> >> +	.set_termios	= arc_serial_set_termios,
> >> +	.type		= arc_serial_type,
> >> +	.release_port	= arc_serial_release_port,
> >> +	.request_port	= arc_serial_request_port,
> >> +	.config_port	= arc_serial_config_port,
> >> +	.verify_port	= arc_serial_verify_port,
> >> +#ifdef CONFIG_CONSOLE_POLL
> >> +	.poll_put_char = arc_serial_poll_putchar,
> >> +	.poll_get_char = arc_serial_poll_getchar,
> >> +#endif
> >> +};
> >> +
> >> +static int __devinit
> >> +arc_uart_init_one(struct platform_device *pdev, struct arc_uart_port *uart)
> >> +{
> >> +	struct resource *res, *res2;
> >> +	unsigned long *plat_data;
> >> +
> >> +	if (pdev->id < 0 || pdev->id >= CONFIG_SERIAL_ARC_NR_PORTS) {
> >> +		dev_err(&pdev->dev, "Wrong uart platform device id.\n");
> >> +		return -ENOENT;
> >> +	}
> >> +
> >> +	plat_data = ((unsigned long *)(pdev->dev.platform_data));
> >> +	uart->baud = plat_data[0];
> >> +
> >> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	if (!res)
> >> +		return -ENODEV;
> >> +
> >> +	res2 = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> +	if (!res2)
> >> +		return -ENODEV;
> >> +
> >> +	uart->port.mapbase = res->start;
> >> +	uart->port.membase = ioremap_nocache(res->start, resource_size(res));
> >> +	if (!uart->port.membase)
> >> +		/* No point of pr_err since UART itself is hosed here */
> >> +		return -ENXIO;
> >> +
> >> +	uart->port.irq = res2->start;
> >> +	uart->port.dev = &pdev->dev;
> >> +	uart->port.iotype = UPIO_MEM;
> >> +	uart->port.flags = UPF_BOOT_AUTOCONF;
> >> +	uart->port.line = pdev->id;
> >> +	uart->port.ops = &arc_serial_pops;
> >> +
> >> +	uart->port.uartclk = plat_data[1];
> >> +	uart->port.fifosize = ARC_UART_TX_FIFO_SIZE;
> >> +
> >> +	/*
> >> +	 * uart_insert_char( ) uses it in decideding whether to ignore a
> >> +	 * char or not. Explicitly setting it here, removes the subtelty
> >> +	 */
> >> +	uart->port.ignore_status_mask = 0;
> >> +
> >> +	/* Real Hardware vs. emulated to work around a bug */
> >> +	uart->is_emulated = !!plat_data[2];
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +#ifdef CONFIG_SERIAL_ARC_CONSOLE
> >> +
> >> +static int __devinit arc_serial_console_setup(struct console *co, char *options)
> >> +{
> >> +	struct uart_port *port;
> >> +	int baud = 115200;
> >> +	int bits = 8;
> >> +	int parity = 'n';
> >> +	int flow = 'n';
> >> +
> >> +	if (co->index < 0 || co->index >= CONFIG_SERIAL_ARC_NR_PORTS)
> >> +		return -ENODEV;
> >> +
> >> +	/*
> >> +	 * The uart port backing the console (e.g. ttyARC1) might not have been
> >> +	 * init yet. If so, defer the console setup to after the port.
> >> +	 */
> >> +	port = &arc_uart_ports[co->index].port;
> >> +	if (!port->membase)
> >> +		return -ENODEV;
> >> +
> >> +	if (options)
> >> +		uart_parse_options(options, &baud, &parity, &bits, &flow);
> >> +
> >> +	/*
> >> +	 * Serial core will call port->ops->set_termios( )
> >> +	 * which will set the baud reg
> >> +	 */
> >> +	return uart_set_options(port, co, baud, parity, bits, flow);
> >> +}
> >> +
> >> +static void arc_serial_console_putchar(struct uart_port *port, int ch)
> >> +{
> >> +	arc_serial_poll_putchar(port, (unsigned char)ch);
> >> +}
> >> +
> >> +/*
> >> + * Interrupts are disabled on entering
> >> + */
> >> +static void arc_serial_console_write(struct console *co, const char *s,
> >> +				     unsigned int count)
> >> +{
> >> +	struct uart_port *port = &arc_uart_ports[co->index].port;
> >> +	unsigned long flags;
> >> +
> >> +	spin_lock_irqsave(&port->lock, flags);
> >> +	uart_console_write(port, s, count, arc_serial_console_putchar);
> >> +	spin_unlock_irqrestore(&port->lock, flags);
> >> +}
> >> +
> >> +static struct console arc_console = {
> >> +	.name	= ARC_SERIAL_DEV_NAME,
> >> +	.write	= arc_serial_console_write,
> >> +	.device	= uart_console_device,
> >> +	.setup	= arc_serial_console_setup,
> >> +	.flags	= CON_PRINTBUFFER,
> >> +	.index	= -1,
> >> +	.data	= &arc_uart_driver
> >> +};
> >> +
> >> +static __init void early_serial_write(struct console *con, const char *s,
> >> +					unsigned int n)
> >> +{
> >> +	struct uart_port *port = &arc_uart_ports[con->index].port;
> >> +	unsigned int i;
> >> +
> >> +	for (i = 0; i < n; i++, s++) {
> >> +		if (*s == '\n')
> >> +			arc_serial_poll_putchar(port, '\r');
> >> +		arc_serial_poll_putchar(port, *s);
> >> +	}
> >> +}
> >> +
> >> +static struct __initdata console arc_early_serial_console = {
> >> +	.name = "early_ARCuart",
> >> +	.write = early_serial_write,
> >> +	.flags = CON_PRINTBUFFER | CON_BOOT,
> >> +	.index = -1
> >> +};
> >> +
> >> +static int __devinit arc_serial_probe_earlyprintk(struct platform_device *pdev)
> >> +{
> >> +	arc_early_serial_console.index = pdev->id;
> >> +
> >> +	arc_uart_init_one(pdev, &arc_uart_ports[pdev->id]);
> >> +
> >> +	arc_serial_console_setup(&arc_early_serial_console, NULL);
> >> +
> >> +	register_console(&arc_early_serial_console);
> >> +	return 0;
> >> +}
> >> +#else
> >> +static int __devinit arc_serial_probe_earlyprintk(struct platform_device *pdev)
> >> +{
> >> +	return -ENODEV;
> >> +}
> >> +#endif	/* CONFIG_SERIAL_ARC_CONSOLE */
> >> +
> >> +static int __devinit arc_serial_probe(struct platform_device *pdev)
> >> +{
> >> +	struct arc_uart_port *uart;
> >> +	int rc;
> >> +
> >> +	if (is_early_platform_device(pdev))
> >> +		return arc_serial_probe_earlyprintk(pdev);
> >> +
> >> +	uart = &arc_uart_ports[pdev->id];
> >> +	rc = arc_uart_init_one(pdev, uart);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	return uart_add_one_port(&arc_uart_driver, &uart->port);
> >> +}
> >> +
> >> +static int __devexit arc_serial_remove(struct platform_device *pdev)
> >> +{
> >> +	/* This will never be called */
> >> +	return 0;
> >> +}
> >> +
> >> +static struct platform_driver arc_platform_driver = {
> >> +	.probe = arc_serial_probe,
> >> +	.remove = __devexit_p(arc_serial_remove),
> >> +	.driver = {
> >> +		.name = DRIVER_NAME,
> >> +		.owner = THIS_MODULE,
> >> +	 },
> >> +};
> >> +
> >> +#ifdef CONFIG_SERIAL_ARC_CONSOLE
> >> +/*
> >> + * Register an early platform driver of "earlyprintk" class.
> >> + * ARCH platform code installs the driver and probes the early devices
> >> + * The installation could rely on user specifying earlyprintk=xyx in cmd line
> >> + * or it could be done independently, for all "earlyprintk" class drivers.
> >> + * [see arch/arc/plat-arcfpga/platform.c]
> >> + */
> >> +early_platform_init("earlyprintk", &arc_platform_driver);
> >> +
> >> +#endif  /* CONFIG_SERIAL_ARC_CONSOLE */
> >> +
> >> +static int __init arc_serial_init(void)
> >> +{
> >> +	int ret;
> >> +
> >> +	pr_info("Serial: ARC serial driver: platform register\n");
> > please remove this line, it's just useless IMHO.
> 
> It has helped me enough in past when debugging the uncoupling of driver
> from ARC platform code. I'd rather keep it !

then make it a debugging print ;-)

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists