diff mbox series

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

Message ID 20200401082721.19431-1-hyunki00.koo@samsung.com (mailing list archive)
State Superseded
Headers show
Series tty: samsung_tty: 32-bit access for TX/RX hold registers | expand

Commit Message

Hyunki Koo April 1, 2020, 8:27 a.m. UTC 
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(-)

Comments

Greg Kroah-Hartman April 1, 2020, 8:55 a.m. UTC | #1 
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
Krzysztof Kozlowski April 1, 2020, 9:19 a.m. UTC | #2 
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 April 2, 2020, 9:44 a.m. UTC | #3 
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;
+		}
Krzysztof Kozlowski April 2, 2020, 9:48 a.m. UTC | #4 
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
diff mbox series

Patch

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,