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

[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

[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);

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

[Heikki Krogerus]

Hi Noam,

On Thu, Dec 10, 2015 at 04:33:39PM +0000, Noam Camus wrote:
> Please see
> https://lkml.org/lkml/2015/8/3/806
> Why I added private accessors.

Greg is not saying anything about the iotype checking there? Looks
more like confusion about what exactly is that patch trying to
achieve. I think Greg just thought you moved the writel call from
the beginning of the function to be called later inside the "else"
condition.

You need to start your series with a patch where you just separate the
lcr checking to its own function and follow that with patches where
you introduce the big-endian support. I think this is also what Andy
told you. Use the diff I gave you.

One more thing that I forgot to comment before:

s/dw8250_check_LCR/dw8250_check_lcr/


Thanks,

-- 
heikki