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

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

[Thomas Richard]

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,
-- 
Thomas Richard <thomas.richard@bootlin.com>

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

[Thomas Richard]

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(-)

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))

-- 
2.39.2

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Thomas Richard]

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,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Tony Lindgren]

* 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

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

[Thomas Richard]

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, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Linus Walleij]

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

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

[Thomas Richard]

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
-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Bartosz Golaszewski]

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
>

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

[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:

> > 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

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

[Thomas Richard]

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,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Linus Walleij]

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

[PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Thomas Richard]

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).

The deprecated API has been removed to use the new one (dev_pm_ops struct).

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pinctrl/pinctrl-single.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 19cc0db771a5..4ad0f66cf42a 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1690,12 +1690,11 @@ static void pcs_restore_context(struct pcs_device *pcs)
 	}
 }
 
-static int pinctrl_single_suspend(struct platform_device *pdev,
-					pm_message_t state)
+static int pinctrl_single_suspend_noirq(struct device *dev)
 {
 	struct pcs_device *pcs;
 
-	pcs = platform_get_drvdata(pdev);
+	pcs = dev_get_drvdata(dev);
 	if (!pcs)
 		return -EINVAL;
 
@@ -1710,11 +1709,11 @@ static int pinctrl_single_suspend(struct platform_device *pdev,
 	return pinctrl_force_sleep(pcs->pctl);
 }
 
-static int pinctrl_single_resume(struct platform_device *pdev)
+static int pinctrl_single_resume_noirq(struct device *dev)
 {
 	struct pcs_device *pcs;
 
-	pcs = platform_get_drvdata(pdev);
+	pcs = dev_get_drvdata(dev);
 	if (!pcs)
 		return -EINVAL;
 
@@ -1723,6 +1722,11 @@ static int pinctrl_single_resume(struct platform_device *pdev)
 
 	return pinctrl_force_default(pcs->pctl);
 }
+
+static const struct dev_pm_ops pinctrl_single_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq,
+				      pinctrl_single_resume_noirq)
+};
 #endif
 
 /**
@@ -1986,11 +1990,8 @@ static struct platform_driver pcs_driver = {
 	.driver = {
 		.name		= DRIVER_NAME,
 		.of_match_table	= pcs_of_match,
+		.pm = pm_ptr(&pinctrl_single_pm_ops),
 	},
-#ifdef CONFIG_PM
-	.suspend = pinctrl_single_suspend,
-	.resume = pinctrl_single_resume,
-#endif
 };
 
 module_platform_driver(pcs_driver);

-- 
2.39.2

Re: [PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Andy Shevchenko]

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)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Thomas Richard]

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).

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Thomas Richard]

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.

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Linus Walleij]

On Mon, Jan 15, 2024 at 5: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).
>
> The deprecated API has been removed to use the new one (dev_pm_ops struct).
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

This needs testing on OMAP and ACK from Tony before I can merge it.
Preferably Haojian should test it too, this is a pretty serious semantic
change.

Yours,
Linus Walleij

[PATCH 03/14] i2c: omap: wakeup the controller during suspend callback

[Thomas Richard]

A device may need the controller up during suspend_noirq or
resume_noirq.
But if the controller is autosuspended, there is no way to wakeup it
during suspend_noirq or resume_noirq because runtime pm is disabled
at this time.

The suspend callback wakes up the controller, so it is available until
its suspend_noirq callback (pm_runtime_force_suspend).
During the resume, it's restored by resume_noirq callback
(pm_runtime_force_resume). Then resume callback enables autosuspend.

So the controller is up during a little time slot in suspend and resume
sequences even if it's not used.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/i2c/busses/i2c-omap.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 42165ef57946..57f0738fa748 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1575,9 +1575,24 @@ static int __maybe_unused omap_i2c_runtime_resume(struct device *dev)
 	return 0;
 }
 
+
+static int omap_i2c_suspend(struct device *dev)
+{
+	return pm_runtime_resume_and_get(dev);
+}
+
+static int omap_i2c_resume(struct device *dev)
+{
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
 static const struct dev_pm_ops omap_i2c_pm_ops = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
 				      pm_runtime_force_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, omap_i2c_resume)
 	SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
 			   omap_i2c_runtime_resume, NULL)
 };

-- 
2.39.2

Re: [PATCH 03/14] i2c: omap: wakeup the controller during suspend callback

[Andy Shevchenko]

On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> A device may need the controller up during suspend_noirq or
> resume_noirq.
> But if the controller is autosuspended, there is no way to wakeup it
> during suspend_noirq or resume_noirq because runtime pm is disabled
> at this time.
>
> The suspend callback wakes up the controller, so it is available until
> its suspend_noirq callback (pm_runtime_force_suspend).
> During the resume, it's restored by resume_noirq callback
> (pm_runtime_force_resume). Then resume callback enables autosuspend.
>
> So the controller is up during a little time slot in suspend and resume
> sequences even if it's not used.

Same comment

->...callback...()

...

>  }
>
> +

Stray blank line is added.

> +static int omap_i2c_suspend(struct device *dev)

-- 
With Best Regards,
Andy Shevchenko

[PATCH 04/14] mux: mmio: Add resume support

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

Implement resume support

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;
+		int ret;
+
+		if (val == MUX_IDLE_AS_IS)
+			continue;
+
+		ret = mux_mmio_set(mux, val);
+		if (ret) {
+			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,
 };

-- 
2.39.2

Re: [PATCH 04/14] mux: mmio: Add resume support

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 04/14] mux: mmio: Add resume support

[Peter Rosin]

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,
>  };
> 

Re: [PATCH 04/14] mux: mmio: Add resume support

[Thomas Richard]

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, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 05/14] phy: ti: phy-j721e-wiz: make wiz_clock_init callable multiple times

[Thomas Richard]

For suspend and resume support, wiz_clock_init needs to be called
multiple times.
Add a parameter to wiz_clock_init to be able to skip clocks
registration.

Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 60 +++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index fc3cd98c60ff..09f7edf16562 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1076,7 +1076,7 @@ static int wiz_clock_register(struct wiz *wiz)
 	return ret;
 }
 
-static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
+static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe)
 {
 	const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
 	struct device *dev = wiz->dev;
@@ -1087,14 +1087,36 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 	int ret;
 	int i;
 
-	clk = devm_clk_get(dev, "core_ref_clk");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "core_ref_clk clock not found\n");
-		ret = PTR_ERR(clk);
-		return ret;
+	if (probe) {
+		clk = devm_clk_get(dev, "core_ref_clk");
+		if (IS_ERR(clk)) {
+			dev_err(dev, "core_ref_clk clock not found\n");
+			ret = PTR_ERR(clk);
+			return ret;
+		}
+		wiz->input_clks[WIZ_CORE_REFCLK] = clk;
+
+		if (wiz->data->pma_cmn_refclk1_int_mode) {
+			clk = devm_clk_get(dev, "core_ref1_clk");
+			if (IS_ERR(clk)) {
+				dev_err(dev, "core_ref1_clk clock not found\n");
+				ret = PTR_ERR(clk);
+				return ret;
+			}
+			wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
+		}
+
+		clk = devm_clk_get(dev, "ext_ref_clk");
+		if (IS_ERR(clk)) {
+			dev_err(dev, "ext_ref_clk clock not found\n");
+			ret = PTR_ERR(clk);
+			return ret;
+		}
+		wiz->input_clks[WIZ_EXT_REFCLK] = clk;
 	}
-	wiz->input_clks[WIZ_CORE_REFCLK] = clk;
 
+
+	clk = wiz->input_clks[WIZ_CORE_REFCLK];
 	rate = clk_get_rate(clk);
 	if (rate >= 100000000)
 		regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1);
@@ -1121,14 +1143,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 	}
 
 	if (wiz->data->pma_cmn_refclk1_int_mode) {
-		clk = devm_clk_get(dev, "core_ref1_clk");
-		if (IS_ERR(clk)) {
-			dev_err(dev, "core_ref1_clk clock not found\n");
-			ret = PTR_ERR(clk);
-			return ret;
-		}
-		wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
-
+		clk = wiz->input_clks[WIZ_CORE_REFCLK1];
 		rate = clk_get_rate(clk);
 		if (rate >= 100000000)
 			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
@@ -1136,20 +1151,17 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
 			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
 	}
 
-	clk = devm_clk_get(dev, "ext_ref_clk");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "ext_ref_clk clock not found\n");
-		ret = PTR_ERR(clk);
-		return ret;
-	}
-	wiz->input_clks[WIZ_EXT_REFCLK] = clk;
-
+	clk = wiz->input_clks[WIZ_EXT_REFCLK];
 	rate = clk_get_rate(clk);
 	if (rate >= 100000000)
 		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0);
 	else
 		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2);
 
+	/* What follows is about registering clocks. */
+	if (!probe)
+		return 0;
+
 	switch (wiz->type) {
 	case AM64_WIZ_10G:
 	case J7200_WIZ_10G:
@@ -1592,7 +1604,7 @@ static int wiz_probe(struct platform_device *pdev)
 		goto err_get_sync;
 	}
 
-	ret = wiz_clock_init(wiz, node);
+	ret = wiz_clock_init(wiz, node, true);
 	if (ret < 0) {
 		dev_warn(dev, "Failed to initialize clocks\n");
 		goto err_get_sync;

-- 
2.39.2

Re: [PATCH 05/14] phy: ti: phy-j721e-wiz: make wiz_clock_init callable multiple times

[Vinod Koul]

On 15-01-24, 17:14, Thomas Richard wrote:
> For suspend and resume support, wiz_clock_init needs to be called
> multiple times.
> Add a parameter to wiz_clock_init to be able to skip clocks
> registration.
> 
> Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/phy/ti/phy-j721e-wiz.c | 60 +++++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
> index fc3cd98c60ff..09f7edf16562 100644
> --- a/drivers/phy/ti/phy-j721e-wiz.c
> +++ b/drivers/phy/ti/phy-j721e-wiz.c
> @@ -1076,7 +1076,7 @@ static int wiz_clock_register(struct wiz *wiz)
>  	return ret;
>  }
>  
> -static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
> +static int wiz_clock_init(struct wiz *wiz, struct device_node *node, bool probe)
>  {
>  	const struct wiz_clk_mux_sel *clk_mux_sel = wiz->clk_mux_sel;
>  	struct device *dev = wiz->dev;
> @@ -1087,14 +1087,36 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
>  	int ret;
>  	int i;
>  
> -	clk = devm_clk_get(dev, "core_ref_clk");
> -	if (IS_ERR(clk)) {
> -		dev_err(dev, "core_ref_clk clock not found\n");
> -		ret = PTR_ERR(clk);
> -		return ret;
> +	if (probe) {
> +		clk = devm_clk_get(dev, "core_ref_clk");
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "core_ref_clk clock not found\n");
> +			ret = PTR_ERR(clk);
> +			return ret;
> +		}
> +		wiz->input_clks[WIZ_CORE_REFCLK] = clk;
> +
> +		if (wiz->data->pma_cmn_refclk1_int_mode) {
> +			clk = devm_clk_get(dev, "core_ref1_clk");
> +			if (IS_ERR(clk)) {
> +				dev_err(dev, "core_ref1_clk clock not found\n");
> +				ret = PTR_ERR(clk);
> +				return ret;
> +			}
> +			wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
> +		}
> +
> +		clk = devm_clk_get(dev, "ext_ref_clk");
> +		if (IS_ERR(clk)) {
> +			dev_err(dev, "ext_ref_clk clock not found\n");
> +			ret = PTR_ERR(clk);
> +			return ret;
> +		}
> +		wiz->input_clks[WIZ_EXT_REFCLK] = clk;
>  	}
> -	wiz->input_clks[WIZ_CORE_REFCLK] = clk;
>  
> +
> +	clk = wiz->input_clks[WIZ_CORE_REFCLK];
>  	rate = clk_get_rate(clk);
>  	if (rate >= 100000000)
>  		regmap_field_write(wiz->pma_cmn_refclk_int_mode, 0x1);
> @@ -1121,14 +1143,7 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
>  	}
>  
>  	if (wiz->data->pma_cmn_refclk1_int_mode) {
> -		clk = devm_clk_get(dev, "core_ref1_clk");
> -		if (IS_ERR(clk)) {
> -			dev_err(dev, "core_ref1_clk clock not found\n");
> -			ret = PTR_ERR(clk);
> -			return ret;
> -		}
> -		wiz->input_clks[WIZ_CORE_REFCLK1] = clk;
> -
> +		clk = wiz->input_clks[WIZ_CORE_REFCLK1];
>  		rate = clk_get_rate(clk);
>  		if (rate >= 100000000)
>  			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x1);
> @@ -1136,20 +1151,17 @@ static int wiz_clock_init(struct wiz *wiz, struct device_node *node)
>  			regmap_field_write(wiz->pma_cmn_refclk1_int_mode, 0x3);
>  	}
>  
> -	clk = devm_clk_get(dev, "ext_ref_clk");
> -	if (IS_ERR(clk)) {
> -		dev_err(dev, "ext_ref_clk clock not found\n");
> -		ret = PTR_ERR(clk);
> -		return ret;
> -	}
> -	wiz->input_clks[WIZ_EXT_REFCLK] = clk;
> -
> +	clk = wiz->input_clks[WIZ_EXT_REFCLK];
>  	rate = clk_get_rate(clk);
>  	if (rate >= 100000000)
>  		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x0);
>  	else
>  		regmap_field_write(wiz->pma_cmn_refclk_mode, 0x2);
>  
> +	/* What follows is about registering clocks. */
> +	if (!probe)
> +		return 0;
> +
>  	switch (wiz->type) {
>  	case AM64_WIZ_10G:
>  	case J7200_WIZ_10G:
> @@ -1592,7 +1604,7 @@ static int wiz_probe(struct platform_device *pdev)
>  		goto err_get_sync;
>  	}
>  
> -	ret = wiz_clock_init(wiz, node);
> +	ret = wiz_clock_init(wiz, node, true);

You are calling it one once? So what am I missing

-- 
~Vinod

Re: [PATCH 05/14] phy: ti: phy-j721e-wiz: make wiz_clock_init callable multiple times

[Thomas Richard]

>>  
>> -	ret = wiz_clock_init(wiz, node);
>> +	ret = wiz_clock_init(wiz, node, true);
> 
> You are calling it one once? So what am I missing

In patch 6/14, wiz_clock_init is called in resume_noirq callback:

ret = wiz_clock_init(wiz, node, false);

Regards

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 05/14] phy: ti: phy-j721e-wiz: make wiz_clock_init callable multiple times

[Vinod Koul]

On 23-01-24, 09:20, Thomas Richard wrote:
> >>  
> >> -	ret = wiz_clock_init(wiz, node);
> >> +	ret = wiz_clock_init(wiz, node, true);
> > 
> > You are calling it one once? So what am I missing
> 
> In patch 6/14, wiz_clock_init is called in resume_noirq callback:
> 
> ret = wiz_clock_init(wiz, node, false);

noticed later, I think this would be useful to mention in changelog

-- 
~Vinod

[PATCH 06/14] phy: ti: phy-j721e-wiz: add resume support

[Thomas Richard]

Add resume support.
It has been tested on J7200 SR1.0 and SR2.0.

Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index 09f7edf16562..fac7070a9d6b 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1666,12 +1666,51 @@ static void wiz_remove(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 }
 
+#ifdef CONFIG_PM
+static int wiz_resume_noirq(struct device *dev)
+{
+	struct device_node *node = dev->of_node;
+	struct wiz *wiz = dev_get_drvdata(dev);
+	int ret;
+
+	/* Enable supplemental Control override if available */
+	if (wiz->sup_legacy_clk_override)
+		regmap_field_write(wiz->sup_legacy_clk_override, 1);
+
+	ret = wiz_clock_init(wiz, node, false);
+	if (ret < 0) {
+		dev_warn(dev, "Failed to initialize clocks\n");
+		goto err_get_sync;
+	}
+
+	ret = wiz_init(wiz);
+	if (ret) {
+		dev_err(dev, "WIZ initialization failed\n");
+		goto err_wiz_init;
+	}
+
+	return 0;
+
+err_wiz_init:
+	wiz_clock_cleanup(wiz, node);
+
+err_get_sync:
+
+	return ret;
+}
+
+static const struct dev_pm_ops wiz_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, wiz_resume_noirq)
+};
+#endif
+
 static struct platform_driver wiz_driver = {
 	.probe		= wiz_probe,
 	.remove_new	= wiz_remove,
 	.driver		= {
 		.name	= "wiz",
 		.of_match_table = wiz_id_table,
+		.pm	= pm_ptr(&wiz_pm_ops),
 	},
 };
 module_platform_driver(wiz_driver);

-- 
2.39.2

[PATCH 07/14] phy: cadence-torrent: extract calls to clk_get from cdns_torrent_clk

[Thomas Richard]

Extract calls to clk_get from cdns_torrent_clk into a separate function.
It needs to call cdns_torrent_clk at resume without looking up the clock.

Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index a75c96385c57..94298ad9f875 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -2681,18 +2681,22 @@ static int cdns_torrent_reset(struct cdns_torrent_phy *cdns_phy)
 	return 0;
 }
 
-static int cdns_torrent_clk(struct cdns_torrent_phy *cdns_phy)
+static int cdns_torrent_of_get_clk(struct cdns_torrent_phy *cdns_phy)
 {
-	struct device *dev = cdns_phy->dev;
-	unsigned long ref_clk_rate;
-	int ret;
-
-	cdns_phy->clk = devm_clk_get(dev, "refclk");
+	cdns_phy->clk = devm_clk_get(cdns_phy->dev, "refclk");
 	if (IS_ERR(cdns_phy->clk)) {
-		dev_err(dev, "phy ref clock not found\n");
+		dev_err(cdns_phy->dev, "phy ref clock not found\n");
 		return PTR_ERR(cdns_phy->clk);
 	}
 
+	return 0;
+}
+
+static int cdns_torrent_clk(struct cdns_torrent_phy *cdns_phy)
+{
+	unsigned long ref_clk_rate;
+	int ret;
+
 	ret = clk_prepare_enable(cdns_phy->clk);
 	if (ret) {
 		dev_err(cdns_phy->dev, "Failed to prepare ref clock\n");
@@ -2776,6 +2780,10 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = cdns_torrent_of_get_clk(cdns_phy);
+	if (ret)
+		goto clk_cleanup;
+
 	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &already_configured);
 
 	if (!already_configured) {

-- 
2.39.2

[PATCH 08/14] phy: cadence-torrent: register resets even if the phy is already configured

[Thomas Richard]

Resets are needed during suspend and resume stages.
So they shall be registered during the probe even the phy is already
initialized.
The function cdns_torrent_reset is renamed cdns_torrent_of_get_reset to
make it clear.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 94298ad9f875..100b58965558 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -2660,7 +2660,7 @@ static int cdns_torrent_clk_register(struct cdns_torrent_phy *cdns_phy)
 	return 0;
 }
 
-static int cdns_torrent_reset(struct cdns_torrent_phy *cdns_phy)
+static int cdns_torrent_of_get_reset(struct cdns_torrent_phy *cdns_phy)
 {
 	struct device *dev = cdns_phy->dev;
 
@@ -2780,6 +2780,10 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = cdns_torrent_of_get_reset(cdns_phy);
+	if (ret)
+		goto clk_cleanup;
+
 	ret = cdns_torrent_of_get_clk(cdns_phy);
 	if (ret)
 		goto clk_cleanup;
@@ -2787,10 +2791,6 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &already_configured);
 
 	if (!already_configured) {
-		ret = cdns_torrent_reset(cdns_phy);
-		if (ret)
-			goto clk_cleanup;
-
 		ret = cdns_torrent_clk(cdns_phy);
 		if (ret)
 			goto clk_cleanup;

-- 
2.39.2

[PATCH 09/14] phy: cadence-torrent: move already_configured to struct cdns_torrent_phy

[Thomas Richard]

Move already_configured to struct cdns_torrent_phy, so it can be used at
differents stages.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 100b58965558..a1e9d06453e6 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -358,6 +358,7 @@ struct cdns_torrent_phy {
 	enum cdns_torrent_ref_clk ref_clk_rate;
 	struct cdns_torrent_inst phys[MAX_NUM_LANES];
 	int nsubnodes;
+	int already_configured;
 	const struct cdns_torrent_data *init_data;
 	struct regmap *regmap_common_cdb;
 	struct regmap *regmap_phy_pcs_common_cdb;
@@ -2741,7 +2742,6 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	struct device_node *child;
 	int ret, subnodes, node = 0, i;
 	u32 total_num_lanes = 0;
-	int already_configured;
 	u8 init_dp_regmap = 0;
 	u32 phy_type;
 
@@ -2788,9 +2788,9 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	if (ret)
 		goto clk_cleanup;
 
-	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &already_configured);
+	regmap_field_read(cdns_phy->phy_pma_cmn_ctrl_1, &cdns_phy->already_configured);
 
-	if (!already_configured) {
+	if (!cdns_phy->already_configured) {
 		ret = cdns_torrent_clk(cdns_phy);
 		if (ret)
 			goto clk_cleanup;
@@ -2870,7 +2870,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 		of_property_read_u32(child, "cdns,ssc-mode",
 				     &cdns_phy->phys[node].ssc_mode);
 
-		if (!already_configured)
+		if (!cdns_phy->already_configured)
 			gphy = devm_phy_create(dev, child, &cdns_torrent_phy_ops);
 		else
 			gphy = devm_phy_create(dev, child, &noop_ops);
@@ -2956,7 +2956,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 		goto put_lnk_rst;
 	}
 
-	if (cdns_phy->nsubnodes > 1 && !already_configured) {
+	if (cdns_phy->nsubnodes > 1 && !cdns_phy->already_configured) {
 		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
 		if (ret)
 			goto put_lnk_rst;

-- 
2.39.2

[PATCH 10/14] phy: cadence-torrent: remove noop_ops phy operations

[Thomas Richard]

Even if a phy is already configured, the phy operations are needed
during resume stage, as the phy is in reset state.
The noop_ops phy operations is removed to always have phy operations.
The already_configured flag is checked at the begening of init,
configure and poweron operations to keep the already_configured
behaviour.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index a1e9d06453e6..70413fca5776 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -1593,6 +1593,9 @@ static int cdns_torrent_dp_configure(struct phy *phy,
 	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(phy->dev.parent);
 	int ret;
 
+	if (cdns_phy->already_configured)
+		return 0;
+
 	ret = cdns_torrent_dp_verify_config(inst, &opts->dp);
 	if (ret) {
 		dev_err(&phy->dev, "invalid params for phy configure\n");
@@ -1628,6 +1631,12 @@ static int cdns_torrent_phy_on(struct phy *phy)
 	u32 read_val;
 	int ret;
 
+	if (cdns_phy->already_configured) {
+		/* Give 5ms to 10ms delay for the PIPE clock to be stable */
+		usleep_range(5000, 10000);
+		return 0;
+	}
+
 	if (cdns_phy->nsubnodes == 1) {
 		/* Take the PHY lane group out of reset */
 		reset_control_deassert(inst->lnk_rst);
@@ -2306,6 +2315,9 @@ static int cdns_torrent_phy_init(struct phy *phy)
 	u32 num_regs;
 	int i, j;
 
+	if (cdns_phy->already_configured)
+		return 0;
+
 	if (cdns_phy->nsubnodes > 1) {
 		if (phy_type == TYPE_DP)
 			return cdns_torrent_dp_multilink_init(cdns_phy, inst, phy);
@@ -2443,19 +2455,6 @@ static const struct phy_ops cdns_torrent_phy_ops = {
 	.owner		= THIS_MODULE,
 };
 
-static int cdns_torrent_noop_phy_on(struct phy *phy)
-{
-	/* Give 5ms to 10ms delay for the PIPE clock to be stable */
-	usleep_range(5000, 10000);
-
-	return 0;
-}
-
-static const struct phy_ops noop_ops = {
-	.power_on	= cdns_torrent_noop_phy_on,
-	.owner		= THIS_MODULE,
-};
-
 static
 int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy)
 {
@@ -2870,10 +2869,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 		of_property_read_u32(child, "cdns,ssc-mode",
 				     &cdns_phy->phys[node].ssc_mode);
 
-		if (!cdns_phy->already_configured)
-			gphy = devm_phy_create(dev, child, &cdns_torrent_phy_ops);
-		else
-			gphy = devm_phy_create(dev, child, &noop_ops);
+		gphy = devm_phy_create(dev, child, &cdns_torrent_phy_ops);
 		if (IS_ERR(gphy)) {
 			ret = PTR_ERR(gphy);
 			goto put_child;

-- 
2.39.2

[PATCH 11/14] phy: cadence-torrent: add suspend and resume support

[Thomas Richard]

Add suspend and resume support.
The alread_configured flag is cleared during suspend stage to force the
phy initialization during the resume stage.

Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 57 +++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 70413fca5776..31b2594e5942 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -3006,6 +3006,62 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
 	cdns_torrent_clk_cleanup(cdns_phy);
 }
 
+#ifdef CONFIG_PM
+static int cdns_torrent_phy_suspend_noirq(struct device *dev)
+{
+	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
+	int i;
+
+	reset_control_assert(cdns_phy->phy_rst);
+	reset_control_assert(cdns_phy->apb_rst);
+	for (i = 0; i < cdns_phy->nsubnodes; i++)
+		reset_control_assert(cdns_phy->phys[i].lnk_rst);
+
+	if (!cdns_phy->already_configured)
+		clk_disable_unprepare(cdns_phy->clk);
+	else
+		cdns_phy->already_configured = 0;
+
+	return 0;
+}
+
+static int cdns_torrent_phy_resume_noirq(struct device *dev)
+{
+	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
+	int node = cdns_phy->nsubnodes;
+	int ret, i;
+
+	ret = cdns_torrent_clk(cdns_phy);
+	if (ret)
+		goto clk_cleanup;
+
+	/* Enable APB */
+	reset_control_deassert(cdns_phy->apb_rst);
+
+	if (cdns_phy->nsubnodes > 1) {
+		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
+		if (ret)
+			goto put_lnk_rst;
+	}
+
+	return 0;
+
+put_lnk_rst:
+	for (i = 0; i < node; i++)
+		reset_control_put(cdns_phy->phys[i].lnk_rst);
+	reset_control_assert(cdns_phy->apb_rst);
+	clk_disable_unprepare(cdns_phy->clk);
+clk_cleanup:
+	cdns_torrent_clk_cleanup(cdns_phy);
+	return ret;
+}
+
+static const struct dev_pm_ops cdns_torrent_phy_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cdns_torrent_phy_suspend_noirq,
+				      cdns_torrent_phy_resume_noirq)
+};
+#endif
+
 /* USB and DP link configuration */
 static struct cdns_reg_pairs usb_dp_link_cmn_regs[] = {
 	{0x0002, PHY_PLL_CFG},
@@ -4577,6 +4633,7 @@ static struct platform_driver cdns_torrent_phy_driver = {
 	.driver = {
 		.name	= "cdns-torrent-phy",
 		.of_match_table	= cdns_torrent_phy_of_match,
+		.pm	= pm_ptr(&cdns_torrent_phy_pm_ops),
 	}
 };
 module_platform_driver(cdns_torrent_phy_driver);

-- 
2.39.2

Re: [PATCH 11/14] phy: cadence-torrent: add suspend and resume support

[Bjorn Helgaas]

On Mon, Jan 15, 2024 at 05:14:52PM +0100, Thomas Richard wrote:
> Add suspend and resume support.
> The alread_configured flag is cleared during suspend stage to force the
> phy initialization during the resume stage.

s/alread_configured/already_configured/

Wrap to fill 75 columns.  Add a blank line if you intend two
paragraphs.

I don't know whether there's a strong convention in drivers/phy, but I
see several commit logs that capitalize "PHY".  "Phy" is not a
standard English word, so I think the capitalization makes it easier
to read.

Bjorn

Re: [PATCH 11/14] phy: cadence-torrent: add suspend and resume support

[Thomas Richard]

Hello,

On 1/16/24 19:22, Bjorn Helgaas wrote:
> On Mon, Jan 15, 2024 at 05:14:52PM +0100, Thomas Richard wrote:
>> Add suspend and resume support.
>> The alread_configured flag is cleared during suspend stage to force the
>> phy initialization during the resume stage.
> 
> s/alread_configured/already_configured/
> 
> Wrap to fill 75 columns.  Add a blank line if you intend two
> paragraphs.
> 
> I don't know whether there's a strong convention in drivers/phy, but I
> see several commit logs that capitalize "PHY".  "Phy" is not a
> standard English word, so I think the capitalization makes it easier
> to read.
> 

Yes indeed, PHY is used in lots of commit logs.
So I guess I can use it.

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 11/14] phy: cadence-torrent: add suspend and resume support

[Philipp Zabel]

Hi Thomas,

On Mo, 2024-01-15 at 17:14 +0100, Thomas Richard wrote:
> Add suspend and resume support.
> The alread_configured flag is cleared during suspend stage to force the
> phy initialization during the resume stage.
> 
> Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/phy/cadence/phy-cadence-torrent.c | 57 +++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
> index 70413fca5776..31b2594e5942 100644
> --- a/drivers/phy/cadence/phy-cadence-torrent.c
> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> @@ -3006,6 +3006,62 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
[...]
> +static int cdns_torrent_phy_resume_noirq(struct device *dev)
> +{
> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
> +	int node = cdns_phy->nsubnodes;
> +	int ret, i;
> +
> +	ret = cdns_torrent_clk(cdns_phy);
> +	if (ret)
> +		goto clk_cleanup;
> +
> +	/* Enable APB */
> +	reset_control_deassert(cdns_phy->apb_rst);
> +
> +	if (cdns_phy->nsubnodes > 1) {
> +		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
> +		if (ret)
> +			goto put_lnk_rst;
> +	}
> +
> +	return 0;
> +
> +put_lnk_rst:
> +	for (i = 0; i < node; i++)
> +		reset_control_put(cdns_phy->phys[i].lnk_rst);

What is this intended to do? I expect this to explode in _remove, where
the lnk_rst are put again. Should this be:

		reset_control_assert(cdns_phy->phys[i].lnk_rst);

?


regards
Philipp

Re: [PATCH 11/14] phy: cadence-torrent: add suspend and resume support

[Thomas Richard]

Hello Philipp,

On 1/17/24 16:12, Philipp Zabel wrote:
> Hi Thomas,
> 
> On Mo, 2024-01-15 at 17:14 +0100, Thomas Richard wrote:
>> Add suspend and resume support.
>> The alread_configured flag is cleared during suspend stage to force the
>> phy initialization during the resume stage.
>>
>> Based on the work of Théo Lebrun <theo.lebrun@bootlin.com>
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> ---
>>  drivers/phy/cadence/phy-cadence-torrent.c | 57 +++++++++++++++++++++++++++++++
>>  1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
>> index 70413fca5776..31b2594e5942 100644
>> --- a/drivers/phy/cadence/phy-cadence-torrent.c
>> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
>> @@ -3006,6 +3006,62 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
> [...]
>> +static int cdns_torrent_phy_resume_noirq(struct device *dev)
>> +{
>> +	struct cdns_torrent_phy *cdns_phy = dev_get_drvdata(dev);
>> +	int node = cdns_phy->nsubnodes;
>> +	int ret, i;
>> +
>> +	ret = cdns_torrent_clk(cdns_phy);
>> +	if (ret)
>> +		goto clk_cleanup;
>> +
>> +	/* Enable APB */
>> +	reset_control_deassert(cdns_phy->apb_rst);
>> +
>> +	if (cdns_phy->nsubnodes > 1) {
>> +		ret = cdns_torrent_phy_configure_multilink(cdns_phy);
>> +		if (ret)
>> +			goto put_lnk_rst;
>> +	}
>> +
>> +	return 0;
>> +
>> +put_lnk_rst:
>> +	for (i = 0; i < node; i++)
>> +		reset_control_put(cdns_phy->phys[i].lnk_rst);
> 
> What is this intended to do? I expect this to explode in _remove, where
> the lnk_rst are put again. Should this be:
> 
> 		reset_control_assert(cdns_phy->phys[i].lnk_rst);
> 
> ?

Yes indeed, it's reset_control_assert instead of reset_control_put.

Thanks for the review.

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 12/14] PCI: cadence: add resume support to cdns_pcie_host_setup()

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

That function mixes probe structure init and hardware config.
The whole hardware config part must be done at resume after a suspend to
ram.
We therefore pass it a boolean flag determining if we are at probe or at
resume.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pci-j721e.c         |  2 +-
 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      |  4 +-
 4 files changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 2c87e7728a65..9b343a46da11 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -509,7 +509,7 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 			gpiod_set_value_cansleep(gpiod, 1);
 		}
 
-		ret = cdns_pcie_host_setup(rc);
+		ret = cdns_pcie_host_setup(rc, true);
 		if (ret < 0) {
 			clk_disable_unprepare(pcie->refclk);
 			goto err_pcie_setup;
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
index 5b14f7ee3c79..dd4d876a9138 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-host.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
@@ -497,14 +497,14 @@ static int cdns_pcie_host_init(struct device *dev,
 	return cdns_pcie_host_init_address_translation(rc);
 }
 
-int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
+int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe)
 {
 	struct device *dev = rc->pcie.dev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = dev->of_node;
 	struct pci_host_bridge *bridge;
 	enum cdns_pcie_rp_bar bar;
-	struct cdns_pcie *pcie;
+	struct cdns_pcie *pcie = &rc->pcie;
 	struct resource *res;
 	int ret;
 
@@ -512,26 +512,27 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	if (!bridge)
 		return -ENOMEM;
 
-	pcie = &rc->pcie;
-	pcie->is_rc = true;
+	if (probe) {
+		pcie->is_rc = true;
 
-	rc->vendor_id = 0xffff;
-	of_property_read_u32(np, "vendor-id", &rc->vendor_id);
+		rc->vendor_id = 0xffff;
+		of_property_read_u32(np, "vendor-id", &rc->vendor_id);
 
-	rc->device_id = 0xffff;
-	of_property_read_u32(np, "device-id", &rc->device_id);
+		rc->device_id = 0xffff;
+		of_property_read_u32(np, "device-id", &rc->device_id);
 
-	pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
-	if (IS_ERR(pcie->reg_base)) {
-		dev_err(dev, "missing \"reg\"\n");
-		return PTR_ERR(pcie->reg_base);
-	}
+		pcie->reg_base = devm_platform_ioremap_resource_byname(pdev, "reg");
+		if (IS_ERR(pcie->reg_base)) {
+			dev_err(dev, "missing \"reg\"\n");
+			return PTR_ERR(pcie->reg_base);
+		}
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
-	rc->cfg_base = devm_pci_remap_cfg_resource(dev, res);
-	if (IS_ERR(rc->cfg_base))
-		return PTR_ERR(rc->cfg_base);
-	rc->cfg_res = res;
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
+		rc->cfg_base = devm_pci_remap_cfg_resource(dev, res);
+		if (IS_ERR(rc->cfg_base))
+			return PTR_ERR(rc->cfg_base);
+		rc->cfg_res = res;
+	}
 
 	if (rc->quirk_detect_quiet_flag)
 		cdns_pcie_detect_quiet_min_delay_set(&rc->pcie);
@@ -555,12 +556,14 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
 	if (ret)
 		return ret;
 
-	if (!bridge->ops)
-		bridge->ops = &cdns_pcie_host_ops;
+	if (probe) {
+		if (!bridge->ops)
+			bridge->ops = &cdns_pcie_host_ops;
 
-	ret = pci_host_probe(bridge);
-	if (ret < 0)
-		goto err_init;
+		ret = pci_host_probe(bridge);
+		if (ret < 0)
+			goto err_init;
+	}
 
 	return 0;
 
diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
index 0456845dabb9..071423091668 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
@@ -86,7 +86,7 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
 			goto err_get_sync;
 		}
 
-		ret = cdns_pcie_host_setup(rc);
+		ret = cdns_pcie_host_setup(rc, true);
 		if (ret)
 			goto err_init;
 	} else {
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 373cb50fcd15..3b0da889ed64 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -515,11 +515,11 @@ static inline bool cdns_pcie_link_up(struct cdns_pcie *pcie)
 }
 
 #ifdef CONFIG_PCIE_CADENCE_HOST
-int cdns_pcie_host_setup(struct cdns_pcie_rc *rc);
+int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe);
 void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where);
 #else
-static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
+static inline int cdns_pcie_host_setup(struct cdns_pcie_rc *rc, bool probe)
 {
 	return 0;
 }

-- 
2.39.2

[PATCH 13/14] PCI: j721e: move reset GPIO to device struct

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

Move reset GPIO to device struct, so it can be used at suspend and
resume stages.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pci-j721e.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 9b343a46da11..477275d72257 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -54,6 +54,7 @@ struct j721e_pcie {
 	struct clk		*refclk;
 	u32			mode;
 	u32			num_lanes;
+	struct gpio_desc	*reset_gpio;
 	void __iomem		*user_cfg_base;
 	void __iomem		*intd_cfg_base;
 	u32			linkdown_irq_regfield;
@@ -359,7 +360,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 	struct j721e_pcie *pcie;
 	struct cdns_pcie_rc *rc = NULL;
 	struct cdns_pcie_ep *ep = NULL;
-	struct gpio_desc *gpiod;
 	void __iomem *base;
 	struct clk *clk;
 	u32 num_lanes;
@@ -468,9 +468,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 
 	switch (mode) {
 	case PCI_MODE_RC:
-		gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-		if (IS_ERR(gpiod)) {
-			ret = PTR_ERR(gpiod);
+		pcie->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+		if (IS_ERR(pcie->reset_gpio)) {
+			ret = PTR_ERR(pcie->reset_gpio);
 			if (ret != -EPROBE_DEFER)
 				dev_err(dev, "Failed to get reset GPIO\n");
 			goto err_get_sync;
@@ -504,9 +504,9 @@ static int j721e_pcie_probe(struct platform_device *pdev)
 		 * mode is selected while enabling the PHY. So deassert PERST#
 		 * after 100 us.
 		 */
-		if (gpiod) {
+		if (pcie->reset_gpio) {
 			usleep_range(100, 200);
-			gpiod_set_value_cansleep(gpiod, 1);
+			gpiod_set_value_cansleep(pcie->reset_gpio, 1);
 		}
 
 		ret = cdns_pcie_host_setup(rc, true);

-- 
2.39.2

Re: [PATCH 13/14] PCI: j721e: move reset GPIO to device struct

[Andy Shevchenko]

On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard
<thomas.richard@bootlin.com> wrote:
>
> From: Théo Lebrun <theo.lebrun@bootlin.com>
>
> Move reset GPIO to device struct, so it can be used at suspend and
> resume stages.

...

> -       struct gpio_desc *gpiod;

You can leave this and make the patch much lighter.

...

>                         if (ret != -EPROBE_DEFER)
>                                 dev_err(dev, "Failed to get reset GPIO\n");

Side note (not related to this change): perhaps dev_err_probe()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/14] PCI: j721e: move reset GPIO to device struct

[Bjorn Helgaas]

On Mon, Jan 15, 2024 at 05:14:54PM +0100, Thomas Richard wrote:
> From: Théo Lebrun <theo.lebrun@bootlin.com>
> 
> Move reset GPIO to device struct, so it can be used at suspend and
> resume stages.

s/Move/Add/ since we're not moving it from one struct to another.  (In
subject also.)

s/device struct/struct j721e_pcie/ since "device struct" could also
refer to the "struct device", which is obviously not relevant here.

BTW, if you capitalize the PCI subject lines to match previous
history, it will save some work when applying this series.

Also rewrap commit logs to fill 75 columns and add blank lines between
paragraphs (noticed in patch 12/14).

> @@ -54,6 +54,7 @@ struct j721e_pcie {
>  	struct clk		*refclk;
>  	u32			mode;
>  	u32			num_lanes;
> +	struct gpio_desc	*reset_gpio;
>  	void __iomem		*user_cfg_base;
>  	void __iomem		*intd_cfg_base;
>  	u32			linkdown_irq_regfield;
> @@ -359,7 +360,6 @@ static int j721e_pcie_probe(struct platform_device *pdev)
>  	struct j721e_pcie *pcie;
>  	struct cdns_pcie_rc *rc = NULL;
>  	struct cdns_pcie_ep *ep = NULL;
> -	struct gpio_desc *gpiod;
>  	void __iomem *base;
>  	struct clk *clk;
>  	u32 num_lanes;

[PATCH 14/14] PCI: j721e: add suspend and resume support

[Thomas Richard]

From: Théo Lebrun <theo.lebrun@bootlin.com>

Add suspend and resume support for rc mode.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/pci/controller/cadence/pci-j721e.c    | 72 +++++++++++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h |  3 +-
 2 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c
index 477275d72257..51867a3f2499 100644
--- a/drivers/pci/controller/cadence/pci-j721e.c
+++ b/drivers/pci/controller/cadence/pci-j721e.c
@@ -6,6 +6,7 @@
  * Author: Kishon Vijay Abraham I <kishon@ti.com>
  */
 
+#include <linux/clk-provider.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
@@ -554,6 +555,76 @@ static void j721e_pcie_remove(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 }
 
+#ifdef CONFIG_PM
+static int j721e_pcie_suspend_noirq(struct device *dev)
+{
+	struct j721e_pcie *pcie = dev_get_drvdata(dev);
+
+	if (pcie->mode == PCI_MODE_RC) {
+		if (pcie->reset_gpio)
+			gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+
+		clk_disable_unprepare(pcie->refclk);
+	}
+
+	cdns_pcie_disable_phy(pcie->cdns_pcie);
+
+	return 0;
+}
+
+static int j721e_pcie_resume_noirq(struct device *dev)
+{
+	struct j721e_pcie *pcie = dev_get_drvdata(dev);
+	struct cdns_pcie *cdns_pcie = pcie->cdns_pcie;
+	int ret;
+
+	ret = j721e_pcie_ctrl_init(pcie);
+	if (ret < 0) {
+		dev_err(dev, "j721e_pcie_ctrl_init failed\n");
+		return ret;
+	}
+
+	j721e_pcie_config_link_irq(pcie);
+
+	/*
+	 * This is not called explicitly in the probe, it is called by
+	 * cdns_pcie_init_phy.
+	 */
+	ret = cdns_pcie_enable_phy(pcie->cdns_pcie);
+	if (ret < 0) {
+		dev_err(dev, "cdns_pcie_enable_phy failed\n");
+		return -ENODEV;
+	}
+
+	if (pcie->mode == PCI_MODE_RC) {
+		struct cdns_pcie_rc *rc = cdns_pcie_to_rc(cdns_pcie);
+
+		ret = clk_prepare_enable(pcie->refclk);
+		if (ret < 0) {
+			dev_err(dev, "clk_prepare_enable failed\n");
+			return -ENODEV;
+		}
+
+		if (pcie->reset_gpio) {
+			usleep_range(100, 200);
+			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;
+		}
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops j721e_pcie_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(j721e_pcie_suspend_noirq, j721e_pcie_resume_noirq)
+};
+#endif
+
 static struct platform_driver j721e_pcie_driver = {
 	.probe  = j721e_pcie_probe,
 	.remove_new = j721e_pcie_remove,
@@ -561,6 +632,7 @@ static struct platform_driver j721e_pcie_driver = {
 		.name	= "j721e-pcie",
 		.of_match_table = of_j721e_pcie_match,
 		.suppress_bind_attrs = true,
+		.pm	= pm_ptr(&j721e_pcie_pm_ops),
 	},
 };
 builtin_platform_driver(j721e_pcie_driver);
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index 3b0da889ed64..05d4b96fc71d 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -331,6 +331,8 @@ struct cdns_pcie_rc {
 	unsigned int		quirk_detect_quiet_flag:1;
 };
 
+#define cdns_pcie_to_rc(p) container_of(p, struct cdns_pcie_rc, pcie)
+
 /**
  * struct cdns_pcie_epf - Structure to hold info about endpoint function
  * @epf: Info about virtual functions attached to the physical function
@@ -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)
 {

-- 
2.39.2

Re: [PATCH 14/14] PCI: j721e: add suspend and resume support

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 14/14] PCI: j721e: add suspend and resume support

[Thomas Richard]

Hello Andy,

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:
>>
>> 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?

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].

> 
>> +                       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

> 
>> +                       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?

Yes I should return ret instead.

> 
>> +               }
> 
> ...
> 
>> +#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).

Regards,

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 14/14] PCI: j721e: add suspend and resume support

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 14/14] PCI: j721e: add suspend and resume support

[Thomas Richard]

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, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com