| Message ID | 20200814095540.32115-3-rojay@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement Shutdown callback for i2c | expand |
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
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
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
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(+)