mbox series

[00/14] Add suspend to ram support for PCIe on J7200

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

Message

Thomas Richard Jan. 15, 2024, 4:14 p.m. UTC
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,

Comments

Andy Shevchenko Jan. 15, 2024, 8:02 p.m. UTC | #1
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)?
Andy Shevchenko Jan. 15, 2024, 8:05 p.m. UTC | #2
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?
Andy Shevchenko Jan. 15, 2024, 8:13 p.m. UTC | #3
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.
Peter Rosin Jan. 15, 2024, 10:31 p.m. UTC | #4
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,
>  };
>
Tony Lindgren Jan. 16, 2024, 7:43 a.m. UTC | #5
* 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, 4:08 p.m. UTC | #6
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).
Andy Shevchenko Jan. 19, 2024, 4:11 p.m. UTC | #7
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.
Thomas Richard Jan. 19, 2024, 4:25 p.m. UTC | #8
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,
Thomas Richard Jan. 19, 2024, 5:01 p.m. UTC | #9
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,
Thomas Richard Jan. 22, 2024, 2:33 p.m. UTC | #10
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.
Andy Shevchenko Jan. 22, 2024, 9:36 p.m. UTC | #11
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.
Andy Shevchenko Jan. 23, 2024, 12:44 p.m. UTC | #12
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.
Thomas Richard Jan. 24, 2024, 2:09 p.m. UTC | #13
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.
Thomas Richard Feb. 8, 2024, 4:19 p.m. UTC | #14
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 | #15
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 | #16
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 | #17
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 | #18
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