* [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; 10+ 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 related [flat|nested] 10+ 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; 10+ 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 related [flat|nested] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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
2020-11-03 20:35 ` Wolfram Sang
0 siblings, 1 reply; 10+ 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] 10+ messages in thread
* Re: [PATCH v2 2/3] Revert "i2c: i2c-qcom-geni: Fix DMA transfer race"
2020-10-26 21:15 ` Bjorn Andersson
@ 2020-11-03 20:35 ` Wolfram Sang
0 siblings, 0 replies; 10+ messages in thread
From: Wolfram Sang @ 2020-11-03 20:35 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: 213 bytes --]
> 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
Thanks, pulled.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 10+ 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; 10+ 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 related [flat|nested] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ messages in thread