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
