diff mbox series

[01/14] gpio: pca953x: move suspend/resume to suspend_noirq/resume_noirq

Message ID 20240102-j7200-pcie-s2r-v1-1-84e55da52400@bootlin.com
State Superseded
Headers show
Series Add suspend to ram support for PCIe on J7200 | expand

Commit Message

Thomas Richard Jan. 15, 2024, 4:14 p.m. UTC
Some IOs can be needed during suspend_noirq/resume_noirq.
So move suspend/resume callbacks to noirq.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/gpio/gpio-pca953x.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Tony Lindgren Jan. 16, 2024, 7:43 a.m. UTC | #1
* Thomas Richard <thomas.richard@bootlin.com> [240115 16:16]:
> Some IOs can be needed during suspend_noirq/resume_noirq.
> So move suspend/resume callbacks to noirq.

So have you checked that the pca953x_save_context() and restore works
this way? There's i2c traffic and regulators may sleep.. I wonder if
you instead just need to leave gpio-pca953x enabled in some cases
instead?

Regards,

Tony
Thomas Richard Jan. 19, 2024, 5:01 p.m. UTC | #2
Hello Tony,

On 1/16/24 08:43, Tony Lindgren wrote:
> * Thomas Richard <thomas.richard@bootlin.com> [240115 16:16]:
>> Some IOs can be needed during suspend_noirq/resume_noirq.
>> So move suspend/resume callbacks to noirq.
> 
> So have you checked that the pca953x_save_context() and restore works
> this way? There's i2c traffic and regulators may sleep.. I wonder if
> you instead just need to leave gpio-pca953x enabled in some cases
> instead?
> 

Yes I tested it, and it works (with my setup).
But this patch may have an impact for other people.
How could I leave it enabled in some cases ?

Regards,
Linus Walleij Jan. 28, 2024, 12:12 a.m. UTC | #3
On Fri, Jan 19, 2024 at 6:01 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> On 1/16/24 08:43, Tony Lindgren wrote:
> > * Thomas Richard <thomas.richard@bootlin.com> [240115 16:16]:
> >> Some IOs can be needed during suspend_noirq/resume_noirq.
> >> So move suspend/resume callbacks to noirq.
> >
> > So have you checked that the pca953x_save_context() and restore works
> > this way? There's i2c traffic and regulators may sleep.. I wonder if
> > you instead just need to leave gpio-pca953x enabled in some cases
> > instead?
> >
>
> Yes I tested it, and it works (with my setup).
> But this patch may have an impact for other people.
> How could I leave it enabled in some cases ?

I guess you could define both pca953x_suspend() and
pca953x_suspend_noirq() and selectively bail out on one
path on some systems?

Worst case using if (of_machine_is_compatible("my,machine"))...

Yours,
Linus Walleij
Thomas Richard Feb. 8, 2024, 4:19 p.m. UTC | #4
On 1/28/24 01:12, Linus Walleij wrote:
> On Fri, Jan 19, 2024 at 6:01 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>> On 1/16/24 08:43, Tony Lindgren wrote:
>>> * Thomas Richard <thomas.richard@bootlin.com> [240115 16:16]:
>>>> Some IOs can be needed during suspend_noirq/resume_noirq.
>>>> So move suspend/resume callbacks to noirq.
>>>
>>> So have you checked that the pca953x_save_context() and restore works
>>> this way? There's i2c traffic and regulators may sleep.. I wonder if
>>> you instead just need to leave gpio-pca953x enabled in some cases
>>> instead?
>>>
>>
>> Yes I tested it, and it works (with my setup).
>> But this patch may have an impact for other people.
>> How could I leave it enabled in some cases ?
> 
> I guess you could define both pca953x_suspend() and
> pca953x_suspend_noirq() and selectively bail out on one
> path on some systems?

Yes.

What do you think if I use a property like for example "ti,pm-noirq" to
select the right path ?
Is a property relevant for this use case ?

Regards,

> 
> Worst case using if (of_machine_is_compatible("my,machine"))...
> 
> Yours,
> Linus Walleij
Bartosz Golaszewski Feb. 8, 2024, 4:53 p.m. UTC | #5
On Thu, Feb 8, 2024 at 5:19 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> On 1/28/24 01:12, Linus Walleij wrote:
> > On Fri, Jan 19, 2024 at 6:01 PM Thomas Richard
> > <thomas.richard@bootlin.com> wrote:
> >> On 1/16/24 08:43, Tony Lindgren wrote:
> >>> * Thomas Richard <thomas.richard@bootlin.com> [240115 16:16]:
> >>>> Some IOs can be needed during suspend_noirq/resume_noirq.
> >>>> So move suspend/resume callbacks to noirq.
> >>>
> >>> So have you checked that the pca953x_save_context() and restore works
> >>> this way? There's i2c traffic and regulators may sleep.. I wonder if
> >>> you instead just need to leave gpio-pca953x enabled in some cases
> >>> instead?
> >>>
> >>
> >> Yes I tested it, and it works (with my setup).
> >> But this patch may have an impact for other people.
> >> How could I leave it enabled in some cases ?
> >
> > I guess you could define both pca953x_suspend() and
> > pca953x_suspend_noirq() and selectively bail out on one
> > path on some systems?
>
> Yes.
>
> What do you think if I use a property like for example "ti,pm-noirq" to
> select the right path ?
> Is a property relevant for this use case ?
>

I prefer a new property than calling of_machine_is_compatible().
Please do run it by the DT maintainers, I think it should be fine.
Maybe even don't limit it to TI but make it a generic property.

Bart

> Regards,
>
> >
> > Worst case using if (of_machine_is_compatible("my,machine"))...
> >
> > Yours,
> > Linus Walleij
> --
> Thomas Richard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
Linus Walleij Feb. 8, 2024, 9:29 p.m. UTC | #6
On Thu, Feb 8, 2024 at 5:19 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
> On 1/28/24 01:12, Linus Walleij wrote:

> > I guess you could define both pca953x_suspend() and
> > pca953x_suspend_noirq() and selectively bail out on one
> > path on some systems?
>
> Yes.
>
> What do you think if I use a property like for example "ti,pm-noirq" to
> select the right path ?
> Is a property relevant for this use case ?

That's a Linux-specific property and that's useless for other operating
systems and not normally allowed. PM noirq is just some Linux thing.

*FIRST* we should check if putting the callbacks to noirq is fine with
other systems too, and I don't see why not. Perhaps we need to even
merge it if we don't get any test results.

If it doesn't work we can think of other options.

Yours,
Linus Walleij
Thomas Richard Feb. 9, 2024, 7:44 a.m. UTC | #7
On 2/8/24 22:29, Linus Walleij wrote:
> On Thu, Feb 8, 2024 at 5:19 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>> On 1/28/24 01:12, Linus Walleij wrote:
> 
>>> I guess you could define both pca953x_suspend() and
>>> pca953x_suspend_noirq() and selectively bail out on one
>>> path on some systems?
>>
>> Yes.
>>
>> What do you think if I use a property like for example "ti,pm-noirq" to
>> select the right path ?
>> Is a property relevant for this use case ?
> 
> That's a Linux-specific property and that's useless for other operating
> systems and not normally allowed. PM noirq is just some Linux thing.
> 
> *FIRST* we should check if putting the callbacks to noirq is fine with
> other systems too, and I don't see why not. Perhaps we need to even
> merge it if we don't get any test results.
> 
> If it doesn't work we can think of other options.

I think all systems using a i2c controller which uses autosuspend should
be impacted.
I guess a patch (like I did in this series for i2c-omap [1]) should be
applied for all i2c controller which use autosuspend.

[1]
https://lore.kernel.org/all/hqnxyffdsiqz5t43bexcqrwmynpjubxbzjchjaagxecso75dc7@y7lznovxg3go/

Regards,
Linus Walleij Feb. 9, 2024, 10:50 a.m. UTC | #8
On Fri, Feb 9, 2024 at 8:44 AM Thomas Richard
<thomas.richard@bootlin.com> wrote:

> > *FIRST* we should check if putting the callbacks to noirq is fine with
> > other systems too, and I don't see why not. Perhaps we need to even
> > merge it if we don't get any test results.
> >
> > If it doesn't work we can think of other options.
>
> I think all systems using a i2c controller which uses autosuspend should
> be impacted.
> I guess a patch (like I did in this series for i2c-omap [1]) should be
> applied for all i2c controller which use autosuspend.
>
> [1]
> https://lore.kernel.org/all/hqnxyffdsiqz5t43bexcqrwmynpjubxbzjchjaagxecso75dc7@y7lznovxg3go/

I think this is the right thing to do.

Maybe we should just go over all of them? (Also SPI controllers?)

We will soon merge a patch to move the pinctrl-single PM to noirq, and
that actually affects more than just OMAP, it also has effect on e.g.
HiSilicon so we can expect a bit of shakeout unless we take a global
approach to this.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 00ffa168e405..83da5d213c55 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -1234,7 +1234,7 @@  static void pca953x_save_context(struct pca953x_chip *chip)
 	regcache_cache_only(chip->regmap, true);
 }
 
-static int pca953x_suspend(struct device *dev)
+static int pca953x_suspend_noirq(struct device *dev)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 
@@ -1248,7 +1248,7 @@  static int pca953x_suspend(struct device *dev)
 	return 0;
 }
 
-static int pca953x_resume(struct device *dev)
+static int pca953x_resume_noirq(struct device *dev)
 {
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 	int ret;
@@ -1268,7 +1268,9 @@  static int pca953x_resume(struct device *dev)
 	return ret;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume);
+static const struct dev_pm_ops pca953x_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pca953x_suspend_noirq, pca953x_resume_noirq)
+};
 
 /* convenience to stop overlong match-table lines */
 #define OF_653X(__nrgpio, __int) ((void *)(__nrgpio | PCAL653X_TYPE | __int))