[PATCH V11 0/2] Implement Shutdown callback for geni_i2c

[PATCH V11 0/2] Implement Shutdown callback for geni_i2c

[Roja Rani Yarubandi]

Hi,

Please review the patches.

Roja Rani Yarubandi (2):
  i2c: i2c-qcom-geni: Add shutdown callback for i2c
  i2c: i2c-qcom-geni: Suspend and resume the bus during SYSTEM_SLEEP_PM
    ops

 drivers/i2c/busses/i2c-qcom-geni.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

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

[PATCH V11 1/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, implement shutdown callback for i2c driver to suspend the bus
during system "reboot" or "shutdown".

Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
Changes in V2:
 - As per Stephen's comments added seperate function for stop transfer,
   fixed minor nitpicks.
 - As per Stephen's comments, changed commit text.

Changes in V3:
 - As per Stephen's comments, squashed patch 1 into patch 2, added Fixes tag.
 - As per Akash's comments, included FIFO case in stop_xfer, fixed minor nitpicks.

Changes in V4:
 - As per Stephen's comments cleaned up geni_i2c_stop_xfer function,
   added dma_buf in geni_i2c_dev struct to call i2c_put_dma_safe_msg_buf()
   from other functions, removed "iova" check in geni_se_rx_dma_unprep()
   and geni_se_tx_dma_unprep() functions.
 - Added two helper functions geni_i2c_rx_one_msg_done() and
   geni_i2c_tx_one_msg_done() to unwrap the things after rx/tx FIFO/DMA
   transfers, so that the same can be used in geni_i2c_stop_xfer() function
   during shutdown callback. Updated commit text accordingly.
 - Checking whether it is tx/rx transfer using I2C_M_RD which is valid for both
   FIFO and DMA cases, so dropped DMA_RX_ACTIVE and DMA_TX_ACTIVE bit checking

Changes in V5:
 - As per Stephen's comments, added spin_lock_irqsave & spin_unlock_irqsave in
   geni_i2c_stop_xfer() function.

Changes in V6:
 - As per Stephen's comments, taken care of unsafe lock order in
   geni_i2c_stop_xfer().
 - Moved spin_lock/unlock to geni_i2c_rx_msg_cleanup() and
   geni_i2c_tx_msg_cleanup() functions.

Changes in V7:
 - No changes

Changes in V8:
 - As per Wolfram Sang comment, removed goto and modified geni_i2c_stop_xfer()
   accordingly.

Changes in V9:
 - Fixed possbile race by protecting gi2c->cur and calling geni_i2c_abort_xfer()
   with adding another parameter to differentiate from which sequence is the
   geni_i2c_abort_xfer() called and handle the spin_lock/spin_unlock accordingly
   inside geni_i2c_abort_xfer(). For this added two macros ABORT_XFER and
   STOP_AND_ABORT_XFER.
 - Added a bool variable "stop_xfer" in geni_i2c_dev struct, used to put stop
   to upcoming geni_i2c_rx_one_msg() and geni_i2c_tx_one_msg() calls once we
   recieve the shutdown call.
 - Added gi2c->cur == NULL check in geni_i2c_irq() to not to process the irq
   even if any transfer is queued and shutdown to HW received.

Changes in V10:
 - As per Stephen's comments, removed ongoing transfers flush and only 
   suspending i2c bus in shutdown callback.
 - Also removed all other changes which have been made for ongoing transfers 
   flush, handling race issues etc., during shutdown callback.
 - Updated commit text accordingly.

Changes in V11:
 - As per Stephen's comments, suspended bus in shutdown call using 
   i2c_mark_adapter_suspended() call to block any future transfers.

 drivers/i2c/busses/i2c-qcom-geni.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 214b4c913a13..c3ae66ba6345 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -650,6 +650,14 @@ static int geni_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void geni_i2c_shutdown(struct platform_device *pdev)
+{
+	struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
+
+	/* Make client i2c transfers start failing */
+	i2c_mark_adapter_suspended(&gi2c->adap);
+}
+
 static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 {
 	int ret;
@@ -714,6 +722,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,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

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

[Stephen Boyd]

Quoting Roja Rani Yarubandi (2021-05-25 06:10:50)
> 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 for i2c driver to suspend the bus
> during system "reboot" or "shutdown".
>
> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 831 bytes --]

On Tue, May 25, 2021 at 06:40:50PM +0530, 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 for i2c driver to suspend the bus
> during system "reboot" or "shutdown".
> 
> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>

Do we need patch 1 after patch 2 was applied? I always thought all
devices are suspended before shutdown/reboot?

Nice to see that 'mark_adapter_suspended' becomes useful again!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[rojay]

On 2021-05-28 13:41, Wolfram Sang wrote:
> On Tue, May 25, 2021 at 06:40:50PM +0530, 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 for i2c driver to suspend the bus
>> during system "reboot" or "shutdown".
>> 
>> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the 
>> Qualcomm GENI I2C controller")
>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> 
> Do we need patch 1 after patch 2 was applied? I always thought all
> devices are suspended before shutdown/reboot?
> 

Yes, both patch 1 and patch 2 are required.
Devices are not suspended during shutdown/reboot.

> Nice to see that 'mark_adapter_suspended' becomes useful again!

Thanks,
Roja

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

[rojay]

On 2021-05-28 13:41, Wolfram Sang wrote:
> On Tue, May 25, 2021 at 06:40:50PM +0530, 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 for i2c driver to suspend the bus
>> during system "reboot" or "shutdown".
>> 
>> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the 
>> Qualcomm GENI I2C controller")
>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> 
> Do we need patch 1 after patch 2 was applied? I always thought all
> devices are suspended before shutdown/reboot?
> 

Yes, both patch 1 and patch 2 are required.
Devices are not suspended during shutdown/reboot.

> Nice to see that 'mark_adapter_suspended' becomes useful again!

Thanks,
Roja

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Tue, May 25, 2021 at 06:40:50PM +0530, 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 for i2c driver to suspend the bus
> during system "reboot" or "shutdown".
> 
> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH V11 2/2] i2c: i2c-qcom-geni: Suspend and resume the bus during SYSTEM_SLEEP_PM ops

[Roja Rani Yarubandi]

Mark bus as suspended during system suspend to block the future
transfers. Implement geni_i2c_resume_noirq() to resume the bus.

Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
Changes in V11:
 - This is newly added patch in this V11 series.

 drivers/i2c/busses/i2c-qcom-geni.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index c3ae66ba6345..671f4a52275e 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -698,6 +698,8 @@ static int __maybe_unused geni_i2c_suspend_noirq(struct device *dev)
 {
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
+	i2c_mark_adapter_suspended(&gi2c->adap);
+
 	if (!gi2c->suspended) {
 		geni_i2c_runtime_suspend(dev);
 		pm_runtime_disable(dev);
@@ -707,8 +709,16 @@ static int __maybe_unused geni_i2c_suspend_noirq(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused geni_i2c_resume_noirq(struct device *dev)
+{
+	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
+
+	i2c_mark_adapter_resumed(&gi2c->adap);
+	return 0;
+}
+
 static const struct dev_pm_ops geni_i2c_pm_ops = {
-	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(geni_i2c_suspend_noirq, NULL)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(geni_i2c_suspend_noirq, geni_i2c_resume_noirq)
 	SET_RUNTIME_PM_OPS(geni_i2c_runtime_suspend, geni_i2c_runtime_resume,
 									NULL)
 };
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH V11 2/2] i2c: i2c-qcom-geni: Suspend and resume the bus during SYSTEM_SLEEP_PM ops

[Stephen Boyd]

Quoting Roja Rani Yarubandi (2021-05-25 06:10:51)
> Mark bus as suspended during system suspend to block the future
> transfers. Implement geni_i2c_resume_noirq() to resume the bus.

May also be worth noting that this causes some warnings on trogdor
boards because suspend ordering is incorrect and the bus is being
accessed after it is suspended. Unless that is all resolved?

>
> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH V11 2/2] i2c: i2c-qcom-geni: Suspend and resume the bus during SYSTEM_SLEEP_PM ops

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On Tue, May 25, 2021 at 06:40:51PM +0530, Roja Rani Yarubandi wrote:
> Mark bus as suspended during system suspend to block the future
> transfers. Implement geni_i2c_resume_noirq() to resume the bus.
> 
> Fixes: 37692de5d523 ("i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller")
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]