| Message ID | 20170204213931.3566-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 5, 2017 at 12:56 PM, Fabio Estevam <fabio.estevam@nxp.com> wrote: > [Sorry, for the top-posting] > > > imx6sx uart should be of a IMX6Q_UART instead. > > > Does this patch fix the issue? It seems this dts change will not fix the problem. > From: Kai Ruhnau <kai.ruhnau@target-sg.com> > > In 1c06bde6 with the introduction of DMA support for the i.MX53 platform, > DMA was enabled for all platforms instead. On an i.MX6SX (which is a > IMX21_UART), this breaks gpsd talking to an UART module with > > imx-uart 21f4000.serial: DMA transaction error. > > This patch makes sure, UART DMA is only activated for i.MX53 and i.MX6Q. > > Signed-off-by: Kai Ruhnau <kai.ruhnau@target-sg.com> > Reviewed-by: Marek Vasut <marex@denx.de> > Fixes: 1c06bde643d0 ("Allowing UART DMA to be configured on i.MX53") > Cc: <stable@vger.kernel.org> # 4.9+ > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Martyn Welch <martyn.welch@collabora.co.uk> > Cc: Shawn Guo <shawnguo@kernel.org> > --- > drivers/tty/serial/imx.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index a70356d..8e2b5f2 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -1269,7 +1269,8 @@ static int imx_startup(struct uart_port *port) > writel(temp & ~UCR4_DREN, sport->port.membase + UCR4); > > /* Can we enable the DMA support? */ > - if (!uart_console(port) && !sport->dma_is_inited) > + if ((is_imx6q_uart(sport) || is_imx53_uart(sport)) && > + !uart_console(port) && !sport->dma_is_inited) > imx_uart_dma_init(sport); Looks like imx serial DMA is broken in 4.9. There is also this report from Clemens: https://lkml.org/lkml/2017/1/4/579 Could anyone run a git bisect?
On Sun, Feb 05, 2017 at 02:36:10PM -0200, Fabio Estevam wrote: > On Sun, Feb 5, 2017 at 12:56 PM, Fabio Estevam <fabio.estevam@nxp.com> wrote: > > [Sorry, for the top-posting] > > > > > > imx6sx uart should be of a IMX6Q_UART instead. > > > > > > Does this patch fix the issue? > > It seems this dts change will not fix the problem. > > > From: Kai Ruhnau <kai.ruhnau@target-sg.com> > > > > In 1c06bde6 with the introduction of DMA support for the i.MX53 platform, > > DMA was enabled for all platforms instead. On an i.MX6SX (which is a > > IMX21_UART), this breaks gpsd talking to an UART module with > > > > imx-uart 21f4000.serial: DMA transaction error. > > > > This patch makes sure, UART DMA is only activated for i.MX53 and i.MX6Q. > > > > Signed-off-by: Kai Ruhnau <kai.ruhnau@target-sg.com> > > Reviewed-by: Marek Vasut <marex@denx.de> > > Fixes: 1c06bde643d0 ("Allowing UART DMA to be configured on i.MX53") > > Cc: <stable@vger.kernel.org> # 4.9+ > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > Cc: Martyn Welch <martyn.welch@collabora.co.uk> > > Cc: Shawn Guo <shawnguo@kernel.org> > > --- > > drivers/tty/serial/imx.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > > index a70356d..8e2b5f2 100644 > > --- a/drivers/tty/serial/imx.c > > +++ b/drivers/tty/serial/imx.c > > @@ -1269,7 +1269,8 @@ static int imx_startup(struct uart_port *port) > > writel(temp & ~UCR4_DREN, sport->port.membase + UCR4); > > > > /* Can we enable the DMA support? */ > > - if (!uart_console(port) && !sport->dma_is_inited) > > + if ((is_imx6q_uart(sport) || is_imx53_uart(sport)) && > > + !uart_console(port) && !sport->dma_is_inited) > > imx_uart_dma_init(sport); > > Looks like imx serial DMA is broken in 4.9. There are probably two separate problems: A) I also observed the DMA transaction error message when I used the ROM SDMA scripts. Seems to affect single- and multi-core systems and is fixed by: github.com/Freescale/linux-fslc/commit/a2ec8df37f5d (?) This is also not RS-485 specific, but affects RS-232 too. B) The RS-485 DMA problem I reported, which only affects multi-core systems. RS-232 DMA is not affected. > > There is also this report from Clemens: https://lkml.org/lkml/2017/1/4/579 > > Could anyone run a git bisect? I reverted tty/serial/imx.c to the first implementation of RS-485, namely to commit 17b8f2a3fdca ("serial: imx: add support for half duplex rs485"). Also had to add "fsl," in front of uart-has-rts-cts in the DT. But it does not work either (same behavior). Looks like RS-485 DMA never worked on SMP systems..? It's still a mistery to me what is causing the xmit->tail index to walk over the xmit->head index, leading to the rest of the whole UART_XMIT_SIZE buffer to be sent out. But it must be connected to the transmit-complete interrupt, because if I disable it, this does not happen. Thanks, Clemens
On 02/05/2017 05:36 PM, Fabio Estevam wrote: > On Sun, Feb 5, 2017 at 12:56 PM, Fabio Estevam <fabio.estevam@nxp.com> wrote: >> [Sorry, for the top-posting] >> >> >> imx6sx uart should be of a IMX6Q_UART instead. >> >> >> Does this patch fix the issue? > > It seems this dts change will not fix the problem. Yeah, it won't , see the commit that this patch fixes. It introduces a bug and if you want DMA on MX6SX, this should be another separate patch. >> From: Kai Ruhnau <kai.ruhnau@target-sg.com> >> >> In 1c06bde6 with the introduction of DMA support for the i.MX53 platform, >> DMA was enabled for all platforms instead. On an i.MX6SX (which is a >> IMX21_UART), this breaks gpsd talking to an UART module with >> >> imx-uart 21f4000.serial: DMA transaction error. >> >> This patch makes sure, UART DMA is only activated for i.MX53 and i.MX6Q. >> >> Signed-off-by: Kai Ruhnau <kai.ruhnau@target-sg.com> >> Reviewed-by: Marek Vasut <marex@denx.de> >> Fixes: 1c06bde643d0 ("Allowing UART DMA to be configured on i.MX53") >> Cc: <stable@vger.kernel.org> # 4.9+ >> Cc: Fabio Estevam <fabio.estevam@nxp.com> >> Cc: Martyn Welch <martyn.welch@collabora.co.uk> >> Cc: Shawn Guo <shawnguo@kernel.org> >> --- >> drivers/tty/serial/imx.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >> index a70356d..8e2b5f2 100644 >> --- a/drivers/tty/serial/imx.c >> +++ b/drivers/tty/serial/imx.c >> @@ -1269,7 +1269,8 @@ static int imx_startup(struct uart_port *port) >> writel(temp & ~UCR4_DREN, sport->port.membase + UCR4); >> >> /* Can we enable the DMA support? */ >> - if (!uart_console(port) && !sport->dma_is_inited) >> + if ((is_imx6q_uart(sport) || is_imx53_uart(sport)) && >> + !uart_console(port) && !sport->dma_is_inited) >> imx_uart_dma_init(sport); > > Looks like imx serial DMA is broken in 4.9. Well yes, DMA is broken in 4.9 , that's why it's CC stable 4.9+ :) > There is also this report from Clemens: https://lkml.org/lkml/2017/1/4/579 > > Could anyone run a git bisect? Ummmmmmmm ... see above ? :) Fixes: 1c06bde643d0 ("Allowing UART DMA to be configured on i.MX53")
On 02/05/2017 06:51 PM, Clemens Gruber wrote: > On Sun, Feb 05, 2017 at 02:36:10PM -0200, Fabio Estevam wrote: >> On Sun, Feb 5, 2017 at 12:56 PM, Fabio Estevam <fabio.estevam@nxp.com> wrote: >>> [Sorry, for the top-posting] >>> >>> >>> imx6sx uart should be of a IMX6Q_UART instead. >>> >>> >>> Does this patch fix the issue? >> >> It seems this dts change will not fix the problem. >> >>> From: Kai Ruhnau <kai.ruhnau@target-sg.com> >>> >>> In 1c06bde6 with the introduction of DMA support for the i.MX53 platform, >>> DMA was enabled for all platforms instead. On an i.MX6SX (which is a >>> IMX21_UART), this breaks gpsd talking to an UART module with >>> >>> imx-uart 21f4000.serial: DMA transaction error. >>> >>> This patch makes sure, UART DMA is only activated for i.MX53 and i.MX6Q. >>> >>> Signed-off-by: Kai Ruhnau <kai.ruhnau@target-sg.com> >>> Reviewed-by: Marek Vasut <marex@denx.de> >>> Fixes: 1c06bde643d0 ("Allowing UART DMA to be configured on i.MX53") >>> Cc: <stable@vger.kernel.org> # 4.9+ >>> Cc: Fabio Estevam <fabio.estevam@nxp.com> >>> Cc: Martyn Welch <martyn.welch@collabora.co.uk> >>> Cc: Shawn Guo <shawnguo@kernel.org> >>> --- >>> drivers/tty/serial/imx.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c >>> index a70356d..8e2b5f2 100644 >>> --- a/drivers/tty/serial/imx.c >>> +++ b/drivers/tty/serial/imx.c >>> @@ -1269,7 +1269,8 @@ static int imx_startup(struct uart_port *port) >>> writel(temp & ~UCR4_DREN, sport->port.membase + UCR4); >>> >>> /* Can we enable the DMA support? */ >>> - if (!uart_console(port) && !sport->dma_is_inited) >>> + if ((is_imx6q_uart(sport) || is_imx53_uart(sport)) && >>> + !uart_console(port) && !sport->dma_is_inited) >>> imx_uart_dma_init(sport); >> >> Looks like imx serial DMA is broken in 4.9. > > There are probably two separate problems: > > A) > I also observed the DMA transaction error message when I used the ROM > SDMA scripts. Seems to affect single- and multi-core systems and is > fixed by: github.com/Freescale/linux-fslc/commit/a2ec8df37f5d (?) > This is also not RS-485 specific, but affects RS-232 too. OK, thanks for the hint, but this is unrelated, since DMA should not be enabled on MX6SX in 4.9 (but it is , due to the error in 1c06bde643d0 ) and newer. If you want to enable/fix DMA on MX6SX , that should be a separate patch which is a feature addition IMO. > B) > The RS-485 DMA problem I reported, which only affects multi-core > systems. > RS-232 DMA is not affected. This has nothing to do with RS485/232 , the problem this patch fixes is that the DMA is errornously enabled on MX6SX and it should not be . [...]
Hi Marek,
On Mon, Feb 6, 2017 at 5:05 PM, Marek Vasut <marex@denx.de> wrote:
> Well yes, DMA is broken in 4.9 , that's why it's CC stable 4.9+ :)
This patch is based on the fact that the UART on the imx6sx is not of
a imx6q UART type, which is not correct.
Current imx6sx.dtsi misses to add a "fsl,imx6q-uart" compatible
string, just like all other imx6 SoC dtsi.
When we fix the compatible uart problem on imx6sx.dtsi, then DMA will
be enabled and things will break again.
That's why we need a better fix for this issue:
1) Let's see if Clemens' suggestion of using the latest SDMA firmware
can make DMA robust on your usecase.
2) If the system fails even with the latest SDMA, then the board dts
can disable dma by passing empty dma nodes.
Not all mx6 system seem to suffer from such SDMA problem. On a
imx6sx-udoo-neo we could use UART with DMA to connect to the Bluetooth
chip.
Thanks
On 02/06/2017 08:38 PM, Fabio Estevam wrote: > Hi Marek, Hi, > On Mon, Feb 6, 2017 at 5:05 PM, Marek Vasut <marex@denx.de> wrote: > >> Well yes, DMA is broken in 4.9 , that's why it's CC stable 4.9+ :) > > This patch is based on the fact that the UART on the imx6sx is not of > a imx6q UART type, which is not correct. > > Current imx6sx.dtsi misses to add a "fsl,imx6q-uart" compatible > string, just like all other imx6 SoC dtsi. Well given that the DMA does not work on 6SX, I guess it's not really compatible with 6Q. > When we fix the compatible uart problem on imx6sx.dtsi, then DMA will > be enabled and things will break again. > > That's why we need a better fix for this issue: > > 1) Let's see if Clemens' suggestion of using the latest SDMA firmware > can make DMA robust on your usecase. a) This will break if you don't use the latest SDMA firmware. This can be repaired by checking the SDMA firmware version and selectively disabling the DMA for broken versions. b) The DMA should not have been enabled on 4.9 , but it incorrectly is. Enabling it on 4.9 is a feature addition, which shouldn't go into stable kernel. > 2) If the system fails even with the latest SDMA, then the board dts > can disable dma by passing empty dma nodes. This might actually be the thing to do for 4.9 if you want to add the mx6q compatible to mx6sx uart . > Not all mx6 system seem to suffer from such SDMA problem. On a > imx6sx-udoo-neo we could use UART with DMA to connect to the Bluetooth > chip. Clearly some do, so it's probably also a matter of the link partner behavior.
On Mon, Feb 6, 2017 at 5:43 PM, Marek Vasut <marex@denx.de> wrote: > Well given that the DMA does not work on 6SX, I guess it's not really > compatible with 6Q. They have the same programming model, so yes, mx6sx.dtsi should list fsl,imx6q-uarti in the uart compatible. I will send a patch doing that. As Clemens explained, the DMA transaction error is not mx6sx specific.
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index a70356d..8e2b5f2 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -1269,7 +1269,8 @@ static int imx_startup(struct uart_port *port) writel(temp & ~UCR4_DREN, sport->port.membase + UCR4); /* Can we enable the DMA support? */ - if (!uart_console(port) && !sport->dma_is_inited) + if ((is_imx6q_uart(sport) || is_imx53_uart(sport)) && + !uart_console(port) && !sport->dma_is_inited) imx_uart_dma_init(sport); spin_lock_irqsave(&sport->port.lock, flags);