[PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Stephen Warren]

From: Dilan Lee <dilee-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

We found the register settings of wm8903(an audio codec) can't be modified
in snd_soc_suspend since I2C bus has been suspended before snd_soc_suspend.

Pop noise will occur when system resume from LP0 if the register settings of wm8903
haven't be modified correctly in snd_soc_suspend.

So, we use the suspend_noirq/resume_noirq callbacks to make sure I2C bus still
operates while running snd_soc_suspend.

Signed-off-by: Dilan Lee <dilee-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
When applying this, you'll get a trivial merge conflict with the other
i2c-tegra.c patch I just reposted titled "i2c: Tegra: Add of_match_table".
Just add the two lines to tegra_i2c_driver.driver in whichever order you
prefer:-).

 drivers/i2c/busses/i2c-tegra.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 0c6e840..3d2bf19 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -687,8 +687,9 @@ static int tegra_i2c_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM
-static int tegra_i2c_suspend(struct platform_device *pdev, pm_message_t state)
+static int tegra_i2c_suspend_noirq(struct device *dev)
 {
+	struct platform_device *pdev = to_platform_device(dev);
 	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
 
 	i2c_lock_adapter(&i2c_dev->adapter);
@@ -698,8 +699,9 @@ static int tegra_i2c_suspend(struct platform_device *pdev, pm_message_t state)
 	return 0;
 }
 
-static int tegra_i2c_resume(struct platform_device *pdev)
+static int tegra_i2c_resume_noirq(struct device *dev)
 {
+	struct platform_device *pdev = to_platform_device(dev);
 	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
 	int ret;
 
@@ -718,18 +720,23 @@ static int tegra_i2c_resume(struct platform_device *pdev)
 
 	return 0;
 }
+
+static const struct dev_pm_ops tegra_i2c_dev_pm_ops = {
+	.suspend_noirq = tegra_i2c_suspend_noirq,
+	.resume_noirq = tegra_i2c_resume_noirq,
+};
+#define TEGRA_I2C_DEV_PM_OPS (&tegra_i2c_dev_pm_ops)
+#else
+#define TEGRA_I2C_DEV_PM_OPS NULL
 #endif
 
 static struct platform_driver tegra_i2c_driver = {
 	.probe   = tegra_i2c_probe,
 	.remove  = tegra_i2c_remove,
-#ifdef CONFIG_PM
-	.suspend = tegra_i2c_suspend,
-	.resume  = tegra_i2c_resume,
-#endif
 	.driver  = {
 		.name  = "tegra-i2c",
 		.owner = THIS_MODULE,
+		.pm    = TEGRA_I2C_DEV_PM_OPS,
 	},
 };
 
-- 
1.7.0.4

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Colin Cross]

On Fri, Aug 5, 2011 at 4:15 PM, Stephen Warren <swarren@nvidia.com> wrote:
> From: Dilan Lee <dilee@nvidia.com>
>
> We found the register settings of wm8903(an audio codec) can't be modified
> in snd_soc_suspend since I2C bus has been suspended before snd_soc_suspend.
>
> Pop noise will occur when system resume from LP0 if the register settings of wm8903
> haven't be modified correctly in snd_soc_suspend.
>
> So, we use the suspend_noirq/resume_noirq callbacks to make sure I2C bus still
> operates while running snd_soc_suspend.
>
> Signed-off-by: Dilan Lee <dilee@nvidia.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> When applying this, you'll get a trivial merge conflict with the other
> i2c-tegra.c patch I just reposted titled "i2c: Tegra: Add of_match_table".
> Just add the two lines to tegra_i2c_driver.driver in whichever order you
> prefer:-).
>
>  drivers/i2c/busses/i2c-tegra.c |   19 +++++++++++++------
>  1 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 0c6e840..3d2bf19 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -687,8 +687,9 @@ static int tegra_i2c_remove(struct platform_device *pdev)
>  }
>
>  #ifdef CONFIG_PM
> -static int tegra_i2c_suspend(struct platform_device *pdev, pm_message_t state)
> +static int tegra_i2c_suspend_noirq(struct device *dev)
>  {
> +       struct platform_device *pdev = to_platform_device(dev);
>        struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
>
>        i2c_lock_adapter(&i2c_dev->adapter);
> @@ -698,8 +699,9 @@ static int tegra_i2c_suspend(struct platform_device *pdev, pm_message_t state)
>        return 0;
>  }
>
> -static int tegra_i2c_resume(struct platform_device *pdev)
> +static int tegra_i2c_resume_noirq(struct device *dev)
>  {
> +       struct platform_device *pdev = to_platform_device(dev);
>        struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
>        int ret;
>
> @@ -718,18 +720,23 @@ static int tegra_i2c_resume(struct platform_device *pdev)
>
>        return 0;
>  }
> +
> +static const struct dev_pm_ops tegra_i2c_dev_pm_ops = {
> +       .suspend_noirq = tegra_i2c_suspend_noirq,
> +       .resume_noirq = tegra_i2c_resume_noirq,
> +};
> +#define TEGRA_I2C_DEV_PM_OPS (&tegra_i2c_dev_pm_ops)
> +#else
> +#define TEGRA_I2C_DEV_PM_OPS NULL
>  #endif
>
>  static struct platform_driver tegra_i2c_driver = {
>        .probe   = tegra_i2c_probe,
>        .remove  = tegra_i2c_remove,
> -#ifdef CONFIG_PM
> -       .suspend = tegra_i2c_suspend,
> -       .resume  = tegra_i2c_resume,
> -#endif
>        .driver  = {
>                .name  = "tegra-i2c",
>                .owner = THIS_MODULE,
> +               .pm    = TEGRA_I2C_DEV_PM_OPS,
>        },
>  };
>
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

NAK - moving the suspend order around is not the correct way to solve
this.  If wm8903 needs to talk to the i2c bus in its suspend handler,
it needs to be child device on the i2c bus.  suspend_noirq is for
devices that must suspend with system irqs turned off, not for
ordering suspend handlers.

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Mark Brown]

On Fri, Aug 05, 2011 at 09:33:31PM -0700, Colin Cross wrote:

Please delete unneeded context from e-mails, it makes it much easier to
find the content.

> NAK - moving the suspend order around is not the correct way to solve
> this.  If wm8903 needs to talk to the i2c bus in its suspend handler,
> it needs to be child device on the i2c bus.  suspend_noirq is for

WM8903 is an I2C device.  The problem is that it's suspended as part of
the ASoC suspend since the audio subsystem is composed of multiple
devices that all need to work together coherently.  I did start doing
some stuff to bodge around this like we do on probe but there are enough
system wide problems with this that it didn't seem worth the complexity
when the existing workarounds are so straightforward.

> devices that must suspend with system irqs turned off, not for
> ordering suspend handlers.

Unfortunately it's the only tool Linux has for dealing with this sort of
issue right now.  We were supposed to be getting support for telling the
PM core about dependencies but Linus didn't like that, and it's possible
Grant's stuff with allowing device binding to be retried to handle cross 
bus dependencies will help.  Right now the Linux model just doesn't have
any understanding of cross device dependencies other than control buses.

One other example of this is that you're also going to have the same
problem with PMICs - random devices on the system might want to control
their regulators as part of the suspend process and typically the PMIC
is going to be connected over I2C so it needs to be available for as
long as possible.

RE: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Stephen Warren]

Mark Brown wrote at Saturday, August 06, 2011 2:48 AM:
> On Fri, Aug 05, 2011 at 09:33:31PM -0700, Colin Cross wrote:
...
> > NAK - moving the suspend order around is not the correct way to solve
> > this.  If wm8903 needs to talk to the i2c bus in its suspend handler,
> > it needs to be child device on the i2c bus.  suspend_noirq is for
>
> WM8903 is an I2C device.  The problem is that it's suspended as part of
> the ASoC suspend since the audio subsystem is composed of multiple
> devices that all need to work together coherently.  I did start doing
> some stuff to bodge around this like we do on probe but there are enough
> system wide problems with this that it didn't seem worth the complexity
> when the existing workarounds are so straightforward.

Colin, given Mark's explanation, are you OK with the patch now?

Thanks.

-- 
nvpublic

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Colin Cross]

On Thu, Aug 11, 2011 at 12:35 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Mark Brown wrote at Saturday, August 06, 2011 2:48 AM:
>> On Fri, Aug 05, 2011 at 09:33:31PM -0700, Colin Cross wrote:
> ...
>> > NAK - moving the suspend order around is not the correct way to solve
>> > this.  If wm8903 needs to talk to the i2c bus in its suspend handler,
>> > it needs to be child device on the i2c bus.  suspend_noirq is for
>>
>> WM8903 is an I2C device.  The problem is that it's suspended as part of
>> the ASoC suspend since the audio subsystem is composed of multiple
>> devices that all need to work together coherently.  I did start doing
>> some stuff to bodge around this like we do on probe but there are enough
>> system wide problems with this that it didn't seem worth the complexity
>> when the existing workarounds are so straightforward.
>
> Colin, given Mark's explanation, are you OK with the patch now?

It's still not the right way to handle this, are you going to mark
every I2C controller as suspend_noirq?  What happens when you find an
I2C controller that needs its irq on to suspend?  These are the kinds
of hacks we've been asked not to do in ARM, so I'd like to see a
response from the I2C maintainers.

RE: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Stephen Warren]

Colin Cross wrote at Thursday, August 11, 2011 2:51 PM:
> Cc: Mark Brown; Ben Dooks; Dilan Lee; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq
> 
> On Thu, Aug 11, 2011 at 12:35 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Mark Brown wrote at Saturday, August 06, 2011 2:48 AM:
> >> On Fri, Aug 05, 2011 at 09:33:31PM -0700, Colin Cross wrote:
> > ...
> >> > NAK - moving the suspend order around is not the correct way to solve
> >> > this.  If wm8903 needs to talk to the i2c bus in its suspend handler,
> >> > it needs to be child device on the i2c bus.  suspend_noirq is for
> >>
> >> WM8903 is an I2C device.  The problem is that it's suspended as part of
> >> the ASoC suspend since the audio subsystem is composed of multiple
> >> devices that all need to work together coherently.  I did start doing
> >> some stuff to bodge around this like we do on probe but there are enough
> >> system wide problems with this that it didn't seem worth the complexity
> >> when the existing workarounds are so straightforward.
> >
> > Colin, given Mark's explanation, are you OK with the patch now?
> 
> It's still not the right way to handle this, are you going to mark
> every I2C controller as suspend_noirq?  What happens when you find an
> I2C controller that needs its irq on to suspend?  These are the kinds
> of hacks we've been asked not to do in ARM, so I'd like to see a
> response from the I2C maintainers.

Well, while that's true, the thing is the right way to handle it doesn't
appear to exist at present.

I did briefly try to move the entire ASoC suspend to early suspend (or
something like that), but ran into a whole slew of issues, that I
unfortunately don't recall right now. That would also impact every user
of ASoC, not just Tegra...

So, in other words, how do you suggest I proceed with this?

Mark Brown wrote:
> Unfortunately it's the only tool Linux has for dealing with this sort of
> issue right now.  We were supposed to be getting support for telling the
> PM core about dependencies but Linus didn't like that,

Mark, can you fill in a little more detail; did Linus not like the concept
of drivers registering dependencies on each-other, or was there a problem
with the specific implementation that was proposed? I assume we're still a
long way away from anything like that working. You also mentioned Grant's
device registration retry stuff, but doesn't that only solve the initial
probing, not shutdown/suspend dependencies, or do devices take locks on
each-other to handle that too?

Thanks.

-- 
nvpubilc

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Colin Cross]

On Thu, Aug 11, 2011 at 2:09 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Colin Cross wrote at Thursday, August 11, 2011 2:51 PM:
>> Cc: Mark Brown; Ben Dooks; Dilan Lee; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq
>>
>> On Thu, Aug 11, 2011 at 12:35 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > Mark Brown wrote at Saturday, August 06, 2011 2:48 AM:
>> >> On Fri, Aug 05, 2011 at 09:33:31PM -0700, Colin Cross wrote:
>> > ...
>> >> > NAK - moving the suspend order around is not the correct way to solve
>> >> > this.  If wm8903 needs to talk to the i2c bus in its suspend handler,
>> >> > it needs to be child device on the i2c bus.  suspend_noirq is for
>> >>
>> >> WM8903 is an I2C device.  The problem is that it's suspended as part of
>> >> the ASoC suspend since the audio subsystem is composed of multiple
>> >> devices that all need to work together coherently.  I did start doing
>> >> some stuff to bodge around this like we do on probe but there are enough
>> >> system wide problems with this that it didn't seem worth the complexity
>> >> when the existing workarounds are so straightforward.
>> >
>> > Colin, given Mark's explanation, are you OK with the patch now?
>>
>> It's still not the right way to handle this, are you going to mark
>> every I2C controller as suspend_noirq?  What happens when you find an
>> I2C controller that needs its irq on to suspend?  These are the kinds
>> of hacks we've been asked not to do in ARM, so I'd like to see a
>> response from the I2C maintainers.
>
> Well, while that's true, the thing is the right way to handle it doesn't
> appear to exist at present.

Yes, but the maintainers have been asked to stop using one-off hacks
to fix everything that isn't supported in common code, and work
towards getting the common code fixed instead.  If the common code
maintainers say this is the best way to fix it for now, fine, but I'd
like to hear an opinion first.

> I did briefly try to move the entire ASoC suspend to early suspend (or
> something like that), but ran into a whole slew of issues, that I
> unfortunately don't recall right now. That would also impact every user
> of ASoC, not just Tegra...
>
> So, in other words, how do you suggest I proceed with this?

device_pm_move_after(deva, devb) makes should make deva suspend before
devb.  Maybe subsystems that link multiple devices together should
call device_pm_move_after() to make sure that all of their devices
suspend before the parents of any of their devices?

> Mark Brown wrote:
>> Unfortunately it's the only tool Linux has for dealing with this sort of
>> issue right now.  We were supposed to be getting support for telling the
>> PM core about dependencies but Linus didn't like that,
>
> Mark, can you fill in a little more detail; did Linus not like the concept
> of drivers registering dependencies on each-other, or was there a problem
> with the specific implementation that was proposed? I assume we're still a
> long way away from anything like that working. You also mentioned Grant's
> device registration retry stuff, but doesn't that only solve the initial
> probing, not shutdown/suspend dependencies, or do devices take locks on
> each-other to handle that too?
>
> Thanks.
>
> --
> nvpubilc
>
>

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Mark Brown]

On Thu, 2011-08-11 at 14:43 -0700, Colin Cross wrote:
> On Thu, Aug 11, 2011 at 2:09 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> > Well, while that's true, the thing is the right way to handle it doesn't
> > appear to exist at present.

> Yes, but the maintainers have been asked to stop using one-off hacks
> to fix everything that isn't supported in common code, and work
> towards getting the common code fixed instead.  If the common code
> maintainers say this is the best way to fix it for now, fine, but I'd
> like to hear an opinion first.

We've been through this quite a few times already at the driver core
level, mostly in the context of PMIC probing. Fixing this properly
requires some surgery on the core device model infrastructure which it's
not realistic to expect driver authors to implement.

> > So, in other words, how do you suggest I proceed with this?

> device_pm_move_after(deva, devb) makes should make deva suspend before
> devb.  Maybe subsystems that link multiple devices together should
> call device_pm_move_after() to make sure that all of their devices
> suspend before the parents of any of their devices?

This wouldn't be a robust solution for bus independent subsystems as it
doesn't maintain any sort of sorting - it just does a direct move so
it'll only capture the last dependency we saw. If you've got two
dependencies we could easily end up breaking things.

For example with ASoC we'd sort all the components before the ASoC card
without regard for their bus dependencies or any other dependencies they
have (eg, their regulators). Since the ASoC card is a platform device
it's likely to have registered early with no regard for where the buses
the card needs are registered. I'd expect there's a reasonable chance
it'll actually make things worse in the short term.

I'll mail Ted and see if we can get this on the topic for KS, it's
getting more and more serious.

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Colin Cross]

On Thu, Aug 11, 2011 at 5:45 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Thu, 2011-08-11 at 14:43 -0700, Colin Cross wrote:
>> On Thu, Aug 11, 2011 at 2:09 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
>> > Well, while that's true, the thing is the right way to handle it doesn't
>> > appear to exist at present.
>
>> Yes, but the maintainers have been asked to stop using one-off hacks
>> to fix everything that isn't supported in common code, and work
>> towards getting the common code fixed instead.  If the common code
>> maintainers say this is the best way to fix it for now, fine, but I'd
>> like to hear an opinion first.
>
> We've been through this quite a few times already at the driver core
> level, mostly in the context of PMIC probing. Fixing this properly
> requires some surgery on the core device model infrastructure which it's
> not realistic to expect driver authors to implement.
>
>> > So, in other words, how do you suggest I proceed with this?
>
>> device_pm_move_after(deva, devb) makes should make deva suspend before
>> devb.  Maybe subsystems that link multiple devices together should
>> call device_pm_move_after() to make sure that all of their devices
>> suspend before the parents of any of their devices?
>
> This wouldn't be a robust solution for bus independent subsystems as it
> doesn't maintain any sort of sorting - it just does a direct move so
> it'll only capture the last dependency we saw. If you've got two
> dependencies we could easily end up breaking things.
>
> For example with ASoC we'd sort all the components before the ASoC card
> without regard for their bus dependencies or any other dependencies they
> have (eg, their regulators). Since the ASoC card is a platform device
> it's likely to have registered early with no regard for where the buses
> the card needs are registered. I'd expect there's a reasonable chance
> it'll actually make things worse in the short term.

You can't just move everything after the card, you have to move
everything after the last device that was probed, and it only works if
nothing depends on any of the devices that are moved.

> I'll mail Ted and see if we can get this on the topic for KS, it's
> getting more and more serious.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Mark Brown]

On Thu, Aug 11, 2011 at 07:59:27PM -0700, Colin Cross wrote:
> On Thu, Aug 11, 2011 at 5:45 PM, Mark Brown

> > For example with ASoC we'd sort all the components before the ASoC card
> > without regard for their bus dependencies or any other dependencies they
> > have (eg, their regulators). Since the ASoC card is a platform device
> > it's likely to have registered early with no regard for where the buses
> > the card needs are registered. I'd expect there's a reasonable chance
> > it'll actually make things worse in the short term.

> You can't just move everything after the card, you have to move
> everything after the last device that was probed, and it only works if
> nothing depends on any of the devices that are moved.

Sorry, I said that the wrong way round due to trying to reply quickly -
the card would be the thing that moves since that's the thing that
actually does the suspend but we've *no* idea which device we need to
move it after.  Since all the function does is a direct move after or
before a single device all we can do is pick one and pray that it's the
right device.

RE: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Stephen Warren]

Mark Brown wrote at Thursday, August 11, 2011 9:15 PM:
> On Thu, Aug 11, 2011 at 07:59:27PM -0700, Colin Cross wrote:
> > On Thu, Aug 11, 2011 at 5:45 PM, Mark Brown
> 
> > > For example with ASoC we'd sort all the components before the ASoC card
> > > without regard for their bus dependencies or any other dependencies they
> > > have (eg, their regulators). Since the ASoC card is a platform device
> > > it's likely to have registered early with no regard for where the buses
> > > the card needs are registered. I'd expect there's a reasonable chance
> > > it'll actually make things worse in the short term.
> 
> > You can't just move everything after the card, you have to move
> > everything after the last device that was probed, and it only works if
> > nothing depends on any of the devices that are moved.
> 
> Sorry, I said that the wrong way round due to trying to reply quickly -
> the card would be the thing that moves since that's the thing that
> actually does the suspend but we've *no* idea which device we need to
> move it after.  Since all the function does is a direct move after or
> before a single device all we can do is pick one and pray that it's the
> right device.

Colin, 

This thread seems to have died down; how should we make progress?

It sounds like the suspend_irq solution is the current de-facto standard;
not optimal, but all we really have right now and already in use. I could
certainly see avoiding this solution if it was the first time it was
employed, but re-using it seems reasonable to me?

Alternatively, are you attending either Linux Plumbers Conference or the
Kernel Summit? Mark implied this topic might well come up for discussion
there. Unfortunately, I won't be able to make LPC due to a conflict.

(and you'd mentioned having the subsystem maintainers weigh in on this;
which sub-system; IRQ, power, I2C, ...?)

Thanks!

-- 
nvpublic

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Colin Cross]

On Wed, Aug 24, 2011 at 2:28 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Mark Brown wrote at Thursday, August 11, 2011 9:15 PM:
>> On Thu, Aug 11, 2011 at 07:59:27PM -0700, Colin Cross wrote:
>> > On Thu, Aug 11, 2011 at 5:45 PM, Mark Brown
>>
>> > > For example with ASoC we'd sort all the components before the ASoC card
>> > > without regard for their bus dependencies or any other dependencies they
>> > > have (eg, their regulators). Since the ASoC card is a platform device
>> > > it's likely to have registered early with no regard for where the buses
>> > > the card needs are registered. I'd expect there's a reasonable chance
>> > > it'll actually make things worse in the short term.
>>
>> > You can't just move everything after the card, you have to move
>> > everything after the last device that was probed, and it only works if
>> > nothing depends on any of the devices that are moved.
>>
>> Sorry, I said that the wrong way round due to trying to reply quickly -
>> the card would be the thing that moves since that's the thing that
>> actually does the suspend but we've *no* idea which device we need to
>> move it after.  Since all the function does is a direct move after or
>> before a single device all we can do is pick one and pray that it's the
>> right device.
>
> Colin,
>
> This thread seems to have died down; how should we make progress?
>
> It sounds like the suspend_irq solution is the current de-facto standard;
> not optimal, but all we really have right now and already in use. I could
> certainly see avoiding this solution if it was the first time it was
> employed, but re-using it seems reasonable to me?
>
> Alternatively, are you attending either Linux Plumbers Conference or the
> Kernel Summit? Mark implied this topic might well come up for discussion
> there. Unfortunately, I won't be able to make LPC due to a conflict.
I don't think I'll be able to make it.

> (and you'd mentioned having the subsystem maintainers weigh in on this;
> which sub-system; IRQ, power, I2C, ...?)

If Ben says its OK, its fine with me.  Or maybe Arnd wants to weigh in?

RE: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Stephen Warren]

Ben, Arnd,

Could you please ack/nack the patch at the start of this thread for Colin;
see below.

Thanks.

Colin Cross wrote at Wednesday, August 24, 2011 3:34 PM:
> On Wed, Aug 24, 2011 at 2:28 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Mark Brown wrote at Thursday, August 11, 2011 9:15 PM:
> >> On Thu, Aug 11, 2011 at 07:59:27PM -0700, Colin Cross wrote:
> >> > On Thu, Aug 11, 2011 at 5:45 PM, Mark Brown
> >>
> >> > > For example with ASoC we'd sort all the components before the ASoC card
> >> > > without regard for their bus dependencies or any other dependencies they
> >> > > have (eg, their regulators). Since the ASoC card is a platform device
> >> > > it's likely to have registered early with no regard for where the buses
> >> > > the card needs are registered. I'd expect there's a reasonable chance
> >> > > it'll actually make things worse in the short term.
> >>
> >> > You can't just move everything after the card, you have to move
> >> > everything after the last device that was probed, and it only works if
> >> > nothing depends on any of the devices that are moved.
> >>
> >> Sorry, I said that the wrong way round due to trying to reply quickly -
> >> the card would be the thing that moves since that's the thing that
> >> actually does the suspend but we've *no* idea which device we need to
> >> move it after.  Since all the function does is a direct move after or
> >> before a single device all we can do is pick one and pray that it's the
> >> right device.
> >
> > Colin,
> >
> > This thread seems to have died down; how should we make progress?
> >
> > It sounds like the suspend_irq solution is the current de-facto standard;
> > not optimal, but all we really have right now and already in use. I could
> > certainly see avoiding this solution if it was the first time it was
> > employed, but re-using it seems reasonable to me?
> >
> > Alternatively, are you attending either Linux Plumbers Conference or the
> > Kernel Summit? Mark implied this topic might well come up for discussion
> > there. Unfortunately, I won't be able to make LPC due to a conflict.
> I don't think I'll be able to make it.
> 
> > (and you'd mentioned having the subsystem maintainers weigh in on this;
> > which sub-system; IRQ, power, I2C, ...?)
> 
> If Ben says its OK, its fine with me.  Or maybe Arnd wants to weigh in?

-- 
nvpublic

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Arnd Bergmann]

On Tuesday 30 August 2011, Stephen Warren wrote:
> Ben, Arnd,
> 
> Could you please ack/nack the patch at the start of this thread for Colin;
> see below.

Sorry, I normally like to give my opinion on everything, but I really don't
have a clue what this one is about. I don't understand i2c or power management ;-)

	Arnd

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Mark Brown]

On Wed, Aug 31, 2011 at 06:28:40PM +0200, Arnd Bergmann wrote:

> Sorry, I normally like to give my opinion on everything, but I really don't
> have a clue what this one is about. I don't understand i2c or power management ;-)

The issue is that due to our sequencing suspend and resume using the
control heirachy rather than dependencies we attempt to suspend the I2C
controller prior to some devices that need it.  As we don't have a good
solution for this at the minute where it's an issue we've been avoiding
it by using the _noirq() suspend and resume functions for the controller
to reorder their suspend with respect to the rest of the system.  It's
not great but practically speaking it works.

RE: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Stephen Warren]

Stephen Warren wrote at Tuesday, August 30, 2011 10:25 AM:
> Ben, Arnd,
> 
> Could you please ack/nack the patch at the start of this thread for Colin;
> see below.

Ben, can you please comment on the acceptability of this patch?

Or Arnd, did Mark's most recent explanation of the situation provide enough
context for you to ack/nak it?

Thanks.

> Thanks.
> 
> Colin Cross wrote at Wednesday, August 24, 2011 3:34 PM:
> > On Wed, Aug 24, 2011 at 2:28 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > > Mark Brown wrote at Thursday, August 11, 2011 9:15 PM:
> > >> On Thu, Aug 11, 2011 at 07:59:27PM -0700, Colin Cross wrote:
> > >> > On Thu, Aug 11, 2011 at 5:45 PM, Mark Brown
> > >>
> > >> > > For example with ASoC we'd sort all the components before the ASoC card
> > >> > > without regard for their bus dependencies or any other dependencies they
> > >> > > have (eg, their regulators). Since the ASoC card is a platform device
> > >> > > it's likely to have registered early with no regard for where the buses
> > >> > > the card needs are registered. I'd expect there's a reasonable chance
> > >> > > it'll actually make things worse in the short term.
> > >>
> > >> > You can't just move everything after the card, you have to move
> > >> > everything after the last device that was probed, and it only works if
> > >> > nothing depends on any of the devices that are moved.
> > >>
> > >> Sorry, I said that the wrong way round due to trying to reply quickly -
> > >> the card would be the thing that moves since that's the thing that
> > >> actually does the suspend but we've *no* idea which device we need to
> > >> move it after.  Since all the function does is a direct move after or
> > >> before a single device all we can do is pick one and pray that it's the
> > >> right device.
> > >
> > > Colin,
> > >
> > > This thread seems to have died down; how should we make progress?
> > >
> > > It sounds like the suspend_irq solution is the current de-facto standard;
> > > not optimal, but all we really have right now and already in use. I could
> > > certainly see avoiding this solution if it was the first time it was
> > > employed, but re-using it seems reasonable to me?
> > >
> > > Alternatively, are you attending either Linux Plumbers Conference or the
> > > Kernel Summit? Mark implied this topic might well come up for discussion
> > > there. Unfortunately, I won't be able to make LPC due to a conflict.
> > I don't think I'll be able to make it.
> >
> > > (and you'd mentioned having the subsystem maintainers weigh in on this;
> > > which sub-system; IRQ, power, I2C, ...?)
> >
> > If Ben says its OK, its fine with me.  Or maybe Arnd wants to weigh in?

--
nvpublic

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Arnd Bergmann]

On Tuesday 20 September 2011, Stephen Warren wrote:
> Stephen Warren wrote at Tuesday, August 30, 2011 10:25 AM:
> > Ben, Arnd,
> > 
> > Could you please ack/nack the patch at the start of this thread for Colin;
> > see below.
> 
> Ben, can you please comment on the acceptability of this patch?
> 
> Or Arnd, did Mark's most recent explanation of the situation provide enough
> context for you to ack/nak it?
> 

After looking through the discussion again, my feeling is that it's ok
and I think that Mark's word should count more than mine anyway on this
issue.

If you think it helps, you can add my Acked-by, but this is really
something for Ben to decide. Just resubmit the patch to Ben with an
an updated changelog that summarises the discussion, in case
Ben got bored already and put this mail thread into the ignore
folder...

	Arnd

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Mark Brown]

On Wed, Aug 24, 2011 at 02:28:29PM -0700, Stephen Warren wrote:

> (and you'd mentioned having the subsystem maintainers weigh in on this;
> which sub-system; IRQ, power, I2C, ...?)

I'm one of the regulator API subsystem maintainers FWIW.

RE: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Mark Brown]

On Thu, 2011-08-11 at 14:09 -0700, Stephen Warren wrote:
> Colin Cross wrote at Thursday, August 11, 2011 2:51 PM:

> Mark Brown wrote:
> > Unfortunately it's the only tool Linux has for dealing with this sort of
> > issue right now.  We were supposed to be getting support for telling the
> > PM core about dependencies but Linus didn't like that,

> Mark, can you fill in a little more detail; did Linus not like the concept
> of drivers registering dependencies on each-other, or was there a problem
> with the specific implementation that was proposed? I assume we're still a

He didn't like either, I think - he felt that probe ordering should be
sufficient. Which is true within the device model, in that devices can't
probe until all their resources are ready. This is coming up for ASoC
because that mechanism just doesn't work at all for us an we have to
work around it.

> long way away from anything like that working. You also mentioned Grant's
> device registration retry stuff, but doesn't that only solve the initial
> probing, not shutdown/suspend dependencies, or do devices take locks on
> each-other to handle that too?

Our current mechanism is based entirely on the order in which things get
instantiated so depending on the implementation of deferred binding we
may be able to change the ordering of the dpm_list.

Re: [PATCH] i2c/tegra: I2C driver uses the suspend_noirq/resume_noirq

[Mark Brown]

On Fri, Aug 05, 2011 at 05:15:02PM -0600, Stephen Warren wrote:
> From: Dilan Lee <dilee-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> We found the register settings of wm8903(an audio codec) can't be modified
> in snd_soc_suspend since I2C bus has been suspended before snd_soc_suspend.
> 
> Pop noise will occur when system resume from LP0 if the register settings of wm8903
> haven't be modified correctly in snd_soc_suspend.
> 
> So, we use the suspend_noirq/resume_noirq callbacks to make sure I2C bus still
> operates while running snd_soc_suspend.
> 
> Signed-off-by: Dilan Lee <dilee-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

At Stephen's request

Reviewed-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>