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

[rojay]

On 2020-08-19 09:13, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2020-08-14 02:55:40)
> > If the hardware is still accessing memory after SMMU translation
> > is disabled(as part of smmu shutdown callback), then the
>
> Put a space before (

Ok.

> > IOVAs(I/O virtual address) which it was using will go on the bus
>
> Put a space before (
>
> > as the physical addresses which will result in unknown crashes
> > like NoC/interconnect errors.
> >
> > So, adding shutdown callback to i2c driver to unmap DMA mappings
> > during system "reboot" or "shutdown".
>
> Deserves a Fixes: tag if it's fixing a problem, which it looks like it
> is.

It is not fixing a problem. We are anticipating a problem and 
implementing
Shutdown callback.

> > Signed-off-by: Roja Rani Yarubandi <rojay@xxxxxxxxxxxxxx>
> > ---
> >  drivers/i2c/busses/i2c-qcom-geni.c | 36 
> > ++++++++++++++++++++++++++++++
> >  include/linux/qcom-geni-se.h       |  5 +++++
>
> I'd prefer this is squashed with the previous patch. The first patch
> doesn't really stand on its own anyway.

Sorry, I did not understand.
First patch can be compiled independently.

> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
> > b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 53ca41f76080..749c225f95c4 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -613,6 +613,41 @@ static int geni_i2c_remove(struct platform_device 
> > *pdev)
> >         return 0;
> >  }
> >
> > +static void geni_i2c_shutdown(struct platform_device *pdev)
> > +{
> > +       int ret;
> > +       struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
> > +       struct geni_se *se = &gi2c->se;
> > +       u32 dma;
> > +       u32 dma_dbg_reg;
>
> Typically this is just called 'val'.

Ok.

> > +
> > +       ret = pm_runtime_get_sync(gi2c->se.dev);
> > +       if (ret < 0) {
> > +               dev_err(gi2c->se.dev, "Failed to resume device:%d\n", 
> > ret);
>
> Maybe just write: "Failed to resume device\n"? Not sure anyone cares
> what the error code is. And if they did, it would be connected to the
> colon so it will be hard to read. Add a space after colon if you want 
> to
> keep the return value please.

Ok, will add space after colon.

> > +               return;
> > +       }
> > +
> > +       dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> > +       if (dma) {
> > +               dma_dbg_reg = readl_relaxed(gi2c->se.base + 
> > SE_DMA_DEBUG_REG0);
> > +               if (dma_dbg_reg & DMA_TX_ACTIVE) {
> > +                       geni_i2c_abort_xfer(gi2c);
> > +                       gi2c->cur_wr = 0;
> > +                       if (gi2c->err)
> > +                               geni_i2c_tx_fsm_rst(gi2c);
> > +                       geni_se_tx_dma_unprep(se, gi2c->tx_dma, 
> > gi2c->xfer_len);
> > +               }
> > +               if (dma_dbg_reg & DMA_RX_ACTIVE) {
> > +                       geni_i2c_abort_xfer(gi2c);
> > +                       gi2c->cur_rd = 0;
> > +                       if (gi2c->err)
> > +                               geni_i2c_rx_fsm_rst(gi2c);
> > +                       geni_se_rx_dma_unprep(se, gi2c->rx_dma, 
> > gi2c->xfer_len);
> > +               }
>
> Can this be a function? It sort of seems like we should be doing the
> same sort of stuff if we're canceling a DMA transaction in flight.

Ok. Will make the changes.

> > +       }
> > +       pm_runtime_put_sync_suspend(gi2c->se.dev);
> > +}
> > +
> >  static int __maybe_unused geni_i2c_runtime_suspend(struct device 
> > *dev)
> >  {
> >         int ret;
> > diff --git a/include/linux/qcom-geni-se.h 
> > b/include/linux/qcom-geni-se.h
> > index dd464943f717..acad69be747d 100644
> > --- a/include/linux/qcom-geni-se.h
> > +++ b/include/linux/qcom-geni-se.h
> > @@ -77,6 +77,7 @@ struct geni_se {
> >  #define SE_DMA_RX_FSM_RST              0xd58
> >  #define SE_HW_PARAM_0                  0xe24
> >  #define SE_HW_PARAM_1                  0xe28
> > +#define SE_DMA_DEBUG_REG0              0xe40
> >
> >  /* GENI_FORCE_DEFAULT_REG fields */
> >  #define FORCE_DEFAULT  BIT(0)
> > @@ -207,6 +208,10 @@ struct geni_se {
> >  #define RX_GENI_CANCEL_IRQ             BIT(11)
> >  #define RX_GENI_GP_IRQ_EXT             GENMASK(13, 12)
> >
> > +/* DMA DEBUG Register fields */
>
> Please follow other style, ie. SE_DMA_DEBUG_REG0 fields

Ok.

> > +#define DMA_TX_ACTIVE                  BIT(0)
> > +#define DMA_RX_ACTIVE                  BIT(1)
> > +
> >  /* SE_HW_PARAM_0 fields */
> >  #define TX_FIFO_WIDTH_MSK              GENMASK(29, 24)
> >  #define TX_FIFO_WIDTH_SHFT             24