[RFC] tty/serial/8250: use mctrl_gpio helpers

Message ID	1457521394-1328-1-git-send-email-yegorslists@googlemail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-omap-owner@kernel.org> From: yegorslists@googlemail.com To: linux-serial@vger.kernel.org Cc: linux-omap@vger.kernel.org, gregkh@linuxfoundation.org, nm@ti.com, bigeasy@linutronix.de, tony@atomide.com, nsekhar@ti.com, Yegor Yefremov <yegorslists@googlemail.com> Subject: [RFC] tty/serial/8250: use mctrl_gpio helpers Date: Wed, 9 Mar 2016 12:03:14 +0100 Message-Id: <1457521394-1328-1-git-send-email-yegorslists@googlemail.com> Sender: linux-omap-owner@vger.kernel.org Precedence: bulk

Yegor Yefremov March 9, 2016, 11:03 a.m. UTC

From: Yegor Yefremov <yegorslists@googlemail.com>

This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
signals.

Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
---
 Documentation/devicetree/bindings/serial/8250.txt | 21 +++++++++++++++++++++
 drivers/tty/serial/8250/8250.h                    |  2 ++
 drivers/tty/serial/8250/8250_core.c               |  5 +++++
 drivers/tty/serial/8250/8250_port.c               | 11 ++++++++++-
 drivers/tty/serial/8250/Kconfig                   |  1 +
 include/linux/serial_8250.h                       |  1 +
 6 files changed, 40 insertions(+), 1 deletion(-)

Yegor Yefremov March 9, 2016, 11:07 a.m. UTC | #1

On Wed, Mar 9, 2016 at 12:03 PM,  <yegorslists@googlemail.com> wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
> signals.
>
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
>  Documentation/devicetree/bindings/serial/8250.txt | 21 +++++++++++++++++++++
>  drivers/tty/serial/8250/8250.h                    |  2 ++
>  drivers/tty/serial/8250/8250_core.c               |  5 +++++
>  drivers/tty/serial/8250/8250_port.c               | 11 ++++++++++-
>  drivers/tty/serial/8250/Kconfig                   |  1 +
>  include/linux/serial_8250.h                       |  1 +
>  6 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> index 936ab5b..277e0ee 100644
> --- a/Documentation/devicetree/bindings/serial/8250.txt
> +++ b/Documentation/devicetree/bindings/serial/8250.txt
> @@ -42,6 +42,9 @@ Optional properties:
>  - auto-flow-control: one way to enable automatic flow control support. The
>    driver is allowed to detect support for the capability even without this
>    property.
> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
> +  line respectively. It will use specified GPIO instead of the peripheral
> +  function pin for the UART feature. If unsure, don't specify this property.
>
>  Note:
>  * fsl,ns16550:
> @@ -63,3 +66,21 @@ Example:
>                 interrupts = <10>;
>                 reg-shift = <2>;
>         };
> +
> +Example for OMAP UART using GPIO-based modem control signals:
> +
> +       uart4: serial@49042000 {
> +               compatible = "ti,omap3-uart";
> +               reg = <0x49042000 0x400>;
> +               interrupts = <80>;
> +               dmas = <&sdma 81 &sdma 82>;
> +               dma-names = "tx", "rx";
> +               ti,hwmods = "uart4";
> +               clock-frequency = <48000000>;
> +               cts-gpios = <&gpio3 5 GPIO_ACTIVE_LOW>;
> +               rts-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
> +               dtr-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
> +               dsr-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> +               dcd-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
> +               rng-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
> +       };
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index d54dcd8..98ee73c 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -15,6 +15,8 @@
>  #include <linux/serial_reg.h>
>  #include <linux/dmaengine.h>
>
> +#include "../serial_mctrl_gpio.h"
> +
>  struct uart_8250_dma {
>         int (*tx_dma)(struct uart_8250_port *p);
>         int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index c9720a9..cadcaa5 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1004,6 +1004,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>                 if (up->port.flags & UPF_FIXED_TYPE)
>                         uart->port.type = up->port.type;
>
> +               uart->gpios = mctrl_gpio_init(&uart->port, 0);
> +               if (IS_ERR(uart->gpios))
> +                       return PTR_ERR(uart->gpios);
> +
>                 serial8250_set_defaults(uart);
>
>                 /* Possibly override default I/O functions.  */
> @@ -1049,6 +1053,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>
>                         ret = 0;
>                 }
> +
>         }
>         mutex_unlock(&serial_mutex);
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 8d262bc..e7bfeef 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1393,6 +1393,8 @@ static void serial8250_disable_ms(struct uart_port *port)
>         if (up->bugs & UART_BUG_NOMSR)
>                 return;
>
> +       mctrl_gpio_disable_ms(up->gpios);
> +
>         up->ier &= ~UART_IER_MSI;
>         serial_port_out(port, UART_IER, up->ier);
>  }
> @@ -1405,6 +1407,8 @@ static void serial8250_enable_ms(struct uart_port *port)
>         if (up->bugs & UART_BUG_NOMSR)
>                 return;
>
> +       mctrl_gpio_enable_ms(up->gpios);
> +
>         up->ier |= UART_IER_MSI;
>
>         serial8250_rpm_get(up);
> @@ -1680,7 +1684,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>                 ret |= TIOCM_DSR;
>         if (status & UART_MSR_CTS)
>                 ret |= TIOCM_CTS;
> -       return ret;
> +
> +       return mctrl_gpio_get(up->gpios, &ret);
>  }
>
>  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
> @@ -1707,10 +1712,14 @@ EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
>
>  static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
> +       struct uart_8250_port *up = up_to_u8250p(port);
> +
>         if (port->set_mctrl)
>                 port->set_mctrl(port, mctrl);
>         else
>                 serial8250_do_set_mctrl(port, mctrl);
> +
> +       mctrl_gpio_set(up->gpios, mctrl);
>  }
>
>  static void serial8250_break_ctl(struct uart_port *port, int break_state)
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index b03cb517..997e9a8 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -6,6 +6,7 @@
>  config SERIAL_8250
>         tristate "8250/16550 and compatible serial support"
>         select SERIAL_CORE
> +       select SERIAL_MCTRL_GPIO if GPIOLIB
>         ---help---
>           This selects whether you want to include the driver for the standard
>           serial ports.  The standard answer is Y.  People who might say N
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index faa0e03..86a3ef4 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -104,6 +104,7 @@ struct uart_8250_port {
>                                                  *   if no_console_suspend
>                                                  */
>         unsigned char           probe;
> +       struct mctrl_gpios      *gpios;
>  #define UART_PROBE_RSA (1 << 0)
>
>         /*

This patch was tested on a am335x based machine. So far everything is
working as expected aside from interrupts. As soon as the first CTS
interrupt comes I get:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at drivers/tty/serial/serial_core.c:2893
uart_handle_cts_change+0x9c/0xd8()
Modules linked in: musb_dsps musb_am335x
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.5.0-rc6 #37
Hardware name: Generic AM33XX (Flattened Device Tree)
[<c0017e34>] (unwind_backtrace) from [<c0014088>] (show_stack+0x10/0x14)
[<c0014088>] (show_stack) from [<c02c5b24>] (dump_stack+0xb0/0xe4)
[<c02c5b24>] (dump_stack) from [<c003bca4>] (warn_slowpath_common+0x7c/0xb8)
[<c003bca4>] (warn_slowpath_common) from [<c003bd7c>]
(warn_slowpath_null+0x1c/0x24)
[<c003bd7c>] (warn_slowpath_null) from [<c0337bc8>]
(uart_handle_cts_change+0x9c/0xd8)
[<c0337bc8>] (uart_handle_cts_change) from [<c033f430>]
(mctrl_gpio_irq_handle+0xb0/0xcc)
[<c033f430>] (mctrl_gpio_irq_handle) from [<c0096db4>]
(handle_irq_event_percpu+0x50/0x268)
[<c0096db4>] (handle_irq_event_percpu) from [<c0097004>]
(handle_irq_event+0x38/0x5c)
[<c0097004>] (handle_irq_event) from [<c009a3a0>] (handle_edge_irq+0xf0/0x1bc)
[<c009a3a0>] (handle_edge_irq) from [<c0096438>] (generic_handle_irq+0x20/0x34)
[<c0096438>] (generic_handle_irq) from [<c0302038>]
(omap_gpio_irq_handler+0x12c/0x16c)
[<c0302038>] (omap_gpio_irq_handler) from [<c0096db4>]
(handle_irq_event_percpu+0x50/0x268)
[<c0096db4>] (handle_irq_event_percpu) from [<c0097004>]
(handle_irq_event+0x38/0x5c)
[<c0097004>] (handle_irq_event) from [<c009a070>] (handle_level_irq+0xb8/0x14c)
[<c009a070>] (handle_level_irq) from [<c0096438>] (generic_handle_irq+0x20/0x34)
[<c0096438>] (generic_handle_irq) from [<c009672c>]
(__handle_domain_irq+0x64/0xe0)
[<c009672c>] (__handle_domain_irq) from [<c06802f8>] (__irq_svc+0x58/0x78)
[<c06802f8>] (__irq_svc) from [<c00103a0>] (arch_cpu_idle+0x20/0x3c)
[<c00103a0>] (arch_cpu_idle) from [<c007fea4>] (cpu_startup_entry+0x2d0/0x358)
[<c007fea4>] (cpu_startup_entry) from [<c08eac24>] (start_kernel+0x364/0x3e8)
---[ end trace 68b3b5769f257e1c ]---

When I toggle on CTS once more I don't get such an error and for both
toggle sessions I get the proper CTS value.

Any idea?

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

Uwe Kleine-König March 10, 2016, 7:43 a.m. UTC | #2

Hello,

On Wed, Mar 09, 2016 at 12:07:39PM +0100, Yegor Yefremov wrote:
> On Wed, Mar 9, 2016 at 12:03 PM,  <yegorslists@googlemail.com> wrote:
> > From: Yegor Yefremov <yegorslists@googlemail.com>
> >
> > This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
> > signals.
> >
> > Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> > ---
> >  Documentation/devicetree/bindings/serial/8250.txt | 21 +++++++++++++++++++++
> >  drivers/tty/serial/8250/8250.h                    |  2 ++
> >  drivers/tty/serial/8250/8250_core.c               |  5 +++++
> >  drivers/tty/serial/8250/8250_port.c               | 11 ++++++++++-
> >  drivers/tty/serial/8250/Kconfig                   |  1 +
> >  include/linux/serial_8250.h                       |  1 +
> >  6 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> > index 936ab5b..277e0ee 100644
> > --- a/Documentation/devicetree/bindings/serial/8250.txt
> > +++ b/Documentation/devicetree/bindings/serial/8250.txt
> > @@ -42,6 +42,9 @@ Optional properties:
> >  - auto-flow-control: one way to enable automatic flow control support. The
> >    driver is allowed to detect support for the capability even without this
> >    property.
> > +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
> > +  line respectively. It will use specified GPIO instead of the peripheral
> > +  function pin for the UART feature. If unsure, don't specify this property.
> >
> >  Note:
> >  * fsl,ns16550:
> > @@ -63,3 +66,21 @@ Example:
> >                 interrupts = <10>;
> >                 reg-shift = <2>;
> >         };
> > +
> > +Example for OMAP UART using GPIO-based modem control signals:
> > +
> > +       uart4: serial@49042000 {
> > +               compatible = "ti,omap3-uart";
> > +               reg = <0x49042000 0x400>;
> > +               interrupts = <80>;
> > +               dmas = <&sdma 81 &sdma 82>;
> > +               dma-names = "tx", "rx";
> > +               ti,hwmods = "uart4";
> > +               clock-frequency = <48000000>;
> > +               cts-gpios = <&gpio3 5 GPIO_ACTIVE_LOW>;
> > +               rts-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
> > +               dtr-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
> > +               dsr-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> > +               dcd-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
> > +               rng-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
> > +       };
> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> > index d54dcd8..98ee73c 100644
> > --- a/drivers/tty/serial/8250/8250.h
> > +++ b/drivers/tty/serial/8250/8250.h
> > @@ -15,6 +15,8 @@
> >  #include <linux/serial_reg.h>
> >  #include <linux/dmaengine.h>
> >
> > +#include "../serial_mctrl_gpio.h"
> > +
> >  struct uart_8250_dma {
> >         int (*tx_dma)(struct uart_8250_port *p);
> >         int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
> > diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> > index c9720a9..cadcaa5 100644
> > --- a/drivers/tty/serial/8250/8250_core.c
> > +++ b/drivers/tty/serial/8250/8250_core.c
> > @@ -1004,6 +1004,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
> >                 if (up->port.flags & UPF_FIXED_TYPE)
> >                         uart->port.type = up->port.type;
> >
> > +               uart->gpios = mctrl_gpio_init(&uart->port, 0);
> > +               if (IS_ERR(uart->gpios))
> > +                       return PTR_ERR(uart->gpios);
> > +
> >                 serial8250_set_defaults(uart);
> >
> >                 /* Possibly override default I/O functions.  */
> > @@ -1049,6 +1053,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
> >
> >                         ret = 0;
> >                 }
> > +

This hunk should be dropped.

Otherwise the patch looks good.

> >         }
> >         mutex_unlock(&serial_mutex);
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 8d262bc..e7bfeef 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -1393,6 +1393,8 @@ static void serial8250_disable_ms(struct uart_port *port)
> >         if (up->bugs & UART_BUG_NOMSR)
> >                 return;
> >
> > +       mctrl_gpio_disable_ms(up->gpios);
> > +
> >         up->ier &= ~UART_IER_MSI;
> >         serial_port_out(port, UART_IER, up->ier);
> >  }
> > @@ -1405,6 +1407,8 @@ static void serial8250_enable_ms(struct uart_port *port)
> >         if (up->bugs & UART_BUG_NOMSR)
> >                 return;
> >
> > +       mctrl_gpio_enable_ms(up->gpios);
> > +
> >         up->ier |= UART_IER_MSI;
> >
> >         serial8250_rpm_get(up);
> > @@ -1680,7 +1684,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
> >                 ret |= TIOCM_DSR;
> >         if (status & UART_MSR_CTS)
> >                 ret |= TIOCM_CTS;
> > -       return ret;
> > +
> > +       return mctrl_gpio_get(up->gpios, &ret);
> >  }
> >
> >  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > @@ -1707,10 +1712,14 @@ EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
> >
> >  static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
> >  {
> > +       struct uart_8250_port *up = up_to_u8250p(port);
> > +
> >         if (port->set_mctrl)
> >                 port->set_mctrl(port, mctrl);
> >         else
> >                 serial8250_do_set_mctrl(port, mctrl);
> > +
> > +       mctrl_gpio_set(up->gpios, mctrl);
> >  }
> >
> >  static void serial8250_break_ctl(struct uart_port *port, int break_state)
> > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> > index b03cb517..997e9a8 100644
> > --- a/drivers/tty/serial/8250/Kconfig
> > +++ b/drivers/tty/serial/8250/Kconfig
> > @@ -6,6 +6,7 @@
> >  config SERIAL_8250
> >         tristate "8250/16550 and compatible serial support"
> >         select SERIAL_CORE
> > +       select SERIAL_MCTRL_GPIO if GPIOLIB
> >         ---help---
> >           This selects whether you want to include the driver for the standard
> >           serial ports.  The standard answer is Y.  People who might say N
> > diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> > index faa0e03..86a3ef4 100644
> > --- a/include/linux/serial_8250.h
> > +++ b/include/linux/serial_8250.h
> > @@ -104,6 +104,7 @@ struct uart_8250_port {
> >                                                  *   if no_console_suspend
> >                                                  */
> >         unsigned char           probe;
> > +       struct mctrl_gpios      *gpios;
> >  #define UART_PROBE_RSA (1 << 0)
> >
> >         /*
> 
> This patch was tested on a am335x based machine. So far everything is
> working as expected aside from interrupts. As soon as the first CTS
> interrupt comes I get:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at drivers/tty/serial/serial_core.c:2893
> uart_handle_cts_change+0x9c/0xd8()
> Modules linked in: musb_dsps musb_am335x
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.5.0-rc6 #37
> Hardware name: Generic AM33XX (Flattened Device Tree)
> [<c0017e34>] (unwind_backtrace) from [<c0014088>] (show_stack+0x10/0x14)
> [<c0014088>] (show_stack) from [<c02c5b24>] (dump_stack+0xb0/0xe4)
> [<c02c5b24>] (dump_stack) from [<c003bca4>] (warn_slowpath_common+0x7c/0xb8)
> [<c003bca4>] (warn_slowpath_common) from [<c003bd7c>]
> (warn_slowpath_null+0x1c/0x24)
> [<c003bd7c>] (warn_slowpath_null) from [<c0337bc8>]
> (uart_handle_cts_change+0x9c/0xd8)
> [<c0337bc8>] (uart_handle_cts_change) from [<c033f430>]
> (mctrl_gpio_irq_handle+0xb0/0xcc)
> [<c033f430>] (mctrl_gpio_irq_handle) from [<c0096db4>]
> (handle_irq_event_percpu+0x50/0x268)
> [<c0096db4>] (handle_irq_event_percpu) from [<c0097004>]
> (handle_irq_event+0x38/0x5c)
> [<c0097004>] (handle_irq_event) from [<c009a3a0>] (handle_edge_irq+0xf0/0x1bc)
> [<c009a3a0>] (handle_edge_irq) from [<c0096438>] (generic_handle_irq+0x20/0x34)
> [<c0096438>] (generic_handle_irq) from [<c0302038>]
> (omap_gpio_irq_handler+0x12c/0x16c)
> [<c0302038>] (omap_gpio_irq_handler) from [<c0096db4>]
> (handle_irq_event_percpu+0x50/0x268)
> [<c0096db4>] (handle_irq_event_percpu) from [<c0097004>]
> (handle_irq_event+0x38/0x5c)
> [<c0097004>] (handle_irq_event) from [<c009a070>] (handle_level_irq+0xb8/0x14c)
> [<c009a070>] (handle_level_irq) from [<c0096438>] (generic_handle_irq+0x20/0x34)
> [<c0096438>] (generic_handle_irq) from [<c009672c>]
> (__handle_domain_irq+0x64/0xe0)
> [<c009672c>] (__handle_domain_irq) from [<c06802f8>] (__irq_svc+0x58/0x78)
> [<c06802f8>] (__irq_svc) from [<c00103a0>] (arch_cpu_idle+0x20/0x3c)
> [<c00103a0>] (arch_cpu_idle) from [<c007fea4>] (cpu_startup_entry+0x2d0/0x358)
> [<c007fea4>] (cpu_startup_entry) from [<c08eac24>] (start_kernel+0x364/0x3e8)
> ---[ end trace 68b3b5769f257e1c ]---
> 
> When I toggle on CTS once more I don't get such an error and for both
> toggle sessions I get the proper CTS value.
> 
> Any idea?

The warning at line drivers/tty/serial/serial_core.c:2893 is

	lockdep_assert_held_once(&uport->lock);

That means mctrl_gpio_irq_handle lacks some locking.

I think we need to take port->lock (with irqs off) before

	mctrl = gpios->mctrl_prev

and drop it before returning.

Note there is another issue with mctrl-gpio reported with Message-Id:
1456225442-27184-1-git-send-email-inguin@gmx.de.

Best regards
Uwe

Peter Hurley March 10, 2016, 4:36 p.m. UTC | #3

Hi Yegor,

On 03/09/2016 03:03 AM, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
> 
> This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
> signals.

Comments below.


> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
>  Documentation/devicetree/bindings/serial/8250.txt | 21 +++++++++++++++++++++
>  drivers/tty/serial/8250/8250.h                    |  2 ++
>  drivers/tty/serial/8250/8250_core.c               |  5 +++++
>  drivers/tty/serial/8250/8250_port.c               | 11 ++++++++++-
>  drivers/tty/serial/8250/Kconfig                   |  1 +
>  include/linux/serial_8250.h                       |  1 +
>  6 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> index 936ab5b..277e0ee 100644
> --- a/Documentation/devicetree/bindings/serial/8250.txt
> +++ b/Documentation/devicetree/bindings/serial/8250.txt
> @@ -42,6 +42,9 @@ Optional properties:
>  - auto-flow-control: one way to enable automatic flow control support. The
>    driver is allowed to detect support for the capability even without this
>    property.
> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
> +  line respectively. It will use specified GPIO instead of the peripheral
> +  function pin for the UART feature. If unsure, don't specify this property.
>  
>  Note:
>  * fsl,ns16550:
> @@ -63,3 +66,21 @@ Example:
>  		interrupts = <10>;
>  		reg-shift = <2>;
>  	};
> +
> +Example for OMAP UART using GPIO-based modem control signals:
> +
> +	uart4: serial@49042000 {
> +		compatible = "ti,omap3-uart";
> +		reg = <0x49042000 0x400>;
> +		interrupts = <80>;

> +		dmas = <&sdma 81 &sdma 82>;
> +		dma-names = "tx", "rx";

Drop the DMA example; omap3 sdma is broken wrt to uart.


> +		ti,hwmods = "uart4";
> +		clock-frequency = <48000000>;
> +		cts-gpios = <&gpio3 5 GPIO_ACTIVE_LOW>;
> +		rts-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
> +		dtr-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
> +		dsr-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> +		dcd-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
> +		rng-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
> +	};
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index d54dcd8..98ee73c 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -15,6 +15,8 @@
>  #include <linux/serial_reg.h>
>  #include <linux/dmaengine.h>
>  
> +#include "../serial_mctrl_gpio.h"
> +
>  struct uart_8250_dma {
>  	int (*tx_dma)(struct uart_8250_port *p);
>  	int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index c9720a9..cadcaa5 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1004,6 +1004,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>  		if (up->port.flags & UPF_FIXED_TYPE)
>  			uart->port.type = up->port.type;
>  
> +		uart->gpios = mctrl_gpio_init(&uart->port, 0);
> +		if (IS_ERR(uart->gpios))
> +			return PTR_ERR(uart->gpios);
> +
>  		serial8250_set_defaults(uart);
>  
>  		/* Possibly override default I/O functions.  */
> @@ -1049,6 +1053,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>  
>  			ret = 0;
>  		}
> +
>  	}
>  	mutex_unlock(&serial_mutex);
>  
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 8d262bc..e7bfeef 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1393,6 +1393,8 @@ static void serial8250_disable_ms(struct uart_port *port)
>  	if (up->bugs & UART_BUG_NOMSR)
>  		return;
>  
> +	mctrl_gpio_disable_ms(up->gpios);
> +
>  	up->ier &= ~UART_IER_MSI;
>  	serial_port_out(port, UART_IER, up->ier);
>  }
> @@ -1405,6 +1407,8 @@ static void serial8250_enable_ms(struct uart_port *port)
>  	if (up->bugs & UART_BUG_NOMSR)
>  		return;
>  
> +	mctrl_gpio_enable_ms(up->gpios);
> +
>  	up->ier |= UART_IER_MSI;
>  
>  	serial8250_rpm_get(up);
> @@ -1680,7 +1684,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>  		ret |= TIOCM_DSR;
>  	if (status & UART_MSR_CTS)
>  		ret |= TIOCM_CTS;
> -	return ret;
> +
> +	return mctrl_gpio_get(up->gpios, &ret);

This is ugly; why is mctrl pass-by-reference when it's returned anyway?

	return mctrl_gpio_get(up->gpio, ret);


>  }
>  
>  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
> @@ -1707,10 +1712,14 @@ EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
>  
>  static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +
>  	if (port->set_mctrl)
>  		port->set_mctrl(port, mctrl);
>  	else
>  		serial8250_do_set_mctrl(port, mctrl);
> +
> +	mctrl_gpio_set(up->gpios, mctrl);

The 8250 driver virtualizes RTS because some h/w does not
support RTS override with autoRTS on.

So this won't work properly.
And this won't work for RS485.

Rather, transform

	serial_out(port, UART_MCR, val);
	serial_port_out(up, UART_MCR, val);

to

	serial8250_out_MCR(up, val);

and add the gpio setting to that.
Do the transform first in a prequel patch; then add gpio to that.

Similarly for serial_in(port, UART_MCR)

Regards,
Peter Hurley


>  }
>  
>  static void serial8250_break_ctl(struct uart_port *port, int break_state)
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index b03cb517..997e9a8 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -6,6 +6,7 @@
>  config SERIAL_8250
>  	tristate "8250/16550 and compatible serial support"
>  	select SERIAL_CORE
> +	select SERIAL_MCTRL_GPIO if GPIOLIB
>  	---help---
>  	  This selects whether you want to include the driver for the standard
>  	  serial ports.  The standard answer is Y.  People who might say N
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index faa0e03..86a3ef4 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -104,6 +104,7 @@ struct uart_8250_port {
>  						 *   if no_console_suspend
>  						 */
>  	unsigned char		probe;
> +	struct mctrl_gpios	*gpios;
>  #define UART_PROBE_RSA	(1 << 0)
>  
>  	/*
> 

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

Peter Hurley March 10, 2016, 7:55 p.m. UTC | #4

On 03/10/2016 08:36 AM, Peter Hurley wrote:
> Hi Yegor,
> 
> On 03/09/2016 03:03 AM, yegorslists@googlemail.com wrote:
>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>
>> This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>> signals.
> 
> Comments below.
> 
> 
>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> ---
>>  Documentation/devicetree/bindings/serial/8250.txt | 21 +++++++++++++++++++++
>>  drivers/tty/serial/8250/8250.h                    |  2 ++
>>  drivers/tty/serial/8250/8250_core.c               |  5 +++++
>>  drivers/tty/serial/8250/8250_port.c               | 11 ++++++++++-
>>  drivers/tty/serial/8250/Kconfig                   |  1 +
>>  include/linux/serial_8250.h                       |  1 +
>>  6 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
>> index 936ab5b..277e0ee 100644
>> --- a/Documentation/devicetree/bindings/serial/8250.txt
>> +++ b/Documentation/devicetree/bindings/serial/8250.txt
>> @@ -42,6 +42,9 @@ Optional properties:
>>  - auto-flow-control: one way to enable automatic flow control support. The
>>    driver is allowed to detect support for the capability even without this
>>    property.
>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
>> +  line respectively. It will use specified GPIO instead of the peripheral
>> +  function pin for the UART feature. If unsure, don't specify this property.
>>  
>>  Note:
>>  * fsl,ns16550:
>> @@ -63,3 +66,21 @@ Example:
>>  		interrupts = <10>;
>>  		reg-shift = <2>;
>>  	};
>> +
>> +Example for OMAP UART using GPIO-based modem control signals:
>> +
>> +	uart4: serial@49042000 {
>> +		compatible = "ti,omap3-uart";
>> +		reg = <0x49042000 0x400>;
>> +		interrupts = <80>;
> 
>> +		dmas = <&sdma 81 &sdma 82>;
>> +		dma-names = "tx", "rx";
> 
> Drop the DMA example; omap3 sdma is broken wrt to uart.
> 
> 
>> +		ti,hwmods = "uart4";
>> +		clock-frequency = <48000000>;
>> +		cts-gpios = <&gpio3 5 GPIO_ACTIVE_LOW>;
>> +		rts-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
>> +		dtr-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
>> +		dsr-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
>> +		dcd-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
>> +		rng-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>> +	};
>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> index d54dcd8..98ee73c 100644
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -15,6 +15,8 @@
>>  #include <linux/serial_reg.h>
>>  #include <linux/dmaengine.h>
>>  
>> +#include "../serial_mctrl_gpio.h"
>> +
>>  struct uart_8250_dma {
>>  	int (*tx_dma)(struct uart_8250_port *p);
>>  	int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index c9720a9..cadcaa5 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -1004,6 +1004,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>>  		if (up->port.flags & UPF_FIXED_TYPE)
>>  			uart->port.type = up->port.type;
>>  
>> +		uart->gpios = mctrl_gpio_init(&uart->port, 0);
>> +		if (IS_ERR(uart->gpios))
>> +			return PTR_ERR(uart->gpios);

Also, autoRTS needs to be disabled if RTS is a gpio.
Similarly for autoCTS.


>> +
>>  		serial8250_set_defaults(uart);
>>  
>>  		/* Possibly override default I/O functions.  */
>> @@ -1049,6 +1053,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>>  
>>  			ret = 0;
>>  		}
>> +
>>  	}
>>  	mutex_unlock(&serial_mutex);
>>  
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 8d262bc..e7bfeef 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -1393,6 +1393,8 @@ static void serial8250_disable_ms(struct uart_port *port)
>>  	if (up->bugs & UART_BUG_NOMSR)
>>  		return;
>>  
>> +	mctrl_gpio_disable_ms(up->gpios);
>> +
>>  	up->ier &= ~UART_IER_MSI;
>>  	serial_port_out(port, UART_IER, up->ier);
>>  }
>> @@ -1405,6 +1407,8 @@ static void serial8250_enable_ms(struct uart_port *port)
>>  	if (up->bugs & UART_BUG_NOMSR)
>>  		return;
>>  
>> +	mctrl_gpio_enable_ms(up->gpios);
>> +
>>  	up->ier |= UART_IER_MSI;
>>  
>>  	serial8250_rpm_get(up);
>> @@ -1680,7 +1684,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>>  		ret |= TIOCM_DSR;
>>  	if (status & UART_MSR_CTS)
>>  		ret |= TIOCM_CTS;
>> -	return ret;
>> +
>> +	return mctrl_gpio_get(up->gpios, &ret);
> 
> This is ugly; why is mctrl pass-by-reference when it's returned anyway?
> 
> 	return mctrl_gpio_get(up->gpio, ret);
> 
> 
>>  }
>>  
>>  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> @@ -1707,10 +1712,14 @@ EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
>>  
>>  static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>  {
>> +	struct uart_8250_port *up = up_to_u8250p(port);
>> +
>>  	if (port->set_mctrl)
>>  		port->set_mctrl(port, mctrl);
>>  	else
>>  		serial8250_do_set_mctrl(port, mctrl);
>> +
>> +	mctrl_gpio_set(up->gpios, mctrl);
> 
> The 8250 driver virtualizes RTS because some h/w does not
> support RTS override with autoRTS on.
> 
> So this won't work properly.
> And this won't work for RS485.
> 
> Rather, transform
> 
> 	serial_out(port, UART_MCR, val);
> 	serial_port_out(up, UART_MCR, val);
> 
> to
> 
> 	serial8250_out_MCR(up, val);
> 
> and add the gpio setting to that.
> Do the transform first in a prequel patch; then add gpio to that.
> 
> Similarly for serial_in(port, UART_MCR)
> 
> Regards,
> Peter Hurley
> 
> 
>>  }
>>  
>>  static void serial8250_break_ctl(struct uart_port *port, int break_state)
>> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>> index b03cb517..997e9a8 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -6,6 +6,7 @@
>>  config SERIAL_8250
>>  	tristate "8250/16550 and compatible serial support"
>>  	select SERIAL_CORE
>> +	select SERIAL_MCTRL_GPIO if GPIOLIB
>>  	---help---
>>  	  This selects whether you want to include the driver for the standard
>>  	  serial ports.  The standard answer is Y.  People who might say N
>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> index faa0e03..86a3ef4 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -104,6 +104,7 @@ struct uart_8250_port {
>>  						 *   if no_console_suspend
>>  						 */
>>  	unsigned char		probe;
>> +	struct mctrl_gpios	*gpios;
>>  #define UART_PROBE_RSA	(1 << 0)
>>  
>>  	/*
>>
> 

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

Yegor Yefremov March 11, 2016, 9:24 a.m. UTC | #5

On Thu, Mar 10, 2016 at 8:43 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> On Wed, Mar 09, 2016 at 12:07:39PM +0100, Yegor Yefremov wrote:
>> On Wed, Mar 9, 2016 at 12:03 PM,  <yegorslists@googlemail.com> wrote:
>> > From: Yegor Yefremov <yegorslists@googlemail.com>
>> >
>> > This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>> > signals.
>> >
>> > Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>> > ---
>> >  Documentation/devicetree/bindings/serial/8250.txt | 21 +++++++++++++++++++++
>> >  drivers/tty/serial/8250/8250.h                    |  2 ++
>> >  drivers/tty/serial/8250/8250_core.c               |  5 +++++
>> >  drivers/tty/serial/8250/8250_port.c               | 11 ++++++++++-
>> >  drivers/tty/serial/8250/Kconfig                   |  1 +
>> >  include/linux/serial_8250.h                       |  1 +
>> >  6 files changed, 40 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
>> > index 936ab5b..277e0ee 100644
>> > --- a/Documentation/devicetree/bindings/serial/8250.txt
>> > +++ b/Documentation/devicetree/bindings/serial/8250.txt
>> > @@ -42,6 +42,9 @@ Optional properties:
>> >  - auto-flow-control: one way to enable automatic flow control support. The
>> >    driver is allowed to detect support for the capability even without this
>> >    property.
>> > +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
>> > +  line respectively. It will use specified GPIO instead of the peripheral
>> > +  function pin for the UART feature. If unsure, don't specify this property.
>> >
>> >  Note:
>> >  * fsl,ns16550:
>> > @@ -63,3 +66,21 @@ Example:
>> >                 interrupts = <10>;
>> >                 reg-shift = <2>;
>> >         };
>> > +
>> > +Example for OMAP UART using GPIO-based modem control signals:
>> > +
>> > +       uart4: serial@49042000 {
>> > +               compatible = "ti,omap3-uart";
>> > +               reg = <0x49042000 0x400>;
>> > +               interrupts = <80>;
>> > +               dmas = <&sdma 81 &sdma 82>;
>> > +               dma-names = "tx", "rx";
>> > +               ti,hwmods = "uart4";
>> > +               clock-frequency = <48000000>;
>> > +               cts-gpios = <&gpio3 5 GPIO_ACTIVE_LOW>;
>> > +               rts-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
>> > +               dtr-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
>> > +               dsr-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
>> > +               dcd-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
>> > +               rng-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>> > +       };
>> > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> > index d54dcd8..98ee73c 100644
>> > --- a/drivers/tty/serial/8250/8250.h
>> > +++ b/drivers/tty/serial/8250/8250.h
>> > @@ -15,6 +15,8 @@
>> >  #include <linux/serial_reg.h>
>> >  #include <linux/dmaengine.h>
>> >
>> > +#include "../serial_mctrl_gpio.h"
>> > +
>> >  struct uart_8250_dma {
>> >         int (*tx_dma)(struct uart_8250_port *p);
>> >         int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
>> > diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> > index c9720a9..cadcaa5 100644
>> > --- a/drivers/tty/serial/8250/8250_core.c
>> > +++ b/drivers/tty/serial/8250/8250_core.c
>> > @@ -1004,6 +1004,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>> >                 if (up->port.flags & UPF_FIXED_TYPE)
>> >                         uart->port.type = up->port.type;
>> >
>> > +               uart->gpios = mctrl_gpio_init(&uart->port, 0);
>> > +               if (IS_ERR(uart->gpios))
>> > +                       return PTR_ERR(uart->gpios);
>> > +
>> >                 serial8250_set_defaults(uart);
>> >
>> >                 /* Possibly override default I/O functions.  */
>> > @@ -1049,6 +1053,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>> >
>> >                         ret = 0;
>> >                 }
>> > +
>
> This hunk should be dropped.

OK

> Otherwise the patch looks good.
>
>> >         }
>> >         mutex_unlock(&serial_mutex);
>> >
>> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> > index 8d262bc..e7bfeef 100644
>> > --- a/drivers/tty/serial/8250/8250_port.c
>> > +++ b/drivers/tty/serial/8250/8250_port.c
>> > @@ -1393,6 +1393,8 @@ static void serial8250_disable_ms(struct uart_port *port)
>> >         if (up->bugs & UART_BUG_NOMSR)
>> >                 return;
>> >
>> > +       mctrl_gpio_disable_ms(up->gpios);
>> > +
>> >         up->ier &= ~UART_IER_MSI;
>> >         serial_port_out(port, UART_IER, up->ier);
>> >  }
>> > @@ -1405,6 +1407,8 @@ static void serial8250_enable_ms(struct uart_port *port)
>> >         if (up->bugs & UART_BUG_NOMSR)
>> >                 return;
>> >
>> > +       mctrl_gpio_enable_ms(up->gpios);
>> > +
>> >         up->ier |= UART_IER_MSI;
>> >
>> >         serial8250_rpm_get(up);
>> > @@ -1680,7 +1684,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>> >                 ret |= TIOCM_DSR;
>> >         if (status & UART_MSR_CTS)
>> >                 ret |= TIOCM_CTS;
>> > -       return ret;
>> > +
>> > +       return mctrl_gpio_get(up->gpios, &ret);
>> >  }
>> >
>> >  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> > @@ -1707,10 +1712,14 @@ EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
>> >
>> >  static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> >  {
>> > +       struct uart_8250_port *up = up_to_u8250p(port);
>> > +
>> >         if (port->set_mctrl)
>> >                 port->set_mctrl(port, mctrl);
>> >         else
>> >                 serial8250_do_set_mctrl(port, mctrl);
>> > +
>> > +       mctrl_gpio_set(up->gpios, mctrl);
>> >  }
>> >
>> >  static void serial8250_break_ctl(struct uart_port *port, int break_state)
>> > diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>> > index b03cb517..997e9a8 100644
>> > --- a/drivers/tty/serial/8250/Kconfig
>> > +++ b/drivers/tty/serial/8250/Kconfig
>> > @@ -6,6 +6,7 @@
>> >  config SERIAL_8250
>> >         tristate "8250/16550 and compatible serial support"
>> >         select SERIAL_CORE
>> > +       select SERIAL_MCTRL_GPIO if GPIOLIB
>> >         ---help---
>> >           This selects whether you want to include the driver for the standard
>> >           serial ports.  The standard answer is Y.  People who might say N
>> > diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> > index faa0e03..86a3ef4 100644
>> > --- a/include/linux/serial_8250.h
>> > +++ b/include/linux/serial_8250.h
>> > @@ -104,6 +104,7 @@ struct uart_8250_port {
>> >                                                  *   if no_console_suspend
>> >                                                  */
>> >         unsigned char           probe;
>> > +       struct mctrl_gpios      *gpios;
>> >  #define UART_PROBE_RSA (1 << 0)
>> >
>> >         /*
>>
>> This patch was tested on a am335x based machine. So far everything is
>> working as expected aside from interrupts. As soon as the first CTS
>> interrupt comes I get:
>>
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at drivers/tty/serial/serial_core.c:2893
>> uart_handle_cts_change+0x9c/0xd8()
>> Modules linked in: musb_dsps musb_am335x
>> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W       4.5.0-rc6 #37
>> Hardware name: Generic AM33XX (Flattened Device Tree)
>> [<c0017e34>] (unwind_backtrace) from [<c0014088>] (show_stack+0x10/0x14)
>> [<c0014088>] (show_stack) from [<c02c5b24>] (dump_stack+0xb0/0xe4)
>> [<c02c5b24>] (dump_stack) from [<c003bca4>] (warn_slowpath_common+0x7c/0xb8)
>> [<c003bca4>] (warn_slowpath_common) from [<c003bd7c>]
>> (warn_slowpath_null+0x1c/0x24)
>> [<c003bd7c>] (warn_slowpath_null) from [<c0337bc8>]
>> (uart_handle_cts_change+0x9c/0xd8)
>> [<c0337bc8>] (uart_handle_cts_change) from [<c033f430>]
>> (mctrl_gpio_irq_handle+0xb0/0xcc)
>> [<c033f430>] (mctrl_gpio_irq_handle) from [<c0096db4>]
>> (handle_irq_event_percpu+0x50/0x268)
>> [<c0096db4>] (handle_irq_event_percpu) from [<c0097004>]
>> (handle_irq_event+0x38/0x5c)
>> [<c0097004>] (handle_irq_event) from [<c009a3a0>] (handle_edge_irq+0xf0/0x1bc)
>> [<c009a3a0>] (handle_edge_irq) from [<c0096438>] (generic_handle_irq+0x20/0x34)
>> [<c0096438>] (generic_handle_irq) from [<c0302038>]
>> (omap_gpio_irq_handler+0x12c/0x16c)
>> [<c0302038>] (omap_gpio_irq_handler) from [<c0096db4>]
>> (handle_irq_event_percpu+0x50/0x268)
>> [<c0096db4>] (handle_irq_event_percpu) from [<c0097004>]
>> (handle_irq_event+0x38/0x5c)
>> [<c0097004>] (handle_irq_event) from [<c009a070>] (handle_level_irq+0xb8/0x14c)
>> [<c009a070>] (handle_level_irq) from [<c0096438>] (generic_handle_irq+0x20/0x34)
>> [<c0096438>] (generic_handle_irq) from [<c009672c>]
>> (__handle_domain_irq+0x64/0xe0)
>> [<c009672c>] (__handle_domain_irq) from [<c06802f8>] (__irq_svc+0x58/0x78)
>> [<c06802f8>] (__irq_svc) from [<c00103a0>] (arch_cpu_idle+0x20/0x3c)
>> [<c00103a0>] (arch_cpu_idle) from [<c007fea4>] (cpu_startup_entry+0x2d0/0x358)
>> [<c007fea4>] (cpu_startup_entry) from [<c08eac24>] (start_kernel+0x364/0x3e8)
>> ---[ end trace 68b3b5769f257e1c ]---
>>
>> When I toggle on CTS once more I don't get such an error and for both
>> toggle sessions I get the proper CTS value.
>>
>> Any idea?
>
> The warning at line drivers/tty/serial/serial_core.c:2893 is
>
>         lockdep_assert_held_once(&uport->lock);
>
> That means mctrl_gpio_irq_handle lacks some locking.
>
> I think we need to take port->lock (with irqs off) before
>
>         mctrl = gpios->mctrl_prev
>
> and drop it before returning.
>
> Note there is another issue with mctrl-gpio reported with Message-Id:
> 1456225442-27184-1-git-send-email-inguin@gmx.de.

Could you add me to CC, if you'll answer to this thread? Thanks.

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

Yegor Yefremov March 11, 2016, 4:06 p.m. UTC | #6

On Thu, Mar 10, 2016 at 8:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 03/10/2016 08:36 AM, Peter Hurley wrote:
>> Hi Yegor,
>>
>> On 03/09/2016 03:03 AM, yegorslists@googlemail.com wrote:
>>> From: Yegor Yefremov <yegorslists@googlemail.com>
>>>
>>> This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
>>> signals.
>>
>> Comments below.
>>
>>
>>> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
>>> ---
>>>  Documentation/devicetree/bindings/serial/8250.txt | 21 +++++++++++++++++++++
>>>  drivers/tty/serial/8250/8250.h                    |  2 ++
>>>  drivers/tty/serial/8250/8250_core.c               |  5 +++++
>>>  drivers/tty/serial/8250/8250_port.c               | 11 ++++++++++-
>>>  drivers/tty/serial/8250/Kconfig                   |  1 +
>>>  include/linux/serial_8250.h                       |  1 +
>>>  6 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
>>> index 936ab5b..277e0ee 100644
>>> --- a/Documentation/devicetree/bindings/serial/8250.txt
>>> +++ b/Documentation/devicetree/bindings/serial/8250.txt
>>> @@ -42,6 +42,9 @@ Optional properties:
>>>  - auto-flow-control: one way to enable automatic flow control support. The
>>>    driver is allowed to detect support for the capability even without this
>>>    property.
>>> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
>>> +  line respectively. It will use specified GPIO instead of the peripheral
>>> +  function pin for the UART feature. If unsure, don't specify this property.
>>>
>>>  Note:
>>>  * fsl,ns16550:
>>> @@ -63,3 +66,21 @@ Example:
>>>              interrupts = <10>;
>>>              reg-shift = <2>;
>>>      };
>>> +
>>> +Example for OMAP UART using GPIO-based modem control signals:
>>> +
>>> +    uart4: serial@49042000 {
>>> +            compatible = "ti,omap3-uart";
>>> +            reg = <0x49042000 0x400>;
>>> +            interrupts = <80>;
>>
>>> +            dmas = <&sdma 81 &sdma 82>;
>>> +            dma-names = "tx", "rx";
>>
>> Drop the DMA example; omap3 sdma is broken wrt to uart.
>>
>>
>>> +            ti,hwmods = "uart4";
>>> +            clock-frequency = <48000000>;
>>> +            cts-gpios = <&gpio3 5 GPIO_ACTIVE_LOW>;
>>> +            rts-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
>>> +            dtr-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
>>> +            dsr-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
>>> +            dcd-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
>>> +            rng-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
>>> +    };
>>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>>> index d54dcd8..98ee73c 100644
>>> --- a/drivers/tty/serial/8250/8250.h
>>> +++ b/drivers/tty/serial/8250/8250.h
>>> @@ -15,6 +15,8 @@
>>>  #include <linux/serial_reg.h>
>>>  #include <linux/dmaengine.h>
>>>
>>> +#include "../serial_mctrl_gpio.h"
>>> +
>>>  struct uart_8250_dma {
>>>      int (*tx_dma)(struct uart_8250_port *p);
>>>      int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
>>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>>> index c9720a9..cadcaa5 100644
>>> --- a/drivers/tty/serial/8250/8250_core.c
>>> +++ b/drivers/tty/serial/8250/8250_core.c
>>> @@ -1004,6 +1004,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>>>              if (up->port.flags & UPF_FIXED_TYPE)
>>>                      uart->port.type = up->port.type;
>>>
>>> +            uart->gpios = mctrl_gpio_init(&uart->port, 0);
>>> +            if (IS_ERR(uart->gpios))
>>> +                    return PTR_ERR(uart->gpios);
>
> Also, autoRTS needs to be disabled if RTS is a gpio.
> Similarly for autoCTS.
>
>
>>> +
>>>              serial8250_set_defaults(uart);
>>>
>>>              /* Possibly override default I/O functions.  */
>>> @@ -1049,6 +1053,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>>>
>>>                      ret = 0;
>>>              }
>>> +
>>>      }
>>>      mutex_unlock(&serial_mutex);
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>>> index 8d262bc..e7bfeef 100644
>>> --- a/drivers/tty/serial/8250/8250_port.c
>>> +++ b/drivers/tty/serial/8250/8250_port.c
>>> @@ -1393,6 +1393,8 @@ static void serial8250_disable_ms(struct uart_port *port)
>>>      if (up->bugs & UART_BUG_NOMSR)
>>>              return;
>>>
>>> +    mctrl_gpio_disable_ms(up->gpios);
>>> +
>>>      up->ier &= ~UART_IER_MSI;
>>>      serial_port_out(port, UART_IER, up->ier);
>>>  }
>>> @@ -1405,6 +1407,8 @@ static void serial8250_enable_ms(struct uart_port *port)
>>>      if (up->bugs & UART_BUG_NOMSR)
>>>              return;
>>>
>>> +    mctrl_gpio_enable_ms(up->gpios);
>>> +
>>>      up->ier |= UART_IER_MSI;
>>>
>>>      serial8250_rpm_get(up);
>>> @@ -1680,7 +1684,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>>>              ret |= TIOCM_DSR;
>>>      if (status & UART_MSR_CTS)
>>>              ret |= TIOCM_CTS;
>>> -    return ret;
>>> +
>>> +    return mctrl_gpio_get(up->gpios, &ret);
>>
>> This is ugly; why is mctrl pass-by-reference when it's returned anyway?
>>
>>       return mctrl_gpio_get(up->gpio, ret);
>>
>>
>>>  }
>>>
>>>  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>> @@ -1707,10 +1712,14 @@ EXPORT_SYMBOL_GPL(serial8250_do_set_mctrl);
>>>
>>>  static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>>  {
>>> +    struct uart_8250_port *up = up_to_u8250p(port);
>>> +
>>>      if (port->set_mctrl)
>>>              port->set_mctrl(port, mctrl);
>>>      else
>>>              serial8250_do_set_mctrl(port, mctrl);
>>> +
>>> +    mctrl_gpio_set(up->gpios, mctrl);
>>
>> The 8250 driver virtualizes RTS because some h/w does not
>> support RTS override with autoRTS on.
>>
>> So this won't work properly.
>> And this won't work for RS485.
>>
>> Rather, transform
>>
>>       serial_out(port, UART_MCR, val);
>>       serial_port_out(up, UART_MCR, val);
>>
>> to
>>
>>       serial8250_out_MCR(up, val);
>>
>> and add the gpio setting to that.
>> Do the transform first in a prequel patch; then add gpio to that.
>>
>> Similarly for serial_in(port, UART_MCR)

To implement serial8250_in_MCR() we will need something like
mctrl_gpio_get_mcr(), because mctrl_gpio_get() returns only GPIO
values in input mode. Peter, Uwe what do you think about it?

Yegor

>>>  }
>>>
>>>  static void serial8250_break_ctl(struct uart_port *port, int break_state)
>>> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>>> index b03cb517..997e9a8 100644
>>> --- a/drivers/tty/serial/8250/Kconfig
>>> +++ b/drivers/tty/serial/8250/Kconfig
>>> @@ -6,6 +6,7 @@
>>>  config SERIAL_8250
>>>      tristate "8250/16550 and compatible serial support"
>>>      select SERIAL_CORE
>>> +    select SERIAL_MCTRL_GPIO if GPIOLIB
>>>      ---help---
>>>        This selects whether you want to include the driver for the standard
>>>        serial ports.  The standard answer is Y.  People who might say N
>>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>>> index faa0e03..86a3ef4 100644
>>> --- a/include/linux/serial_8250.h
>>> +++ b/include/linux/serial_8250.h
>>> @@ -104,6 +104,7 @@ struct uart_8250_port {
>>>                                               *   if no_console_suspend
>>>                                               */
>>>      unsigned char           probe;
>>> +    struct mctrl_gpios      *gpios;
>>>  #define UART_PROBE_RSA      (1 << 0)
>>>
>>>      /*
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König March 11, 2016, 4:19 p.m. UTC | #7

Hello Yegor,

On Fri, Mar 11, 2016 at 05:06:30PM +0100, Yegor Yefremov wrote:
> On Thu, Mar 10, 2016 at 8:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> > On 03/10/2016 08:36 AM, Peter Hurley wrote:
> >> Rather, transform
> >>
> >>       serial_out(port, UART_MCR, val);
> >>       serial_port_out(up, UART_MCR, val);
> >>
> >> to
> >>
> >>       serial8250_out_MCR(up, val);
> >>
> >> and add the gpio setting to that.
> >> Do the transform first in a prequel patch; then add gpio to that.
> >>
> >> Similarly for serial_in(port, UART_MCR)
> 
> To implement serial8250_in_MCR() we will need something like
> mctrl_gpio_get_mcr(), because mctrl_gpio_get() returns only GPIO
> values in input mode. Peter, Uwe what do you think about it?

I don't know 8250 good enough to understand the problem here. Is MCR the
register to set the (output) handshake lines? Why do you need this?

Best regards
Uwe

Peter Hurley March 11, 2016, 4:26 p.m. UTC | #8

On 03/11/2016 08:19 AM, Uwe Kleine-König wrote:
> Hello Yegor,
> 
> On Fri, Mar 11, 2016 at 05:06:30PM +0100, Yegor Yefremov wrote:
>> On Thu, Mar 10, 2016 at 8:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>> On 03/10/2016 08:36 AM, Peter Hurley wrote:
>>>> Rather, transform
>>>>
>>>>       serial_out(port, UART_MCR, val);
>>>>       serial_port_out(up, UART_MCR, val);
>>>>
>>>> to
>>>>
>>>>       serial8250_out_MCR(up, val);
>>>>
>>>> and add the gpio setting to that.
>>>> Do the transform first in a prequel patch; then add gpio to that.
>>>>
>>>> Similarly for serial_in(port, UART_MCR)
>>
>> To implement serial8250_in_MCR() we will need something like
>> mctrl_gpio_get_mcr(), because mctrl_gpio_get() returns only GPIO
>> values in input mode. Peter, Uwe what do you think about it?

mctrl_gpio_read() ?

> I don't know 8250 good enough to understand the problem here. Is MCR the
> register to set the (output) handshake lines? Why do you need this?

Save and restore, probably.

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

Yegor Yefremov March 11, 2016, 9:30 p.m. UTC | #9

On Fri, Mar 11, 2016 at 5:26 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> On 03/11/2016 08:19 AM, Uwe Kleine-König wrote:
>> Hello Yegor,
>>
>> On Fri, Mar 11, 2016 at 05:06:30PM +0100, Yegor Yefremov wrote:
>>> On Thu, Mar 10, 2016 at 8:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>>>> On 03/10/2016 08:36 AM, Peter Hurley wrote:
>>>>> Rather, transform
>>>>>
>>>>>       serial_out(port, UART_MCR, val);
>>>>>       serial_port_out(up, UART_MCR, val);
>>>>>
>>>>> to
>>>>>
>>>>>       serial8250_out_MCR(up, val);
>>>>>
>>>>> and add the gpio setting to that.
>>>>> Do the transform first in a prequel patch; then add gpio to that.
>>>>>
>>>>> Similarly for serial_in(port, UART_MCR)
>>>
>>> To implement serial8250_in_MCR() we will need something like
>>> mctrl_gpio_get_mcr(), because mctrl_gpio_get() returns only GPIO
>>> values in input mode. Peter, Uwe what do you think about it?
>
> mctrl_gpio_read() ?

What about renaming the functions to reflect their actual meaning?

mctrl_gpio_get_mcr()
mctrl_gpio_set_mcr()
mctrl_gpio_get_msr()

>> I don't know 8250 good enough to understand the problem here. Is MCR the
>> register to set the (output) handshake lines? Why do you need this?
>
> Save and restore, probably.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König March 12, 2016, 11:22 a.m. UTC | #10

Hello Yegor,

On Fri, Mar 11, 2016 at 10:30:14PM +0100, Yegor Yefremov wrote:
> On Fri, Mar 11, 2016 at 5:26 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> > On 03/11/2016 08:19 AM, Uwe Kleine-König wrote:
> >> On Fri, Mar 11, 2016 at 05:06:30PM +0100, Yegor Yefremov wrote:
> >>> On Thu, Mar 10, 2016 at 8:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
> >>>> On 03/10/2016 08:36 AM, Peter Hurley wrote:
> >>>>> Rather, transform
> >>>>>
> >>>>>       serial_out(port, UART_MCR, val);
> >>>>>       serial_port_out(up, UART_MCR, val);
> >>>>>
> >>>>> to
> >>>>>
> >>>>>       serial8250_out_MCR(up, val);
> >>>>>
> >>>>> and add the gpio setting to that.
> >>>>> Do the transform first in a prequel patch; then add gpio to that.
> >>>>>
> >>>>> Similarly for serial_in(port, UART_MCR)
> >>>
> >>> To implement serial8250_in_MCR() we will need something like
> >>> mctrl_gpio_get_mcr(), because mctrl_gpio_get() returns only GPIO
> >>> values in input mode. Peter, Uwe what do you think about it?
> >
> > mctrl_gpio_read() ?
> 
> What about renaming the functions to reflect their actual meaning?
> 
> mctrl_gpio_get_mcr()
> mctrl_gpio_set_mcr()
> mctrl_gpio_get_msr()

I think this is only understandable, if you have 8250 in mind where (I
guess) the registers to set and read the handshaking lines are called
mcr and msr.

So I don't think this is a good idea.

Best regards
Uwe

Yegor Yefremov March 17, 2016, 3:54 p.m. UTC | #11

On Sat, Mar 12, 2016 at 12:22 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Yegor,
>
> On Fri, Mar 11, 2016 at 10:30:14PM +0100, Yegor Yefremov wrote:
>> On Fri, Mar 11, 2016 at 5:26 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> > On 03/11/2016 08:19 AM, Uwe Kleine-König wrote:
>> >> On Fri, Mar 11, 2016 at 05:06:30PM +0100, Yegor Yefremov wrote:
>> >>> On Thu, Mar 10, 2016 at 8:55 PM, Peter Hurley <peter@hurleysoftware.com> wrote:
>> >>>> On 03/10/2016 08:36 AM, Peter Hurley wrote:
>> >>>>> Rather, transform
>> >>>>>
>> >>>>>       serial_out(port, UART_MCR, val);
>> >>>>>       serial_port_out(up, UART_MCR, val);
>> >>>>>
>> >>>>> to
>> >>>>>
>> >>>>>       serial8250_out_MCR(up, val);
>> >>>>>
>> >>>>> and add the gpio setting to that.
>> >>>>> Do the transform first in a prequel patch; then add gpio to that.
>> >>>>>
>> >>>>> Similarly for serial_in(port, UART_MCR)
>> >>>
>> >>> To implement serial8250_in_MCR() we will need something like
>> >>> mctrl_gpio_get_mcr(), because mctrl_gpio_get() returns only GPIO
>> >>> values in input mode. Peter, Uwe what do you think about it?
>> >
>> > mctrl_gpio_read() ?
>>
>> What about renaming the functions to reflect their actual meaning?
>>
>> mctrl_gpio_get_mcr()
>> mctrl_gpio_set_mcr()
>> mctrl_gpio_get_msr()
>
> I think this is only understandable, if you have 8250 in mind where (I
> guess) the registers to set and read the handshaking lines are called
> mcr and msr.
>
> So I don't think this is a good idea.

What is the best way to check, if for example RTS is implemented via
GPIO? Following code fails to compile:

drivers/tty/serial/8250/8250_omap.c: In function ‘omap8250_set_mctrl’:
drivers/tty/serial/8250/8250_omap.c:131:15: error: dereferencing
pointer to incomplete type
  if (up->gpios->gpio[TIOCM_RTS]) {

Should I move

struct mctrl_gpios {
        struct uart_port *port;
        struct gpio_desc *gpio[UART_GPIO_MAX];
        int irq[UART_GPIO_MAX];
        unsigned int mctrl_prev;
        bool mctrl_on;
};

to serial_mctrl_gpio.h or should I create some routine, that tells me,
if given signal is GPIO or not?

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

Uwe Kleine-König March 17, 2016, 5:02 p.m. UTC | #12

Hello Yegor,

On Thu, Mar 17, 2016 at 04:54:44PM +0100, Yegor Yefremov wrote:
> Should I move
> 
> struct mctrl_gpios {
>         struct uart_port *port;
>         struct gpio_desc *gpio[UART_GPIO_MAX];
>         int irq[UART_GPIO_MAX];
>         unsigned int mctrl_prev;
>         bool mctrl_on;
> };
> 
> to serial_mctrl_gpio.h or should I create some routine, that tells me,
> if given signal is GPIO or not?

That already exists. Check the at91 driver.

Best regards
Uwe

Yegor Yefremov March 21, 2016, 11:25 a.m. UTC | #13

On Thu, Mar 17, 2016 at 6:02 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Yegor,
>
> On Thu, Mar 17, 2016 at 04:54:44PM +0100, Yegor Yefremov wrote:
>> Should I move
>>
>> struct mctrl_gpios {
>>         struct uart_port *port;
>>         struct gpio_desc *gpio[UART_GPIO_MAX];
>>         int irq[UART_GPIO_MAX];
>>         unsigned int mctrl_prev;
>>         bool mctrl_on;
>> };
>>
>> to serial_mctrl_gpio.h or should I create some routine, that tells me,
>> if given signal is GPIO or not?
>
> That already exists. Check the at91 driver.

See it. Thanks.

Another question: I've got following crash from kernels patch testing
daemon. Any idea?

[    8.304454] page_owner is disabled
[    8.307923] rtc_cmos 00:00: setting system clock to 2016-03-09
19:56:39 UTC (1457553399)
[    8.311883] ------------[ cut here ]------------
[    8.312286] WARNING: CPU: 0 PID: 1 at drivers/tty/tty_io.c:1548
tty_init_dev+0x154/0x290()
[    8.313091] tty_init_dev: ttyS driver does not set tty->port. This
will crash the kernel later. Fix the driver!
[    8.313937] Modules linked in:
[    8.314223] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.5.0-rc4-00223-gd294b23 #904
[    8.314842]  00000246 00000246 40277d1c 4137c480 40277d44 41b4345c
0000060c 40277d34
[    8.315576]  4106ff33 4146dcd4 4ad00c10 00000001 00000000 40277d4c
4106ff86 00000009
[    8.316312]  40277d44 41b436a8 40277d60 40277d7c 4146dcd4 41b4345c
0000060c 41b436a8
[    8.317048] Call Trace:
[    8.317259]  [<4137c480>] dump_stack+0x8c/0xbc
[    8.317625]  [<4106ff33>] warn_slowpath_common+0xa3/0xd0
[    8.318064]  [<4146dcd4>] ? tty_init_dev+0x154/0x290
[    8.318468]  [<4106ff86>] warn_slowpath_fmt+0x26/0x30
[    8.318878]  [<4146dcd4>] tty_init_dev+0x154/0x290
[    8.319274]  [<4146e5c6>] tty_open+0x7b6/0xa90
[    8.319640]  [<41221ed5>] chrdev_open+0x205/0x2d0
[    8.320028]  [<412177e4>] do_dentry_open+0x544/0x5e0
[    8.320488]  [<41221cd0>] ? cdev_put+0x40/0x40
[    8.320852]  [<4121949a>] vfs_open+0x8a/0x90
[    8.321208]  [<4123246d>] do_last+0xb7d/0x1130
[    8.321571]  [<4122a917>] ? inode_permission+0x67/0xc0
[    8.321992]  [<41232b11>] path_openat+0xf1/0x4c0
[    8.322375]  [<41235229>] do_filp_open+0x69/0x120
[    8.322762]  [<41791b42>] ? _raw_spin_unlock+0x52/0x70
[    8.323187]  [<41249d68>] ? __alloc_fd+0x218/0x2b0
[    8.323580]  [<41219c7d>] do_sys_open+0x35d/0x440
[    8.323963]  [<41219d78>] SyS_open+0x18/0x20
[    8.324320]  [<41e09f1f>] kernel_init_freeable+0x106/0x1aa
[    8.324769]  [<41787469>] kernel_init+0x9/0x150
[    8.325146]  [<41792451>] ret_from_kernel_thread+0x21/0x40
[    8.325589]  [<41787460>] ? rest_init+0xb0/0xb0
[    8.326675] ---[ end trace a0fa66e39705bc4d ]---
[    8.327076] BUG: unable to handle kernel NULL pointer dereference at 00000098

git bisect start d294b233a2bdc7abdb5dfa856fc00dc67e96d57a
a95fc9c8e576dc238ad849f65c67e4fd33c01d3b --
# first bad commit: [d294b233a2bdc7abdb5dfa856fc00dc67e96d57a]
tty/serial/8250: use mctrl_gpio helpers
git bisect good a95fc9c8e576dc238ad849f65c67e4fd33c01d3b  # 20:02
66+      3  serial: 8250: describe CONFIG_SERIAL_8250_RSA
# extra tests with DEBUG_INFO
git bisect  bad d294b233a2bdc7abdb5dfa856fc00dc67e96d57a  # 20:06
0-     66  tty/serial/8250: use mctrl_gpio helpers
# extra tests on HEAD of
linux-review/yegorslists-googlemail-com/tty-serial-8250-use-mctrl_gpio-helpers/20160309-190741
git bisect  bad d294b233a2bdc7abdb5dfa856fc00dc67e96d57a  # 20:06
0-     13  tty/serial/8250: use mctrl_gpio helpers
# extra tests on tree/branch
linux-review/yegorslists-googlemail-com/tty-serial-8250-use-mctrl_gpio-helpers/20160309-190741
git bisect  bad d294b233a2bdc7abdb5dfa856fc00dc67e96d57a  # 20:06
0-     13  tty/serial/8250: use mctrl_gpio helpers
# extra tests with first bad commit reverted
git bisect good d232311183c0c28045b797481f732154ee93411b  # 20:21
66+      2  Revert "tty/serial/8250: use mctrl_gpio helpers"
# extra tests on tree/branch linus/master
git bisect good 7f02bf6b5f5de90b7a331759b5364e41c0f39bf9  # 20:25
61+      1  Merge tag 'sound-4.5' of
git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
# extra tests on tree/branch linux-next/master
git bisect good 7811b4ffc31285e08ee7a6a96a94848c37a43e09  # 20:30
61+      2  Add linux-next specific files for 20160309

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

Yegor Yefremov March 22, 2016, 10:53 a.m. UTC | #14

On Mon, Mar 21, 2016 at 12:25 PM, Yegor Yefremov
<yegorslists@googlemail.com> wrote:
> On Thu, Mar 17, 2016 at 6:02 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> Hello Yegor,
>>
>> On Thu, Mar 17, 2016 at 04:54:44PM +0100, Yegor Yefremov wrote:
>>> Should I move
>>>
>>> struct mctrl_gpios {
>>>         struct uart_port *port;
>>>         struct gpio_desc *gpio[UART_GPIO_MAX];
>>>         int irq[UART_GPIO_MAX];
>>>         unsigned int mctrl_prev;
>>>         bool mctrl_on;
>>> };
>>>
>>> to serial_mctrl_gpio.h or should I create some routine, that tells me,
>>> if given signal is GPIO or not?
>>
>> That already exists. Check the at91 driver.
>
> See it. Thanks.
>
> Another question: I've got following crash from kernels patch testing
> daemon. Any idea?
>
> [    8.304454] page_owner is disabled
> [    8.307923] rtc_cmos 00:00: setting system clock to 2016-03-09
> 19:56:39 UTC (1457553399)
> [    8.311883] ------------[ cut here ]------------
> [    8.312286] WARNING: CPU: 0 PID: 1 at drivers/tty/tty_io.c:1548
> tty_init_dev+0x154/0x290()
> [    8.313091] tty_init_dev: ttyS driver does not set tty->port. This
> will crash the kernel later. Fix the driver!
> [    8.313937] Modules linked in:
> [    8.314223] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 4.5.0-rc4-00223-gd294b23 #904
> [    8.314842]  00000246 00000246 40277d1c 4137c480 40277d44 41b4345c
> 0000060c 40277d34
> [    8.315576]  4106ff33 4146dcd4 4ad00c10 00000001 00000000 40277d4c
> 4106ff86 00000009
> [    8.316312]  40277d44 41b436a8 40277d60 40277d7c 4146dcd4 41b4345c
> 0000060c 41b436a8
> [    8.317048] Call Trace:
> [    8.317259]  [<4137c480>] dump_stack+0x8c/0xbc
> [    8.317625]  [<4106ff33>] warn_slowpath_common+0xa3/0xd0
> [    8.318064]  [<4146dcd4>] ? tty_init_dev+0x154/0x290
> [    8.318468]  [<4106ff86>] warn_slowpath_fmt+0x26/0x30
> [    8.318878]  [<4146dcd4>] tty_init_dev+0x154/0x290
> [    8.319274]  [<4146e5c6>] tty_open+0x7b6/0xa90
> [    8.319640]  [<41221ed5>] chrdev_open+0x205/0x2d0
> [    8.320028]  [<412177e4>] do_dentry_open+0x544/0x5e0
> [    8.320488]  [<41221cd0>] ? cdev_put+0x40/0x40
> [    8.320852]  [<4121949a>] vfs_open+0x8a/0x90
> [    8.321208]  [<4123246d>] do_last+0xb7d/0x1130
> [    8.321571]  [<4122a917>] ? inode_permission+0x67/0xc0
> [    8.321992]  [<41232b11>] path_openat+0xf1/0x4c0
> [    8.322375]  [<41235229>] do_filp_open+0x69/0x120
> [    8.322762]  [<41791b42>] ? _raw_spin_unlock+0x52/0x70
> [    8.323187]  [<41249d68>] ? __alloc_fd+0x218/0x2b0
> [    8.323580]  [<41219c7d>] do_sys_open+0x35d/0x440
> [    8.323963]  [<41219d78>] SyS_open+0x18/0x20
> [    8.324320]  [<41e09f1f>] kernel_init_freeable+0x106/0x1aa
> [    8.324769]  [<41787469>] kernel_init+0x9/0x150
> [    8.325146]  [<41792451>] ret_from_kernel_thread+0x21/0x40
> [    8.325589]  [<41787460>] ? rest_init+0xb0/0xb0
> [    8.326675] ---[ end trace a0fa66e39705bc4d ]---
> [    8.327076] BUG: unable to handle kernel NULL pointer dereference at 00000098
>
> git bisect start d294b233a2bdc7abdb5dfa856fc00dc67e96d57a
> a95fc9c8e576dc238ad849f65c67e4fd33c01d3b --
> # first bad commit: [d294b233a2bdc7abdb5dfa856fc00dc67e96d57a]
> tty/serial/8250: use mctrl_gpio helpers
> git bisect good a95fc9c8e576dc238ad849f65c67e4fd33c01d3b  # 20:02
> 66+      3  serial: 8250: describe CONFIG_SERIAL_8250_RSA
> # extra tests with DEBUG_INFO
> git bisect  bad d294b233a2bdc7abdb5dfa856fc00dc67e96d57a  # 20:06
> 0-     66  tty/serial/8250: use mctrl_gpio helpers
> # extra tests on HEAD of
> linux-review/yegorslists-googlemail-com/tty-serial-8250-use-mctrl_gpio-helpers/20160309-190741
> git bisect  bad d294b233a2bdc7abdb5dfa856fc00dc67e96d57a  # 20:06
> 0-     13  tty/serial/8250: use mctrl_gpio helpers
> # extra tests on tree/branch
> linux-review/yegorslists-googlemail-com/tty-serial-8250-use-mctrl_gpio-helpers/20160309-190741
> git bisect  bad d294b233a2bdc7abdb5dfa856fc00dc67e96d57a  # 20:06
> 0-     13  tty/serial/8250: use mctrl_gpio helpers
> # extra tests with first bad commit reverted
> git bisect good d232311183c0c28045b797481f732154ee93411b  # 20:21
> 66+      2  Revert "tty/serial/8250: use mctrl_gpio helpers"
> # extra tests on tree/branch linus/master
> git bisect good 7f02bf6b5f5de90b7a331759b5364e41c0f39bf9  # 20:25
> 61+      1  Merge tag 'sound-4.5' of
> git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
> # extra tests on tree/branch linux-next/master
> git bisect good 7811b4ffc31285e08ee7a6a96a94848c37a43e09  # 20:30
> 61+      2  Add linux-next specific files for 20160309

I could more or less reproduce the issue with qemu. If I don't return
from serial8250_register_8250_port() with an error, but something like
this, the system boots thorough:

uart->gpios = mctrl_gpio_init(&uart->port, 0);
                if (IS_ERR(uart->gpios))
                        dev_info(uart->port.dev,"Failed to init gpios\n");

What would be the appropriate solution? To use dev_dbg() or to put
#ifdef SERIAL_MCTRL_GPIO?

Basically we can have an x86 system using both platform serial devices
as also PCI based in one device, so #ifdef SERIAL_MCTRL_GPIO is not
appropriate.

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

[RFC] tty/serial/8250: use mctrl_gpio helpers

Commit Message

Comments

Patch