| Message ID | 20181204182721.28258-1-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: vc5: Add suspend/resume support | expand |
Quoting Marek Vasut (2018-12-04 10:27:21) > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > index decffb3826ec..ac90fb36af1a 100644 > --- a/drivers/clk/clk-versaclock5.c > +++ b/drivers/clk/clk-versaclock5.c > @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client) > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > +static int vc5_suspend(struct device *dev) Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP > +{ > + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); > + int ret; > + > + ret = regcache_sync(vc5->regmap); > + if (ret != 0) { > + dev_err(dev, "Failed to save register map: %d\n", ret); > + return ret; Do we need to block suspend if we can't save the register map away? Or can we just throw up our hands and not restore on resume? > + } > + regcache_cache_only(vc5->regmap, true); > + regcache_mark_dirty(vc5->regmap); > + > + return 0; > +} > + > +static int vc5_resume(struct device *dev) > +{ > + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); > + int ret; > + > + regcache_cache_only(vc5->regmap, false); > + ret = regcache_sync(vc5->regmap); > + if (ret != 0) { > + dev_err(dev, "Failed to restore register map: %d\n", ret); > + return ret; > + } Simplify to if (ret) dev_err() retun ret; > + > + return 0; > +} > +#endif > + > static const struct vc5_chip_info idt_5p49v5923_info = { > .model = IDT_VC5_5P49V5923, > .clk_fod_cnt = 2,
On 12/04/2018 09:52 PM, Stephen Boyd wrote: > Quoting Marek Vasut (2018-12-04 10:27:21) >> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c >> index decffb3826ec..ac90fb36af1a 100644 >> --- a/drivers/clk/clk-versaclock5.c >> +++ b/drivers/clk/clk-versaclock5.c >> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client) >> return 0; >> } >> >> +#ifdef CONFIG_PM_SLEEP >> +static int vc5_suspend(struct device *dev) > > Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP > >> +{ >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = regcache_sync(vc5->regmap); >> + if (ret != 0) { >> + dev_err(dev, "Failed to save register map: %d\n", ret); >> + return ret; > > Do we need to block suspend if we can't save the register map away? Or > can we just throw up our hands and not restore on resume? Some hardware will fail on resume, so I'd say -- yes ? The rest is fixed. >> + } >> + regcache_cache_only(vc5->regmap, true); >> + regcache_mark_dirty(vc5->regmap); >> + >> + return 0; >> +} >> + >> +static int vc5_resume(struct device *dev) >> +{ >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); >> + int ret; >> + >> + regcache_cache_only(vc5->regmap, false); >> + ret = regcache_sync(vc5->regmap); >> + if (ret != 0) { >> + dev_err(dev, "Failed to restore register map: %d\n", ret); >> + return ret; >> + } > > Simplify to > > if (ret) > dev_err() > retun ret; > >> + >> + return 0; >> +} >> +#endif >> + >> static const struct vc5_chip_info idt_5p49v5923_info = { >> .model = IDT_VC5_5P49V5923, >> .clk_fod_cnt = 2,
Hi Marek, On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote: > On 12/04/2018 09:52 PM, Stephen Boyd wrote: > > Quoting Marek Vasut (2018-12-04 10:27:21) > > > >> diff --git a/drivers/clk/clk-versaclock5.c > >> b/drivers/clk/clk-versaclock5.c > >> index decffb3826ec..ac90fb36af1a 100644 > >> --- a/drivers/clk/clk-versaclock5.c > >> +++ b/drivers/clk/clk-versaclock5.c > >> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client) > >> > >> return 0; > >> > >> } > >> > >> +#ifdef CONFIG_PM_SLEEP > >> +static int vc5_suspend(struct device *dev) > > > > Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP > > > >> +{ > >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); > >> + int ret; > >> + > >> + ret = regcache_sync(vc5->regmap); > >> + if (ret != 0) { > >> + dev_err(dev, "Failed to save register map: %d\n", ret); > >> + return ret; > > > > Do we need to block suspend if we can't save the register map away? Or > > can we just throw up our hands and not restore on resume? > > Some hardware will fail on resume, so I'd say -- yes ? But why do you need to sync on suspend in the first place ? What could cause the map to be dirty at this stage, and require syncing before suspend, that couldn't work with the sync be delayed to resume time ? > The rest is fixed. > > >> + } > >> + regcache_cache_only(vc5->regmap, true); > >> + regcache_mark_dirty(vc5->regmap); > >> + > >> + return 0; > >> +} > >> + > >> +static int vc5_resume(struct device *dev) > >> +{ > >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); > >> + int ret; > >> + > >> + regcache_cache_only(vc5->regmap, false); > >> + ret = regcache_sync(vc5->regmap); > >> + if (ret != 0) { > >> + dev_err(dev, "Failed to restore register map: %d\n", > >> ret); > >> + return ret; > >> + } > > > > Simplify to > > > > if (ret) > > > > dev_err() > > > > retun ret; > > > >> + > >> + return 0; > >> +} > >> +#endif > >> + > >> > >> static const struct vc5_chip_info idt_5p49v5923_info = { > >> > >> .model = IDT_VC5_5P49V5923, > >> .clk_fod_cnt = 2,
On 12/05/2018 06:21 AM, Laurent Pinchart wrote: > Hi Marek, > > On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote: >> On 12/04/2018 09:52 PM, Stephen Boyd wrote: >>> Quoting Marek Vasut (2018-12-04 10:27:21) >>> >>>> diff --git a/drivers/clk/clk-versaclock5.c >>>> b/drivers/clk/clk-versaclock5.c >>>> index decffb3826ec..ac90fb36af1a 100644 >>>> --- a/drivers/clk/clk-versaclock5.c >>>> +++ b/drivers/clk/clk-versaclock5.c >>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client) >>>> >>>> return 0; >>>> >>>> } >>>> >>>> +#ifdef CONFIG_PM_SLEEP >>>> +static int vc5_suspend(struct device *dev) >>> >>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP >>> >>>> +{ >>>> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + ret = regcache_sync(vc5->regmap); >>>> + if (ret != 0) { >>>> + dev_err(dev, "Failed to save register map: %d\n", ret); >>>> + return ret; >>> >>> Do we need to block suspend if we can't save the register map away? Or >>> can we just throw up our hands and not restore on resume? >> >> Some hardware will fail on resume, so I'd say -- yes ? > > But why do you need to sync on suspend in the first place ? What could cause > the map to be dirty at this stage, and require syncing before suspend, that > couldn't work with the sync be delayed to resume time ? Possibly a configuration coming from eg. bootloader time , or some other configuration not done by Linux.
Hi Marek, On Wednesday, 5 December 2018 14:29:22 EET Marek Vasut wrote: > On 12/05/2018 06:21 AM, Laurent Pinchart wrote: > > On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote: > >> On 12/04/2018 09:52 PM, Stephen Boyd wrote: > >>> Quoting Marek Vasut (2018-12-04 10:27:21) > >>> > >>>> diff --git a/drivers/clk/clk-versaclock5.c > >>>> b/drivers/clk/clk-versaclock5.c > >>>> index decffb3826ec..ac90fb36af1a 100644 > >>>> --- a/drivers/clk/clk-versaclock5.c > >>>> +++ b/drivers/clk/clk-versaclock5.c > >>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client) > >>>> return 0; > >>>> } > >>>> > >>>> +#ifdef CONFIG_PM_SLEEP > >>>> +static int vc5_suspend(struct device *dev) > >>> > >>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP > >>> > >>>> +{ > >>>> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); > >>>> + int ret; > >>>> + > >>>> + ret = regcache_sync(vc5->regmap); > >>>> + if (ret != 0) { > >>>> + dev_err(dev, "Failed to save register map: %d\n", ret); > >>>> + return ret; > >>> > >>> Do we need to block suspend if we can't save the register map away? Or > >>> can we just throw up our hands and not restore on resume? > >> > >> Some hardware will fail on resume, so I'd say -- yes ? > > > > But why do you need to sync on suspend in the first place ? What could > > cause the map to be dirty at this stage, and require syncing before > > suspend, that couldn't work with the sync be delayed to resume time ? > > Possibly a configuration coming from eg. bootloader time , or some other > configuration not done by Linux. I still don't get it. As far as I know, regcache_sync() will write the content of the regmap to the hardware, not the other way around. You call it at resume time, so the hardware should be programmed properly, regardless of what the boot loader or the firmware does when resuming. Could you please explain why this is needed at suspend time ?
On 12/05/2018 02:54 PM, Laurent Pinchart wrote: > Hi Marek, Hi, > On Wednesday, 5 December 2018 14:29:22 EET Marek Vasut wrote: >> On 12/05/2018 06:21 AM, Laurent Pinchart wrote: >>> On Wednesday, 5 December 2018 01:48:01 EET Marek Vasut wrote: >>>> On 12/04/2018 09:52 PM, Stephen Boyd wrote: >>>>> Quoting Marek Vasut (2018-12-04 10:27:21) >>>>> >>>>>> diff --git a/drivers/clk/clk-versaclock5.c >>>>>> b/drivers/clk/clk-versaclock5.c >>>>>> index decffb3826ec..ac90fb36af1a 100644 >>>>>> --- a/drivers/clk/clk-versaclock5.c >>>>>> +++ b/drivers/clk/clk-versaclock5.c >>>>>> @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client) >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +#ifdef CONFIG_PM_SLEEP >>>>>> +static int vc5_suspend(struct device *dev) >>>>> >>>>> Please mark as __maybe_unused and drop the #ifdef CONFIG_PM_SLEEP >>>>> >>>>>> +{ >>>>>> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); >>>>>> + int ret; >>>>>> + >>>>>> + ret = regcache_sync(vc5->regmap); >>>>>> + if (ret != 0) { >>>>>> + dev_err(dev, "Failed to save register map: %d\n", ret); >>>>>> + return ret; >>>>> >>>>> Do we need to block suspend if we can't save the register map away? Or >>>>> can we just throw up our hands and not restore on resume? >>>> >>>> Some hardware will fail on resume, so I'd say -- yes ? >>> >>> But why do you need to sync on suspend in the first place ? What could >>> cause the map to be dirty at this stage, and require syncing before >>> suspend, that couldn't work with the sync be delayed to resume time ? >> >> Possibly a configuration coming from eg. bootloader time , or some other >> configuration not done by Linux. > > I still don't get it. As far as I know, regcache_sync() will write the content > of the regmap to the hardware, not the other way around. You call it at resume > time, so the hardware should be programmed properly, regardless of what the > boot loader or the firmware does when resuming. > > Could you please explain why this is needed at suspend time ? It is not, can be dropped.
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c index decffb3826ec..ac90fb36af1a 100644 --- a/drivers/clk/clk-versaclock5.c +++ b/drivers/clk/clk-versaclock5.c @@ -906,6 +906,39 @@ static int vc5_remove(struct i2c_client *client) return 0; } +#ifdef CONFIG_PM_SLEEP +static int vc5_suspend(struct device *dev) +{ + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); + int ret; + + ret = regcache_sync(vc5->regmap); + if (ret != 0) { + dev_err(dev, "Failed to save register map: %d\n", ret); + return ret; + } + regcache_cache_only(vc5->regmap, true); + regcache_mark_dirty(vc5->regmap); + + return 0; +} + +static int vc5_resume(struct device *dev) +{ + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); + int ret; + + regcache_cache_only(vc5->regmap, false); + ret = regcache_sync(vc5->regmap); + if (ret != 0) { + dev_err(dev, "Failed to restore register map: %d\n", ret); + return ret; + } + + return 0; +} +#endif + static const struct vc5_chip_info idt_5p49v5923_info = { .model = IDT_VC5_5P49V5923, .clk_fod_cnt = 2, @@ -961,9 +994,12 @@ static const struct of_device_id clk_vc5_of_match[] = { }; MODULE_DEVICE_TABLE(of, clk_vc5_of_match); +static SIMPLE_DEV_PM_OPS(vc5_pm_ops, vc5_suspend, vc5_resume); + static struct i2c_driver vc5_driver = { .driver = { .name = "vc5", + .pm = &vc5_pm_ops, .of_match_table = clk_vc5_of_match, }, .probe = vc5_probe,
Add simple suspend/resume handlers to the driver to restore the chip configuration after resume. It is possible that the chip was configured with non-default values before suspend-resume cycle and that the chip is powered down during this cycle, so the configuration could get lost. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Alexey Firago <alexey_firago@mentor.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Michael Turquette <mturquette@baylibre.com> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: linux-renesas-soc@vger.kernel.org --- drivers/clk/clk-versaclock5.c | 36 +++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)