ASoC: soundwire codecs: improve regmap handling

[plbossart]

This PR suggests two improvements to set the regmaps to cache-only on probe and when a device becomes UNATTACHED. This should help with transient errors or delays, but this is not good enough to signal that a card/device is not functional.

Note that rt715 and rt715-sdca did not support the UNATTACHED transition at all, other devices did not only set the hw_init state to false.

@bardliao @RanderWang @shumingfan @oder-chiou @jack-cy-yu @ryans-lee comments welcome.

[bardliao]

@plbossart Currently, we set regcache_cache_only false on resume. Is it possible that the codec is unattached and attached, but no resume occurs? For example, the codec is unattached after resume. Can this corner case happen?

[shumingfan]

@plbossart As Bard mentioned (https://github.com/thesofproject/linux/issues/3924#issuecomment-1280361548), we need to regcache_cache_only=false when the device is attached. One situation: the playback starts immediately after the sound card is registered and the codec driver doesn't runtime suspend yet. We will not hear the sound output.

[plbossart]

Yes absolutely @bardliao the codec driver can resume, and due to some glitch the SoundWire codec might detach-reattach. That case will be handled with two calls to update_status(). What I was trying to do is prevent access to registers while the codec is not attached.

@shumingfan you're right, we need to do the regcache_cache_only=false upon attachment, and the code below is not good enough:

int rt711_io_init(struct device *dev, struct sdw_slave *slave)
{
	struct rt711_priv *rt711 = dev_get_drvdata(dev);

	rt711->disable_irq = false;

	if (rt711->hw_init)
		return 0;

	if (rt711->first_hw_init) {
		regcache_cache_only(rt711->regmap, false);
		regcache_cache_bypass(rt711->regmap, true);
	}

I do not recall why we do this only after the FIRST attachment. This should be done always when a device becomes attached, no?

I also don't understand what we do with this first_hw_init. The main point was to enable pm_runtime, but we also use it for thsi

	if (rt711->first_hw_init)
		rt711_calibration(rt711);
	else
		schedule_work(&rt711->calibration_work);

I don't quite get that sequence.

[shumingfan]

int rt711_io_init(struct device *dev, struct sdw_slave *slave)
{
	struct rt711_priv *rt711 = dev_get_drvdata(dev);

	rt711->disable_irq = false;

	if (rt711->hw_init)
		return 0;

	if (rt711->first_hw_init) {
		regcache_cache_only(rt711->regmap, false);
		regcache_cache_bypass(rt711->regmap, true);
	}

I do not recall why we do this only after the FIRST attachment. This should be done always when a device becomes attached, no?

If the system suspends, it will cut off the power of the codec. The codec driver wants to write the initial settings again directly after the next device attachment. Therefore, the codec driver will set cache_only=false and cache_bypass=true.

	if (rt711->first_hw_init)
		rt711_calibration(rt711);
	else
		schedule_work(&rt711->calibration_work);

The calibration time of rt711 is longer than rt711-sdca. I want to use the work queue to do the calibration process when the sound card registers. However, the playback/recording starts again from the suspended state, I want to wait for the calibration process before the playback/recording.

[plbossart]

If the system suspends, it will cut off the power of the codec. The codec driver wants to write the initial settings again directly after the next device attachment. Therefore, the codec driver will set cache_only=false and cache_bypass=true.

that's right, but 'first_hw_init' is not a good variable to test the 'next device attachment'. It works in the nominal case of suspend-resume, but if the regular does become unattached, we would need to do the same. So I think here we need

if (!hw_init) { cache_only=false cache_bypass=true. }

	if (rt711->first_hw_init)
		rt711_calibration(rt711);
	else
		schedule_work(&rt711->calibration_work);

The calibration time of rt711 is longer than rt711-sdca. I want to use the work queue to do the calibration process when the sound card registers. However, the playback/recording starts again from the suspended state, I want to wait for the calibration process before the playback/recording.

ok so that could stay as is, presumably if the device does a detach-reattach we can do the same as for suspend-resume.

[shumingfan]

that's right, but 'first_hw_init' is not a good variable to test the 'next device attachment'. It works in the nominal case of suspend-resume, but if the regular does become unattached, we would need to do the same. So I think here we need

if (!hw_init) { cache_only=false cache_bypass=true. }

Another reason why using 'first_hw_init' flag is that I want to store the initial settings into the regmap cache. If the runtime suspend-resume, the codec driver will do 'regcache_sync' to restore the settings without a detach-reattach. Is it possible there is this situation? like ClockStopMode0?

[plbossart]

@shumingfan please see the update. The code is pretty much the same except that if we see a transition to UNATTACHED, the regcache will be re-sync'ed. This happens normally only on resume.

I need to introduce helpers to make things simpler.

[greg-intel]

SOFCI TEST

[plbossart]

fascinating new issue in https://sof-ci.01.org/linuxpr/PR3941/build1375/devicetest/?model=CML_RVP_SDW&testcase=verify-kernel-boot-log

[   15.125038] kernel: IO error in rt700_jack_detect_handler, ret -16

Not sure how I managed to add this :-)

[shumingfan]

@plbossart The first time the driver does the 'io_init' function will be completed on the condition regcache_cache_only=true. There is a problem is that the calibration needs the correct sequences to be triggered. Could we use the 'unattached_init' flag to set regcache_cache_only=false at the beginning of the io_init function? e.g.

 static int rt1308_io_init(struct device *dev, struct sdw_slave *slave)
 {
        struct rt1308_sdw_priv *rt1308 = dev_get_drvdata(dev);
        int ret = 0;

        if (rt1308->hw_init)
                return 0;

        if (rt1308->first_hw_init) {
                regcache_cache_only(rt1308->regmap, false);
                regcache_cache_bypass(rt1308->regmap, true);
        }

+       if (rt1308->unattached_init)
+               regcache_cache_only(rt1308->regmap, false);
+

then we don't need the 'regcache_sync' at the bottom of the io_init function.

[plbossart]

@shumingfan please take a look at the update, you're absolutely right that my earlier version was completely wrong for the initial attachment. I am chasing multiple issues but this time I think it should be more consistent.

[shumingfan]

@plbossart Thanks for the update. I just notice that the driver does the regcache_sync twice in the normal case. When starting playback/recording from the suspended state, the regcache_sync function was done at the io_init function the first time, then at the resume function the second time.

[plbossart]

I may stilll need to deal with the 'unattach request' @shumingfan. That's used to force the device to reset, so we need to make the difference between a 'normal' reset during resume and a 'not so normal reset' outside of reset. Thanks for the comments, much appreciated.

[plbossart]

I'll close this as well. We should definitively have all all caches bypassed before the codec is initialized, but it's become too invasive to do this change now, unless @bardliao @shumingfan @jack-cy-yu want to do this with a new PR?