[PATCH v4 0/5] spi: spi-geni-qcom: Fixes / perf improvements

[PATCH v4 0/5] spi: spi-geni-qcom: Fixes / perf improvements

[Douglas Anderson]

This patch series is a new version of the previous patch posted:
  [PATCH v2] spi: spi-geni-qcom: Speculative fix of "nobody cared" about interrupt
  https://lore.kernel.org/r/20200317133653.v2.1.I752ebdcfd5e8bf0de06d66e767b8974932b3620e@changeid

At this point I've done enough tracing to know that there was a real
race in the old code (not just weakly ordered memory problems) and
that should be fixed with the locking patches.

While looking at this driver, I also noticed we weren't properly
noting error interrupts and also weren't actually using our FIFO
effectively, so I fixed those.

The last patch in the series addresses review feedback about dislike
for the "cur_mcmd" state variable.  It also could possibly make
"abort" work ever-so-slightly more reliably.

Changes in v4:
- Drop 'controller' in comment.
- Use Stephen's diagram to explain the race better.

Changes in v3:
- ("spi: spi-geni-qcom: No need for irqsave variant...") new for v3
- Split out some lock cleanup to previous patch.
- Don't need to read IRQ status register inside spinlock.
- Don't check for state CMD_NONE; later patch is removing state var.
- Don't hold the lock for all of setup_fifo_xfer().
- Comment about why it's safe to Ack interrupts at the end.
- Subject/desc changed since race is definitely there.
- ("spi: spi-geni-qcom: Check for error IRQs") new in v3.
- ("spi: spi-geni-qcom: Actually use our FIFO") new in v3.
- ("spi: spi-geni-qcom: Don't keep a local state variable") new in v3.

Changes in v2:
- Detect true spurious interrupt.
- Still return IRQ_NONE for state machine mismatch, but print warn.

Douglas Anderson (5):
  spi: spi-geni-qcom: No need for irqsave variant of spinlock calls
  spi: spi-geni-qcom: Mo' betta locking
  spi: spi-geni-qcom: Check for error IRQs
  spi: spi-geni-qcom: Actually use our FIFO
  spi: spi-geni-qcom: Don't keep a local state variable

 drivers/spi/spi-geni-qcom.c | 120 ++++++++++++++++++++++++------------
 1 file changed, 81 insertions(+), 39 deletions(-)

-- 
2.27.0.290.gba653c62da-goog

[PATCH v4 1/5] spi: spi-geni-qcom: No need for irqsave variant of spinlock calls

[Douglas Anderson]

The driver locks its locks in two places.

In the first usage of the lock the function doing the locking already
has a sleeping call and thus we know we can't be called from interrupt
context.  That means we can use the "spin_lock_irq" variant of the
function.

In the second usage of the lock the function is the interrupt handler
and we know interrupt handlers are called with interrupts disabled.
That means we can use the "spin_lock" variant of the function.

This patch is expected to be a no-op and is just a cleanup / slight
optimization.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v4: None
Changes in v3:
- ("spi: spi-geni-qcom: No need for irqsave variant...") new for v3

Changes in v2: None

 drivers/spi/spi-geni-qcom.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index c3972424af71..c7d2c7e45b3f 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -122,23 +122,23 @@ static void handle_fifo_timeout(struct spi_master *spi,
 				struct spi_message *msg)
 {
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
-	unsigned long time_left, flags;
+	unsigned long time_left;
 	struct geni_se *se = &mas->se;
 
-	spin_lock_irqsave(&mas->lock, flags);
+	spin_lock_irq(&mas->lock);
 	reinit_completion(&mas->xfer_done);
 	mas->cur_mcmd = CMD_CANCEL;
 	geni_se_cancel_m_cmd(se);
 	writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
-	spin_unlock_irqrestore(&mas->lock, flags);
+	spin_unlock_irq(&mas->lock);
 	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
 	if (time_left)
 		return;
 
-	spin_lock_irqsave(&mas->lock, flags);
+	spin_lock_irq(&mas->lock);
 	reinit_completion(&mas->xfer_done);
 	geni_se_abort_m_cmd(se);
-	spin_unlock_irqrestore(&mas->lock, flags);
+	spin_unlock_irq(&mas->lock);
 	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
 	if (!time_left)
 		dev_err(mas->dev, "Failed to cancel/abort m_cmd\n");
@@ -477,12 +477,11 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
 	struct geni_se *se = &mas->se;
 	u32 m_irq;
-	unsigned long flags;
 
 	if (mas->cur_mcmd == CMD_NONE)
 		return IRQ_NONE;
 
-	spin_lock_irqsave(&mas->lock, flags);
+	spin_lock(&mas->lock);
 	m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
 
 	if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
@@ -524,7 +523,7 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 	}
 
 	writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
-	spin_unlock_irqrestore(&mas->lock, flags);
+	spin_unlock(&mas->lock);
 	return IRQ_HANDLED;
 }
 
-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH v4 1/5] spi: spi-geni-qcom: No need for irqsave variant of spinlock calls

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On Thu, Jun 18, 2020 at 08:06:22AM -0700, Douglas Anderson wrote:
> The driver locks its locks in two places.
> 
> In the first usage of the lock the function doing the locking already
> has a sleeping call and thus we know we can't be called from interrupt
> context.  That means we can use the "spin_lock_irq" variant of the
> function.

Please do not submit new versions of already applied patches, please
submit incremental updates to the existing code.  Modifying existing
commits creates problems for other users building on top of those
commits so it's best practice to only change pubished git commits if
absolutely essential.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v4 2/5] spi: spi-geni-qcom: Mo' betta locking

[Douglas Anderson]

If you added a bit of a delay (like a trace_printk) into the ISR for
the spi-geni-qcom driver, you would suddenly start seeing some errors
spit out.  The problem was that, though the ISR itself held a lock,
other parts of the driver didn't always grab the lock.

One example race was this:
  CPU0                                         CPU1
  ----                                         ----
  spi_geni_set_cs()
   mas->cur_mcmd = CMD_CS;
   geni_se_setup_m_cmd(...)
   wait_for_completion_timeout(&xfer_done);
                                              <INTERRUPT>
                                               geni_spi_isr()
                                                complete(&xfer_done);
   <wakeup>
   pm_runtime_put(mas->dev);
  ... // back to SPI core
  spi_geni_transfer_one()
   setup_fifo_xfer()
    mas->cur_mcmd = CMD_XFER;
                                                mas->cur_cmd = CMD_NONE; // bad!
                                                return IRQ_HANDLED;

Let's fix this.  Before we start messing with hardware, we'll grab the
lock to make sure that the IRQ handler from some previous command has
really finished.  We don't need to hold the lock unless we're in a
state where more interrupts can come in, but we at least need to make
sure the previous IRQ is done.  This lock is used exclusively to
prevent the IRQ handler and non-IRQ from stomping on each other.  The
SPI core handles all other mutual exclusion.

As part of this, we change the way that the IRQ handler detects
spurious interrupts.  Previously we checked for our state variable
being set to IRQ_NONE, but that was done outside the spinlock.  We
could move it into the spinlock, but instead let's just change it to
look for the lack of any IRQ status bits being set.  This can be done
outside the lock--the hardware certainly isn't grabbing or looking at
the spinlock when it updates its status register.

It's possible that this will fix real (but very rare) errors seen in
the field that look like:
  irq ...: nobody cared (try booting with the "irqpoll" option)

NOTE: an alternate strategy considered here was to always make the
complete() / spi_finalize_current_transfer() the very last thing in
our IRQ handler.  With such a change you could consider that we could
be "lockless".  In that case, though, we'd have to be very careful w/
memory barriers so we made sure we didn't have any bugs with weakly
ordered memory.  Using spinlocks makes the driver much easier to
understand.

Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v4:
- Drop 'controller' in comment.
- Use Stephen's diagram to explain the race better.

Changes in v3:
- Split out some lock cleanup to previous patch.
- Don't need to read IRQ status register inside spinlock.
- Don't check for state CMD_NONE; later patch is removing state var.
- Don't hold the lock for all of setup_fifo_xfer().
- Comment about why it's safe to Ack interrupts at the end.
- Subject/desc changed since race is definitely there.

Changes in v2:
- Detect true spurious interrupt.
- Still return IRQ_NONE for state machine mismatch, but print warn.

 drivers/spi/spi-geni-qcom.c | 45 ++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index c7d2c7e45b3f..7d022ccb1b6c 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -151,16 +151,18 @@ static void spi_geni_set_cs(struct spi_device *slv, bool set_flag)
 	struct geni_se *se = &mas->se;
 	unsigned long time_left;
 
-	reinit_completion(&mas->xfer_done);
 	pm_runtime_get_sync(mas->dev);
 	if (!(slv->mode & SPI_CS_HIGH))
 		set_flag = !set_flag;
 
+	spin_lock_irq(&mas->lock);
+	reinit_completion(&mas->xfer_done);
 	mas->cur_mcmd = CMD_CS;
 	if (set_flag)
 		geni_se_setup_m_cmd(se, SPI_CS_ASSERT, 0);
 	else
 		geni_se_setup_m_cmd(se, SPI_CS_DEASSERT, 0);
+	spin_unlock_irq(&mas->lock);
 
 	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
 	if (!time_left)
@@ -307,6 +309,21 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 	u32 spi_tx_cfg, len;
 	struct geni_se *se = &mas->se;
 
+	/*
+	 * Ensure that our interrupt handler isn't still running from some
+	 * prior command before we start messing with the hardware behind
+	 * its back.  We don't need to _keep_ the lock here since we're only
+	 * worried about racing with out interrupt handler.  The SPI core
+	 * already handles making sure that we're not trying to do two
+	 * transfers at once or setting a chip select and doing a transfer
+	 * concurrently.
+	 *
+	 * NOTE: we actually _can't_ hold the lock here because possibly we
+	 * might call clk_set_rate() which needs to be able to sleep.
+	 */
+	spin_lock_irq(&mas->lock);
+	spin_unlock_irq(&mas->lock);
+
 	spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG);
 	if (xfer->bits_per_word != mas->cur_bits_per_word) {
 		spi_setup_word_len(mas, mode, xfer->bits_per_word);
@@ -367,6 +384,12 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 	}
 	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
 	mas->cur_mcmd = CMD_XFER;
+
+	/*
+	 * Lock around right before we start the transfer since our
+	 * interrupt could come in at any time now.
+	 */
+	spin_lock_irq(&mas->lock);
 	geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
 
 	/*
@@ -376,6 +399,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 	 */
 	if (m_cmd & SPI_TX_ONLY)
 		writel(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG);
+	spin_unlock_irq(&mas->lock);
 }
 
 static int spi_geni_transfer_one(struct spi_master *spi,
@@ -478,11 +502,11 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 	struct geni_se *se = &mas->se;
 	u32 m_irq;
 
-	if (mas->cur_mcmd == CMD_NONE)
+	m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
+	if (!m_irq)
 		return IRQ_NONE;
 
 	spin_lock(&mas->lock);
-	m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS);
 
 	if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
 		geni_spi_handle_rx(mas);
@@ -522,8 +546,23 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 		complete(&mas->xfer_done);
 	}
 
+	/*
+	 * It's safe or a good idea to Ack all of our our interrupts at the
+	 * end of the function. Specifically:
+	 * - M_CMD_DONE_EN / M_RX_FIFO_LAST_EN: Edge triggered interrupts and
+	 *   clearing Acks. Clearing at the end relies on nobody else having
+	 *   started a new transfer yet or else we could be clearing _their_
+	 *   done bit, but everyone grabs the spinlock before starting a new
+	 *   transfer.
+	 * - M_RX_FIFO_WATERMARK_EN / M_TX_FIFO_WATERMARK_EN: These appear
+	 *   to be "latched level" interrupts so it's important to clear them
+	 *   _after_ you've handled the condition and always safe to do so
+	 *   since they'll re-assert if they're still happening.
+	 */
 	writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR);
+
 	spin_unlock(&mas->lock);
+
 	return IRQ_HANDLED;
 }
 
-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH v4 2/5] spi: spi-geni-qcom: Mo' betta locking

[Stephen Boyd]

Quoting Douglas Anderson (2020-06-18 08:06:23)
> If you added a bit of a delay (like a trace_printk) into the ISR for
> the spi-geni-qcom driver, you would suddenly start seeing some errors
> spit out.  The problem was that, though the ISR itself held a lock,
> other parts of the driver didn't always grab the lock.
> 
> One example race was this:
>   CPU0                                         CPU1
>   ----                                         ----
>   spi_geni_set_cs()
>    mas->cur_mcmd = CMD_CS;
>    geni_se_setup_m_cmd(...)
>    wait_for_completion_timeout(&xfer_done);
>                                               <INTERRUPT>
>                                                geni_spi_isr()
>                                                 complete(&xfer_done);
>    <wakeup>
>    pm_runtime_put(mas->dev);
>   ... // back to SPI core
>   spi_geni_transfer_one()
>    setup_fifo_xfer()
>     mas->cur_mcmd = CMD_XFER;
>                                                 mas->cur_cmd = CMD_NONE; // bad!
>                                                 return IRQ_HANDLED;
> 
> Let's fix this.  Before we start messing with hardware, we'll grab the
> lock to make sure that the IRQ handler from some previous command has
> really finished.  We don't need to hold the lock unless we're in a
> state where more interrupts can come in, but we at least need to make
> sure the previous IRQ is done.  This lock is used exclusively to
> prevent the IRQ handler and non-IRQ from stomping on each other.  The
> SPI core handles all other mutual exclusion.
> 
> As part of this, we change the way that the IRQ handler detects
> spurious interrupts.  Previously we checked for our state variable
> being set to IRQ_NONE, but that was done outside the spinlock.  We
> could move it into the spinlock, but instead let's just change it to
> look for the lack of any IRQ status bits being set.  This can be done
> outside the lock--the hardware certainly isn't grabbing or looking at
> the spinlock when it updates its status register.
> 
> It's possible that this will fix real (but very rare) errors seen in
> the field that look like:
>   irq ...: nobody cared (try booting with the "irqpoll" option)
> 
> NOTE: an alternate strategy considered here was to always make the
> complete() / spi_finalize_current_transfer() the very last thing in
> our IRQ handler.  With such a change you could consider that we could
> be "lockless".  In that case, though, we'd have to be very careful w/
> memory barriers so we made sure we didn't have any bugs with weakly
> ordered memory.  Using spinlocks makes the driver much easier to
> understand.
> 
> Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH v4 3/5] spi: spi-geni-qcom: Check for error IRQs

[Douglas Anderson]

From reading the #defines it seems like we should shout if we ever see
one of these error bits.  Let's do so.  This doesn't do anything
functional except print a yell in the log if the error bits are seen.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v4: None
Changes in v3:
- ("spi: spi-geni-qcom: Check for error IRQs") new in v3.

Changes in v2: None

 drivers/spi/spi-geni-qcom.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 7d022ccb1b6c..11f36d237c77 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -506,6 +506,11 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 	if (!m_irq)
 		return IRQ_NONE;
 
+	if (m_irq & (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN |
+		     M_RX_FIFO_RD_ERR_EN | M_RX_FIFO_WR_ERR_EN |
+		     M_TX_FIFO_RD_ERR_EN | M_TX_FIFO_WR_ERR_EN))
+		dev_warn(mas->dev, "Unexpected IRQ err status %#010x\n", m_irq);
+
 	spin_lock(&mas->lock);
 
 	if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN))
-- 
2.27.0.290.gba653c62da-goog

[PATCH v4 4/5] spi: spi-geni-qcom: Actually use our FIFO

[Douglas Anderson]

The geni hardware has a FIFO that can hold up to 64 bytes (it has 16
entries that can hold 4 bytes each), at least on the two SoCs I tested
(sdm845 and sc7180).  We configured our RX Watermark to 0, which
basically meant we got an interrupt as soon as the first 4 bytes
showed up in the FIFO.  Tracing the IRQ handler showed that we often
only read 4 or 8 bytes per IRQ handler.

I tried setting the RX Watermark to "fifo size - 2" but that just got
me a bunch of overrun errors reported.  Setting it to "fifo size - 3"
seemed to work great, though.  This made me worried that we'd start
getting overruns if we had long interrupt latency, but that doesn't
appear to be the case and delays inserted in the IRQ handler while
using "fifo size - 3" didn't cause any errors.  Presumably there is
some interaction with the poorly-documented RFR (ready for receive)
level means that "fifo size - 3" is the max.  We are the SPI master,
so it makes sense that there would be no problems with overruns, the
master should just stop clocking.

Despite "fifo size - 3" working, I chose "fifo size / 2" (8 entries =
32 bytes) which gives us a little extra time to get to the interrupt
handler and should reduce dead time on the SPI wires.  With this
setting, I often saw the IRQ handler handle 40 bytes but sometimes up
to 56 if we had bad interrupt latency.

Testing by running "flashrom -p ec -r" on a Chromebook saw interrupts
from the SPI driver cut roughly in half.  Time was roughly the same.

Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
Technically this is an improvement, not a fix.  ...but it should be
such a decrease in interrupts that I've tagged it as a Fix nonetheless
as per suggestion during review.

Changes in v4: None
Changes in v3:
- ("spi: spi-geni-qcom: Actually use our FIFO") new in v3.

Changes in v2: None

 drivers/spi/spi-geni-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 11f36d237c77..5b8ca8b93b06 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -285,7 +285,7 @@ static int spi_geni_init(struct spi_geni_master *mas)
 	 * Hardware programming guide suggests to configure
 	 * RX FIFO RFR level to fifo_depth-2.
 	 */
-	geni_se_init(se, 0x0, mas->tx_fifo_depth - 2);
+	geni_se_init(se, mas->tx_fifo_depth / 2, mas->tx_fifo_depth - 2);
 	/* Transmit an entire FIFO worth of data per IRQ */
 	mas->tx_wm = 1;
 	ver = geni_se_get_qup_hw_version(se);
-- 
2.27.0.290.gba653c62da-goog

[PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Douglas Anderson]

The variable "cur_mcmd" kept track of our current state (idle, xfer,
cs, cancel).  We don't really need it, so get rid of it.  Instead:
* Use separate condition variables for "chip select done", "cancel
  done", and "abort done".  This is important so that if a "done"
  comes through (perhaps some previous interrupt finally came through)
  it can't confuse the cancel/abort function.
* Use the "done" interrupt only for when a chip select or transfer is
  done and we can tell the difference by looking at whether "cur_xfer"
  is NULL.

This is mostly a no-op change.  However, it is possible it could fix
an issue where a super delayed interrupt for a cancel command could
have confused our waiting for an abort command.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v4: None
Changes in v3:
- ("spi: spi-geni-qcom: Don't keep a local state variable") new in v3.

Changes in v2: None

 drivers/spi/spi-geni-qcom.c | 55 ++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 5b8ca8b93b06..0c534d151370 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -63,13 +63,6 @@
 #define TIMESTAMP_AFTER		BIT(3)
 #define POST_CMD_DELAY		BIT(4)
 
-enum spi_m_cmd_opcode {
-	CMD_NONE,
-	CMD_XFER,
-	CMD_CS,
-	CMD_CANCEL,
-};
-
 struct spi_geni_master {
 	struct geni_se se;
 	struct device *dev;
@@ -81,10 +74,11 @@ struct spi_geni_master {
 	unsigned int tx_rem_bytes;
 	unsigned int rx_rem_bytes;
 	const struct spi_transfer *cur_xfer;
-	struct completion xfer_done;
+	struct completion cs_done;
+	struct completion cancel_done;
+	struct completion abort_done;
 	unsigned int oversampling;
 	spinlock_t lock;
-	enum spi_m_cmd_opcode cur_mcmd;
 	int irq;
 };
 
@@ -126,20 +120,23 @@ static void handle_fifo_timeout(struct spi_master *spi,
 	struct geni_se *se = &mas->se;
 
 	spin_lock_irq(&mas->lock);
-	reinit_completion(&mas->xfer_done);
-	mas->cur_mcmd = CMD_CANCEL;
-	geni_se_cancel_m_cmd(se);
+	reinit_completion(&mas->cancel_done);
 	writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
+	mas->cur_xfer = NULL;
+	mas->tx_rem_bytes = mas->rx_rem_bytes = 0;
+	geni_se_cancel_m_cmd(se);
 	spin_unlock_irq(&mas->lock);
-	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
+
+	time_left = wait_for_completion_timeout(&mas->cancel_done, HZ);
 	if (time_left)
 		return;
 
 	spin_lock_irq(&mas->lock);
-	reinit_completion(&mas->xfer_done);
+	reinit_completion(&mas->abort_done);
 	geni_se_abort_m_cmd(se);
 	spin_unlock_irq(&mas->lock);
-	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
+
+	time_left = wait_for_completion_timeout(&mas->abort_done, HZ);
 	if (!time_left)
 		dev_err(mas->dev, "Failed to cancel/abort m_cmd\n");
 }
@@ -156,15 +153,14 @@ static void spi_geni_set_cs(struct spi_device *slv, bool set_flag)
 		set_flag = !set_flag;
 
 	spin_lock_irq(&mas->lock);
-	reinit_completion(&mas->xfer_done);
-	mas->cur_mcmd = CMD_CS;
+	reinit_completion(&mas->cs_done);
 	if (set_flag)
 		geni_se_setup_m_cmd(se, SPI_CS_ASSERT, 0);
 	else
 		geni_se_setup_m_cmd(se, SPI_CS_DEASSERT, 0);
 	spin_unlock_irq(&mas->lock);
 
-	time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);
+	time_left = wait_for_completion_timeout(&mas->cs_done, HZ);
 	if (!time_left)
 		handle_fifo_timeout(spi, NULL);
 
@@ -383,7 +379,6 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 		mas->rx_rem_bytes = xfer->len;
 	}
 	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
-	mas->cur_mcmd = CMD_XFER;
 
 	/*
 	 * Lock around right before we start the transfer since our
@@ -520,11 +515,13 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 		geni_spi_handle_tx(mas);
 
 	if (m_irq & M_CMD_DONE_EN) {
-		if (mas->cur_mcmd == CMD_XFER)
+		if (mas->cur_xfer) {
 			spi_finalize_current_transfer(spi);
-		else if (mas->cur_mcmd == CMD_CS)
-			complete(&mas->xfer_done);
-		mas->cur_mcmd = CMD_NONE;
+			mas->cur_xfer = NULL;
+		} else {
+			complete(&mas->cs_done);
+		}
+
 		/*
 		 * If this happens, then a CMD_DONE came before all the Tx
 		 * buffer bytes were sent out. This is unusual, log this
@@ -546,10 +543,10 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 				mas->rx_rem_bytes, mas->cur_bits_per_word);
 	}
 
-	if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN)) {
-		mas->cur_mcmd = CMD_NONE;
-		complete(&mas->xfer_done);
-	}
+	if (m_irq & M_CMD_CANCEL_EN)
+		complete(&mas->cancel_done);
+	if (m_irq & M_CMD_ABORT_EN)
+		complete(&mas->abort_done);
 
 	/*
 	 * It's safe or a good idea to Ack all of our our interrupts at the
@@ -617,7 +614,9 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spi->handle_err = handle_fifo_timeout;
 	spi->set_cs = spi_geni_set_cs;
 
-	init_completion(&mas->xfer_done);
+	init_completion(&mas->cs_done);
+	init_completion(&mas->cancel_done);
+	init_completion(&mas->abort_done);
 	spin_lock_init(&mas->lock);
 	pm_runtime_enable(dev);
 
-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Stephen Boyd]

Quoting Douglas Anderson (2020-06-18 08:06:26)
> The variable "cur_mcmd" kept track of our current state (idle, xfer,
> cs, cancel).  We don't really need it, so get rid of it.  Instead:
> * Use separate condition variables for "chip select done", "cancel
>   done", and "abort done".  This is important so that if a "done"
>   comes through (perhaps some previous interrupt finally came through)
>   it can't confuse the cancel/abort function.
> * Use the "done" interrupt only for when a chip select or transfer is
>   done and we can tell the difference by looking at whether "cur_xfer"
>   is NULL.
> 
> This is mostly a no-op change.  However, it is possible it could fix
> an issue where a super delayed interrupt for a cancel command could
> have confused our waiting for an abort command.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Stephen Boyd]

Quoting Douglas Anderson (2020-06-18 08:06:26)
> @@ -126,20 +120,23 @@ static void handle_fifo_timeout(struct spi_master *spi,
>         struct geni_se *se = &mas->se;
>  
>         spin_lock_irq(&mas->lock);
> -       reinit_completion(&mas->xfer_done);
> -       mas->cur_mcmd = CMD_CANCEL;
> -       geni_se_cancel_m_cmd(se);
> +       reinit_completion(&mas->cancel_done);
>         writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> +       mas->cur_xfer = NULL;

BTW, is this necessary? It's subtlely placed here without a comment why.

> +       mas->tx_rem_bytes = mas->rx_rem_bytes = 0;
> +       geni_se_cancel_m_cmd(se);
>         spin_unlock_irq(&mas->lock);
> -       time_left = wait_for_completion_timeout(&mas->xfer_done, HZ);

Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 11:05 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Douglas Anderson (2020-06-18 08:06:26)
> > @@ -126,20 +120,23 @@ static void handle_fifo_timeout(struct spi_master *spi,
> >         struct geni_se *se = &mas->se;
> >
> >         spin_lock_irq(&mas->lock);
> > -       reinit_completion(&mas->xfer_done);
> > -       mas->cur_mcmd = CMD_CANCEL;
> > -       geni_se_cancel_m_cmd(se);
> > +       reinit_completion(&mas->cancel_done);
> >         writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> > +       mas->cur_xfer = NULL;
>
> BTW, is this necessary? It's subtlely placed here without a comment why.

I believe so.  Now that we don't have the "cur_mcmd" we rely on
cur_xfer being NULL to tell the difference between a "done" for chip
select vs. a "done" for transfer.

* When we start a transfer we set "cur_xfer" to a non-NULL pointer.
When the transfer finishes we set it to NULL again.

* When we start a chip select transfer we _don't_ explicitly set it to
NULL because it should already be NULL.

* When we are aborting a transfer we need to NULL so we can handle the
chip select that will come next.

I suppose it's possible that we could get by without without NULLing
it because I believe when the "abort" IRQ finally fires then it will
include a "DONE" and that would presumably NULL it out.  ...but I
guess if both the cancel and abort timed out and no IRQ ever fired
then nothing would have NULLed it and the next chip select would be
confused.

Prior to getting rid of "cur_mcmd" this all wasn't needed because
"cur_xfer" was only ever looked at if "cur_mcmd" was set to
"CMD_XFER".


One part of my change that is technically not related to the removal
of "cur_mcmd" is the part where I do "mas->tx_rem_bytes =
mas->rx_rem_bytes = 0;".  I can split that as a separate change if you
want but it seemed fine to just clean up this extra bit of state here.

-Doug

Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Stephen Boyd]

Quoting Doug Anderson (2020-06-18 13:09:47)
> On Thu, Jun 18, 2020 at 11:05 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Douglas Anderson (2020-06-18 08:06:26)
> > > @@ -126,20 +120,23 @@ static void handle_fifo_timeout(struct spi_master *spi,
> > >         struct geni_se *se = &mas->se;
> > >
> > >         spin_lock_irq(&mas->lock);
> > > -       reinit_completion(&mas->xfer_done);
> > > -       mas->cur_mcmd = CMD_CANCEL;
> > > -       geni_se_cancel_m_cmd(se);
> > > +       reinit_completion(&mas->cancel_done);
> > >         writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> > > +       mas->cur_xfer = NULL;
> >
> > BTW, is this necessary? It's subtlely placed here without a comment why.
> 
> I believe so.  Now that we don't have the "cur_mcmd" we rely on
> cur_xfer being NULL to tell the difference between a "done" for chip
> select vs. a "done" for transfer.
> 
> * When we start a transfer we set "cur_xfer" to a non-NULL pointer.
> When the transfer finishes we set it to NULL again.
> 
> * When we start a chip select transfer we _don't_ explicitly set it to
> NULL because it should already be NULL.
> 
> * When we are aborting a transfer we need to NULL so we can handle the
> chip select that will come next.
> 
> I suppose it's possible that we could get by without without NULLing
> it because I believe when the "abort" IRQ finally fires then it will
> include a "DONE" and that would presumably NULL it out.  ...but I
> guess if both the cancel and abort timed out and no IRQ ever fired
> then nothing would have NULLed it and the next chip select would be
> confused.

I was going to say that we should set it NULL when starting CS but that
is not as important as clearing it out when a cancel/abort is processing
so that a stale transfer isn't kept around.

> 
> Prior to getting rid of "cur_mcmd" this all wasn't needed because
> "cur_xfer" was only ever looked at if "cur_mcmd" was set to
> "CMD_XFER".
> 
> 
> One part of my change that is technically not related to the removal
> of "cur_mcmd" is the part where I do "mas->tx_rem_bytes =
> mas->rx_rem_bytes = 0;".  I can split that as a separate change if you
> want but it seemed fine to just clean up this extra bit of state here.
> 

How about a comment like this?

-----8<----
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index d8f03ffb8594..670f83793aa4 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -121,6 +121,10 @@ static void handle_fifo_timeout(struct spi_master *spi,
 	spin_lock_irq(&mas->lock);
 	reinit_completion(&mas->cancel_done);
 	writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
+	/*
+	 * Make sure we don't finalize a spi transfer that timed out but
+	 * came in while cancelling.
+	 */
 	mas->cur_xfer = NULL;
 	mas->tx_rem_bytes = mas->rx_rem_bytes = 0;
 	geni_se_cancel_m_cmd(se);

Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 2:52 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-06-18 13:09:47)
> > On Thu, Jun 18, 2020 at 11:05 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Douglas Anderson (2020-06-18 08:06:26)
> > > > @@ -126,20 +120,23 @@ static void handle_fifo_timeout(struct spi_master *spi,
> > > >         struct geni_se *se = &mas->se;
> > > >
> > > >         spin_lock_irq(&mas->lock);
> > > > -       reinit_completion(&mas->xfer_done);
> > > > -       mas->cur_mcmd = CMD_CANCEL;
> > > > -       geni_se_cancel_m_cmd(se);
> > > > +       reinit_completion(&mas->cancel_done);
> > > >         writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> > > > +       mas->cur_xfer = NULL;
> > >
> > > BTW, is this necessary? It's subtlely placed here without a comment why.
> >
> > I believe so.  Now that we don't have the "cur_mcmd" we rely on
> > cur_xfer being NULL to tell the difference between a "done" for chip
> > select vs. a "done" for transfer.
> >
> > * When we start a transfer we set "cur_xfer" to a non-NULL pointer.
> > When the transfer finishes we set it to NULL again.
> >
> > * When we start a chip select transfer we _don't_ explicitly set it to
> > NULL because it should already be NULL.
> >
> > * When we are aborting a transfer we need to NULL so we can handle the
> > chip select that will come next.
> >
> > I suppose it's possible that we could get by without without NULLing
> > it because I believe when the "abort" IRQ finally fires then it will
> > include a "DONE" and that would presumably NULL it out.  ...but I
> > guess if both the cancel and abort timed out and no IRQ ever fired
> > then nothing would have NULLed it and the next chip select would be
> > confused.
>
> I was going to say that we should set it NULL when starting CS but that
> is not as important as clearing it out when a cancel/abort is processing
> so that a stale transfer isn't kept around.
>
> >
> > Prior to getting rid of "cur_mcmd" this all wasn't needed because
> > "cur_xfer" was only ever looked at if "cur_mcmd" was set to
> > "CMD_XFER".
> >
> >
> > One part of my change that is technically not related to the removal
> > of "cur_mcmd" is the part where I do "mas->tx_rem_bytes =
> > mas->rx_rem_bytes = 0;".  I can split that as a separate change if you
> > want but it seemed fine to just clean up this extra bit of state here.
> >
>
> How about a comment like this?
>
> -----8<----
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index d8f03ffb8594..670f83793aa4 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -121,6 +121,10 @@ static void handle_fifo_timeout(struct spi_master *spi,
>         spin_lock_irq(&mas->lock);
>         reinit_completion(&mas->cancel_done);
>         writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> +       /*
> +        * Make sure we don't finalize a spi transfer that timed out but
> +        * came in while cancelling.
> +        */
>         mas->cur_xfer = NULL;
>         mas->tx_rem_bytes = mas->rx_rem_bytes = 0;
>         geni_se_cancel_m_cmd(se);

Sure.  It gets the point across, though
spi_finalize_current_transfer() is actually pretty harmless if you
call it while cancelling.  It just calls a completion.  I'd rather say
something like "If we're here because the SPI controller was calling
handle_err() then the transfer is done and we shouldn't hold onto it
anymore".

-Doug

Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Stephen Boyd]

Quoting Doug Anderson (2020-06-18 15:00:10)
> On Thu, Jun 18, 2020 at 2:52 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > -----8<----
> > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > index d8f03ffb8594..670f83793aa4 100644
> > --- a/drivers/spi/spi-geni-qcom.c
> > +++ b/drivers/spi/spi-geni-qcom.c
> > @@ -121,6 +121,10 @@ static void handle_fifo_timeout(struct spi_master *spi,
> >         spin_lock_irq(&mas->lock);
> >         reinit_completion(&mas->cancel_done);
> >         writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> > +       /*
> > +        * Make sure we don't finalize a spi transfer that timed out but
> > +        * came in while cancelling.
> > +        */
> >         mas->cur_xfer = NULL;
> >         mas->tx_rem_bytes = mas->rx_rem_bytes = 0;
> >         geni_se_cancel_m_cmd(se);
> 
> Sure.  It gets the point across, though
> spi_finalize_current_transfer() is actually pretty harmless if you
> call it while cancelling.  It just calls a completion.  I'd rather say
> something like "If we're here because the SPI controller was calling
> handle_err() then the transfer is done and we shouldn't hold onto it
> anymore".
> 

Agreed it's mostly harmless. I thought the concern was that 'cur_xfer'
may reference a freed piece of memory so it's best to remove ownership
of the pointer from here so that the irq handler doesn't try to finalize
a transfer that may no longer exist. "Shouldn't hold onto it anymore"
doesn't tell us why it shouldn't be held onto, leaving it to the reader
to figure out why, which isn't good.

Re: [PATCH v4 5/5] spi: spi-geni-qcom: Don't keep a local state variable

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 4:37 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Doug Anderson (2020-06-18 15:00:10)
> > On Thu, Jun 18, 2020 at 2:52 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > -----8<----
> > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > > index d8f03ffb8594..670f83793aa4 100644
> > > --- a/drivers/spi/spi-geni-qcom.c
> > > +++ b/drivers/spi/spi-geni-qcom.c
> > > @@ -121,6 +121,10 @@ static void handle_fifo_timeout(struct spi_master *spi,
> > >         spin_lock_irq(&mas->lock);
> > >         reinit_completion(&mas->cancel_done);
> > >         writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> > > +       /*
> > > +        * Make sure we don't finalize a spi transfer that timed out but
> > > +        * came in while cancelling.
> > > +        */
> > >         mas->cur_xfer = NULL;
> > >         mas->tx_rem_bytes = mas->rx_rem_bytes = 0;
> > >         geni_se_cancel_m_cmd(se);
> >
> > Sure.  It gets the point across, though
> > spi_finalize_current_transfer() is actually pretty harmless if you
> > call it while cancelling.  It just calls a completion.  I'd rather say
> > something like "If we're here because the SPI controller was calling
> > handle_err() then the transfer is done and we shouldn't hold onto it
> > anymore".
> >
>
> Agreed it's mostly harmless. I thought the concern was that 'cur_xfer'
> may reference a freed piece of memory so it's best to remove ownership
> of the pointer from here so that the irq handler doesn't try to finalize
> a transfer that may no longer exist. "Shouldn't hold onto it anymore"
> doesn't tell us why it shouldn't be held onto, leaving it to the reader
> to figure out why, which isn't good.

Right.  The point is that 'cur_xfer' isn't valid anymore after
handle_err() finishes so we shouldn't hold the pointer.  I'm OK with
your wording and am happy if Mark squashes it when he applies or I can
send out a new version soon.

-Doug

[PATCH 6/5] spi: spi-geni-qcom: Simplify setup_fifo_xfer()

[Stephen Boyd]

The definition of SPI_FULL_DUPLEX (3) is really SPI_TX_ONLY (1) ORed
with SPI_RX_ONLY (2). Let's drop the define and simplify the code here a
bit by collapsing the setting of 'm_cmd' into conditions that are the
same.

This is a non-functional change, just cleanup to consolidate code.

Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/spi/spi-geni-qcom.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 636c3da15db0..670f83793aa4 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -51,7 +51,6 @@
 /* M_CMD OP codes for SPI */
 #define SPI_TX_ONLY		1
 #define SPI_RX_ONLY		2
-#define SPI_FULL_DUPLEX		3
 #define SPI_TX_RX		7
 #define SPI_CS_ASSERT		8
 #define SPI_CS_DEASSERT		9
@@ -357,12 +356,6 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 
 	mas->tx_rem_bytes = 0;
 	mas->rx_rem_bytes = 0;
-	if (xfer->tx_buf && xfer->rx_buf)
-		m_cmd = SPI_FULL_DUPLEX;
-	else if (xfer->tx_buf)
-		m_cmd = SPI_TX_ONLY;
-	else if (xfer->rx_buf)
-		m_cmd = SPI_RX_ONLY;
 
 	spi_tx_cfg &= ~CS_TOGGLE;
 
@@ -373,12 +366,14 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 	len &= TRANS_LEN_MSK;
 
 	mas->cur_xfer = xfer;
-	if (m_cmd & SPI_TX_ONLY) {
+	if (xfer->tx_buf) {
+		m_cmd |= SPI_TX_ONLY;
 		mas->tx_rem_bytes = xfer->len;
 		writel(len, se->base + SE_SPI_TX_TRANS_LEN);
 	}
 
-	if (m_cmd & SPI_RX_ONLY) {
+	if (xfer->rx_buf) {
+		m_cmd |= SPI_RX_ONLY;
 		writel(len, se->base + SE_SPI_RX_TRANS_LEN);
 		mas->rx_rem_bytes = xfer->len;
 	}
-- 
Sent by a computer, using git, on the internet

Re: [PATCH 6/5] spi: spi-geni-qcom: Simplify setup_fifo_xfer()

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 4:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> The definition of SPI_FULL_DUPLEX (3) is really SPI_TX_ONLY (1) ORed
> with SPI_RX_ONLY (2). Let's drop the define and simplify the code here a
> bit by collapsing the setting of 'm_cmd' into conditions that are the
> same.
>
> This is a non-functional change, just cleanup to consolidate code.
>
> Cc: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/spi/spi-geni-qcom.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 636c3da15db0..670f83793aa4 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -51,7 +51,6 @@
>  /* M_CMD OP codes for SPI */
>  #define SPI_TX_ONLY            1
>  #define SPI_RX_ONLY            2
> -#define SPI_FULL_DUPLEX                3
>  #define SPI_TX_RX              7
>  #define SPI_CS_ASSERT          8
>  #define SPI_CS_DEASSERT                9
> @@ -357,12 +356,6 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>
>         mas->tx_rem_bytes = 0;
>         mas->rx_rem_bytes = 0;
> -       if (xfer->tx_buf && xfer->rx_buf)
> -               m_cmd = SPI_FULL_DUPLEX;
> -       else if (xfer->tx_buf)
> -               m_cmd = SPI_TX_ONLY;
> -       else if (xfer->rx_buf)
> -               m_cmd = SPI_RX_ONLY;
>
>         spi_tx_cfg &= ~CS_TOGGLE;
>
> @@ -373,12 +366,14 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>         len &= TRANS_LEN_MSK;
>
>         mas->cur_xfer = xfer;
> -       if (m_cmd & SPI_TX_ONLY) {
> +       if (xfer->tx_buf) {
> +               m_cmd |= SPI_TX_ONLY;
>                 mas->tx_rem_bytes = xfer->len;
>                 writel(len, se->base + SE_SPI_TX_TRANS_LEN);
>         }
>
> -       if (m_cmd & SPI_RX_ONLY) {
> +       if (xfer->rx_buf) {
> +               m_cmd |= SPI_RX_ONLY;

If you're going to touch this, could you change "SPI_TX_ONLY" to
"SPI_TX_ENABLED" and "SPI_RX_ONLY" to 'SPI_RX_ENABLED".  It felt
really weird to me that if you had full duplex you were setting
"RX_ONLY" and "TX_ONLY".

Other than that, your change is nice and cleans things up a bit, so
even if you don't do the extra cleanup:

Reviewed-by: Douglas Anderson <dianders@chromium.org>



-Doug

Re: [PATCH 6/5] spi: spi-geni-qcom: Simplify setup_fifo_xfer()

[Stephen Boyd]

Quoting Doug Anderson (2020-06-18 17:40:59)
> On Thu, Jun 18, 2020 at 4:40 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > @@ -373,12 +366,14 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
> >         len &= TRANS_LEN_MSK;
> >
> >         mas->cur_xfer = xfer;
> > -       if (m_cmd & SPI_TX_ONLY) {
> > +       if (xfer->tx_buf) {
> > +               m_cmd |= SPI_TX_ONLY;
> >                 mas->tx_rem_bytes = xfer->len;
> >                 writel(len, se->base + SE_SPI_TX_TRANS_LEN);
> >         }
> >
> > -       if (m_cmd & SPI_RX_ONLY) {
> > +       if (xfer->rx_buf) {
> > +               m_cmd |= SPI_RX_ONLY;
> 
> If you're going to touch this, could you change "SPI_TX_ONLY" to
> "SPI_TX_ENABLED" and "SPI_RX_ONLY" to 'SPI_RX_ENABLED".  It felt
> really weird to me that if you had full duplex you were setting
> "RX_ONLY" and "TX_ONLY".

I agree, except in the register documentation it is called "TX only",
"RX only" and "Full-Duplex". So I'd rather leave it alone.

> 
> Other than that, your change is nice and cleans things up a bit, so
> even if you don't do the extra cleanup:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> 

Thanks.

Re: [PATCH 6/5] spi: spi-geni-qcom: Simplify setup_fifo_xfer()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Thu, Jun 18, 2020 at 04:39:58PM -0700, Stephen Boyd wrote:
> The definition of SPI_FULL_DUPLEX (3) is really SPI_TX_ONLY (1) ORed
> with SPI_RX_ONLY (2). Let's drop the define and simplify the code here a
> bit by collapsing the setting of 'm_cmd' into conditions that are the
> same.

Please don't add extra patches after someone else's series like this, it
makes things harder to follow and really confuses tooling which tries to
parse serieses off the list.  Just send a separate series.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stephen Boyd]

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@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 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);
 		} else {
 			complete(&mas->cs_done);
 		}
-
-		/*
-		 * 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);
 	}
 
 	if (m_irq & M_CMD_CANCEL_EN)
-- 
Sent by a computer, using git, on the internet

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

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 4:40 PM Stephen Boyd <swboyd@chromium.org> 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@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  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@chromium.org>

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Thu, Jun 18, 2020 at 04:39:59PM -0700, Stephen Boyd 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

This doesn't apply against current code, please check and resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 0/5] spi: spi-geni-qcom: Fixes / perf improvements

[Mark Brown]

On Thu, 18 Jun 2020 08:06:21 -0700, Douglas Anderson wrote:
> This patch series is a new version of the previous patch posted:
>   [PATCH v2] spi: spi-geni-qcom: Speculative fix of "nobody cared" about interrupt
>   https://lore.kernel.org/r/20200317133653.v2.1.I752ebdcfd5e8bf0de06d66e767b8974932b3620e@changeid
> 
> At this point I've done enough tracing to know that there was a real
> race in the old code (not just weakly ordered memory problems) and
> that should be fixed with the locking patches.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/4] spi: spi-geni-qcom: Mo' betta locking
      commit: 2ee471a1e28ec79fbfcdc8900ed0ed74132b0efe
[2/4] spi: spi-geni-qcom: Check for error IRQs
      commit: e191a082d764e80a36c198da61fbf2851ebf425a
[3/4] spi: spi-geni-qcom: Actually use our FIFO
      commit: 902481a78ee4173926dc59f060526dee21aeb7a8
[4/4] spi: spi-geni-qcom: Don't keep a local state variable
      commit: 7ba9bdcb91f694b0eaf486a825afd9c2d99532b7

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v4 0/5] spi: spi-geni-qcom: Fixes / perf improvements

[Mark Brown]

On Thu, 18 Jun 2020 08:06:21 -0700, Douglas Anderson wrote:
> This patch series is a new version of the previous patch posted:
>   [PATCH v2] spi: spi-geni-qcom: Speculative fix of "nobody cared" about interrupt
>   https://lore.kernel.org/r/20200317133653.v2.1.I752ebdcfd5e8bf0de06d66e767b8974932b3620e@changeid
> 
> At this point I've done enough tracing to know that there was a real
> race in the old code (not just weakly ordered memory problems) and
> that should be fixed with the locking patches.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/2] spi: spi-geni-qcom: Simplify setup_fifo_xfer()
      commit: 0d574c6b59c6ac0ae5b581a2ffb813d446a50a3d
[2/2] spi: spi-geni-qcom: Don't set {tx,rx}_rem_bytes unnecessarily
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark