[PATCH v9 0/3] serial: 8250_dw add big endian support

[PATCH v9 0/3] serial: 8250_dw add big endian support

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.

v9 change:
1) split patch into check_lcr routine and then add big endian feature
2) add new iotype support for BE 32 bit
3) make each call to readl/writel depend on iotype

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".

Noam Camus (3):
  serial: 8250_dw: Avoid serial_outx code duplicate with new
    dw8250_check_lcr()
  serial: 8250_dw: Add support for big-endian MMIO accesses
  serial: 8250_dw: Do not use readl/writel before checking port iotype

 drivers/tty/serial/8250/8250_dw.c |  126 ++++++++++++++++++++++---------------
 1 files changed, 75 insertions(+), 51 deletions(-)

[PATCH v9 0/3] serial: 8250_dw add big endian support

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Add support for UPIO_MEM32BE in addition to UPIO_MEM32.

v9 change:
1) split patch into check_lcr routine and then add big endian feature
2) add new iotype support for BE 32 bit
3) make each call to readl/writel depend on iotype

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".

Noam Camus (3):
  serial: 8250_dw: Avoid serial_outx code duplicate with new
    dw8250_check_lcr()
  serial: 8250_dw: Add support for big-endian MMIO accesses
  serial: 8250_dw: Do not use readl/writel before checking port iotype

 drivers/tty/serial/8250/8250_dw.c |  126 ++++++++++++++++++++++---------------
 1 files changed, 75 insertions(+), 51 deletions(-)

[PATCH v9 1/3] serial: 8250_dw: Avoid serial_outx code duplicate with new dw8250_check_lcr()

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

With the help of Heikki we take common code that
makes sure LCR write wasn't ignored and put it in new function called
dw8250_check_lcr(). This function serves 3 serial_out routines:
dw8250_serial_out(), dw8250_serial_out32(), and dw8250_serial_outq().

This patch only brings better code reuse.

Signed-off-by: Noam Camus <noamc@ezchip.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c |   91 +++++++++++++++++--------------------
 1 files changed, 42 insertions(+), 49 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a5d319e..ffe5e0c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -95,25 +95,43 @@ static void dw8250_force_idle(struct uart_port *p)
 	(void)p->serial_in(p, UART_RX);
 }
 
-static void dw8250_serial_out(struct uart_port *p, int offset, int value)
+static void dw8250_check_lcr(struct uart_port *p, int value)
 {
-	writeb(value, p->membase + (offset << p->regshift));
+	void __iomem *offset = p->membase + (UART_LCR << p->regshift);
+	int tries = 1000;
 
 	/* 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);
-		 */
+	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);
+
+#ifdef CONFIG_64BIT
+		__raw_writeq(value & 0xff, offset);
+#else
+		if (p->iotype == UPIO_MEM32)
+			writel(value, offset);
+		else
+			writeb(value, offset);
+#endif
 	}
+	/*
+	 * 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));
+
+	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)
@@ -135,49 +153,26 @@ static unsigned int dw8250_serial_inq(struct uart_port *p, int offset)
 
 static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 {
+	struct dw8250_data *d = p->private_data;
+
 	value &= 0xff;
 	__raw_writeq(value, p->membase + (offset << p->regshift));
 	/* Read back to ensure register write ordering. */
 	__raw_readq(p->membase + (UART_LCR << 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);
-			__raw_writeq(value & 0xff,
-				     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);
 }
 #endif /* CONFIG_64BIT */
 
 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 +458,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);
-- 
1.7.1

[PATCH v9 1/3] serial: 8250_dw: Avoid serial_outx code duplicate with new dw8250_check_lcr()

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

With the help of Heikki we take common code that
makes sure LCR write wasn't ignored and put it in new function called
dw8250_check_lcr(). This function serves 3 serial_out routines:
dw8250_serial_out(), dw8250_serial_out32(), and dw8250_serial_outq().

This patch only brings better code reuse.

Signed-off-by: Noam Camus <noamc@ezchip.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c |   91 +++++++++++++++++--------------------
 1 files changed, 42 insertions(+), 49 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a5d319e..ffe5e0c 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -95,25 +95,43 @@ static void dw8250_force_idle(struct uart_port *p)
 	(void)p->serial_in(p, UART_RX);
 }
 
-static void dw8250_serial_out(struct uart_port *p, int offset, int value)
+static void dw8250_check_lcr(struct uart_port *p, int value)
 {
-	writeb(value, p->membase + (offset << p->regshift));
+	void __iomem *offset = p->membase + (UART_LCR << p->regshift);
+	int tries = 1000;
 
 	/* 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);
-		 */
+	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);
+
+#ifdef CONFIG_64BIT
+		__raw_writeq(value & 0xff, offset);
+#else
+		if (p->iotype == UPIO_MEM32)
+			writel(value, offset);
+		else
+			writeb(value, offset);
+#endif
 	}
+	/*
+	 * 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));
+
+	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)
@@ -135,49 +153,26 @@ static unsigned int dw8250_serial_inq(struct uart_port *p, int offset)
 
 static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 {
+	struct dw8250_data *d = p->private_data;
+
 	value &= 0xff;
 	__raw_writeq(value, p->membase + (offset << p->regshift));
 	/* Read back to ensure register write ordering. */
 	__raw_readq(p->membase + (UART_LCR << 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);
-			__raw_writeq(value & 0xff,
-				     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);
 }
 #endif /* CONFIG_64BIT */
 
 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 +458,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);
-- 
1.7.1

[PATCH v9 2/3] 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.

For big endian we use 2 new accessors similar to little endian,
called dw8250_serial_out32be() and dw8250_serial_in32be().

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

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index ffe5e0c..92c4a9b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -182,6 +182,24 @@ 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)
+{
+	struct dw8250_data *d = p->private_data;
+
+	iowrite32be(value, p->membase + (offset << p->regshift));
+
+	if (offset == UART_LCR && !d->uart_16550_compatible)
+		dw8250_check_lcr(p, 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;
@@ -276,6 +294,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 			data->skip_autocfg = true;
 		}
 #endif
+		if (of_device_is_big_endian(p->dev->of_node)) {
+			p->iotype = UPIO_MEM32BE;
+			p->serial_in = dw8250_serial_in32be;
+			p->serial_out = dw8250_serial_out32be;
+		}
 	} else if (has_acpi_companion(p->dev)) {
 		p->iotype = UPIO_MEM32;
 		p->regshift = 2;
-- 
1.7.1

[PATCH v9 2/3] 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.

For big endian we use 2 new accessors similar to little endian,
called dw8250_serial_out32be() and dw8250_serial_in32be().

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

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index ffe5e0c..92c4a9b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -182,6 +182,24 @@ 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)
+{
+	struct dw8250_data *d = p->private_data;
+
+	iowrite32be(value, p->membase + (offset << p->regshift));
+
+	if (offset == UART_LCR && !d->uart_16550_compatible)
+		dw8250_check_lcr(p, 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;
@@ -276,6 +294,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 			data->skip_autocfg = true;
 		}
 #endif
+		if (of_device_is_big_endian(p->dev->of_node)) {
+			p->iotype = UPIO_MEM32BE;
+			p->serial_in = dw8250_serial_in32be;
+			p->serial_out = dw8250_serial_out32be;
+		}
 	} else if (has_acpi_companion(p->dev)) {
 		p->iotype = UPIO_MEM32;
 		p->regshift = 2;
-- 
1.7.1

[PATCH v9 3/3] serial: 8250_dw: Do not use readl/writel before checking port iotype

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Direct call to readl()/writel() is checked against iotype
and in case of UPIO_MEM32BE we use ioread32be()/iowrite32be()
instead of them.

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

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 92c4a9b..30810ac 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -114,6 +114,8 @@ static void dw8250_check_lcr(struct uart_port *p, int value)
 #else
 		if (p->iotype == UPIO_MEM32)
 			writel(value, offset);
+		else if (p->iotype == UPIO_MEM32BE)
+			iowrite32be(value, offset);
 		else
 			writeb(value, offset);
 #endif
@@ -327,14 +329,20 @@ static void dw8250_setup_port(struct uart_port *p)
 	 * 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);
+	if (p->iotype == UPIO_MEM32BE)
+		reg = ioread32be(p->membase + DW_UART_UCV);
+	else
+		reg = readl(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);
+	if (p->iotype == UPIO_MEM32BE)
+		reg = ioread32be(p->membase + DW_UART_CPR);
+	else
+		reg = readl(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
-- 
1.7.1

[PATCH v9 3/3] serial: 8250_dw: Do not use readl/writel before checking port iotype

[Noam Camus]

From: Noam Camus <noamc@ezchip.com>

Direct call to readl()/writel() is checked against iotype
and in case of UPIO_MEM32BE we use ioread32be()/iowrite32be()
instead of them.

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

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 92c4a9b..30810ac 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -114,6 +114,8 @@ static void dw8250_check_lcr(struct uart_port *p, int value)
 #else
 		if (p->iotype == UPIO_MEM32)
 			writel(value, offset);
+		else if (p->iotype == UPIO_MEM32BE)
+			iowrite32be(value, offset);
 		else
 			writeb(value, offset);
 #endif
@@ -327,14 +329,20 @@ static void dw8250_setup_port(struct uart_port *p)
 	 * 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);
+	if (p->iotype == UPIO_MEM32BE)
+		reg = ioread32be(p->membase + DW_UART_UCV);
+	else
+		reg = readl(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);
+	if (p->iotype == UPIO_MEM32BE)
+		reg = ioread32be(p->membase + DW_UART_CPR);
+	else
+		reg = readl(p->membase + DW_UART_CPR);
 	if (!reg)
 		return;
 
-- 
1.7.1

Re: [PATCH v9 1/3] serial: 8250_dw: Avoid serial_outx code duplicate with new dw8250_check_lcr()

[Andy Shevchenko]

On Sat, Dec 12, 2015 at 7:18 PM, Noam Camus <noamc@ezchip.com> wrote:
> From: Noam Camus <noamc@ezchip.com>
>
> With the help of Heikki we take common code that
> makes sure LCR write wasn't ignored and put it in new function called
> dw8250_check_lcr(). This function serves 3 serial_out routines:
> dw8250_serial_out(), dw8250_serial_out32(), and dw8250_serial_outq().
>
> This patch only brings better code reuse.
>
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

All three fine by me:
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> ---
>  drivers/tty/serial/8250/8250_dw.c |   91 +++++++++++++++++--------------------
>  1 files changed, 42 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index a5d319e..ffe5e0c 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -95,25 +95,43 @@ static void dw8250_force_idle(struct uart_port *p)
>         (void)p->serial_in(p, UART_RX);
>  }
>
> -static void dw8250_serial_out(struct uart_port *p, int offset, int value)
> +static void dw8250_check_lcr(struct uart_port *p, int value)
>  {
> -       writeb(value, p->membase + (offset << p->regshift));
> +       void __iomem *offset = p->membase + (UART_LCR << p->regshift);
> +       int tries = 1000;
>
>         /* 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);
> -                */
> +       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);
> +
> +#ifdef CONFIG_64BIT
> +               __raw_writeq(value & 0xff, offset);
> +#else
> +               if (p->iotype == UPIO_MEM32)
> +                       writel(value, offset);
> +               else
> +                       writeb(value, offset);
> +#endif
>         }
> +       /*
> +        * 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));
> +
> +       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)
> @@ -135,49 +153,26 @@ static unsigned int dw8250_serial_inq(struct uart_port *p, int offset)
>
>  static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
>  {
> +       struct dw8250_data *d = p->private_data;
> +
>         value &= 0xff;
>         __raw_writeq(value, p->membase + (offset << p->regshift));
>         /* Read back to ensure register write ordering. */
>         __raw_readq(p->membase + (UART_LCR << 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);
> -                       __raw_writeq(value & 0xff,
> -                                    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);
>  }
>  #endif /* CONFIG_64BIT */
>
>  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 +458,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);
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 1/3] serial: 8250_dw: Avoid serial_outx code duplicate with new dw8250_check_lcr()

[Heikki Krogerus]

On Sat, Dec 12, 2015 at 07:18:25PM +0200, Noam Camus wrote:
> From: Noam Camus <noamc@ezchip.com>
> 
> With the help of Heikki we take common code that
> makes sure LCR write wasn't ignored and put it in new function called
> dw8250_check_lcr(). This function serves 3 serial_out routines:
> dw8250_serial_out(), dw8250_serial_out32(), and dw8250_serial_outq().
> 
> This patch only brings better code reuse.
> 
> Signed-off-by: Noam Camus <noamc@ezchip.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>

For all tree:

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>


Thanks,

-- 
heikki