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

[Ryan Lee]

> > >> 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.
> >
> > that description is aligned with my analysis that there's something
> > very wrong happening here, it's not just a simple miss in the regmap
> > handling but a major conceptual bug or misunderstanding in the way
> > reset is handled.
> >
> > First, there's the spec: on a reset initiated by the host or if the
> > device loses sync for ANY reason, its status cannot remain
> ATTACHED.
> > There's got to be a 16-frame period at least where the device has to
> > monitor the sync pattern and cannot drive anything on the bus.
> >
> > Then there's the hardware behavior on resume: on resume by default
> the
> > Intel host will toggle the data pin for at least 4096 frames, which by
> > spec means severe reset.
> >
> > And last, there's the software init: we also force the status as
> > UNATTACHED in drivers/soundwire/intel.c:
> >
> >         /*
> >          * make sure all Slaves are tagged as UNATTACHED and provide
> >          * reason for reinitialization
> >          */
> >         sdw_clear_slave_status(bus,
> > SDW_UNATTACH_REQUEST_MASTER_RESET);
> >
> > But we've also seen the opposite effect of an amplifier reporting
> > attached but losing sync immediately after the end of enumeration
> and
> > never coming back on the bus, see issue
> > https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%
> >
> 2Fgithub.com%2Fthesofproject%2Flinux%2Fissues%2F3063&data
> >
> =04%7C01%7Cryans.lee%40maximintegrated.com%7Cb9f84a1267ec4
> >
> f50b7a008d981edcc46%7Cfbd909dfea694788a554f24b7854ad03%7C0
> > %7C0%7C637683680607026027%7CUnknown%7CTWFpbGZsb3d8eyJ
> >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> > %3D%7C1000&sdata=rARkwTSB3DN%2BCYxGaehOhtCGEj1eLBl
> 6
> > Mk7QhynQSY8%3D&reserved=0
> >
> > In other words, we need to check what really happens on resume
> and why
> > the amplifier keeps reporting its status as ATTACHED despite the spec
> > requirements and software init, or loses this status after
> > enumeration....Something really does not add-up, again it's not just a
> > regmap management issue.
> >
> >
> >
> I do not see #3063 issue on my side. No initialization failure or time-
> out has occurred.
> 
> Now I'm trying to solve the issue with max98373_io_init() function as
> suggested instead of adding
> regmap_cache_dirty() in the suspend function.
> max98373_io_init() was not called from max98373_update_status()
> when audio resume because
> max98373->hw_init was 1 and Status was SDW_SLAVE_ATTACHED.
> max98373_update_status() do not get SDW_SLAVE_UNATTACHED.
> I confirmed that the issue could be resolved if
> SDW_SLAVE_UNATTACHED event arrives at
> max98373_update_status() before SDW_SLAVE_ATTACHED is
> triggered.
> Actually sdw_handle_slave_status() get SDW_SLAVE_UNATTACHED
> but this function exits at
> https://github.com/thesofproject/linux/blob/topic/sof-
> dev/drivers/soundwire/bus.c#L1765
> before reaching to
> https://github.com/thesofproject/linux/blob/topic/sof-
> dev/drivers/soundwire/bus.c#L1825
> I'm not sure how to solve this issue because this code is commonly
> used for other Soundwire drivers as well.
> 
> I share the debug messages for the resume event as your reference.
> [  127.490644] [DEBUG3] intel_resume_runtime [  127.490655]
> [DEBUG3] intel_resume_runtime SDW_INTEL_CLK_STOP_BUS_RESET
> [  127.490658] [DEBUG3] intel_init [  127.490660] [DEBUG3]
> intel_link_power_up [  127.490977] [DEBUG3] intel_resume_runtime
> SDW_UNATTACH_REQUEST_MASTER_RESET ..
> [  127.490980] [DEBUG4] sdw_clear_slave_status request: 1
> [  127.490983] [DEBUG4] sdw_modify_slave_status, ID:7, status: 0
> [  127.490986] [DEBUG4] sdw_modify_slave_status, ID:3, status: 0
> [  127.490994] [DEBUG3] intel_shim_wake wake_enable:0
> [  127.491060] [DEBUG3] intel_shim_wake wake_enable:0
> [  127.491191] [DEBUG] max98373_resume, first_hw_init: 1,
> unattach_request: 1 [  127.491194] [DEBUG] max98373_resume, INF
> MODE: 0 [  127.491953] [DEBUG4] sdw_handle_slave_status IN
> [  127.491956] [DEBUG4] sdw_handle_slave_status, status[1] : 0,
> slave->status: 0, id:7	// UNATTACHED
> [  127.491958] [DEBUG4] sdw_handle_slave_status, status[2] : 0,
> slave->status: 0, id:3 [  127.491960] [DEBUG4]
> sdw_handle_slave_status IN2 status[0] = 1 [  127.492808] [DEBUG4]
> sdw_handle_slave_status IN
> [  127.492810] [DEBUG4] sdw_handle_slave_status, status[1] : 1,
> slave->status: 0, id:7	// ATTACHED
> [  127.492812] [DEBUG4] sdw_handle_slave_status, status[2] : 1,
> slave->status: 0, id:3 [  127.492814] [DEBUG4]
> sdw_handle_slave_status IN2 status[0] = 0 [  127.492816] [DEBUG4]
> sdw_handle_slave_status IN3 [  127.492818] [DEBUG4]
> sdw_handle_slave_status status[1] = SDW_SLAVE_ATTACHED, slave-
> >status : 0, slave:7, prev_status:0 [  127.492820] [DEBUG4]
> sdw_modify_slave_status, ID:7, status: 1 [  127.493008] [DEBUG4]
> sdw_update_slave_status update_status(1) IN slave:7 [  127.493010]
> [DEBUG4] sdw_update_slave_status update_status(1) OUT
> [  127.493012] [DEBUG] max98373_update_status IN hw_init:1, status:
> 1, slave :7 [  127.493015] [DEBUG] max98373_update_status IN2
> hw_init:1, max98373->first_hw_init: 1, status: 1 [  127.493017]
> [DEBUG4] sdw_handle_slave_status status[2] =
> SDW_SLAVE_ATTACHED, slave->status : 0, slave:3, prev_status:0
> [  127.493019] [DEBUG4] sdw_modify_slave_status, ID:3, status: 1
> [  127.493199] [DEBUG4] sdw_update_slave_status update_status(1)
> IN slave:3 [  127.493201] [DEBUG4] sdw_update_slave_status
> update_status(1) OUT [  127.493204] [DEBUG]
> max98373_update_status IN hw_init:1, status: 1, slave :3
> [  127.493207] [DEBUG] max98373_update_status IN2 hw_init:1,
> max98373->first_hw_init: 1, status: 1

Thanks for the comments.
I tried to find the reason why the amp was not detached from the bus properly and
found information about CLOCK_STOP_NOW bit in 0x0044 SCP_Ctrl register.
It seems like 0x2(ClockStopNow) needs to be configured before the host CLOCK STOP.
I was able to get a good result if I add this command in the amp driver suspend function.
The amp driver receives the detachment event and register restoration was done properly
after the audio resume.
I can modify the amp driver for this change but it looks like this needs to be done
from the host side. May I have a comment on this? Thanks.