| Message ID | 1411150399-30902-1-git-send-email-j.uzycki@elproma.com.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Please for comments in order to resend clean patches. 4/4: can be applied separately TODO: - tx_empty fix - port.lock - pm support best regards Janusz W dniu 2014-09-19 20:13, 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 | 77 +++++++++++++++++++++- > 3 files changed, 84 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 52d14e3..4690200 100644 > --- a/drivers/tty/serial/mxs-auart.c > +++ b/drivers/tty/serial/mxs-auart.c > @@ -38,6 +38,9 @@ > > #include <asm/cacheflush.h> > > +#include <linux/err.h> > +#include "serial_mctrl_gpio.h" > + > #define MXS_AUART_MAJOR 242 > #define MXS_AUART_PORTS 5 > #define MXS_AUART_FIFO_SIZE 16 > @@ -156,6 +159,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[] = { > @@ -415,8 +420,43 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl) > ctrl |= AUART_CTRL2_RTS; > } > > + /* Does tty layer modify mctrl for input lines > + * and the code below is required? */ > +#if 0 > s->ctrl = mctrl; > +#else > + s->ctrl &= ~(TIOCM_RTS | TIOCM_DTR); > + s->ctrl |= mctrl & (TIOCM_RTS | TIOCM_DTR); > +#endif > writel(ctrl, u->membase + AUART_CTRL2); > + > + mctrl_gpio_set(s->gpios, mctrl); > +} > + > +/* mxs_auart_modem_status() based on 8250.c, > + * similar function could be as serial's helper... */ > +#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->ctrl; > + s->ctrl = mctrl; > + if (mctrl_diff & MCTRL_ANY_DELTA && > + /*interrupt_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) > @@ -433,7 +473,9 @@ static u32 mxs_auart_get_mctrl(struct uart_port *u) > if (ctrl2 & AUART_CTRL2_RTS) > mctrl |= TIOCM_RTS; > > - return mctrl; > + /* should mxs_auart_modem_status() be in interrupt only? */ > + return mxs_auart_modem_status(s, > + mctrl_gpio_get(s->gpios, &mctrl)); > } > > static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s); > @@ -637,7 +679,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: DMA for hardware flow control only? */ > + 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, > @@ -696,8 +743,14 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context) > | AUART_INTR_CTSMIS), > s->port.membase + AUART_INTR_CLR); > > + mxs_auart_modem_status(s, > + mctrl_gpio_get(s->gpios, &s->ctrl)); > + > 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; > @@ -758,6 +811,8 @@ 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->ctrl); > return 0; > } > > @@ -1021,6 +1076,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 = > @@ -1078,6 +1141,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); > @@ -1094,6 +1162,7 @@ static int mxs_auart_probe(struct platform_device *pdev) > return 0; > > out_free_irq: > + mctrl_gpio_free(&pdev->dev, s->gpios); /*useless?*/ > auart_port[pdev->id] = NULL; > free_irq(s->irq, s); > out_free_clk: > @@ -1112,6 +1181,8 @@ static int mxs_auart_remove(struct platform_device *pdev) > > auart_port[pdev->id] = NULL; > > + mctrl_gpio_free(&pdev->dev, s->gpios); /*useless?*/ > + > clk_put(s->clk); > free_irq(s->irq, s); > kfree(s);
W dniu 2014-09-25 17:29, Richard Genoud pisze: > 2014-09-20 10:08 GMT+02:00 Janusz U?ycki <j.uzycki@elproma.com.pl>: > Hi, Hi Richard, first, thanks for the very useful answers. >> Could you look on my patch series [1...4] >> serial: mxs-auart: >> use mctrl_gpio helpers for handling modem signals (v2.2c) >> at http://news.gmane.org/gmane.linux.serial ? > If you resend them, put me in the CC list. > That will be easier for me to have a look at them. sure >> I added some comments in patch's code. >> In atmel_serial mctrl_gpio_free() is ommited, >> is it useless? > If you take a look at mctrl_gpio_init(), you'll see that all the > allocations are done with devm_* functions. > So that they are automatically deallocated when the module unloads. > I still wrote mctrl_gpio_free() for some cases where we want to free > memory without unloading the module. > > So you don't have to call mctrl_gpio_free() at the end of > mxs_auart_probe() and mxs_auart_remove() > (cf Documentation/serial/driver ) It is clear now, thanks. >> Atmel_serial also does not parse modem >> line status in mxs_auart_get_mctrl(), >> 8250 driver does it. Which solution is ok? > Well, in atmel_get_mctrl(), we have to retrieve the line status from > the uart controller : > that's done with : > status = UART_GET_CSR(port); > Then, for the lines controlled via GPIO, we are calling > mctrl_gpio_get() that updates the mctrl flags with gpios states. > > It doesn't seems different from 8250. But serial8250_get_mctrl() in 8250_core.c calls serial8250_modem_status() which calls eg. uart_handle_cts_change() even if enable_ms() wasn't called. This is the difference. The serial8250_modem_status() is also called in the interrupt and, what I don't understand, in serial8250_console_write(). >> Do you happen to know if tty layer >> modifies mctrl between mxs_auart_get_mctrl(), >> mxs_auart_set_mctrl() and mxs_auart_get_mctrl() again? > I don't quite understand what you mean. > The only way for the kernel to get line status (DCD, CTS,DSR,RI) is > via get_mctrl. > And the only way for the kernel to set line status > (RTS,DTR,out1,out2,loop) is via set_mctrl. Yes, but: * mxs_auart_set_mctrl() in the mxs-auart stores the lastest mctrl in private ctrl field * the stored value is used to supply initial returned value by mxs_auart_get_mctrl() * uart_update_mctrl() in serial_core.c modifies only selected bits in port->mctrl field - OK, input bits stay untouched * uart_port_startup() in serial_code.c sets RTS and DTR and zeroes others (also input bits) * uart_set_termios() in serial_core.c modifies the bits too * uart_suspend_port() in serial_core.c sets all mctrl bits to 0 It looks that mxs_auart_set_mctrl() doesn't have to store mctrl at all because uart_tiocmget() in serial_core.c restores the output bits from port->mctrl field. So the code is duplicated. Do you agree? P.S. It was even implemented in serial_core.c in 2.6.12. best regards Janusz > best regards, > Richard.
On Fri, Sep 26, 2014 at 02:59:18PM +0200, Janusz U?ycki wrote: > But serial8250_get_mctrl() in 8250_core.c calls serial8250_modem_status() > which calls eg. uart_handle_cts_change() even if enable_ms() wasn't called. > This is the difference. > The serial8250_modem_status() is also called in the interrupt > and, what I don't understand, in serial8250_console_write(). Reading the MSR register clears the interrupts. So, whenever MSR is read, you have to deal with any state changes which _would_ have been passed to the interrupt function. Plus, it's not quite as you make out above. If enable_ms() is not called, then UART_IER_MSI will not be set in up->ier. Hence, uart_handle_cts_change() will not be called. The reason for the call in the console function is to account for the state changes during console write with CTS flow control - this again needs the MSR register to be read, and we have to account for MSR state changes after the console write has completed.
W dniu 2014-09-26 15:11, Russell King - ARM Linux pisze: > On Fri, Sep 26, 2014 at 02:59:18PM +0200, Janusz U?ycki wrote: >> But serial8250_get_mctrl() in 8250_core.c calls serial8250_modem_status() >> which calls eg. uart_handle_cts_change() even if enable_ms() wasn't called. >> This is the difference. >> The serial8250_modem_status() is also called in the interrupt >> and, what I don't understand, in serial8250_console_write(). > Reading the MSR register clears the interrupts. So, whenever MSR is read, > you have to deal with any state changes which _would_ have been passed > to the interrupt function. > > Plus, it's not quite as you make out above. > > If enable_ms() is not called, then UART_IER_MSI will not be set in up->ier. > Hence, uart_handle_cts_change() will not be called. Oh, now I understand the condition "up->ier & UART_IER_MSI" in serial8250_modem_status(). So in case of gpio modem lines interrupt is the only right place like in atmel_serial.c. > The reason for the call in the console function is to account for the > state changes during console write with CTS flow control - this again > needs the MSR register to be read, and we have to account for MSR state > changes after the console write has completed. It means that gpio modem lines does not support flow control for console on the moment, right Richard? best regards Janusz
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 52d14e3..4690200 100644 --- a/drivers/tty/serial/mxs-auart.c +++ b/drivers/tty/serial/mxs-auart.c @@ -38,6 +38,9 @@ #include <asm/cacheflush.h> +#include <linux/err.h> +#include "serial_mctrl_gpio.h" + #define MXS_AUART_MAJOR 242 #define MXS_AUART_PORTS 5 #define MXS_AUART_FIFO_SIZE 16 @@ -156,6 +159,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[] = { @@ -415,8 +420,43 @@ static void mxs_auart_set_mctrl(struct uart_port *u, unsigned mctrl) ctrl |= AUART_CTRL2_RTS; } + /* Does tty layer modify mctrl for input lines + * and the code below is required? */ +#if 0 s->ctrl = mctrl; +#else + s->ctrl &= ~(TIOCM_RTS | TIOCM_DTR); + s->ctrl |= mctrl & (TIOCM_RTS | TIOCM_DTR); +#endif writel(ctrl, u->membase + AUART_CTRL2); + + mctrl_gpio_set(s->gpios, mctrl); +} + +/* mxs_auart_modem_status() based on 8250.c, + * similar function could be as serial's helper... */ +#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->ctrl; + s->ctrl = mctrl; + if (mctrl_diff & MCTRL_ANY_DELTA && + /*interrupt_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) @@ -433,7 +473,9 @@ static u32 mxs_auart_get_mctrl(struct uart_port *u) if (ctrl2 & AUART_CTRL2_RTS) mctrl |= TIOCM_RTS; - return mctrl; + /* should mxs_auart_modem_status() be in interrupt only? */ + return mxs_auart_modem_status(s, + mctrl_gpio_get(s->gpios, &mctrl)); } static int mxs_auart_dma_prep_rx(struct mxs_auart_port *s); @@ -637,7 +679,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: DMA for hardware flow control only? */ + 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, @@ -696,8 +743,14 @@ static irqreturn_t mxs_auart_irq_handle(int irq, void *context) | AUART_INTR_CTSMIS), s->port.membase + AUART_INTR_CLR); + mxs_auart_modem_status(s, + mctrl_gpio_get(s->gpios, &s->ctrl)); + 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; @@ -758,6 +811,8 @@ 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->ctrl); return 0; } @@ -1021,6 +1076,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 = @@ -1078,6 +1141,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); @@ -1094,6 +1162,7 @@ static int mxs_auart_probe(struct platform_device *pdev) return 0; out_free_irq: + mctrl_gpio_free(&pdev->dev, s->gpios); /*useless?*/ auart_port[pdev->id] = NULL; free_irq(s->irq, s); out_free_clk: @@ -1112,6 +1181,8 @@ static int mxs_auart_remove(struct platform_device *pdev) auart_port[pdev->id] = NULL; + mctrl_gpio_free(&pdev->dev, s->gpios); /*useless?*/ + clk_put(s->clk); free_irq(s->irq, s); kfree(s);
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 | 77 +++++++++++++++++++++- 3 files changed, 84 insertions(+), 4 deletions(-)