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