[PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

[PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

[Johannes Thumshirn]

16z135 IP Core has changed so the driver needs to be updated to respect
these changes. The following changes have been made:

* Don't invert the 16z135 modem status register when reading.
* Add module parameter to configure the (baud rate dependent) RX timeout.
  Character timeout in seconds = (timeout_reg * baud_reg * 4)/freq_reg.
* Enable the handling of UART core's automatic flow control feature.
  When AFE is active disable generation of modem status IRQs.
* Rework the handling of IRQs to be conform with newer FPGA versions and
  take precautions not to miss an interrupt because of the destructive read
  of the IIR register.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@men.de>
---
 drivers/tty/serial/men_z135_uart.c | 87 +++++++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
index 30e9e60..d31b8a4 100644
--- a/drivers/tty/serial/men_z135_uart.c
+++ b/drivers/tty/serial/men_z135_uart.c
@@ -34,12 +34,11 @@
 #define MEN_Z135_CONF_REG		0x808
 #define MEN_Z135_UART_FREQ		0x80c
 #define MEN_Z135_BAUD_REG		0x810
-#define MENZ135_TIMEOUT			0x814
+#define MEN_Z135_TIMEOUT		0x814
 
 #define MEN_Z135_MEM_SIZE		0x818
 
-#define IS_IRQ(x) ((x) & 1)
-#define IRQ_ID(x) (((x) >> 1) & 7)
+#define IRQ_ID(x) ((x) & 0x1f)
 
 #define MEN_Z135_IER_RXCIEN BIT(0)		/* RX Space IRQ */
 #define MEN_Z135_IER_TXCIEN BIT(1)		/* TX Space IRQ */
@@ -94,11 +93,11 @@
 #define MEN_Z135_LSR_TEXP BIT(6)
 #define MEN_Z135_LSR_RXFIFOERR BIT(7)
 
-#define MEN_Z135_IRQ_ID_MST 0
-#define MEN_Z135_IRQ_ID_TSA 1
-#define MEN_Z135_IRQ_ID_RDA 2
-#define MEN_Z135_IRQ_ID_RLS 3
-#define MEN_Z135_IRQ_ID_CTI 6
+#define MEN_Z135_IRQ_ID_RLS BIT(0)
+#define MEN_Z135_IRQ_ID_RDA BIT(1)
+#define MEN_Z135_IRQ_ID_CTI BIT(2)
+#define MEN_Z135_IRQ_ID_TSA BIT(3)
+#define MEN_Z135_IRQ_ID_MST BIT(4)
 
 #define LCR(x) (((x) >> MEN_Z135_LCR_SHIFT) & 0xff)
 
@@ -118,6 +117,10 @@ static int align;
 module_param(align, int, S_IRUGO);
 MODULE_PARM_DESC(align, "Keep hardware FIFO write pointer aligned, default 0");
 
+static int rx_timeout;
+module_param(rx_timeout, uint, S_IRUGO);
+MODULE_PARM_DESC(rx_timeout, "RX timeout");
+
 struct men_z135_port {
 	struct uart_port port;
 	struct mcb_device *mdev;
@@ -373,43 +376,47 @@ out:
  * @irq: The IRQ number
  * @data: Pointer to UART port
  *
- * Check IIR register to see which tasklet to start.
+ * Check IIR register to find the cause of the interrupt and handle it.
+ * It is possible that multiple interrupts reason bits are set and reading
+ * the IIR is a destructive read, so we always need to check for all possible
+ * interrupts and handle them.
  */
 static irqreturn_t men_z135_intr(int irq, void *data)
 {
 	struct men_z135_port *uart = (struct men_z135_port *)data;
 	struct uart_port *port = &uart->port;
+	int handled = 0;
 	int irq_id;
 
 	uart->stat_reg = ioread32(port->membase + MEN_Z135_STAT_REG);
-	/* IRQ pending is low active */
-	if (IS_IRQ(uart->stat_reg))
-		return IRQ_NONE;
-
 	irq_id = IRQ_ID(uart->stat_reg);
-	switch (irq_id) {
-	case MEN_Z135_IRQ_ID_MST:
-		men_z135_handle_modem_status(uart);
-		break;
-	case MEN_Z135_IRQ_ID_TSA:
-		men_z135_handle_tx(uart);
-		break;
-	case MEN_Z135_IRQ_ID_CTI:
-		dev_dbg(&uart->mdev->dev, "Character Timeout Indication\n");
-		/* Fallthrough */
-	case MEN_Z135_IRQ_ID_RDA:
-		/* Reading data clears RX IRQ */
-		men_z135_handle_rx(uart);
-		break;
-	case MEN_Z135_IRQ_ID_RLS:
+
+	if (!irq_id)
+		goto out;
+
+	if (irq_id & MEN_Z135_IRQ_ID_RLS) {
 		men_z135_handle_lsr(uart);
-		break;
-	default:
-		dev_warn(&uart->mdev->dev, "Unknown IRQ id %d\n", irq_id);
-		return IRQ_NONE;
+		handled = 1;
 	}
 
-	return IRQ_HANDLED;
+	if (irq_id & MEN_Z135_IRQ_ID_RDA || irq_id & MEN_Z135_IRQ_ID_CTI) {
+		if (irq_id & MEN_Z135_IRQ_ID_CTI)
+			dev_dbg(&uart->mdev->dev, "Character Timeout Indication\n");
+		men_z135_handle_rx(uart);
+		handled = 1;
+	}
+
+	if (irq_id & MEN_Z135_IRQ_ID_TSA) {
+		men_z135_handle_tx(uart);
+		handled = 1;
+	}
+
+	if (irq_id & MEN_Z135_IRQ_ID_MST) {
+		men_z135_handle_modem_status(uart);
+		handled = 1;
+	}
+out:
+	return IRQ_RETVAL(handled);
 }
 
 /**
@@ -495,7 +502,7 @@ static unsigned int men_z135_get_mctrl(struct uart_port *port)
 
 	stat_reg = ioread32(port->membase + MEN_Z135_STAT_REG);
 
-	msr = ~((stat_reg >> 8) & 0xff);
+	msr = (stat_reg >> 8) & 0xff;
 
 	if (msr & MEN_Z135_MSR_CTS)
 		mctrl |= TIOCM_CTS;
@@ -584,6 +591,9 @@ static int men_z135_startup(struct uart_port *port)
 
 	iowrite32(conf_reg, port->membase + MEN_Z135_CONF_REG);
 
+	if (rx_timeout)
+		iowrite32(rx_timeout, port->membase + MEN_Z135_TIMEOUT);
+
 	return 0;
 }
 
@@ -603,6 +613,7 @@ static void men_z135_set_termios(struct uart_port *port,
 				struct ktermios *termios,
 				struct ktermios *old)
 {
+	struct men_z135_port *uart = to_men_z135(port);
 	unsigned int baud;
 	u32 conf_reg;
 	u32 bd_reg;
@@ -643,6 +654,14 @@ static void men_z135_set_termios(struct uart_port *port,
 	} else
 		lcr |= MEN_Z135_PAR_DIS << MEN_Z135_PEN_SHIFT;
 
+	if (termios->c_cflag & CRTSCTS) {
+		conf_reg |= MEN_Z135_MCR_RCFC;
+		men_z135_reg_clr(uart, MEN_Z135_CONF_REG, MEN_Z135_IER_MSIEN);
+	} else {
+		conf_reg &= ~MEN_Z135_MCR_RCFC;
+		men_z135_reg_set(uart, MEN_Z135_CONF_REG, MEN_Z135_IER_MSIEN);
+	}
+
 	termios->c_cflag &= ~CMSPAR; /* Mark/Space parity is not supported */
 
 	conf_reg |= lcr << MEN_Z135_LCR_SHIFT;
-- 
1.9.1

Re: [PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

[Greg Kroah-Hartman]

On Thu, Aug 28, 2014 at 02:32:20PM +0200, Johannes Thumshirn wrote:
> 16z135 IP Core has changed so the driver needs to be updated to respect
> these changes. The following changes have been made:
> 
> * Don't invert the 16z135 modem status register when reading.
> * Add module parameter to configure the (baud rate dependent) RX timeout.
>   Character timeout in seconds = (timeout_reg * baud_reg * 4)/freq_reg.
> * Enable the handling of UART core's automatic flow control feature.
>   When AFE is active disable generation of modem status IRQs.
> * Rework the handling of IRQs to be conform with newer FPGA versions and
>   take precautions not to miss an interrupt because of the destructive read
>   of the IIR register.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@men.de>
> ---
>  drivers/tty/serial/men_z135_uart.c | 87 +++++++++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/tty/serial/men_z135_uart.c b/drivers/tty/serial/men_z135_uart.c
> index 30e9e60..d31b8a4 100644
> --- a/drivers/tty/serial/men_z135_uart.c
> +++ b/drivers/tty/serial/men_z135_uart.c
> @@ -34,12 +34,11 @@
>  #define MEN_Z135_CONF_REG		0x808
>  #define MEN_Z135_UART_FREQ		0x80c
>  #define MEN_Z135_BAUD_REG		0x810
> -#define MENZ135_TIMEOUT			0x814
> +#define MEN_Z135_TIMEOUT		0x814
>  
>  #define MEN_Z135_MEM_SIZE		0x818
>  
> -#define IS_IRQ(x) ((x) & 1)
> -#define IRQ_ID(x) (((x) >> 1) & 7)
> +#define IRQ_ID(x) ((x) & 0x1f)
>  
>  #define MEN_Z135_IER_RXCIEN BIT(0)		/* RX Space IRQ */
>  #define MEN_Z135_IER_TXCIEN BIT(1)		/* TX Space IRQ */
> @@ -94,11 +93,11 @@
>  #define MEN_Z135_LSR_TEXP BIT(6)
>  #define MEN_Z135_LSR_RXFIFOERR BIT(7)
>  
> -#define MEN_Z135_IRQ_ID_MST 0
> -#define MEN_Z135_IRQ_ID_TSA 1
> -#define MEN_Z135_IRQ_ID_RDA 2
> -#define MEN_Z135_IRQ_ID_RLS 3
> -#define MEN_Z135_IRQ_ID_CTI 6
> +#define MEN_Z135_IRQ_ID_RLS BIT(0)
> +#define MEN_Z135_IRQ_ID_RDA BIT(1)
> +#define MEN_Z135_IRQ_ID_CTI BIT(2)
> +#define MEN_Z135_IRQ_ID_TSA BIT(3)
> +#define MEN_Z135_IRQ_ID_MST BIT(4)
>  
>  #define LCR(x) (((x) >> MEN_Z135_LCR_SHIFT) & 0xff)
>  
> @@ -118,6 +117,10 @@ static int align;
>  module_param(align, int, S_IRUGO);
>  MODULE_PARM_DESC(align, "Keep hardware FIFO write pointer aligned, default 0");
>  
> +static int rx_timeout;
> +module_param(rx_timeout, uint, S_IRUGO);
> +MODULE_PARM_DESC(rx_timeout, "RX timeout");

Why add a module parameter?  Who will know to set this?  And to what
value?

And what about this driver working on old hardware, didn't you just
break this?

thanks,

greg k-h

Re: [PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

[Johannes Thumshirn]

On Mon, Sep 08, 2014 at 04:17:33PM -0700, Greg Kroah-Hartman wrote:
> >
> > @@ -118,6 +117,10 @@ static int align;
> >  module_param(align, int, S_IRUGO);
> >  MODULE_PARM_DESC(align, "Keep hardware FIFO write pointer aligned, default 0");
> >
> > +static int rx_timeout;
> > +module_param(rx_timeout, uint, S_IRUGO);
> > +MODULE_PARM_DESC(rx_timeout, "RX timeout");
>
> Why add a module parameter?  Who will know to set this?  And to what
> value?
>

There will be a user manual describing the parameter. It's like the rxlvl and
txlvl parameters, you'll have to read the manual if you need other values than
the default ones.


> And what about this driver working on old hardware, didn't you just
> break this?
>

No. Currently the hardware is in a testing stage at only one customer and it is
an IP-Core inside a FPGA, so it can be (and already is) updated. So we can be
save to assume this is O.K.

> thanks,
>
> greg k-h

Hope that helped.
Johannes

Re: [PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

[Greg Kroah-Hartman]

On Tue, Sep 09, 2014 at 07:56:15AM +0200, Johannes Thumshirn wrote:
> On Mon, Sep 08, 2014 at 04:17:33PM -0700, Greg Kroah-Hartman wrote:
> > >
> > > @@ -118,6 +117,10 @@ static int align;
> > >  module_param(align, int, S_IRUGO);
> > >  MODULE_PARM_DESC(align, "Keep hardware FIFO write pointer aligned, default 0");
> > >
> > > +static int rx_timeout;
> > > +module_param(rx_timeout, uint, S_IRUGO);
> > > +MODULE_PARM_DESC(rx_timeout, "RX timeout");
> >
> > Why add a module parameter?  Who will know to set this?  And to what
> > value?
> >
> 
> There will be a user manual describing the parameter. It's like the rxlvl and
> txlvl parameters, you'll have to read the manual if you need other values than
> the default ones.

Ugh, but module parameters are for all devices in the system of this
type, which is why we try not to have them at all anymore.

And I don't like the other module parameters either :(

But it's an odd-one-off driver, I guess we can live...

> > And what about this driver working on old hardware, didn't you just
> > break this?
> >
> 
> No. Currently the hardware is in a testing stage at only one customer and it is
> an IP-Core inside a FPGA, so it can be (and already is) updated. So we can be
> save to assume this is O.K.

Ok, can you please resend this, it's gone from my queue now.

greg k-h

Re: [PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

[Johannes Thumshirn]

On Tue, Sep 09, 2014 at 05:26:35PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Sep 09, 2014 at 07:56:15AM +0200, Johannes Thumshirn wrote:
> > On Mon, Sep 08, 2014 at 04:17:33PM -0700, Greg Kroah-Hartman wrote:
> > > >
> > > > @@ -118,6 +117,10 @@ static int align;
> > > >  module_param(align, int, S_IRUGO);
> > > >  MODULE_PARM_DESC(align, "Keep hardware FIFO write pointer aligned, default 0");
> > > >
> > > > +static int rx_timeout;
> > > > +module_param(rx_timeout, uint, S_IRUGO);
> > > > +MODULE_PARM_DESC(rx_timeout, "RX timeout");
> > >
> > > Why add a module parameter?  Who will know to set this?  And to what
> > > value?
> > >
> >
> > There will be a user manual describing the parameter. It's like the rxlvl and
> > txlvl parameters, you'll have to read the manual if you need other values than
> > the default ones.
>
> Ugh, but module parameters are for all devices in the system of this
> type, which is why we try not to have them at all anymore.
>
> And I don't like the other module parameters either :(
>
> But it's an odd-one-off driver, I guess we can live...

Just for my information, what would be the correct way to do these kinds of
hardware specific settings? Sysfs? Debugfs? Custom ioctl()s (no struct file_operations,
so probably not)?

I guess we (and others) have a lot hardware that can be tweaked some way other
than what the framework supports and if module parameters aren't the way to go I
there must be some other way to configure this.

>
> > > And what about this driver working on old hardware, didn't you just
> > > break this?
> > >
> >
> > No. Currently the hardware is in a testing stage at only one customer and it is
> > an IP-Core inside a FPGA, so it can be (and already is) updated. So we can be
> > save to assume this is O.K.
>
> Ok, can you please resend this, it's gone from my queue now.

O.K. On the way.

Thanks,
Johannes

Re: [PATCH] tty: serial: men_z135_uart: Fix driver for changes in hardware

[Greg Kroah-Hartman]

On Wed, Sep 10, 2014 at 08:07:24AM +0200, Johannes Thumshirn wrote:
> On Tue, Sep 09, 2014 at 05:26:35PM -0700, Greg Kroah-Hartman wrote:
> > On Tue, Sep 09, 2014 at 07:56:15AM +0200, Johannes Thumshirn wrote:
> > > On Mon, Sep 08, 2014 at 04:17:33PM -0700, Greg Kroah-Hartman wrote:
> > > > >
> > > > > @@ -118,6 +117,10 @@ static int align;
> > > > >  module_param(align, int, S_IRUGO);
> > > > >  MODULE_PARM_DESC(align, "Keep hardware FIFO write pointer aligned, default 0");
> > > > >
> > > > > +static int rx_timeout;
> > > > > +module_param(rx_timeout, uint, S_IRUGO);
> > > > > +MODULE_PARM_DESC(rx_timeout, "RX timeout");
> > > >
> > > > Why add a module parameter?  Who will know to set this?  And to what
> > > > value?
> > > >
> > >
> > > There will be a user manual describing the parameter. It's like the rxlvl and
> > > txlvl parameters, you'll have to read the manual if you need other values than
> > > the default ones.
> >
> > Ugh, but module parameters are for all devices in the system of this
> > type, which is why we try not to have them at all anymore.
> >
> > And I don't like the other module parameters either :(
> >
> > But it's an odd-one-off driver, I guess we can live...
> 
> Just for my information, what would be the correct way to do these kinds of
> hardware specific settings? Sysfs? Debugfs? Custom ioctl()s (no struct file_operations,
> so probably not)?

For serial ports, odds are there is already some other way to do this
through a common interface...

But sysfs is a good way to go if you really need it, just document it
properly in Documentation/ABI/

thanks,

greg k-h