Re: [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
From: rojay
Date: Mon May 17 2021 - 02:33:20 EST
Hi Stephen,
On 2021-05-13 00:27, Stephen Boyd wrote:
Quoting Roja Rani Yarubandi (2021-05-12 01:22:20)
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@xxxxxxxxxxxxxx>
---
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.
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..277ab7e7dd51 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);
+
+ if (!pm_runtime_status_suspended(gi2c->se.dev))
+ pm_runtime_set_suspended(gi2c->se.dev);
What was wrong with my suggested approach of telling i2c core that the
bus is suspended? This looks to do a bunch of work right before we're
shutting down, when it would be simpler to just mark the bus as
suspended and have it block future transactions and spit out a warning.
Sorry, I have overlooked your previous change. I will do that.
I do see that we should be marking it suspended/resumed during runtime
suspend/resume. I'm also confused if during system wide suspend/resume
we actually do anything in this driver. Is runtime suspend called for
system wide suspend?
In this geni i2c driver, runtime suspend will be called during system
suspend
if the device is NOT already suspended.
----8<----
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index 214b4c913a13..ca12d348336b 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -656,6 +656,7 @@ static int __maybe_unused
geni_i2c_runtime_suspend(struct device *dev)
struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
disable_irq(gi2c->irq);
+ i2c_mark_adapter_suspended(&gi2c->adap);
ret = geni_se_resources_off(&gi2c->se);
if (ret) {
enable_irq(gi2c->irq);
@@ -682,6 +683,7 @@ static int __maybe_unused
geni_i2c_runtime_resume(struct device *dev)
return ret;
enable_irq(gi2c->irq);
+ i2c_mark_adapter_resumed(&gi2c->adap);
gi2c->suspended = 0;
return 0;
}
Now, I have made the changes, calling i2c_mark_adapter_suspended() in
shutdown() and i2c_mark_adapter_suspended()/_resumed() from runtime
suspend/resume also and validated the changes. I have also picked
your patch [1] for this validation.
During the device boot up I am seeing multiple traces shown below.
Are these expected now and needs to be fixed from rt5682/respective
client driver?
Trace1:
[ 11.709477] i2c i2c-9: Transfer while suspended
[ 11.905595] Call trace:
[ 11.908124] __i2c_transfer+0xb8/0x38c
[ 11.911984] i2c_transfer+0xa0/0xf4
[ 11.915569] i2c_transfer_buffer_flags+0x68/0x9c
[ 11.920314] regmap_i2c_write+0x34/0x64
[ 11.924255] _regmap_raw_write_impl+0x4e8/0x7bc
[ 11.928911] _regmap_bus_raw_write+0x70/0x8c
[ 11.933301] _regmap_write+0x100/0x150
[ 11.937152] regmap_write+0x54/0x78
[ 11.940744] soc_component_write_no_lock+0x34/0xa8
[ 11.945666] snd_soc_component_write+0x3c/0x5c
[ 11.950242] rt5682_set_component_pll+0x1e4/0x2b4 [snd_soc_rt5682]
[ 11.956588] snd_soc_component_set_pll+0x50/0xa8
[ 11.961328] snd_soc_dai_set_pll+0x74/0xc8
[ 11.965542] sc7180_snd_startup+0x9c/0x120 [snd_soc_sc7180]
[ 11.971262] snd_soc_link_startup+0x34/0x88
[ 11.975557] soc_pcm_open+0x100/0x538
[ 11.979323] snd_pcm_open_substream+0x530/0x704
[ 11.983980] snd_pcm_open+0xc8/0x210
[ 11.987653] snd_pcm_playback_open+0x50/0x80
[ 11.992049] snd_open+0x120/0x150
[ 11.995462] chrdev_open+0xb8/0x1a4
[ 11.999056] do_dentry_open+0x238/0x358
[ 12.003001] vfs_open+0x34/0x40
[ 12.006235] path_openat+0x9e8/0xd60
[ 12.009913] do_filp_open+0x90/0x10c
[ 12.013587] do_sys_open+0x148/0x314
[ 12.017260] __arm64_compat_sys_openat+0x28/0x34
[ 12.022009] el0_svc_common+0xa4/0x16c
[ 12.025860] el0_svc_compat_handler+0x2c/0x40
[ 12.030337] el0_svc_compat+0x8/0x10
[ 12.034018] ---[ end trace 745ead557fcbb5dc ]---
[ 12.040151] rt5682 9-001a: ASoC: error at soc_component_write_no_lock
on rt5682.9-001a: -108
[ 12.049055] rt5682 9-001a: ASoC: error at soc_component_write_no_lock
on rt5682.9-001a: -108
[ 12.057742] rt5682 9-001a: ASoC: error at
snd_soc_component_update_bits on rt5682.9-001a: -108
Trace2:
[ 3.515390] i2c i2c-2: Transfer while suspended
[ 3.606749] Call trace:
[ 3.606751] __i2c_transfer+0xb8/0x38c
[ 3.606752] i2c_transfer+0xa0/0xf4
[ 3.606754] i2c_transfer_buffer_flags+0x68/0x9c
[ 3.639599] hub 2-1.4:1.0: USB hub found
[ 3.644375] regmap_i2c_write+0x34/0x64
[ 3.644376] _regmap_raw_write_impl+0x4e8/0x7bc
[ 3.644378] _regmap_bus_raw_write+0x70/0x8c
[ 3.644379] _regmap_write+0x100/0x150
[ 3.644381] regmap_write+0x54/0x78
[ 3.644383] ti_sn_aux_transfer+0x90/0x244
[ 3.650695] hub 2-1.4:1.0: 4 ports detected
[ 3.655288] drm_dp_dpcd_access+0x8c/0x11c
[ 3.655289] drm_dp_dpcd_read+0x64/0x10c
[ 3.655290] ti_sn_bridge_enable+0x5c/0x824
[ 3.655292] drm_atomic_bridge_chain_enable+0x78/0xa0
[ 3.655294] drm_atomic_helper_commit_modeset_enables+0x198/0x238
[ 3.655295] msm_atomic_commit_tail+0x324/0x714
[ 3.655297] commit_tail+0xa4/0x108
[ 3.664985] usb 1-1.4: new high-speed USB device number 4 using
xhci-hcd
[ 3.666204] drm_atomic_helper_commit+0xf4/0xfc
[ 3.666205] drm_atomic_commit+0x50/0x5c
[ 3.666206] drm_atomic_helper_set_config+0x64/0x98
[ 3.666208] drm_mode_setcrtc+0x26c/0x590
[ 3.666209] drm_ioctl_kernel+0x9c/0x114
[ 3.701074] hub 2-1.4:1.0: USB hub found
[ 3.703347] drm_ioctl+0x288/0x420
[ 3.703349] drm_compat_ioctl+0xd0/0xe0
[ 3.703351] __arm64_compat_sys_ioctl+0x100/0x2108
[ 3.703354] el0_svc_common+0xa4/0x16c
[ 3.708499] hub 2-1.4:1.0: 4 ports detected
[ 3.711588] el0_svc_compat_handler+0x2c/0x40
[ 3.711590] el0_svc_compat+0x8/0x10
[ 3.711591] ---[ end trace 745ead557fcbb5db ]---
[ 3.772120] usb 1-1.4: New USB device found, idVendor=0bda,
idProduct=5411, bcdDevice= 1.04
[ 3.794990] ti_sn65dsi86 2-002d: [drm:ti_sn_bridge_enable] *ERROR*
Can't read lane count (-108); assuming 4
[1]
https://lore.kernel.org/r/20210508075151.1626903-2-swboyd@xxxxxxxxxxxx
Thanks,
Roja