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

> +}
> +
> +static void
> +__qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> +                                unsigned int count)
> +{
> +       int i;
> +       u32 bytes_to_send = count;
> +
> +       for (i = 0; i < count; i++) {
> +               if (s[i] == '\n')

Can you add a comment that uart_console_write() adds a carriage return
for each newline?

> +                       bytes_to_send++;
> +       }
> +
> +       writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> +       qcom_geni_serial_setup_tx(uport, bytes_to_send);
> +       for (i = 0; i < count; ) {
> +               size_t chars_to_write = 0;
> +               size_t avail = DEF_FIFO_DEPTH_WORDS - DEF_TX_WM;
> +
> +               /*
> +                * If the WM bit never set, then the Tx state machine is not
> +                * in a valid state, so break, cancel/abort any existing
> +                * command. Unfortunately the current data being written is
> +                * lost.
> +                */
> +               if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +                                               M_TX_FIFO_WATERMARK_EN, true))
> +                       break;
> +               chars_to_write = min_t(size_t, (size_t)(count - i), avail / 2);

The _t part of min_t should do the casting already, so we can drop the
cast on count - i?

> +               uart_console_write(uport, s + i, chars_to_write,
> +                                               qcom_geni_serial_wr_char);
> +               writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> +                                                       SE_GENI_M_IRQ_CLEAR);
> +               i += chars_to_write;
> +       }
> +       qcom_geni_serial_poll_tx_done(uport);
> +}
> +
> +static void qcom_geni_serial_console_write(struct console *co, const char *s,
> +                             unsigned int count)
> +{
> +       struct uart_port *uport;
> +       struct qcom_geni_serial_port *port;
> +       bool locked = true;
> +       unsigned long flags;
> +
> +       WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> +
> +       port = get_port_from_line(co->index);
> +       if (IS_ERR(port))
> +               return;
> +
> +       uport = &port->uport;
> +       if (oops_in_progress)
> +               locked = spin_trylock_irqsave(&uport->lock, flags);
> +       else
> +               spin_lock_irqsave(&uport->lock, flags);
> +
> +       /* Cancel the current write to log the fault */
> +       if (!locked) {
> +               geni_se_cancel_m_cmd(&port->se);
> +               if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +                                               M_CMD_CANCEL_EN, true)) {
> +                       geni_se_abort_m_cmd(&port->se);
> +                       qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +                                                       M_CMD_ABORT_EN, true);
> +                       writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> +                                                       SE_GENI_M_IRQ_CLEAR);
> +               }
> +               writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> +                                                       SE_GENI_M_IRQ_CLEAR);
> +       }
> +
> +       __qcom_geni_serial_console_write(uport, s, count);
> +       if (locked)
> +               spin_unlock_irqrestore(&uport->lock, flags);
> +}

Can you also support the OF_EARLYCON_DECLARE method of console writing
so we can get an early printk style debug console?

> +
> +static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
> +{
> +       u32 i;
> +       unsigned char buf[sizeof(u32)];
> +       struct tty_port *tport;
> +       struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> +       tport = &uport->state->port;
> +       for (i = 0; i < bytes; ) {
> +               int c;
> +               int chunk = min_t(int, bytes - i, port->rx_bytes_pw);
> +
> +               ioread32_rep(uport->membase + SE_GENI_RX_FIFOn, buf, 1);
> +               i += chunk;
> +               if (drop)
> +                       continue;
> +
> +               for (c = 0; c < chunk; c++) {
> +                       int sysrq;
> +
> +                       uport->icount.rx++;
> +                       if (port->brk && buf[c] == 0) {
> +                               port->brk = false;
> +                               if (uart_handle_break(uport))
> +                                       continue;

Thanks!

> +                       }
> +
> +                       sysrq = uart_handle_sysrq_char(uport, buf[c]);
> +                       if (!sysrq)
> +                               tty_insert_flip_char(tport, buf[c], TTY_NORMAL);
> +               }
> +       }
> +       if (!drop)
> +               tty_flip_buffer_push(tport);
> +       return 0;
> +}
[...]
> +
> +static void qcom_geni_serial_handle_tx(struct uart_port *uport)
> +{
> +       struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +       struct circ_buf *xmit = &uport->state->xmit;
> +       size_t avail;
> +       size_t remaining;
> +       int i;
> +       u32 status;
> +       unsigned int chunk;
> +       int tail;
> +
> +       chunk = uart_circ_chars_pending(xmit);
> +       status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> +       /* Both FIFO and framework buffer are drained */
> +       if (chunk == port->xmit_size && !status) {
> +               port->xmit_size = 0;
> +               uart_circ_clear(xmit);
> +               qcom_geni_serial_stop_tx(uport);
> +               goto out_write_wakeup;
> +       }
> +       chunk -= port->xmit_size;
> +
> +       avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
> +       tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1);
> +       if (chunk > (UART_XMIT_SIZE - tail))
> +               chunk = UART_XMIT_SIZE - tail;
> +       if (chunk > avail)
> +               chunk = avail;

chunk = min3(chunk, UART_XMIT_SIZE - tail, avail);

> +
> +       if (!chunk)
> +               goto out_write_wakeup;
> +
> +       qcom_geni_serial_setup_tx(uport, chunk);
> +
> +       remaining = chunk;
> +       for (i = 0; i < chunk; ) {
> +               unsigned int tx_bytes;
> +               unsigned int buf = 0;

Make buf into a u8 array of 4?

> +               int c;
> +
> +               tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw);
> +               for (c = 0; c < tx_bytes ; c++)
> +                       buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE));

And then just put buf[c] = xmit->buf[tail + c] here?

> +
> +               writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn);

This also needs to be an iowrite32_rep(uport->membase, buf, 1) for
endian reasons.

> +
> +               i += tx_bytes;
> +               tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
> +               uport->icount.tx += tx_bytes;
> +               remaining -= tx_bytes;
> +       }
> +       qcom_geni_serial_poll_tx_done(uport);
> +       port->xmit_size += chunk;
> +out_write_wakeup:
> +       uart_write_wakeup(uport);
> +}
> +
> +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> +{
> +       unsigned int m_irq_status;
> +       unsigned int s_irq_status;
> +       struct uart_port *uport = dev;
> +       unsigned long flags;
> +       unsigned int m_irq_en;
> +       bool drop_rx = false;
> +       struct tty_port *tport = &uport->state->port;
> +       struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> +       if (uport->suspended)
> +               return IRQ_HANDLED;

Is it a spurious IRQ if this happens? Return IRQ_NONE instead?

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

> +               port->brk = true;
> +       }
> +
> +       if (s_irq_status & S_RX_FIFO_WATERMARK_EN ||
> +                                       s_irq_status & S_RX_FIFO_LAST_EN)
> +               qcom_geni_serial_handle_rx(uport, drop_rx);
> +
> +out_unlock:
> +       spin_unlock_irqrestore(&uport->lock, flags);
> +       return IRQ_HANDLED;
> +}
> +
> +static int get_tx_fifo_size(struct qcom_geni_serial_port *port)
> +{
> +       struct uart_port *uport;
> +
> +       if (!port)
> +               return -ENODEV;

This happens?

> +
> +       uport = &port->uport;
> +       port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se);
> +       port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se);
> +       port->rx_fifo_depth = geni_se_get_rx_fifo_depth(&port->se);
> +       uport->fifosize =
> +               (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
> +       return 0;
> +}
> +
[...]
> +
> +#ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> +static int __init qcom_geni_console_setup(struct console *co, char *options)
> +{
> +       struct uart_port *uport;
> +       struct qcom_geni_serial_port *port;
> +       int baud;
> +       int bits = 8;
> +       int parity = 'n';
> +       int flow = 'n';
> +
> +       if (co->index >= GENI_UART_CONS_PORTS  || co->index < 0)
> +               return -ENXIO;
> +
> +       port = get_port_from_line(co->index);
> +       if (IS_ERR(port)) {
> +               pr_err("Invalid line %d(%d)\n", co->index, (int)PTR_ERR(port));

Use %ld to avoid casting the error value? Or just don't print it at all
because it doesn't really help the end user.

> +               return PTR_ERR(port);
> +       }
> +
> +       uport = &port->uport;
> +
> +       if (unlikely(!uport->membase))
> +               return -ENXIO;
> +
> +       if (geni_se_resources_on(&port->se)) {
> +               dev_err(port->se.dev, "Error turning on resources\n");
> +               return -ENXIO;
> +       }
> +
> +       if (unlikely(geni_se_read_proto(&port->se) != GENI_SE_UART)) {
> +               geni_se_resources_off(&port->se);
> +               return -ENXIO;
> +       }
> +
> +       if (!port->setup) {
> +               port->tx_bytes_pw = 1;
> +               port->rx_bytes_pw = RX_BYTES_PW;
> +               qcom_geni_serial_stop_rx(uport);
> +               qcom_geni_serial_port_setup(uport);
> +       }
> +
> +       if (options)
> +               uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> +       return uart_set_options(uport, co, baud, parity, bits, flow);
> +}
[..]
> +
> +static int qcom_geni_serial_probe(struct platform_device *pdev)
> +{
> +       int ret = 0;
> +       int line = -1;
> +       struct qcom_geni_serial_port *port;
> +       struct uart_port *uport;
> +       struct resource *res;
> +
> +       if (pdev->dev.of_node)
> +               line = of_alias_get_id(pdev->dev.of_node, "serial");
> +       else
> +               line = pdev->id;

Do we need to support the non-alias method?

> +
> +       if (line < 0 || line >= GENI_UART_CONS_PORTS)
> +               return -ENXIO;
> +       port = get_port_from_line(line);
> +       if (IS_ERR(port)) {
> +               ret = PTR_ERR(port);
> +               dev_err(&pdev->dev, "Invalid line %d(%d)\n", line, ret);
> +               return ret;
> +       }
> +
> +       uport = &port->uport;
> +       /* Don't allow 2 drivers to access the same port */
> +       if (uport->private_data)
> +               return -ENODEV;
> +
> +       uport->dev = &pdev->dev;
> +       port->se.dev = &pdev->dev;
> +       port->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> +       port->se.clk = devm_clk_get(&pdev->dev, "se");
> +       if (IS_ERR(port->se.clk)) {
> +               ret = PTR_ERR(port->se.clk);
> +               dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> +               return ret;
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (IS_ERR(res))
> +               return PTR_ERR(res);
> +       uport->mapbase = res->start;
> +
> +       port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> +       port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> +       port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> +
> +       uport->irq = platform_get_irq(pdev, 0);
> +       if (uport->irq < 0) {
> +               dev_err(&pdev->dev, "Failed to get IRQ %d\n", uport->irq);
> +               return uport->irq;
> +       }
> +
> +       uport->private_data = &qcom_geni_console_driver;
> +       platform_set_drvdata(pdev, port);
> +       port->handle_rx = handle_rx_console;
> +       port->setup = false;

This would be set to false already?

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

> +       }
> +       return 0;
> +}
> +
> +static int __init qcom_geni_serial_init(void)
> +{
> +       int ret;
> +
> +       qcom_geni_console_port.uport.iotype = UPIO_MEM;
> +       qcom_geni_console_port.uport.ops = &qcom_geni_console_pops;
> +       qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF;
> +       qcom_geni_console_port.uport.line = 0;

Why can't this be done statically?
> +
> +       ret = console_register(&qcom_geni_console_driver);
> +       if (ret)
> +               return ret;
> +
> +       ret = platform_driver_register(&qcom_geni_serial_platform_driver);
> +       if (ret)
> +               console_unregister(&qcom_geni_console_driver);
> +       return ret;
> +}
> +module_init(qcom_geni_serial_init);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux