| 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.
* 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 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 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 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
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,