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

[Pierre-Louis Bossart]

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

This register is already taken care of in drivers/soundwire/intel.c and
cadence_master.c

for pm_runtime suspend, the sequence uses sdw_cdns_clock_stop(), which
will try and prepare devices for clock-stop with a callback, in case any
imp-def registers is required, then it will call sdw_bus_clk_stop()
which does a broadcast write:

sdw_bus_clk_stop(struct sdw_bus *bus)
{
	int ret;

	/*
	 * broadcast clock stop now, attached Slaves will ACK this,
	 * unattached will ignore
	 */
	ret = sdw_bwrite_no_pm(bus, SDW_BROADCAST_DEV_NUM,
			       SDW_SCP_CTRL, SDW_SCP_CTRL_CLK_STP_NOW);
	if (ret < 0) {
		if (ret != -ENODATA)
			dev_err(bus->dev, "ClockStopNow Broadcast msg failed %d\n", ret);
		return ret;
	}

The codec driver is not supposed to set this bit on its own, what this
indicates is that the clock will actually stop at the end of the frame.
Only the master/controller driver can transmit this - there's a very
strong reason why its a bus functionality.

The other point is that on pm_runtime resume, the Intel host will start
a SEVERE_RESET sequence. That's a bit different from the 'traditional'
description of the clock stop due to a power optimization on the Intel
side (see more below), but doing a reset has precedence over any other
configuration that might have happened before the clock stopped so the
amplifier SHALL transition to UNATTACHED on a reset.

Somehow it looks like the amplifiers don't see the clock stopped and
don't see the reset, that's rather surprising.

If this happens for system suspend/resume, then it's a different story:
we don't use the clock stop mode at all, the bus will be completely
reconfigured.

You could try to see if the results change by using the 'traditional'
clock stop mode with a kernel module parameters

option snd-sof-intel-hda-common sdw_clock_stop_quirks=0

the default is SDW_INTEL_CLK_STOP_BUS_RESET

/*
 * Require a bus reset (and complete re-enumeration) when exiting
 * clock stop modes. This may be needed if the controller power was
 * turned off and all context lost. This quirk shall not be used if a
 * Slave device needs to remain enumerated and keep its context,
 * e.g. to provide the reasons for the wake, report acoustic events or
 * pass a history buffer.
 */
#define SDW_INTEL_CLK_STOP_BUS_RESET		BIT(3)

In this case, the bus will not be reset, I wonder if this is the part
that's problematic for the amplifier.

Hope this helps
-Pierre