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%2BCYxGaehOhtCGEj1eLBl6 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/... before reaching to https://github.com/thesofproject/linux/blob/topic/sof-dev/drivers/soundwire/... 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