On Mon, Jun 24, 2024 at 02:23:52PM -0700, Doug Anderson wrote:
> On Mon, Jun 24, 2024 at 6:31 AM Johan Hovold <johan+linaro@xxxxxxxxxx> wrote:
> > @@ -665,16 +660,28 @@ static void qcom_geni_serial_start_tx_fifo(struct uart_port *uport)
> > static void qcom_geni_serial_stop_tx_fifo(struct uart_port *uport)
> > {
> > u32 irq_en;
> > - struct qcom_geni_serial_port *port = to_dev_port(uport);
> >
> > irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> > irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN);
> > writel(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
> > writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> > - /* Possible stop tx is called multiple times. */
>
> If qcom_geni_serial_stop_tx_fifo() is supposed to be used for UART
> flow control and you have a way to stop the transfer immediately
> without losing data (by using geni_se_cancel_m_cmd), maybe we should
> do that? If the other side wants us to stop transferring data and we
> can stop it right away that would be ideal...
Right, but since cancelling commands seems fragile at best (e.g.
potentially lost data, lockups) it seems best to just let the fifo
drain. But sure, if we can get cancel and restart to work reliably
eventually then even better.
> > +}
> > +
> > +static void qcom_geni_serial_clear_tx_fifo(struct uart_port *uport)
> > +{
> > + struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +
> > if (!qcom_geni_serial_main_active(uport))
> > return;
> >
> > + /*
> > + * Increase watermark level so that TX can be restarted and wait for
> > + * sequencer to start to prevent lockups.
> > + */
> > + writel(port->tx_fifo_depth, uport->membase + SE_GENI_TX_WATERMARK_REG);
> > + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> > + M_TX_FIFO_WATERMARK_EN, true);
>
> Oh, maybe this "wait for sequencer to start to prevent lockups." is
> the part that I was missing? Can you explain more about what's going
> on here? Why does waiting for the watermark interrupt to fire prevent
> lockups? I would have imagined that the watermark interrupt would be
> part of the geni hardware and have nothing to do with the firmware
> running on the other end, so I'm not sure why it firing somehow would
> prevent a lockup. Was this just by trial and error?
Yes, I saw two kinds of lockups in my experiments. The first was due to
data being left in the fifo so that the watermark interrupt never fired
on start_tx(), but there was one more case where it seemed like the hw
would get stuck if a cancel command was issues immediately after a new
command had been started.
Waiting for one character to be sent to avoid that race and seems to
address the latter hang.
Note that I hit this also when never filling the FIFO completely (e.g.
so that a watermark of 16 should have fired as there were never more
than 15 words in the fifo).
> > @@ -684,6 +691,8 @@ static void qcom_geni_serial_stop_tx_fifo(struct uart_port *uport)
> > writel(M_CMD_ABORT_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
> > }
> > writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
> > +
> > + port->tx_remaining = 0;
> > }
> >
> > static void qcom_geni_serial_handle_rx_fifo(struct uart_port *uport, bool drop)
> > @@ -1069,11 +1078,10 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
> > {
> > disable_irq(uport->irq);
> >
> > - if (uart_console(uport))
> > - return;
>
> Can you explain this part of the patch? I'm not saying it's wrong to
> remove this special case since this driver seems to have lots of
> needless special cases that are already handled by the core or by
> other parts of the driver, but this change seems unrelated to the rest
> of the patch. Could it be a separate patch?
We need to stop tx and clear the FIFO also when the port is used as a
console.
I added back the above check in commit 9aff74cc4e9e ("serial: qcom-geni:
fix console shutdown hang") as a quick way to work around a previous
regression where we would hit this soft lockup. With the issue fixed,
the workaround is no longer needed.
Johan
