enable IDDMAEN in UCR4 to let sdma script has the chance to detect
the idle status and transfer the last tail data with the interrupt
notifying uart driver.Otherwise, the last dma done interrupt of the
tail data in rxfifo whose size is less than watermark may never be
received by uart driver.
Signed-off-by: Robin Gong <[email protected]>
---
drivers/tty/serial/imx.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 239c0fa..bbb1693 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1278,7 +1278,7 @@ static int imx_uart_dma_init(struct imx_port *sport)
static void imx_uart_enable_dma(struct imx_port *sport)
{
- u32 ucr1;
+ u32 ucr1, ucr4;
imx_uart_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
@@ -1287,18 +1287,26 @@ static void imx_uart_enable_dma(struct imx_port *sport)
ucr1 |= UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN;
imx_uart_writel(sport, ucr1, UCR1);
+ ucr4 = imx_uart_readl(sport, UCR4);
+ ucr4 |= UCR4_IDDMAEN;
+ imx_uart_writel(sport, ucr4, UCR4);
+
sport->dma_is_enabled = 1;
}
static void imx_uart_disable_dma(struct imx_port *sport)
{
- u32 ucr1;
+ u32 ucr1, ucr4;
/* clear UCR1 */
ucr1 = imx_uart_readl(sport, UCR1);
ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
imx_uart_writel(sport, ucr1, UCR1);
+ ucr4 = imx_uart_readl(sport, UCR4);
+ ucr4 &= ~UCR4_IDDMAEN;
+ imx_uart_writel(sport, ucr4, UCR4);
+
imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
sport->dma_is_enabled = 0;
--
2.7.4
Hello,
On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote:
> enable IDDMAEN in UCR4 to let sdma script has the chance to detect
> the idle status and transfer the last tail data with the interrupt
> notifying uart driver.Otherwise, the last dma done interrupt of the
> tail data in rxfifo whose size is less than watermark may never be
> received by uart driver.
Is this a theoretic issue? Or does it fix a real problem?
If the former I'd say UCR1_ATDMAEN being set should prevent this stall.
Also I'd say the SDMA scripts I know don't properly handle the IDDMA
request.
Best regards
Uwe
> Signed-off-by: Robin Gong <[email protected]>
> ---
> drivers/tty/serial/imx.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 239c0fa..bbb1693 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1278,7 +1278,7 @@ static int imx_uart_dma_init(struct imx_port *sport)
>
> static void imx_uart_enable_dma(struct imx_port *sport)
> {
> - u32 ucr1;
> + u32 ucr1, ucr4;
>
> imx_uart_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
>
> @@ -1287,18 +1287,26 @@ static void imx_uart_enable_dma(struct imx_port *sport)
> ucr1 |= UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN;
> imx_uart_writel(sport, ucr1, UCR1);
>
> + ucr4 = imx_uart_readl(sport, UCR4);
> + ucr4 |= UCR4_IDDMAEN;
> + imx_uart_writel(sport, ucr4, UCR4);
> +
> sport->dma_is_enabled = 1;
> }
>
> static void imx_uart_disable_dma(struct imx_port *sport)
> {
> - u32 ucr1;
> + u32 ucr1, ucr4;
>
> /* clear UCR1 */
> ucr1 = imx_uart_readl(sport, UCR1);
> ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
> imx_uart_writel(sport, ucr1, UCR1);
>
> + ucr4 = imx_uart_readl(sport, UCR4);
> + ucr4 &= ~UCR4_IDDMAEN;
> + imx_uart_writel(sport, ucr4, UCR4);
> +
> imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>
> sport->dma_is_enabled = 0;
> --
> 2.7.4
>
>
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
> -----Original Message-----
> From: Uwe Kleine-König <[email protected]>
> Sent: 2018年9月20日 15:55
> To: Robin Gong <[email protected]>
> Cc: [email protected]; Andy Duan <[email protected]>;
> [email protected]; [email protected]; dl-linux-imx
> <[email protected]>; [email protected]
> Subject: Re: [PATCH v1] tty: serial: imx: enable IDDMAEN for the last tail data
>
> Hello,
>
> On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote:
> > enable IDDMAEN in UCR4 to let sdma script has the chance to detect the
> > idle status and transfer the last tail data with the interrupt
> > notifying uart driver.Otherwise, the last dma done interrupt of the
> > tail data in rxfifo whose size is less than watermark may never be
> > received by uart driver.
>
> Is this a theoretic issue? Or does it fix a real problem?
>
> If the former I'd say UCR1_ATDMAEN being set should prevent this stall.
It's a real fix, you will see 'timeout We read 0 byte' with the attached stress test
running on i.mx7d or i.mx6 if no this patch applied.
' while [ 1 ]; do /unit_tests/UART/mxc_uart_stress_test.out /dev/ttymxc5 2000000 D L 100 100 L; done &'
Please make sure dma enabled on ttymxcX for your test, and sdma firmware setup in your rootfs
/lib/firmware/imx/sdma/sdma-imx6q.bin or /lib/firmware/imx/sdma/sdma-imx7d.bin.
You can get sdma firmware from below:
https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/imx/sdma
>
> Also I'd say the SDMA scripts I know don't properly handle the IDDMA request.
>
> Best regards
> Uwe
>
> > Signed-off-by: Robin Gong <[email protected]>
> > ---
> > drivers/tty/serial/imx.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> > 239c0fa..bbb1693 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1278,7 +1278,7 @@ static int imx_uart_dma_init(struct imx_port
> > *sport)
> >
> > static void imx_uart_enable_dma(struct imx_port *sport) {
> > - u32 ucr1;
> > + u32 ucr1, ucr4;
> >
> > imx_uart_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
> >
> > @@ -1287,18 +1287,26 @@ static void imx_uart_enable_dma(struct
> imx_port *sport)
> > ucr1 |= UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN;
> > imx_uart_writel(sport, ucr1, UCR1);
> >
> > + ucr4 = imx_uart_readl(sport, UCR4);
> > + ucr4 |= UCR4_IDDMAEN;
> > + imx_uart_writel(sport, ucr4, UCR4);
> > +
> > sport->dma_is_enabled = 1;
> > }
> >
> > static void imx_uart_disable_dma(struct imx_port *sport) {
> > - u32 ucr1;
> > + u32 ucr1, ucr4;
> >
> > /* clear UCR1 */
> > ucr1 = imx_uart_readl(sport, UCR1);
> > ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
> > imx_uart_writel(sport, ucr1, UCR1);
> >
> > + ucr4 = imx_uart_readl(sport, UCR4);
> > + ucr4 &= ~UCR4_IDDMAEN;
> > + imx_uart_writel(sport, ucr4, UCR4);
> > +
> > imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
> >
> > sport->dma_is_enabled = 0;
> > --
> > 2.7.4
> >
> >
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&data=02%7C01%7Cyibin.gong%40nxp.com%7Cebbc2
> 26ea12d4f6dd93b08d61ece6add%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636730269265048370&sdata=7NstF74mOGOFajEwFjBw7FAh
> h3JdvGAIC9nliwrsf0Y%3D&reserved=0 |
Am Donnerstag, den 20.09.2018, 08:39 +0000 schrieb Robin Gong:
> > -----Original Message-----
> > From: Uwe Kleine-König <[email protected]>
> > Sent: 2018年9月20日 15:55
> > To: Robin Gong <[email protected]>
> > Cc: [email protected]; Andy Duan <[email protected]>;
> > [email protected]; [email protected]; dl-
> > linux-imx
> > <[email protected]>; [email protected]
> > Subject: Re: [PATCH v1] tty: serial: imx: enable IDDMAEN for the
> > last tail data
> >
> > Hello,
> >
> > On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote:
> > > enable IDDMAEN in UCR4 to let sdma script has the chance to
> > > detect the
> > > idle status and transfer the last tail data with the interrupt
> > > notifying uart driver.Otherwise, the last dma done interrupt of
> > > the
> > > tail data in rxfifo whose size is less than watermark may never
> > > be
> > > received by uart driver.
> >
> > Is this a theoretic issue? Or does it fix a real problem?
> >
> > If the former I'd say UCR1_ATDMAEN being set should prevent this
> > stall.
>
> It's a real fix, you will see 'timeout We read 0 byte' with the
> attached stress test
> running on i.mx7d or i.mx6 if no this patch applied.
> ' while [ 1 ]; do /unit_tests/UART/mxc_uart_stress_test.out
> /dev/ttymxc5 2000000 D L 100 100 L; done &'
> Please make sure dma enabled on ttymxcX for your test, and sdma
> firmware setup in your rootfs
> /lib/firmware/imx/sdma/sdma-imx6q.bin or /lib/firmware/imx/sdma/sdma-
> imx7d.bin.
> You can get sdma firmware from below:
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-
> firmware.git/tree/imx/sdma
Please see commit 905c0decad28402aa166975023fb88c8f62f93c8 on why using
the idle detect together with with SDMA is wrong. We don't want to re-
introduce the broken behavior.
Regards,
Lucas
From: Lucas Stach <[email protected]> Sent: 2018年9月20日 17:40
> Am Donnerstag, den 20.09.2018, 08:39 +0000 schrieb Robin Gong:
> > > -----Original Message-----
> > > From: Uwe Kleine-König <[email protected]>
> > > Sent: 2018年9月20日 15:55
> > > To: Robin Gong <[email protected]>
> > > Cc: [email protected]; Andy Duan <[email protected]>;
> > > [email protected]; [email protected]; dl-
> > > linux-imx <[email protected]>; [email protected]
> > > Subject: Re: [PATCH v1] tty: serial: imx: enable IDDMAEN for the
> > > last tail data
> > >
> > > Hello,
> > >
> > > On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote:
> > > > enable IDDMAEN in UCR4 to let sdma script has the chance to detect
> > > > the idle status and transfer the last tail data with the interrupt
> > > > notifying uart driver.Otherwise, the last dma done interrupt of
> > > > the tail data in rxfifo whose size is less than watermark may
> > > > never be received by uart driver.
> > >
> > > Is this a theoretic issue? Or does it fix a real problem?
> > >
> > > If the former I'd say UCR1_ATDMAEN being set should prevent this
> > > stall.
> >
> > It's a real fix, you will see 'timeout We read 0 byte' with the
> > attached stress test running on i.mx7d or i.mx6 if no this patch
> > applied.
> > ' while [ 1 ]; do /unit_tests/UART/mxc_uart_stress_test.out
> > /dev/ttymxc5 2000000 D L 100 100 L; done &'
> > Please make sure dma enabled on ttymxcX for your test, and sdma
> > firmware setup in your rootfs /lib/firmware/imx/sdma/sdma-imx6q.bin
> or
> > /lib/firmware/imx/sdma/sdma- imx7d.bin.
> > You can get sdma firmware from below:
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> t
> > .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ffirmware%2Flin
> ux-&
> > ;data=02%7C01%7Cfugang.duan%40nxp.com%7C213076f6d1114148b5
> ce08d61edcfb
> >
> 88%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636730331
> 814804545&
> > ;sdata=d0uZMjLc%2FrIae3Wd6PWtX2gEGlvZcUjPBVqKK9550jg%3D&
> ;reserved=0
> > firmware.git/tree/imx/sdma
>
> Please see commit 905c0decad28402aa166975023fb88c8f62f93c8 on
> why using the idle detect together with with SDMA is wrong. We don't
> want to re- introduce the broken behavior.
>
The patch just want to use idle timer to stop SDMA transfer using the new ram script, whose action is not the same as rom script.
Sdma rom script how to stop sdma transfer if only one byte in rx FIFO that trigger SDMA request ?
Am Donnerstag, den 20.09.2018, 10:33 +0000 schrieb Andy Duan:
> From: Lucas Stach <[email protected]> Sent: 2018年9月20日 17:40
> > Am Donnerstag, den 20.09.2018, 08:39 +0000 schrieb Robin Gong:
> > > > -----Original Message-----
> > > > From: Uwe Kleine-König <[email protected]>
> > > > Sent: 2018年9月20日 15:55
> > > > To: Robin Gong <[email protected]>
> > > > Cc: [email protected]; Andy Duan <[email protected]>;
> > > > [email protected]; [email protected]; dl-
> > > > linux-imx <[email protected]>; [email protected]
> > > > Subject: Re: [PATCH v1] tty: serial: imx: enable IDDMAEN for
> > > > the
> > > > last tail data
> > > >
> > > > Hello,
> > > >
> > > > On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote:
> > > > > enable IDDMAEN in UCR4 to let sdma script has the chance to
> > > > > detect
> > > > > the idle status and transfer the last tail data with the
> > > > > interrupt
> > > > > notifying uart driver.Otherwise, the last dma done interrupt
> > > > > of
> > > > > the tail data in rxfifo whose size is less than watermark may
> > > > > never be received by uart driver.
> > > >
> > > > Is this a theoretic issue? Or does it fix a real problem?
> > > >
> > > > If the former I'd say UCR1_ATDMAEN being set should prevent
> > > > this
> > > > stall.
> > >
> > > It's a real fix, you will see 'timeout We read 0 byte' with the
> > > attached stress test running on i.mx7d or i.mx6 if no this patch
> > > applied.
> > > ' while [ 1 ]; do /unit_tests/UART/mxc_uart_stress_test.out
> > > /dev/ttymxc5 2000000 D L 100 100 L; done &'
> > > Please make sure dma enabled on ttymxcX for your test, and sdma
> > > firmware setup in your rootfs /lib/firmware/imx/sdma/sdma-
> > > imx6q.bin
> >
> > or
> > > /lib/firmware/imx/sdma/sdma- imx7d.bin.
> > > You can get sdma firmware from below:
> > >
> >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > gi
> > t
> > > .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ffirmware%2Flin
> >
> > ux-&
> > > ;data=02%7C01%7Cfugang.duan%40nxp.com%7C213076f6d1114148b5
> >
> > ce08d61edcfb
> > >
> >
> > 88%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636730331
> > 814804545&
> > > ;sdata=d0uZMjLc%2FrIae3Wd6PWtX2gEGlvZcUjPBVqKK9550jg%3D&
> >
> > ;reserved=0
> > > firmware.git/tree/imx/sdma
> >
> > Please see commit 905c0decad28402aa166975023fb88c8f62f93c8 on
> > why using the idle detect together with with SDMA is wrong. We
> > don't
> > want to re- introduce the broken behavior.
> >
>
> The patch just want to use idle timer to stop SDMA transfer using the
> new ram script, whose action is not the same as rom script.
> Sdma rom script how to stop sdma transfer if only one byte in rx FIFO
> that trigger SDMA request ?
Right, and using the idle detect doesn't work with the ROM script as
this doesn't read the remaining bytes in the FIFO when the idle detect
condition is hit.
Instead with the ROM script the aging timer is used to terminate the
DMA transfer. So if 1 byte is sitting in the FIFO below the watermark
level, the serial core will trigger a DMA request when the aging time
is hit. The ROM script checks the aging status and reads the remaining
bytes from the FIFO, then terminates the transfer. This is all
documented in the reference manual.
I fail to see why this shouldn't work with the RAM script. If it
doesn't work with the RAM firmware, this script is violating the
interface between the serial core and the script in an incompatible way
and needs fixing.
Regards,
Lucas
From: Lucas Stach <[email protected]> Sent: 2018年9月20日 18:42
> Am Donnerstag, den 20.09.2018, 10:33 +0000 schrieb Andy Duan:
> > From: Lucas Stach <[email protected]> Sent: 2018年9月20日
> 17:40
> > > Am Donnerstag, den 20.09.2018, 08:39 +0000 schrieb Robin Gong:
> > > > > -----Original Message-----
> > > > > From: Uwe Kleine-König <[email protected]>
> > > > > Sent: 2018年9月20日 15:55
> > > > > To: Robin Gong <[email protected]>
> > > > > Cc: [email protected]; Andy Duan <[email protected]>;
> > > > > [email protected]; [email protected]; dl-
> > > > > linux-imx <[email protected]>; [email protected]
> > > > > Subject: Re: [PATCH v1] tty: serial: imx: enable IDDMAEN for the
> > > > > last tail data
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Thu, Sep 20, 2018 at 11:26:00PM +0800, Robin Gong wrote:
> > > > > > enable IDDMAEN in UCR4 to let sdma script has the chance to
> > > > > > detect the idle status and transfer the last tail data with
> > > > > > the interrupt notifying uart driver.Otherwise, the last dma
> > > > > > done interrupt of the tail data in rxfifo whose size is less
> > > > > > than watermark may never be received by uart driver.
> > > > >
> > > > > Is this a theoretic issue? Or does it fix a real problem?
> > > > >
> > > > > If the former I'd say UCR1_ATDMAEN being set should prevent this
> > > > > stall.
> > > >
> > > > It's a real fix, you will see 'timeout We read 0 byte' with the
> > > > attached stress test running on i.mx7d or i.mx6 if no this patch
> > > > applied.
> > > > ' while [ 1 ]; do /unit_tests/UART/mxc_uart_stress_test.out
> > > > /dev/ttymxc5 2000000 D L 100 100 L; done &'
> > > > Please make sure dma enabled on ttymxcX for your test, and sdma
> > > > firmware setup in your rootfs /lib/firmware/imx/sdma/sdma-
> > > > imx6q.bin
> > >
> > > or
> > > > /lib/firmware/imx/sdma/sdma- imx7d.bin.
> > > > You can get sdma firmware from below:
> > > >
> > >
> > >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > gi
> > > t
> > > > .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ffirmware%2
> Flin
> > >
> > > ux-&
> > > > ;data=02%7C01%7Cfugang.duan%40nxp.com%7C213076f6d111414
> 8b5
> > >
> > > ce08d61edcfb
> > > >
> > >
> > >
> 88%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636730331
> > > 814804545&
> > > > ;sdata=d0uZMjLc%2FrIae3Wd6PWtX2gEGlvZcUjPBVqKK9550jg%3D&
> amp
> > >
> > > ;reserved=0
> > > > firmware.git/tree/imx/sdma
> > >
> > > Please see commit 905c0decad28402aa166975023fb88c8f62f93c8
> on why
> > > using the idle detect together with with SDMA is wrong. We don't
> > > want to re- introduce the broken behavior.
> > >
> >
> > The patch just want to use idle timer to stop SDMA transfer using the
> > new ram script, whose action is not the same as rom script.
> > Sdma rom script how to stop sdma transfer if only one byte in rx FIFO
> > that trigger SDMA request ?
>
> Right, and using the idle detect doesn't work with the ROM script as this
> doesn't read the remaining bytes in the FIFO when the idle detect
> condition is hit.
>
> Instead with the ROM script the aging timer is used to terminate the DMA
> transfer. So if 1 byte is sitting in the FIFO below the watermark level, the
> serial core will trigger a DMA request when the aging time is hit. The ROM
> script checks the aging status and reads the remaining bytes from the FIFO,
> then terminates the transfer. This is all documented in the reference
> manual.
>
> I fail to see why this shouldn't work with the RAM script. If it doesn't work
> with the RAM firmware, this script is violating the interface between the
> serial core and the script in an incompatible way and needs fixing.
>
If rom script stop DMA transfer when SDMA is triggered by aging timer, then idler timer is not needed. And the data copy action should have no problem. I will do stress test with SDMA rom script.
For ram script, there have many changes and different action with rom script. Aging timer trigger cannot stop current SDMA transfer, so use idler timer as the stop condition trigger source.
We will do more test on rom script, if pass our all test cases, we can align ram script with rom script.
Thanks.
Andy