[PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Jamie Iles]

Some platforms contain a Synopsys DesignWare APB UART that is attached
to a 32-bit APB bus where sub-word accesses are not allowed. Add a new
IO type (UPIO_DWAPB32) that performs 32 bit acccesses to the UART.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/serial/8250.c        |   18 ++++++++++++++++--
 drivers/serial/serial_core.c |    2 ++
 include/linux/serial_core.h  |    1 +
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 09a5508..244a982 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -464,7 +464,12 @@ static void dwapb_serial_out(struct uart_port *p, int offset, int value)
 		struct uart_8250_port *up = (struct uart_8250_port *)p;
 		up->lcr = value;
 	}
-	writeb(value, p->membase + offset);
+
+	if (UPIO_DWAPB == p->iotype)
+		writeb(value, p->membase + offset);
+	else
+		writel(value, p->membase + offset);
+
 	/* Read the IER to ensure any interrupt is cleared before
 	 * returning from ISR. */
 	if (save_offset == UART_TX || save_offset == UART_IER)
@@ -518,6 +523,11 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_out = dwapb_serial_out;
 		break;
 
+	case UPIO_DWAPB32:
+		p->serial_in = mem32_serial_in;
+		p->serial_out = dwapb_serial_out;
+		break;
+
 	default:
 		p->serial_in = io_serial_in;
 		p->serial_out = io_serial_out;
@@ -536,6 +546,7 @@ serial_out_sync(struct uart_8250_port *up, int offset, int value)
 	case UPIO_MEM32:
 	case UPIO_AU:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		p->serial_out(p, offset, value);
 		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
 		break;
@@ -1581,7 +1592,8 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
 			handled = 1;
 
 			end = NULL;
-		} else if (up->port.iotype == UPIO_DWAPB &&
+		} else if ((up->port.iotype == UPIO_DWAPB ||
+			    up->port.iotype == UPIO_DWAPB32) &&
 			  (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
 			/* The DesignWare APB UART has an Busy Detect (0x07)
 			 * interrupt meaning an LCR write attempt occured while the
@@ -2476,6 +2488,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
@@ -2513,6 +2526,7 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 9ffa5be..143103b 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2134,6 +2134,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		snprintf(address, sizeof(address),
 			 "MMIO 0x%llx", (unsigned long long)port->mapbase);
 		break;
@@ -2554,6 +2555,7 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		return (port1->mapbase == port2->mapbase);
 	}
 	return 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 9ff9b7d..7a6daf1 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -314,6 +314,7 @@ struct uart_port {
 #define UPIO_TSI		(5)			/* Tsi108/109 type IO */
 #define UPIO_DWAPB		(6)			/* DesignWare APB UART */
 #define UPIO_RM9000		(7)			/* RM9000 type IO */
+#define UPIO_DWAPB32		(8)			/* DesignWare APB UART (32 bit accesses) */
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
-- 
1.7.2.3

Re: [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Greg KH]

On Mon, Nov 22, 2010 at 02:26:47PM +0000, Jamie Iles wrote:
> Some platforms contain a Synopsys DesignWare APB UART that is attached
> to a 32-bit APB bus where sub-word accesses are not allowed. Add a new
> IO type (UPIO_DWAPB32) that performs 32 bit acccesses to the UART.
> 
> Signed-off-by: Jamie Iles <jamie@jamieiles.com>
> ---
>  drivers/serial/8250.c        |   18 ++++++++++++++++--
>  drivers/serial/serial_core.c |    2 ++
>  include/linux/serial_core.h  |    1 +
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 09a5508..244a982 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -464,7 +464,12 @@ static void dwapb_serial_out(struct uart_port *p, int offset, int value)
>  		struct uart_8250_port *up = (struct uart_8250_port *)p;
>  		up->lcr = value;
>  	}
> -	writeb(value, p->membase + offset);
> +
> +	if (UPIO_DWAPB == p->iotype)

I understand why people feel they have to write code like this, but
please, don't, put the variable test the other way around.  The complier
will complain if you accidentally use '=' instead of '=='


> +		writeb(value, p->membase + offset);
> +	else
> +		writel(value, p->membase + offset);
> +

As this is on the "fast path" for writing data out, isn't it going to
slow down the UPIO_DWAPB users as well?

thanks,

greg k-h

Re: [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Jamie Iles]

On Tue, Nov 30, 2010 at 05:17:58PM -0800, Greg KH wrote:
> On Mon, Nov 22, 2010 at 02:26:47PM +0000, Jamie Iles wrote:
> > +		writeb(value, p->membase + offset);
> > +	else
> > +		writel(value, p->membase + offset);
> > +
> 
> As this is on the "fast path" for writing data out, isn't it going to
> slow down the UPIO_DWAPB users as well?
Yes, that's a fair point. Perhaps I was trying too hard to avoid copy and 
paste code! How about this instead?

Jamie

>From 19f6bf46ed0da675e8f98f58b3374634cfe3a7f2 Mon Sep 17 00:00:00 2001
From: Jamie Iles <jamie@jamieiles.com>
Date: Wed, 1 Dec 2010 07:44:37 +0000
Subject: [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses

Some platforms contain a Synopsys DesignWare APB UART that is attached
to a 32-bit APB bus where sub-word accesses are not allowed. Add a new
IO type (UPIO_DWAPB32) that performs 32 bit acccesses to the UART.

v2:
	- don't test for 32 bit in the output fast path, provide a
	  separate dwabp32_serial_out() function. Refactor
	  dwabp_serial_out() so that we can reuse the LCR saving
	  code.

Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/serial/8250.c        |   50 ++++++++++++++++++++++++++++++++---------
 drivers/serial/serial_core.c |    2 +
 include/linux/serial_core.h  |    1 +
 3 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 4d8e14b..df43cc3 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -454,21 +454,40 @@ static void tsi_serial_out(struct uart_port *p, int offset, int value)
 		writeb(value, p->membase + offset);
 }
 
+/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
+static inline void dwapb_save_out_value(struct uart_port *p, int offset,
+					int value)
+{
+	struct uart_8250_port *up =
+		container_of(p, struct uart_8250_port, port);
+
+	if (offset == UART_LCR)
+		up->lcr = value;
+}
+
+/* Read the IER to ensure any interrupt is cleared before returning from ISR. */
+static inline void dwapb_check_clear_ier(struct uart_port *p, int offset)
+{
+	if (offset == UART_TX || offset == UART_IER)
+		p->serial_in(p, UART_IER);
+}
+
 static void dwapb_serial_out(struct uart_port *p, int offset, int value)
 {
 	int save_offset = offset;
 	offset = map_8250_out_reg(p, offset) << p->regshift;
-	/* Save the LCR value so it can be re-written when a
-	 * Busy Detect interrupt occurs. */
-	if (save_offset == UART_LCR) {
-		struct uart_8250_port *up = (struct uart_8250_port *)p;
-		up->lcr = value;
-	}
+	dwapb_save_out_value(p, save_offset, value);
 	writeb(value, p->membase + offset);
-	/* Read the IER to ensure any interrupt is cleared before
-	 * returning from ISR. */
-	if (save_offset == UART_TX || save_offset == UART_IER)
-		value = p->serial_in(p, UART_IER);
+	dwapb_check_clear_ier(p, save_offset);
+}
+
+static void dwapb32_serial_out(struct uart_port *p, int offset, int value)
+{
+	int save_offset = offset;
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	dwapb_save_out_value(p, save_offset, value);
+	writel(value, p->membase + offset);
+	dwapb_check_clear_ier(p, save_offset);
 }
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
@@ -518,6 +537,11 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_out = dwapb_serial_out;
 		break;
 
+	case UPIO_DWAPB32:
+		p->serial_in = mem32_serial_in;
+		p->serial_out = dwapb32_serial_out;
+		break;
+
 	default:
 		p->serial_in = io_serial_in;
 		p->serial_out = io_serial_out;
@@ -536,6 +560,7 @@ serial_out_sync(struct uart_8250_port *up, int offset, int value)
 	case UPIO_MEM32:
 	case UPIO_AU:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		p->serial_out(p, offset, value);
 		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
 		break;
@@ -1581,7 +1606,8 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
 			handled = 1;
 
 			end = NULL;
-		} else if (up->port.iotype == UPIO_DWAPB &&
+		} else if ((up->port.iotype == UPIO_DWAPB ||
+			    up->port.iotype == UPIO_DWAPB32) &&
 			  (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
 			/* The DesignWare APB UART has an Busy Detect (0x07)
 			 * interrupt meaning an LCR write attempt occured while the
@@ -2476,6 +2502,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
@@ -2513,6 +2540,7 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 9ffa5be..143103b 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2134,6 +2134,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		snprintf(address, sizeof(address),
 			 "MMIO 0x%llx", (unsigned long long)port->mapbase);
 		break;
@@ -2554,6 +2555,7 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		return (port1->mapbase == port2->mapbase);
 	}
 	return 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 212eb4c..8681e7c 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -311,6 +311,7 @@ struct uart_port {
 #define UPIO_TSI		(5)			/* Tsi108/109 type IO */
 #define UPIO_DWAPB		(6)			/* DesignWare APB UART */
 #define UPIO_RM9000		(7)			/* RM9000 type IO */
+#define UPIO_DWAPB32		(8)			/* DesignWare APB UART (32 bit accesses) */
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
-- 
1.7.2.3

Re: [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Greg KH]

On Wed, Dec 01, 2010 at 08:10:44AM +0000, Jamie Iles wrote:
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -454,21 +454,40 @@ static void tsi_serial_out(struct uart_port *p, int offset, int value)
>  		writeb(value, p->membase + offset);
>  }
>  
> +/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
> +static inline void dwapb_save_out_value(struct uart_port *p, int offset,
> +					int value)
> +{
> +	struct uart_8250_port *up =
> +		container_of(p, struct uart_8250_port, port);

container_of, when the original code did a simple cast:

>  static void dwapb_serial_out(struct uart_port *p, int offset, int value)
>  {
>  	int save_offset = offset;
>  	offset = map_8250_out_reg(p, offset) << p->regshift;
> -	/* Save the LCR value so it can be re-written when a
> -	 * Busy Detect interrupt occurs. */
> -	if (save_offset == UART_LCR) {
> -		struct uart_8250_port *up = (struct uart_8250_port *)p;
> -		up->lcr = value;
> -	}
> +	dwapb_save_out_value(p, save_offset, value);

Because of that, are you sure this is correct now?  You might just be
getting lucky due to the location of the pointer within the structure,
but then the old code was wrong.

Either way, I don't feel comfortable with this, do you?

thanks,

greg k-h

Re: [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Jamie Iles]

On Wed, Dec 01, 2010 at 09:01:25AM -0800, Greg KH wrote:
> On Wed, Dec 01, 2010 at 08:10:44AM +0000, Jamie Iles wrote:
> > --- a/drivers/serial/8250.c
> > +++ b/drivers/serial/8250.c
> > @@ -454,21 +454,40 @@ static void tsi_serial_out(struct uart_port *p, int offset, int value)
> >  		writeb(value, p->membase + offset);
> >  }
> >  
> > +/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
> > +static inline void dwapb_save_out_value(struct uart_port *p, int offset,
> > +					int value)
> > +{
> > +	struct uart_8250_port *up =
> > +		container_of(p, struct uart_8250_port, port);
> 
> container_of, when the original code did a simple cast:
> 
> >  static void dwapb_serial_out(struct uart_port *p, int offset, int value)
> >  {
> >  	int save_offset = offset;
> >  	offset = map_8250_out_reg(p, offset) << p->regshift;
> > -	/* Save the LCR value so it can be re-written when a
> > -	 * Busy Detect interrupt occurs. */
> > -	if (save_offset == UART_LCR) {
> > -		struct uart_8250_port *up = (struct uart_8250_port *)p;
> > -		up->lcr = value;
> > -	}
> > +	dwapb_save_out_value(p, save_offset, value);
> 
> Because of that, are you sure this is correct now?  You might just be
> getting lucky due to the location of the pointer within the structure,
> but then the old code was wrong.
> 
> Either way, I don't feel comfortable with this, do you?
struct uart_8250_port is defined in drivers/serial/8250.c as:

struct uart_8250_port {
        struct uart_port        port;
        struct timer_list       timer;          /* "no irq" timer */
	...
};

So yes, I'm happy with the change but I could be missing something... I'm also 
happy to convert it back to a cast but container_of() feels a bit safer.  

I've just tried the patch below (on top of my previous patch) to convert all 
of the explicit casts (apart from the timer callbacks that take an unsigned 
long) to container_of() and that works fine on my board. Is this worth 
applying or would you rather I just keep the original cast?

Jamie

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index df43cc3..9839199 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -504,7 +504,8 @@ static void io_serial_out(struct uart_port *p, int offset, int value)
 
 static void set_io_from_upio(struct uart_port *p)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)p;
+	struct uart_8250_port *up =
+		container_of(p, struct uart_8250_port, port);
 	switch (p->iotype) {
 	case UPIO_HUB6:
 		p->serial_in = hub6_serial_in;
@@ -1344,7 +1345,8 @@ static inline void __stop_tx(struct uart_8250_port *p)
 
 static void serial8250_stop_tx(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	__stop_tx(up);
 
@@ -1361,7 +1363,8 @@ static void transmit_chars(struct uart_8250_port *up);
 
 static void serial8250_start_tx(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	if (!(up->ier & UART_IER_THRI)) {
 		up->ier |= UART_IER_THRI;
@@ -1389,7 +1392,8 @@ static void serial8250_start_tx(struct uart_port *port)
 
 static void serial8250_stop_rx(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	up->ier &= ~UART_IER_RLSI;
 	up->port.read_status_mask &= ~UART_LSR_DR;
@@ -1398,7 +1402,8 @@ static void serial8250_stop_rx(struct uart_port *port)
 
 static void serial8250_enable_ms(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	/* no MSR capabilities */
 	if (up->bugs & UART_BUG_NOMSR)
@@ -1807,7 +1812,8 @@ static void serial8250_backup_timeout(unsigned long data)
 
 static unsigned int serial8250_tx_empty(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 	unsigned int lsr;
 
@@ -1821,7 +1827,8 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
 
 static unsigned int serial8250_get_mctrl(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned int status;
 	unsigned int ret;
 
@@ -1841,7 +1848,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
 
 static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned char mcr = 0;
 
 	if (mctrl & TIOCM_RTS)
@@ -1862,7 +1870,8 @@ static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 
 static void serial8250_break_ctl(struct uart_port *port, int break_state)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 
 	spin_lock_irqsave(&up->port.lock, flags);
@@ -1916,7 +1925,8 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
 
 static int serial8250_get_poll_char(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned char lsr = serial_inp(up, UART_LSR);
 
 	if (!(lsr & UART_LSR_DR))
@@ -1930,7 +1940,8 @@ static void serial8250_put_poll_char(struct uart_port *port,
 			 unsigned char c)
 {
 	unsigned int ier;
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	/*
 	 *	First save the IER then disable the interrupts
@@ -1964,7 +1975,8 @@ static void serial8250_put_poll_char(struct uart_port *port,
 
 static int serial8250_startup(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 	unsigned char lsr, iir;
 	int retval;
@@ -2192,7 +2204,8 @@ dont_test_tx_en:
 
 static void serial8250_shutdown(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 
 	/*
@@ -2261,7 +2274,8 @@ void
 serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 		          struct ktermios *old)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned char cval, fcr = 0;
 	unsigned long flags;
 	unsigned int baud, quot;
@@ -2461,7 +2475,8 @@ serial8250_set_ldisc(struct uart_port *port, int new)
 void serial8250_do_pm(struct uart_port *port, unsigned int state,
 		      unsigned int oldstate)
 {
-	struct uart_8250_port *p = (struct uart_8250_port *)port;
+	struct uart_8250_port *p =
+		container_of(port, struct uart_8250_port, port);
 
 	serial8250_set_sleep(p, state != 0);
 }
@@ -2594,7 +2609,8 @@ static void serial8250_release_rsa_resource(struct uart_8250_port *up)
 
 static void serial8250_release_port(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	serial8250_release_std_resource(up);
 	if (up->port.type == PORT_RSA)
@@ -2603,7 +2619,8 @@ static void serial8250_release_port(struct uart_port *port)
 
 static int serial8250_request_port(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	int ret = 0;
 
 	ret = serial8250_request_std_resource(up);
@@ -2618,7 +2635,8 @@ static int serial8250_request_port(struct uart_port *port)
 
 static void serial8250_config_port(struct uart_port *port, int flags)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	int probeflags = PROBE_ANY;
 	int ret;
 
@@ -2799,7 +2817,8 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
 
 static void serial8250_console_putchar(struct uart_port *port, int ch)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	wait_for_xmitr(up, UART_LSR_THRE);
 	serial_out(up, UART_TX, ch);

Re: [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Greg KH]

On Wed, Dec 01, 2010 at 05:17:00PM +0000, Jamie Iles wrote:
> On Wed, Dec 01, 2010 at 09:01:25AM -0800, Greg KH wrote:
> > On Wed, Dec 01, 2010 at 08:10:44AM +0000, Jamie Iles wrote:
> > > --- a/drivers/serial/8250.c
> > > +++ b/drivers/serial/8250.c
> > > @@ -454,21 +454,40 @@ static void tsi_serial_out(struct uart_port *p, int offset, int value)
> > >  		writeb(value, p->membase + offset);
> > >  }
> > >  
> > > +/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
> > > +static inline void dwapb_save_out_value(struct uart_port *p, int offset,
> > > +					int value)
> > > +{
> > > +	struct uart_8250_port *up =
> > > +		container_of(p, struct uart_8250_port, port);
> > 
> > container_of, when the original code did a simple cast:
> > 
> > >  static void dwapb_serial_out(struct uart_port *p, int offset, int value)
> > >  {
> > >  	int save_offset = offset;
> > >  	offset = map_8250_out_reg(p, offset) << p->regshift;
> > > -	/* Save the LCR value so it can be re-written when a
> > > -	 * Busy Detect interrupt occurs. */
> > > -	if (save_offset == UART_LCR) {
> > > -		struct uart_8250_port *up = (struct uart_8250_port *)p;
> > > -		up->lcr = value;
> > > -	}
> > > +	dwapb_save_out_value(p, save_offset, value);
> > 
> > Because of that, are you sure this is correct now?  You might just be
> > getting lucky due to the location of the pointer within the structure,
> > but then the old code was wrong.
> > 
> > Either way, I don't feel comfortable with this, do you?
> struct uart_8250_port is defined in drivers/serial/8250.c as:
> 
> struct uart_8250_port {
>         struct uart_port        port;
>         struct timer_list       timer;          /* "no irq" timer */
> 	...
> };
> 

Ah, yeah, you are right.

> So yes, I'm happy with the change but I could be missing something... I'm also 
> happy to convert it back to a cast but container_of() feels a bit safer.  

I totally agree, it is safer and makes more sense.

> I've just tried the patch below (on top of my previous patch) to convert all 
> of the explicit casts (apart from the timer callbacks that take an unsigned 
> long) to container_of() and that works fine on my board. Is this worth 
> applying or would you rather I just keep the original cast?

Yes, it's worth applying.

So, care to send me 2 patches that I can apply with the proper changelog
comments and signed-off-by: for this?  I don't care which order you do
things in, which ever is easier for you.

thanks,

greg k-h

[PATCH 1/2] 8250: use container_of() instead of casting

[Jamie Iles]

The 8250 driver structure uart_8250_port took advantage of the fact
that the struct uart_port was the first member of its structure and
used an explicit cast to convert to the derived class. Replace the
explicit casts with container_of() for safety and clarity.

Cc: Greg KH <greg@kroah.com>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/serial/8250.c |   60 ++++++++++++++++++++++++++++++++----------------
 1 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 4d8e14b..0b4ce47 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -461,7 +461,8 @@ static void dwapb_serial_out(struct uart_port *p, int offset, int value)
 	/* Save the LCR value so it can be re-written when a
 	 * Busy Detect interrupt occurs. */
 	if (save_offset == UART_LCR) {
-		struct uart_8250_port *up = (struct uart_8250_port *)p;
+		struct uart_8250_port *up =
+			container_of(p, struct uart_8250_port, port);
 		up->lcr = value;
 	}
 	writeb(value, p->membase + offset);
@@ -485,7 +486,8 @@ static void io_serial_out(struct uart_port *p, int offset, int value)
 
 static void set_io_from_upio(struct uart_port *p)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)p;
+	struct uart_8250_port *up =
+		container_of(p, struct uart_8250_port, port);
 	switch (p->iotype) {
 	case UPIO_HUB6:
 		p->serial_in = hub6_serial_in;
@@ -1319,7 +1321,8 @@ static inline void __stop_tx(struct uart_8250_port *p)
 
 static void serial8250_stop_tx(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	__stop_tx(up);
 
@@ -1336,7 +1339,8 @@ static void transmit_chars(struct uart_8250_port *up);
 
 static void serial8250_start_tx(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	if (!(up->ier & UART_IER_THRI)) {
 		up->ier |= UART_IER_THRI;
@@ -1364,7 +1368,8 @@ static void serial8250_start_tx(struct uart_port *port)
 
 static void serial8250_stop_rx(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	up->ier &= ~UART_IER_RLSI;
 	up->port.read_status_mask &= ~UART_LSR_DR;
@@ -1373,7 +1378,8 @@ static void serial8250_stop_rx(struct uart_port *port)
 
 static void serial8250_enable_ms(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	/* no MSR capabilities */
 	if (up->bugs & UART_BUG_NOMSR)
@@ -1781,7 +1787,8 @@ static void serial8250_backup_timeout(unsigned long data)
 
 static unsigned int serial8250_tx_empty(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 	unsigned int lsr;
 
@@ -1795,7 +1802,8 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
 
 static unsigned int serial8250_get_mctrl(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned int status;
 	unsigned int ret;
 
@@ -1815,7 +1823,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
 
 static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned char mcr = 0;
 
 	if (mctrl & TIOCM_RTS)
@@ -1836,7 +1845,8 @@ static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
 
 static void serial8250_break_ctl(struct uart_port *port, int break_state)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 
 	spin_lock_irqsave(&up->port.lock, flags);
@@ -1890,7 +1900,8 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits)
 
 static int serial8250_get_poll_char(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned char lsr = serial_inp(up, UART_LSR);
 
 	if (!(lsr & UART_LSR_DR))
@@ -1904,7 +1915,8 @@ static void serial8250_put_poll_char(struct uart_port *port,
 			 unsigned char c)
 {
 	unsigned int ier;
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	/*
 	 *	First save the IER then disable the interrupts
@@ -1938,7 +1950,8 @@ static void serial8250_put_poll_char(struct uart_port *port,
 
 static int serial8250_startup(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 	unsigned char lsr, iir;
 	int retval;
@@ -2166,7 +2179,8 @@ dont_test_tx_en:
 
 static void serial8250_shutdown(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned long flags;
 
 	/*
@@ -2235,7 +2249,8 @@ void
 serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 		          struct ktermios *old)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	unsigned char cval, fcr = 0;
 	unsigned long flags;
 	unsigned int baud, quot;
@@ -2435,7 +2450,8 @@ serial8250_set_ldisc(struct uart_port *port, int new)
 void serial8250_do_pm(struct uart_port *port, unsigned int state,
 		      unsigned int oldstate)
 {
-	struct uart_8250_port *p = (struct uart_8250_port *)port;
+	struct uart_8250_port *p =
+		container_of(port, struct uart_8250_port, port);
 
 	serial8250_set_sleep(p, state != 0);
 }
@@ -2566,7 +2582,8 @@ static void serial8250_release_rsa_resource(struct uart_8250_port *up)
 
 static void serial8250_release_port(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	serial8250_release_std_resource(up);
 	if (up->port.type == PORT_RSA)
@@ -2575,7 +2592,8 @@ static void serial8250_release_port(struct uart_port *port)
 
 static int serial8250_request_port(struct uart_port *port)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	int ret = 0;
 
 	ret = serial8250_request_std_resource(up);
@@ -2590,7 +2608,8 @@ static int serial8250_request_port(struct uart_port *port)
 
 static void serial8250_config_port(struct uart_port *port, int flags)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 	int probeflags = PROBE_ANY;
 	int ret;
 
@@ -2771,7 +2790,8 @@ serial8250_register_ports(struct uart_driver *drv, struct device *dev)
 
 static void serial8250_console_putchar(struct uart_port *port, int ch)
 {
-	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
 
 	wait_for_xmitr(up, UART_LSR_THRE);
 	serial_out(up, UART_TX, ch);
-- 
1.7.2.3

[PATCH 2/2] 8250: add a UPIO_DWAPB32 for 32 bit accesses

[Jamie Iles]

Some platforms contain a Synopsys DesignWare APB UART that is attached
to a 32-bit APB bus where sub-word accesses are not allowed. Add a new
IO type (UPIO_DWAPB32) that performs 32 bit acccesses to the UART.

v2:
	- don't test for 32 bit in the output fast path, provide a
	  separate dwabp32_serial_out() function. Refactor
	  dwabp_serial_out() so that we can reuse the LCR saving
	  code.
v3:
	- rebased on top of "8250: use container_of() instead of
	  casting"

Cc: Greg KH <greg@kroah.com>
Signed-off-by: Jamie Iles <jamie@jamieiles.com>
---
 drivers/serial/8250.c        |   51 ++++++++++++++++++++++++++++++++----------
 drivers/serial/serial_core.c |    2 +
 include/linux/serial_core.h  |    1 +
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 0b4ce47..9839199 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -454,22 +454,40 @@ static void tsi_serial_out(struct uart_port *p, int offset, int value)
 		writeb(value, p->membase + offset);
 }
 
+/* Save the LCR value so it can be re-written when a Busy Detect IRQ occurs. */
+static inline void dwapb_save_out_value(struct uart_port *p, int offset,
+					int value)
+{
+	struct uart_8250_port *up =
+		container_of(p, struct uart_8250_port, port);
+
+	if (offset == UART_LCR)
+		up->lcr = value;
+}
+
+/* Read the IER to ensure any interrupt is cleared before returning from ISR. */
+static inline void dwapb_check_clear_ier(struct uart_port *p, int offset)
+{
+	if (offset == UART_TX || offset == UART_IER)
+		p->serial_in(p, UART_IER);
+}
+
 static void dwapb_serial_out(struct uart_port *p, int offset, int value)
 {
 	int save_offset = offset;
 	offset = map_8250_out_reg(p, offset) << p->regshift;
-	/* Save the LCR value so it can be re-written when a
-	 * Busy Detect interrupt occurs. */
-	if (save_offset == UART_LCR) {
-		struct uart_8250_port *up =
-			container_of(p, struct uart_8250_port, port);
-		up->lcr = value;
-	}
+	dwapb_save_out_value(p, save_offset, value);
 	writeb(value, p->membase + offset);
-	/* Read the IER to ensure any interrupt is cleared before
-	 * returning from ISR. */
-	if (save_offset == UART_TX || save_offset == UART_IER)
-		value = p->serial_in(p, UART_IER);
+	dwapb_check_clear_ier(p, save_offset);
+}
+
+static void dwapb32_serial_out(struct uart_port *p, int offset, int value)
+{
+	int save_offset = offset;
+	offset = map_8250_out_reg(p, offset) << p->regshift;
+	dwapb_save_out_value(p, save_offset, value);
+	writel(value, p->membase + offset);
+	dwapb_check_clear_ier(p, save_offset);
 }
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)
@@ -520,6 +538,11 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_out = dwapb_serial_out;
 		break;
 
+	case UPIO_DWAPB32:
+		p->serial_in = mem32_serial_in;
+		p->serial_out = dwapb32_serial_out;
+		break;
+
 	default:
 		p->serial_in = io_serial_in;
 		p->serial_out = io_serial_out;
@@ -538,6 +561,7 @@ serial_out_sync(struct uart_8250_port *up, int offset, int value)
 	case UPIO_MEM32:
 	case UPIO_AU:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		p->serial_out(p, offset, value);
 		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
 		break;
@@ -1587,7 +1611,8 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
 			handled = 1;
 
 			end = NULL;
-		} else if (up->port.iotype == UPIO_DWAPB &&
+		} else if ((up->port.iotype == UPIO_DWAPB ||
+			    up->port.iotype == UPIO_DWAPB32) &&
 			  (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
 			/* The DesignWare APB UART has an Busy Detect (0x07)
 			 * interrupt meaning an LCR write attempt occured while the
@@ -2492,6 +2517,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
@@ -2529,6 +2555,7 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 9ffa5be..143103b 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2134,6 +2134,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		snprintf(address, sizeof(address),
 			 "MMIO 0x%llx", (unsigned long long)port->mapbase);
 		break;
@@ -2554,6 +2555,7 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		return (port1->mapbase == port2->mapbase);
 	}
 	return 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 212eb4c..8681e7c 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -311,6 +311,7 @@ struct uart_port {
 #define UPIO_TSI		(5)			/* Tsi108/109 type IO */
 #define UPIO_DWAPB		(6)			/* DesignWare APB UART */
 #define UPIO_RM9000		(7)			/* RM9000 type IO */
+#define UPIO_DWAPB32		(8)			/* DesignWare APB UART (32 bit accesses) */
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
-- 
1.7.2.3

[PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses

[Jamie Iles]

Some platforms contain a Synopsys DesignWare APB UART that is attached
to a 32-bit APB bus where sub-word accesses are not allowed. Add a new
IO type (UPIO_DWAPB32) that performs 32 bit acccesses to the UART.

Signed-off-by: Jamie Iles <jamie.iles@picochip.com>
---
 drivers/serial/8250.c        |   18 ++++++++++++++++--
 drivers/serial/8250_early.c  |    6 +++++-
 drivers/serial/serial_core.c |    2 ++
 include/linux/serial_core.h  |    1 +
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index c87f7bd..b03a8c8 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -463,7 +463,12 @@ static void dwapb_serial_out(struct uart_port *p, int offset, int value)
 		struct uart_8250_port *up = (struct uart_8250_port *)p;
 		up->lcr = value;
 	}
-	writeb(value, p->membase + offset);
+
+	if (UPIO_DWAPB == p->iotype)
+		writeb(value, p->membase + offset);
+	else
+		writel(value, p->membase + offset);
+
 	/* Read the IER to ensure any interrupt is cleared before
 	 * returning from ISR. */
 	if (save_offset == UART_TX || save_offset == UART_IER)
@@ -518,6 +523,11 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_out = dwapb_serial_out;
 		break;
 
+	case UPIO_DWAPB32:
+		p->serial_in = mem32_serial_in;
+		p->serial_out = dwapb_serial_out;
+		break;
+
 	default:
 		p->serial_in = io_serial_in;
 		p->serial_out = io_serial_out;
@@ -538,6 +548,7 @@ serial_out_sync(struct uart_8250_port *up, int offset, int value)
 	case UPIO_AU:
 #endif
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		p->serial_out(p, offset, value);
 		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
 		break;
@@ -1574,7 +1585,8 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
 			handled = 1;
 
 			end = NULL;
-		} else if (up->port.iotype == UPIO_DWAPB &&
+		} else if ((up->port.iotype == UPIO_DWAPB ||
+			    up->port.iotype == UPIO_DWAPB32) &&
 			  (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
 			/* The DesignWare APB UART has an Busy Detect (0x07)
 			 * interrupt meaning an LCR write attempt occured while the
@@ -2444,6 +2456,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
@@ -2481,6 +2494,7 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
diff --git a/drivers/serial/8250_early.c b/drivers/serial/8250_early.c
index f279745..51a3497 100644
--- a/drivers/serial/8250_early.c
+++ b/drivers/serial/8250_early.c
@@ -48,7 +48,9 @@ static struct early_serial8250_device early_device;
 
 static unsigned int __init serial_in(struct uart_port *port, int offset)
 {
-	if (port->iotype == UPIO_MEM)
+        if (port->iotype == UPIO_MEM32)
+                return readl(port->membase + offset);
+        else if (port->iotype == UPIO_MEM)
 		return readb(port->membase + offset);
 	else
 		return inb(port->iobase + offset);
@@ -56,6 +58,8 @@ static unsigned int __init serial_in(struct uart_port *port, int offset)
 
 static void __init serial_out(struct uart_port *port, int offset, int value)
 {
+        if (port->iotype == UPIO_MEM32)
+                writel(value, port->membase + offset);
 	if (port->iotype == UPIO_MEM)
 		writeb(value, port->membase + offset);
 	else
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index fa4f170..71ce227 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2142,6 +2142,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		snprintf(address, sizeof(address),
 			 "MMIO 0x%llx", (unsigned long long)port->mapbase);
 		break;
@@ -2555,6 +2556,7 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		return (port1->mapbase == port2->mapbase);
 	}
 	return 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index d40db83..6eea6cf 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -289,6 +289,7 @@ struct uart_port {
 #define UPIO_TSI		(5)			/* Tsi108/109 type IO */
 #define UPIO_DWAPB		(6)			/* DesignWare APB UART */
 #define UPIO_RM9000		(7)			/* RM9000 type IO */
+#define UPIO_DWAPB32		(8)			/* DesignWare APB UART (32 bit accesses) */
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
-- 
1.6.5.4

Resend of [PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses

[Jamie Iles]

The original patch was whitespace broken for 8250_early.c. The following
patch addresses the brokenness.

Jamie

[PATCH] 8250: add a UPIO_DWAPB32 for 32 bit accesses (v2)

[Jamie Iles]

Some platforms contain a Synopsys DesignWare APB UART that is attached
to a 32-bit APB bus where sub-word accesses are not allowed. Add a new
IO type (UPIO_DWAPB32) that performs 32 bit acccesses to the UART.

Signed-off-by: Jamie Iles <jamie.iles@picochip.com>
---
 drivers/serial/8250.c        |   18 ++++++++++++++++--
 drivers/serial/8250_early.c  |    6 +++++-
 drivers/serial/serial_core.c |    2 ++
 include/linux/serial_core.h  |    1 +
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index c87f7bd..b03a8c8 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -463,7 +463,12 @@ static void dwapb_serial_out(struct uart_port *p, int offset, int value)
 		struct uart_8250_port *up = (struct uart_8250_port *)p;
 		up->lcr = value;
 	}
-	writeb(value, p->membase + offset);
+
+	if (UPIO_DWAPB == p->iotype)
+		writeb(value, p->membase + offset);
+	else
+		writel(value, p->membase + offset);
+
 	/* Read the IER to ensure any interrupt is cleared before
 	 * returning from ISR. */
 	if (save_offset == UART_TX || save_offset == UART_IER)
@@ -518,6 +523,11 @@ static void set_io_from_upio(struct uart_port *p)
 		p->serial_out = dwapb_serial_out;
 		break;
 
+	case UPIO_DWAPB32:
+		p->serial_in = mem32_serial_in;
+		p->serial_out = dwapb_serial_out;
+		break;
+
 	default:
 		p->serial_in = io_serial_in;
 		p->serial_out = io_serial_out;
@@ -538,6 +548,7 @@ serial_out_sync(struct uart_8250_port *up, int offset, int value)
 	case UPIO_AU:
 #endif
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		p->serial_out(p, offset, value);
 		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
 		break;
@@ -1574,7 +1585,8 @@ static irqreturn_t serial8250_interrupt(int irq, void *dev_id)
 			handled = 1;
 
 			end = NULL;
-		} else if (up->port.iotype == UPIO_DWAPB &&
+		} else if ((up->port.iotype == UPIO_DWAPB ||
+			    up->port.iotype == UPIO_DWAPB32) &&
 			  (iir & UART_IIR_BUSY) == UART_IIR_BUSY) {
 			/* The DesignWare APB UART has an Busy Detect (0x07)
 			 * interrupt meaning an LCR write attempt occured while the
@@ -2444,6 +2456,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
@@ -2481,6 +2494,7 @@ static void serial8250_release_std_resource(struct uart_8250_port *up)
 	case UPIO_MEM32:
 	case UPIO_MEM:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		if (!up->port.mapbase)
 			break;
 
diff --git a/drivers/serial/8250_early.c b/drivers/serial/8250_early.c
index f279745..e142463 100644
--- a/drivers/serial/8250_early.c
+++ b/drivers/serial/8250_early.c
@@ -48,7 +48,9 @@ static struct early_serial8250_device early_device;
 
 static unsigned int __init serial_in(struct uart_port *port, int offset)
 {
-	if (port->iotype == UPIO_MEM)
+	if (port->iotype == UPIO_MEM32)
+		return readl(port->membase + offset);
+	else if (port->iotype == UPIO_MEM)
 		return readb(port->membase + offset);
 	else
 		return inb(port->iobase + offset);
@@ -56,6 +58,8 @@ static unsigned int __init serial_in(struct uart_port *port, int offset)
 
 static void __init serial_out(struct uart_port *port, int offset, int value)
 {
+	if (port->iotype == UPIO_MEM32)
+		writel(value, port->membase + offset);
 	if (port->iotype == UPIO_MEM)
 		writeb(value, port->membase + offset);
 	else
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index fa4f170..71ce227 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2142,6 +2142,7 @@ uart_report_port(struct uart_driver *drv, struct uart_port *port)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		snprintf(address, sizeof(address),
 			 "MMIO 0x%llx", (unsigned long long)port->mapbase);
 		break;
@@ -2555,6 +2556,7 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+	case UPIO_DWAPB32:
 		return (port1->mapbase == port2->mapbase);
 	}
 	return 0;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index d40db83..6eea6cf 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -289,6 +289,7 @@ struct uart_port {
 #define UPIO_TSI		(5)			/* Tsi108/109 type IO */
 #define UPIO_DWAPB		(6)			/* DesignWare APB UART */
 #define UPIO_RM9000		(7)			/* RM9000 type IO */
+#define UPIO_DWAPB32		(8)			/* DesignWare APB UART (32 bit accesses) */
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
-- 
1.6.5.4