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.
