| Message ID | 1411811197-12638-3-git-send-email-j.uzycki@elproma.com.pl |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sat, Sep 27, 2014 at 11:46:35AM +0200, Janusz Uzycki wrote: > @@ -635,7 +645,12 @@ static void mxs_auart_settermios(struct uart_port *u, > ctrl |= AUART_LINECTRL_STP2; > > /* figure out the hardware flow control settings */ > - if (cflag & CRTSCTS) { > + /* FIXME: Likely DMA could be enabled not only for HW flow control */ > + if (cflag & CRTSCTS && > + (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, > + UART_GPIO_RTS)) || > + IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, > + UART_GPIO_CTS)))) { There has to be a better way to do this. > +static int 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 -1; > + return 0; I personally hate the practise of returning -1 - it ends up getting returned to userspace as an error code, and it means EPERM. If you want to indicate success/failure, use the bool return type, and return true/false. > @@ -1074,6 +1100,11 @@ static int mxs_auart_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, s); > > + ret = mxs_auart_init_gpios(s, &pdev->dev); > + if (ret < 0) > + dev_err(&pdev->dev, "%s", > + "Failed to initialize GPIOs. The serial port may not work as expected"); Does the "%s" do anything for this message? More importantly though, it's lacking a newline.
Could somebody comment if DMA could be enabled also if CTS/RTS is supported by GPIOs? Is it safe? It would allow me to avoid ugly code in mxs_auart_settermios(), as commented Russel King. Otherwise I have to keep backward compatibility on DMA and HW flow control, and fix the ugly code. Marek Vasut: "Why do you think DMA would do any good on long transfers without flowcontrol." In past: "serial: mxs: enable the DMA only when the RTS/CTS is valid": https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/drivers/tty/serial/mxs-auart.c?id=8418e67d95235c3449df6f2e5b33863343fa72f9 "The original DMA support works only when RTS/CTS is enabled. (see the "e800163 serial: mxs-auart: add the DMA support for mx28") But after several patches, DMA support has lost this limit. (see the "bcc20f9 serial: mxs-auart: move to use generic DMA helper") So an UART without the RTS/CTS lines may also enables the DMA support, in which case the UART may gets unpredictable results. This patch adds an optional property for the UART DT node which indicates the UART has RTS and CTS lines, and it also means you enable the DMA support for this UART. This patch also adds a macro MXS_AUART_RTSCTS, and uses it to check RTS/CTS before we enable the DMA for the UART. " best regards Janusz W dniu 2014-09-27 11:46, Janusz Uzycki pisze: > 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> > --- > .../devicetree/bindings/serial/fsl-mxs-auart.txt | 10 +++++- > drivers/tty/serial/Kconfig | 1 + > drivers/tty/serial/mxs-auart.c | 37 ++++++++++++++++++++-- > 3 files changed, 44 insertions(+), 4 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 c712108..fdfa8a9 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,10 +423,13 @@ 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); > int ctrl2 = readl(u->membase + AUART_CTRL2); > u32 mctrl = 0; > @@ -431,7 +441,7 @@ static u32 mxs_auart_get_mctrl(struct uart_port *u) > if (ctrl2 & AUART_CTRL2_RTS) > mctrl |= TIOCM_RTS; > > - return mctrl; > + return mctrl_gpio_get(s->gpios, &mctrl); > } > > static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s); > @@ -635,7 +645,12 @@ static void mxs_auart_settermios(struct uart_port *u, > ctrl |= AUART_LINECTRL_STP2; > > /* figure out the hardware flow control settings */ > - if (cflag & CRTSCTS) { > + /* FIXME: Likely DMA could be enabled not only for HW flow control */ > + if (cflag & CRTSCTS && > + (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, > + UART_GPIO_RTS)) || > + IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, > + UART_GPIO_CTS)))) { > /* > * The DMA has a bug(see errata:2836) in mx23. > * So we can not implement the DMA for auart in mx23, > @@ -695,7 +710,10 @@ 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 (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, > + UART_GPIO_CTS))) > + uart_handle_cts_change(&s->port, > + stat & AUART_STAT_CTS); > writel(AUART_INTR_CTSMIS, > s->port.membase + AUART_INTR_CLR); > istat &= ~AUART_INTR_CTSMIS; > @@ -1019,6 +1037,14 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s, > return 0; > } > > +static int 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 -1; > + return 0; > +} > + > static int mxs_auart_probe(struct platform_device *pdev) > { > const struct of_device_id *of_id = > @@ -1074,6 +1100,11 @@ static int mxs_auart_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, s); > > + ret = mxs_auart_init_gpios(s, &pdev->dev); > + if (ret < 0) > + dev_err(&pdev->dev, "%s", > + "Failed to initialize GPIOs. The serial port may not work as expected"); > + > auart_port[s->port.line] = s; > > mxs_auart_reset(&s->port); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 27, 2014 at 12:33:16PM +0200, Janusz Użycki wrote: > Could somebody comment if DMA could be enabled also > if CTS/RTS is supported by GPIOs? Is it safe? > It would allow me to avoid ugly code in mxs_auart_settermios(), > as commented Russel King. Russel*L* please, otherwise you are giving me implicit permission to mis-spell your name. > Otherwise I have to keep backward compatibility on DMA > and HW flow control, and fix the ugly code. > > Marek Vasut: > "Why do you think DMA would do any good on long transfers without > flowcontrol." The problem with DMA without hardware flow control is that the UART transmission will run at full speed, with characters back to back. You will have a variable amount of latency when dealing with the CTS signal - interrupt handling latency in Linux can be quite variable. (I'm not sure whether this has improved recently, USB used to be particularly bad, causing 3+ ms latencies.) When CTS is deasserted, you have to pause the transmission as soon as possible to avoid overflowing the remote end. At 115200 baud, each character takes around 90us to be transmitted. If you are delayed in handling the CTS interrupt for 3ms, then you will have transmitted around 35 characters in that time, which could be enough to cause an overflow at the remote end. Since the idea of flow control is to prevent overruns/overflow at the receiver, this is far from ideal. If Linux had a way to handle flow control interrupts as a higher priority than other interrupts (especially interrupting an already in-progress interrupt handler) then having GPIOs as flow control signals would be a much saner proposition. Unfortunately, lockdep completely gets in the way of that.
W dniu 2014-09-27 12:54, Russell King - ARM Linux pisze: > On Sat, Sep 27, 2014 at 12:33:16PM +0200, Janusz Użycki wrote: >> Could somebody comment if DMA could be enabled also >> if CTS/RTS is supported by GPIOs? Is it safe? >> It would allow me to avoid ugly code in mxs_auart_settermios(), >> as commented Russel King. > Russel*L* please, otherwise you are giving me implicit permission to > mis-spell your name. Sorry. I will care about. Many people change my name also from Janusz to Januzs. >> Otherwise I have to keep backward compatibility on DMA >> and HW flow control, and fix the ugly code. >> >> Marek Vasut: >> "Why do you think DMA would do any good on long transfers without >> flowcontrol." > The problem with DMA without hardware flow control is that the UART > transmission will run at full speed, with characters back to back. > > You will have a variable amount of latency when dealing with the CTS > signal - interrupt handling latency in Linux can be quite variable. > (I'm not sure whether this has improved recently, USB used to be > particularly bad, causing 3+ ms latencies.) > > When CTS is deasserted, you have to pause the transmission as soon as > possible to avoid overflowing the remote end. At 115200 baud, each > character takes around 90us to be transmitted. If you are delayed in > handling the CTS interrupt for 3ms, then you will have transmitted > around 35 characters in that time, which could be enough to cause an > overflow at the remote end. > > Since the idea of flow control is to prevent overruns/overflow at the > receiver, this is far from ideal. > > If Linux had a way to handle flow control interrupts as a higher > priority than other interrupts (especially interrupting an already > in-progress interrupt handler) then having GPIOs as flow control > signals would be a much saner proposition. > > Unfortunately, lockdep completely gets in the way of that. It means it is possible but not in Linux today? I think the problem is not interrupt delay, which is often much below 100us (we get ~10us delay on DCD). USB or PCIe are exceptions - interrupt is packed to frame by serialized and even polled by USB controller. Isn't the problem rather how to break DMA transmission fast? Hardware flow control not always requires to stop transmission immediately. Usually delay of some chars is acceptable if remote receiver implements low/high water mark (overflow trigger lower than RX FIFO size). best regards Janusz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 27, 2014 at 01:15:00PM +0200, Janusz Użycki wrote: > It means it is possible but not in Linux today? It depends on your maximum interrupt latency. > I think the problem is not interrupt delay, which is often much below 100us > (we get ~10us delay on DCD). http://archive.arm.linux.org.uk/lurker/message/20130724.145238.44ad37b5.en.html I can believe why this all happens, when you see USB interrupts taking upwards of 3ms to complete: Longest time: 3247506ns Longest irq: 24 Longest handler: usb_hcd_irq+0x0/0x68 This no longer seems to be the case - the maximum interrupt time I'm seeing on recent kernels is: Longest time: 382236ns Longest irq: 62 Longest handler: mv_interrupt+0x0/0x948 My Dove kernel runs permanently with my own maximum interrupt latency tracking enabled. > Isn't the problem rather how to break DMA transmission fast? There are two factors there. The first is being able to tell the DMA engine to stop, the second is the UART hardware FIFO draining. > Hardware flow control not always requires to stop transmission > immediately. No. Hardware flow control is normally implemented at the UART, and CTS is normally implemented to prevent the transmitter starting a new character. Any in-progress character is completed before the transmitter stops, so that there are no errors. At least, this is the behaviour found on the UARTs which I've seen implementing hardware flow control, and it is the only sane way to do it. > Usually delay of some chars is acceptable if remote receiver > implements low/high water mark (overflow trigger lower than RX FIFO size). The receiver you may be communicating with may have a receive FIFO of only 16 characters. It may deassert CTS when it reaches half-full state. That leaves you with only 8 characters to send before it overflows. At 115200 baud, you will have started to send the 9th characters by 700us. However, that isn't the full picture. The full picture is that CTS is normally controlled by software as well, and depends on the availability of buffers to store characters. There's latency at the remote end to deassert CTS in software when the software buffers reach their high watermark, and there's the hope that there is sufficient room in those buffers to take the excess characters that the remote end may continue to send.
Is it better in mxs_auart_init_gpios(): a) to block DMA by disabling MXS_AUART_RTSCTS (misleading name problem) b) or just warn (depends on DT) if MXS_AUART_RTSCTS is set and CTS or RTS is defined as GPIO line? Then MXS_AUART_RTSCTS could be used instead of CTS_AT_AUART() in the interrupt and mxs_auart_settermios(). What do you think about RTSEN? It is set in mxs_auart_settermios() and mxs_auart_set_mctrl(). [PATCH 1/2] serial: mxs-auart: use mctrl_gpio helpers for handling * RTS_AT_AUART() and CTS_AT_AUART() macro defined * DMA engine disabled if RTS or CTS is GPIO line * 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 2/2] serial: mxs-auart: add interrupts for modem control * rebased -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 c712108..fdfa8a9 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,10 +423,13 @@ 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); int ctrl2 = readl(u->membase + AUART_CTRL2); u32 mctrl = 0; @@ -431,7 +441,7 @@ static u32 mxs_auart_get_mctrl(struct uart_port *u) if (ctrl2 & AUART_CTRL2_RTS) mctrl |= TIOCM_RTS; - return mctrl; + return mctrl_gpio_get(s->gpios, &mctrl); } static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s); @@ -635,7 +645,12 @@ static void mxs_auart_settermios(struct uart_port *u, ctrl |= AUART_LINECTRL_STP2; /* figure out the hardware flow control settings */ - if (cflag & CRTSCTS) { + /* FIXME: Likely DMA could be enabled not only for HW flow control */ + if (cflag & CRTSCTS && + (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, + UART_GPIO_RTS)) || + IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, + UART_GPIO_CTS)))) { /* * The DMA has a bug(see errata:2836) in mx23. * So we can not implement the DMA for auart in mx23, @@ -695,7 +710,10 @@ 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 (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(s->gpios, + UART_GPIO_CTS))) + uart_handle_cts_change(&s->port, + stat & AUART_STAT_CTS); writel(AUART_INTR_CTSMIS, s->port.membase + AUART_INTR_CLR); istat &= ~AUART_INTR_CTSMIS; @@ -1019,6 +1037,14 @@ static int serial_mxs_probe_dt(struct mxs_auart_port *s, return 0; } +static int 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 -1; + return 0; +} + static int mxs_auart_probe(struct platform_device *pdev) { const struct of_device_id *of_id = @@ -1074,6 +1100,11 @@ static int mxs_auart_probe(struct platform_device *pdev) platform_set_drvdata(pdev, s); + ret = mxs_auart_init_gpios(s, &pdev->dev); + if (ret < 0) + dev_err(&pdev->dev, "%s", + "Failed to initialize GPIOs. The serial port may not work as expected"); + auart_port[s->port.line] = s; mxs_auart_reset(&s->port);
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> --- .../devicetree/bindings/serial/fsl-mxs-auart.txt | 10 +++++- drivers/tty/serial/Kconfig | 1 + drivers/tty/serial/mxs-auart.c | 37 ++++++++++++++++++++-- 3 files changed, 44 insertions(+), 4 deletions(-)