Quoting Karthik Ramasubramanian (2018-03-20 15:53:25) > > > On 3/20/2018 9:37 AM, Stephen Boyd wrote: > > Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:49) > >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > >> new file mode 100644 > >> index 0000000..1442777 > >> --- /dev/null > >> +++ b/drivers/tty/serial/qcom_geni_serial.c > >> @@ -0,0 +1,1158 @@ > >> + > >> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE > >> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch) > >> +{ > >> + writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn); > > > > Does this expect the whole word to have data to write? Or does the FIFO > > output a character followed by three NUL bytes each time it gets > > written? The way that uart_console_write() works is to take each > > character a byte at a time, put it into an int (so extend that byte with > > zero) and then pass it to the putchar function. I would expect that at > > this point the hardware sees the single character and then 3 NULs enter > > the FIFO each time. > > > > For previous MSM uarts I had to handle this oddity by packing the words > > into the fifo four at a time. You may need to do the same here. > The packing configuration 1 * 8 (done using geni_se_config_packing) > ensures that only one byte per FIFO word needs to be transmitted. From > that perspective, we need not have such oddity. Ok! That's good to hear. > > > > Can you also support the OF_EARLYCON_DECLARE method of console writing > > so we can get an early printk style debug console? > Do you prefer that as part of this patch itself or is it ok if I upload > the earlycon support once this gets merged. I think this already got merged? So just split it out into another patch would be fine. I see the config is already selecting the earlycon support so it must be planned. > > > > > >> + > >> + spin_lock_irqsave(&uport->lock, flags); > >> + m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS); > >> + s_irq_status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS); > >> + m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN); > >> + writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR); > >> + writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR); > >> + > >> + if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN)) > >> + goto out_unlock; > >> + > >> + if (s_irq_status & S_RX_FIFO_WR_ERR_EN) { > >> + uport->icount.overrun++; > >> + tty_insert_flip_char(tport, 0, TTY_OVERRUN); > >> + } > >> + > >> + if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) && > >> + m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN)) > >> + qcom_geni_serial_handle_tx(uport); > >> + > >> + if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) { > >> + if (s_irq_status & S_GP_IRQ_0_EN) > >> + uport->icount.parity++; > >> + drop_rx = true; > >> + } else if (s_irq_status & S_GP_IRQ_2_EN || > >> + s_irq_status & S_GP_IRQ_3_EN) { > >> + uport->icount.brk++; > > > > Maybe move this stat accounting to the place where brk is handled? > Since other error accounting like overrun, parity are happening here, it > feels logical to keep that accounting here. Alright. > >> + return uart_add_one_port(&qcom_geni_console_driver, uport); > >> +} > >> + > >> +static int qcom_geni_serial_remove(struct platform_device *pdev) > >> +{ > >> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev); > >> + struct uart_driver *drv = port->uport.private_data; > >> + > >> + uart_remove_one_port(drv, &port->uport); > >> + return 0; > >> +} > >> + > >> +static int __maybe_unused qcom_geni_serial_sys_suspend_noirq(struct device *dev) > >> +{ > >> + struct platform_device *pdev = to_platform_device(dev); > >> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev); > >> + struct uart_port *uport = &port->uport; > >> + > >> + uart_suspend_port(uport->private_data, uport); > >> + return 0; > >> +} > >> + > >> +static int __maybe_unused qcom_geni_serial_sys_resume_noirq(struct device *dev) > >> +{ > >> + struct platform_device *pdev = to_platform_device(dev); > >> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev); > >> + struct uart_port *uport = &port->uport; > >> + > >> + if (console_suspend_enabled && uport->suspended) { > >> + uart_resume_port(uport->private_data, uport); > >> + disable_irq(uport->irq); > > > > I missed the enable_irq() part. Is this still necessary? > Suspending the uart console port invokes the uart port shutdown > operation. The shutdown operation disables and frees the concerned IRQ. > Resuming the uart console port invokes the uart port startup operation > which requests for the IRQ. The request_irq operation auto-enables the > IRQ. In addition, resume_noirq implicitly enables the IRQ. This leads to > an unbalanced IRQ enable warning. > > This disable_irq() helps with suppressing that warning. That's not obvious so we need a big comment here. I thought we would move this to the normal suspend/resume callbacks and skip the noirq variants. That would make this disable_irq() unnecessary then? BTW, free_irq() should disable the irq itself, so it looks odd to have a disable_irq() followed directly by a free_irq(). -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html