| Message ID | 20210215121713.57687-18-marcan@marcan.st (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
On Mon, Feb 15, 2021 at 09:17:05PM +0900, Hector Martin wrote: > Instead of patching a single global ops structure depending on the port > type, use a separate s3c64xx_serial_ops for the S3C64XX type. This > allows us to mark the structures as const. > > Also split out s3c64xx_serial_shutdown into a separate function now that > we have a separate ops structure; this avoids excessive branching > control flow and mirrors s3c64xx_serial_startup. tx_claimed and > rx_claimed are only used in the S3C24XX functions. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/tty/serial/samsung_tty.c | 69 ++++++++++++++++++++++++-------- > 1 file changed, 53 insertions(+), 16 deletions(-) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index 8ae3e03fbd8c..6b661f3ec1ae 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -1098,27 +1098,36 @@ static void s3c24xx_serial_shutdown(struct uart_port *port) > struct s3c24xx_uart_port *ourport = to_ourport(port); > > if (ourport->tx_claimed) { > - if (!s3c24xx_serial_has_interrupt_mask(port)) > - free_irq(ourport->tx_irq, ourport); > + free_irq(ourport->tx_irq, ourport); > ourport->tx_enabled = 0; > ourport->tx_claimed = 0; > ourport->tx_mode = 0; > } > > if (ourport->rx_claimed) { > - if (!s3c24xx_serial_has_interrupt_mask(port)) > - free_irq(ourport->rx_irq, ourport); > + free_irq(ourport->rx_irq, ourport); > ourport->rx_claimed = 0; > ourport->rx_enabled = 0; > } > > - /* Clear pending interrupts and mask all interrupts */ > - if (s3c24xx_serial_has_interrupt_mask(port)) { > - free_irq(port->irq, ourport); > + if (ourport->dma) > + s3c24xx_serial_release_dma(ourport); > > - wr_regl(port, S3C64XX_UINTP, 0xf); > - wr_regl(port, S3C64XX_UINTM, 0xf); > - } > + ourport->tx_in_progress = 0; > +} > + > +static void s3c64xx_serial_shutdown(struct uart_port *port) > +{ > + struct s3c24xx_uart_port *ourport = to_ourport(port); > + > + free_irq(port->irq, ourport); > + > + wr_regl(port, S3C64XX_UINTP, 0xf); > + wr_regl(port, S3C64XX_UINTM, 0xf); > + > + ourport->tx_enabled = 0; > + ourport->tx_mode = 0; > + ourport->rx_enabled = 0; For S3C64xx type this is not equivalent: the assignments were happening before free_irq() and wr_regl(). Honestly I don't know whether it matters (except some barriers coming from these functions) but please do not change the order of code in this patch. If needed, the re-ordering should be a patch on its own. With explanation why. > > if (ourport->dma) > s3c24xx_serial_release_dma(ourport); > @@ -1193,9 +1202,7 @@ static int s3c64xx_serial_startup(struct uart_port *port) > > /* For compatibility with s3c24xx Soc's */ > ourport->rx_enabled = 1; > - ourport->rx_claimed = 1; > ourport->tx_enabled = 0; > - ourport->tx_claimed = 1; > > spin_lock_irqsave(&port->lock, flags); > > @@ -1631,6 +1638,29 @@ static struct uart_ops s3c24xx_serial_ops = { > #endif > }; > > +static const struct uart_ops s3c64xx_serial_ops = { > + .pm = s3c24xx_serial_pm, > + .tx_empty = s3c24xx_serial_tx_empty, > + .get_mctrl = s3c24xx_serial_get_mctrl, > + .set_mctrl = s3c24xx_serial_set_mctrl, > + .stop_tx = s3c24xx_serial_stop_tx, > + .start_tx = s3c24xx_serial_start_tx, > + .stop_rx = s3c24xx_serial_stop_rx, > + .break_ctl = s3c24xx_serial_break_ctl, > + .startup = s3c64xx_serial_startup, > + .shutdown = s3c64xx_serial_shutdown, > + .set_termios = s3c24xx_serial_set_termios, > + .type = s3c24xx_serial_type, > + .release_port = s3c24xx_serial_release_port, > + .request_port = s3c24xx_serial_request_port, > + .config_port = s3c24xx_serial_config_port, > + .verify_port = s3c24xx_serial_verify_port, > +#if defined(CONFIG_SERIAL_SAMSUNG_CONSOLE) && defined(CONFIG_CONSOLE_POLL) > + .poll_get_char = s3c24xx_serial_get_poll_char, > + .poll_put_char = s3c24xx_serial_put_poll_char, > +#endif > +}; > + Make the s3c24xx_serial_ops const as well. Best regards, Krzysztof
On 16/02/2021 03.06, Krzysztof Kozlowski wrote: > On Mon, Feb 15, 2021 at 09:17:05PM +0900, Hector Martin wrote: >> +static void s3c64xx_serial_shutdown(struct uart_port *port) >> +{ >> + struct s3c24xx_uart_port *ourport = to_ourport(port); >> + >> + free_irq(port->irq, ourport); >> + >> + wr_regl(port, S3C64XX_UINTP, 0xf); >> + wr_regl(port, S3C64XX_UINTM, 0xf); >> + >> + ourport->tx_enabled = 0; >> + ourport->tx_mode = 0; >> + ourport->rx_enabled = 0; > > For S3C64xx type this is not equivalent: the assignments were > happening before free_irq() and wr_regl(). Honestly I don't know whether > it matters (except some barriers coming from these functions) but please > do not change the order of code in this patch. If needed, the > re-ordering should be a patch on its own. With explanation why. Honestly, I think if anything the masking should happen first (to make sure no IRQs go off), but at this point it's probably better to play it safe and not introduce any logic changes, so I've moved the assignments first to retain the old behavior. > Make the s3c24xx_serial_ops const as well. Done for v3, thanks.
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 8ae3e03fbd8c..6b661f3ec1ae 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -1098,27 +1098,36 @@ static void s3c24xx_serial_shutdown(struct uart_port *port) struct s3c24xx_uart_port *ourport = to_ourport(port); if (ourport->tx_claimed) { - if (!s3c24xx_serial_has_interrupt_mask(port)) - free_irq(ourport->tx_irq, ourport); + free_irq(ourport->tx_irq, ourport); ourport->tx_enabled = 0; ourport->tx_claimed = 0; ourport->tx_mode = 0; } if (ourport->rx_claimed) { - if (!s3c24xx_serial_has_interrupt_mask(port)) - free_irq(ourport->rx_irq, ourport); + free_irq(ourport->rx_irq, ourport); ourport->rx_claimed = 0; ourport->rx_enabled = 0; } - /* Clear pending interrupts and mask all interrupts */ - if (s3c24xx_serial_has_interrupt_mask(port)) { - free_irq(port->irq, ourport); + if (ourport->dma) + s3c24xx_serial_release_dma(ourport); - wr_regl(port, S3C64XX_UINTP, 0xf); - wr_regl(port, S3C64XX_UINTM, 0xf); - } + ourport->tx_in_progress = 0; +} + +static void s3c64xx_serial_shutdown(struct uart_port *port) +{ + struct s3c24xx_uart_port *ourport = to_ourport(port); + + free_irq(port->irq, ourport); + + wr_regl(port, S3C64XX_UINTP, 0xf); + wr_regl(port, S3C64XX_UINTM, 0xf); + + ourport->tx_enabled = 0; + ourport->tx_mode = 0; + ourport->rx_enabled = 0; if (ourport->dma) s3c24xx_serial_release_dma(ourport); @@ -1193,9 +1202,7 @@ static int s3c64xx_serial_startup(struct uart_port *port) /* For compatibility with s3c24xx Soc's */ ourport->rx_enabled = 1; - ourport->rx_claimed = 1; ourport->tx_enabled = 0; - ourport->tx_claimed = 1; spin_lock_irqsave(&port->lock, flags); @@ -1631,6 +1638,29 @@ static struct uart_ops s3c24xx_serial_ops = { #endif }; +static const struct uart_ops s3c64xx_serial_ops = { + .pm = s3c24xx_serial_pm, + .tx_empty = s3c24xx_serial_tx_empty, + .get_mctrl = s3c24xx_serial_get_mctrl, + .set_mctrl = s3c24xx_serial_set_mctrl, + .stop_tx = s3c24xx_serial_stop_tx, + .start_tx = s3c24xx_serial_start_tx, + .stop_rx = s3c24xx_serial_stop_rx, + .break_ctl = s3c24xx_serial_break_ctl, + .startup = s3c64xx_serial_startup, + .shutdown = s3c64xx_serial_shutdown, + .set_termios = s3c24xx_serial_set_termios, + .type = s3c24xx_serial_type, + .release_port = s3c24xx_serial_release_port, + .request_port = s3c24xx_serial_request_port, + .config_port = s3c24xx_serial_config_port, + .verify_port = s3c24xx_serial_verify_port, +#if defined(CONFIG_SERIAL_SAMSUNG_CONSOLE) && defined(CONFIG_CONSOLE_POLL) + .poll_get_char = s3c24xx_serial_get_poll_char, + .poll_put_char = s3c24xx_serial_put_poll_char, +#endif +}; + static struct uart_driver s3c24xx_uart_drv = { .owner = THIS_MODULE, .driver_name = "s3c2410_serial", @@ -1868,10 +1898,6 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport, /* setup info for port */ port->dev = &platdev->dev; - /* Startup sequence is different for s3c64xx and higher SoC's */ - if (s3c24xx_serial_has_interrupt_mask(port)) - s3c24xx_serial_ops.startup = s3c64xx_serial_startup; - port->uartclk = 1; if (cfg->uart_flags & UPF_CONS_FLOW) { @@ -2019,6 +2045,17 @@ static int s3c24xx_serial_probe(struct platform_device *pdev) dev_get_platdata(&pdev->dev) : ourport->drv_data->def_cfg; + switch (ourport->info->type) { + case PORT_S3C2410: + case PORT_S3C2412: + case PORT_S3C2440: + ourport->port.ops = &s3c24xx_serial_ops; + break; + case PORT_S3C6400: + ourport->port.ops = &s3c64xx_serial_ops; + break; + } + if (np) { of_property_read_u32(np, "samsung,uart-fifosize", &ourport->port.fifosize);
Instead of patching a single global ops structure depending on the port type, use a separate s3c64xx_serial_ops for the S3C64XX type. This allows us to mark the structures as const. Also split out s3c64xx_serial_shutdown into a separate function now that we have a separate ops structure; this avoids excessive branching control flow and mirrors s3c64xx_serial_startup. tx_claimed and rx_claimed are only used in the S3C24XX functions. Signed-off-by: Hector Martin <marcan@marcan.st> --- drivers/tty/serial/samsung_tty.c | 69 ++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 16 deletions(-)