2014-10-24 17:51 GMT+02:00 Janusz Użycki <j.uzycki@xxxxxxxxxxxxxx>:
>
> 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?
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@xxxxxxxxx>
>
>
--
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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
