| 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: <56F0FC0F.8000107@gmail.com>
Date: Tue, 22 Mar 2016 10:02:23 +0200
From: Ivaylo Dimitrov <ivo.g.dimitrov.75@...il.com>
To: Sebastian Reichel <sre@...nel.org>
Cc: Mark Brown <broonie@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Peter Ujfalusi <peter.ujfalusi@...com>,
Grygorii Strashko <grygorii.strashko@...com>,
Pali Rohár <pali.rohar@...il.com>,
Jarkko Nikula <jarkko.nikula@...mer.com>,
Tony Lindgren <tony@...mide.com>,
Lars-Peter Clausen <lars@...afoo.de>,
linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
Pavel Machek <pavel@....cz>,
Aaro Koskinen <aaro.koskinen@....fi>,
Nishanth Menon <nm@...com>, merlijn@...zup.org
Subject: Re: Nokia N900 - audio TPA6130A2 problems
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
Powered by blists - more mailing lists