Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1565866 > unrolled thread

[PATCH 0/8] serial: st-asc: Allow handling of RTS line

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] serial: st-asc: Allow handling of RTS line Lee Jones <lee.jones@linaro.org> - 2017-01-24 14:50 +0100
    [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control Lee Jones <lee.jones@linaro.org> - 2017-01-24 14:50 +0100
      Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Peter Griffin <peter.griffin@linaro.org> - 2017-01-25 12:10 +0100
        Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Peter Griffin <peter.griffin@linaro.org> - 2017-01-25 12:50 +0100
          Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Lee Jones <lee.jones@linaro.org> - 2017-01-27 12:40 +0100
        Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW  flow-control Lee Jones <lee.jones@linaro.org> - 2017-01-27 12:40 +0100
    [PATCH 3/8] serial: st-asc: Read in all Pinctrl states Lee Jones <lee.jones@linaro.org> - 2017-01-24 14:50 +0100
      Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Peter Griffin <peter.griffin@linaro.org> - 2017-01-25 12:30 +0100
        Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Lee Jones <lee.jones@linaro.org> - 2017-01-27 13:00 +0100
          Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Peter Griffin <peter.griffin@linaro.org> - 2017-01-30 15:40 +0100
            Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Lee Jones <lee.jones@linaro.org> - 2017-01-30 16:40 +0100
              Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl  states Peter Griffin <peter.griffin@linaro.org> - 2017-01-30 17:20 +0100
    Re: [PATCH 0/8] serial: st-asc: Allow handling of RTS line Greg KH <gregkh@linuxfoundation.org> - 2017-01-25 11:10 +0100
      Re: [PATCH 0/8] serial: st-asc: Allow handling of RTS line Lee Jones <lee.jones@linaro.org> - 2017-01-25 16:40 +0100

#1565866 — [PATCH 0/8] serial: st-asc: Allow handling of RTS line

When hardware flow-control is disabled, manual toggling of the UART's
reset line (RTS) using userland applications (e.g. stty) is not
possible, since the ASC IP does not provide this functionality in the
same was as some other IPs do.  Thus, we have to do this manually.

This set ensures the correct Pinctrl groups are configured and
obtained for both manual toggling of the RTS line and for the IP to
take over the lines when HW flow-control is requested by the user.

Lee Jones (8):
  serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled
  serial: st-asc: Provide RTS functionality
  serial: st-asc: Read in all Pinctrl states
  serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
  ARM: dts: STiH410-b2260: Identify the UART RTS line
  ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control
  ARM: dts: STiH407-family: Use new Pinctrl groups
  ARM: dts: STiH407-family: Enable HW flow-control

 arch/arm/boot/dts/stih407-family.dtsi  |  7 +--
 arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++-
 arch/arm/boot/dts/stih410-b2260.dts    |  1 +
 drivers/tty/serial/st-asc.c            | 98 +++++++++++++++++++++++++++++++---
 4 files changed, 105 insertions(+), 13 deletions(-)

-- 
2.10.2

[toc] | [next] | [standalone]

#1565867 — [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

Hardware flow-control capability must be specified at a platform
level in order to inform the ASC driver that the platform is capable
(i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
capable, so let's provide the property.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 9789978..7ada8ea 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -226,7 +226,7 @@
 			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
 			pinctrl-1 = <&pinctrl_serial0>;
 			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
-
+			st,hw-flow-control;
 			status = "disabled";
 		};
 
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

#1566496 — Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> Hardware flow-control capability must be specified at a platform
> level in order to inform the ASC driver that the platform is capable
> (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> capable, so let's provide the property.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index 9789978..7ada8ea 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -226,7 +226,7 @@
>  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
>  			pinctrl-1 = <&pinctrl_serial0>;
>  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> -
> +			st,hw-flow-control;

There is a generic serial binding for this already. As this ST property
hasn't been used upstream, it seems like it would be worth dropping it
and switching to the generic uart-has-rtscts one.

See Documentation/devicetree/bindings/serial/serial.txt

  - uart-has-rtscts: The presence of this property indicates that the
    UART has dedicated lines for RTS/CTS hardware flow control, and that
    they are available for use (wired and enabled by pinmux configuration).
    This depends on both the UART hardware and the board wiring.
    Note that this property is mutually-exclusive with "cts-gpios" and
    "rts-gpios" above.

Also you should put this in the board dtsi, as it is board dependent property.
By putting it here you are enabling hw-flow-control for all stih407-family
based boards.

regards,

Peter.

[toc] | [prev] | [next] | [standalone]

#1566525 — Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

On Wed, 25 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Tue, 24 Jan 2017, Lee Jones wrote:
> 
> > Hardware flow-control capability must be specified at a platform
> > level in order to inform the ASC driver that the platform is capable
> > (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> > capable, so let's provide the property.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > index 9789978..7ada8ea 100644
> > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > @@ -226,7 +226,7 @@
> >  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> >  			pinctrl-1 = <&pinctrl_serial0>;
> >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> > -
> > +			st,hw-flow-control;
> 
> There is a generic serial binding for this already. As this ST property
> hasn't been used upstream, it seems like it would be worth dropping it
> and switching to the generic uart-has-rtscts one.
> 
> See Documentation/devicetree/bindings/serial/serial.txt
> 
>   - uart-has-rtscts: The presence of this property indicates that the
>     UART has dedicated lines for RTS/CTS hardware flow control, and that
>     they are available for use (wired and enabled by pinmux configuration).
>     This depends on both the UART hardware and the board wiring.
>     Note that this property is mutually-exclusive with "cts-gpios" and
>     "rts-gpios" above.

Thinking some more this generic binding definition is a bit restrictive,
particularly the "Note that this property is mutually-exclusive with "cts-gpios"
and "rts-gpios"" part.

As with this series we will have dynamic on-the-fly changing from hw flow
control to gpio control for other configs which the HW IP doesn't support.

IMO the uart-has-rtscts definition should be updated to reflect that they can
be used together in certain circumstances.

regards,

Peter.

[toc] | [prev] | [next] | [standalone]

#1568220 — Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

On Wed, 25 Jan 2017, Peter Griffin wrote:
> On Wed, 25 Jan 2017, Peter Griffin wrote:
> > On Tue, 24 Jan 2017, Lee Jones wrote:
> > 
> > > Hardware flow-control capability must be specified at a platform
> > > level in order to inform the ASC driver that the platform is capable
> > > (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> > > capable, so let's provide the property.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > > index 9789978..7ada8ea 100644
> > > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > > @@ -226,7 +226,7 @@
> > >  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> > >  			pinctrl-1 = <&pinctrl_serial0>;
> > >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> > > -
> > > +			st,hw-flow-control;
> > 
> > There is a generic serial binding for this already. As this ST property
> > hasn't been used upstream, it seems like it would be worth dropping it
> > and switching to the generic uart-has-rtscts one.
> > 
> > See Documentation/devicetree/bindings/serial/serial.txt
> > 
> >   - uart-has-rtscts: The presence of this property indicates that the
> >     UART has dedicated lines for RTS/CTS hardware flow control, and that
> >     they are available for use (wired and enabled by pinmux configuration).
> >     This depends on both the UART hardware and the board wiring.
> >     Note that this property is mutually-exclusive with "cts-gpios" and
> >     "rts-gpios" above.
> 
> Thinking some more this generic binding definition is a bit restrictive,
> particularly the "Note that this property is mutually-exclusive with "cts-gpios"
> and "rts-gpios"" part.
> 
> As with this series we will have dynamic on-the-fly changing from hw flow
> control to gpio control for other configs which the HW IP doesn't support.
> 
> IMO the uart-has-rtscts definition should be updated to reflect that they can
> be used together in certain circumstances.

Ah, you noticed that too.  Yes, will update the doc.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1568228 — Re: [STLinux Kernel] [PATCH 8/8] ARM: dts: STiH407-family: Enable HW flow-control

On Wed, 25 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Tue, 24 Jan 2017, Lee Jones wrote:
> 
> > Hardware flow-control capability must be specified at a platform
> > level in order to inform the ASC driver that the platform is capable
> > (i.e. are the lines wired up, etc).  STiH4{07,10} devices are indeed
> > capable, so let's provide the property.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/stih407-family.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> > index 9789978..7ada8ea 100644
> > --- a/arch/arm/boot/dts/stih407-family.dtsi
> > +++ b/arch/arm/boot/dts/stih407-family.dtsi
> > @@ -226,7 +226,7 @@
> >  			pinctrl-0 = <&pinctrl_serial0_flowctrl>;
> >  			pinctrl-1 = <&pinctrl_serial0>;
> >  			clocks = <&clk_s_c0_flexgen CLK_EXT2F_A9>;
> > -
> > +			st,hw-flow-control;
> 
> There is a generic serial binding for this already. As this ST property
> hasn't been used upstream, it seems like it would be worth dropping it
> and switching to the generic uart-has-rtscts one.
> 
> See Documentation/devicetree/bindings/serial/serial.txt
> 
>   - uart-has-rtscts: The presence of this property indicates that the
>     UART has dedicated lines for RTS/CTS hardware flow control, and that
>     they are available for use (wired and enabled by pinmux configuration).
>     This depends on both the UART hardware and the board wiring.
>     Note that this property is mutually-exclusive with "cts-gpios" and
>     "rts-gpios" above.

I've done some digging and I can't see anywhere where
st,hw-flow-control is being used, even in the BSP kernel(s).  It's
also not documented in dt-bindings.  With that in mind, I think it's
probably okay to use the generic binding.

Although, the "Note" at the bottom of the uart-has-rtscts is not
correct in our case, since we handle the case dynamically.

> Also you should put this in the board dtsi, as it is board dependent property.
> By putting it here you are enabling hw-flow-control for all stih407-family
> based boards.

Fair shout.
  
On the B2120 UART0 is hooked up to the Smart Card Reader.
  
Will fix.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1565869 — [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

There are now 2 possible separate/different Pinctrl states which can
be provided from platform data.  One which encompasses the lines
required for HW flow-control (CTS/RTS) and another which does not
specify these lines, such that they can be used via GPIO mechanisms
for manually toggling (i.e. from a request by `stty`).

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index 397df50..03801ed 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -37,10 +37,16 @@
 #define ASC_FIFO_SIZE 16
 #define ASC_MAX_PORTS 8
 
+/* Pinctrl states */
+#define DEFAULT		0
+#define MANUAL_RTS	1
+
 struct asc_port {
 	struct uart_port port;
 	struct gpio_desc *rts;
 	struct clk *clk;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *states[2];
 	unsigned int hw_flow_control:1;
 	unsigned int force_m1:1;
 };
@@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
 {
 	struct uart_port *port = &ascport->port;
 	struct resource *res;
+	int ret;
 
 	port->iotype	= UPIO_MEM;
 	port->flags	= UPF_BOOT_AUTOCONF;
@@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
 	WARN_ON(ascport->port.uartclk == 0);
 	clk_disable_unprepare(ascport->clk);
 
+	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (IS_ERR(ascport->pinctrl)) {
+		ret = PTR_ERR(ascport->pinctrl);
+		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
+	}
+
+	ascport->states[DEFAULT] =
+		pinctrl_lookup_state(ascport->pinctrl, "default");
+	if (IS_ERR(ascport->states[DEFAULT])) {
+		ret = PTR_ERR(ascport->states[DEFAULT]);
+		dev_err(&pdev->dev,
+			"Failed to look up Pinctrl state 'default': %d\n", ret);
+		return ret;
+	}
+
+	/* "manual-rts" state is optional */
+	ascport->states[MANUAL_RTS] =
+		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
+	if (IS_ERR(ascport->states[MANUAL_RTS]))
+		ascport->states[MANUAL_RTS] = NULL;
+
 	return 0;
 }
 
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

#1566508 — Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

Hi Lee,

On Tue, 24 Jan 2017, Lee Jones wrote:

> There are now 2 possible separate/different Pinctrl states which can
> be provided from platform data.  One which encompasses the lines
> required for HW flow-control (CTS/RTS) and another which does not
> specify these lines, such that they can be used via GPIO mechanisms
> for manually toggling (i.e. from a request by `stty`).
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index 397df50..03801ed 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -37,10 +37,16 @@
>  #define ASC_FIFO_SIZE 16
>  #define ASC_MAX_PORTS 8
>  
> +/* Pinctrl states */
> +#define DEFAULT		0
> +#define MANUAL_RTS	1

Nit: Would be better to have them aligned.

> +
>  struct asc_port {
>  	struct uart_port port;
>  	struct gpio_desc *rts;
>  	struct clk *clk;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *states[2];
>  	unsigned int hw_flow_control:1;
>  	unsigned int force_m1:1;
>  };
> @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
>  {
>  	struct uart_port *port = &ascport->port;
>  	struct resource *res;
> +	int ret;
>  
>  	port->iotype	= UPIO_MEM;
>  	port->flags	= UPF_BOOT_AUTOCONF;
> @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
>  	WARN_ON(ascport->port.uartclk == 0);
>  	clk_disable_unprepare(ascport->clk);
>  
> +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(ascport->pinctrl)) {
> +		ret = PTR_ERR(ascport->pinctrl);
> +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> +	}
> +
> +	ascport->states[DEFAULT] =
> +		pinctrl_lookup_state(ascport->pinctrl, "default");
> +	if (IS_ERR(ascport->states[DEFAULT])) {
> +		ret = PTR_ERR(ascport->states[DEFAULT]);
> +		dev_err(&pdev->dev,
> +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* "manual-rts" state is optional */
> +	ascport->states[MANUAL_RTS] =
> +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> +		ascport->states[MANUAL_RTS] = NULL;
> +

The different pinctrl states looks like a neat solution to the problem.

My only concern here is that 'default' state is implying a hw-flow-control
pinmux config, and manual-rts is implying what is the current upstream
'default' pinmux config.

Which maybe ok if you update all uarts, but currently only serial0
is updated. So the other uarts current 'default' is actually the same as serial0
'manual-rts' grouping, which conceptually is odd.

Would it not be better to make 'manual-rts' the default state? As that aligns
to what is currently already the default for the other UARTS? And then make
hw-flow-control the optional state for serial0?

That also has the advantage that 'default' has the same meaning with older DT's.

regards,

Peter.

[toc] | [prev] | [next] | [standalone]

#1568269 — Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

On Wed, 25 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Tue, 24 Jan 2017, Lee Jones wrote:
> 
> > There are now 2 possible separate/different Pinctrl states which can
> > be provided from platform data.  One which encompasses the lines
> > required for HW flow-control (CTS/RTS) and another which does not
> > specify these lines, such that they can be used via GPIO mechanisms
> > for manually toggling (i.e. from a request by `stty`).
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > index 397df50..03801ed 100644
> > --- a/drivers/tty/serial/st-asc.c
> > +++ b/drivers/tty/serial/st-asc.c
> > @@ -37,10 +37,16 @@
> >  #define ASC_FIFO_SIZE 16
> >  #define ASC_MAX_PORTS 8
> >  
> > +/* Pinctrl states */
> > +#define DEFAULT		0
> > +#define MANUAL_RTS	1
> 
> Nit: Would be better to have them aligned.

These are aligned in code.  The patch format throws things out.

Look, same lines with the "> > +" removed:

/* Pinctrl states */
#define DEFAULT		0
#define MANUAL_RTS	1

Try it.

> > +
> >  struct asc_port {
> >  	struct uart_port port;
> >  	struct gpio_desc *rts;
> >  	struct clk *clk;
> > +	struct pinctrl *pinctrl;
> > +	struct pinctrl_state *states[2];
> >  	unsigned int hw_flow_control:1;
> >  	unsigned int force_m1:1;
> >  };
> > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> >  {
> >  	struct uart_port *port = &ascport->port;
> >  	struct resource *res;
> > +	int ret;
> >  
> >  	port->iotype	= UPIO_MEM;
> >  	port->flags	= UPF_BOOT_AUTOCONF;
> > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> >  	WARN_ON(ascport->port.uartclk == 0);
> >  	clk_disable_unprepare(ascport->clk);
> >  
> > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +	if (IS_ERR(ascport->pinctrl)) {
> > +		ret = PTR_ERR(ascport->pinctrl);
> > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > +	}
> > +
> > +	ascport->states[DEFAULT] =
> > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > +		dev_err(&pdev->dev,
> > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* "manual-rts" state is optional */
> > +	ascport->states[MANUAL_RTS] =
> > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > +		ascport->states[MANUAL_RTS] = NULL;
> > +
> 
> The different pinctrl states looks like a neat solution to the problem.
> 
> My only concern here is that 'default' state is implying a hw-flow-control
> pinmux config, and manual-rts is implying what is the current upstream
> 'default' pinmux config.
> 
> Which maybe ok if you update all uarts, but currently only serial0
> is updated. So the other uarts current 'default' is actually the same as serial0
> 'manual-rts' grouping, which conceptually is odd.
> 
> Would it not be better to make 'manual-rts' the default state? As that aligns
> to what is currently already the default for the other UARTS? And then make
> hw-flow-control the optional state for serial0?
> 
> That also has the advantage that 'default' has the same meaning with older DT's.

The reason it was done is this was because none of the other UARTs
require 2 separate Pinctrl configurations, only this one.  Moreover,
if they support RTS/CTS then I believe that the lines should be
defined in Pinctrl.  Thus, it was my plan to update all UART's default
Pinctrl configs to include the RTS/CTS lines.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1569787 — Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

Hi Lee,

On Fri, 27 Jan 2017, Lee Jones wrote:

> On Wed, 25 Jan 2017, Peter Griffin wrote:
> 
> > Hi Lee,
> > 
> > On Tue, 24 Jan 2017, Lee Jones wrote:
> > 
> > > There are now 2 possible separate/different Pinctrl states which can
> > > be provided from platform data.  One which encompasses the lines
> > > required for HW flow-control (CTS/RTS) and another which does not
> > > specify these lines, such that they can be used via GPIO mechanisms
> > > for manually toggling (i.e. from a request by `stty`).
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > index 397df50..03801ed 100644
> > > --- a/drivers/tty/serial/st-asc.c
> > > +++ b/drivers/tty/serial/st-asc.c
> > > @@ -37,10 +37,16 @@
> > >  #define ASC_FIFO_SIZE 16
> > >  #define ASC_MAX_PORTS 8
> > >  
> > > +/* Pinctrl states */
> > > +#define DEFAULT		0
> > > +#define MANUAL_RTS	1
> > 
> > Nit: Would be better to have them aligned.
> 
> These are aligned in code.  The patch format throws things out.
> 
> Look, same lines with the "> > +" removed:

Ah OK.

> 
> /* Pinctrl states */
> #define DEFAULT		0
> #define MANUAL_RTS	1
> 
> Try it.
> 
> > > +
> > >  struct asc_port {
> > >  	struct uart_port port;
> > >  	struct gpio_desc *rts;
> > >  	struct clk *clk;
> > > +	struct pinctrl *pinctrl;
> > > +	struct pinctrl_state *states[2];
> > >  	unsigned int hw_flow_control:1;
> > >  	unsigned int force_m1:1;
> > >  };
> > > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> > >  {
> > >  	struct uart_port *port = &ascport->port;
> > >  	struct resource *res;
> > > +	int ret;
> > >  
> > >  	port->iotype	= UPIO_MEM;
> > >  	port->flags	= UPF_BOOT_AUTOCONF;
> > > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> > >  	WARN_ON(ascport->port.uartclk == 0);
> > >  	clk_disable_unprepare(ascport->clk);
> > >  
> > > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > +	if (IS_ERR(ascport->pinctrl)) {
> > > +		ret = PTR_ERR(ascport->pinctrl);
> > > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > > +	}
> > > +
> > > +	ascport->states[DEFAULT] =
> > > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > > +		dev_err(&pdev->dev,
> > > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* "manual-rts" state is optional */
> > > +	ascport->states[MANUAL_RTS] =
> > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > +		ascport->states[MANUAL_RTS] = NULL;
> > > +
> > 
> > The different pinctrl states looks like a neat solution to the problem.
> > 
> > My only concern here is that 'default' state is implying a hw-flow-control
> > pinmux config, and manual-rts is implying what is the current upstream
> > 'default' pinmux config.
> > 
> > Which maybe ok if you update all uarts, but currently only serial0
> > is updated. So the other uarts current 'default' is actually the same as serial0
> > 'manual-rts' grouping, which conceptually is odd.
> > 
> > Would it not be better to make 'manual-rts' the default state? As that aligns
> > to what is currently already the default for the other UARTS? And then make
> > hw-flow-control the optional state for serial0?
> > 
> > That also has the advantage that 'default' has the same meaning with older DT's.
> 
> The reason it was done is this was because none of the other UARTs
> require 2 separate Pinctrl configurations, only this one.  Moreover,
> if they support RTS/CTS then I believe that the lines should be
> defined in Pinctrl.

Yes I agree with that.

> Thus, it was my plan to update all UART's default
> Pinctrl configs to include the RTS/CTS lines.
> 

I still don't see the point in changing the meaning of 'default' group and breaking
ABI if you don't need to?

As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
groups you get all the benefits of this series whilst also maintaining backwards
compatbility with older DT's.

regards,

Peter.

[toc] | [prev] | [next] | [standalone]

#1569839 — Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

On Mon, 30 Jan 2017, Peter Griffin wrote:

> Hi Lee,
> 
> On Fri, 27 Jan 2017, Lee Jones wrote:
> 
> > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > 
> > > Hi Lee,
> > > 
> > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > 
> > > > There are now 2 possible separate/different Pinctrl states which can
> > > > be provided from platform data.  One which encompasses the lines
> > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > for manually toggling (i.e. from a request by `stty`).
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > > 
> > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > index 397df50..03801ed 100644
> > > > --- a/drivers/tty/serial/st-asc.c
> > > > +++ b/drivers/tty/serial/st-asc.c
> > > > @@ -37,10 +37,16 @@
> > > >  #define ASC_FIFO_SIZE 16
> > > >  #define ASC_MAX_PORTS 8
> > > >  
> > > > +/* Pinctrl states */
> > > > +#define DEFAULT		0
> > > > +#define MANUAL_RTS	1
> > > 
> > > Nit: Would be better to have them aligned.
> > 
> > These are aligned in code.  The patch format throws things out.
> > 
> > Look, same lines with the "> > +" removed:
> 
> Ah OK.
> 
> > 
> > /* Pinctrl states */
> > #define DEFAULT		0
> > #define MANUAL_RTS	1
> > 
> > Try it.
> > 
> > > > +
> > > >  struct asc_port {
> > > >  	struct uart_port port;
> > > >  	struct gpio_desc *rts;
> > > >  	struct clk *clk;
> > > > +	struct pinctrl *pinctrl;
> > > > +	struct pinctrl_state *states[2];
> > > >  	unsigned int hw_flow_control:1;
> > > >  	unsigned int force_m1:1;
> > > >  };
> > > > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> > > >  {
> > > >  	struct uart_port *port = &ascport->port;
> > > >  	struct resource *res;
> > > > +	int ret;
> > > >  
> > > >  	port->iotype	= UPIO_MEM;
> > > >  	port->flags	= UPF_BOOT_AUTOCONF;
> > > > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> > > >  	WARN_ON(ascport->port.uartclk == 0);
> > > >  	clk_disable_unprepare(ascport->clk);
> > > >  
> > > > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > > +	if (IS_ERR(ascport->pinctrl)) {
> > > > +		ret = PTR_ERR(ascport->pinctrl);
> > > > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > > > +	}
> > > > +
> > > > +	ascport->states[DEFAULT] =
> > > > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > > > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > > > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > > > +		dev_err(&pdev->dev,
> > > > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	/* "manual-rts" state is optional */
> > > > +	ascport->states[MANUAL_RTS] =
> > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > +
> > > 
> > > The different pinctrl states looks like a neat solution to the problem.
> > > 
> > > My only concern here is that 'default' state is implying a hw-flow-control
> > > pinmux config, and manual-rts is implying what is the current upstream
> > > 'default' pinmux config.
> > > 
> > > Which maybe ok if you update all uarts, but currently only serial0
> > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > 'manual-rts' grouping, which conceptually is odd.
> > > 
> > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > to what is currently already the default for the other UARTS? And then make
> > > hw-flow-control the optional state for serial0?
> > > 
> > > That also has the advantage that 'default' has the same meaning with older DT's.
> > 
> > The reason it was done is this was because none of the other UARTs
> > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > if they support RTS/CTS then I believe that the lines should be
> > defined in Pinctrl.
> 
> Yes I agree with that.
> 
> > Thus, it was my plan to update all UART's default
> > Pinctrl configs to include the RTS/CTS lines.
> > 
> 
> I still don't see the point in changing the meaning of 'default' group and breaking
> ABI if you don't need to?
> 
> As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> groups you get all the benefits of this series whilst also maintaining backwards
> compatbility with older DT's.

What makes you think this will break ABI?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1569870 — Re: [STLinux Kernel] [PATCH 3/8] serial: st-asc: Read in all Pinctrl states

Hi Lee,

On Mon, 30 Jan 2017, Lee Jones wrote:

> On Mon, 30 Jan 2017, Peter Griffin wrote:
> 
> > Hi Lee,
> > 
> > On Fri, 27 Jan 2017, Lee Jones wrote:
> > 
> > > On Wed, 25 Jan 2017, Peter Griffin wrote:
> > > 
> > > > Hi Lee,
> > > > 
> > > > On Tue, 24 Jan 2017, Lee Jones wrote:
> > > > 
> > > > > There are now 2 possible separate/different Pinctrl states which can
> > > > > be provided from platform data.  One which encompasses the lines
> > > > > required for HW flow-control (CTS/RTS) and another which does not
> > > > > specify these lines, such that they can be used via GPIO mechanisms
> > > > > for manually toggling (i.e. from a request by `stty`).
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/tty/serial/st-asc.c | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> > > > > index 397df50..03801ed 100644
> > > > > --- a/drivers/tty/serial/st-asc.c
> > > > > +++ b/drivers/tty/serial/st-asc.c
> > > > > @@ -37,10 +37,16 @@
> > > > >  #define ASC_FIFO_SIZE 16
> > > > >  #define ASC_MAX_PORTS 8
> > > > >  
> > > > > +/* Pinctrl states */
> > > > > +#define DEFAULT		0
> > > > > +#define MANUAL_RTS	1
> > > > 
> > > > Nit: Would be better to have them aligned.
> > > 
> > > These are aligned in code.  The patch format throws things out.
> > > 
> > > Look, same lines with the "> > +" removed:
> > 
> > Ah OK.
> > 
> > > 
> > > /* Pinctrl states */
> > > #define DEFAULT		0
> > > #define MANUAL_RTS	1
> > > 
> > > Try it.
> > > 
> > > > > +
> > > > >  struct asc_port {
> > > > >  	struct uart_port port;
> > > > >  	struct gpio_desc *rts;
> > > > >  	struct clk *clk;
> > > > > +	struct pinctrl *pinctrl;
> > > > > +	struct pinctrl_state *states[2];
> > > > >  	unsigned int hw_flow_control:1;
> > > > >  	unsigned int force_m1:1;
> > > > >  };
> > > > > @@ -694,6 +700,7 @@ static int asc_init_port(struct asc_port *ascport,
> > > > >  {
> > > > >  	struct uart_port *port = &ascport->port;
> > > > >  	struct resource *res;
> > > > > +	int ret;
> > > > >  
> > > > >  	port->iotype	= UPIO_MEM;
> > > > >  	port->flags	= UPF_BOOT_AUTOCONF;
> > > > > @@ -720,6 +727,27 @@ static int asc_init_port(struct asc_port *ascport,
> > > > >  	WARN_ON(ascport->port.uartclk == 0);
> > > > >  	clk_disable_unprepare(ascport->clk);
> > > > >  
> > > > > +	ascport->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > > > +	if (IS_ERR(ascport->pinctrl)) {
> > > > > +		ret = PTR_ERR(ascport->pinctrl);
> > > > > +		dev_err(&pdev->dev, "Failed to get Pinctrl: %d\n", ret);
> > > > > +	}
> > > > > +
> > > > > +	ascport->states[DEFAULT] =
> > > > > +		pinctrl_lookup_state(ascport->pinctrl, "default");
> > > > > +	if (IS_ERR(ascport->states[DEFAULT])) {
> > > > > +		ret = PTR_ERR(ascport->states[DEFAULT]);
> > > > > +		dev_err(&pdev->dev,
> > > > > +			"Failed to look up Pinctrl state 'default': %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	/* "manual-rts" state is optional */
> > > > > +	ascport->states[MANUAL_RTS] =
> > > > > +		pinctrl_lookup_state(ascport->pinctrl, "manual-rts");
> > > > > +	if (IS_ERR(ascport->states[MANUAL_RTS]))
> > > > > +		ascport->states[MANUAL_RTS] = NULL;
> > > > > +
> > > > 
> > > > The different pinctrl states looks like a neat solution to the problem.
> > > > 
> > > > My only concern here is that 'default' state is implying a hw-flow-control
> > > > pinmux config, and manual-rts is implying what is the current upstream
> > > > 'default' pinmux config.
> > > > 
> > > > Which maybe ok if you update all uarts, but currently only serial0
> > > > is updated. So the other uarts current 'default' is actually the same as serial0
> > > > 'manual-rts' grouping, which conceptually is odd.
> > > > 
> > > > Would it not be better to make 'manual-rts' the default state? As that aligns
> > > > to what is currently already the default for the other UARTS? And then make
> > > > hw-flow-control the optional state for serial0?
> > > > 
> > > > That also has the advantage that 'default' has the same meaning with older DT's.
> > > 
> > > The reason it was done is this was because none of the other UARTs
> > > require 2 separate Pinctrl configurations, only this one.  Moreover,
> > > if they support RTS/CTS then I believe that the lines should be
> > > defined in Pinctrl.
> > 
> > Yes I agree with that.
> > 
> > > Thus, it was my plan to update all UART's default
> > > Pinctrl configs to include the RTS/CTS lines.
> > > 
> > 
> > I still don't see the point in changing the meaning of 'default' group and breaking
> > ABI if you don't need to?
> > 
> > As far as I can tell if you swap the meaning of 'default' and 'maunal-rts'
> > groups you get all the benefits of this series whilst also maintaining backwards
> > compatbility with older DT's.
> 
> What makes you think this will break ABI?
>

I've not tried it, but an older DT defines one group, 'default' which contains
the same pin config as your new optional 'manual-rts' group.

The driver now reads like the manual-rts pin config is optional and should be stored in
ascport->states[MANUAL_RTS]. An older DT will pass that same pin config as the default
group and it will be stored in ascport->states[DEFAULT].

That seems wrong to me, and if it executes OK it wouldn't be what you
expect by reading the code.

regards,

Peter.

[toc] | [prev] | [next] | [standalone]

#1566440

On Tue, Jan 24, 2017 at 01:43:02PM +0000, Lee Jones wrote:
> When hardware flow-control is disabled, manual toggling of the UART's
> reset line (RTS) using userland applications (e.g. stty) is not
> possible, since the ASC IP does not provide this functionality in the
> same was as some other IPs do.  Thus, we have to do this manually.
> 
> This set ensures the correct Pinctrl groups are configured and
> obtained for both manual toggling of the RTS line and for the IP to
> take over the lines when HW flow-control is requested by the user.
> 
> Lee Jones (8):
>   serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled
>   serial: st-asc: Provide RTS functionality
>   serial: st-asc: Read in all Pinctrl states
>   serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
>   ARM: dts: STiH410-b2260: Identify the UART RTS line
>   ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control
>   ARM: dts: STiH407-family: Use new Pinctrl groups
>   ARM: dts: STiH407-family: Enable HW flow-control
> 
>  arch/arm/boot/dts/stih407-family.dtsi  |  7 +--
>  arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++-
>  arch/arm/boot/dts/stih410-b2260.dts    |  1 +
>  drivers/tty/serial/st-asc.c            | 98 +++++++++++++++++++++++++++++++---
>  4 files changed, 105 insertions(+), 13 deletions(-)
> 
> -- 
> 2.10.2

I'll ignore this series due to the kbuild problems :(

[toc] | [prev] | [next] | [standalone]

#1566687

On Wed, 25 Jan 2017, Greg KH wrote:

> On Tue, Jan 24, 2017 at 01:43:02PM +0000, Lee Jones wrote:
> > When hardware flow-control is disabled, manual toggling of the UART's
> > reset line (RTS) using userland applications (e.g. stty) is not
> > possible, since the ASC IP does not provide this functionality in the
> > same was as some other IPs do.  Thus, we have to do this manually.
> > 
> > This set ensures the correct Pinctrl groups are configured and
> > obtained for both manual toggling of the RTS line and for the IP to
> > take over the lines when HW flow-control is requested by the user.
> > 
> > Lee Jones (8):
> >   serial: st-asc: Ignore the parity error bit if 8-bit mode is enabled
> >   serial: st-asc: Provide RTS functionality
> >   serial: st-asc: Read in all Pinctrl states
> >   serial: st-asc: (De)Register GPIOD and swap Pinctrl profiles
> >   ARM: dts: STiH410-b2260: Identify the UART RTS line
> >   ARM: dts: STiH407-pinctrl: Add Pinctrl group for HW flow-control
> >   ARM: dts: STiH407-family: Use new Pinctrl groups
> >   ARM: dts: STiH407-family: Enable HW flow-control
> > 
> >  arch/arm/boot/dts/stih407-family.dtsi  |  7 +--
> >  arch/arm/boot/dts/stih407-pinctrl.dtsi | 12 ++++-
> >  arch/arm/boot/dts/stih410-b2260.dts    |  1 +
> >  drivers/tty/serial/st-asc.c            | 98 +++++++++++++++++++++++++++++++---
> >  4 files changed, 105 insertions(+), 13 deletions(-)
> > 
> 
> I'll ignore this series due to the kbuild problems :(

It's strange.

These didn't happen locally ... and aiaiai was also happy.

Nevertheless, I will fix-up and resend.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web