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.

-- 
Regards,

Laurent Pinchart