| Message ID | 1561687972-19319-3-git-send-email-skomatineni@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | SC7 entry and exit support for Tegra210 | expand |
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 <treding@nvidia.com> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > 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 = &tegra_pinctrl_suspend, > + .resume = &tegra_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(&pdev->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(&pdev->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(&pdev->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(&pdev->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(&pdev->dev, &tegra_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 { > > int nbanks; > void __iomem **regs; > + size_t *reg_bank_size; > + u32 *backup_regs; > }; > > enum tegra_pinconf_param { > @@ -193,6 +195,7 @@ struct tegra_pinctrl_soc_data { > bool drvtype_in_mux; > }; > > +extern const struct dev_pm_ops tegra_pinctrl_pm; Please add a newline here. > 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 = &tegra_pinctrl_pm, > }, > .probe = tegra210_pinctrl_probe, > }; > Could you please address my comments in the next revision if there will be one?
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 <treding@nvidia.com> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> 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 = &tegra_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.
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 <treding@nvidia.com> >>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>> --- >>> 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 = &tegra_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
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 <treding@nvidia.com> >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>> --- >>>> 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 = &tegra_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.
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 <treding@nvidia.com> >>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>>> --- >>>>> 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 = &tegra_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.
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 <treding@nvidia.com> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > 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 = &tegra_pinctrl_suspend, > + .resume = &tegra_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.
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 <treding@nvidia.com> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> 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 = &tegra_pinctrl_suspend, >> + .resume = &tegra_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.
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 <treding@nvidia.com> >>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>> --- >>> 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 = &tegra_pinctrl_suspend, >>> + .resume = &tegra_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.
On Fri, Jun 28, 2019 at 4:13 AM Sowjanya Komatineni <skomatineni@nvidia.com> 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 <treding@nvidia.com> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> Looks good. Can I just apply this patch or does it need to go in with the other (clk) changes? Yours, Linus Walleij
On Sat, Jun 29, 2019 at 5:58 PM Dmitry Osipenko <digetx@gmail.com> 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
04.07.2019 10:31, Linus Walleij пишет: > On Sat, Jun 29, 2019 at 5:58 PM Dmitry Osipenko <digetx@gmail.com> 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/
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 <digetx@gmail.com> 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.
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 <treding@nvidia.com> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> 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 = &tegra_pinctrl_suspend, >> + .resume = &tegra_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.
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 <digetx@gmail.com> >>> 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!
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 = &tegra_pinctrl_suspend, + .resume = &tegra_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(&pdev->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(&pdev->dev, pmx->nbanks, + sizeof(*pmx->reg_bank_size), + GFP_KERNEL); + if (!pmx->reg_bank_size) + return -ENOMEM; + + pmx->backup_regs = devm_kzalloc(&pdev->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(&pdev->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(&pdev->dev, &tegra_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 { int nbanks; void __iomem **regs; + size_t *reg_bank_size; + u32 *backup_regs; }; enum tegra_pinconf_param { @@ -193,6 +195,7 @@ struct tegra_pinctrl_soc_data { bool drvtype_in_mux; }; +extern const struct dev_pm_ops tegra_pinctrl_pm; 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 = &tegra_pinctrl_pm, }, .probe = tegra210_pinctrl_probe, };