| Message ID | 20210304213902.83903-22-marcan@marcan.st |
|---|---|
| State | New |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
On 04/03/2021 22:38, Hector Martin wrote: > * Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq, > where only the latter acquires the port lock. This will be necessary > on platforms which have edge-triggered IRQs, as we need to call > s3c24xx_serial_tx_chars to kick off transmission from outside IRQ > context, with the port lock held. > > * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for > consistency with the above. All it does now is call two other > functions anyway. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/tty/serial/samsung_tty.c | 34 +++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
On 06/03/2021 00.17, Andy Shevchenko wrote:
> Add a separate change that removes flags from the spin lock in the IRQ handler.
This commit should have no functional changes; I am just splitting an
existing function into two, where one takes the lock and the other does
the work. Do you mean using a different locking function? I'm not
entirely sure what you're suggesting.
--
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub
On 06/03/2021 01.20, Andy Shevchenko wrote: >> I am just splitting an >> existing function into two, where one takes the lock and the other does >> the work. Do you mean using a different locking function? I'm not >> entirely sure what you're suggesting. > > Yes, as a prerequisite > > spin_lock_irqsave -> spin_lock(). Krzysztof, is this something you want in this series? I was trying to avoid logic changes to the non-Apple paths. -- Hector Martin (marcan@marcan.st) Public Key: https://mrcn.st/pub
On Sun, Mar 7, 2021 at 12:34 PM Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > On 05/03/2021 17:29, Hector Martin wrote: > > On 06/03/2021 01.20, Andy Shevchenko wrote: > >>> I am just splitting an > >>> existing function into two, where one takes the lock and the other does > >>> the work. Do you mean using a different locking function? I'm not > >>> entirely sure what you're suggesting. > >> > >> Yes, as a prerequisite > >> > >> spin_lock_irqsave -> spin_lock(). > > > > Krzysztof, is this something you want in this series? I was trying to > > avoid logic changes to the non-Apple paths. > > I don't quite get the need for such change (the code will be still > called in interrupt handler, right?), but assuming the "why?" is > properly documented, it can be a separate patch here. This is only for readability: the common rule is to not disable interrupts when they are already disabled, so a reader might wonder if this instance of the handler is special in some case that it might be called with interrupts enabled. There is also a small overhead in accessing the global irq mask register on some architectures, but for a uart that does not make any difference of course. While I'm generally in favor of that kind of cleanup, I'd also prefer to leave it out of this series -- once you get into details like this the series gets harder to review. Arnd
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 39b2eb165bdc..7106eb238d8c 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -827,7 +827,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id) return IRQ_HANDLED; } -static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id) +static irqreturn_t s3c24xx_serial_rx_irq(int irq, void *dev_id) { struct s3c24xx_uart_port *ourport = dev_id; @@ -836,16 +836,12 @@ static irqreturn_t s3c24xx_serial_rx_chars(int irq, void *dev_id) return s3c24xx_serial_rx_chars_pio(dev_id); } -static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) +static void s3c24xx_serial_tx_chars(struct s3c24xx_uart_port *ourport) { - struct s3c24xx_uart_port *ourport = id; struct uart_port *port = &ourport->port; struct circ_buf *xmit = &port->state->xmit; - unsigned long flags; int count, dma_count = 0; - spin_lock_irqsave(&port->lock, flags); - count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE); if (ourport->dma && ourport->dma->tx_chan && @@ -862,7 +858,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) wr_reg(port, S3C2410_UTXH, port->x_char); port->icount.tx++; port->x_char = 0; - goto out; + return; } /* if there isn't anything more to transmit, or the uart is now @@ -871,7 +867,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) if (uart_circ_empty(xmit) || uart_tx_stopped(port)) { s3c24xx_serial_stop_tx(port); - goto out; + return; } /* try and drain the buffer... */ @@ -893,7 +889,7 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) if (!count && dma_count) { s3c24xx_serial_start_tx_dma(ourport, dma_count); - goto out; + return; } if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) { @@ -904,8 +900,18 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) if (uart_circ_empty(xmit)) s3c24xx_serial_stop_tx(port); +} + +static irqreturn_t s3c24xx_serial_tx_irq(int irq, void *id) +{ + struct s3c24xx_uart_port *ourport = id; + struct uart_port *port = &ourport->port; + unsigned long flags; + + spin_lock_irqsave(&port->lock, flags); + + s3c24xx_serial_tx_chars(ourport); -out: spin_unlock_irqrestore(&port->lock, flags); return IRQ_HANDLED; } @@ -919,11 +925,11 @@ static irqreturn_t s3c64xx_serial_handle_irq(int irq, void *id) irqreturn_t ret = IRQ_HANDLED; if (pend & S3C64XX_UINTM_RXD_MSK) { - ret = s3c24xx_serial_rx_chars(irq, id); + ret = s3c24xx_serial_rx_irq(irq, id); wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_RXD_MSK); } if (pend & S3C64XX_UINTM_TXD_MSK) { - ret = s3c24xx_serial_tx_chars(irq, id); + ret = s3c24xx_serial_tx_irq(irq, id); wr_regl(port, S3C64XX_UINTP, S3C64XX_UINTM_TXD_MSK); } return ret; @@ -1155,7 +1161,7 @@ static int s3c24xx_serial_startup(struct uart_port *port) ourport->rx_enabled = 1; - ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_chars, 0, + ret = request_irq(ourport->rx_irq, s3c24xx_serial_rx_irq, 0, s3c24xx_serial_portname(port), ourport); if (ret != 0) { @@ -1169,7 +1175,7 @@ static int s3c24xx_serial_startup(struct uart_port *port) ourport->tx_enabled = 1; - ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_chars, 0, + ret = request_irq(ourport->tx_irq, s3c24xx_serial_tx_irq, 0, s3c24xx_serial_portname(port), ourport); if (ret) {
* Split out s3c24xx_serial_tx_chars from s3c24xx_serial_tx_irq, where only the latter acquires the port lock. This will be necessary on platforms which have edge-triggered IRQs, as we need to call s3c24xx_serial_tx_chars to kick off transmission from outside IRQ context, with the port lock held. * Rename s3c24xx_serial_rx_chars to s3c24xx_serial_rx_irq for consistency with the above. All it does now is call two other functions anyway. Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/tty/serial/samsung_tty.c | 34 +++++++++++++++++++------------- 1 file changed, 20 insertions(+), 14 deletions(-)