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@xxxxxxxxx>
>>>> Cc: Alexey Firago <alexey_firago@xxxxxxxxxx>
>>>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>> Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
>>>> Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
>>>> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
>>>> ---
>>>> 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() .
--
Best regards,
Marek Vasut
