Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs
From: Andy Shevchenko
Date: Fri Mar 05 2021 - 10:29:39 EST
On Thu, Mar 4, 2021 at 11:42 PM Hector Martin <marcan@xxxxxxxxx> wrote:
>
> Apple SoCs are a distant descendant of Samsung designs and use yet
> another variant of their UART style, with different interrupt handling.
>
> In particular, this variant has the following differences with existing
> ones:
>
> * It includes a built-in interrupt controller with different registers,
> using only a single platform IRQ
>
> * Internal interrupt sources are treated as edge-triggered, even though
> the IRQ output is level-triggered. This chiefly affects the TX IRQ
> path: the driver can no longer rely on the TX buffer empty IRQ
> immediately firing after TX is enabled, but instead must prime the
> FIFO with data directly.
...
> + case TYPE_APPLE_S5L:
> + WARN_ON(1); // No DMA
Oh, no, please use the ONCE variant.
...
> + /* Apple types use these bits for IRQ masks */
> + if (ourport->info->type != TYPE_APPLE_S5L) {
> + ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK |
> + S3C64XX_UCON_EMPTYINT_EN |
> + S3C64XX_UCON_DMASUS_EN |
> + S3C64XX_UCON_TIMEOUT_EN);
> + ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT |
Can you spell 0xf with named constant(s), please?
In case they are repetitive via the code, introduce either a temporary
variable (in case it scoped to one function only), or define it as a
constant.
> + S3C64XX_UCON_TIMEOUT_EN;
> + }
...
> +/* interrupt handler for Apple SoC's.*/
> +static irqreturn_t apple_serial_handle_irq(int irq, void *id)
> +{
> + struct s3c24xx_uart_port *ourport = id;
> + struct uart_port *port = &ourport->port;
> + unsigned int pend = rd_regl(port, S3C2410_UTRSTAT);
> + irqreturn_t ret = IRQ_NONE;
Redundant. You may return directly.
> +
> + if (pend & (APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO)) {
> + wr_regl(port, S3C2410_UTRSTAT,
> + APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO);
> + ret = s3c24xx_serial_rx_irq(irq, id);
> + }
> + if (pend & APPLE_S5L_UTRSTAT_TXTHRESH) {
> + wr_regl(port, S3C2410_UTRSTAT, APPLE_S5L_UTRSTAT_TXTHRESH);
> + ret = s3c24xx_serial_tx_irq(irq, id);
> + }
No IO serialization?
> + return ret;
> +}
...
> +static void apple_s5l_serial_shutdown(struct uart_port *port)
> +{
> + struct s3c24xx_uart_port *ourport = to_ourport(port);
> +
Extra blank line (check your entire series for a such)
> + unsigned int ucon;
> + ourport->tx_in_progress = 0;
> +}
...
> + ourport->rx_enabled = 1;
> + ourport->tx_enabled = 0;
How are these protected against race?
...
> + case TYPE_APPLE_S5L: {
> + unsigned int ucon;
> + int ret;
> +
> + ret = clk_prepare_enable(ourport->clk);
> + if (ret) {
> + dev_err(dev, "clk_enable clk failed: %d\n", ret);
> + return ret;
> + }
> + if (!IS_ERR(ourport->baudclk)) {
> + ret = clk_prepare_enable(ourport->baudclk);
> + if (ret) {
> + dev_err(dev, "clk_enable baudclk failed: %d\n", ret);
> + clk_disable_unprepare(ourport->clk);
> + return ret;
> + }
> + }
Wouldn't it be better to use CLK bulk API?
> + ucon = rd_regl(port, S3C2410_UCON);
> +
> + ucon &= ~(APPLE_S5L_UCON_TXTHRESH_ENA_MSK |
> + APPLE_S5L_UCON_RXTHRESH_ENA_MSK |
> + APPLE_S5L_UCON_RXTO_ENA_MSK);
> +
> + if (ourport->tx_enabled)
> + ucon |= APPLE_S5L_UCON_TXTHRESH_ENA_MSK;
> + if (ourport->rx_enabled)
> + ucon |= APPLE_S5L_UCON_RXTHRESH_ENA_MSK |
> + APPLE_S5L_UCON_RXTO_ENA_MSK;
> +
> + wr_regl(port, S3C2410_UCON, ucon);
> +
> + if (!IS_ERR(ourport->baudclk))
> + clk_disable_unprepare(ourport->baudclk);
> + clk_disable_unprepare(ourport->clk);
> + break;
> + }
...
> +#ifdef CONFIG_ARCH_APPLE
Why? Wouldn't you like the one kernel to work on many SoCs?
> +static struct s3c24xx_serial_drv_data s5l_serial_drv_data = {
> + .info = &(struct s3c24xx_uart_info) {
> + .name = "Apple S5L UART",
> + .type = TYPE_APPLE_S5L,
> + .port_type = PORT_8250,
> + .fifosize = 16,
> + .rx_fifomask = S3C2410_UFSTAT_RXMASK,
> + .rx_fifoshift = S3C2410_UFSTAT_RXSHIFT,
> + .rx_fifofull = S3C2410_UFSTAT_RXFULL,
> + .tx_fifofull = S3C2410_UFSTAT_TXFULL,
> + .tx_fifomask = S3C2410_UFSTAT_TXMASK,
> + .tx_fifoshift = S3C2410_UFSTAT_TXSHIFT,
> + .def_clk_sel = S3C2410_UCON_CLKSEL0,
> + .num_clks = 1,
> + .clksel_mask = 0,
> + .clksel_shift = 0,
> + },
> + .def_cfg = &(struct s3c2410_uartcfg) {
> + .ucon = APPLE_S5L_UCON_DEFAULT,
> + .ufcon = S3C2410_UFCON_DEFAULT,
> + },
> +};
> +#define S5L_SERIAL_DRV_DATA ((kernel_ulong_t)&s5l_serial_drv_data)
> +#else
> +#define S5L_SERIAL_DRV_DATA ((kernel_ulong_t)NULL)
> +#endif
...
> +#define APPLE_S5L_UCON_RXTO_ENA_MSK (1 << APPLE_S5L_UCON_RXTO_ENA)
> +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK (1 << APPLE_S5L_UCON_RXTHRESH_ENA)
> +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK (1 << APPLE_S5L_UCON_TXTHRESH_ENA)
BIT() ?
...
> +#define APPLE_S5L_UCON_DEFAULT (S3C2410_UCON_TXIRQMODE | \
> + S3C2410_UCON_RXIRQMODE | \
> + S3C2410_UCON_RXFIFO_TOI)
Indentation level is too high. Hint: start a value of the definition
on the new line.
...
> +#define APPLE_S5L_UTRSTAT_RXTHRESH (1<<4)
> +#define APPLE_S5L_UTRSTAT_TXTHRESH (1<<5)
> +#define APPLE_S5L_UTRSTAT_RXTO (1<<9)
> +#define APPLE_S5L_UTRSTAT_ALL_FLAGS (0x3f0)
BIT() ?
--
With Best Regards,
Andy Shevchenko