Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360072 > unrolled thread

Re: Nokia N900 - audio TPA6130A2 problems

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-17 18:30 +0100
    Re: Nokia N900 - audio TPA6130A2 problems Peter Ujfalusi <peter.ujfalusi@ti.com> - 2016-03-18 11:40 +0100
      Re: Nokia N900 - audio TPA6130A2 problems Ивайло Димитров   <ivo.g.dimitrov.75@gmail.com> - 2016-03-18 14:20 +0100
        Re: Nokia N900 - audio TPA6130A2 problems Sebastian Reichel <sre@kernel.org> - 2016-03-18 14:40 +0100
          Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-18 14:50 +0100
            Re: Nokia N900 - audio TPA6130A2 problems Sebastian Reichel <sre@kernel.org> - 2016-03-18 16:10 +0100
              Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-18 17:00 +0100
              Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-19 10:00 +0100
                Re: Nokia N900 - audio TPA6130A2 problems Sebastian Reichel <sre@kernel.org> - 2016-03-20 06:20 +0100
                  Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-20 20:50 +0100
                    Re: Nokia N900 - audio TPA6130A2 problems Sebastian Reichel <sre@kernel.org> - 2016-03-21 01:10 +0100
                      Re: Nokia N900 - audio TPA6130A2 problems Sebastian Reichel <sre@kernel.org> - 2016-03-21 02:50 +0100
                      Re: Nokia N900 - audio TPA6130A2 problems Mark Brown <broonie@kernel.org> - 2016-03-21 13:10 +0100
                  Re: Nokia N900 - audio TPA6130A2 problems Mark Brown <broonie@kernel.org> - 2016-03-21 12:50 +0100
                    Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-21 14:40 +0100
                      Re: Nokia N900 - audio TPA6130A2 problems Mark Brown <broonie@kernel.org> - 2016-03-21 14:50 +0100
                        Re: Nokia N900 - audio TPA6130A2 problems Sebastian Reichel <sre@kernel.org> - 2016-03-21 16:00 +0100
                          Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-21 20:40 +0100
                            Re: Nokia N900 - audio TPA6130A2 problems Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> - 2016-03-22 09:10 +0100

#1360072 — Re: Nokia N900 - audio TPA6130A2 problems

Hi,

>
> can you try this:
> diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c
> index 11d85c5c787a..7f5881bff5d9 100644
> --- a/sound/soc/codecs/tpa6130a2.c
> +++ b/sound/soc/codecs/tpa6130a2.c
> @@ -386,6 +386,8 @@ static int tpa6130a2_probe(struct i2c_client *client,
>                  data->power_gpio = pdata->power_gpio;
>          } else if (np) {
>                  data->power_gpio = of_get_named_gpio(np, "power-gpio", 0);
> +               if (data->power_gpio == -EPROBE_DEFER)
> +                       return data->power_gpio;
>          } else {
>                  dev_err(dev, "Platform data not set\n");
>                  dump_stack();
>


Doesn't help :(

Ivo

[toc] | [next] | [standalone]

#1360503

On 03/17/16 19:26, Ivaylo Dimitrov wrote:
> Hi,
> 
>>
>> can you try this:
>> diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c
>> index 11d85c5c787a..7f5881bff5d9 100644
>> --- a/sound/soc/codecs/tpa6130a2.c
>> +++ b/sound/soc/codecs/tpa6130a2.c
>> @@ -386,6 +386,8 @@ static int tpa6130a2_probe(struct i2c_client *client,
>>                  data->power_gpio = pdata->power_gpio;
>>          } else if (np) {
>>                  data->power_gpio = of_get_named_gpio(np, "power-gpio", 0);
>> +               if (data->power_gpio == -EPROBE_DEFER)
>> +                       return data->power_gpio;
>>          } else {
>>                  dev_err(dev, "Platform data not set\n");
>>                  dump_stack();
>>
> 
> 
> Doesn't help :(

it worth a try ;)

But enabling the pull via DT for the i2c2 works?

-- 
Péter

[toc] | [prev] | [next] | [standalone]

#1360608

Hi

On Fri Mar 18 12:33:14 2016 Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
> But enabling the pull via DT for the i2c2 works?
> 

No :(. I even migrated the driver to regmap - no gain. Maybe i2c bus is blocked by another device held in reset. The next thing I am going to try is to deassert reset/power gpios on all the devices on i2c-2 bus, to see if it will make any difference. Or if you have any other ideas, please share.

Ivo

[toc] | [prev] | [next] | [standalone]

#1360621

[Multipart message — attachments visible in raw view] — view raw

On Fri, Mar 18, 2016 at 03:13:49PM +0200, Ивайло Димитров wrote:
> On Fri Mar 18 12:33:14 2016 Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > But enabling the pull via DT for the i2c2 works?
> 
> No :(. I even migrated the driver to regmap - no gain. Maybe i2c
> bus is blocked by another device held in reset. The next thing I
> am going to try is to deassert reset/power gpios on all the
> devices on i2c-2 bus, to see if it will make any difference. Or if
> you have any other ideas, please share.

Have you tried the ideas from <20160317004917.GA6750@earth> (Date:
Thu, 17 Mar 2016 01:49:18 +0100)?

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1360636

Hi,

On 18.03.2016 15:36, Sebastian Reichel wrote:

>
> Have you tried the ideas from <20160317004917.GA6750@earth> (Date:
> Thu, 17 Mar 2016 01:49:18 +0100)?
>

To the extend I understood them :)

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 didn't try building the driver as a modules, instead I played with 
bind/unbind functionality - makes no difference.

Thanks,
Ivo

[toc] | [prev] | [next] | [standalone]

#1360716

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Fri, Mar 18, 2016 at 03:45:26PM +0200, Ivaylo Dimitrov wrote:
> On 18.03.2016 15:36, Sebastian Reichel wrote:
> 
> >Have you tried the ideas from <20160317004917.GA6750@earth> (Date:
> >Thu, 17 Mar 2016 01:49:18 +0100)?
> >
> 
> To the extend I understood them :)
> 
> 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.

Did you by chance also test adding a delay after setting the power
gpio? According to the datasheet:

Start-up time from shutdown: typical 5ms

> I didn't try building the driver as a module, instead I played with
> bind/unbind functionality - makes no difference.

Ok.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1360751

Hi,

On 18.03.2016 17:04, Sebastian Reichel wrote:
> Did you by chance also test adding a delay after setting the power
> gpio? According to the datasheet:
>
> Start-up time from shutdown: typical 5ms
>

:)

if (data->power_gpio) {
	dev_err(&tpa6130a2_client->dev,
		"!!!!!!!!!!!! GPIO SET: %d\n", ret);
	mdelay(10);
	gpiod_set_value(data->power_gpio, 1);
	mdelay(10);
}

Ivo

[toc] | [prev] | [next] | [standalone]

#1361060

Hi,

On 18.03.2016 17:04, Sebastian Reichel wrote:
> Hi,
>
> 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

after restart from stock kernel:

[    2.388610] twl4030ldo_get_voltage_sel VMMC2 vsel 0x0000000a
[    2.394958] twl4030reg_enable VMMC2 grp 0x00000028
[    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.

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.

Ivo

[toc] | [prev] | [next] | [standalone]

#1361216

[Multipart message — attachments visible in raw view] — view raw

Hi,

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.

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

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

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

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

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1361433

Hi

On 20.03.2016 07:17, Sebastian Reichel wrote:
> Hi,
>
> 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.

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

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

>> [    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?

Thanks,
Ivo

[toc] | [prev] | [next] | [standalone]

#1361471

[Multipart message — attachments visible in raw view] — view raw

Hi,

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.
2. if regulator is in sleep mode, regulator enable should
   disable sleep mode.

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1361477

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1361751

[Multipart message — attachments visible in raw view] — view raw

On Mon, Mar 21, 2016 at 01:04:18AM +0100, Sebastian Reichel wrote:

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

Absolutely not!  Like all other regulator drivers the twl driver should
be taking no decisions about what voltage to set except in response to a
set_voltage() call.  All the logic about setting voltages is factored
out into the core.

Please try to understand the abstractions we're using in the framework.

[toc] | [prev] | [next] | [standalone]

#1361736

[Multipart message — attachments visible in raw view] — view raw

On Sun, Mar 20, 2016 at 06:17:04AM +0100, Sebastian Reichel wrote:
> On Sat, Mar 19, 2016 at 10:49:57AM +0200, Ivaylo Dimitrov wrote:

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

No, if the voltage is variable we can't tell what the current
constraints are without something telling us so we just don't vary the
voltage until we're told to do this.  If we immediately lower the
voltage to the minimum supported voltage that's going to break things.

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

This is completely broken.  A consumer should never need to check to see
if the voltage can be varied and if the device doesn't actually need to
change the voltage as a part of the functionality it delivers then it
should just leave it up to the board constraints to set something, the
actual constaints will be much tighter than the device constraints and
there's a reasonable chance that there may be software compatible
variants of the device with different supply requirements.

[toc] | [prev] | [next] | [standalone]

#1361844

Hi,

On 21.03.2016 13:45, Mark Brown wrote:
>
> No, if the voltage is variable we can't tell what the current
> constraints are without something telling us so we just don't vary the
> voltage until we're told to do this.  If we immediately lower the
> voltage to the minimum supported voltage that's going to break things.
>

There are constraints set by the board DTS. Isn't it reasonable the 
framework to set the voltage to minimum voltage from the dts if the 
current set one is bellow it?

Regards,
Ivo

[toc] | [prev] | [next] | [standalone]

#1361856

[Multipart message — attachments visible in raw view] — view raw

On Mon, Mar 21, 2016 at 03:39:15PM +0200, Ivaylo Dimitrov wrote:
> On 21.03.2016 13:45, Mark Brown wrote:

> >No, if the voltage is variable we can't tell what the current
> >constraints are without something telling us so we just don't vary the
> >voltage until we're told to do this.  If we immediately lower the
> >voltage to the minimum supported voltage that's going to break things.

> There are constraints set by the board DTS. Isn't it reasonable the
> framework to set the voltage to minimum voltage from the dts if the current
> set one is bellow it?

Yes, if it's out of bounds for the constraints we should bring it
up/down to the minimum/maximum (when copying people into a thread it's a
good idea to explain what the problem you are trying to solve is,
especially if you're throwing around bodges).

[toc] | [prev] | [next] | [standalone]

#1361913

[Multipart message — attachments visible in raw view] — view raw

Hi Mark,

On Mon, Mar 21, 2016 at 01:45:15PM +0000, Mark Brown wrote:
> On Mon, Mar 21, 2016 at 03:39:15PM +0200, Ivaylo Dimitrov wrote:
> > On 21.03.2016 13:45, Mark Brown wrote:
> 
> > >No, if the voltage is variable we can't tell what the current
> > >constraints are without something telling us so we just don't vary the
> > >voltage until we're told to do this.  If we immediately lower the
> > >voltage to the minimum supported voltage that's going to break things.
> 
> > There are constraints set by the board DTS. Isn't it reasonable the
> > framework to set the voltage to minimum voltage from the dts if the current
> > set one is bellow it?
> 
> Yes, if it's out of bounds for the constraints we should bring it
> up/down to the minimum/maximum (when copying people into a thread it's a
> good idea to explain what the problem you are trying to solve is,
> especially if you're throwing around bodges).

We have this regulator definition in omap3-n900.dts:

&vmmc2 {
	regulator-name = "V28_A";
	regulator-min-microvolt = <2800000>;
	regulator-max-microvolt = <3000000>;
	regulator-always-on; /* due VIO leak to AIC34 VDDs */
};

The regulator is enabled during probe, but the voltage is not
configured. The default reset voltage of the regulator is 2.6V.
So basically when the regulator is enabled, it uses a voltage,
which is out of the DT specified range.

We also have a second problem: If the system has been rebooted from
Nokia's stock kernel the regulator is left in STANDBY mode. Since
the mode is not configured during probe, it results in different
problems. According to my understanding it can be fixed trivially
by adding

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

-- Sebastian

[toc] | [prev] | [next] | [standalone]

#1362099

On 21.03.2016 16:53, Sebastian Reichel wrote:
> Hi Mark,
>
> On Mon, Mar 21, 2016 at 01:45:15PM +0000, Mark Brown wrote:
>> On Mon, Mar 21, 2016 at 03:39:15PM +0200, Ivaylo Dimitrov wrote:
>>> On 21.03.2016 13:45, Mark Brown wrote:
>>
>>>> No, if the voltage is variable we can't tell what the current
>>>> constraints are without something telling us so we just don't vary the
>>>> voltage until we're told to do this.  If we immediately lower the
>>>> voltage to the minimum supported voltage that's going to break things.
>>
>>> There are constraints set by the board DTS. Isn't it reasonable the
>>> framework to set the voltage to minimum voltage from the dts if the current
>>> set one is bellow it?
>>
>> Yes, if it's out of bounds for the constraints we should bring it
>> up/down to the minimum/maximum (when copying people into a thread it's a
>> good idea to explain what the problem you are trying to solve is,
>> especially if you're throwing around bodges).
>
> We have this regulator definition in omap3-n900.dts:
>
> &vmmc2 {
> 	regulator-name = "V28_A";
> 	regulator-min-microvolt = <2800000>;
> 	regulator-max-microvolt = <3000000>;
> 	regulator-always-on; /* due VIO leak to AIC34 VDDs */
> };
>
> The regulator is enabled during probe, but the voltage is not
> configured. The default reset voltage of the regulator is 2.6V.
> So basically when the regulator is enabled, it uses a voltage,
> which is out of the DT specified range.
>
> We also have a second problem: If the system has been rebooted from
> Nokia's stock kernel the regulator is left in STANDBY mode. Since
> the mode is not configured during probe, it results in different
> problems. According to my understanding it can be fixed trivially
> by adding
>
> &vmmc2 {
>      regulator-initial-mode = <2>;
> };
>

doesn't work:

"regulator-vmmc2: mapping for mode 2 not defined"

twl-regulator is missing .of_map_mode function.

Also, if we go that route, we should set the initial modes for all the 
regulators, not only vmmc2 (and not only for N900), as we don't really 
know what is the status of regulators at startup. I think a better 
approach is if regulator framework sets all always-on regulators to 
enabled, unless stated otherwise (which it already does iiuc).

I think there is a bug in twl-regulator twl4030reg_enable() and/or 
twl4030reg_is_enabled() - the latter only checks if DEV_GRP is P1, but 
not for the actual state of the regulator (bits 3:0). Also, what looks 
suspicious to me is that all the regulators are put in P1 device group. 
Legacy board code spreads the regulators all over the groups, so maybe 
this is simply a regression compared to legacy boot.

Regards,
Ivo

[toc] | [prev] | [next] | [standalone]

#1362410

On 21.03.2016 21:34, Ivaylo Dimitrov wrote:
>
>
> On 21.03.2016 16:53, Sebastian Reichel wrote:
>> Hi Mark,
>>
>> On Mon, Mar 21, 2016 at 01:45:15PM +0000, Mark Brown wrote:
>>> On Mon, Mar 21, 2016 at 03:39:15PM +0200, Ivaylo Dimitrov wrote:
>>>> On 21.03.2016 13:45, Mark Brown wrote:
>>>
>>>>> No, if the voltage is variable we can't tell what the current
>>>>> constraints are without something telling us so we just don't vary the
>>>>> voltage until we're told to do this.  If we immediately lower the
>>>>> voltage to the minimum supported voltage that's going to break things.
>>>
>>>> There are constraints set by the board DTS. Isn't it reasonable the
>>>> framework to set the voltage to minimum voltage from the dts if the
>>>> current
>>>> set one is bellow it?
>>>
>>> Yes, if it's out of bounds for the constraints we should bring it
>>> up/down to the minimum/maximum (when copying people into a thread it's a
>>> good idea to explain what the problem you are trying to solve is,
>>> especially if you're throwing around bodges).
>>
>> We have this regulator definition in omap3-n900.dts:
>>
>> &vmmc2 {
>>     regulator-name = "V28_A";
>>     regulator-min-microvolt = <2800000>;
>>     regulator-max-microvolt = <3000000>;
>>     regulator-always-on; /* due VIO leak to AIC34 VDDs */
>> };
>>
>> The regulator is enabled during probe, but the voltage is not
>> configured. The default reset voltage of the regulator is 2.6V.
>> So basically when the regulator is enabled, it uses a voltage,
>> which is out of the DT specified range.
>>
>> We also have a second problem: If the system has been rebooted from
>> Nokia's stock kernel the regulator is left in STANDBY mode. Since
>> the mode is not configured during probe, it results in different
>> problems. According to my understanding it can be fixed trivially
>> by adding
>>
>> &vmmc2 {
>>      regulator-initial-mode = <2>;
>> };
>>
>
> doesn't work:
>
> "regulator-vmmc2: mapping for mode 2 not defined"
>
> twl-regulator is missing .of_map_mode function.
>
> Also, if we go that route, we should set the initial modes for all the
> regulators, not only vmmc2 (and not only for N900), as we don't really
> know what is the status of regulators at startup. I think a better
> approach is if regulator framework sets all always-on regulators to
> enabled, unless stated otherwise (which it already does iiuc).
>
> I think there is a bug in twl-regulator twl4030reg_enable() and/or
> twl4030reg_is_enabled() - the latter only checks if DEV_GRP is P1, but
> not for the actual state of the regulator (bits 3:0). Also, what looks
> suspicious to me is that all the regulators are put in P1 device group.
> Legacy board code spreads the regulators all over the groups, so maybe
> this is simply a regression compared to legacy boot.
>

This is what seems to work, I would like some comments from those who 
are more experienced with twl4030 than me before posting a formal patch. 
I borrowed the code from stock Nokia kernel.

diff --git a/drivers/regulator/twl-regulator.c 
b/drivers/regulator/twl-regulator.c
index 955a6fb..3740df4 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -21,7 +21,7 @@
  #include <linux/regulator/machine.h>
  #include <linux/regulator/of_regulator.h>
  #include <linux/i2c/twl.h>
-
+#include <linux/delay.h>

  /*
   * The TWL4030/TW5030/TPS659x0/TWL6030 family chips include power 
management, a
@@ -165,7 +165,7 @@ static int twl4030reg_is_enabled(struct 
regulator_dev *rdev)
  	if (state < 0)
  		return state;

-	return state & P1_GRP_4030;
+	return (state & 0x0f) != 0;
  }

  static int twl6030reg_is_enabled(struct regulator_dev *rdev)
@@ -188,11 +188,75 @@ static int twl6030reg_is_enabled(struct 
regulator_dev *rdev)
  	return grp && (val == TWL6030_CFG_STATE_ON);
  }

+static int twl4030_wait_pb_ready(void)
+{
+
+	int ret, timeout = 10;
+	u8 pb_state;
+
+	do {
+		ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &pb_state,
+				      TWL4030_PM_MASTER_PB_CFG);
+		if (ret < 0)
+			return ret;
+
+		if (!(pb_state & 1))
+			return 0;
+
+		mdelay(1);
+		timeout--;
+
+	} while (timeout);
+
+	return -ETIMEDOUT;
+}
+
+static int twl4030_send_pb_msg(unsigned msg)
+{
+	u8 pb_state;
+	int ret;
+
+	/* save powerbus configuration */
+	ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &pb_state,
+			      TWL4030_PM_MASTER_PB_CFG);
+	if (ret < 0)
+		return ret;
+
+	/* Enable I2C access to powerbus */
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, pb_state | BIT(1),
+			       TWL4030_PM_MASTER_PB_CFG);
+	if (ret < 0)
+		return ret;
+
+	ret = twl4030_wait_pb_ready();
+	if (ret < 0)
+		return ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg >> 8,
+			       TWL4030_PM_MASTER_PB_WORD_MSB);
+	if (ret < 0)
+		return ret;
+
+	ret = twl_i2c_write_u8(TWL_MODULE_PM_MASTER, msg & 0xff,
+			       TWL4030_PM_MASTER_PB_WORD_LSB);
+	if (ret < 0)
+		return ret;
+
+	ret = twl4030_wait_pb_ready();
+	if (ret < 0)
+		return ret;
+
+	/* Restore powerbus configuration */
+	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER, pb_state,
+				TWL_MODULE_PM_MASTER);
+}
+
  static int twl4030reg_enable(struct regulator_dev *rdev)
  {
  	struct twlreg_info	*info = rdev_get_drvdata(rdev);
  	int			grp;
  	int			ret;
+	unsigned 		message;

  	grp = twlreg_grp(rdev);
  	if (grp < 0)
@@ -201,8 +265,12 @@ static int twl4030reg_enable(struct regulator_dev 
*rdev)
  	grp |= P1_GRP_4030;

  	ret = twlreg_write(info, TWL_MODULE_PM_RECEIVER, VREG_GRP, grp);
+	if (ret < 0)
+		return ret;

-	return ret;
+	message = MSG_SINGULAR(DEV_GRP_P1, info->id, RES_STATE_ACTIVE);
+
+	return twl4030_send_pb_msg(message);
  }

  static int twl6030reg_enable(struct regulator_dev *rdev)
@@ -324,13 +392,7 @@ static int twl4030reg_set_mode(struct regulator_dev 
*rdev, unsigned mode)
  	if (!(status & (P3_GRP_4030 | P2_GRP_4030 | P1_GRP_4030)))
  		return -EACCES;

-	status = twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
-			message >> 8, TWL4030_PM_MASTER_PB_WORD_MSB);
-	if (status < 0)
-		return status;
-
-	return twl_i2c_write_u8(TWL_MODULE_PM_MASTER,
-			message & 0xff, TWL4030_PM_MASTER_PB_WORD_LSB);
+	return twl4030_send_pb_msg(message);
  }

  static int twl6030reg_set_mode(struct regulator_dev *rdev, unsigned mode)

Regards,
Ivo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web