[PATCH 0/4] *** 8250_dw ***

[PATCH 0/4] *** 8250_dw ***

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Coming to use 8250_dw driver with my serial device I had few problems
I solved with following patch set.

First and fourth patches are aimed to support BIG endian device.
Second patch is is aimed to support device without test loop support.
Third patch is aimed to minimize chance for getting interrupt before
we called request_irq() during initialize probing.


Noam Camus (4):
  serial: 8250_dw: Add support for big-endian MMIO accesses
  serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree
  serial: 8250_dw: Add UPF_FIXED_TYPE to flags
  serial: 8250_dw: use serial_in() and not readl()

 .../bindings/serial/snps-dw-apb-uart.txt           |    2 +
 drivers/tty/serial/8250/8250_dw.c                  |   61 +++++++++++++++++---
 2 files changed, 54 insertions(+), 9 deletions(-)

[PATCH 1/4] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |   42 ++++++++++++++++++++++++++++++------
 1 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index d48b506..fe0b487 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -173,15 +173,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_64BIT */
 
-static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+static void dw8250_check_control(struct uart_port *p, int offset, int value)
 {
 	struct dw8250_data *d = p->private_data;
 
 	if (offset == UART_MCR)
 		d->last_mcr = value;
 
-	writel(value, p->membase + (offset << p->regshift));
-
 	/* Make sure LCR write wasn't ignored */
 	if (offset == UART_LCR) {
 		int tries = 1000;
@@ -190,7 +188,12 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
-			writel(value, p->membase + (UART_LCR << p->regshift));
+			if (p->iotype == UPIO_MEM32BE)
+				iowrite32be(value,
+					p->membase + (UART_LCR << p->regshift));
+			else
+				writel(value,
+					p->membase + (UART_LCR << p->regshift));
 		}
 		/*
 		 * FIXME: this deadlocks if port->lock is already held
@@ -199,6 +202,12 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 	}
 }
 
+static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+{
+	writel(value, p->membase + (offset << p->regshift));
+	dw8250_check_control(p, offset, value);
+}
+
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 {
 	unsigned int value = readl(p->membase + (offset << p->regshift));
@@ -206,6 +215,19 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 	return dw8250_modify_msr(p, offset, value);
 }
 
+static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
+{
+	iowrite32be(value, p->membase + (offset << p->regshift));
+	dw8250_check_control(p, offset, value);
+}
+
+static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
+{
+	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
+
+	return dw8250_modify_msr(p, offset, value);
+}
+
 static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct dw8250_data *d = p->private_data;
@@ -322,9 +344,15 @@ static int dw8250_probe_of(struct uart_port *p,
 		case 1:
 			break;
 		case 4:
-			p->iotype = UPIO_MEM32;
-			p->serial_in = dw8250_serial_in32;
-			p->serial_out = dw8250_serial_out32;
+			p->iotype = of_device_is_big_endian(np) ?
+				    UPIO_MEM32BE : UPIO_MEM32;
+			if (p->iotype == UPIO_MEM32) {
+				p->serial_in = dw8250_serial_in32;
+				p->serial_out = dw8250_serial_out32;
+			} else {
+				p->serial_in = dw8250_serial_in32be;
+				p->serial_out = dw8250_serial_out32be;
+			}
 			break;
 		default:
 			dev_err(p->dev, "unsupported reg-io-width (%u)\n", val);
-- 
1.7.1

[PATCH 2/4] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for OF option "no-loopback-test"

use case: simulator which does not implements loopback test mode.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 .../bindings/serial/snps-dw-apb-uart.txt           |    2 ++
 drivers/tty/serial/8250/8250_dw.c                  |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
index 289c40e..5d16047 100644
--- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
+++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
@@ -33,6 +33,8 @@ Optional properties:
 - ri-override : Override the RI modem status signal. This signal will always be
   reported as inactive instead of being obtained from the modem status register.
   Define this if your serial port does not use this pin.
+- no-loopback-test: set to indicate that the port does not implements loopback
+  test mode
 
 Example:
 
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index fe0b487..1a57105 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -370,6 +370,9 @@ static int dw8250_probe_of(struct uart_port *p,
 		up->dma->txconf.dst_maxburst = p->fifosize / 4;
 	}
 
+	if (of_find_property(np, "no-loopback-test", NULL))
+		p->flags |= UPF_SKIP_TEST;
+
 	if (!of_property_read_u32(np, "reg-shift", &val))
 		p->regshift = val;
 
-- 
1.7.1

[PATCH 3/4] serial: 8250_dw: Add UPF_FIXED_TYPE to flags

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Always add UPF_FIXED_TYPE to flags so autoconf() will be skipped.
We do that since autoconf() performs many writes to LCR that cause
BUSY interrupt.
The problem with such interrupt is that driver is not yet called to
request_irq() and generic IRQ subsystem will mask the UART line.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1a57105..620f983 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -362,6 +362,14 @@ static int dw8250_probe_of(struct uart_port *p,
 	if (has_ucv)
 		dw8250_setup_port(up);
 
+	/* Writing to LCR may cause BUSY interrupt before we
+	 * register the IRQ line.
+	 * Currently autoconf() uses several writes to LCR.
+	 * In order to avoid calling to autoconf() always add
+	 * following flag.
+	 */
+	p->flags |= UPF_FIXED_TYPE;
+
 	/* if we have a valid fifosize, try hooking up DMA here */
 	if (p->fifosize) {
 		up->dma = &data->dma;
-- 
1.7.1

[PATCH 4/4] serial: 8250_dw: use serial_in() and not readl()

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

This is now matches device endianness.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 620f983..a64197c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -291,7 +291,9 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
 static void dw8250_setup_port(struct uart_8250_port *up)
 {
 	struct uart_port	*p = &up->port;
-	u32			reg = readl(p->membase + DW_UART_UCV);
+	u32			reg = (p->iotype == UPIO_MEM32BE) ?
+					ioread32be(p->membase + DW_UART_UCV) :
+					readl(p->membase + DW_UART_UCV);
 
 	/*
 	 * If the Component Version Register returns zero, we know that
@@ -303,7 +305,9 @@ static void dw8250_setup_port(struct uart_8250_port *up)
 	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
-	reg = readl(p->membase + DW_UART_CPR);
+	reg = (p->iotype == UPIO_MEM32BE) ?
+		ioread32be(p->membase + DW_UART_CPR) :
+		readl(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
-- 
1.7.1

Re: [PATCH 4/4] serial: 8250_dw: use serial_in() and not readl()

[Peter Hurley]

Hi Noam,

On 07/22/2015 05:34 AM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> This is now matches device endianness.

I'm not seeing where serial_in() is used here, as claimed by the $subject.

Regards,
Peter Hurley

> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 620f983..a64197c 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -291,7 +291,9 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
>  static void dw8250_setup_port(struct uart_8250_port *up)
>  {
>  	struct uart_port	*p = &up->port;
> -	u32			reg = readl(p->membase + DW_UART_UCV);
> +	u32			reg = (p->iotype == UPIO_MEM32BE) ?
> +					ioread32be(p->membase + DW_UART_UCV) :
> +					readl(p->membase + DW_UART_UCV);
>  
>  	/*
>  	 * If the Component Version Register returns zero, we know that
> @@ -303,7 +305,9 @@ static void dw8250_setup_port(struct uart_8250_port *up)
>  	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>  
> -	reg = readl(p->membase + DW_UART_CPR);
> +	reg = (p->iotype == UPIO_MEM32BE) ?
> +		ioread32be(p->membase + DW_UART_CPR) :
> +		readl(p->membase + DW_UART_CPR);
>  	if (!reg)
>  		return;
>  
> 

RE: [PATCH 4/4] serial: 8250_dw: use serial_in() and not readl()

[Noam Camus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 532 bytes --]

From: Peter Hurley [mailto:peter@hurleysoftware.com] 
Sent: Wednesday, July 22, 2015 3:52 PM

> Subject: Re: [PATCH 4/4] serial: 8250_dw: use serial_in() and not readl()

> From: Noam Camus <noamc@ezchip.com>
> 
> This is now matches device endianness.

> I'm not seeing where serial_in() is used here, as claimed by the $subject.

Thanks, I will "reword" my commit in v2
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 3/4] serial: 8250_dw: Add UPF_FIXED_TYPE to flags

[Peter Hurley]

Hi Noam,

On 07/22/2015 05:34 AM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Always add UPF_FIXED_TYPE to flags so autoconf() will be skipped.
> We do that since autoconf() performs many writes to LCR that cause
> BUSY interrupt.
> The problem with such interrupt is that driver is not yet called to
> request_irq() and generic IRQ subsystem will mask the UART line.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 1a57105..620f983 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -362,6 +362,14 @@ static int dw8250_probe_of(struct uart_port *p,
>  	if (has_ucv)
>  		dw8250_setup_port(up);
>  
> +	/* Writing to LCR may cause BUSY interrupt before we
> +	 * register the IRQ line.
> +	 * Currently autoconf() uses several writes to LCR.
> +	 * In order to avoid calling to autoconf() always add
> +	 * following flag.
> +	 */
> +	p->flags |= UPF_FIXED_TYPE;

Why only for devicetree DW8250's?  Don't all DW8250's have this LCR "feature"?

And what port type does this id DW8250's as, PORT_8250? Except with fifos,
autoflow control, dma, etc.?

If the port type is being fixed, then please fix it to a new port type.

> +
>  	/* if we have a valid fifosize, try hooking up DMA here */
>  	if (p->fifosize) {
>  		up->dma = &data->dma;
> 

RE: [PATCH 3/4] serial: 8250_dw: Add UPF_FIXED_TYPE to flags

[Noam Camus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1295 bytes --]

From: Peter Hurley [mailto:peter@hurleysoftware.com] 
Sent: Wednesday, July 22, 2015 3:39 PM

>> +	/* Writing to LCR may cause BUSY interrupt before we
>> +	 * register the IRQ line.
>> +	 * Currently autoconf() uses several writes to LCR.
>> +	 * In order to avoid calling to autoconf() always add
>> +	 * following flag.
>> +	 */
>> +	p->flags |= UPF_FIXED_TYPE; 
>
> Why only for devicetree DW8250's?  Don't all DW8250's have this LCR "feature"?
>
Yes, all DW8250 got this "feature" i.e. busy functionality.

> And what port type does this id DW8250's as, PORT_8250? Except with fifos, autoflow control, dma, etc.?
The only thing that DW8250 is not fully 16550 compatibility is this busy functionality feature.
This feature means that a serial transfer is in progress.
It is indicated by special identification in IIR, and raised when LCR is written while device is busy.

> If the port type is being fixed, then please fix it to a new port type.
As an extension to 16550 type is not fixed.

Seem that I need to rethink on this.
I will remove this commit from v2 patch set till I will be sure what is the right thing to do.

Noam


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/4] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Peter Hurley]

Hi Noam,

On 07/22/2015 05:34 AM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for OF option "no-loopback-test"

Changes to devicetree need to at least get acks from DT maintainers.

Regards,
Peter Hurley

> use case: simulator which does not implements loopback test mode.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  .../bindings/serial/snps-dw-apb-uart.txt           |    2 ++
>  drivers/tty/serial/8250/8250_dw.c                  |    3 +++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> index 289c40e..5d16047 100644
> --- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> +++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> @@ -33,6 +33,8 @@ Optional properties:
>  - ri-override : Override the RI modem status signal. This signal will always be
>    reported as inactive instead of being obtained from the modem status register.
>    Define this if your serial port does not use this pin.
> +- no-loopback-test: set to indicate that the port does not implements loopback
> +  test mode
>  
>  Example:
>  
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index fe0b487..1a57105 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -370,6 +370,9 @@ static int dw8250_probe_of(struct uart_port *p,
>  		up->dma->txconf.dst_maxburst = p->fifosize / 4;
>  	}
>  
> +	if (of_find_property(np, "no-loopback-test", NULL))
> +		p->flags |= UPF_SKIP_TEST;
> +
>  	if (!of_property_read_u32(np, "reg-shift", &val))
>  		p->regshift = val;
>  
> 

RE: [PATCH 2/4] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Noam Camus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 582 bytes --]

From: Peter Hurley [mailto:peter@hurleysoftware.com] 
Sent: Wednesday, July 22, 2015 3:21 PM

>>On 07/22/2015 05:34 AM, Noam Camus wrote:
>> From: Noam Camus <noamc@ezchip.com>
>> 
>> Add support for OF option "no-loopback-test"

> Changes to devicetree need to at least get acks from DT maintainers.
I will add them in my v2
Should I take this patch apart from this set or should I add DT maintainers to whole set?

Noam
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 2/4] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Frans Klaver]

On Thu, Jul 23, 2015 at 8:21 AM, Noam Camus <noamc@ezchip.com> wrote:
> From: Peter Hurley [mailto:peter@hurleysoftware.com]
> Sent: Wednesday, July 22, 2015 3:21 PM
>
>>>On 07/22/2015 05:34 AM, Noam Camus wrote:
>>> From: Noam Camus <noamc@ezchip.com>
>>>
>>> Add support for OF option "no-loopback-test"
>
>> Changes to devicetree need to at least get acks from DT maintainers.
> I will add them in my v2
> Should I take this patch apart from this set or should I add DT maintainers to whole set?

Add them at least to your cover letter and this one specifically. It's
a small set, so it's fine to add them to all patches, so they can see
the bigger picture. No need to take this patch out of the set (and
context).

Frans

Re: [PATCH 1/4] serial: 8250_dw: Add support for big-endian MMIO accesses

[Peter Hurley]

Hi Noam,

On 07/22/2015 05:34 AM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for UPIO_MEM32BE in addition to UPIO_MEM32.

This is not an adequate changelog.
Please describe the refactoring.

Regards,
Peter Hurley

> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |   42 ++++++++++++++++++++++++++++++------
>  1 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index d48b506..fe0b487 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -173,15 +173,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
>  }
>  #endif /* CONFIG_64BIT */
>  
> -static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> +static void dw8250_check_control(struct uart_port *p, int offset, int value)
>  {
>  	struct dw8250_data *d = p->private_data;
>  
>  	if (offset == UART_MCR)
>  		d->last_mcr = value;
>  
> -	writel(value, p->membase + (offset << p->regshift));
> -
>  	/* Make sure LCR write wasn't ignored */
>  	if (offset == UART_LCR) {
>  		int tries = 1000;
> @@ -190,7 +188,12 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>  			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
>  				return;
>  			dw8250_force_idle(p);
> -			writel(value, p->membase + (UART_LCR << p->regshift));
> +			if (p->iotype == UPIO_MEM32BE)
> +				iowrite32be(value,
> +					p->membase + (UART_LCR << p->regshift));
> +			else
> +				writel(value,
> +					p->membase + (UART_LCR << p->regshift));
>  		}
>  		/*
>  		 * FIXME: this deadlocks if port->lock is already held
> @@ -199,6 +202,12 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>  	}
>  }
>  
> +static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> +{
> +	writel(value, p->membase + (offset << p->regshift));
> +	dw8250_check_control(p, offset, value);
> +}
> +
>  static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
>  {
>  	unsigned int value = readl(p->membase + (offset << p->regshift));
> @@ -206,6 +215,19 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
>  	return dw8250_modify_msr(p, offset, value);
>  }
>  
> +static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
> +{
> +	iowrite32be(value, p->membase + (offset << p->regshift));
> +	dw8250_check_control(p, offset, value);
> +}
> +
> +static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
> +{
> +	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
> +
> +	return dw8250_modify_msr(p, offset, value);
> +}
> +
>  static int dw8250_handle_irq(struct uart_port *p)
>  {
>  	struct dw8250_data *d = p->private_data;
> @@ -322,9 +344,15 @@ static int dw8250_probe_of(struct uart_port *p,
>  		case 1:
>  			break;
>  		case 4:
> -			p->iotype = UPIO_MEM32;
> -			p->serial_in = dw8250_serial_in32;
> -			p->serial_out = dw8250_serial_out32;
> +			p->iotype = of_device_is_big_endian(np) ?
> +				    UPIO_MEM32BE : UPIO_MEM32;
> +			if (p->iotype == UPIO_MEM32) {
> +				p->serial_in = dw8250_serial_in32;
> +				p->serial_out = dw8250_serial_out32;
> +			} else {
> +				p->serial_in = dw8250_serial_in32be;
> +				p->serial_out = dw8250_serial_out32be;
> +			}
>  			break;
>  		default:
>  			dev_err(p->dev, "unsupported reg-io-width (%u)\n", val);
> 

Re: [PATCH 1/4] serial: 8250_dw: Add support for big-endian MMIO accesses

[Peter Hurley]

On 07/22/2015 08:19 AM, Peter Hurley wrote:
> Hi Noam,
> 
> On 07/22/2015 05:34 AM, Noam Camus wrote:
>> From: Noam Camus <noamc@ezchip.com>
>>
>> Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
> 
> This is not an adequate changelog.
> Please describe the refactoring.
> 
> Regards,
> Peter Hurley
> 
>> Signed-off-by: Noam Camus <noamc@ezchip.com>
>> ---
>>  drivers/tty/serial/8250/8250_dw.c |   42 ++++++++++++++++++++++++++++++------
>>  1 files changed, 35 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>> index d48b506..fe0b487 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -173,15 +173,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
>>  }
>>  #endif /* CONFIG_64BIT */
>>  
>> -static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>> +static void dw8250_check_control(struct uart_port *p, int offset, int value)

Also, I think this fn name should be dw8250_check_LCR() to distinguish it
from modem control.

Regards,
Peter Hurley

>>  {
>>  	struct dw8250_data *d = p->private_data;
>>  
>>  	if (offset == UART_MCR)
>>  		d->last_mcr = value;
>>  
>> -	writel(value, p->membase + (offset << p->regshift));
>> -
>>  	/* Make sure LCR write wasn't ignored */
>>  	if (offset == UART_LCR) {
>>  		int tries = 1000;
>> @@ -190,7 +188,12 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>>  			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
>>  				return;
>>  			dw8250_force_idle(p);
>> -			writel(value, p->membase + (UART_LCR << p->regshift));
>> +			if (p->iotype == UPIO_MEM32BE)
>> +				iowrite32be(value,
>> +					p->membase + (UART_LCR << p->regshift));
>> +			else
>> +				writel(value,
>> +					p->membase + (UART_LCR << p->regshift));
>>  		}
>>  		/*
>>  		 * FIXME: this deadlocks if port->lock is already held
>> @@ -199,6 +202,12 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>>  	}
>>  }
>>  
>> +static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>> +{
>> +	writel(value, p->membase + (offset << p->regshift));
>> +	dw8250_check_control(p, offset, value);
>> +}
>> +
>>  static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
>>  {
>>  	unsigned int value = readl(p->membase + (offset << p->regshift));
>> @@ -206,6 +215,19 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
>>  	return dw8250_modify_msr(p, offset, value);
>>  }
>>  
>> +static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
>> +{
>> +	iowrite32be(value, p->membase + (offset << p->regshift));
>> +	dw8250_check_control(p, offset, value);
>> +}
>> +
>> +static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
>> +{
>> +	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
>> +
>> +	return dw8250_modify_msr(p, offset, value);
>> +}
>> +
>>  static int dw8250_handle_irq(struct uart_port *p)
>>  {
>>  	struct dw8250_data *d = p->private_data;
>> @@ -322,9 +344,15 @@ static int dw8250_probe_of(struct uart_port *p,
>>  		case 1:
>>  			break;
>>  		case 4:
>> -			p->iotype = UPIO_MEM32;
>> -			p->serial_in = dw8250_serial_in32;
>> -			p->serial_out = dw8250_serial_out32;
>> +			p->iotype = of_device_is_big_endian(np) ?
>> +				    UPIO_MEM32BE : UPIO_MEM32;
>> +			if (p->iotype == UPIO_MEM32) {
>> +				p->serial_in = dw8250_serial_in32;
>> +				p->serial_out = dw8250_serial_out32;
>> +			} else {
>> +				p->serial_in = dw8250_serial_in32be;
>> +				p->serial_out = dw8250_serial_out32be;
>> +			}
>>  			break;
>>  		default:
>>  			dev_err(p->dev, "unsupported reg-io-width (%u)\n", val);
>>
> 

RE: [PATCH 1/4] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 902 bytes --]

From: Peter Hurley [mailto:peter@hurleysoftware.com] 
Sent: Wednesday, July 22, 2015 3:41 PM

>> diff --git a/drivers/tty/serial/8250/8250_dw.c 
>> b/drivers/tty/serial/8250/8250_dw.c
>> index d48b506..fe0b487 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -173,15 +173,13 @@ static void dw8250_serial_outq(struct uart_port 
>> *p, int offset, int value)  }  #endif /* CONFIG_64BIT */
>>  
>> -static void dw8250_serial_out32(struct uart_port *p, int offset, int 
>> value)
>> +static void dw8250_check_control(struct uart_port *p, int offset, 
>> +int value)

> Also, I think this fn name should be dw8250_check_LCR() to distinguish it from modem control.

No problem will rename in my v2
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 1/4] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 359 bytes --]

From: Peter Hurley [mailto:peter@hurleysoftware.com] 
Sent: Wednesday, July 22, 2015 3:19 PM

> This is not an adequate changelog.
> Please describe the refactoring.

I will in my v2 patch set

Noam
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [v2 2/3] serial: 8250_dw: dw8250_setup_port() use endianness aware read.

[Greg KH]

On Sun, Jul 26, 2015 at 07:54:37AM +0300, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
> Instead we use ioread32be for uart port of type UPIO_MEM32BE.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 5c60ec8..9bc4cac 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -291,7 +291,9 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
>  static void dw8250_setup_port(struct uart_8250_port *up)
>  {
>  	struct uart_port	*p = &up->port;
> -	u32			reg = readl(p->membase + DW_UART_UCV);
> +	u32			reg = (p->iotype == UPIO_MEM32BE) ?
> +					ioread32be(p->membase + DW_UART_UCV) :
> +					readl(p->membase + DW_UART_UCV);

Ugh.  Just use a "real" if statement for this please.  Don't abuse ? :
lines for no reason.


>  
>  	/*
>  	 * If the Component Version Register returns zero, we know that
> @@ -303,7 +305,9 @@ static void dw8250_setup_port(struct uart_8250_port *up)
>  	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>  
> -	reg = readl(p->membase + DW_UART_CPR);
> +	reg = (p->iotype == UPIO_MEM32BE) ?
> +		ioread32be(p->membase + DW_UART_CPR) :
> +		readl(p->membase + DW_UART_CPR);

Same here.

And shouldn't all of this be "hidden" behind something else?  You should
not have to do this for each readl call...

thanks,

greg k-h

Re: [v2 2/3] serial: 8250_dw: dw8250_setup_port() use endianness aware read.

[Noam Camus]

From: Greg KH <greg@kroah.com>
Sent: Tuesday, August 4, 2015 2:42 AM

> > -     reg = readl(p->membase + DW_UART_CPR);
> > +     reg = (p->iotype == UPIO_MEM32BE) ?
> > +             ioread32be(p->membase + DW_UART_CPR) :
> > +             readl(p->membase + DW_UART_CPR);
>
> Same here.
>
> And shouldn't all of this be "hidden" behind something else?  You should
not have to do this for each readl call...
>

As I wrote for last patch, I will add another level for accessors and use it here.

Noam

Re: [v2 3/3] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Greg KH]

On Sun, Jul 26, 2015 at 07:54:38AM +0300, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for OF option "no-loopback-test"
> 
> use case: simulator which does not implements loopback test mode.

Can't you just fix the simulator to be "correct"?

Re: [v2 3/3] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Noam Camus]

From: Greg KH <greg@kroah.com>
Sent: Tuesday, August 4, 2015 2:44 AM

> > From: Noam Camus <noamc@ezchip.com>
> >
> > Add support for OF option "no-loopback-test"
> >
> > use case: simulator which does not implements loopback test mode.
>
> Can't you just fix the simulator to be "correct"?

I just got this simulator pre-built and as kernel developer I borrowed from generic 8250 this idea for OF option. seem to me that loopback test is optional since kernel for generic 8250 already support  "no-loopback-test" option.

Noam

Re: [v2 1/3] serial: 8250_dw: Add support for big-endian MMIO accesses

[Greg KH]

On Sun, Jul 26, 2015 at 07:54:36AM +0300, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
> dw8250_serial_out32() main functionality was moved to new
> function called dw8250_check_LCR().
> 
> We use new 2 accessors similar to little endian, called
> dw8250_serial_out32be() and dw8250_serial_in32be().
> 
> Both little and big endian accessors use dw8250_check_LCR()
> for their dw8250_serial_out32{,be}().
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |   42 ++++++++++++++++++++++++++++++------
>  1 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index d48b506..5c60ec8 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -173,15 +173,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
>  }
>  #endif /* CONFIG_64BIT */
>  
> -static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> +static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
>  {
>  	struct dw8250_data *d = p->private_data;
>  
>  	if (offset == UART_MCR)
>  		d->last_mcr = value;
>  
> -	writel(value, p->membase + (offset << p->regshift));

Why drop this write?

> -
>  	/* Make sure LCR write wasn't ignored */
>  	if (offset == UART_LCR) {
>  		int tries = 1000;
> @@ -190,7 +188,12 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>  			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
>  				return;
>  			dw8250_force_idle(p);
> -			writel(value, p->membase + (UART_LCR << p->regshift));
> +			if (p->iotype == UPIO_MEM32BE)
> +				iowrite32be(value,
> +					p->membase + (UART_LCR << p->regshift));
> +			else
> +				writel(value,
> +					p->membase + (UART_LCR << p->regshift));

Shouldn't this be hidden behind some other type of accessor?  Why is
this one writel() "special"?

thanks,

greg k-h

RE: [v2 1/3] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Greg KH [mailto:greg@kroah.com] 
Sent: Tuesday, August 04, 2015 2:43 AM

> > -	writel(value, p->membase + (offset << p->regshift));

> Why drop this write?
This was not dropped, it is now part of dw8250_serial_out32().
Now it is called before updating last_mcr.

> > -			writel(value, p->membase + (UART_LCR << p->regshift));
> > +			if (p->iotype == UPIO_MEM32BE)
> > +				iowrite32be(value,
> > +					p->membase + (UART_LCR << p->regshift));
> > +			else
> > +				writel(value,
> > +					p->membase + (UART_LCR << p->regshift));

> Shouldn't this be hidden behind some other type of accessor?  Why is this one writel() "special"?

I will add inner level accessors into "struct dw8250_data" for in32/out32. new accessors will be used in few places in this driver that still uses writel/readl without considering iotype.

Noam

Re: [v3 3/3] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Peter Hurley]

Hi Noam,

On 08/12/2015 07:18 AM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for OF option "no-loopback-test"
> 
> use case: simulator which does not implements loopback test mode.

I think Greg's question about the simulator still applies: why upstream this?
The simulator is not even identified so how is someone supposed to know
this workaround applies?

The fact there are no in-tree DT users of this workaround argues against
its acceptance.

Regards,
Peter Hurley


> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  .../bindings/serial/snps-dw-apb-uart.txt           |    2 ++
>  drivers/tty/serial/8250/8250_dw.c                  |    3 +++
>  2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> index 289c40e..5d16047 100644
> --- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> +++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.txt
> @@ -33,6 +33,8 @@ Optional properties:
>  - ri-override : Override the RI modem status signal. This signal will always be
>    reported as inactive instead of being obtained from the modem status register.
>    Define this if your serial port does not use this pin.
> +- no-loopback-test: set to indicate that the port does not implements loopback
> +  test mode
>  
>  Example:
>  
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 62f766a..0f397ae 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -394,6 +394,9 @@ static int dw8250_probe_of(struct uart_port *p,
>  		up->dma->txconf.dst_maxburst = p->fifosize / 4;
>  	}
>  
> +	if (of_find_property(np, "no-loopback-test", NULL))
> +		p->flags |= UPF_SKIP_TEST;
> +
>  	if (!of_property_read_u32(np, "reg-shift", &val))
>  		p->regshift = val;
>  
> 

RE: [v3 3/3] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Noam Camus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1161 bytes --]

> From: Peter Hurley [mailto:peter@hurleysoftware.com] 
> Sent: Wednesday, August 12, 2015 4:17 PM

> I think Greg's question about the simulator still applies: why upstream this?
> The simulator is not even identified so how is someone supposed to know this workaround applies?

> The fact there are no in-tree DT users of this workaround argues against its acceptance.

I am using UART peripheral for Synopsys simulator same as one used by arch/arc/plat-sim
I know this platform do not use CONFIG_SERIAL_8250_DW due to some problem I suspect it is relate to the loop test.

Maybe Vineet Gupta or Alexey Brodkin from Synopsys which are CC here can comment.

So It just happen for me to be a pioneer with this.

More than that "no-loopback-test" is an option already exist for core 8250, and since DW is only an extension for this driver it should also benefit from this option.

If all this is yet not enough, should I re-send this "patch set" again without this specific patch?

Regards,
Noam Camus
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [v3 3/3] serial: 8250_dw: Add UPF_SKIP_TEST to flags depend on device tree

[Vineet Gupta]

Hi Peter, Greg

On Wednesday 12 August 2015 09:21 PM, Noam Camus wrote:
>> From: Peter Hurley [mailto:peter@hurleysoftware.com] 
>> Sent: Wednesday, August 12, 2015 4:17 PM
>> I think Greg's question about the simulator still applies: why upstream this?
>> The simulator is not even identified so how is someone supposed to know this workaround applies?
>> The fact there are no in-tree DT users of this workaround argues against its acceptance.
> I am using UART peripheral for Synopsys simulator same as one used by arch/arc/plat-sim

The osci virtual platform uses nSIM with SystemC based peripheral models. The
issue is in that model and not in nsim per-se.

> I know this platform do not use CONFIG_SERIAL_8250_DW due to some problem I suspect it is relate to the loop test.

Indeed. If you look at git log of osci platform, there was a commit which switched
DT from dw uart driver to stock 8250. Unfortunately the changelog doesn't describe
in detail what the root cause was.

2013-05-16 6eda477b3c54 ARC: [nsimosci] Change .dts to use generic 8250 UART 


> Maybe Vineet Gupta or Alexey Brodkin from Synopsys which are CC here can comment.
>
> So It just happen for me to be a pioneer with this.
>
> More than that "no-loopback-test" is an option already exist for core 8250, and since DW is only an extension for this driver it should also benefit from this option.

It seems Noam has made some changes to model today and we might need this patch
after all. Noam ?

> If all this is yet not enough, should I re-send this "patch set" again without this specific patch?
>
> Regards,
> Noam Camus
>

[v4 0/2] *** 8250_dw ***

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

v4 change
Remove patch for skipping looptest through DT option.
This is now handled in our simulator model.
Thanks to Vineet Gupta from Synopsys for his help.

We are left with 2 patches which adds BIG endian support.

V3 change:
Use second level accessors for big/little endian port.
The new accessors are now pointed from uart_port->private_data
These accessors are initialized during driver probe().
Driver shouldn't access directly to readl/writel but to
these new second level accessors (first level is at uart_port).
e.g. at dw8250_check_LCR() and dw8250_setup_port() I replaced such
direct calls.

V2 changes:
1) better description for each commit.
2) adding to CC list the device tree maintainer.
3) rename dw8250_check_control() --> dw8250_check_LCR().
4) remove bad patch of "add UPF_FIXED_TYPE to flags".

Noam Camus (2):
  serial: 8250_dw: Add support for big-endian MMIO accesses
  serial: 8250_dw: dw8250_setup_port() use endianness aware read.

 drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++-----
 1 files changed, 63 insertions(+), 9 deletions(-)

[v4 1/2] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
dw8250_serial_out32() extra functionality that is not part of
the 8250 core driver was moved to new function called
dw8250_check_LCR().

For big endian we use 2 new accessors similar to little endian,
called dw8250_serial_out32be() and dw8250_serial_in32be().
Both little and big endian accessors use dw8250_check_LCR()
for their dw8250_serial_out32{,be}().

In addition I added another 2 accessors inside private_data field of
uart_port. This second level accessors are saved during probe in
private_data field of uart_port. Now any direct call for readl/writel
is replaced with those accessors which are aware of endianness.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |   67 +++++++++++++++++++++++++++++++++----
 1 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index d48b506..f479433 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -64,6 +64,9 @@ struct dw8250_data {
 	struct clk		*pclk;
 	struct reset_control	*rst;
 	struct uart_8250_dma	dma;
+	unsigned int		(*serial_in)(const void __iomem *addr);
+	void			(*serial_out)(unsigned int value,
+					      void __iomem *addr);
 };
 
 #define BYT_PRV_CLK			0x800
@@ -173,15 +176,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_64BIT */
 
-static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
 {
 	struct dw8250_data *d = p->private_data;
 
 	if (offset == UART_MCR)
 		d->last_mcr = value;
 
-	writel(value, p->membase + (offset << p->regshift));
-
 	/* Make sure LCR write wasn't ignored */
 	if (offset == UART_LCR) {
 		int tries = 1000;
@@ -190,7 +191,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
-			writel(value, p->membase + (UART_LCR << p->regshift));
+			d->serial_out(value,
+				      p->membase + (UART_LCR << p->regshift));
 		}
 		/*
 		 * FIXME: this deadlocks if port->lock is already held
@@ -199,6 +201,22 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 	}
 }
 
+static void _dw8250_serial_out32(unsigned int value, void __iomem *addr)
+{
+	writel(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32(const void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+{
+	writel(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 {
 	unsigned int value = readl(p->membase + (offset << p->regshift));
@@ -206,6 +224,29 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 	return dw8250_modify_msr(p, offset, value);
 }
 
+static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
+{
+	iowrite32be(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32be(const void __iomem *addr)
+{
+	return ioread32be(addr);
+}
+
+static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
+{
+	iowrite32be(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
+static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
+{
+	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
+
+	return dw8250_modify_msr(p, offset, value);
+}
+
 static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct dw8250_data *d = p->private_data;
@@ -322,9 +363,19 @@ static int dw8250_probe_of(struct uart_port *p,
 		case 1:
 			break;
 		case 4:
-			p->iotype = UPIO_MEM32;
-			p->serial_in = dw8250_serial_in32;
-			p->serial_out = dw8250_serial_out32;
+			p->iotype = of_device_is_big_endian(np) ?
+				    UPIO_MEM32BE : UPIO_MEM32;
+			if (p->iotype == UPIO_MEM32) {
+				p->serial_in = dw8250_serial_in32;
+				p->serial_out = dw8250_serial_out32;
+				data->serial_in = _dw8250_serial_in32;
+				data->serial_out = _dw8250_serial_out32;
+			} else {
+				p->serial_in = dw8250_serial_in32be;
+				p->serial_out = dw8250_serial_out32be;
+				data->serial_in = _dw8250_serial_in32be;
+				data->serial_out = _dw8250_serial_out32be;
+			}
 			break;
 		default:
 			dev_err(p->dev, "unsupported reg-io-width (%u)\n", val);
@@ -504,6 +555,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	data->dma.rx_param = data;
 	data->dma.tx_param = data;
 	data->dma.fn = dw8250_dma_filter;
+	data->serial_in = _dw8250_serial_in32;
+	data->serial_out = _dw8250_serial_out32;
 
 	uart.port.iotype = UPIO_MEM;
 	uart.port.serial_in = dw8250_serial_in;
-- 
1.7.1

[v4 2/2] serial: 8250_dw: dw8250_setup_port() use endianness aware read.

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
Instead we use the serial_in32() accessor which is initialized
properly according to endianness.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f479433..62f766a 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -310,7 +310,8 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
 static void dw8250_setup_port(struct uart_8250_port *up)
 {
 	struct uart_port	*p = &up->port;
-	u32			reg = readl(p->membase + DW_UART_UCV);
+	struct dw8250_data	*d = p->private_data;
+	u32			reg = d->serial_in(p->membase + DW_UART_UCV);
 
 	/*
 	 * If the Component Version Register returns zero, we know that
@@ -322,7 +323,7 @@ static void dw8250_setup_port(struct uart_8250_port *up)
 	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
-	reg = readl(p->membase + DW_UART_CPR);
+	reg = d->serial_in(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
-- 
1.7.1

Re: [v4 2/2] serial: 8250_dw: dw8250_setup_port() use endianness aware read.

[Vineet Gupta]

Hi Noam,

On Friday 21 August 2015 04:05 PM, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
>
> readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
> Instead we use the serial_in32() accessor which is initialized
> properly according to endianness.
>
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index f479433..62f766a 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -310,7 +310,8 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
>  static void dw8250_setup_port(struct uart_8250_port *up)
>  {
>  	struct uart_port	*p = &up->port;
> -	u32			reg = readl(p->membase + DW_UART_UCV);
> +	struct dw8250_data	*d = p->private_data;
> +	u32			reg = d->serial_in(p->membase + DW_UART_UCV);
>  
>  	/*
>  	 * If the Component Version Register returns zero, we know that
> @@ -322,7 +323,7 @@ static void dw8250_setup_port(struct uart_8250_port *up)
>  	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>  
> -	reg = readl(p->membase + DW_UART_CPR);
> +	reg = d->serial_in(p->membase + DW_UART_CPR);
>  	if (!reg)
>  		return;
>  

I think this can be folded into the previous patch - I don't see how this
logically seperates anything from patch 1/2

-Vineet

[v5] *** 8250_dw ***

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

v5 change:
Two patches is now squashed into single one

v4 change
Remove patch for skipping looptest through DT option.
This is now handled in our simulator model.
Thanks to Vineet Gupta from Synopsys for his help.

We are left with 2 patches which adds BIG endian support.

V3 change:
Use second level accessors for big/little endian port.
The new accessors are now pointed from uart_port->private_data
These accessors are initialized during driver probe().
Driver shouldn't access directly to readl/writel but to
these new second level accessors (first level is at uart_port).
e.g. at dw8250_check_LCR() and dw8250_setup_port() I replaced such
direct calls.

V2 changes:
1) better description for each commit.
2) adding to CC list the device tree maintainer.
3) rename dw8250_check_control() --> dw8250_check_LCR().
4) remove bad patch of "add UPF_FIXED_TYPE to flags".

Noam Camus (1):
  serial: 8250_dw: Add support for big-endian MMIO accesses

 drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++-----
 1 files changed, 63 insertions(+), 9 deletions(-)

[v5] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
dw8250_serial_out32() extra functionality that is not part of
the 8250 core driver was moved to new function called
dw8250_check_LCR().

For big endian we use 2 new accessors similar to little endian,
called dw8250_serial_out32be() and dw8250_serial_in32be().
Both little and big endian accessors use dw8250_check_LCR()
for their dw8250_serial_out32{,be}().

In addition I added another 2 accessors inside private_data field of
uart_port. This second level accessors are set during probe in
private_data field of uart_port. Now any direct call to readl/writel
is replaced with those accessors which are endianness aware.

Last issue:
readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
Instead we use the serial_in32() accessor which is initialized
properly according to endianness.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++-----
 1 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index d48b506..62f766a 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -64,6 +64,9 @@ struct dw8250_data {
 	struct clk		*pclk;
 	struct reset_control	*rst;
 	struct uart_8250_dma	dma;
+	unsigned int		(*serial_in)(const void __iomem *addr);
+	void			(*serial_out)(unsigned int value,
+					      void __iomem *addr);
 };
 
 #define BYT_PRV_CLK			0x800
@@ -173,15 +176,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_64BIT */
 
-static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
 {
 	struct dw8250_data *d = p->private_data;
 
 	if (offset == UART_MCR)
 		d->last_mcr = value;
 
-	writel(value, p->membase + (offset << p->regshift));
-
 	/* Make sure LCR write wasn't ignored */
 	if (offset == UART_LCR) {
 		int tries = 1000;
@@ -190,7 +191,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
-			writel(value, p->membase + (UART_LCR << p->regshift));
+			d->serial_out(value,
+				      p->membase + (UART_LCR << p->regshift));
 		}
 		/*
 		 * FIXME: this deadlocks if port->lock is already held
@@ -199,6 +201,22 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 	}
 }
 
+static void _dw8250_serial_out32(unsigned int value, void __iomem *addr)
+{
+	writel(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32(const void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+{
+	writel(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 {
 	unsigned int value = readl(p->membase + (offset << p->regshift));
@@ -206,6 +224,29 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 	return dw8250_modify_msr(p, offset, value);
 }
 
+static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
+{
+	iowrite32be(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32be(const void __iomem *addr)
+{
+	return ioread32be(addr);
+}
+
+static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
+{
+	iowrite32be(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
+static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
+{
+	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
+
+	return dw8250_modify_msr(p, offset, value);
+}
+
 static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct dw8250_data *d = p->private_data;
@@ -269,7 +310,8 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
 static void dw8250_setup_port(struct uart_8250_port *up)
 {
 	struct uart_port	*p = &up->port;
-	u32			reg = readl(p->membase + DW_UART_UCV);
+	struct dw8250_data	*d = p->private_data;
+	u32			reg = d->serial_in(p->membase + DW_UART_UCV);
 
 	/*
 	 * If the Component Version Register returns zero, we know that
@@ -281,7 +323,7 @@ static void dw8250_setup_port(struct uart_8250_port *up)
 	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
-	reg = readl(p->membase + DW_UART_CPR);
+	reg = d->serial_in(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
@@ -322,9 +364,19 @@ static int dw8250_probe_of(struct uart_port *p,
 		case 1:
 			break;
 		case 4:
-			p->iotype = UPIO_MEM32;
-			p->serial_in = dw8250_serial_in32;
-			p->serial_out = dw8250_serial_out32;
+			p->iotype = of_device_is_big_endian(np) ?
+				    UPIO_MEM32BE : UPIO_MEM32;
+			if (p->iotype == UPIO_MEM32) {
+				p->serial_in = dw8250_serial_in32;
+				p->serial_out = dw8250_serial_out32;
+				data->serial_in = _dw8250_serial_in32;
+				data->serial_out = _dw8250_serial_out32;
+			} else {
+				p->serial_in = dw8250_serial_in32be;
+				p->serial_out = dw8250_serial_out32be;
+				data->serial_in = _dw8250_serial_in32be;
+				data->serial_out = _dw8250_serial_out32be;
+			}
 			break;
 		default:
 			dev_err(p->dev, "unsupported reg-io-width (%u)\n", val);
@@ -504,6 +556,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	data->dma.rx_param = data;
 	data->dma.tx_param = data;
 	data->dma.fn = dw8250_dma_filter;
+	data->serial_in = _dw8250_serial_in32;
+	data->serial_out = _dw8250_serial_out32;
 
 	uart.port.iotype = UPIO_MEM;
 	uart.port.serial_in = dw8250_serial_in;
-- 
1.7.1

Re: [v5] serial: 8250_dw: Add support for big-endian MMIO accesses

[Greg KH]

On Tue, Aug 25, 2015 at 11:57:09AM +0300, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
> dw8250_serial_out32() extra functionality that is not part of
> the 8250 core driver was moved to new function called
> dw8250_check_LCR().
> 
> For big endian we use 2 new accessors similar to little endian,
> called dw8250_serial_out32be() and dw8250_serial_in32be().
> Both little and big endian accessors use dw8250_check_LCR()
> for their dw8250_serial_out32{,be}().
> 
> In addition I added another 2 accessors inside private_data field of
> uart_port. This second level accessors are set during probe in
> private_data field of uart_port. Now any direct call to readl/writel
> is replaced with those accessors which are endianness aware.
> 
> Last issue:
> readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
> Instead we use the serial_in32() accessor which is initialized
> properly according to endianness.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++-----
>  1 files changed, 63 insertions(+), 9 deletions(-)

This doesn't apply to my tree at all anymore, what did you make it
against?

Can you please respin it and resend?

thanks,

greg k-h

[v6] *** 8250_dw ***

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

v6 change:
Adapt patch to latest version (nothing functional)

v5 change:
Two patches is now squashed into single one

v4 change
Remove patch for skipping looptest through DT option.
This is now handled in our simulator model.
Thanks to Vineet Gupta from Synopsys for his help.

We are left with 2 patches which adds BIG endian support.

V3 change:
Use second level accessors for big/little endian port.
The new accessors are now pointed from uart_port->private_data
These accessors are initialized during driver probe().
Driver shouldn't access directly to readl/writel but to
these new second level accessors (first level is at uart_port).
e.g. at dw8250_check_LCR() and dw8250_setup_port() I replaced such
direct calls.

V2 changes:
1) better description for each commit.
2) adding to CC list the device tree maintainer.
3) rename dw8250_check_control() --> dw8250_check_LCR().
4) remove bad patch of "add UPF_FIXED_TYPE to flags".

Noam Camus (1):
  serial: 8250_dw: Add support for big-endian MMIO accesses

 drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++----
 1 files changed, 64 insertions(+), 8 deletions(-)

[v6] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
dw8250_serial_out32() extra functionality that is not part of
the 8250 core driver was moved to new function called
dw8250_check_LCR().

For big endian we use 2 new accessors similar to little endian,
called dw8250_serial_out32be() and dw8250_serial_in32be().
Both little and big endian accessors use dw8250_check_LCR()
for their dw8250_serial_out32{,be}().

In addition I added another 2 accessors inside private_data field of
uart_port. This second level accessors are set during probe in
private_data field of uart_port. Now any direct call to readl/writel
is replaced with those accessors which are endianness aware.

Last issue:
readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
Instead we use the serial_in32() accessor which is initialized
properly according to endianness.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++----
 1 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 06324f1..2cb62cf 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -63,6 +63,9 @@ struct dw8250_data {
 	struct clk		*pclk;
 	struct reset_control	*rst;
 	struct uart_8250_dma	dma;
+	unsigned int		(*serial_in)(const void __iomem *addr);
+	void			(*serial_out)(unsigned int value,
+					      void __iomem *addr);
 };
 
 #define BYT_PRV_CLK			0x800
@@ -156,9 +159,9 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_64BIT */
 
-static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
 {
-	writel(value, p->membase + (offset << p->regshift));
+	struct dw8250_data *d = p->private_data;
 
 	/* Make sure LCR write wasn't ignored */
 	if (offset == UART_LCR) {
@@ -168,7 +171,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
-			writel(value, p->membase + (UART_LCR << p->regshift));
+			d->serial_out(value,
+				      p->membase + (UART_LCR << p->regshift));
 		}
 		/*
 		 * FIXME: this deadlocks if port->lock is already held
@@ -177,6 +181,22 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 	}
 }
 
+static void _dw8250_serial_out32(unsigned int value, void __iomem *addr)
+{
+	writel(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32(const void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+{
+	writel(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 {
 	unsigned int value = readl(p->membase + (offset << p->regshift));
@@ -184,6 +204,29 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 	return dw8250_modify_msr(p, offset, value);
 }
 
+static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
+{
+	iowrite32be(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32be(const void __iomem *addr)
+{
+	return ioread32be(addr);
+}
+
+static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
+{
+	iowrite32be(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
+static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
+{
+	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
+
+	return dw8250_modify_msr(p, offset, value);
+}
+
 static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct dw8250_data *d = p->private_data;
@@ -252,7 +295,8 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
 static void dw8250_setup_port(struct uart_8250_port *up)
 {
 	struct uart_port	*p = &up->port;
-	u32			reg = readl(p->membase + DW_UART_UCV);
+	struct dw8250_data	*d = p->private_data;
+	u32			reg = d->serial_in(p->membase + DW_UART_UCV);
 
 	/*
 	 * If the Component Version Register returns zero, we know that
@@ -264,7 +308,7 @@ static void dw8250_setup_port(struct uart_8250_port *up)
 	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
-	reg = readl(p->membase + DW_UART_CPR);
+	reg = d->serial_in(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
@@ -305,9 +349,19 @@ static int dw8250_probe_of(struct uart_port *p,
 		case 1:
 			break;
 		case 4:
-			p->iotype = UPIO_MEM32;
-			p->serial_in = dw8250_serial_in32;
-			p->serial_out = dw8250_serial_out32;
+			p->iotype = of_device_is_big_endian(np) ?
+				    UPIO_MEM32BE : UPIO_MEM32;
+			if (p->iotype == UPIO_MEM32) {
+				p->serial_in = dw8250_serial_in32;
+				p->serial_out = dw8250_serial_out32;
+				data->serial_in = _dw8250_serial_in32;
+				data->serial_out = _dw8250_serial_out32;
+			} else {
+				p->serial_in = dw8250_serial_in32be;
+				p->serial_out = dw8250_serial_out32be;
+				data->serial_in = _dw8250_serial_in32be;
+				data->serial_out = _dw8250_serial_out32be;
+			}
 			break;
 		default:
 			dev_err(p->dev, "unsupported reg-io-width (%u)\n", val);
@@ -487,6 +541,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	data->dma.rx_param = data;
 	data->dma.tx_param = data;
 	data->dma.fn = dw8250_dma_filter;
+	data->serial_in = _dw8250_serial_in32;
+	data->serial_out = _dw8250_serial_out32;
 
 	uart.port.iotype = UPIO_MEM;
 	uart.port.serial_in = dw8250_serial_in;
-- 
1.7.1

Re: [v6] serial: 8250_dw: Add support for big-endian MMIO accesses

[kbuild test robot]

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

Hi Noam,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-i0-201540 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/tty/serial/8250/8250_dw.c: In function '_dw8250_serial_in32be':
>> drivers/tty/serial/8250/8250_dw.c:214:20: warning: passing argument 1 of 'ioread32be' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     return ioread32be(addr);
                       ^
   In file included from arch/x86/include/asm/io.h:203:0,
                    from include/linux/io.h:25,
                    from drivers/tty/serial/8250/8250_dw.c:17:
   include/asm-generic/iomap.h:32:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32be(void __iomem *);
                        ^

vim +214 drivers/tty/serial/8250/8250_dw.c

   198	}
   199	
   200	static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
   201	{
   202		unsigned int value = readl(p->membase + (offset << p->regshift));
   203	
   204		return dw8250_modify_msr(p, offset, value);
   205	}
   206	
   207	static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
   208	{
   209		iowrite32be(value, addr);
   210	}
   211	
   212	static unsigned int _dw8250_serial_in32be(const void __iomem *addr)
   213	{
 > 214		return ioread32be(addr);
   215	}
   216	
   217	static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
   218	{
   219		iowrite32be(value, p->membase + (offset << p->regshift));
   220		dw8250_check_LCR(p, offset, value);
   221	}
   222	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23229 bytes --]

Re: [v6] serial: 8250_dw: Add support for big-endian MMIO accesses

[kbuild test robot]

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

Hi Noam,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: alpha-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All warnings (new ones prefixed by >>):

   In file included from include/linux/swab.h:4:0,
                    from include/uapi/linux/byteorder/little_endian.h:12,
                    from include/linux/byteorder/little_endian.h:4,
                    from arch/alpha/include/uapi/asm/byteorder.h:4,
                    from include/asm-generic/bitops/le.h:5,
                    from arch/alpha/include/asm/bitops.h:454,
                    from include/linux/bitops.h:36,
                    from include/linux/kernel.h:10,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from drivers/tty/serial/8250/8250_dw.c:16:
   drivers/tty/serial/8250/8250_dw.c: In function '_dw8250_serial_in32be':
>> arch/alpha/include/asm/io.h:495:35: warning: passing argument 1 of 'ioread32' discards 'const' qualifier from pointer target type
    #define ioread32be(p) be32_to_cpu(ioread32(p))
                                      ^
   include/uapi/linux/swab.h:115:32: note: in definition of macro '__swab32'
     (__builtin_constant_p((__u32)(x)) ? \
                                   ^
>> include/linux/byteorder/generic.h:94:21: note: in expansion of macro '__be32_to_cpu'
    #define be32_to_cpu __be32_to_cpu
                        ^
>> drivers/tty/serial/8250/8250_dw.c:214:9: note: in expansion of macro 'ioread32be'
     return ioread32be(addr);
            ^
   In file included from arch/alpha/include/asm/io.h:15:0,
                    from include/linux/io.h:25,
                    from drivers/tty/serial/8250/8250_dw.c:17:
   include/asm-generic/iomap.h:31:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32(void __iomem *);
                        ^
   In file included from include/linux/swab.h:4:0,
                    from include/uapi/linux/byteorder/little_endian.h:12,
                    from include/linux/byteorder/little_endian.h:4,
                    from arch/alpha/include/uapi/asm/byteorder.h:4,
                    from include/asm-generic/bitops/le.h:5,
                    from arch/alpha/include/asm/bitops.h:454,
                    from include/linux/bitops.h:36,
                    from include/linux/kernel.h:10,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from drivers/tty/serial/8250/8250_dw.c:16:
>> arch/alpha/include/asm/io.h:495:35: warning: passing argument 1 of 'ioread32' discards 'const' qualifier from pointer target type
    #define ioread32be(p) be32_to_cpu(ioread32(p))
                                      ^
   include/uapi/linux/swab.h:17:12: note: in definition of macro '___constant_swab32'
     (((__u32)(x) & (__u32)0x000000ffUL) << 24) |  \
               ^
   include/uapi/linux/byteorder/little_endian.h:39:26: note: in expansion of macro '__swab32'
    #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
                             ^
>> include/linux/byteorder/generic.h:94:21: note: in expansion of macro '__be32_to_cpu'
    #define be32_to_cpu __be32_to_cpu
                        ^
>> drivers/tty/serial/8250/8250_dw.c:214:9: note: in expansion of macro 'ioread32be'
     return ioread32be(addr);
            ^
   In file included from arch/alpha/include/asm/io.h:15:0,
                    from include/linux/io.h:25,
                    from drivers/tty/serial/8250/8250_dw.c:17:
   include/asm-generic/iomap.h:31:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32(void __iomem *);
                        ^
   In file included from include/linux/swab.h:4:0,
                    from include/uapi/linux/byteorder/little_endian.h:12,
                    from include/linux/byteorder/little_endian.h:4,
                    from arch/alpha/include/uapi/asm/byteorder.h:4,
                    from include/asm-generic/bitops/le.h:5,
                    from arch/alpha/include/asm/bitops.h:454,
                    from include/linux/bitops.h:36,
                    from include/linux/kernel.h:10,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from drivers/tty/serial/8250/8250_dw.c:16:
>> arch/alpha/include/asm/io.h:495:35: warning: passing argument 1 of 'ioread32' discards 'const' qualifier from pointer target type
    #define ioread32be(p) be32_to_cpu(ioread32(p))
                                      ^
   include/uapi/linux/swab.h:18:12: note: in definition of macro '___constant_swab32'
     (((__u32)(x) & (__u32)0x0000ff00UL) <<  8) |  \
               ^
   include/uapi/linux/byteorder/little_endian.h:39:26: note: in expansion of macro '__swab32'
    #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
                             ^
>> include/linux/byteorder/generic.h:94:21: note: in expansion of macro '__be32_to_cpu'
    #define be32_to_cpu __be32_to_cpu
                        ^
>> drivers/tty/serial/8250/8250_dw.c:214:9: note: in expansion of macro 'ioread32be'
     return ioread32be(addr);
            ^
   In file included from arch/alpha/include/asm/io.h:15:0,
                    from include/linux/io.h:25,
                    from drivers/tty/serial/8250/8250_dw.c:17:
   include/asm-generic/iomap.h:31:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32(void __iomem *);
                        ^
   In file included from include/linux/swab.h:4:0,
                    from include/uapi/linux/byteorder/little_endian.h:12,
                    from include/linux/byteorder/little_endian.h:4,
                    from arch/alpha/include/uapi/asm/byteorder.h:4,
                    from include/asm-generic/bitops/le.h:5,
                    from arch/alpha/include/asm/bitops.h:454,
                    from include/linux/bitops.h:36,
                    from include/linux/kernel.h:10,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from drivers/tty/serial/8250/8250_dw.c:16:
>> arch/alpha/include/asm/io.h:495:35: warning: passing argument 1 of 'ioread32' discards 'const' qualifier from pointer target type
    #define ioread32be(p) be32_to_cpu(ioread32(p))
                                      ^
   include/uapi/linux/swab.h:19:12: note: in definition of macro '___constant_swab32'
     (((__u32)(x) & (__u32)0x00ff0000UL) >>  8) |  \
               ^
   include/uapi/linux/byteorder/little_endian.h:39:26: note: in expansion of macro '__swab32'
    #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
                             ^
>> include/linux/byteorder/generic.h:94:21: note: in expansion of macro '__be32_to_cpu'
    #define be32_to_cpu __be32_to_cpu
                        ^
>> drivers/tty/serial/8250/8250_dw.c:214:9: note: in expansion of macro 'ioread32be'
     return ioread32be(addr);
            ^
   In file included from arch/alpha/include/asm/io.h:15:0,
                    from include/linux/io.h:25,
                    from drivers/tty/serial/8250/8250_dw.c:17:
   include/asm-generic/iomap.h:31:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32(void __iomem *);
                        ^
   In file included from include/linux/swab.h:4:0,
                    from include/uapi/linux/byteorder/little_endian.h:12,
                    from include/linux/byteorder/little_endian.h:4,
                    from arch/alpha/include/uapi/asm/byteorder.h:4,
                    from include/asm-generic/bitops/le.h:5,
                    from arch/alpha/include/asm/bitops.h:454,
                    from include/linux/bitops.h:36,
                    from include/linux/kernel.h:10,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from drivers/tty/serial/8250/8250_dw.c:16:
>> arch/alpha/include/asm/io.h:495:35: warning: passing argument 1 of 'ioread32' discards 'const' qualifier from pointer target type
    #define ioread32be(p) be32_to_cpu(ioread32(p))
                                      ^
   include/uapi/linux/swab.h:20:12: note: in definition of macro '___constant_swab32'
     (((__u32)(x) & (__u32)0xff000000UL) >> 24)))
               ^
   include/uapi/linux/byteorder/little_endian.h:39:26: note: in expansion of macro '__swab32'
    #define __be32_to_cpu(x) __swab32((__force __u32)(__be32)(x))
                             ^
>> include/linux/byteorder/generic.h:94:21: note: in expansion of macro '__be32_to_cpu'
    #define be32_to_cpu __be32_to_cpu
                        ^
>> drivers/tty/serial/8250/8250_dw.c:214:9: note: in expansion of macro 'ioread32be'
     return ioread32be(addr);
            ^
   In file included from arch/alpha/include/asm/io.h:15:0,
                    from include/linux/io.h:25,
                    from drivers/tty/serial/8250/8250_dw.c:17:
   include/asm-generic/iomap.h:31:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32(void __iomem *);
                        ^
   In file included from include/linux/swab.h:4:0,
                    from include/uapi/linux/byteorder/little_endian.h:12,
                    from include/linux/byteorder/little_endian.h:4,
                    from arch/alpha/include/uapi/asm/byteorder.h:4,
                    from include/asm-generic/bitops/le.h:5,
                    from arch/alpha/include/asm/bitops.h:454,
                    from include/linux/bitops.h:36,
                    from include/linux/kernel.h:10,
                    from include/linux/list.h:8,
                    from include/linux/kobject.h:20,
                    from include/linux/device.h:17,
                    from drivers/tty/serial/8250/8250_dw.c:16:
>> arch/alpha/include/asm/io.h:495:35: warning: passing argument 1 of 'ioread32' discards 'const' qualifier from pointer target type
    #define ioread32be(p) be32_to_cpu(ioread32(p))
                                      ^
   include/uapi/linux/swab.h:117:12: note: in definition of macro '__swab32'
     __fswab32(x))
               ^
>> include/linux/byteorder/generic.h:94:21: note: in expansion of macro '__be32_to_cpu'
    #define be32_to_cpu __be32_to_cpu
                        ^
>> drivers/tty/serial/8250/8250_dw.c:214:9: note: in expansion of macro 'ioread32be'
     return ioread32be(addr);
            ^
   In file included from arch/alpha/include/asm/io.h:15:0,
                    from include/linux/io.h:25,
                    from drivers/tty/serial/8250/8250_dw.c:17:
   include/asm-generic/iomap.h:31:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32(void __iomem *);
                        ^

vim +/ioread32be +214 drivers/tty/serial/8250/8250_dw.c

   198	}
   199	
   200	static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
   201	{
   202		unsigned int value = readl(p->membase + (offset << p->regshift));
   203	
   204		return dw8250_modify_msr(p, offset, value);
   205	}
   206	
   207	static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
   208	{
   209		iowrite32be(value, addr);
   210	}
   211	
   212	static unsigned int _dw8250_serial_in32be(const void __iomem *addr)
   213	{
 > 214		return ioread32be(addr);
   215	}
   216	
   217	static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
   218	{
   219		iowrite32be(value, p->membase + (offset << p->regshift));
   220		dw8250_check_LCR(p, offset, value);
   221	}
   222	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 43617 bytes --]

Re: [v6] serial: 8250_dw: Add support for big-endian MMIO accesses

[Greg KH]

On Tue, Oct 06, 2015 at 09:39:50AM +0300, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
> dw8250_serial_out32() extra functionality that is not part of
> the 8250 core driver was moved to new function called
> dw8250_check_LCR().
> 
> For big endian we use 2 new accessors similar to little endian,
> called dw8250_serial_out32be() and dw8250_serial_in32be().
> Both little and big endian accessors use dw8250_check_LCR()
> for their dw8250_serial_out32{,be}().
> 
> In addition I added another 2 accessors inside private_data field of
> uart_port. This second level accessors are set during probe in
> private_data field of uart_port. Now any direct call to readl/writel
> is replaced with those accessors which are endianness aware.
> 
> Last issue:
> readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
> Instead we use the serial_in32() accessor which is initialized
> properly according to endianness.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++----
>  1 files changed, 64 insertions(+), 8 deletions(-)

Can you please fix the warnings as pointed out by the kbuild testing and
resend this?

thanks,

greg k-h

Re: [v6] serial: 8250_dw: Add support for big-endian MMIO accesses

[kbuild test robot]

Hi Noam,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/tty/serial/8250/8250_dw.c:214:27: sparse: incorrect type in argument 1 (different modifiers)
   drivers/tty/serial/8250/8250_dw.c:214:27:    expected void [noderef] <asn:2>*<noident>
   drivers/tty/serial/8250/8250_dw.c:214:27:    got void const [noderef] <asn:2>*addr
   drivers/tty/serial/8250/8250_dw.c: In function '_dw8250_serial_in32be':
   drivers/tty/serial/8250/8250_dw.c:214:20: warning: passing argument 1 of 'ioread32be' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
     return ioread32be(addr);
                       ^
   In file included from arch/x86/include/asm/io.h:203:0,
                    from arch/x86/include/asm/realmode.h:5,
                    from arch/x86/include/asm/acpi.h:33,
                    from arch/x86/include/asm/fixmap.h:19,
                    from arch/x86/include/asm/apic.h:12,
                    from arch/x86/include/asm/smp.h:12,
                    from arch/x86/include/asm/mmzone_64.h:10,
                    from arch/x86/include/asm/mmzone.h:4,
                    from include/linux/mmzone.h:934,
                    from include/linux/gfp.h:5,
                    from include/linux/device.h:29,
                    from drivers/tty/serial/8250/8250_dw.c:16:
   include/asm-generic/iomap.h:32:21: note: expected 'void *' but argument is of type 'const void *'
    extern unsigned int ioread32be(void __iomem *);
                        ^

vim +214 drivers/tty/serial/8250/8250_dw.c

   198	}
   199	
   200	static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
   201	{
   202		unsigned int value = readl(p->membase + (offset << p->regshift));
   203	
   204		return dw8250_modify_msr(p, offset, value);
   205	}
   206	
   207	static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
   208	{
   209		iowrite32be(value, addr);
   210	}
   211	
   212	static unsigned int _dw8250_serial_in32be(const void __iomem *addr)
   213	{
 > 214		return ioread32be(addr);
   215	}
   216	
   217	static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
   218	{
   219		iowrite32be(value, p->membase + (offset << p->regshift));
   220		dw8250_check_LCR(p, offset, value);
   221	}
   222	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[v7] *** 8250_dw ***

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

v7 change:
Fix build warning due to redundant "const" qualifier at
_dw8250_serial_in32be() signature.

v6 change:
Adapt patch to latest version (nothing functional)

v5 change:
Two patches is now squashed into single one

v4 change
Remove patch for skipping looptest through DT option.
This is now handled in our simulator model.
Thanks to Vineet Gupta from Synopsys for his help.

We are left with 2 patches which adds BIG endian support.

V3 change:
Use second level accessors for big/little endian port.
The new accessors are now pointed from uart_port->private_data
These accessors are initialized during driver probe().
Driver shouldn't access directly to readl/writel but to
these new second level accessors (first level is at uart_port).
e.g. at dw8250_check_LCR() and dw8250_setup_port() I replaced such
direct calls.

V2 changes:
1) better description for each commit.
2) adding to CC list the device tree maintainer.
3) rename dw8250_check_control() --> dw8250_check_LCR().
4) remove bad patch of "add UPF_FIXED_TYPE to flags".

Noam Camus (1):
  serial: 8250_dw: Add support for big-endian MMIO accesses

 drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++----
 1 files changed, 64 insertions(+), 8 deletions(-)

[v7] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
dw8250_serial_out32() extra functionality that is not part of
the 8250 core driver was moved to new function called
dw8250_check_LCR().

For big endian we use 2 new accessors similar to little endian,
called dw8250_serial_out32be() and dw8250_serial_in32be().
Both little and big endian accessors use dw8250_check_LCR()
for their dw8250_serial_out32{,be}().

In addition I added another 2 accessors inside private_data field of
uart_port. This second level accessors are set during probe in
private_data field of uart_port. Now any direct call to readl/writel
is replaced with those accessors which are endianness aware.

Last issue:
readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
Instead we use the serial_in32() accessor which is initialized
properly according to endianness.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
 drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++----
 1 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 06324f1..3beff92 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -63,6 +63,9 @@ struct dw8250_data {
 	struct clk		*pclk;
 	struct reset_control	*rst;
 	struct uart_8250_dma	dma;
+	unsigned int		(*serial_in)(void __iomem *addr);
+	void			(*serial_out)(unsigned int value,
+					      void __iomem *addr);
 };
 
 #define BYT_PRV_CLK			0x800
@@ -156,9 +159,9 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_64BIT */
 
-static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
 {
-	writel(value, p->membase + (offset << p->regshift));
+	struct dw8250_data *d = p->private_data;
 
 	/* Make sure LCR write wasn't ignored */
 	if (offset == UART_LCR) {
@@ -168,7 +171,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
-			writel(value, p->membase + (UART_LCR << p->regshift));
+			d->serial_out(value,
+				      p->membase + (UART_LCR << p->regshift));
 		}
 		/*
 		 * FIXME: this deadlocks if port->lock is already held
@@ -177,6 +181,22 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 	}
 }
 
+static void _dw8250_serial_out32(unsigned int value, void __iomem *addr)
+{
+	writel(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32(void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+{
+	writel(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 {
 	unsigned int value = readl(p->membase + (offset << p->regshift));
@@ -184,6 +204,29 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 	return dw8250_modify_msr(p, offset, value);
 }
 
+static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
+{
+	iowrite32be(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32be(void __iomem *addr)
+{
+	return ioread32be(addr);
+}
+
+static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
+{
+	iowrite32be(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
+static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
+{
+	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
+
+	return dw8250_modify_msr(p, offset, value);
+}
+
 static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct dw8250_data *d = p->private_data;
@@ -252,7 +295,8 @@ static bool dw8250_dma_filter(struct dma_chan *chan, void *param)
 static void dw8250_setup_port(struct uart_8250_port *up)
 {
 	struct uart_port	*p = &up->port;
-	u32			reg = readl(p->membase + DW_UART_UCV);
+	struct dw8250_data	*d = p->private_data;
+	u32			reg = d->serial_in(p->membase + DW_UART_UCV);
 
 	/*
 	 * If the Component Version Register returns zero, we know that
@@ -264,7 +308,7 @@ static void dw8250_setup_port(struct uart_8250_port *up)
 	dev_dbg_ratelimited(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
-	reg = readl(p->membase + DW_UART_CPR);
+	reg = d->serial_in(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
@@ -305,9 +349,19 @@ static int dw8250_probe_of(struct uart_port *p,
 		case 1:
 			break;
 		case 4:
-			p->iotype = UPIO_MEM32;
-			p->serial_in = dw8250_serial_in32;
-			p->serial_out = dw8250_serial_out32;
+			p->iotype = of_device_is_big_endian(np) ?
+				    UPIO_MEM32BE : UPIO_MEM32;
+			if (p->iotype == UPIO_MEM32) {
+				p->serial_in = dw8250_serial_in32;
+				p->serial_out = dw8250_serial_out32;
+				data->serial_in = _dw8250_serial_in32;
+				data->serial_out = _dw8250_serial_out32;
+			} else {
+				p->serial_in = dw8250_serial_in32be;
+				p->serial_out = dw8250_serial_out32be;
+				data->serial_in = _dw8250_serial_in32be;
+				data->serial_out = _dw8250_serial_out32be;
+			}
 			break;
 		default:
 			dev_err(p->dev, "unsupported reg-io-width (%u)\n", val);
@@ -487,6 +541,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	data->dma.rx_param = data;
 	data->dma.tx_param = data;
 	data->dma.fn = dw8250_dma_filter;
+	data->serial_in = _dw8250_serial_in32;
+	data->serial_out = _dw8250_serial_out32;
 
 	uart.port.iotype = UPIO_MEM;
 	uart.port.serial_in = dw8250_serial_in;
-- 
1.7.1

Re: [v7] serial: 8250_dw: Add support for big-endian MMIO accesses

[Greg KH]

On Tue, Oct 06, 2015 at 11:53:30AM +0300, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
> dw8250_serial_out32() extra functionality that is not part of
> the 8250 core driver was moved to new function called
> dw8250_check_LCR().
> 
> For big endian we use 2 new accessors similar to little endian,
> called dw8250_serial_out32be() and dw8250_serial_in32be().
> Both little and big endian accessors use dw8250_check_LCR()
> for their dw8250_serial_out32{,be}().
> 
> In addition I added another 2 accessors inside private_data field of
> uart_port. This second level accessors are set during probe in
> private_data field of uart_port. Now any direct call to readl/writel
> is replaced with those accessors which are endianness aware.
> 
> Last issue:
> readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
> Instead we use the serial_in32() accessor which is initialized
> properly according to endianness.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c |   72 ++++++++++++++++++++++++++++++++----
>  1 files changed, 64 insertions(+), 8 deletions(-)

This patch still doesn't apply at all to my tty-next branch, what are
you making it against?

Please fix it up and resend.

thanks,

greg k-h

RE: [v7] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

> From: Greg KH [mailto:gregkh@linuxfoundation.org] 
> Sent: Sunday, October 18, 2015 7:06 AM

> This patch still doesn't apply at all to my tty-next branch, what are you making it against?

> Please fix it up and resend.

The patch is against v4.3-rc4
I will update to rc5

thanks,

Noam Camus

Re: [v7] serial: 8250_dw: Add support for big-endian MMIO accesses

[Greg KH]

On Sun, Oct 18, 2015 at 05:30:23AM +0000, Noam Camus wrote:
> > From: Greg KH [mailto:gregkh@linuxfoundation.org] 
> > Sent: Sunday, October 18, 2015 7:06 AM
> 
> > This patch still doesn't apply at all to my tty-next branch, what are you making it against?
> 
> > Please fix it up and resend.
> 
> The patch is against v4.3-rc4
> I will update to rc5

Please make it against my tty-testing branch of tty.git on
git.kernel.org, that is the issue here, you are working against a
"clean" tree, not one with all of the other tty/serial changes in it.

thanks,

greg k-h

Re: [v7] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Greg KH <gregkh@linuxfoundation.org>
Sent: Sunday, October 18, 2015 8:57 AM

> Please make it against my tty-testing branch of tty.git on
git.kernel.org, that is the issue here, you are working against a
"clean" tree, not one with all of the other tty/serial changes in it.

I havn't see any respond for my v8 of this patch.
https://lkml.org/lkml/2015/10/18/135

Is it reviewed?

-Noam

Re: [v7] serial: 8250_dw: Add support for big-endian MMIO accesses

[Andy Shevchenko]

On Wed, Dec 9, 2015 at 12:57 AM, Noam Camus <noamc@ezchip.com> wrote:
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Sunday, October 18, 2015 8:57 AM
>
>> Please make it against my tty-testing branch of tty.git on
> git.kernel.org, that is the issue here, you are working against a
> "clean" tree, not one with all of the other tty/serial changes in it.
>
> I havn't see any respond for my v8 of this patch.
> https://lkml.org/lkml/2015/10/18/135
>
> Is it reviewed?

Can you send  v9 with Cc to me and Heikki?

My concerns here are a) to split refactor part (check LCR), and b) do
the actual feature enhancement.

-- 
With Best Regards,
Andy Shevchenko

RE: [v7] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 469 bytes --]

From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] 
Sent: Wednesday, December 09, 2015 4:19 PM

>Can you send  v9 with Cc to me and Heikki?
No problem

>My concerns here are a) to split refactor part (check LCR), and b) do the actual feature enhancement.
I will split this into couple of commits

-Noam
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH-v8] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.
dw8250_serial_out32() extra functionality that is not part of
the 8250 core driver was moved to new function called
dw8250_check_LCR().

For big endian we use 2 new accessors similar to little endian,
called dw8250_serial_out32be() and dw8250_serial_in32be().
Both little and big endian accessors use dw8250_check_LCR()
for their dw8250_serial_out32{,be}().

In addition I added another 2 accessors inside private_data field of
uart_port. This second level accessors are set during probe in
private_data field of uart_port. Now any direct call to readl/writel
is replaced with those accessors which are endianness aware.

Last issue:
readl() for UCV and CPR will not work for port type UPIO_MEM32BE.
Instead we use the serial_in32() accessor which is initialized
properly according to endianness.

Signed-off-by: Noam Camus <noamc@ezchip.com>
---
V8 change:
rebase on tty-next head, no functional change.

V7 change:
Fix build warning due to redundant "const" qualifier at
_dw8250_serial_in32be() signature.

V6 change:
Adapt patch to latest version (nothing functional)

V5 change:
Two patches is now squashed into single one

V4 change
Remove patch for skipping looptest through DT option.
This is now handled in our simulator model.
Thanks to Vineet Gupta from Synopsys for his help.

We are left with 2 patches which adds BIG endian support.

V3 change:
Use second level accessors for big/little endian port.
The new accessors are now pointed from uart_port->private_data
These accessors are initialized during driver probe().
Driver shouldn't access directly to readl/writel but to
these new second level accessors (first level is at uart_port).
e.g. at dw8250_check_LCR() and dw8250_setup_port() I replaced such
direct calls.

V2 changes:
1) better description for each commit.
2) adding to CC list the device tree maintainer.
3) rename dw8250_check_control() --> dw8250_check_LCR().
4) remove bad patch of "add UPF_FIXED_TYPE to flags".

 drivers/tty/serial/8250/8250_dw.c |   73 +++++++++++++++++++++++++++++++++----
 1 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a0cdbf3..880f712 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -63,6 +63,9 @@ struct dw8250_data {
 	struct clk		*pclk;
 	struct reset_control	*rst;
 	struct uart_8250_dma	dma;
+	unsigned int		(*serial_in)(void __iomem *addr);
+	void			(*serial_out)(unsigned int value,
+					      void __iomem *addr);
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
@@ -159,9 +162,9 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 }
 #endif /* CONFIG_64BIT */
 
-static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
 {
-	writel(value, p->membase + (offset << p->regshift));
+	struct dw8250_data *d = p->private_data;
 
 	/* Make sure LCR write wasn't ignored */
 	if (offset == UART_LCR) {
@@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 				return;
 			dw8250_force_idle(p);
-			writel(value, p->membase + (UART_LCR << p->regshift));
+			d->serial_out(value,
+				      p->membase + (UART_LCR << p->regshift));
 		}
 		/*
 		 * FIXME: this deadlocks if port->lock is already held
@@ -180,6 +184,22 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 	}
 }
 
+static void _dw8250_serial_out32(unsigned int value, void __iomem *addr)
+{
+	writel(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32(void __iomem *addr)
+{
+	return readl(addr);
+}
+
+static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
+{
+	writel(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 {
 	unsigned int value = readl(p->membase + (offset << p->regshift));
@@ -187,6 +207,29 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 	return dw8250_modify_msr(p, offset, value);
 }
 
+static void _dw8250_serial_out32be(unsigned int value, void __iomem *addr)
+{
+	iowrite32be(value, addr);
+}
+
+static unsigned int _dw8250_serial_in32be(void __iomem *addr)
+{
+	return ioread32be(addr);
+}
+
+static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
+{
+	iowrite32be(value, p->membase + (offset << p->regshift));
+	dw8250_check_LCR(p, offset, value);
+}
+
+static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
+{
+	unsigned int value = ioread32be(p->membase + (offset << p->regshift));
+
+	return dw8250_modify_msr(p, offset, value);
+}
+
 static int dw8250_handle_irq(struct uart_port *p)
 {
 	struct dw8250_data *d = p->private_data;
@@ -307,20 +350,21 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 static void dw8250_setup_port(struct uart_port *p)
 {
 	struct uart_8250_port *up = up_to_u8250p(p);
+	struct dw8250_data *d = p->private_data;
 	u32 reg;
 
 	/*
 	 * If the Component Version Register returns zero, we know that
 	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
 	 */
-	reg = readl(p->membase + DW_UART_UCV);
+	reg = d->serial_in(p->membase + DW_UART_UCV);
 	if (!reg)
 		return;
 
 	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
-	reg = readl(p->membase + DW_UART_CPR);
+	reg = d->serial_in(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
@@ -390,9 +434,19 @@ static int dw8250_probe(struct platform_device *pdev)
 
 	err = device_property_read_u32(p->dev, "reg-io-width", &val);
 	if (!err && val == 4) {
-		p->iotype = UPIO_MEM32;
-		p->serial_in = dw8250_serial_in32;
-		p->serial_out = dw8250_serial_out32;
+		p->iotype = of_device_is_big_endian(p->dev->of_node) ?
+			    UPIO_MEM32BE : UPIO_MEM32;
+		if (p->iotype == UPIO_MEM32) {
+			p->serial_in = dw8250_serial_in32;
+			p->serial_out = dw8250_serial_out32;
+			data->serial_in = _dw8250_serial_in32;
+			data->serial_out = _dw8250_serial_out32;
+		} else {
+			p->serial_in = dw8250_serial_in32be;
+			p->serial_out = dw8250_serial_out32be;
+			data->serial_in = _dw8250_serial_in32be;
+			data->serial_out = _dw8250_serial_out32be;
+		}
 	}
 
 	if (device_property_read_bool(p->dev, "dcd-override")) {
@@ -466,6 +520,9 @@ static int dw8250_probe(struct platform_device *pdev)
 
 	dw8250_quirks(p, data);
 
+	data->serial_in = _dw8250_serial_in32;
+	data->serial_out = _dw8250_serial_out32;
+
 	/* If the Busy Functionality is not implemented, don't handle it */
 	if (data->uart_16550_compatible) {
 		p->serial_out = NULL;
-- 
1.7.1

Re: [PATCH-v8] serial: 8250_dw: Add support for big-endian MMIO accesses

[Heikki Krogerus]

Hi Noam,

On Sun, Oct 18, 2015 at 12:01:48PM +0300, Noam Camus wrote:
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index a0cdbf3..880f712 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -63,6 +63,9 @@ struct dw8250_data {
>  	struct clk		*pclk;
>  	struct reset_control	*rst;
>  	struct uart_8250_dma	dma;
> +	unsigned int		(*serial_in)(void __iomem *addr);
> +	void			(*serial_out)(unsigned int value,
> +					      void __iomem *addr);

This is really ideal ..

>  	unsigned int		skip_autocfg:1;
>  	unsigned int		uart_16550_compatible:1;
> @@ -159,9 +162,9 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
>  }
>  #endif /* CONFIG_64BIT */
>  
> -static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> +static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
>  {
> -	writel(value, p->membase + (offset << p->regshift));
> +	struct dw8250_data *d = p->private_data;
>  
>  	/* Make sure LCR write wasn't ignored */
>  	if (offset == UART_LCR) {
> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>  			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
>  				return;
>  			dw8250_force_idle(p);
> -			writel(value, p->membase + (UART_LCR << p->regshift));
> +			d->serial_out(value,
> +				      p->membase + (UART_LCR << p->regshift));
>  		}

.. The way I see it, this is the only place where you would really
need the new private serial_in/out hooks, so why not just check the
iotype here and based on that use writeb/writel/iowrite32be or what
ever ..

<snip>

>  static void dw8250_setup_port(struct uart_port *p)
>  {
>  	struct uart_8250_port *up = up_to_u8250p(p);
> +	struct dw8250_data *d = p->private_data;
>  	u32 reg;
>  
>  	/*
>  	 * If the Component Version Register returns zero, we know that
>  	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
>  	 */
> -	reg = readl(p->membase + DW_UART_UCV);
> +	reg = d->serial_in(p->membase + DW_UART_UCV);
>  	if (!reg)
>  		return;
>  
>  	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>  
> -	reg = readl(p->membase + DW_UART_CPR);
> +	reg = d->serial_in(p->membase + DW_UART_CPR);
>  	if (!reg)
>  		return;

.. Here you could as well replace the direct readl calls with
serial_port_in calls.

> @@ -390,9 +434,19 @@ static int dw8250_probe(struct platform_device *pdev)
>  
>  	err = device_property_read_u32(p->dev, "reg-io-width", &val);
>  	if (!err && val == 4) {
> -		p->iotype = UPIO_MEM32;
> -		p->serial_in = dw8250_serial_in32;
> -		p->serial_out = dw8250_serial_out32;
> +		p->iotype = of_device_is_big_endian(p->dev->of_node) ?
> +			    UPIO_MEM32BE : UPIO_MEM32;

If this has to be tied to DT, do this check in dw8250_quirks.

> +		if (p->iotype == UPIO_MEM32) {
> +			p->serial_in = dw8250_serial_in32;
> +			p->serial_out = dw8250_serial_out32;
> +			data->serial_in = _dw8250_serial_in32;
> +			data->serial_out = _dw8250_serial_out32;
> +		} else {
> +			p->serial_in = dw8250_serial_in32be;
> +			p->serial_out = dw8250_serial_out32be;
> +			data->serial_in = _dw8250_serial_in32be;
> +			data->serial_out = _dw8250_serial_out32be;
> +		}
>  	}
>  
>  	if (device_property_read_bool(p->dev, "dcd-override")) {
> @@ -466,6 +520,9 @@ static int dw8250_probe(struct platform_device *pdev)
>  
>  	dw8250_quirks(p, data);
>  
> +	data->serial_in = _dw8250_serial_in32;
> +	data->serial_out = _dw8250_serial_out32;

I don't think I understand what is going on here? You would never
actually even use _dw8250_serial_in32be/out32be, no?

Maybe I'm misunderstanding something.


Thanks,

-- 
heikki

Re: [PATCH-v8] serial: 8250_dw: Add support for big-endian MMIO accesses

[Heikki Krogerus]

On Wed, Dec 09, 2015 at 03:19:39PM +0200, Heikki Krogerus wrote:
> Hi Noam,
> 
> On Sun, Oct 18, 2015 at 12:01:48PM +0300, Noam Camus wrote:
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index a0cdbf3..880f712 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -63,6 +63,9 @@ struct dw8250_data {
> >  	struct clk		*pclk;
> >  	struct reset_control	*rst;
> >  	struct uart_8250_dma	dma;
> > +	unsigned int		(*serial_in)(void __iomem *addr);
> > +	void			(*serial_out)(unsigned int value,
> > +					      void __iomem *addr);
> 
> This is really ideal ..

Meant to say _not_ ideal. Sorry about that.

Cheers,

-- 
heikki

RE: [PATCH-v8] serial: 8250_dw: Add support for big-endian MMIO accesses

[Noam Camus]

>From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] 
>Sent: Wednesday, December 09, 2015 3:20 PM


>> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>>  			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
>>  				return;
>>  			dw8250_force_idle(p);
>> -			writel(value, p->membase + (UART_LCR << p->regshift));
>> +			d->serial_out(value,
>> +				      p->membase + (UART_LCR << p->regshift));
>>  		}

>.. The way I see it, this is the only place where you would really need the new private serial_in/out hooks, so why not just check the >iotype here and based on that use writeb/writel/iowrite32be or what ever ..

In previous versions (V2) Greg dis-liked using iotype here this is why I added the private serial_in/serial_out

>>  static void dw8250_setup_port(struct uart_port *p)  {
>>  	struct uart_8250_port *up = up_to_u8250p(p);
>> +	struct dw8250_data *d = p->private_data;
>>  	u32 reg;
>>  
>>  	/*
>>  	 * If the Component Version Register returns zero, we know that
>>  	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
>>  	 */
>> -	reg = readl(p->membase + DW_UART_UCV);
>> +	reg = d->serial_in(p->membase + DW_UART_UCV);
>>  	if (!reg)
>>  		return;
>>  
>>  	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
>>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>>  
>> -	reg = readl(p->membase + DW_UART_CPR);
>> +	reg = d->serial_in(p->membase + DW_UART_CPR);
>>  	if (!reg)
>>  		return;

>.. Here you could as well replace the direct readl calls with serial_port_in calls.
Again, if you meant here for using iotype it was rejected.

>> @@ -390,9 +434,19 @@ static int dw8250_probe(struct platform_device 
>> *pdev)
>>  
>>  	err = device_property_read_u32(p->dev, "reg-io-width", &val);
>>  	if (!err && val == 4) {
>> -		p->iotype = UPIO_MEM32;
>> -		p->serial_in = dw8250_serial_in32;
>> -		p->serial_out = dw8250_serial_out32;
>> +		p->iotype = of_device_is_big_endian(p->dev->of_node) ?
>> +			    UPIO_MEM32BE : UPIO_MEM32;

>If this has to be tied to DT, do this check in dw8250_quirks.
Thanks , I will move this to dw8250_quirks.

>>  	dw8250_quirks(p, data);
>>  
>> +	data->serial_in = _dw8250_serial_in32;
>> +	data->serial_out = _dw8250_serial_out32;

>I don't think I understand what is going on here? You would never actually even use _dw8250_serial_in32be/out32be, no?

>Maybe I'm misunderstanding something.
This is a default in case  where "reg-io-width != 4".
What is the case you see that they are not used? (_dw8250_serial_in32be is used from dw8250_setup_port just few lines below)

-Noam

Re: [PATCH-v8] serial: 8250_dw: Add support for big-endian MMIO accesses

[Heikki Krogerus]

Hi Noam,

On Thu, Dec 10, 2015 at 07:26:42AM +0000, Noam Camus wrote:
> >From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] 
> >Sent: Wednesday, December 09, 2015 3:20 PM
> 
> 
> >> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> >>  			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> >>  				return;
> >>  			dw8250_force_idle(p);
> >> -			writel(value, p->membase + (UART_LCR << p->regshift));
> >> +			d->serial_out(value,
> >> +				      p->membase + (UART_LCR << p->regshift));
> >>  		}
> 
> >.. The way I see it, this is the only place where you would really need the
> >new private serial_in/out hooks, so why not just check the >iotype here and
> >based on that use writeb/writel/iowrite32be or what ever ..
> 
> In previous versions (V2) Greg dis-liked using iotype here this is why I added
> the private serial_in/serial_out

I couldn't find that comment in the thread? All he said in his
comment for this was that you should use real if condition instead of
the ternary operator you had there (condition ? true : false).

Why would it not be acceptable? If it would really not be acceptable
(which I don't believe) then you should simply duplicate the lcr
checking to dw8250_seria_out32be like it is done now in
dw8250_serial_out and dw8250_serial_out32, but there still is no need
for the private serial_in/out hooks.

I'm attaching a diff that I think would work as a good starting point
for you.

> >>  static void dw8250_setup_port(struct uart_port *p)  {
> >>  	struct uart_8250_port *up = up_to_u8250p(p);
> >> +	struct dw8250_data *d = p->private_data;
> >>  	u32 reg;
> >>  
> >>  	/*
> >>  	 * If the Component Version Register returns zero, we know that
> >>  	 * ADDITIONAL_FEATURES are not enabled. No need to go any further.
> >>  	 */
> >> -	reg = readl(p->membase + DW_UART_UCV);
> >> +	reg = d->serial_in(p->membase + DW_UART_UCV);
> >>  	if (!reg)
> >>  		return;
> >>  
> >>  	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
> >>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
> >>  
> >> -	reg = readl(p->membase + DW_UART_CPR);
> >> +	reg = d->serial_in(p->membase + DW_UART_CPR);
> >>  	if (!reg)
> >>  		return;
> 
> >.. Here you could as well replace the direct readl calls with serial_port_in
> >calls.
> Again, if you meant here for using iotype it was rejected.

No, I mean instead of d->serial_in you could just use p->serial_in
here (which is the same as calling serial_port_in()).

> >> @@ -390,9 +434,19 @@ static int dw8250_probe(struct platform_device 
> >> *pdev)
> >>  
> >>  	err = device_property_read_u32(p->dev, "reg-io-width", &val);
> >>  	if (!err && val == 4) {
> >> -		p->iotype = UPIO_MEM32;
> >> -		p->serial_in = dw8250_serial_in32;
> >> -		p->serial_out = dw8250_serial_out32;
> >> +		p->iotype = of_device_is_big_endian(p->dev->of_node) ?
> >> +			    UPIO_MEM32BE : UPIO_MEM32;
> 
> >If this has to be tied to DT, do this check in dw8250_quirks.
> Thanks , I will move this to dw8250_quirks.
> 
> >>  	dw8250_quirks(p, data);
> >>  
> >> +	data->serial_in = _dw8250_serial_in32;
> >> +	data->serial_out = _dw8250_serial_out32;
> 
> >I don't think I understand what is going on here? You would never actually
> >even use _dw8250_serial_in32be/out32be, no?
> 
> >Maybe I'm misunderstanding something.
> This is a default in case  where "reg-io-width != 4".
> What is the case you see that they are not used? (_dw8250_serial_in32be is
> used from dw8250_setup_port just few lines below)

But dw8250_setup_port will call data->serial_in hook based on this
patch, which will now be pointing to _dw8250_serial_in32, not
_dw8250_serial_in32be?


Thanks,

-- 
heikki

Re: [PATCH-v8] serial: 8250_dw: Add support for big-endian MMIO accesses

[Heikki Krogerus]

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

On Thu, Dec 10, 2015 at 11:31:04AM +0200, Heikki Krogerus wrote:
> Hi Noam,
> 
> On Thu, Dec 10, 2015 at 07:26:42AM +0000, Noam Camus wrote:
> > >From: Heikki Krogerus [mailto:heikki.krogerus@linux.intel.com] 
> > >Sent: Wednesday, December 09, 2015 3:20 PM
> > 
> > 
> > >> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> > >>  			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> > >>  				return;
> > >>  			dw8250_force_idle(p);
> > >> -			writel(value, p->membase + (UART_LCR << p->regshift));
> > >> +			d->serial_out(value,
> > >> +				      p->membase + (UART_LCR << p->regshift));
> > >>  		}
> > 
> > >.. The way I see it, this is the only place where you would really need the
> > >new private serial_in/out hooks, so why not just check the >iotype here and
> > >based on that use writeb/writel/iowrite32be or what ever ..
> > 
> > In previous versions (V2) Greg dis-liked using iotype here this is why I added
> > the private serial_in/serial_out
> 
> I couldn't find that comment in the thread? All he said in his
> comment for this was that you should use real if condition instead of
> the ternary operator you had there (condition ? true : false).
> 
> Why would it not be acceptable? If it would really not be acceptable
> (which I don't believe) then you should simply duplicate the lcr
> checking to dw8250_seria_out32be like it is done now in
> dw8250_serial_out and dw8250_serial_out32, but there still is no need
> for the private serial_in/out hooks.
> 
> I'm attaching a diff that I think would work as a good starting point
> for you.

Now actually attaching the diff :).

-- 
heikki

[-- Attachment #2: dw8250_check_lcr.diff --]
[-- Type: text/plain, Size: 2912 bytes --]

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a5d319e..b2ea0c2 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -95,25 +95,39 @@ static void dw8250_force_idle(struct uart_port *p)
 	(void)p->serial_in(p, UART_RX);
 }
 
+static void dw8250_check_lcr(struct uart_port *p, int value)
+{
+	void __iomem *offset = p->membase + (UART_LCR << p->regshift);
+	int tries = 1000;
+
+	while (tries--) {
+		unsigned int lcr = p->serial_in(p, UART_LCR);
+
+		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
+			return;
+
+		dw8250_force_idle(p);
+
+		if (p->iotype == UPIO_MEM32)
+			writel(value, offset);
+		else
+			writeb(value, offset);
+	}
+	/*
+	 * FIXME: this deadlocks if port->lock is already held
+	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+	 */
+}
+
 static void dw8250_serial_out(struct uart_port *p, int offset, int value)
 {
+	struct dw8250_data *d = p->private_data;
+
 	writeb(value, p->membase + (offset << p->regshift));
 
 	/* Make sure LCR write wasn't ignored */
-	if (offset == UART_LCR) {
-		int tries = 1000;
-		while (tries--) {
-			unsigned int lcr = p->serial_in(p, UART_LCR);
-			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
-				return;
-			dw8250_force_idle(p);
-			writeb(value, p->membase + (UART_LCR << p->regshift));
-		}
-		/*
-		 * FIXME: this deadlocks if port->lock is already held
-		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
-		 */
-	}
+	if (offset == UART_LCR && !d->uart_16550_compatible)
+		dw8250_check_lcr(p, value);
 }
 
 static unsigned int dw8250_serial_in(struct uart_port *p, int offset)
@@ -161,23 +175,13 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 
 static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 {
+	struct dw8250_data *d = p->private_data;
+
 	writel(value, p->membase + (offset << p->regshift));
 
 	/* Make sure LCR write wasn't ignored */
-	if (offset == UART_LCR) {
-		int tries = 1000;
-		while (tries--) {
-			unsigned int lcr = p->serial_in(p, UART_LCR);
-			if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
-				return;
-			dw8250_force_idle(p);
-			writel(value, p->membase + (UART_LCR << p->regshift));
-		}
-		/*
-		 * FIXME: this deadlocks if port->lock is already held
-		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
-		 */
-	}
+	if (offset == UART_LCR && !d->uart_16550_compatible)
+		dw8250_check_lcr(p, value);
 }
 
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
@@ -463,10 +467,8 @@ static int dw8250_probe(struct platform_device *pdev)
 	dw8250_quirks(p, data);
 
 	/* If the Busy Functionality is not implemented, don't handle it */
-	if (data->uart_16550_compatible) {
-		p->serial_out = NULL;
+	if (data->uart_16550_compatible)
 		p->handle_irq = NULL;
-	}
 
 	if (!data->skip_autocfg)
 		dw8250_setup_port(p);