Hi,
On Tue, Jul 21, 2020 at 12:08 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Stephen Boyd (2020-07-20 22:59:14)
> >
> > I worry that we also need a dmb() here to make sure the dma buffer is
> > properly mapped before this write to the device is attempted. But it may
> > only matter to be before the I2C_READ.
> >
>
> I'm suggesting this patch instead where we make geni_se_setup_m_cmd()
> use a writel() so that it has the proper barrier semantics to wait for
> the other memory writes that happened in program order before this point
> to complete before the device is kicked to do a read or a write.
Are you saying that dma_map_single() isn't guaranteed to have a
barrier or something? I tried to do some searching and found a thread
[1] where someone tried to add a barrierless variant of them. To me
that means that the current APIs have barriers.
...or is there something else you're worried about?
> ----8<----
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 18d1e4fd4cf3..7f130829bf01 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -367,7 +367,6 @@ 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);
> @@ -375,6 +374,8 @@ 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);
I guess it's true that we only need the setup_m_cmd moved.
> +
> time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> if (!time_left)
> geni_i2c_abort_xfer(gi2c);
> @@ -408,7 +409,6 @@ 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,6 +416,8 @@ 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);
> +
True, it's probably safer to do the TX too even if I'm not seeing
problems there. Of course, I don't think I'm doing any large writes
so probably never triggering this path anyway.
> if (!dma_buf) /* Get FIFO IRQ */
> writel_relaxed(1, se->base + SE_GENI_TX_WATERMARK_REG);
>
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index dd464943f717..1dc134e9eb36 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -262,7 +262,7 @@ static inline void geni_se_setup_m_cmd(struct geni_se *se, u32 cmd, u32 params)
> u32 m_cmd;
>
> m_cmd = (cmd << M_OPCODE_SHFT) | (params & M_PARAMS_MSK);
> - writel_relaxed(m_cmd, se->base + SE_GENI_M_CMD0);
> + writel(m_cmd, se->base + SE_GENI_M_CMD0);
I'll wait a little bit to see if you agree that the implicit barrier
that's part of dma_map_single() gets rid of the need to change
geni_se_setup_m_cmd(). If you agree then I'll send a v2 that moves
just the setup_m_cmd and does TX in addition to RX. I'll plan to keep
accumulated tags unless someone says this is a bad idea.
[1] https://lore.kernel.org/r/1264473346-32721-1-git-send-email-adharmap@xxxxxxxxxxxxxx/
-Doug
