| Message ID | 20240102-j7200-pcie-s2r-v1-1-84e55da52400@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add suspend to ram support for PCIe on J7200 | expand |
On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > Some IOs can be needed during suspend_noirq/resume_noirq. ->suspend_noirq() / ->resume_noirq() > So move suspend/resume callbacks to noirq. ... > -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) > +}; Please, use correct / modern macro.
* 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
On 1/15/24 20:56, Andy Shevchenko wrote: > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> >> Some IOs can be needed during suspend_noirq/resume_noirq. > > ->suspend_noirq() / ->resume_noirq() > >> So move suspend/resume callbacks to noirq. > > ... > >> -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) >> +}; > > Please, use correct / modern macro. > Hello Andy, Thanks for the reviews. I applied your comments for the next iteration. Regards,
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,
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
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
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 >
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
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,
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 --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))
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(-)