i2c: tegra: Various fixes and improvements
cover i2c: tegra: Various fixes and improvements
From: Thierry Reding <[email protected]>
Hi Wolfram, Dmitry,
here's the full set of patches that I've been running local tests on and which looks like it has no other issues. I'm going to run this through our internal test system to make sure there aren't any regressions. I can carry these in the Tegra tree for a little bit until we're reasonable sure that these patches are all safe to apply and then send a pull request sometime next week.
I've also included a patch as suggested by Dmitry to keep I2C interrupts enabled during suspend/resume and I'm currently checking if that may be a fix for a suspend/resume issue that I've seen on Jetson Nano where we don't have working suspend/resume yet.
Thierry
Dmitry Osipenko (2):
i2c: tegra: Better handle case where CPU0 is busy for a long time
i2c: tegra: Synchronize DMA before termination
Thierry Reding (3):
Revert "i2c: tegra: Fix suspending in active runtime PM state"
i2c: tegra: Restore pinmux on system resume
i2c: tegra: Keep IRQs enabled during suspend/resume
drivers/i2c/busses/i2c-tegra.c | 61 +++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 26 deletions(-)
Signature · 1 line
2.24.1
PATCH 1/5 Revert "i2c: tegra: Fix suspending in active runtime PM state"
From: Thierry Reding <[email protected]>
This reverts commit 9f42de8d4ec2304f10bbc51dc0484f3503d61196.
It's not safe to use pm_runtime_force_{suspend,resume}(), especially during the noirq phase of suspend. See also the guidance provided in commit 1e2ef05bb8cf ("PM: Limit race conditions between runtime PM and system sleep (v2)").
--- drivers/i2c/busses/i2c-tegra.c | 9 --------- 1 file changed, 9 deletions(-)
drivers/i2c/busses/i2c-tegra.c +0 -9
@@ -1769,14 +1769,9 @@ static int tegra_i2c_remove(struct platform_device *pdev)static int __maybe_unused tegra_i2c_suspend (struct device *dev) {struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); -int err;i2c_mark_adapter_suspended (&i2c_dev->adapter); - err =pm_runtime_force_suspend (dev); -if (err <0 ) -return err; -return 0 ; }
@@ -1797,10 +1792,6 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)if (err)return err; - err =pm_runtime_force_resume (dev); -if (err <0 ) -return err; -i2c_mark_adapter_resumed (&i2c_dev->adapter);return 0 ;
Signature · 1 line
2.24.1
PATCH 2/5 i2c: tegra: Restore pinmux on system resume
In reply to Thierry RedingFrom: Thierry Reding <[email protected]>
Depending on the board design, the I2C controllers found on Tegra SoCs may require pinmuxing in order to function. This is done as part of the driver's runtime suspend/resume operations. However, the PM core does not allow devices to go into runtime suspend during system sleep to avoid potential races with the suspend/resume of their parents.
As a result of this, when Tegra SoCs resume from system suspend, their I2C controllers may have lost the pinmux state in hardware, whereas the pinctrl subsystem is not aware of this. To fix this, make sure that if the I2C controller is not runtime suspended, the runtime suspend code is still executed in order to disable the module clock (which we don't need to be enabled during sleep) and set the pinmux to the idle state.
Conversely, make sure that the I2C controller is properly resumed when waking up from sleep so that pinmux settings are properly restored.
This fixes a bug seen with DDC transactions to an HDMI monitor timing out when resuming from system suspend.
--- drivers/i2c/busses/i2c-tegra.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-tegra.c +10 -4
@@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)static int __maybe_unused tegra_i2c_suspend (struct device *dev) {struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); +int err =0 ;i2c_mark_adapter_suspended (&i2c_dev->adapter); -return 0 ; +if (!pm_runtime_status_suspended (dev)) + err =tegra_i2c_runtime_suspend (dev); + +return err; }static int __maybe_unused tegra_i2c_resume (struct device *dev)
@@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)if (err)return err; - err =tegra_i2c_runtime_suspend (dev); -if (err) -return err; +if (pm_runtime_status_suspended (dev)) { + err =tegra_i2c_runtime_suspend (dev); +if (err) +return err; + }i2c_mark_adapter_resumed (&i2c_dev->adapter);
Signature · 1 line
2.24.1
Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
On Wed, May 06, 2020 at 09:33:55PM +0200, Thierry Reding wrote:
[...]
Quoted 19 lines
drivers/i2c/busses/i2c-tegra.c +5
-1@@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)static int __maybe_unused tegra_i2c_suspend (struct device *dev) {struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); +int err =0 ;i2c_mark_adapter_suspended (&i2c_dev->adapter); -return 0 ; +if (!pm_runtime_status_suspended (dev)) + err =tegra_i2c_runtime_suspend (dev); + +return err; }static int __maybe_unused tegra_i2c_resume (struct device *dev)
drivers/i2c/busses/i2c-tegra.c static int __maybe_unused tegra_i2c_resume(struct device *dev) · line 1795
Quoted 7 lines
drivers/i2c/busses/i2c-tegra.c +1
-3@@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)if (err)return err; - err =tegra_i2c_runtime_suspend (dev); -if (err) -return err; +if (pm_runtime_status_suspended (dev)) {
[...]
Shouldn't this be negated as in suspend? I would assume that inbetween suspend and resume nothing changes the stored state.
Best Regards,
Michał Mirosław
Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
Quoted 24 lines · On Thu, May 07, 2020 at 12:43:36AM +0200…
On Wed, May 06, 2020 at 09:33:55PM +0200, Thierry Reding wrote:
[...]Quoted 22 lines
drivers/i2c/busses/i2c-tegra.c +5
-1@@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)static int __maybe_unused tegra_i2c_suspend (struct device *dev) {struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); +int err =0 ;i2c_mark_adapter_suspended (&i2c_dev->adapter); -return 0 ; +if (!pm_runtime_status_suspended (dev)) + err =tegra_i2c_runtime_suspend (dev); + +return err; }static int __maybe_unused tegra_i2c_resume (struct device *dev)@@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)if (err)return err;
Quoted 7 lines
drivers/i2c/busses/i2c-tegra.c static int __maybe_unused tegra_i2c_resume(struct device *dev) · line 1795
Quoted 4 lines
drivers/i2c/busses/i2c-tegra.c +1
-3- err =tegra_i2c_runtime_suspend (dev); -if (err) -return err; +if (pm_runtime_status_suspended (dev)) {[...]
Shouldn't this be negated as in suspend? I would assume that inbetween suspend and resume nothing changes the stored state.
I know this is confusing because I have now twice had the same doubts after looking at the patch after I sent it out and thought I had sent out a wrong version.
However, I think it starts to make more sense when you look at the resulting code, which I'll reproduce below:
static int __maybe_unused tegra_i2c_resume(struct device *dev)
{
struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
int err;
err = tegra_i2c_runtime_resume(dev);
if (err)
return err;
err = tegra_i2c_init(i2c_dev, false);
if (err)
return err;
if (pm_runtime_status_suspended(dev)) {
err = tegra_i2c_runtime_suspend(dev);
if (err)
return err;
}
i2c_mark_adapter_resumed(&i2c_dev->adapter);
return 0;
}So the purpose here is to runtime resume the I2C controller temporarily so that the register context can be reprogrammed because it was lost during suspend. Now, if the controller was runtime suspended prior to system suspend, we want to put it back into suspend after the context was loaded again. Conversely, if it was not runtime suspended, then we want to keep it on.
If it helps I can sprinkle some comments throughout this function to try and explain why exactly this is being done.
Thierry
Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
Quoted 71 lines · Thierry Reding
Quoted 24 lines · On Thu, May 07, 2020 at 12:43:36AM +0200…
On Wed, May 06, 2020 at 09:33:55PM +0200, Thierry Reding wrote:
[...]Quoted 22 lines
drivers/i2c/busses/i2c-tegra.c +5
-1@@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)static int __maybe_unused tegra_i2c_suspend (struct device *dev) {struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); +int err =0 ;i2c_mark_adapter_suspended (&i2c_dev->adapter); -return 0 ; +if (!pm_runtime_status_suspended (dev)) + err =tegra_i2c_runtime_suspend (dev); + +return err; }static int __maybe_unused tegra_i2c_resume (struct device *dev)@@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)if (err)return err;Quoted 7 lines
drivers/i2c/busses/i2c-tegra.c static int __maybe_unused tegra_i2c_resume(struct device *dev) · line 1795
Quoted 4 lines
drivers/i2c/busses/i2c-tegra.c +1
-3- err =tegra_i2c_runtime_suspend (dev); -if (err) -return err; +if (pm_runtime_status_suspended (dev)) {[...]
Shouldn't this be negated as in suspend? I would assume that inbetween suspend and resume nothing changes the stored state.I know this is confusing because I have now twice had the same doubts after looking at the patch after I sent it out and thought I had sent out a wrong version.
However, I think it starts to make more sense when you look at the resulting code, which I'll reproduce below:
static int __maybe_unused tegra_i2c_resume(struct device *dev) { struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev); int err; err = tegra_i2c_runtime_resume(dev); if (err) return err; err = tegra_i2c_init(i2c_dev, false); if (err) return err; if (pm_runtime_status_suspended(dev)) { err = tegra_i2c_runtime_suspend(dev); if (err) return err; } i2c_mark_adapter_resumed(&i2c_dev->adapter); return 0; }So the purpose here is to runtime resume the I2C controller temporarily so that the register context can be reprogrammed because it was lost during suspend. Now, if the controller was runtime suspended prior to system suspend, we want to put it back into suspend after the context was loaded again. Conversely, if it was not runtime suspended, then we want to keep it on.
Quoted 2 lines
If it helps I can sprinkle some comments throughout this function to try and explain why exactly this is being done.
Now it makes sense. Thanks!
The full function is the missing context. What you wrote here put in commit message should also do the job.
Best Regards,
Michał Mirosław
Dmitry Osipenko
reviewerRe: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
In reply to Thierry Reding06.05.2020 22:33, Thierry Reding пишет:
Quoted 52 lines
From: Thierry Reding <[email protected]>
Depending on the board design, the I2C controllers found on Tegra SoCs may require pinmuxing in order to function. This is done as part of the driver's runtime suspend/resume operations. However, the PM core does not allow devices to go into runtime suspend during system sleep to avoid potential races with the suspend/resume of their parents.
As a result of this, when Tegra SoCs resume from system suspend, their I2C controllers may have lost the pinmux state in hardware, whereas the pinctrl subsystem is not aware of this. To fix this, make sure that if the I2C controller is not runtime suspended, the runtime suspend code is still executed in order to disable the module clock (which we don't need to be enabled during sleep) and set the pinmux to the idle state.
Conversely, make sure that the I2C controller is properly resumed when waking up from sleep so that pinmux settings are properly restored.
This fixes a bug seen with DDC transactions to an HDMI monitor timing out when resuming from system suspend.
Signed-off-by Thierry Reding <[email protected]>--- drivers/i2c/busses/i2c-tegra.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)drivers/i2c/busses/i2c-tegra.c +5
-2@@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)static int __maybe_unused tegra_i2c_suspend (struct device *dev) {struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); +int err =0 ;i2c_mark_adapter_suspended (&i2c_dev->adapter); -return 0 ; +if (!pm_runtime_status_suspended (dev)) + err =tegra_i2c_runtime_suspend (dev); + +return err; }static int __maybe_unused tegra_i2c_resume (struct device *dev)@@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)if (err)return err; - err =tegra_i2c_runtime_suspend (dev);
drivers/i2c/busses/i2c-tegra.c static int __maybe_unused tegra_i2c_resume(struct device *dev) · line 1802
Quoted 10 lines
drivers/i2c/busses/i2c-tegra.c +5
-2-if (err) -return err; +if (pm_runtime_status_suspended (dev)) { + err =tegra_i2c_runtime_suspend (dev); +if (err) +return err; + }i2c_mark_adapter_resumed (&i2c_dev->adapter);
Is it legal to touch DPAUX registers while DPAUX is in a suspended state?
Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
Quoted 60 lines · Dmitry Osipenko
06.05.2020 22:33, Thierry Reding пишет:
Quoted 59 lines
From: Thierry Reding <[email protected]>
Depending on the board design, the I2C controllers found on Tegra SoCs may require pinmuxing in order to function. This is done as part of the driver's runtime suspend/resume operations. However, the PM core does not allow devices to go into runtime suspend during system sleep to avoid potential races with the suspend/resume of their parents.
As a result of this, when Tegra SoCs resume from system suspend, their I2C controllers may have lost the pinmux state in hardware, whereas the pinctrl subsystem is not aware of this. To fix this, make sure that if the I2C controller is not runtime suspended, the runtime suspend code is still executed in order to disable the module clock (which we don't need to be enabled during sleep) and set the pinmux to the idle state.
Conversely, make sure that the I2C controller is properly resumed when waking up from sleep so that pinmux settings are properly restored.
This fixes a bug seen with DDC transactions to an HDMI monitor timing out when resuming from system suspend.
Signed-off-by Thierry Reding <[email protected]>--- drivers/i2c/busses/i2c-tegra.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)drivers/i2c/busses/i2c-tegra.c +10
-4@@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)static int __maybe_unused tegra_i2c_suspend (struct device *dev) {struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); +int err =0 ;i2c_mark_adapter_suspended (&i2c_dev->adapter); -return 0 ; +if (!pm_runtime_status_suspended (dev)) + err =tegra_i2c_runtime_suspend (dev); + +return err; }static int __maybe_unused tegra_i2c_resume (struct device *dev)@@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)if (err)return err; - err =tegra_i2c_runtime_suspend (dev); -if (err) -return err; +if (pm_runtime_status_suspended (dev)) { + err =tegra_i2c_runtime_suspend (dev); +if (err) +return err; + }
Quoted 5 lines
drivers/i2c/busses/i2c-tegra.c static int __maybe_unused tegra_i2c_resume(struct device *dev) · line 1802
Quoted 3 lines
drivers/i2c/busses/i2c-tegra.c +0
-0i2c_mark_adapter_resumed (&i2c_dev->adapter);Is it legal to touch DPAUX registers while DPAUX is in a suspended state?
DPAUX is never runtime suspended and the dependency from the I2C controller on DPAUX should ensure that they are suspended and resumed in the right order during system sleep.
Thierry
Dmitry Osipenko
reviewerRe: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
08.05.2020 13:31, Thierry Reding пишет:
...
Quoted 5 lines
Quoted 1 line
Is it legal to touch DPAUX registers while DPAUX is in a suspended state?
DPAUX is never runtime suspended and the dependency from the I2C controller on DPAUX should ensure that they are suspended and resumed in the right order during system sleep.
1. Could you please explain why DPAUX is never suspended? Isn't it a problem?
It looks a bit odd that driver is doing this [1][2]. RPM is supposed to be used for the *dynamic* power management. Should we remove RPM usage from the DPAUX driver?
[1]
https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L524
[2]
https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L591
2. Could you please explain why I2C driver has to care about restoring the pinmux state? Why pinctrl driver isn't doing that for I2C and everything else?
Dmitry Osipenko
reviewerRe: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
Quoted 3 lines
2. Could you please explain why I2C driver has to care about restoring the pinmux state? Why pinctrl driver isn't doing that for I2C and everything else?
Although, now I see what you meant in the commit's message.
Perhaps the "their I2C controllers may have lost the pinmux state in hardware" paragraph should be removed from the commit's message because it's irrelevant to this patch. The pinctrl state is changed once tegra_i2c_runtime_resume() is invoked and it is not about the change made by this patch.
Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
Quoted 11 lines · Dmitry Osipenko
Quoted 3 lines
2. Could you please explain why I2C driver has to care about restoring the pinmux state? Why pinctrl driver isn't doing that for I2C and everything else?
Although, now I see what you meant in the commit's message.
Perhaps the "their I2C controllers may have lost the pinmux state in hardware" paragraph should be removed from the commit's message because it's irrelevant to this patch. The pinctrl state is changed once
tegra_i2c_runtime_resume()is invoked and it is not about the change made by this patch.
The pinctrl state is changed in tegra_i2c_runtime_resume() *only if* tegra_i2c_runtime_resume() has previously been called. So this patch does indeed cause the pinmux to be restored, even though it does so indirectly.
I think that paragraph is necessary to explain that. It's the pinctrl that doesn't "notice" that the actual pinctrl state has changed from "I2C" to "idle", so on resume it still thinks we're at "I2C" and won't try to reapply the same state. Calling tegra_i2c_runtime_suspend() is making sure that pinctrl explicitly switches to "idle", so that during resume it will apply the "I2C" state since it is different from "idle".
Thierry
Re: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
In reply to Dmitry OsipenkoQuoted 11 lines · Dmitry Osipenko
08.05.2020 13:31, Thierry Reding пишет:
...Quoted 5 lines
Quoted 1 line
Is it legal to touch DPAUX registers while DPAUX is in a suspended state?
DPAUX is never runtime suspended and the dependency from the I2C controller on DPAUX should ensure that they are suspended and resumed in the right order during system sleep.
1. Could you please explain why DPAUX is never suspended? Isn't it a problem?
Quoted 8 lines
It looks a bit odd that driver is doing this [1][2]. RPM is supposed to be used for the *dynamic* power management. Should we remove RPM usage from the DPAUX driver?
[1]
https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L524
[2]
https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/gpu/drm/tegra/dpaux.c#L591
Looks more like the intention had been to eventually enable dynamic power management but I never got around to it. The runtime PM implementations are what's necessary to runtime suspend the device, so all that should be needed is hook up the pm_runtime_get() and pm_runtime_put() calls correctly.
Quoted 3 lines
2. Could you please explain why I2C driver has to care about restoring the pinmux state? Why pinctrl driver isn't doing that for I2C and everything else?
We could probably do it either way. I did it this way because the runtime suspend/resume will get called either way, so might as well reuse the same code paths rather than add context save/restore.
Thierry
Dmitry Osipenko
reviewerRe: [PATCH 2/5] i2c: tegra: Restore pinmux on system resume
In reply to Thierry Reding06.05.2020 22:33, Thierry Reding пишет:
...
drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_remove(struct platform_device *pdev) · line 1777
Quoted 10 lines
static int
__maybe_unusedtegra_i2c_suspend(struct device *dev){drivers/i2c/busses/i2c-tegra.c +3
-1struct tegra_i2c_dev *i2c_dev =dev_get_drvdata (dev); +int err =0 ;i2c_mark_adapter_suspended (&i2c_dev->adapter); -return 0 ; +if (!pm_runtime_status_suspended (dev)) + err =tegra_i2c_runtime_suspend (dev);
Could you please explain what determines whether I2C is RPM-suspended or resumed during of the system's suspension?
PATCH 3/5 i2c: tegra: Keep IRQs enabled during suspend/resume
In reply to Thierry RedingFrom: Thierry Reding <[email protected]>
One of the I2C controllers on Tegra SoCs is typically connected to a system PMIC, which provides controls for critical power supplies for most platforms.
Some drivers, such as PCI, need to disable these regulators during a very late stage during suspend and resume them at a very early stage during resume.
To support these use-cases, keep interrupts disabled during suspend/ resume.
--- drivers/i2c/busses/i2c-tegra.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-tegra.c +2 -2
@@ -1694,8 +1694,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)irq_set_status_flags (i2c_dev->irq,IRQ_NOAUTOEN ); - ret =devm_request_irq (&pdev->dev, i2c_dev->irq, - tegra_i2c_isr,0 ,dev_name (&pdev->dev), i2c_dev); + ret =devm_request_irq (&pdev->dev, i2c_dev->irq, tegra_i2c_isr, +IRQF_NO_SUSPEND ,dev_name (&pdev->dev), i2c_dev);if (ret) {dev_err (&pdev->dev,"Failed to request irq %i\n" , i2c_dev->irq); goto release_dma;
Signature · 1 line
2.24.1
PATCH 4/5 i2c: tegra: Better handle case where CPU0 is busy for a long time
In reply to Thierry RedingFrom: Dmitry Osipenko <[email protected]>
Boot CPU0 always handle I2C interrupt and under some rare circumstances (like running KASAN + NFS root) it may stuck in uninterruptible state for a significant time. In this case we will get timeout if I2C transfer is running on a sibling CPU, despite of IRQ being raised. In order to handle this rare condition, the IRQ status needs to be checked after completion timeout.
--- drivers/i2c/busses/i2c-tegra.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)
drivers/i2c/busses/i2c-tegra.c +15 -12
@@ -996,14 +996,13 @@ tegra_i2c_poll_completion_timeout(struct tegra_i2c_dev *i2c_dev, do { u32 status = i2c_readl(i2c_dev, I2C_INT_STATUS); - if (status) { + if (status) tegra_i2c_isr(i2c_dev->irq, i2c_dev); - if (completion_done(complete)) { - s64 delta = ktime_ms_delta(ktimeout, ktime); + if (completion_done(complete)) { + s64 delta = ktime_ms_delta(ktimeout, ktime); - return msecs_to_jiffies(delta) ?: 1; - } + return msecs_to_jiffies(delta) ?: 1; } ktime = ktime_get();
@@ -1030,14 +1029,18 @@ tegra_i2c_wait_completion_timeout(struct tegra_i2c_dev *i2c_dev,disable_irq (i2c_dev->irq);/* -* There is a chance that completion may happen after IRQ -* synchronization, which is done by disable_irq(). +* Under some rare circumstances (like running KASAN + +* NFS root) CPU, which handles interrupt, may stuck in +* uninterruptible state for a significant time. In this +* case we will get timeout if I2C transfer is running on +* a sibling CPU, despite of IRQ being raised. +* +* In order to handle this rare condition, the IRQ status +* needs to be checked after timeout. */ -if (ret ==0 &&completion_done (complete)) { -dev_warn (i2c_dev->dev, -"completion done after timeout\n" ); - ret =1 ; - } +if (ret ==0 ) + ret =tegra_i2c_poll_completion_timeout (i2c_dev, + complete,0 ); }return ret;
Signature · 1 line
2.24.1
PATCH 5/5 i2c: tegra: Synchronize DMA before termination
In reply to Thierry RedingFrom: Dmitry Osipenko <[email protected]>
DMA transfer could be completed, but CPU (which handles DMA interrupt) may get too busy and can't handle the interrupt in a timely manner, despite of DMA IRQ being raised. In this case the DMA state needs to synchronized before terminating DMA transfer in order not to miss the DMA transfer completion.
--- drivers/i2c/busses/i2c-tegra.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/i2c/busses/i2c-tegra.c +9 -0
@@ -1219,6 +1219,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, time_left =tegra_i2c_wait_completion_timeout ( i2c_dev, &i2c_dev->dma_complete, xfer_time); +/* +* Synchronize DMA first, since dmaengine_terminate_sync() +* performs synchronization after the transfer's termination +* and we want to get a completion if transfer succeeded. +*/ +dmaengine_synchronize (i2c_dev->msg_read ? + i2c_dev->rx_dma_chan : + i2c_dev->tx_dma_chan); +dmaengine_terminate_sync (i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan);
Signature · 1 line
2.24.1