Re: Nokia N900 - audio TPA6130A2 problems

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6113 bytes --]

Hi,

On Mon, Mar 21, 2016 at 01:04:18AM +0100, Sebastian Reichel wrote:
> On Sun, Mar 20, 2016 at 09:43:11PM +0200, Ivaylo Dimitrov wrote:
> > On 20.03.2016 07:17, Sebastian Reichel wrote:
> > >On Sat, Mar 19, 2016 at 10:49:57AM +0200, Ivaylo Dimitrov wrote:
> > >>On 18.03.2016 17:04, Sebastian Reichel wrote:
> > >>>On Fri, Mar 18, 2016 at 03:45:26PM +0200, Ivaylo Dimitrov wrote:
> > >>>>On 18.03.2016 15:36, Sebastian Reichel wrote:
> > >>>>Regulator is V28_A, which is always-on, so it is enabled no matter what
> > >>>>probe does. Anyway, I added a various delays after regulator_enable(), to no
> > >>>>success.
> > >>
> > >>I guess we're getting closer - I put some printks in various functions in
> > >>the twl-regulator.c, here is the result:
> > >>
> > >>on power-up:
> > >>
> > >>[    2.378601] twl4030ldo_get_voltage_sel VMMC2 vsel 0x00000008
> > >>[    2.384948] twl4030reg_enable VMMC2 grp 0x00000020
> > >>[    2.408416] twl4030ldo_get_voltage_sel VMMC2 vsel 0x00000008
> > >>[    7.196685] twl4030reg_is_enabled VMMC2 state 0x0000002e
> > >>[    7.202819] twl4030reg_is_enabled VMMC2 state 0x0000002e
> > >>[    7.209777] twl4030reg_is_enabled VMMC2 state 0x0000002e
> > >>[    7.215728] twl4030reg_is_enabled VMMC2 state 0x0000002e
> > >>[    7.223205] twl4030reg_is_enabled VMMC2 state 0x0000002e
> > >
> > >Ok, so normal power up results in running VMMC2 (always-on works),
> > >but voltage is not configured correctly. 2.6V is default according
> > >to the TRM. I think this is a "bug" in the regulator framework. It
> > >should setup the minimum allowed voltage before enabling the
> > >always-on regulator.
> > >
> > 
> > /sys/kernel/debug/regulator/regulator_summary shows 2850mV for V28_A, so I
> > would remove the quotes. Also, always-on is because if V28_A regulator is
> > turned off, there is a leakage through tlv320aic34 VIO. BTW one of the
> > things I did while trying to find the problem, was to remove that always-on
> > property from the DTS - it didn't help.
> 
> Right thinking about it, the voltage must also be configured for the
> non always-on cases. So it's not a problem with the regulator
> framework, but with twl-regulator's probe function, that should take
> care of this.
> 
> > >In case of the tpa6130a2/tpa6140a2 driver it may also be nice to add
> > >something like this to the driver (Vdd may be between 2.5V and 5.5V
> > >according to both datasheets):
> > >
> > >if (regulator_can_change_voltage(data->supply))
> > >     regulator_set_voltage(data->supply, 2500000, 5500000);
> > >
> > 
> > and add DT property for that voltage range, as max output power and
> > harmonics depend on the supply voltage.
> 
> I guess that's 2nd step.
> 
> > >>after restart from stock kernel:
> > >>
> > >>[    2.388610] twl4030ldo_get_voltage_sel VMMC2 vsel 0x0000000a
> > >>[    2.394958] twl4030reg_enable VMMC2 grp 0x00000028
> > >
> > >I had a quick glance at this. I think stock kernel put VMMC2
> > >into sleep mode. Mainline kernel does not expect sleep mode
> > >being set and does not disable it.
> > >
> > 
> > Well, one would think that kernel should not have expectations on what would
> > be the state of the hardware by the time it takes control over it, but setup
> > everything needed instead.
> 
> I thought it's obvious, that this is not the desired behaviour :)
> 
> > >>[    2.418426] twl4030ldo_get_voltage_sel VMMC2 vsel 0x0000000a
> > >>[    7.186645] twl4030reg_is_enabled VMMC2 state 0x00000020
> > >>[    7.192718] twl4030reg_is_enabled VMMC2 state 0x00000020
> > >>[    7.199615] twl4030reg_is_enabled VMMC2 state 0x00000020
> > >>[    7.205535] twl4030reg_is_enabled VMMC2 state 0x00000020
> > >>[    7.212951] twl4030reg_is_enabled VMMC2 state 0x00000020
> > >>
> > >>I don't see twl4030ldo_set_voltage_sel() for VMMC2(V28_A) regulator, though
> > >>there are calls for VMMC1 and VAUX3.
> > >
> > >I guess that's because the voltage is only configured if at least
> > >one regulator consumer requests anything specific.
> > >
> > 
> > But then the board DTS is simply ignored. Doesn't look good :)
> >
> > >>So, it seems to me that V28_A is not enabled or correctly set-up
> > >>and all devices connected to it does not function. And it looks
> > >>like even after power-on VMMC2 is not correctly set-up - it is
> > >>supposed to have voltage of 2.85V (10) but kernel leaves it to
> > >>2.60V (8). However my twl-fu ends here so any help is appreciated.
> > >
> > >So in case of reboot from stock kernel voltage is already configured
> > >to 2.8V, but it does not work, because of the sleep mode.
> > >
> > 
> > Yeah, that sleep is pretty clear, I was rather asking - "any idea how to fix
> > that?". Or it is someone else expected to fix it?
> 
> You may have noticed, that I included Mark and Liam. I hope they
> can give some feedback. I think there are two bugs:
> 
> 1. twl_probe() should setup a default voltage based on DT
>    information.

I just had a look at the regulator core code. I think the voltage
should be set automatically during regulator_register():

regulator_register()
-> set_machine_constraints()
--> machine_constraints_voltage()
---> _regulator_do_set_voltage()
----> _regulator_call_set_voltage()
-----> ops->set_voltage()

Looks like this currently only works automatically, if DT specifies
min-voltage = max-voltage. Adding this to twl-regulator's probe
function before devm_regulator_register()) should enable the voltage
setting:

init_data->constraints.apply_uV = 1;

I hope Mark can tell us why this only done, when the voltage is fixed.

> 2. if regulator is in sleep mode, regulator enable should
>    disable sleep mode.

Stroke that. It should be disabled in probe of course, since
it can be modified later using regulator_set_mode(). Actually
it is already supported by adding this to the omap3-n900.dts:

&vmmc2 {
    regulator-initial-mode = <2>;
};

Ivo, Can you try if this fixes your problems with tpa6130a2 after
rebooting from Nokia kernel?

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]