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

[Doug Anderson]

Hi,

On Fri, Jun 7, 2024 at 12:43 AM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> 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.