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

[Karthik Ramasubramanian]

On 3/21/2018 11:20 AM, Stephen Boyd wrote:
> 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 000..1442777
 --- /dev/null
 +++ b/drivers/tty/serial/qcom_geni_serial.c
 @@ -0,0 +1,1158 @@
 +
> 
>>>
>>> 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.
Yes it is definitely in the plan. Since the serial driver got merged, I
will address the pending comments in this patch series. I will upload
the early console support in another patch.
> 
>>>
>>>
 +
 +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 = >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?
For a non-console UART(eg. 4-wire UART), to reduce the wakeup latency
_noirq variant is used so that the resources can be turned on as soon as
possible. The idea is to use the same suspend and resume operation for
both debug-uart and regular uart. Hence using the _noirq variant.

I will add a detailed comment regarding why we are disabling the IRQ.
> 
> BTW, free_irq() should disable the irq itself, so it looks odd to have a
> disable_irq() followed directly by a free_irq().
I will update to just free_irq.
> 
Regards,
Karthik.
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stephen Boyd]

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 000..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(>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(_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, >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 = >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 = >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 

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

[Evan Green]

On Tue, Mar 20, 2018 at 4:44 PM Karthik Ramasubramanian <
krama...@codeaurora.org> wrote:



> On 3/20/2018 12:39 PM, Evan Green wrote:
> > Hi Karthik,
> >
> > On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian <
> > krama...@codeaurora.org> wrote:
> >
> >> +
> >> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> >> +   int offset, int field, bool set)
> >> +{
> >> +   u32 reg;
> >> +   struct qcom_geni_serial_port *port;
> >> +   unsigned int baud;
> >> +   unsigned int fifo_bits;
> >> +   unsigned long timeout_us = 2;
> >> +
> >> +   /* Ensure polling is not re-ordered before the prior
writes/reads
> > */
> >> +   mb();
> >
> > These barriers sprinkled around everywhere are new. Did
> > you find that you needed them for something? In this case, the
> > readl_poll_timeout_atomic should already have a read barrier (nor do you
> > probably care about read reordering, right?) Perhaps this should
> > only be a mmiowb rather than a full barrier, because you really just
want
> > to say "make sure all my old writes got out to hardware before spinning"
> These barriers have been there from v3. Regarding this barrier - since
> readl_poll_timeout_atomic has a read barrier, this one can be converted
> to just write barrier.

Thanks. I must have missed them in v3. Is that mmiowb that would go there,
or wmb? I'm unsure.

> >
> >> +
> >> +   if (uport->private_data) {
> >> +   port = to_dev_port(uport, uport);
> >> +   baud = port->baud;
> >> +   if (!baud)
> >> +   baud = 115200;
> >> +   fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> >> +   /*
> >> +* Total polling iterations based on FIFO worth of
bytes
> > to be
> >> +* sent at current baud. Add a little fluff to the
wait.
> >> +*/
> >> +   timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
> >> +   }
> >> +
> >> +   return !readl_poll_timeout_atomic(uport->membase + offset, reg,
> >> +(bool)(reg & field) == set, 10, timeout_us);
> >> +}
> > [...]
> >> +
> >> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
> >> +{
> >> +   u32 irq_en;
> >> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> >> +   u32 status;
> >> +
> >> +   if (port->xfer_mode == GENI_SE_FIFO) {
> >> +   status = readl_relaxed(uport->membase +
SE_GENI_STATUS);
> >> +   if (status & M_GENI_CMD_ACTIVE)
> >> +   return;
> >> +
> >> +   if (!qcom_geni_serial_tx_empty(uport))
> >> +   return;
> >> +
> >> +   /*
> >> +* Ensure writing to IRQ_EN & watermark registers are
not
> >> +* re-ordered before checking the status of the Serial
> >> +* Engine and TX FIFO
> >> +*/
> >> +   mb();
> >
> > Here's another one. You should just be able to use a regular readl when
> > reading SE_GENI_STATUS and remove this barrier, since readl has an rmb
> > which orders your read of M_IRQ_EN below. I don't think you need to
worry
> > about the writes below going above the read above, since there's logic
in
> > between that needs the result of the read. Maybe that also saves you in
the
> > read case, too. Either way, this barrier seems heavy handed.
> Write to the watermark register does not have any dependency on the
> reads. Hence it can be re-ordered. But writing to that register alone
> without enabling the watermark interrupt will not have any impact. From
> that perspective, using readl while checking the GENI_STATUS is
> sufficient to maintain the required order. I will put a comment
> regarding the use of readl so that the reason is not lost.

Yes, I see what you mean, and without the branch I'd agree. My thinking
though was that you have a branch between the read and writes. So to
determine whether or not to even execute the writes, you must have
successfully evaluated the read, since program flow depends on it. I will
admit this is where my barrier knowledge gets fuzzy. Can speculative
execution perform register writes? (ie if that branch was guessed wrong
before the read actually completes, and then the writes happen before the
read? That seems like it couldn't possibly happen, as it would result in
weird spurious speculative writes to registers. Perhaps the mapping bits
prevent this sort of thing...)

If what I've said makes any sort of sense, and you still want to keep the
barriers here and below, then I'll let it go.

> >
> >> +
> >> +   irq_en = readl_relaxed(uport->membase +
SE_GENI_M_IRQ_EN);
> >> +   irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
> >> +
> >> +   writel_relaxed(port->tx_wm, uport->membase +
> >> +
SE_GENI_TX_WATERMARK_REG);
> >> +   writel_relaxed(irq_en, 

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

[Karthik Ramasubramanian]

On 3/20/2018 12:39 PM, Evan Green wrote:
> Hi Karthik,
> 
> On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian <
> krama...@codeaurora.org> wrote:
> 
>> +
>> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>> +   int offset, int field, bool set)
>> +{
>> +   u32 reg;
>> +   struct qcom_geni_serial_port *port;
>> +   unsigned int baud;
>> +   unsigned int fifo_bits;
>> +   unsigned long timeout_us = 2;
>> +
>> +   /* Ensure polling is not re-ordered before the prior writes/reads
> */
>> +   mb();
> 
> These barriers sprinkled around everywhere are new. Did
> you find that you needed them for something? In this case, the
> readl_poll_timeout_atomic should already have a read barrier (nor do you
> probably care about read reordering, right?) Perhaps this should
> only be a mmiowb rather than a full barrier, because you really just want
> to say "make sure all my old writes got out to hardware before spinning"
These barriers have been there from v3. Regarding this barrier - since
readl_poll_timeout_atomic has a read barrier, this one can be converted
to just write barrier.
> 
>> +
>> +   if (uport->private_data) {
>> +   port = to_dev_port(uport, uport);
>> +   baud = port->baud;
>> +   if (!baud)
>> +   baud = 115200;
>> +   fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
>> +   /*
>> +* Total polling iterations based on FIFO worth of bytes
> to be
>> +* sent at current baud. Add a little fluff to the wait.
>> +*/
>> +   timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
>> +   }
>> +
>> +   return !readl_poll_timeout_atomic(uport->membase + offset, reg,
>> +(bool)(reg & field) == set, 10, timeout_us);
>> +}
> [...]
>> +
>> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
>> +{
>> +   u32 irq_en;
>> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>> +   u32 status;
>> +
>> +   if (port->xfer_mode == GENI_SE_FIFO) {
>> +   status = readl_relaxed(uport->membase + SE_GENI_STATUS);
>> +   if (status & M_GENI_CMD_ACTIVE)
>> +   return;
>> +
>> +   if (!qcom_geni_serial_tx_empty(uport))
>> +   return;
>> +
>> +   /*
>> +* Ensure writing to IRQ_EN & watermark registers are not
>> +* re-ordered before checking the status of the Serial
>> +* Engine and TX FIFO
>> +*/
>> +   mb();
> 
> Here's another one. You should just be able to use a regular readl when
> reading SE_GENI_STATUS and remove this barrier, since readl has an rmb
> which orders your read of M_IRQ_EN below. I don't think you need to worry
> about the writes below going above the read above, since there's logic in
> between that needs the result of the read. Maybe that also saves you in the
> read case, too. Either way, this barrier seems heavy handed.
Write to the watermark register does not have any dependency on the
reads. Hence it can be re-ordered. But writing to that register alone
without enabling the watermark interrupt will not have any impact. From
that perspective, using readl while checking the GENI_STATUS is
sufficient to maintain the required order. I will put a comment
regarding the use of readl so that the reason is not lost.
> 
>> +
>> +   irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
>> +   irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>> +
>> +   writel_relaxed(port->tx_wm, uport->membase +
>> +   SE_GENI_TX_WATERMARK_REG);
>> +   writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
>> +   }
>> +}
>> +
>> +static void qcom_geni_serial_stop_tx(struct uart_port *uport)
>> +{
>> +   u32 irq_en;
>> +   u32 status;
>> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>> +
>> +   irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
>> +   irq_en &= ~M_CMD_DONE_EN;
>> +   if (port->xfer_mode == GENI_SE_FIFO) {
>> +   irq_en &= ~M_TX_FIFO_WATERMARK_EN;
>> +   writel_relaxed(0, uport->membase +
>> +SE_GENI_TX_WATERMARK_REG);
>> +   }
>> +   port->xmit_size = 0;
>> +   writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
>> +   status = readl_relaxed(uport->membase + SE_GENI_STATUS);
>> +   /* Possible stop tx is called multiple times. */
>> +   if (!(status & M_GENI_CMD_ACTIVE))
>> +   return;
>> +
>> +   /*
>> +* Ensure cancel command write is not re-ordered before checking
>> +* the status of the Primary Sequencer.
>> +*/
>> +   mb();
> 
> I don't see how what's 

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

[Karthik Ramasubramanian]

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 000..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.
> 
>> +}
>> +
>> +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?
Ok.
> 
>> +   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?
Ok.
> 
>> +   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 = >uport;
>> +   if (oops_in_progress)
>> +   locked = spin_trylock_irqsave(>lock, flags);
>> +   else
>> +   spin_lock_irqsave(>lock, flags);
>> +
>> +   /* Cancel the current write to log the fault */
>> +   if (!locked) {
>> +   geni_se_cancel_m_cmd(>se);
>> +   if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>> +   M_CMD_CANCEL_EN, true)) {
>> +   geni_se_abort_m_cmd(>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(>lock, flags);
>> +}
> 
> Can you also support the OF_EARLYCON_DECLARE method of console writing
> so we can get an early printk style 

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

[Evan Green]

Hi Karthik,

On Wed, Mar 14, 2018 at 4:59 PM Karthikeyan Ramasubramanian <
krama...@codeaurora.org> wrote:

> +
> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> +   int offset, int field, bool set)
> +{
> +   u32 reg;
> +   struct qcom_geni_serial_port *port;
> +   unsigned int baud;
> +   unsigned int fifo_bits;
> +   unsigned long timeout_us = 2;
> +
> +   /* Ensure polling is not re-ordered before the prior writes/reads
*/
> +   mb();

These barriers sprinkled around everywhere are new. Did
you find that you needed them for something? In this case, the
readl_poll_timeout_atomic should already have a read barrier (nor do you
probably care about read reordering, right?) Perhaps this should
only be a mmiowb rather than a full barrier, because you really just want
to say "make sure all my old writes got out to hardware before spinning"

> +
> +   if (uport->private_data) {
> +   port = to_dev_port(uport, uport);
> +   baud = port->baud;
> +   if (!baud)
> +   baud = 115200;
> +   fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> +   /*
> +* Total polling iterations based on FIFO worth of bytes
to be
> +* sent at current baud. Add a little fluff to the wait.
> +*/
> +   timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
> +   }
> +
> +   return !readl_poll_timeout_atomic(uport->membase + offset, reg,
> +(bool)(reg & field) == set, 10, timeout_us);
> +}
[...]
> +
> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
> +{
> +   u32 irq_en;
> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +   u32 status;
> +
> +   if (port->xfer_mode == GENI_SE_FIFO) {
> +   status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> +   if (status & M_GENI_CMD_ACTIVE)
> +   return;
> +
> +   if (!qcom_geni_serial_tx_empty(uport))
> +   return;
> +
> +   /*
> +* Ensure writing to IRQ_EN & watermark registers are not
> +* re-ordered before checking the status of the Serial
> +* Engine and TX FIFO
> +*/
> +   mb();

Here's another one. You should just be able to use a regular readl when
reading SE_GENI_STATUS and remove this barrier, since readl has an rmb
which orders your read of M_IRQ_EN below. I don't think you need to worry
about the writes below going above the read above, since there's logic in
between that needs the result of the read. Maybe that also saves you in the
read case, too. Either way, this barrier seems heavy handed.

> +
> +   irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> +   irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
> +
> +   writel_relaxed(port->tx_wm, uport->membase +
> +   SE_GENI_TX_WATERMARK_REG);
> +   writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> +   }
> +}
> +
> +static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> +{
> +   u32 irq_en;
> +   u32 status;
> +   struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> +   irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> +   irq_en &= ~M_CMD_DONE_EN;
> +   if (port->xfer_mode == GENI_SE_FIFO) {
> +   irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> +   writel_relaxed(0, uport->membase +
> +SE_GENI_TX_WATERMARK_REG);
> +   }
> +   port->xmit_size = 0;
> +   writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> +   status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> +   /* Possible stop tx is called multiple times. */
> +   if (!(status & M_GENI_CMD_ACTIVE))
> +   return;
> +
> +   /*
> +* Ensure cancel command write is not re-ordered before checking
> +* the status of the Primary Sequencer.
> +*/
> +   mb();

I don't see how what's stated in your comment could happen, as that would
be a logic error. This barrier seems unneeded to me.

> +
> +   geni_se_cancel_m_cmd(>se);
> +   if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +   M_CMD_CANCEL_EN, true)) {
> +   geni_se_abort_m_cmd(>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);
> +}
> +
> +static void qcom_geni_serial_start_rx(struct uart_port *uport)
> +{
> 

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

[Stephen Boyd]

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 000..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 = >uport;
> +   if (oops_in_progress)
> +   locked = spin_trylock_irqsave(>lock, flags);
> +   else
> +   spin_lock_irqsave(>lock, flags);
> +
> +   /* Cancel the current write to log the fault */
> +   if (!locked) {
> +   geni_se_cancel_m_cmd(>se);
> +   if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> +   M_CMD_CANCEL_EN, true)) {
> +   geni_se_abort_m_cmd(>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(>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 = >state->port;
> +   for (i = 0; i < bytes; ) {
> +   int c;
> +   int 

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

[Karthikeyan Ramasubramanian]

This driver supports GENI based UART Controller in the Qualcomm SOCs. The
Qualcomm Generic Interface (GENI) is a programmable module supporting a
wide range of serial interfaces including UART. This driver support console
operations using FIFO mode of transfer.

Signed-off-by: Girish Mahadevan 
Signed-off-by: Karthikeyan Ramasubramanian 
Signed-off-by: Sagar Dharia 
Signed-off-by: Doug Anderson 
---
 drivers/tty/serial/Kconfig|   15 +
 drivers/tty/serial/Makefile   |1 +
 drivers/tty/serial/qcom_geni_serial.c | 1158 +
 3 files changed, 1174 insertions(+)
 create mode 100644 drivers/tty/serial/qcom_geni_serial.c

diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 3682fd3..d132971 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1104,6 +1104,21 @@ config SERIAL_MSM_CONSOLE
select SERIAL_CORE_CONSOLE
select SERIAL_EARLYCON
 
+config SERIAL_QCOM_GENI
+   tristate "QCOM on-chip GENI based serial port support"
+   depends on ARCH_QCOM || COMPILE_TEST
+   depends on QCOM_GENI_SE
+   select SERIAL_CORE
+
+config SERIAL_QCOM_GENI_CONSOLE
+   bool "QCOM GENI Serial Console support"
+   depends on SERIAL_QCOM_GENI=y
+   select SERIAL_CORE_CONSOLE
+   select SERIAL_EARLYCON
+   help
+ Serial console driver for Qualcomm Technologies Inc's GENI based
+ QUP hardware.
+
 config SERIAL_VT8500
bool "VIA VT8500 on-chip serial port support"
depends on ARCH_VT8500
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index 842d185..64a8d82 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_SERIAL_SGI_IOC3) += ioc3_serial.o
 obj-$(CONFIG_SERIAL_ATMEL) += atmel_serial.o
 obj-$(CONFIG_SERIAL_UARTLITE) += uartlite.o
 obj-$(CONFIG_SERIAL_MSM) += msm_serial.o
+obj-$(CONFIG_SERIAL_QCOM_GENI) += qcom_geni_serial.o
 obj-$(CONFIG_SERIAL_NETX) += netx-serial.o
 obj-$(CONFIG_SERIAL_KS8695) += serial_ks8695.o
 obj-$(CONFIG_SERIAL_OMAP) += omap-serial.o
diff --git a/drivers/tty/serial/qcom_geni_serial.c 
b/drivers/tty/serial/qcom_geni_serial.c
new file mode 100644
index 000..1442777
--- /dev/null
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -0,0 +1,1158 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* UART specific GENI registers */
+#define SE_UART_TX_TRANS_CFG   0x25c
+#define SE_UART_TX_WORD_LEN0x268
+#define SE_UART_TX_STOP_BIT_LEN0x26c
+#define SE_UART_TX_TRANS_LEN   0x270
+#define SE_UART_RX_TRANS_CFG   0x280
+#define SE_UART_RX_WORD_LEN0x28c
+#define SE_UART_RX_STALE_CNT   0x294
+#define SE_UART_TX_PARITY_CFG  0x2a4
+#define SE_UART_RX_PARITY_CFG  0x2a8
+
+/* SE_UART_TRANS_CFG */
+#define UART_TX_PAR_EN BIT(0)
+#define UART_CTS_MASK  BIT(1)
+
+/* SE_UART_TX_WORD_LEN */
+#define TX_WORD_LEN_MSKGENMASK(9, 0)
+
+/* SE_UART_TX_STOP_BIT_LEN */
+#define TX_STOP_BIT_LEN_MSKGENMASK(23, 0)
+#define TX_STOP_BIT_LEN_1  0
+#define TX_STOP_BIT_LEN_1_51
+#define TX_STOP_BIT_LEN_2  2
+
+/* SE_UART_TX_TRANS_LEN */
+#define TX_TRANS_LEN_MSK   GENMASK(23, 0)
+
+/* SE_UART_RX_TRANS_CFG */
+#define UART_RX_INS_STATUS_BIT BIT(2)
+#define UART_RX_PAR_EN BIT(3)
+
+/* SE_UART_RX_WORD_LEN */
+#define RX_WORD_LEN_MASK   GENMASK(9, 0)
+
+/* SE_UART_RX_STALE_CNT */
+#define RX_STALE_CNT   GENMASK(23, 0)
+
+/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */
+#define PAR_CALC_ENBIT(0)
+#define PAR_MODE_MSK   GENMASK(2, 1)
+#define PAR_MODE_SHFT  1
+#define PAR_EVEN   0x00
+#define PAR_ODD0x01
+#define PAR_SPACE  0x10
+#define PAR_MARK   0x11
+
+/* UART M_CMD OP codes */
+#define UART_START_TX  0x1
+#define UART_START_BREAK   0x4
+#define UART_STOP_BREAK0x5
+/* UART S_CMD OP codes */
+#define UART_START_READ0x1
+#define UART_PARAM 0x1
+
+#define UART_OVERSAMPLING  32
+#define STALE_TIMEOUT  16
+#define DEFAULT_BITS_PER_CHAR  10
+#define GENI_UART_CONS_PORTS   1
+#define DEF_FIFO_DEPTH_WORDS   16
+#define DEF_TX_WM  2
+#define DEF_FIFO_WIDTH_BITS32
+#define UART_CONSOLE_RX_WM 2
+
+#ifdef CONFIG_CONSOLE_POLL
+#define RX_BYTES_PW 1
+#else
+#define RX_BYTES_PW 4
+#endif
+
+struct qcom_geni_serial_port {
+   struct uart_port uport;
+   struct geni_se se;
+   char name[20];
+   u32 tx_fifo_depth;
+   u32 tx_fifo_width;
+   u32 rx_fifo_depth;
+