* [RESEND PATCH V6 0/2] Implement Shutdown callback for geni-i2c @ 2020-12-03 10:31 Roja Rani Yarubandi 2020-12-03 10:31 ` [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct Roja Rani Yarubandi 2020-12-03 10:31 ` [RESEND PATCH V6 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi 0 siblings, 2 replies; 5+ messages in thread From: Roja Rani Yarubandi @ 2020-12-03 10:31 UTC (permalink / raw) To: wsa Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, akashast, msavaliy, skakit, vkaur, pyarlaga, rnayak, agross, bjorn.andersson, linux-arm-msm, linux-i2c, linux-kernel, sumit.semwal, linux-media, Roja Rani Yarubandi Hi, Below patch is picked: [V6,1/3] soc: qcom: geni: Remove "iova" check Resending below patches for review: [V6,2/3] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct [V6,3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c Thanks, Roja Roja Rani Yarubandi (2): i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct i2c: i2c-qcom-geni: Add shutdown callback for i2c drivers/i2c/busses/i2c-qcom-geni.c | 104 ++++++++++++++++++++++++----- 1 file changed, 88 insertions(+), 16 deletions(-) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct 2020-12-03 10:31 [RESEND PATCH V6 0/2] Implement Shutdown callback for geni-i2c Roja Rani Yarubandi @ 2020-12-03 10:31 ` Roja Rani Yarubandi 2020-12-09 12:59 ` Akash Asthana 2020-12-03 10:31 ` [RESEND PATCH V6 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi 1 sibling, 1 reply; 5+ messages in thread From: Roja Rani Yarubandi @ 2020-12-03 10:31 UTC (permalink / raw) To: wsa Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, akashast, msavaliy, skakit, vkaur, pyarlaga, rnayak, agross, bjorn.andersson, linux-arm-msm, linux-i2c, linux-kernel, sumit.semwal, linux-media, Roja Rani Yarubandi Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping data scope. For example during shutdown callback to unmap DMA mapping, this stored DMA mapping data can be used to call geni_se_tx_dma_unprep and geni_se_rx_dma_unprep functions. Add two helper functions geni_i2c_rx_msg_cleanup and geni_i2c_tx_msg_cleanup to unwrap the things after rx/tx FIFO/DMA transfers, so that the same can be used in geni_i2c_stop_xfer() function during shutdown callback. Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> --- Changes in V5: - As per Stephen's comments separated this patch from shutdown callback patch, gi2c->cur = NULL is not removed from geni_i2c_abort_xfer(), and made a copy of gi2c->cur and passed to cleanup functions. Changes in V6: - Added spin_lock/unlock in geni_i2c_rx_msg_cleanup() and geni_i2c_tx_msg_cleanup() functions. drivers/i2c/busses/i2c-qcom-geni.c | 69 +++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 16 deletions(-) diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index dce75b85253c..bfbc80f65006 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -86,6 +86,9 @@ struct geni_i2c_dev { u32 clk_freq_out; const struct geni_i2c_clk_fld *clk_fld; int suspended; + void *dma_buf; + size_t xfer_len; + dma_addr_t dma_addr; }; struct geni_i2c_err_log { @@ -348,14 +351,49 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n"); } +static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c, + struct i2c_msg *cur) +{ + struct geni_se *se = &gi2c->se; + unsigned long flags; + + spin_lock_irqsave(&gi2c->lock, flags); + gi2c->cur_rd = 0; + if (gi2c->dma_buf) { + if (gi2c->err) + geni_i2c_rx_fsm_rst(gi2c); + geni_se_rx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len); + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err); + } + spin_unlock_irqrestore(&gi2c->lock, flags); +} + +static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c, + struct i2c_msg *cur) +{ + struct geni_se *se = &gi2c->se; + unsigned long flags; + + spin_lock_irqsave(&gi2c->lock, flags); + gi2c->cur_wr = 0; + if (gi2c->dma_buf) { + if (gi2c->err) + geni_i2c_tx_fsm_rst(gi2c); + geni_se_tx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len); + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err); + } + spin_unlock_irqrestore(&gi2c->lock, flags); +} + static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { - dma_addr_t rx_dma; + dma_addr_t rx_dma = 0; unsigned long time_left; void *dma_buf = NULL; struct geni_se *se = &gi2c->se; size_t len = msg->len; + struct i2c_msg *cur; if (!of_machine_is_compatible("lenovo,yoga-c630")) dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); @@ -372,19 +410,18 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, geni_se_select_mode(se, GENI_SE_FIFO); i2c_put_dma_safe_msg_buf(dma_buf, msg, false); dma_buf = NULL; + } else { + gi2c->xfer_len = len; + gi2c->dma_addr = rx_dma; + gi2c->dma_buf = dma_buf; } + cur = gi2c->cur; time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); if (!time_left) geni_i2c_abort_xfer(gi2c); - gi2c->cur_rd = 0; - if (dma_buf) { - if (gi2c->err) - geni_i2c_rx_fsm_rst(gi2c); - geni_se_rx_dma_unprep(se, rx_dma, len); - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); - } + geni_i2c_rx_msg_cleanup(gi2c, cur); return gi2c->err; } @@ -392,11 +429,12 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { - dma_addr_t tx_dma; + dma_addr_t tx_dma = 0; unsigned long time_left; void *dma_buf = NULL; struct geni_se *se = &gi2c->se; size_t len = msg->len; + struct i2c_msg *cur; if (!of_machine_is_compatible("lenovo,yoga-c630")) dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); @@ -413,22 +451,21 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, geni_se_select_mode(se, GENI_SE_FIFO); i2c_put_dma_safe_msg_buf(dma_buf, msg, false); dma_buf = NULL; + } else { + gi2c->xfer_len = len; + gi2c->dma_addr = tx_dma; + gi2c->dma_buf = dma_buf; } if (!dma_buf) /* Get FIFO IRQ */ writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG); + cur = gi2c->cur; time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); if (!time_left) geni_i2c_abort_xfer(gi2c); - gi2c->cur_wr = 0; - if (dma_buf) { - if (gi2c->err) - geni_i2c_tx_fsm_rst(gi2c); - geni_se_tx_dma_unprep(se, tx_dma, len); - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); - } + geni_i2c_tx_msg_cleanup(gi2c, cur); return gi2c->err; } -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct 2020-12-03 10:31 ` [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct Roja Rani Yarubandi @ 2020-12-09 12:59 ` Akash Asthana 2020-12-21 12:34 ` rojay 0 siblings, 1 reply; 5+ messages in thread From: Akash Asthana @ 2020-12-09 12:59 UTC (permalink / raw) To: Roja Rani Yarubandi, wsa Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, msavaliy, skakit, vkaur, pyarlaga, rnayak, agross, bjorn.andersson, linux-arm-msm, linux-i2c, linux-kernel, sumit.semwal, linux-media Hi Roja, On 12/3/2020 4:01 PM, Roja Rani Yarubandi wrote: > Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping > data scope. For example during shutdown callback to unmap DMA mapping, > this stored DMA mapping data can be used to call geni_se_tx_dma_unprep > and geni_se_rx_dma_unprep functions. > > Add two helper functions geni_i2c_rx_msg_cleanup and > geni_i2c_tx_msg_cleanup to unwrap the things after rx/tx FIFO/DMA > transfers, so that the same can be used in geni_i2c_stop_xfer() > function during shutdown callback. > > Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> > --- > Changes in V5: > - As per Stephen's comments separated this patch from shutdown > callback patch, gi2c->cur = NULL is not removed from > geni_i2c_abort_xfer(), and made a copy of gi2c->cur and passed > to cleanup functions. > > Changes in V6: > - Added spin_lock/unlock in geni_i2c_rx_msg_cleanup() and > geni_i2c_tx_msg_cleanup() functions. > > drivers/i2c/busses/i2c-qcom-geni.c | 69 +++++++++++++++++++++++------- > 1 file changed, 53 insertions(+), 16 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index dce75b85253c..bfbc80f65006 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -86,6 +86,9 @@ struct geni_i2c_dev { > u32 clk_freq_out; > const struct geni_i2c_clk_fld *clk_fld; > int suspended; > + void *dma_buf; > + size_t xfer_len; > + dma_addr_t dma_addr; > }; > > struct geni_i2c_err_log { > @@ -348,14 +351,49 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) > dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n"); > } > > +static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c, > + struct i2c_msg *cur) > +{ > + struct geni_se *se = &gi2c->se; > + unsigned long flags; > + > + spin_lock_irqsave(&gi2c->lock, flags); > + gi2c->cur_rd = 0; > + if (gi2c->dma_buf) { > + if (gi2c->err) > + geni_i2c_rx_fsm_rst(gi2c); Which race we are trying to avoid here by holding spinlock? We cannot call any sleeping API by holding spinlock, geni_i2c_rx_fsm_rst calls *wait-for-completion*, which is a sleeping call. > + geni_se_rx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len); > + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err); > + } > + spin_unlock_irqrestore(&gi2c->lock, flags); > +} > + > +static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c, > + struct i2c_msg *cur) > +{ > + struct geni_se *se = &gi2c->se; > + unsigned long flags; > + > + spin_lock_irqsave(&gi2c->lock, flags); > + gi2c->cur_wr = 0; > + if (gi2c->dma_buf) { > + if (gi2c->err) > + geni_i2c_tx_fsm_rst(gi2c); Same here Regards, Akash > + geni_se_tx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len); > + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err); > + } > + spin_unlock_irqrestore(&gi2c->lock, flags); > +} > + > static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > - dma_addr_t rx_dma; > + dma_addr_t rx_dma = 0; > unsigned long time_left; > void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > size_t len = msg->len; > + struct i2c_msg *cur; > > if (!of_machine_is_compatible("lenovo,yoga-c630")) > dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > @@ -372,19 +410,18 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > geni_se_select_mode(se, GENI_SE_FIFO); > i2c_put_dma_safe_msg_buf(dma_buf, msg, false); > dma_buf = NULL; > + } else { > + gi2c->xfer_len = len; > + gi2c->dma_addr = rx_dma; > + gi2c->dma_buf = dma_buf; > } > > + cur = gi2c->cur; > time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); > if (!time_left) > geni_i2c_abort_xfer(gi2c); > > - gi2c->cur_rd = 0; > - if (dma_buf) { > - if (gi2c->err) > - geni_i2c_rx_fsm_rst(gi2c); > - geni_se_rx_dma_unprep(se, rx_dma, len); > - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); > - } > + geni_i2c_rx_msg_cleanup(gi2c, cur); > > return gi2c->err; > } > @@ -392,11 +429,12 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > - dma_addr_t tx_dma; > + dma_addr_t tx_dma = 0; > unsigned long time_left; > void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > size_t len = msg->len; > + struct i2c_msg *cur; > > if (!of_machine_is_compatible("lenovo,yoga-c630")) > dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > @@ -413,22 +451,21 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > geni_se_select_mode(se, GENI_SE_FIFO); > i2c_put_dma_safe_msg_buf(dma_buf, msg, false); > dma_buf = NULL; > + } else { > + gi2c->xfer_len = len; > + gi2c->dma_addr = tx_dma; > + gi2c->dma_buf = dma_buf; > } > > if (!dma_buf) /* Get FIFO IRQ */ > writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG); > > + cur = gi2c->cur; > time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); > if (!time_left) > geni_i2c_abort_xfer(gi2c); > > - gi2c->cur_wr = 0; > - if (dma_buf) { > - if (gi2c->err) > - geni_i2c_tx_fsm_rst(gi2c); > - geni_se_tx_dma_unprep(se, tx_dma, len); > - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); > - } > + geni_i2c_tx_msg_cleanup(gi2c, cur); > > return gi2c->err; > } -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct 2020-12-09 12:59 ` Akash Asthana @ 2020-12-21 12:34 ` rojay 0 siblings, 0 replies; 5+ messages in thread From: rojay @ 2020-12-21 12:34 UTC (permalink / raw) To: Akash Asthana Cc: wsa, swboyd, dianders, saiprakash.ranjan, gregkh, mka, msavaliy, skakit, vkaur, pyarlaga, rnayak, agross, bjorn.andersson, linux-arm-msm, linux-i2c, linux-kernel, sumit.semwal, linux-media On 2020-12-09 18:29, Akash Asthana wrote: > Hi Roja, > > On 12/3/2020 4:01 PM, Roja Rani Yarubandi wrote: >> Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping >> data scope. For example during shutdown callback to unmap DMA mapping, >> this stored DMA mapping data can be used to call geni_se_tx_dma_unprep >> and geni_se_rx_dma_unprep functions. >> >> Add two helper functions geni_i2c_rx_msg_cleanup and >> geni_i2c_tx_msg_cleanup to unwrap the things after rx/tx FIFO/DMA >> transfers, so that the same can be used in geni_i2c_stop_xfer() >> function during shutdown callback. >> >> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> >> --- >> Changes in V5: >> - As per Stephen's comments separated this patch from shutdown >> callback patch, gi2c->cur = NULL is not removed from >> geni_i2c_abort_xfer(), and made a copy of gi2c->cur and passed >> to cleanup functions. >> >> Changes in V6: >> - Added spin_lock/unlock in geni_i2c_rx_msg_cleanup() and >> geni_i2c_tx_msg_cleanup() functions. >> >> drivers/i2c/busses/i2c-qcom-geni.c | 69 >> +++++++++++++++++++++++------- >> 1 file changed, 53 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c >> b/drivers/i2c/busses/i2c-qcom-geni.c >> index dce75b85253c..bfbc80f65006 100644 >> --- a/drivers/i2c/busses/i2c-qcom-geni.c >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> @@ -86,6 +86,9 @@ struct geni_i2c_dev { >> u32 clk_freq_out; >> const struct geni_i2c_clk_fld *clk_fld; >> int suspended; >> + void *dma_buf; >> + size_t xfer_len; >> + dma_addr_t dma_addr; >> }; >> struct geni_i2c_err_log { >> @@ -348,14 +351,49 @@ static void geni_i2c_tx_fsm_rst(struct >> geni_i2c_dev *gi2c) >> dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n"); >> } >> +static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c, >> + struct i2c_msg *cur) >> +{ >> + struct geni_se *se = &gi2c->se; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&gi2c->lock, flags); >> + gi2c->cur_rd = 0; >> + if (gi2c->dma_buf) { >> + if (gi2c->err) >> + geni_i2c_rx_fsm_rst(gi2c); > > Which race we are trying to avoid here by holding spinlock? > Thought that race might occur with "cur" here. > We cannot call any sleeping API by holding spinlock, > geni_i2c_rx_fsm_rst calls *wait-for-completion*, which is a sleeping > call. > Fixed this. >> + geni_se_rx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len); >> + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err); >> + } >> + spin_unlock_irqrestore(&gi2c->lock, flags); >> +} >> + >> +static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c, >> + struct i2c_msg *cur) >> +{ >> + struct geni_se *se = &gi2c->se; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&gi2c->lock, flags); >> + gi2c->cur_wr = 0; >> + if (gi2c->dma_buf) { >> + if (gi2c->err) >> + geni_i2c_tx_fsm_rst(gi2c); > > Same here > > Regards, > > Akash > >> + geni_se_tx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len); >> + i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err); >> + } >> + spin_unlock_irqrestore(&gi2c->lock, flags); >> +} >> + >> static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct >> i2c_msg *msg, >> u32 m_param) >> { >> - dma_addr_t rx_dma; >> + dma_addr_t rx_dma = 0; >> unsigned long time_left; >> void *dma_buf = NULL; >> struct geni_se *se = &gi2c->se; >> size_t len = msg->len; >> + struct i2c_msg *cur; >> if (!of_machine_is_compatible("lenovo,yoga-c630")) >> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); >> @@ -372,19 +410,18 @@ static int geni_i2c_rx_one_msg(struct >> geni_i2c_dev *gi2c, struct i2c_msg *msg, >> geni_se_select_mode(se, GENI_SE_FIFO); >> i2c_put_dma_safe_msg_buf(dma_buf, msg, false); >> dma_buf = NULL; >> + } else { >> + gi2c->xfer_len = len; >> + gi2c->dma_addr = rx_dma; >> + gi2c->dma_buf = dma_buf; >> } >> + cur = gi2c->cur; >> time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); >> if (!time_left) >> geni_i2c_abort_xfer(gi2c); >> - gi2c->cur_rd = 0; >> - if (dma_buf) { >> - if (gi2c->err) >> - geni_i2c_rx_fsm_rst(gi2c); >> - geni_se_rx_dma_unprep(se, rx_dma, len); >> - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); >> - } >> + geni_i2c_rx_msg_cleanup(gi2c, cur); >> return gi2c->err; >> } >> @@ -392,11 +429,12 @@ static int geni_i2c_rx_one_msg(struct >> geni_i2c_dev *gi2c, struct i2c_msg *msg, >> static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct >> i2c_msg *msg, >> u32 m_param) >> { >> - dma_addr_t tx_dma; >> + dma_addr_t tx_dma = 0; >> unsigned long time_left; >> void *dma_buf = NULL; >> struct geni_se *se = &gi2c->se; >> size_t len = msg->len; >> + struct i2c_msg *cur; >> if (!of_machine_is_compatible("lenovo,yoga-c630")) >> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); >> @@ -413,22 +451,21 @@ static int geni_i2c_tx_one_msg(struct >> geni_i2c_dev *gi2c, struct i2c_msg *msg, >> geni_se_select_mode(se, GENI_SE_FIFO); >> i2c_put_dma_safe_msg_buf(dma_buf, msg, false); >> dma_buf = NULL; >> + } else { >> + gi2c->xfer_len = len; >> + gi2c->dma_addr = tx_dma; >> + gi2c->dma_buf = dma_buf; >> } >> if (!dma_buf) /* Get FIFO IRQ */ >> writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG); >> + cur = gi2c->cur; >> time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); >> if (!time_left) >> geni_i2c_abort_xfer(gi2c); >> - gi2c->cur_wr = 0; >> - if (dma_buf) { >> - if (gi2c->err) >> - geni_i2c_tx_fsm_rst(gi2c); >> - geni_se_tx_dma_unprep(se, tx_dma, len); >> - i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); >> - } >> + geni_i2c_tx_msg_cleanup(gi2c, cur); >> return gi2c->err; >> } ^ permalink raw reply [flat|nested] 5+ messages in thread
* [RESEND PATCH V6 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c 2020-12-03 10:31 [RESEND PATCH V6 0/2] Implement Shutdown callback for geni-i2c Roja Rani Yarubandi 2020-12-03 10:31 ` [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct Roja Rani Yarubandi @ 2020-12-03 10:31 ` Roja Rani Yarubandi 1 sibling, 0 replies; 5+ messages in thread From: Roja Rani Yarubandi @ 2020-12-03 10:31 UTC (permalink / raw) To: wsa Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, akashast, msavaliy, skakit, vkaur, pyarlaga, rnayak, agross, bjorn.andersson, linux-arm-msm, linux-i2c, linux-kernel, sumit.semwal, linux-media, Roja Rani Yarubandi If the hardware is still accessing memory after SMMU translation is disabled (as part of smmu shutdown callback), then the IOVAs (I/O virtual address) which it was using will go on the bus as the physical addresses which will result in unknown crashes like NoC/interconnect errors. So, implement shutdown callback to i2c driver to stop on-going transfer and unmap DMA mappings during system "reboot" or "shutdown". Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller") Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org> --- Changes in V2: - As per Stephen's comments added seperate function for stop transfer, fixed minor nitpicks. - As per Stephen's comments, changed commit text. Changes in V3: - As per Stephen's comments, squashed patch 1 into patch 2, added Fixes tag. - As per Akash's comments, included FIFO case in stop_xfer, fixed minor nitpicks. Changes in V4: - As per Stephen's comments cleaned up geni_i2c_stop_xfer function, added dma_buf in geni_i2c_dev struct to call i2c_put_dma_safe_msg_buf() from other functions, removed "iova" check in geni_se_rx_dma_unprep() and geni_se_tx_dma_unprep() functions. - Added two helper functions geni_i2c_rx_one_msg_done() and geni_i2c_tx_one_msg_done() to unwrap the things after rx/tx FIFO/DMA transfers, so that the same can be used in geni_i2c_stop_xfer() function during shutdown callback. Updated commit text accordingly. - Checking whether it is tx/rx transfer using I2C_M_RD which is valid for both FIFO and DMA cases, so dropped DMA_RX_ACTIVE and DMA_TX_ACTIVE bit checking Changes in V5: - As per Stephen's comments, added spin_lock_irqsave & spin_unlock_irqsave in geni_i2c_stop_xfer() function. Changes in V6: - As per Stephen's comments, taken care of unsafe lock order in geni_i2c_stop_xfer(). - Moved spin_lock/unlock to geni_i2c_rx_msg_cleanup() and geni_i2c_tx_msg_cleanup() functions. drivers/i2c/busses/i2c-qcom-geni.c | 35 ++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index bfbc80f65006..9a6bd7a0a56f 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -385,6 +385,33 @@ static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c, spin_unlock_irqrestore(&gi2c->lock, flags); } +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c) +{ + int ret; + u32 geni_status; + struct i2c_msg *cur; + + /* Resume device, as runtime suspend can happen anytime during transfer */ + ret = pm_runtime_get_sync(gi2c->se.dev); + if (ret < 0) { + dev_err(gi2c->se.dev, "Failed to resume device: %d\n", ret); + return; + } + + geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS); + if (!(geni_status & M_GENI_CMD_ACTIVE)) + goto out; + + cur = gi2c->cur; + geni_i2c_abort_xfer(gi2c); + if (cur->flags & I2C_M_RD) + geni_i2c_rx_msg_cleanup(gi2c, cur); + else + geni_i2c_tx_msg_cleanup(gi2c, cur); +out: + pm_runtime_put_sync_suspend(gi2c->se.dev); +} + static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { @@ -664,6 +691,13 @@ static int geni_i2c_remove(struct platform_device *pdev) return 0; } +static void geni_i2c_shutdown(struct platform_device *pdev) +{ + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev); + + geni_i2c_stop_xfer(gi2c); +} + static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev) { int ret; @@ -728,6 +762,7 @@ MODULE_DEVICE_TABLE(of, geni_i2c_dt_match); static struct platform_driver geni_i2c_driver = { .probe = geni_i2c_probe, .remove = geni_i2c_remove, + .shutdown = geni_i2c_shutdown, .driver = { .name = "geni_i2c", .pm = &geni_i2c_pm_ops, -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-21 12:35 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-03 10:31 [RESEND PATCH V6 0/2] Implement Shutdown callback for geni-i2c Roja Rani Yarubandi 2020-12-03 10:31 ` [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct Roja Rani Yarubandi 2020-12-09 12:59 ` Akash Asthana 2020-12-21 12:34 ` rojay 2020-12-03 10:31 ` [RESEND PATCH V6 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).