Message ID | 20210204203951.52105-6-marcan@marcan.st (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | Apple M1 SoC platform bring-up | expand |
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
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
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.
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.
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.
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.
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
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.
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
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.
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.
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?
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
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
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?
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 --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 */
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(-)