[PATCH] tty: samsung_tty: 32-bit access for TX/RX hold registers

[PATCH] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

Support 32-bit access for the TX/RX hold registers UTXH and URXH.

This is required for some newer SoCs.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
 drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 73f951d65b93..17d2ead7cfe2 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
 #define portaddrl(port, reg) \
 	((unsigned long *)(unsigned long)((port)->membase + (reg)))
 
-#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
+static unsigned int rd_reg(struct uart_port *port, int reg)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		return readb_relaxed(portaddr(port, reg));
+	case UPIO_MEM32:
+		return readl_relaxed(portaddr(port, reg));
+	default:
+		return 0;
+	}
+	return 0;
+}
+
 #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
 
-#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
+static void wr_reg(struct uart_port *port, int reg, int val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb_relaxed(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel_relaxed(val, portaddr(port, reg));
+		break;
+	}
+}
+
 #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
 
+static void write_buf(struct uart_port *port, int reg, int val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel(val, portaddr(port, reg));
+		break;
+	}
+}
+
 /* Byte-order aware bit setting/clearing functions. */
 
 static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
@@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 		fifocnt--;
 
 		uerstat = rd_regl(port, S3C2410_UERSTAT);
-		ch = rd_regb(port, S3C2410_URXH);
+		ch = rd_reg(port, S3C2410_URXH);
 
 		if (port->flags & UPF_CONS_FLOW) {
 			int txe = s3c24xx_serial_txempty_nofifo(port);
@@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (port->x_char) {
-		wr_regb(port, S3C2410_UTXH, port->x_char);
+		wr_reg(port, S3C2410_UTXH, port->x_char);
 		port->icount.tx++;
 		port->x_char = 0;
 		goto out;
@@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
 			break;
 
-		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
+		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		port->icount.tx++;
 		count--;
@@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 /* no modem control lines */
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
-	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
+	unsigned int umstat = rd_regl(port, S3C2410_UMSTAT);
 
 	if (umstat & S3C2410_UMSTAT_CTS)
 		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
@@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct s3c24xx_uart_port *ourport;
 	int index = probe_index;
-	int ret;
+	int ret, prop = 0;
 
 	if (np) {
 		ret = of_alias_get_id(np, "serial");
@@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 			dev_get_platdata(&pdev->dev) :
 			ourport->drv_data->def_cfg;
 
-	if (np)
+	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
 
+		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+			switch (prop) {
+			case 1:
+				ourport->port.iotype = UPIO_MEM;
+				break;
+			case 4:
+				ourport->port.iotype = UPIO_MEM32;
+				break;
+			default:
+				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
+						prop);
+				ret = -EINVAL;
+				break;
+			}
+		}
+	}
+
 	if (ourport->drv_data->fifosize[index])
 		ourport->port.fifosize = ourport->drv_data->fifosize[index];
 	else if (ourport->info->fifosize)
@@ -2185,7 +2237,7 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 	if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
 		return NO_POLL_CHAR;
 
-	return rd_regb(port, S3C2410_URXH);
+	return rd_reg(port, S3C2410_URXH);
 }
 
 static void s3c24xx_serial_put_poll_char(struct uart_port *port,
@@ -2200,7 +2252,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, c);
+	wr_reg(port, S3C2410_UTXH, c);
 }
 
 #endif /* CONFIG_CONSOLE_POLL */
@@ -2212,7 +2264,7 @@ s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, ch);
+	wr_reg(port, S3C2410_UTXH, ch);
 }
 
 static void
@@ -2612,7 +2664,7 @@ static void samsung_early_putc(struct uart_port *port, int c)
 	else
 		samsung_early_busyuart(port);
 
-	writeb(c, port->membase + S3C2410_UTXH);
+	write_buf(port, S3C2410_UTXH, c);
 }
 
 static void samsung_early_write(struct console *con, const char *s,
-- 
2.15.0.rc1

Re: [PATCH] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Greg Kroah-Hartman]

On Wed, Apr 01, 2020 at 05:27:20PM +0900, Hyunki Koo wrote:
> -	if (np)
> +	if (np) {
>  		of_property_read_u32(np,
>  			"samsung,uart-fifosize", &ourport->port.fifosize);
>  
> +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> +			switch (prop) {
> +			case 1:
> +				ourport->port.iotype = UPIO_MEM;
> +				break;
> +			case 4:
> +				ourport->port.iotype = UPIO_MEM32;
> +				break;
> +			default:
> +				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> +						prop);
> +				ret = -EINVAL;
> +				break;
> +			}
> +		}
> +	}
> +

Does this mean that reg-io-width is now a required property for all
samsung uarts?  Does this break older dts files?  Or should you
fall-back to the previous operation if the attribute is not there?

And please fix your email client, the headers were all messed up,
causing my initial response to be only sent to you :(

thanks,

greg k-h

Re: [PATCH] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Wed, Apr 01, 2020 at 10:55:48AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 01, 2020 at 05:27:20PM +0900, Hyunki Koo wrote:
> > -	if (np)
> > +	if (np) {
> >  		of_property_read_u32(np,
> >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> >  
> > +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> > +			switch (prop) {
> > +			case 1:
> > +				ourport->port.iotype = UPIO_MEM;
> > +				break;
> > +			case 4:
> > +				ourport->port.iotype = UPIO_MEM32;
> > +				break;
> > +			default:
> > +				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> > +						prop);
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> 
> Does this mean that reg-io-width is now a required property for all
> samsung uarts?  Does this break older dts files?  Or should you
> fall-back to the previous operation if the attribute is not there?

Yes, it looks like silently breaking all boards.  Since
of_property_read_u32() will return errno, the warning message won't be
printed and all register reads will fail (return 0).

This looks like not tested on real HW.

Best regards,
Krzysztof

RE: [PATCH] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On Wed, Apr 01, 2020 at 6:20:20PM +0900, Krzysztof Kozlowski
wrote:
> On Wed, Apr 01, 2020 at 10:55:48AM +0200, Greg Kroah-Hartman
> wrote:
> > On Wed, Apr 01, 2020 at 05:27:20PM +0900, Hyunki Koo wrote:
> > > -	if (np)
> > > +	if (np) {
> > >  		of_property_read_u32(np,
> > >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> > >
> > > +		if (of_property_read_u32(np, "reg-io-width", &prop) ==
> 0) {
> > > +			switch (prop) {
> > > +			case 1:
> > > +				ourport->port.iotype = UPIO_MEM;
> > > +				break;
> > > +			case 4:
> > > +				ourport->port.iotype = UPIO_MEM32;
> > > +				break;
> > > +			default:
> > > +				dev_warn(&pdev->dev, "unsupported
> reg-io-width (%d)\n",
> > > +						prop);
> > > +				ret = -EINVAL;
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > > +
> >
> > Does this mean that reg-io-width is now a required property for all
> > samsung uarts?  Does this break older dts files?  Or should you
> > fall-back to the previous operation if the attribute is not there?
> 
> Yes, it looks like silently breaking all boards.  Since
> of_property_read_u32() will return errno, the warning message won't be
> printed and all register reads will fail (return 0).
> 
> This looks like not tested on real HW.
> 
> Best regards,
> Krzysztof

[Hyunki Koo] 
reg-io-width =4 is required for Samsung uart
To do not break older dts files, I will set default value in else of of_property_read_u32 like below.
+		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+ ...
+		} else {
+			ourport->port.iotype = UPIO_MEM;
+		}

Re: [PATCH] tty: samsung_tty: 32-bit access for TX/RX hold registers

['Krzysztof Kozlowski']

On Thu, Apr 02, 2020 at 06:44:58PM +0900, Hyunki Koo wrote:
> On Wed, Apr 01, 2020 at 6:20:20PM +0900, Krzysztof Kozlowski
> wrote:
> > On Wed, Apr 01, 2020 at 10:55:48AM +0200, Greg Kroah-Hartman
> > wrote:
> > > On Wed, Apr 01, 2020 at 05:27:20PM +0900, Hyunki Koo wrote:
> > > > -	if (np)
> > > > +	if (np) {
> > > >  		of_property_read_u32(np,
> > > >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> > > >
> > > > +		if (of_property_read_u32(np, "reg-io-width", &prop) ==
> > 0) {
> > > > +			switch (prop) {
> > > > +			case 1:
> > > > +				ourport->port.iotype = UPIO_MEM;
> > > > +				break;
> > > > +			case 4:
> > > > +				ourport->port.iotype = UPIO_MEM32;
> > > > +				break;
> > > > +			default:
> > > > +				dev_warn(&pdev->dev, "unsupported
> > reg-io-width (%d)\n",
> > > > +						prop);
> > > > +				ret = -EINVAL;
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +
> > >
> > > Does this mean that reg-io-width is now a required property for all
> > > samsung uarts?  Does this break older dts files?  Or should you
> > > fall-back to the previous operation if the attribute is not there?
> > 
> > Yes, it looks like silently breaking all boards.  Since
> > of_property_read_u32() will return errno, the warning message won't be
> > printed and all register reads will fail (return 0).
> > 
> > This looks like not tested on real HW.
> > 
> > Best regards,
> > Krzysztof
> 
> [Hyunki Koo] 
> reg-io-width =4 is required for Samsung uart
> To do not break older dts files, I will set default value in else of of_property_read_u32 like below.
> +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> + ...
> +		} else {
> +			ourport->port.iotype = UPIO_MEM;
> +		}

Thanks. Also, please test your patch on available Exynos boards, e.g.
Odroid XU4 or HC1.

Best regards,
Krzysztof

[PATCH v2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

Support 32-bit access for the TX/RX hold registers UTXH and URXH.

This is required for some newer SoCs.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
 drivers/tty/serial/samsung_tty.c | 78 +++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 73f951d65b93..826d8c5846a6 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
 #define portaddrl(port, reg) \
 	((unsigned long *)(unsigned long)((port)->membase + (reg)))
 
-#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
+static unsigned int rd_reg(struct uart_port *port, int reg)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		return readb_relaxed(portaddr(port, reg));
+	case UPIO_MEM32:
+		return readl_relaxed(portaddr(port, reg));
+	default:
+		return 0;
+	}
+	return 0;
+}
+
 #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
 
-#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
+static void wr_reg(struct uart_port *port, int reg, int val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb_relaxed(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel_relaxed(val, portaddr(port, reg));
+		break;
+	}
+}
+
 #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
 
+static void write_buf(struct uart_port *port, int reg, int val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel(val, portaddr(port, reg));
+		break;
+	}
+}
+
 /* Byte-order aware bit setting/clearing functions. */
 
 static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
@@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 		fifocnt--;
 
 		uerstat = rd_regl(port, S3C2410_UERSTAT);
-		ch = rd_regb(port, S3C2410_URXH);
+		ch = rd_reg(port, S3C2410_URXH);
 
 		if (port->flags & UPF_CONS_FLOW) {
 			int txe = s3c24xx_serial_txempty_nofifo(port);
@@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (port->x_char) {
-		wr_regb(port, S3C2410_UTXH, port->x_char);
+		wr_reg(port, S3C2410_UTXH, port->x_char);
 		port->icount.tx++;
 		port->x_char = 0;
 		goto out;
@@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
 			break;
 
-		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
+		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		port->icount.tx++;
 		count--;
@@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 /* no modem control lines */
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
-	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
+	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
 
 	if (umstat & S3C2410_UMSTAT_CTS)
 		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
@@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct s3c24xx_uart_port *ourport;
 	int index = probe_index;
-	int ret;
+	int ret, prop = 0;
 
 	if (np) {
 		ret = of_alias_get_id(np, "serial");
@@ -2000,10 +2035,29 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 			dev_get_platdata(&pdev->dev) :
 			ourport->drv_data->def_cfg;
 
-	if (np)
+	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
 
+		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+			switch (prop) {
+			case 1:
+				ourport->port.iotype = UPIO_MEM;
+				break;
+			case 4:
+				ourport->port.iotype = UPIO_MEM32;
+				break;
+			default:
+				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
+						prop);
+				ret = -EINVAL;
+				break;
+			}
+		} else {
+			ourport->port.iotype = UPIO_MEM;
+		}
+	}
+
 	if (ourport->drv_data->fifosize[index])
 		ourport->port.fifosize = ourport->drv_data->fifosize[index];
 	else if (ourport->info->fifosize)
@@ -2185,7 +2239,7 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 	if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
 		return NO_POLL_CHAR;
 
-	return rd_regb(port, S3C2410_URXH);
+	return rd_reg(port, S3C2410_URXH);
 }
 
 static void s3c24xx_serial_put_poll_char(struct uart_port *port,
@@ -2200,7 +2254,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, c);
+	wr_reg(port, S3C2410_UTXH, c);
 }
 
 #endif /* CONFIG_CONSOLE_POLL */
@@ -2212,7 +2266,7 @@ s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, ch);
+	wr_reg(port, S3C2410_UTXH, ch);
 }
 
 static void
@@ -2612,7 +2666,7 @@ static void samsung_early_putc(struct uart_port *port, int c)
 	else
 		samsung_early_busyuart(port);
 
-	writeb(c, port->membase + S3C2410_UTXH);
+	write_buf(port, S3C2410_UTXH, c);
 }
 
 static void samsung_early_write(struct console *con, const char *s,
-- 
2.15.0.rc1

Re: [PATCH v2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Greg KH]

On Thu, Apr 02, 2020 at 08:04:29PM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 78 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 66 insertions(+), 12 deletions(-)

What changed from v1?  Always put that under the --- line, as documented
to do so.

Please make a v3 with that information.

thanks,

greg k-h

Re: [PATCH v2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Thu, Apr 02, 2020 at 08:04:29PM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
>  drivers/tty/serial/samsung_tty.c | 78 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 66 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 73f951d65b93..826d8c5846a6 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
>  #define portaddrl(port, reg) \
>  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
>  
> -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> +static unsigned int rd_reg(struct uart_port *port, int reg)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		return readb_relaxed(portaddr(port, reg));
> +	case UPIO_MEM32:
> +		return readl_relaxed(portaddr(port, reg));
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
>  
> -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> +static void wr_reg(struct uart_port *port, int reg, int val)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb_relaxed(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel_relaxed(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
>  
> +static void write_buf(struct uart_port *port, int reg, int val)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  /* Byte-order aware bit setting/clearing functions. */
>  
>  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>  		fifocnt--;
>  
>  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> -		ch = rd_regb(port, S3C2410_URXH);
> +		ch = rd_reg(port, S3C2410_URXH);
>  
>  		if (port->flags & UPF_CONS_FLOW) {
>  			int txe = s3c24xx_serial_txempty_nofifo(port);
> @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>  	}
>  
>  	if (port->x_char) {
> -		wr_regb(port, S3C2410_UTXH, port->x_char);
> +		wr_reg(port, S3C2410_UTXH, port->x_char);
>  		port->icount.tx++;
>  		port->x_char = 0;
>  		goto out;
> @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
>  			break;
>  
> -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
>  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>  		port->icount.tx++;
>  		count--;
> @@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>  /* no modem control lines */
>  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>  {
> -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
>  
>  	if (umstat & S3C2410_UMSTAT_CTS)
>  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> @@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	struct s3c24xx_uart_port *ourport;
>  	int index = probe_index;
> -	int ret;
> +	int ret, prop = 0;
>  
>  	if (np) {
>  		ret = of_alias_get_id(np, "serial");
> @@ -2000,10 +2035,29 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  			dev_get_platdata(&pdev->dev) :
>  			ourport->drv_data->def_cfg;
>  
> -	if (np)
> +	if (np) {
>  		of_property_read_u32(np,
>  			"samsung,uart-fifosize", &ourport->port.fifosize);
>  
> +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> +			switch (prop) {
> +			case 1:
> +				ourport->port.iotype = UPIO_MEM;
> +				break;
> +			case 4:
> +				ourport->port.iotype = UPIO_MEM32;
> +				break;
> +			default:
> +				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> +						prop);
> +				ret = -EINVAL;
> +				break;
> +			}
> +		} else {
> +			ourport->port.iotype = UPIO_MEM;
> +		}
> +	}

I think this still breaks all non-DT platforms (e.g. s3c).

Best regards,
Krzysztof

RE: [PATCH v2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On Thu, Apr 02, 2020 at 10:59:29PM +0900, Krzysztof Kozlowski
> On Thu, Apr 02, 2020 at 08:04:29PM +0900, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> >
> > This is required for some newer SoCs.
> >
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > ---
> >  drivers/tty/serial/samsung_tty.c | 78
> > +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 73f951d65b93..826d8c5846a6 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {  #define
> > portaddrl(port, reg) \
> >  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
> >
> > -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> > +static unsigned int rd_reg(struct uart_port *port, int reg) {
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		return readb_relaxed(portaddr(port, reg));
> > +	case UPIO_MEM32:
> > +		return readl_relaxed(portaddr(port, reg));
> > +	default:
> > +		return 0;
> > +	}
> > +	return 0;
> > +}
> > +
> >  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
> >
> > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port,
> > reg))
> > +static void wr_reg(struct uart_port *port, int reg, int val) {
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port,
> > reg))
> >
> > +static void write_buf(struct uart_port *port, int reg, int val) {
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  /* Byte-order aware bit setting/clearing functions. */
> >
> >  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> > @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct
> s3c24xx_uart_port *ourport)
> >  		fifocnt--;
> >
> >  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> > -		ch = rd_regb(port, S3C2410_URXH);
> > +		ch = rd_reg(port, S3C2410_URXH);
> >
> >  		if (port->flags & UPF_CONS_FLOW) {
> >  			int txe = s3c24xx_serial_txempty_nofifo(port);
> > @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int
> irq, void *id)
> >  	}
> >
> >  	if (port->x_char) {
> > -		wr_regb(port, S3C2410_UTXH, port->x_char);
> > +		wr_reg(port, S3C2410_UTXH, port->x_char);
> >  		port->icount.tx++;
> >  		port->x_char = 0;
> >  		goto out;
> > @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int
> irq, void *id)
> >  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info-
> >tx_fifofull)
> >  			break;
> >
> > -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> >  		port->icount.tx++;
> >  		count--;
> > @@ -916,7 +951,7 @@ static unsigned int
> s3c24xx_serial_tx_empty(struct
> > uart_port *port)
> >  /* no modem control lines */
> >  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
> > {
> > -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> > +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> >
> >  	if (umstat & S3C2410_UMSTAT_CTS)
> >  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS; @@ -
> 1974,7 +2009,7 @@
> > static int s3c24xx_serial_probe(struct platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct s3c24xx_uart_port *ourport;
> >  	int index = probe_index;
> > -	int ret;
> > +	int ret, prop = 0;
> >
> >  	if (np) {
> >  		ret = of_alias_get_id(np, "serial"); @@ -2000,10
> +2035,29 @@ static
> > int s3c24xx_serial_probe(struct platform_device *pdev)
> >  			dev_get_platdata(&pdev->dev) :
> >  			ourport->drv_data->def_cfg;
> >
> > -	if (np)
> > +	if (np) {
> >  		of_property_read_u32(np,
> >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> >
> > +		if (of_property_read_u32(np, "reg-io-width", &prop) ==
> 0) {
> > +			switch (prop) {
> > +			case 1:
> > +				ourport->port.iotype = UPIO_MEM;
> > +				break;
> > +			case 4:
> > +				ourport->port.iotype = UPIO_MEM32;
> > +				break;
> > +			default:
> > +				dev_warn(&pdev->dev, "unsupported
> reg-io-width (%d)\n",
> > +						prop);
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +		} else {
> > +			ourport->port.iotype = UPIO_MEM;
> > +		}
> > +	}
> 
> I think this still breaks all non-DT platforms (e.g. s3c).
> 
> Best regards,
> Krzysztof

Thank you for your comment.
I  hope ourport->port.iotype  is initialized by below table for non-DT platforms

1662 static struct s3c24xx_uart_port                                         
1663 s3c24xx_serial_ports[CONFIG_SERIAL_SAMSUNG_UARTS] = {                   
1664         [0] = {                                                         
1665                 .port = {                                               
1666                         .lock           = __PORT_LOCK_UNLOCKED(0),      
1667                         .iotype         = UPIO_MEM,                     
1668                         .uartclk        = 0,                            
1669                         .fifosize       = 16,                           
1670                         .ops            = &s3c24xx_serial_ops,          
1671                         .flags          = UPF_BOOT_AUTOCONF,            
1672                         .line           = 0,                            
1673                 }                                                       
1674         },                                                              
1675         [1] = {                                                         
1676                 .port = {                                               
1677                         .lock           = __PORT_LOCK_UNLOCKED(1),      
1678                         .iotype         = UPIO_MEM,                     
1679                         .uartclk        = 0,                            
1680                         .fifosize       = 16,                           
1681                         .ops            = &s3c24xx_serial_ops,          
1682                         .flags          = UPF_BOOT_AUTOCONF,            
1683                         .line           = 1,                            
1684                 }                                                       
1685         },

Re: [PATCH v2] tty: samsung_tty: 32-bit access for TX/RX hold registers

['Krzysztof Kozlowski']

On Fri, Apr 03, 2020 at 04:30:38PM +0900, Hyunki Koo wrote:
> On Thu, Apr 02, 2020 at 10:59:29PM +0900, Krzysztof Kozlowski
> > On Thu, Apr 02, 2020 at 08:04:29PM +0900, Hyunki Koo wrote:
> > > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> > >
> > > This is required for some newer SoCs.
> > >
> > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > > ---
> > >  drivers/tty/serial/samsung_tty.c | 78
> > > +++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 66 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/samsung_tty.c
> > > b/drivers/tty/serial/samsung_tty.c
> > > index 73f951d65b93..826d8c5846a6 100644
> > > --- a/drivers/tty/serial/samsung_tty.c
> > > +++ b/drivers/tty/serial/samsung_tty.c
> > > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {  #define
> > > portaddrl(port, reg) \
> > >  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
> > >
> > > -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> > > +static unsigned int rd_reg(struct uart_port *port, int reg) {
> > > +	switch (port->iotype) {
> > > +	case UPIO_MEM:
> > > +		return readb_relaxed(portaddr(port, reg));
> > > +	case UPIO_MEM32:
> > > +		return readl_relaxed(portaddr(port, reg));
> > > +	default:
> > > +		return 0;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > >  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
> > >
> > > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port,
> > > reg))
> > > +static void wr_reg(struct uart_port *port, int reg, int val) {
> > > +	switch (port->iotype) {
> > > +	case UPIO_MEM:
> > > +		writeb_relaxed(val, portaddr(port, reg));
> > > +		break;
> > > +	case UPIO_MEM32:
> > > +		writel_relaxed(val, portaddr(port, reg));
> > > +		break;
> > > +	}
> > > +}
> > > +
> > >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port,
> > > reg))
> > >
> > > +static void write_buf(struct uart_port *port, int reg, int val) {
> > > +	switch (port->iotype) {
> > > +	case UPIO_MEM:
> > > +		writeb(val, portaddr(port, reg));
> > > +		break;
> > > +	case UPIO_MEM32:
> > > +		writel(val, portaddr(port, reg));
> > > +		break;
> > > +	}
> > > +}
> > > +
> > >  /* Byte-order aware bit setting/clearing functions. */
> > >
> > >  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> > > @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct
> > s3c24xx_uart_port *ourport)
> > >  		fifocnt--;
> > >
> > >  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> > > -		ch = rd_regb(port, S3C2410_URXH);
> > > +		ch = rd_reg(port, S3C2410_URXH);
> > >
> > >  		if (port->flags & UPF_CONS_FLOW) {
> > >  			int txe = s3c24xx_serial_txempty_nofifo(port);
> > > @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int
> > irq, void *id)
> > >  	}
> > >
> > >  	if (port->x_char) {
> > > -		wr_regb(port, S3C2410_UTXH, port->x_char);
> > > +		wr_reg(port, S3C2410_UTXH, port->x_char);
> > >  		port->icount.tx++;
> > >  		port->x_char = 0;
> > >  		goto out;
> > > @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int
> > irq, void *id)
> > >  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info-
> > >tx_fifofull)
> > >  			break;
> > >
> > > -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > > +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > >  		port->icount.tx++;
> > >  		count--;
> > > @@ -916,7 +951,7 @@ static unsigned int
> > s3c24xx_serial_tx_empty(struct
> > > uart_port *port)
> > >  /* no modem control lines */
> > >  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
> > > {
> > > -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> > > +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> > >
> > >  	if (umstat & S3C2410_UMSTAT_CTS)
> > >  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS; @@ -
> > 1974,7 +2009,7 @@
> > > static int s3c24xx_serial_probe(struct platform_device *pdev)
> > >  	struct device_node *np = pdev->dev.of_node;
> > >  	struct s3c24xx_uart_port *ourport;
> > >  	int index = probe_index;
> > > -	int ret;
> > > +	int ret, prop = 0;
> > >
> > >  	if (np) {
> > >  		ret = of_alias_get_id(np, "serial"); @@ -2000,10
> > +2035,29 @@ static
> > > int s3c24xx_serial_probe(struct platform_device *pdev)
> > >  			dev_get_platdata(&pdev->dev) :
> > >  			ourport->drv_data->def_cfg;
> > >
> > > -	if (np)
> > > +	if (np) {
> > >  		of_property_read_u32(np,
> > >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> > >
> > > +		if (of_property_read_u32(np, "reg-io-width", &prop) ==
> > 0) {
> > > +			switch (prop) {
> > > +			case 1:
> > > +				ourport->port.iotype = UPIO_MEM;
> > > +				break;
> > > +			case 4:
> > > +				ourport->port.iotype = UPIO_MEM32;
> > > +				break;
> > > +			default:
> > > +				dev_warn(&pdev->dev, "unsupported
> > reg-io-width (%d)\n",
> > > +						prop);
> > > +				ret = -EINVAL;
> > > +				break;
> > > +			}
> > > +		} else {
> > > +			ourport->port.iotype = UPIO_MEM;
> > > +		}
> > > +	}
> > 
> > I think this still breaks all non-DT platforms (e.g. s3c).
> > 
> > Best regards,
> > Krzysztof
> 
> Thank you for your comment.
> I  hope ourport->port.iotype  is initialized by below table for non-DT platforms

Indeed, you're right. In this case, this else() you added is not needed.
The default value for non-DT and existing DT platforms will be the same
(UPIO_MEM).

Best regards,
Krzysztof

RE: [PATCH v2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On Thu, Apr 02, 2020 at 4:51:29PM +0900, Krzysztof Kozlowski
> On Fri, Apr 03, 2020 at 04:30:38PM +0900, Hyunki Koo wrote:
> > On Thu, Apr 02, 2020 at 10:59:29PM +0900, Krzysztof Kozlowski
> > > On Thu, Apr 02, 2020 at 08:04:29PM +0900, Hyunki Koo wrote:
> > > > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> > > >
> > > > This is required for some newer SoCs.
> > > >
> > > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > > > ---
> > > >  drivers/tty/serial/samsung_tty.c | 78
> > > > +++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 66 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/samsung_tty.c
> > > > b/drivers/tty/serial/samsung_tty.c
> > > > index 73f951d65b93..826d8c5846a6 100644
> > > > --- a/drivers/tty/serial/samsung_tty.c
> > > > +++ b/drivers/tty/serial/samsung_tty.c
> > > > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {  #define
> > > > portaddrl(port, reg) \
> > > >  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
> > > >
> > > > -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> > > > +static unsigned int rd_reg(struct uart_port *port, int reg) {
> > > > +	switch (port->iotype) {
> > > > +	case UPIO_MEM:
> > > > +		return readb_relaxed(portaddr(port, reg));
> > > > +	case UPIO_MEM32:
> > > > +		return readl_relaxed(portaddr(port, reg));
> > > > +	default:
> > > > +		return 0;
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
> > > >
> > > > -#define wr_regb(port, reg, val) writeb_relaxed(val,
> > > > portaddr(port,
> > > > reg))
> > > > +static void wr_reg(struct uart_port *port, int reg, int val) {
> > > > +	switch (port->iotype) {
> > > > +	case UPIO_MEM:
> > > > +		writeb_relaxed(val, portaddr(port, reg));
> > > > +		break;
> > > > +	case UPIO_MEM32:
> > > > +		writel_relaxed(val, portaddr(port, reg));
> > > > +		break;
> > > > +	}
> > > > +}
> > > > +
> > > >  #define wr_regl(port, reg, val) writel_relaxed(val,
> > > > portaddr(port,
> > > > reg))
> > > >
> > > > +static void write_buf(struct uart_port *port, int reg, int val) {
> > > > +	switch (port->iotype) {
> > > > +	case UPIO_MEM:
> > > > +		writeb(val, portaddr(port, reg));
> > > > +		break;
> > > > +	case UPIO_MEM32:
> > > > +		writel(val, portaddr(port, reg));
> > > > +		break;
> > > > +	}
> > > > +}
> > > > +
> > > >  /* Byte-order aware bit setting/clearing functions. */
> > > >
> > > >  static inline void s3c24xx_set_bit(struct uart_port *port, int
> > > > idx, @@ -714,7 +749,7 @@ static void
> > > > s3c24xx_serial_rx_drain_fifo(struct
> > > s3c24xx_uart_port *ourport)
> > > >  		fifocnt--;
> > > >
> > > >  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> > > > -		ch = rd_regb(port, S3C2410_URXH);
> > > > +		ch = rd_reg(port, S3C2410_URXH);
> > > >
> > > >  		if (port->flags & UPF_CONS_FLOW) {
> > > >  			int txe = s3c24xx_serial_txempty_nofifo(port);
> > > > @@ -826,7 +861,7 @@ static irqreturn_t
> s3c24xx_serial_tx_chars(int
> > > irq, void *id)
> > > >  	}
> > > >
> > > >  	if (port->x_char) {
> > > > -		wr_regb(port, S3C2410_UTXH, port->x_char);
> > > > +		wr_reg(port, S3C2410_UTXH, port->x_char);
> > > >  		port->icount.tx++;
> > > >  		port->x_char = 0;
> > > >  		goto out;
> > > > @@ -852,7 +887,7 @@ static irqreturn_t
> s3c24xx_serial_tx_chars(int
> > > irq, void *id)
> > > >  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info-
> > > >tx_fifofull)
> > > >  			break;
> > > >
> > > > -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > > > +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > > >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > > >  		port->icount.tx++;
> > > >  		count--;
> > > > @@ -916,7 +951,7 @@ static unsigned int
> > > s3c24xx_serial_tx_empty(struct
> > > > uart_port *port)
> > > >  /* no modem control lines */
> > > >  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port
> > > > *port) {
> > > > -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> > > > +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> > > >
> > > >  	if (umstat & S3C2410_UMSTAT_CTS)
> > > >  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS; @@ -
> > > 1974,7 +2009,7 @@
> > > > static int s3c24xx_serial_probe(struct platform_device *pdev)
> > > >  	struct device_node *np = pdev->dev.of_node;
> > > >  	struct s3c24xx_uart_port *ourport;
> > > >  	int index = probe_index;
> > > > -	int ret;
> > > > +	int ret, prop = 0;
> > > >
> > > >  	if (np) {
> > > >  		ret = of_alias_get_id(np, "serial"); @@ -2000,10
> > > +2035,29 @@ static
> > > > int s3c24xx_serial_probe(struct platform_device *pdev)
> > > >  			dev_get_platdata(&pdev->dev) :
> > > >  			ourport->drv_data->def_cfg;
> > > >
> > > > -	if (np)
> > > > +	if (np) {
> > > >  		of_property_read_u32(np,
> > > >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> > > >
> > > > +		if (of_property_read_u32(np, "reg-io-width", &prop) ==
> > > 0) {
> > > > +			switch (prop) {
> > > > +			case 1:
> > > > +				ourport->port.iotype = UPIO_MEM;
> > > > +				break;
> > > > +			case 4:
> > > > +				ourport->port.iotype = UPIO_MEM32;
> > > > +				break;
> > > > +			default:
> > > > +				dev_warn(&pdev->dev, "unsupported
> > > reg-io-width (%d)\n",
> > > > +						prop);
> > > > +				ret = -EINVAL;
> > > > +				break;
> > > > +			}
> > > > +		} else {
> > > > +			ourport->port.iotype = UPIO_MEM;
> > > > +		}
> > > > +	}
> > >
> > > I think this still breaks all non-DT platforms (e.g. s3c).
> > >
> > > Best regards,
> > > Krzysztof
> >
> > Thank you for your comment.
> > I  hope ourport->port.iotype  is initialized by below table for non-DT
> > platforms
> 
> Indeed, you're right. In this case, this else() you added is not needed.
> The default value for non-DT and existing DT platforms will be the same
> (UPIO_MEM).
> 
> Best regards,
> Krzysztof

Thank you  for your comment.
I will remove  this line also in v3
+		} else {
+			ourport->port.iotype = UPIO_MEM;

[PATCH v3] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

Support 32-bit access for the TX/RX hold registers UTXH and URXH.

This is required for some newer SoCs.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
v3: change rd_regl to rd_reg in line 954 for backward compatibility.
---

 drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 73f951d65b93..a674a80163ed 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
 #define portaddrl(port, reg) \
 	((unsigned long *)(unsigned long)((port)->membase + (reg)))
 
-#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
+static unsigned int rd_reg(struct uart_port *port, int reg)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		return readb_relaxed(portaddr(port, reg));
+	case UPIO_MEM32:
+		return readl_relaxed(portaddr(port, reg));
+	default:
+		return 0;
+	}
+	return 0;
+}
+
 #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
 
-#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
+static void wr_reg(struct uart_port *port, int reg, int val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb_relaxed(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel_relaxed(val, portaddr(port, reg));
+		break;
+	}
+}
+
 #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
 
+static void write_buf(struct uart_port *port, int reg, int val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel(val, portaddr(port, reg));
+		break;
+	}
+}
+
 /* Byte-order aware bit setting/clearing functions. */
 
 static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
@@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 		fifocnt--;
 
 		uerstat = rd_regl(port, S3C2410_UERSTAT);
-		ch = rd_regb(port, S3C2410_URXH);
+		ch = rd_reg(port, S3C2410_URXH);
 
 		if (port->flags & UPF_CONS_FLOW) {
 			int txe = s3c24xx_serial_txempty_nofifo(port);
@@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (port->x_char) {
-		wr_regb(port, S3C2410_UTXH, port->x_char);
+		wr_reg(port, S3C2410_UTXH, port->x_char);
 		port->icount.tx++;
 		port->x_char = 0;
 		goto out;
@@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
 			break;
 
-		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
+		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		port->icount.tx++;
 		count--;
@@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 /* no modem control lines */
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
-	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
+	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
 
 	if (umstat & S3C2410_UMSTAT_CTS)
 		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
@@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct s3c24xx_uart_port *ourport;
 	int index = probe_index;
-	int ret;
+	int ret, prop = 0;
 
 	if (np) {
 		ret = of_alias_get_id(np, "serial");
@@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 			dev_get_platdata(&pdev->dev) :
 			ourport->drv_data->def_cfg;
 
-	if (np)
+	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
 
+		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+			switch (prop) {
+			case 1:
+				ourport->port.iotype = UPIO_MEM;
+				break;
+			case 4:
+				ourport->port.iotype = UPIO_MEM32;
+				break;
+			default:
+				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
+						prop);
+				ret = -EINVAL;
+				break;
+			}
+		}
+	}
+
 	if (ourport->drv_data->fifosize[index])
 		ourport->port.fifosize = ourport->drv_data->fifosize[index];
 	else if (ourport->info->fifosize)
@@ -2185,7 +2237,7 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 	if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
 		return NO_POLL_CHAR;
 
-	return rd_regb(port, S3C2410_URXH);
+	return rd_reg(port, S3C2410_URXH);
 }
 
 static void s3c24xx_serial_put_poll_char(struct uart_port *port,
@@ -2200,7 +2252,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, c);
+	wr_reg(port, S3C2410_UTXH, c);
 }
 
 #endif /* CONFIG_CONSOLE_POLL */
@@ -2212,7 +2264,7 @@ s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, ch);
+	wr_reg(port, S3C2410_UTXH, ch);
 }
 
 static void
@@ -2612,7 +2664,7 @@ static void samsung_early_putc(struct uart_port *port, int c)
 	else
 		samsung_early_busyuart(port);
 
-	writeb(c, port->membase + S3C2410_UTXH);
+	write_buf(port, S3C2410_UTXH, c);
 }
 
 static void samsung_early_write(struct console *con, const char *s,
-- 
2.15.0.rc1

Re: [PATCH v3] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Fri, Apr 03, 2020 at 07:20:41PM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v3: change rd_regl to rd_reg in line 954 for backward compatibility.

I cannot find this change against v2.

> ---
> 
>  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 73f951d65b93..a674a80163ed 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
>  #define portaddrl(port, reg) \
>  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
>  
> -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> +static unsigned int rd_reg(struct uart_port *port, int reg)

You should return here u32 to be consistent with readl_relaxed.

> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		return readb_relaxed(portaddr(port, reg));
> +	case UPIO_MEM32:
> +		return readl_relaxed(portaddr(port, reg));
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
>  
> -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> +static void wr_reg(struct uart_port *port, int reg, int val)

val should be u32.

> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb_relaxed(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel_relaxed(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
>  
> +static void write_buf(struct uart_port *port, int reg, int val)

buf is misleading, you do not write here any buffer. Maybe
"wr_reg_barrier()" or "wr_reg_order()"?

Best regards,
Krzysztof

RE: [PATCH v3] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On Thu, Apr 02, 2020 at 7:48:38PM +0900, Krzysztof Kozlowski
> On Fri, Apr 03, 2020 at 07:20:41PM +0900, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> >
> > This is required for some newer SoCs.
> >
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > ---
> > v3: change rd_regl to rd_reg in line 954 for backward compatibility.
> 
> I cannot find this change against v2.
Okay, I will add all changes.
> 
> > ---
> >
> >  drivers/tty/serial/samsung_tty.c | 76
> > +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 64 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 73f951d65b93..a674a80163ed 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {  #define
> > portaddrl(port, reg) \
> >  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
> >
> > -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> > +static unsigned int rd_reg(struct uart_port *port, int reg)
> 
> You should return here u32 to be consistent with readl_relaxed.
> 
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		return readb_relaxed(portaddr(port, reg));
> > +	case UPIO_MEM32:
> > +		return readl_relaxed(portaddr(port, reg));
> > +	default:
> > +		return 0;
> > +	}
> > +	return 0;
> > +}
> > +
> >  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
> >
> > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port,
> > reg))
> > +static void wr_reg(struct uart_port *port, int reg, int val)
> 
> val should be u32.
Okay, I will apply it.
> 
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port,
> > reg))
> >
> > +static void write_buf(struct uart_port *port, int reg, int val)
> 
> buf is misleading, you do not write here any buffer. Maybe
> "wr_reg_barrier()" or "wr_reg_order()"?
Okay, wr_reg_barrier  would be good, I will apply it.
> 
> Best regards,
> Krzysztof

[PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

Support 32-bit access for the TX/RX hold registers UTXH and URXH.

This is required for some newer SoCs.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
v2: 
line 954 : change rd_regl to rd_reg in for backward compatibility.
line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
v3:
line 2031: remove redundant init value  for ourport->port.iotype 
v4:
correct variable types and change misleading function name
---
 drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 73f951d65b93..bdf1d4d12cb1 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
 #define portaddrl(port, reg) \
 	((unsigned long *)(unsigned long)((port)->membase + (reg)))
 
-#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
+static u32 rd_reg(struct uart_port *port, u32 reg)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		return readb_relaxed(portaddr(port, reg));
+	case UPIO_MEM32:
+		return readl_relaxed(portaddr(port, reg));
+	default:
+		return 0;
+	}
+	return 0;
+}
+
 #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
 
-#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
+static void wr_reg(struct uart_port *port, u32 reg, u32 val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb_relaxed(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel_relaxed(val, portaddr(port, reg));
+		break;
+	}
+}
+
 #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
 
+static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel(val, portaddr(port, reg));
+		break;
+	}
+}
+
 /* Byte-order aware bit setting/clearing functions. */
 
 static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
@@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 		fifocnt--;
 
 		uerstat = rd_regl(port, S3C2410_UERSTAT);
-		ch = rd_regb(port, S3C2410_URXH);
+		ch = rd_reg(port, S3C2410_URXH);
 
 		if (port->flags & UPF_CONS_FLOW) {
 			int txe = s3c24xx_serial_txempty_nofifo(port);
@@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (port->x_char) {
-		wr_regb(port, S3C2410_UTXH, port->x_char);
+		wr_reg(port, S3C2410_UTXH, port->x_char);
 		port->icount.tx++;
 		port->x_char = 0;
 		goto out;
@@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
 			break;
 
-		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
+		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		port->icount.tx++;
 		count--;
@@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 /* no modem control lines */
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
-	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
+	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
 
 	if (umstat & S3C2410_UMSTAT_CTS)
 		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
@@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct s3c24xx_uart_port *ourport;
 	int index = probe_index;
-	int ret;
+	int ret, prop = 0;
 
 	if (np) {
 		ret = of_alias_get_id(np, "serial");
@@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 			dev_get_platdata(&pdev->dev) :
 			ourport->drv_data->def_cfg;
 
-	if (np)
+	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
 
+		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+			switch (prop) {
+			case 1:
+				ourport->port.iotype = UPIO_MEM;
+				break;
+			case 4:
+				ourport->port.iotype = UPIO_MEM32;
+				break;
+			default:
+				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
+						prop);
+				ret = -EINVAL;
+				break;
+			}
+		}
+	}
+
 	if (ourport->drv_data->fifosize[index])
 		ourport->port.fifosize = ourport->drv_data->fifosize[index];
 	else if (ourport->info->fifosize)
@@ -2185,7 +2237,7 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 	if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
 		return NO_POLL_CHAR;
 
-	return rd_regb(port, S3C2410_URXH);
+	return rd_reg(port, S3C2410_URXH);
 }
 
 static void s3c24xx_serial_put_poll_char(struct uart_port *port,
@@ -2200,7 +2252,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, c);
+	wr_reg(port, S3C2410_UTXH, c);
 }
 
 #endif /* CONFIG_CONSOLE_POLL */
@@ -2212,7 +2264,7 @@ s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, ch);
+	wr_reg(port, S3C2410_UTXH, ch);
 }
 
 static void
@@ -2612,7 +2664,7 @@ static void samsung_early_putc(struct uart_port *port, int c)
 	else
 		samsung_early_busyuart(port);
 
-	writeb(c, port->membase + S3C2410_UTXH);
+	wr_reg_barrier(port, S3C2410_UTXH, c);
 }
 
 static void samsung_early_write(struct console *con, const char *s,
-- 
2.15.0.rc1

Re: [PATCH v3] tty: samsung_tty: 32-bit access for TX/RX hold registers

['Krzysztof Kozlowski']

On Fri, Apr 03, 2020 at 08:12:55PM +0900, Hyunki Koo wrote:
> On Thu, Apr 02, 2020 at 7:48:38PM +0900, Krzysztof Kozlowski
> > On Fri, Apr 03, 2020 at 07:20:41PM +0900, Hyunki Koo wrote:
> > > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> > >
> > > This is required for some newer SoCs.
> > >
> > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > > ---
> > > v3: change rd_regl to rd_reg in line 954 for backward compatibility.
> > 
> > I cannot find this change against v2.
> Okay, I will add all changes.

No, I mean, I cannot find the change rd_regl->rd_reg around line 954.
There was no changes around line 954 in v2.


Best regards,
Krzysztof

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Greg KH]

On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v2: 
> line 954 : change rd_regl to rd_reg in for backward compatibility.
> line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
> v3:
> line 2031: remove redundant init value  for ourport->port.iotype 
> v4:
> correct variable types and change misleading function name
> ---
>  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 73f951d65b93..bdf1d4d12cb1 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
>  #define portaddrl(port, reg) \
>  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
>  
> -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> +static u32 rd_reg(struct uart_port *port, u32 reg)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		return readb_relaxed(portaddr(port, reg));
> +	case UPIO_MEM32:
> +		return readl_relaxed(portaddr(port, reg));
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
>  
> -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb_relaxed(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel_relaxed(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
>  
> +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel(val, portaddr(port, reg));
> +		break;
> +	}
> +}

why do you have a default for the read function, but not the write
functions?


> +
>  /* Byte-order aware bit setting/clearing functions. */
>  
>  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>  		fifocnt--;
>  
>  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> -		ch = rd_regb(port, S3C2410_URXH);
> +		ch = rd_reg(port, S3C2410_URXH);
>  
>  		if (port->flags & UPF_CONS_FLOW) {
>  			int txe = s3c24xx_serial_txempty_nofifo(port);
> @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>  	}
>  
>  	if (port->x_char) {
> -		wr_regb(port, S3C2410_UTXH, port->x_char);
> +		wr_reg(port, S3C2410_UTXH, port->x_char);
>  		port->icount.tx++;
>  		port->x_char = 0;
>  		goto out;
> @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
>  			break;
>  
> -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
>  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>  		port->icount.tx++;
>  		count--;
> @@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>  /* no modem control lines */
>  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>  {
> -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
>  
>  	if (umstat & S3C2410_UMSTAT_CTS)
>  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> @@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	struct s3c24xx_uart_port *ourport;
>  	int index = probe_index;
> -	int ret;
> +	int ret, prop = 0;
>  
>  	if (np) {
>  		ret = of_alias_get_id(np, "serial");
> @@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  			dev_get_platdata(&pdev->dev) :
>  			ourport->drv_data->def_cfg;
>  
> -	if (np)
> +	if (np) {
>  		of_property_read_u32(np,
>  			"samsung,uart-fifosize", &ourport->port.fifosize);
>  
> +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> +			switch (prop) {
> +			case 1:
> +				ourport->port.iotype = UPIO_MEM;
> +				break;
> +			case 4:
> +				ourport->port.iotype = UPIO_MEM32;
> +				break;
> +			default:
> +				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> +						prop);
> +				ret = -EINVAL;
> +				break;
> +			}
> +		}
> +	}

If the property is not there, the type will be uninitialized and no
warning will be spit out, are you sure you want to do that?

Can you break this into 2 patches, one that changes the names of the
macros and the calls to them, and the second that adds the new
functionality?  That way it's easier to review/read.

thanks,

greg k-h

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Fri, Apr 03, 2020 at 01:42:37PM +0200, Greg KH wrote:
> On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> > 
> > This is required for some newer SoCs.
> > 
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > ---
> > v2: 
> > line 954 : change rd_regl to rd_reg in for backward compatibility.
> > line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
> > v3:
> > line 2031: remove redundant init value  for ourport->port.iotype 
> > v4:
> > correct variable types and change misleading function name
> > ---
> >  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 64 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > index 73f951d65b93..bdf1d4d12cb1 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
> >  #define portaddrl(port, reg) \
> >  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
> >  
> > -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> > +static u32 rd_reg(struct uart_port *port, u32 reg)
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		return readb_relaxed(portaddr(port, reg));
> > +	case UPIO_MEM32:
> > +		return readl_relaxed(portaddr(port, reg));
> > +	default:
> > +		return 0;
> > +	}
> > +	return 0;
> > +}
> > +
> >  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
> >  
> > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> > +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
> >  
> > +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> 
> why do you have a default for the read function, but not the write
> functions?

The default for read will never happen and it returns bogus value just
to satisfy the compiler. That's my understanding. What would you like to
do for writes()? Print error msg? No point, it should not happen because
of check in probe().

> 
> > +
> >  /* Byte-order aware bit setting/clearing functions. */
> >  
> >  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> > @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
> >  		fifocnt--;
> >  
> >  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> > -		ch = rd_regb(port, S3C2410_URXH);
> > +		ch = rd_reg(port, S3C2410_URXH);
> >  
> >  		if (port->flags & UPF_CONS_FLOW) {
> >  			int txe = s3c24xx_serial_txempty_nofifo(port);
> > @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
> >  	}
> >  
> >  	if (port->x_char) {
> > -		wr_regb(port, S3C2410_UTXH, port->x_char);
> > +		wr_reg(port, S3C2410_UTXH, port->x_char);
> >  		port->icount.tx++;
> >  		port->x_char = 0;
> >  		goto out;
> > @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
> >  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
> >  			break;
> >  
> > -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> >  		port->icount.tx++;
> >  		count--;
> > @@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
> >  /* no modem control lines */
> >  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
> >  {
> > -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> > +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> >  
> >  	if (umstat & S3C2410_UMSTAT_CTS)
> >  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> > @@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct s3c24xx_uart_port *ourport;
> >  	int index = probe_index;
> > -	int ret;
> > +	int ret, prop = 0;
> >  
> >  	if (np) {
> >  		ret = of_alias_get_id(np, "serial");
> > @@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
> >  			dev_get_platdata(&pdev->dev) :
> >  			ourport->drv_data->def_cfg;
> >  
> > -	if (np)
> > +	if (np) {
> >  		of_property_read_u32(np,
> >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> >  
> > +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> > +			switch (prop) {
> > +			case 1:
> > +				ourport->port.iotype = UPIO_MEM;
> > +				break;
> > +			case 4:
> > +				ourport->port.iotype = UPIO_MEM32;
> > +				break;
> > +			default:
> > +				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> > +						prop);
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +		}
> > +	}
> 
> If the property is not there, the type will be uninitialized and no
> warning will be spit out, are you sure you want to do that?

The default value from initial ourport will be used, which is UPIO_MEM.
This way it is backward compatible.

Best regards,
Krzysztof

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Greg KH]

On Fri, Apr 03, 2020 at 01:53:13PM +0200, Krzysztof Kozlowski wrote:
> On Fri, Apr 03, 2020 at 01:42:37PM +0200, Greg KH wrote:
> > On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> > > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> > > 
> > > This is required for some newer SoCs.
> > > 
> > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > > ---
> > > v2: 
> > > line 954 : change rd_regl to rd_reg in for backward compatibility.
> > > line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
> > > v3:
> > > line 2031: remove redundant init value  for ourport->port.iotype 
> > > v4:
> > > correct variable types and change misleading function name
> > > ---
> > >  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 64 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > > index 73f951d65b93..bdf1d4d12cb1 100644
> > > --- a/drivers/tty/serial/samsung_tty.c
> > > +++ b/drivers/tty/serial/samsung_tty.c
> > > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
> > >  #define portaddrl(port, reg) \
> > >  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
> > >  
> > > -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> > > +static u32 rd_reg(struct uart_port *port, u32 reg)
> > > +{
> > > +	switch (port->iotype) {
> > > +	case UPIO_MEM:
> > > +		return readb_relaxed(portaddr(port, reg));
> > > +	case UPIO_MEM32:
> > > +		return readl_relaxed(portaddr(port, reg));
> > > +	default:
> > > +		return 0;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > >  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
> > >  
> > > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> > > +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
> > > +{
> > > +	switch (port->iotype) {
> > > +	case UPIO_MEM:
> > > +		writeb_relaxed(val, portaddr(port, reg));
> > > +		break;
> > > +	case UPIO_MEM32:
> > > +		writel_relaxed(val, portaddr(port, reg));
> > > +		break;
> > > +	}
> > > +}
> > > +
> > >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
> > >  
> > > +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> > > +{
> > > +	switch (port->iotype) {
> > > +	case UPIO_MEM:
> > > +		writeb(val, portaddr(port, reg));
> > > +		break;
> > > +	case UPIO_MEM32:
> > > +		writel(val, portaddr(port, reg));
> > > +		break;
> > > +	}
> > > +}
> > 
> > why do you have a default for the read function, but not the write
> > functions?
> 
> The default for read will never happen and it returns bogus value just
> to satisfy the compiler. That's my understanding. What would you like to
> do for writes()? Print error msg? No point, it should not happen because
> of check in probe().

Consistancy please, either do it for all, or none, otherwise it makes no
sense at all.

> > > +
> > >  /* Byte-order aware bit setting/clearing functions. */
> > >  
> > >  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> > > @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
> > >  		fifocnt--;
> > >  
> > >  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> > > -		ch = rd_regb(port, S3C2410_URXH);
> > > +		ch = rd_reg(port, S3C2410_URXH);
> > >  
> > >  		if (port->flags & UPF_CONS_FLOW) {
> > >  			int txe = s3c24xx_serial_txempty_nofifo(port);
> > > @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
> > >  	}
> > >  
> > >  	if (port->x_char) {
> > > -		wr_regb(port, S3C2410_UTXH, port->x_char);
> > > +		wr_reg(port, S3C2410_UTXH, port->x_char);
> > >  		port->icount.tx++;
> > >  		port->x_char = 0;
> > >  		goto out;
> > > @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
> > >  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
> > >  			break;
> > >  
> > > -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > > +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> > >  		port->icount.tx++;
> > >  		count--;
> > > @@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
> > >  /* no modem control lines */
> > >  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
> > >  {
> > > -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> > > +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> > >  
> > >  	if (umstat & S3C2410_UMSTAT_CTS)
> > >  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> > > @@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
> > >  	struct device_node *np = pdev->dev.of_node;
> > >  	struct s3c24xx_uart_port *ourport;
> > >  	int index = probe_index;
> > > -	int ret;
> > > +	int ret, prop = 0;
> > >  
> > >  	if (np) {
> > >  		ret = of_alias_get_id(np, "serial");
> > > @@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
> > >  			dev_get_platdata(&pdev->dev) :
> > >  			ourport->drv_data->def_cfg;
> > >  
> > > -	if (np)
> > > +	if (np) {
> > >  		of_property_read_u32(np,
> > >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> > >  
> > > +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> > > +			switch (prop) {
> > > +			case 1:
> > > +				ourport->port.iotype = UPIO_MEM;
> > > +				break;
> > > +			case 4:
> > > +				ourport->port.iotype = UPIO_MEM32;
> > > +				break;
> > > +			default:
> > > +				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
> > > +						prop);
> > > +				ret = -EINVAL;
> > > +				break;
> > > +			}
> > > +		}
> > > +	}
> > 
> > If the property is not there, the type will be uninitialized and no
> > warning will be spit out, are you sure you want to do that?
> 
> The default value from initial ourport will be used, which is UPIO_MEM.
> This way it is backward compatible.

Where is iotype set to UPIO_MEM as a default?

thanks,

greg k-h

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v2: 
> line 954 : change rd_regl to rd_reg in for backward compatibility.

I did not see any change around line 954 in v1... so what was it?

Rest looks good for me, although you should address Greg's comments.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Tested on Odroid HC1 (Exynos5422):
Tested-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Fri, Apr 03, 2020 at 01:57:15PM +0200, Greg KH wrote:
> > > If the property is not there, the type will be uninitialized and no
> > > warning will be spit out, are you sure you want to do that?
> > 
> > The default value from initial ourport will be used, which is UPIO_MEM.
> > This way it is backward compatible.
> 
> Where is iotype set to UPIO_MEM as a default?

Here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/samsung_tty.c?h=v5.6#n1626
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/samsung_tty.c?h=v5.6#n1989

Best regards,
Krzysztof

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v2: 
> line 954 : change rd_regl to rd_reg in for backward compatibility.
> line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
> v3:
> line 2031: remove redundant init value  for ourport->port.iotype 
> v4:
> correct variable types and change misleading function name
> ---
>  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 73f951d65b93..bdf1d4d12cb1 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
>  #define portaddrl(port, reg) \
>  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
>  
> -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> +static u32 rd_reg(struct uart_port *port, u32 reg)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		return readb_relaxed(portaddr(port, reg));
> +	case UPIO_MEM32:
> +		return readl_relaxed(portaddr(port, reg));
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}
> +
>  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
>  
> -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb_relaxed(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel_relaxed(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
>  
> +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  /* Byte-order aware bit setting/clearing functions. */
>  
>  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
>  		fifocnt--;
>  
>  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> -		ch = rd_regb(port, S3C2410_URXH);
> +		ch = rd_reg(port, S3C2410_URXH);
>  
>  		if (port->flags & UPF_CONS_FLOW) {
>  			int txe = s3c24xx_serial_txempty_nofifo(port);
> @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>  	}
>  
>  	if (port->x_char) {
> -		wr_regb(port, S3C2410_UTXH, port->x_char);
> +		wr_reg(port, S3C2410_UTXH, port->x_char);
>  		port->icount.tx++;
>  		port->x_char = 0;
>  		goto out;
> @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
>  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
>  			break;
>  
> -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
>  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>  		port->icount.tx++;
>  		count--;
> @@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
>  /* no modem control lines */
>  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>  {
> -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
>  
>  	if (umstat & S3C2410_UMSTAT_CTS)
>  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
> @@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node;
>  	struct s3c24xx_uart_port *ourport;
>  	int index = probe_index;
> -	int ret;
> +	int ret, prop = 0;
>  
>  	if (np) {
>  		ret = of_alias_get_id(np, "serial");
> @@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  			dev_get_platdata(&pdev->dev) :
>  			ourport->drv_data->def_cfg;
>  
> -	if (np)
> +	if (np) {
>  		of_property_read_u32(np,
>  			"samsung,uart-fifosize", &ourport->port.fifosize);
>  
> +		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {

I got more thoughts... where is the binding for it? It looked like
standard DT property but it is not described in device tree spec.

Also, where is the user (DTS) with it? I expect such changes go with the
user itself... otherwise, next cleanup it will go away.

Best regards,
Krzysztof

RE: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On Fri, Apr 03, 2020 at 9:00: 10PM +0900, Krzysztof Kozlowski wrote:
> On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> >
> > This is required for some newer SoCs.
> >
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > ---
> > v2:
> > line 954 : change rd_regl to rd_reg in for backward compatibility.
> 
> I did not see any change around line 954 in v1... so what was it?
954  line  is changed like below.
V1 : rd_regb --> rd_regl : we thought, this register need to change to 
V2: rd_regl --> rd_reg : we discuss internally, and 
I decided not to change  to existing devices for backward compatibility.
So we changed to rd_reg.
> 
> Rest looks good for me, although you should address Greg's comments.
> 
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Tested on Odroid
> HC1 (Exynos5422):
> Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Best regards,
> Krzysztof

RE: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On Fri, Apr 03, 2020 at 10:35:10PM +0900, Krzysztof Kozlowski wrote:
> On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> >
> > This is required for some newer SoCs.
> >
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > ---
> > v2:
> > line 954 : change rd_regl to rd_reg in for backward compatibility.
> > line 2031: Add init value for ourport->port.iotype  to UPIO_MEM
> > v3:
> > line 2031: remove redundant init value  for ourport->port.iotype
> > v4:
> > correct variable types and change misleading function name
> > ---
> >  drivers/tty/serial/samsung_tty.c | 76
> > +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 64 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 73f951d65b93..bdf1d4d12cb1 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {  #define
> > portaddrl(port, reg) \
> >  	((unsigned long *)(unsigned long)((port)->membase + (reg)))
> >
> > -#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
> > +static u32 rd_reg(struct uart_port *port, u32 reg) {
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		return readb_relaxed(portaddr(port, reg));
> > +	case UPIO_MEM32:
> > +		return readl_relaxed(portaddr(port, reg));
> > +	default:
> > +		return 0;
> > +	}
> > +	return 0;
> > +}
> > +
> >  #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
> >
> > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port,
> > reg))
> > +static void wr_reg(struct uart_port *port, u32 reg, u32 val) {
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port,
> > reg))
> >
> > +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  /* Byte-order aware bit setting/clearing functions. */
> >
> >  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> > @@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct
> s3c24xx_uart_port *ourport)
> >  		fifocnt--;
> >
> >  		uerstat = rd_regl(port, S3C2410_UERSTAT);
> > -		ch = rd_regb(port, S3C2410_URXH);
> > +		ch = rd_reg(port, S3C2410_URXH);
> >
> >  		if (port->flags & UPF_CONS_FLOW) {
> >  			int txe = s3c24xx_serial_txempty_nofifo(port);
> > @@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int
> irq, void *id)
> >  	}
> >
> >  	if (port->x_char) {
> > -		wr_regb(port, S3C2410_UTXH, port->x_char);
> > +		wr_reg(port, S3C2410_UTXH, port->x_char);
> >  		port->icount.tx++;
> >  		port->x_char = 0;
> >  		goto out;
> > @@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int
> irq, void *id)
> >  		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info-
> >tx_fifofull)
> >  			break;
> >
> > -		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> > +		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
> >  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> >  		port->icount.tx++;
> >  		count--;
> > @@ -916,7 +951,7 @@ static unsigned int
> s3c24xx_serial_tx_empty(struct
> > uart_port *port)
> >  /* no modem control lines */
> >  static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
> > {
> > -	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
> > +	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
> >
> >  	if (umstat & S3C2410_UMSTAT_CTS)
> >  		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS; @@ -
> 1974,7 +2009,7 @@
> > static int s3c24xx_serial_probe(struct platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct s3c24xx_uart_port *ourport;
> >  	int index = probe_index;
> > -	int ret;
> > +	int ret, prop = 0;
> >
> >  	if (np) {
> >  		ret = of_alias_get_id(np, "serial"); @@ -2000,10
> +2035,27 @@ static
> > int s3c24xx_serial_probe(struct platform_device *pdev)
> >  			dev_get_platdata(&pdev->dev) :
> >  			ourport->drv_data->def_cfg;
> >
> > -	if (np)
> > +	if (np) {
> >  		of_property_read_u32(np,
> >  			"samsung,uart-fifosize", &ourport->port.fifosize);
> >
> > +		if (of_property_read_u32(np, "reg-io-width", &prop) ==
> 0) {
> 
> I got more thoughts... where is the binding for it? It looked like standard
> DT property but it is not described in device tree spec.
> 
> Also, where is the user (DTS) with it? I expect such changes go with the
> user itself... otherwise, next cleanup it will go away.
> 
> Best regards,
> Krzysztof

Do you think this kind of change is needed?
Do I have to make as a series patches with previous patch?

diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
index 9d2ce347875b..a57b1233c691 100644
--- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
+++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
@@ -29,6 +29,14 @@ properties:
   reg:
     maxItems: 1
 
+  reg-io-width:
+    description: |
+      The size (in bytes) of the IO accesses that should be performed
+      on the device.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [ 1, 4 ]
+

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

['Krzysztof Kozlowski']

On Mon, Apr 06, 2020 at 06:41:09AM +0900, Hyunki Koo wrote:
> > 
> > I got more thoughts... where is the binding for it? It looked like standard
> > DT property but it is not described in device tree spec.
> > 
> > Also, where is the user (DTS) with it? I expect such changes go with the
> > user itself... otherwise, next cleanup it will go away.
> > 
> > Best regards,
> > Krzysztof
> 
> Do you think this kind of change is needed?

You mean the user of this binding (DTS)? It is not required because with
binding comes ABI stability. However it would be both appreciated and
useful because it would clearly note that this feature is used.

The feature brings additional complexity and +1 function call for each
simple register access. Therefore sometime in the future, one could see
it is not being used and start cleaning it up. Cleaning up usually
involves looking for users, then making binding deprecated and finally
removing the feature.

The collaboration between the Samsung LSI and upstream is quite
limited... And it basically does not exist between the Samsung mobile
division and upstream. This means that when we reorganize the
code, deprecate features/drivers or certain Exynos chips (e.g. 4212 and
4415 in the past) we look for users of them and if none are found - bye
bye feature.

The solution is either to participate (but this is difficult for
mentioned Samsung divisions because of internal policies) or just add
the user of such feature (e.g. DTS for evalkit).

> Do I have to make as a series patches with previous patch?

The DT binding you posted looks good. It should go as first patch in this
series.

Best regards,
Krzysztof



> 
> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> index 9d2ce347875b..a57b1233c691 100644
> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> @@ -29,6 +29,14 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  reg-io-width:
> +    description: |
> +      The size (in bytes) of the IO accesses that should be performed
> +      on the device.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [ 1, 4 ]
> +
> 

Re: [PATCH v4] tty: samsung_tty: 32-bit access for TX/RX hold registers

['Krzysztof Kozlowski']

On Mon, Apr 06, 2020 at 06:35:46AM +0900, Hyunki Koo wrote:
> On Fri, Apr 03, 2020 at 9:00: 10PM +0900, Krzysztof Kozlowski wrote:
> > On Fri, Apr 03, 2020 at 08:15:10PM +0900, Hyunki Koo wrote:
> > > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> > >
> > > This is required for some newer SoCs.
> > >
> > > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > > ---
> > > v2:
> > > line 954 : change rd_regl to rd_reg in for backward compatibility.
> > 
> > I did not see any change around line 954 in v1... so what was it?
> 954  line  is changed like below.
> V1 : rd_regb --> rd_regl : we thought, this register need to change to 
> V2: rd_regl --> rd_reg : we discuss internally, and 
> I decided not to change  to existing devices for backward compatibility.
> So we changed to rd_reg.

Thanks.

Best regards,
Krzysztof

[PATCH v5 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

Support 32-bit access for the TX/RX hold registers UTXH and URXH.

This is required for some newer SoCs.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
v2: 
line 954 : change rd_regl to rd_reg in for backward compatibility.
line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
v3:
line 2031: remove redundant init value  for ourport->port.iotype 
v4:
correct variable types and change misleading function name
v5:
add dt-binding and go as first patch in this series.

---
 drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 73f951d65b93..bdf1d4d12cb1 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
 #define portaddrl(port, reg) \
 	((unsigned long *)(unsigned long)((port)->membase + (reg)))
 
-#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
+static u32 rd_reg(struct uart_port *port, u32 reg)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		return readb_relaxed(portaddr(port, reg));
+	case UPIO_MEM32:
+		return readl_relaxed(portaddr(port, reg));
+	default:
+		return 0;
+	}
+	return 0;
+}
+
 #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
 
-#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
+static void wr_reg(struct uart_port *port, u32 reg, u32 val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb_relaxed(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel_relaxed(val, portaddr(port, reg));
+		break;
+	}
+}
+
 #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
 
+static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel(val, portaddr(port, reg));
+		break;
+	}
+}
+
 /* Byte-order aware bit setting/clearing functions. */
 
 static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
@@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 		fifocnt--;
 
 		uerstat = rd_regl(port, S3C2410_UERSTAT);
-		ch = rd_regb(port, S3C2410_URXH);
+		ch = rd_reg(port, S3C2410_URXH);
 
 		if (port->flags & UPF_CONS_FLOW) {
 			int txe = s3c24xx_serial_txempty_nofifo(port);
@@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (port->x_char) {
-		wr_regb(port, S3C2410_UTXH, port->x_char);
+		wr_reg(port, S3C2410_UTXH, port->x_char);
 		port->icount.tx++;
 		port->x_char = 0;
 		goto out;
@@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
 			break;
 
-		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
+		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		port->icount.tx++;
 		count--;
@@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 /* no modem control lines */
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
-	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
+	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
 
 	if (umstat & S3C2410_UMSTAT_CTS)
 		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
@@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct s3c24xx_uart_port *ourport;
 	int index = probe_index;
-	int ret;
+	int ret, prop = 0;
 
 	if (np) {
 		ret = of_alias_get_id(np, "serial");
@@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 			dev_get_platdata(&pdev->dev) :
 			ourport->drv_data->def_cfg;
 
-	if (np)
+	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
 
+		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+			switch (prop) {
+			case 1:
+				ourport->port.iotype = UPIO_MEM;
+				break;
+			case 4:
+				ourport->port.iotype = UPIO_MEM32;
+				break;
+			default:
+				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
+						prop);
+				ret = -EINVAL;
+				break;
+			}
+		}
+	}
+
 	if (ourport->drv_data->fifosize[index])
 		ourport->port.fifosize = ourport->drv_data->fifosize[index];
 	else if (ourport->info->fifosize)
@@ -2185,7 +2237,7 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 	if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
 		return NO_POLL_CHAR;
 
-	return rd_regb(port, S3C2410_URXH);
+	return rd_reg(port, S3C2410_URXH);
 }
 
 static void s3c24xx_serial_put_poll_char(struct uart_port *port,
@@ -2200,7 +2252,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, c);
+	wr_reg(port, S3C2410_UTXH, c);
 }
 
 #endif /* CONFIG_CONSOLE_POLL */
@@ -2212,7 +2264,7 @@ s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, ch);
+	wr_reg(port, S3C2410_UTXH, ch);
 }
 
 static void
@@ -2612,7 +2664,7 @@ static void samsung_early_putc(struct uart_port *port, int c)
 	else
 		samsung_early_busyuart(port);
 
-	writeb(c, port->membase + S3C2410_UTXH);
+	wr_reg_barrier(port, S3C2410_UTXH, c);
 }
 
 static void samsung_early_write(struct console *con, const char *s,
-- 
2.15.0.rc1

[PATCH v5 1/2] dt-bindings: serial: Add reg-io-width compatible

[Hyunki Koo]

Add a description for reg-io-width options for the samsung serial
UART peripheral.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
 Documentation/devicetree/bindings/serial/samsung_uart.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
index 9d2ce347875b..a57b1233c691 100644
--- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
+++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
@@ -29,6 +29,14 @@ properties:
   reg:
     maxItems: 1
 
+  reg-io-width:
+    description: |
+      The size (in bytes) of the IO accesses that should be performed
+      on the device.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [ 1, 4 ]
+
   clocks:
     minItems: 2
     maxItems: 5
-- 
2.15.0.rc1

Re: [PATCH v5 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Mon, Apr 06, 2020 at 07:31:25PM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v2: 
> line 954 : change rd_regl to rd_reg in for backward compatibility.
> line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
> v3:
> line 2031: remove redundant init value  for ourport->port.iotype 
> v4:
> correct variable types and change misleading function name
> v5:
> add dt-binding and go as first patch in this series.
> 
> ---
>  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 12 deletions(-)

For me it is fine, although Greg wanted the write functions to be
consistent with read around default case.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Tested-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v5 1/2] dt-bindings: serial: Add reg-io-width compatible

[Krzysztof Kozlowski]

On Mon, Apr 06, 2020 at 07:31:26PM +0900, Hyunki Koo wrote:
> Add a description for reg-io-width options for the samsung serial
> UART peripheral.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> index 9d2ce347875b..a57b1233c691 100644
> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> @@ -29,6 +29,14 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  reg-io-width:
> +    description: |
> +      The size (in bytes) of the IO accesses that should be performed
> +      on the device.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint32
> +      - enum: [ 1, 4 ]

I just noticed that the allOf is not needed. Just enum [1, 2] is enough.

With that change:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

> +
>    clocks:
>      minItems: 2
>      maxItems: 5
> -- 
> 2.15.0.rc1
> 

[PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

Support 32-bit access for the TX/RX hold registers UTXH and URXH.

This is required for some newer SoCs.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
v2: 
line 954 : change rd_regl to rd_reg in for backward compatibility.
line 2031: Add init value for ourport->port.iotype  to UPIO_MEM 
v3:
line 2031: remove redundant init value  for ourport->port.iotype 
v4:
correct variable types and change misleading function name
v5:
add dt-binding and go as first patch in this series.
v6:
no change in this patch, only chaged in [PATCH v6 1/2]
---
 drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 73f951d65b93..bdf1d4d12cb1 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
 #define portaddrl(port, reg) \
 	((unsigned long *)(unsigned long)((port)->membase + (reg)))
 
-#define rd_regb(port, reg) (readb_relaxed(portaddr(port, reg)))
+static u32 rd_reg(struct uart_port *port, u32 reg)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		return readb_relaxed(portaddr(port, reg));
+	case UPIO_MEM32:
+		return readl_relaxed(portaddr(port, reg));
+	default:
+		return 0;
+	}
+	return 0;
+}
+
 #define rd_regl(port, reg) (readl_relaxed(portaddr(port, reg)))
 
-#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
+static void wr_reg(struct uart_port *port, u32 reg, u32 val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb_relaxed(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel_relaxed(val, portaddr(port, reg));
+		break;
+	}
+}
+
 #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
 
+static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
+{
+	switch (port->iotype) {
+	case UPIO_MEM:
+		writeb(val, portaddr(port, reg));
+		break;
+	case UPIO_MEM32:
+		writel(val, portaddr(port, reg));
+		break;
+	}
+}
+
 /* Byte-order aware bit setting/clearing functions. */
 
 static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
@@ -714,7 +749,7 @@ static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport)
 		fifocnt--;
 
 		uerstat = rd_regl(port, S3C2410_UERSTAT);
-		ch = rd_regb(port, S3C2410_URXH);
+		ch = rd_reg(port, S3C2410_URXH);
 
 		if (port->flags & UPF_CONS_FLOW) {
 			int txe = s3c24xx_serial_txempty_nofifo(port);
@@ -826,7 +861,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	}
 
 	if (port->x_char) {
-		wr_regb(port, S3C2410_UTXH, port->x_char);
+		wr_reg(port, S3C2410_UTXH, port->x_char);
 		port->icount.tx++;
 		port->x_char = 0;
 		goto out;
@@ -852,7 +887,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		if (rd_regl(port, S3C2410_UFSTAT) & ourport->info->tx_fifofull)
 			break;
 
-		wr_regb(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
+		wr_reg(port, S3C2410_UTXH, xmit->buf[xmit->tail]);
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
 		port->icount.tx++;
 		count--;
@@ -916,7 +951,7 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 /* no modem control lines */
 static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 {
-	unsigned int umstat = rd_regb(port, S3C2410_UMSTAT);
+	unsigned int umstat = rd_reg(port, S3C2410_UMSTAT);
 
 	if (umstat & S3C2410_UMSTAT_CTS)
 		return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS;
@@ -1974,7 +2009,7 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct s3c24xx_uart_port *ourport;
 	int index = probe_index;
-	int ret;
+	int ret, prop = 0;
 
 	if (np) {
 		ret = of_alias_get_id(np, "serial");
@@ -2000,10 +2035,27 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
 			dev_get_platdata(&pdev->dev) :
 			ourport->drv_data->def_cfg;
 
-	if (np)
+	if (np) {
 		of_property_read_u32(np,
 			"samsung,uart-fifosize", &ourport->port.fifosize);
 
+		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
+			switch (prop) {
+			case 1:
+				ourport->port.iotype = UPIO_MEM;
+				break;
+			case 4:
+				ourport->port.iotype = UPIO_MEM32;
+				break;
+			default:
+				dev_warn(&pdev->dev, "unsupported reg-io-width (%d)\n",
+						prop);
+				ret = -EINVAL;
+				break;
+			}
+		}
+	}
+
 	if (ourport->drv_data->fifosize[index])
 		ourport->port.fifosize = ourport->drv_data->fifosize[index];
 	else if (ourport->info->fifosize)
@@ -2185,7 +2237,7 @@ static int s3c24xx_serial_get_poll_char(struct uart_port *port)
 	if (s3c24xx_serial_rx_fifocnt(ourport, ufstat) == 0)
 		return NO_POLL_CHAR;
 
-	return rd_regb(port, S3C2410_URXH);
+	return rd_reg(port, S3C2410_URXH);
 }
 
 static void s3c24xx_serial_put_poll_char(struct uart_port *port,
@@ -2200,7 +2252,7 @@ static void s3c24xx_serial_put_poll_char(struct uart_port *port,
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, c);
+	wr_reg(port, S3C2410_UTXH, c);
 }
 
 #endif /* CONFIG_CONSOLE_POLL */
@@ -2212,7 +2264,7 @@ s3c24xx_serial_console_putchar(struct uart_port *port, int ch)
 
 	while (!s3c24xx_serial_console_txrdy(port, ufcon))
 		cpu_relax();
-	wr_regb(port, S3C2410_UTXH, ch);
+	wr_reg(port, S3C2410_UTXH, ch);
 }
 
 static void
@@ -2612,7 +2664,7 @@ static void samsung_early_putc(struct uart_port *port, int c)
 	else
 		samsung_early_busyuart(port);
 
-	writeb(c, port->membase + S3C2410_UTXH);
+	wr_reg_barrier(port, S3C2410_UTXH, c);
 }
 
 static void samsung_early_write(struct console *con, const char *s,
-- 
2.15.0.rc1

[PATCH v6 1/2] dt-bindings: serial: Add reg-io-width compatible

[Hyunki Koo]

Add a description for reg-io-width options for the samsung serial
UART peripheral.

Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
v5: first added in this series
v6: clean description of reg-io-width
---
 Documentation/devicetree/bindings/serial/samsung_uart.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
index 9d2ce347875b..1a0bb7619e2e 100644
--- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
+++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
@@ -29,6 +29,12 @@ properties:
   reg:
     maxItems: 1
 
+  reg-io-width:
+    description:
+      The size (in bytes) of the IO accesses that should be performed
+      on the device. If omitted, default of 1 is used.
+      - enum: [ 1, 4 ]
+
   clocks:
     minItems: 2
     maxItems: 5
-- 
2.15.0.rc1

Re: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Jiri Slaby]

On 07. 04. 20, 1:08, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
...
> ---
>  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 73f951d65b93..bdf1d4d12cb1 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
...
> -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb_relaxed(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel_relaxed(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
>  
> +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)

You need to explain, why you need this _barrier variant now. This change
should be done in a separate patch too.

> +{
> +	switch (port->iotype) {
> +	case UPIO_MEM:
> +		writeb(val, portaddr(port, reg));
> +		break;
> +	case UPIO_MEM32:
> +		writel(val, portaddr(port, reg));
> +		break;
> +	}
> +}
> +
>  /* Byte-order aware bit setting/clearing functions. */
>  
>  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,

thanks,
-- 
js
suse labs

RE: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On 07. 04. 20, 1:49, Jiri Slaby wrote:
> On 07. 04. 20, 1:08, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> >
> > This is required for some newer SoCs.
> >
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ...
> > ---
> >  drivers/tty/serial/samsung_tty.c | 76
> > +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 64 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 73f951d65b93..bdf1d4d12cb1 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
> ...
> > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port,
> > reg))
> > +static void wr_reg(struct uart_port *port, u32 reg, u32 val) {
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port,
> > reg))
> >
> > +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> 
> You need to explain, why you need this _barrier variant now. This change
> should be done in a separate patch too.
> 
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  /* Byte-order aware bit setting/clearing functions. */
> >
> >  static inline void s3c24xx_set_bit(struct uart_port *port, int idx,
> 
> thanks,
> --
> js
> suse labs

The purpose of this patch is to support 32bit access for registers, and it is also working exsisting device.
There are 3 operations what I have to to change which are rd_regb, wr_regb, and writeb.
rd_regb, wr_regb are changed to rd_reg, wr_reg.
and writeb is changed to wr_reg_barrier.
So I make as a one patch.

wr_reg_barrier is not a different patch, itis just replaced from writeb.

Re: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Tue, Apr 07, 2020 at 06:49:29AM +0200, Jiri Slaby wrote:
> On 07. 04. 20, 1:08, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> > 
> > This is required for some newer SoCs.
> > 
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ...
> > ---
> >  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 64 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > index 73f951d65b93..bdf1d4d12cb1 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
> ...
> > -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> > +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
> > +{
> > +	switch (port->iotype) {
> > +	case UPIO_MEM:
> > +		writeb_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	case UPIO_MEM32:
> > +		writel_relaxed(val, portaddr(port, reg));
> > +		break;
> > +	}
> > +}
> > +
> >  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
> >  
> > +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> 
> You need to explain, why you need this _barrier variant now. This change
> should be done in a separate patch too.

There is no functional change in regard of barrier.  The ordered IO was
used there before.

Best regards,
Krzysztof

Re: [PATCH v6 1/2] dt-bindings: serial: Add reg-io-width compatible

[Krzysztof Kozlowski]

On Tue, Apr 07, 2020 at 08:08:50AM +0900, Hyunki Koo wrote:
> Add a description for reg-io-width options for the samsung serial
> UART peripheral.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v5: first added in this series
> v6: clean description of reg-io-width
> ---
>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)

Please keep accumulated tags (review, ack, tested etc.) with new
versions of paches.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Tue, Apr 07, 2020 at 08:08:49AM +0900, Hyunki Koo wrote:
> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> 
> This is required for some newer SoCs.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---

Why I am adding these for the third time?

Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Jiri Slaby]

On 07. 04. 20, 8:26, Krzysztof Kozlowski wrote:
> On Tue, Apr 07, 2020 at 08:08:49AM +0900, Hyunki Koo wrote:
>> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
>>
>> This is required for some newer SoCs.
>>
>> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
>> ---
> 
> Why I am adding these for the third time?

I don't know as I don't care about your tags anyway.

> Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

-- 
js
suse labs

Re: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Jiri Slaby]

On 07. 04. 20, 8:24, Krzysztof Kozlowski wrote:
> On Tue, Apr 07, 2020 at 06:49:29AM +0200, Jiri Slaby wrote:
>> On 07. 04. 20, 1:08, Hyunki Koo wrote:
>>> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
>>>
>>> This is required for some newer SoCs.
>>>
>>> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
>> ...
>>> ---
>>>  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 64 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
>>> index 73f951d65b93..bdf1d4d12cb1 100644
>>> --- a/drivers/tty/serial/samsung_tty.c
>>> +++ b/drivers/tty/serial/samsung_tty.c
>>> @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
>> ...
>>> -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
>>> +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
>>> +{
>>> +	switch (port->iotype) {
>>> +	case UPIO_MEM:
>>> +		writeb_relaxed(val, portaddr(port, reg));
>>> +		break;
>>> +	case UPIO_MEM32:
>>> +		writel_relaxed(val, portaddr(port, reg));
>>> +		break;
>>> +	}
>>> +}
>>> +
>>>  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
>>>  
>>> +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
>>
>> You need to explain, why you need this _barrier variant now. This change
>> should be done in a separate patch too.
> 
> There is no functional change in regard of barrier.  The ordered IO was
> used there before.

The patch changes one wr_reg to wr_reg_barrier without any explanation.
This will hardly be accepted.

thanks,
-- 
js
suse labs

Re: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Jiri Slaby]

On 07. 04. 20, 8:28, Jiri Slaby wrote:
> On 07. 04. 20, 8:26, Krzysztof Kozlowski wrote:
>> On Tue, Apr 07, 2020 at 08:08:49AM +0900, Hyunki Koo wrote:
>>> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
>>>
>>> This is required for some newer SoCs.
>>>
>>> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
>>> ---
>>
>> Why I am adding these for the third time?
> 
> I don't know as I don't care about your tags anyway.

Sorry, my bad, I was somehow mislead by thunderbird, thinking I am
replying to a different thread.

sorry,
-- 
js
suse labs

RE: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Hyunki Koo]

On Tue, Apr 07, 2020 at 3:27 :00PM +0900, Krzysztof Kozlowski wrote:
> On Tue, Apr 07, 2020 at 08:08:49AM +0900, Hyunki Koo wrote:
> > Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> >
> > This is required for some newer SoCs.
> >
> > Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> > ---
> 
> Why I am adding these for the third time?
Sorry, I didn't knew that,
I will keep this next time
> 
> Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Best regards,
> Krzysztof

Re: [PATCH v6 2/2] tty: samsung_tty: 32-bit access for TX/RX hold registers

[Krzysztof Kozlowski]

On Tue, Apr 07, 2020 at 08:32:56AM +0200, Jiri Slaby wrote:
> On 07. 04. 20, 8:24, Krzysztof Kozlowski wrote:
> > On Tue, Apr 07, 2020 at 06:49:29AM +0200, Jiri Slaby wrote:
> >> On 07. 04. 20, 1:08, Hyunki Koo wrote:
> >>> Support 32-bit access for the TX/RX hold registers UTXH and URXH.
> >>>
> >>> This is required for some newer SoCs.
> >>>
> >>> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> >> ...
> >>> ---
> >>>  drivers/tty/serial/samsung_tty.c | 76 +++++++++++++++++++++++++++++++++-------
> >>>  1 file changed, 64 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> >>> index 73f951d65b93..bdf1d4d12cb1 100644
> >>> --- a/drivers/tty/serial/samsung_tty.c
> >>> +++ b/drivers/tty/serial/samsung_tty.c
> >>> @@ -154,12 +154,47 @@ struct s3c24xx_uart_port {
> >> ...
> >>> -#define wr_regb(port, reg, val) writeb_relaxed(val, portaddr(port, reg))
> >>> +static void wr_reg(struct uart_port *port, u32 reg, u32 val)
> >>> +{
> >>> +	switch (port->iotype) {
> >>> +	case UPIO_MEM:
> >>> +		writeb_relaxed(val, portaddr(port, reg));
> >>> +		break;
> >>> +	case UPIO_MEM32:
> >>> +		writel_relaxed(val, portaddr(port, reg));
> >>> +		break;
> >>> +	}
> >>> +}
> >>> +
> >>>  #define wr_regl(port, reg, val) writel_relaxed(val, portaddr(port, reg))
> >>>  
> >>> +static void wr_reg_barrier(struct uart_port *port, u32 reg, u32 val)
> >>
> >> You need to explain, why you need this _barrier variant now. This change
> >> should be done in a separate patch too.
> > 
> > There is no functional change in regard of barrier.  The ordered IO was
> > used there before.
> 
> The patch changes one wr_reg to wr_reg_barrier without any explanation.
> This will hardly be accepted.

I cannot find such change... I see only:

@@ -2612,7 +2664,7 @@ static void samsung_early_putc(struct uart_port *port, int c)
-       writeb(c, port->membase + S3C2410_UTXH);
+       wr_reg_barrier(port, S3C2410_UTXH, c);

which is the same except 'b' -> 'b/l'.

Best regards,
Krzysztof

Re: [PATCH v6 1/2] dt-bindings: serial: Add reg-io-width compatible

[Rob Herring]

On Tue,  7 Apr 2020 08:08:50 +0900, Hyunki Koo wrote:
> Add a description for reg-io-width options for the samsung serial
> UART peripheral.
> 
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v5: first added in this series
> v6: clean description of reg-io-width
> ---
>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/serial/samsung_uart.yaml:  mapping values are not allowed in this context
  in "<unicode string>", line 36, column 13
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/serial/samsung_uart.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/serial/samsung_uart.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
Makefile:1262: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1267104

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH v6 1/2] dt-bindings: serial: Add reg-io-width compatible

[Rob Herring]

On Mon, Apr 6, 2020 at 5:09 PM Hyunki Koo <hyunki00.koo@samsung.com> wrote:
>
> Add a description for reg-io-width options for the samsung serial
> UART peripheral.
>
> Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
> ---
> v5: first added in this series
> v6: clean description of reg-io-width
> ---
>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> index 9d2ce347875b..1a0bb7619e2e 100644
> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> @@ -29,6 +29,12 @@ properties:
>    reg:
>      maxItems: 1
>
> +  reg-io-width:
> +    description:
> +      The size (in bytes) of the IO accesses that should be performed
> +      on the device. If omitted, default of 1 is used.
> +      - enum: [ 1, 4 ]

Can't this be implied by the compatible strings?

This isn't actual json-schema either with the enum under the
description. Run 'make dt_binding_check' before you send schemas.

There's a keyword for expressing the default value too. Hint: It's 'default'.

Rob