All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/4] serial: mxs-auart: gpios as modem signals
@ 2014-10-10 16:53 ` Janusz Uzycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Richard Genoud,
	Fabio Estevam, linux-serial, devicetree, linux-arm-kernel,
	Janusz Uzycki

It contains improved version of the old v3 patchset
"[PATCH v3] serial: mxs-auart: gpios as modem signals (dirty)"

v3 -> v4 changelog:
[PATCH 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
* renamed from "serial: mxs-auart: ctrl removed from mxs_auart_port"
* mxs_auart_get_mctrl() read back RTS line. It is removed too.

[PATCH 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling
 modem signals
* RTS_AT_AUART() and CTS_AT_AUART() macro defined
* DMA engine disabled if RTS or CTS is GPIO line
* warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
* CTSEN can't be enabled for hardware flow control block
  if CTS is defined as GPIO line
* RTSEN can be enabled for hardware flow control block
  even if RTS is defined as GPIO line.
  RTS pin depends on pinctrl configuration which
  selects RTS output from hardware flow control block or GPIO line.
* mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
* mxs_auart_irq_handle(): CTS_AT_AUART() used
* mxs_auart_init_gpios() returns true(success)/false(failure)
* dev_err() message fixed in mxs_auart_probe()

[PATCH 3/4] serial: mxs-auart: add interrupts for modem control lines
* rebased

[PATCH 4/4] serial: mxs-auart: enable PPS support
* coding style: braces in both branches of condition


v2 -> v3 changelog:
* own patches reordered to apply mainline
* patch "serial: mxs-auart: add sysrq support"
  removed from the patchset and resent as independent

new [PATCH 1/4] serial: mxs-auart: ctrl removed from mxs_auart_port
* the ctrl variable duplicated mctrl, member of uart_port structure
  in serial_core.h
* the code duplicated uart_update_mctrl() and uart_tiocmget()
  in serial_core.c
* mxs_auart_get_mctrl() reads back RTS line. It could be removed too
  but not sure.

[PATCH 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling
 modem signals
* mctrl_gpio_free() removed to simplify:
  mctrl_gpio_free() is not necessary in mxs_auart_probe() and
  mxs_auart_remove() because mctrl_gpio_init() does all
  allocations with devm_* functions.
  (see Documentation/serial/driver since kernel 3.16)
* DMA on HW flow control comment updated, still not sure about the comment
* mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
  mctrl_gpio_get() does not clear gpio interrupt pendings like
  8250_core.c does with MSR.
* mxs_auart_modem_status() moved to [3/4]
  If enable_ms() is not called, uart_handle_cts_change()
  shouldn't be called.

[PATCH 3/4] serial: mxs-auart: add interrupts for modem control lines
* introduces mctrl_prev instead of removed ctrl
* mxs_auart_modem_status() moved from [3/4]
* mxs_auart_modem_status() interrupt_enabled meant s->ms_irq_enabled

[PATCH 4/4] serial: mxs-auart: enable PPS support
* no changes


Janusz Uzycki (4):
  serial: mxs-auart: clean get_mctrl and set_mctrl
  serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  serial: mxs-auart: add interrupts for modem control lines
  serial: mxs-auart: enable PPS support

 .../devicetree/bindings/serial/fsl-mxs-auart.txt   |  10 +-
 drivers/tty/serial/Kconfig           |   1 +
 drivers/tty/serial/mxs-auart.c       | 237 +++++++++++++++++++--
 3 files changed, 233 insertions(+), 15 deletions(-)

-- 
1.7.11.3


^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 0/4] serial: mxs-auart: gpios as modem signals
@ 2014-10-10 16:53 ` Janusz Uzycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: linux-arm-kernel

It contains improved version of the old v3 patchset
"[PATCH v3] serial: mxs-auart: gpios as modem signals (dirty)"

v3 -> v4 changelog:
[PATCH 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
* renamed from "serial: mxs-auart: ctrl removed from mxs_auart_port"
* mxs_auart_get_mctrl() read back RTS line. It is removed too.

[PATCH 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling
 modem signals
* RTS_AT_AUART() and CTS_AT_AUART() macro defined
* DMA engine disabled if RTS or CTS is GPIO line
* warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
* CTSEN can't be enabled for hardware flow control block
  if CTS is defined as GPIO line
* RTSEN can be enabled for hardware flow control block
  even if RTS is defined as GPIO line.
  RTS pin depends on pinctrl configuration which
  selects RTS output from hardware flow control block or GPIO line.
* mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
* mxs_auart_irq_handle(): CTS_AT_AUART() used
* mxs_auart_init_gpios() returns true(success)/false(failure)
* dev_err() message fixed in mxs_auart_probe()

[PATCH 3/4] serial: mxs-auart: add interrupts for modem control lines
* rebased

[PATCH 4/4] serial: mxs-auart: enable PPS support
* coding style: braces in both branches of condition


v2 -> v3 changelog:
* own patches reordered to apply mainline
* patch "serial: mxs-auart: add sysrq support"
  removed from the patchset and resent as independent

new [PATCH 1/4] serial: mxs-auart: ctrl removed from mxs_auart_port
* the ctrl variable duplicated mctrl, member of uart_port structure
  in serial_core.h
* the code duplicated uart_update_mctrl() and uart_tiocmget()
  in serial_core.c
* mxs_auart_get_mctrl() reads back RTS line. It could be removed too
  but not sure.

[PATCH 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling
 modem signals
* mctrl_gpio_free() removed to simplify:
  mctrl_gpio_free() is not necessary in mxs_auart_probe() and
  mxs_auart_remove() because mctrl_gpio_init() does all
  allocations with devm_* functions.
  (see Documentation/serial/driver since kernel 3.16)
* DMA on HW flow control comment updated, still not sure about the comment
* mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
  mctrl_gpio_get() does not clear gpio interrupt pendings like
  8250_core.c does with MSR.
* mxs_auart_modem_status() moved to [3/4]
  If enable_ms() is not called, uart_handle_cts_change()
  shouldn't be called.

[PATCH 3/4] serial: mxs-auart: add interrupts for modem control lines
* introduces mctrl_prev instead of removed ctrl
* mxs_auart_modem_status() moved from [3/4]
* mxs_auart_modem_status() interrupt_enabled meant s->ms_irq_enabled

[PATCH 4/4] serial: mxs-auart: enable PPS support
* no changes


Janusz Uzycki (4):
  serial: mxs-auart: clean get_mctrl and set_mctrl
  serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  serial: mxs-auart: add interrupts for modem control lines
  serial: mxs-auart: enable PPS support

 .../devicetree/bindings/serial/fsl-mxs-auart.txt   |  10 +-
 drivers/tty/serial/Kconfig           |   1 +
 drivers/tty/serial/mxs-auart.c       | 237 +++++++++++++++++++--
 3 files changed, 233 insertions(+), 15 deletions(-)

-- 
1.7.11.3

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
  2014-10-10 16:53 ` Janusz Uzycki
@ 2014-10-10 16:53   ` Janusz Uzycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Richard Genoud,
	Fabio Estevam, linux-serial, devicetree, linux-arm-kernel,
	Janusz Uzycki

Russell King:
The only thing which the .get_mctrl method is supposed to do is
to return the state of the /input/ lines, which are CTS, DCD, DSR, RI.
The output line state is stored in port->mctrl, and is added to
the returned value by serial_core when it's required.
RTS output state should not be returned from the .get_mctrl method.

This patch removes ctrl variable from mxs_auart_port
and removes useless reading back RTS line.

The ctrl variable in mxs_auart_port duplicated mctrl,
member of uart_port structure in serial_core.h.
The removed code from mxs_auart_set_mctrl() and mxs_auart_get_mctrl
duplicated uart_update_mctrl() and uart_tiocmget()
in serial_core.c.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
[PATCH 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
* renamed from "serial: mxs-auart: ctrl removed from mxs_auart_port"
* mxs_auart_get_mctrl() read back RTS line. It is removed too.

v2 -> v3 changelog:
The patch was introduced:
new [PATCH 1/4] serial: mxs-auart: ctrl removed from mxs_auart_port
* the ctrl variable duplicated mctrl, member of uart_port structure
  in serial_core.h
* the code duplicated uart_update_mctrl() and uart_tiocmget()
  in serial_core.c
* mxs_auart_get_mctrl() reads back RTS line. It could be removed too
  but not sure.

---
 drivers/tty/serial/mxs-auart.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index e838e84..2d49901 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -143,7 +143,6 @@ struct mxs_auart_port {
 #define MXS_AUART_DMA_RX_READY	3  /* bit 3 */
 #define MXS_AUART_RTSCTS	4  /* bit 4 */
 	unsigned long flags;
-	unsigned int ctrl;
 	enum mxs_auart_type devtype;
 
 	unsigned int irq;
@@ -406,8 +405,6 @@ static void mxs_auart_release_port(struct uart_port *u)
 
 static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 {
-	struct mxs_auart_port *s = to_auart_port(u);
-
 	u32 ctrl = readl(u->membase + AUART_CTRL2);
 
 	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
@@ -418,24 +415,17 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 			ctrl |= AUART_CTRL2_RTS;
 	}
 
-	s->ctrl = mctrl;
 	writel(ctrl, u->membase + AUART_CTRL2);
 }
 
 static u32 mxs_auart_get_mctrl(struct uart_port *u)
 {
-	struct mxs_auart_port *s = to_auart_port(u);
 	u32 stat = readl(u->membase + AUART_STAT);
-	int ctrl2 = readl(u->membase + AUART_CTRL2);
-	u32 mctrl = s->ctrl;
+	u32 mctrl = 0;
 
-	mctrl &= ~TIOCM_CTS;
 	if (stat & AUART_STAT_CTS)
 		mctrl |= TIOCM_CTS;
 
-	if (ctrl2 & AUART_CTRL2_RTS)
-		mctrl |= TIOCM_RTS;
-
 	return mctrl;
 }
 
@@ -1071,8 +1061,6 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	s->port.type = PORT_IMX;
 	s->port.dev = s->dev = &pdev->dev;
 
-	s->ctrl = 0;
-
 	s->irq = platform_get_irq(pdev, 0);
 	s->port.irq = s->irq;
 	ret = request_irq(s->irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s);
-- 
1.7.11.3


^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH v4 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
@ 2014-10-10 16:53   ` Janusz Uzycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: linux-arm-kernel

Russell King:
The only thing which the .get_mctrl method is supposed to do is
to return the state of the /input/ lines, which are CTS, DCD, DSR, RI.
The output line state is stored in port->mctrl, and is added to
the returned value by serial_core when it's required.
RTS output state should not be returned from the .get_mctrl method.

This patch removes ctrl variable from mxs_auart_port
and removes useless reading back RTS line.

The ctrl variable in mxs_auart_port duplicated mctrl,
member of uart_port structure in serial_core.h.
The removed code from mxs_auart_set_mctrl() and mxs_auart_get_mctrl
duplicated uart_update_mctrl() and uart_tiocmget()
in serial_core.c.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
[PATCH 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
* renamed from "serial: mxs-auart: ctrl removed from mxs_auart_port"
* mxs_auart_get_mctrl() read back RTS line. It is removed too.

v2 -> v3 changelog:
The patch was introduced:
new [PATCH 1/4] serial: mxs-auart: ctrl removed from mxs_auart_port
* the ctrl variable duplicated mctrl, member of uart_port structure
  in serial_core.h
* the code duplicated uart_update_mctrl() and uart_tiocmget()
  in serial_core.c
* mxs_auart_get_mctrl() reads back RTS line. It could be removed too
  but not sure.

---
 drivers/tty/serial/mxs-auart.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index e838e84..2d49901 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -143,7 +143,6 @@ struct mxs_auart_port {
 #define MXS_AUART_DMA_RX_READY	3  /* bit 3 */
 #define MXS_AUART_RTSCTS	4  /* bit 4 */
 	unsigned long flags;
-	unsigned int ctrl;
 	enum mxs_auart_type devtype;
 
 	unsigned int irq;
@@ -406,8 +405,6 @@ static void mxs_auart_release_port(struct uart_port *u)
 
 static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 {
-	struct mxs_auart_port *s = to_auart_port(u);
-
 	u32 ctrl = readl(u->membase + AUART_CTRL2);
 
 	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
@@ -418,24 +415,17 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 			ctrl |= AUART_CTRL2_RTS;
 	}
 
-	s->ctrl = mctrl;
 	writel(ctrl, u->membase + AUART_CTRL2);
 }
 
 static u32 mxs_auart_get_mctrl(struct uart_port *u)
 {
-	struct mxs_auart_port *s = to_auart_port(u);
 	u32 stat = readl(u->membase + AUART_STAT);
-	int ctrl2 = readl(u->membase + AUART_CTRL2);
-	u32 mctrl = s->ctrl;
+	u32 mctrl = 0;
 
-	mctrl &= ~TIOCM_CTS;
 	if (stat & AUART_STAT_CTS)
 		mctrl |= TIOCM_CTS;
 
-	if (ctrl2 & AUART_CTRL2_RTS)
-		mctrl |= TIOCM_RTS;
-
 	return mctrl;
 }
 
@@ -1071,8 +1061,6 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	s->port.type = PORT_IMX;
 	s->port.dev = s->dev = &pdev->dev;
 
-	s->ctrl = 0;
-
 	s->irq = platform_get_irq(pdev, 0);
 	s->port.irq = s->irq;
 	ret = request_irq(s->irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s);
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-10-10 16:53 ` Janusz Uzycki
@ 2014-10-10 16:53   ` Janusz Uzycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Richard Genoud,
	Fabio Estevam, linux-serial, devicetree, linux-arm-kernel,
	Janusz Uzycki

Dedicated CTS and RTS pins are unusable together with a lot of other
peripherals because they share the same line. Pinctrl is limited.

Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
so we have to control them via GPIO.

This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
signals.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
* RTS_AT_AUART() and CTS_AT_AUART() macro defined
* DMA engine disabled if RTS or CTS is GPIO line
* warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
* CTSEN can't be enabled for hardware flow control block
  if CTS is defined as GPIO line
* RTSEN can be enabled for hardware flow control block
  even if RTS is defined as GPIO line.
  RTS pin depends on pinctrl configuration which
  selects RTS output from hardware flow control block or GPIO line.
* mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
* mxs_auart_irq_handle(): CTS_AT_AUART() used
* mxs_auart_init_gpios() returns true(success)/false(failure)
* dev_err() message fixed in mxs_auart_probe()

v2 -> v3 changelog:
* mctrl_gpio_free() removed to simplify:
  mctrl_gpio_free() is not necessary in mxs_auart_probe() and
  mxs_auart_remove() because mctrl_gpio_init() does all
  allocations with devm_* functions.
  (see Documentation/serial/driver since kernel 3.16)
* DMA on HW flow control comment updated, still not sure about the comment
* mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
  mctrl_gpio_get() does not clear gpio interrupt pendings like
  8250_core.c does with MSR.
* mxs_auart_modem_status() moved to [3/4]
  If enable_ms() is not called, uart_handle_cts_change()
  shouldn't be called.

---
 .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
 drivers/tty/serial/Kconfig           |  1 +
 drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
index 59a40f1..7c408c8 100644
--- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
+++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
@@ -11,8 +11,13 @@ Required properties:
 - dma-names: "rx" for RX channel, "tx" for TX channel.
 
 Optional properties:
-- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
+- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
+  for hardware flow control,
 	it also means you enable the DMA support for this UART.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified PIO instead of the peripheral
+  function pin for the USART feature.
+  If unsure, don't specify this property.
 
 Example:
 auart0: serial@8006a000 {
@@ -21,6 +26,9 @@ auart0: serial@8006a000 {
 	interrupts = <112>;
 	dmas = <&dma_apbx 8>, <&dma_apbx 9>;
 	dma-names = "rx", "tx";
+	cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
+	dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
+	dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
 };
 
 Note: Each auart port should have an alias correctly numbered in "aliases"
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 4fe8ca1..90e8516 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
 	depends on ARCH_MXS
 	tristate "MXS AUART support"
 	select SERIAL_CORE
+	select SERIAL_MCTRL_GPIO if GPIOLIB
 	help
 	  This driver supports the MXS Application UART (AUART) port.
 
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 2d49901..8bbcfd1 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -42,6 +42,9 @@
 
 #include <asm/cacheflush.h>
 
+#include <linux/err.h>
+#include "serial_mctrl_gpio.h"
+
 #define MXS_AUART_PORTS 5
 #define MXS_AUART_FIFO_SIZE		16
 
@@ -158,6 +161,8 @@ struct mxs_auart_port {
 	struct scatterlist rx_sgl;
 	struct dma_chan	*rx_dma_chan;
 	void *rx_dma_buf;
+
+	struct mctrl_gpios	*gpios;
 };
 
 static struct platform_device_id mxs_auart_devtype[] = {
@@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port *u)
 
 static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 {
+	struct mxs_auart_port *s = to_auart_port(u);
+
 	u32 ctrl = readl(u->membase + AUART_CTRL2);
 
 	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
@@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 	}
 
 	writel(ctrl, u->membase + AUART_CTRL2);
+
+	mctrl_gpio_set(s->gpios, mctrl);
 }
 
 static u32 mxs_auart_get_mctrl(struct uart_port *u)
 {
+	struct mxs_auart_port *s = to_auart_port(u);
 	u32 stat = readl(u->membase + AUART_STAT);
 	u32 mctrl = 0;
 
 	if (stat & AUART_STAT_CTS)
 		mctrl |= TIOCM_CTS;
 
-	return mctrl;
+	return mctrl_gpio_get(s->gpios, &mctrl);
 }
 
 static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
@@ -554,6 +564,10 @@ err_out:
 
 }
 
+#define RTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
+							UART_GPIO_RTS))
+#define CTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
+							UART_GPIO_CTS))
 static void mxs_auart_settermios(struct uart_port *u,
 				 struct ktermios *termios,
 				 struct ktermios *old)
@@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
 		ctrl |= AUART_LINECTRL_STP2;
 
 	/* figure out the hardware flow control settings */
+	ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
 	if (cflag & CRTSCTS) {
 		/*
 		 * The DMA has a bug(see errata:2836) in mx23.
@@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port *u,
 				ctrl2 |= AUART_CTRL2_TXDMAE | AUART_CTRL2_RXDMAE
 				       | AUART_CTRL2_DMAONERR;
 		}
-		ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
-	} else {
-		ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
+		/* Even if RTS is GPIO line RTSEN can be enabled because
+		 * the pinctrl configuration decides about RTS pin function */
+		ctrl2 |= AUART_CTRL2_RTSEN;
+		if (CTS_AT_AUART())
+			ctrl2 |= AUART_CTRL2_CTSEN;
 	}
 
 	/* set baud rate */
@@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 			s->port.membase + AUART_INTR_CLR);
 
 	if (istat & AUART_INTR_CTSMIS) {
-		uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
+		if (CTS_AT_AUART())
+			uart_handle_cts_change(&s->port,
+					stat & AUART_STAT_CTS);
 		writel(AUART_INTR_CTSMIS,
 				s->port.membase + AUART_INTR_CLR);
 		istat &= ~AUART_INTR_CTSMIS;
@@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
 	return 0;
 }
 
+static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
+{
+	s->gpios = mctrl_gpio_init(dev, 0);
+	if (IS_ERR_OR_NULL(s->gpios))
+		return false;
+
+	/* Block (enabled before) DMA option if RTS or CTS is GPIO line */
+	if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
+		if (test_bit(MXS_AUART_RTSCTS, &s->flags))
+			dev_warn(dev,
+				 "DMA and flow control via gpio may cause some problems. DMA disabled!\n");
+		clear_bit(MXS_AUART_RTSCTS, &s->flags);
+	}
+
+	return true;
+}
+
 static int mxs_auart_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id =
@@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, s);
 
+	if (!mxs_auart_init_gpios(s, &pdev->dev))
+		dev_err(&pdev->dev,
+			"Failed to initialize GPIOs. The serial port may not work as expected\n");
+
 	auart_port[s->port.line] = s;
 
 	mxs_auart_reset(&s->port);
-- 
1.7.11.3


^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-10-10 16:53   ` Janusz Uzycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: linux-arm-kernel

Dedicated CTS and RTS pins are unusable together with a lot of other
peripherals because they share the same line. Pinctrl is limited.

Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
so we have to control them via GPIO.

This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
signals.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
* RTS_AT_AUART() and CTS_AT_AUART() macro defined
* DMA engine disabled if RTS or CTS is GPIO line
* warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
* CTSEN can't be enabled for hardware flow control block
  if CTS is defined as GPIO line
* RTSEN can be enabled for hardware flow control block
  even if RTS is defined as GPIO line.
  RTS pin depends on pinctrl configuration which
  selects RTS output from hardware flow control block or GPIO line.
* mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
* mxs_auart_irq_handle(): CTS_AT_AUART() used
* mxs_auart_init_gpios() returns true(success)/false(failure)
* dev_err() message fixed in mxs_auart_probe()

v2 -> v3 changelog:
* mctrl_gpio_free() removed to simplify:
  mctrl_gpio_free() is not necessary in mxs_auart_probe() and
  mxs_auart_remove() because mctrl_gpio_init() does all
  allocations with devm_* functions.
  (see Documentation/serial/driver since kernel 3.16)
* DMA on HW flow control comment updated, still not sure about the comment
* mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
  mctrl_gpio_get() does not clear gpio interrupt pendings like
  8250_core.c does with MSR.
* mxs_auart_modem_status() moved to [3/4]
  If enable_ms() is not called, uart_handle_cts_change()
  shouldn't be called.

---
 .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
 drivers/tty/serial/Kconfig           |  1 +
 drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
index 59a40f1..7c408c8 100644
--- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
+++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
@@ -11,8 +11,13 @@ Required properties:
 - dma-names: "rx" for RX channel, "tx" for TX channel.
 
 Optional properties:
-- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
+- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
+  for hardware flow control,
 	it also means you enable the DMA support for this UART.
+- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
+  line respectively. It will use specified PIO instead of the peripheral
+  function pin for the USART feature.
+  If unsure, don't specify this property.
 
 Example:
 auart0: serial at 8006a000 {
@@ -21,6 +26,9 @@ auart0: serial at 8006a000 {
 	interrupts = <112>;
 	dmas = <&dma_apbx 8>, <&dma_apbx 9>;
 	dma-names = "rx", "tx";
+	cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
+	dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
+	dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
 };
 
 Note: Each auart port should have an alias correctly numbered in "aliases"
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 4fe8ca1..90e8516 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
 	depends on ARCH_MXS
 	tristate "MXS AUART support"
 	select SERIAL_CORE
+	select SERIAL_MCTRL_GPIO if GPIOLIB
 	help
 	  This driver supports the MXS Application UART (AUART) port.
 
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 2d49901..8bbcfd1 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -42,6 +42,9 @@
 
 #include <asm/cacheflush.h>
 
+#include <linux/err.h>
+#include "serial_mctrl_gpio.h"
+
 #define MXS_AUART_PORTS 5
 #define MXS_AUART_FIFO_SIZE		16
 
@@ -158,6 +161,8 @@ struct mxs_auart_port {
 	struct scatterlist rx_sgl;
 	struct dma_chan	*rx_dma_chan;
 	void *rx_dma_buf;
+
+	struct mctrl_gpios	*gpios;
 };
 
 static struct platform_device_id mxs_auart_devtype[] = {
@@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port *u)
 
 static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 {
+	struct mxs_auart_port *s = to_auart_port(u);
+
 	u32 ctrl = readl(u->membase + AUART_CTRL2);
 
 	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
@@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 	}
 
 	writel(ctrl, u->membase + AUART_CTRL2);
+
+	mctrl_gpio_set(s->gpios, mctrl);
 }
 
 static u32 mxs_auart_get_mctrl(struct uart_port *u)
 {
+	struct mxs_auart_port *s = to_auart_port(u);
 	u32 stat = readl(u->membase + AUART_STAT);
 	u32 mctrl = 0;
 
 	if (stat & AUART_STAT_CTS)
 		mctrl |= TIOCM_CTS;
 
-	return mctrl;
+	return mctrl_gpio_get(s->gpios, &mctrl);
 }
 
 static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
@@ -554,6 +564,10 @@ err_out:
 
 }
 
+#define RTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
+							UART_GPIO_RTS))
+#define CTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
+							UART_GPIO_CTS))
 static void mxs_auart_settermios(struct uart_port *u,
 				 struct ktermios *termios,
 				 struct ktermios *old)
@@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
 		ctrl |= AUART_LINECTRL_STP2;
 
 	/* figure out the hardware flow control settings */
+	ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
 	if (cflag & CRTSCTS) {
 		/*
 		 * The DMA has a bug(see errata:2836) in mx23.
@@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port *u,
 				ctrl2 |= AUART_CTRL2_TXDMAE | AUART_CTRL2_RXDMAE
 				       | AUART_CTRL2_DMAONERR;
 		}
-		ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
-	} else {
-		ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
+		/* Even if RTS is GPIO line RTSEN can be enabled because
+		 * the pinctrl configuration decides about RTS pin function */
+		ctrl2 |= AUART_CTRL2_RTSEN;
+		if (CTS_AT_AUART())
+			ctrl2 |= AUART_CTRL2_CTSEN;
 	}
 
 	/* set baud rate */
@@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 			s->port.membase + AUART_INTR_CLR);
 
 	if (istat & AUART_INTR_CTSMIS) {
-		uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
+		if (CTS_AT_AUART())
+			uart_handle_cts_change(&s->port,
+					stat & AUART_STAT_CTS);
 		writel(AUART_INTR_CTSMIS,
 				s->port.membase + AUART_INTR_CLR);
 		istat &= ~AUART_INTR_CTSMIS;
@@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
 	return 0;
 }
 
+static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
+{
+	s->gpios = mctrl_gpio_init(dev, 0);
+	if (IS_ERR_OR_NULL(s->gpios))
+		return false;
+
+	/* Block (enabled before) DMA option if RTS or CTS is GPIO line */
+	if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
+		if (test_bit(MXS_AUART_RTSCTS, &s->flags))
+			dev_warn(dev,
+				 "DMA and flow control via gpio may cause some problems. DMA disabled!\n");
+		clear_bit(MXS_AUART_RTSCTS, &s->flags);
+	}
+
+	return true;
+}
+
 static int mxs_auart_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id =
@@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, s);
 
+	if (!mxs_auart_init_gpios(s, &pdev->dev))
+		dev_err(&pdev->dev,
+			"Failed to initialize GPIOs. The serial port may not work as expected\n");
+
 	auart_port[s->port.line] = s;
 
 	mxs_auart_reset(&s->port);
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH v4 3/4] serial: mxs-auart: add interrupts for modem control lines
  2014-10-10 16:53 ` Janusz Uzycki
@ 2014-10-10 16:53   ` Janusz Uzycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Richard Genoud,
	Fabio Estevam, linux-serial, devicetree, linux-arm-kernel,
	Janusz Uzycki

Handle CTS/DSR/RI/DCD GPIO interrupts in mxs-auart.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
* rebased

v2 -> v3 changelog:
* introduces mctrl_prev instead of removed ctrl
* mxs_auart_modem_status() moved from [3/4]
* mxs_auart_modem_status() interrupt_enabled meant s->ms_irq_enabled

---
 drivers/tty/serial/mxs-auart.c | 174 ++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 8bbcfd1..5ce4a50 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -42,7 +42,10 @@
 
 #include <asm/cacheflush.h>
 
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/err.h>
+#include <linux/irq.h>
 #include "serial_mctrl_gpio.h"
 
 #define MXS_AUART_PORTS 5
@@ -146,6 +149,7 @@ struct mxs_auart_port {
 #define MXS_AUART_DMA_RX_READY	3  /* bit 3 */
 #define MXS_AUART_RTSCTS	4  /* bit 4 */
 	unsigned long flags;
+	unsigned int mctrl_prev;
 	enum mxs_auart_type devtype;
 
 	unsigned int irq;
@@ -163,6 +167,8 @@ struct mxs_auart_port {
 	void *rx_dma_buf;
 
 	struct mctrl_gpios	*gpios;
+	int			gpio_irq[UART_GPIO_MAX];
+	bool			ms_irq_enabled;
 };
 
 static struct platform_device_id mxs_auart_devtype[] = {
@@ -427,6 +433,29 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 	mctrl_gpio_set(s->gpios, mctrl);
 }
 
+#define MCTRL_ANY_DELTA        (TIOCM_RI | TIOCM_DSR | TIOCM_CD | TIOCM_CTS)
+static u32 mxs_auart_modem_status(struct mxs_auart_port *s, u32 mctrl)
+{
+	u32 mctrl_diff;
+
+	mctrl_diff = mctrl ^ s->mctrl_prev;
+	s->mctrl_prev = mctrl;
+	if (mctrl_diff & MCTRL_ANY_DELTA && s->ms_irq_enabled &&
+						s->port.state != NULL) {
+		if (mctrl_diff & TIOCM_RI)
+			s->port.icount.rng++;
+		if (mctrl_diff & TIOCM_DSR)
+			s->port.icount.dsr++;
+		if (mctrl_diff & TIOCM_CD)
+			uart_handle_dcd_change(&s->port, mctrl & TIOCM_CD);
+		if (mctrl_diff & TIOCM_CTS)
+			uart_handle_cts_change(&s->port, mctrl & TIOCM_CTS);
+
+		wake_up_interruptible(&s->port.state->port.delta_msr_wait);
+	}
+	return mctrl;
+}
+
 static u32 mxs_auart_get_mctrl(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
@@ -439,6 +468,64 @@ static u32 mxs_auart_get_mctrl(struct uart_port *u)
 	return mctrl_gpio_get(s->gpios, &mctrl);
 }
 
+/*
+ * Enable modem status interrupts
+ */
+static void mxs_auart_enable_ms(struct uart_port *port)
+{
+	struct mxs_auart_port *s = to_auart_port(port);
+
+	/*
+	 * Interrupt should not be enabled twice
+	 */
+	if (s->ms_irq_enabled)
+		return;
+
+	s->ms_irq_enabled = true;
+
+	if (s->gpio_irq[UART_GPIO_CTS] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_CTS]);
+	/* TODO: enable AUART_INTR_CTSMIEN otherwise */
+
+	if (s->gpio_irq[UART_GPIO_DSR] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_DSR]);
+
+	if (s->gpio_irq[UART_GPIO_RI] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_RI]);
+
+	if (s->gpio_irq[UART_GPIO_DCD] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_DCD]);
+}
+
+/*
+ * Disable modem status interrupts
+ */
+static void mxs_auart_disable_ms(struct uart_port *port)
+{
+	struct mxs_auart_port *s = to_auart_port(port);
+
+	/*
+	 * Interrupt should not be disabled twice
+	 */
+	if (!s->ms_irq_enabled)
+		return;
+
+	s->ms_irq_enabled = false;
+
+	if (s->gpio_irq[UART_GPIO_CTS] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_CTS]);
+	/* TODO: disable AUART_INTR_CTSMIEN otherwise */
+
+	if (s->gpio_irq[UART_GPIO_DSR] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_DSR]);
+
+	if (s->gpio_irq[UART_GPIO_RI] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_RI]);
+
+	if (s->gpio_irq[UART_GPIO_DCD] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_DCD]);
+}
+
 static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
 static void dma_rx_callback(void *arg)
 {
@@ -689,6 +776,12 @@ static void mxs_auart_settermios(struct uart_port *u,
 			dev_err(s->dev, "We can not start up the DMA.\n");
 		}
 	}
+
+	/* CTS flow-control and modem-status interrupts */
+	if (UART_ENABLE_MS(u, termios->c_cflag))
+		mxs_auart_enable_ms(u);
+	else
+		mxs_auart_disable_ms(u);
 }
 
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
@@ -706,8 +799,18 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 		| AUART_INTR_CTSMIS),
 			s->port.membase + AUART_INTR_CLR);
 
+	/*
+	 * Dealing with GPIO interrupt
+	 */
+	if (irq == s->gpio_irq[UART_GPIO_CTS] ||
+	    irq == s->gpio_irq[UART_GPIO_DCD] ||
+	    irq == s->gpio_irq[UART_GPIO_DSR] ||
+	    irq == s->gpio_irq[UART_GPIO_RI])
+		mxs_auart_modem_status(s,
+				mctrl_gpio_get(s->gpios, &s->mctrl_prev));
+
 	if (istat & AUART_INTR_CTSMIS) {
-		if (CTS_AT_AUART())
+		if (CTS_AT_AUART() && s->ms_irq_enabled)
 			uart_handle_cts_change(&s->port,
 					stat & AUART_STAT_CTS);
 		writel(AUART_INTR_CTSMIS,
@@ -770,6 +873,10 @@ static int mxs_auart_startup(struct uart_port *u)
 	 */
 	writel(AUART_LINECTRL_FEN, u->membase + AUART_LINECTRL_SET);
 
+	/* get initial status of modem lines */
+	mctrl_gpio_get(s->gpios, &s->mctrl_prev);
+
+	s->ms_irq_enabled = false;
 	return 0;
 }
 
@@ -777,6 +884,8 @@ static void mxs_auart_shutdown(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
 
+	mxs_auart_disable_ms(u);
+
 	if (auart_dma_enabled(s))
 		mxs_auart_dma_exit(s);
 
@@ -833,6 +942,7 @@ static struct uart_ops mxs_auart_ops = {
 	.start_tx       = mxs_auart_start_tx,
 	.stop_tx	= mxs_auart_stop_tx,
 	.stop_rx	= mxs_auart_stop_rx,
+	.enable_ms      = mxs_auart_enable_ms,
 	.break_ctl      = mxs_auart_break_ctl,
 	.set_mctrl	= mxs_auart_set_mctrl,
 	.get_mctrl      = mxs_auart_get_mctrl,
@@ -1035,6 +1145,9 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
 
 static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
 {
+	enum mctrl_gpio_idx i;
+	struct gpio_desc *gpiod;
+
 	s->gpios = mctrl_gpio_init(dev, 0);
 	if (IS_ERR_OR_NULL(s->gpios))
 		return false;
@@ -1047,9 +1160,54 @@ static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
 		clear_bit(MXS_AUART_RTSCTS, &s->flags);
 	}
 
+	for (i = 0; i < UART_GPIO_MAX; i++) {
+		gpiod = mctrl_gpio_to_gpiod(s->gpios, i);
+		if (gpiod && (gpiod_get_direction(gpiod) == GPIOF_DIR_IN))
+			s->gpio_irq[i] = gpiod_to_irq(gpiod);
+		else
+			s->gpio_irq[i] = -EINVAL;
+	}
+
 	return true;
 }
 
+static void mxs_auart_free_gpio_irq(struct mxs_auart_port *s)
+{
+	enum mctrl_gpio_idx i;
+
+	for (i = 0; i < UART_GPIO_MAX; i++)
+		if (s->gpio_irq[i] >= 0)
+			free_irq(s->gpio_irq[i], s);
+}
+
+static int mxs_auart_request_gpio_irq(struct mxs_auart_port *s)
+{
+	int *irq = s->gpio_irq;
+	enum mctrl_gpio_idx i;
+	int err = 0;
+
+	for (i = 0; (i < UART_GPIO_MAX) && !err; i++) {
+		if (irq[i] < 0)
+			continue;
+
+		irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
+		err = request_irq(irq[i], mxs_auart_irq_handle,
+				IRQ_TYPE_EDGE_BOTH, dev_name(s->dev), s);
+		if (err)
+			dev_err(s->dev, "%s - Can't get %d irq\n",
+				__func__, irq[i]);
+	}
+
+	/*
+	 * If something went wrong, rollback.
+	 */
+	while (err && (--i >= 0))
+		if (irq[i] >= 0)
+			free_irq(irq[i], s);
+
+	return err;
+}
+
 static int mxs_auart_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id =
@@ -1097,6 +1255,8 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	s->port.type = PORT_IMX;
 	s->port.dev = s->dev = &pdev->dev;
 
+	s->mctrl_prev = 0;
+
 	s->irq = platform_get_irq(pdev, 0);
 	s->port.irq = s->irq;
 	ret = request_irq(s->irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s);
@@ -1109,13 +1269,20 @@ static int mxs_auart_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Failed to initialize GPIOs. The serial port may not work as expected\n");
 
+	/*
+	 * Get the GPIO lines IRQ
+	 */
+	ret = mxs_auart_request_gpio_irq(s);
+	if (ret)
+		goto out_free_irq;
+
 	auart_port[s->port.line] = s;
 
 	mxs_auart_reset(&s->port);
 
 	ret = uart_add_one_port(&auart_driver, &s->port);
 	if (ret)
-		goto out_free_irq;
+		goto out_free_gpio_irq;
 
 	version = readl(s->port.membase + AUART_VERSION);
 	dev_info(&pdev->dev, "Found APPUART %d.%d.%d\n",
@@ -1124,6 +1291,8 @@ static int mxs_auart_probe(struct platform_device *pdev)
 
 	return 0;
 
+out_free_gpio_irq:
+	mxs_auart_free_gpio_irq(s);
 out_free_irq:
 	auart_port[pdev->id] = NULL;
 	free_irq(s->irq, s);
@@ -1143,6 +1312,7 @@ static int mxs_auart_remove(struct platform_device *pdev)
 
 	auart_port[pdev->id] = NULL;
 
+	mxs_auart_free_gpio_irq(s);
 	clk_put(s->clk);
 	free_irq(s->irq, s);
 	kfree(s);
-- 
1.7.11.3


^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH v4 3/4] serial: mxs-auart: add interrupts for modem control lines
@ 2014-10-10 16:53   ` Janusz Uzycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: linux-arm-kernel

Handle CTS/DSR/RI/DCD GPIO interrupts in mxs-auart.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
* rebased

v2 -> v3 changelog:
* introduces mctrl_prev instead of removed ctrl
* mxs_auart_modem_status() moved from [3/4]
* mxs_auart_modem_status() interrupt_enabled meant s->ms_irq_enabled

---
 drivers/tty/serial/mxs-auart.c | 174 ++++++++++++++++++++++++++-
 1 file changed, 172 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 8bbcfd1..5ce4a50 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -42,7 +42,10 @@
 
 #include <asm/cacheflush.h>
 
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/err.h>
+#include <linux/irq.h>
 #include "serial_mctrl_gpio.h"
 
 #define MXS_AUART_PORTS 5
@@ -146,6 +149,7 @@ struct mxs_auart_port {
 #define MXS_AUART_DMA_RX_READY	3  /* bit 3 */
 #define MXS_AUART_RTSCTS	4  /* bit 4 */
 	unsigned long flags;
+	unsigned int mctrl_prev;
 	enum mxs_auart_type devtype;
 
 	unsigned int irq;
@@ -163,6 +167,8 @@ struct mxs_auart_port {
 	void *rx_dma_buf;
 
 	struct mctrl_gpios	*gpios;
+	int			gpio_irq[UART_GPIO_MAX];
+	bool			ms_irq_enabled;
 };
 
 static struct platform_device_id mxs_auart_devtype[] = {
@@ -427,6 +433,29 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
 	mctrl_gpio_set(s->gpios, mctrl);
 }
 
+#define MCTRL_ANY_DELTA        (TIOCM_RI | TIOCM_DSR | TIOCM_CD | TIOCM_CTS)
+static u32 mxs_auart_modem_status(struct mxs_auart_port *s, u32 mctrl)
+{
+	u32 mctrl_diff;
+
+	mctrl_diff = mctrl ^ s->mctrl_prev;
+	s->mctrl_prev = mctrl;
+	if (mctrl_diff & MCTRL_ANY_DELTA && s->ms_irq_enabled &&
+						s->port.state != NULL) {
+		if (mctrl_diff & TIOCM_RI)
+			s->port.icount.rng++;
+		if (mctrl_diff & TIOCM_DSR)
+			s->port.icount.dsr++;
+		if (mctrl_diff & TIOCM_CD)
+			uart_handle_dcd_change(&s->port, mctrl & TIOCM_CD);
+		if (mctrl_diff & TIOCM_CTS)
+			uart_handle_cts_change(&s->port, mctrl & TIOCM_CTS);
+
+		wake_up_interruptible(&s->port.state->port.delta_msr_wait);
+	}
+	return mctrl;
+}
+
 static u32 mxs_auart_get_mctrl(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
@@ -439,6 +468,64 @@ static u32 mxs_auart_get_mctrl(struct uart_port *u)
 	return mctrl_gpio_get(s->gpios, &mctrl);
 }
 
+/*
+ * Enable modem status interrupts
+ */
+static void mxs_auart_enable_ms(struct uart_port *port)
+{
+	struct mxs_auart_port *s = to_auart_port(port);
+
+	/*
+	 * Interrupt should not be enabled twice
+	 */
+	if (s->ms_irq_enabled)
+		return;
+
+	s->ms_irq_enabled = true;
+
+	if (s->gpio_irq[UART_GPIO_CTS] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_CTS]);
+	/* TODO: enable AUART_INTR_CTSMIEN otherwise */
+
+	if (s->gpio_irq[UART_GPIO_DSR] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_DSR]);
+
+	if (s->gpio_irq[UART_GPIO_RI] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_RI]);
+
+	if (s->gpio_irq[UART_GPIO_DCD] >= 0)
+		enable_irq(s->gpio_irq[UART_GPIO_DCD]);
+}
+
+/*
+ * Disable modem status interrupts
+ */
+static void mxs_auart_disable_ms(struct uart_port *port)
+{
+	struct mxs_auart_port *s = to_auart_port(port);
+
+	/*
+	 * Interrupt should not be disabled twice
+	 */
+	if (!s->ms_irq_enabled)
+		return;
+
+	s->ms_irq_enabled = false;
+
+	if (s->gpio_irq[UART_GPIO_CTS] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_CTS]);
+	/* TODO: disable AUART_INTR_CTSMIEN otherwise */
+
+	if (s->gpio_irq[UART_GPIO_DSR] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_DSR]);
+
+	if (s->gpio_irq[UART_GPIO_RI] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_RI]);
+
+	if (s->gpio_irq[UART_GPIO_DCD] >= 0)
+		disable_irq(s->gpio_irq[UART_GPIO_DCD]);
+}
+
 static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
 static void dma_rx_callback(void *arg)
 {
@@ -689,6 +776,12 @@ static void mxs_auart_settermios(struct uart_port *u,
 			dev_err(s->dev, "We can not start up the DMA.\n");
 		}
 	}
+
+	/* CTS flow-control and modem-status interrupts */
+	if (UART_ENABLE_MS(u, termios->c_cflag))
+		mxs_auart_enable_ms(u);
+	else
+		mxs_auart_disable_ms(u);
 }
 
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
@@ -706,8 +799,18 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 		| AUART_INTR_CTSMIS),
 			s->port.membase + AUART_INTR_CLR);
 
+	/*
+	 * Dealing with GPIO interrupt
+	 */
+	if (irq == s->gpio_irq[UART_GPIO_CTS] ||
+	    irq == s->gpio_irq[UART_GPIO_DCD] ||
+	    irq == s->gpio_irq[UART_GPIO_DSR] ||
+	    irq == s->gpio_irq[UART_GPIO_RI])
+		mxs_auart_modem_status(s,
+				mctrl_gpio_get(s->gpios, &s->mctrl_prev));
+
 	if (istat & AUART_INTR_CTSMIS) {
-		if (CTS_AT_AUART())
+		if (CTS_AT_AUART() && s->ms_irq_enabled)
 			uart_handle_cts_change(&s->port,
 					stat & AUART_STAT_CTS);
 		writel(AUART_INTR_CTSMIS,
@@ -770,6 +873,10 @@ static int mxs_auart_startup(struct uart_port *u)
 	 */
 	writel(AUART_LINECTRL_FEN, u->membase + AUART_LINECTRL_SET);
 
+	/* get initial status of modem lines */
+	mctrl_gpio_get(s->gpios, &s->mctrl_prev);
+
+	s->ms_irq_enabled = false;
 	return 0;
 }
 
@@ -777,6 +884,8 @@ static void mxs_auart_shutdown(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
 
+	mxs_auart_disable_ms(u);
+
 	if (auart_dma_enabled(s))
 		mxs_auart_dma_exit(s);
 
@@ -833,6 +942,7 @@ static struct uart_ops mxs_auart_ops = {
 	.start_tx       = mxs_auart_start_tx,
 	.stop_tx	= mxs_auart_stop_tx,
 	.stop_rx	= mxs_auart_stop_rx,
+	.enable_ms      = mxs_auart_enable_ms,
 	.break_ctl      = mxs_auart_break_ctl,
 	.set_mctrl	= mxs_auart_set_mctrl,
 	.get_mctrl      = mxs_auart_get_mctrl,
@@ -1035,6 +1145,9 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
 
 static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
 {
+	enum mctrl_gpio_idx i;
+	struct gpio_desc *gpiod;
+
 	s->gpios = mctrl_gpio_init(dev, 0);
 	if (IS_ERR_OR_NULL(s->gpios))
 		return false;
@@ -1047,9 +1160,54 @@ static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
 		clear_bit(MXS_AUART_RTSCTS, &s->flags);
 	}
 
+	for (i = 0; i < UART_GPIO_MAX; i++) {
+		gpiod = mctrl_gpio_to_gpiod(s->gpios, i);
+		if (gpiod && (gpiod_get_direction(gpiod) == GPIOF_DIR_IN))
+			s->gpio_irq[i] = gpiod_to_irq(gpiod);
+		else
+			s->gpio_irq[i] = -EINVAL;
+	}
+
 	return true;
 }
 
+static void mxs_auart_free_gpio_irq(struct mxs_auart_port *s)
+{
+	enum mctrl_gpio_idx i;
+
+	for (i = 0; i < UART_GPIO_MAX; i++)
+		if (s->gpio_irq[i] >= 0)
+			free_irq(s->gpio_irq[i], s);
+}
+
+static int mxs_auart_request_gpio_irq(struct mxs_auart_port *s)
+{
+	int *irq = s->gpio_irq;
+	enum mctrl_gpio_idx i;
+	int err = 0;
+
+	for (i = 0; (i < UART_GPIO_MAX) && !err; i++) {
+		if (irq[i] < 0)
+			continue;
+
+		irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
+		err = request_irq(irq[i], mxs_auart_irq_handle,
+				IRQ_TYPE_EDGE_BOTH, dev_name(s->dev), s);
+		if (err)
+			dev_err(s->dev, "%s - Can't get %d irq\n",
+				__func__, irq[i]);
+	}
+
+	/*
+	 * If something went wrong, rollback.
+	 */
+	while (err && (--i >= 0))
+		if (irq[i] >= 0)
+			free_irq(irq[i], s);
+
+	return err;
+}
+
 static int mxs_auart_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id =
@@ -1097,6 +1255,8 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	s->port.type = PORT_IMX;
 	s->port.dev = s->dev = &pdev->dev;
 
+	s->mctrl_prev = 0;
+
 	s->irq = platform_get_irq(pdev, 0);
 	s->port.irq = s->irq;
 	ret = request_irq(s->irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s);
@@ -1109,13 +1269,20 @@ static int mxs_auart_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Failed to initialize GPIOs. The serial port may not work as expected\n");
 
+	/*
+	 * Get the GPIO lines IRQ
+	 */
+	ret = mxs_auart_request_gpio_irq(s);
+	if (ret)
+		goto out_free_irq;
+
 	auart_port[s->port.line] = s;
 
 	mxs_auart_reset(&s->port);
 
 	ret = uart_add_one_port(&auart_driver, &s->port);
 	if (ret)
-		goto out_free_irq;
+		goto out_free_gpio_irq;
 
 	version = readl(s->port.membase + AUART_VERSION);
 	dev_info(&pdev->dev, "Found APPUART %d.%d.%d\n",
@@ -1124,6 +1291,8 @@ static int mxs_auart_probe(struct platform_device *pdev)
 
 	return 0;
 
+out_free_gpio_irq:
+	mxs_auart_free_gpio_irq(s);
 out_free_irq:
 	auart_port[pdev->id] = NULL;
 	free_irq(s->irq, s);
@@ -1143,6 +1312,7 @@ static int mxs_auart_remove(struct platform_device *pdev)
 
 	auart_port[pdev->id] = NULL;
 
+	mxs_auart_free_gpio_irq(s);
 	clk_put(s->clk);
 	free_irq(s->irq, s);
 	kfree(s);
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH v4 4/4] serial: mxs-auart: enable PPS support
  2014-10-10 16:53 ` Janusz Uzycki
@ 2014-10-10 16:53   ` Janusz Uzycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Richard Genoud,
	Fabio Estevam, linux-serial, devicetree, linux-arm-kernel,
	Janusz Uzycki

Enables PPS support in mxs-auart serial driver to make PPS API working.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
* coding style: braces in both branches of condition

v2 -> v3 changelog:
* no changes

---
 drivers/tty/serial/mxs-auart.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 5ce4a50..840c1f7 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -784,6 +784,16 @@ static void mxs_auart_settermios(struct uart_port *u,
 		mxs_auart_disable_ms(u);
 }
 
+static void mxs_auart_set_ldisc(struct uart_port *port, int new)
+{
+	if (new == N_PPS) {
+		port->flags |= UPF_HARDPPS_CD;
+		mxs_auart_enable_ms(port);
+	} else {
+		port->flags &= ~UPF_HARDPPS_CD;
+	}
+}
+
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 {
 	u32 istat;
@@ -949,6 +959,7 @@ static struct uart_ops mxs_auart_ops = {
 	.startup	= mxs_auart_startup,
 	.shutdown       = mxs_auart_shutdown,
 	.set_termios    = mxs_auart_settermios,
+	.set_ldisc      = mxs_auart_set_ldisc,
 	.type	   	= mxs_auart_type,
 	.release_port   = mxs_auart_release_port,
 	.request_port   = mxs_auart_request_port,
-- 
1.7.11.3


^ permalink raw reply related	[flat|nested] 36+ messages in thread
* [PATCH v4 4/4] serial: mxs-auart: enable PPS support
@ 2014-10-10 16:53   ` Janusz Uzycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Uzycki @ 2014-10-10 16:53 UTC (permalink / raw)
  To: linux-arm-kernel

Enables PPS support in mxs-auart serial driver to make PPS API working.

Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
---
v3 -> v4 changelog:
* coding style: braces in both branches of condition

v2 -> v3 changelog:
* no changes

---
 drivers/tty/serial/mxs-auart.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 5ce4a50..840c1f7 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -784,6 +784,16 @@ static void mxs_auart_settermios(struct uart_port *u,
 		mxs_auart_disable_ms(u);
 }
 
+static void mxs_auart_set_ldisc(struct uart_port *port, int new)
+{
+	if (new == N_PPS) {
+		port->flags |= UPF_HARDPPS_CD;
+		mxs_auart_enable_ms(port);
+	} else {
+		port->flags &= ~UPF_HARDPPS_CD;
+	}
+}
+
 static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
 {
 	u32 istat;
@@ -949,6 +959,7 @@ static struct uart_ops mxs_auart_ops = {
 	.startup	= mxs_auart_startup,
 	.shutdown       = mxs_auart_shutdown,
 	.set_termios    = mxs_auart_settermios,
+	.set_ldisc      = mxs_auart_set_ldisc,
 	.type	   	= mxs_auart_type,
 	.release_port   = mxs_auart_release_port,
 	.request_port   = mxs_auart_request_port,
-- 
1.7.11.3

^ permalink raw reply related	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
  2014-10-10 16:53   ` Janusz Uzycki
@ 2014-10-24 15:31     ` Richard Genoud
  -1 siblings, 0 replies; 36+ messages in thread
From: Richard Genoud @ 2014-10-24 15:31 UTC (permalink / raw)
  To: Janusz Uzycki, Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel

On 10/10/2014 18:53, Janusz Uzycki wrote:
> Russell King:
> The only thing which the .get_mctrl method is supposed to do is
> to return the state of the /input/ lines, which are CTS, DCD, DSR, RI.
> The output line state is stored in port->mctrl, and is added to
> the returned value by serial_core when it's required.
> RTS output state should not be returned from the .get_mctrl method.
> 
> This patch removes ctrl variable from mxs_auart_port
> and removes useless reading back RTS line.
> 
> The ctrl variable in mxs_auart_port duplicated mctrl,
> member of uart_port structure in serial_core.h.
> The removed code from mxs_auart_set_mctrl() and mxs_auart_get_mctrl
> duplicated uart_update_mctrl() and uart_tiocmget()
> in serial_core.c.
> 
> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
> ---
> v3 -> v4 changelog:
> [PATCH 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
> * renamed from "serial: mxs-auart: ctrl removed from mxs_auart_port"
> * mxs_auart_get_mctrl() read back RTS line. It is removed too.
> 
> v2 -> v3 changelog:
> The patch was introduced:
> new [PATCH 1/4] serial: mxs-auart: ctrl removed from mxs_auart_port
> * the ctrl variable duplicated mctrl, member of uart_port structure
>   in serial_core.h
> * the code duplicated uart_update_mctrl() and uart_tiocmget()
>   in serial_core.c
> * mxs_auart_get_mctrl() reads back RTS line. It could be removed too
>   but not sure.
> 
> ---
>  drivers/tty/serial/mxs-auart.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index e838e84..2d49901 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -143,7 +143,6 @@ struct mxs_auart_port {
>  #define MXS_AUART_DMA_RX_READY	3  /* bit 3 */
>  #define MXS_AUART_RTSCTS	4  /* bit 4 */
>  	unsigned long flags;
> -	unsigned int ctrl;
>  	enum mxs_auart_type devtype;
>  
>  	unsigned int irq;
> @@ -406,8 +405,6 @@ static void mxs_auart_release_port(struct uart_port *u)
>  
>  static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  {
> -	struct mxs_auart_port *s = to_auart_port(u);
> -
>  	u32 ctrl = readl(u->membase + AUART_CTRL2);
>  
>  	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
> @@ -418,24 +415,17 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  			ctrl |= AUART_CTRL2_RTS;
>  	}
>  
> -	s->ctrl = mctrl;
>  	writel(ctrl, u->membase + AUART_CTRL2);
>  }
>  
>  static u32 mxs_auart_get_mctrl(struct uart_port *u)
>  {
> -	struct mxs_auart_port *s = to_auart_port(u);
>  	u32 stat = readl(u->membase + AUART_STAT);
> -	int ctrl2 = readl(u->membase + AUART_CTRL2);
> -	u32 mctrl = s->ctrl;
> +	u32 mctrl = 0;
>  
> -	mctrl &= ~TIOCM_CTS;
>  	if (stat & AUART_STAT_CTS)
>  		mctrl |= TIOCM_CTS;
>  
> -	if (ctrl2 & AUART_CTRL2_RTS)
> -		mctrl |= TIOCM_RTS;
> -
>  	return mctrl;
>  }
>  
> @@ -1071,8 +1061,6 @@ static int mxs_auart_probe(struct platform_device *pdev)
>  	s->port.type = PORT_IMX;
>  	s->port.dev = s->dev = &pdev->dev;
>  
> -	s->ctrl = 0;
> -
>  	s->irq = platform_get_irq(pdev, 0);
>  	s->port.irq = s->irq;
>  	ret = request_irq(s->irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s);
> 
Seems good to me.

Reviewed-by: Richard Genoud <richard.genoud@gmail.com>

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
@ 2014-10-24 15:31     ` Richard Genoud
  0 siblings, 0 replies; 36+ messages in thread
From: Richard Genoud @ 2014-10-24 15:31 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/10/2014 18:53, Janusz Uzycki wrote:
> Russell King:
> The only thing which the .get_mctrl method is supposed to do is
> to return the state of the /input/ lines, which are CTS, DCD, DSR, RI.
> The output line state is stored in port->mctrl, and is added to
> the returned value by serial_core when it's required.
> RTS output state should not be returned from the .get_mctrl method.
> 
> This patch removes ctrl variable from mxs_auart_port
> and removes useless reading back RTS line.
> 
> The ctrl variable in mxs_auart_port duplicated mctrl,
> member of uart_port structure in serial_core.h.
> The removed code from mxs_auart_set_mctrl() and mxs_auart_get_mctrl
> duplicated uart_update_mctrl() and uart_tiocmget()
> in serial_core.c.
> 
> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
> ---
> v3 -> v4 changelog:
> [PATCH 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl
> * renamed from "serial: mxs-auart: ctrl removed from mxs_auart_port"
> * mxs_auart_get_mctrl() read back RTS line. It is removed too.
> 
> v2 -> v3 changelog:
> The patch was introduced:
> new [PATCH 1/4] serial: mxs-auart: ctrl removed from mxs_auart_port
> * the ctrl variable duplicated mctrl, member of uart_port structure
>   in serial_core.h
> * the code duplicated uart_update_mctrl() and uart_tiocmget()
>   in serial_core.c
> * mxs_auart_get_mctrl() reads back RTS line. It could be removed too
>   but not sure.
> 
> ---
>  drivers/tty/serial/mxs-auart.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index e838e84..2d49901 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -143,7 +143,6 @@ struct mxs_auart_port {
>  #define MXS_AUART_DMA_RX_READY	3  /* bit 3 */
>  #define MXS_AUART_RTSCTS	4  /* bit 4 */
>  	unsigned long flags;
> -	unsigned int ctrl;
>  	enum mxs_auart_type devtype;
>  
>  	unsigned int irq;
> @@ -406,8 +405,6 @@ static void mxs_auart_release_port(struct uart_port *u)
>  
>  static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  {
> -	struct mxs_auart_port *s = to_auart_port(u);
> -
>  	u32 ctrl = readl(u->membase + AUART_CTRL2);
>  
>  	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
> @@ -418,24 +415,17 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  			ctrl |= AUART_CTRL2_RTS;
>  	}
>  
> -	s->ctrl = mctrl;
>  	writel(ctrl, u->membase + AUART_CTRL2);
>  }
>  
>  static u32 mxs_auart_get_mctrl(struct uart_port *u)
>  {
> -	struct mxs_auart_port *s = to_auart_port(u);
>  	u32 stat = readl(u->membase + AUART_STAT);
> -	int ctrl2 = readl(u->membase + AUART_CTRL2);
> -	u32 mctrl = s->ctrl;
> +	u32 mctrl = 0;
>  
> -	mctrl &= ~TIOCM_CTS;
>  	if (stat & AUART_STAT_CTS)
>  		mctrl |= TIOCM_CTS;
>  
> -	if (ctrl2 & AUART_CTRL2_RTS)
> -		mctrl |= TIOCM_RTS;
> -
>  	return mctrl;
>  }
>  
> @@ -1071,8 +1061,6 @@ static int mxs_auart_probe(struct platform_device *pdev)
>  	s->port.type = PORT_IMX;
>  	s->port.dev = s->dev = &pdev->dev;
>  
> -	s->ctrl = 0;
> -
>  	s->irq = platform_get_irq(pdev, 0);
>  	s->port.irq = s->irq;
>  	ret = request_irq(s->irq, mxs_auart_irq_handle, 0, dev_name(&pdev->dev), s);
> 
Seems good to me.

Reviewed-by: Richard Genoud <richard.genoud@gmail.com>

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-10-10 16:53   ` Janusz Uzycki
@ 2014-10-24 15:32     ` Richard Genoud
  -1 siblings, 0 replies; 36+ messages in thread
From: Richard Genoud @ 2014-10-24 15:32 UTC (permalink / raw)
  To: Janusz Uzycki, Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel

On 10/10/2014 18:53, Janusz Uzycki wrote:
> Dedicated CTS and RTS pins are unusable together with a lot of other
> peripherals because they share the same line. Pinctrl is limited.
> 
> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
> so we have to control them via GPIO.
> 
> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
> signals.
> 
> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
> ---
> v3 -> v4 changelog:
> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
> * DMA engine disabled if RTS or CTS is GPIO line
> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
> * CTSEN can't be enabled for hardware flow control block
>   if CTS is defined as GPIO line
> * RTSEN can be enabled for hardware flow control block
>   even if RTS is defined as GPIO line.
>   RTS pin depends on pinctrl configuration which
>   selects RTS output from hardware flow control block or GPIO line.
> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
> * mxs_auart_irq_handle(): CTS_AT_AUART() used
> * mxs_auart_init_gpios() returns true(success)/false(failure)
> * dev_err() message fixed in mxs_auart_probe()
> 
> v2 -> v3 changelog:
> * mctrl_gpio_free() removed to simplify:
>   mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>   mxs_auart_remove() because mctrl_gpio_init() does all
>   allocations with devm_* functions.
>   (see Documentation/serial/driver since kernel 3.16)
> * DMA on HW flow control comment updated, still not sure about the comment
> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>   mctrl_gpio_get() does not clear gpio interrupt pendings like
>   8250_core.c does with MSR.
> * mxs_auart_modem_status() moved to [3/4]
>   If enable_ms() is not called, uart_handle_cts_change()
>   shouldn't be called.
> 
> ---
>  .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>  drivers/tty/serial/Kconfig           |  1 +
>  drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>  3 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
> index 59a40f1..7c408c8 100644
> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
> @@ -11,8 +11,13 @@ Required properties:
>  - dma-names: "rx" for RX channel, "tx" for TX channel.
>  
>  Optional properties:
> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
> +  for hardware flow control,
>  	it also means you enable the DMA support for this UART.
> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
> +  line respectively. It will use specified PIO instead of the peripheral
> +  function pin for the USART feature.
> +  If unsure, don't specify this property.
>  
>  Example:
>  auart0: serial@8006a000 {
> @@ -21,6 +26,9 @@ auart0: serial@8006a000 {
>  	interrupts = <112>;
>  	dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>  	dma-names = "rx", "tx";
> +	cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
> +	dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +	dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>  };
>  
>  Note: Each auart port should have an alias correctly numbered in "aliases"
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 4fe8ca1..90e8516 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>  	depends on ARCH_MXS
>  	tristate "MXS AUART support"
>  	select SERIAL_CORE
> +	select SERIAL_MCTRL_GPIO if GPIOLIB
>  	help
>  	  This driver supports the MXS Application UART (AUART) port.
>  
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 2d49901..8bbcfd1 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -42,6 +42,9 @@
>  
>  #include <asm/cacheflush.h>
>  
> +#include <linux/err.h>
> +#include "serial_mctrl_gpio.h"
> +
>  #define MXS_AUART_PORTS 5
>  #define MXS_AUART_FIFO_SIZE		16
>  
> @@ -158,6 +161,8 @@ struct mxs_auart_port {
>  	struct scatterlist rx_sgl;
>  	struct dma_chan	*rx_dma_chan;
>  	void *rx_dma_buf;
> +
> +	struct mctrl_gpios	*gpios;
>  };
>  
>  static struct platform_device_id mxs_auart_devtype[] = {
> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port *u)
>  
>  static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  {
> +	struct mxs_auart_port *s = to_auart_port(u);
> +
>  	u32 ctrl = readl(u->membase + AUART_CTRL2);
>  
>  	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  	}
>  
>  	writel(ctrl, u->membase + AUART_CTRL2);
> +
> +	mctrl_gpio_set(s->gpios, mctrl);
>  }
>  
>  static u32 mxs_auart_get_mctrl(struct uart_port *u)
>  {
> +	struct mxs_auart_port *s = to_auart_port(u);
>  	u32 stat = readl(u->membase + AUART_STAT);
>  	u32 mctrl = 0;
>  
>  	if (stat & AUART_STAT_CTS)
>  		mctrl |= TIOCM_CTS;
>  
> -	return mctrl;
> +	return mctrl_gpio_get(s->gpios, &mctrl);
>  }
>  
>  static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
> @@ -554,6 +564,10 @@ err_out:
>  
>  }
>  
> +#define RTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
> +							UART_GPIO_RTS))
> +#define CTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
> +							UART_GPIO_CTS))
>  static void mxs_auart_settermios(struct uart_port *u,
>  				 struct ktermios *termios,
>  				 struct ktermios *old)
> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>  		ctrl |= AUART_LINECTRL_STP2;
>  
>  	/* figure out the hardware flow control settings */
> +	ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>  	if (cflag & CRTSCTS) {
>  		/*
>  		 * The DMA has a bug(see errata:2836) in mx23.
> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port *u,
>  				ctrl2 |= AUART_CTRL2_TXDMAE | AUART_CTRL2_RXDMAE
>  				       | AUART_CTRL2_DMAONERR;
>  		}
> -		ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
> -	} else {
> -		ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
> +		/* Even if RTS is GPIO line RTSEN can be enabled because
> +		 * the pinctrl configuration decides about RTS pin function */
> +		ctrl2 |= AUART_CTRL2_RTSEN;
> +		if (CTS_AT_AUART())
> +			ctrl2 |= AUART_CTRL2_CTSEN;
>  	}
>  
>  	/* set baud rate */
> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
>  			s->port.membase + AUART_INTR_CLR);
>  
>  	if (istat & AUART_INTR_CTSMIS) {
> -		uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
> +		if (CTS_AT_AUART())
> +			uart_handle_cts_change(&s->port,
> +					stat & AUART_STAT_CTS);
>  		writel(AUART_INTR_CTSMIS,
>  				s->port.membase + AUART_INTR_CLR);
>  		istat &= ~AUART_INTR_CTSMIS;
> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
>  	return 0;
>  }
>  
> +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
> +{
> +	s->gpios = mctrl_gpio_init(dev, 0);
> +	if (IS_ERR_OR_NULL(s->gpios))
> +		return false;
> +
> +	/* Block (enabled before) DMA option if RTS or CTS is GPIO line */
This is confusing, so I think some more comments won't be too much.
Something like:
/*
 * The DMA only work if the lines CTS/RTS are handled by the controller
 * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
valid
 * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
bit should be
 * cleaned to indicate that the RTS/CTS lines are not handled by the
controller.
 */
(If I understood correctly what is done here.)
> +	if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
> +		if (test_bit(MXS_AUART_RTSCTS, &s->flags))
> +			dev_warn(dev,
> +				 "DMA and flow control via gpio may cause some problems. DMA disabled!\n");
> +		clear_bit(MXS_AUART_RTSCTS, &s->flags);
> +	}
> +
> +	return true;
> +}
> +
>  static int mxs_auart_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *of_id =
> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, s);
>  
> +	if (!mxs_auart_init_gpios(s, &pdev->dev))
> +		dev_err(&pdev->dev,
> +			"Failed to initialize GPIOs. The serial port may not work as expected\n");
> +
>  	auart_port[s->port.line] = s;
>  
>  	mxs_auart_reset(&s->port);
> 
Reviewed-by: Richard Genoud <richard.genoud@gmail.com>

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-10-24 15:32     ` Richard Genoud
  0 siblings, 0 replies; 36+ messages in thread
From: Richard Genoud @ 2014-10-24 15:32 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/10/2014 18:53, Janusz Uzycki wrote:
> Dedicated CTS and RTS pins are unusable together with a lot of other
> peripherals because they share the same line. Pinctrl is limited.
> 
> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
> so we have to control them via GPIO.
> 
> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
> signals.
> 
> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
> ---
> v3 -> v4 changelog:
> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
> * DMA engine disabled if RTS or CTS is GPIO line
> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
> * CTSEN can't be enabled for hardware flow control block
>   if CTS is defined as GPIO line
> * RTSEN can be enabled for hardware flow control block
>   even if RTS is defined as GPIO line.
>   RTS pin depends on pinctrl configuration which
>   selects RTS output from hardware flow control block or GPIO line.
> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
> * mxs_auart_irq_handle(): CTS_AT_AUART() used
> * mxs_auart_init_gpios() returns true(success)/false(failure)
> * dev_err() message fixed in mxs_auart_probe()
> 
> v2 -> v3 changelog:
> * mctrl_gpio_free() removed to simplify:
>   mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>   mxs_auart_remove() because mctrl_gpio_init() does all
>   allocations with devm_* functions.
>   (see Documentation/serial/driver since kernel 3.16)
> * DMA on HW flow control comment updated, still not sure about the comment
> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>   mctrl_gpio_get() does not clear gpio interrupt pendings like
>   8250_core.c does with MSR.
> * mxs_auart_modem_status() moved to [3/4]
>   If enable_ms() is not called, uart_handle_cts_change()
>   shouldn't be called.
> 
> ---
>  .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>  drivers/tty/serial/Kconfig           |  1 +
>  drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>  3 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
> index 59a40f1..7c408c8 100644
> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
> @@ -11,8 +11,13 @@ Required properties:
>  - dma-names: "rx" for RX channel, "tx" for TX channel.
>  
>  Optional properties:
> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
> +  for hardware flow control,
>  	it also means you enable the DMA support for this UART.
> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
> +  line respectively. It will use specified PIO instead of the peripheral
> +  function pin for the USART feature.
> +  If unsure, don't specify this property.
>  
>  Example:
>  auart0: serial at 8006a000 {
> @@ -21,6 +26,9 @@ auart0: serial at 8006a000 {
>  	interrupts = <112>;
>  	dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>  	dma-names = "rx", "tx";
> +	cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
> +	dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +	dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>  };
>  
>  Note: Each auart port should have an alias correctly numbered in "aliases"
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 4fe8ca1..90e8516 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>  	depends on ARCH_MXS
>  	tristate "MXS AUART support"
>  	select SERIAL_CORE
> +	select SERIAL_MCTRL_GPIO if GPIOLIB
>  	help
>  	  This driver supports the MXS Application UART (AUART) port.
>  
> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 2d49901..8bbcfd1 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -42,6 +42,9 @@
>  
>  #include <asm/cacheflush.h>
>  
> +#include <linux/err.h>
> +#include "serial_mctrl_gpio.h"
> +
>  #define MXS_AUART_PORTS 5
>  #define MXS_AUART_FIFO_SIZE		16
>  
> @@ -158,6 +161,8 @@ struct mxs_auart_port {
>  	struct scatterlist rx_sgl;
>  	struct dma_chan	*rx_dma_chan;
>  	void *rx_dma_buf;
> +
> +	struct mctrl_gpios	*gpios;
>  };
>  
>  static struct platform_device_id mxs_auart_devtype[] = {
> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port *u)
>  
>  static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  {
> +	struct mxs_auart_port *s = to_auart_port(u);
> +
>  	u32 ctrl = readl(u->membase + AUART_CTRL2);
>  
>  	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>  	}
>  
>  	writel(ctrl, u->membase + AUART_CTRL2);
> +
> +	mctrl_gpio_set(s->gpios, mctrl);
>  }
>  
>  static u32 mxs_auart_get_mctrl(struct uart_port *u)
>  {
> +	struct mxs_auart_port *s = to_auart_port(u);
>  	u32 stat = readl(u->membase + AUART_STAT);
>  	u32 mctrl = 0;
>  
>  	if (stat & AUART_STAT_CTS)
>  		mctrl |= TIOCM_CTS;
>  
> -	return mctrl;
> +	return mctrl_gpio_get(s->gpios, &mctrl);
>  }
>  
>  static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
> @@ -554,6 +564,10 @@ err_out:
>  
>  }
>  
> +#define RTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
> +							UART_GPIO_RTS))
> +#define CTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
> +							UART_GPIO_CTS))
>  static void mxs_auart_settermios(struct uart_port *u,
>  				 struct ktermios *termios,
>  				 struct ktermios *old)
> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>  		ctrl |= AUART_LINECTRL_STP2;
>  
>  	/* figure out the hardware flow control settings */
> +	ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>  	if (cflag & CRTSCTS) {
>  		/*
>  		 * The DMA has a bug(see errata:2836) in mx23.
> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port *u,
>  				ctrl2 |= AUART_CTRL2_TXDMAE | AUART_CTRL2_RXDMAE
>  				       | AUART_CTRL2_DMAONERR;
>  		}
> -		ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
> -	} else {
> -		ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
> +		/* Even if RTS is GPIO line RTSEN can be enabled because
> +		 * the pinctrl configuration decides about RTS pin function */
> +		ctrl2 |= AUART_CTRL2_RTSEN;
> +		if (CTS_AT_AUART())
> +			ctrl2 |= AUART_CTRL2_CTSEN;
>  	}
>  
>  	/* set baud rate */
> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
>  			s->port.membase + AUART_INTR_CLR);
>  
>  	if (istat & AUART_INTR_CTSMIS) {
> -		uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
> +		if (CTS_AT_AUART())
> +			uart_handle_cts_change(&s->port,
> +					stat & AUART_STAT_CTS);
>  		writel(AUART_INTR_CTSMIS,
>  				s->port.membase + AUART_INTR_CLR);
>  		istat &= ~AUART_INTR_CTSMIS;
> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
>  	return 0;
>  }
>  
> +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
> +{
> +	s->gpios = mctrl_gpio_init(dev, 0);
> +	if (IS_ERR_OR_NULL(s->gpios))
> +		return false;
> +
> +	/* Block (enabled before) DMA option if RTS or CTS is GPIO line */
This is confusing, so I think some more comments won't be too much.
Something like:
/*
 * The DMA only work if the lines CTS/RTS are handled by the controller
 * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
valid
 * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
bit should be
 * cleaned to indicate that the RTS/CTS lines are not handled by the
controller.
 */
(If I understood correctly what is done here.)
> +	if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
> +		if (test_bit(MXS_AUART_RTSCTS, &s->flags))
> +			dev_warn(dev,
> +				 "DMA and flow control via gpio may cause some problems. DMA disabled!\n");
> +		clear_bit(MXS_AUART_RTSCTS, &s->flags);
> +	}
> +
> +	return true;
> +}
> +
>  static int mxs_auart_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *of_id =
> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, s);
>  
> +	if (!mxs_auart_init_gpios(s, &pdev->dev))
> +		dev_err(&pdev->dev,
> +			"Failed to initialize GPIOs. The serial port may not work as expected\n");
> +
>  	auart_port[s->port.line] = s;
>  
>  	mxs_auart_reset(&s->port);
> 
Reviewed-by: Richard Genoud <richard.genoud@gmail.com>

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-10-24 15:32     ` Richard Genoud
@ 2014-10-24 15:51       ` Janusz Użycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-10-24 15:51 UTC (permalink / raw)
  To: Richard Genoud, Greg Kroah-Hartman
  Cc: Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel


W dniu 2014-10-24 o 17:32, Richard Genoud pisze:
> On 10/10/2014 18:53, Janusz Uzycki wrote:
>> Dedicated CTS and RTS pins are unusable together with a lot of other
>> peripherals because they share the same line. Pinctrl is limited.
>>
>> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
>> so we have to control them via GPIO.
>>
>> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>> signals.
>>
>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
>> ---
>> v3 -> v4 changelog:
>> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
>> * DMA engine disabled if RTS or CTS is GPIO line
>> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
>> * CTSEN can't be enabled for hardware flow control block
>>    if CTS is defined as GPIO line
>> * RTSEN can be enabled for hardware flow control block
>>    even if RTS is defined as GPIO line.
>>    RTS pin depends on pinctrl configuration which
>>    selects RTS output from hardware flow control block or GPIO line.
>> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
>> * mxs_auart_irq_handle(): CTS_AT_AUART() used
>> * mxs_auart_init_gpios() returns true(success)/false(failure)
>> * dev_err() message fixed in mxs_auart_probe()
>>
>> v2 -> v3 changelog:
>> * mctrl_gpio_free() removed to simplify:
>>    mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>>    mxs_auart_remove() because mctrl_gpio_init() does all
>>    allocations with devm_* functions.
>>    (see Documentation/serial/driver since kernel 3.16)
>> * DMA on HW flow control comment updated, still not sure about the comment
>> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>>    mctrl_gpio_get() does not clear gpio interrupt pendings like
>>    8250_core.c does with MSR.
>> * mxs_auart_modem_status() moved to [3/4]
>>    If enable_ms() is not called, uart_handle_cts_change()
>>    shouldn't be called.
>>
>> ---
>>   .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>>   drivers/tty/serial/Kconfig           |  1 +
>>   drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>> index 59a40f1..7c408c8 100644
>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>> @@ -11,8 +11,13 @@ Required properties:
>>   - dma-names: "rx" for RX channel, "tx" for TX channel.
>>   
>>   Optional properties:
>> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
>> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
>> +  for hardware flow control,
>>   	it also means you enable the DMA support for this UART.
>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
>> +  line respectively. It will use specified PIO instead of the peripheral
>> +  function pin for the USART feature.
>> +  If unsure, don't specify this property.
>>   
>>   Example:
>>   auart0: serial@8006a000 {
>> @@ -21,6 +26,9 @@ auart0: serial@8006a000 {
>>   	interrupts = <112>;
>>   	dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>>   	dma-names = "rx", "tx";
>> +	cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>> +	dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>> +	dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>   };
>>   
>>   Note: Each auart port should have an alias correctly numbered in "aliases"
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index 4fe8ca1..90e8516 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>>   	depends on ARCH_MXS
>>   	tristate "MXS AUART support"
>>   	select SERIAL_CORE
>> +	select SERIAL_MCTRL_GPIO if GPIOLIB
>>   	help
>>   	  This driver supports the MXS Application UART (AUART) port.
>>   
>> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
>> index 2d49901..8bbcfd1 100644
>> --- a/drivers/tty/serial/mxs-auart.c
>> +++ b/drivers/tty/serial/mxs-auart.c
>> @@ -42,6 +42,9 @@
>>   
>>   #include <asm/cacheflush.h>
>>   
>> +#include <linux/err.h>
>> +#include "serial_mctrl_gpio.h"
>> +
>>   #define MXS_AUART_PORTS 5
>>   #define MXS_AUART_FIFO_SIZE		16
>>   
>> @@ -158,6 +161,8 @@ struct mxs_auart_port {
>>   	struct scatterlist rx_sgl;
>>   	struct dma_chan	*rx_dma_chan;
>>   	void *rx_dma_buf;
>> +
>> +	struct mctrl_gpios	*gpios;
>>   };
>>   
>>   static struct platform_device_id mxs_auart_devtype[] = {
>> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port *u)
>>   
>>   static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>   {
>> +	struct mxs_auart_port *s = to_auart_port(u);
>> +
>>   	u32 ctrl = readl(u->membase + AUART_CTRL2);
>>   
>>   	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
>> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>   	}
>>   
>>   	writel(ctrl, u->membase + AUART_CTRL2);
>> +
>> +	mctrl_gpio_set(s->gpios, mctrl);
>>   }
>>   
>>   static u32 mxs_auart_get_mctrl(struct uart_port *u)
>>   {
>> +	struct mxs_auart_port *s = to_auart_port(u);
>>   	u32 stat = readl(u->membase + AUART_STAT);
>>   	u32 mctrl = 0;
>>   
>>   	if (stat & AUART_STAT_CTS)
>>   		mctrl |= TIOCM_CTS;
>>   
>> -	return mctrl;
>> +	return mctrl_gpio_get(s->gpios, &mctrl);
>>   }
>>   
>>   static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
>> @@ -554,6 +564,10 @@ err_out:
>>   
>>   }
>>   
>> +#define RTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
>> +							UART_GPIO_RTS))
>> +#define CTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
>> +							UART_GPIO_CTS))
>>   static void mxs_auart_settermios(struct uart_port *u,
>>   				 struct ktermios *termios,
>>   				 struct ktermios *old)
>> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>>   		ctrl |= AUART_LINECTRL_STP2;
>>   
>>   	/* figure out the hardware flow control settings */
>> +	ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>   	if (cflag & CRTSCTS) {
>>   		/*
>>   		 * The DMA has a bug(see errata:2836) in mx23.
>> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port *u,
>>   				ctrl2 |= AUART_CTRL2_TXDMAE | AUART_CTRL2_RXDMAE
>>   				       | AUART_CTRL2_DMAONERR;
>>   		}
>> -		ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
>> -	} else {
>> -		ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>> +		/* Even if RTS is GPIO line RTSEN can be enabled because
>> +		 * the pinctrl configuration decides about RTS pin function */
>> +		ctrl2 |= AUART_CTRL2_RTSEN;
>> +		if (CTS_AT_AUART())
>> +			ctrl2 |= AUART_CTRL2_CTSEN;
>>   	}
>>   
>>   	/* set baud rate */
>> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
>>   			s->port.membase + AUART_INTR_CLR);
>>   
>>   	if (istat & AUART_INTR_CTSMIS) {
>> -		uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
>> +		if (CTS_AT_AUART())
>> +			uart_handle_cts_change(&s->port,
>> +					stat & AUART_STAT_CTS);
>>   		writel(AUART_INTR_CTSMIS,
>>   				s->port.membase + AUART_INTR_CLR);
>>   		istat &= ~AUART_INTR_CTSMIS;
>> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
>>   	return 0;
>>   }
>>   
>> +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
>> +{
>> +	s->gpios = mctrl_gpio_init(dev, 0);
>> +	if (IS_ERR_OR_NULL(s->gpios))
>> +		return false;
>> +
>> +	/* Block (enabled before) DMA option if RTS or CTS is GPIO line */
> This is confusing, so I think some more comments won't be too much.
> Something like:
> /*
>   * The DMA only work if the lines CTS/RTS are handled by the controller
>   * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
> valid
>   * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
> bit should be
>   * cleaned to indicate that the RTS/CTS lines are not handled by the
> controller.
>   */
> (If I understood correctly what is done here.)

This is not clear in the driver. If hardware RTS/CTS (non-gpio) is not 
defined in DT
only DMA is not used. It is still possible to use hardware RTS/CTS lines
if you set hw-flowcontrol in termios. So it is also possible to mix 
hardware RTS/CTS
and gpio RTS/CTS. In mixed or gpio case the code blocks/disables DMA.
I know my comment is not clear but the reason is in the earlier code.
Can you propose better solution?

After review and small fixes requested new version is required for a 
patchset
or rather patch resend?

kind regards
Janusz

>> +	if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
>> +		if (test_bit(MXS_AUART_RTSCTS, &s->flags))
>> +			dev_warn(dev,
>> +				 "DMA and flow control via gpio may cause some problems. DMA disabled!\n");
>> +		clear_bit(MXS_AUART_RTSCTS, &s->flags);
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   static int mxs_auart_probe(struct platform_device *pdev)
>>   {
>>   	const struct of_device_id *of_id =
>> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, s);
>>   
>> +	if (!mxs_auart_init_gpios(s, &pdev->dev))
>> +		dev_err(&pdev->dev,
>> +			"Failed to initialize GPIOs. The serial port may not work as expected\n");
>> +
>>   	auart_port[s->port.line] = s;
>>   
>>   	mxs_auart_reset(&s->port);
>>
> Reviewed-by: Richard Genoud <richard.genoud@gmail.com>


^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-10-24 15:51       ` Janusz Użycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-10-24 15:51 UTC (permalink / raw)
  To: linux-arm-kernel


W dniu 2014-10-24 o 17:32, Richard Genoud pisze:
> On 10/10/2014 18:53, Janusz Uzycki wrote:
>> Dedicated CTS and RTS pins are unusable together with a lot of other
>> peripherals because they share the same line. Pinctrl is limited.
>>
>> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
>> so we have to control them via GPIO.
>>
>> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>> signals.
>>
>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
>> ---
>> v3 -> v4 changelog:
>> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
>> * DMA engine disabled if RTS or CTS is GPIO line
>> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
>> * CTSEN can't be enabled for hardware flow control block
>>    if CTS is defined as GPIO line
>> * RTSEN can be enabled for hardware flow control block
>>    even if RTS is defined as GPIO line.
>>    RTS pin depends on pinctrl configuration which
>>    selects RTS output from hardware flow control block or GPIO line.
>> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
>> * mxs_auart_irq_handle(): CTS_AT_AUART() used
>> * mxs_auart_init_gpios() returns true(success)/false(failure)
>> * dev_err() message fixed in mxs_auart_probe()
>>
>> v2 -> v3 changelog:
>> * mctrl_gpio_free() removed to simplify:
>>    mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>>    mxs_auart_remove() because mctrl_gpio_init() does all
>>    allocations with devm_* functions.
>>    (see Documentation/serial/driver since kernel 3.16)
>> * DMA on HW flow control comment updated, still not sure about the comment
>> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>>    mctrl_gpio_get() does not clear gpio interrupt pendings like
>>    8250_core.c does with MSR.
>> * mxs_auart_modem_status() moved to [3/4]
>>    If enable_ms() is not called, uart_handle_cts_change()
>>    shouldn't be called.
>>
>> ---
>>   .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>>   drivers/tty/serial/Kconfig           |  1 +
>>   drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>> index 59a40f1..7c408c8 100644
>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>> @@ -11,8 +11,13 @@ Required properties:
>>   - dma-names: "rx" for RX channel, "tx" for TX channel.
>>   
>>   Optional properties:
>> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
>> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
>> +  for hardware flow control,
>>   	it also means you enable the DMA support for this UART.
>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
>> +  line respectively. It will use specified PIO instead of the peripheral
>> +  function pin for the USART feature.
>> +  If unsure, don't specify this property.
>>   
>>   Example:
>>   auart0: serial at 8006a000 {
>> @@ -21,6 +26,9 @@ auart0: serial at 8006a000 {
>>   	interrupts = <112>;
>>   	dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>>   	dma-names = "rx", "tx";
>> +	cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>> +	dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>> +	dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>   };
>>   
>>   Note: Each auart port should have an alias correctly numbered in "aliases"
>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>> index 4fe8ca1..90e8516 100644
>> --- a/drivers/tty/serial/Kconfig
>> +++ b/drivers/tty/serial/Kconfig
>> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>>   	depends on ARCH_MXS
>>   	tristate "MXS AUART support"
>>   	select SERIAL_CORE
>> +	select SERIAL_MCTRL_GPIO if GPIOLIB
>>   	help
>>   	  This driver supports the MXS Application UART (AUART) port.
>>   
>> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
>> index 2d49901..8bbcfd1 100644
>> --- a/drivers/tty/serial/mxs-auart.c
>> +++ b/drivers/tty/serial/mxs-auart.c
>> @@ -42,6 +42,9 @@
>>   
>>   #include <asm/cacheflush.h>
>>   
>> +#include <linux/err.h>
>> +#include "serial_mctrl_gpio.h"
>> +
>>   #define MXS_AUART_PORTS 5
>>   #define MXS_AUART_FIFO_SIZE		16
>>   
>> @@ -158,6 +161,8 @@ struct mxs_auart_port {
>>   	struct scatterlist rx_sgl;
>>   	struct dma_chan	*rx_dma_chan;
>>   	void *rx_dma_buf;
>> +
>> +	struct mctrl_gpios	*gpios;
>>   };
>>   
>>   static struct platform_device_id mxs_auart_devtype[] = {
>> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port *u)
>>   
>>   static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>   {
>> +	struct mxs_auart_port *s = to_auart_port(u);
>> +
>>   	u32 ctrl = readl(u->membase + AUART_CTRL2);
>>   
>>   	ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
>> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>   	}
>>   
>>   	writel(ctrl, u->membase + AUART_CTRL2);
>> +
>> +	mctrl_gpio_set(s->gpios, mctrl);
>>   }
>>   
>>   static u32 mxs_auart_get_mctrl(struct uart_port *u)
>>   {
>> +	struct mxs_auart_port *s = to_auart_port(u);
>>   	u32 stat = readl(u->membase + AUART_STAT);
>>   	u32 mctrl = 0;
>>   
>>   	if (stat & AUART_STAT_CTS)
>>   		mctrl |= TIOCM_CTS;
>>   
>> -	return mctrl;
>> +	return mctrl_gpio_get(s->gpios, &mctrl);
>>   }
>>   
>>   static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
>> @@ -554,6 +564,10 @@ err_out:
>>   
>>   }
>>   
>> +#define RTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
>> +							UART_GPIO_RTS))
>> +#define CTS_AT_AUART()	IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,	\
>> +							UART_GPIO_CTS))
>>   static void mxs_auart_settermios(struct uart_port *u,
>>   				 struct ktermios *termios,
>>   				 struct ktermios *old)
>> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>>   		ctrl |= AUART_LINECTRL_STP2;
>>   
>>   	/* figure out the hardware flow control settings */
>> +	ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>   	if (cflag & CRTSCTS) {
>>   		/*
>>   		 * The DMA has a bug(see errata:2836) in mx23.
>> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port *u,
>>   				ctrl2 |= AUART_CTRL2_TXDMAE | AUART_CTRL2_RXDMAE
>>   				       | AUART_CTRL2_DMAONERR;
>>   		}
>> -		ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
>> -	} else {
>> -		ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>> +		/* Even if RTS is GPIO line RTSEN can be enabled because
>> +		 * the pinctrl configuration decides about RTS pin function */
>> +		ctrl2 |= AUART_CTRL2_RTSEN;
>> +		if (CTS_AT_AUART())
>> +			ctrl2 |= AUART_CTRL2_CTSEN;
>>   	}
>>   
>>   	/* set baud rate */
>> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context)
>>   			s->port.membase + AUART_INTR_CLR);
>>   
>>   	if (istat & AUART_INTR_CTSMIS) {
>> -		uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
>> +		if (CTS_AT_AUART())
>> +			uart_handle_cts_change(&s->port,
>> +					stat & AUART_STAT_CTS);
>>   		writel(AUART_INTR_CTSMIS,
>>   				s->port.membase + AUART_INTR_CLR);
>>   		istat &= ~AUART_INTR_CTSMIS;
>> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s,
>>   	return 0;
>>   }
>>   
>> +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct device *dev)
>> +{
>> +	s->gpios = mctrl_gpio_init(dev, 0);
>> +	if (IS_ERR_OR_NULL(s->gpios))
>> +		return false;
>> +
>> +	/* Block (enabled before) DMA option if RTS or CTS is GPIO line */
> This is confusing, so I think some more comments won't be too much.
> Something like:
> /*
>   * The DMA only work if the lines CTS/RTS are handled by the controller
>   * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
> valid
>   * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
> bit should be
>   * cleaned to indicate that the RTS/CTS lines are not handled by the
> controller.
>   */
> (If I understood correctly what is done here.)

This is not clear in the driver. If hardware RTS/CTS (non-gpio) is not 
defined in DT
only DMA is not used. It is still possible to use hardware RTS/CTS lines
if you set hw-flowcontrol in termios. So it is also possible to mix 
hardware RTS/CTS
and gpio RTS/CTS. In mixed or gpio case the code blocks/disables DMA.
I know my comment is not clear but the reason is in the earlier code.
Can you propose better solution?

After review and small fixes requested new version is required for a 
patchset
or rather patch resend?

kind regards
Janusz

>> +	if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
>> +		if (test_bit(MXS_AUART_RTSCTS, &s->flags))
>> +			dev_warn(dev,
>> +				 "DMA and flow control via gpio may cause some problems. DMA disabled!\n");
>> +		clear_bit(MXS_AUART_RTSCTS, &s->flags);
>> +	}
>> +
>> +	return true;
>> +}
>> +
>>   static int mxs_auart_probe(struct platform_device *pdev)
>>   {
>>   	const struct of_device_id *of_id =
>> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, s);
>>   
>> +	if (!mxs_auart_init_gpios(s, &pdev->dev))
>> +		dev_err(&pdev->dev,
>> +			"Failed to initialize GPIOs. The serial port may not work as expected\n");
>> +
>>   	auart_port[s->port.line] = s;
>>   
>>   	mxs_auart_reset(&s->port);
>>
> Reviewed-by: Richard Genoud <richard.genoud@gmail.com>

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-10-24 15:51       ` Janusz Użycki
@ 2014-10-31  8:48         ` Richard Genoud
  -1 siblings, 0 replies; 36+ messages in thread
From: Richard Genoud @ 2014-10-31  8:48 UTC (permalink / raw)
  To: Janusz Użycki, Huang Shijie
  Cc: Greg Kroah-Hartman, Russell King - ARM Linux, Jiri Slaby,
	Fabio Estevam, linux-serial, devicetree, linux-arm-kernel

2014-10-24 17:51 GMT+02:00 Janusz Użycki <j.uzycki@elproma.com.pl>:
>
> W dniu 2014-10-24 o 17:32, Richard Genoud pisze:
>
>> On 10/10/2014 18:53, Janusz Uzycki wrote:
>>>
>>> Dedicated CTS and RTS pins are unusable together with a lot of other
>>> peripherals because they share the same line. Pinctrl is limited.
>>>
>>> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
>>> so we have to control them via GPIO.
>>>
>>> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>>> signals.
>>>
>>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
>>> ---
>>> v3 -> v4 changelog:
>>> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
>>> * DMA engine disabled if RTS or CTS is GPIO line
>>> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
>>> * CTSEN can't be enabled for hardware flow control block
>>>    if CTS is defined as GPIO line
>>> * RTSEN can be enabled for hardware flow control block
>>>    even if RTS is defined as GPIO line.
>>>    RTS pin depends on pinctrl configuration which
>>>    selects RTS output from hardware flow control block or GPIO line.
>>> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
>>> * mxs_auart_irq_handle(): CTS_AT_AUART() used
>>> * mxs_auart_init_gpios() returns true(success)/false(failure)
>>> * dev_err() message fixed in mxs_auart_probe()
>>>
>>> v2 -> v3 changelog:
>>> * mctrl_gpio_free() removed to simplify:
>>>    mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>>>    mxs_auart_remove() because mctrl_gpio_init() does all
>>>    allocations with devm_* functions.
>>>    (see Documentation/serial/driver since kernel 3.16)
>>> * DMA on HW flow control comment updated, still not sure about the
>>> comment
>>> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>>>    mctrl_gpio_get() does not clear gpio interrupt pendings like
>>>    8250_core.c does with MSR.
>>> * mxs_auart_modem_status() moved to [3/4]
>>>    If enable_ms() is not called, uart_handle_cts_change()
>>>    shouldn't be called.
>>>
>>> ---
>>>   .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>>>   drivers/tty/serial/Kconfig           |  1 +
>>>   drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> index 59a40f1..7c408c8 100644
>>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> @@ -11,8 +11,13 @@ Required properties:
>>>   - dma-names: "rx" for RX channel, "tx" for TX channel.
>>>     Optional properties:
>>> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
>>> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
>>> +  for hardware flow control,
>>>         it also means you enable the DMA support for this UART.
>>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for
>>> RTS/CTS/DTR/DSR/RI/DCD
>>> +  line respectively. It will use specified PIO instead of the peripheral
>>> +  function pin for the USART feature.
>>> +  If unsure, don't specify this property.
>>>     Example:
>>>   auart0: serial@8006a000 {
>>> @@ -21,6 +26,9 @@ auart0: serial@8006a000 {
>>>         interrupts = <112>;
>>>         dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>>>         dma-names = "rx", "tx";
>>> +       cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>>> +       dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>>> +       dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>>   };
>>>     Note: Each auart port should have an alias correctly numbered in
>>> "aliases"
>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>> index 4fe8ca1..90e8516 100644
>>> --- a/drivers/tty/serial/Kconfig
>>> +++ b/drivers/tty/serial/Kconfig
>>> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>>>         depends on ARCH_MXS
>>>         tristate "MXS AUART support"
>>>         select SERIAL_CORE
>>> +       select SERIAL_MCTRL_GPIO if GPIOLIB
>>>         help
>>>           This driver supports the MXS Application UART (AUART) port.
>>>   diff --git a/drivers/tty/serial/mxs-auart.c
>>> b/drivers/tty/serial/mxs-auart.c
>>> index 2d49901..8bbcfd1 100644
>>> --- a/drivers/tty/serial/mxs-auart.c
>>> +++ b/drivers/tty/serial/mxs-auart.c
>>> @@ -42,6 +42,9 @@
>>>     #include <asm/cacheflush.h>
>>>   +#include <linux/err.h>
>>> +#include "serial_mctrl_gpio.h"
>>> +
>>>   #define MXS_AUART_PORTS 5
>>>   #define MXS_AUART_FIFO_SIZE           16
>>>   @@ -158,6 +161,8 @@ struct mxs_auart_port {
>>>         struct scatterlist rx_sgl;
>>>         struct dma_chan *rx_dma_chan;
>>>         void *rx_dma_buf;
>>> +
>>> +       struct mctrl_gpios      *gpios;
>>>   };
>>>     static struct platform_device_id mxs_auart_devtype[] = {
>>> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port
>>> *u)
>>>     static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>>   {
>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>> +
>>>         u32 ctrl = readl(u->membase + AUART_CTRL2);
>>>         ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
>>> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port
>>> *u, unsigned mctrl)
>>>         }
>>>         writel(ctrl, u->membase + AUART_CTRL2);
>>> +
>>> +       mctrl_gpio_set(s->gpios, mctrl);
>>>   }
>>>     static u32 mxs_auart_get_mctrl(struct uart_port *u)
>>>   {
>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>>         u32 stat = readl(u->membase + AUART_STAT);
>>>         u32 mctrl = 0;
>>>         if (stat & AUART_STAT_CTS)
>>>                 mctrl |= TIOCM_CTS;
>>>   -     return mctrl;
>>> +       return mctrl_gpio_get(s->gpios, &mctrl);
>>>   }
>>>     static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
>>> @@ -554,6 +564,10 @@ err_out:
>>>     }
>>>   +#define RTS_AT_AUART()
>>> IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>> +                                                       UART_GPIO_RTS))
>>> +#define CTS_AT_AUART() IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>> +                                                       UART_GPIO_CTS))
>>>   static void mxs_auart_settermios(struct uart_port *u,
>>>                                  struct ktermios *termios,
>>>                                  struct ktermios *old)
>>> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>>>                 ctrl |= AUART_LINECTRL_STP2;
>>>         /* figure out the hardware flow control settings */
>>> +       ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>>         if (cflag & CRTSCTS) {
>>>                 /*
>>>                  * The DMA has a bug(see errata:2836) in mx23.
>>> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port
>>> *u,
>>>                                 ctrl2 |= AUART_CTRL2_TXDMAE |
>>> AUART_CTRL2_RXDMAE
>>>                                        | AUART_CTRL2_DMAONERR;
>>>                 }
>>> -               ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
>>> -       } else {
>>> -               ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>> +               /* Even if RTS is GPIO line RTSEN can be enabled because
>>> +                * the pinctrl configuration decides about RTS pin
>>> function */
>>> +               ctrl2 |= AUART_CTRL2_RTSEN;
>>> +               if (CTS_AT_AUART())
>>> +                       ctrl2 |= AUART_CTRL2_CTSEN;
>>>         }
>>>         /* set baud rate */
>>> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void
>>> *context)
>>>                         s->port.membase + AUART_INTR_CLR);
>>>         if (istat & AUART_INTR_CTSMIS) {
>>> -               uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
>>> +               if (CTS_AT_AUART())
>>> +                       uart_handle_cts_change(&s->port,
>>> +                                       stat & AUART_STAT_CTS);
>>>                 writel(AUART_INTR_CTSMIS,
>>>                                 s->port.membase + AUART_INTR_CLR);
>>>                 istat &= ~AUART_INTR_CTSMIS;
>>> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct
>>> mxs_auart_port *s,
>>>         return 0;
>>>   }
>>>   +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct
>>> device *dev)
>>> +{
>>> +       s->gpios = mctrl_gpio_init(dev, 0);
>>> +       if (IS_ERR_OR_NULL(s->gpios))
>>> +               return false;
>>> +
>>> +       /* Block (enabled before) DMA option if RTS or CTS is GPIO line
>>> */
>>
>> This is confusing, so I think some more comments won't be too much.
>> Something like:
>> /*
>>   * The DMA only work if the lines CTS/RTS are handled by the controller
>>   * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
>> valid
>>   * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
>> bit should be
>>   * cleaned to indicate that the RTS/CTS lines are not handled by the
>> controller.
>>   */
>> (If I understood correctly what is done here.)
>
>
> This is not clear in the driver. If hardware RTS/CTS (non-gpio) is not
> defined in DT
> only DMA is not used. It is still possible to use hardware RTS/CTS lines
> if you set hw-flowcontrol in termios. So it is also possible to mix hardware
> RTS/CTS
> and gpio RTS/CTS. In mixed or gpio case the code blocks/disables DMA.
> I know my comment is not clear but the reason is in the earlier code.
> Can you propose better solution?
Maybe Huang Shijie knows more on this DMA vs RTS/CTS incompatibility.
[added Huang Shijie in Cc ]

> After review and small fixes requested new version is required for a
> patchset
> or rather patch resend?
It's better to resend the whole patchset (after giving some time for
Huang Shijie to give its thoughts )

>
> kind regards
> Janusz
>
>
>>> +       if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
>>> +               if (test_bit(MXS_AUART_RTSCTS, &s->flags))
>>> +                       dev_warn(dev,
>>> +                                "DMA and flow control via gpio may cause
>>> some problems. DMA disabled!\n");
>>> +               clear_bit(MXS_AUART_RTSCTS, &s->flags);
>>> +       }
>>> +
>>> +       return true;
>>> +}
>>> +
>>>   static int mxs_auart_probe(struct platform_device *pdev)
>>>   {
>>>         const struct of_device_id *of_id =
>>> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device
>>> *pdev)
>>>         platform_set_drvdata(pdev, s);
>>>   +     if (!mxs_auart_init_gpios(s, &pdev->dev))
>>> +               dev_err(&pdev->dev,
>>> +                       "Failed to initialize GPIOs. The serial port may
>>> not work as expected\n");
>>> +
>>>         auart_port[s->port.line] = s;
>>>         mxs_auart_reset(&s->port);
>>>
>> Reviewed-by: Richard Genoud <richard.genoud@gmail.com>
>
>



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-10-31  8:48         ` Richard Genoud
  0 siblings, 0 replies; 36+ messages in thread
From: Richard Genoud @ 2014-10-31  8:48 UTC (permalink / raw)
  To: linux-arm-kernel

2014-10-24 17:51 GMT+02:00 Janusz U?ycki <j.uzycki@elproma.com.pl>:
>
> W dniu 2014-10-24 o 17:32, Richard Genoud pisze:
>
>> On 10/10/2014 18:53, Janusz Uzycki wrote:
>>>
>>> Dedicated CTS and RTS pins are unusable together with a lot of other
>>> peripherals because they share the same line. Pinctrl is limited.
>>>
>>> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
>>> so we have to control them via GPIO.
>>>
>>> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>>> signals.
>>>
>>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
>>> ---
>>> v3 -> v4 changelog:
>>> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
>>> * DMA engine disabled if RTS or CTS is GPIO line
>>> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
>>> * CTSEN can't be enabled for hardware flow control block
>>>    if CTS is defined as GPIO line
>>> * RTSEN can be enabled for hardware flow control block
>>>    even if RTS is defined as GPIO line.
>>>    RTS pin depends on pinctrl configuration which
>>>    selects RTS output from hardware flow control block or GPIO line.
>>> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
>>> * mxs_auart_irq_handle(): CTS_AT_AUART() used
>>> * mxs_auart_init_gpios() returns true(success)/false(failure)
>>> * dev_err() message fixed in mxs_auart_probe()
>>>
>>> v2 -> v3 changelog:
>>> * mctrl_gpio_free() removed to simplify:
>>>    mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>>>    mxs_auart_remove() because mctrl_gpio_init() does all
>>>    allocations with devm_* functions.
>>>    (see Documentation/serial/driver since kernel 3.16)
>>> * DMA on HW flow control comment updated, still not sure about the
>>> comment
>>> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>>>    mctrl_gpio_get() does not clear gpio interrupt pendings like
>>>    8250_core.c does with MSR.
>>> * mxs_auart_modem_status() moved to [3/4]
>>>    If enable_ms() is not called, uart_handle_cts_change()
>>>    shouldn't be called.
>>>
>>> ---
>>>   .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>>>   drivers/tty/serial/Kconfig           |  1 +
>>>   drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>>>   3 files changed, 55 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> index 59a40f1..7c408c8 100644
>>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>> @@ -11,8 +11,13 @@ Required properties:
>>>   - dma-names: "rx" for RX channel, "tx" for TX channel.
>>>     Optional properties:
>>> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
>>> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
>>> +  for hardware flow control,
>>>         it also means you enable the DMA support for this UART.
>>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for
>>> RTS/CTS/DTR/DSR/RI/DCD
>>> +  line respectively. It will use specified PIO instead of the peripheral
>>> +  function pin for the USART feature.
>>> +  If unsure, don't specify this property.
>>>     Example:
>>>   auart0: serial at 8006a000 {
>>> @@ -21,6 +26,9 @@ auart0: serial at 8006a000 {
>>>         interrupts = <112>;
>>>         dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>>>         dma-names = "rx", "tx";
>>> +       cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>>> +       dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>>> +       dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>>   };
>>>     Note: Each auart port should have an alias correctly numbered in
>>> "aliases"
>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>> index 4fe8ca1..90e8516 100644
>>> --- a/drivers/tty/serial/Kconfig
>>> +++ b/drivers/tty/serial/Kconfig
>>> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>>>         depends on ARCH_MXS
>>>         tristate "MXS AUART support"
>>>         select SERIAL_CORE
>>> +       select SERIAL_MCTRL_GPIO if GPIOLIB
>>>         help
>>>           This driver supports the MXS Application UART (AUART) port.
>>>   diff --git a/drivers/tty/serial/mxs-auart.c
>>> b/drivers/tty/serial/mxs-auart.c
>>> index 2d49901..8bbcfd1 100644
>>> --- a/drivers/tty/serial/mxs-auart.c
>>> +++ b/drivers/tty/serial/mxs-auart.c
>>> @@ -42,6 +42,9 @@
>>>     #include <asm/cacheflush.h>
>>>   +#include <linux/err.h>
>>> +#include "serial_mctrl_gpio.h"
>>> +
>>>   #define MXS_AUART_PORTS 5
>>>   #define MXS_AUART_FIFO_SIZE           16
>>>   @@ -158,6 +161,8 @@ struct mxs_auart_port {
>>>         struct scatterlist rx_sgl;
>>>         struct dma_chan *rx_dma_chan;
>>>         void *rx_dma_buf;
>>> +
>>> +       struct mctrl_gpios      *gpios;
>>>   };
>>>     static struct platform_device_id mxs_auart_devtype[] = {
>>> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port
>>> *u)
>>>     static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>>   {
>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>> +
>>>         u32 ctrl = readl(u->membase + AUART_CTRL2);
>>>         ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
>>> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port
>>> *u, unsigned mctrl)
>>>         }
>>>         writel(ctrl, u->membase + AUART_CTRL2);
>>> +
>>> +       mctrl_gpio_set(s->gpios, mctrl);
>>>   }
>>>     static u32 mxs_auart_get_mctrl(struct uart_port *u)
>>>   {
>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>>         u32 stat = readl(u->membase + AUART_STAT);
>>>         u32 mctrl = 0;
>>>         if (stat & AUART_STAT_CTS)
>>>                 mctrl |= TIOCM_CTS;
>>>   -     return mctrl;
>>> +       return mctrl_gpio_get(s->gpios, &mctrl);
>>>   }
>>>     static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
>>> @@ -554,6 +564,10 @@ err_out:
>>>     }
>>>   +#define RTS_AT_AUART()
>>> IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>> +                                                       UART_GPIO_RTS))
>>> +#define CTS_AT_AUART() IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>> +                                                       UART_GPIO_CTS))
>>>   static void mxs_auart_settermios(struct uart_port *u,
>>>                                  struct ktermios *termios,
>>>                                  struct ktermios *old)
>>> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>>>                 ctrl |= AUART_LINECTRL_STP2;
>>>         /* figure out the hardware flow control settings */
>>> +       ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>>         if (cflag & CRTSCTS) {
>>>                 /*
>>>                  * The DMA has a bug(see errata:2836) in mx23.
>>> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port
>>> *u,
>>>                                 ctrl2 |= AUART_CTRL2_TXDMAE |
>>> AUART_CTRL2_RXDMAE
>>>                                        | AUART_CTRL2_DMAONERR;
>>>                 }
>>> -               ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
>>> -       } else {
>>> -               ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>> +               /* Even if RTS is GPIO line RTSEN can be enabled because
>>> +                * the pinctrl configuration decides about RTS pin
>>> function */
>>> +               ctrl2 |= AUART_CTRL2_RTSEN;
>>> +               if (CTS_AT_AUART())
>>> +                       ctrl2 |= AUART_CTRL2_CTSEN;
>>>         }
>>>         /* set baud rate */
>>> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void
>>> *context)
>>>                         s->port.membase + AUART_INTR_CLR);
>>>         if (istat & AUART_INTR_CTSMIS) {
>>> -               uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
>>> +               if (CTS_AT_AUART())
>>> +                       uart_handle_cts_change(&s->port,
>>> +                                       stat & AUART_STAT_CTS);
>>>                 writel(AUART_INTR_CTSMIS,
>>>                                 s->port.membase + AUART_INTR_CLR);
>>>                 istat &= ~AUART_INTR_CTSMIS;
>>> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct
>>> mxs_auart_port *s,
>>>         return 0;
>>>   }
>>>   +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct
>>> device *dev)
>>> +{
>>> +       s->gpios = mctrl_gpio_init(dev, 0);
>>> +       if (IS_ERR_OR_NULL(s->gpios))
>>> +               return false;
>>> +
>>> +       /* Block (enabled before) DMA option if RTS or CTS is GPIO line
>>> */
>>
>> This is confusing, so I think some more comments won't be too much.
>> Something like:
>> /*
>>   * The DMA only work if the lines CTS/RTS are handled by the controller
>>   * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
>> valid
>>   * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
>> bit should be
>>   * cleaned to indicate that the RTS/CTS lines are not handled by the
>> controller.
>>   */
>> (If I understood correctly what is done here.)
>
>
> This is not clear in the driver. If hardware RTS/CTS (non-gpio) is not
> defined in DT
> only DMA is not used. It is still possible to use hardware RTS/CTS lines
> if you set hw-flowcontrol in termios. So it is also possible to mix hardware
> RTS/CTS
> and gpio RTS/CTS. In mixed or gpio case the code blocks/disables DMA.
> I know my comment is not clear but the reason is in the earlier code.
> Can you propose better solution?
Maybe Huang Shijie knows more on this DMA vs RTS/CTS incompatibility.
[added Huang Shijie in Cc ]

> After review and small fixes requested new version is required for a
> patchset
> or rather patch resend?
It's better to resend the whole patchset (after giving some time for
Huang Shijie to give its thoughts )

>
> kind regards
> Janusz
>
>
>>> +       if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
>>> +               if (test_bit(MXS_AUART_RTSCTS, &s->flags))
>>> +                       dev_warn(dev,
>>> +                                "DMA and flow control via gpio may cause
>>> some problems. DMA disabled!\n");
>>> +               clear_bit(MXS_AUART_RTSCTS, &s->flags);
>>> +       }
>>> +
>>> +       return true;
>>> +}
>>> +
>>>   static int mxs_auart_probe(struct platform_device *pdev)
>>>   {
>>>         const struct of_device_id *of_id =
>>> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device
>>> *pdev)
>>>         platform_set_drvdata(pdev, s);
>>>   +     if (!mxs_auart_init_gpios(s, &pdev->dev))
>>> +               dev_err(&pdev->dev,
>>> +                       "Failed to initialize GPIOs. The serial port may
>>> not work as expected\n");
>>> +
>>>         auart_port[s->port.line] = s;
>>>         mxs_auart_reset(&s->port);
>>>
>> Reviewed-by: Richard Genoud <richard.genoud@gmail.com>
>
>



-- 
for me, ck means con kolivas and not calvin klein... does it mean I'm a geek ?

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-10-31  8:48         ` Richard Genoud
@ 2014-11-06 11:13           ` Janusz Użycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-06 11:13 UTC (permalink / raw)
  To: Richard Genoud, Huang Shijie
  Cc: Greg Kroah-Hartman, Russell King - ARM Linux, Jiri Slaby,
	Fabio Estevam, linux-serial, devicetree, linux-arm-kernel

Any news?

best regards
Janusz

W dniu 2014-10-31 o 09:48, Richard Genoud pisze:
> 2014-10-24 17:51 GMT+02:00 Janusz Użycki <j.uzycki@elproma.com.pl>:
>> W dniu 2014-10-24 o 17:32, Richard Genoud pisze:
>>
>>> On 10/10/2014 18:53, Janusz Uzycki wrote:
>>>> Dedicated CTS and RTS pins are unusable together with a lot of other
>>>> peripherals because they share the same line. Pinctrl is limited.
>>>>
>>>> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
>>>> so we have to control them via GPIO.
>>>>
>>>> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>>>> signals.
>>>>
>>>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
>>>> ---
>>>> v3 -> v4 changelog:
>>>> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
>>>> * DMA engine disabled if RTS or CTS is GPIO line
>>>> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
>>>> * CTSEN can't be enabled for hardware flow control block
>>>>     if CTS is defined as GPIO line
>>>> * RTSEN can be enabled for hardware flow control block
>>>>     even if RTS is defined as GPIO line.
>>>>     RTS pin depends on pinctrl configuration which
>>>>     selects RTS output from hardware flow control block or GPIO line.
>>>> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
>>>> * mxs_auart_irq_handle(): CTS_AT_AUART() used
>>>> * mxs_auart_init_gpios() returns true(success)/false(failure)
>>>> * dev_err() message fixed in mxs_auart_probe()
>>>>
>>>> v2 -> v3 changelog:
>>>> * mctrl_gpio_free() removed to simplify:
>>>>     mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>>>>     mxs_auart_remove() because mctrl_gpio_init() does all
>>>>     allocations with devm_* functions.
>>>>     (see Documentation/serial/driver since kernel 3.16)
>>>> * DMA on HW flow control comment updated, still not sure about the
>>>> comment
>>>> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>>>>     mctrl_gpio_get() does not clear gpio interrupt pendings like
>>>>     8250_core.c does with MSR.
>>>> * mxs_auart_modem_status() moved to [3/4]
>>>>     If enable_ms() is not called, uart_handle_cts_change()
>>>>     shouldn't be called.
>>>>
>>>> ---
>>>>    .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>>>>    drivers/tty/serial/Kconfig           |  1 +
>>>>    drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>>>>    3 files changed, 55 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> index 59a40f1..7c408c8 100644
>>>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> @@ -11,8 +11,13 @@ Required properties:
>>>>    - dma-names: "rx" for RX channel, "tx" for TX channel.
>>>>      Optional properties:
>>>> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
>>>> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
>>>> +  for hardware flow control,
>>>>          it also means you enable the DMA support for this UART.
>>>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for
>>>> RTS/CTS/DTR/DSR/RI/DCD
>>>> +  line respectively. It will use specified PIO instead of the peripheral
>>>> +  function pin for the USART feature.
>>>> +  If unsure, don't specify this property.
>>>>      Example:
>>>>    auart0: serial@8006a000 {
>>>> @@ -21,6 +26,9 @@ auart0: serial@8006a000 {
>>>>          interrupts = <112>;
>>>>          dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>>>>          dma-names = "rx", "tx";
>>>> +       cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>>>> +       dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>>>> +       dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>>>    };
>>>>      Note: Each auart port should have an alias correctly numbered in
>>>> "aliases"
>>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>>> index 4fe8ca1..90e8516 100644
>>>> --- a/drivers/tty/serial/Kconfig
>>>> +++ b/drivers/tty/serial/Kconfig
>>>> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>>>>          depends on ARCH_MXS
>>>>          tristate "MXS AUART support"
>>>>          select SERIAL_CORE
>>>> +       select SERIAL_MCTRL_GPIO if GPIOLIB
>>>>          help
>>>>            This driver supports the MXS Application UART (AUART) port.
>>>>    diff --git a/drivers/tty/serial/mxs-auart.c
>>>> b/drivers/tty/serial/mxs-auart.c
>>>> index 2d49901..8bbcfd1 100644
>>>> --- a/drivers/tty/serial/mxs-auart.c
>>>> +++ b/drivers/tty/serial/mxs-auart.c
>>>> @@ -42,6 +42,9 @@
>>>>      #include <asm/cacheflush.h>
>>>>    +#include <linux/err.h>
>>>> +#include "serial_mctrl_gpio.h"
>>>> +
>>>>    #define MXS_AUART_PORTS 5
>>>>    #define MXS_AUART_FIFO_SIZE           16
>>>>    @@ -158,6 +161,8 @@ struct mxs_auart_port {
>>>>          struct scatterlist rx_sgl;
>>>>          struct dma_chan *rx_dma_chan;
>>>>          void *rx_dma_buf;
>>>> +
>>>> +       struct mctrl_gpios      *gpios;
>>>>    };
>>>>      static struct platform_device_id mxs_auart_devtype[] = {
>>>> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port
>>>> *u)
>>>>      static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>>>    {
>>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>>> +
>>>>          u32 ctrl = readl(u->membase + AUART_CTRL2);
>>>>          ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
>>>> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port
>>>> *u, unsigned mctrl)
>>>>          }
>>>>          writel(ctrl, u->membase + AUART_CTRL2);
>>>> +
>>>> +       mctrl_gpio_set(s->gpios, mctrl);
>>>>    }
>>>>      static u32 mxs_auart_get_mctrl(struct uart_port *u)
>>>>    {
>>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>>>          u32 stat = readl(u->membase + AUART_STAT);
>>>>          u32 mctrl = 0;
>>>>          if (stat & AUART_STAT_CTS)
>>>>                  mctrl |= TIOCM_CTS;
>>>>    -     return mctrl;
>>>> +       return mctrl_gpio_get(s->gpios, &mctrl);
>>>>    }
>>>>      static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
>>>> @@ -554,6 +564,10 @@ err_out:
>>>>      }
>>>>    +#define RTS_AT_AUART()
>>>> IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>>> +                                                       UART_GPIO_RTS))
>>>> +#define CTS_AT_AUART() IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>>> +                                                       UART_GPIO_CTS))
>>>>    static void mxs_auart_settermios(struct uart_port *u,
>>>>                                   struct ktermios *termios,
>>>>                                   struct ktermios *old)
>>>> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>>>>                  ctrl |= AUART_LINECTRL_STP2;
>>>>          /* figure out the hardware flow control settings */
>>>> +       ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>>>          if (cflag & CRTSCTS) {
>>>>                  /*
>>>>                   * The DMA has a bug(see errata:2836) in mx23.
>>>> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port
>>>> *u,
>>>>                                  ctrl2 |= AUART_CTRL2_TXDMAE |
>>>> AUART_CTRL2_RXDMAE
>>>>                                         | AUART_CTRL2_DMAONERR;
>>>>                  }
>>>> -               ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
>>>> -       } else {
>>>> -               ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>>> +               /* Even if RTS is GPIO line RTSEN can be enabled because
>>>> +                * the pinctrl configuration decides about RTS pin
>>>> function */
>>>> +               ctrl2 |= AUART_CTRL2_RTSEN;
>>>> +               if (CTS_AT_AUART())
>>>> +                       ctrl2 |= AUART_CTRL2_CTSEN;
>>>>          }
>>>>          /* set baud rate */
>>>> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void
>>>> *context)
>>>>                          s->port.membase + AUART_INTR_CLR);
>>>>          if (istat & AUART_INTR_CTSMIS) {
>>>> -               uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
>>>> +               if (CTS_AT_AUART())
>>>> +                       uart_handle_cts_change(&s->port,
>>>> +                                       stat & AUART_STAT_CTS);
>>>>                  writel(AUART_INTR_CTSMIS,
>>>>                                  s->port.membase + AUART_INTR_CLR);
>>>>                  istat &= ~AUART_INTR_CTSMIS;
>>>> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct
>>>> mxs_auart_port *s,
>>>>          return 0;
>>>>    }
>>>>    +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct
>>>> device *dev)
>>>> +{
>>>> +       s->gpios = mctrl_gpio_init(dev, 0);
>>>> +       if (IS_ERR_OR_NULL(s->gpios))
>>>> +               return false;
>>>> +
>>>> +       /* Block (enabled before) DMA option if RTS or CTS is GPIO line
>>>> */
>>> This is confusing, so I think some more comments won't be too much.
>>> Something like:
>>> /*
>>>    * The DMA only work if the lines CTS/RTS are handled by the controller
>>>    * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
>>> valid
>>>    * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
>>> bit should be
>>>    * cleaned to indicate that the RTS/CTS lines are not handled by the
>>> controller.
>>>    */
>>> (If I understood correctly what is done here.)
>>
>> This is not clear in the driver. If hardware RTS/CTS (non-gpio) is not
>> defined in DT
>> only DMA is not used. It is still possible to use hardware RTS/CTS lines
>> if you set hw-flowcontrol in termios. So it is also possible to mix hardware
>> RTS/CTS
>> and gpio RTS/CTS. In mixed or gpio case the code blocks/disables DMA.
>> I know my comment is not clear but the reason is in the earlier code.
>> Can you propose better solution?
> Maybe Huang Shijie knows more on this DMA vs RTS/CTS incompatibility.
> [added Huang Shijie in Cc ]
>
>> After review and small fixes requested new version is required for a
>> patchset
>> or rather patch resend?
> It's better to resend the whole patchset (after giving some time for
> Huang Shijie to give its thoughts )
>
>> kind regards
>> Janusz
>>
>>
>>>> +       if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
>>>> +               if (test_bit(MXS_AUART_RTSCTS, &s->flags))
>>>> +                       dev_warn(dev,
>>>> +                                "DMA and flow control via gpio may cause
>>>> some problems. DMA disabled!\n");
>>>> +               clear_bit(MXS_AUART_RTSCTS, &s->flags);
>>>> +       }
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>>    static int mxs_auart_probe(struct platform_device *pdev)
>>>>    {
>>>>          const struct of_device_id *of_id =
>>>> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device
>>>> *pdev)
>>>>          platform_set_drvdata(pdev, s);
>>>>    +     if (!mxs_auart_init_gpios(s, &pdev->dev))
>>>> +               dev_err(&pdev->dev,
>>>> +                       "Failed to initialize GPIOs. The serial port may
>>>> not work as expected\n");
>>>> +
>>>>          auart_port[s->port.line] = s;
>>>>          mxs_auart_reset(&s->port);
>>>>
>>> Reviewed-by: Richard Genoud <richard.genoud@gmail.com>
>>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-06 11:13           ` Janusz Użycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-06 11:13 UTC (permalink / raw)
  To: linux-arm-kernel

Any news?

best regards
Janusz

W dniu 2014-10-31 o 09:48, Richard Genoud pisze:
> 2014-10-24 17:51 GMT+02:00 Janusz U?ycki <j.uzycki@elproma.com.pl>:
>> W dniu 2014-10-24 o 17:32, Richard Genoud pisze:
>>
>>> On 10/10/2014 18:53, Janusz Uzycki wrote:
>>>> Dedicated CTS and RTS pins are unusable together with a lot of other
>>>> peripherals because they share the same line. Pinctrl is limited.
>>>>
>>>> Moreover, the AUART controller doesn't handle DTR/DSR/DCD/RI signals,
>>>> so we have to control them via GPIO.
>>>>
>>>> This patch permits to use GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>>>> signals.
>>>>
>>>> Signed-off-by: Janusz Uzycki <j.uzycki@elproma.com.pl>
>>>> ---
>>>> v3 -> v4 changelog:
>>>> * RTS_AT_AUART() and CTS_AT_AUART() macro defined
>>>> * DMA engine disabled if RTS or CTS is GPIO line
>>>> * warn if fsl,uart-has-rtscts and RTS/CTS via GPIO are mixed
>>>> * CTSEN can't be enabled for hardware flow control block
>>>>     if CTS is defined as GPIO line
>>>> * RTSEN can be enabled for hardware flow control block
>>>>     even if RTS is defined as GPIO line.
>>>>     RTS pin depends on pinctrl configuration which
>>>>     selects RTS output from hardware flow control block or GPIO line.
>>>> * mxs_auart_settermios(): RTS_AT_AUART() and CTS_AT_AUART() used
>>>> * mxs_auart_irq_handle(): CTS_AT_AUART() used
>>>> * mxs_auart_init_gpios() returns true(success)/false(failure)
>>>> * dev_err() message fixed in mxs_auart_probe()
>>>>
>>>> v2 -> v3 changelog:
>>>> * mctrl_gpio_free() removed to simplify:
>>>>     mctrl_gpio_free() is not necessary in mxs_auart_probe() and
>>>>     mxs_auart_remove() because mctrl_gpio_init() does all
>>>>     allocations with devm_* functions.
>>>>     (see Documentation/serial/driver since kernel 3.16)
>>>> * DMA on HW flow control comment updated, still not sure about the
>>>> comment
>>>> * mxs_auart_modem_status() removed from mxs_auart_get_mctrl():
>>>>     mctrl_gpio_get() does not clear gpio interrupt pendings like
>>>>     8250_core.c does with MSR.
>>>> * mxs_auart_modem_status() moved to [3/4]
>>>>     If enable_ms() is not called, uart_handle_cts_change()
>>>>     shouldn't be called.
>>>>
>>>> ---
>>>>    .../devicetree/bindings/serial/fsl-mxs-auart.txt   | 10 ++++-
>>>>    drivers/tty/serial/Kconfig           |  1 +
>>>>    drivers/tty/serial/mxs-auart.c       | 50 +++++++++++++++++++---
>>>>    3 files changed, 55 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> index 59a40f1..7c408c8 100644
>>>> --- a/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> +++ b/Documentation/devicetree/bindings/serial/fsl-mxs-auart.txt
>>>> @@ -11,8 +11,13 @@ Required properties:
>>>>    - dma-names: "rx" for RX channel, "tx" for TX channel.
>>>>      Optional properties:
>>>> -- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines,
>>>> +- fsl,uart-has-rtscts : Indicate the UART has RTS and CTS lines
>>>> +  for hardware flow control,
>>>>          it also means you enable the DMA support for this UART.
>>>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for
>>>> RTS/CTS/DTR/DSR/RI/DCD
>>>> +  line respectively. It will use specified PIO instead of the peripheral
>>>> +  function pin for the USART feature.
>>>> +  If unsure, don't specify this property.
>>>>      Example:
>>>>    auart0: serial at 8006a000 {
>>>> @@ -21,6 +26,9 @@ auart0: serial at 8006a000 {
>>>>          interrupts = <112>;
>>>>          dmas = <&dma_apbx 8>, <&dma_apbx 9>;
>>>>          dma-names = "rx", "tx";
>>>> +       cts-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>>>> +       dsr-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
>>>> +       dcd-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>>>    };
>>>>      Note: Each auart port should have an alias correctly numbered in
>>>> "aliases"
>>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>>> index 4fe8ca1..90e8516 100644
>>>> --- a/drivers/tty/serial/Kconfig
>>>> +++ b/drivers/tty/serial/Kconfig
>>>> @@ -1357,6 +1357,7 @@ config SERIAL_MXS_AUART
>>>>          depends on ARCH_MXS
>>>>          tristate "MXS AUART support"
>>>>          select SERIAL_CORE
>>>> +       select SERIAL_MCTRL_GPIO if GPIOLIB
>>>>          help
>>>>            This driver supports the MXS Application UART (AUART) port.
>>>>    diff --git a/drivers/tty/serial/mxs-auart.c
>>>> b/drivers/tty/serial/mxs-auart.c
>>>> index 2d49901..8bbcfd1 100644
>>>> --- a/drivers/tty/serial/mxs-auart.c
>>>> +++ b/drivers/tty/serial/mxs-auart.c
>>>> @@ -42,6 +42,9 @@
>>>>      #include <asm/cacheflush.h>
>>>>    +#include <linux/err.h>
>>>> +#include "serial_mctrl_gpio.h"
>>>> +
>>>>    #define MXS_AUART_PORTS 5
>>>>    #define MXS_AUART_FIFO_SIZE           16
>>>>    @@ -158,6 +161,8 @@ struct mxs_auart_port {
>>>>          struct scatterlist rx_sgl;
>>>>          struct dma_chan *rx_dma_chan;
>>>>          void *rx_dma_buf;
>>>> +
>>>> +       struct mctrl_gpios      *gpios;
>>>>    };
>>>>      static struct platform_device_id mxs_auart_devtype[] = {
>>>> @@ -405,6 +410,8 @@ static void mxs_auart_release_port(struct uart_port
>>>> *u)
>>>>      static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl)
>>>>    {
>>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>>> +
>>>>          u32 ctrl = readl(u->membase + AUART_CTRL2);
>>>>          ctrl &= ~(AUART_CTRL2_RTSEN | AUART_CTRL2_RTS);
>>>> @@ -416,17 +423,20 @@ static void mxs_auart_set_mctrl(struct uart_port
>>>> *u, unsigned mctrl)
>>>>          }
>>>>          writel(ctrl, u->membase + AUART_CTRL2);
>>>> +
>>>> +       mctrl_gpio_set(s->gpios, mctrl);
>>>>    }
>>>>      static u32 mxs_auart_get_mctrl(struct uart_port *u)
>>>>    {
>>>> +       struct mxs_auart_port *s = to_auart_port(u);
>>>>          u32 stat = readl(u->membase + AUART_STAT);
>>>>          u32 mctrl = 0;
>>>>          if (stat & AUART_STAT_CTS)
>>>>                  mctrl |= TIOCM_CTS;
>>>>    -     return mctrl;
>>>> +       return mctrl_gpio_get(s->gpios, &mctrl);
>>>>    }
>>>>      static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s);
>>>> @@ -554,6 +564,10 @@ err_out:
>>>>      }
>>>>    +#define RTS_AT_AUART()
>>>> IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>>> +                                                       UART_GPIO_RTS))
>>>> +#define CTS_AT_AUART() IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios,    \
>>>> +                                                       UART_GPIO_CTS))
>>>>    static void mxs_auart_settermios(struct uart_port *u,
>>>>                                   struct ktermios *termios,
>>>>                                   struct ktermios *old)
>>>> @@ -630,6 +644,7 @@ static void mxs_auart_settermios(struct uart_port *u,
>>>>                  ctrl |= AUART_LINECTRL_STP2;
>>>>          /* figure out the hardware flow control settings */
>>>> +       ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>>>          if (cflag & CRTSCTS) {
>>>>                  /*
>>>>                   * The DMA has a bug(see errata:2836) in mx23.
>>>> @@ -644,9 +659,11 @@ static void mxs_auart_settermios(struct uart_port
>>>> *u,
>>>>                                  ctrl2 |= AUART_CTRL2_TXDMAE |
>>>> AUART_CTRL2_RXDMAE
>>>>                                         | AUART_CTRL2_DMAONERR;
>>>>                  }
>>>> -               ctrl2 |= AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN;
>>>> -       } else {
>>>> -               ctrl2 &= ~(AUART_CTRL2_CTSEN | AUART_CTRL2_RTSEN);
>>>> +               /* Even if RTS is GPIO line RTSEN can be enabled because
>>>> +                * the pinctrl configuration decides about RTS pin
>>>> function */
>>>> +               ctrl2 |= AUART_CTRL2_RTSEN;
>>>> +               if (CTS_AT_AUART())
>>>> +                       ctrl2 |= AUART_CTRL2_CTSEN;
>>>>          }
>>>>          /* set baud rate */
>>>> @@ -690,7 +707,9 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void
>>>> *context)
>>>>                          s->port.membase + AUART_INTR_CLR);
>>>>          if (istat & AUART_INTR_CTSMIS) {
>>>> -               uart_handle_cts_change(&s->port, stat & AUART_STAT_CTS);
>>>> +               if (CTS_AT_AUART())
>>>> +                       uart_handle_cts_change(&s->port,
>>>> +                                       stat & AUART_STAT_CTS);
>>>>                  writel(AUART_INTR_CTSMIS,
>>>>                                  s->port.membase + AUART_INTR_CLR);
>>>>                  istat &= ~AUART_INTR_CTSMIS;
>>>> @@ -1014,6 +1033,23 @@ static int serial_mxs_probe_dt(struct
>>>> mxs_auart_port *s,
>>>>          return 0;
>>>>    }
>>>>    +static bool mxs_auart_init_gpios(struct mxs_auart_port *s, struct
>>>> device *dev)
>>>> +{
>>>> +       s->gpios = mctrl_gpio_init(dev, 0);
>>>> +       if (IS_ERR_OR_NULL(s->gpios))
>>>> +               return false;
>>>> +
>>>> +       /* Block (enabled before) DMA option if RTS or CTS is GPIO line
>>>> */
>>> This is confusing, so I think some more comments won't be too much.
>>> Something like:
>>> /*
>>>    * The DMA only work if the lines CTS/RTS are handled by the controller
>>>    * cf. 8418e67d9523 serial: mxs: enable the DMA only when the RTS/CTS is
>>> valid
>>>    * So, If the CTS/RTS lines are controlled via GPIO the MXS_AUART_RTSCTS
>>> bit should be
>>>    * cleaned to indicate that the RTS/CTS lines are not handled by the
>>> controller.
>>>    */
>>> (If I understood correctly what is done here.)
>>
>> This is not clear in the driver. If hardware RTS/CTS (non-gpio) is not
>> defined in DT
>> only DMA is not used. It is still possible to use hardware RTS/CTS lines
>> if you set hw-flowcontrol in termios. So it is also possible to mix hardware
>> RTS/CTS
>> and gpio RTS/CTS. In mixed or gpio case the code blocks/disables DMA.
>> I know my comment is not clear but the reason is in the earlier code.
>> Can you propose better solution?
> Maybe Huang Shijie knows more on this DMA vs RTS/CTS incompatibility.
> [added Huang Shijie in Cc ]
>
>> After review and small fixes requested new version is required for a
>> patchset
>> or rather patch resend?
> It's better to resend the whole patchset (after giving some time for
> Huang Shijie to give its thoughts )
>
>> kind regards
>> Janusz
>>
>>
>>>> +       if (!RTS_AT_AUART() || !CTS_AT_AUART()) {
>>>> +               if (test_bit(MXS_AUART_RTSCTS, &s->flags))
>>>> +                       dev_warn(dev,
>>>> +                                "DMA and flow control via gpio may cause
>>>> some problems. DMA disabled!\n");
>>>> +               clear_bit(MXS_AUART_RTSCTS, &s->flags);
>>>> +       }
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>>    static int mxs_auart_probe(struct platform_device *pdev)
>>>>    {
>>>>          const struct of_device_id *of_id =
>>>> @@ -1069,6 +1105,10 @@ static int mxs_auart_probe(struct platform_device
>>>> *pdev)
>>>>          platform_set_drvdata(pdev, s);
>>>>    +     if (!mxs_auart_init_gpios(s, &pdev->dev))
>>>> +               dev_err(&pdev->dev,
>>>> +                       "Failed to initialize GPIOs. The serial port may
>>>> not work as expected\n");
>>>> +
>>>>          auart_port[s->port.line] = s;
>>>>          mxs_auart_reset(&s->port);
>>>>
>>> Reviewed-by: Richard Genoud <richard.genoud@gmail.com>
>>
>
>

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
       [not found]           ` <CAMiH66FKJm8hcgtR=-ZzzpCq+PQ8xkixbUnMzRPVd_cM_6VM1w@mail.gmail.com>
@ 2014-11-07  8:03               ` Marek Vasut
  0 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-07  8:03 UTC (permalink / raw)
  To: Huang Shijie
  Cc: Janusz Użycki, Richard Genoud, Greg Kroah-Hartman,
	Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel

On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
> why change them to gpio?  If we change them to gpio.  Could the DMA still
> works fine?
> did you test the DMA with this patch?
> 
> Add Marek for this patch too.

I didn't look too deep into the patch, so here's just my experience:

1) The AUART block signals and GPIO block signals are not sychronised using the 
   same clock. Therefore, the latency between toggling of the AUART lines and
   the GPIO-driven pins will not be deterministic and will vary. There might be
   a way to approximate that, but that's definitelly not a reliable solution.

   This is very bad for example if you drive RS485 DIR line with the RTS pin as
   a GPIO ; the RTS pin will toggle at non-deterministic time compared to the
   end of UART transmission. This will trigger bit-loss on the RS485 line and
   you just don't want that.

2) Speaking of RS485, there's [1] and [2]. which I believe apply to any combo
   of UART+GPIO toggling.

So I hate to bring the bad news , but UART+GPIO combo toggling is really a bad
bad idea.

HTH

[1] http://comments.gmane.org/gmane.linux.serial/6770
[2] http://article.gmane.org/gmane.linux.serial/3619/

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-07  8:03               ` Marek Vasut
  0 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-07  8:03 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
> why change them to gpio?  If we change them to gpio.  Could the DMA still
> works fine?
> did you test the DMA with this patch?
> 
> Add Marek for this patch too.

I didn't look too deep into the patch, so here's just my experience:

1) The AUART block signals and GPIO block signals are not sychronised using the 
   same clock. Therefore, the latency between toggling of the AUART lines and
   the GPIO-driven pins will not be deterministic and will vary. There might be
   a way to approximate that, but that's definitelly not a reliable solution.

   This is very bad for example if you drive RS485 DIR line with the RTS pin as
   a GPIO ; the RTS pin will toggle at non-deterministic time compared to the
   end of UART transmission. This will trigger bit-loss on the RS485 line and
   you just don't want that.

2) Speaking of RS485, there's [1] and [2]. which I believe apply to any combo
   of UART+GPIO toggling.

So I hate to bring the bad news , but UART+GPIO combo toggling is really a bad
bad idea.

HTH

[1] http://comments.gmane.org/gmane.linux.serial/6770
[2] http://article.gmane.org/gmane.linux.serial/3619/

Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-11-07  8:03               ` Marek Vasut
@ 2014-11-07 10:04                 ` Janusz Użycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-07 10:04 UTC (permalink / raw)
  To: Marek Vasut, Huang Shijie
  Cc: Richard Genoud, Greg Kroah-Hartman, Russell King - ARM Linux,
	Jiri Slaby, Fabio Estevam, linux-serial, devicetree,
	linux-arm-kernel


W dniu 2014-11-07 o 09:03, Marek Vasut pisze:
> On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
>> why change them to gpio?

Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
in order to obtain as much uarts as possible using i.mx283.
Therefore gpios can be used for "hardware" flow control.

>>    If we change them to gpio.  Could the DMA still
>> works fine?
>> did you test the DMA with this patch?
>>
>> Add Marek for this patch too.
> I didn't look too deep into the patch, so here's just my experience:
>
> 1) The AUART block signals and GPIO block signals are not sychronised using the
>     same clock. Therefore, the latency between toggling of the AUART lines and
>     the GPIO-driven pins will not be deterministic and will vary. There might be
>     a way to approximate that, but that's definitelly not a reliable solution.
>
>     This is very bad for example if you drive RS485 DIR line with the RTS pin as
>     a GPIO ; the RTS pin will toggle at non-deterministic time compared to the
>     end of UART transmission. This will trigger bit-loss on the RS485 line and
>     you just don't want that.
>
> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any combo
>     of UART+GPIO toggling.
>
> So I hate to bring the bad news , but UART+GPIO combo toggling is really a bad
> bad idea.

Unfortunately if hardware is limited there is no choice and UART+GPIO is 
necessary.

Your experience confirms the discussion [3] with Russell King. DMA 
should be disabled and
the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS 
line is set as gpio.
So I didn't test the patch with the DMA - I've just disabled it.

The question is different. The driver supports the cases for RTS/CTS:
1)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
      DT sets fsl,uart-has-rtscts)
     a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabled
     b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS 
controlled by get/set_mctrl()
2)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
      DT doesn't set fsl,uart-has-rtscts)
     a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
     b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS 
controlled by get/set_mctrl()

and after the patch it is more complex (because of mixed cases):
3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts):
     the patch cancels fsl,uart-has-rtscts flag to disable DMA support,
     settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controlled 
by get/set_mctrl(),
     both lines by gpios. In addition:
     a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is also 
controlled but
         the case assumes it is not selected by pinmux in DT
     b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also 
enabled but
         the case assumes it is not selected by pinmux in DT
4) RTS assigned to hardware AUART (pinmux configured by DT, 
fsl,uart-has-rtscts set or not),
     CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to 
disable DMA support,
     a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio) 
read by get_mctrl(),
         RTS of hardware AUART controlled by set_mctrl()
     b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read by 
get_mctrl(),
        auto RTS is enabled
    So case 4) is exactly 3) case with just different pinmux 
configuration and the patch
    threats 3) and 4) as the same case.
5) CTS assigned to hardware AUART (pinmux configured by DT, 
fsl,uart-has-rtscts set or not),
     RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to 
disable DMA support,
     a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardware 
AUART read by
         get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in 
addition RTS of hardware AUART
         is also controlled but the case assumes it is not selected by 
pinmux in DT
     b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled but
         RTS is controlled by set_mctrl() as gpio because the case 
assumes it is not selected
         by pinmux in DT

It is not nice description but I hadn't idea how to write it more clear.
The mixed cases 4) and 5) are likely not so useful but possible and not 
so expensive
to support.
Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
do you agree? It rather should include "dma" word in the name. Any 
suggestion?

[3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077

best regards
Janusz

> HTH
>
> [1] http://comments.gmane.org/gmane.linux.serial/6770
> [2] http://article.gmane.org/gmane.linux.serial/3619/
>
> Best regards,
> Marek Vasut


^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-07 10:04                 ` Janusz Użycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-07 10:04 UTC (permalink / raw)
  To: linux-arm-kernel


W dniu 2014-11-07 o 09:03, Marek Vasut pisze:
> On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
>> why change them to gpio?

Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
in order to obtain as much uarts as possible using i.mx283.
Therefore gpios can be used for "hardware" flow control.

>>    If we change them to gpio.  Could the DMA still
>> works fine?
>> did you test the DMA with this patch?
>>
>> Add Marek for this patch too.
> I didn't look too deep into the patch, so here's just my experience:
>
> 1) The AUART block signals and GPIO block signals are not sychronised using the
>     same clock. Therefore, the latency between toggling of the AUART lines and
>     the GPIO-driven pins will not be deterministic and will vary. There might be
>     a way to approximate that, but that's definitelly not a reliable solution.
>
>     This is very bad for example if you drive RS485 DIR line with the RTS pin as
>     a GPIO ; the RTS pin will toggle at non-deterministic time compared to the
>     end of UART transmission. This will trigger bit-loss on the RS485 line and
>     you just don't want that.
>
> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any combo
>     of UART+GPIO toggling.
>
> So I hate to bring the bad news , but UART+GPIO combo toggling is really a bad
> bad idea.

Unfortunately if hardware is limited there is no choice and UART+GPIO is 
necessary.

Your experience confirms the discussion [3] with Russell King. DMA 
should be disabled and
the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS 
line is set as gpio.
So I didn't test the patch with the DMA - I've just disabled it.

The question is different. The driver supports the cases for RTS/CTS:
1)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
      DT sets fsl,uart-has-rtscts)
     a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabled
     b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS 
controlled by get/set_mctrl()
2)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
      DT doesn't set fsl,uart-has-rtscts)
     a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
     b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS 
controlled by get/set_mctrl()

and after the patch it is more complex (because of mixed cases):
3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts):
     the patch cancels fsl,uart-has-rtscts flag to disable DMA support,
     settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controlled 
by get/set_mctrl(),
     both lines by gpios. In addition:
     a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is also 
controlled but
         the case assumes it is not selected by pinmux in DT
     b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also 
enabled but
         the case assumes it is not selected by pinmux in DT
4) RTS assigned to hardware AUART (pinmux configured by DT, 
fsl,uart-has-rtscts set or not),
     CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to 
disable DMA support,
     a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio) 
read by get_mctrl(),
         RTS of hardware AUART controlled by set_mctrl()
     b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read by 
get_mctrl(),
        auto RTS is enabled
    So case 4) is exactly 3) case with just different pinmux 
configuration and the patch
    threats 3) and 4) as the same case.
5) CTS assigned to hardware AUART (pinmux configured by DT, 
fsl,uart-has-rtscts set or not),
     RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to 
disable DMA support,
     a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardware 
AUART read by
         get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in 
addition RTS of hardware AUART
         is also controlled but the case assumes it is not selected by 
pinmux in DT
     b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled but
         RTS is controlled by set_mctrl() as gpio because the case 
assumes it is not selected
         by pinmux in DT

It is not nice description but I hadn't idea how to write it more clear.
The mixed cases 4) and 5) are likely not so useful but possible and not 
so expensive
to support.
Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
do you agree? It rather should include "dma" word in the name. Any 
suggestion?

[3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077

best regards
Janusz

> HTH
>
> [1] http://comments.gmane.org/gmane.linux.serial/6770
> [2] http://article.gmane.org/gmane.linux.serial/3619/
>
> Best regards,
> Marek Vasut

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-11-07 10:04                 ` Janusz Użycki
@ 2014-11-07 11:02                   ` Marek Vasut
  -1 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-07 11:02 UTC (permalink / raw)
  To: Janusz Użycki
  Cc: Huang Shijie, Richard Genoud, Greg Kroah-Hartman,
	Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel, Alexandre Belloni

On Friday, November 07, 2014 at 11:04:27 AM, Janusz Użycki wrote:
> W dniu 2014-11-07 o 09:03, Marek Vasut pisze:
> > On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
> >> why change them to gpio?

+CC Alexandre, since he might be interested :-)

> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
> in order to obtain as much uarts as possible using i.mx283.
> Therefore gpios can be used for "hardware" flow control.

Your logic is outright flawed here, the first sentence doesn't implicate the 
second sentence. In fact, those two are completely unrelated.

> >>    If we change them to gpio.  Could the DMA still
> >> 
> >> works fine?
> >> did you test the DMA with this patch?
> >> 
> >> Add Marek for this patch too.
> > 
> > I didn't look too deep into the patch, so here's just my experience:
> > 
> > 1) The AUART block signals and GPIO block signals are not sychronised
> > using the
> > 
> >     same clock. Therefore, the latency between toggling of the AUART
> >     lines and the GPIO-driven pins will not be deterministic and will
> >     vary. There might be a way to approximate that, but that's
> >     definitelly not a reliable solution.
> >     
> >     This is very bad for example if you drive RS485 DIR line with the RTS
> >     pin as a GPIO ; the RTS pin will toggle at non-deterministic time
> >     compared to the end of UART transmission. This will trigger bit-loss
> >     on the RS485 line and you just don't want that.
> > 
> > 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
> > combo
> > 
> >     of UART+GPIO toggling.
> > 
> > So I hate to bring the bad news , but UART+GPIO combo toggling is really
> > a bad bad idea.
> 
> Unfortunately if hardware is limited there is no choice and UART+GPIO is
> necessary.

You will run into timing problems (see above).

What you're proposing here is a workaround for broken hardware, which was proven
to be a bad idea and NAK'd already multiple times in the past (please see the 
links I posted in my last email).

> Your experience confirms the discussion [3] with Russell King. DMA
> should be disabled and
> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
> line is set as gpio.

DMA has nothing to do with those problems here. DMA can be safely ignored
for the purpose of the discussion altogether.

Like I explained already -- the problem is with the GPIO and AUART block not
being synchronised by the same clock. It is therefore impossible to predict
and control when the GPIO signals and the AUART signals will toggle in relation
to one another.

> So I didn't test the patch with the DMA - I've just disabled it.

This would break the driver for pretty much everyone using higher baud rates.

> The question is different. The driver supports the cases for RTS/CTS:
> 1)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>       DT sets fsl,uart-has-rtscts)
>      a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabled
>      b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
> controlled by get/set_mctrl()
> 2)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>       DT doesn't set fsl,uart-has-rtscts)
>      a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
>      b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
> controlled by get/set_mctrl()
> 
> and after the patch it is more complex (because of mixed cases):
> 3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts):
>      the patch cancels fsl,uart-has-rtscts flag to disable DMA support,
>      settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controlled
> by get/set_mctrl(),
>      both lines by gpios. In addition:
>      a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is also
> controlled but
>          the case assumes it is not selected by pinmux in DT
>      b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also
> enabled but
>          the case assumes it is not selected by pinmux in DT
> 4) RTS assigned to hardware AUART (pinmux configured by DT,
> fsl,uart-has-rtscts set or not),
>      CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
> disable DMA support,
>      a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio)
> read by get_mctrl(),
>          RTS of hardware AUART controlled by set_mctrl()
>      b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read by
> get_mctrl(),
>         auto RTS is enabled
>     So case 4) is exactly 3) case with just different pinmux
> configuration and the patch
>     threats 3) and 4) as the same case.
> 5) CTS assigned to hardware AUART (pinmux configured by DT,
> fsl,uart-has-rtscts set or not),
>      RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
> disable DMA support,
>      a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardware
> AUART read by
>          get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in
> addition RTS of hardware AUART
>          is also controlled but the case assumes it is not selected by
> pinmux in DT
>      b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
> but RTS is controlled by set_mctrl() as gpio because the case assumes it
> is not selected
>          by pinmux in DT
> 
> It is not nice description but I hadn't idea how to write it more clear.
> The mixed cases 4) and 5) are likely not so useful but possible and not
> so expensive
> to support.
> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
> do you agree? It rather should include "dma" word in the name. Any
> suggestion?
> 
> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077

The best suggestion I can give you is to fix your hardware early, before you
run into nasty deep s.....tuff. These workarounds do not work and they will
bit you later on, when it's hard to fix the hardware anymore.
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-07 11:02                   ` Marek Vasut
  0 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-07 11:02 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday, November 07, 2014 at 11:04:27 AM, Janusz U?ycki wrote:
> W dniu 2014-11-07 o 09:03, Marek Vasut pisze:
> > On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
> >> why change them to gpio?

+CC Alexandre, since he might be interested :-)

> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
> in order to obtain as much uarts as possible using i.mx283.
> Therefore gpios can be used for "hardware" flow control.

Your logic is outright flawed here, the first sentence doesn't implicate the 
second sentence. In fact, those two are completely unrelated.

> >>    If we change them to gpio.  Could the DMA still
> >> 
> >> works fine?
> >> did you test the DMA with this patch?
> >> 
> >> Add Marek for this patch too.
> > 
> > I didn't look too deep into the patch, so here's just my experience:
> > 
> > 1) The AUART block signals and GPIO block signals are not sychronised
> > using the
> > 
> >     same clock. Therefore, the latency between toggling of the AUART
> >     lines and the GPIO-driven pins will not be deterministic and will
> >     vary. There might be a way to approximate that, but that's
> >     definitelly not a reliable solution.
> >     
> >     This is very bad for example if you drive RS485 DIR line with the RTS
> >     pin as a GPIO ; the RTS pin will toggle at non-deterministic time
> >     compared to the end of UART transmission. This will trigger bit-loss
> >     on the RS485 line and you just don't want that.
> > 
> > 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
> > combo
> > 
> >     of UART+GPIO toggling.
> > 
> > So I hate to bring the bad news , but UART+GPIO combo toggling is really
> > a bad bad idea.
> 
> Unfortunately if hardware is limited there is no choice and UART+GPIO is
> necessary.

You will run into timing problems (see above).

What you're proposing here is a workaround for broken hardware, which was proven
to be a bad idea and NAK'd already multiple times in the past (please see the 
links I posted in my last email).

> Your experience confirms the discussion [3] with Russell King. DMA
> should be disabled and
> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
> line is set as gpio.

DMA has nothing to do with those problems here. DMA can be safely ignored
for the purpose of the discussion altogether.

Like I explained already -- the problem is with the GPIO and AUART block not
being synchronised by the same clock. It is therefore impossible to predict
and control when the GPIO signals and the AUART signals will toggle in relation
to one another.

> So I didn't test the patch with the DMA - I've just disabled it.

This would break the driver for pretty much everyone using higher baud rates.

> The question is different. The driver supports the cases for RTS/CTS:
> 1)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>       DT sets fsl,uart-has-rtscts)
>      a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabled
>      b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
> controlled by get/set_mctrl()
> 2)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>       DT doesn't set fsl,uart-has-rtscts)
>      a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
>      b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
> controlled by get/set_mctrl()
> 
> and after the patch it is more complex (because of mixed cases):
> 3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts):
>      the patch cancels fsl,uart-has-rtscts flag to disable DMA support,
>      settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controlled
> by get/set_mctrl(),
>      both lines by gpios. In addition:
>      a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is also
> controlled but
>          the case assumes it is not selected by pinmux in DT
>      b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also
> enabled but
>          the case assumes it is not selected by pinmux in DT
> 4) RTS assigned to hardware AUART (pinmux configured by DT,
> fsl,uart-has-rtscts set or not),
>      CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
> disable DMA support,
>      a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio)
> read by get_mctrl(),
>          RTS of hardware AUART controlled by set_mctrl()
>      b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read by
> get_mctrl(),
>         auto RTS is enabled
>     So case 4) is exactly 3) case with just different pinmux
> configuration and the patch
>     threats 3) and 4) as the same case.
> 5) CTS assigned to hardware AUART (pinmux configured by DT,
> fsl,uart-has-rtscts set or not),
>      RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
> disable DMA support,
>      a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardware
> AUART read by
>          get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in
> addition RTS of hardware AUART
>          is also controlled but the case assumes it is not selected by
> pinmux in DT
>      b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
> but RTS is controlled by set_mctrl() as gpio because the case assumes it
> is not selected
>          by pinmux in DT
> 
> It is not nice description but I hadn't idea how to write it more clear.
> The mixed cases 4) and 5) are likely not so useful but possible and not
> so expensive
> to support.
> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
> do you agree? It rather should include "dma" word in the name. Any
> suggestion?
> 
> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077

The best suggestion I can give you is to fix your hardware early, before you
run into nasty deep s.....tuff. These workarounds do not work and they will
bit you later on, when it's hard to fix the hardware anymore.

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-11-07 11:02                   ` Marek Vasut
@ 2014-11-07 13:23                     ` Janusz Użycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-07 13:23 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Huang Shijie, Richard Genoud, Greg Kroah-Hartman,
	Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel, Alexandre Belloni


W dniu 2014-11-07 o 12:02, Marek Vasut pisze:
> On Friday, November 07, 2014 at 11:04:27 AM, Janusz Użycki wrote:
>> W dniu 2014-11-07 o 09:03, Marek Vasut pisze:
>>> On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
>>>> why change them to gpio?
> +CC Alexandre, since he might be interested :-)

:)

>
>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
>> in order to obtain as much uarts as possible using i.mx283.
>> Therefore gpios can be used for "hardware" flow control.
> Your logic is outright flawed here, the first sentence doesn't implicate the
> second sentence. In fact, those two are completely unrelated.

I didn't write MUST but CAN. There is a choice. Is flexibility of the 
driver disadvantage?

>>>>     If we change them to gpio.  Could the DMA still
>>>>
>>>> works fine?
>>>> did you test the DMA with this patch?
>>>>
>>>> Add Marek for this patch too.
>>> I didn't look too deep into the patch, so here's just my experience:
>>>
>>> 1) The AUART block signals and GPIO block signals are not sychronised
>>> using the
>>>
>>>      same clock. Therefore, the latency between toggling of the AUART
>>>      lines and the GPIO-driven pins will not be deterministic and will
>>>      vary. There might be a way to approximate that, but that's
>>>      definitelly not a reliable solution.
>>>      
>>>      This is very bad for example if you drive RS485 DIR line with the RTS
>>>      pin as a GPIO ; the RTS pin will toggle at non-deterministic time
>>>      compared to the end of UART transmission. This will trigger bit-loss
>>>      on the RS485 line and you just don't want that.
>>>
>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
>>> combo
>>>
>>>      of UART+GPIO toggling.
>>>
>>> So I hate to bring the bad news , but UART+GPIO combo toggling is really
>>> a bad bad idea.
>> Unfortunately if hardware is limited there is no choice and UART+GPIO is
>> necessary.
> You will run into timing problems (see above).

A lot of 8250-compatible devices has no hardware flow control and in 
most cases
they works and it is enough even for 115200 speed if CTS is handled by irq.
So it depends on your needs.

>
> What you're proposing here is a workaround for broken hardware, which was proven
> to be a bad idea and NAK'd already multiple times in the past (please see the
> links I posted in my last email).

It is not broken  hardware - rather limited to lower speeds but still 
very useful solution.

>> Your experience confirms the discussion [3] with Russell King. DMA
>> should be disabled and
>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
>> line is set as gpio.
> DMA has nothing to do with those problems here. DMA can be safely ignored
> for the purpose of the discussion altogether.

When gpios are used for RTS/CTS DMA is not used. However DMA is related 
due to the driver
and "fsl,uart-has-rtscts". If you look into code of the driver you 
should agree.

>
> Like I explained already -- the problem is with the GPIO and AUART block not
> being synchronised by the same clock. It is therefore impossible to predict
> and control when the GPIO signals and the AUART signals will toggle in relation
> to one another.
>
>> So I didn't test the patch with the DMA - I've just disabled it.
> This would break the driver for pretty much everyone using higher baud rates.

NO, there is a choice. If you don't use RTS/CTS as GPIO DMA isn't enabled.
It seems you didn't read neither the patch nor the driver's code and
analized only the sentence.

>
>> The question is different. The driver supports the cases for RTS/CTS:
>> 1)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>>        DT sets fsl,uart-has-rtscts)
>>       a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabled
>>       b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
>> controlled by get/set_mctrl()
>> 2)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>>        DT doesn't set fsl,uart-has-rtscts)
>>       a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
>>       b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
>> controlled by get/set_mctrl()
>>
>> and after the patch it is more complex (because of mixed cases):
>> 3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts):
>>       the patch cancels fsl,uart-has-rtscts flag to disable DMA support,
>>       settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controlled
>> by get/set_mctrl(),
>>       both lines by gpios. In addition:
>>       a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is also
>> controlled but
>>           the case assumes it is not selected by pinmux in DT
>>       b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also
>> enabled but
>>           the case assumes it is not selected by pinmux in DT
>> 4) RTS assigned to hardware AUART (pinmux configured by DT,
>> fsl,uart-has-rtscts set or not),
>>       CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
>> disable DMA support,
>>       a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio)
>> read by get_mctrl(),
>>           RTS of hardware AUART controlled by set_mctrl()
>>       b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read by
>> get_mctrl(),
>>          auto RTS is enabled
>>      So case 4) is exactly 3) case with just different pinmux
>> configuration and the patch
>>      threats 3) and 4) as the same case.
>> 5) CTS assigned to hardware AUART (pinmux configured by DT,
>> fsl,uart-has-rtscts set or not),
>>       RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
>> disable DMA support,
>>       a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardware
>> AUART read by
>>           get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in
>> addition RTS of hardware AUART
>>           is also controlled but the case assumes it is not selected by
>> pinmux in DT
>>       b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
>> but RTS is controlled by set_mctrl() as gpio because the case assumes it
>> is not selected
>>           by pinmux in DT
>>
>> It is not nice description but I hadn't idea how to write it more clear.
>> The mixed cases 4) and 5) are likely not so useful but possible and not
>> so expensive
>> to support.
>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
>> do you agree? It rather should include "dma" word in the name. Any
>> suggestion?
>>
>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
> The best suggestion I can give you is to fix your hardware early, before you
> run into nasty deep s.....tuff. These workarounds do not work and they will
> bit you later on, when it's hard to fix the hardware anymore.
The speed is limited but why don't you accept SW-HW mixed solutions?
Exactly the same method is accepted for 8250.
It is good to have choice than not. We can comment that speed is limited
for gpio-based hardware flow control. So please, rethink again.

best regards
Janusz

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-07 13:23                     ` Janusz Użycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-07 13:23 UTC (permalink / raw)
  To: linux-arm-kernel


W dniu 2014-11-07 o 12:02, Marek Vasut pisze:
> On Friday, November 07, 2014 at 11:04:27 AM, Janusz U?ycki wrote:
>> W dniu 2014-11-07 o 09:03, Marek Vasut pisze:
>>> On Friday, November 07, 2014 at 02:34:31 AM, Huang Shijie wrote:
>>>> why change them to gpio?
> +CC Alexandre, since he might be interested :-)

:)

>
>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
>> in order to obtain as much uarts as possible using i.mx283.
>> Therefore gpios can be used for "hardware" flow control.
> Your logic is outright flawed here, the first sentence doesn't implicate the
> second sentence. In fact, those two are completely unrelated.

I didn't write MUST but CAN. There is a choice. Is flexibility of the 
driver disadvantage?

>>>>     If we change them to gpio.  Could the DMA still
>>>>
>>>> works fine?
>>>> did you test the DMA with this patch?
>>>>
>>>> Add Marek for this patch too.
>>> I didn't look too deep into the patch, so here's just my experience:
>>>
>>> 1) The AUART block signals and GPIO block signals are not sychronised
>>> using the
>>>
>>>      same clock. Therefore, the latency between toggling of the AUART
>>>      lines and the GPIO-driven pins will not be deterministic and will
>>>      vary. There might be a way to approximate that, but that's
>>>      definitelly not a reliable solution.
>>>      
>>>      This is very bad for example if you drive RS485 DIR line with the RTS
>>>      pin as a GPIO ; the RTS pin will toggle at non-deterministic time
>>>      compared to the end of UART transmission. This will trigger bit-loss
>>>      on the RS485 line and you just don't want that.
>>>
>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
>>> combo
>>>
>>>      of UART+GPIO toggling.
>>>
>>> So I hate to bring the bad news , but UART+GPIO combo toggling is really
>>> a bad bad idea.
>> Unfortunately if hardware is limited there is no choice and UART+GPIO is
>> necessary.
> You will run into timing problems (see above).

A lot of 8250-compatible devices has no hardware flow control and in 
most cases
they works and it is enough even for 115200 speed if CTS is handled by irq.
So it depends on your needs.

>
> What you're proposing here is a workaround for broken hardware, which was proven
> to be a bad idea and NAK'd already multiple times in the past (please see the
> links I posted in my last email).

It is not broken  hardware - rather limited to lower speeds but still 
very useful solution.

>> Your experience confirms the discussion [3] with Russell King. DMA
>> should be disabled and
>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
>> line is set as gpio.
> DMA has nothing to do with those problems here. DMA can be safely ignored
> for the purpose of the discussion altogether.

When gpios are used for RTS/CTS DMA is not used. However DMA is related 
due to the driver
and "fsl,uart-has-rtscts". If you look into code of the driver you 
should agree.

>
> Like I explained already -- the problem is with the GPIO and AUART block not
> being synchronised by the same clock. It is therefore impossible to predict
> and control when the GPIO signals and the AUART signals will toggle in relation
> to one another.
>
>> So I didn't test the patch with the DMA - I've just disabled it.
> This would break the driver for pretty much everyone using higher baud rates.

NO, there is a choice. If you don't use RTS/CTS as GPIO DMA isn't enabled.
It seems you didn't read neither the patch nor the driver's code and
analized only the sentence.

>
>> The question is different. The driver supports the cases for RTS/CTS:
>> 1)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>>        DT sets fsl,uart-has-rtscts)
>>       a) settermios() sets CRTSCTS: DMA enabled, auto RTS/CTS is enabled
>>       b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
>> controlled by get/set_mctrl()
>> 2)  both RTS/CTS assigned to hardware AUART (pinmux configured by DT,
>>        DT doesn't set fsl,uart-has-rtscts)
>>       a) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
>>       b) settermios() doesn't set CRTSCTS: DMA disabled, RTS/CTS
>> controlled by get/set_mctrl()
>>
>> and after the patch it is more complex (because of mixed cases):
>> 3) both RTS/CTS assigned to gpios (DT sets or not fsl,uart-has-rtscts):
>>       the patch cancels fsl,uart-has-rtscts flag to disable DMA support,
>>       settermios() sets or not CRTSCTS: DMA disabled, RTS/CTS controlled
>> by get/set_mctrl(),
>>       both lines by gpios. In addition:
>>       a) settermios() doesn't set CRTSCTS: RTS of hardware AUART is also
>> controlled but
>>           the case assumes it is not selected by pinmux in DT
>>       b) settermios() sets CRTSCTS: auto RTS of hardware AUART is also
>> enabled but
>>           the case assumes it is not selected by pinmux in DT
>> 4) RTS assigned to hardware AUART (pinmux configured by DT,
>> fsl,uart-has-rtscts set or not),
>>       CTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
>> disable DMA support,
>>       a) settermios() doesn't set CRTSCTS: DMA disabled, CTS (as gpio)
>> read by get_mctrl(),
>>           RTS of hardware AUART controlled by set_mctrl()
>>       b) settermios() sets CRTSCTS: DMA disabled, CTS (as gpio) read by
>> get_mctrl(),
>>          auto RTS is enabled
>>      So case 4) is exactly 3) case with just different pinmux
>> configuration and the patch
>>      threats 3) and 4) as the same case.
>> 5) CTS assigned to hardware AUART (pinmux configured by DT,
>> fsl,uart-has-rtscts set or not),
>>       RTS assigned to gpio: the patch cancels fsl,uart-has-rtscts flag to
>> disable DMA support,
>>       a) settermios() doesn't set CRTSCTS: DMA disabled, CTS of hardware
>> AUART read by
>>           get_mctrl(), RTS (as gpio) controlled by set_mctrl(), in
>> addition RTS of hardware AUART
>>           is also controlled but the case assumes it is not selected by
>> pinmux in DT
>>       b) settermios() sets CRTSCTS: DMA disabled, auto RTS/CTS is enabled
>> but RTS is controlled by set_mctrl() as gpio because the case assumes it
>> is not selected
>>           by pinmux in DT
>>
>> It is not nice description but I hadn't idea how to write it more clear.
>> The mixed cases 4) and 5) are likely not so useful but possible and not
>> so expensive
>> to support.
>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
>> do you agree? It rather should include "dma" word in the name. Any
>> suggestion?
>>
>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
> The best suggestion I can give you is to fix your hardware early, before you
> run into nasty deep s.....tuff. These workarounds do not work and they will
> bit you later on, when it's hard to fix the hardware anymore.
The speed is limited but why don't you accept SW-HW mixed solutions?
Exactly the same method is accepted for 8250.
It is good to have choice than not. We can comment that speed is limited
for gpio-based hardware flow control. So please, rethink again.

best regards
Janusz

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-11-07 13:23                     ` Janusz Użycki
@ 2014-11-07 14:48                       ` Marek Vasut
  -1 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-07 14:48 UTC (permalink / raw)
  To: Janusz Użycki
  Cc: Huang Shijie, Richard Genoud, Greg Kroah-Hartman,
	Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel, Alexandre Belloni

On Friday, November 07, 2014 at 02:23:23 PM, Janusz Użycki wrote:
[...]

> >> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
> >> in order to obtain as much uarts as possible using i.mx283.
> >> Therefore gpios can be used for "hardware" flow control.
> > 
> > Your logic is outright flawed here, the first sentence doesn't implicate
> > the second sentence. In fact, those two are completely unrelated.
> 
> I didn't write MUST but CAN. There is a choice. Is flexibility of the
> driver disadvantage?

If the flexibility brings in known problems, then yes, it is a problem. Not 
because of the flexibility, but because it brings in bugs.

> >>>>     If we change them to gpio.  Could the DMA still
> >>>> 
> >>>> works fine?
> >>>> did you test the DMA with this patch?
> >>>> 
> >>>> Add Marek for this patch too.
> >>> 
> >>> I didn't look too deep into the patch, so here's just my experience:
> >>> 
> >>> 1) The AUART block signals and GPIO block signals are not sychronised
> >>> using the
> >>> 
> >>>      same clock. Therefore, the latency between toggling of the AUART
> >>>      lines and the GPIO-driven pins will not be deterministic and will
> >>>      vary. There might be a way to approximate that, but that's
> >>>      definitelly not a reliable solution.
> >>>      
> >>>      This is very bad for example if you drive RS485 DIR line with the
> >>>      RTS pin as a GPIO ; the RTS pin will toggle at non-deterministic
> >>>      time compared to the end of UART transmission. This will trigger
> >>>      bit-loss on the RS485 line and you just don't want that.
> >>> 
> >>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
> >>> combo
> >>> 
> >>>      of UART+GPIO toggling.
> >>> 
> >>> So I hate to bring the bad news , but UART+GPIO combo toggling is
> >>> really a bad bad idea.
> >> 
> >> Unfortunately if hardware is limited there is no choice and UART+GPIO is
> >> necessary.
> > 
> > You will run into timing problems (see above).
> 
> A lot of 8250-compatible devices has no hardware flow control and in
> most cases
> they works and it is enough even for 115200 speed if CTS is handled by irq.
> So it depends on your needs.

I presume that in such a case , the 8250 still handles the CTS line, not some 
external GPIO block, yes ?

> > What you're proposing here is a workaround for broken hardware, which was
> > proven to be a bad idea and NAK'd already multiple times in the past
> > (please see the links I posted in my last email).
> 
> It is not broken  hardware - rather limited to lower speeds but still
> very useful solution.

What exact "lower speed" are you talking about here and why ?

> >> Your experience confirms the discussion [3] with Russell King. DMA
> >> should be disabled and
> >> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
> >> line is set as gpio.
> > 
> > DMA has nothing to do with those problems here. DMA can be safely ignored
> > for the purpose of the discussion altogether.
> 
> When gpios are used for RTS/CTS DMA is not used. However DMA is related
> due to the driver
> and "fsl,uart-has-rtscts". If you look into code of the driver you
> should agree.

This makes me believe that the DMA introduces too many timing fluctuations,
so it's really not possible for you to keep toggling the GPIOs such that the
bus would work. Is that the case ?

[...]

> >> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
> >> do you agree? It rather should include "dma" word in the name. Any
> >> suggestion?
> >> 
> >> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
> > 
> > The best suggestion I can give you is to fix your hardware early, before
> > you run into nasty deep s.....tuff. These workarounds do not work and
> > they will bit you later on, when it's hard to fix the hardware anymore.
> 
> The speed is limited but why don't you accept SW-HW mixed solutions?

Did you read up on the RS485 timing problems and why that solution was never
accepted for any driver ? I believe the threads explained that quite clearly.

> Exactly the same method is accepted for 8250.

Can you point out this code please ?

> It is good to have choice than not. We can comment that speed is limited
> for gpio-based hardware flow control. So please, rethink again.
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-07 14:48                       ` Marek Vasut
  0 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-07 14:48 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday, November 07, 2014 at 02:23:23 PM, Janusz U?ycki wrote:
[...]

> >> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
> >> in order to obtain as much uarts as possible using i.mx283.
> >> Therefore gpios can be used for "hardware" flow control.
> > 
> > Your logic is outright flawed here, the first sentence doesn't implicate
> > the second sentence. In fact, those two are completely unrelated.
> 
> I didn't write MUST but CAN. There is a choice. Is flexibility of the
> driver disadvantage?

If the flexibility brings in known problems, then yes, it is a problem. Not 
because of the flexibility, but because it brings in bugs.

> >>>>     If we change them to gpio.  Could the DMA still
> >>>> 
> >>>> works fine?
> >>>> did you test the DMA with this patch?
> >>>> 
> >>>> Add Marek for this patch too.
> >>> 
> >>> I didn't look too deep into the patch, so here's just my experience:
> >>> 
> >>> 1) The AUART block signals and GPIO block signals are not sychronised
> >>> using the
> >>> 
> >>>      same clock. Therefore, the latency between toggling of the AUART
> >>>      lines and the GPIO-driven pins will not be deterministic and will
> >>>      vary. There might be a way to approximate that, but that's
> >>>      definitelly not a reliable solution.
> >>>      
> >>>      This is very bad for example if you drive RS485 DIR line with the
> >>>      RTS pin as a GPIO ; the RTS pin will toggle at non-deterministic
> >>>      time compared to the end of UART transmission. This will trigger
> >>>      bit-loss on the RS485 line and you just don't want that.
> >>> 
> >>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
> >>> combo
> >>> 
> >>>      of UART+GPIO toggling.
> >>> 
> >>> So I hate to bring the bad news , but UART+GPIO combo toggling is
> >>> really a bad bad idea.
> >> 
> >> Unfortunately if hardware is limited there is no choice and UART+GPIO is
> >> necessary.
> > 
> > You will run into timing problems (see above).
> 
> A lot of 8250-compatible devices has no hardware flow control and in
> most cases
> they works and it is enough even for 115200 speed if CTS is handled by irq.
> So it depends on your needs.

I presume that in such a case , the 8250 still handles the CTS line, not some 
external GPIO block, yes ?

> > What you're proposing here is a workaround for broken hardware, which was
> > proven to be a bad idea and NAK'd already multiple times in the past
> > (please see the links I posted in my last email).
> 
> It is not broken  hardware - rather limited to lower speeds but still
> very useful solution.

What exact "lower speed" are you talking about here and why ?

> >> Your experience confirms the discussion [3] with Russell King. DMA
> >> should be disabled and
> >> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
> >> line is set as gpio.
> > 
> > DMA has nothing to do with those problems here. DMA can be safely ignored
> > for the purpose of the discussion altogether.
> 
> When gpios are used for RTS/CTS DMA is not used. However DMA is related
> due to the driver
> and "fsl,uart-has-rtscts". If you look into code of the driver you
> should agree.

This makes me believe that the DMA introduces too many timing fluctuations,
so it's really not possible for you to keep toggling the GPIOs such that the
bus would work. Is that the case ?

[...]

> >> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
> >> do you agree? It rather should include "dma" word in the name. Any
> >> suggestion?
> >> 
> >> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
> > 
> > The best suggestion I can give you is to fix your hardware early, before
> > you run into nasty deep s.....tuff. These workarounds do not work and
> > they will bit you later on, when it's hard to fix the hardware anymore.
> 
> The speed is limited but why don't you accept SW-HW mixed solutions?

Did you read up on the RS485 timing problems and why that solution was never
accepted for any driver ? I believe the threads explained that quite clearly.

> Exactly the same method is accepted for 8250.

Can you point out this code please ?

> It is good to have choice than not. We can comment that speed is limited
> for gpio-based hardware flow control. So please, rethink again.
[...]

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-11-07 14:48                       ` Marek Vasut
@ 2014-11-07 16:29                         ` Janusz Użycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-07 16:29 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Huang Shijie, Richard Genoud, Greg Kroah-Hartman,
	Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel, Alexandre Belloni


W dniu 2014-11-07 o 15:48, Marek Vasut pisze:
> On Friday, November 07, 2014 at 02:23:23 PM, Janusz Użycki wrote:
> [...]
>
>>>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
>>>> in order to obtain as much uarts as possible using i.mx283.
>>>> Therefore gpios can be used for "hardware" flow control.
>>> Your logic is outright flawed here, the first sentence doesn't implicate
>>> the second sentence. In fact, those two are completely unrelated.
>> I didn't write MUST but CAN. There is a choice. Is flexibility of the
>> driver disadvantage?
> If the flexibility brings in known problems, then yes, it is a problem. Not
> because of the flexibility, but because it brings in bugs.

New features new bugs :) Does it mean to stop development?

>
>>>>>>      If we change them to gpio.  Could the DMA still
>>>>>>
>>>>>> works fine?
>>>>>> did you test the DMA with this patch?
>>>>>>
>>>>>> Add Marek for this patch too.
>>>>> I didn't look too deep into the patch, so here's just my experience:
>>>>>
>>>>> 1) The AUART block signals and GPIO block signals are not sychronised
>>>>> using the
>>>>>
>>>>>       same clock. Therefore, the latency between toggling of the AUART
>>>>>       lines and the GPIO-driven pins will not be deterministic and will
>>>>>       vary. There might be a way to approximate that, but that's
>>>>>       definitelly not a reliable solution.
>>>>>       
>>>>>       This is very bad for example if you drive RS485 DIR line with the
>>>>>       RTS pin as a GPIO ; the RTS pin will toggle at non-deterministic
>>>>>       time compared to the end of UART transmission. This will trigger
>>>>>       bit-loss on the RS485 line and you just don't want that.
>>>>>
>>>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
>>>>> combo
>>>>>
>>>>>       of UART+GPIO toggling.
>>>>>
>>>>> So I hate to bring the bad news , but UART+GPIO combo toggling is
>>>>> really a bad bad idea.
>>>> Unfortunately if hardware is limited there is no choice and UART+GPIO is
>>>> necessary.
>>> You will run into timing problems (see above).
>> A lot of 8250-compatible devices has no hardware flow control and in
>> most cases
>> they works and it is enough even for 115200 speed if CTS is handled by irq.
>> So it depends on your needs.
> I presume that in such a case , the 8250 still handles the CTS line, not some
> external GPIO block, yes ?

Yes. However mxs includes both GPIO and AUART. Clocks differs but it is 
still the same silicon.
External GPIO block is extreme example highly not recommended here.

>
>>> What you're proposing here is a workaround for broken hardware, which was
>>> proven to be a bad idea and NAK'd already multiple times in the past
>>> (please see the links I posted in my last email).
>> It is not broken  hardware - rather limited to lower speeds but still
>> very useful solution.
> What exact "lower speed" are you talking about here and why ?

For example not more than 115200 but it depends on CPU load of course, 
FIFO size
and device on the opposite site. RTS/CTS via GPIO require to know the limit
in an application.

I googled even so exotic thing like:
"8250: add support for DTR/DSR hardware flow control"

>
>>>> Your experience confirms the discussion [3] with Russell King. DMA
>>>> should be disabled and
>>>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
>>>> line is set as gpio.
>>> DMA has nothing to do with those problems here. DMA can be safely ignored
>>> for the purpose of the discussion altogether.
>> When gpios are used for RTS/CTS DMA is not used. However DMA is related
>> due to the driver
>> and "fsl,uart-has-rtscts". If you look into code of the driver you
>> should agree.
> This makes me believe that the DMA introduces too many timing fluctuations,
> so it's really not possible for you to keep toggling the GPIOs such that the
> bus would work. Is that the case ?

Yes, for RTS/CTS based on gpio DMA is not used. They are just toggled.
So you probably misunderstood me.

> [...]
>
>>>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
>>>> do you agree? It rather should include "dma" word in the name. Any
>>>> suggestion?
>>>>
>>>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
>>> The best suggestion I can give you is to fix your hardware early, before
>>> you run into nasty deep s.....tuff. These workarounds do not work and
>>> they will bit you later on, when it's hard to fix the hardware anymore.
>> The speed is limited but why don't you accept SW-HW mixed solutions?
> Did you read up on the RS485 timing problems and why that solution was never
> accepted for any driver ? I believe the threads explained that quite clearly.

Example of RS485 implementation where RTS is toggled by software:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/tty/serial/omap-serial.c?id=4a0ac0f55b18dc297a87a85417fcf068658bf103
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/tty/serial/omap-serial.c

>
>> Exactly the same method is accepted for 8250.
> Can you point out this code please ?

If 8250 doesn't support auto flow control RTS/CTS are also toggled by 
software,
uart_trottle(), uart_untrothle(), uart_handle_cts_change():
http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L635
http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L2796
Detected by irq (ms enabled when CRTSCTS) CTS change stops tx because
UPF_HARD_FLOW flag is not set by the mxs-auart driver.
Of course timing problem exists but in many cases it is not critical -
the toggle method was implemented many years ago and it seems to work.

>
>> It is good to have choice than not. We can comment that speed is limited
>> for gpio-based hardware flow control. So please, rethink again.
> [...]

The only problem for me is misleading "fsl,uart-has-rtscts" name because 
the flag
only enables DMA if CRTSCTS is set and hardware flow control of AUART 
block is used.

best regards
Janusz

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-07 16:29                         ` Janusz Użycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-07 16:29 UTC (permalink / raw)
  To: linux-arm-kernel


W dniu 2014-11-07 o 15:48, Marek Vasut pisze:
> On Friday, November 07, 2014 at 02:23:23 PM, Janusz U?ycki wrote:
> [...]
>
>>>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
>>>> in order to obtain as much uarts as possible using i.mx283.
>>>> Therefore gpios can be used for "hardware" flow control.
>>> Your logic is outright flawed here, the first sentence doesn't implicate
>>> the second sentence. In fact, those two are completely unrelated.
>> I didn't write MUST but CAN. There is a choice. Is flexibility of the
>> driver disadvantage?
> If the flexibility brings in known problems, then yes, it is a problem. Not
> because of the flexibility, but because it brings in bugs.

New features new bugs :) Does it mean to stop development?

>
>>>>>>      If we change them to gpio.  Could the DMA still
>>>>>>
>>>>>> works fine?
>>>>>> did you test the DMA with this patch?
>>>>>>
>>>>>> Add Marek for this patch too.
>>>>> I didn't look too deep into the patch, so here's just my experience:
>>>>>
>>>>> 1) The AUART block signals and GPIO block signals are not sychronised
>>>>> using the
>>>>>
>>>>>       same clock. Therefore, the latency between toggling of the AUART
>>>>>       lines and the GPIO-driven pins will not be deterministic and will
>>>>>       vary. There might be a way to approximate that, but that's
>>>>>       definitelly not a reliable solution.
>>>>>       
>>>>>       This is very bad for example if you drive RS485 DIR line with the
>>>>>       RTS pin as a GPIO ; the RTS pin will toggle at non-deterministic
>>>>>       time compared to the end of UART transmission. This will trigger
>>>>>       bit-loss on the RS485 line and you just don't want that.
>>>>>
>>>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to any
>>>>> combo
>>>>>
>>>>>       of UART+GPIO toggling.
>>>>>
>>>>> So I hate to bring the bad news , but UART+GPIO combo toggling is
>>>>> really a bad bad idea.
>>>> Unfortunately if hardware is limited there is no choice and UART+GPIO is
>>>> necessary.
>>> You will run into timing problems (see above).
>> A lot of 8250-compatible devices has no hardware flow control and in
>> most cases
>> they works and it is enough even for 115200 speed if CTS is handled by irq.
>> So it depends on your needs.
> I presume that in such a case , the 8250 still handles the CTS line, not some
> external GPIO block, yes ?

Yes. However mxs includes both GPIO and AUART. Clocks differs but it is 
still the same silicon.
External GPIO block is extreme example highly not recommended here.

>
>>> What you're proposing here is a workaround for broken hardware, which was
>>> proven to be a bad idea and NAK'd already multiple times in the past
>>> (please see the links I posted in my last email).
>> It is not broken  hardware - rather limited to lower speeds but still
>> very useful solution.
> What exact "lower speed" are you talking about here and why ?

For example not more than 115200 but it depends on CPU load of course, 
FIFO size
and device on the opposite site. RTS/CTS via GPIO require to know the limit
in an application.

I googled even so exotic thing like:
"8250: add support for DTR/DSR hardware flow control"

>
>>>> Your experience confirms the discussion [3] with Russell King. DMA
>>>> should be disabled and
>>>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
>>>> line is set as gpio.
>>> DMA has nothing to do with those problems here. DMA can be safely ignored
>>> for the purpose of the discussion altogether.
>> When gpios are used for RTS/CTS DMA is not used. However DMA is related
>> due to the driver
>> and "fsl,uart-has-rtscts". If you look into code of the driver you
>> should agree.
> This makes me believe that the DMA introduces too many timing fluctuations,
> so it's really not possible for you to keep toggling the GPIOs such that the
> bus would work. Is that the case ?

Yes, for RTS/CTS based on gpio DMA is not used. They are just toggled.
So you probably misunderstood me.

> [...]
>
>>>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading now,
>>>> do you agree? It rather should include "dma" word in the name. Any
>>>> suggestion?
>>>>
>>>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
>>> The best suggestion I can give you is to fix your hardware early, before
>>> you run into nasty deep s.....tuff. These workarounds do not work and
>>> they will bit you later on, when it's hard to fix the hardware anymore.
>> The speed is limited but why don't you accept SW-HW mixed solutions?
> Did you read up on the RS485 timing problems and why that solution was never
> accepted for any driver ? I believe the threads explained that quite clearly.

Example of RS485 implementation where RTS is toggled by software:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/tty/serial/omap-serial.c?id=4a0ac0f55b18dc297a87a85417fcf068658bf103
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/tty/serial/omap-serial.c

>
>> Exactly the same method is accepted for 8250.
> Can you point out this code please ?

If 8250 doesn't support auto flow control RTS/CTS are also toggled by 
software,
uart_trottle(), uart_untrothle(), uart_handle_cts_change():
http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L635
http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L2796
Detected by irq (ms enabled when CRTSCTS) CTS change stops tx because
UPF_HARD_FLOW flag is not set by the mxs-auart driver.
Of course timing problem exists but in many cases it is not critical -
the toggle method was implemented many years ago and it seems to work.

>
>> It is good to have choice than not. We can comment that speed is limited
>> for gpio-based hardware flow control. So please, rethink again.
> [...]

The only problem for me is misleading "fsl,uart-has-rtscts" name because 
the flag
only enables DMA if CRTSCTS is set and hardware flow control of AUART 
block is used.

best regards
Janusz

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-11-07 16:29                         ` Janusz Użycki
@ 2014-11-08 11:22                           ` Marek Vasut
  -1 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-08 11:22 UTC (permalink / raw)
  To: Janusz Użycki
  Cc: Huang Shijie, Richard Genoud, Greg Kroah-Hartman,
	Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel, Alexandre Belloni

On Friday, November 07, 2014 at 05:29:33 PM, Janusz Użycki wrote:
> W dniu 2014-11-07 o 15:48, Marek Vasut pisze:
> > On Friday, November 07, 2014 at 02:23:23 PM, Janusz Użycki wrote:
> > [...]
> > 
> >>>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
> >>>> in order to obtain as much uarts as possible using i.mx283.
> >>>> Therefore gpios can be used for "hardware" flow control.
> >>> 
> >>> Your logic is outright flawed here, the first sentence doesn't
> >>> implicate the second sentence. In fact, those two are completely
> >>> unrelated.
> >> 
> >> I didn't write MUST but CAN. There is a choice. Is flexibility of the
> >> driver disadvantage?
> > 
> > If the flexibility brings in known problems, then yes, it is a problem.
> > Not because of the flexibility, but because it brings in bugs.
> 
> New features new bugs :) Does it mean to stop development?

You shouldn't push code which is known to be defective by design into mainline.

> >>>>>>      If we change them to gpio.  Could the DMA still
> >>>>>> 
> >>>>>> works fine?
> >>>>>> did you test the DMA with this patch?
> >>>>>> 
> >>>>>> Add Marek for this patch too.
> >>>>> 
> >>>>> I didn't look too deep into the patch, so here's just my experience:
> >>>>> 
> >>>>> 1) The AUART block signals and GPIO block signals are not sychronised
> >>>>> using the
> >>>>> 
> >>>>>       same clock. Therefore, the latency between toggling of the
> >>>>>       AUART lines and the GPIO-driven pins will not be deterministic
> >>>>>       and will vary. There might be a way to approximate that, but
> >>>>>       that's definitelly not a reliable solution.
> >>>>>       
> >>>>>       This is very bad for example if you drive RS485 DIR line with
> >>>>>       the RTS pin as a GPIO ; the RTS pin will toggle at
> >>>>>       non-deterministic time compared to the end of UART
> >>>>>       transmission. This will trigger bit-loss on the RS485 line and
> >>>>>       you just don't want that.
> >>>>> 
> >>>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to
> >>>>> any combo
> >>>>> 
> >>>>>       of UART+GPIO toggling.
> >>>>> 
> >>>>> So I hate to bring the bad news , but UART+GPIO combo toggling is
> >>>>> really a bad bad idea.
> >>>> 
> >>>> Unfortunately if hardware is limited there is no choice and UART+GPIO
> >>>> is necessary.
> >>> 
> >>> You will run into timing problems (see above).
> >> 
> >> A lot of 8250-compatible devices has no hardware flow control and in
> >> most cases
> >> they works and it is enough even for 115200 speed if CTS is handled by
> >> irq. So it depends on your needs.
> > 
> > I presume that in such a case , the 8250 still handles the CTS line, not
> > some external GPIO block, yes ?
> 
> Yes. However mxs includes both GPIO and AUART. Clocks differs but it is
> still the same silicon.
> External GPIO block is extreme example highly not recommended here.

The way you implemented this particular change, it is possible to use arbitrary
GPIO pin. There is no way you can guarantee anything about the latency of the
GPIO toggling (I am repeating myself).

> >>> What you're proposing here is a workaround for broken hardware, which
> >>> was proven to be a bad idea and NAK'd already multiple times in the
> >>> past (please see the links I posted in my last email).
> >> 
> >> It is not broken  hardware - rather limited to lower speeds but still
> >> very useful solution.
> > 
> > What exact "lower speed" are you talking about here and why ?
> 
> For example not more than 115200 but it depends on CPU load of course,
> FIFO size
> and device on the opposite site. RTS/CTS via GPIO require to know the limit
> in an application.

OK, this is completely unreliable solution, which works just by sheer luck.

> I googled even so exotic thing like:
> "8250: add support for DTR/DSR hardware flow control"

The fact that those perversions exists doesn't make them right. It doesn't
even make them mainlinable.

> >>>> Your experience confirms the discussion [3] with Russell King. DMA
> >>>> should be disabled and
> >>>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
> >>>> line is set as gpio.
> >>> 
> >>> DMA has nothing to do with those problems here. DMA can be safely
> >>> ignored for the purpose of the discussion altogether.
> >> 
> >> When gpios are used for RTS/CTS DMA is not used. However DMA is related
> >> due to the driver
> >> and "fsl,uart-has-rtscts". If you look into code of the driver you
> >> should agree.
> > 
> > This makes me believe that the DMA introduces too many timing
> > fluctuations, so it's really not possible for you to keep toggling the
> > GPIOs such that the bus would work. Is that the case ?
> 
> Yes, for RTS/CTS based on gpio DMA is not used. They are just toggled.
> So you probably misunderstood me.

I understand you -- in case DMA is enabled on the AUART block, your hack
is no longer capable to working correctly, so everything falls apart.

> > [...]
> > 
> >>>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading
> >>>> now, do you agree? It rather should include "dma" word in the name.
> >>>> Any suggestion?
> >>>> 
> >>>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
> >>> 
> >>> The best suggestion I can give you is to fix your hardware early,
> >>> before you run into nasty deep s.....tuff. These workarounds do not
> >>> work and they will bit you later on, when it's hard to fix the
> >>> hardware anymore.
> >> 
> >> The speed is limited but why don't you accept SW-HW mixed solutions?
> > 
> > Did you read up on the RS485 timing problems and why that solution was
> > never accepted for any driver ? I believe the threads explained that
> > quite clearly.
> 
> Example of RS485 implementation where RTS is toggled by software:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/dri
> vers/tty/serial/omap-serial.c?id=4a0ac0f55b18dc297a87a85417fcf068658bf103
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/
> drivers/tty/serial/omap-serial.c

This is a question for Greg. I checked the discussion about this patch [1]
and I see this timing issue was brought up, but the patch was applied anyway.
I cannot tell you why , I just know that this GPIO approach has problems and
I wrestled those a couple of days ago (without success, it's not possible to
get correct timing).

[1] http://www.spinics.net/lists/linux-serial/msg10574.html

> >> Exactly the same method is accepted for 8250.
> > 
> > Can you point out this code please ?
> 
> If 8250 doesn't support auto flow control RTS/CTS are also toggled by
> software,
> uart_trottle(), uart_untrothle(), uart_handle_cts_change():
> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L635
> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L2796
> Detected by irq (ms enabled when CRTSCTS) CTS change stops tx because
> UPF_HARD_FLOW flag is not set by the mxs-auart driver.

This doesn't use GPIO to toggle the RTS/CTS pins, it does toggle then via the
8250 IP block registers, right ?

> Of course timing problem exists but in many cases it is not critical -
> the toggle method was implemented many years ago and it seems to work.

Yes, it does seem to work initially, that's why so many hardware people
implement it, thinking the software people can fix those flubs. Problem
is, this is one of those nasty problems, which cannot be fixed in software.

> >> It is good to have choice than not. We can comment that speed is limited
> >> for gpio-based hardware flow control. So please, rethink again.
> > 
> > [...]
> 
> The only problem for me is misleading "fsl,uart-has-rtscts" name because
> the flag
> only enables DMA if CRTSCTS is set and hardware flow control of AUART
> block is used.
> 
> best regards
> Janusz
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-08 11:22                           ` Marek Vasut
  0 siblings, 0 replies; 36+ messages in thread
From: Marek Vasut @ 2014-11-08 11:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Friday, November 07, 2014 at 05:29:33 PM, Janusz U?ycki wrote:
> W dniu 2014-11-07 o 15:48, Marek Vasut pisze:
> > On Friday, November 07, 2014 at 02:23:23 PM, Janusz U?ycki wrote:
> > [...]
> > 
> >>>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
> >>>> in order to obtain as much uarts as possible using i.mx283.
> >>>> Therefore gpios can be used for "hardware" flow control.
> >>> 
> >>> Your logic is outright flawed here, the first sentence doesn't
> >>> implicate the second sentence. In fact, those two are completely
> >>> unrelated.
> >> 
> >> I didn't write MUST but CAN. There is a choice. Is flexibility of the
> >> driver disadvantage?
> > 
> > If the flexibility brings in known problems, then yes, it is a problem.
> > Not because of the flexibility, but because it brings in bugs.
> 
> New features new bugs :) Does it mean to stop development?

You shouldn't push code which is known to be defective by design into mainline.

> >>>>>>      If we change them to gpio.  Could the DMA still
> >>>>>> 
> >>>>>> works fine?
> >>>>>> did you test the DMA with this patch?
> >>>>>> 
> >>>>>> Add Marek for this patch too.
> >>>>> 
> >>>>> I didn't look too deep into the patch, so here's just my experience:
> >>>>> 
> >>>>> 1) The AUART block signals and GPIO block signals are not sychronised
> >>>>> using the
> >>>>> 
> >>>>>       same clock. Therefore, the latency between toggling of the
> >>>>>       AUART lines and the GPIO-driven pins will not be deterministic
> >>>>>       and will vary. There might be a way to approximate that, but
> >>>>>       that's definitelly not a reliable solution.
> >>>>>       
> >>>>>       This is very bad for example if you drive RS485 DIR line with
> >>>>>       the RTS pin as a GPIO ; the RTS pin will toggle at
> >>>>>       non-deterministic time compared to the end of UART
> >>>>>       transmission. This will trigger bit-loss on the RS485 line and
> >>>>>       you just don't want that.
> >>>>> 
> >>>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to
> >>>>> any combo
> >>>>> 
> >>>>>       of UART+GPIO toggling.
> >>>>> 
> >>>>> So I hate to bring the bad news , but UART+GPIO combo toggling is
> >>>>> really a bad bad idea.
> >>>> 
> >>>> Unfortunately if hardware is limited there is no choice and UART+GPIO
> >>>> is necessary.
> >>> 
> >>> You will run into timing problems (see above).
> >> 
> >> A lot of 8250-compatible devices has no hardware flow control and in
> >> most cases
> >> they works and it is enough even for 115200 speed if CTS is handled by
> >> irq. So it depends on your needs.
> > 
> > I presume that in such a case , the 8250 still handles the CTS line, not
> > some external GPIO block, yes ?
> 
> Yes. However mxs includes both GPIO and AUART. Clocks differs but it is
> still the same silicon.
> External GPIO block is extreme example highly not recommended here.

The way you implemented this particular change, it is possible to use arbitrary
GPIO pin. There is no way you can guarantee anything about the latency of the
GPIO toggling (I am repeating myself).

> >>> What you're proposing here is a workaround for broken hardware, which
> >>> was proven to be a bad idea and NAK'd already multiple times in the
> >>> past (please see the links I posted in my last email).
> >> 
> >> It is not broken  hardware - rather limited to lower speeds but still
> >> very useful solution.
> > 
> > What exact "lower speed" are you talking about here and why ?
> 
> For example not more than 115200 but it depends on CPU load of course,
> FIFO size
> and device on the opposite site. RTS/CTS via GPIO require to know the limit
> in an application.

OK, this is completely unreliable solution, which works just by sheer luck.

> I googled even so exotic thing like:
> "8250: add support for DTR/DSR hardware flow control"

The fact that those perversions exists doesn't make them right. It doesn't
even make them mainlinable.

> >>>> Your experience confirms the discussion [3] with Russell King. DMA
> >>>> should be disabled and
> >>>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
> >>>> line is set as gpio.
> >>> 
> >>> DMA has nothing to do with those problems here. DMA can be safely
> >>> ignored for the purpose of the discussion altogether.
> >> 
> >> When gpios are used for RTS/CTS DMA is not used. However DMA is related
> >> due to the driver
> >> and "fsl,uart-has-rtscts". If you look into code of the driver you
> >> should agree.
> > 
> > This makes me believe that the DMA introduces too many timing
> > fluctuations, so it's really not possible for you to keep toggling the
> > GPIOs such that the bus would work. Is that the case ?
> 
> Yes, for RTS/CTS based on gpio DMA is not used. They are just toggled.
> So you probably misunderstood me.

I understand you -- in case DMA is enabled on the AUART block, your hack
is no longer capable to working correctly, so everything falls apart.

> > [...]
> > 
> >>>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading
> >>>> now, do you agree? It rather should include "dma" word in the name.
> >>>> Any suggestion?
> >>>> 
> >>>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
> >>> 
> >>> The best suggestion I can give you is to fix your hardware early,
> >>> before you run into nasty deep s.....tuff. These workarounds do not
> >>> work and they will bit you later on, when it's hard to fix the
> >>> hardware anymore.
> >> 
> >> The speed is limited but why don't you accept SW-HW mixed solutions?
> > 
> > Did you read up on the RS485 timing problems and why that solution was
> > never accepted for any driver ? I believe the threads explained that
> > quite clearly.
> 
> Example of RS485 implementation where RTS is toggled by software:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/dri
> vers/tty/serial/omap-serial.c?id=4a0ac0f55b18dc297a87a85417fcf068658bf103
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/
> drivers/tty/serial/omap-serial.c

This is a question for Greg. I checked the discussion about this patch [1]
and I see this timing issue was brought up, but the patch was applied anyway.
I cannot tell you why , I just know that this GPIO approach has problems and
I wrestled those a couple of days ago (without success, it's not possible to
get correct timing).

[1] http://www.spinics.net/lists/linux-serial/msg10574.html

> >> Exactly the same method is accepted for 8250.
> > 
> > Can you point out this code please ?
> 
> If 8250 doesn't support auto flow control RTS/CTS are also toggled by
> software,
> uart_trottle(), uart_untrothle(), uart_handle_cts_change():
> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L635
> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L2796
> Detected by irq (ms enabled when CRTSCTS) CTS change stops tx because
> UPF_HARD_FLOW flag is not set by the mxs-auart driver.

This doesn't use GPIO to toggle the RTS/CTS pins, it does toggle then via the
8250 IP block registers, right ?

> Of course timing problem exists but in many cases it is not critical -
> the toggle method was implemented many years ago and it seems to work.

Yes, it does seem to work initially, that's why so many hardware people
implement it, thinking the software people can fix those flubs. Problem
is, this is one of those nasty problems, which cannot be fixed in software.

> >> It is good to have choice than not. We can comment that speed is limited
> >> for gpio-based hardware flow control. So please, rethink again.
> > 
> > [...]
> 
> The only problem for me is misleading "fsl,uart-has-rtscts" name because
> the flag
> only enables DMA if CRTSCTS is set and hardware flow control of AUART
> block is used.
> 
> best regards
> Janusz

^ permalink raw reply	[flat|nested] 36+ messages in thread
* Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
  2014-11-08 11:22                           ` Marek Vasut
@ 2014-11-08 13:38                             ` Janusz Użycki
  -1 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-08 13:38 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Huang Shijie, Richard Genoud, Greg Kroah-Hartman,
	Russell King - ARM Linux, Jiri Slaby, Fabio Estevam,
	linux-serial, devicetree, linux-arm-kernel, Alexandre Belloni


W dniu 2014-11-08 o 12:22, Marek Vasut pisze:
> On Friday, November 07, 2014 at 05:29:33 PM, Janusz Użycki wrote:
>> W dniu 2014-11-07 o 15:48, Marek Vasut pisze:
>>> On Friday, November 07, 2014 at 02:23:23 PM, Janusz Użycki wrote:
>>> [...]
>>>
>>>>>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
>>>>>> in order to obtain as much uarts as possible using i.mx283.
>>>>>> Therefore gpios can be used for "hardware" flow control.
>>>>> Your logic is outright flawed here, the first sentence doesn't
>>>>> implicate the second sentence. In fact, those two are completely
>>>>> unrelated.
>>>> I didn't write MUST but CAN. There is a choice. Is flexibility of the
>>>> driver disadvantage?
>>> If the flexibility brings in known problems, then yes, it is a problem.
>>> Not because of the flexibility, but because it brings in bugs.
>> New features new bugs :) Does it mean to stop development?
> You shouldn't push code which is known to be defective by design into mainline.

I've written in general. The patch is not defective and it has been 
discussed before (V4).
Did you read the code and the patch?

>>>>>>>>       If we change them to gpio.  Could the DMA still
>>>>>>>>
>>>>>>>> works fine?
>>>>>>>> did you test the DMA with this patch?
>>>>>>>>
>>>>>>>> Add Marek for this patch too.
>>>>>>> I didn't look too deep into the patch, so here's just my experience:
>>>>>>>
>>>>>>> 1) The AUART block signals and GPIO block signals are not sychronised
>>>>>>> using the
>>>>>>>
>>>>>>>        same clock. Therefore, the latency between toggling of the
>>>>>>>        AUART lines and the GPIO-driven pins will not be deterministic
>>>>>>>        and will vary. There might be a way to approximate that, but
>>>>>>>        that's definitelly not a reliable solution.
>>>>>>>        
>>>>>>>        This is very bad for example if you drive RS485 DIR line with
>>>>>>>        the RTS pin as a GPIO ; the RTS pin will toggle at
>>>>>>>        non-deterministic time compared to the end of UART
>>>>>>>        transmission. This will trigger bit-loss on the RS485 line and
>>>>>>>        you just don't want that.
>>>>>>>
>>>>>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to
>>>>>>> any combo
>>>>>>>
>>>>>>>        of UART+GPIO toggling.
>>>>>>>
>>>>>>> So I hate to bring the bad news , but UART+GPIO combo toggling is
>>>>>>> really a bad bad idea.
>>>>>> Unfortunately if hardware is limited there is no choice and UART+GPIO
>>>>>> is necessary.
>>>>> You will run into timing problems (see above).
>>>> A lot of 8250-compatible devices has no hardware flow control and in
>>>> most cases
>>>> they works and it is enough even for 115200 speed if CTS is handled by
>>>> irq. So it depends on your needs.
>>> I presume that in such a case , the 8250 still handles the CTS line, not
>>> some external GPIO block, yes ?
>> Yes. However mxs includes both GPIO and AUART. Clocks differs but it is
>> still the same silicon.
>> External GPIO block is extreme example highly not recommended here.
> The way you implemented this particular change, it is possible to use arbitrary
> GPIO pin. There is no way you can guarantee anything about the latency of the
> GPIO toggling (I am repeating myself).

GPIO latency in SoC is usually smaller than any pin toggled in 8250 chip.
You can write some gpio-limiter patch for Richard's GPIO patch but in my 
opinion
it has no sense. DTB maker has to know what he is doing and why.
My patch concerns the mxs-auart driver so I can't understand
why your notes are addressed to me. You complain on much more patches in 
mainline,
not mine.

>
>>>>> What you're proposing here is a workaround for broken hardware, which
>>>>> was proven to be a bad idea and NAK'd already multiple times in the
>>>>> past (please see the links I posted in my last email).
>>>> It is not broken  hardware - rather limited to lower speeds but still
>>>> very useful solution.
>>> What exact "lower speed" are you talking about here and why ?
>> For example not more than 115200 but it depends on CPU load of course,
>> FIFO size
>> and device on the opposite site. RTS/CTS via GPIO require to know the limit
>> in an application.
> OK, this is completely unreliable solution, which works just by sheer luck.
>
>> I googled even so exotic thing like:
>> "8250: add support for DTR/DSR hardware flow control"
> The fact that those perversions exists doesn't make them right. It doesn't
> even make them mainlinable.

Again, not addressed to me. The world has accepted them.

>
>>>>>> Your experience confirms the discussion [3] with Russell King. DMA
>>>>>> should be disabled and
>>>>>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
>>>>>> line is set as gpio.
>>>>> DMA has nothing to do with those problems here. DMA can be safely
>>>>> ignored for the purpose of the discussion altogether.
>>>> When gpios are used for RTS/CTS DMA is not used. However DMA is related
>>>> due to the driver
>>>> and "fsl,uart-has-rtscts". If you look into code of the driver you
>>>> should agree.
>>> This makes me believe that the DMA introduces too many timing
>>> fluctuations, so it's really not possible for you to keep toggling the
>>> GPIOs such that the bus would work. Is that the case ?
>> Yes, for RTS/CTS based on gpio DMA is not used. They are just toggled.
>> So you probably misunderstood me.
> I understand you -- in case DMA is enabled on the AUART block, your hack
> is no longer capable to working correctly, so everything falls apart.

You still didn't read the code or not carefully :)
If RTS/CTS aren't set as gpio it will work exactly as before - including 
DMA.
Otherwise I wouldn't push the patchset.

>
>>> [...]
>>>
>>>>>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading
>>>>>> now, do you agree? It rather should include "dma" word in the name.
>>>>>> Any suggestion?
>>>>>>
>>>>>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
>>>>> The best suggestion I can give you is to fix your hardware early,
>>>>> before you run into nasty deep s.....tuff. These workarounds do not
>>>>> work and they will bit you later on, when it's hard to fix the
>>>>> hardware anymore.
>>>> The speed is limited but why don't you accept SW-HW mixed solutions?
>>> Did you read up on the RS485 timing problems and why that solution was
>>> never accepted for any driver ? I believe the threads explained that
>>> quite clearly.
>> Example of RS485 implementation where RTS is toggled by software:
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/dri
>> vers/tty/serial/omap-serial.c?id=4a0ac0f55b18dc297a87a85417fcf068658bf103
>> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/
>> drivers/tty/serial/omap-serial.c
> This is a question for Greg. I checked the discussion about this patch [1]
> and I see this timing issue was brought up, but the patch was applied anyway.
> I cannot tell you why , I just know that this GPIO approach has problems and
> I wrestled those a couple of days ago (without success, it's not possible to
> get correct timing).
>
> [1] http://www.spinics.net/lists/linux-serial/msg10574.html

Yes, this is the question for others - tty/serial's guru.
The timming isn't great but your opinion makes impossible to work most 
RS485 devices,
especially older but not only. You want to change the world here I 
think. In practive
costs usually have higher value than perfect timming if a solution works 
great without.
In the past hardware RS485 support even didn't exist. It is changing but 
slowly.

>>>> Exactly the same method is accepted for 8250.
>>> Can you point out this code please ?
>> If 8250 doesn't support auto flow control RTS/CTS are also toggled by
>> software,
>> uart_trottle(), uart_untrothle(), uart_handle_cts_change():
>> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L635
>> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L2796
>> Detected by irq (ms enabled when CRTSCTS) CTS change stops tx because
>> UPF_HARD_FLOW flag is not set by the mxs-auart driver.
> This doesn't use GPIO to toggle the RTS/CTS pins, it does toggle then via the
> 8250 IP block registers, right ?

Right. My patch allows to toggle the pins via SoC's pins - there is a 
choice in DT.
About latency I've written above.

>> Of course timing problem exists but in many cases it is not critical -
>> the toggle method was implemented many years ago and it seems to work.
> Yes, it does seem to work initially, that's why so many hardware people
> implement it, thinking the software people can fix those flubs. Problem
> is, this is one of those nasty problems, which cannot be fixed in software.

Look into the past. And, not each application needs perfect things. 
Hardware people do
not expect of magic things - they must know reality better than you assume.
It seems you have young people's approach. The real world is not 
perfect. There are
costs, limited set of available chips, limited PCB's space, habits, etc.

The thread doesn't concern my patchset at all now :)

thanks for the nice discussion
Janusz

>> The only problem for me is misleading "fsl,uart-has-rtscts" name because
>> the flag
>> only enables DMA if CRTSCTS is set and hardware flow control of AUART
>> block is used.
>>
>> best regards
>> Janusz

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 36+ messages in thread
* [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals
@ 2014-11-08 13:38                             ` Janusz Użycki
  0 siblings, 0 replies; 36+ messages in thread
From: Janusz Użycki @ 2014-11-08 13:38 UTC (permalink / raw)
  To: linux-arm-kernel


W dniu 2014-11-08 o 12:22, Marek Vasut pisze:
> On Friday, November 07, 2014 at 05:29:33 PM, Janusz U?ycki wrote:
>> W dniu 2014-11-07 o 15:48, Marek Vasut pisze:
>>> On Friday, November 07, 2014 at 02:23:23 PM, Janusz U?ycki wrote:
>>> [...]
>>>
>>>>>> Hardware RTS/CTS lines can be occupied by RX/TX of other AUART port
>>>>>> in order to obtain as much uarts as possible using i.mx283.
>>>>>> Therefore gpios can be used for "hardware" flow control.
>>>>> Your logic is outright flawed here, the first sentence doesn't
>>>>> implicate the second sentence. In fact, those two are completely
>>>>> unrelated.
>>>> I didn't write MUST but CAN. There is a choice. Is flexibility of the
>>>> driver disadvantage?
>>> If the flexibility brings in known problems, then yes, it is a problem.
>>> Not because of the flexibility, but because it brings in bugs.
>> New features new bugs :) Does it mean to stop development?
> You shouldn't push code which is known to be defective by design into mainline.

I've written in general. The patch is not defective and it has been 
discussed before (V4).
Did you read the code and the patch?

>>>>>>>>       If we change them to gpio.  Could the DMA still
>>>>>>>>
>>>>>>>> works fine?
>>>>>>>> did you test the DMA with this patch?
>>>>>>>>
>>>>>>>> Add Marek for this patch too.
>>>>>>> I didn't look too deep into the patch, so here's just my experience:
>>>>>>>
>>>>>>> 1) The AUART block signals and GPIO block signals are not sychronised
>>>>>>> using the
>>>>>>>
>>>>>>>        same clock. Therefore, the latency between toggling of the
>>>>>>>        AUART lines and the GPIO-driven pins will not be deterministic
>>>>>>>        and will vary. There might be a way to approximate that, but
>>>>>>>        that's definitelly not a reliable solution.
>>>>>>>        
>>>>>>>        This is very bad for example if you drive RS485 DIR line with
>>>>>>>        the RTS pin as a GPIO ; the RTS pin will toggle at
>>>>>>>        non-deterministic time compared to the end of UART
>>>>>>>        transmission. This will trigger bit-loss on the RS485 line and
>>>>>>>        you just don't want that.
>>>>>>>
>>>>>>> 2) Speaking of RS485, there's [1] and [2]. which I believe apply to
>>>>>>> any combo
>>>>>>>
>>>>>>>        of UART+GPIO toggling.
>>>>>>>
>>>>>>> So I hate to bring the bad news , but UART+GPIO combo toggling is
>>>>>>> really a bad bad idea.
>>>>>> Unfortunately if hardware is limited there is no choice and UART+GPIO
>>>>>> is necessary.
>>>>> You will run into timing problems (see above).
>>>> A lot of 8250-compatible devices has no hardware flow control and in
>>>> most cases
>>>> they works and it is enough even for 115200 speed if CTS is handled by
>>>> irq. So it depends on your needs.
>>> I presume that in such a case , the 8250 still handles the CTS line, not
>>> some external GPIO block, yes ?
>> Yes. However mxs includes both GPIO and AUART. Clocks differs but it is
>> still the same silicon.
>> External GPIO block is extreme example highly not recommended here.
> The way you implemented this particular change, it is possible to use arbitrary
> GPIO pin. There is no way you can guarantee anything about the latency of the
> GPIO toggling (I am repeating myself).

GPIO latency in SoC is usually smaller than any pin toggled in 8250 chip.
You can write some gpio-limiter patch for Richard's GPIO patch but in my 
opinion
it has no sense. DTB maker has to know what he is doing and why.
My patch concerns the mxs-auart driver so I can't understand
why your notes are addressed to me. You complain on much more patches in 
mainline,
not mine.

>
>>>>> What you're proposing here is a workaround for broken hardware, which
>>>>> was proven to be a bad idea and NAK'd already multiple times in the
>>>>> past (please see the links I posted in my last email).
>>>> It is not broken  hardware - rather limited to lower speeds but still
>>>> very useful solution.
>>> What exact "lower speed" are you talking about here and why ?
>> For example not more than 115200 but it depends on CPU load of course,
>> FIFO size
>> and device on the opposite site. RTS/CTS via GPIO require to know the limit
>> in an application.
> OK, this is completely unreliable solution, which works just by sheer luck.
>
>> I googled even so exotic thing like:
>> "8250: add support for DTR/DSR hardware flow control"
> The fact that those perversions exists doesn't make them right. It doesn't
> even make them mainlinable.

Again, not addressed to me. The world has accepted them.

>
>>>>>> Your experience confirms the discussion [3] with Russell King. DMA
>>>>>> should be disabled and
>>>>>> the patch disables DMA support in mxs_auart_init_gpios() if RTS or CTS
>>>>>> line is set as gpio.
>>>>> DMA has nothing to do with those problems here. DMA can be safely
>>>>> ignored for the purpose of the discussion altogether.
>>>> When gpios are used for RTS/CTS DMA is not used. However DMA is related
>>>> due to the driver
>>>> and "fsl,uart-has-rtscts". If you look into code of the driver you
>>>> should agree.
>>> This makes me believe that the DMA introduces too many timing
>>> fluctuations, so it's really not possible for you to keep toggling the
>>> GPIOs such that the bus would work. Is that the case ?
>> Yes, for RTS/CTS based on gpio DMA is not used. They are just toggled.
>> So you probably misunderstood me.
> I understand you -- in case DMA is enabled on the AUART block, your hack
> is no longer capable to working correctly, so everything falls apart.

You still didn't read the code or not carefully :)
If RTS/CTS aren't set as gpio it will work exactly as before - including 
DMA.
Otherwise I wouldn't push the patchset.

>
>>> [...]
>>>
>>>>>> Now the question: "fsl,uart-has-rtscts" name seems to be misleading
>>>>>> now, do you agree? It rather should include "dma" word in the name.
>>>>>> Any suggestion?
>>>>>>
>>>>>> [3] http://thread.gmane.org/gmane.linux.serial/16069/focus=16077
>>>>> The best suggestion I can give you is to fix your hardware early,
>>>>> before you run into nasty deep s.....tuff. These workarounds do not
>>>>> work and they will bit you later on, when it's hard to fix the
>>>>> hardware anymore.
>>>> The speed is limited but why don't you accept SW-HW mixed solutions?
>>> Did you read up on the RS485 timing problems and why that solution was
>>> never accepted for any driver ? I believe the threads explained that
>>> quite clearly.
>> Example of RS485 implementation where RTS is toggled by software:
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/dri
>> vers/tty/serial/omap-serial.c?id=4a0ac0f55b18dc297a87a85417fcf068658bf103
>> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/
>> drivers/tty/serial/omap-serial.c
> This is a question for Greg. I checked the discussion about this patch [1]
> and I see this timing issue was brought up, but the patch was applied anyway.
> I cannot tell you why , I just know that this GPIO approach has problems and
> I wrestled those a couple of days ago (without success, it's not possible to
> get correct timing).
>
> [1] http://www.spinics.net/lists/linux-serial/msg10574.html

Yes, this is the question for others - tty/serial's guru.
The timming isn't great but your opinion makes impossible to work most 
RS485 devices,
especially older but not only. You want to change the world here I 
think. In practive
costs usually have higher value than perfect timming if a solution works 
great without.
In the past hardware RS485 support even didn't exist. It is changing but 
slowly.

>>>> Exactly the same method is accepted for 8250.
>>> Can you point out this code please ?
>> If 8250 doesn't support auto flow control RTS/CTS are also toggled by
>> software,
>> uart_trottle(), uart_untrothle(), uart_handle_cts_change():
>> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L635
>> http://lxr.free-electrons.com/source/drivers/tty/serial/serial_core.c#L2796
>> Detected by irq (ms enabled when CRTSCTS) CTS change stops tx because
>> UPF_HARD_FLOW flag is not set by the mxs-auart driver.
> This doesn't use GPIO to toggle the RTS/CTS pins, it does toggle then via the
> 8250 IP block registers, right ?

Right. My patch allows to toggle the pins via SoC's pins - there is a 
choice in DT.
About latency I've written above.

>> Of course timing problem exists but in many cases it is not critical -
>> the toggle method was implemented many years ago and it seems to work.
> Yes, it does seem to work initially, that's why so many hardware people
> implement it, thinking the software people can fix those flubs. Problem
> is, this is one of those nasty problems, which cannot be fixed in software.

Look into the past. And, not each application needs perfect things. 
Hardware people do
not expect of magic things - they must know reality better than you assume.
It seems you have young people's approach. The real world is not 
perfect. There are
costs, limited set of available chips, limited PCB's space, habits, etc.

The thread doesn't concern my patchset at all now :)

thanks for the nice discussion
Janusz

>> The only problem for me is misleading "fsl,uart-has-rtscts" name because
>> the flag
>> only enables DMA if CRTSCTS is set and hardware flow control of AUART
>> block is used.
>>
>> best regards
>> Janusz

^ permalink raw reply	[flat|nested] 36+ messages in thread
end of thread, other threads:[~2014-11-08 13:38 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-10 16:53 [PATCH v4 0/4] serial: mxs-auart: gpios as modem signals Janusz Uzycki
2014-10-10 16:53 ` Janusz Uzycki
2014-10-10 16:53 ` [PATCH v4 1/4] serial: mxs-auart: clean get_mctrl and set_mctrl Janusz Uzycki
2014-10-10 16:53   ` Janusz Uzycki
2014-10-24 15:31   ` Richard Genoud
2014-10-24 15:31     ` Richard Genoud
2014-10-10 16:53 ` [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals Janusz Uzycki
2014-10-10 16:53   ` Janusz Uzycki
2014-10-24 15:32   ` Richard Genoud
2014-10-24 15:32     ` Richard Genoud
2014-10-24 15:51     ` Janusz Użycki
2014-10-24 15:51       ` Janusz Użycki
2014-10-31  8:48       ` Richard Genoud
2014-10-31  8:48         ` Richard Genoud
2014-11-06 11:13         ` Janusz Użycki
2014-11-06 11:13           ` Janusz Użycki
     [not found]           ` <CAMiH66FKJm8hcgtR=-ZzzpCq+PQ8xkixbUnMzRPVd_cM_6VM1w@mail.gmail.com>
2014-11-07  8:03             ` Marek Vasut
2014-11-07  8:03               ` Marek Vasut
2014-11-07 10:04               ` Janusz Użycki
2014-11-07 10:04                 ` Janusz Użycki
2014-11-07 11:02                 ` Marek Vasut
2014-11-07 11:02                   ` Marek Vasut
2014-11-07 13:23                   ` Janusz Użycki
2014-11-07 13:23                     ` Janusz Użycki
2014-11-07 14:48                     ` Marek Vasut
2014-11-07 14:48                       ` Marek Vasut
2014-11-07 16:29                       ` Janusz Użycki
2014-11-07 16:29                         ` Janusz Użycki
2014-11-08 11:22                         ` Marek Vasut
2014-11-08 11:22                           ` Marek Vasut
2014-11-08 13:38                           ` Janusz Użycki
2014-11-08 13:38                             ` Janusz Użycki
2014-10-10 16:53 ` [PATCH v4 3/4] serial: mxs-auart: add interrupts for modem control lines Janusz Uzycki
2014-10-10 16:53   ` Janusz Uzycki
2014-10-10 16:53 ` [PATCH v4 4/4] serial: mxs-auart: enable PPS support Janusz Uzycki
2014-10-10 16:53   ` Janusz Uzycki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.