Store DMA mapping data in geni_i2c_dev struct.
Implement Shutdown callback for geni i2c driver.
Changes in V2:
- Changed commit text.
- As per Stephen's comments added separate function for stop transfer.
Roja Rani Yarubandi (2):
i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct
i2c: i2c-qcom-geni: Add shutdown callback for i2c
drivers/i2c/busses/i2c-qcom-geni.c | 50 ++++++++++++++++++++++++++++++
include/linux/qcom-geni-se.h | 5 +++
2 files changed, 55 insertions(+)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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, implement shutdown callback to i2c driver to unmap DMA mappings
during system "reboot" or "shutdown".
Signed-off-by: Roja Rani Yarubandi <[email protected]>
---
Changes in V2:
- As per Stephen's comments added seperate function for stop transfer,
fixed minor nitpicks.
drivers/i2c/busses/i2c-qcom-geni.c | 43 ++++++++++++++++++++++++++++++
include/linux/qcom-geni-se.h | 5 ++++
2 files changed, 48 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 1fda5c7c2cfc..d07f2f33bb75 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -486,6 +486,28 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
return ret;
}
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ u32 val;
+ struct geni_se *se = &gi2c->se;
+
+ val = readl_relaxed(gi2c->se.base + SE_DMA_DEBUG_REG0);
+ if (val & 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 (val & 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);
+ }
+}
+
static u32 geni_i2c_func(struct i2c_adapter *adap)
{
return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
@@ -617,6 +639,26 @@ static int geni_i2c_remove(struct platform_device *pdev)
return 0;
}
+static void geni_i2c_shutdown(struct platform_device *pdev)
+{
+ int ret;
+ u32 dma;
+ struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
+ 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);
+ return;
+ }
+
+ dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
+ if (dma)
+ geni_i2c_stop_xfer(gi2c);
+
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
{
int ret;
@@ -677,6 +719,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..c3c016496d98 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)
+/* SE_DMA_DEBUG_REG0 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
Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
data scope. For example during shutdown callback to unmap DMA mapping,
this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
and geni_se_rx_dma_unprep functions.
Signed-off-by: Roja Rani Yarubandi <[email protected]>
---
Changes in V2:
- As per Stephen's comments, changed commit text, fixed minor nitpicks.
drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 7f130829bf01..1fda5c7c2cfc 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;
+ size_t xfer_len;
};
struct geni_i2c_err_log {
@@ -358,6 +361,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
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);
@@ -384,6 +388,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);
+ gi2c->rx_dma = rx_dma;
geni_se_rx_dma_unprep(se, rx_dma, len);
i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
}
@@ -400,6 +405,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
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);
@@ -429,6 +435,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);
+ gi2c->tx_dma = tx_dma;
geni_se_tx_dma_unprep(se, tx_dma, 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
Quoting Roja Rani Yarubandi (2020-08-20 03:35:21)
> Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
> data scope. For example during shutdown callback to unmap DMA mapping,
> this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
> and geni_se_rx_dma_unprep functions.
>
> Signed-off-by: Roja Rani Yarubandi <[email protected]>
> ---
Can this be squashed with the next patch? I don't see how this stands on
its own.
Quoting Roja Rani Yarubandi (2020-08-20 03:35:22)
> 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, implement shutdown callback to i2c driver to unmap DMA mappings
> during system "reboot" or "shutdown".
>
> Signed-off-by: Roja Rani Yarubandi <[email protected]>
> ---
I'd still put a Fixes tag because it's fixing the driver when someone
runs shutdown.
> Changes in V2:
> - As per Stephen's comments added seperate function for stop transfer,
> fixed minor nitpicks.
>
> drivers/i2c/busses/i2c-qcom-geni.c | 43 ++++++++++++++++++++++++++++++
> include/linux/qcom-geni-se.h | 5 ++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 1fda5c7c2cfc..d07f2f33bb75 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -486,6 +486,28 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
> return ret;
> }
>
> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
> +{
> + u32 val;
> + struct geni_se *se = &gi2c->se;
> +
> + val = readl_relaxed(gi2c->se.base + SE_DMA_DEBUG_REG0);
> + if (val & 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 (val & 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);
> + }
> +}
> +
> static u32 geni_i2c_func(struct i2c_adapter *adap)
> {
> return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> @@ -617,6 +639,26 @@ static int geni_i2c_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static void geni_i2c_shutdown(struct platform_device *pdev)
> +{
> + int ret;
> + u32 dma;
> + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
> + 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);
> + return;
> + }
> +
> + dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> + if (dma)
> + geni_i2c_stop_xfer(gi2c);
Any reason the if (dma) check isn't inside geni_i2c_stop_xfer()?
Checking for dma and then bailing out early should work and keep the
logic in one function. Then the pm_runtime_sync() call could go in there
too and if (!dma) goto out. This assumes that we're going to call
geni_i2c_stop_xfer() from somewhere else, like if a transfer times out
or something?
> +
> + pm_runtime_put_sync_suspend(gi2c->se.dev);
> +}
> +
> static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
> {
> int ret;
Hi Roja,
On 8/20/2020 4:05 PM, Roja Rani Yarubandi wrote:
> Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
> data scope. For example during shutdown callback to unmap DMA mapping,
> this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
> and geni_se_rx_dma_unprep functions.
>
> Signed-off-by: Roja Rani Yarubandi <[email protected]>
> ---
> Changes in V2:
> - As per Stephen's comments, changed commit text, fixed minor nitpicks.
>
> drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 7f130829bf01..1fda5c7c2cfc 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;
> + size_t xfer_len;
> };
>
> struct geni_i2c_err_log {
> @@ -358,6 +361,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> struct geni_se *se = &gi2c->se;
> size_t len = msg->len;
>
> + gi2c->xfer_len = msg->len;
nit: gi2c->xfer = len, for tx_one_msg as well.
Regards,
Akash
> if (!of_machine_is_compatible("lenovo,yoga-c630"))
> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>
> @@ -384,6 +388,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);
> + gi2c->rx_dma = rx_dma;
> geni_se_rx_dma_unprep(se, rx_dma, len);
> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
> }
> @@ -400,6 +405,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> 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);
>
> @@ -429,6 +435,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);
> + gi2c->tx_dma = tx_dma;
> geni_se_tx_dma_unprep(se, tx_dma, len);
> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
> }
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
Hi Roja,
On 8/20/2020 4:05 PM, Roja Rani Yarubandi wrote:
> 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, implement shutdown callback to i2c driver to unmap DMA mappings
> during system "reboot" or "shutdown".
>
> Signed-off-by: Roja Rani Yarubandi <[email protected]>
> ---
> Changes in V2:
> - As per Stephen's comments added seperate function for stop transfer,
> fixed minor nitpicks.
>
> drivers/i2c/busses/i2c-qcom-geni.c | 43 ++++++++++++++++++++++++++++++
> include/linux/qcom-geni-se.h | 5 ++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 1fda5c7c2cfc..d07f2f33bb75 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -486,6 +486,28 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
> return ret;
> }
>
> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
> +{
> + u32 val;
> + struct geni_se *se = &gi2c->se;
> +
> + val = readl_relaxed(gi2c->se.base + SE_DMA_DEBUG_REG0);
> + if (val & 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);
> + }
should be 'else if', because TX and RX can't happen parallel.
> + if (val & 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);
> + }
> +}
> +
> static u32 geni_i2c_func(struct i2c_adapter *adap)
> {
> return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> @@ -617,6 +639,26 @@ static int geni_i2c_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static void geni_i2c_shutdown(struct platform_device *pdev)
> +{
> + int ret;
> + u32 dma;
> + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
> + 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);
> + return;
> + }
> +
> + dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
Wrt to current issue context it may be suffice to stop just DMA mode
transfers but why not stop all mode of active transfer during shutdown
in a generic way.
Regards,
Akash
> + if (dma)
> + geni_i2c_stop_xfer(gi2c);
> +
> + pm_runtime_put_sync_suspend(gi2c->se.dev);
> +}
> +
> static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
> {
> int ret;
> @@ -677,6 +719,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..c3c016496d98 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)
>
> +/* SE_DMA_DEBUG_REG0 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
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
On 2020-08-21 05:48, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2020-08-20 03:35:21)
>> Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
>> data scope. For example during shutdown callback to unmap DMA mapping,
>> this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
>> and geni_se_rx_dma_unprep functions.
>>
>> Signed-off-by: Roja Rani Yarubandi <[email protected]>
>> ---
>
> Can this be squashed with the next patch? I don't see how this stands
> on
> its own.
Okay.
On 2020-08-21 05:52, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2020-08-20 03:35:22)
>> 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, implement shutdown callback to i2c driver to unmap DMA mappings
>> during system "reboot" or "shutdown".
>>
>> Signed-off-by: Roja Rani Yarubandi <[email protected]>
>> ---
>
> I'd still put a Fixes tag because it's fixing the driver when someone
> runs shutdown.
>
Okay, will add fixes tag.
>> Changes in V2:
>> - As per Stephen's comments added seperate function for stop
>> transfer,
>> fixed minor nitpicks.
>>
>> drivers/i2c/busses/i2c-qcom-geni.c | 43
>> ++++++++++++++++++++++++++++++
>> include/linux/qcom-geni-se.h | 5 ++++
>> 2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 1fda5c7c2cfc..d07f2f33bb75 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -486,6 +486,28 @@ static int geni_i2c_xfer(struct i2c_adapter
>> *adap,
>> return ret;
>> }
>>
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 val;
>> + struct geni_se *se = &gi2c->se;
>> +
>> + val = readl_relaxed(gi2c->se.base + SE_DMA_DEBUG_REG0);
>> + if (val & 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 (val & 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);
>> + }
>> +}
>> +
>> static u32 geni_i2c_func(struct i2c_adapter *adap)
>> {
>> return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL &
>> ~I2C_FUNC_SMBUS_QUICK);
>> @@ -617,6 +639,26 @@ static int geni_i2c_remove(struct platform_device
>> *pdev)
>> return 0;
>> }
>>
>> +static void geni_i2c_shutdown(struct platform_device *pdev)
>> +{
>> + int ret;
>> + u32 dma;
>> + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
>> + 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);
>> + return;
>> + }
>> +
>> + dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
>> + if (dma)
>> + geni_i2c_stop_xfer(gi2c);
>
> Any reason the if (dma) check isn't inside geni_i2c_stop_xfer()?
> Checking for dma and then bailing out early should work and keep the
> logic in one function. Then the pm_runtime_sync() call could go in
> there
> too and if (!dma) goto out. This assumes that we're going to call
> geni_i2c_stop_xfer() from somewhere else, like if a transfer times out
> or something?
>
Okay, will do the changes.
>> +
>> + pm_runtime_put_sync_suspend(gi2c->se.dev);
>> +}
>> +
>> static int __maybe_unused geni_i2c_runtime_suspend(struct device
>> *dev)
>> {
>> int ret;
On 2020-08-26 17:25, Akash Asthana wrote:
> Hi Roja,
>
> On 8/20/2020 4:05 PM, Roja Rani Yarubandi wrote:
>> Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
>> data scope. For example during shutdown callback to unmap DMA mapping,
>> this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
>> and geni_se_rx_dma_unprep functions.
>>
>> Signed-off-by: Roja Rani Yarubandi <[email protected]>
>> ---
>> Changes in V2:
>> - As per Stephen's comments, changed commit text, fixed minor
>> nitpicks.
>>
>> drivers/i2c/busses/i2c-qcom-geni.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 7f130829bf01..1fda5c7c2cfc 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;
>> + size_t xfer_len;
>> };
>> struct geni_i2c_err_log {
>> @@ -358,6 +361,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev
>> *gi2c, struct i2c_msg *msg,
>> struct geni_se *se = &gi2c->se;
>> size_t len = msg->len;
>> + gi2c->xfer_len = msg->len;
>
> nit: gi2c->xfer = len, for tx_one_msg as well.
>
> Regards,
>
> Akash
>
Okay
>> if (!of_machine_is_compatible("lenovo,yoga-c630"))
>> dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
>> @@ -384,6 +388,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);
>> + gi2c->rx_dma = rx_dma;
>> geni_se_rx_dma_unprep(se, rx_dma, len);
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>> }
>> @@ -400,6 +405,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev
>> *gi2c, struct i2c_msg *msg,
>> 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);
>> @@ -429,6 +435,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);
>> + gi2c->tx_dma = tx_dma;
>> geni_se_tx_dma_unprep(se, tx_dma, len);
>> i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
>> }
On 2020-08-26 17:26, Akash Asthana wrote:
> Hi Roja,
>
> On 8/20/2020 4:05 PM, Roja Rani Yarubandi wrote:
>> 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, implement shutdown callback to i2c driver to unmap DMA mappings
>> during system "reboot" or "shutdown".
>>
>> Signed-off-by: Roja Rani Yarubandi <[email protected]>
>> ---
>> Changes in V2:
>> - As per Stephen's comments added seperate function for stop
>> transfer,
>> fixed minor nitpicks.
>>
>> drivers/i2c/busses/i2c-qcom-geni.c | 43
>> ++++++++++++++++++++++++++++++
>> include/linux/qcom-geni-se.h | 5 ++++
>> 2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 1fda5c7c2cfc..d07f2f33bb75 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -486,6 +486,28 @@ static int geni_i2c_xfer(struct i2c_adapter
>> *adap,
>> return ret;
>> }
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + u32 val;
>> + struct geni_se *se = &gi2c->se;
>> +
>> + val = readl_relaxed(gi2c->se.base + SE_DMA_DEBUG_REG0);
>> + if (val & 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);
>> + }
> should be 'else if', because TX and RX can't happen parallel.
>> + if (val & 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);
>> + }
>> +}
>> +
>> static u32 geni_i2c_func(struct i2c_adapter *adap)
>> {
>> return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL &
>> ~I2C_FUNC_SMBUS_QUICK);
>> @@ -617,6 +639,26 @@ static int geni_i2c_remove(struct platform_device
>> *pdev)
>> return 0;
>> }
>> +static void geni_i2c_shutdown(struct platform_device *pdev)
>> +{
>> + int ret;
>> + u32 dma;
>> + struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
>> + 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);
>> + return;
>> + }
>> +
>> + dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
>
> Wrt to current issue context it may be suffice to stop just DMA mode
> transfers but why not stop all mode of active transfer during shutdown
> in a generic way.
>
> Regards,
>
> Akash
>
Okay, I will include FIFO transfer case also in stop_xfer
>> + if (dma)
>> + geni_i2c_stop_xfer(gi2c);
>> +
>> + pm_runtime_put_sync_suspend(gi2c->se.dev);
>> +}
>> +
>> static int __maybe_unused geni_i2c_runtime_suspend(struct device
>> *dev)
>> {
>> int ret;
>> @@ -677,6 +719,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..c3c016496d98 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)
>> +/* SE_DMA_DEBUG_REG0 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