Received-SPF: pass (google.com: domain of linux-kernel+bounces-188183-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240523162207.2.I0f81a5baa37d368f291c96ee4830abca337e3c87@changeid>
References: <20240523232216.3148367-1-dianders@chromium.org> <20240523162207.2.I0f81a5baa37d368f291c96ee4830abca337e3c87@changeid>
From: Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date: Thu, 23 May 2024 19:38:34 -0500
Message-ID: <CAE-0n53F3Xg2vOdgy-Vpjw4Kirdgi6B+BnO51fd6qOtDu0iXCg@mail.gmail.com>
Subject: Re: [PATCH 2/2] serial: qcom-geni: Fix qcom_geni_serial_stop_tx_fifo()
 while xfer
To: Douglas Anderson <dianders@chromium.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	Jiri Slaby <jirislaby@kernel.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, 
	John Ogness <john.ogness@linutronix.de>, Tony Lindgren <tony@atomide.com>, 
	linux-arm-msm@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>, 
	=?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <u.kleine-koenig@pengutronix.de>, 
	Yicong Yang <yangyicong@hisilicon.com>, James Clark <james.clark@arm.com>, 
	Thomas Gleixner <tglx@linutronix.de>, Vijaya Krishna Nivarthi <quic_vnivarth@quicinc.com>, 
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Quoting Douglas Anderson (2024-05-23 16:22:13)
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 2bd25afe0d92..9110ac4bdbbf 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -265,8 +265,8 @@ static bool qcom_geni_serial_secondary_active(struct uart_port *uport)
>         return readl(uport->membase + SE_GENI_STATUS) & S_GENI_CMD_ACTIVE;
>  }
>
> -static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> -                               int offset, int field, bool set)
> +static bool qcom_geni_serial_poll_bitfield(struct uart_port *uport,
> +                                          int offset, int field, u32 val)

Can these be unsigned offset and field?

>  {
>         u32 reg;
>         struct qcom_geni_serial_port *port;
> @@ -295,7 +295,7 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>         timeout_us = DIV_ROUND_UP(timeout_us, 10) * 10;
>         while (timeout_us) {
>                 reg = readl(uport->membase + offset);
> -               if ((bool)(reg & field) == set)
> +               if ((reg & field) == val)
>                         return true;
>                 udelay(10);
>                 timeout_us -= 10;
> @@ -303,6 +303,12 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>         return false;
>  }
>
> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> +                                     int offset, int field, bool set)

Can these be unsigned offset and field?

> +{
> +       return qcom_geni_serial_poll_bitfield(uport, offset, field, set ? field : 0);
> +}
> +
>  static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
>  {
>         u32 m_cmd;
> @@ -675,6 +681,31 @@ static void qcom_geni_serial_stop_tx_fifo(struct uart_port *uport)
>         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_TX_FIFO_STATUS, TX_FIFO_WC, 0);
> +
> +       /*
> +        * If we send the cancel immediately after the FIFO reports that it's
> +        * empty then bytes still seem to get lost. From trial and error, it
> +        * appears that a small delay here keeps bytes from being lost and
> +        * there is (apparently) no bit that we can poll instead of this.
> +        * Specifically it can be noted that the sequencer is still "active"
> +        * if it's waiting for us to send it more bytes from the current
> +        * transfer.
> +        */
> +       mdelay(1);

I wonder if the FIFO is in a different 1kb chunk of device memory and so
this needs to be an instruction barrier (isb()) to prevent the cancel
from being executed before or in parallel to the FIFO polling. Hopefully
someone at qcom can confirm this. It looks like SE_GENI_TX_FIFO_STATUS
is 0x800 offset and the cancel is at 0x600 so it looks like it may be
this problem. Device memory doesn't save us even if that has ordered
accesses :(

> +
> +       /*
> +        * 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 the cancel doesn't take, we'll also send an
> +        * abort.
> +        */
>         geni_se_cancel_m_cmd(&port->se);
>         if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>                                                 M_CMD_CANCEL_EN, true)) {