Re: [PATCH 7/5] spi: spi-geni-qcom: Don't set {tx,rx}_rem_bytes unnecessarily

From: Doug Anderson
Date: Thu Jun 18 2020 - 20:40:30 EST


Hi,

On Thu, Jun 18, 2020 at 4:40 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> We only need to test for these counters being non-zero when we see the
> end of a transfer. If we're doing a CS change then they will already be
> zero. This implies that we don't need to set these to 0 if we're
> cancelling an in flight transfer too, because we only care to test these
> counters when the 'DONE' bit is set in the hardware and we've set them
> to non-zero for a transfer.
>
> This is a non-functional change, just cleanup to consolidate code.
>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
> drivers/spi/spi-geni-qcom.c | 42 ++++++++++++++++++-------------------
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 670f83793aa4..828cfc988a3f 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -126,7 +126,6 @@ static void handle_fifo_timeout(struct spi_master *spi,
> * came in while cancelling.
> */
> mas->cur_xfer = NULL;
> - mas->tx_rem_bytes = mas->rx_rem_bytes = 0;
> geni_se_cancel_m_cmd(se);
> spin_unlock_irq(&mas->lock);
>
> @@ -517,29 +516,30 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
> if (mas->cur_xfer) {
> spi_finalize_current_transfer(spi);
> mas->cur_xfer = NULL;
> + /*
> + * If this happens, then a CMD_DONE came before all the
> + * Tx buffer bytes were sent out. This is unusual, log
> + * this condition and disable the WM interrupt to
> + * prevent the system from stalling due an interrupt
> + * storm.
> + *
> + * If this happens when all Rx bytes haven't been
> + * received, log the condition. The only known time
> + * this can happen is if bits_per_word != 8 and some
> + * registers that expect xfer lengths in num spi_words
> + * weren't written correctly.
> + */
> + if (mas->tx_rem_bytes) {
> + writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> + dev_err(mas->dev, "Premature done. tx_rem = %d bpw%d\n",
> + mas->tx_rem_bytes, mas->cur_bits_per_word);
> + }
> + if (mas->rx_rem_bytes)
> + dev_err(mas->dev, "Premature done. rx_rem = %d bpw%d\n",
> + mas->rx_rem_bytes, mas->cur_bits_per_word);

...or we just remove these extra error-checks totally. ...but if we
want to keep them:

Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>