[v1] Add suspend to ram support for PCIe on J7200

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

Posted by Thomas Richard 1 year, 11 months ago

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

Posted by Linus Walleij 1 year, 10 months ago

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

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

Posted by Andy Shevchenko 1 year, 11 months ago

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

Posted by Thomas Richard 1 year, 11 months ago

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

Posted by Andy Shevchenko 1 year, 11 months ago

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

Posted by Thomas Richard 1 year, 10 months ago

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

Posted by Andy Shevchenko 1 year, 10 months ago

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