diff mbox series

[v2] tty: serial: samsung_tty: support more than 4 uart ports

Message ID 20220629005538.60132-1-chanho61.park@samsung.com (mailing list archive)
State New, archived
Headers show
Series [v2] tty: serial: samsung_tty: support more than 4 uart ports | expand

Commit Message

Chanho Park June 29, 2022, 12:55 a.m. UTC
Regarding Exynos Auto v9 SoC, it supports uarts up to 12. However, the
maximum number of the ports has been derived from
CONFIG_SERIAL_SAMSUNG_UARTS and tightly coupled with the config for
previous Samsung SoCs such as s3c24xx and s3c64xx. To overcome this
limitation, this changes the usage of the definition to UART_NR which is
widely used from other serial drivers. This also defines the value to 12
only for ARM64 SoCs to not affect the change to previous arm32 SoCs.

Instead of enumerating all the ports as predefined arrays, this
introduces s3c24xx_serial_init_port_default that is initializing the
structure as the default value.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
Changes from v1:
- Remove initial value assignment of s3c24xx_serial_ports by assigning
  them dynamically as suggested by Greg and Krzysztof.

 drivers/tty/serial/samsung_tty.c | 82 ++++++++++----------------------
 1 file changed, 25 insertions(+), 57 deletions(-)

Comments

Krzysztof Kozlowski June 29, 2022, 7:30 a.m. UTC | #1
On 29/06/2022 02:55, Chanho Park wrote:
> Regarding Exynos Auto v9 SoC, it supports uarts up to 12. However, the
> maximum number of the ports has been derived from
> CONFIG_SERIAL_SAMSUNG_UARTS and tightly coupled with the config for
> previous Samsung SoCs such as s3c24xx and s3c64xx. To overcome this
> limitation, this changes the usage of the definition to UART_NR which is
> widely used from other serial drivers. This also defines the value to 12
> only for ARM64 SoCs to not affect the change to previous arm32 SoCs.
> 
> Instead of enumerating all the ports as predefined arrays, this
> introduces s3c24xx_serial_init_port_default that is initializing the
> structure as the default value.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof
Hector Martin June 29, 2022, 11:13 a.m. UTC | #2
On 29/06/2022 09.55, Chanho Park wrote:
> Regarding Exynos Auto v9 SoC, it supports uarts up to 12. However, the
> maximum number of the ports has been derived from
> CONFIG_SERIAL_SAMSUNG_UARTS and tightly coupled with the config for
> previous Samsung SoCs such as s3c24xx and s3c64xx. To overcome this
> limitation, this changes the usage of the definition to UART_NR which is
> widely used from other serial drivers. This also defines the value to 12
> only for ARM64 SoCs to not affect the change to previous arm32 SoCs.
> 
> Instead of enumerating all the ports as predefined arrays, this
> introduces s3c24xx_serial_init_port_default that is initializing the
> structure as the default value.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>

Reviewed-by: Hector Martin <marcan@marcan.st>
Jiri Slaby July 4, 2022, 7:40 a.m. UTC | #3
On 29. 06. 22, 2:55, Chanho Park wrote:
> Regarding Exynos Auto v9 SoC, it supports uarts up to 12. However, the
> maximum number of the ports has been derived from
> CONFIG_SERIAL_SAMSUNG_UARTS and tightly coupled with the config for
> previous Samsung SoCs such as s3c24xx and s3c64xx. To overcome this
> limitation, this changes the usage of the definition to UART_NR which is
> widely used from other serial drivers. This also defines the value to 12
> only for ARM64 SoCs to not affect the change to previous arm32 SoCs.
> 
> Instead of enumerating all the ports as predefined arrays, this
> introduces s3c24xx_serial_init_port_default that is initializing the
> structure as the default value.
...
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
...
> @@ -1810,67 +1816,27 @@ static const struct uart_ops apple_s5l_serial_ops = {
...
> +static struct s3c24xx_uart_port s3c24xx_serial_ports[UART_NR];
> +
> +static void s3c24xx_serial_init_port_default(int index) {

The opening brace should be on a separate line.

> +	struct uart_port *port = &s3c24xx_serial_ports[index].port;
> +
> +	spin_lock_init(&port->lock);
> +
> +	port->iotype = UPIO_MEM;
> +	port->uartclk = 0;
> +	port->fifosize = 16;
> +	port->ops = &s3c24xx_serial_ops;
> +	port->flags = UPF_BOOT_AUTOCONF;
> +	port->line = index;
> +}
>   
>   /* s3c24xx_serial_resetport
>    *
> @@ -2186,6 +2152,8 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>   	}
>   	ourport = &s3c24xx_serial_ports[index];
>   
> +	s3c24xx_serial_init_port_default(index);

Perhaps pass &ourport->port to the function too -- you'd save the 
refetch there.

thanks,
Chanho Park July 4, 2022, 8:11 a.m. UTC | #4
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> ...
> > @@ -1810,67 +1816,27 @@ static const struct uart_ops
> > apple_s5l_serial_ops = {
> ...
> > +static struct s3c24xx_uart_port s3c24xx_serial_ports[UART_NR];
> > +
> > +static void s3c24xx_serial_init_port_default(int index) {
> 
> The opening brace should be on a separate line.

Oh, I missed this. Thanks for your review.
I'll make a separate patch to fix this.

> 
> > +	struct uart_port *port = &s3c24xx_serial_ports[index].port;
> > +
> > +	spin_lock_init(&port->lock);
> > +
> > +	port->iotype = UPIO_MEM;
> > +	port->uartclk = 0;
> > +	port->fifosize = 16;
> > +	port->ops = &s3c24xx_serial_ops;
> > +	port->flags = UPF_BOOT_AUTOCONF;
> > +	port->line = index;
> > +}
> >
> >   /* s3c24xx_serial_resetport
> >    *
> > @@ -2186,6 +2152,8 @@ static int s3c24xx_serial_probe(struct
> platform_device *pdev)
> >   	}
> >   	ourport = &s3c24xx_serial_ports[index];
> >
> > +	s3c24xx_serial_init_port_default(index);
> 
> Perhaps pass &ourport->port to the function too -- you'd save the refetch
> there.

&ourport->port will be retrieved by index in the s3c24xx_serial_init_port_default.
Why do I need to pass it?

+	struct uart_port *port = &s3c24xx_serial_ports[index].port;

Best Regards,
Chanho Park
diff mbox series

Patch

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 03ef4ff506fd..8971fbb49fa3 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -48,6 +48,12 @@ 
 #define S3C24XX_SERIAL_MAJOR	204
 #define S3C24XX_SERIAL_MINOR	64
 
+#ifdef CONFIG_ARM64
+#define UART_NR			12
+#else
+#define UART_NR			CONFIG_SERIAL_SAMSUNG_UARTS
+#endif
+
 #define S3C24XX_TX_PIO			1
 #define S3C24XX_TX_DMA			2
 #define S3C24XX_RX_PIO			1
@@ -87,7 +93,7 @@  struct s3c24xx_uart_info {
 struct s3c24xx_serial_drv_data {
 	const struct s3c24xx_uart_info	info;
 	const struct s3c2410_uartcfg	def_cfg;
-	const unsigned int		fifosize[CONFIG_SERIAL_SAMSUNG_UARTS];
+	const unsigned int		fifosize[UART_NR];
 };
 
 struct s3c24xx_uart_dma {
@@ -1810,67 +1816,27 @@  static const struct uart_ops apple_s5l_serial_ops = {
 static struct uart_driver s3c24xx_uart_drv = {
 	.owner		= THIS_MODULE,
 	.driver_name	= "s3c2410_serial",
-	.nr		= CONFIG_SERIAL_SAMSUNG_UARTS,
+	.nr		= UART_NR,
 	.cons		= S3C24XX_SERIAL_CONSOLE,
 	.dev_name	= S3C24XX_SERIAL_NAME,
 	.major		= S3C24XX_SERIAL_MAJOR,
 	.minor		= S3C24XX_SERIAL_MINOR,
 };
 
-#define __PORT_LOCK_UNLOCKED(i) \
-	__SPIN_LOCK_UNLOCKED(s3c24xx_serial_ports[i].port.lock)
-static struct s3c24xx_uart_port
-s3c24xx_serial_ports[CONFIG_SERIAL_SAMSUNG_UARTS] = {
-	[0] = {
-		.port = {
-			.lock		= __PORT_LOCK_UNLOCKED(0),
-			.iotype		= UPIO_MEM,
-			.uartclk	= 0,
-			.fifosize	= 16,
-			.ops		= &s3c24xx_serial_ops,
-			.flags		= UPF_BOOT_AUTOCONF,
-			.line		= 0,
-		}
-	},
-	[1] = {
-		.port = {
-			.lock		= __PORT_LOCK_UNLOCKED(1),
-			.iotype		= UPIO_MEM,
-			.uartclk	= 0,
-			.fifosize	= 16,
-			.ops		= &s3c24xx_serial_ops,
-			.flags		= UPF_BOOT_AUTOCONF,
-			.line		= 1,
-		}
-	},
-#if CONFIG_SERIAL_SAMSUNG_UARTS > 2
-	[2] = {
-		.port = {
-			.lock		= __PORT_LOCK_UNLOCKED(2),
-			.iotype		= UPIO_MEM,
-			.uartclk	= 0,
-			.fifosize	= 16,
-			.ops		= &s3c24xx_serial_ops,
-			.flags		= UPF_BOOT_AUTOCONF,
-			.line		= 2,
-		}
-	},
-#endif
-#if CONFIG_SERIAL_SAMSUNG_UARTS > 3
-	[3] = {
-		.port = {
-			.lock		= __PORT_LOCK_UNLOCKED(3),
-			.iotype		= UPIO_MEM,
-			.uartclk	= 0,
-			.fifosize	= 16,
-			.ops		= &s3c24xx_serial_ops,
-			.flags		= UPF_BOOT_AUTOCONF,
-			.line		= 3,
-		}
-	}
-#endif
-};
-#undef __PORT_LOCK_UNLOCKED
+static struct s3c24xx_uart_port s3c24xx_serial_ports[UART_NR];
+
+static void s3c24xx_serial_init_port_default(int index) {
+	struct uart_port *port = &s3c24xx_serial_ports[index].port;
+
+	spin_lock_init(&port->lock);
+
+	port->iotype = UPIO_MEM;
+	port->uartclk = 0;
+	port->fifosize = 16;
+	port->ops = &s3c24xx_serial_ops;
+	port->flags = UPF_BOOT_AUTOCONF;
+	port->line = index;
+}
 
 /* s3c24xx_serial_resetport
  *
@@ -2186,6 +2152,8 @@  static int s3c24xx_serial_probe(struct platform_device *pdev)
 	}
 	ourport = &s3c24xx_serial_ports[index];
 
+	s3c24xx_serial_init_port_default(index);
+
 	ourport->drv_data = s3c24xx_get_driver_data(pdev);
 	if (!ourport->drv_data) {
 		dev_err(&pdev->dev, "could not find driver data\n");
@@ -2584,7 +2552,7 @@  s3c24xx_serial_console_setup(struct console *co, char *options)
 
 	/* is this a valid port */
 
-	if (co->index == -1 || co->index >= CONFIG_SERIAL_SAMSUNG_UARTS)
+	if (co->index == -1 || co->index >= UART_NR)
 		co->index = 0;
 
 	port = &s3c24xx_serial_ports[co->index].port;