Quoting Karthik Ramasubramanian (2018-03-20 15:53:25)

On 3/20/2018 9:37 AM, Stephen Boyd wrote:

quoted

Quoting Karthikeyan Ramasubramanian (2018-03-14 16:58:49)

quoted

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.

quoted

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.

quoted

quoted

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

quoted

quoted

+       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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html