Re: [PATCH v4 4/6] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux