| Message ID | 20240102-j7200-pcie-s2r-v1-0-84e55da52400@bootlin.com |
|---|---|
| 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: > > The goal is to extend the active period of pinctrl. > Some devices may need active pinctrl after suspend and/or before resume. > So move suspend/resume to suspend_noirq/resume_noirq to have active > pinctrl until suspend_noirq (included), and from resume_noirq > (included). ->...callback...() (Same comment I have given for the first patch) ... > struct pcs_device *pcs; > > - pcs = platform_get_drvdata(pdev); > + pcs = dev_get_drvdata(dev); > if (!pcs) > return -EINVAL; Drop dead code. This should be simple one line after your change. struct pcs_device *pcs = dev_get_drvdata(dev); ... > struct pcs_device *pcs; > > - pcs = platform_get_drvdata(pdev); > + pcs = dev_get_drvdata(dev); > if (!pcs) > return -EINVAL; Ditto. ... > +static const struct dev_pm_ops pinctrl_single_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, > + pinctrl_single_resume_noirq) > +}; Use proper / modern macro. ... > #endif Why ifdeferry is needed (esp. taking into account pm_ptr() use below)?
On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Implement resume support Why? ... > +#ifdef CONFIG_PM No ifdeffery, use proper macros. ... > + dev_err(dev, "control %u: error restoring mux: %d\n", i, ret); Won't anything go wrong and spam the log buffer?
On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Add suspend and resume support for rc mode. Same comments as for earlier patches. Since it's wide, please, check the whole series for the same issues and address them. ... > + if (pcie->reset_gpio) Dup, why? > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); ... > + if (pcie->reset_gpio) { > + usleep_range(100, 200); fsleep() ? Btw, why is it needed here, perhaps a comment? > + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > + } ... > + ret = cdns_pcie_host_setup(rc, false); > + if (ret < 0) { > + clk_disable_unprepare(pcie->refclk); > + return -ENODEV; Why is the error code being shadowed? > + } ... > +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) Is container_of.h included in this file? ... > @@ -381,7 +383,6 @@ struct cdns_pcie_ep { > unsigned int quirk_disable_flr:1; > }; > > - > /* Register access */ > static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) > { Stray change.
Hi! 2024-01-15 at 17:14, Thomas Richard wrote: > From: Théo Lebrun <theo.lebrun@bootlin.com> > > Implement resume support What Andy said, and please don't omit punctuation. Try to make it a pleasure to read your patches! > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> > --- > drivers/mux/mmio.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c > index fd1d121a584b..ab4ef195fc0d 100644 > --- a/drivers/mux/mmio.c > +++ b/drivers/mux/mmio.c > @@ -125,13 +125,47 @@ static int mux_mmio_probe(struct platform_device *pdev) > > mux_chip->ops = &mux_mmio_ops; > > + dev_set_drvdata(dev, mux_chip); > + > return devm_mux_chip_register(dev, mux_chip); > } > > +#ifdef CONFIG_PM > +static int mux_mmio_resume_noirq(struct device *dev) > +{ > + struct mux_chip *mux_chip = dev_get_drvdata(dev); > + int global_ret = 0; > + unsigned int i; > + > + for (i = 0; i < mux_chip->controllers; i++) { > + struct mux_control *mux = &mux_chip->mux[i]; > + int val = mux->cached_state; You are not supposed to look at (or change) cached_state outside the mux core. > + int ret; > + > + if (val == MUX_IDLE_AS_IS) The cached_state can never be MUX_IDLE_AS_IS. Sure, it happens to have the same actual value as the correct MUX_CACHE_UNKNOWN, but abusing that is all kinds of wrong. > + continue; > + > + ret = mux_mmio_set(mux, val); > + if (ret) { If mux_mmio_set() fails, cached_state ends up wrong as it should be set to MUX_CACHE_UNKNOWN on failure. Low-level stuff like this needs to be done by the mux core, or things becomes a maintenance hazard... So, the meat of this function belongs in the mux core since none of it looks mmio specific. It could probably be named mux_chip_resume() or something such. That makes it simple to use the correct constant, and the mux_control_set() helper makes it easy to get the handling of cached_state right. Cheers, Peter > + dev_err(dev, "control %u: error restoring mux: %d\n", i, ret); > + if (!global_ret) > + global_ret = ret; > + } > + } > + > + return global_ret; > +} > +#endif > + > +static const struct dev_pm_ops mux_mmio_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, mux_mmio_resume_noirq) > +}; > + > static struct platform_driver mux_mmio_driver = { > .driver = { > .name = "mmio-mux", > .of_match_table = mux_mmio_dt_ids, > + .pm = &mux_mmio_pm_ops, > }, > .probe = mux_mmio_probe, > }; >
* 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 21:02, Andy Shevchenko wrote: > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> >> The goal is to extend the active period of pinctrl. >> Some devices may need active pinctrl after suspend and/or before resume. >> So move suspend/resume to suspend_noirq/resume_noirq to have active >> pinctrl until suspend_noirq (included), and from resume_noirq >> (included). > > ->...callback...() > (Same comment I have given for the first patch) fixed > > ... > >> struct pcs_device *pcs; >> >> - pcs = platform_get_drvdata(pdev); >> + pcs = dev_get_drvdata(dev); >> if (!pcs) >> return -EINVAL; > > Drop dead code. > This should be simple one line after your change. > > struct pcs_device *pcs = dev_get_drvdata(dev); > dead code dropped > ... > >> struct pcs_device *pcs; >> >> - pcs = platform_get_drvdata(pdev); >> + pcs = dev_get_drvdata(dev); >> if (!pcs) >> return -EINVAL; > > Ditto. > > ... dead code dropped > >> +static const struct dev_pm_ops pinctrl_single_pm_ops = { >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, >> + pinctrl_single_resume_noirq) >> +}; > > Use proper / modern macro. fixed, use DEFINE_NOIRQ_DEV_PM_OPS now > > ... > >> #endif > > Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? We may have an "unused variable" warning for pinctrl_single_pm_ops if CONFIG_PM is undefined (due to pm_ptr).
On Fri, Jan 19, 2024 at 6:08 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 1/15/24 21:02, Andy Shevchenko wrote: > > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: ... > >> +static const struct dev_pm_ops pinctrl_single_pm_ops = { > >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, > >> + pinctrl_single_resume_noirq) > >> +}; > > > > Use proper / modern macro. > > fixed, use DEFINE_NOIRQ_DEV_PM_OPS now ... > >> #endif > > > > Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? > > We may have an "unused variable" warning for pinctrl_single_pm_ops if > CONFIG_PM is undefined (due to pm_ptr). This is coupled with the above. Fixing above will automatically make the right thing.
Hello Peter, Thanks for the review. On 1/15/24 23:31, Peter Rosin wrote: > Hi! > > 2024-01-15 at 17:14, Thomas Richard wrote: >> From: Théo Lebrun <theo.lebrun@bootlin.com> >> >> Implement resume support > > What Andy said, and please don't omit punctuation. Try to make it a > pleasure to read your patches! Yes my commit message needs to be more verbose, sorry. > >> >> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> >> --- >> drivers/mux/mmio.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c >> index fd1d121a584b..ab4ef195fc0d 100644 >> --- a/drivers/mux/mmio.c >> +++ b/drivers/mux/mmio.c >> @@ -125,13 +125,47 @@ static int mux_mmio_probe(struct platform_device *pdev) >> >> mux_chip->ops = &mux_mmio_ops; >> >> + dev_set_drvdata(dev, mux_chip); >> + >> return devm_mux_chip_register(dev, mux_chip); >> } >> >> +#ifdef CONFIG_PM >> +static int mux_mmio_resume_noirq(struct device *dev) >> +{ >> + struct mux_chip *mux_chip = dev_get_drvdata(dev); >> + int global_ret = 0; >> + unsigned int i; >> + >> + for (i = 0; i < mux_chip->controllers; i++) { >> + struct mux_control *mux = &mux_chip->mux[i]; >> + int val = mux->cached_state; > > You are not supposed to look at (or change) cached_state outside the > mux core. > >> + int ret; >> + >> + if (val == MUX_IDLE_AS_IS) > > The cached_state can never be MUX_IDLE_AS_IS. Sure, it happens to have > the same actual value as the correct MUX_CACHE_UNKNOWN, but abusing > that is all kinds of wrong. > >> + continue; >> + >> + ret = mux_mmio_set(mux, val); >> + if (ret) { > > If mux_mmio_set() fails, cached_state ends up wrong as it should be set > to MUX_CACHE_UNKNOWN on failure. Low-level stuff like this needs to be > done by the mux core, or things becomes a maintenance hazard... > > So, the meat of this function belongs in the mux core since none of > it looks mmio specific. It could probably be named mux_chip_resume() > or something such. That makes it simple to use the correct constant, > and the mux_control_set() helper makes it easy to get the handling of > cached_state right. > Thanks for the explanations. So I created a mux_chip_resume function in the mux core. This function restores each mux using mux_control_set. The restored state is the cached state. The muxes with a MUX_CACHE_UNKNOWN cache state are ignored. So this patch will be splitted, one patch for the core, one for the mmio driver. 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 1/19/24 17:11, Andy Shevchenko wrote: > On Fri, Jan 19, 2024 at 6:08 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> On 1/15/24 21:02, Andy Shevchenko wrote: >>> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard >>> <thomas.richard@bootlin.com> wrote: > > ... > >>>> +static const struct dev_pm_ops pinctrl_single_pm_ops = { >>>> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, >>>> + pinctrl_single_resume_noirq) >>>> +}; >>> >>> Use proper / modern macro. >> >> fixed, use DEFINE_NOIRQ_DEV_PM_OPS now > > ... > >>>> #endif >>> >>> Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? >> >> We may have an "unused variable" warning for pinctrl_single_pm_ops if >> CONFIG_PM is undefined (due to pm_ptr). > > This is coupled with the above. Fixing above will automatically make > the right thing. Yes you're right. By the way I can use pm_sleep_ptr instead of pm_ptr.
On Mon, Jan 22, 2024 at 5:30 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 1/15/24 21:13, Andy Shevchenko wrote: > > On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: ... > >> + if (pcie->reset_gpio) > > > > Dup, why? > > This pcie->reset_gpio corresponds to PERST# of PCIe endpoints. > I assert it during suspend, because I have to deassert it (with a delay) > during resume stage [1]. Ah, sorry for being unclear, I meant that gpiod_set_value*() already has that check, you don't need it here. > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); ... > >> + if (pcie->reset_gpio) { > >> + usleep_range(100, 200); > > > > fsleep() ? > > Btw, why is it needed here, perhaps a comment? > > The comment should be the same than in the probe [1]. > Should I copy it? Or should I just add a reference to the probe? > > [1] > https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pci-j721e.c#L535 Either way works for me. > >> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); > >> + } ... > >> +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) > > > > Is container_of.h included in this file? > > linux/container_of.h is included in linux/kernel.h. > And linux/kernel.h is included in pcie-cadence.h > (https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pcie-cadence.h#L9). Okay, so, try to clean up pcie-cadence.h so it won't use "proxy" headers. There is an IWYU (include what you use) principle, please follow it.
On Mon, Jan 22, 2024 at 4:33 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 1/19/24 17:11, Andy Shevchenko wrote: > > On Fri, Jan 19, 2024 at 6:08 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: > >> On 1/15/24 21:02, Andy Shevchenko wrote: > >>> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard > >>> <thomas.richard@bootlin.com> wrote: ... > >>>> +static const struct dev_pm_ops pinctrl_single_pm_ops = { > >>>> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq, > >>>> + pinctrl_single_resume_noirq) > >>>> +}; > >>> > >>> Use proper / modern macro. > >> > >> fixed, use DEFINE_NOIRQ_DEV_PM_OPS now > > > > ... > > > >>>> #endif > >>> > >>> Why ifdeferry is needed (esp. taking into account pm_ptr() use below)? > >> > >> We may have an "unused variable" warning for pinctrl_single_pm_ops if > >> CONFIG_PM is undefined (due to pm_ptr). > > > > This is coupled with the above. Fixing above will automatically make > > the right thing. > > Yes you're right. > By the way I can use pm_sleep_ptr instead of pm_ptr. Yes, pm_sleep_ptr() is the correct one in this case.
On 1/22/24 22:36, Andy Shevchenko wrote: > On Mon, Jan 22, 2024 at 5:30 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> On 1/15/24 21:13, Andy Shevchenko wrote: >>> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard >>> <thomas.richard@bootlin.com> wrote: > > ... > >>>> + if (pcie->reset_gpio) >>> >>> Dup, why? >> >> This pcie->reset_gpio corresponds to PERST# of PCIe endpoints. >> I assert it during suspend, because I have to deassert it (with a delay) >> during resume stage [1]. > > Ah, sorry for being unclear, I meant that gpiod_set_value*() already > has that check, you don't need it here. ok understood, check is useless. > >>>> + gpiod_set_value_cansleep(pcie->reset_gpio, 0); > > ... > >>>> + if (pcie->reset_gpio) { >>>> + usleep_range(100, 200); >>> >>> fsleep() ? >>> Btw, why is it needed here, perhaps a comment? >> >> The comment should be the same than in the probe [1]. >> Should I copy it? Or should I just add a reference to the probe? >> >> [1] >> https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pci-j721e.c#L535 > > Either way works for me. > >>>> + gpiod_set_value_cansleep(pcie->reset_gpio, 1); >>>> + } > > ... > >>>> +#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie) >>> >>> Is container_of.h included in this file? >> >> linux/container_of.h is included in linux/kernel.h. >> And linux/kernel.h is included in pcie-cadence.h >> (https://elixir.bootlin.com/linux/v6.8-rc1/source/drivers/pci/controller/cadence/pcie-cadence.h#L9). > > Okay, so, try to clean up pcie-cadence.h so it won't use "proxy" headers. > There is an IWYU (include what you use) principle, please follow it. In fact, as cdns_pcie_to_rc is only used in pci-j721e.c, no need to have it in a header file. I prefer to move cdns_pcie_to_rc definition in pci-j721e.c. As I don't modify pcie-cadence.h, the cleanup to not use proxy headers is something that it can be done outside this series.
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
This add suspend to ram support for the PCIe (RC mode) on J7200 platform. In RC mode, the reset pin for endpoints is managed by a gpio expander on a i2c bus. This pin shall be managed in suspend_noirq and resume_noirq. The suspend/resume has been moved to suspend_noirq/resume_noirq for pca953x (expander) and pinctrl-single. To do i2c accesses during suspend_noirq/resume_noirq, we need to force the wakeup of the i2c controller (which is autosuspended) during suspend callback. It's the only way to wakeup the controller if it's autosuspended, as runtime pm is disabled in suspend_noirq and resume_noirq. Signed-off-by: Thomas Richard <thomas.richard@bootlin.com> --- Thomas Richard (10): gpio: pca953x: move suspend/resume to suspend_noirq/resume_noirq pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq i2c: omap: wakeup the controller during suspend callback phy: ti: phy-j721e-wiz: make wiz_clock_init callable multiple times phy: ti: phy-j721e-wiz: add resume support phy: cadence-torrent: extract calls to clk_get from cdns_torrent_clk phy: cadence-torrent: register resets even if the phy is already configured phy: cadence-torrent: move already_configured to struct cdns_torrent_phy phy: cadence-torrent: remove noop_ops phy operations phy: cadence-torrent: add suspend and resume support Théo Lebrun (4): mux: mmio: Add resume support PCI: cadence: add resume support to cdns_pcie_host_setup() PCI: j721e: move reset GPIO to device struct PCI: j721e: add suspend and resume support drivers/gpio/gpio-pca953x.c | 8 +- drivers/i2c/busses/i2c-omap.c | 15 +++ drivers/mux/mmio.c | 34 ++++++ drivers/pci/controller/cadence/pci-j721e.c | 86 ++++++++++++-- drivers/pci/controller/cadence/pcie-cadence-host.c | 49 ++++---- drivers/pci/controller/cadence/pcie-cadence-plat.c | 2 +- drivers/pci/controller/cadence/pcie-cadence.h | 7 +- drivers/phy/cadence/phy-cadence-torrent.c | 125 +++++++++++++++------ drivers/phy/ti/phy-j721e-wiz.c | 99 ++++++++++++---- drivers/pinctrl/pinctrl-single.c | 19 ++-- 10 files changed, 342 insertions(+), 102 deletions(-) --- base-commit: 00ff0f9ce40db8e64fe16c424a965fd7ab769c42 change-id: 20240102-j7200-pcie-s2r-ecb1a979e357 Best regards,