[PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"

[PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"

[Michael Walle]

This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.

This broke the earlycon on LS1021A processors because the order of the
earlycon_setup() functions were changed. Before the commit the normal
lpuart32_early_console_setup() was called. After the commit the
lpuart32_imx_early_console_setup() is called instead.

Fixes: a659652f6169 ("tty: serial: fsl_lpuart: drop EARLYCON_DECLARE")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 91e2805e6441..27fdc131c352 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2390,6 +2390,8 @@ static int __init lpuart32_imx_early_console_setup(struct earlycon_device *devic
 OF_EARLYCON_DECLARE(lpuart, "fsl,vf610-lpuart", lpuart_early_console_setup);
 OF_EARLYCON_DECLARE(lpuart32, "fsl,ls1021a-lpuart", lpuart32_early_console_setup);
 OF_EARLYCON_DECLARE(lpuart32, "fsl,imx7ulp-lpuart", lpuart32_imx_early_console_setup);
+EARLYCON_DECLARE(lpuart, lpuart_early_console_setup);
+EARLYCON_DECLARE(lpuart32, lpuart32_early_console_setup);
 
 #define LPUART_CONSOLE	(&lpuart_console)
 #define LPUART32_CONSOLE	(&lpuart32_console)
-- 
2.20.1

[PATCH 2/7] tty: serial: fsl_lpuart: free IDs allocated by IDA

[Michael Walle]

Since commit 3bc3206e1c0f ("serial: fsl_lpuart: Remove the alias node
dependence") the port line number can also be allocated by IDA, but in
case of an error the ID will no be removed again. More importantly, any
ID will be freed in remove(), even if it wasn't allocated but instead
fetched by of_alias_get_id(). If it was not allocated by IDA there will
be a warning:
  WARN(1, "ida_free called for id=%d which is not allocated.\n", id);

Move the ID allocation more to the end of the probe() so that we still
can use plain return in the first error cases.

Fixes: 3bc3206e1c0f ("serial: fsl_lpuart: Remove the alias node dependence")
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 39 ++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 27fdc131c352..c31b8f3db6bf 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -264,6 +264,7 @@ struct lpuart_port {
 	int			rx_dma_rng_buf_len;
 	unsigned int		dma_tx_nents;
 	wait_queue_head_t	dma_wait;
+	bool			id_allocated;
 };
 
 struct lpuart_soc_data {
@@ -2422,19 +2423,6 @@ static int lpuart_probe(struct platform_device *pdev)
 	if (!sport)
 		return -ENOMEM;
 
-	ret = of_alias_get_id(np, "serial");
-	if (ret < 0) {
-		ret = ida_simple_get(&fsl_lpuart_ida, 0, UART_NR, GFP_KERNEL);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "port line is full, add device failed\n");
-			return ret;
-		}
-	}
-	if (ret >= ARRAY_SIZE(lpuart_ports)) {
-		dev_err(&pdev->dev, "serial%d out of range\n", ret);
-		return -EINVAL;
-	}
-	sport->port.line = ret;
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sport->port.membase = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(sport->port.membase))
@@ -2479,9 +2467,25 @@ static int lpuart_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = of_alias_get_id(np, "serial");
+	if (ret < 0) {
+		ret = ida_simple_get(&fsl_lpuart_ida, 0, UART_NR, GFP_KERNEL);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "port line is full, add device failed\n");
+			return ret;
+		}
+		sport->id_allocated = true;
+	}
+	if (ret >= ARRAY_SIZE(lpuart_ports)) {
+		dev_err(&pdev->dev, "serial%d out of range\n", ret);
+		ret = -EINVAL;
+		goto failed_out_of_range;
+	}
+	sport->port.line = ret;
+
 	ret = lpuart_enable_clks(sport);
 	if (ret)
-		return ret;
+		goto failed_clock_enable;
 	sport->port.uartclk = lpuart_get_baud_clk_rate(sport);
 
 	lpuart_ports[sport->port.line] = sport;
@@ -2531,6 +2535,10 @@ static int lpuart_probe(struct platform_device *pdev)
 failed_attach_port:
 failed_irq_request:
 	lpuart_disable_clks(sport);
+failed_clock_enable:
+failed_out_of_range:
+	if (sport->id_allocated)
+		ida_simple_remove(&fsl_lpuart_ida, sport->port.line);
 	return ret;
 }
 
@@ -2540,7 +2548,8 @@ static int lpuart_remove(struct platform_device *pdev)
 
 	uart_remove_one_port(&lpuart_reg, &sport->port);
 
-	ida_simple_remove(&fsl_lpuart_ida, sport->port.line);
+	if (sport->id_allocated)
+		ida_simple_remove(&fsl_lpuart_ida, sport->port.line);
 
 	lpuart_disable_clks(sport);
 
-- 
2.20.1

[PATCH 3/7] tty: serial: fsl_lpuart: handle EPROBE_DEFER for DMA

[Michael Walle]

The DMA channel might not be available at the first probe time. This is
esp. the case if the DMA controller has an IOMMU mapping.

Use the new dma_request_chan() API and handle EPROBE_DEFER errors. Also
reorder the code a bit, so that we don't prepare the whole UART just to
determine that the DMA channel is not ready yet and we have to undo all
the stuff. Try to map the DMA channels earlier.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 35 +++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index c31b8f3db6bf..fd9f60d0817a 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2416,6 +2416,7 @@ static int lpuart_probe(struct platform_device *pdev)
 	const struct lpuart_soc_data *sdata = of_id->data;
 	struct device_node *np = pdev->dev.of_node;
 	struct lpuart_port *sport;
+	struct dma_chan *dma_chan;
 	struct resource *res;
 	int ret;
 
@@ -2483,6 +2484,26 @@ static int lpuart_probe(struct platform_device *pdev)
 	}
 	sport->port.line = ret;
 
+	dma_chan = dma_request_chan(sport->port.dev, "tx");
+	if (PTR_ERR(dma_chan) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto failed_request_tx_dma;
+	} else if (IS_ERR(dma_chan))
+		dev_info(sport->port.dev, "DMA tx channel request failed, "
+				"operating without tx DMA\n");
+	else
+		sport->dma_tx_chan = dma_chan;
+
+	dma_chan = dma_request_chan(sport->port.dev, "rx");
+	if (PTR_ERR(dma_chan) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto failed_request_rx_dma;
+	} else if (IS_ERR(dma_chan))
+		dev_info(sport->port.dev, "DMA rx channel request failed, "
+				"operating without rx DMA\n");
+	else
+		sport->dma_rx_chan = dma_chan;
+
 	ret = lpuart_enable_clks(sport);
 	if (ret)
 		goto failed_clock_enable;
@@ -2520,22 +2541,16 @@ static int lpuart_probe(struct platform_device *pdev)
 
 	sport->port.rs485_config(&sport->port, &sport->port.rs485);
 
-	sport->dma_tx_chan = dma_request_slave_channel(sport->port.dev, "tx");
-	if (!sport->dma_tx_chan)
-		dev_info(sport->port.dev, "DMA tx channel request failed, "
-				"operating without tx DMA\n");
-
-	sport->dma_rx_chan = dma_request_slave_channel(sport->port.dev, "rx");
-	if (!sport->dma_rx_chan)
-		dev_info(sport->port.dev, "DMA rx channel request failed, "
-				"operating without rx DMA\n");
-
 	return 0;
 
 failed_attach_port:
 failed_irq_request:
 	lpuart_disable_clks(sport);
 failed_clock_enable:
+	dma_release_channel(sport->dma_rx_chan);
+failed_request_rx_dma:
+	dma_release_channel(sport->dma_tx_chan);
+failed_request_tx_dma:
 failed_out_of_range:
 	if (sport->id_allocated)
 		ida_simple_remove(&fsl_lpuart_ida, sport->port.line);
-- 
2.20.1

[PATCH 4/7] dt-bindings: serial: lpuart: add ls1028a compatibility

[Michael Walle]

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../devicetree/bindings/serial/fsl-lpuart.txt          | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/fsl-lpuart.txt b/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
index c904e2e68332..e7448b92dd9d 100644
--- a/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
+++ b/Documentation/devicetree/bindings/serial/fsl-lpuart.txt
@@ -6,6 +6,8 @@ Required properties:
     on Vybrid vf610 SoC with 8-bit register organization
   - "fsl,ls1021a-lpuart" for lpuart compatible with the one integrated
     on LS1021A SoC with 32-bit big-endian register organization
+  - "fsl,ls1028a-lpuart" for lpuart compatible with the one integrated
+    on LS1028A SoC with 32-bit little-endian register organization
   - "fsl,imx7ulp-lpuart" for lpuart compatible with the one integrated
     on i.MX7ULP SoC with 32-bit little-endian register organization
   - "fsl,imx8qxp-lpuart" for lpuart compatible with the one integrated
@@ -15,10 +17,10 @@ Required properties:
 - reg : Address and length of the register set for the device
 - interrupts : Should contain uart interrupt
 - clocks : phandle + clock specifier pairs, one for each entry in clock-names
-- clock-names : For vf610/ls1021a/imx7ulp, "ipg" clock is for uart bus/baud
-  clock. For imx8qxp lpuart, "ipg" clock is bus clock that is used to access
-  lpuart controller registers, it also requires "baud" clock for module to
-  receive/transmit data.
+- clock-names : For vf610/ls1021a/ls1028a/imx7ulp, "ipg" clock is for uart
+  bus/baud clock. For imx8qxp lpuart, "ipg" clock is bus clock that is used
+  to access lpuart controller registers, it also requires "baud" clock for
+  module to receive/transmit data.
 
 Optional properties:
 - dmas: A list of two dma specifiers, one for each entry in dma-names.
-- 
2.20.1

[PATCH 5/7] tty: serial: fsl_lpuart: add LS1028A support

[Michael Walle]

The LS1028A uses little endian register access and has a different FIFO
size encoding.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index fd9f60d0817a..6953eea166dc 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -234,6 +234,7 @@ static DEFINE_IDA(fsl_lpuart_ida);
 enum lpuart_type {
 	VF610_LPUART,
 	LS1021A_LPUART,
+	LS1028A_LPUART,
 	IMX7ULP_LPUART,
 	IMX8QXP_LPUART,
 };
@@ -278,11 +279,16 @@ static const struct lpuart_soc_data vf_data = {
 	.iotype = UPIO_MEM,
 };
 
-static const struct lpuart_soc_data ls_data = {
+static const struct lpuart_soc_data ls1021a_data = {
 	.devtype = LS1021A_LPUART,
 	.iotype = UPIO_MEM32BE,
 };
 
+static const struct lpuart_soc_data ls1028a_data = {
+	.devtype = LS1028A_LPUART,
+	.iotype = UPIO_MEM32,
+};
+
 static struct lpuart_soc_data imx7ulp_data = {
 	.devtype = IMX7ULP_LPUART,
 	.iotype = UPIO_MEM32,
@@ -297,7 +303,8 @@ static struct lpuart_soc_data imx8qxp_data = {
 
 static const struct of_device_id lpuart_dt_ids[] = {
 	{ .compatible = "fsl,vf610-lpuart",	.data = &vf_data, },
-	{ .compatible = "fsl,ls1021a-lpuart",	.data = &ls_data, },
+	{ .compatible = "fsl,ls1021a-lpuart",	.data = &ls1021a_data, },
+	{ .compatible = "fsl,ls1028a-lpuart",	.data = &ls1028a_data, },
 	{ .compatible = "fsl,imx7ulp-lpuart",	.data = &imx7ulp_data, },
 	{ .compatible = "fsl,imx8qxp-lpuart",	.data = &imx8qxp_data, },
 	{ /* sentinel */ }
@@ -307,6 +314,11 @@ MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
 /* Forward declare this for the dma callbacks*/
 static void lpuart_dma_tx_complete(void *arg);
 
+static inline bool is_ls1028a_lpuart(struct lpuart_port *sport)
+{
+	return sport->devtype == LS1028A_LPUART;
+}
+
 static inline bool is_imx8qxp_lpuart(struct lpuart_port *sport)
 {
 	return sport->devtype == IMX8QXP_LPUART;
@@ -1589,6 +1601,17 @@ static int lpuart32_startup(struct uart_port *port)
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
+	/*
+	 * The LS1028A has a fixed length of 16 words. Although it supports the
+	 * RX/TXSIZE fields their encoding is different. Eg the reference manual
+	 * states 0b101 is 16 words.
+	 */
+	if (is_ls1028a_lpuart(sport)) {
+		sport->rxfifo_size = 16;
+		sport->txfifo_size = 16;
+		sport->port.fifosize = sport->txfifo_size;
+	}
+
 	lpuart32_setup_watermark_enable(sport);
 
 
-- 
2.20.1

[PATCH 6/7] tty: serial: fsl_lpuart: add LS1028A earlycon support

[Michael Walle]

Add a early_console_setup() for the LS1028A SoC with 32bit, little
endian access. If the bootloader does a fixup of the clock-frequency
node the baudrate divisor register will automatically be set.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 51 +++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 6953eea166dc..4386ffdfe275 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1834,11 +1834,12 @@ lpuart_set_termios(struct uart_port *port, struct ktermios *termios,
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
-static void
-lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int baudrate)
+static void __lpuart32_serial_setbrg(struct uart_port *port,
+				     unsigned int baudrate, bool use_rx_dma,
+				     bool use_tx_dma)
 {
 	u32 sbr, osr, baud_diff, tmp_osr, tmp_sbr, tmp_diff, tmp;
-	u32 clk = sport->port.uartclk;
+	u32 clk = port->uartclk;
 
 	/*
 	 * The idea is to use the best OSR (over-sampling rate) possible.
@@ -1884,10 +1885,10 @@ lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int baudrate)
 
 	/* handle buadrate outside acceptable rate */
 	if (baud_diff > ((baudrate / 100) * 3))
-		dev_warn(sport->port.dev,
+		dev_warn(port->dev,
 			 "unacceptable baud rate difference of more than 3%%\n");
 
-	tmp = lpuart32_read(&sport->port, UARTBAUD);
+	tmp = lpuart32_read(port, UARTBAUD);
 
 	if ((osr > 3) && (osr < 8))
 		tmp |= UARTBAUD_BOTHEDGE;
@@ -1898,14 +1899,23 @@ lpuart32_serial_setbrg(struct lpuart_port *sport, unsigned int baudrate)
 	tmp &= ~UARTBAUD_SBR_MASK;
 	tmp |= sbr & UARTBAUD_SBR_MASK;
 
-	if (!sport->lpuart_dma_rx_use)
+	if (!use_rx_dma)
 		tmp &= ~UARTBAUD_RDMAE;
-	if (!sport->lpuart_dma_tx_use)
+	if (!use_tx_dma)
 		tmp &= ~UARTBAUD_TDMAE;
 
-	lpuart32_write(&sport->port, tmp, UARTBAUD);
+	lpuart32_write(port, tmp, UARTBAUD);
+}
+
+static void lpuart32_serial_setbrg(struct lpuart_port *sport,
+				   unsigned int baudrate)
+{
+	__lpuart32_serial_setbrg(&sport->port, baudrate,
+				 sport->lpuart_dma_rx_use,
+				 sport->lpuart_dma_tx_use);
 }
 
+
 static void
 lpuart32_set_termios(struct uart_port *port, struct ktermios *termios,
 		   struct ktermios *old)
@@ -2399,6 +2409,30 @@ static int __init lpuart32_early_console_setup(struct earlycon_device *device,
 	return 0;
 }
 
+static int __init ls1028a_early_console_setup(struct earlycon_device *device,
+					      const char *opt)
+{
+	u32 cr;
+
+	if (!device->port.membase)
+		return -ENODEV;
+
+	device->port.iotype = UPIO_MEM32;
+	device->con->write = lpuart32_early_write;
+
+	/* set the baudrate */
+	if (device->port.uartclk && device->baud)
+		__lpuart32_serial_setbrg(&device->port, device->baud,
+					 false, false);
+
+	/* enable transmitter */
+	cr = lpuart32_read(&device->port, UARTCTRL);
+	cr |= UARTCTRL_TE;
+	lpuart32_write(&device->port, cr, UARTCTRL);
+
+	return 0;
+}
+
 static int __init lpuart32_imx_early_console_setup(struct earlycon_device *device,
 						   const char *opt)
 {
@@ -2413,6 +2447,7 @@ static int __init lpuart32_imx_early_console_setup(struct earlycon_device *devic
 }
 OF_EARLYCON_DECLARE(lpuart, "fsl,vf610-lpuart", lpuart_early_console_setup);
 OF_EARLYCON_DECLARE(lpuart32, "fsl,ls1021a-lpuart", lpuart32_early_console_setup);
+OF_EARLYCON_DECLARE(lpuart32, "fsl,ls1028a-lpuart", ls1028a_early_console_setup);
 OF_EARLYCON_DECLARE(lpuart32, "fsl,imx7ulp-lpuart", lpuart32_imx_early_console_setup);
 EARLYCON_DECLARE(lpuart, lpuart_early_console_setup);
 EARLYCON_DECLARE(lpuart32, lpuart32_early_console_setup);
-- 
2.20.1

[PATCH 7/7] arm64: dts: ls1028a: add missing LPUART nodes

[Michael Walle]

The LS1028A has six LPUART controllers. Add the nodes.

This was tested on a custom board.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
index 40d82c49b174..f30ffc46063d 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -390,6 +390,61 @@
 			status = "disabled";
 		};
 
+
+		lpuart0: serial@2260000 {
+			compatible = "fsl,ls1028a-lpuart";
+			reg = <0x0 0x2260000 0x0 0x1000>;
+			interrupts = <GIC_SPI 232 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clockgen 4 1>;
+			clock-names = "ipg";
+			status = "disabled";
+		};
+
+		lpuart1: serial@2270000 {
+			compatible = "fsl,ls1028a-lpuart";
+			reg = <0x0 0x2270000 0x0 0x1000>;
+			interrupts = <GIC_SPI 233 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clockgen 4 1>;
+			clock-names = "ipg";
+			status = "disabled";
+		};
+
+		lpuart2: serial@2280000 {
+			compatible = "fsl,ls1028a-lpuart";
+			reg = <0x0 0x2280000 0x0 0x1000>;
+			interrupts = <GIC_SPI 234 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clockgen 4 1>;
+			clock-names = "ipg";
+			status = "disabled";
+		};
+
+		lpuart3: serial@2290000 {
+			compatible = "fsl,ls1028a-lpuart";
+			reg = <0x0 0x2290000 0x0 0x1000>;
+			interrupts = <GIC_SPI 235 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clockgen 4 1>;
+			clock-names = "ipg";
+			status = "disabled";
+		};
+
+		lpuart4: serial@22a0000 {
+			compatible = "fsl,ls1028a-lpuart";
+			reg = <0x0 0x22a0000 0x0 0x1000>;
+			interrupts = <GIC_SPI 236 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clockgen 4 1>;
+			clock-names = "ipg";
+			status = "disabled";
+		};
+
+		lpuart5: serial@22b0000 {
+			compatible = "fsl,ls1028a-lpuart";
+			reg = <0x0 0x22b0000 0x0 0x1000>;
+			interrupts = <GIC_SPI 237 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clockgen 4 1>;
+			clock-names = "ipg";
+			status = "disabled";
+		};
+
 		edma0: dma-controller@22c0000 {
 			#dma-cells = <2>;
 			compatible = "fsl,ls1028a-edma";
-- 
2.20.1

RE: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"

[Peng Fan]

> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"
> 
> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
> 
> This broke the earlycon on LS1021A processors because the order of the
> earlycon_setup() functions were changed. Before the commit the normal
> lpuart32_early_console_setup() was called. After the commit the
> lpuart32_imx_early_console_setup() is called instead.

How do you pass earlycon args to kernel?

Regards,
Peng. 

> 
> Fixes: a659652f6169 ("tty: serial: fsl_lpuart: drop EARLYCON_DECLARE")
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index
> 91e2805e6441..27fdc131c352 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2390,6 +2390,8 @@ static int __init
> lpuart32_imx_early_console_setup(struct earlycon_device *devic
> OF_EARLYCON_DECLARE(lpuart, "fsl,vf610-lpuart",
> lpuart_early_console_setup);  OF_EARLYCON_DECLARE(lpuart32,
> "fsl,ls1021a-lpuart", lpuart32_early_console_setup);
> OF_EARLYCON_DECLARE(lpuart32, "fsl,imx7ulp-lpuart",
> lpuart32_imx_early_console_setup);
> +EARLYCON_DECLARE(lpuart, lpuart_early_console_setup);
> +EARLYCON_DECLARE(lpuart32, lpuart32_early_console_setup);
> 
>  #define LPUART_CONSOLE	(&lpuart_console)
>  #define LPUART32_CONSOLE	(&lpuart32_console)
> --
> 2.20.1

Re: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"

[Michael Walle]

Hi,

Am 2020-02-21 02:30, schrieb Peng Fan:
>> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop 
>> EARLYCON_DECLARE"
>> 
>> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
>> 
>> This broke the earlycon on LS1021A processors because the order of the
>> earlycon_setup() functions were changed. Before the commit the normal
>> lpuart32_early_console_setup() was called. After the commit the
>> lpuart32_imx_early_console_setup() is called instead.
> 
> How do you pass earlycon args to kernel?

earlycon=lpuart32,mmio32be,0x2950000,115200

please note that there are two possible declarations: (1) an OF/ACPI 
based
earlycon, eg just "earlycon" on the bootargs and (2) an elaborate one 
where
you can give the offset and access method yourself, eg. the one from 
above.

(1) will still work even with the EARLYCON_DECLARE() removed. But (2) 
will
search through all possible
   OF_DELARE_EARLYCON(lpuart32,..)
   EARLYCON_DECLARE(lpuart32,..)

and doesn't take the compatible into account. So which setup function is
actually called depends on (a) the order of the OF_DECLARE_EARLYCON() 
and
EARLYCON_DECLARE() statements and (b) on the compiler (thats just a
guess!). For me, the order in which it will actually end up in the
__earlycon_table is reversed, eg. the last one is called. So now that
you've removed the EARLYCON_DECLARE() the last one is the imx setup
function which will add the reg offset and doesn't work on LS1021A.

I've proposed a fix of the underlying problem [1]. But that fix also 
requires
the EARLYCON_DECLARE() in this driver.


-michael

[1] 
https://lore.kernel.org/linux-serial/20200220174607.24285-1-michael@walle.cc/

RE: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"

[Peng Fan]

> Subject: Re: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
> EARLYCON_DECLARE"
> 
> Hi,
> 
> Am 2020-02-21 02:30, schrieb Peng Fan:
> >> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
> >> EARLYCON_DECLARE"
> >>
> >> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
> >>
> >> This broke the earlycon on LS1021A processors because the order of
> >> the
> >> earlycon_setup() functions were changed. Before the commit the normal
> >> lpuart32_early_console_setup() was called. After the commit the
> >> lpuart32_imx_early_console_setup() is called instead.
> >
> > How do you pass earlycon args to kernel?
> 
> earlycon=lpuart32,mmio32be,0x2950000,115200
> 
> please note that there are two possible declarations: (1) an OF/ACPI based
> earlycon, eg just "earlycon" on the bootargs and (2) an elaborate one where
> you can give the offset and access method yourself, eg. the one from above.
> 
> (1) will still work even with the EARLYCON_DECLARE() removed. But (2) will
> search through all possible
>    OF_DELARE_EARLYCON(lpuart32,..)
>    EARLYCON_DECLARE(lpuart32,..)
> 
> and doesn't take the compatible into account. So which setup function is
> actually called depends on (a) the order of the OF_DECLARE_EARLYCON() and
> EARLYCON_DECLARE() statements and (b) on the compiler (thats just a guess!).
> For me, the order in which it will actually end up in the __earlycon_table is
> reversed, eg. the last one is called. So now that you've removed the
> EARLYCON_DECLARE() the last one is the imx setup function which will add
> the reg offset and doesn't work on LS1021A.

You mean the OF_DECLARD_EARLYCON for i.MX7ULP will be put before LS1021A
in Image? I am not sure about this. If this is true, you could try below diff, to see
whether it works. i.MX always use little endian.

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 91e2805e6441..1b0aa3b836c5 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -2381,8 +2381,10 @@ static int __init lpuart32_imx_early_console_setup(struct earlycon_device *devic
        if (!device->port.membase)
                return -ENODEV;

-       device->port.iotype = UPIO_MEM32;
-       device->port.membase += IMX_REG_OFF;
+       if (device->port.iotype != UPIO_MEM32BE) {
+               device->port.iotype = UPIO_MEM32;
+               device->port.membase += IMX_REG_OFF;
+       }
        device->con->write = lpuart32_early_write;

        return 0;

Thanks,
Peng.

> 
> I've proposed a fix of the underlying problem [1]. But that fix also requires the
> EARLYCON_DECLARE() in this driver.
> 
> 
> -michael
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke
> rnel.org%2Flinux-serial%2F20200220174607.24285-1-michael%40walle.cc%
> 2F&data=02%7C01%7Cpeng.fan%40nxp.com%7Cc411bf1d5d45435be2a
> 308d7b6b14a2c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63
> 7178744883261681&sdata=gjl8rie%2FXhUQ0pxcAbbDI4NGqGDYj1jpqfR
> mRi%2FAgFk%3D&reserved=0

Re: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop EARLYCON_DECLARE"

[Michael Walle]

Hi,


Am 2020-02-24 02:12, schrieb Peng Fan:
>> Subject: Re: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
>> EARLYCON_DECLARE"
>> 
>> Hi,
>> 
>> Am 2020-02-21 02:30, schrieb Peng Fan:
>> >> Subject: [PATCH 1/7] Revert "tty: serial: fsl_lpuart: drop
>> >> EARLYCON_DECLARE"
>> >>
>> >> This reverts commit a659652f6169240a5818cb244b280c5a362ef5a4.
>> >>
>> >> This broke the earlycon on LS1021A processors because the order of
>> >> the
>> >> earlycon_setup() functions were changed. Before the commit the normal
>> >> lpuart32_early_console_setup() was called. After the commit the
>> >> lpuart32_imx_early_console_setup() is called instead.
>> >
>> > How do you pass earlycon args to kernel?
>> 
>> earlycon=lpuart32,mmio32be,0x2950000,115200
>> 
>> please note that there are two possible declarations: (1) an OF/ACPI 
>> based
>> earlycon, eg just "earlycon" on the bootargs and (2) an elaborate one 
>> where
>> you can give the offset and access method yourself, eg. the one from 
>> above.
>> 
>> (1) will still work even with the EARLYCON_DECLARE() removed. But (2) 
>> will
>> search through all possible
>>    OF_DELARE_EARLYCON(lpuart32,..)
>>    EARLYCON_DECLARE(lpuart32,..)
>> 
>> and doesn't take the compatible into account. So which setup function 
>> is
>> actually called depends on (a) the order of the OF_DECLARE_EARLYCON() 
>> and
>> EARLYCON_DECLARE() statements and (b) on the compiler (thats just a 
>> guess!).
>> For me, the order in which it will actually end up in the 
>> __earlycon_table is
>> reversed, eg. the last one is called. So now that you've removed the
>> EARLYCON_DECLARE() the last one is the imx setup function which will 
>> add
>> the reg offset and doesn't work on LS1021A.
> 
> You mean the OF_DECLARD_EARLYCON for i.MX7ULP will be put before 
> LS1021A
> in Image? I am not sure about this. If this is true, you could try
> below diff, to see

well, you could have a look at the disassembly; so no need for real 
hardware,
if thats what you're missing.


> whether it works. i.MX always use little endian.
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c 
> b/drivers/tty/serial/fsl_lpuart.c
> index 91e2805e6441..1b0aa3b836c5 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2381,8 +2381,10 @@ static int __init
> lpuart32_imx_early_console_setup(struct earlycon_device *devic
>         if (!device->port.membase)
>                 return -ENODEV;
> 
> -       device->port.iotype = UPIO_MEM32;
> -       device->port.membase += IMX_REG_OFF;
> +       if (device->port.iotype != UPIO_MEM32BE) {
> +               device->port.iotype = UPIO_MEM32;
> +               device->port.membase += IMX_REG_OFF;
> +       }
>         device->con->write = lpuart32_early_write;
> 
>         return 0;

while that would also fix the problem (I guess; I'm too lazy to try), it 
is
really bad because:
  (A) it doesn't fix the underlying problem that there are multiple setup
      functions for lpuart32 (and this is not only a problem for 
lpuart32)
  (B) it would've broken the lpuart32 for IMX in the form (2), if it had 
ever
      been working.
  (C) it will break if some developer adds a new setup function to the 
end of
      the list
  (D) you basically make every setup() function behave like the ls1021a 
setup()
      if its called in form (2)

Please, lets try to find a more generic solution for this, like my 
proposed
patch [1].

-michael

[1] 
https://lore.kernel.org/linux-serial/20200220174607.24285-1-michael@walle.cc