Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] i2c: i2c-qcom-geni: More properly fix the DMA race
@ 2020-10-13 21:25 Douglas Anderson
  2020-10-13 21:25 ` [PATCH v2 1/3] soc: qcom: geni: More properly switch to DMA mode Douglas Anderson
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Douglas Anderson @ 2020-10-13 21:25 UTC (permalink / raw)
  To: Bjorn Andersson, Wolfram Sang, Akash Asthana
  Cc: linux-i2c, Roja Rani Yarubandi, Stephen Boyd, Dmitry Baryshkov,
	linux-arm-msm, Douglas Anderson, Andy Gross, Girish Mahadevan,
	Karthikeyan Ramasubramanian, Mukesh Kumar Savaliya, Sagar Dharia,
	linux-kernel

Previously I landed commit 02b9aec59243 ("i2c: i2c-qcom-geni: Fix DMA
transfer race") to fix a race we were seeing.  While that most
definitely fixed the race we were seeing, it looks like it causes
problems in the TX path, which we didn't stress test until we started
trying to update firmware on devices.

Let's revert that patch and try another way: fix the original problem
by disabling the interrupts that aren't relevant to DMA transfers.
Now we can stress both TX and RX cases and see no problems.  I also
can't find any place to put an msleep() that causes problems anymore.

Since this problem only affects i2c, I'm hoping for an Ack from Bjorn
and then all these patches can go through the i2c tree.  However, if
maintainers want to work a different way out to land that's OK too.

NOTE: the 3rd patch in the series could certianly be squashed with
patch #1 or I could re-order / rejigger.  To me it seemed like a good
idea to first fix the probelm (and make the two functions as much of
an inverse as possible) and later try to clean things up.  Yell if you
want something different.

Changes in v2:
- Consistently use "val_old" to keep track of old value.
- Add comments about why UART is special.

Douglas Anderson (3):
  soc: qcom: geni: More properly switch to DMA mode
  Revert "i2c: i2c-qcom-geni: Fix DMA transfer race"
  soc: qcom: geni: Optimize/comment select fifo/dma mode

 drivers/i2c/busses/i2c-qcom-geni.c |  6 ++--
 drivers/soc/qcom/qcom-geni-se.c    | 55 ++++++++++++++++++++++--------
 2 files changed, 43 insertions(+), 18 deletions(-)

-- 
2.28.0.1011.ga647a8990f-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] soc: qcom: geni: More properly switch to DMA mode
  2020-10-13 21:25 [PATCH v2 0/3] i2c: i2c-qcom-geni: More properly fix the DMA race Douglas Anderson
@ 2020-10-13 21:25 ` Douglas Anderson
  2020-10-13 21:25 ` [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race" Douglas Anderson
  2020-10-13 21:25 ` [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode Douglas Anderson
  2 siblings, 0 replies; 9+ messages in thread
From: Douglas Anderson @ 2020-10-13 21:25 UTC (permalink / raw)
  To: Bjorn Andersson, Wolfram Sang, Akash Asthana
  Cc: linux-i2c, Roja Rani Yarubandi, Stephen Boyd, Dmitry Baryshkov,
	linux-arm-msm, Douglas Anderson, Andy Gross, Girish Mahadevan,
	Karthikeyan Ramasubramanian, Mukesh Kumar Savaliya, Sagar Dharia,
	linux-kernel

On geni-i2c transfers using DMA, it was seen that if you program the
command (I2C_READ) before calling geni_se_rx_dma_prep() that it could
cause interrupts to fire.  If we get unlucky, these interrupts can
just keep firing (and not be handled) blocking further progress and
hanging the system.

In commit 02b9aec59243 ("i2c: i2c-qcom-geni: Fix DMA transfer race")
we avoided that by making sure we didn't program the command until
after geni_se_rx_dma_prep() was called.  While that avoided the
problems, it also turns out to be invalid.  At least in the TX case we
started seeing sporadic corrupted transfers.  This is easily seen by
adding an msleep() between the DMA prep and the writing of the
command, which makes the problem worse.  That means we need to revert
that commit and find another way to fix the bogus IRQs.

Specifically, after reverting commit 02b9aec59243 ("i2c:
i2c-qcom-geni: Fix DMA transfer race"), I put some traces in.  I found
that the when the interrupts were firing like crazy:
- "m_stat" had bits for M_RX_IRQ_EN, M_RX_FIFO_WATERMARK_EN set.
- "dma" was set.

Further debugging showed that I could make the problem happen more
reliably by adding an "msleep(1)" any time after geni_se_setup_m_cmd()
ran up until geni_se_rx_dma_prep() programmed the length.

A rather simple fix is to change geni_se_select_dma_mode() so it's a
true inverse of geni_se_select_fifo_mode() and disables all the FIFO
related interrupts.  Now the problematic interrupts can't fire and we
can program things in the correct order without worrying.

As part of this, let's also change the writel_relaxed() in the prepare
function to a writel() so that our DMA is guaranteed to be prepared
now that we can't rely on geni_se_setup_m_cmd()'s writel().

NOTE: the only current user of GENI_SE_DMA in mainline is i2c.

Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
Fixes: 02b9aec59243 ("i2c: i2c-qcom-geni: Fix DMA transfer race")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

(no changes since v1)

 drivers/soc/qcom/qcom-geni-se.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index d0e4f520cff8..751a49f6534f 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -289,10 +289,23 @@ static void geni_se_select_fifo_mode(struct geni_se *se)
 
 static void geni_se_select_dma_mode(struct geni_se *se)
 {
+	u32 proto = geni_se_read_proto(se);
 	u32 val;
 
 	geni_se_irq_clear(se);
 
+	val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
+	if (proto != GENI_SE_UART) {
+		val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN);
+		val &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
+	}
+	writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
+
+	val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
+	if (proto != GENI_SE_UART)
+		val &= ~S_CMD_DONE_EN;
+	writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
+
 	val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
 	val |= GENI_DMA_MODE_EN;
 	writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
@@ -651,7 +664,7 @@ int geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len,
 	writel_relaxed(lower_32_bits(*iova), se->base + SE_DMA_TX_PTR_L);
 	writel_relaxed(upper_32_bits(*iova), se->base + SE_DMA_TX_PTR_H);
 	writel_relaxed(GENI_SE_DMA_EOT_BUF, se->base + SE_DMA_TX_ATTR);
-	writel_relaxed(len, se->base + SE_DMA_TX_LEN);
+	writel(len, se->base + SE_DMA_TX_LEN);
 	return 0;
 }
 EXPORT_SYMBOL(geni_se_tx_dma_prep);
@@ -688,7 +701,7 @@ int geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len,
 	writel_relaxed(upper_32_bits(*iova), se->base + SE_DMA_RX_PTR_H);
 	/* RX does not have EOT buffer type bit. So just reset RX_ATTR */
 	writel_relaxed(0, se->base + SE_DMA_RX_ATTR);
-	writel_relaxed(len, se->base + SE_DMA_RX_LEN);
+	writel(len, se->base + SE_DMA_RX_LEN);
 	return 0;
 }
 EXPORT_SYMBOL(geni_se_rx_dma_prep);
-- 
2.28.0.1011.ga647a8990f-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race"
  2020-10-13 21:25 [PATCH v2 0/3] i2c: i2c-qcom-geni: More properly fix the DMA race Douglas Anderson
  2020-10-13 21:25 ` [PATCH v2 1/3] soc: qcom: geni: More properly switch to DMA mode Douglas Anderson
@ 2020-10-13 21:25 ` Douglas Anderson
  2020-10-26 15:05   ` Bjorn Andersson
  2020-10-13 21:25 ` [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode Douglas Anderson
  2 siblings, 1 reply; 9+ messages in thread
From: Douglas Anderson @ 2020-10-13 21:25 UTC (permalink / raw)
  To: Bjorn Andersson, Wolfram Sang, Akash Asthana
  Cc: linux-i2c, Roja Rani Yarubandi, Stephen Boyd, Dmitry Baryshkov,
	linux-arm-msm, Douglas Anderson, Andy Gross, Mukesh Savaliya,
	linux-kernel

This reverts commit 02b9aec59243c6240fc42884acc958602146ddf6.

As talked about in the patch ("soc: qcom: geni: More properly switch
to DMA mode"), swapping the order of geni_se_setup_m_cmd() and
geni_se_xx_dma_prep() can sometimes cause corrupted transfers.  Thus
we traded one problem for another.  Now that we've debugged the
problem further and fixed the geni helper functions to more disable
FIFO interrupts when we move to DMA mode we can revert it and end up
with (hopefully) zero problems!

To be explicit, the patch ("soc: qcom: geni: More properly switch
to DMA mode") is a prerequisite for this one.

Fixes: 02b9aec59243 ("i2c: i2c-qcom-geni: Fix DMA transfer race")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---

(no changes since v1)

 drivers/i2c/busses/i2c-qcom-geni.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index dead5db3315a..32b2a9921b14 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -367,6 +367,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 		geni_se_select_mode(se, GENI_SE_FIFO);
 
 	writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
+	geni_se_setup_m_cmd(se, I2C_READ, m_param);
 
 	if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
 		geni_se_select_mode(se, GENI_SE_FIFO);
@@ -374,8 +375,6 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 		dma_buf = NULL;
 	}
 
-	geni_se_setup_m_cmd(se, I2C_READ, m_param);
-
 	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
 	if (!time_left)
 		geni_i2c_abort_xfer(gi2c);
@@ -409,6 +408,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 		geni_se_select_mode(se, GENI_SE_FIFO);
 
 	writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
+	geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
 
 	if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) {
 		geni_se_select_mode(se, GENI_SE_FIFO);
@@ -416,8 +416,6 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 		dma_buf = NULL;
 	}
 
-	geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
-
 	if (!dma_buf) /* Get FIFO IRQ */
 		writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
 
-- 
2.28.0.1011.ga647a8990f-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode
  2020-10-13 21:25 [PATCH v2 0/3] i2c: i2c-qcom-geni: More properly fix the DMA race Douglas Anderson
  2020-10-13 21:25 ` [PATCH v2 1/3] soc: qcom: geni: More properly switch to DMA mode Douglas Anderson
  2020-10-13 21:25 ` [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race" Douglas Anderson
@ 2020-10-13 21:25 ` Douglas Anderson
  2020-10-14  9:17   ` Akash Asthana
  2020-10-14 23:51   ` Stephen Boyd
  2 siblings, 2 replies; 9+ messages in thread
From: Douglas Anderson @ 2020-10-13 21:25 UTC (permalink / raw)
  To: Bjorn Andersson, Wolfram Sang, Akash Asthana
  Cc: linux-i2c, Roja Rani Yarubandi, Stephen Boyd, Dmitry Baryshkov,
	linux-arm-msm, Douglas Anderson, Andy Gross, linux-kernel

The functions geni_se_select_fifo_mode() and
geni_se_select_fifo_mode() are a little funny.  They read/write a
bunch of memory mapped registers even if they don't change or aren't
relevant for the current protocol.  Let's make them a little more
sane.  We'll also add a comment explaining why we don't do some of the
operations for UART.

NOTE: there is no evidence at all that this makes any performance
difference and it fixes no bugs.  However, it seems (to me) like it
makes the functions a little easier to understand.  Decreasing the
amount of times we read/write memory mapped registers is also nice,
even if we are using "relaxed" variants.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Note that I didn't add Dmitry Baryshkov's Tested-by tag to the 3rd
patch since it changed subtly.  Also note that when adding comments
about why the UART is special it seems clear to me that we really
shouldn't be managing these interrupt enables in the common code.  It
seems like drivers should manage / enable the interrupts that they
care about.  That seems like a bigger change, though, and I didn't
want to muddy the waters and potentially delay the important fix from
patch #1 and #2.  Hopefully someone from Qualcomm can take on cleaning
this stuff up after these fixes land.

Changes in v2:
- Consistently use "val_old" to keep track of old value.
- Add comments about why UART is special.

 drivers/soc/qcom/qcom-geni-se.c | 50 +++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 751a49f6534f..7649b2057b9a 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -266,49 +266,63 @@ EXPORT_SYMBOL(geni_se_init);
 static void geni_se_select_fifo_mode(struct geni_se *se)
 {
 	u32 proto = geni_se_read_proto(se);
-	u32 val;
+	u32 val, val_old;
 
 	geni_se_irq_clear(se);
 
-	val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
+	/*
+	 * The RX path for the UART is asynchronous and so needs more
+	 * complex logic for enabling / disabling its interrupts.
+	 *
+	 * Specific notes:
+	 * - The done and TX-related interrupts are managed manually.
+	 * - We don't RX from the main sequencer (we use the secondary) so
+	 *   we don't need the RX-related interrupts enabled in the main
+	 *   sequencer for UART.
+	 */
 	if (proto != GENI_SE_UART) {
+		val_old = val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
 		val |= M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN;
 		val |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
-	}
-	writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
+		if (val != val_old)
+			writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
 
-	val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
-	if (proto != GENI_SE_UART)
+		val_old = val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
 		val |= S_CMD_DONE_EN;
-	writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
+		if (val != val_old)
+			writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
+	}
 
-	val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
+	val_old = val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
 	val &= ~GENI_DMA_MODE_EN;
-	writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
+	if (val != val_old)
+		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
 }
 
 static void geni_se_select_dma_mode(struct geni_se *se)
 {
 	u32 proto = geni_se_read_proto(se);
-	u32 val;
+	u32 val, val_old;
 
 	geni_se_irq_clear(se);
 
-	val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
 	if (proto != GENI_SE_UART) {
+		val_old = val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
 		val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN);
 		val &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
-	}
-	writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
+		if (val != val_old)
+			writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
 
-	val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
-	if (proto != GENI_SE_UART)
+		val_old = val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
 		val &= ~S_CMD_DONE_EN;
-	writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
+		if (val != val_old)
+			writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
+	}
 
-	val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
+	val_old = val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
 	val |= GENI_DMA_MODE_EN;
-	writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
+	if (val != val_old)
+		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
 }
 
 /**
-- 
2.28.0.1011.ga647a8990f-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode
  2020-10-13 21:25 ` [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode Douglas Anderson
@ 2020-10-14  9:17   ` Akash Asthana
  2020-10-14 23:51   ` Stephen Boyd
  1 sibling, 0 replies; 9+ messages in thread
From: Akash Asthana @ 2020-10-14  9:17 UTC (permalink / raw)
  To: Douglas Anderson, Bjorn Andersson, Wolfram Sang
  Cc: linux-i2c, Roja Rani Yarubandi, Stephen Boyd, Dmitry Baryshkov,
	linux-arm-msm, Andy Gross, linux-kernel


On 10/14/2020 2:55 AM, Douglas Anderson wrote:
> The functions geni_se_select_fifo_mode() and
> geni_se_select_fifo_mode() are a little funny.  They read/write a
> bunch of memory mapped registers even if they don't change or aren't
> relevant for the current protocol.  Let's make them a little more
> sane.  We'll also add a comment explaining why we don't do some of the
> operations for UART.
>
> NOTE: there is no evidence at all that this makes any performance
> difference and it fixes no bugs.  However, it seems (to me) like it
> makes the functions a little easier to understand.  Decreasing the
> amount of times we read/write memory mapped registers is also nice,
> even if we are using "relaxed" variants.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Akash Asthana <akashast@codeaurora.org>

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode
  2020-10-13 21:25 ` [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode Douglas Anderson
  2020-10-14  9:17   ` Akash Asthana
@ 2020-10-14 23:51   ` Stephen Boyd
  1 sibling, 0 replies; 9+ messages in thread
From: Stephen Boyd @ 2020-10-14 23:51 UTC (permalink / raw)
  To: Akash Asthana, Bjorn Andersson, Douglas Anderson, Wolfram Sang
  Cc: linux-i2c, Roja Rani Yarubandi, Dmitry Baryshkov, linux-arm-msm,
	Douglas Anderson, Andy Gross, linux-kernel

Quoting Douglas Anderson (2020-10-13 14:25:30)
> The functions geni_se_select_fifo_mode() and
> geni_se_select_fifo_mode() are a little funny.  They read/write a
> bunch of memory mapped registers even if they don't change or aren't
> relevant for the current protocol.  Let's make them a little more
> sane.  We'll also add a comment explaining why we don't do some of the
> operations for UART.
> 
> NOTE: there is no evidence at all that this makes any performance
> difference and it fixes no bugs.  However, it seems (to me) like it
> makes the functions a little easier to understand.  Decreasing the
> amount of times we read/write memory mapped registers is also nice,
> even if we are using "relaxed" variants.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race"
  2020-10-13 21:25 ` [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race" Douglas Anderson
@ 2020-10-26 15:05   ` Bjorn Andersson
  2020-10-26 15:13     ` Wolfram Sang
  0 siblings, 1 reply; 9+ messages in thread
From: Bjorn Andersson @ 2020-10-26 15:05 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Douglas Anderson, Akash Asthana, linux-i2c, Roja Rani Yarubandi,
	Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, Andy Gross,
	Mukesh Savaliya, linux-kernel

On Tue 13 Oct 16:25 CDT 2020, Douglas Anderson wrote:

> This reverts commit 02b9aec59243c6240fc42884acc958602146ddf6.
> 
> As talked about in the patch ("soc: qcom: geni: More properly switch
> to DMA mode"), swapping the order of geni_se_setup_m_cmd() and
> geni_se_xx_dma_prep() can sometimes cause corrupted transfers.  Thus
> we traded one problem for another.  Now that we've debugged the
> problem further and fixed the geni helper functions to more disable
> FIFO interrupts when we move to DMA mode we can revert it and end up
> with (hopefully) zero problems!
> 
> To be explicit, the patch ("soc: qcom: geni: More properly switch
> to DMA mode") is a prerequisite for this one.
> 
> Fixes: 02b9aec59243 ("i2c: i2c-qcom-geni: Fix DMA transfer race")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Reviewed-by: Akash Asthana <akashast@codeaurora.org>
> Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Wolfram, would you like to pick this patch or would you prefer that it
goes together with the other two through the soc tree?

Per Doug's description it has a functional dependency on patch 1, but as
long as all three patches makes it into v5.11 I'm okay with either way.

Regards,
Bjorn

> ---
> 
> (no changes since v1)
> 
>  drivers/i2c/busses/i2c-qcom-geni.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index dead5db3315a..32b2a9921b14 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -367,6 +367,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>  		geni_se_select_mode(se, GENI_SE_FIFO);
>  
>  	writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
> +	geni_se_setup_m_cmd(se, I2C_READ, m_param);
>  
>  	if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
>  		geni_se_select_mode(se, GENI_SE_FIFO);
> @@ -374,8 +375,6 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>  		dma_buf = NULL;
>  	}
>  
> -	geni_se_setup_m_cmd(se, I2C_READ, m_param);
> -
>  	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>  	if (!time_left)
>  		geni_i2c_abort_xfer(gi2c);
> @@ -409,6 +408,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>  		geni_se_select_mode(se, GENI_SE_FIFO);
>  
>  	writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
> +	geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
>  
>  	if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) {
>  		geni_se_select_mode(se, GENI_SE_FIFO);
> @@ -416,8 +416,6 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>  		dma_buf = NULL;
>  	}
>  
> -	geni_se_setup_m_cmd(se, I2C_WRITE, m_param);
> -
>  	if (!dma_buf) /* Get FIFO IRQ */
>  		writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
>  
> -- 
> 2.28.0.1011.ga647a8990f-goog
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race"
  2020-10-26 15:05   ` Bjorn Andersson
@ 2020-10-26 15:13     ` Wolfram Sang
  2020-10-26 21:15       ` Bjorn Andersson
  0 siblings, 1 reply; 9+ messages in thread
From: Wolfram Sang @ 2020-10-26 15:13 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Douglas Anderson, Akash Asthana, linux-i2c, Roja Rani Yarubandi,
	Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, Andy Gross,
	Mukesh Savaliya, linux-kernel


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


> Wolfram, would you like to pick this patch or would you prefer that it
> goes together with the other two through the soc tree?

Actually, I prefer the soc tree because of the functional dependency. I
am not aware of any pending qcom-geni patches, yet I think an immutable
branch for me to pull in would be nice in this case. Could you provide
one for me?


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race"
  2020-10-26 15:13     ` Wolfram Sang
@ 2020-10-26 21:15       ` Bjorn Andersson
  0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Andersson @ 2020-10-26 21:15 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Douglas Anderson, Akash Asthana, linux-i2c, Roja Rani Yarubandi,
	Stephen Boyd, Dmitry Baryshkov, linux-arm-msm, Andy Gross,
	Mukesh Savaliya, linux-kernel

On Mon 26 Oct 10:13 CDT 2020, Wolfram Sang wrote:

> 
> > Wolfram, would you like to pick this patch or would you prefer that it
> > goes together with the other two through the soc tree?
> 
> Actually, I prefer the soc tree because of the functional dependency. I
> am not aware of any pending qcom-geni patches, yet I think an immutable
> branch for me to pull in would be nice in this case. Could you provide
> one for me?
> 

Sounds good, please find the series applied on top of v5.10-rc1 at:

https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git tags/20201013212531.428538-1-dianders@chromium.org


I've merged the same into the qcom tree for 5.11.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-13 21:25 [PATCH v2 0/3] i2c: i2c-qcom-geni: More properly fix the DMA race Douglas Anderson
2020-10-13 21:25 ` [PATCH v2 1/3] soc: qcom: geni: More properly switch to DMA mode Douglas Anderson
2020-10-13 21:25 ` [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race" Douglas Anderson
2020-10-26 15:05   ` Bjorn Andersson
2020-10-26 15:13     ` Wolfram Sang
2020-10-26 21:15       ` Bjorn Andersson
2020-10-13 21:25 ` [PATCH v2 3/3] soc: qcom: geni: Optimize/comment select fifo/dma mode Douglas Anderson
2020-10-14  9:17   ` Akash Asthana
2020-10-14 23:51   ` Stephen Boyd

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git