| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cfacc3d6-2daa-6aa3-ba19-281b7e48bb47@collabora.com>
Date: Fri, 10 Feb 2023 14:39:56 +0000
From: Lucas Tanure <lucas.tanure@...labora.com>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: David Rhodes <david.rhodes@...rus.com>,
Liam Girdwood <lgirdwood@...il.com>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Mark Brown <broonie@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
devicetree@...r.kernel.org, patches@...nsource.cirrus.com,
linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature
On 10-02-2023 13:43, Charles Keepax wrote:
> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>> Shared boost allows two amplifiers to share a single boost circuit by
>> communicating on the MDSYNC bus.
>> The passive amplifier does not control the boost and receives data from
>> the active amplifier.
>>
>> Shared Boost is not supported in HDA Systems.
>> Based on David Rhodes shared boost patches.
>>
>> Signed-off-by: Lucas Tanure <lucas.tanure@...labora.com>
>> ---
>
> Ok I found a copy of David's internal patch which helps a litte.
>
>> --- a/sound/soc/codecs/cs35l41-lib.c
>> +++ b/sound/soc/codecs/cs35l41-lib.c
>> @@ -1114,12 +1114,31 @@ static const struct reg_sequence cs35l41_reset_to_safe[] = {
>> { 0x00000040, 0x00000033 },
>> };
>>
>> +static const struct reg_sequence cs35l41_actv_seq[] = {
>> + /* SYNC_BST_CTL_RX_EN = 0; SYNC_BST_CTL_TX_EN = 1 */
>> + {CS35L41_MDSYNC_EN, 0x00001000},
>
> David's internal patch appears to set 0x3000 on the active side,
> not sure where that difference snuck in, or which is the correct
> value. Your settings appear to make logical sense to me though, TX
> on the active side, RX on the passive side.
Yes, I an e-mail David said that the passive was controlling the boost
and out putting the value to the active one, but in the Documentation
patch he sent the explanation was exactly the opposite.
Quote:
" The passive amplifier does not control the boost and receives data
from the active amplifier."
And as the patch sets TX and RX in the same chip I changed to follow the
documentation.
>
>> + /* BST_CTL_SEL = CLASSH */
>> + {CS35L41_BSTCVRT_VCTRL2, 0x00000001},
>
> BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it
> is called in the datasheet, yay us for using the same names).
> That does not mean this write is wrong, could just be the
> comment, but what this does write is a bit odd so I would like
> David to confirm this isn't some typo in his original patch.
I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is at
0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ).
This write here is to select the boost control source, which for the
active should be "Class H tracking value".
So I still think this is correct.
>
>> +};
>> +
>> +static const struct reg_sequence cs35l41_pass_seq[] = {
>> + /* SYNC_BST_CTL_RX_EN = 1; SYNC_BST_CTL_TX_EN = 0 */
>> + {CS35L41_MDSYNC_EN, 0x00002000},
>> + /* BST_EN = 0 */
>> + {CS35L41_PWR_CTRL2, 0x00003300},
>> + /* BST_CTL_SEL = MDSYNC */
>> + {CS35L41_BSTCVRT_VCTRL2, 0x00000002},
>
> Ditto here, comment doesn't match the write.
Same as above, re-write to be the passive, with RX not TX.
>
>> -int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable)
>> +int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
>> + struct completion *pll_lock)
>> {
>> int ret;
>> + unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3;
>> + struct reg_sequence cs35l41_mdsync_down_seq[] = {
>> + {CS35L41_PWR_CTRL3, 0},
>> + {CS35L41_GPIO_PAD_CONTROL, 0},
>> + {CS35L41_PWR_CTRL1, 0, 3000},
>> + };
>> + struct reg_sequence cs35l41_mdsync_up_seq[] = {
>> + {CS35L41_PWR_CTRL3, 0},
>> + {CS35L41_PWR_CTRL1, 0x00000000, 3000},
>> + {CS35L41_PWR_CTRL1, 0x00000001, 3000},
>> + };
>>
>> switch (b_type) {
>> + case CS35L41_SHD_BOOST_ACTV:
>> + case CS35L41_SHD_BOOST_PASS:
>> + regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
>> + regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);
>> +
>> + pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;
>> + pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;
>
> Are you sure this is what you want? In the case of powering up,
> the sequence would end up being:
>
> mdsync_down
> -> sets GLOBAL_EN on
> mdsync_up
> -> sets GLOBAL_EN off
> -> sets GLOBAL_EN on
>
> Feels like mdsync_down should always turn global_enable off? But
> again I don't know for sure. But then I guess why is there the
> extra write to turn it off in mdsync_up?
For the disable case (DAPM turning everything off) SYNC and Global
enable are off and the code hits
if (!enable)
break;
But for for enable case (DAPM turning everything On) the code continues
enabling SYNC_EN, and turning off Global enable, as requested by
"4.10.1 Multidevice Synchronization Enable" page 70.
But as it is a enable path Global should be enabled again.
I can't see any sign of
> GLOBAL_EN bouncing in David's internal patch.
Yes, but it is required by :
"4.10.1 Multidevice Synchronization Enable" page 70.
Thanks
Lucas
>
>> +
>> + gpio1_func = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
>> + gpio1_func <<= CS35L41_GPIO1_CTRL_SHIFT;
>
> Hm... this is a good point, probably would be nice to return an
> error if the user sets a shared boost mode and a specific function
> for the GPIO1 pin.
>
>> + pad_control &= ~CS35L41_GPIO1_CTRL_MASK;
>> + pad_control |= gpio1_func & CS35L41_GPIO1_CTRL_MASK;
>> +
>> + cs35l41_mdsync_down_seq[0].def = pwr_ctrl3;
>> + cs35l41_mdsync_down_seq[1].def = pad_control;
>> + cs35l41_mdsync_down_seq[2].def = pwr_ctrl1;
>> + ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq,
>> + ARRAY_SIZE(cs35l41_mdsync_down_seq));
>> + if (!enable)
>> + break;
>> +
>> + if (!pll_lock)
>> + return -EINVAL;
>> +
>> + ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
>> + if (ret == 0) {
>> + ret = -ETIMEDOUT;
>> + } else {
>> + regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
>> + pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
>> + cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
>> + ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
>> + ARRAY_SIZE(cs35l41_mdsync_up_seq));
>> + }
>> + break;
>
> Thanks,
> Charles
Powered by blists - more mailing lists