Re: [PATCH V2] clk: vc5: Add suspend/resume support

[Marek Vasut <marek.vasut@gmail.com>]

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() .

-- 
Best regards,
Marek Vasut