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

[rojay@codeaurora.org]

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@codeaurora.org>
>> ---
>>  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