RE: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before entering sleep

[Ryan Lee]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Monday, September 27, 2021 9:49 AM
> To: Mark Brown <broonie@xxxxxxxxxx>; Ryan Lee
> <RyanS.Lee@xxxxxxxxxxxxxxxxxxx>
> Cc: lgirdwood@xxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx; yung-
> chuan.liao@xxxxxxxxxxxxxxx; guennadi.liakhovetski@xxxxxxxxxxxxxxx; alsa-
> devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> sathya.prakash.m.r@xxxxxxxxx; ryan.lee.maxim@xxxxxxxxx
> Subject: Re: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty
> before entering sleep
> 
> EXTERNAL EMAIL
> 
> 
> 
> On 9/27/21 11:06 AM, Mark Brown wrote:
> > On Mon, Sep 27, 2021 at 04:01:25PM +0000, Ryan Lee wrote:
> >
> >>>>       regcache_cache_only(max98373->regmap, true);
> >>>> +     regcache_mark_dirty(max98373->regmap);
> >
> >>> We already do the following sequence in max98373_io_init() when the
> >>> amplifier re-attaches:
> >
> >>>         if (max98373->first_hw_init) {
> >>>                 regcache_cache_bypass(max98373->regmap, false);
> >>>                 regcache_mark_dirty(max98373->regmap);
> >>>         }
> >
> >>> I don't see what marking the cache as dirty on suspend might do, we
> >>> will do a sync only in the resume step.
> >
> >>> IIRC this is a patch that we've seen before and removed since it
> >>> wasn't aligned with any other codec driver.
> >
> >> Yes, it does. There was an mute problem report due to amp register
> >> reset during suspend/resume. and we confirmed that the modification
> >> is effective.
> >>
> (https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> >>
> rtnerissuetracker.corp.google.com%2Fissues%2F194472331&amp;data=04%
> 7C
> >>
> 01%7Cryans.lee%40maximintegrated.com%7C56f7bb3f05ae4c199dce08d98
> 1d6b8
> >>
> 94%7Cfbd909dfea694788a554f24b7854ad03%7C0%7C0%7C6376835814870
> 22873%7C
> >>
> Unknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJB
> TiI6Ik1
> >>
> haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=U1LZabH5MbrotS976TKK8Rh
> 9nqi0ueRO
> >> %2FxR0obeQlrM%3D&amp;reserved=0) The added code helps to re-write
> >> valid values in cache to the amp hardware when audio resume. Same
> >> code was there on i2c driver, but not on Soundwire driver.
> 
> Ryan, we removed this in f184892613dd ('ASoC: codecs: max98373-sdw:
> align regmap use with other codecs'), so even if this was needed you'd need
> a mention that this is a revert and why this sequence is better.
> You are suggesting a change based on an analogy with I2C which is
> questionable: when a SoundWire device regains sync on the bus, it will be re-
> initialized using a callback, and the resume waits for the initialization to
> complete.

I think there is always possibility that amp lose its power or is reset by hw reset
pin control during audio suspension to minimize current consumption.
Register restoration process is required for both i2c and Soundwire case
when the amp was reset by some reason.
max98373_update_status () is called when audio resume but
' sdw_slave_status' remains ' SDW_SLAVE_ATTACHED' and ' first_hw_init' is 1,
so restoration is not happening. This is software variable and the value remains
the same for the amp hardware reset not triggered by the software driver.
If regcache_mark_dirty() is not called, regcache_sync() will assume that
the hardware state still matches the cache state which causes the mute
problem here.

> 
> > More specifically what it does is make the invalidation of the
> > register cache unconditional.  It doesn't really matter if the
> > invalidation is done on suspend or resume, so long as it happens
> > before we attempt to resync - this could also be done by deleting the
> first_hw_init check.

I tried to delete the first_hw_init, but it didn't work because
other status variable is also related.
Calling regcache_mark_dirty() is needed to solve a synchronization issue
between cache and actual hardware value during suspend/resume,
but it was not called.

> 
> Mark, that's exactly my point: if the amp rejoins the bus, we will
> *always* mark the cache as dirty, before the resync is done in the resume
> sequence.
> 
> I am really trying to figure out if we have a major flaw in the resume
> sequence and why things are different in the case of the Maxim amp.

Maybe other amps were not reset during audio suspend/resume.
Even max98373 was okay with the previous gen. Intel board.

> 
> Instead of changing the suspend sequence, can we please try to modify the
> max98373_io_init() routine to unconditionally flag the cache as dirty, maybe
> this points to a problem with the management of the
> max98373->first_hw_init flag.

max98373_io_init() is not called because ' sdw_slave_status' remains
' SDW_SLAVE_ATTACHED' and 'max98373->hw_init' is already true.
Removing 'if (max98373->hw_init || status != SDW_SLAVE_ATTACHED)'
condition in max98373_update_status() function instead of adding
regcache_mark_dirty() into max98373_suspend() can be an alternative way.
I think it is all about where regcache_mark_dirty() is called from.
The difference is that max98373_io_init() really do the software reset and
do amp initialization again which could be an overhead.