by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] serial: qcom-geni: Fix suspend while active UART xfer

Hi,

On Fri, Jun 7, 2024 at 12:43 AM Ilpo Järvinen
<[email protected]> wrote:
>
> > @@ -311,11 +312,14 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> >
> > static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
> > {
> > + struct qcom_geni_serial_port *port = to_dev_port(uport);
> > u32 m_cmd;
> >
> > writel(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
> > m_cmd = UART_START_TX << M_OPCODE_SHFT;
>
> Unrelated to this patch and won't belong to this patch but I noticed it
> while reviewing. This could be converted into:
>
> m_cmd = FIELD_PREP(M_OPCODE_MSK, UART_START_TX);
>
> (and after converting the other use in the header file, the SHFT define
> becomes unused).

Sure. I'm going to leave that to someone in the future, though. I've
already spent more time than I should on this series and, if we're
going to do this, we should convert the whole driver (and perhaps all
the geni drivers).

> > @@ -335,6 +339,64 @@ static void qcom_geni_serial_poll_tx_done(struct uart_port *uport)
> > writel(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
> > }
> >
> > +static void qcom_geni_serial_drain_tx_fifo(struct uart_port *uport)
> > +{
> > + struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +
> > + /*
> > + * If the main sequencer is inactive it means that the TX command has
> > + * been completed and all bytes have been sent. Nothing to do in that
> > + * case.
> > + */
> > + if (!qcom_geni_serial_main_active(uport))
> > + return;
> > +
> > + /*
> > + * Wait until the FIFO has been drained. We've already taken bytes out
> > + * of the higher level queue in qcom_geni_serial_send_chunk_fifo() so
> > + * if we don't drain the FIFO but send the "cancel" below they seem to
> > + * get lost.
> > + */
> > + qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GENMASK(31, 0),
>
> That GENMASK(31, 0) is a field in a register (even if it covers the
> entire register)? It should be named with a define instead of creating the
> field mask here in an online fashion.

Sure. Done.

> > + port->tx_total - port->tx_remaining);
> > +
> > + /*
> > + * If clearing the FIFO made us inactive then we're done--no need for
> > + * a cancel.
> > + */
> > + if (!qcom_geni_serial_main_active(uport))
> > + return;
> > +
> > + /*
> > + * Cancel the current command. After this the main sequencer will
> > + * stop reporting that it's active and we'll have to start a new
> > + * transfer command.
> > + *
> > + * If we skip doing this cancel and then continue with a system
> > + * suspend while there's an active command in the main sequencer
> > + * then after resume time we won't get any more interrupts on the
> > + * main sequencer until we send the cancel.
> > + */
> > + geni_se_cancel_m_cmd(&port->se);
> > + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> > + M_CMD_CANCEL_EN, true)) {
> > + /* The cancel failed; try an abort as a fallback. */
> > + geni_se_abort_m_cmd(&port->se);
> > + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> > + M_CMD_ABORT_EN, true);
>
> Misaligned.

Done.

2024-06-10 22:28:31

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v3 2/7] serial: qcom-geni: Fix the timeout in qcom_geni_serial_poll_bit()

Hi,

On Fri, Jun 7, 2024 at 12:50 AM Ilpo Järvinen
<[email protected]> wrote:
>
> > + /*
> > + * This function is used to poll bits, some of which (like CMD_DONE)
> > + * might take as long as it takes for the FIFO plus the temp register
> > + * on the geni side to drain. The Linux core calculates such a timeout
> > + * for us and we can get it from uart_fifo_timeout().
> > + *
> > + * It should be noted that during earlycon the variables that
> > + * uart_fifo_timeout() makes use of in "uport" may not be setup yet.
> > + * It's difficult to set things up for earlycon since it can't
> > + * necessarily figure out the baud rate and reading the FIFO depth
> > + * from the wrapper means some extra MMIO maps that we don't get by
> > + * default. This isn't a big problem, though, since uart_fifo_timeout()
> > + * gives back its "slop" of 20ms as a minimum and that should be
> > + * plenty of time for earlycon unless we're running at an extremely
> > + * low baud rate.
> > + */
> > + timeout_us = jiffies_to_usecs(uart_fifo_timeout(uport));
>
> Hi,
>
> While this is not exactly incorrect, the back and forth conversions nsecs
> -> jiffies -> usecs feels somewhat odd, perhaps reworking
> uart_fifo_timeout()'s return type from jiffies to e.g. usecs would be
> preferrable. As is, the jiffies as its return type seems a small obstacle
> for using uart_fifo_timeout() which has come up in other contexts too.

Sure. I'll change it to "ms" instead of "us". We don't need the
fidelity of "us" here given that the function is adding 20 ms of slop
anyway so might as well return ms so that callers don't need to do so
much math and don't need to work with u64.

This means that I'll have to add a "* USEC_PER_MSEC" in my driver, but
it still feels like the more correct thing to do. It also has the nice
side effect of allowing the driver to remove the awkward
"DIV_ROUND_UP(timeout_us, 10) * 10" because we know that the timeout
will always be a proper multiple.

I'll also add a new function with the _ms suffix instead of changing
the old one. The suffix makes it clear to the caller what the unit of
the returned value is and we might as well leave the old wrapper
there--otherwise we just need to move the jiffies conversion into the
existing callers.