Messages in this thread | | | Date | Thu, 17 Sep 2020 17:33:30 +0530 | From | rojay@codeauro ... | Subject | Re: [PATCH V3] i2c: i2c-qcom-geni: Add shutdown callback for i2c |
| |
Hi Stephen,
On 2020-09-09 01:00, Stephen Boyd wrote: > Why is dri-devel on here? And linaro-mm-sig? >
Ok, I will remove these lists.
> Quoting Roja Rani Yarubandi (2020-09-07 06:07:31) >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c >> b/drivers/i2c/busses/i2c-qcom-geni.c >> index dead5db3315a..b3609760909f 100644 >> --- a/drivers/i2c/busses/i2c-qcom-geni.c >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> struct geni_i2c_err_log { >> @@ -384,7 +387,8 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev >> *gi2c, struct i2c_msg *msg, >> if (dma_buf) { >> if (gi2c->err) >> geni_i2c_rx_fsm_rst(gi2c); >> - geni_se_rx_dma_unprep(se, rx_dma, len); >> + geni_se_rx_dma_unprep(se, gi2c->rx_dma, len); >> + gi2c->rx_dma = (dma_addr_t)NULL; >> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); >> } >> >> @@ -394,12 +398,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; >> unsigned long time_left; >> void *dma_buf = NULL; >> struct geni_se *se = &gi2c->se; >> size_t len = msg->len; >> >> + gi2c->xfer_len = len; >> if (!of_machine_is_compatible("lenovo,yoga-c630")) >> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); >> >> @@ -410,7 +414,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev >> *gi2c, struct i2c_msg *msg, >> >> writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN); >> >> - if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, &tx_dma)) >> { >> + if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, >> &gi2c->tx_dma)) { >> geni_se_select_mode(se, GENI_SE_FIFO); >> i2c_put_dma_safe_msg_buf(dma_buf, msg, false); >> dma_buf = NULL; >> @@ -429,7 +433,8 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev >> *gi2c, struct i2c_msg *msg, >> if (dma_buf) { >> if (gi2c->err) >> geni_i2c_tx_fsm_rst(gi2c); >> - geni_se_tx_dma_unprep(se, tx_dma, len); >> + geni_se_tx_dma_unprep(se, gi2c->tx_dma, len); >> + gi2c->tx_dma = (dma_addr_t)NULL; >> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); >> } >> >> @@ -479,6 +484,51 @@ static int geni_i2c_xfer(struct i2c_adapter >> *adap, >> return ret; >> } >> >> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c) >> +{ >> + int ret; >> + u32 dma; >> + u32 val; >> + u32 geni_status; >> + struct geni_se *se = &gi2c->se; >> + >> + ret = pm_runtime_get_sync(gi2c->se.dev); >> + if (ret < 0) { >> + dev_err(gi2c->se.dev, "Failed to resume device: %d\n", >> ret); > > Is this print really necessary? Doesn't PM core already print this sort > of information? >
PM core will not print any such information. Here we wanted to know our driver's pm runtime resume is successful.
>> + return; >> + } >> + >> + geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS); >> + if (geni_status & M_GENI_CMD_ACTIVE) { > > Please try to de-indent all this. >
Okay.
> if (!(geni_status & M_GENI_CMD_ACTIVE)) > goto out; > >> + geni_i2c_abort_xfer(gi2c); >> + dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN); >> + if (dma) { > > if (!dma) > goto out; > >> + val = readl_relaxed(gi2c->se.base + >> SE_DMA_DEBUG_REG0); >> + if (val & DMA_TX_ACTIVE) { >> + gi2c->cur_wr = 0; >> + if (gi2c->err) >> + geni_i2c_tx_fsm_rst(gi2c); >> + if (gi2c->tx_dma) { >> + geni_se_tx_dma_unprep(se, >> + gi2c->tx_dma, >> gi2c->xfer_len); >> + gi2c->tx_dma = >> (dma_addr_t)NULL; > > Almost nobody does this. In fact, grep shows me one hit in the kernel. > If nobody else is doing it something is probably wrong. When would dma > mode be active and tx_dma not be set to something that should be > stopped? If it really is necessary I suppose we should assign this to > DMA_MAPPING_ERROR instead of casting NULL. Then the check above for > tx_dma being valid can be dropped because geni_se_tx_dma_unprep() > already checks for a valid mapping before doing anything. But really, > we > should probably be tracking the dma buffer mapped to the CPU as well as > the dma address that was used for the mapping. Not storing both is a > problem, see below. >
You are correct, setting gi2c->tx_dma to NULL and check for tx_dma is not required. Will correct this.
>> + } >> + } else if (val & DMA_RX_ACTIVE) { >> + gi2c->cur_rd = 0; >> + if (gi2c->err) >> + geni_i2c_rx_fsm_rst(gi2c); >> + if (gi2c->rx_dma) { >> + geni_se_rx_dma_unprep(se, >> + gi2c->rx_dma, >> gi2c->xfer_len); > > Looking closely it seems that the geni dma wrappers shouldn't even be > checking for an iova being non-zero. Instead they should make sure that > it just isn't invalid with !dma_mapping_error(). >
Yes. I will remove iova check in geni_se_rx_dma_unprep() function(also in tx_dma_unprep)
>> + gi2c->rx_dma = >> (dma_addr_t)NULL; > > If we're stopping some dma transaction doesn't that mean the > > i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err); > > code needs to run also? I fail to see where we free the buffer that has > been mapped for DMA. >
Yes, this is required. I will do this cleanup.
>> + } >> + } >> + } >> + } >> + > > out: > >> + pm_runtime_put_sync_suspend(gi2c->se.dev); >> +} >> +
| |