[PATCH v2] serial: pl011: UPSTAT_AUTORTS requires .throttle/unthrottle

[PATCH v2] serial: pl011: UPSTAT_AUTORTS requires .throttle/unthrottle

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

The driver must provide throttle and unthrottle in uart_ops when it
sets UPSTAT_AUTORTS. Add them using existing stop_rx &
enable_interrupts functions.

Compile tested (w/o linking).

Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
Fixes: 2a76fa283098 (serial: pl011: Adopt generic flag to store auto
		     RTS status)
Cc: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

---
v2:
- Fix dbl locking in pl011_unthrottle_rx calling pl011_enable_interrupts

 drivers/tty/serial/amba-pl011.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 4d11a3e547f9..8f9bf32e278a 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1339,6 +1339,15 @@ static void pl011_stop_rx(struct uart_port *port)
 	pl011_dma_rx_stop(uap);
 }
 
+static void pl011_throttle_rx(struct uart_port *port)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+	pl011_stop_rx(port);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
 static void pl011_enable_ms(struct uart_port *port)
 {
 	struct uart_amba_port *uap =
@@ -1760,9 +1769,10 @@ static int pl011_allocate_irq(struct uart_amba_port *uap)
  */
 static void pl011_enable_interrupts(struct uart_amba_port *uap)
 {
+	unsigned long flags;
 	unsigned int i;
 
-	spin_lock_irq(&uap->port.lock);
+	spin_lock_irqsave(&uap->port.lock, flags);
 
 	/* Clear out any spuriously appearing RX interrupts */
 	pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
@@ -1784,7 +1794,14 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
 	if (!pl011_dma_rx_running(uap))
 		uap->im |= UART011_RXIM;
 	pl011_write(uap->im, uap, REG_IMSC);
-	spin_unlock_irq(&uap->port.lock);
+	spin_unlock_irqrestore(&uap->port.lock, flags);
+}
+
+static void pl011_unthrottle_rx(struct uart_port *port)
+{
+	struct uart_amba_port *uap = container_of(port, struct uart_amba_port, port);
+
+	pl011_enable_interrupts(uap);
 }
 
 static int pl011_startup(struct uart_port *port)
@@ -2243,6 +2260,8 @@ static const struct uart_ops sbsa_uart_pops = {
 	.stop_tx	= pl011_stop_tx,
 	.start_tx	= pl011_start_tx,
 	.stop_rx	= pl011_stop_rx,
+	.throttle	= pl011_throttle_rx,
+	.unthrottle	= pl011_unthrottle_rx,
 	.startup	= sbsa_uart_startup,
 	.shutdown	= sbsa_uart_shutdown,
 	.set_termios	= sbsa_uart_set_termios,

-- 
tg: (19317433057d..) pl011/add-throttle (depends on: tty-linus)

Re: [PATCH v2] serial: pl011: UPSTAT_AUTORTS requires .throttle/unthrottle

[Greg KH]

On Fri, Apr 29, 2022 at 02:57:16PM +0300, Ilpo Järvinen wrote:
> The driver must provide throttle and unthrottle in uart_ops when it
> sets UPSTAT_AUTORTS. Add them using existing stop_rx &
> enable_interrupts functions.
> 
> Compile tested (w/o linking).

That shouldn't be here at all, put it below the --- line if its needed.


> 
> Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
> Fixes: 2a76fa283098 (serial: pl011: Adopt generic flag to store auto
> 		     RTS status)

Nit, this should be all in one line.

> Cc: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Any reason why you are not cc:ing all the needed people as per
scripts/get_maintainer.pl?

thanks,

greg k-h

Re: [PATCH v2] serial: pl011: UPSTAT_AUTORTS requires .throttle/unthrottle

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

On Fri, 29 Apr 2022, Greg KH wrote:

> On Fri, Apr 29, 2022 at 02:57:16PM +0300, Ilpo Järvinen wrote:
> > The driver must provide throttle and unthrottle in uart_ops when it
> > sets UPSTAT_AUTORTS. Add them using existing stop_rx &
> > enable_interrupts functions.
> > 
> > Compile tested (w/o linking).
> 
> That shouldn't be here at all, put it below the --- line if its needed.
> 
> 
> > 
> > Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
> > Fixes: 2a76fa283098 (serial: pl011: Adopt generic flag to store auto
> > 		     RTS status)
> 
> Nit, this should be all in one line.
>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Any reason why you are not cc:ing all the needed people as per
> scripts/get_maintainer.pl?

Yes there is ;-). Around here I'm being told that I should generally
avoid as much as possible bugging you unnecessarily. ...Or sending garbage
to you which I feel a compile tested patch for hw I'm not particularly 
familiar with surely could be. After all this should be tested first and 
then I'd be resending another version with Tested-by: xx Cc'ing you
and you would avoid all those intermediate garbage versions :-).


-- 
 i.

Re: [PATCH v2] serial: pl011: UPSTAT_AUTORTS requires .throttle/unthrottle

[Greg KH]

On Fri, Apr 29, 2022 at 03:33:00PM +0300, Ilpo Järvinen wrote:
> On Fri, 29 Apr 2022, Greg KH wrote:
> 
> > On Fri, Apr 29, 2022 at 02:57:16PM +0300, Ilpo Järvinen wrote:
> > > The driver must provide throttle and unthrottle in uart_ops when it
> > > sets UPSTAT_AUTORTS. Add them using existing stop_rx &
> > > enable_interrupts functions.
> > > 
> > > Compile tested (w/o linking).
> > 
> > That shouldn't be here at all, put it below the --- line if its needed.
> > 
> > 
> > > 
> > > Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
> > > Fixes: 2a76fa283098 (serial: pl011: Adopt generic flag to store auto
> > > 		     RTS status)
> > 
> > Nit, this should be all in one line.
> >
> > > Cc: Lukas Wunner <lukas@wunner.de>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > Any reason why you are not cc:ing all the needed people as per
> > scripts/get_maintainer.pl?
> 
> Yes there is ;-). Around here I'm being told that I should generally
> avoid as much as possible bugging you unnecessarily. ...Or sending garbage
> to you which I feel a compile tested patch for hw I'm not particularly 
> familiar with surely could be. After all this should be tested first and 
> then I'd be resending another version with Tested-by: xx Cc'ing you
> and you would avoid all those intermediate garbage versions :-).

As you can tell, I do see these patches and get concerned when you don't
want the subsystem maintainers to review them.

And our tools can pick up the tested-by lines just fine.  If you don't
want us to apply them, then use RFC and I will gladly ignore it.

thanks,

greg k-h