| Message ID | 20181212014130.18634-1-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [V2] clk: vc5: Add suspend/resume support | expand |
Hi Marex, Thank you for the patch. On Wednesday, 12 December 2018 03:41:30 EET Marek Vasut wrote: > 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 > --- > V2: Replace ifdef with __maybe_unused > Simplify return value handling in resume > --- > drivers/clk/clk-versaclock5.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > index decffb3826ec..b66586a3abb7 100644 > --- a/drivers/clk/clk-versaclock5.c > +++ b/drivers/clk/clk-versaclock5.c > @@ -906,6 +906,34 @@ static int vc5_remove(struct i2c_client *client) > return 0; > } > > +static int __maybe_unused vc5_suspend(struct device *dev) > +{ > + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); > + int ret; > + > + ret = regcache_sync(vc5->regmap); Didn't you say the sync here was unneeded and would be dropped ? > + 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 __maybe_unused 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) > + dev_err(dev, "Failed to restore register map: %d\n", ret); > + return ret; > +} > + > static const struct vc5_chip_info idt_5p49v5923_info = { > .model = IDT_VC5_5P49V5923, > .clk_fod_cnt = 2, > @@ -961,9 +989,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,
On 12/12/2018 09:43 AM, Laurent Pinchart wrote: > Hi Marex, Hi, > Thank you for the patch. > > On Wednesday, 12 December 2018 03:41:30 EET Marek Vasut wrote: >> 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 >> --- >> V2: Replace ifdef with __maybe_unused >> Simplify return value handling in resume >> --- >> drivers/clk/clk-versaclock5.c | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c >> index decffb3826ec..b66586a3abb7 100644 >> --- a/drivers/clk/clk-versaclock5.c >> +++ b/drivers/clk/clk-versaclock5.c >> @@ -906,6 +906,34 @@ static int vc5_remove(struct i2c_client *client) >> return 0; >> } >> >> +static int __maybe_unused vc5_suspend(struct device *dev) >> +{ >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = regcache_sync(vc5->regmap); > > Didn't you say the sync here was unneeded and would be dropped ? > >> + if (ret != 0) { >> + dev_err(dev, "Failed to save register map: %d\n", ret); >> + return ret; >> + } If you have a setup with working DU and VGA on something close to next (it's broken in next), can you try dropping this hunk (basically do ret = 0;//regcache_sync(vc5->regmap); ) and see if the regcache stays consistent ? It should. If so, I'll drop this in V3. >> + regcache_cache_only(vc5->regmap, true); >> + regcache_mark_dirty(vc5->regmap); [...]
Hi Marek, On Thursday, 13 December 2018 04:09:19 EET Marek Vasut wrote: > On 12/12/2018 09:43 AM, Laurent Pinchart wrote: > > On Wednesday, 12 December 2018 03:41:30 EET Marek Vasut wrote: > >> 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 > >> --- > >> V2: Replace ifdef with __maybe_unused > >> > >> Simplify return value handling in resume > >> > >> --- > >> > >> drivers/clk/clk-versaclock5.c | 31 +++++++++++++++++++++++++++++++ > >> 1 file changed, 31 insertions(+) > >> > >> diff --git a/drivers/clk/clk-versaclock5.c > >> b/drivers/clk/clk-versaclock5.c > >> index decffb3826ec..b66586a3abb7 100644 > >> --- a/drivers/clk/clk-versaclock5.c > >> +++ b/drivers/clk/clk-versaclock5.c > >> @@ -906,6 +906,34 @@ static int vc5_remove(struct i2c_client *client) > >> return 0; > >> } > >> > >> +static int __maybe_unused vc5_suspend(struct device *dev) > >> +{ > >> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); > >> + int ret; > >> + > >> + ret = regcache_sync(vc5->regmap); > > > > Didn't you say the sync here was unneeded and would be dropped ? > > > >> + if (ret != 0) { > >> + dev_err(dev, "Failed to save register map: %d\n", ret); > >> + return ret; > >> + } > > If you have a setup with working DU and VGA on something close to next > (it's broken in next), can you try dropping this hunk (basically do > ret = 0;//regcache_sync(vc5->regmap); ) and see if the regcache stays > consistent ? It should. If so, I'll drop this in V3. > > >> + regcache_cache_only(vc5->regmap, true); > >> + regcache_mark_dirty(vc5->regmap); > > [...] Here's my test procedure: - Boot the board - Dump the VC5 state (into vc5-next+*-1-boot.log) - Enable the VGA output with kmstest -c VGA-1 - Dump the VC5 state (into vc5-next+*-2-display-on.log) - Suspend and resume - Dump the VC5 state (into vc5-next+*-3-post-suspend.log) - Stop kmstest - Dump the VC5 state (into vc5-next+*-4-display-off.log) To dump the VC5 state, I use ----------------------------------------------------------------------------- #!/bin/sh echo "-------- i2cdump --------" i2cdump -f -y 4 0x6a for f in /sys/kernel/debug/regmap/4-006a/* ; do echo "-------- $f --------" cat $f done ----------------------------------------------------------------------------- The base kernel version is v4.20-rc6 + the fixes branch from linux media. I've tested the following three configurations, in order: next+0 - Base next+1 - Base + this patch next+2 - Base + this patch + removal of regcache_sync() from vc5_suspend() With base, the VGA output would remain off after resume, and running kmstest again wouldn't help. With the other two configurations the problem is fixed and the VGA output is functional. Furthermore, there are no differences in the VC5 dumps between next+1 and next+2, neither are there between the boot and display-on dumps between any of the three configurations. I've attached the logs to this e-mail.
On 12/13/2018 05:52 AM, Laurent Pinchart wrote: > Hi Marek, Hi, > On Thursday, 13 December 2018 04:09:19 EET Marek Vasut wrote: >> On 12/12/2018 09:43 AM, Laurent Pinchart wrote: >>> On Wednesday, 12 December 2018 03:41:30 EET Marek Vasut wrote: >>>> 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 >>>> --- >>>> V2: Replace ifdef with __maybe_unused >>>> >>>> Simplify return value handling in resume >>>> >>>> --- >>>> >>>> drivers/clk/clk-versaclock5.c | 31 +++++++++++++++++++++++++++++++ >>>> 1 file changed, 31 insertions(+) >>>> >>>> diff --git a/drivers/clk/clk-versaclock5.c >>>> b/drivers/clk/clk-versaclock5.c >>>> index decffb3826ec..b66586a3abb7 100644 >>>> --- a/drivers/clk/clk-versaclock5.c >>>> +++ b/drivers/clk/clk-versaclock5.c >>>> @@ -906,6 +906,34 @@ static int vc5_remove(struct i2c_client *client) >>>> return 0; >>>> } >>>> >>>> +static int __maybe_unused vc5_suspend(struct device *dev) >>>> +{ >>>> + struct vc5_driver_data *vc5 = dev_get_drvdata(dev); >>>> + int ret; >>>> + >>>> + ret = regcache_sync(vc5->regmap); >>> >>> Didn't you say the sync here was unneeded and would be dropped ? >>> >>>> + if (ret != 0) { >>>> + dev_err(dev, "Failed to save register map: %d\n", ret); >>>> + return ret; >>>> + } >> >> If you have a setup with working DU and VGA on something close to next >> (it's broken in next), can you try dropping this hunk (basically do >> ret = 0;//regcache_sync(vc5->regmap); ) and see if the regcache stays >> consistent ? It should. If so, I'll drop this in V3. >> >>>> + regcache_cache_only(vc5->regmap, true); >>>> + regcache_mark_dirty(vc5->regmap); >> >> [...] > > Here's my test procedure: > > - Boot the board > - Dump the VC5 state (into vc5-next+*-1-boot.log) > - Enable the VGA output with kmstest -c VGA-1 I didn't know this was needed, all right. > - Dump the VC5 state (into vc5-next+*-2-display-on.log) > - Suspend and resume > - Dump the VC5 state (into vc5-next+*-3-post-suspend.log) > - Stop kmstest > - Dump the VC5 state (into vc5-next+*-4-display-off.log) > > To dump the VC5 state, I use > > ----------------------------------------------------------------------------- > #!/bin/sh > > echo "-------- i2cdump --------" > i2cdump -f -y 4 0x6a > > for f in /sys/kernel/debug/regmap/4-006a/* ; do > echo "-------- $f --------" > cat $f > done > ----------------------------------------------------------------------------- > > The base kernel version is v4.20-rc6 + the fixes branch from linux media. I've > tested the following three configurations, in order: > > next+0 - Base > next+1 - Base + this patch > next+2 - Base + this patch + removal of regcache_sync() from vc5_suspend() > > With base, the VGA output would remain off after resume, and running kmstest > again wouldn't help. With the other two configurations the problem is fixed > and the VGA output is functional. > > Furthermore, there are no differences in the VC5 dumps between next+1 and > next+2, neither are there between the boot and display-on dumps between any of > the three configurations. > > I've attached the logs to this e-mail. Thanks for the test, I sent out a V3 without the regcache_sync() .
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c index decffb3826ec..b66586a3abb7 100644 --- a/drivers/clk/clk-versaclock5.c +++ b/drivers/clk/clk-versaclock5.c @@ -906,6 +906,34 @@ static int vc5_remove(struct i2c_client *client) return 0; } +static int __maybe_unused 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 __maybe_unused 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) + dev_err(dev, "Failed to restore register map: %d\n", ret); + return ret; +} + static const struct vc5_chip_info idt_5p49v5923_info = { .model = IDT_VC5_5P49V5923, .clk_fod_cnt = 2, @@ -961,9 +989,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 --- V2: Replace ifdef with __maybe_unused Simplify return value handling in resume --- drivers/clk/clk-versaclock5.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)