[PATCH 0/3] 8250_men_mcb: Make UART port autoconfigurable

[PATCH 0/3] 8250_men_mcb: Make UART port autoconfigurable

[Rodríguez Barbarin, José Javier]

Make configuration be handled by the 8250 UART subsystem to avoid
weird behaviours and for better maintainability.

Also fix a couple configuration issues when using F215 duagon
boards and how the number of ports are calculated for IP Cores
Z025 and Z057.

Javier Rodriguez (3):
  8250_men_mcb: Add clockrate speed for G215/F215 boards
  8250_men_mcb: Read num ports from register data.
  8250_men_mcb: Make UART config auto configurable

 drivers/tty/serial/8250/8250_men_mcb.c | 206 ++++++++++++++++++-------
 1 file changed, 150 insertions(+), 56 deletions(-)

-- 
2.34.1

[PATCH 1/3] 8250_men_mcb: Add clockrate speed for G215/F215 boards

[Rodríguez Barbarin, José Javier]

Some F215 FPGA multifunction boards announce themselves as 215.
This leads to a misconfigured clockrate. The F215 is the same board
as G215 but with different cPCI interface so make them get the same
configuration

Co-developed-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Javier Rodriguez <josejavier.rodriguez@duagon.com>
---
 drivers/tty/serial/8250/8250_men_mcb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_men_mcb.c b/drivers/tty/serial/8250/8250_men_mcb.c
index f46ca13ff4aa..a2cdaeb61e00 100644
--- a/drivers/tty/serial/8250/8250_men_mcb.c
+++ b/drivers/tty/serial/8250/8250_men_mcb.c
@@ -37,10 +37,10 @@ static u32 men_lookup_uartclk(struct mcb_device *mdev)
 		clkval = 1041666;
 	else if (strncmp(mdev->bus->name, "F216", 4) == 0)
 		clkval = 1843200;
-	else if (strncmp(mdev->bus->name, "G215", 4) == 0)
-		clkval = 1843200;
 	else if (strncmp(mdev->bus->name, "F210", 4) == 0)
 		clkval = 115200;
+	else if (strstr(mdev->bus->name, "215"))
+		clkval = 1843200;
 	else
 		dev_info(&mdev->dev,
 			 "board not detected, using default uartclk\n");
-- 
2.34.1

[PATCH 2/3] 8250_men_mcb: Read num ports from register data.

[Rodríguez Barbarin, José Javier]

The IP Core Z025 and Z057 have a register where the amount of UART
ports is specified. Such register is located at offset 0x40.

This patch fixes the way the UART ports is calculated by reading
the actual register. Additionally a refactor was needed to achieve
this so we can keep track of the UART line and its offset which
also improves the remove callback.

Co-developed-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Javier Rodriguez <josejavier.rodriguez@duagon.com>
---
 drivers/tty/serial/8250/8250_men_mcb.c | 189 ++++++++++++++++++-------
 1 file changed, 139 insertions(+), 50 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_men_mcb.c b/drivers/tty/serial/8250/8250_men_mcb.c
index a2cdaeb61e00..d6cfebb3ee8f 100644
--- a/drivers/tty/serial/8250/8250_men_mcb.c
+++ b/drivers/tty/serial/8250/8250_men_mcb.c
@@ -12,11 +12,42 @@
 #define MEN_UART_ID_Z057 0x39
 #define MEN_UART_ID_Z125 0x7d
 
-#define MEN_UART_MEM_SIZE 0x10
+/*
+ * IP Cores Z025 and Z057 can have up to 4 UART
+ * The UARTs available are stored in a global
+ * register saved in physical address + 0x40
+ * Is saved as follows:
+ *
+ * 7                                                              0
+ * +------+-------+-------+-------+-------+-------+-------+-------+
+ * |UART4 | UART3 | UART2 | UART1 | U4irq | U3irq | U2irq | U1irq |
+ * +------+-------+-------+-------+-------+-------+-------+-------+
+ */
+#define MEN_UART1_MASK	0x01
+#define MEN_UART2_MASK	0x02
+#define MEN_UART3_MASK	0x04
+#define MEN_UART4_MASK	0x08
+
+#define MEN_Z125_UARTS_AVAILABLE	0x01
+
+#define MEN_Z025_MAX_UARTS		4
+#define MEN_UART_MEM_SIZE		0x10
+#define MEM_UART_REGISTER_SIZE		0x01
+#define MEN_Z025_REGISTER_OFFSET	0x40
+
+#define MEN_UART1_OFFSET	0
+#define MEN_UART2_OFFSET	(MEN_UART1_OFFSET + MEN_UART_MEM_SIZE)
+#define MEN_UART3_OFFSET	(MEN_UART2_OFFSET + MEN_UART_MEM_SIZE)
+#define MEN_UART4_OFFSET	(MEN_UART3_OFFSET + MEN_UART_MEM_SIZE)
+
+#define MEN_READ_REGISTER(addr)	readb((void *)addr)
+
+#define MAX_PORTS	4
 
 struct serial_8250_men_mcb_data {
-	struct uart_8250_port uart;
-	int line;
+	int num_ports;
+	unsigned int line[MAX_PORTS];
+	unsigned int offset[MAX_PORTS];
 };
 
 /*
@@ -50,16 +81,82 @@ static u32 men_lookup_uartclk(struct mcb_device *mdev)
 	return clkval;
 }
 
-static int get_num_ports(struct mcb_device *mdev,
-				  void __iomem *membase)
+static int read_uarts_available_from_register(void __iomem *membase,
+					      u8 *uarts_available)
+{
+	void __iomem *mem;
+	int reg_value;
+
+	mem = membase + MEN_Z025_REGISTER_OFFSET;
+
+	reg_value = MEN_READ_REGISTER(membase);
+
+	*uarts_available = reg_value >> 4;
+
+	return 0;
+}
+
+static int read_serial_data(struct mcb_device *mdev,
+			    void __iomem *membase,
+			    struct serial_8250_men_mcb_data *serial_data)
+{
+	u8 uarts_available;
+	int count = 0;
+	int mask;
+	int res;
+	int i;
+
+	res = read_uarts_available_from_register(membase, &uarts_available);
+	if (res < 0)
+		return res;
+
+	for (i = 0; i < MAX_PORTS; i++) {
+		mask = 0x1 << i;
+		switch (uarts_available & mask) {
+		case MEN_UART1_MASK:
+			serial_data->offset[count] = MEN_UART1_OFFSET;
+			count++;
+			break;
+		case MEN_UART2_MASK:
+			serial_data->offset[count] = MEN_UART2_OFFSET;
+			count++;
+			break;
+		case MEN_UART3_MASK:
+			serial_data->offset[count] = MEN_UART3_OFFSET;
+			count++;
+			break;
+		case MEN_UART4_MASK:
+			serial_data->offset[count] = MEN_UART4_OFFSET;
+			count++;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	if (count <= 0 || count > MAX_PORTS) {
+		dev_err(&mdev->dev, "unexpected number of ports: %u\n",
+			count);
+		return -ENODEV;
+	}
+
+	serial_data->num_ports = count;
+
+	return 0;
+}
+
+static int init_serial_data(struct mcb_device *mdev,
+			    void __iomem *membase,
+			    struct serial_8250_men_mcb_data *serial_data)
 {
 	switch (mdev->id) {
 	case MEN_UART_ID_Z125:
-		return 1U;
+		serial_data->num_ports = 1;
+		serial_data->offset[0] = 0;
+		return 0;
 	case MEN_UART_ID_Z025:
-		return readb(membase) >> 4;
 	case MEN_UART_ID_Z057:
-		return 4U;
+		return read_serial_data(mdev, membase, serial_data);
 	default:
 		dev_err(&mdev->dev, "no supported device!\n");
 		return -ENODEV;
@@ -69,11 +166,12 @@ static int get_num_ports(struct mcb_device *mdev,
 static int serial_8250_men_mcb_probe(struct mcb_device *mdev,
 				     const struct mcb_device_id *id)
 {
+	struct uart_8250_port uart;
 	struct serial_8250_men_mcb_data *data;
 	struct resource *mem;
-	int num_ports;
 	int i;
 	void __iomem *membase;
+	int res;
 
 	mem = mcb_get_resource(mdev, IORESOURCE_MEM);
 	if (mem == NULL)
@@ -82,49 +180,46 @@ static int serial_8250_men_mcb_probe(struct mcb_device *mdev,
 	if (IS_ERR(membase))
 		return PTR_ERR_OR_ZERO(membase);
 
-	num_ports = get_num_ports(mdev, membase);
-
-	dev_dbg(&mdev->dev, "found a 16z%03u with %u ports\n",
-		mdev->id, num_ports);
-
-	if (num_ports <= 0 || num_ports > 4) {
-		dev_err(&mdev->dev, "unexpected number of ports: %u\n",
-			num_ports);
-		return -ENODEV;
-	}
-
-	data = devm_kcalloc(&mdev->dev, num_ports,
+	data = devm_kzalloc(&mdev->dev,
 			    sizeof(struct serial_8250_men_mcb_data),
 			    GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
+	res = init_serial_data(mdev, membase, data);
+	if (res < 0)
+		return res;
+
+	dev_dbg(&mdev->dev, "found a 16z%03u with %u ports\n",
+		mdev->id, data->num_ports);
+
 	mcb_set_drvdata(mdev, data);
 
-	for (i = 0; i < num_ports; i++) {
-		data[i].uart.port.dev = mdev->dma_dev;
-		spin_lock_init(&data[i].uart.port.lock);
-
-		data[i].uart.port.type = PORT_16550;
-		data[i].uart.port.flags = UPF_SKIP_TEST | UPF_SHARE_IRQ
-					  | UPF_FIXED_TYPE;
-		data[i].uart.port.iotype = UPIO_MEM;
-		data[i].uart.port.uartclk = men_lookup_uartclk(mdev);
-		data[i].uart.port.regshift = 0;
-		data[i].uart.port.irq = mcb_get_irq(mdev);
-		data[i].uart.port.membase = membase;
-		data[i].uart.port.fifosize = 60;
-		data[i].uart.port.mapbase = (unsigned long) mem->start
-					    + i * MEN_UART_MEM_SIZE;
-		data[i].uart.port.iobase = data[i].uart.port.mapbase;
+	for (i = 0; i < data->num_ports; i++) {
+		uart.port.dev = mdev->dma_dev;
+		spin_lock_init(&uart.port.lock);
+
+		uart.port.type = PORT_16550;
+		uart.port.flags = UPF_SKIP_TEST |
+				  UPF_SHARE_IRQ |
+				  UPF_FIXED_TYPE;
+		uart.port.iotype = UPIO_MEM;
+		uart.port.uartclk = men_lookup_uartclk(mdev);
+		uart.port.regshift = 0;
+		uart.port.irq = mcb_get_irq(mdev);
+		uart.port.membase = membase;
+		uart.port.fifosize = 60;
+		uart.port.mapbase = (unsigned long) mem->start
+					    + data->offset[i];
+		uart.port.iobase = uart.port.mapbase;
 
 		/* ok, register the port */
-		data[i].line = serial8250_register_8250_port(&data[i].uart);
-		if (data[i].line < 0) {
+		data->line[i] = serial8250_register_8250_port(&uart);
+		if (data->line[i] < 0) {
 			dev_err(&mdev->dev, "unable to register UART port\n");
-			return data[i].line;
+			return data->line[i];
 		}
-		dev_info(&mdev->dev, "found MCB UART: ttyS%d\n", data[i].line);
+		dev_info(&mdev->dev, "found MCB UART: ttyS%d\n", data->line[i]);
 	}
 
 	return 0;
@@ -132,20 +227,14 @@ static int serial_8250_men_mcb_probe(struct mcb_device *mdev,
 
 static void serial_8250_men_mcb_remove(struct mcb_device *mdev)
 {
-	int num_ports, i;
+	int i;
 	struct serial_8250_men_mcb_data *data = mcb_get_drvdata(mdev);
 
 	if (!data)
 		return;
 
-	num_ports = get_num_ports(mdev, data[0].uart.port.membase);
-	if (num_ports <= 0 || num_ports > 4) {
-		dev_err(&mdev->dev, "error retrieving number of ports!\n");
-		return;
-	}
-
-	for (i = 0; i < num_ports; i++)
-		serial8250_unregister_port(data[i].line);
+	for (i = 0; i < data->num_ports; i++)
+		serial8250_unregister_port(data->line[i]);
 }
 
 static const struct mcb_device_id serial_8250_men_mcb_ids[] = {
-- 
2.34.1

[PATCH 3/3] 8250_men_mcb: Make UART config auto configurable

[Rodríguez Barbarin, José Javier]

The UART ports created by this driver were not usable out of
the box, so let the configuration be handled by the 8250 UART
subsystem. This makes the implementation simpler and the UART
port more usable.

The 8250 UART subsystem will take care of requesting the memory
resources, but the driver needs to first read the register where
the num ports is set, so a request of the resource is needed
before registering the UART port.

Co-developed-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Jorge Sanjuan Garcia <jorge.sanjuangarcia@duagon.com>
Signed-off-by: Javier Rodriguez <josejavier.rodriguez@duagon.com>
---
 drivers/tty/serial/8250/8250_men_mcb.c | 43 ++++++++++++++------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_men_mcb.c b/drivers/tty/serial/8250/8250_men_mcb.c
index d6cfebb3ee8f..c3143ffddea0 100644
--- a/drivers/tty/serial/8250/8250_men_mcb.c
+++ b/drivers/tty/serial/8250/8250_men_mcb.c
@@ -81,15 +81,28 @@ static u32 men_lookup_uartclk(struct mcb_device *mdev)
 	return clkval;
 }
 
-static int read_uarts_available_from_register(void __iomem *membase,
+static int read_uarts_available_from_register(struct resource *mem_res,
 					      u8 *uarts_available)
 {
 	void __iomem *mem;
 	int reg_value;
 
-	mem = membase + MEN_Z025_REGISTER_OFFSET;
+	if (!request_mem_region(mem_res->start + MEN_Z025_REGISTER_OFFSET,
+				MEM_UART_REGISTER_SIZE,  KBUILD_MODNAME)) {
+		return -EBUSY;
+	}
+
+	mem = ioremap(mem_res->start + MEN_Z025_REGISTER_OFFSET,
+		      MEM_UART_REGISTER_SIZE);
+	if (IS_ERR(mem))
+		return -ENOMEM;
+
+	reg_value = MEN_READ_REGISTER(mem);
+
+	iounmap(mem);
 
-	reg_value = MEN_READ_REGISTER(membase);
+	release_mem_region(mem_res->start + MEN_Z025_REGISTER_OFFSET,
+			   MEM_UART_REGISTER_SIZE);
 
 	*uarts_available = reg_value >> 4;
 
@@ -97,7 +110,7 @@ static int read_uarts_available_from_register(void __iomem *membase,
 }
 
 static int read_serial_data(struct mcb_device *mdev,
-			    void __iomem *membase,
+			    struct resource *mem_res,
 			    struct serial_8250_men_mcb_data *serial_data)
 {
 	u8 uarts_available;
@@ -106,7 +119,7 @@ static int read_serial_data(struct mcb_device *mdev,
 	int res;
 	int i;
 
-	res = read_uarts_available_from_register(membase, &uarts_available);
+	res = read_uarts_available_from_register(mem_res, &uarts_available);
 	if (res < 0)
 		return res;
 
@@ -146,7 +159,7 @@ static int read_serial_data(struct mcb_device *mdev,
 }
 
 static int init_serial_data(struct mcb_device *mdev,
-			    void __iomem *membase,
+			    struct resource *mem_res,
 			    struct serial_8250_men_mcb_data *serial_data)
 {
 	switch (mdev->id) {
@@ -156,7 +169,7 @@ static int init_serial_data(struct mcb_device *mdev,
 		return 0;
 	case MEN_UART_ID_Z025:
 	case MEN_UART_ID_Z057:
-		return read_serial_data(mdev, membase, serial_data);
+		return read_serial_data(mdev, mem_res, serial_data);
 	default:
 		dev_err(&mdev->dev, "no supported device!\n");
 		return -ENODEV;
@@ -170,15 +183,11 @@ static int serial_8250_men_mcb_probe(struct mcb_device *mdev,
 	struct serial_8250_men_mcb_data *data;
 	struct resource *mem;
 	int i;
-	void __iomem *membase;
 	int res;
 
 	mem = mcb_get_resource(mdev, IORESOURCE_MEM);
 	if (mem == NULL)
 		return -ENXIO;
-	membase = devm_ioremap_resource(&mdev->dev, mem);
-	if (IS_ERR(membase))
-		return PTR_ERR_OR_ZERO(membase);
 
 	data = devm_kzalloc(&mdev->dev,
 			    sizeof(struct serial_8250_men_mcb_data),
@@ -186,7 +195,7 @@ static int serial_8250_men_mcb_probe(struct mcb_device *mdev,
 	if (!data)
 		return -ENOMEM;
 
-	res = init_serial_data(mdev, membase, data);
+	res = init_serial_data(mdev, mem, data);
 	if (res < 0)
 		return res;
 
@@ -196,22 +205,18 @@ static int serial_8250_men_mcb_probe(struct mcb_device *mdev,
 	mcb_set_drvdata(mdev, data);
 
 	for (i = 0; i < data->num_ports; i++) {
-		uart.port.dev = mdev->dma_dev;
+		memset(&uart, 0, sizeof(struct uart_8250_port));
 		spin_lock_init(&uart.port.lock);
 
-		uart.port.type = PORT_16550;
 		uart.port.flags = UPF_SKIP_TEST |
 				  UPF_SHARE_IRQ |
-				  UPF_FIXED_TYPE;
+				  UPF_BOOT_AUTOCONF |
+				  UPF_IOREMAP;
 		uart.port.iotype = UPIO_MEM;
 		uart.port.uartclk = men_lookup_uartclk(mdev);
-		uart.port.regshift = 0;
 		uart.port.irq = mcb_get_irq(mdev);
-		uart.port.membase = membase;
-		uart.port.fifosize = 60;
 		uart.port.mapbase = (unsigned long) mem->start
 					    + data->offset[i];
-		uart.port.iobase = uart.port.mapbase;
 
 		/* ok, register the port */
 		data->line[i] = serial8250_register_8250_port(&uart);
-- 
2.34.1

Re: [PATCH 3/3] 8250_men_mcb: Make UART config auto configurable

[andy.shevchenko]

Wed, Jul 05, 2023 at 01:15:14PM +0000, Rodríguez Barbarin, José Javier kirjoitti:
> The UART ports created by this driver were not usable out of
> the box, so let the configuration be handled by the 8250 UART
> subsystem. This makes the implementation simpler and the UART
> port more usable.
> 
> The 8250 UART subsystem will take care of requesting the memory
> resources, but the driver needs to first read the register where
> the num ports is set, so a request of the resource is needed
> before registering the UART port.

I see this is already pending to the next cylce, but
formally I would like to NAK this change as explained
in reply to cover letter why.

...

Side note:

>  		uart.port.mapbase = (unsigned long) mem->start
>  					    + data->offset[i];

Weird indentation and strange casting.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] 8250_men_mcb: Make UART config auto configurable

[andy.shevchenko]

Wed, Jul 26, 2023 at 11:44:46AM +0300, andy.shevchenko@gmail.com kirjoitti:
> Wed, Jul 05, 2023 at 01:15:14PM +0000, Rodríguez Barbarin, José Javier kirjoitti:
> > The UART ports created by this driver were not usable out of
> > the box, so let the configuration be handled by the 8250 UART
> > subsystem. This makes the implementation simpler and the UART
> > port more usable.
> > 
> > The 8250 UART subsystem will take care of requesting the memory
> > resources, but the driver needs to first read the register where
> > the num ports is set, so a request of the resource is needed
> > before registering the UART port.
> 
> I see this is already pending to the next cylce, but
> formally I would like to NAK this change as explained
> in reply to cover letter why.

Okay, after a bit of digging it seems not so bad as I was thinking initially.
The IRQ test requires specific flag which seems is not set in this case.
This removes my main worries.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] 8250_men_mcb: Make UART port autoconfigurable

[andy.shevchenko]

Wed, Jul 05, 2023 at 01:14:51PM +0000, Rodríguez Barbarin, José Javier kirjoitti:
> Make configuration be handled by the 8250 UART subsystem

Actually this is not the best idea. 

> to avoid weird behaviours 

The opposite.

8250 detection is full of quirks and was developed to handle tons of different
UARTs when the driver was in a single file. Since you have a separate 8250_*.c
module for your UART and you _know_ the type beforehand, why on earth you need
to rely on the old and maybe not very suitable code? Have you thought about
corner cases with IRQ detection, for example?

> and for better maintainability.

The opposite.

I don't know if it affects your hardware to the date, but it may be different
for the future models, or models that you hadn't tested.

That said, I highly recommend to reconsider.

-- 
With Best Regards,
Andy Shevchenko