| Message ID | 21a183b5a08cb23b193af78d4b1114cc59419272.1701906455.git.soyer@irl.hu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: hda/tas2781: Add tas2563 support | expand |
On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote: > The amp has 3 level addressing (BOOK, PAGE, REG). > The regcache couldn't handle it. So the books aren't currently used so the driver actually works? > static int tas2781_system_suspend(struct device *dev) > @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev) > return ret; > > /* Shutdown chip before system suspend */ > - regcache_cache_only(tas_priv->regmap, false); > tasdevice_tuning_switch(tas_priv, 1); > - regcache_cache_only(tas_priv->regmap, true); > - regcache_mark_dirty(tas_priv->regmap); > > /* > * Reset GPIO may be shared, so cannot reset here. How can this work over system suspend? This just removes the cache with no replacement so if the device looses power over suspend (which seems likely) then all the register state will be lost. A similar issue may potentially exist over runtime suspend on an ACPI system with sufficiently heavily optimised power management.
On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote: > On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote: > > > The amp has 3 level addressing (BOOK, PAGE, REG). > > The regcache couldn't handle it. > > So the books aren't currently used so the driver actually works? > It writes to the book 0 and 8c. The initialization works with regcache, because it writes also the i2c devices. > > static int tas2781_system_suspend(struct device *dev) > > @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev) > > return ret; > > > > /* Shutdown chip before system suspend */ > > - regcache_cache_only(tas_priv->regmap, false); > > tasdevice_tuning_switch(tas_priv, 1); > > - regcache_cache_only(tas_priv->regmap, true); > > - regcache_mark_dirty(tas_priv->regmap); > > > > /* > > * Reset GPIO may be shared, so cannot reset here. > > How can this work over system suspend? This just removes the cache with > no replacement so if the device looses power over suspend (which seems > likely) then all the register state will be lost. A similar issue may > potentially exist over runtime suspend on an ACPI system with > sufficiently heavily optimised power management. In runtime_resume, only one of the two amplifiers goes back. The runtime_suspend sets the current book/prog/conf to -1 on all devices, and tas2781_hda_playback_hook will restore the program/configuration/profile with tasdevice_tuning_switch. And only one, because tasdevice_change_chn_book directly changes the address of i2c_client, so the unlucky one gets invalid values in its actual book from regcache_sync. system_restore doesn't work at all, because regcache_cache_only stays true since system_suspend. It works without the regcache functions.
On Thu, Dec 07, 2023 at 09:19:34PM +0100, Gergo Koteles wrote: > On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote: > > On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote: > > > The amp has 3 level addressing (BOOK, PAGE, REG). > > > The regcache couldn't handle it. > > So the books aren't currently used so the driver actually works? > It writes to the book 0 and 8c. The initialization works with regcache, > because it writes also the i2c devices. I can't see any references to 0x8c in the driver? > > > static int tas2781_system_suspend(struct device *dev) > > > @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev) > > > return ret; > > > > > > /* Shutdown chip before system suspend */ > > > - regcache_cache_only(tas_priv->regmap, false); > > > tasdevice_tuning_switch(tas_priv, 1); > > > - regcache_cache_only(tas_priv->regmap, true); > > > - regcache_mark_dirty(tas_priv->regmap); > > How can this work over system suspend? This just removes the cache with > > no replacement so if the device looses power over suspend (which seems > > likely) then all the register state will be lost. A similar issue may > > potentially exist over runtime suspend on an ACPI system with > > sufficiently heavily optimised power management. > In runtime_resume, only one of the two amplifiers goes back. > The runtime_suspend sets the current book/prog/conf to -1 on all > devices, and tas2781_hda_playback_hook will restore the > program/configuration/profile with tasdevice_tuning_switch. What does "go back" mean? > And only one, because tasdevice_change_chn_book directly changes the > address of i2c_client, so the unlucky one gets invalid values in its > actual book from regcache_sync. The code creates the impression that writing to one tas2781 writes to all of them, is that not the case? > system_restore doesn't work at all, because regcache_cache_only stays > true since system_suspend. Presumably the next runtime resume would make the device writable again? > It works without the regcache functions. How would the devices get their configuration restored? This sounds very much like a case of something working for your specific system in your specific test through some external factor rather than working by design, whatever problems might exist it seems fairly obvious to inspection that this patch would make things worse for other systems. At a minimum this patch needs a much clearer changelog (all the patches I looked at could use clearer changelogs) which explains what's going on here, I would really expect to see something that replaces the use of the cache sync to restore the device state for example.
On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > On Thu, Dec 07, 2023 at 09:19:34PM +0100, Gergo Koteles wrote: > > On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote: > > > On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote: > > > > > The amp has 3 level addressing (BOOK, PAGE, REG). > > > > The regcache couldn't handle it. > > > > So the books aren't currently used so the driver actually works? > > > It writes to the book 0 and 8c. The initialization works with regcache, > > because it writes also the i2c devices. > > I can't see any references to 0x8c in the driver? The firmware has different programs. The programs have blocks, that the driver writes to the amplifier. The address comes from the blocks. > > > > > static int tas2781_system_suspend(struct device *dev) > > > > @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev) > > > > return ret; > > > > > > > > /* Shutdown chip before system suspend */ > > > > - regcache_cache_only(tas_priv->regmap, false); > > > > tasdevice_tuning_switch(tas_priv, 1); > > > > - regcache_cache_only(tas_priv->regmap, true); > > > > - regcache_mark_dirty(tas_priv->regmap); > > > > How can this work over system suspend? This just removes the cache with > > > no replacement so if the device looses power over suspend (which seems > > > likely) then all the register state will be lost. A similar issue may > > > potentially exist over runtime suspend on an ACPI system with > > > sufficiently heavily optimised power management. > > > In runtime_resume, only one of the two amplifiers goes back. > > The runtime_suspend sets the current book/prog/conf to -1 on all > > devices, and tas2781_hda_playback_hook will restore the > > program/configuration/profile with tasdevice_tuning_switch. > > What does "go back" mean? Sorry for imprecise wording. The speaker is silent, I didn't checked why, maybe the amp is in error state or something. > > > And only one, because tasdevice_change_chn_book directly changes the > > address of i2c_client, so the unlucky one gets invalid values in its > > actual book from regcache_sync. > > The code creates the impression that writing to one tas2781 writes to > all of them, is that not the case? > Yes, the tasdevice_* functions, but the regcache_sync doesn't know this. > > system_restore doesn't work at all, because regcache_cache_only stays > > true since system_suspend. > > Presumably the next runtime resume would make the device writable again? > Yes, then one of the speakers works. > > It works without the regcache functions. > > How would the devices get their configuration restored? > tasdevice_tuning_switch calls tasdevice_select_tuningprm_cfg which checks whether the devices needs a new program or configuration. the runtime_suspend and system resume set the devices cur_prog, cur_conf to -1. for (i = 0; i < tas_hda->priv->ndev; i++) { tas_hda->priv->tasdevice[i].cur_book = -1; tas_hda->priv->tasdevice[i].cur_prog = -1; tas_hda->priv->tasdevice[i].cur_conf = -1; } And the tasdevice_select_tuningprm_cfg checks with if (tas_priv->tasdevice[i].cur_prog != prm_no ... If needed, it writes the new program/configuration to the device. The tas2781_hda_playback_hook calls the tasdevice_tuning_switch > This sounds very much like a case of something working for your specific > system in your specific test through some external factor rather than > working by design, whatever problems might exist it seems fairly obvious > to inspection that this patch would make things worse for other systems. > > At a minimum this patch needs a much clearer changelog (all the patches > I looked at could use clearer changelogs) which explains what's going on > here, I would really expect to see something that replaces the use of > the cache sync to restore the device state for example.
On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote: > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > > > And only one, because tasdevice_change_chn_book directly changes the > > > address of i2c_client, so the unlucky one gets invalid values in its > > > actual book from regcache_sync. > > The code creates the impression that writing to one tas2781 writes to > > all of them, is that not the case? > Yes, the tasdevice_* functions, but the regcache_sync doesn't know > this. So this syncing is done in software not hardware? My understanding was that this was a hardware thing. > > How would the devices get their configuration restored? > tasdevice_tuning_switch calls tasdevice_select_tuningprm_cfg which > checks whether the devices needs a new program or configuration. > the runtime_suspend and system resume set the devices cur_prog, > cur_conf to -1. ... > The tas2781_hda_playback_hook calls the tasdevice_tuning_switch And there are no registers other than these programs?
On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote: > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote: > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > > > > > And only one, because tasdevice_change_chn_book directly changes the > > > > address of i2c_client, so the unlucky one gets invalid values in its > > > > actual book from regcache_sync. > > > > The code creates the impression that writing to one tas2781 writes to > > > all of them, is that not the case? > > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know > > this. > > So this syncing is done in software not hardware? My understanding was > that this was a hardware thing. If you mean that the amplifier does not know that there are several programs or configurations or profiles, but only runs the current one, yes. > > > > How would the devices get their configuration restored? > > > tasdevice_tuning_switch calls tasdevice_select_tuningprm_cfg which > > checks whether the devices needs a new program or configuration. > > > the runtime_suspend and system resume set the devices cur_prog, > > cur_conf to -1. > > ... > > > The tas2781_hda_playback_hook calls the tasdevice_tuning_switch > > And there are no registers other than these programs? The tas2781-hda writes 4 things: 1. Profiles from RCA file eg. INT8866RCA2.bin has 4 profile: Music degree 0 calibration voice call earpiece spk2 bypass The profiles contain pre-power-up and pre-shutdown register+value sequences for each amplifier. 2. Programs from DSP firmware. eg. TAS2XXX3870.bin has 1 program: Tuning Mode 3. Configurations from the DSP firmware. eg. TAS2XXX3870.bin has 2 configurations: configuration_Normal_Tuning Mode_48 KHz_s2_0 calibration_Tuning Mode_48 KHz_s2_0 Programs and configurations contain blocks with addresses where they should be written. 4. Calibration data from EFI variables. R0, INV_R0, R0LOW, POWER, TLIM, Based on the chip, they should be written to 5 registers. The code restores all of these in playback_hook, runtime_resume, system_resume functions without regmap_cache_sync.
On Fri, Dec 15, 2023 at 02:17:01AM +0100, Gergo Koteles wrote: > On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote: > > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote: > > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > > > > The code creates the impression that writing to one tas2781 writes to > > > > all of them, is that not the case? > > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know > > > this. > > So this syncing is done in software not hardware? My understanding was > > that this was a hardware thing. > If you mean that the amplifier does not know that there are several > programs or configurations or profiles, but only runs the current one, > yes. No, I mean that the amplifiers don't talk to each other at a hardware level and the grouping is all in software.
On Fri, 2023-12-15 at 12:55 +0000, Mark Brown wrote: > On Fri, Dec 15, 2023 at 02:17:01AM +0100, Gergo Koteles wrote: > > On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote: > > > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote: > > > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > > > > > > The code creates the impression that writing to one tas2781 writes to > > > > > all of them, is that not the case? > > > > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know > > > > this. > > > > So this syncing is done in software not hardware? My understanding was > > > that this was a hardware thing. > > > If you mean that the amplifier does not know that there are several > > programs or configurations or profiles, but only runs the current one, > > yes. > > No, I mean that the amplifiers don't talk to each other at a hardware > level and the grouping is all in software. No, they don't talk to each other. But they have a global i2c address to speed up configuration, but the module doesn't use it yet.
On Fri, Dec 15, 2023 at 03:42:43PM +0100, Gergo Koteles wrote: > On Fri, 2023-12-15 at 12:55 +0000, Mark Brown wrote: > > No, I mean that the amplifiers don't talk to each other at a hardware > > level and the grouping is all in software. > No, they don't talk to each other. But they have a global i2c address > to speed up configuration, but the module doesn't use it yet. That's hardware level synchronisation between the devices, that makes all this a bit less horrifying though it seems like a lot of the issues would go away if the broadcast write address were actually being used more.
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 077a01521eef..d272d3d08b29 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -717,8 +717,6 @@ static int tas2781_runtime_suspend(struct device *dev) tas_priv->tasdevice[i].cur_conf = -1; } - regcache_cache_only(tas_priv->regmap, true); - regcache_mark_dirty(tas_priv->regmap); mutex_unlock(&tas_priv->codec_lock); @@ -730,20 +728,11 @@ static int tas2781_runtime_resume(struct device *dev) struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); unsigned long calib_data_sz = tas_priv->ndev * TASDEVICE_SPEAKER_CALIBRATION_SIZE; - int ret; dev_dbg(tas_priv->dev, "Runtime Resume\n"); mutex_lock(&tas_priv->codec_lock); - regcache_cache_only(tas_priv->regmap, false); - ret = regcache_sync(tas_priv->regmap); - if (ret) { - dev_err(tas_priv->dev, - "Failed to restore register cache: %d\n", ret); - goto out; - } - tasdevice_prmg_load(tas_priv, tas_priv->cur_prog); /* If calibrated data occurs error, dsp will still works with default @@ -752,10 +741,9 @@ static int tas2781_runtime_resume(struct device *dev) if (tas_priv->cali_data.total_sz > calib_data_sz) tas2781_apply_calib(tas_priv); -out: mutex_unlock(&tas_priv->codec_lock); - return ret; + return 0; } static int tas2781_system_suspend(struct device *dev) @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev) return ret; /* Shutdown chip before system suspend */ - regcache_cache_only(tas_priv->regmap, false); tasdevice_tuning_switch(tas_priv, 1); - regcache_cache_only(tas_priv->regmap, true); - regcache_mark_dirty(tas_priv->regmap); /* * Reset GPIO may be shared, so cannot reset here. diff --git a/sound/soc/codecs/tas2781-comlib.c b/sound/soc/codecs/tas2781-comlib.c index ffb26e4a7e2f..933cd008e9f5 100644 --- a/sound/soc/codecs/tas2781-comlib.c +++ b/sound/soc/codecs/tas2781-comlib.c @@ -39,7 +39,7 @@ static const struct regmap_range_cfg tasdevice_ranges[] = { static const struct regmap_config tasdevice_regmap = { .reg_bits = 8, .val_bits = 8, - .cache_type = REGCACHE_RBTREE, + .cache_type = REGCACHE_NONE, .ranges = tasdevice_ranges, .num_ranges = ARRAY_SIZE(tasdevice_ranges), .max_register = 256 * 128,
The amp has 3 level addressing (BOOK, PAGE, REG). The regcache couldn't handle it. Signed-off-by: Gergo Koteles <soyer@irl.hu> --- sound/pci/hda/tas2781_hda_i2c.c | 17 +---------------- sound/soc/codecs/tas2781-comlib.c | 2 +- 2 files changed, 2 insertions(+), 17 deletions(-)