* [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
2017-07-05 13:07 [PATCH v2 0/6] serial: imx: various improvements Romain Perier
@ 2017-07-05 13:07 ` Romain Perier
2017-07-05 13:29 ` Uwe Kleine-König
2017-07-05 13:07 ` [PATCH v2 2/6] serial: imx: Simplify DMA disablement Romain Perier
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: linux-arm-kernel
From: Nandor Han <nandor.han@ge.com>
Avoid the situation when `dma_is_rxing` could incorrectly signal that
DMA RX channel is receiving data in case DMA preparation or sg mapping
fails.
This commit fixes the issues by moving the assignment of dma_is_rxing
out of imx_disable_rx_int(), then the variable is set to 1 from
start_rx_dma() only when the preparation is correctly done.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3bae4ea..01df430 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -729,8 +729,6 @@ static void imx_disable_rx_int(struct imx_port *sport)
{
unsigned long temp;
- sport->dma_is_rxing = 1;
-
/* disable the receiver ready and aging timer interrupts */
temp = readl(sport->port.membase + UCR1);
temp &= ~(UCR1_RRDYEN);
@@ -1083,6 +1081,7 @@ static int start_rx_dma(struct imx_port *sport)
desc->callback_param = sport;
dev_dbg(dev, "RX: prepare for the DMA.\n");
+ sport->dma_is_rxing = 1;
sport->rx_cookie = dmaengine_submit(desc);
dma_async_issue_pending(chan);
return 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
2017-07-05 13:07 ` [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts Romain Perier
@ 2017-07-05 13:29 ` Uwe Kleine-König
2017-07-06 8:31 ` Romain Perier
0 siblings, 1 reply; 14+ messages in thread
From: Uwe Kleine-König @ 2017-07-05 13:29 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On Wed, Jul 05, 2017 at 03:07:01PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
>
> Avoid the situation when `dma_is_rxing` could incorrectly signal that
> DMA RX channel is receiving data in case DMA preparation or sg mapping
> fails.
>
> This commit fixes the issues by moving the assignment of dma_is_rxing
> out of imx_disable_rx_int(), then the variable is set to 1 from
> start_rx_dma() only when the preparation is correctly done.
I'd write:
There are a few issues with setting dma_is_rxing to 1 in
imx_disable_rx_int:
- Currently always after imx_disable_rx_int() the function
start_rx_dma() is called. This dependency isn't obvious though.
- start_rx_dma() does error checking and might exit without enabling
DMA but keeping dma_is_rxing 1.
So the more natural place for setting dma_is_rxing to 1 is in
start_rx_dma after all errors are checked.
If you use this, there is nothing left of Nandor Han's patch and you can
drop his authorship.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
2017-07-05 13:29 ` Uwe Kleine-König
@ 2017-07-06 8:31 ` Romain Perier
2017-07-06 10:02 ` Uwe Kleine-König
0 siblings, 1 reply; 14+ messages in thread
From: Romain Perier @ 2017-07-06 8:31 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
Le 05/07/2017 ? 15:29, Uwe Kleine-K?nig a ?crit :
> Hello,
>
> On Wed, Jul 05, 2017 at 03:07:01PM +0200, Romain Perier wrote:
>> From: Nandor Han <nandor.han@ge.com>
>>
>> Avoid the situation when `dma_is_rxing` could incorrectly signal that
>> DMA RX channel is receiving data in case DMA preparation or sg mapping
>> fails.
>>
>> This commit fixes the issues by moving the assignment of dma_is_rxing
>> out of imx_disable_rx_int(), then the variable is set to 1 from
>> start_rx_dma() only when the preparation is correctly done.
> I'd write:
>
> There are a few issues with setting dma_is_rxing to 1 in
> imx_disable_rx_int:
>
> - Currently always after imx_disable_rx_int() the function
> start_rx_dma() is called. This dependency isn't obvious though.
> - start_rx_dma() does error checking and might exit without enabling
> DMA but keeping dma_is_rxing 1.
>
> So the more natural place for setting dma_is_rxing to 1 is in
> start_rx_dma after all errors are checked.
>
> If you use this, there is nothing left of Nandor Han's patch and you can
> drop his authorship.
>
> Best regards
> Uwe
>
Ok, will do. No other feedback for the rest of the series ? (just to
know if I send a v3 or If I wait a bit...)
Thanks,
Romain
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts
2017-07-06 8:31 ` Romain Perier
@ 2017-07-06 10:02 ` Uwe Kleine-König
0 siblings, 0 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2017-07-06 10:02 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jul 06, 2017 at 10:31:38AM +0200, Romain Perier wrote:
> Hello,
>
>
> Le 05/07/2017 ? 15:29, Uwe Kleine-K?nig a ?crit :
> > Hello,
> >
> > On Wed, Jul 05, 2017 at 03:07:01PM +0200, Romain Perier wrote:
> >> From: Nandor Han <nandor.han@ge.com>
> >>
> >> Avoid the situation when `dma_is_rxing` could incorrectly signal that
> >> DMA RX channel is receiving data in case DMA preparation or sg mapping
> >> fails.
> >>
> >> This commit fixes the issues by moving the assignment of dma_is_rxing
> >> out of imx_disable_rx_int(), then the variable is set to 1 from
> >> start_rx_dma() only when the preparation is correctly done.
> > I'd write:
> >
> > There are a few issues with setting dma_is_rxing to 1 in
> > imx_disable_rx_int:
> >
> > - Currently always after imx_disable_rx_int() the function
> > start_rx_dma() is called. This dependency isn't obvious though.
> > - start_rx_dma() does error checking and might exit without enabling
> > DMA but keeping dma_is_rxing 1.
> >
> > So the more natural place for setting dma_is_rxing to 1 is in
> > start_rx_dma after all errors are checked.
> >
> > If you use this, there is nothing left of Nandor Han's patch and you can
> > drop his authorship.
> >
> > Best regards
> > Uwe
> >
> Ok, will do. No other feedback for the rest of the series ? (just to
> know if I send a v3 or If I wait a bit...)
I didn't come around looking in the patches I didn't comment yet.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/6] serial: imx: Simplify DMA disablement
2017-07-05 13:07 [PATCH v2 0/6] serial: imx: various improvements Romain Perier
2017-07-05 13:07 ` [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts Romain Perier
@ 2017-07-05 13:07 ` Romain Perier
2017-07-07 1:45 ` kbuild test robot
2017-07-05 13:07 ` [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling Romain Perier
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: linux-arm-kernel
From: Nandor Han <nandor.han@ge.com>
This commits simplify the function imx_disable_dma() by moving
the code for disabling RX and TX DMAs to dedicated functions.
This is a preparation for the next commit.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 01df430..5291b86 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -360,6 +360,24 @@ static void imx_port_rts_auto(struct imx_port *sport, unsigned long *ucr2)
*ucr2 |= UCR2_CTSC;
}
+static void imx_stop_tx_dma(struct imx_port *sport)
+{
+ unsigned long temp;
+
+ temp = readl(sport->port.membase + UCR1);
+ temp &= ~UCR1_TDMAEN;
+ writel(temp, sport->port.membase + UCR1);
+}
+
+static void imx_stop_rx_dma(struct imx_port *sport)
+{
+ unsigned long temp;
+
+ temp = readl(sport->port.membase + UCR1);
+ temp &= ~(UCR1_RDMAEN | UCR1_ATDMAEN);
+ writel(temp, sport->port.membase + UCR1);
+}
+
/*
* interrupts disabled on entry
*/
@@ -1228,12 +1246,8 @@ static void imx_enable_dma(struct imx_port *sport)
static void imx_disable_dma(struct imx_port *sport)
{
- unsigned long temp;
-
- /* clear UCR1 */
- temp = readl(sport->port.membase + UCR1);
- temp &= ~(UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN);
- writel(temp, sport->port.membase + UCR1);
+ imx_stop_rx_dma(sport);
+ imx_stop_tx_dma(sport);
/* clear UCR2 */
temp = readl(sport->port.membase + UCR2);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 2/6] serial: imx: Simplify DMA disablement
2017-07-05 13:07 ` [PATCH v2 2/6] serial: imx: Simplify DMA disablement Romain Perier
@ 2017-07-07 1:45 ` kbuild test robot
0 siblings, 0 replies; 14+ messages in thread
From: kbuild test robot @ 2017-07-07 1:45 UTC (permalink / raw)
To: linux-arm-kernel
Hi Nandor,
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.12 next-20170706]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Romain-Perier/serial-imx-various-improvements/20170706-153226
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arm-multi_v5_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
Note: the linux-review/Romain-Perier/serial-imx-various-improvements/20170706-153226 HEAD c99e6e0fd195719d99b5ce9a4f9a155243772578 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/tty/serial/imx.c: In function 'imx_disable_dma':
>> drivers/tty/serial/imx.c:1250:2: error: 'temp' undeclared (first use in this function)
temp = readl(sport->port.membase + UCR2);
^~~~
drivers/tty/serial/imx.c:1250:2: note: each undeclared identifier is reported only once for each function it appears in
vim +/temp +1250 drivers/tty/serial/imx.c
b4cdc8f6 Huang Shijie 2013-07-08 1244 static void imx_disable_dma(struct imx_port *sport)
b4cdc8f6 Huang Shijie 2013-07-08 1245 {
52fe0ee0 Nandor Han 2017-07-05 1246 imx_stop_rx_dma(sport);
52fe0ee0 Nandor Han 2017-07-05 1247 imx_stop_tx_dma(sport);
b4cdc8f6 Huang Shijie 2013-07-08 1248
b4cdc8f6 Huang Shijie 2013-07-08 1249 /* clear UCR2 */
b4cdc8f6 Huang Shijie 2013-07-08 @1250 temp = readl(sport->port.membase + UCR2);
86a04ba6 Lucas Stach 2015-09-04 1251 temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
b4cdc8f6 Huang Shijie 2013-07-08 1252 writel(temp, sport->port.membase + UCR2);
b4cdc8f6 Huang Shijie 2013-07-08 1253
:::::: The code at line 1250 was first introduced by commit
:::::: b4cdc8f61beb2a55c8c3d22dfcaf5f34a919fe9b serial: imx: add DMA support for imx6q
:::::: TO: Huang Shijie <b32955@freescale.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 29150 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170707/1a9907e3/attachment-0001.gz>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling
2017-07-05 13:07 [PATCH v2 0/6] serial: imx: various improvements Romain Perier
2017-07-05 13:07 ` [PATCH v2 1/6] serial: imx: only set DMA rx-ing when DMA starts Romain Perier
2017-07-05 13:07 ` [PATCH v2 2/6] serial: imx: Simplify DMA disablement Romain Perier
@ 2017-07-05 13:07 ` Romain Perier
2017-07-05 13:38 ` Uwe Kleine-König
2017-07-05 13:07 ` [PATCH v2 4/6] serial: imx: unmap sg buffers when DMA channel is released Romain Perier
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: linux-arm-kernel
From: Nandor Han <nandor.han@ge.com>
CTSC and CTS are not related to DMA and might add
disruption in some cases.
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 5291b86..dd3ebb4 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
imx_stop_rx_dma(sport);
imx_stop_tx_dma(sport);
- /* clear UCR2 */
- temp = readl(sport->port.membase + UCR2);
- temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
- writel(temp, sport->port.membase + UCR2);
-
imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
sport->dma_is_enabled = 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling
2017-07-05 13:07 ` [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling Romain Perier
@ 2017-07-05 13:38 ` Uwe Kleine-König
2017-07-05 14:42 ` Clemens Gruber
2017-09-15 20:57 ` Martyn Welch
0 siblings, 2 replies; 14+ messages in thread
From: Uwe Kleine-König @ 2017-07-05 13:38 UTC (permalink / raw)
To: linux-arm-kernel
Cc += Clemens Gruber + Fabio Estevam
On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> From: Nandor Han <nandor.han@ge.com>
>
> CTSC and CTS are not related to DMA and might add
> disruption in some cases.
>
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
If it was Nandor Han who created this patch, it would be great to get
his sob. If it was you, drop the From: line above.
> ---
> drivers/tty/serial/imx.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 5291b86..dd3ebb4 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> imx_stop_rx_dma(sport);
> imx_stop_tx_dma(sport);
>
> - /* clear UCR2 */
> - temp = readl(sport->port.membase + UCR2);
> - temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> - writel(temp, sport->port.membase + UCR2);
> -
Before this patch imx_disable_dma resulted in the #CTS pin being high
(inactive).
Does this qualify as a fix? If so, you should sort this patch to the
beginning of the series. Did you do test this patch and its effects
separately?
@Clemens: maybe this patch makes a relevant difference when the port is
operated in rs485 mode. Do you care to test?
> imx_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>
> sport->dma_is_enabled = 0;
> --
> 1.8.3.1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling
2017-07-05 13:38 ` Uwe Kleine-König
@ 2017-07-05 14:42 ` Clemens Gruber
2017-09-15 20:57 ` Martyn Welch
1 sibling, 0 replies; 14+ messages in thread
From: Clemens Gruber @ 2017-07-05 14:42 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Wed, Jul 05, 2017 at 03:38:45PM +0200, Uwe Kleine-K?nig wrote:
> Cc += Clemens Gruber + Fabio Estevam
>
> On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> > From: Nandor Han <nandor.han@ge.com>
> >
> > CTSC and CTS are not related to DMA and might add
> > disruption in some cases.
> >
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
>
> If it was Nandor Han who created this patch, it would be great to get
> his sob. If it was you, drop the From: line above.
>
> > ---
> > drivers/tty/serial/imx.c | 5 -----
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 5291b86..dd3ebb4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> > imx_stop_rx_dma(sport);
> > imx_stop_tx_dma(sport);
> >
> > - /* clear UCR2 */
> > - temp = readl(sport->port.membase + UCR2);
> > - temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > - writel(temp, sport->port.membase + UCR2);
> > -
>
> Before this patch imx_disable_dma resulted in the #CTS pin being high
> (inactive).
>
> Does this qualify as a fix? If so, you should sort this patch to the
> beginning of the series. Did you do test this patch and its effects
> separately?
>
> @Clemens: maybe this patch makes a relevant difference when the port is
> operated in rs485 mode. Do you care to test?
I just finished testing it. The results are about the same as with v1 of
this patch series:
Applying v2 of patch 1/6, 2/6 and 3/6 (or even 3/6 alone) does not fix
the RS-485 DMA bug. It behaves exactly the same as without these
patches, meaning the whole xmit circ_buf (UART_XMIT_SIZE bytes) is sent
out, as seen in the logic analyzer screenshots from my first bug report:
https://pqgruber.com/rs485_results.png
Applying the whole series does make a (small) difference though:
The first few transmissions after a fresh boot work correctly! However,
after a few transmissions, characters are sent out twice and longer
transmissions are garbled, although not in the same way as before. The
whole circ_buf is no longer sent out.
With all patches from this series applied, I see the following on the
logic analyzer, when calling "echo Test > /dev/ttymxc4":
https://pqgruber.com/rs485txtest.png
(This pattern is not always the same, sometimes it is TeTesstt\n\n,
sometimes TeTesst\nt\n or TeTsestt\n\n and so on)
This behavior is reproducible on i.MX6Q and i.MX6D, not on i.MX6S/etc.,
I therefore assume that it is a SMP-/locking-related problem.
I will try to debug this further, and verify if - with this series
applied - xmit->tail is still jumping over xmit->head. And when exactly
this is happening.
Help is much appreciated!
Best regards,
Clemens
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling
2017-07-05 13:38 ` Uwe Kleine-König
2017-07-05 14:42 ` Clemens Gruber
@ 2017-09-15 20:57 ` Martyn Welch
1 sibling, 0 replies; 14+ messages in thread
From: Martyn Welch @ 2017-09-15 20:57 UTC (permalink / raw)
To: linux-arm-kernel
Hi
On Wed, Jul 05, 2017 at 03:38:45PM +0200, Uwe Kleine-K?nig wrote:
> Cc += Clemens Gruber + Fabio Estevam
>
> On Wed, Jul 05, 2017 at 03:07:03PM +0200, Romain Perier wrote:
> > From: Nandor Han <nandor.han@ge.com>
> >
> > CTSC and CTS are not related to DMA and might add
> > disruption in some cases.
> >
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
>
> If it was Nandor Han who created this patch, it would be great to get
> his sob. If it was you, drop the From: line above.
>
This patch was broken out from a larger one written by Nandor, who is
happy for me to add his sob.
> > ---
> > drivers/tty/serial/imx.c | 5 -----
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 5291b86..dd3ebb4 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -1249,11 +1249,6 @@ static void imx_disable_dma(struct imx_port *sport)
> > imx_stop_rx_dma(sport);
> > imx_stop_tx_dma(sport);
> >
> > - /* clear UCR2 */
> > - temp = readl(sport->port.membase + UCR2);
> > - temp &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> > - writel(temp, sport->port.membase + UCR2);
> > -
>
> Before this patch imx_disable_dma resulted in the #CTS pin being high
> (inactive).
>
> Does this qualify as a fix? If so, you should sort this patch to the
> beginning of the series. Did you do test this patch and its effects
> separately?
>
I've been giving the RTS/CTS lines a bit of a kick with (and without) this
patch and I'm seeing what I'd expect to see on the CTS pin (the Wandboard
I'm using runs this in DCE mode even though it really should be in DTE
mode, hey ho). Using a little test app I've found/modified (listed below),
the CTS line can be (de)asserted whilst the device is open and the line
gets deasserted when the device closes. I have tested this port both when
acting as a console (and thus with DMA turned off) and when not used as a
console, with DMA enabled (confirmed with existing debug in driver).
This matches the behaviour of a FTDI based debug board that I've also
tried (in this case I looked at the RTS line as the device is running as a
DTE).
On my PC the same test app sets the RTS line (the serial port running as a
DTE, 8250_pnp driver) results in the CTS line being set appropriately as
well. It also stays in that state even after the serial device is closed,
this does seem right either but there you go.
With regards to the operation of the CTS/RTS line when twiddling it via
the ioctl, I think the behaviour of the IMX/FTDI is the expected one. Is
that the case?
Which I guess brings us to the lines above.
When running as an RS232 port (i.e. not rs485) the driver is using the
automatic CTSC control based on a rxFIFO watermark unless the state of the
CTS line is explictly set via an ioctl call. As such, unless I'm missing
something, the rxFIFO (and thus the automatic CTS control) is independent
of whether the DMA is running or not and thus this section looks wrong or
is at the very least in the wrong place.
Have I misunderstood something?
IIRC, the timing of DMA being enabled and disabled was changed reasonably
recently did that fix the #CTS issue possibly?
Martyn
----
Test app:
#include <stdio.h>
#include <stdlib.h>
#include <termios.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdbool.h>
static struct termios oldterminfo;
void closeserial(int fd)
{
tcsetattr(fd, TCSANOW, &oldterminfo);
if (close(fd) < 0)
perror("closeserial()");
}
int openserial(char *devicename)
{
int fd;
struct termios attr;
if ((fd = open(devicename, O_RDWR)) == -1) {
perror("openserial(): open()");
return 0;
}
if (tcgetattr(fd, &oldterminfo) == -1) {
perror("openserial(): tcgetattr()");
return 0;
}
attr = oldterminfo;
attr.c_cflag |= CRTSCTS | CLOCAL;
attr.c_oflag = 0;
if (tcflush(fd, TCIOFLUSH) == -1) {
perror("openserial(): tcflush()");
return 0;
}
if (tcsetattr(fd, TCSANOW, &attr) == -1) {
perror("initserial(): tcsetattr()");
return 0;
}
return fd;
}
int setRTS(int fd, int level)
{
int status;
if (ioctl(fd, TIOCMGET, &status) == -1) {
perror("setRTS(): TIOCMGET");
return 0;
}
if (level)
status |= TIOCM_RTS;
else
status &= ~TIOCM_RTS;
if (ioctl(fd, TIOCMSET, &status) == -1) {
perror("setRTS(): TIOCMSET");
return 0;
}
return 1;
}
int main(int argc, char *argv[])
{
int fd;
bool loop = true;
int state = 1;
int got;
fd = openserial(argv[1]);
if (!fd) {
fprintf(stderr, "Error while initializing %s.\n", argv[1]);
return 1;
}
while(loop) {
printf("Switching RTS %s\n", state ? "on" : "off");
setRTS(fd, state);
state = (++state) % 2;
got = getchar();
if (got == 'q')
break;
}
closeserial(fd);
return 0;
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/6] serial: imx: unmap sg buffers when DMA channel is released
2017-07-05 13:07 [PATCH v2 0/6] serial: imx: various improvements Romain Perier
` (2 preceding siblings ...)
2017-07-05 13:07 ` [PATCH v2 3/6] serial: imx: remove CTSC and CTS handling Romain Perier
@ 2017-07-05 13:07 ` Romain Perier
2017-07-05 13:07 ` [PATCH v2 5/6] serial: imx: update the stop rx,tx procedures Romain Perier
2017-07-05 13:07 ` [PATCH v2 6/6] serial: imx: Fix imx_shutdown procedure Romain Perier
5 siblings, 0 replies; 14+ messages in thread
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: linux-arm-kernel
From: Nandor Han <nandor.han@ge.com>
This commits unmaps sg buffers when the DMA channel is released. It also
sets to zero `dma_is_rxing` and `dma_is_txing` to state that the
corresponding channels cannot transmit/receive data, as these are
disabled.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dd3ebb4..3112d88 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -367,6 +367,12 @@ static void imx_stop_tx_dma(struct imx_port *sport)
temp = readl(sport->port.membase + UCR1);
temp &= ~UCR1_TDMAEN;
writel(temp, sport->port.membase + UCR1);
+
+ if (sport->dma_is_txing) {
+ dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
+ sport->dma_tx_nents, DMA_TO_DEVICE);
+ sport->dma_is_txing = 0;
+ }
}
static void imx_stop_rx_dma(struct imx_port *sport)
@@ -376,6 +382,12 @@ static void imx_stop_rx_dma(struct imx_port *sport)
temp = readl(sport->port.membase + UCR1);
temp &= ~(UCR1_RDMAEN | UCR1_ATDMAEN);
writel(temp, sport->port.membase + UCR1);
+
+ if (sport->dma_is_rxing) {
+ dma_unmap_sg(sport->port.dev, &sport->rx_sgl, 1,
+ DMA_FROM_DEVICE);
+ sport->dma_is_rxing = 0;
+ }
}
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 5/6] serial: imx: update the stop rx,tx procedures
2017-07-05 13:07 [PATCH v2 0/6] serial: imx: various improvements Romain Perier
` (3 preceding siblings ...)
2017-07-05 13:07 ` [PATCH v2 4/6] serial: imx: unmap sg buffers when DMA channel is released Romain Perier
@ 2017-07-05 13:07 ` Romain Perier
2017-07-05 13:07 ` [PATCH v2 6/6] serial: imx: Fix imx_shutdown procedure Romain Perier
5 siblings, 0 replies; 14+ messages in thread
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: linux-arm-kernel
From: Nandor Han <nandor.han@ge.com>
According to "Documentation/serial/driver" both procedures should stop
receiving or sending data. Based on this the procedures should stop the
activity regardless if DMA is enabled or not.
This commit updates both imx_stop_{rx|tx} procedures to stop the
activity and disable the interrupts related to that.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 36 +++++++++++++++++-------------------
1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3112d88..d35112f 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -398,15 +398,14 @@ static void imx_stop_tx(struct uart_port *port)
struct imx_port *sport = (struct imx_port *)port;
unsigned long temp;
- /*
- * We are maybe in the SMP context, so if the DMA TX thread is running
- * on other cpu, we have to wait for it to finish.
- */
- if (sport->dma_is_enabled && sport->dma_is_txing)
- return;
+ sport->tx_bytes = 0;
+
+ if (sport->dma_is_enabled)
+ imx_stop_tx_dma(sport);
temp = readl(port->membase + UCR1);
- writel(temp & ~UCR1_TXMPTYEN, port->membase + UCR1);
+ temp &= ~(UCR1_TXMPTYEN | UCR1_TRDYEN | UCR1_RTSDEN);
+ writel(temp, port->membase + UCR1);
/* in rs485 mode disable transmitter if shifter is empty */
if (port->rs485.flags & SER_RS485_ENABLED &&
@@ -433,21 +432,20 @@ static void imx_stop_rx(struct uart_port *port)
struct imx_port *sport = (struct imx_port *)port;
unsigned long temp;
- if (sport->dma_is_enabled && sport->dma_is_rxing) {
- if (sport->port.suspended) {
- dmaengine_terminate_all(sport->dma_chan_rx);
- sport->dma_is_rxing = 0;
- } else {
- return;
- }
- }
+ if (sport->dma_is_enabled)
+ imx_stop_rx_dma(sport);
+
+ temp = readl(sport->port.membase + UCR1);
+ temp &= ~UCR1_RRDYEN;
+ writel(temp, sport->port.membase + UCR1);
temp = readl(sport->port.membase + UCR2);
- writel(temp & ~UCR2_RXEN, sport->port.membase + UCR2);
+ temp &= ~UCR2_ATEN;
+ writel(temp, sport->port.membase + UCR2);
- /* disable the `Receiver Ready Interrrupt` */
- temp = readl(sport->port.membase + UCR1);
- writel(temp & ~UCR1_RRDYEN, sport->port.membase + UCR1);
+ temp = readl(sport->port.membase + UCR3);
+ temp &= ~UCR3_AWAKEN;
+ writel(temp, sport->port.membase + UCR3);
}
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 6/6] serial: imx: Fix imx_shutdown procedure
2017-07-05 13:07 [PATCH v2 0/6] serial: imx: various improvements Romain Perier
` (4 preceding siblings ...)
2017-07-05 13:07 ` [PATCH v2 5/6] serial: imx: update the stop rx,tx procedures Romain Perier
@ 2017-07-05 13:07 ` Romain Perier
5 siblings, 0 replies; 14+ messages in thread
From: Romain Perier @ 2017-07-05 13:07 UTC (permalink / raw)
To: linux-arm-kernel
From: Nandor Han <nandor.han@ge.com>
In some cases, it looks that interrupts can happen after the dma was
disabled and port was not yet shutdown. This will result in interrupts
handled by imx_rxint.
This commits updates the shutdown function to ensure that underlying
components are disabled in the right order. This disables RX and TX
blocks, then it disabled interrupts. In case DMA is enabled, it disables
DMA and free corresponding resources. It disables UART port and stop
clocks.
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
drivers/tty/serial/imx.c | 34 +++++++++++++++-------------------
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d35112f..bfe7180 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1396,44 +1396,40 @@ static void imx_shutdown(struct uart_port *port)
unsigned long temp;
unsigned long flags;
- if (sport->dma_is_enabled) {
- sport->dma_is_rxing = 0;
- sport->dma_is_txing = 0;
- dmaengine_terminate_sync(sport->dma_chan_tx);
- dmaengine_terminate_sync(sport->dma_chan_rx);
-
+ if (!sport->port.suspended) {
spin_lock_irqsave(&sport->port.lock, flags);
imx_stop_tx(port);
imx_stop_rx(port);
- imx_disable_dma(sport);
+
+ if (sport->dma_is_inited && sport->dma_is_enabled)
+ imx_disable_dma(sport);
+
spin_unlock_irqrestore(&sport->port.lock, flags);
imx_uart_dma_exit(sport);
}
- mctrl_gpio_disable_ms(sport->gpios);
-
spin_lock_irqsave(&sport->port.lock, flags);
temp = readl(sport->port.membase + UCR2);
- temp &= ~(UCR2_TXEN);
+ temp &= ~(UCR2_TXEN | UCR2_RXEN);
writel(temp, sport->port.membase + UCR2);
+ temp = readl(sport->port.membase + UCR4);
+ temp &= ~UCR4_OREN;
+ writel(temp, sport->port.membase + UCR4);
spin_unlock_irqrestore(&sport->port.lock, flags);
- /*
- * Stop our timer.
- */
- del_timer_sync(&sport->timer);
+ mctrl_gpio_disable_ms(sport->gpios);
- /*
- * Disable all interrupts, port and break condition.
- */
+ /* Stop our timer. */
+ del_timer_sync(&sport->timer);
+ /* Disable port. */
spin_lock_irqsave(&sport->port.lock, flags);
temp = readl(sport->port.membase + UCR1);
- temp &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN);
-
+ temp &= ~UCR1_UARTEN;
writel(temp, sport->port.membase + UCR1);
spin_unlock_irqrestore(&sport->port.lock, flags);
+ /* Disable clocks. */
clk_disable_unprepare(sport->clk_per);
clk_disable_unprepare(sport->clk_ipg);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
