diff mbox series

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

Message ID 20210204203951.52105-6-marcan@marcan.st (mailing list archive)
State Changes Requested
Headers show
Series Apple M1 SoC platform bring-up | expand

Commit Message

Hector Martin Feb. 4, 2021, 8:39 p.m. UTC
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 <marcan@marcan.st>
---
 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(-)

Comments

kernel test robot Feb. 4, 2021, 11:55 p.m. UTC | #1
Hi Hector,

I love your patch! Perhaps something to improve:

[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on robh/for-next linus/master v5.11-rc6 next-20210125]
[cannot apply to tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hector-Martin/Apple-M1-SoC-platform-bring-up/20210205-045228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/c3bf64138f141e20577083d30e0542004c194a20
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hector-Martin/Apple-M1-SoC-platform-bring-up/20210205-045228
        git checkout c3bf64138f141e20577083d30e0542004c194a20
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/tty/serial/samsung_tty.c:60: warning: "NO_IRQ" redefined
      60 | #define NO_IRQ -1
         | 
   In file included from arch/arm/include/asm/hardirq.h:5,
                    from include/linux/hardirq.h:10,
                    from include/linux/interrupt.h:11,
                    from include/linux/serial_core.h:13,
                    from drivers/tty/serial/samsung_tty.c:36:
   arch/arm/include/asm/irq.h:22: note: this is the location of the previous definition
      22 | #define NO_IRQ ((unsigned int)(-1))
         | 
   drivers/tty/serial/samsung_tty.c: In function 's3c24xx_serial_resume_noirq':
   drivers/tty/serial/samsung_tty.c:2309:4: error: a label can only be part of a statement and a declaration is not a statement
    2309 |    unsigned int ucon;
         |    ^~~~~~~~
   drivers/tty/serial/samsung_tty.c:2334:4: error: a label can only be part of a statement and a declaration is not a statement
    2334 |    unsigned int uintm = 0xf;
         |    ^~~~~~~~


vim +/NO_IRQ +60 drivers/tty/serial/samsung_tty.c

    58	
    59	/* IRQ number used when the handler is called in non-IRQ context */
  > 60	#define NO_IRQ -1
    61	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Feb. 5, 2021, 2:19 a.m. UTC | #2
Hi Hector,

I love your patch! Yet something to improve:

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on robh/for-next linus/master v5.11-rc6 next-20210125]
[cannot apply to tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Hector-Martin/Apple-M1-SoC-platform-bring-up/20210205-045228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/c3bf64138f141e20577083d30e0542004c194a20
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Hector-Martin/Apple-M1-SoC-platform-bring-up/20210205-045228
        git checkout c3bf64138f141e20577083d30e0542004c194a20
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/tty/serial/samsung_tty.c:60: warning: "NO_IRQ" redefined
      60 | #define NO_IRQ -1
         | 
   In file included from arch/arm/include/asm/hardirq.h:5,
                    from include/linux/hardirq.h:10,
                    from include/linux/interrupt.h:11,
                    from include/linux/serial_core.h:13,
                    from drivers/tty/serial/samsung_tty.c:36:
   arch/arm/include/asm/irq.h:22: note: this is the location of the previous definition
      22 | #define NO_IRQ ((unsigned int)(-1))
         | 
   drivers/tty/serial/samsung_tty.c: In function 's3c24xx_serial_resume_noirq':
>> drivers/tty/serial/samsung_tty.c:2309:4: error: a label can only be part of a statement and a declaration is not a statement
    2309 |    unsigned int ucon;
         |    ^~~~~~~~
   drivers/tty/serial/samsung_tty.c:2334:4: error: a label can only be part of a statement and a declaration is not a statement
    2334 |    unsigned int uintm = 0xf;
         |    ^~~~~~~~


vim +2309 drivers/tty/serial/samsung_tty.c

  2299	
  2300	static int s3c24xx_serial_resume_noirq(struct device *dev)
  2301	{
  2302		struct uart_port *port = s3c24xx_dev_to_port(dev);
  2303		struct s3c24xx_uart_port *ourport = to_ourport(port);
  2304	
  2305		if (port) {
  2306			/* restore IRQ mask */
  2307			switch (s3c24xx_irq_type(port)) {
  2308			case IRQ_APPLE:
> 2309				unsigned int ucon;
  2310	
  2311				clk_prepare_enable(ourport->clk);
  2312				if (!IS_ERR(ourport->baudclk))
  2313					clk_prepare_enable(ourport->baudclk);
  2314	
  2315				ucon = rd_regl(port, S3C2410_UCON);
  2316	
  2317				ucon &= ~(APPLE_UCON_TXTHRESH_ENA_MSK |
  2318					APPLE_UCON_RXTHRESH_ENA_MSK |
  2319					APPLE_UCON_RXTO_ENA_MSK);
  2320	
  2321				if (ourport->tx_enabled)
  2322					ucon |= APPLE_UCON_TXTHRESH_ENA_MSK;
  2323				if (ourport->rx_enabled)
  2324					ucon |= APPLE_UCON_RXTHRESH_ENA_MSK |
  2325						APPLE_UCON_RXTO_ENA_MSK;
  2326	
  2327				wr_regl(port, S3C2410_UCON, ucon);
  2328	
  2329				if (!IS_ERR(ourport->baudclk))
  2330					clk_disable_unprepare(ourport->baudclk);
  2331				clk_disable_unprepare(ourport->clk);
  2332				break;
  2333			case IRQ_S3C6400:
  2334				unsigned int uintm = 0xf;
  2335	
  2336				if (ourport->tx_enabled)
  2337					uintm &= ~S3C64XX_UINTM_TXD_MSK;
  2338				if (ourport->rx_enabled)
  2339					uintm &= ~S3C64XX_UINTM_RXD_MSK;
  2340				clk_prepare_enable(ourport->clk);
  2341				if (!IS_ERR(ourport->baudclk))
  2342					clk_prepare_enable(ourport->baudclk);
  2343				wr_regl(port, S3C64XX_UINTM, uintm);
  2344				if (!IS_ERR(ourport->baudclk))
  2345					clk_disable_unprepare(ourport->baudclk);
  2346				clk_disable_unprepare(ourport->clk);
  2347				break;
  2348			}
  2349		}
  2350	
  2351		return 0;
  2352	}
  2353	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hector Martin Feb. 5, 2021, 9:44 a.m. UTC | #3
On 05/02/2021 08.55, kernel test robot wrote:
>>> drivers/tty/serial/samsung_tty.c:60: warning: "NO_IRQ" redefined
>        60 | #define NO_IRQ -1

Turns out arm (32) defines NO_IRQ. Replaced with NOT_IN_IRQ for v2.
Marc Zyngier Feb. 6, 2021, 1:15 p.m. UTC | #4
Hi Hector,

On Thu, 04 Feb 2021 20:39:38 +0000,
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.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  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,
> +};
> +
> +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;
> +	}
> +

nit: For ease of reviewing, it'd be good if you could split this patch
with introducing the S3C6400 and "discrete" support initially, and
only then add the new stuff.

>  }
>  
>  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);

Instead of directly calling into the handler (which has its own
problems, see below), could you just tickle the interrupt status
register to make an interrupt pending and trigger an actual interrupt?
I have no idea whether the HW supports this kind of trick though.

>  }
>  
>  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);

Isn't that actually dangerous? What prevents the interrupt from firing
right in the middle of this sequence and create havoc when called from
enable_tx_pio()? I fail to see what you gain with sidestepping the
locking.

>  
>  	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;

The default should be IRQ_NONE, otherwise the kernel cannot detect a
screaming spurious interrupt.

> +
> +	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);

Why IRQF_SHARED? Do you expect any other device sharing the same line
with this UART?

> +	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";
>  	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;
> +		break;
> +	}
>  
>  	port->uartclk = 1;
>  
> @@ -1905,7 +2053,7 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
>  		ourport->tx_irq = ret + 1;
>  	}
>  
> -	if (!s3c24xx_serial_has_interrupt_mask(port)) {
> +	if (s3c24xx_irq_type(port) == IRQ_DISCRETE) {
>  		ret = platform_get_irq(platdev, 1);
>  		if (ret > 0)
>  			ourport->tx_irq = ret;
> @@ -1945,10 +2093,24 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
>  		pr_warn("uart: failed to enable baudclk\n");
>  
>  	/* Keep all interrupts masked and cleared */
> -	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);
> +		break;
> +	}
> +	case IRQ_S3C6400:
>  		wr_regl(port, S3C64XX_UINTM, 0xf);
>  		wr_regl(port, S3C64XX_UINTP, 0xf);
>  		wr_regl(port, S3C64XX_UINTSP, 0xf);
> +		break;
>  	}
>  
>  	dev_dbg(port->dev, "port: map=%pa, mem=%p, irq=%d (%d,%d), clock=%u\n",
> @@ -2142,7 +2304,33 @@ static int s3c24xx_serial_resume_noirq(struct device *dev)
>  
>  	if (port) {
>  		/* restore IRQ mask */
> -		if (s3c24xx_serial_has_interrupt_mask(port)) {
> +		switch (s3c24xx_irq_type(port)) {
> +		case IRQ_APPLE:
> +			unsigned int ucon;
> +
> +			clk_prepare_enable(ourport->clk);
> +			if (!IS_ERR(ourport->baudclk))
> +				clk_prepare_enable(ourport->baudclk);
> +
> +			ucon = rd_regl(port, S3C2410_UCON);
> +
> +			ucon &= ~(APPLE_UCON_TXTHRESH_ENA_MSK |
> +				APPLE_UCON_RXTHRESH_ENA_MSK |
> +				APPLE_UCON_RXTO_ENA_MSK);
> +
> +			if (ourport->tx_enabled)
> +				ucon |= APPLE_UCON_TXTHRESH_ENA_MSK;
> +			if (ourport->rx_enabled)
> +				ucon |= APPLE_UCON_RXTHRESH_ENA_MSK |
> +					APPLE_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;
> +		case IRQ_S3C6400:
>  			unsigned int uintm = 0xf;
>  
>  			if (ourport->tx_enabled)
> @@ -2156,6 +2344,7 @@ static int s3c24xx_serial_resume_noirq(struct device *dev)
>  			if (!IS_ERR(ourport->baudclk))
>  				clk_disable_unprepare(ourport->baudclk);
>  			clk_disable_unprepare(ourport->clk);
> +			break;
>  		}
>  	}
>  
> @@ -2556,6 +2745,34 @@ static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = {
>  #define EXYNOS5433_SERIAL_DRV_DATA (kernel_ulong_t)NULL
>  #endif
>  
> +#if defined(CONFIG_ARCH_APPLE)
> +static struct s3c24xx_serial_drv_data s5l_serial_drv_data = {
> +	.info = &(struct s3c24xx_uart_info) {
> +		.name		= "Apple S5L UART",
> +		.type		= PORT_APPLE,
> +		.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_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
> +
> +
>  static const struct platform_device_id s3c24xx_serial_driver_ids[] = {
>  	{
>  		.name		= "s3c2410-uart",
> @@ -2578,6 +2795,9 @@ static const struct platform_device_id s3c24xx_serial_driver_ids[] = {
>  	}, {
>  		.name		= "exynos5433-uart",
>  		.driver_data	= EXYNOS5433_SERIAL_DRV_DATA,
> +	}, {
> +		.name		= "s5l-uart",
> +		.driver_data	= S5L_SERIAL_DRV_DATA,
>  	},
>  	{ },
>  };
> @@ -2599,6 +2819,8 @@ static const struct of_device_id s3c24xx_uart_dt_match[] = {
>  		.data = (void *)EXYNOS4210_SERIAL_DRV_DATA },
>  	{ .compatible = "samsung,exynos5433-uart",
>  		.data = (void *)EXYNOS5433_SERIAL_DRV_DATA },
> +	{ .compatible = "AAPL,s5l-uart",
> +		.data = (void *)S5L_SERIAL_DRV_DATA },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
> @@ -2694,6 +2916,9 @@ static int __init s3c2410_early_console_setup(struct earlycon_device *device,
>  
>  OF_EARLYCON_DECLARE(s3c2410, "samsung,s3c2410-uart",
>  			s3c2410_early_console_setup);
> +/* Apple SoCs are close enough to s3c2410 for earlycon */
> +OF_EARLYCON_DECLARE(s5l, "AAPL,s5l-uart",
> +			s3c2410_early_console_setup);
>  
>  /* S3C2412, S3C2440, S3C64xx */
>  static struct samsung_early_console_data s3c2440_early_console_data = {
> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
> index ca2c5393dc6b..377c8c6e5b97 100644
> --- a/include/linux/serial_s3c.h
> +++ b/include/linux/serial_s3c.h
> @@ -246,6 +246,22 @@
>  				 S5PV210_UFCON_TXTRIG4 |	\
>  				 S5PV210_UFCON_RXTRIG4)
>  
> +
> +#define APPLE_UCON_RXTO_ENA	9
> +#define APPLE_UCON_RXTHRESH_ENA	12
> +#define APPLE_UCON_TXTHRESH_ENA	13
> +#define APPLE_UCON_RXTO_ENA_MSK		(1 << APPLE_UCON_RXTO_ENA)
> +#define APPLE_UCON_RXTHRESH_ENA_MSK	(1 << APPLE_UCON_RXTHRESH_ENA)
> +#define APPLE_UCON_TXTHRESH_ENA_MSK	(1 << APPLE_UCON_TXTHRESH_ENA)
> +
> +#define APPLE_UCON_DEFAULT	  S3C2410_UCON_RXFIFO_TOI
> +
> +#define APPLE_UTRSTAT_RXTHRESH	(1<<4)
> +#define APPLE_UTRSTAT_TXTHRESH	(1<<5)
> +#define APPLE_UTRSTAT_RXTO	(1<<9)
> +#define APPLE_UTRSTAT_ALL_FLAGS	(0x3f0)
> +
> +
>  #ifndef __ASSEMBLY__
>  
>  #include <linux/serial_core.h>
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 62c22045fe65..59d102b674db 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -277,4 +277,7 @@
>  /* Freescale LINFlexD UART */
>  #define PORT_LINFLEXUART	122
>  
> +/* Apple Silicon (M1/T8103) UART (Samsung variant) */
> +#define PORT_APPLE	123
> +

Do you actually need a new port type here? Looking at the driver
itself, it is mainly used to work out the IRQ model. Maybe introducing
a new irq_type field in the port structure would be better than
exposing this to userspace (which should see something that is exactly
the same as a S3C UART).

Thanks,

	M.
Hector Martin Feb. 7, 2021, 9:12 a.m. UTC | #5
On 06/02/2021 22.15, Marc Zyngier wrote:
>> -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;
>> +	}
>> +
> 
> nit: For ease of reviewing, it'd be good if you could split this patch
> with introducing the S3C6400 and "discrete" support initially, and
> only then add the new stuff.

Good idea, will do for v2.

>> +	if (s3c24xx_irq_type(port) == IRQ_APPLE)
>> +		s3c24xx_serial_tx_chars(NO_IRQ, ourport);
> 
> Instead of directly calling into the handler (which has its own
> problems, see below), could you just tickle the interrupt status
> register to make an interrupt pending and trigger an actual interrupt?
> I have no idea whether the HW supports this kind of trick though.

I thought of that, but I tried really hard to find such a feature with 
no success. The best I can do is unmask and trigger the *RX* timeout 
interrupt which will eventually fire but... this doesn't work so well in 
practice. There is no way to trigger IRQ flags directly (as those bits 
are write-1-to-clear).

>> -	spin_lock_irqsave(&port->lock, flags);
>> +	/* Only lock if called from IRQ context */
>> +	if (irq != NO_IRQ)
>> +		spin_lock_irqsave(&port->lock, flags);
> 
> Isn't that actually dangerous? What prevents the interrupt from firing
> right in the middle of this sequence and create havoc when called from
> enable_tx_pio()? I fail to see what you gain with sidestepping the
> locking.

The callpath here is:

uart_start -> __uart_start -> (uart_ops.start_tx) 
s3c24xx_serial_start_tx -> s3c24xx_serial_start_tx_pio -> enable_tx_pio 
-> s3c24xx_serial_tx_chars

And uart_start takes the uart_port lock. None of the serial functions 
take the lock because the serial core already does, but obviously the 
IRQ handler needs to, *if* it's called as an IRQ handler only.

> The default should be IRQ_NONE, otherwise the kernel cannot detect a
> screaming spurious interrupt.

Good point, and this needs fixing in s3c64xx_serial_handle_irq too then 
(which is what I based mine off of).

>> +	ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
>> +			  s3c24xx_serial_portname(port), ourport);
> 
> Why IRQF_SHARED? Do you expect any other device sharing the same line
> with this UART?

This also came from s3c64xx_serial_startup and... now I wonder why that 
one needs it. Maybe on some SoCs it does get shared? Certainly not for 
discrete rx/tx irq chips (and indeed those don't set the flag)...

CCing Thomas, who added the S3C64xx support (and should probably review 
this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the 
build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.

Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED 
for v2.

>> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
>> index 62c22045fe65..59d102b674db 100644
>> --- a/include/uapi/linux/serial_core.h
>> +++ b/include/uapi/linux/serial_core.h
>> @@ -277,4 +277,7 @@
>>   /* Freescale LINFlexD UART */
>>   #define PORT_LINFLEXUART	122
>>   
>> +/* Apple Silicon (M1/T8103) UART (Samsung variant) */
>> +#define PORT_APPLE	123
>> +
> 
> Do you actually need a new port type here? Looking at the driver
> itself, it is mainly used to work out the IRQ model. Maybe introducing
> a new irq_type field in the port structure would be better than
> exposing this to userspace (which should see something that is exactly
> the same as a S3C UART).

Well... every S3C variant already has its own port type here.

#define PORT_S3C2410    55
#define PORT_S3C2440    61
#define PORT_S3C2400    67
#define PORT_S3C2412    73
#define PORT_S3C6400    84

If we don't introduce a new one, which one should we pretend to be? :)

I agree that it might make sense to merge all of these into one, though; 
I don't know what the original reason for splitting them out is. But now 
that they're part of the userspace API, this might not be a good idea. 
Though, unsurprisingly, some googling suggests there are zero users of 
these defines in userspace.
Hector Martin Feb. 7, 2021, 9:26 a.m. UTC | #6
On 07/02/2021 18.12, Hector Martin 'marcan' wrote:
> On 06/02/2021 22.15, Marc Zyngier wrote:
>> The default should be IRQ_NONE, otherwise the kernel cannot detect a
>> screaming spurious interrupt.
> 
> Good point, and this needs fixing in s3c64xx_serial_handle_irq too then
> (which is what I based mine off of).
> 
>>> +	ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
>>> +			  s3c24xx_serial_portname(port), ourport);
>>
>> Why IRQF_SHARED? Do you expect any other device sharing the same line
>> with this UART?
> 
> This also came from s3c64xx_serial_startup and... now I wonder why that
> one needs it. Maybe on some SoCs it does get shared? Certainly not for
> discrete rx/tx irq chips (and indeed those don't set the flag)...
> 
> CCing Thomas, who added the S3C64xx support (and should probably review
> this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the
> build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.

Seems Thomas does not work for Linaro any more :)

CCing Krzysztof instead, who is the Samsung arch maintainer.

> 
> Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED
> for v2.
Krzysztof Kozlowski Feb. 8, 2021, 9:36 a.m. UTC | #7
On Sun, Feb 07, 2021 at 06:26:43PM +0900, Hector Martin 'marcan' wrote:
> On 07/02/2021 18.12, Hector Martin 'marcan' wrote:
> > On 06/02/2021 22.15, Marc Zyngier wrote:
> > > The default should be IRQ_NONE, otherwise the kernel cannot detect a
> > > screaming spurious interrupt.
> > 
> > Good point, and this needs fixing in s3c64xx_serial_handle_irq too then
> > (which is what I based mine off of).
> > 
> > > > +	ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
> > > > +			  s3c24xx_serial_portname(port), ourport);
> > > 
> > > Why IRQF_SHARED? Do you expect any other device sharing the same line
> > > with this UART?
> > 
> > This also came from s3c64xx_serial_startup and... now I wonder why that
> > one needs it. Maybe on some SoCs it does get shared? Certainly not for
> > discrete rx/tx irq chips (and indeed those don't set the flag)...
> > 
> > CCing Thomas, who added the S3C64xx support (and should probably review
> > this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the
> > build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.
> 
> Seems Thomas does not work for Linaro any more :)
> 
> CCing Krzysztof instead, who is the Samsung arch maintainer.

Please use the scripts/get_maintainers.pl to get the list of people to
Cc. The script would point necessary folks.

A different issue is that all your emails from this thread were marked
by Google as spam.  I don't see any particular warning signs in the
header so it looks more of content-based match for spam.

> > Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED
> > for v2.

Please send a v2 after fixing issues pointed out by kbuild.

Best regards,
Krzysztof
Marc Zyngier Feb. 8, 2021, 10:34 a.m. UTC | #8
On 2021-02-07 09:12, Hector Martin 'marcan' wrote:
> On 06/02/2021 22.15, Marc Zyngier wrote:

[...]

>>> -	spin_lock_irqsave(&port->lock, flags);
>>> +	/* Only lock if called from IRQ context */
>>> +	if (irq != NO_IRQ)
>>> +		spin_lock_irqsave(&port->lock, flags);
>> 
>> Isn't that actually dangerous? What prevents the interrupt from firing
>> right in the middle of this sequence and create havoc when called from
>> enable_tx_pio()? I fail to see what you gain with sidestepping the
>> locking.
> 
> The callpath here is:
> 
> uart_start -> __uart_start -> (uart_ops.start_tx)
> s3c24xx_serial_start_tx -> s3c24xx_serial_start_tx_pio ->
> enable_tx_pio -> s3c24xx_serial_tx_chars
> 
> And uart_start takes the uart_port lock. None of the serial functions
> take the lock because the serial core already does, but obviously the
> IRQ handler needs to, *if* it's called as an IRQ handler only.

Right, that's the part I missed, thanks for pointing this out.
It'd probably be cleaner to move the whole s3c24xx_serial_tx_chars()
into a helper and keep the locking in the interrupt handler proper.

[...]

>>> diff --git a/include/uapi/linux/serial_core.h 
>>> b/include/uapi/linux/serial_core.h
>>> index 62c22045fe65..59d102b674db 100644
>>> --- a/include/uapi/linux/serial_core.h
>>> +++ b/include/uapi/linux/serial_core.h
>>> @@ -277,4 +277,7 @@
>>>   /* Freescale LINFlexD UART */
>>>   #define PORT_LINFLEXUART	122
>>>   +/* Apple Silicon (M1/T8103) UART (Samsung variant) */
>>> +#define PORT_APPLE	123
>>> +
>> 
>> Do you actually need a new port type here? Looking at the driver
>> itself, it is mainly used to work out the IRQ model. Maybe introducing
>> a new irq_type field in the port structure would be better than
>> exposing this to userspace (which should see something that is exactly
>> the same as a S3C UART).
> 
> Well... every S3C variant already has its own port type here.
> 
> #define PORT_S3C2410    55
> #define PORT_S3C2440    61
> #define PORT_S3C2400    67
> #define PORT_S3C2412    73
> #define PORT_S3C6400    84
> 
> If we don't introduce a new one, which one should we pretend to be? :)

Pick one! :D

> I agree that it might make sense to merge all of these into one,
> though; I don't know what the original reason for splitting them out
> is. But now that they're part of the userspace API, this might not be
> a good idea. Though, unsurprisingly, some googling suggests there are
> zero users of these defines in userspace.

I don't think we can do that, but I don't think we should keep adding
to this unless there is a very good reason. Greg would know, I expect.

Thanks,

          M.
Krzysztof Kozlowski Feb. 8, 2021, 10:54 a.m. UTC | #9
On Fri, Feb 05, 2021 at 05:39:38AM +0900, Hector Martin 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.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  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).

> +};
> +
> +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.

>  }
>  
>  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.

>  
>  	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"?

>  	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
Hector Martin Feb. 8, 2021, 4:10 p.m. UTC | #10
On 08/02/2021 19.54, Krzysztof Kozlowski wrote:
>> +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).

This ties into little changes in a bunch of places, so separate uart_ops 
  callbacks for every one of those would end up duplicating a lot of code :(

That list is just used to map the port type to something that only 
represents the type of IRQs, but its only real purpose for the 
indirection is so I can do "== IRQ_DISCRETE" in some codepaths to mean 
"not apple or S3C6400".

e.g.

     if (s3c24xx_irq_type(port) == IRQ_DISCRETE)
         free_irq(ourport->rx_irq, ourport);

Would have to become

     if (port->type != IRQ_S3C6400 && port->type != IRQ_APPLE)
         free_irq(ourport->rx_irq, ourport);

or

     switch (port->type) {
     case IRQ_S3C6400:
     case IRQ_APPLE:
         break;
     default:
         free_irq(ourport->rx_irq, ourport);
     }

Which one do you prefer?

Aside: Marc didn't like introducing new port types into uapi, but if we 
don't do that then we need a "real" port type in drv_data that isn't the 
uapi-exposed port or something along those lines, with a separate enum 
containing all the true port type values for that.

>>   	/* 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.

s3c24xx_serial_ops was already doing that here, but I can move that to a 
a separate uart_ops too.

Ack on all the other comments, I'll make the changes for v2.
Hector Martin Feb. 8, 2021, 4:14 p.m. UTC | #11
On 08/02/2021 18.36, Krzysztof Kozlowski wrote:
> Please use the scripts/get_maintainers.pl to get the list of people to
> Cc. The script would point necessary folks.

I thought I'd try Thomas first since he introduced the IRQF_SHARED 
specifically, but I should've gone straight to maintainers (I did use 
get_maintainers.pl to find you).

> A different issue is that all your emails from this thread were marked
> by Google as spam.  I don't see any particular warning signs in the
> header so it looks more of content-based match for spam.

That's weird. Maybe it's because my server doesn't do DKIM yet, and they 
went through a mailing list? It's probably time to set that up...

>>> Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED
>>> for v2.
> 
> Please send a v2 after fixing issues pointed out by kbuild.

Will do, already have those fixed in my WIP tree.
Hector Martin Feb. 8, 2021, 4:18 p.m. UTC | #12
On 08/02/2021 19.34, Marc Zyngier wrote:
> On 2021-02-07 09:12, Hector Martin 'marcan' wrote:
>> On 06/02/2021 22.15, Marc Zyngier wrote:
>>> Do you actually need a new port type here? Looking at the driver
>>> itself, it is mainly used to work out the IRQ model. Maybe introducing
>>> a new irq_type field in the port structure would be better than
>>> exposing this to userspace (which should see something that is exactly
>>> the same as a S3C UART).
>>
>> Well... every S3C variant already has its own port type here.
>>
>> #define PORT_S3C2410    55
>> #define PORT_S3C2440    61
>> #define PORT_S3C2400    67
>> #define PORT_S3C2412    73
>> #define PORT_S3C6400    84
>>
>> If we don't introduce a new one, which one should we pretend to be? :)
> 
> Pick one! :D

*queries /dev/urandom* :-)

>> I agree that it might make sense to merge all of these into one,
>> though; I don't know what the original reason for splitting them out
>> is. But now that they're part of the userspace API, this might not be
>> a good idea. Though, unsurprisingly, some googling suggests there are
>> zero users of these defines in userspace.
> 
> I don't think we can do that, but I don't think we should keep adding
> to this unless there is a very good reason. Greg would know, I expect.

Greg, what do you think? Add more PORT_ UART types for Samsung variants, 
or overload one of the existing ones and deal with it in the driver?
Greg KH Feb. 8, 2021, 4:46 p.m. UTC | #13
On Tue, Feb 09, 2021 at 01:18:21AM +0900, Hector Martin wrote:
> On 08/02/2021 19.34, Marc Zyngier wrote:
> > On 2021-02-07 09:12, Hector Martin 'marcan' wrote:
> > > On 06/02/2021 22.15, Marc Zyngier wrote:
> > > > Do you actually need a new port type here? Looking at the driver
> > > > itself, it is mainly used to work out the IRQ model. Maybe introducing
> > > > a new irq_type field in the port structure would be better than
> > > > exposing this to userspace (which should see something that is exactly
> > > > the same as a S3C UART).
> > > 
> > > Well... every S3C variant already has its own port type here.
> > > 
> > > #define PORT_S3C2410    55
> > > #define PORT_S3C2440    61
> > > #define PORT_S3C2400    67
> > > #define PORT_S3C2412    73
> > > #define PORT_S3C6400    84
> > > 
> > > If we don't introduce a new one, which one should we pretend to be? :)
> > 
> > Pick one! :D
> 
> *queries /dev/urandom* :-)
> 
> > > I agree that it might make sense to merge all of these into one,
> > > though; I don't know what the original reason for splitting them out
> > > is. But now that they're part of the userspace API, this might not be
> > > a good idea. Though, unsurprisingly, some googling suggests there are
> > > zero users of these defines in userspace.
> > 
> > I don't think we can do that, but I don't think we should keep adding
> > to this unless there is a very good reason. Greg would know, I expect.
> 
> Greg, what do you think? Add more PORT_ UART types for Samsung variants, or
> overload one of the existing ones and deal with it in the driver?

I HATE adding new PORT_ types, as I am almost positive no one uses them,
but as they are in the uapi files, we can't delete them.

So, just use an existing one, why do you want a new one?  If you don't
have a userspace tool that requires it, don't bother.

Just use PORT_8250 and be done with it.  I should force all new drivers
to use that as well :)

thanks,

greg k-h
Krzysztof Kozlowski Feb. 8, 2021, 6:37 p.m. UTC | #14
On Tue, Feb 09, 2021 at 01:10:02AM +0900, Hector Martin wrote:
> On 08/02/2021 19.54, Krzysztof Kozlowski wrote:
> > > +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).
> 
> This ties into little changes in a bunch of places, so separate uart_ops
> callbacks for every one of those would end up duplicating a lot of code :(
> 
> That list is just used to map the port type to something that only
> represents the type of IRQs, but its only real purpose for the indirection
> is so I can do "== IRQ_DISCRETE" in some codepaths to mean "not apple or
> S3C6400".
> 
> e.g.
> 
>     if (s3c24xx_irq_type(port) == IRQ_DISCRETE)
>         free_irq(ourport->rx_irq, ourport);
> 
> Would have to become
> 
>     if (port->type != IRQ_S3C6400 && port->type != IRQ_APPLE)
>         free_irq(ourport->rx_irq, ourport);
> 
> or
> 
>     switch (port->type) {
>     case IRQ_S3C6400:
>     case IRQ_APPLE:
>         break;
>     default:
>         free_irq(ourport->rx_irq, ourport);
>     }
> 
> Which one do you prefer?

The latter, especially that switches will appear quite a lot in such
case.

However this pattern (== IRQ_DISCRETE) appears only in three places so
it should not be the main case considered here.

However I saw later you actually replaced the
s3c24xx_serial_has_interrupt_mask(), so it's not that bad.

In most of your code, there will be actually a switch with all three
cases.

> 
> Aside: Marc didn't like introducing new port types into uapi, but if we
> don't do that then we need a "real" port type in drv_data that isn't the
> uapi-exposed port or something along those lines, with a separate enum
> containing all the true port type values for that.

Looking at Greg's comment, we can get rid of the PORT_ stuff entirely.
First of all, PORT_S3C2410 == PORT_S3C2412, so this define is not
accurate.

This leaves us with three types (s3c2400, s3c2440, s3c6410 and Apple).
The s3c2440 could be removed with adding a new "ucon_mask" field to
s3c24xx_serial_drv_data.

This would end with s3c24xx, s3c6410 and Apple - quite nice choice.

> 
> > >   	/* 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.
> 
> s3c24xx_serial_ops was already doing that here, but I can move that to a a
> separate uart_ops too.

You're right. This one would have to be improved before your change.
Instead of replacing specific op calls in startup, I think it's better
to have entirely separate ops instance for each variant:

static const struct uart_ops s3c24xx_serial_ops;
static const struct uart_ops s3c64xx_serial_ops;
static const struct uart_ops s5l_serial_ops;

This allows to add a "const", since uart_port takes such const pointer.

Still during s3c24xx_serial_probe() correct ops would have to be
assigned, but at least all ops are easily visible.

Best regards,
Krzysztof
Hector Martin Feb. 8, 2021, 11:22 p.m. UTC | #15
On 09/02/2021 01.46, Greg Kroah-Hartman wrote:
> I HATE adding new PORT_ types, as I am almost positive no one uses them,
> but as they are in the uapi files, we can't delete them.
> 
> So, just use an existing one, why do you want a new one?  If you don't
> have a userspace tool that requires it, don't bother.
> 
> Just use PORT_8250 and be done with it.  I should force all new drivers
> to use that as well :)

Krzysztof: given this, I think it would be fair to add an enum to the 
driver to keep track of the actual hardware type (grouped into probably 
just 3, S3C24XX, S3C6400, APPLE), get rid of any code in there that 
cares about the uapi-visible port type (other than setting it correctly 
for those that do exist, to maintain current behavior), and just make 
everything else use PORT_8250 for that?
Hector Martin Feb. 8, 2021, 11:23 p.m. UTC | #16
On 09/02/2021 03.37, Krzysztof Kozlowski wrote:
> Looking at Greg's comment, we can get rid of the PORT_ stuff entirely.
> First of all, PORT_S3C2410 == PORT_S3C2412, so this define is not
> accurate.
> 
> This leaves us with three types (s3c2400, s3c2440, s3c6410 and Apple).
> The s3c2440 could be removed with adding a new "ucon_mask" field to
> s3c24xx_serial_drv_data.
> 
> This would end with s3c24xx, s3c6410 and Apple - quite nice choice.

Works for me :)

> You're right. This one would have to be improved before your change.
> Instead of replacing specific op calls in startup, I think it's better
> to have entirely separate ops instance for each variant:
> 
> static const struct uart_ops s3c24xx_serial_ops;
> static const struct uart_ops s3c64xx_serial_ops;
> static const struct uart_ops s5l_serial_ops;
> 
> This allows to add a "const", since uart_port takes such const pointer.
> 
> Still during s3c24xx_serial_probe() correct ops would have to be
> assigned, but at least all ops are easily visible.

Roger, will do this for v2.
diff mbox series

Patch

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,
+};
+
+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;
+	}
+
 }
 
 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);
 
 	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";
 	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;
+		break;
+	}
 
 	port->uartclk = 1;
 
@@ -1905,7 +2053,7 @@  static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
 		ourport->tx_irq = ret + 1;
 	}
 
-	if (!s3c24xx_serial_has_interrupt_mask(port)) {
+	if (s3c24xx_irq_type(port) == IRQ_DISCRETE) {
 		ret = platform_get_irq(platdev, 1);
 		if (ret > 0)
 			ourport->tx_irq = ret;
@@ -1945,10 +2093,24 @@  static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
 		pr_warn("uart: failed to enable baudclk\n");
 
 	/* Keep all interrupts masked and cleared */
-	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);
+		break;
+	}
+	case IRQ_S3C6400:
 		wr_regl(port, S3C64XX_UINTM, 0xf);
 		wr_regl(port, S3C64XX_UINTP, 0xf);
 		wr_regl(port, S3C64XX_UINTSP, 0xf);
+		break;
 	}
 
 	dev_dbg(port->dev, "port: map=%pa, mem=%p, irq=%d (%d,%d), clock=%u\n",
@@ -2142,7 +2304,33 @@  static int s3c24xx_serial_resume_noirq(struct device *dev)
 
 	if (port) {
 		/* restore IRQ mask */
-		if (s3c24xx_serial_has_interrupt_mask(port)) {
+		switch (s3c24xx_irq_type(port)) {
+		case IRQ_APPLE:
+			unsigned int ucon;
+
+			clk_prepare_enable(ourport->clk);
+			if (!IS_ERR(ourport->baudclk))
+				clk_prepare_enable(ourport->baudclk);
+
+			ucon = rd_regl(port, S3C2410_UCON);
+
+			ucon &= ~(APPLE_UCON_TXTHRESH_ENA_MSK |
+				APPLE_UCON_RXTHRESH_ENA_MSK |
+				APPLE_UCON_RXTO_ENA_MSK);
+
+			if (ourport->tx_enabled)
+				ucon |= APPLE_UCON_TXTHRESH_ENA_MSK;
+			if (ourport->rx_enabled)
+				ucon |= APPLE_UCON_RXTHRESH_ENA_MSK |
+					APPLE_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;
+		case IRQ_S3C6400:
 			unsigned int uintm = 0xf;
 
 			if (ourport->tx_enabled)
@@ -2156,6 +2344,7 @@  static int s3c24xx_serial_resume_noirq(struct device *dev)
 			if (!IS_ERR(ourport->baudclk))
 				clk_disable_unprepare(ourport->baudclk);
 			clk_disable_unprepare(ourport->clk);
+			break;
 		}
 	}
 
@@ -2556,6 +2745,34 @@  static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = {
 #define EXYNOS5433_SERIAL_DRV_DATA (kernel_ulong_t)NULL
 #endif
 
+#if defined(CONFIG_ARCH_APPLE)
+static struct s3c24xx_serial_drv_data s5l_serial_drv_data = {
+	.info = &(struct s3c24xx_uart_info) {
+		.name		= "Apple S5L UART",
+		.type		= PORT_APPLE,
+		.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_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
+
+
 static const struct platform_device_id s3c24xx_serial_driver_ids[] = {
 	{
 		.name		= "s3c2410-uart",
@@ -2578,6 +2795,9 @@  static const struct platform_device_id s3c24xx_serial_driver_ids[] = {
 	}, {
 		.name		= "exynos5433-uart",
 		.driver_data	= EXYNOS5433_SERIAL_DRV_DATA,
+	}, {
+		.name		= "s5l-uart",
+		.driver_data	= S5L_SERIAL_DRV_DATA,
 	},
 	{ },
 };
@@ -2599,6 +2819,8 @@  static const struct of_device_id s3c24xx_uart_dt_match[] = {
 		.data = (void *)EXYNOS4210_SERIAL_DRV_DATA },
 	{ .compatible = "samsung,exynos5433-uart",
 		.data = (void *)EXYNOS5433_SERIAL_DRV_DATA },
+	{ .compatible = "AAPL,s5l-uart",
+		.data = (void *)S5L_SERIAL_DRV_DATA },
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
@@ -2694,6 +2916,9 @@  static int __init s3c2410_early_console_setup(struct earlycon_device *device,
 
 OF_EARLYCON_DECLARE(s3c2410, "samsung,s3c2410-uart",
 			s3c2410_early_console_setup);
+/* Apple SoCs are close enough to s3c2410 for earlycon */
+OF_EARLYCON_DECLARE(s5l, "AAPL,s5l-uart",
+			s3c2410_early_console_setup);
 
 /* S3C2412, S3C2440, S3C64xx */
 static struct samsung_early_console_data s3c2440_early_console_data = {
diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
index ca2c5393dc6b..377c8c6e5b97 100644
--- a/include/linux/serial_s3c.h
+++ b/include/linux/serial_s3c.h
@@ -246,6 +246,22 @@ 
 				 S5PV210_UFCON_TXTRIG4 |	\
 				 S5PV210_UFCON_RXTRIG4)
 
+
+#define APPLE_UCON_RXTO_ENA	9
+#define APPLE_UCON_RXTHRESH_ENA	12
+#define APPLE_UCON_TXTHRESH_ENA	13
+#define APPLE_UCON_RXTO_ENA_MSK		(1 << APPLE_UCON_RXTO_ENA)
+#define APPLE_UCON_RXTHRESH_ENA_MSK	(1 << APPLE_UCON_RXTHRESH_ENA)
+#define APPLE_UCON_TXTHRESH_ENA_MSK	(1 << APPLE_UCON_TXTHRESH_ENA)
+
+#define APPLE_UCON_DEFAULT	  S3C2410_UCON_RXFIFO_TOI
+
+#define APPLE_UTRSTAT_RXTHRESH	(1<<4)
+#define APPLE_UTRSTAT_TXTHRESH	(1<<5)
+#define APPLE_UTRSTAT_RXTO	(1<<9)
+#define APPLE_UTRSTAT_ALL_FLAGS	(0x3f0)
+
+
 #ifndef __ASSEMBLY__
 
 #include <linux/serial_core.h>
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 62c22045fe65..59d102b674db 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -277,4 +277,7 @@ 
 /* Freescale LINFlexD UART */
 #define PORT_LINFLEXUART	122
 
+/* Apple Silicon (M1/T8103) UART (Samsung variant) */
+#define PORT_APPLE	123
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */