Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
13.07.2019 8:31, Sowjanya Komatineni пишет: > > On 7/4/19 3:40 AM, Dmitry Osipenko wrote: >> 04.07.2019 10:31, Linus Walleij пишет: >>> On Sat, Jun 29, 2019 at 5:58 PM Dmitry Osipenko >>> wrote: >>> Oh, also what about GPIO-pinctrl suspend resume ordering .. is it okay that pinctrl will be resumed after GPIO? Shouldn't a proper pin-muxing be selected at first? >>> Thierry sent some initial patches about this I think. We need to use >>> device links for this to work properly so he adds support for >>> linking the pinctrl and GPIO devices through the ranges. >>> >>> For links between pin control handles and their consumers, see also: >>> 036f394dd77f pinctrl: Enable device link creation for pin control >>> c6045b4e3cad pinctrl: stmfx: enable links creations >>> 489b64d66325 pinctrl: stm32: Add links to consumers >>> >>> I am using STM32 as guinea pig for this, consider adding links also >>> from the Tegra pinctrl. I might simply make these pinctrl consumer >>> to producer links default because I think it makes a lot sense. >> IIUC, currently the plan is to resume pinctrl *after* GPIO for >> Tegra210 [1]. But this >> contradicts to what was traditionally done for older Tegras where >> pinctrl was always >> resumed first and apparently it won't work well for the GPIO ranges as >> well. I think this >> and the other patchsets related to suspend-resume still need some more >> thought. >> >> [1] https://patchwork.kernel.org/patch/11012077/ > > Park bit was introduced from Tegra210 onwards and during suspend/resume, > requirement of gpio restore prior to pinctrl restore is not required for > prior Tegra210. > > Also currently pinctrl suspend/resume implementation for prior Tegra210 > is not yet upstreamed but having gpio restore prior to pinmux during > suspend/resume should not cause any issue for prior tegra's as well as > gpio resume restores pins back to same gpio config as they were during > suspend entry. > Okay!
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
On 6/29/19 8:46 AM, Dmitry Osipenko wrote:
28.06.2019 5:12, Sowjanya Komatineni пишет:
This patch adds support for Tegra pinctrl driver suspend and resume.
During suspend, context of all pinctrl registers are stored and
on resume they are all restored to have all the pinmux and pad
configuration for normal operation.
Acked-by: Thierry Reding
Signed-off-by: Sowjanya Komatineni
---
drivers/pinctrl/tegra/pinctrl-tegra.c| 52
drivers/pinctrl/tegra/pinctrl-tegra.h| 3 ++
drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
3 files changed, 56 insertions(+)
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 34596b246578..e7c0a1011cba 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -621,6 +621,43 @@ static void tegra_pinctrl_clear_parked_bits(struct
tegra_pmx *pmx)
}
}
+static int tegra_pinctrl_suspend(struct device *dev)
+{
+ struct tegra_pmx *pmx = dev_get_drvdata(dev);
+ u32 *backup_regs = pmx->backup_regs;
+ u32 *regs;
+ unsigned int i, j;
+
+ for (i = 0; i < pmx->nbanks; i++) {
+ regs = pmx->regs[i];
+ for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
+ *backup_regs++ = readl(regs++);
+ }
+
+ return pinctrl_force_sleep(pmx->pctl);
+}
+
+static int tegra_pinctrl_resume(struct device *dev)
+{
+ struct tegra_pmx *pmx = dev_get_drvdata(dev);
+ u32 *backup_regs = pmx->backup_regs;
+ u32 *regs;
+ unsigned int i, j;
+
+ for (i = 0; i < pmx->nbanks; i++) {
+ regs = pmx->regs[i];
+ for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
+ writel(*backup_regs++, regs++);
+ }
+
+ return 0;
+}
+
+const struct dev_pm_ops tegra_pinctrl_pm = {
+ .suspend = _pinctrl_suspend,
+ .resume = _pinctrl_resume
+};
Hm, so this are the generic platform-driver suspend-resume OPS here, which is
very
nice! But.. shouldn't pinctrl be resumed before the CLK driver (which is
syscore_ops
in this version of the series)? .. Given that "clock" function may need to be
selected for some of the pins.
selection of clock functions on some Tegra pins through corresponding
pinmux (like extperiph clks) can happen after clock driver resume as
well where clock source is restored to state during suspend before
selecting clock function on that pin.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
On 7/4/19 3:40 AM, Dmitry Osipenko wrote: 04.07.2019 10:31, Linus Walleij пишет: On Sat, Jun 29, 2019 at 5:58 PM Dmitry Osipenko wrote: Oh, also what about GPIO-pinctrl suspend resume ordering .. is it okay that pinctrl will be resumed after GPIO? Shouldn't a proper pin-muxing be selected at first? Thierry sent some initial patches about this I think. We need to use device links for this to work properly so he adds support for linking the pinctrl and GPIO devices through the ranges. For links between pin control handles and their consumers, see also: 036f394dd77f pinctrl: Enable device link creation for pin control c6045b4e3cad pinctrl: stmfx: enable links creations 489b64d66325 pinctrl: stm32: Add links to consumers I am using STM32 as guinea pig for this, consider adding links also from the Tegra pinctrl. I might simply make these pinctrl consumer to producer links default because I think it makes a lot sense. IIUC, currently the plan is to resume pinctrl *after* GPIO for Tegra210 [1]. But this contradicts to what was traditionally done for older Tegras where pinctrl was always resumed first and apparently it won't work well for the GPIO ranges as well. I think this and the other patchsets related to suspend-resume still need some more thought. [1] https://patchwork.kernel.org/patch/11012077/ Park bit was introduced from Tegra210 onwards and during suspend/resume, requirement of gpio restore prior to pinctrl restore is not required for prior Tegra210. Also currently pinctrl suspend/resume implementation for prior Tegra210 is not yet upstreamed but having gpio restore prior to pinmux during suspend/resume should not cause any issue for prior tegra's as well as gpio resume restores pins back to same gpio config as they were during suspend entry.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
04.07.2019 10:31, Linus Walleij пишет: > On Sat, Jun 29, 2019 at 5:58 PM Dmitry Osipenko wrote: > >> Oh, also what about GPIO-pinctrl suspend resume ordering .. is it okay that >> pinctrl >> will be resumed after GPIO? Shouldn't a proper pin-muxing be selected at >> first? > > Thierry sent some initial patches about this I think. We need to use > device links for this to work properly so he adds support for > linking the pinctrl and GPIO devices through the ranges. > > For links between pin control handles and their consumers, see also: > 036f394dd77f pinctrl: Enable device link creation for pin control > c6045b4e3cad pinctrl: stmfx: enable links creations > 489b64d66325 pinctrl: stm32: Add links to consumers > > I am using STM32 as guinea pig for this, consider adding links also > from the Tegra pinctrl. I might simply make these pinctrl consumer > to producer links default because I think it makes a lot sense. IIUC, currently the plan is to resume pinctrl *after* GPIO for Tegra210 [1]. But this contradicts to what was traditionally done for older Tegras where pinctrl was always resumed first and apparently it won't work well for the GPIO ranges as well. I think this and the other patchsets related to suspend-resume still need some more thought. [1] https://patchwork.kernel.org/patch/11012077/
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
On Sat, Jun 29, 2019 at 5:58 PM Dmitry Osipenko wrote: > Oh, also what about GPIO-pinctrl suspend resume ordering .. is it okay that > pinctrl > will be resumed after GPIO? Shouldn't a proper pin-muxing be selected at > first? Thierry sent some initial patches about this I think. We need to use device links for this to work properly so he adds support for linking the pinctrl and GPIO devices through the ranges. For links between pin control handles and their consumers, see also: 036f394dd77f pinctrl: Enable device link creation for pin control c6045b4e3cad pinctrl: stmfx: enable links creations 489b64d66325 pinctrl: stm32: Add links to consumers I am using STM32 as guinea pig for this, consider adding links also from the Tegra pinctrl. I might simply make these pinctrl consumer to producer links default because I think it makes a lot sense. Yours, Linus Walleij
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
On Fri, Jun 28, 2019 at 4:13 AM Sowjanya Komatineni wrote: > This patch adds support for Tegra pinctrl driver suspend and resume. > > During suspend, context of all pinctrl registers are stored and > on resume they are all restored to have all the pinmux and pad > configuration for normal operation. > > Acked-by: Thierry Reding > Signed-off-by: Sowjanya Komatineni Looks good. Can I just apply this patch or does it need to go in with the other (clk) changes? Yours, Linus Walleij
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
29.06.2019 18:58, Dmitry Osipenko пишет:
> 29.06.2019 18:46, Dmitry Osipenko пишет:
>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>>> This patch adds support for Tegra pinctrl driver suspend and resume.
>>>
>>> During suspend, context of all pinctrl registers are stored and
>>> on resume they are all restored to have all the pinmux and pad
>>> configuration for normal operation.
>>>
>>> Acked-by: Thierry Reding
>>> Signed-off-by: Sowjanya Komatineni
>>> ---
>>> drivers/pinctrl/tegra/pinctrl-tegra.c| 52
>>>
>>> drivers/pinctrl/tegra/pinctrl-tegra.h| 3 ++
>>> drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
>>> 3 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>> index 34596b246578..e7c0a1011cba 100644
>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>> @@ -621,6 +621,43 @@ static void tegra_pinctrl_clear_parked_bits(struct
>>> tegra_pmx *pmx)
>>> }
>>> }
>>>
>>> +static int tegra_pinctrl_suspend(struct device *dev)
>>> +{
>>> + struct tegra_pmx *pmx = dev_get_drvdata(dev);
>>> + u32 *backup_regs = pmx->backup_regs;
>>> + u32 *regs;
>>> + unsigned int i, j;
>>> +
>>> + for (i = 0; i < pmx->nbanks; i++) {
>>> + regs = pmx->regs[i];
>>> + for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
>>> + *backup_regs++ = readl(regs++);
>>> + }
>>> +
>>> + return pinctrl_force_sleep(pmx->pctl);
>>> +}
>>> +
>>> +static int tegra_pinctrl_resume(struct device *dev)
>>> +{
>>> + struct tegra_pmx *pmx = dev_get_drvdata(dev);
>>> + u32 *backup_regs = pmx->backup_regs;
>>> + u32 *regs;
>>> + unsigned int i, j;
>>> +
>>> + for (i = 0; i < pmx->nbanks; i++) {
>>> + regs = pmx->regs[i];
>>> + for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
>>> + writel(*backup_regs++, regs++);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +const struct dev_pm_ops tegra_pinctrl_pm = {
>>> + .suspend = _pinctrl_suspend,
>>> + .resume = _pinctrl_resume
>>> +};
>>
>> Hm, so this are the generic platform-driver suspend-resume OPS here, which
>> is very
>> nice! But.. shouldn't pinctrl be resumed before the CLK driver (which is
>> syscore_ops
>> in this version of the series)? .. Given that "clock" function may need to be
>> selected for some of the pins.
>>
>
> Oh, also what about GPIO-pinctrl suspend resume ordering .. is it okay that
> pinctrl
> will be resumed after GPIO? Shouldn't a proper pin-muxing be selected at
> first?
>
> This also looks to me very unsafe in a context of older Tegras which are
> initializing
> the static muxing very early during of the boot, otherwise things won't work
> well for
> the drivers.
>
Although, scratch what I wrote about older Tegras. We are probing pinctl driver
very
early, hence it should suspend last and resume first. Should be okay.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
29.06.2019 18:46, Dmitry Osipenko пишет:
> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>> This patch adds support for Tegra pinctrl driver suspend and resume.
>>
>> During suspend, context of all pinctrl registers are stored and
>> on resume they are all restored to have all the pinmux and pad
>> configuration for normal operation.
>>
>> Acked-by: Thierry Reding
>> Signed-off-by: Sowjanya Komatineni
>> ---
>> drivers/pinctrl/tegra/pinctrl-tegra.c| 52
>>
>> drivers/pinctrl/tegra/pinctrl-tegra.h| 3 ++
>> drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
>> 3 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
>> index 34596b246578..e7c0a1011cba 100644
>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>> @@ -621,6 +621,43 @@ static void tegra_pinctrl_clear_parked_bits(struct
>> tegra_pmx *pmx)
>> }
>> }
>>
>> +static int tegra_pinctrl_suspend(struct device *dev)
>> +{
>> +struct tegra_pmx *pmx = dev_get_drvdata(dev);
>> +u32 *backup_regs = pmx->backup_regs;
>> +u32 *regs;
>> +unsigned int i, j;
>> +
>> +for (i = 0; i < pmx->nbanks; i++) {
>> +regs = pmx->regs[i];
>> +for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
>> +*backup_regs++ = readl(regs++);
>> +}
>> +
>> +return pinctrl_force_sleep(pmx->pctl);
>> +}
>> +
>> +static int tegra_pinctrl_resume(struct device *dev)
>> +{
>> +struct tegra_pmx *pmx = dev_get_drvdata(dev);
>> +u32 *backup_regs = pmx->backup_regs;
>> +u32 *regs;
>> +unsigned int i, j;
>> +
>> +for (i = 0; i < pmx->nbanks; i++) {
>> +regs = pmx->regs[i];
>> +for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
>> +writel(*backup_regs++, regs++);
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +const struct dev_pm_ops tegra_pinctrl_pm = {
>> +.suspend = _pinctrl_suspend,
>> +.resume = _pinctrl_resume
>> +};
>
> Hm, so this are the generic platform-driver suspend-resume OPS here, which is
> very
> nice! But.. shouldn't pinctrl be resumed before the CLK driver (which is
> syscore_ops
> in this version of the series)? .. Given that "clock" function may need to be
> selected for some of the pins.
>
Oh, also what about GPIO-pinctrl suspend resume ordering .. is it okay that
pinctrl
will be resumed after GPIO? Shouldn't a proper pin-muxing be selected at first?
This also looks to me very unsafe in a context of older Tegras which are
initializing
the static muxing very early during of the boot, otherwise things won't work
well for
the drivers.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
28.06.2019 5:12, Sowjanya Komatineni пишет:
> This patch adds support for Tegra pinctrl driver suspend and resume.
>
> During suspend, context of all pinctrl registers are stored and
> on resume they are all restored to have all the pinmux and pad
> configuration for normal operation.
>
> Acked-by: Thierry Reding
> Signed-off-by: Sowjanya Komatineni
> ---
> drivers/pinctrl/tegra/pinctrl-tegra.c| 52
>
> drivers/pinctrl/tegra/pinctrl-tegra.h| 3 ++
> drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
> 3 files changed, 56 insertions(+)
>
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
> b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 34596b246578..e7c0a1011cba 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -621,6 +621,43 @@ static void tegra_pinctrl_clear_parked_bits(struct
> tegra_pmx *pmx)
> }
> }
>
> +static int tegra_pinctrl_suspend(struct device *dev)
> +{
> + struct tegra_pmx *pmx = dev_get_drvdata(dev);
> + u32 *backup_regs = pmx->backup_regs;
> + u32 *regs;
> + unsigned int i, j;
> +
> + for (i = 0; i < pmx->nbanks; i++) {
> + regs = pmx->regs[i];
> + for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
> + *backup_regs++ = readl(regs++);
> + }
> +
> + return pinctrl_force_sleep(pmx->pctl);
> +}
> +
> +static int tegra_pinctrl_resume(struct device *dev)
> +{
> + struct tegra_pmx *pmx = dev_get_drvdata(dev);
> + u32 *backup_regs = pmx->backup_regs;
> + u32 *regs;
> + unsigned int i, j;
> +
> + for (i = 0; i < pmx->nbanks; i++) {
> + regs = pmx->regs[i];
> + for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
> + writel(*backup_regs++, regs++);
> + }
> +
> + return 0;
> +}
> +
> +const struct dev_pm_ops tegra_pinctrl_pm = {
> + .suspend = _pinctrl_suspend,
> + .resume = _pinctrl_resume
> +};
Hm, so this are the generic platform-driver suspend-resume OPS here, which is
very
nice! But.. shouldn't pinctrl be resumed before the CLK driver (which is
syscore_ops
in this version of the series)? .. Given that "clock" function may need to be
selected for some of the pins.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
29.06.2019 15:38, Dmitry Osipenko пишет:
> 29.06.2019 2:00, Sowjanya Komatineni пишет:
>>
>> On 6/28/19 5:05 AM, Dmitry Osipenko wrote:
>>> 28.06.2019 14:56, Dmitry Osipenko пишет:
28.06.2019 5:12, Sowjanya Komatineni пишет:
> This patch adds support for Tegra pinctrl driver suspend and resume.
>
> During suspend, context of all pinctrl registers are stored and
> on resume they are all restored to have all the pinmux and pad
> configuration for normal operation.
>
> Acked-by: Thierry Reding
> Signed-off-by: Sowjanya Komatineni
> ---
> int tegra_pinctrl_probe(struct platform_device *pdev,
> const struct tegra_pinctrl_soc_data *soc_data);
> #endif
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c
> b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> index 0b56ad5c9c1c..edd3f4606cdb 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> @@ -1571,6 +1571,7 @@ static struct platform_driver
> tegra210_pinctrl_driver = {
> .driver = {
> .name = "tegra210-pinctrl",
> .of_match_table = tegra210_pinctrl_of_match,
> + .pm = _pinctrl_pm,
> },
> .probe = tegra210_pinctrl_probe,
> };
>
Could you please address my comments in the next revision if there will be
one?
>>> Also, what about adding ".pm' for other Tegras? I'm sure Jon could test
>>> them for you.
>>
>> This series is for Tegra210 SC7 entry/exit along with clocks and pinctrl
>> suspend
>> resume needed for Tegra210 basic sc7 entry and exit.
>>
>> This includes pinctrl, pmc changes, clock-tegra210 driver changes all w.r.t
>> Tegra210
>> platforms specific.
>>
>> Suspend/resume support for other Tegras will be in separate patch series.
>
> Okay, fair enough.
>
It may also make some sense to split this patch into two:
1) add generic tegra-pinctrl suspend-resume support
2) add suspend-resume OPS to the pinctrl-tegra210
For consistency.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
29.06.2019 2:00, Sowjanya Komatineni пишет:
>
> On 6/28/19 5:05 AM, Dmitry Osipenko wrote:
>> 28.06.2019 14:56, Dmitry Osipenko пишет:
>>> 28.06.2019 5:12, Sowjanya Komatineni пишет:
This patch adds support for Tegra pinctrl driver suspend and resume.
During suspend, context of all pinctrl registers are stored and
on resume they are all restored to have all the pinmux and pad
configuration for normal operation.
Acked-by: Thierry Reding
Signed-off-by: Sowjanya Komatineni
---
int tegra_pinctrl_probe(struct platform_device *pdev,
const struct tegra_pinctrl_soc_data *soc_data);
#endif
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c
b/drivers/pinctrl/tegra/pinctrl-tegra210.c
index 0b56ad5c9c1c..edd3f4606cdb 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
@@ -1571,6 +1571,7 @@ static struct platform_driver
tegra210_pinctrl_driver = {
.driver = {
.name = "tegra210-pinctrl",
.of_match_table = tegra210_pinctrl_of_match,
+ .pm = _pinctrl_pm,
},
.probe = tegra210_pinctrl_probe,
};
>>> Could you please address my comments in the next revision if there will be
>>> one?
>>>
>> Also, what about adding ".pm' for other Tegras? I'm sure Jon could test them
>> for you.
>
> This series is for Tegra210 SC7 entry/exit along with clocks and pinctrl
> suspend
> resume needed for Tegra210 basic sc7 entry and exit.
>
> This includes pinctrl, pmc changes, clock-tegra210 driver changes all w.r.t
> Tegra210
> platforms specific.
>
> Suspend/resume support for other Tegras will be in separate patch series.
Okay, fair enough.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
On 6/28/19 5:05 AM, Dmitry Osipenko wrote:
28.06.2019 14:56, Dmitry Osipenko пишет:
28.06.2019 5:12, Sowjanya Komatineni пишет:
This patch adds support for Tegra pinctrl driver suspend and resume.
During suspend, context of all pinctrl registers are stored and
on resume they are all restored to have all the pinmux and pad
configuration for normal operation.
Acked-by: Thierry Reding
Signed-off-by: Sowjanya Komatineni
---
int tegra_pinctrl_probe(struct platform_device *pdev,
const struct tegra_pinctrl_soc_data *soc_data);
#endif
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c
b/drivers/pinctrl/tegra/pinctrl-tegra210.c
index 0b56ad5c9c1c..edd3f4606cdb 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
@@ -1571,6 +1571,7 @@ static struct platform_driver tegra210_pinctrl_driver = {
.driver = {
.name = "tegra210-pinctrl",
.of_match_table = tegra210_pinctrl_of_match,
+ .pm = _pinctrl_pm,
},
.probe = tegra210_pinctrl_probe,
};
Could you please address my comments in the next revision if there will be one?
Also, what about adding ".pm' for other Tegras? I'm sure Jon could test them
for you.
This series is for Tegra210 SC7 entry/exit along with clocks and pinctrl
suspend resume needed for Tegra210 basic sc7 entry and exit.
This includes pinctrl, pmc changes, clock-tegra210 driver changes all
w.r.t Tegra210 platforms specific.
Suspend/resume support for other Tegras will be in separate patch series.
thanks
Sowjanya
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
28.06.2019 14:56, Dmitry Osipenko пишет:
> 28.06.2019 5:12, Sowjanya Komatineni пишет:
>> This patch adds support for Tegra pinctrl driver suspend and resume.
>>
>> During suspend, context of all pinctrl registers are stored and
>> on resume they are all restored to have all the pinmux and pad
>> configuration for normal operation.
>>
>> Acked-by: Thierry Reding
>> Signed-off-by: Sowjanya Komatineni
>> ---
>> int tegra_pinctrl_probe(struct platform_device *pdev,
>> const struct tegra_pinctrl_soc_data *soc_data);
>> #endif
>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c
>> b/drivers/pinctrl/tegra/pinctrl-tegra210.c
>> index 0b56ad5c9c1c..edd3f4606cdb 100644
>> --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
>> @@ -1571,6 +1571,7 @@ static struct platform_driver tegra210_pinctrl_driver
>> = {
>> .driver = {
>> .name = "tegra210-pinctrl",
>> .of_match_table = tegra210_pinctrl_of_match,
>> +.pm = _pinctrl_pm,
>> },
>> .probe = tegra210_pinctrl_probe,
>> };
>>
>
> Could you please address my comments in the next revision if there will be
> one?
>
Also, what about adding ".pm' for other Tegras? I'm sure Jon could test them
for you.
Re: [PATCH V5 02/18] pinctrl: tegra: Add suspend and resume support
28.06.2019 5:12, Sowjanya Komatineni пишет:
> This patch adds support for Tegra pinctrl driver suspend and resume.
>
> During suspend, context of all pinctrl registers are stored and
> on resume they are all restored to have all the pinmux and pad
> configuration for normal operation.
>
> Acked-by: Thierry Reding
> Signed-off-by: Sowjanya Komatineni
> ---
> drivers/pinctrl/tegra/pinctrl-tegra.c| 52
>
> drivers/pinctrl/tegra/pinctrl-tegra.h| 3 ++
> drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
> 3 files changed, 56 insertions(+)
>
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
> b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 34596b246578..e7c0a1011cba 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -621,6 +621,43 @@ static void tegra_pinctrl_clear_parked_bits(struct
> tegra_pmx *pmx)
> }
> }
>
> +static int tegra_pinctrl_suspend(struct device *dev)
> +{
> + struct tegra_pmx *pmx = dev_get_drvdata(dev);
> + u32 *backup_regs = pmx->backup_regs;
> + u32 *regs;
> + unsigned int i, j;
In general it's better not to use "j" in conjunction with "i" because they look
similar and I seen quite a lot of bugs caused by unnoticed typos like that. So
I'm
suggesting to use "i, k" for clarity.
> +
> + for (i = 0; i < pmx->nbanks; i++) {
> + regs = pmx->regs[i];
> + for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
> + *backup_regs++ = readl(regs++);
Please use readl_relaxed(), we don't need memory barriers here.
> + }
> +
> + return pinctrl_force_sleep(pmx->pctl);
> +}
> +
> +static int tegra_pinctrl_resume(struct device *dev)
> +{
> + struct tegra_pmx *pmx = dev_get_drvdata(dev);
> + u32 *backup_regs = pmx->backup_regs;
> + u32 *regs;
> + unsigned int i, j;
> +
> + for (i = 0; i < pmx->nbanks; i++) {
> + regs = pmx->regs[i];> + for (j = 0; j <
> pmx->reg_bank_size[i] / 4; j++)
> + writel(*backup_regs++, regs++);
Same for writel_relaxed(), memory barrier is inserted *before* the write to
ensure
that all previous memory stores are completed. IOREMAP'ed memory is
strongly-ordered,
memory barriers are not needed here.
> + }
> +
> + return 0;
> +}
> +
> +const struct dev_pm_ops tegra_pinctrl_pm = {
> + .suspend = _pinctrl_suspend,
> + .resume = _pinctrl_resume
> +};
> +
> static bool gpio_node_has_range(const char *compatible)
> {
> struct device_node *np;
> @@ -645,6 +682,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
> int i;
> const char **group_pins;
> int fn, gn, gfn;
> + unsigned long backup_regs_size = 0;
>
> pmx = devm_kzalloc(>dev, sizeof(*pmx), GFP_KERNEL);
> if (!pmx)
> @@ -697,6 +735,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
> res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> if (!res)
> break;
> + backup_regs_size += resource_size(res);
> }
> pmx->nbanks = i;
>
> @@ -705,11 +744,24 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
> if (!pmx->regs)
> return -ENOMEM;
>
> + pmx->reg_bank_size = devm_kcalloc(>dev, pmx->nbanks,
> + sizeof(*pmx->reg_bank_size),
> + GFP_KERNEL);
> + if (!pmx->reg_bank_size)
> + return -ENOMEM;
It looks to me that we don't really need to churn with this allocation because
the
bank sizes are already a part of the platform driver's description.
We could add a simple helper function that retrieves the bank sizes, like this:
static unsigned int tegra_pinctrl_bank_size(struct device *dev,
unsigned int bank_id)
{
struct platform_device *pdev;
struct resource *res;
pdev = to_platform_device(dev);
res = platform_get_resource(pdev, IORESOURCE_MEM, bank_id);
return resource_size(res) / 4;
}
> + pmx->backup_regs = devm_kzalloc(>dev, backup_regs_size,
> + GFP_KERNEL);
> + if (!pmx->backup_regs)
> + return -ENOMEM;
> +
> for (i = 0; i < pmx->nbanks; i++) {
> res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> pmx->regs[i] = devm_ioremap_resource(>dev, res);
> if (IS_ERR(pmx->regs[i]))
> return PTR_ERR(pmx->regs[i]);
> +
> + pmx->reg_bank_size[i] = resource_size(res);
> }
>
> pmx->pctl = devm_pinctrl_register(>dev, _pinctrl_desc, pmx);
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h
> b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 287702660783..55456f8d44cf 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -17,6 +17,8 @@ struct tegra_pmx {
>
>
