From mboxrd@z Thu Jan 1 00:00:00 1970 From: j.uzycki@elproma.com.pl (=?UTF-8?B?SmFudXN6IFXFvHlja2k=?=) Date: Thu, 06 Nov 2014 12:13:27 +0100 Subject: [PATCH v4 2/4] serial: mxs-auart: use mctrl_gpio helpers for handling modem signals In-Reply-To: References: <1412960007-28159-1-git-send-email-j.uzycki@elproma.com.pl> <1412960007-28159-3-git-send-email-j.uzycki@elproma.com.pl> <544A70F6.6080800@gmail.com> <544A756C.7000601@elproma.com.pl> Message-ID: <545B57D7.4070609@elproma.com.pl> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 : >> 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 >>>> --- >>>> 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 >>>> +#include >>>> +#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 >> > >