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@collabora.com>
---
 include/sound/cs35l41.h        | 13 +++++-
 sound/pci/hda/cs35l41_hda.c    |  6 +--
 sound/soc/codecs/cs35l41-lib.c | 73 +++++++++++++++++++++++++++++++++-
 sound/soc/codecs/cs35l41.c     | 27 ++++++++++++-
 sound/soc/codecs/cs35l41.h     |  1 +
 5 files changed, 113 insertions(+), 7 deletions(-)

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 9ac5918269a5..7239d943942c 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -11,6 +11,7 @@
 #define __CS35L41_H
 
 #include <linux/regmap.h>
+#include <linux/completion.h>
 #include <linux/firmware/cirrus/cs_dsp.h>
 
 #define CS35L41_FIRSTREG		0x00000000
@@ -677,6 +678,7 @@
 
 #define CS35L36_PUP_DONE_IRQ_UNMASK	0x5F
 #define CS35L36_PUP_DONE_IRQ_MASK	0xBF
+#define CS35L41_SYNC_EN_MASK		BIT(8)
 
 #define CS35L41_AMP_SHORT_ERR		0x80000000
 #define CS35L41_BST_SHORT_ERR		0x0100
@@ -686,6 +688,7 @@
 #define CS35L41_BST_DCM_UVP_ERR		0x80
 #define CS35L41_OTP_BOOT_DONE		0x02
 #define CS35L41_PLL_UNLOCK		0x10
+#define CS35L41_PLL_LOCK		BIT(1)
 #define CS35L41_OTP_BOOT_ERR		0x80000000
 
 #define CS35L41_AMP_SHORT_ERR_RLS	0x02
@@ -705,6 +708,8 @@
 #define CS35L41_INT1_MASK_DEFAULT	0x7FFCFE3F
 #define CS35L41_INT1_UNMASK_PUP		0xFEFFFFFF
 #define CS35L41_INT1_UNMASK_PDN		0xFF7FFFFF
+#define CS35L41_INT3_PLL_LOCK_SHIFT	1
+#define CS35L41_INT3_PLL_LOCK_MASK	BIT(CS35L41_INT3_PLL_LOCK_SHIFT)
 
 #define CS35L41_GPIO_DIR_MASK		0x80000000
 #define CS35L41_GPIO_DIR_SHIFT		31
@@ -742,6 +747,11 @@
 enum cs35l41_boost_type {
 	CS35L41_INT_BOOST,
 	CS35L41_EXT_BOOST,
+	CS35L41_SHD_BOOST_ACTV,
+	CS35L41_SHD_BOOST_PASS,
+
+	// Not present in Binding Documentation, so no system should use this value.
+	// This value is only used in CLSA0100 Laptop
 	CS35L41_EXT_BOOST_NO_VSPK_SWITCH,
 };
 
@@ -891,6 +901,7 @@ int cs35l41_exit_hibernate(struct device *dev, struct regmap *regmap);
 int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
 		       struct cs35l41_hw_cfg *hw_cfg);
 bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type);
-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);
 
 #endif /* __CS35L41_H */
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f7815ee24f83..38c0079ef303 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -515,13 +515,13 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action)
 		break;
 	case HDA_GEN_PCM_ACT_PREPARE:
 		mutex_lock(&cs35l41->fw_mutex);
-		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 1);
+		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 1, NULL);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLEANUP:
 		mutex_lock(&cs35l41->fw_mutex);
 		regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute));
-		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 0);
+		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 0, NULL);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLOSE:
@@ -673,7 +673,7 @@ static int cs35l41_runtime_suspend(struct device *dev)
 	if (cs35l41->playback_started) {
 		regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute,
 				       ARRAY_SIZE(cs35l41_hda_mute));
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0);
+		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0, NULL);
 		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
 				   CS35L41_AMP_EN_MASK, 0 << CS35L41_AMP_EN_SHIFT);
 		if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index 04be71435491..4d6417ce70bb 100644
--- 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},
+	/* BST_CTL_SEL = CLASSH */
+	{CS35L41_BSTCVRT_VCTRL2,    0x00000001},
+};
+
+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},
+};
+
 int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
 		       struct cs35l41_hw_cfg *hw_cfg)
 {
 	int ret;
 
 	switch (hw_cfg->bst_type) {
+	case CS35L41_SHD_BOOST_ACTV:
+		regmap_multi_reg_write(regmap, cs35l41_actv_seq, ARRAY_SIZE(cs35l41_actv_seq));
+		fallthrough;
 	case CS35L41_INT_BOOST:
 		ret = cs35l41_boost_config(dev, regmap, hw_cfg->bst_ind,
 					   hw_cfg->bst_cap, hw_cfg->bst_ipk);
@@ -1138,6 +1157,10 @@ int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
 		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL2, CS35L41_BST_EN_MASK,
 					 CS35L41_BST_DIS_FET_OFF << CS35L41_BST_EN_SHIFT);
 		break;
+	case CS35L41_SHD_BOOST_PASS:
+		ret = regmap_multi_reg_write(regmap, cs35l41_pass_seq,
+					     ARRAY_SIZE(cs35l41_pass_seq));
+		break;
 	default:
 		dev_err(dev, "Boost type %d not supported\n", hw_cfg->bst_type);
 		ret = -EINVAL;
@@ -1165,11 +1188,59 @@ bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type)
 }
 EXPORT_SYMBOL_GPL(cs35l41_safe_reset);
 
-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;
+
+		gpio1_func = enable ? CS35L41_GPIO1_MDSYNC : CS35L41_GPIO1_HIZ;
+		gpio1_func <<= CS35L41_GPIO1_CTRL_SHIFT;
+
+		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;
 	case CS35L41_INT_BOOST:
 		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
 					 enable << CS35L41_GLOBAL_EN_SHIFT);
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index c006364e5335..1624510d09c0 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -360,6 +360,7 @@ static void cs35l41_boost_enable(struct cs35l41_private *cs35l41, unsigned int e
 {
 	switch (cs35l41->hw_cfg.bst_type) {
 	case CS35L41_INT_BOOST:
+	case CS35L41_SHD_BOOST_ACTV:
 		enable = enable ? CS35L41_BST_EN_DEFAULT : CS35L41_BST_DIS_FET_OFF;
 		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2, CS35L41_BST_EN_MASK,
 				enable << CS35L41_BST_EN_SHIFT);
@@ -455,6 +456,12 @@ static irqreturn_t cs35l41_irq(int irq, void *data)
 		ret = IRQ_HANDLED;
 	}
 
+	if (status[2] & CS35L41_PLL_LOCK) {
+		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS3, CS35L41_PLL_LOCK);
+		complete(&cs35l41->pll_lock);
+		ret = IRQ_HANDLED;
+	}
+
 done:
 	pm_runtime_mark_last_busy(cs35l41->dev);
 	pm_runtime_put_autosuspend(cs35l41->dev);
@@ -492,10 +499,12 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
 						cs35l41_pup_patch,
 						ARRAY_SIZE(cs35l41_pup_patch));
 
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 1);
+		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 1,
+				      &cs35l41->pll_lock);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0);
+		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0,
+				      &cs35l41->pll_lock);
 
 		ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
 					       val, val &  CS35L41_PDN_DONE_MASK,
@@ -802,6 +811,10 @@ static const struct snd_pcm_hw_constraint_list cs35l41_constraints = {
 static int cs35l41_pcm_startup(struct snd_pcm_substream *substream,
 			       struct snd_soc_dai *dai)
 {
+	struct cs35l41_private *cs35l41 = snd_soc_component_get_drvdata(dai->component);
+
+	reinit_completion(&cs35l41->pll_lock);
+
 	if (substream->runtime)
 		return snd_pcm_hw_constraint_list(substream->runtime, 0,
 						  SNDRV_PCM_HW_PARAM_RATE,
@@ -1252,6 +1265,10 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 	/* Set interrupt masks for critical errors */
 	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1,
 		     CS35L41_INT1_MASK_DEFAULT);
+	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
+	    cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_ACTV)
+		regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK3, CS35L41_INT3_PLL_LOCK_MASK,
+				   0 << CS35L41_INT3_PLL_LOCK_SHIFT);
 
 	ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL, cs35l41_irq,
 					IRQF_ONESHOT | IRQF_SHARED | irq_pol,
@@ -1275,6 +1292,8 @@ int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *
 	if (ret < 0)
 		goto err;
 
+	init_completion(&cs35l41->pll_lock);
+
 	pm_runtime_set_autosuspend_delay(cs35l41->dev, 3000);
 	pm_runtime_use_autosuspend(cs35l41->dev);
 	pm_runtime_mark_last_busy(cs35l41->dev);
@@ -1317,6 +1336,10 @@ void cs35l41_remove(struct cs35l41_private *cs35l41)
 	pm_runtime_disable(cs35l41->dev);
 
 	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1, 0xFFFFFFFF);
+	if (cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_PASS ||
+	    cs35l41->hw_cfg.bst_type == CS35L41_SHD_BOOST_ACTV)
+		regmap_update_bits(cs35l41->regmap, CS35L41_IRQ1_MASK3, CS35L41_INT3_PLL_LOCK_MASK,
+				   1 << CS35L41_INT3_PLL_LOCK_SHIFT);
 	kfree(cs35l41->dsp.system_name);
 	wm_adsp2_remove(&cs35l41->dsp);
 	cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type);
diff --git a/sound/soc/codecs/cs35l41.h b/sound/soc/codecs/cs35l41.h
index c85cbc1dd333..34d967d4372b 100644
--- a/sound/soc/codecs/cs35l41.h
+++ b/sound/soc/codecs/cs35l41.h
@@ -33,6 +33,7 @@ struct cs35l41_private {
 	int irq;
 	/* GPIO for /RST */
 	struct gpio_desc *reset_gpio;
+	struct completion pll_lock;
 };
 
 int cs35l41_probe(struct cs35l41_private *cs35l41, const struct cs35l41_hw_cfg *hw_cfg);
-- 
2.39.1

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

> +	/* 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.

> +};
> +
> +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.

> -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? I can't see any sign of
GLOBAL_EN bouncing in David's internal patch.

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

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

On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
> On 10-02-2023 13:43, Charles Keepax wrote:
> >On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> >>+	{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.
> And as the patch sets TX and RX in the same chip I changed to follow
> the documentation.

Yeah I mean I suspect this is sensible, unless there is some
reason the controller side also needs to have RX enabled. Perhaps
for feedback or something from the passive side, but I imagine
this is just a typo in the original patch.

> >>+	/* 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.

Yeah this one is a mistake on my part, I was reviewing some
patches on another amp just before I think I have looked at the
wrong datasheet here. You are correct those bits are infact
BST_CTL_SEL. So ignore this one.

> >>+		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;

Yes, so the disable flow makes perfect sense here it is the
enable flow that seemed odd.

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

Hmm... yes that does appear to suggest bouncing the global
enable. Kinda weird, I can't help but wonder if the turning
global enable off is actually needed, but I guess it does say
that so probably safest. It is also rather unclear on who that
sequence should be performed on it says:

"When powering up a second (and each subsequent) CS35L41B onto a
shared MDSYNC bus, the following protocol must
be followed"

But very unclear if that sequence should be followed on only the
new device, the master device, or on all devices. I will try to
find some time to chase some apps guys next week see if anyone
has any ideas.

Thanks,
Charles

On 11-02-2023 17:06, Charles Keepax wrote:
> On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
>> On 10-02-2023 13:43, Charles Keepax wrote:
>>> On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
>>>> +	{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.
>> And as the patch sets TX and RX in the same chip I changed to follow
>> the documentation.
> 
> Yeah I mean I suspect this is sensible, unless there is some
> reason the controller side also needs to have RX enabled. Perhaps
> for feedback or something from the passive side, but I imagine
> this is just a typo in the original patch.

Ok, but the other side doesn't have both RX and TX enabled.
If the active side needed RX to receive information for the other side, 
the passive one would need TX enabled too.
So if a feedback is necessary, both channels on both sides would be 
enabled, not one channel in one side and both on the other.


> 
>>>> +	/* 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.
> 
> Yeah this one is a mistake on my part, I was reviewing some
> patches on another amp just before I think I have looked at the
> wrong datasheet here. You are correct those bits are infact
> BST_CTL_SEL. So ignore this one.
> 
>>>> +		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;
> 
> Yes, so the disable flow makes perfect sense here it is the
> enable flow that seemed odd.
> 
>> 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.
> 
> Hmm... yes that does appear to suggest bouncing the global
> enable. Kinda weird, I can't help but wonder if the turning
> global enable off is actually needed, but I guess it does say
> that so probably safest. It is also rather unclear on who that
> sequence should be performed on it says:
> 
> "When powering up a second (and each subsequent) CS35L41B onto a
> shared MDSYNC bus, the following protocol must
> be followed"
> 
> But very unclear if that sequence should be followed on only the
> new device, the master device, or on all devices. I will try to
> find some time to chase some apps guys next week see if anyone
> has any ideas.
I had long talks with apps guys on this when I was at Cirrus.
And my understanding is:
A new CS35L41 can misunderstand the information on MDSYNC bus if a 
communication is already in place, between another pair of CS35L41, so 
to avoid that, any CS35L41 being turn on in a already active MDSYNC bus, 
must execute those steps.

We could move the active amp up in DAPM graph so its enabled before all 
passive ones, but we would still need to execute that for all passive 
amps. So there is not much point in that.

Here I can see that if I enable SYNC_EN during probe without clocks the 
device becomes unresponsive, at least with the current code.
So following the datasheet and enabling SYNC_EN only after clocks seems 
to resolve Steam decks issue.

Questions I never got an answer from APPS guys:

- Can we enable SYNC_EN during probe if we know there is no playback
happening, no clocks and Global enable off? Steam decks seem to answer 
no here. If yes, why having pm_runtime features makes the device become 
unresponsive?

- Can we leave SYNC_EN enabled during Global enable off and no clocks?

- If SYNC_EN is enabled and we only set Global enable on after the PLL 
lock happened, do we still need to execute those steps? I mean, if the 
driver only deals with Global enable and leaves shared boost configured 
since boost, will MDSYNC bus work?

Thanks
Lucas

> 
> Thanks,
> Charles

On Sun, Feb 12, 2023 at 09:28:39AM +0000, Lucas Tanure wrote:
> On 11-02-2023 17:06, Charles Keepax wrote:
> >On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
> >>On 10-02-2023 13:43, Charles Keepax wrote:
> >>>On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
> Ok, but the other side doesn't have both RX and TX enabled.
> If the active side needed RX to receive information for the other
> side, the passive one would need TX enabled too.
> So if a feedback is necessary, both channels on both sides would be
> enabled, not one channel in one side and both on the other.

A very good point :-)

> >"When powering up a second (and each subsequent) CS35L41B onto a
> >shared MDSYNC bus, the following protocol must
> >be followed"
> >
> >But very unclear if that sequence should be followed on only the
> >new device, the master device, or on all devices. I will try to
> >find some time to chase some apps guys next week see if anyone
> >has any ideas.
> I had long talks with apps guys on this when I was at Cirrus.
> And my understanding is:
> A new CS35L41 can misunderstand the information on MDSYNC bus if a
> communication is already in place, between another pair of CS35L41,
> so to avoid that, any CS35L41 being turn on in a already active
> MDSYNC bus, must execute those steps.

Ok so that implies we are ok, since that suggests we are
saying that only the new amp to the bus needs to execute the
sequence, not the amps already on the bus.

> We could move the active amp up in DAPM graph so its enabled before
> all passive ones, but we would still need to execute that for all
> passive amps. So there is not much point in that.

Agree, fine as is.

> 
> Here I can see that if I enable SYNC_EN during probe without clocks
> the device becomes unresponsive, at least with the current code.
> So following the datasheet and enabling SYNC_EN only after clocks
> seems to resolve Steam decks issue.
> 
> Questions I never got an answer from APPS guys:
> 
> - Can we enable SYNC_EN during probe if we know there is no playback
> happening, no clocks and Global enable off? Steam decks seem to
> answer no here. If yes, why having pm_runtime features makes the
> device become unresponsive?
> 
> - Can we leave SYNC_EN enabled during Global enable off and no clocks?

These two I think are not too much of a concern, turning sync on as
part of powering up the amps doesn't seem to be a big concern,
its not a lot of writes.

> - If SYNC_EN is enabled and we only set Global enable on after the
> PLL lock happened, do we still need to execute those steps? I mean,
> if the driver only deals with Global enable and leaves shared boost
> configured since boost, will MDSYNC bus work?

Yeah I think here it is also probably safer to just do it anyway.

I would still like David to do a quick review, unfortunately he
is off at the moment but should be back Monday next week. But
from my side I think this is probably all good:

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles