Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Mar 4, 2021 at 11:42 PM Hector Martin <marcan@marcan.st> 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