Thread (49 messages) 49 messages, 6 authors, 2021-02-10

Re: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple UARTs

From: Krzysztof Kozlowski <krzk@kernel.org>
Date: 2021-02-08 11:04:54
Also in: linux-arm-kernel, lkml

On Fri, Feb 05, 2021 at 05:39:38AM +0900, Hector Martin wrote:
quoted hunk ↗ jump to hunk
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.

Signed-off-by: Hector Martin <redacted>
---
 drivers/tty/serial/samsung_tty.c | 297 +++++++++++++++++++++++++++----
 include/linux/serial_s3c.h       |  16 ++
 include/uapi/linux/serial_core.h |   3 +
 3 files changed, 280 insertions(+), 36 deletions(-)
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 8ae3e03fbd8c..6d812ba1b748 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -56,6 +56,9 @@
 /* flag to ignore all characters coming in */
 #define RXSTAT_DUMMY_READ (0x10000000)
 
+/* IRQ number used when the handler is called in non-IRQ context */
+#define NO_IRQ -1
+
 struct s3c24xx_uart_info {
 	char			*name;
 	unsigned int		type;
@@ -144,6 +147,14 @@ struct s3c24xx_uart_port {
 #endif
 };
 
+enum s3c24xx_irq_type {
+	IRQ_DISCRETE = 0,
+	IRQ_S3C6400 = 1,
+	IRQ_APPLE = 2,
It seems you add the third structure to differentiate type of UART.
There is already port type and s3c24xx_serial_drv_data, no need for
third structure (kind of similar to tries of Tamseel Shams in recent
patches). It's too much. Instead, differentiate by port type or prepare
own set of uart_ops if it's really different (like you did with startup
op).
quoted hunk ↗ jump to hunk
+};
+
+static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport);
+
 /* conversion functions */
 
 #define s3c24xx_dev_to_port(__dev) dev_get_drvdata(__dev)
@@ -231,11 +242,20 @@ static int s3c24xx_serial_txempty_nofifo(struct uart_port *port)
 /*
  * s3c64xx and later SoC's include the interrupt mask and status registers in
  * the controller itself, unlike the s3c24xx SoC's which have these registers
- * in the interrupt controller. Check if the port type is s3c64xx or higher.
+ * in the interrupt controller. Apple SoCs use a different flavor of mask
+ * and status registers. This function returns the IRQ style to use.
  */
-static int s3c24xx_serial_has_interrupt_mask(struct uart_port *port)
+static int s3c24xx_irq_type(struct uart_port *port)
 {
-	return to_ourport(port)->info->type == PORT_S3C6400;
+	switch (to_ourport(port)->info->type) {
+	case PORT_S3C6400:
+		return IRQ_S3C6400;
+	case PORT_APPLE:
+		return IRQ_APPLE;
+	default:
+		return IRQ_DISCRETE;
+	}
+
No empty lines at end of function.
quoted hunk ↗ jump to hunk
 }
 
 static void s3c24xx_serial_rx_enable(struct uart_port *port)
@@ -289,10 +309,17 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
 	if (!ourport->tx_enabled)
 		return;
 
-	if (s3c24xx_serial_has_interrupt_mask(port))
+	switch (s3c24xx_irq_type(port)) {
+	case IRQ_APPLE:
+		s3c24xx_clear_bit(port, APPLE_UCON_TXTHRESH_ENA, S3C2410_UCON);
+		break;
+	case IRQ_S3C6400:
 		s3c24xx_set_bit(port, S3C64XX_UINTM_TXD, S3C64XX_UINTM);
-	else
+		break;
+	default:
 		disable_irq_nosync(ourport->tx_irq);
+		break;
+	}
 
 	if (dma && dma->tx_chan && ourport->tx_in_progress == S3C24XX_TX_DMA) {
 		dmaengine_pause(dma->tx_chan);
@@ -315,8 +342,6 @@ static void s3c24xx_serial_stop_tx(struct uart_port *port)
 	ourport->tx_mode = 0;
 }
 
-static void s3c24xx_serial_start_next_tx(struct s3c24xx_uart_port *ourport);
-
 static void s3c24xx_serial_tx_dma_complete(void *args)
 {
 	struct s3c24xx_uart_port *ourport = args;
@@ -353,10 +378,17 @@ static void enable_tx_dma(struct s3c24xx_uart_port *ourport)
 	u32 ucon;
 
 	/* Mask Tx interrupt */
-	if (s3c24xx_serial_has_interrupt_mask(port))
+	switch (s3c24xx_irq_type(port)) {
+	case IRQ_APPLE:
+		WARN_ON(1); // No DMA
+		break;
+	case IRQ_S3C6400:
 		s3c24xx_set_bit(port, S3C64XX_UINTM_TXD, S3C64XX_UINTM);
-	else
+		break;
+	default:
 		disable_irq_nosync(ourport->tx_irq);
+		break;
+	}
 
 	/* Enable tx dma mode */
 	ucon = rd_regl(port, S3C2410_UCON);
@@ -369,6 +401,8 @@ static void enable_tx_dma(struct s3c24xx_uart_port *ourport)
 	ourport->tx_mode = S3C24XX_TX_DMA;
 }
 
+static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id);
+
 static void enable_tx_pio(struct s3c24xx_uart_port *ourport)
 {
 	struct uart_port *port = &ourport->port;
@@ -383,16 +417,30 @@ static void enable_tx_pio(struct s3c24xx_uart_port *ourport)
 	ucon = rd_regl(port, S3C2410_UCON);
 	ucon &= ~(S3C64XX_UCON_TXMODE_MASK);
 	ucon |= S3C64XX_UCON_TXMODE_CPU;
-	wr_regl(port,  S3C2410_UCON, ucon);
 
 	/* Unmask Tx interrupt */
-	if (s3c24xx_serial_has_interrupt_mask(port))
-		s3c24xx_clear_bit(port, S3C64XX_UINTM_TXD,
-				  S3C64XX_UINTM);
-	else
+	switch (s3c24xx_irq_type(port)) {
+	case IRQ_APPLE:
+		ucon |= APPLE_UCON_TXTHRESH_ENA_MSK;
+		break;
+	case IRQ_S3C6400:
+		s3c24xx_clear_bit(port, S3C64XX_UINTM_TXD, S3C64XX_UINTM);
+		break;
+	default:
 		enable_irq(ourport->tx_irq);
+		break;
+	}
+
+	wr_regl(port,  S3C2410_UCON, ucon);
 
 	ourport->tx_mode = S3C24XX_TX_PIO;
+
+	/*
+	 * The Apple version only has edge triggered TX IRQs, so we need
+	 * to kick off the process by sending some characters here.
+	 */
+	if (s3c24xx_irq_type(port) == IRQ_APPLE)
+		s3c24xx_serial_tx_chars(NO_IRQ, ourport);
 }
 
 static void s3c24xx_serial_start_tx_pio(struct s3c24xx_uart_port *ourport)
@@ -513,11 +561,18 @@ static void s3c24xx_serial_stop_rx(struct uart_port *port)
 
 	if (ourport->rx_enabled) {
 		dev_dbg(port->dev, "stopping rx\n");
-		if (s3c24xx_serial_has_interrupt_mask(port))
-			s3c24xx_set_bit(port, S3C64XX_UINTM_RXD,
-					S3C64XX_UINTM);
-		else
-			disable_irq_nosync(ourport->rx_irq);
+		switch (s3c24xx_irq_type(port)) {
+		case IRQ_APPLE:
+			s3c24xx_clear_bit(port, APPLE_UCON_RXTHRESH_ENA, S3C2410_UCON);
+			s3c24xx_clear_bit(port, APPLE_UCON_RXTO_ENA, S3C2410_UCON);
+			break;
+		case IRQ_S3C6400:
+			s3c24xx_set_bit(port, S3C64XX_UINTM_RXD, S3C64XX_UINTM);
+			break;
+		default:
+			disable_irq_nosync(ourport->tx_irq);
+			break;
+		}
 		ourport->rx_enabled = 0;
 	}
 	if (dma && dma->rx_chan) {
@@ -651,14 +706,18 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)
 
 	/* set Rx mode to DMA mode */
 	ucon = rd_regl(port, S3C2410_UCON);
-	ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK |
-			S3C64XX_UCON_EMPTYINT_EN |
-			S3C64XX_UCON_DMASUS_EN |
-			S3C64XX_UCON_TIMEOUT_EN |
-			S3C64XX_UCON_RXMODE_MASK);
-	ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT |
-			S3C64XX_UCON_TIMEOUT_EN |
-			S3C64XX_UCON_RXMODE_CPU;
+	ucon &= ~S3C64XX_UCON_RXMODE_MASK;
+	ucon |= S3C64XX_UCON_RXMODE_CPU;
+
+	/* Apple types use these bits for IRQ masks */
+	if (s3c24xx_irq_type(port) != IRQ_APPLE) {
+		ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK |
+				S3C64XX_UCON_EMPTYINT_EN |
+				S3C64XX_UCON_DMASUS_EN |
+				S3C64XX_UCON_TIMEOUT_EN);
+		ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT |
+				S3C64XX_UCON_TIMEOUT_EN;
+	}
 	wr_regl(port, S3C2410_UCON, ucon);
 
 	ourport->rx_mode = S3C24XX_RX_PIO;
@@ -831,7 +890,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 	unsigned long flags;
 	int count, dma_count = 0;
 
-	spin_lock_irqsave(&port->lock, flags);
+	/* Only lock if called from IRQ context */
+	if (irq != NO_IRQ)
+		spin_lock_irqsave(&port->lock, flags);
I would prefer to have two functions - unlocked (doing actual stuff) and
a locking wrapper. Something like is done for regulator_is_enabled().
However the s3c24xx_serial_tx_chars() also unlocks-locks inside, so it
might be not easy to split common part Anyway hacking interrupt handler
to NO_IRQ is confusing and not readable.
quoted hunk ↗ jump to hunk
 
 	count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
 
@@ -893,7 +954,8 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)
 		s3c24xx_serial_stop_tx(port);
 
 out:
-	spin_unlock_irqrestore(&port->lock, flags);
+	if (irq != NO_IRQ)
+		spin_unlock_irqrestore(&port->lock, flags);
 	return IRQ_HANDLED;
 }
 
@@ -916,6 +978,26 @@ static irqreturn_t s3c64xx_serial_handle_irq(int irq, void *id)
 	return ret;
 }
 
+/* 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_HANDLED;
+
+	if (pend & (APPLE_UTRSTAT_RXTHRESH | APPLE_UTRSTAT_RXTO)) {
+		wr_regl(port, S3C2410_UTRSTAT, APPLE_UTRSTAT_RXTHRESH | APPLE_UTRSTAT_RXTO);
+		ret = s3c24xx_serial_rx_chars(irq, id);
+	}
+	if (pend & APPLE_UTRSTAT_TXTHRESH) {
+		wr_regl(port, S3C2410_UTRSTAT, APPLE_UTRSTAT_TXTHRESH);
+		ret = s3c24xx_serial_tx_chars(irq, id);
+	}
+
+	return ret;
+}
+
 static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port)
 {
 	struct s3c24xx_uart_info *info = s3c24xx_port_to_info(port);
@@ -1098,7 +1180,7 @@ 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))
+		if (s3c24xx_irq_type(port) == IRQ_DISCRETE)
 			free_irq(ourport->tx_irq, ourport);
 		ourport->tx_enabled = 0;
 		ourport->tx_claimed = 0;
@@ -1106,18 +1188,34 @@ static void s3c24xx_serial_shutdown(struct uart_port *port)
 	}
 
 	if (ourport->rx_claimed) {
-		if (!s3c24xx_serial_has_interrupt_mask(port))
+		if (s3c24xx_irq_type(port) == IRQ_DISCRETE)
 			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)) {
+	switch (s3c24xx_irq_type(port)) {
+	case IRQ_APPLE: {
+		unsigned int ucon;
+
+		ucon = rd_regl(port, S3C2410_UCON);
+		ucon &= ~(APPLE_UCON_TXTHRESH_ENA_MSK |
+			APPLE_UCON_RXTHRESH_ENA_MSK |
+			APPLE_UCON_RXTO_ENA_MSK);
+		wr_regl(port, S3C2410_UCON, ucon);
+
+		wr_regl(port, S3C2410_UTRSTAT, APPLE_UTRSTAT_ALL_FLAGS);
+
+		free_irq(port->irq, ourport);
+		break;
+	}
+	case IRQ_S3C6400:
 		free_irq(port->irq, ourport);
 
 		wr_regl(port, S3C64XX_UINTP, 0xf);
 		wr_regl(port, S3C64XX_UINTM, 0xf);
+		break;
 	}
 
 	if (ourport->dma)
@@ -1215,6 +1313,47 @@ static int s3c64xx_serial_startup(struct uart_port *port)
 	return ret;
 }
 
+static int apple_serial_startup(struct uart_port *port)
+{
+	struct s3c24xx_uart_port *ourport = to_ourport(port);
+	unsigned long flags;
+	unsigned int ufcon;
+	int ret;
+
+	wr_regl(port, S3C2410_UTRSTAT, APPLE_UTRSTAT_ALL_FLAGS);
+
+	ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
+			  s3c24xx_serial_portname(port), ourport);
+	if (ret) {
+		dev_err(port->dev, "cannot get irq %d\n", port->irq);
+		return ret;
+	}
+
+	/* 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);
+
+	ufcon = rd_regl(port, S3C2410_UFCON);
+	ufcon |= S3C2410_UFCON_RESETRX | S5PV210_UFCON_RXTRIG8;
+	if (!uart_console(port))
+		ufcon |= S3C2410_UFCON_RESETTX;
+	wr_regl(port, S3C2410_UFCON, ufcon);
+
+	enable_rx_pio(ourport);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	/* Enable Rx Interrupt */
+	s3c24xx_set_bit(port, APPLE_UCON_RXTHRESH_ENA, S3C2410_UCON);
+	s3c24xx_set_bit(port, APPLE_UCON_RXTO_ENA, S3C2410_UCON);
+
+	return ret;
+}
+
 /* power power management control */
 
 static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
@@ -1544,6 +1683,8 @@ static const char *s3c24xx_serial_type(struct uart_port *port)
 		return "S3C2412";
 	case PORT_S3C6400:
 		return "S3C6400/10";
+	case PORT_APPLE:
+		return "APPLE";
"Apple S5L"?
quoted hunk ↗ jump to hunk
 	default:
 		return NULL;
 	}
@@ -1868,9 +2009,16 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
 	/* setup info for port */
 	port->dev	= &platdev->dev;
 
+	switch (s3c24xx_irq_type(port)) {
+	/* Startup sequence is different for Apple SoC's */
+	case IRQ_APPLE:
+		s3c24xx_serial_ops.startup = apple_serial_startup;
+		break;
 	/* Startup sequence is different for s3c64xx and higher SoC's */
-	if (s3c24xx_serial_has_interrupt_mask(port))
+	case IRQ_S3C6400:
 		s3c24xx_serial_ops.startup = s3c64xx_serial_startup;
Don't overwrite specific ops. It's difficult to see then which ops are
being used. Instead create a new set of uart_ops matching the needs.

Best regards,
Krzysztof
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help