[PATCH 0/2] Implement Shutdown callback for i2c

[PATCH 0/2] Implement Shutdown callback for i2c

[Roja Rani Yarubandi]

Add tx_dma, rx_dma and xfer_len in geni_i2c_dev struct.
Implement Shutdown callback for geni i2c driver.

Roja Rani Yarubandi (2):
  i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev
    struct
  i2c: i2c-qcom-geni: Add shutdown callback for i2c

 drivers/i2c/busses/i2c-qcom-geni.c | 59 +++++++++++++++++++++++++-----
 include/linux/qcom-geni-se.h       |  5 +++
 2 files changed, 54 insertions(+), 10 deletions(-)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 1/2] i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev struct

[Roja Rani Yarubandi]

Adding tx_dma, rx_dma and xfer length in geni_i2c_dev struct to
store DMA mapping data to enhance its scope. For example during
shutdown callback to unmap DMA mapping, these new struct members
can be used as part of geni_se_tx_dma_unprep and
geni_se_rx_dma_unprep calls.

Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 7f130829bf01..53ca41f76080 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -86,6 +86,9 @@ struct geni_i2c_dev {
 	u32 clk_freq_out;
 	const struct geni_i2c_clk_fld *clk_fld;
 	int suspended;
+	dma_addr_t tx_dma;
+	dma_addr_t rx_dma;
+	u32 xfer_len;
 };
 
 struct geni_i2c_err_log {
@@ -352,12 +355,11 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
 static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 				u32 m_param)
 {
-	dma_addr_t rx_dma;
 	unsigned long time_left;
 	void *dma_buf = NULL;
 	struct geni_se *se = &gi2c->se;
-	size_t len = msg->len;
 
+	gi2c->xfer_len = msg->len;
 	if (!of_machine_is_compatible("lenovo,yoga-c630"))
 		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
 
@@ -366,9 +368,10 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	else
 		geni_se_select_mode(se, GENI_SE_FIFO);
 
-	writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
+	writel_relaxed(gi2c->xfer_len, se->base + SE_I2C_RX_TRANS_LEN);
 
-	if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
+	if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, gi2c->xfer_len,
+					   &gi2c->rx_dma)) {
 		geni_se_select_mode(se, GENI_SE_FIFO);
 		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
 		dma_buf = NULL;
@@ -384,7 +387,7 @@ 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, gi2c->xfer_len);
 		i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
 	}
 
@@ -394,12 +397,11 @@ 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 = msg->len;
 	if (!of_machine_is_compatible("lenovo,yoga-c630"))
 		dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
 
@@ -408,9 +410,10 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	else
 		geni_se_select_mode(se, GENI_SE_FIFO);
 
-	writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
+	writel_relaxed(gi2c->xfer_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, gi2c->xfer_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 +432,7 @@ 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, gi2c->xfer_len);
 		i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
 	}
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

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

[Roja Rani Yarubandi]

If the hardware is still accessing memory after SMMU translation
is disabled(as part of smmu shutdown callback), then the
IOVAs(I/O virtual address) which it was using will go on the bus
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".

Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 36 ++++++++++++++++++++++++++++++
 include/linux/qcom-geni-se.h       |  5 +++++
 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;
+
+	ret = pm_runtime_get_sync(gi2c->se.dev);
+	if (ret < 0) {
+		dev_err(gi2c->se.dev, "Failed to resume device:%d\n", ret);
+		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);
+		}
+	}
+	pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
 static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 {
 	int ret;
@@ -673,6 +708,7 @@ MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
 static struct platform_driver geni_i2c_driver = {
 	.probe  = geni_i2c_probe,
 	.remove = geni_i2c_remove,
+	.shutdown = geni_i2c_shutdown,
 	.driver = {
 		.name = "geni_i2c",
 		.pm = &geni_i2c_pm_ops,
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 */
+#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
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/2] i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev struct

[Stephen Boyd]

Quoting Roja Rani Yarubandi (2020-08-14 02:55:39)
> Adding tx_dma, rx_dma and xfer length in geni_i2c_dev struct to
> store DMA mapping data to enhance its scope. For example during
> shutdown callback to unmap DMA mapping, these new struct members
> can be used as part of geni_se_tx_dma_unprep and
> geni_se_rx_dma_unprep calls.

Please read about how to write commit text from kernel docs[1]. Hint,
use imperative mood.

> 
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> ---
>  drivers/i2c/busses/i2c-qcom-geni.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 7f130829bf01..53ca41f76080 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -86,6 +86,9 @@ struct geni_i2c_dev {
>         u32 clk_freq_out;
>         const struct geni_i2c_clk_fld *clk_fld;
>         int suspended;
> +       dma_addr_t tx_dma;
> +       dma_addr_t rx_dma;
> +       u32 xfer_len;

Why not size_t?

>  };
>  
>  struct geni_i2c_err_log {
> @@ -352,12 +355,11 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
>  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>                                 u32 m_param)
>  {
> -       dma_addr_t rx_dma;
>         unsigned long time_left;
>         void *dma_buf = NULL;
>         struct geni_se *se = &gi2c->se;
> -       size_t len = msg->len;
>  
> +       gi2c->xfer_len = msg->len;

I'd prefer to keep the local variable and then have 

	len = gi2c->xfer_len = msg->len;

>         if (!of_machine_is_compatible("lenovo,yoga-c630"))
>                 dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>  
> @@ -366,9 +368,10 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>         else
>                 geni_se_select_mode(se, GENI_SE_FIFO);
>  
> -       writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
> +       writel_relaxed(gi2c->xfer_len, se->base + SE_I2C_RX_TRANS_LEN);

So that all this doesn't have to change.

>  
> -       if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) {
> +       if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, gi2c->xfer_len,
> +                                          &gi2c->rx_dma)) {
>                 geni_se_select_mode(se, GENI_SE_FIFO);
>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>                 dma_buf = NULL;
> @@ -384,7 +387,7 @@ 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, gi2c->xfer_len);
>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>         }
>  
> @@ -394,12 +397,11 @@ 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 = msg->len;

Same comment.

>         if (!of_machine_is_compatible("lenovo,yoga-c630"))
>                 dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>  

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

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

[Stephen Boyd]

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 (

> 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.

> 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.

>  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'.

> +
> +       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.

> +               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.

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

> +#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

Re: [PATCH 1/2] i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev struct

[rojay]

Hi Stephen,

Thanks for reviewing the patches.

On 2020-08-19 09:09, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2020-08-14 02:55:39)
>> Adding tx_dma, rx_dma and xfer length in geni_i2c_dev struct to
>> store DMA mapping data to enhance its scope. For example during
>> shutdown callback to unmap DMA mapping, these new struct members
>> can be used as part of geni_se_tx_dma_unprep and
>> geni_se_rx_dma_unprep calls.
> 
> Please read about how to write commit text from kernel docs[1]. Hint,
> use imperative mood.
> 

Ok, will update the commit text.

>> 
>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
>> ---
>>  drivers/i2c/busses/i2c-qcom-geni.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 7f130829bf01..53ca41f76080 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -86,6 +86,9 @@ struct geni_i2c_dev {
>>         u32 clk_freq_out;
>>         const struct geni_i2c_clk_fld *clk_fld;
>>         int suspended;
>> +       dma_addr_t tx_dma;
>> +       dma_addr_t rx_dma;
>> +       u32 xfer_len;
> 
> Why not size_t?
> 

Will change it to size_t.

>>  };
>> 
>>  struct geni_i2c_err_log {
>> @@ -352,12 +355,11 @@ static void geni_i2c_tx_fsm_rst(struct 
>> geni_i2c_dev *gi2c)
>>  static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct 
>> i2c_msg *msg,
>>                                 u32 m_param)
>>  {
>> -       dma_addr_t rx_dma;
>>         unsigned long time_left;
>>         void *dma_buf = NULL;
>>         struct geni_se *se = &gi2c->se;
>> -       size_t len = msg->len;
>> 
>> +       gi2c->xfer_len = msg->len;
> 
> I'd prefer to keep the local variable and then have
> 
> 	len = gi2c->xfer_len = msg->len;
> 

Ok, will keep the local variable.

>>         if (!of_machine_is_compatible("lenovo,yoga-c630"))
>>                 dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>> 
>> @@ -366,9 +368,10 @@ static int geni_i2c_rx_one_msg(struct 
>> geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>         else
>>                 geni_se_select_mode(se, GENI_SE_FIFO);
>> 
>> -       writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
>> +       writel_relaxed(gi2c->xfer_len, se->base + 
>> SE_I2C_RX_TRANS_LEN);
> 
> So that all this doesn't have to change.
> 
>> 
>> -       if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, &rx_dma)) 
>> {
>> +       if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, 
>> gi2c->xfer_len,
>> +                                          &gi2c->rx_dma)) {
>>                 geni_se_select_mode(se, GENI_SE_FIFO);
>>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>>                 dma_buf = NULL;
>> @@ -384,7 +387,7 @@ 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, 
>> gi2c->xfer_len);
>>                 i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>>         }
>> 
>> @@ -394,12 +397,11 @@ 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 = msg->len;
> 
> Same comment.
> 

Ok.

>>         if (!of_machine_is_compatible("lenovo,yoga-c630"))
>>                 dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>> 
> 
> [1]
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Thanks,
Roja

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@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