[PATCH v4 0/4] tty: serial: fsl_lpuart: various fixes and LS1028A support

[PATCH v4 0/4] tty: serial: fsl_lpuart: various fixes and LS1028A support

[Michael Walle]

These are various fixes for problems I found during development of the
LS1028A support for the LPUART.

Changes since v3:
 - Dropped patches which were already picked up.
 - Changed the subject of the former patch
     tty: serial: fsl_lpuart: handle EPROBE_DEFER for DMA
   Thanks Rob.
 - Added errno to error/info messages. Thanks Rob.
 - Splitted this series and removed the dt-bindings patches as well as
   the device tree patches. Thanks Leo.

Changes since v2:
Changed DMA channel request handling. Spotted by Rob Herring. Thanks.

Modified patches:
  tty: serial: fsl_lpuart: handle EPROBE_DEFER for DMA

Changes since v1:
DMA support fixes.

New patches:
  tty: serial: fsl_lpuart: fix DMA mapping
  arm64: dts: ls1028a: add "fsl,vf610-edma" compatible

Modified patches:
  arm64: dts: ls1028a: add missing LPUART nodes
   - add dma phandles

Michael Walle (4):
  tty: serial: fsl_lpuart: fix DMA operation when using IOMMU
  tty: serial: fsl_lpuart: fix DMA mapping
  tty: serial: fsl_lpuart: add LS1028A support
  tty: serial: fsl_lpuart: add LS1028A earlycon support

 drivers/tty/serial/fsl_lpuart.c | 210 ++++++++++++++++++++++----------
 1 file changed, 149 insertions(+), 61 deletions(-)

-- 
2.20.1

[PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Michael Walle]

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

There is also another caveat. If there is no DMA controller at all,
dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot test
for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will fail to
probe if, for example, the DMA driver is not enabled in the kernel
configuration.

To workaround this, we request the DMA channel in _startup(). Other
serial drivers do it the same way.

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

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index c31b8f3db6bf..33798df4d727 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct lpuart_port *sport)
 static void lpuart_tx_dma_startup(struct lpuart_port *sport)
 {
 	u32 uartbaud;
+	int ret;
 
-	if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
-		init_waitqueue_head(&sport->dma_wait);
-		sport->lpuart_dma_tx_use = true;
-		if (lpuart_is_32(sport)) {
-			uartbaud = lpuart32_read(&sport->port, UARTBAUD);
-			lpuart32_write(&sport->port,
-				       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
-		} else {
-			writeb(readb(sport->port.membase + UARTCR5) |
-				UARTCR5_TDMAS, sport->port.membase + UARTCR5);
-		}
+	sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
+	if (IS_ERR(sport->dma_tx_chan)) {
+		dev_info_once(sport->port.dev,
+			      "DMA tx channel request failed, operating without tx DMA (%ld)\n",
+			      PTR_ERR(sport->dma_tx_chan));
+		sport->dma_tx_chan = NULL;
+		goto err;
+	}
+
+	ret = lpuart_dma_tx_request(&sport->port);
+	if (!ret)
+		goto err;
+
+	init_waitqueue_head(&sport->dma_wait);
+	sport->lpuart_dma_tx_use = true;
+	if (lpuart_is_32(sport)) {
+		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
+		lpuart32_write(&sport->port,
+			       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
 	} else {
-		sport->lpuart_dma_tx_use = false;
+		writeb(readb(sport->port.membase + UARTCR5) |
+		       UARTCR5_TDMAS, sport->port.membase + UARTCR5);
 	}
+
+	return;
+
+err:
+	sport->lpuart_dma_tx_use = false;
 }
 
 static void lpuart_rx_dma_startup(struct lpuart_port *sport)
 {
-	if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
-		/* set Rx DMA timeout */
-		sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
-		if (!sport->dma_rx_timeout)
-			sport->dma_rx_timeout = 1;
+	int ret;
 
-		sport->lpuart_dma_rx_use = true;
-		rx_dma_timer_init(sport);
-	} else {
-		sport->lpuart_dma_rx_use = false;
+	sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
+	if (IS_ERR(sport->dma_rx_chan)) {
+		dev_info_once(sport->port.dev,
+			      "DMA rx channel request failed, operating without rx DMA (%ld)\n",
+			      PTR_ERR(sport->dma_rx_chan));
+		sport->dma_rx_chan = NULL;
+		goto err;
 	}
+
+	ret = lpuart_start_rx_dma(sport);
+	if (ret)
+		goto err;
+
+	/* set Rx DMA timeout */
+	sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
+	if (!sport->dma_rx_timeout)
+		sport->dma_rx_timeout = 1;
+
+	sport->lpuart_dma_rx_use = true;
+	rx_dma_timer_init(sport);
+
+	return;
+
+err:
+	sport->lpuart_dma_rx_use = false;
 }
 
 static int lpuart_startup(struct uart_port *port)
@@ -1615,6 +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
 			dmaengine_terminate_all(sport->dma_tx_chan);
 		}
 	}
+
+	if (sport->dma_tx_chan)
+		dma_release_channel(sport->dma_tx_chan);
+	if (sport->dma_rx_chan)
+		dma_release_channel(sport->dma_rx_chan);
 }
 
 static void lpuart_shutdown(struct uart_port *port)
@@ -2520,16 +2556,6 @@ 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:
-- 
2.20.1

Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Leonard Crestez]

On 06.03.2020 23:44, Michael Walle wrote:
> The DMA channel might not be available at probe time. This is esp. the
> case if the DMA controller has an IOMMU mapping.
> 
> There is also another caveat. If there is no DMA controller at all,
> dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot test
> for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will fail to
> probe if, for example, the DMA driver is not enabled in the kernel
> configuration.
> 
> To workaround this, we request the DMA channel in _startup(). Other
> serial drivers do it the same way.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

This appears to cause boot hangs on imx8qxp-mek (boards boots fine on 
next-20200324 if this patch is reverted)

> ---
>   drivers/tty/serial/fsl_lpuart.c | 88 +++++++++++++++++++++------------
>   1 file changed, 57 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index c31b8f3db6bf..33798df4d727 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct lpuart_port *sport)
>   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
>   {
>   	u32 uartbaud;
> +	int ret;
>   
> -	if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
> -		init_waitqueue_head(&sport->dma_wait);
> -		sport->lpuart_dma_tx_use = true;
> -		if (lpuart_is_32(sport)) {
> -			uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> -			lpuart32_write(&sport->port,
> -				       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
> -		} else {
> -			writeb(readb(sport->port.membase + UARTCR5) |
> -				UARTCR5_TDMAS, sport->port.membase + UARTCR5);
> -		}
> +	sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
> +	if (IS_ERR(sport->dma_tx_chan)) {
> +		dev_info_once(sport->port.dev,
> +			      "DMA tx channel request failed, operating without tx DMA (%ld)\n",
> +			      PTR_ERR(sport->dma_tx_chan));

It seems that this since this is called from lpuart32_startup with 
&sport->port.lock held and lpuart32_console_write takes the same lock it 
can and hang.

As a workaround I can just remove this print but there are other 
possible error conditions in dmaengine code which can cause a printk.

Maybe the port lock should only be held around register manipulation?

> +		sport->dma_tx_chan = NULL;
> +		goto err;
> +	}
> +
> +	ret = lpuart_dma_tx_request(&sport->port);
> +	if (!ret)
> +		goto err;

This is backwards: lpuart_dma_tx_request returns negative errno on failure.

> +
> +	init_waitqueue_head(&sport->dma_wait);
> +	sport->lpuart_dma_tx_use = true;
> +	if (lpuart_is_32(sport)) {
> +		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> +		lpuart32_write(&sport->port,
> +			       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
>   	} else {
> -		sport->lpuart_dma_tx_use = false;
> +		writeb(readb(sport->port.membase + UARTCR5) |
> +		       UARTCR5_TDMAS, sport->port.membase + UARTCR5);
>   	}
> +
> +	return;
> +
> +err:
> +	sport->lpuart_dma_tx_use = false;
>   }
>   
>   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
>   {
> -	if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
> -		/* set Rx DMA timeout */
> -		sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
> -		if (!sport->dma_rx_timeout)
> -			sport->dma_rx_timeout = 1;
> +	int ret;
>   
> -		sport->lpuart_dma_rx_use = true;
> -		rx_dma_timer_init(sport);
> -	} else {
> -		sport->lpuart_dma_rx_use = false;
> +	sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
> +	if (IS_ERR(sport->dma_rx_chan)) {
> +		dev_info_once(sport->port.dev,
> +			      "DMA rx channel request failed, operating without rx DMA (%ld)\n",
> +			      PTR_ERR(sport->dma_rx_chan));
> +		sport->dma_rx_chan = NULL;
> +		goto err;
>   	}
> +
> +	ret = lpuart_start_rx_dma(sport);
> +	if (ret)
> +		goto err;

This is not backwards.

> +
> +	/* set Rx DMA timeout */
> +	sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
> +	if (!sport->dma_rx_timeout)
> +		sport->dma_rx_timeout = 1;
> +
> +	sport->lpuart_dma_rx_use = true;
> +	rx_dma_timer_init(sport);
> +
> +	return;
> +
> +err:
> +	sport->lpuart_dma_rx_use = false;
>   }
>   
>   static int lpuart_startup(struct uart_port *port)
> @@ -1615,6 +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
>   			dmaengine_terminate_all(sport->dma_tx_chan);
>   		}
>   	}
> +
> +	if (sport->dma_tx_chan)
> +		dma_release_channel(sport->dma_tx_chan);
> +	if (sport->dma_rx_chan)
> +		dma_release_channel(sport->dma_rx_chan);
>   }
>   
>   static void lpuart_shutdown(struct uart_port *port)
> @@ -2520,16 +2556,6 @@ 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:
> 

RE: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Andy Duan]

From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Tuesday, March 24, 2020 11:27 PM
> On 06.03.2020 23:44, Michael Walle wrote:
> > The DMA channel might not be available at probe time. This is esp. the
> > case if the DMA controller has an IOMMU mapping.
> >
> > There is also another caveat. If there is no DMA controller at all,
> > dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot test
> > for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will fail to
> > probe if, for example, the DMA driver is not enabled in the kernel
> > configuration.
If DMA driver is not enabled, we should disable DMA controller node in
dts file to match current sw environment, then driver doesn't do defer probe.

So I still suggest to check -EPROBE_DEFER for dma_request_slave_channel() in
.probe() function.

Andy
> >
> > To workaround this, we request the DMA channel in _startup(). Other
> > serial drivers do it the same way.
> >
> > Signed-off-by: Michael Walle <michael@walle.cc>
> 
> This appears to cause boot hangs on imx8qxp-mek (boards boots fine on
> next-20200324 if this patch is reverted)
> 
> > ---
> >   drivers/tty/serial/fsl_lpuart.c | 88 +++++++++++++++++++++------------
> >   1 file changed, 57 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index c31b8f3db6bf..33798df4d727
> > 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct
> lpuart_port *sport)
> >   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
> >   {
> >   	u32 uartbaud;
> > +	int ret;
> >
> > -	if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
> > -		init_waitqueue_head(&sport->dma_wait);
> > -		sport->lpuart_dma_tx_use = true;
> > -		if (lpuart_is_32(sport)) {
> > -			uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> > -			lpuart32_write(&sport->port,
> > -				       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
> > -		} else {
> > -			writeb(readb(sport->port.membase + UARTCR5) |
> > -				UARTCR5_TDMAS, sport->port.membase + UARTCR5);
> > -		}
> > +	sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
> > +	if (IS_ERR(sport->dma_tx_chan)) {
> > +		dev_info_once(sport->port.dev,
> > +			      "DMA tx channel request failed, operating without tx
> DMA (%ld)\n",
> > +			      PTR_ERR(sport->dma_tx_chan));
> 
> It seems that this since this is called from lpuart32_startup with
> &sport->port.lock held and lpuart32_console_write takes the same lock it can
> and hang.
> 
> As a workaround I can just remove this print but there are other possible error
> conditions in dmaengine code which can cause a printk.
> 
> Maybe the port lock should only be held around register manipulation?
> 
> > +		sport->dma_tx_chan = NULL;
> > +		goto err;
> > +	}
> > +
> > +	ret = lpuart_dma_tx_request(&sport->port);
> > +	if (!ret)
> > +		goto err;
> 
> This is backwards: lpuart_dma_tx_request returns negative errno on failure.
> 
> > +
> > +	init_waitqueue_head(&sport->dma_wait);
> > +	sport->lpuart_dma_tx_use = true;
> > +	if (lpuart_is_32(sport)) {
> > +		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> > +		lpuart32_write(&sport->port,
> > +			       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
> >   	} else {
> > -		sport->lpuart_dma_tx_use = false;
> > +		writeb(readb(sport->port.membase + UARTCR5) |
> > +		       UARTCR5_TDMAS, sport->port.membase + UARTCR5);
> >   	}
> > +
> > +	return;
> > +
> > +err:
> > +	sport->lpuart_dma_tx_use = false;
> >   }
> >
> >   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
> >   {
> > -	if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
> > -		/* set Rx DMA timeout */
> > -		sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
> > -		if (!sport->dma_rx_timeout)
> > -			sport->dma_rx_timeout = 1;
> > +	int ret;
> >
> > -		sport->lpuart_dma_rx_use = true;
> > -		rx_dma_timer_init(sport);
> > -	} else {
> > -		sport->lpuart_dma_rx_use = false;
> > +	sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
> > +	if (IS_ERR(sport->dma_rx_chan)) {
> > +		dev_info_once(sport->port.dev,
> > +			      "DMA rx channel request failed, operating without rx
> DMA (%ld)\n",
> > +			      PTR_ERR(sport->dma_rx_chan));
> > +		sport->dma_rx_chan = NULL;
> > +		goto err;
> >   	}
> > +
> > +	ret = lpuart_start_rx_dma(sport);
> > +	if (ret)
> > +		goto err;
> 
> This is not backwards.
> 
> > +
> > +	/* set Rx DMA timeout */
> > +	sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
> > +	if (!sport->dma_rx_timeout)
> > +		sport->dma_rx_timeout = 1;
> > +
> > +	sport->lpuart_dma_rx_use = true;
> > +	rx_dma_timer_init(sport);
> > +
> > +	return;
> > +
> > +err:
> > +	sport->lpuart_dma_rx_use = false;
> >   }
> >
> >   static int lpuart_startup(struct uart_port *port) @@ -1615,6
> > +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
> >   			dmaengine_terminate_all(sport->dma_tx_chan);
> >   		}
> >   	}
> > +
> > +	if (sport->dma_tx_chan)
> > +		dma_release_channel(sport->dma_tx_chan);
> > +	if (sport->dma_rx_chan)
> > +		dma_release_channel(sport->dma_rx_chan);
> >   }
> >
> >   static void lpuart_shutdown(struct uart_port *port) @@ -2520,16
> > +2556,6 @@ 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:
> >

Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Michael Walle]

Am 2020-03-24 17:12, schrieb Andy Duan:
> From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Tuesday, March
> 24, 2020 11:27 PM
>> On 06.03.2020 23:44, Michael Walle wrote:
>> > The DMA channel might not be available at probe time. This is esp. the
>> > case if the DMA controller has an IOMMU mapping.
>> >
>> > There is also another caveat. If there is no DMA controller at all,
>> > dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot test
>> > for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will fail to
>> > probe if, for example, the DMA driver is not enabled in the kernel
>> > configuration.
> If DMA driver is not enabled, we should disable DMA controller node in
> dts file to match current sw environment, then driver doesn't do defer 
> probe.
> 
> So I still suggest to check -EPROBE_DEFER for 
> dma_request_slave_channel() in
> .probe() function.

I don't know if I can follow you here. This would lead to non functional 
setups,
eg. one build its own kernel with DMA disabled, but still have a device 
tree
with the DMA nodes. And besides, the current workaround to request the 
DMA
channel in startup() is basically working, isn't it? And once the 
underlying
problem is fixed (the infinite EPROBE_DEFER), it could be moved back 
into
_probe().

-michael

> 
> Andy
>> >
>> > To workaround this, we request the DMA channel in _startup(). Other
>> > serial drivers do it the same way.
>> >
>> > Signed-off-by: Michael Walle <michael@walle.cc>
>> 
>> This appears to cause boot hangs on imx8qxp-mek (boards boots fine on
>> next-20200324 if this patch is reverted)
>> 
>> > ---
>> >   drivers/tty/serial/fsl_lpuart.c | 88 +++++++++++++++++++++------------
>> >   1 file changed, 57 insertions(+), 31 deletions(-)
>> >
>> > diff --git a/drivers/tty/serial/fsl_lpuart.c
>> > b/drivers/tty/serial/fsl_lpuart.c index c31b8f3db6bf..33798df4d727
>> > 100644
>> > --- a/drivers/tty/serial/fsl_lpuart.c
>> > +++ b/drivers/tty/serial/fsl_lpuart.c
>> > @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct
>> lpuart_port *sport)
>> >   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
>> >   {
>> >   	u32 uartbaud;
>> > +	int ret;
>> >
>> > -	if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
>> > -		init_waitqueue_head(&sport->dma_wait);
>> > -		sport->lpuart_dma_tx_use = true;
>> > -		if (lpuart_is_32(sport)) {
>> > -			uartbaud = lpuart32_read(&sport->port, UARTBAUD);
>> > -			lpuart32_write(&sport->port,
>> > -				       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
>> > -		} else {
>> > -			writeb(readb(sport->port.membase + UARTCR5) |
>> > -				UARTCR5_TDMAS, sport->port.membase + UARTCR5);
>> > -		}
>> > +	sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
>> > +	if (IS_ERR(sport->dma_tx_chan)) {
>> > +		dev_info_once(sport->port.dev,
>> > +			      "DMA tx channel request failed, operating without tx
>> DMA (%ld)\n",
>> > +			      PTR_ERR(sport->dma_tx_chan));
>> 
>> It seems that this since this is called from lpuart32_startup with
>> &sport->port.lock held and lpuart32_console_write takes the same lock 
>> it can
>> and hang.
>> 
>> As a workaround I can just remove this print but there are other 
>> possible error
>> conditions in dmaengine code which can cause a printk.
>> 
>> Maybe the port lock should only be held around register manipulation?
>> 
>> > +		sport->dma_tx_chan = NULL;
>> > +		goto err;
>> > +	}
>> > +
>> > +	ret = lpuart_dma_tx_request(&sport->port);
>> > +	if (!ret)
>> > +		goto err;
>> 
>> This is backwards: lpuart_dma_tx_request returns negative errno on 
>> failure.
>> 
>> > +
>> > +	init_waitqueue_head(&sport->dma_wait);
>> > +	sport->lpuart_dma_tx_use = true;
>> > +	if (lpuart_is_32(sport)) {
>> > +		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
>> > +		lpuart32_write(&sport->port,
>> > +			       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
>> >   	} else {
>> > -		sport->lpuart_dma_tx_use = false;
>> > +		writeb(readb(sport->port.membase + UARTCR5) |
>> > +		       UARTCR5_TDMAS, sport->port.membase + UARTCR5);
>> >   	}
>> > +
>> > +	return;
>> > +
>> > +err:
>> > +	sport->lpuart_dma_tx_use = false;
>> >   }
>> >
>> >   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
>> >   {
>> > -	if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
>> > -		/* set Rx DMA timeout */
>> > -		sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
>> > -		if (!sport->dma_rx_timeout)
>> > -			sport->dma_rx_timeout = 1;
>> > +	int ret;
>> >
>> > -		sport->lpuart_dma_rx_use = true;
>> > -		rx_dma_timer_init(sport);
>> > -	} else {
>> > -		sport->lpuart_dma_rx_use = false;
>> > +	sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
>> > +	if (IS_ERR(sport->dma_rx_chan)) {
>> > +		dev_info_once(sport->port.dev,
>> > +			      "DMA rx channel request failed, operating without rx
>> DMA (%ld)\n",
>> > +			      PTR_ERR(sport->dma_rx_chan));
>> > +		sport->dma_rx_chan = NULL;
>> > +		goto err;
>> >   	}
>> > +
>> > +	ret = lpuart_start_rx_dma(sport);
>> > +	if (ret)
>> > +		goto err;
>> 
>> This is not backwards.
>> 
>> > +
>> > +	/* set Rx DMA timeout */
>> > +	sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
>> > +	if (!sport->dma_rx_timeout)
>> > +		sport->dma_rx_timeout = 1;
>> > +
>> > +	sport->lpuart_dma_rx_use = true;
>> > +	rx_dma_timer_init(sport);
>> > +
>> > +	return;
>> > +
>> > +err:
>> > +	sport->lpuart_dma_rx_use = false;
>> >   }
>> >
>> >   static int lpuart_startup(struct uart_port *port) @@ -1615,6
>> > +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port *sport)
>> >   			dmaengine_terminate_all(sport->dma_tx_chan);
>> >   		}
>> >   	}
>> > +
>> > +	if (sport->dma_tx_chan)
>> > +		dma_release_channel(sport->dma_tx_chan);
>> > +	if (sport->dma_rx_chan)
>> > +		dma_release_channel(sport->dma_rx_chan);
>> >   }
>> >
>> >   static void lpuart_shutdown(struct uart_port *port) @@ -2520,16
>> > +2556,6 @@ 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:
>> >

RE: [EXT] Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Andy Duan]

From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25, 2020 12:18 AM
> Am 2020-03-24 17:12, schrieb Andy Duan:
> > From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Tuesday, March
> > 24, 2020 11:27 PM
> >> On 06.03.2020 23:44, Michael Walle wrote:
> >> > The DMA channel might not be available at probe time. This is esp.
> >> > the case if the DMA controller has an IOMMU mapping.
> >> >
> >> > There is also another caveat. If there is no DMA controller at all,
> >> > dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot
> >> > test for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will
> >> > fail to probe if, for example, the DMA driver is not enabled in the
> >> > kernel configuration.
> > If DMA driver is not enabled, we should disable DMA controller node in
> > dts file to match current sw environment, then driver doesn't do defer
> > probe.
> >
> > So I still suggest to check -EPROBE_DEFER for
> > dma_request_slave_channel() in
> > .probe() function.
> 
> I don't know if I can follow you here. This would lead to non functional setups,
> eg. one build its own kernel with DMA disabled, but still have a device tree
> with the DMA nodes. And besides, the current workaround to request the
> DMA channel in startup() is basically working, isn't it? And once the underlying
> problem is fixed (the infinite EPROBE_DEFER), it could be moved back into
> _probe().
> 
> -michael

I think the user use wrong dtb file. The dtb file doesn't reflect the real enabled
modules. For such case, there have many problems for syscon,... that other modules
depends on them.

So we cannot support wrong usage cases, that is my thought.

Thanks,
Andy  

> 
> >
> > Andy
> >> >
> >> > To workaround this, we request the DMA channel in _startup(). Other
> >> > serial drivers do it the same way.
> >> >
> >> > Signed-off-by: Michael Walle <michael@walle.cc>
> >>
> >> This appears to cause boot hangs on imx8qxp-mek (boards boots fine on
> >> next-20200324 if this patch is reverted)
> >>
> >> > ---
> >> >   drivers/tty/serial/fsl_lpuart.c | 88
> +++++++++++++++++++++------------
> >> >   1 file changed, 57 insertions(+), 31 deletions(-)
> >> >
> >> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> >> > b/drivers/tty/serial/fsl_lpuart.c index c31b8f3db6bf..33798df4d727
> >> > 100644
> >> > --- a/drivers/tty/serial/fsl_lpuart.c
> >> > +++ b/drivers/tty/serial/fsl_lpuart.c
> >> > @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct
> >> lpuart_port *sport)
> >> >   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
> >> >   {
> >> >    u32 uartbaud;
> >> > +  int ret;
> >> >
> >> > -  if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
> >> > -          init_waitqueue_head(&sport->dma_wait);
> >> > -          sport->lpuart_dma_tx_use = true;
> >> > -          if (lpuart_is_32(sport)) {
> >> > -                  uartbaud = lpuart32_read(&sport->port,
> UARTBAUD);
> >> > -                  lpuart32_write(&sport->port,
> >> > -                                 uartbaud |
> UARTBAUD_TDMAE, UARTBAUD);
> >> > -          } else {
> >> > -                  writeb(readb(sport->port.membase + UARTCR5)
> |
> >> > -                          UARTCR5_TDMAS,
> sport->port.membase + UARTCR5);
> >> > -          }
> >> > +  sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
> >> > + if (IS_ERR(sport->dma_tx_chan)) {
> >> > +          dev_info_once(sport->port.dev,
> >> > +                        "DMA tx channel request failed, operating
> >> > + without tx
> >> DMA (%ld)\n",
> >> > +                        PTR_ERR(sport->dma_tx_chan));
> >>
> >> It seems that this since this is called from lpuart32_startup with
> >> &sport->port.lock held and lpuart32_console_write takes the same lock
> >> it can and hang.
> >>
> >> As a workaround I can just remove this print but there are other
> >> possible error conditions in dmaengine code which can cause a printk.
> >>
> >> Maybe the port lock should only be held around register manipulation?
> >>
> >> > +          sport->dma_tx_chan = NULL;
> >> > +          goto err;
> >> > +  }
> >> > +
> >> > +  ret = lpuart_dma_tx_request(&sport->port);
> >> > +  if (!ret)
> >> > +          goto err;
> >>
> >> This is backwards: lpuart_dma_tx_request returns negative errno on
> >> failure.
> >>
> >> > +
> >> > +  init_waitqueue_head(&sport->dma_wait);
> >> > +  sport->lpuart_dma_tx_use = true;  if (lpuart_is_32(sport)) {
> >> > +          uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> >> > +          lpuart32_write(&sport->port,
> >> > +                         uartbaud | UARTBAUD_TDMAE,
> UARTBAUD);
> >> >    } else {
> >> > -          sport->lpuart_dma_tx_use = false;
> >> > +          writeb(readb(sport->port.membase + UARTCR5) |
> >> > +                 UARTCR5_TDMAS, sport->port.membase +
> UARTCR5);
> >> >    }
> >> > +
> >> > +  return;
> >> > +
> >> > +err:
> >> > +  sport->lpuart_dma_tx_use = false;
> >> >   }
> >> >
> >> >   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
> >> >   {
> >> > -  if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
> >> > -          /* set Rx DMA timeout */
> >> > -          sport->dma_rx_timeout =
> msecs_to_jiffies(DMA_RX_TIMEOUT);
> >> > -          if (!sport->dma_rx_timeout)
> >> > -                  sport->dma_rx_timeout = 1;
> >> > +  int ret;
> >> >
> >> > -          sport->lpuart_dma_rx_use = true;
> >> > -          rx_dma_timer_init(sport);
> >> > -  } else {
> >> > -          sport->lpuart_dma_rx_use = false;
> >> > +  sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
> >> > + if (IS_ERR(sport->dma_rx_chan)) {
> >> > +          dev_info_once(sport->port.dev,
> >> > +                        "DMA rx channel request failed, operating
> >> > + without rx
> >> DMA (%ld)\n",
> >> > +                        PTR_ERR(sport->dma_rx_chan));
> >> > +          sport->dma_rx_chan = NULL;
> >> > +          goto err;
> >> >    }
> >> > +
> >> > +  ret = lpuart_start_rx_dma(sport);  if (ret)
> >> > +          goto err;
> >>
> >> This is not backwards.
> >>
> >> > +
> >> > +  /* set Rx DMA timeout */
> >> > +  sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);  if
> >> > + (!sport->dma_rx_timeout)
> >> > +          sport->dma_rx_timeout = 1;
> >> > +
> >> > +  sport->lpuart_dma_rx_use = true;  rx_dma_timer_init(sport);
> >> > +
> >> > +  return;
> >> > +
> >> > +err:
> >> > +  sport->lpuart_dma_rx_use = false;
> >> >   }
> >> >
> >> >   static int lpuart_startup(struct uart_port *port) @@ -1615,6
> >> > +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port
> >> > +*sport)
> >> >
> dmaengine_terminate_all(sport->dma_tx_chan);
> >> >            }
> >> >    }
> >> > +
> >> > +  if (sport->dma_tx_chan)
> >> > +          dma_release_channel(sport->dma_tx_chan);
> >> > +  if (sport->dma_rx_chan)
> >> > +          dma_release_channel(sport->dma_rx_chan);
> >> >   }
> >> >
> >> >   static void lpuart_shutdown(struct uart_port *port) @@ -2520,16
> >> > +2556,6 @@ 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:
> >> >

Re: [EXT] Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Michael Walle]

Am 2020-03-24 17:28, schrieb Andy Duan:
> From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25, 2020 
> 12:18 AM
>> Am 2020-03-24 17:12, schrieb Andy Duan:
>> > From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Tuesday, March
>> > 24, 2020 11:27 PM
>> >> On 06.03.2020 23:44, Michael Walle wrote:
>> >> > The DMA channel might not be available at probe time. This is esp.
>> >> > the case if the DMA controller has an IOMMU mapping.
>> >> >
>> >> > There is also another caveat. If there is no DMA controller at all,
>> >> > dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot
>> >> > test for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will
>> >> > fail to probe if, for example, the DMA driver is not enabled in the
>> >> > kernel configuration.
>> > If DMA driver is not enabled, we should disable DMA controller node in
>> > dts file to match current sw environment, then driver doesn't do defer
>> > probe.
>> >
>> > So I still suggest to check -EPROBE_DEFER for
>> > dma_request_slave_channel() in
>> > .probe() function.
>> 
>> I don't know if I can follow you here. This would lead to non 
>> functional setups,
>> eg. one build its own kernel with DMA disabled, but still have a 
>> device tree
>> with the DMA nodes. And besides, the current workaround to request the
>> DMA channel in startup() is basically working, isn't it? And once the 
>> underlying
>> problem is fixed (the infinite EPROBE_DEFER), it could be moved back 
>> into
>> _probe().
>> 
>> -michael
> 
> I think the user use wrong dtb file. The dtb file doesn't reflect the
> real enabled
> modules. For such case, there have many problems for syscon,... that
> other modules
> depends on them.

But the user doesn't use the wrong dtb. I don't consider having the DMA 
channels
in the dtb makes it wrong, just because DMA is not enabled in the 
kernel. If
you'd follow that argument, then the dtb is also wrong if there is for 
example
a crypto device, although the kernel doesn't have support for it 
enabled.

-michael

> 
> So we cannot support wrong usage cases, that is my thought.
> 
> Thanks,
> Andy
> 
>> 
>> >
>> > Andy
>> >> >
>> >> > To workaround this, we request the DMA channel in _startup(). Other
>> >> > serial drivers do it the same way.
>> >> >
>> >> > Signed-off-by: Michael Walle <michael@walle.cc>
>> >>
>> >> This appears to cause boot hangs on imx8qxp-mek (boards boots fine on
>> >> next-20200324 if this patch is reverted)
>> >>
>> >> > ---
>> >> >   drivers/tty/serial/fsl_lpuart.c | 88
>> +++++++++++++++++++++------------
>> >> >   1 file changed, 57 insertions(+), 31 deletions(-)
>> >> >
>> >> > diff --git a/drivers/tty/serial/fsl_lpuart.c
>> >> > b/drivers/tty/serial/fsl_lpuart.c index c31b8f3db6bf..33798df4d727
>> >> > 100644
>> >> > --- a/drivers/tty/serial/fsl_lpuart.c
>> >> > +++ b/drivers/tty/serial/fsl_lpuart.c
>> >> > @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct
>> >> lpuart_port *sport)
>> >> >   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
>> >> >   {
>> >> >    u32 uartbaud;
>> >> > +  int ret;
>> >> >
>> >> > -  if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
>> >> > -          init_waitqueue_head(&sport->dma_wait);
>> >> > -          sport->lpuart_dma_tx_use = true;
>> >> > -          if (lpuart_is_32(sport)) {
>> >> > -                  uartbaud = lpuart32_read(&sport->port,
>> UARTBAUD);
>> >> > -                  lpuart32_write(&sport->port,
>> >> > -                                 uartbaud |
>> UARTBAUD_TDMAE, UARTBAUD);
>> >> > -          } else {
>> >> > -                  writeb(readb(sport->port.membase + UARTCR5)
>> |
>> >> > -                          UARTCR5_TDMAS,
>> sport->port.membase + UARTCR5);
>> >> > -          }
>> >> > +  sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
>> >> > + if (IS_ERR(sport->dma_tx_chan)) {
>> >> > +          dev_info_once(sport->port.dev,
>> >> > +                        "DMA tx channel request failed, operating
>> >> > + without tx
>> >> DMA (%ld)\n",
>> >> > +                        PTR_ERR(sport->dma_tx_chan));
>> >>
>> >> It seems that this since this is called from lpuart32_startup with
>> >> &sport->port.lock held and lpuart32_console_write takes the same lock
>> >> it can and hang.
>> >>
>> >> As a workaround I can just remove this print but there are other
>> >> possible error conditions in dmaengine code which can cause a printk.
>> >>
>> >> Maybe the port lock should only be held around register manipulation?
>> >>
>> >> > +          sport->dma_tx_chan = NULL;
>> >> > +          goto err;
>> >> > +  }
>> >> > +
>> >> > +  ret = lpuart_dma_tx_request(&sport->port);
>> >> > +  if (!ret)
>> >> > +          goto err;
>> >>
>> >> This is backwards: lpuart_dma_tx_request returns negative errno on
>> >> failure.
>> >>
>> >> > +
>> >> > +  init_waitqueue_head(&sport->dma_wait);
>> >> > +  sport->lpuart_dma_tx_use = true;  if (lpuart_is_32(sport)) {
>> >> > +          uartbaud = lpuart32_read(&sport->port, UARTBAUD);
>> >> > +          lpuart32_write(&sport->port,
>> >> > +                         uartbaud | UARTBAUD_TDMAE,
>> UARTBAUD);
>> >> >    } else {
>> >> > -          sport->lpuart_dma_tx_use = false;
>> >> > +          writeb(readb(sport->port.membase + UARTCR5) |
>> >> > +                 UARTCR5_TDMAS, sport->port.membase +
>> UARTCR5);
>> >> >    }
>> >> > +
>> >> > +  return;
>> >> > +
>> >> > +err:
>> >> > +  sport->lpuart_dma_tx_use = false;
>> >> >   }
>> >> >
>> >> >   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
>> >> >   {
>> >> > -  if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
>> >> > -          /* set Rx DMA timeout */
>> >> > -          sport->dma_rx_timeout =
>> msecs_to_jiffies(DMA_RX_TIMEOUT);
>> >> > -          if (!sport->dma_rx_timeout)
>> >> > -                  sport->dma_rx_timeout = 1;
>> >> > +  int ret;
>> >> >
>> >> > -          sport->lpuart_dma_rx_use = true;
>> >> > -          rx_dma_timer_init(sport);
>> >> > -  } else {
>> >> > -          sport->lpuart_dma_rx_use = false;
>> >> > +  sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
>> >> > + if (IS_ERR(sport->dma_rx_chan)) {
>> >> > +          dev_info_once(sport->port.dev,
>> >> > +                        "DMA rx channel request failed, operating
>> >> > + without rx
>> >> DMA (%ld)\n",
>> >> > +                        PTR_ERR(sport->dma_rx_chan));
>> >> > +          sport->dma_rx_chan = NULL;
>> >> > +          goto err;
>> >> >    }
>> >> > +
>> >> > +  ret = lpuart_start_rx_dma(sport);  if (ret)
>> >> > +          goto err;
>> >>
>> >> This is not backwards.
>> >>
>> >> > +
>> >> > +  /* set Rx DMA timeout */
>> >> > +  sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);  if
>> >> > + (!sport->dma_rx_timeout)
>> >> > +          sport->dma_rx_timeout = 1;
>> >> > +
>> >> > +  sport->lpuart_dma_rx_use = true;  rx_dma_timer_init(sport);
>> >> > +
>> >> > +  return;
>> >> > +
>> >> > +err:
>> >> > +  sport->lpuart_dma_rx_use = false;
>> >> >   }
>> >> >
>> >> >   static int lpuart_startup(struct uart_port *port) @@ -1615,6
>> >> > +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port
>> >> > +*sport)
>> >> >
>> dmaengine_terminate_all(sport->dma_tx_chan);
>> >> >            }
>> >> >    }
>> >> > +
>> >> > +  if (sport->dma_tx_chan)
>> >> > +          dma_release_channel(sport->dma_tx_chan);
>> >> > +  if (sport->dma_rx_chan)
>> >> > +          dma_release_channel(sport->dma_rx_chan);
>> >> >   }
>> >> >
>> >> >   static void lpuart_shutdown(struct uart_port *port) @@ -2520,16
>> >> > +2556,6 @@ 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:
>> >> >

RE: [EXT] Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Andy Duan]

From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25, 2020 12:35 AM
> Am 2020-03-24 17:28, schrieb Andy Duan:
> > From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25,
> 2020
> > 12:18 AM
> >> Am 2020-03-24 17:12, schrieb Andy Duan:
> >> > From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Tuesday,
> >> > March 24, 2020 11:27 PM
> >> >> On 06.03.2020 23:44, Michael Walle wrote:
> >> >> > The DMA channel might not be available at probe time. This is esp.
> >> >> > the case if the DMA controller has an IOMMU mapping.
> >> >> >
> >> >> > There is also another caveat. If there is no DMA controller at
> >> >> > all,
> >> >> > dma_request_chan() will also return -EPROBE_DEFER. Thus we
> >> >> > cannot test for -EPROBE_DEFER in probe(). Otherwise the lpuart
> >> >> > driver will fail to probe if, for example, the DMA driver is not
> >> >> > enabled in the kernel configuration.
> >> > If DMA driver is not enabled, we should disable DMA controller node
> >> > in dts file to match current sw environment, then driver doesn't do
> >> > defer probe.
> >> >
> >> > So I still suggest to check -EPROBE_DEFER for
> >> > dma_request_slave_channel() in
> >> > .probe() function.
> >>
> >> I don't know if I can follow you here. This would lead to non
> >> functional setups, eg. one build its own kernel with DMA disabled,
> >> but still have a device tree with the DMA nodes. And besides, the
> >> current workaround to request the DMA channel in startup() is
> >> basically working, isn't it? And once the underlying problem is fixed
> >> (the infinite EPROBE_DEFER), it could be moved back into _probe().
> >>
> >> -michael
> >
> > I think the user use wrong dtb file. The dtb file doesn't reflect the
> > real enabled modules. For such case, there have many problems for
> > syscon,... that other modules depends on them.
> 
> But the user doesn't use the wrong dtb. I don't consider having the DMA
> channels in the dtb makes it wrong, just because DMA is not enabled in the
> kernel. If you'd follow that argument, then the dtb is also wrong if there is for
> example a crypto device, although the kernel doesn't have support for it
> enabled.
> 
> -michael

dma_request_chan() is not atomic context.
Even if move it into .startup(), please move it out of spinlock context.

> 
> >
> > So we cannot support wrong usage cases, that is my thought.
> >
> > Thanks,
> > Andy
> >
> >>
> >> >
> >> > Andy
> >> >> >
> >> >> > To workaround this, we request the DMA channel in _startup().
> >> >> > Other serial drivers do it the same way.
> >> >> >
> >> >> > Signed-off-by: Michael Walle <michael@walle.cc>
> >> >>
> >> >> This appears to cause boot hangs on imx8qxp-mek (boards boots fine
> >> >> on
> >> >> next-20200324 if this patch is reverted)
> >> >>
> >> >> > ---
> >> >> >   drivers/tty/serial/fsl_lpuart.c | 88
> >> +++++++++++++++++++++------------
> >> >> >   1 file changed, 57 insertions(+), 31 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> >> >> > b/drivers/tty/serial/fsl_lpuart.c index
> >> >> > c31b8f3db6bf..33798df4d727
> >> >> > 100644
> >> >> > --- a/drivers/tty/serial/fsl_lpuart.c
> >> >> > +++ b/drivers/tty/serial/fsl_lpuart.c
> >> >> > @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct
> >> >> lpuart_port *sport)
> >> >> >   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
> >> >> >   {
> >> >> >    u32 uartbaud;
> >> >> > +  int ret;
> >> >> >
> >> >> > -  if (sport->dma_tx_chan
> && !lpuart_dma_tx_request(&sport->port)) {
> >> >> > -          init_waitqueue_head(&sport->dma_wait);
> >> >> > -          sport->lpuart_dma_tx_use = true;
> >> >> > -          if (lpuart_is_32(sport)) {
> >> >> > -                  uartbaud = lpuart32_read(&sport->port,
> >> UARTBAUD);
> >> >> > -                  lpuart32_write(&sport->port,
> >> >> > -                                 uartbaud |
> >> UARTBAUD_TDMAE, UARTBAUD);
> >> >> > -          } else {
> >> >> > -                  writeb(readb(sport->port.membase +
> UARTCR5)
> >> |
> >> >> > -                          UARTCR5_TDMAS,
> >> sport->port.membase + UARTCR5);
> >> >> > -          }
> >> >> > +  sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
> >> >> > + if (IS_ERR(sport->dma_tx_chan)) {
> >> >> > +          dev_info_once(sport->port.dev,
> >> >> > +                        "DMA tx channel request failed,
> >> >> > + operating without tx
> >> >> DMA (%ld)\n",
> >> >> > +                        PTR_ERR(sport->dma_tx_chan));
> >> >>
> >> >> It seems that this since this is called from lpuart32_startup with
> >> >> &sport->port.lock held and lpuart32_console_write takes the same
> >> >> lock it can and hang.
> >> >>
> >> >> As a workaround I can just remove this print but there are other
> >> >> possible error conditions in dmaengine code which can cause a printk.
> >> >>
> >> >> Maybe the port lock should only be held around register manipulation?
> >> >>
> >> >> > +          sport->dma_tx_chan = NULL;
> >> >> > +          goto err;
> >> >> > +  }
> >> >> > +
> >> >> > +  ret = lpuart_dma_tx_request(&sport->port);
> >> >> > +  if (!ret)
> >> >> > +          goto err;
> >> >>
> >> >> This is backwards: lpuart_dma_tx_request returns negative errno on
> >> >> failure.
> >> >>
> >> >> > +
> >> >> > +  init_waitqueue_head(&sport->dma_wait);
> >> >> > +  sport->lpuart_dma_tx_use = true;  if (lpuart_is_32(sport)) {
> >> >> > +          uartbaud = lpuart32_read(&sport->port, UARTBAUD);
> >> >> > +          lpuart32_write(&sport->port,
> >> >> > +                         uartbaud | UARTBAUD_TDMAE,
> >> UARTBAUD);
> >> >> >    } else {
> >> >> > -          sport->lpuart_dma_tx_use = false;
> >> >> > +          writeb(readb(sport->port.membase + UARTCR5) |
> >> >> > +                 UARTCR5_TDMAS, sport->port.membase +
> >> UARTCR5);
> >> >> >    }
> >> >> > +
> >> >> > +  return;
> >> >> > +
> >> >> > +err:
> >> >> > +  sport->lpuart_dma_tx_use = false;
> >> >> >   }
> >> >> >
> >> >> >   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
> >> >> >   {
> >> >> > -  if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
> >> >> > -          /* set Rx DMA timeout */
> >> >> > -          sport->dma_rx_timeout =
> >> msecs_to_jiffies(DMA_RX_TIMEOUT);
> >> >> > -          if (!sport->dma_rx_timeout)
> >> >> > -                  sport->dma_rx_timeout = 1;
> >> >> > +  int ret;
> >> >> >
> >> >> > -          sport->lpuart_dma_rx_use = true;
> >> >> > -          rx_dma_timer_init(sport);
> >> >> > -  } else {
> >> >> > -          sport->lpuart_dma_rx_use = false;
> >> >> > +  sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
> >> >> > + if (IS_ERR(sport->dma_rx_chan)) {
> >> >> > +          dev_info_once(sport->port.dev,
> >> >> > +                        "DMA rx channel request failed,
> >> >> > + operating without rx
> >> >> DMA (%ld)\n",
> >> >> > +                        PTR_ERR(sport->dma_rx_chan));
> >> >> > +          sport->dma_rx_chan = NULL;
> >> >> > +          goto err;
> >> >> >    }
> >> >> > +
> >> >> > +  ret = lpuart_start_rx_dma(sport);  if (ret)
> >> >> > +          goto err;
> >> >>
> >> >> This is not backwards.
> >> >>
> >> >> > +
> >> >> > +  /* set Rx DMA timeout */
> >> >> > +  sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
> if
> >> >> > + (!sport->dma_rx_timeout)
> >> >> > +          sport->dma_rx_timeout = 1;
> >> >> > +
> >> >> > +  sport->lpuart_dma_rx_use = true;  rx_dma_timer_init(sport);
> >> >> > +
> >> >> > +  return;
> >> >> > +
> >> >> > +err:
> >> >> > +  sport->lpuart_dma_rx_use = false;
> >> >> >   }
> >> >> >
> >> >> >   static int lpuart_startup(struct uart_port *port) @@ -1615,6
> >> >> > +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port
> >> >> > +*sport)
> >> >> >
> >> dmaengine_terminate_all(sport->dma_tx_chan);
> >> >> >            }
> >> >> >    }
> >> >> > +
> >> >> > +  if (sport->dma_tx_chan)
> >> >> > +          dma_release_channel(sport->dma_tx_chan);
> >> >> > +  if (sport->dma_rx_chan)
> >> >> > +          dma_release_channel(sport->dma_rx_chan);
> >> >> >   }
> >> >> >
> >> >> >   static void lpuart_shutdown(struct uart_port *port) @@
> >> >> > -2520,16
> >> >> > +2556,6 @@ 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:
> >> >> >

Re: [EXT] Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Michael Walle]

Hi Andy,

Am 2020-03-25 05:08, schrieb Andy Duan:
> From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25, 2020 
> 12:35 AM
>> Am 2020-03-24 17:28, schrieb Andy Duan:
>> > From: Michael Walle <michael@walle.cc> Sent: Wednesday, March 25,
>> 2020
>> > 12:18 AM
>> >> Am 2020-03-24 17:12, schrieb Andy Duan:
>> >> > From: Leonard Crestez <leonard.crestez@nxp.com> Sent: Tuesday,
>> >> > March 24, 2020 11:27 PM
>> >> >> On 06.03.2020 23:44, Michael Walle wrote:
>> >> >> > The DMA channel might not be available at probe time. This is esp.
>> >> >> > the case if the DMA controller has an IOMMU mapping.
>> >> >> >
>> >> >> > There is also another caveat. If there is no DMA controller at
>> >> >> > all,
>> >> >> > dma_request_chan() will also return -EPROBE_DEFER. Thus we
>> >> >> > cannot test for -EPROBE_DEFER in probe(). Otherwise the lpuart
>> >> >> > driver will fail to probe if, for example, the DMA driver is not
>> >> >> > enabled in the kernel configuration.
>> >> > If DMA driver is not enabled, we should disable DMA controller node
>> >> > in dts file to match current sw environment, then driver doesn't do
>> >> > defer probe.
>> >> >
>> >> > So I still suggest to check -EPROBE_DEFER for
>> >> > dma_request_slave_channel() in
>> >> > .probe() function.
>> >>
>> >> I don't know if I can follow you here. This would lead to non
>> >> functional setups, eg. one build its own kernel with DMA disabled,
>> >> but still have a device tree with the DMA nodes. And besides, the
>> >> current workaround to request the DMA channel in startup() is
>> >> basically working, isn't it? And once the underlying problem is fixed
>> >> (the infinite EPROBE_DEFER), it could be moved back into _probe().
>> >>
>> >> -michael
>> >
>> > I think the user use wrong dtb file. The dtb file doesn't reflect the
>> > real enabled modules. For such case, there have many problems for
>> > syscon,... that other modules depends on them.
>> 
>> But the user doesn't use the wrong dtb. I don't consider having the 
>> DMA
>> channels in the dtb makes it wrong, just because DMA is not enabled in 
>> the
>> kernel. If you'd follow that argument, then the dtb is also wrong if 
>> there is for
>> example a crypto device, although the kernel doesn't have support for 
>> it
>> enabled.
>> 
>> -michael
> 
> dma_request_chan() is not atomic context.
> Even if move it into .startup(), please move it out of spinlock 
> context.

Agreed. I'll send a respin of my fixes patches later.

-michael

> 
>> 
>> >
>> > So we cannot support wrong usage cases, that is my thought.
>> >
>> > Thanks,
>> > Andy
>> >
>> >>
>> >> >
>> >> > Andy
>> >> >> >
>> >> >> > To workaround this, we request the DMA channel in _startup().
>> >> >> > Other serial drivers do it the same way.
>> >> >> >
>> >> >> > Signed-off-by: Michael Walle <michael@walle.cc>
>> >> >>
>> >> >> This appears to cause boot hangs on imx8qxp-mek (boards boots fine
>> >> >> on
>> >> >> next-20200324 if this patch is reverted)
>> >> >>
>> >> >> > ---
>> >> >> >   drivers/tty/serial/fsl_lpuart.c | 88
>> >> +++++++++++++++++++++------------
>> >> >> >   1 file changed, 57 insertions(+), 31 deletions(-)
>> >> >> >
>> >> >> > diff --git a/drivers/tty/serial/fsl_lpuart.c
>> >> >> > b/drivers/tty/serial/fsl_lpuart.c index
>> >> >> > c31b8f3db6bf..33798df4d727
>> >> >> > 100644
>> >> >> > --- a/drivers/tty/serial/fsl_lpuart.c
>> >> >> > +++ b/drivers/tty/serial/fsl_lpuart.c
>> >> >> > @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct
>> >> >> lpuart_port *sport)
>> >> >> >   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
>> >> >> >   {
>> >> >> >    u32 uartbaud;
>> >> >> > +  int ret;
>> >> >> >
>> >> >> > -  if (sport->dma_tx_chan
>> && !lpuart_dma_tx_request(&sport->port)) {
>> >> >> > -          init_waitqueue_head(&sport->dma_wait);
>> >> >> > -          sport->lpuart_dma_tx_use = true;
>> >> >> > -          if (lpuart_is_32(sport)) {
>> >> >> > -                  uartbaud = lpuart32_read(&sport->port,
>> >> UARTBAUD);
>> >> >> > -                  lpuart32_write(&sport->port,
>> >> >> > -                                 uartbaud |
>> >> UARTBAUD_TDMAE, UARTBAUD);
>> >> >> > -          } else {
>> >> >> > -                  writeb(readb(sport->port.membase +
>> UARTCR5)
>> >> |
>> >> >> > -                          UARTCR5_TDMAS,
>> >> sport->port.membase + UARTCR5);
>> >> >> > -          }
>> >> >> > +  sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
>> >> >> > + if (IS_ERR(sport->dma_tx_chan)) {
>> >> >> > +          dev_info_once(sport->port.dev,
>> >> >> > +                        "DMA tx channel request failed,
>> >> >> > + operating without tx
>> >> >> DMA (%ld)\n",
>> >> >> > +                        PTR_ERR(sport->dma_tx_chan));
>> >> >>
>> >> >> It seems that this since this is called from lpuart32_startup with
>> >> >> &sport->port.lock held and lpuart32_console_write takes the same
>> >> >> lock it can and hang.
>> >> >>
>> >> >> As a workaround I can just remove this print but there are other
>> >> >> possible error conditions in dmaengine code which can cause a printk.
>> >> >>
>> >> >> Maybe the port lock should only be held around register manipulation?
>> >> >>
>> >> >> > +          sport->dma_tx_chan = NULL;
>> >> >> > +          goto err;
>> >> >> > +  }
>> >> >> > +
>> >> >> > +  ret = lpuart_dma_tx_request(&sport->port);
>> >> >> > +  if (!ret)
>> >> >> > +          goto err;
>> >> >>
>> >> >> This is backwards: lpuart_dma_tx_request returns negative errno on
>> >> >> failure.
>> >> >>
>> >> >> > +
>> >> >> > +  init_waitqueue_head(&sport->dma_wait);
>> >> >> > +  sport->lpuart_dma_tx_use = true;  if (lpuart_is_32(sport)) {
>> >> >> > +          uartbaud = lpuart32_read(&sport->port, UARTBAUD);
>> >> >> > +          lpuart32_write(&sport->port,
>> >> >> > +                         uartbaud | UARTBAUD_TDMAE,
>> >> UARTBAUD);
>> >> >> >    } else {
>> >> >> > -          sport->lpuart_dma_tx_use = false;
>> >> >> > +          writeb(readb(sport->port.membase + UARTCR5) |
>> >> >> > +                 UARTCR5_TDMAS, sport->port.membase +
>> >> UARTCR5);
>> >> >> >    }
>> >> >> > +
>> >> >> > +  return;
>> >> >> > +
>> >> >> > +err:
>> >> >> > +  sport->lpuart_dma_tx_use = false;
>> >> >> >   }
>> >> >> >
>> >> >> >   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
>> >> >> >   {
>> >> >> > -  if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
>> >> >> > -          /* set Rx DMA timeout */
>> >> >> > -          sport->dma_rx_timeout =
>> >> msecs_to_jiffies(DMA_RX_TIMEOUT);
>> >> >> > -          if (!sport->dma_rx_timeout)
>> >> >> > -                  sport->dma_rx_timeout = 1;
>> >> >> > +  int ret;
>> >> >> >
>> >> >> > -          sport->lpuart_dma_rx_use = true;
>> >> >> > -          rx_dma_timer_init(sport);
>> >> >> > -  } else {
>> >> >> > -          sport->lpuart_dma_rx_use = false;
>> >> >> > +  sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
>> >> >> > + if (IS_ERR(sport->dma_rx_chan)) {
>> >> >> > +          dev_info_once(sport->port.dev,
>> >> >> > +                        "DMA rx channel request failed,
>> >> >> > + operating without rx
>> >> >> DMA (%ld)\n",
>> >> >> > +                        PTR_ERR(sport->dma_rx_chan));
>> >> >> > +          sport->dma_rx_chan = NULL;
>> >> >> > +          goto err;
>> >> >> >    }
>> >> >> > +
>> >> >> > +  ret = lpuart_start_rx_dma(sport);  if (ret)
>> >> >> > +          goto err;
>> >> >>
>> >> >> This is not backwards.
>> >> >>
>> >> >> > +
>> >> >> > +  /* set Rx DMA timeout */
>> >> >> > +  sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
>> if
>> >> >> > + (!sport->dma_rx_timeout)
>> >> >> > +          sport->dma_rx_timeout = 1;
>> >> >> > +
>> >> >> > +  sport->lpuart_dma_rx_use = true;  rx_dma_timer_init(sport);
>> >> >> > +
>> >> >> > +  return;
>> >> >> > +
>> >> >> > +err:
>> >> >> > +  sport->lpuart_dma_rx_use = false;
>> >> >> >   }
>> >> >> >
>> >> >> >   static int lpuart_startup(struct uart_port *port) @@ -1615,6
>> >> >> > +1646,11 @@ static void lpuart_dma_shutdown(struct lpuart_port
>> >> >> > +*sport)
>> >> >> >
>> >> dmaengine_terminate_all(sport->dma_tx_chan);
>> >> >> >            }
>> >> >> >    }
>> >> >> > +
>> >> >> > +  if (sport->dma_tx_chan)
>> >> >> > +          dma_release_channel(sport->dma_tx_chan);
>> >> >> > +  if (sport->dma_rx_chan)
>> >> >> > +          dma_release_channel(sport->dma_rx_chan);
>> >> >> >   }
>> >> >> >
>> >> >> >   static void lpuart_shutdown(struct uart_port *port) @@
>> >> >> > -2520,16
>> >> >> > +2556,6 @@ 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:
>> >> >> >

Re: [PATCH v4 1/4] tty: serial: fsl_lpuart: fix DMA operation when using IOMMU

[Michael Walle]

Am 2020-03-24 16:27, schrieb Leonard Crestez:
> On 06.03.2020 23:44, Michael Walle wrote:
>> The DMA channel might not be available at probe time. This is esp. the
>> case if the DMA controller has an IOMMU mapping.
>> 
>> There is also another caveat. If there is no DMA controller at all,
>> dma_request_chan() will also return -EPROBE_DEFER. Thus we cannot test
>> for -EPROBE_DEFER in probe(). Otherwise the lpuart driver will fail to
>> probe if, for example, the DMA driver is not enabled in the kernel
>> configuration.
>> 
>> To workaround this, we request the DMA channel in _startup(). Other
>> serial drivers do it the same way.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> This appears to cause boot hangs on imx8qxp-mek (boards boots fine on
> next-20200324 if this patch is reverted)
> 
>> ---
>>   drivers/tty/serial/fsl_lpuart.c | 88 
>> +++++++++++++++++++++------------
>>   1 file changed, 57 insertions(+), 31 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/fsl_lpuart.c 
>> b/drivers/tty/serial/fsl_lpuart.c
>> index c31b8f3db6bf..33798df4d727 100644
>> --- a/drivers/tty/serial/fsl_lpuart.c
>> +++ b/drivers/tty/serial/fsl_lpuart.c
>> @@ -1493,36 +1493,67 @@ static void rx_dma_timer_init(struct 
>> lpuart_port *sport)
>>   static void lpuart_tx_dma_startup(struct lpuart_port *sport)
>>   {
>>   	u32 uartbaud;
>> +	int ret;
>> 
>> -	if (sport->dma_tx_chan && !lpuart_dma_tx_request(&sport->port)) {
>> -		init_waitqueue_head(&sport->dma_wait);
>> -		sport->lpuart_dma_tx_use = true;
>> -		if (lpuart_is_32(sport)) {
>> -			uartbaud = lpuart32_read(&sport->port, UARTBAUD);
>> -			lpuart32_write(&sport->port,
>> -				       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
>> -		} else {
>> -			writeb(readb(sport->port.membase + UARTCR5) |
>> -				UARTCR5_TDMAS, sport->port.membase + UARTCR5);
>> -		}
>> +	sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
>> +	if (IS_ERR(sport->dma_tx_chan)) {
>> +		dev_info_once(sport->port.dev,
>> +			      "DMA tx channel request failed, operating without tx DMA 
>> (%ld)\n",
>> +			      PTR_ERR(sport->dma_tx_chan));
> 
> It seems that this since this is called from lpuart32_startup with
> &sport->port.lock held and lpuart32_console_write takes the same lock 
> it
> can and hang.

Shoot.
So basically, you cannot do any dev_dbg/info/warn/err/pr_* when you
the lock is taken (which could also be taken in serial_core.c), correct?

Because thats all over the place.. just happens now because there is a
dev_info() by default.

> 
> As a workaround I can just remove this print but there are other
> possible error conditions in dmaengine code which can cause a printk.
> 
> Maybe the port lock should only be held around register manipulation?
> 
>> +		sport->dma_tx_chan = NULL;
>> +		goto err;
>> +	}
>> +
>> +	ret = lpuart_dma_tx_request(&sport->port);
>> +	if (!ret)
>> +		goto err;
> 
> This is backwards: lpuart_dma_tx_request returns negative errno on 
> failure.

nice catch!

-michael

> 
>> +
>> +	init_waitqueue_head(&sport->dma_wait);
>> +	sport->lpuart_dma_tx_use = true;
>> +	if (lpuart_is_32(sport)) {
>> +		uartbaud = lpuart32_read(&sport->port, UARTBAUD);
>> +		lpuart32_write(&sport->port,
>> +			       uartbaud | UARTBAUD_TDMAE, UARTBAUD);
>>   	} else {
>> -		sport->lpuart_dma_tx_use = false;
>> +		writeb(readb(sport->port.membase + UARTCR5) |
>> +		       UARTCR5_TDMAS, sport->port.membase + UARTCR5);
>>   	}
>> +
>> +	return;
>> +
>> +err:
>> +	sport->lpuart_dma_tx_use = false;
>>   }
>> 
>>   static void lpuart_rx_dma_startup(struct lpuart_port *sport)
>>   {
>> -	if (sport->dma_rx_chan && !lpuart_start_rx_dma(sport)) {
>> -		/* set Rx DMA timeout */
>> -		sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
>> -		if (!sport->dma_rx_timeout)
>> -			sport->dma_rx_timeout = 1;
>> +	int ret;
>> 
>> -		sport->lpuart_dma_rx_use = true;
>> -		rx_dma_timer_init(sport);
>> -	} else {
>> -		sport->lpuart_dma_rx_use = false;
>> +	sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
>> +	if (IS_ERR(sport->dma_rx_chan)) {
>> +		dev_info_once(sport->port.dev,
>> +			      "DMA rx channel request failed, operating without rx DMA 
>> (%ld)\n",
>> +			      PTR_ERR(sport->dma_rx_chan));
>> +		sport->dma_rx_chan = NULL;
>> +		goto err;
>>   	}
>> +
>> +	ret = lpuart_start_rx_dma(sport);
>> +	if (ret)
>> +		goto err;
> 
> This is not backwards.
> 
>> +
>> +	/* set Rx DMA timeout */
>> +	sport->dma_rx_timeout = msecs_to_jiffies(DMA_RX_TIMEOUT);
>> +	if (!sport->dma_rx_timeout)
>> +		sport->dma_rx_timeout = 1;
>> +
>> +	sport->lpuart_dma_rx_use = true;
>> +	rx_dma_timer_init(sport);
>> +
>> +	return;
>> +
>> +err:
>> +	sport->lpuart_dma_rx_use = false;
>>   }
>> 
>>   static int lpuart_startup(struct uart_port *port)
>> @@ -1615,6 +1646,11 @@ static void lpuart_dma_shutdown(struct 
>> lpuart_port *sport)
>>   			dmaengine_terminate_all(sport->dma_tx_chan);
>>   		}
>>   	}
>> +
>> +	if (sport->dma_tx_chan)
>> +		dma_release_channel(sport->dma_tx_chan);
>> +	if (sport->dma_rx_chan)
>> +		dma_release_channel(sport->dma_rx_chan);
>>   }
>> 
>>   static void lpuart_shutdown(struct uart_port *port)
>> @@ -2520,16 +2556,6 @@ 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:
>> 

[PATCH 1/3] tty: serial: fsl_lpuart: move dev_info_once()

[Michael Walle]

Don't take the spinlock and use dev_info_once(). This may cause a hang
because the console takes this spinlock, too. Just print this info after
we've released the lock.

Fixes: 159381df1442f ("tty: serial: fsl_lpuart: fix DMA operation when using IOMMU")
Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 9c6a018b1390..960fc2658f19 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1517,9 +1517,6 @@ static void lpuart_tx_dma_startup(struct lpuart_port *sport)
 
 	sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
 	if (IS_ERR(sport->dma_tx_chan)) {
-		dev_info_once(sport->port.dev,
-			      "DMA tx channel request failed, operating without tx DMA (%ld)\n",
-			      PTR_ERR(sport->dma_tx_chan));
 		sport->dma_tx_chan = NULL;
 		goto err;
 	}
@@ -1551,9 +1548,6 @@ static void lpuart_rx_dma_startup(struct lpuart_port *sport)
 
 	sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
 	if (IS_ERR(sport->dma_rx_chan)) {
-		dev_info_once(sport->port.dev,
-			      "DMA rx channel request failed, operating without rx DMA (%ld)\n",
-			      PTR_ERR(sport->dma_rx_chan));
 		sport->dma_rx_chan = NULL;
 		goto err;
 	}
@@ -1601,6 +1595,13 @@ static int lpuart_startup(struct uart_port *port)
 
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
+	if (!sport->dma_rx_chan)
+		dev_info_once(sport->port.dev,
+			      "DMA rx channel request failed, operating without rx DMA\n");
+	if (!sport->dma_tx_chan)
+		dev_info_once(sport->port.dev,
+			      "DMA tx channel request failed, operating without tx DMA\n");
+
 	return 0;
 }
 
@@ -1653,13 +1654,20 @@ static int lpuart32_startup(struct uart_port *port)
 
 	lpuart32_setup_watermark_enable(sport);
 
-
 	lpuart_rx_dma_startup(sport);
 	lpuart_tx_dma_startup(sport);
 
 	lpuart32_configure(sport);
 
 	spin_unlock_irqrestore(&sport->port.lock, flags);
+
+	if (!sport->dma_rx_chan)
+		dev_info_once(sport->port.dev,
+			      "DMA rx channel request failed, operating without rx DMA\n");
+	if (!sport->dma_tx_chan)
+		dev_info_once(sport->port.dev,
+			      "DMA tx channel request failed, operating without tx DMA\n");
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH 2/3] tty: serial: fsl_lpuart: fix return value checking

[Michael Walle]

The return value of lpuart_dma_tx_request() is a negative errno on
failure and zero on success.

Fixes: 159381df1442f ("tty: serial: fsl_lpuart: fix DMA operation when using IOMMU")
Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 960fc2658f19..bbba298b68a4 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1522,7 +1522,7 @@ static void lpuart_tx_dma_startup(struct lpuart_port *sport)
 	}
 
 	ret = lpuart_dma_tx_request(&sport->port);
-	if (!ret)
+	if (ret)
 		goto err;
 
 	init_waitqueue_head(&sport->dma_wait);
-- 
2.20.1

[RFC PATCH 3/3] tty: serial: fsl_lpuart: fix possible console deadlock

[Michael Walle]

If the kernel console output is on this console any
dev_{err,warn,info}() may result in a deadlock if the sport->port.lock
spinlock is already held. This is because the _console_write() try to
aquire this lock, too. Remove any error messages where the spinlock is
taken or print after the lock is released.

Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/tty/serial/fsl_lpuart.c | 35 +++++++--------------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index bbba298b68a4..0910308b38b1 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -420,7 +420,6 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
 {
 	struct circ_buf *xmit = &sport->port.state->xmit;
 	struct scatterlist *sgl = sport->tx_sgl;
-	struct device *dev = sport->port.dev;
 	struct dma_chan *chan = sport->dma_tx_chan;
 	int ret;
 
@@ -442,10 +441,8 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
 
 	ret = dma_map_sg(chan->device->dev, sgl, sport->dma_tx_nents,
 			 DMA_TO_DEVICE);
-	if (!ret) {
-		dev_err(dev, "DMA mapping error for TX.\n");
+	if (!ret)
 		return;
-	}
 
 	sport->dma_tx_desc = dmaengine_prep_slave_sg(chan, sgl,
 					ret, DMA_MEM_TO_DEV,
@@ -453,7 +450,6 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
 	if (!sport->dma_tx_desc) {
 		dma_unmap_sg(chan->device->dev, sgl, sport->dma_tx_nents,
 			      DMA_TO_DEVICE);
-		dev_err(dev, "Cannot prepare TX slave DMA!\n");
 		return;
 	}
 
@@ -520,21 +516,12 @@ static int lpuart_dma_tx_request(struct uart_port *port)
 	struct lpuart_port *sport = container_of(port,
 					struct lpuart_port, port);
 	struct dma_slave_config dma_tx_sconfig = {};
-	int ret;
 
 	dma_tx_sconfig.dst_addr = lpuart_dma_datareg_addr(sport);
 	dma_tx_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
 	dma_tx_sconfig.dst_maxburst = 1;
 	dma_tx_sconfig.direction = DMA_MEM_TO_DEV;
-	ret = dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);
-
-	if (ret) {
-		dev_err(sport->port.dev,
-				"DMA slave config failed, err = %d\n", ret);
-		return ret;
-	}
-
-	return 0;
+	return dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);
 }
 
 static bool lpuart_is_32(struct lpuart_port *sport)
@@ -1074,8 +1061,8 @@ static void lpuart_copy_rx_to_tty(struct lpuart_port *sport)
 
 	dmastat = dmaengine_tx_status(chan, sport->dma_rx_cookie, &state);
 	if (dmastat == DMA_ERROR) {
-		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");
 		spin_unlock_irqrestore(&sport->port.lock, flags);
+		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");
 		return;
 	}
 
@@ -1179,23 +1166,17 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 	sg_init_one(&sport->rx_sgl, ring->buf, sport->rx_dma_rng_buf_len);
 	nent = dma_map_sg(chan->device->dev, &sport->rx_sgl, 1,
 			  DMA_FROM_DEVICE);
-
-	if (!nent) {
-		dev_err(sport->port.dev, "DMA Rx mapping error\n");
+	if (!nent)
 		return -EINVAL;
-	}
 
 	dma_rx_sconfig.src_addr = lpuart_dma_datareg_addr(sport);
 	dma_rx_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
 	dma_rx_sconfig.src_maxburst = 1;
 	dma_rx_sconfig.direction = DMA_DEV_TO_MEM;
-	ret = dmaengine_slave_config(chan, &dma_rx_sconfig);
 
-	if (ret < 0) {
-		dev_err(sport->port.dev,
-				"DMA Rx slave config failed, err = %d\n", ret);
+	ret = dmaengine_slave_config(chan, &dma_rx_sconfig);
+	if (ret < 0)
 		return ret;
-	}
 
 	sport->dma_rx_desc = dmaengine_prep_dma_cyclic(chan,
 				 sg_dma_address(&sport->rx_sgl),
@@ -1203,10 +1184,8 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 				 sport->rx_sgl.length / 2,
 				 DMA_DEV_TO_MEM,
 				 DMA_PREP_INTERRUPT);
-	if (!sport->dma_rx_desc) {
-		dev_err(sport->port.dev, "Cannot prepare cyclic DMA\n");
+	if (!sport->dma_rx_desc)
 		return -EFAULT;
-	}
 
 	sport->dma_rx_desc->callback = lpuart_dma_rx_complete;
 	sport->dma_rx_desc->callback_param = sport;
-- 
2.20.1

Re: [RFC PATCH 3/3] tty: serial: fsl_lpuart: fix possible console deadlock

[Leonard Crestez]

On 2020-03-24 8:48 PM, Michael Walle wrote:
> If the kernel console output is on this console any
> dev_{err,warn,info}() may result in a deadlock if the sport->port.lock
> spinlock is already held. This is because the _console_write() try to
> aquire this lock, too. Remove any error messages where the spinlock is
> taken or print after the lock is released.
> 
> Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
> Signed-off-by: Michael Walle <michael@walle.cc>

It seems that this was an issue even before commit 159381df1442 ("tty: 
serial: fsl_lpuart: fix DMA operation when using IOMMU") but these error 
prints never triggered.

Would it be possible to move all the dma alloc/config/prep outside the 
serial port lock? As it stands this still calls into dmaengine coode and 
that might decide to print as well.

Really I don't think the lock needs to protect more than bits like 
TDMAE/RDMAE.

BTW: You should add more people in CC for reviews, for example 
linux-imx@nxp.com is checked by a lot of people.

> ---
>   drivers/tty/serial/fsl_lpuart.c | 35 +++++++--------------------------
>   1 file changed, 7 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index bbba298b68a4..0910308b38b1 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -420,7 +420,6 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
>   {
>   	struct circ_buf *xmit = &sport->port.state->xmit;
>   	struct scatterlist *sgl = sport->tx_sgl;
> -	struct device *dev = sport->port.dev;
>   	struct dma_chan *chan = sport->dma_tx_chan;
>   	int ret;
>   
> @@ -442,10 +441,8 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
>   
>   	ret = dma_map_sg(chan->device->dev, sgl, sport->dma_tx_nents,
>   			 DMA_TO_DEVICE);
> -	if (!ret) {
> -		dev_err(dev, "DMA mapping error for TX.\n");
> +	if (!ret)
>   		return;
> -	}
>   
>   	sport->dma_tx_desc = dmaengine_prep_slave_sg(chan, sgl,
>   					ret, DMA_MEM_TO_DEV,
> @@ -453,7 +450,6 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
>   	if (!sport->dma_tx_desc) {
>   		dma_unmap_sg(chan->device->dev, sgl, sport->dma_tx_nents,
>   			      DMA_TO_DEVICE);
> -		dev_err(dev, "Cannot prepare TX slave DMA!\n");
>   		return;
>   	}
>   
> @@ -520,21 +516,12 @@ static int lpuart_dma_tx_request(struct uart_port *port)
>   	struct lpuart_port *sport = container_of(port,
>   					struct lpuart_port, port);
>   	struct dma_slave_config dma_tx_sconfig = {};
> -	int ret;
>   
>   	dma_tx_sconfig.dst_addr = lpuart_dma_datareg_addr(sport);
>   	dma_tx_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>   	dma_tx_sconfig.dst_maxburst = 1;
>   	dma_tx_sconfig.direction = DMA_MEM_TO_DEV;
> -	ret = dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);
> -
> -	if (ret) {
> -		dev_err(sport->port.dev,
> -				"DMA slave config failed, err = %d\n", ret);
> -		return ret;
> -	}
> -
> -	return 0;
> +	return dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);
>   }
>   
>   static bool lpuart_is_32(struct lpuart_port *sport)
> @@ -1074,8 +1061,8 @@ static void lpuart_copy_rx_to_tty(struct lpuart_port *sport)
>   
>   	dmastat = dmaengine_tx_status(chan, sport->dma_rx_cookie, &state);
>   	if (dmastat == DMA_ERROR) {
> -		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");
>   		spin_unlock_irqrestore(&sport->port.lock, flags);
> +		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");
>   		return;
>   	}
>   
> @@ -1179,23 +1166,17 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
>   	sg_init_one(&sport->rx_sgl, ring->buf, sport->rx_dma_rng_buf_len);
>   	nent = dma_map_sg(chan->device->dev, &sport->rx_sgl, 1,
>   			  DMA_FROM_DEVICE);
> -
> -	if (!nent) {
> -		dev_err(sport->port.dev, "DMA Rx mapping error\n");
> +	if (!nent)
>   		return -EINVAL;
> -	}
>   
>   	dma_rx_sconfig.src_addr = lpuart_dma_datareg_addr(sport);
>   	dma_rx_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>   	dma_rx_sconfig.src_maxburst = 1;
>   	dma_rx_sconfig.direction = DMA_DEV_TO_MEM;
> -	ret = dmaengine_slave_config(chan, &dma_rx_sconfig);
>   
> -	if (ret < 0) {
> -		dev_err(sport->port.dev,
> -				"DMA Rx slave config failed, err = %d\n", ret);
> +	ret = dmaengine_slave_config(chan, &dma_rx_sconfig);
> +	if (ret < 0)
>   		return ret;
> -	}
>   
>   	sport->dma_rx_desc = dmaengine_prep_dma_cyclic(chan,
>   				 sg_dma_address(&sport->rx_sgl),
> @@ -1203,10 +1184,8 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
>   				 sport->rx_sgl.length / 2,
>   				 DMA_DEV_TO_MEM,
>   				 DMA_PREP_INTERRUPT);
> -	if (!sport->dma_rx_desc) {
> -		dev_err(sport->port.dev, "Cannot prepare cyclic DMA\n");
> +	if (!sport->dma_rx_desc)
>   		return -EFAULT;
> -	}
>   
>   	sport->dma_rx_desc->callback = lpuart_dma_rx_complete;
>   	sport->dma_rx_desc->callback_param = sport;
> 

Re: [RFC PATCH 3/3] tty: serial: fsl_lpuart: fix possible console deadlock

[Michael Walle]

Am 2020-03-25 13:21, schrieb Leonard Crestez:
> On 2020-03-24 8:48 PM, Michael Walle wrote:
>> If the kernel console output is on this console any
>> dev_{err,warn,info}() may result in a deadlock if the sport->port.lock
>> spinlock is already held. This is because the _console_write() try to
>> aquire this lock, too. Remove any error messages where the spinlock is
>> taken or print after the lock is released.
>> 
>> Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> It seems that this was an issue even before commit 159381df1442 ("tty:
> serial: fsl_lpuart: fix DMA operation when using IOMMU") but these 
> error
> prints never triggered.

Yeah, it just triggers because there is now a print by default (if DMA 
is
not available) thus this RFC doesn't contain the Fixes: tag.

> Would it be possible to move all the dma alloc/config/prep outside the
> serial port lock? As it stands this still calls into dmaengine coode 
> and
> that might decide to print as well.

TBH I don't want to refactor the whole driver. All I wanted to do was to
add LS1028A support to this driver which already resulted in a 7 patches
series with various other bugfixes. Could NXP take this over? I could
certainly do rewiews/testing on my board, though.

> Really I don't think the lock needs to protect more than bits like
> TDMAE/RDMAE.
> 
> BTW: You should add more people in CC for reviews, for example
> linux-imx@nxp.com is checked by a lot of people.

It would be good to have N: lpuart (or fsl_lpuart?) in the
corresponding entry in MAINTAINERS, so it will automatically added.
I'll try to remember for the next patches though.

-michael
> 
>> ---
>>   drivers/tty/serial/fsl_lpuart.c | 35 
>> +++++++--------------------------
>>   1 file changed, 7 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/fsl_lpuart.c 
>> b/drivers/tty/serial/fsl_lpuart.c
>> index bbba298b68a4..0910308b38b1 100644
>> --- a/drivers/tty/serial/fsl_lpuart.c
>> +++ b/drivers/tty/serial/fsl_lpuart.c
>> @@ -420,7 +420,6 @@ static void lpuart_dma_tx(struct lpuart_port 
>> *sport)
>>   {
>>   	struct circ_buf *xmit = &sport->port.state->xmit;
>>   	struct scatterlist *sgl = sport->tx_sgl;
>> -	struct device *dev = sport->port.dev;
>>   	struct dma_chan *chan = sport->dma_tx_chan;
>>   	int ret;
>> 
>> @@ -442,10 +441,8 @@ static void lpuart_dma_tx(struct lpuart_port 
>> *sport)
>> 
>>   	ret = dma_map_sg(chan->device->dev, sgl, sport->dma_tx_nents,
>>   			 DMA_TO_DEVICE);
>> -	if (!ret) {
>> -		dev_err(dev, "DMA mapping error for TX.\n");
>> +	if (!ret)
>>   		return;
>> -	}
>> 
>>   	sport->dma_tx_desc = dmaengine_prep_slave_sg(chan, sgl,
>>   					ret, DMA_MEM_TO_DEV,
>> @@ -453,7 +450,6 @@ static void lpuart_dma_tx(struct lpuart_port 
>> *sport)
>>   	if (!sport->dma_tx_desc) {
>>   		dma_unmap_sg(chan->device->dev, sgl, sport->dma_tx_nents,
>>   			      DMA_TO_DEVICE);
>> -		dev_err(dev, "Cannot prepare TX slave DMA!\n");
>>   		return;
>>   	}
>> 
>> @@ -520,21 +516,12 @@ static int lpuart_dma_tx_request(struct 
>> uart_port *port)
>>   	struct lpuart_port *sport = container_of(port,
>>   					struct lpuart_port, port);
>>   	struct dma_slave_config dma_tx_sconfig = {};
>> -	int ret;
>> 
>>   	dma_tx_sconfig.dst_addr = lpuart_dma_datareg_addr(sport);
>>   	dma_tx_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>>   	dma_tx_sconfig.dst_maxburst = 1;
>>   	dma_tx_sconfig.direction = DMA_MEM_TO_DEV;
>> -	ret = dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);
>> -
>> -	if (ret) {
>> -		dev_err(sport->port.dev,
>> -				"DMA slave config failed, err = %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	return dmaengine_slave_config(sport->dma_tx_chan, &dma_tx_sconfig);
>>   }
>> 
>>   static bool lpuart_is_32(struct lpuart_port *sport)
>> @@ -1074,8 +1061,8 @@ static void lpuart_copy_rx_to_tty(struct 
>> lpuart_port *sport)
>> 
>>   	dmastat = dmaengine_tx_status(chan, sport->dma_rx_cookie, &state);
>>   	if (dmastat == DMA_ERROR) {
>> -		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");
>>   		spin_unlock_irqrestore(&sport->port.lock, flags);
>> +		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");
>>   		return;
>>   	}
>> 
>> @@ -1179,23 +1166,17 @@ static inline int lpuart_start_rx_dma(struct 
>> lpuart_port *sport)
>>   	sg_init_one(&sport->rx_sgl, ring->buf, sport->rx_dma_rng_buf_len);
>>   	nent = dma_map_sg(chan->device->dev, &sport->rx_sgl, 1,
>>   			  DMA_FROM_DEVICE);
>> -
>> -	if (!nent) {
>> -		dev_err(sport->port.dev, "DMA Rx mapping error\n");
>> +	if (!nent)
>>   		return -EINVAL;
>> -	}
>> 
>>   	dma_rx_sconfig.src_addr = lpuart_dma_datareg_addr(sport);
>>   	dma_rx_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>>   	dma_rx_sconfig.src_maxburst = 1;
>>   	dma_rx_sconfig.direction = DMA_DEV_TO_MEM;
>> -	ret = dmaengine_slave_config(chan, &dma_rx_sconfig);
>> 
>> -	if (ret < 0) {
>> -		dev_err(sport->port.dev,
>> -				"DMA Rx slave config failed, err = %d\n", ret);
>> +	ret = dmaengine_slave_config(chan, &dma_rx_sconfig);
>> +	if (ret < 0)
>>   		return ret;
>> -	}
>> 
>>   	sport->dma_rx_desc = dmaengine_prep_dma_cyclic(chan,
>>   				 sg_dma_address(&sport->rx_sgl),
>> @@ -1203,10 +1184,8 @@ static inline int lpuart_start_rx_dma(struct 
>> lpuart_port *sport)
>>   				 sport->rx_sgl.length / 2,
>>   				 DMA_DEV_TO_MEM,
>>   				 DMA_PREP_INTERRUPT);
>> -	if (!sport->dma_rx_desc) {
>> -		dev_err(sport->port.dev, "Cannot prepare cyclic DMA\n");
>> +	if (!sport->dma_rx_desc)
>>   		return -EFAULT;
>> -	}
>> 
>>   	sport->dma_rx_desc->callback = lpuart_dma_rx_complete;
>>   	sport->dma_rx_desc->callback_param = sport;
>> 

Re: [PATCH 1/3] tty: serial: fsl_lpuart: move dev_info_once()

[Michael Walle]

Am 2020-03-24 19:47, schrieb Michael Walle:
> Don't take the spinlock and use dev_info_once(). This may cause a hang
> because the console takes this spinlock, too. Just print this info 
> after
> we've released the lock.
> 
> Fixes: 159381df1442f ("tty: serial: fsl_lpuart: fix DMA operation when
> using IOMMU")
> Reported-by: Leonard Crestez <leonard.crestez@nxp.com>
> Signed-off-by: Michael Walle <michael@walle.cc>

Because the patch subject was renamed: This patch series is superseded
by the v2:
  
https://lore.kernel.org/linux-serial/20200325090658.25967-1-michael@walle.cc/

I didn't include the 3/3 RFC patch though. This is a bigger change, 
which
should be carefully reviewed by the maintainer of the fsl_lpuart.c.

-michael


> ---
>  drivers/tty/serial/fsl_lpuart.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c 
> b/drivers/tty/serial/fsl_lpuart.c
> index 9c6a018b1390..960fc2658f19 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1517,9 +1517,6 @@ static void lpuart_tx_dma_startup(struct
> lpuart_port *sport)
> 
>  	sport->dma_tx_chan = dma_request_chan(sport->port.dev, "tx");
>  	if (IS_ERR(sport->dma_tx_chan)) {
> -		dev_info_once(sport->port.dev,
> -			      "DMA tx channel request failed, operating without tx DMA 
> (%ld)\n",
> -			      PTR_ERR(sport->dma_tx_chan));
>  		sport->dma_tx_chan = NULL;
>  		goto err;
>  	}
> @@ -1551,9 +1548,6 @@ static void lpuart_rx_dma_startup(struct
> lpuart_port *sport)
> 
>  	sport->dma_rx_chan = dma_request_chan(sport->port.dev, "rx");
>  	if (IS_ERR(sport->dma_rx_chan)) {
> -		dev_info_once(sport->port.dev,
> -			      "DMA rx channel request failed, operating without rx DMA 
> (%ld)\n",
> -			      PTR_ERR(sport->dma_rx_chan));
>  		sport->dma_rx_chan = NULL;
>  		goto err;
>  	}
> @@ -1601,6 +1595,13 @@ static int lpuart_startup(struct uart_port 
> *port)
> 
>  	spin_unlock_irqrestore(&sport->port.lock, flags);
> 
> +	if (!sport->dma_rx_chan)
> +		dev_info_once(sport->port.dev,
> +			      "DMA rx channel request failed, operating without rx DMA\n");
> +	if (!sport->dma_tx_chan)
> +		dev_info_once(sport->port.dev,
> +			      "DMA tx channel request failed, operating without tx DMA\n");
> +
>  	return 0;
>  }
> 
> @@ -1653,13 +1654,20 @@ static int lpuart32_startup(struct uart_port 
> *port)
> 
>  	lpuart32_setup_watermark_enable(sport);
> 
> -
>  	lpuart_rx_dma_startup(sport);
>  	lpuart_tx_dma_startup(sport);
> 
>  	lpuart32_configure(sport);
> 
>  	spin_unlock_irqrestore(&sport->port.lock, flags);
> +
> +	if (!sport->dma_rx_chan)
> +		dev_info_once(sport->port.dev,
> +			      "DMA rx channel request failed, operating without rx DMA\n");
> +	if (!sport->dma_tx_chan)
> +		dev_info_once(sport->port.dev,
> +			      "DMA tx channel request failed, operating without tx DMA\n");
> +
>  	return 0;
>  }

[PATCH v4 2/4] tty: serial: fsl_lpuart: fix DMA mapping

[Michael Walle]

Use the correct device to request the DMA mapping. Otherwise the IOMMU
doesn't get the mapping and it will generate a page fault.

The error messages look like:
[   19.012140] arm-smmu 5000000.iommu: Unhandled context fault: fsr=0x402, iova=0xbbfff800, fsynr=0x3e0021, cbfrsynra=0x828, cb=9
[   19.023593] arm-smmu 5000000.iommu: Unhandled context fault: fsr=0x402, iova=0xbbfff800, fsynr=0x3e0021, cbfrsynra=0x828, cb=9

This was tested on a custom board with a LS1028A SoC.

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

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 33798df4d727..f45f97cb769c 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -409,6 +409,7 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
 	struct circ_buf *xmit = &sport->port.state->xmit;
 	struct scatterlist *sgl = sport->tx_sgl;
 	struct device *dev = sport->port.dev;
+	struct dma_chan *chan = sport->dma_tx_chan;
 	int ret;
 
 	if (sport->dma_tx_in_progress)
@@ -427,17 +428,19 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
 		sg_set_buf(sgl + 1, xmit->buf, xmit->head);
 	}
 
-	ret = dma_map_sg(dev, sgl, sport->dma_tx_nents, DMA_TO_DEVICE);
+	ret = dma_map_sg(chan->device->dev, sgl, sport->dma_tx_nents,
+			 DMA_TO_DEVICE);
 	if (!ret) {
 		dev_err(dev, "DMA mapping error for TX.\n");
 		return;
 	}
 
-	sport->dma_tx_desc = dmaengine_prep_slave_sg(sport->dma_tx_chan, sgl,
+	sport->dma_tx_desc = dmaengine_prep_slave_sg(chan, sgl,
 					ret, DMA_MEM_TO_DEV,
 					DMA_PREP_INTERRUPT);
 	if (!sport->dma_tx_desc) {
-		dma_unmap_sg(dev, sgl, sport->dma_tx_nents, DMA_TO_DEVICE);
+		dma_unmap_sg(chan->device->dev, sgl, sport->dma_tx_nents,
+			      DMA_TO_DEVICE);
 		dev_err(dev, "Cannot prepare TX slave DMA!\n");
 		return;
 	}
@@ -446,7 +449,7 @@ static void lpuart_dma_tx(struct lpuart_port *sport)
 	sport->dma_tx_desc->callback_param = sport;
 	sport->dma_tx_in_progress = true;
 	sport->dma_tx_cookie = dmaengine_submit(sport->dma_tx_desc);
-	dma_async_issue_pending(sport->dma_tx_chan);
+	dma_async_issue_pending(chan);
 }
 
 static bool lpuart_stopped_or_empty(struct uart_port *port)
@@ -459,11 +462,13 @@ static void lpuart_dma_tx_complete(void *arg)
 	struct lpuart_port *sport = arg;
 	struct scatterlist *sgl = &sport->tx_sgl[0];
 	struct circ_buf *xmit = &sport->port.state->xmit;
+	struct dma_chan *chan = sport->dma_tx_chan;
 	unsigned long flags;
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
-	dma_unmap_sg(sport->port.dev, sgl, sport->dma_tx_nents, DMA_TO_DEVICE);
+	dma_unmap_sg(chan->device->dev, sgl, sport->dma_tx_nents,
+		     DMA_TO_DEVICE);
 
 	xmit->tail = (xmit->tail + sport->dma_tx_bytes) & (UART_XMIT_SIZE - 1);
 
@@ -529,15 +534,16 @@ static bool lpuart_is_32(struct lpuart_port *sport)
 static void lpuart_flush_buffer(struct uart_port *port)
 {
 	struct lpuart_port *sport = container_of(port, struct lpuart_port, port);
+	struct dma_chan *chan = sport->dma_tx_chan;
 	u32 val;
 
 	if (sport->lpuart_dma_tx_use) {
 		if (sport->dma_tx_in_progress) {
-			dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
+			dma_unmap_sg(chan->device->dev, &sport->tx_sgl[0],
 				sport->dma_tx_nents, DMA_TO_DEVICE);
 			sport->dma_tx_in_progress = false;
 		}
-		dmaengine_terminate_all(sport->dma_tx_chan);
+		dmaengine_terminate_all(chan);
 	}
 
 	if (lpuart_is_32(sport)) {
@@ -993,6 +999,7 @@ static void lpuart_copy_rx_to_tty(struct lpuart_port *sport)
 	struct tty_port *port = &sport->port.state->port;
 	struct dma_tx_state state;
 	enum dma_status dmastat;
+	struct dma_chan *chan = sport->dma_rx_chan;
 	struct circ_buf *ring = &sport->rx_ring;
 	unsigned long flags;
 	int count = 0;
@@ -1053,10 +1060,7 @@ static void lpuart_copy_rx_to_tty(struct lpuart_port *sport)
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 
-	dmastat = dmaengine_tx_status(sport->dma_rx_chan,
-				sport->dma_rx_cookie,
-				&state);
-
+	dmastat = dmaengine_tx_status(chan, sport->dma_rx_cookie, &state);
 	if (dmastat == DMA_ERROR) {
 		dev_err(sport->port.dev, "Rx DMA transfer failed!\n");
 		spin_unlock_irqrestore(&sport->port.lock, flags);
@@ -1064,7 +1068,8 @@ static void lpuart_copy_rx_to_tty(struct lpuart_port *sport)
 	}
 
 	/* CPU claims ownership of RX DMA buffer */
-	dma_sync_sg_for_cpu(sport->port.dev, &sport->rx_sgl, 1, DMA_FROM_DEVICE);
+	dma_sync_sg_for_cpu(chan->device->dev, &sport->rx_sgl, 1,
+			    DMA_FROM_DEVICE);
 
 	/*
 	 * ring->head points to the end of data already written by the DMA.
@@ -1106,7 +1111,7 @@ static void lpuart_copy_rx_to_tty(struct lpuart_port *sport)
 		sport->port.icount.rx += count;
 	}
 
-	dma_sync_sg_for_device(sport->port.dev, &sport->rx_sgl, 1,
+	dma_sync_sg_for_device(chan->device->dev, &sport->rx_sgl, 1,
 			       DMA_FROM_DEVICE);
 
 	spin_unlock_irqrestore(&sport->port.lock, flags);
@@ -1138,6 +1143,7 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 	struct tty_port *port = &sport->port.state->port;
 	struct tty_struct *tty = port->tty;
 	struct ktermios *termios = &tty->termios;
+	struct dma_chan *chan = sport->dma_rx_chan;
 
 	baud = tty_get_baud_rate(tty);
 
@@ -1159,7 +1165,8 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 		return -ENOMEM;
 
 	sg_init_one(&sport->rx_sgl, ring->buf, sport->rx_dma_rng_buf_len);
-	nent = dma_map_sg(sport->port.dev, &sport->rx_sgl, 1, DMA_FROM_DEVICE);
+	nent = dma_map_sg(chan->device->dev, &sport->rx_sgl, 1,
+			  DMA_FROM_DEVICE);
 
 	if (!nent) {
 		dev_err(sport->port.dev, "DMA Rx mapping error\n");
@@ -1170,7 +1177,7 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 	dma_rx_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
 	dma_rx_sconfig.src_maxburst = 1;
 	dma_rx_sconfig.direction = DMA_DEV_TO_MEM;
-	ret = dmaengine_slave_config(sport->dma_rx_chan, &dma_rx_sconfig);
+	ret = dmaengine_slave_config(chan, &dma_rx_sconfig);
 
 	if (ret < 0) {
 		dev_err(sport->port.dev,
@@ -1178,7 +1185,7 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 		return ret;
 	}
 
-	sport->dma_rx_desc = dmaengine_prep_dma_cyclic(sport->dma_rx_chan,
+	sport->dma_rx_desc = dmaengine_prep_dma_cyclic(chan,
 				 sg_dma_address(&sport->rx_sgl),
 				 sport->rx_sgl.length,
 				 sport->rx_sgl.length / 2,
@@ -1192,7 +1199,7 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 	sport->dma_rx_desc->callback = lpuart_dma_rx_complete;
 	sport->dma_rx_desc->callback_param = sport;
 	sport->dma_rx_cookie = dmaengine_submit(sport->dma_rx_desc);
-	dma_async_issue_pending(sport->dma_rx_chan);
+	dma_async_issue_pending(chan);
 
 	if (lpuart_is_32(sport)) {
 		unsigned long temp = lpuart32_read(&sport->port, UARTBAUD);
@@ -1210,11 +1217,12 @@ static void lpuart_dma_rx_free(struct uart_port *port)
 {
 	struct lpuart_port *sport = container_of(port,
 					struct lpuart_port, port);
+	struct dma_chan *chan = sport->dma_rx_chan;
 
-	if (sport->dma_rx_chan)
-		dmaengine_terminate_all(sport->dma_rx_chan);
+	if (chan)
+		dmaengine_terminate_all(chan);
 
-	dma_unmap_sg(sport->port.dev, &sport->rx_sgl, 1, DMA_FROM_DEVICE);
+	dma_unmap_sg(chan->device->dev, &sport->rx_sgl, 1, DMA_FROM_DEVICE);
 	kfree(sport->rx_ring.buf);
 	sport->rx_ring.tail = 0;
 	sport->rx_ring.head = 0;
-- 
2.20.1

[PATCH v4 3/4] 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 f45f97cb769c..1d4f75716e32 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;
@@ -1626,6 +1638,17 @@ static int lpuart32_startup(struct uart_port *port)
 	sport->rxfifo_size = UARTFIFO_DEPTH((temp >> UARTFIFO_RXSIZE_OFF) &
 					    UARTFIFO_FIFOSIZE_MASK);
 
+	/*
+	 * 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;
+	}
+
 	spin_lock_irqsave(&sport->port.lock, flags);
 
 	lpuart32_setup_watermark_enable(sport);
-- 
2.20.1

[PATCH v4 4/4] 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 1d4f75716e32..9c6a018b1390 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1878,11 +1878,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.
@@ -1928,10 +1929,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;
@@ -1942,14 +1943,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)
@@ -2443,6 +2453,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)
 {
@@ -2457,6 +2491,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