| Message ID | 20201013212531.428538-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | i2c: i2c-qcom-geni: More properly fix the DMA race | expand |
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>
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 >
> 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?
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
> 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.
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(-)