Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Ryan Lee <RyanS.Lee@maximintegrated.com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Mark Brown <broonie@kernel.org>
CC:     "lgirdwood@gmail.com" <lgirdwood@gmail.com>,
        "perex@perex.cz" <perex@perex.cz>,
        "tiwai@suse.com" <tiwai@suse.com>,
        "yung-chuan.liao@linux.intel.com" <yung-chuan.liao@linux.intel.com>,
        "guennadi.liakhovetski@linux.intel.com" 
        <guennadi.liakhovetski@linux.intel.com>,
        "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "sathya.prakash.m.r@intel.com" <sathya.prakash.m.r@intel.com>,
        "ryan.lee.maxim@gmail.com" <ryan.lee.maxim@gmail.com>
Subject: RE: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before
 entering sleep
Thread-Topic: [EXTERNAL] Re: [PATCH] ASoC: max98373: Mark cache dirty before
 entering sleep
Thread-Index: AQHXsZF45/jZbOMADUKIjx+r35kUIKu3/BEAgAAPiFCAAAR+AIAAC+UAgAAIKhCAACX/gIAD0nCQgAEkjoA=
Date:   Thu, 30 Sep 2021 23:31:23 +0000
Message-ID: <SJ0PR11MB5661A19958E5E41125FDD695E7AA9@SJ0PR11MB5661.namprd11.prod.outlook.com>
References: <20210924221305.17886-1-ryans.lee@maximintegrated.com>
 <1b21bbf1-12c7-726d-bff8-76ec88ff8635@linux.intel.com>
 <SJ0PR11MB566107A6AB3D18ABDEDCF245E7A79@SJ0PR11MB5661.namprd11.prod.outlook.com>
 <20210927160622.GE4199@sirena.org.uk>
 <7b8c3875-3f12-f3cb-7da8-4e850e59ee2b@linux.intel.com>
 <SJ0PR11MB5661814BCC6B79EDE1B0967AE7A79@SJ0PR11MB5661.namprd11.prod.outlook.com>
 <c5031731-dd58-ff7a-857e-b9e1b748d3b2@linux.intel.com>
 <SJ0PR11MB5661A2F6089A9AEF4143C11CE7AA9@SJ0PR11MB5661.namprd11.prod.outlook.com>
In-Reply-To: <SJ0PR11MB5661A2F6089A9AEF4143C11CE7AA9@SJ0PR11MB5661.namprd11.prod.outlook.com>
Accept-Language: en-US, ko-KR
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?m5fSLobYQiulgynhAAccrJnWmWNlgtJ+ChKqGfSXPPG9/dEpbNELofUwMBee?=
 =?us-ascii?Q?UjMWznDsnzrlYeMuQYV9651/KpfK3chAFkHV+niIbehsVm/D9uWz4QAVhcXe?=
 =?us-ascii?Q?yLPxH60z/4SSvdyul7pO7TXZyTDmhk6wkN9pD6mWEi6m16cO+kmkzjSA1jfb?=
 =?us-ascii?Q?vJPmnvIPCQ7JUMgerIb4EITiaxrKvwiIMS8mHt2+VTSS7GGIoCpymrHgxi9i?=
 =?us-ascii?Q?ffzUFT82JCO/H3L0hVCgZVYodqQkrTAEv2P0rcyBfH8KtNTU/NEAuSV2AOuy?=
 =?us-ascii?Q?jvCj6U4qtOzC95oM/SXjmKHKkq8jfb6QnkjvhqnNhlm1TGxKnJuyEFqy1m/U?=
 =?us-ascii?Q?3/WZK2AY33c6n9SpqQ9reALTbPZzfztkhK1w+BM0t4Zz6FmXVPvYM/GCvz7V?=
 =?us-ascii?Q?pXjOvBDkiKsVy288lsFg7oIrqp6dZMD9Q68ApkbG0SZ7ShU3IB9E5fzU6JZP?=
 =?us-ascii?Q?elokN+fDIA7LLGPozgdVlc93AlLnUMpeULKyd/+e37iZxXgj7/7ddAm3DFHr?=
 =?us-ascii?Q?lIwOpZ2xZ4fK2uaMnOiKYy43/bmThGSnQIJr96k6CvNa9Zh2VOaE15/ta0DH?=
 =?us-ascii?Q?XBEgHJsdJng5UKqY7K10yip9pVksmnwnL8jFg9llkgAZAcoV+us9flYatrvk?=
 =?us-ascii?Q?BfD22SI+5bQzofZYplOaXt7blTO9koEmjmI0FPZfnlKl2a+s+knwKWuuegp0?=
 =?us-ascii?Q?6Zo2/Y8HixZsi9sd2w+6IR8VmYCitblUGUDzFhiz+HujSP4mvUrqm72+hFiJ?=
 =?us-ascii?Q?jKjGM0GoQmzvd0qlUPz7e2FIXeKCaALTgCNPR61N2WLtvMZm034SbmLvOC1h?=
 =?us-ascii?Q?Ix3Sv/HzEhBy7Dj1QylVbHseVMqaqn74VtuHrj4U58istGf42qhC9kckVW6c?=
 =?us-ascii?Q?FIFGkEGjbOOz+AVpK7HueP6GjlqiQ+IGrFl5bD0PWK4O07zwvAxGOgV9CRe2?=
 =?us-ascii?Q?vjO9yj8YuN/tBt5tQGj0TY3gJYcrMA5N9bcH8leXehTX7djPXY2zz11hB6C0?=
 =?us-ascii?Q?xA4mPYXCG+rhlcKpC0iM2dlU80eULSKRtgw8uIBbukJstfw0Q+jiVlbTemDB?=
 =?us-ascii?Q?OEa94h2dMYgKJ+1hA3rSwZmjbU5ZnqfWvTGHKYTsyPOFt63oCciyrVJTlCMV?=
 =?us-ascii?Q?oAzDrGK+/NLmUcBY67pG5F3CCCkP7N5jqXIRGiEiEHJwbeQY4S0R5q+knY7B?=
 =?us-ascii?Q?68KDkWwHxWdTzYVex8VuuLksXuTaItWAeyIq2GdRTMVTH/5O/vLISKEGoD9u?=
 =?us-ascii?Q?7ruDwk9CB1S7ejIEgMGRhEIDv3/Uu4iGa+BzLd1vF3+04HpMBT+blBFnHTUZ?=
 =?us-ascii?Q?MU1/v0n3pVE88p4tLQGOfLUO?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: maximintegrated.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR11MB5661.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5154ecc7-fd6c-4fff-8410-08d9846a6a49
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Sep 2021 23:31:23.2595
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: fbd909df-ea69-4788-a554-f24b7854ad03
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 6vpK/W1I5A+6LP9ytuQTZFCDZ134TQePjn8uU9fY2LJL7fsajRWUmec+dhOTnbKzItRqhhtXaGxYM7MhBP83yk4dZRKUbIqQEhIzF7YK02I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5663
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > >> 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 !=3D
> > 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=3Dhttps%3A%2F%
> >
> 2Fgithub.com%2Fthesofproject%2Flinux%2Fissues%2F3063&amp;data
> >
> =3D04%7C01%7Cryans.lee%40maximintegrated.com%7Cb9f84a1267ec4
> >
> f50b7a008d981edcc46%7Cfbd909dfea694788a554f24b7854ad03%7C0
> > %7C0%7C637683680607026027%7CUnknown%7CTWFpbGZsb3d8eyJ
> >
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> > %3D%7C1000&amp;sdata=3DrARkwTSB3DN%2BCYxGaehOhtCGEj1eLBl
> 6
> > Mk7QhynQSY8%3D&amp;reserved=3D0
> >
> > 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.
>=20
> 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.
>=20
> 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] =3D 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] =3D 0 [  127.492816] [DEBUG4]
> sdw_handle_slave_status IN3 [  127.492818] [DEBUG4]
> sdw_handle_slave_status status[1] =3D 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] =3D
> 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 proper=
ly 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 CLOC=
K STOP.
I was able to get a good result if I add this command in the amp driver sus=
pend function.
The amp driver receives the detachment event and register restoration was d=
one 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.