* [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
@ 2021-05-12 8:22 Roja Rani Yarubandi
2021-05-12 18:57 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Roja Rani Yarubandi @ 2021-05-12 8:22 UTC (permalink / raw)
To: wsa
Cc: swboyd, dianders, saiprakash.ranjan, gregkh, mka, skananth,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media,
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.
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);
+}
+
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-05-12 8:22 [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
@ 2021-05-12 18:57 ` Stephen Boyd
2021-05-17 6:32 ` rojay
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2021-05-12 18:57 UTC (permalink / raw)
To: Roja Rani Yarubandi, wsa
Cc: dianders, saiprakash.ranjan, gregkh, mka, skananth, msavaliy,
skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
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@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.
>
> 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.
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?
----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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-05-12 18:57 ` Stephen Boyd
@ 2021-05-17 6:32 ` rojay
2021-05-20 8:15 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: rojay @ 2021-05-17 6:32 UTC (permalink / raw)
To: Stephen Boyd
Cc: wsa, dianders, saiprakash.ranjan, gregkh, mka, skananth,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
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@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.
>>
>> 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@chromium.org
Thanks,
Roja
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-05-17 6:32 ` rojay
@ 2021-05-20 8:15 ` Stephen Boyd
2021-05-21 16:12 ` rojay
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2021-05-20 8:15 UTC (permalink / raw)
To: rojay
Cc: wsa, dianders, saiprakash.ranjan, gregkh, mka, skananth,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
Quoting rojay@codeaurora.org (2021-05-16 23:32:50)
> Hi Stephen,
>
> 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 ]---
Ah I see. Maybe it isn't correct to mark the device as suspended in
runtime PM operations because the bus will be resumed during the
transfer? So only mark it suspended during system wide suspend/resume
transitions?
-Stephen
> [ 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@chromium.org
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-05-20 8:15 ` Stephen Boyd
@ 2021-05-21 16:12 ` rojay
2021-05-21 19:20 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: rojay @ 2021-05-21 16:12 UTC (permalink / raw)
To: Stephen Boyd
Cc: wsa, dianders, saiprakash.ranjan, gregkh, mka, skananth,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
On 2021-05-20 13:45, Stephen Boyd wrote:
> Quoting rojay@codeaurora.org (2021-05-16 23:32:50)
>> Hi Stephen,
>>
>> 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 ]---
>
> Ah I see. Maybe it isn't correct to mark the device as suspended in
> runtime PM operations because the bus will be resumed during the
> transfer? So only mark it suspended during system wide suspend/resume
> transitions?
>
> -Stephen
>
Yes, we cannot mark device as suspended/resumed during
runtime PM operations. Bus will be resumed during i2c
transfers and before transfer initiation, in __i2c_transfer()
from i2c-core-base.c there is a check to see whether the device
is marked as suspended with "__i2c_check_suspended(adap)" call,
which is "true" in this case and returning from there.
To mark it only suspended during system wide suspend/resume
transitions, currently our geni i2c driver has only
system_suspend implemented (geni_i2c_suspend_noirq()) and
does not have system_resume implemented, which again causes i2c
transfers to fail during system_resume after system_suspend.
Shall I go ahead with marking device suspended during
shutdown() only?
-Roja
>> [ 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@chromium.org
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c
2021-05-21 16:12 ` rojay
@ 2021-05-21 19:20 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2021-05-21 19:20 UTC (permalink / raw)
To: rojay
Cc: wsa, dianders, saiprakash.ranjan, gregkh, mka, skananth,
msavaliy, skakit, rnayak, agross, bjorn.andersson, linux-arm-msm,
linux-i2c, linux-kernel, sumit.semwal, linux-media
Quoting rojay@codeaurora.org (2021-05-21 09:12:02)
>
> Yes, we cannot mark device as suspended/resumed during
> runtime PM operations. Bus will be resumed during i2c
> transfers and before transfer initiation, in __i2c_transfer()
> from i2c-core-base.c there is a check to see whether the device
> is marked as suspended with "__i2c_check_suspended(adap)" call,
> which is "true" in this case and returning from there.
>
> To mark it only suspended during system wide suspend/resume
> transitions, currently our geni i2c driver has only
> system_suspend implemented (geni_i2c_suspend_noirq()) and
> does not have system_resume implemented, which again causes i2c
> transfers to fail during system_resume after system_suspend.
Got it.
>
> Shall I go ahead with marking device suspended during
> shutdown() only?
>
Yes that sounds good. Can you send two patches, one to mark suspend in
the system PM paths and one in the shutdown path? There are various bugs
to fix in other drivers that are trying to use the i2c bus after it has
been suspended and shutdown, but that doesn't block these patches from
moving forward. I'll take a look and see what's going on with
suspend/resume on my board.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-21 19:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12 8:22 [PATCH V10] i2c: i2c-qcom-geni: Add shutdown callback for i2c Roja Rani Yarubandi
2021-05-12 18:57 ` Stephen Boyd
2021-05-17 6:32 ` rojay
2021-05-20 8:15 ` Stephen Boyd
2021-05-21 16:12 ` rojay
2021-05-21 19:20 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).