| Message ID | 20201013142448.v2.2.I7b22281453b8a18ab16ef2bfd4c641fb1cc6a92c@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9cb4c67d7717135d6f4600a49ab07b470ea4ee2f |
| Headers | show |
| Series | i2c: i2c-qcom-geni: More properly fix the DMA race | expand |
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.
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);
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(-)