| Message ID | 20201203103156.32595-2-rojay@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement Shutdown callback for geni-i2c | expand |
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; > }
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; >> }
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; }
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(-)