Re: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxx>

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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux