On Thu, Mar 4, 2021 at 11:42 PM Hector Martin [off-list ref] 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