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);
+}
+
