Re: [PATCH V3] i2c: i2c-qcom-geni: Add shutdown callback for i2c

[rojay]

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