Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758198AbcCVICm (ORCPT ); Tue, 22 Mar 2016 04:02:42 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33589 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751389AbcCVICa (ORCPT ); Tue, 22 Mar 2016 04:02:30 -0400 Subject: Re: Nokia N900 - audio TPA6130A2 problems To: Sebastian Reichel References: <56EBD96A.8090505@ti.com> <1458306829.11841.2.camel@Nokia-N900> <20160318133641.GB16747@earth> <56EC0676.3000509@gmail.com> <20160318150404.GA30829@earth> <56ED12B5.9000103@gmail.com> <20160320051704.GA12934@earth> <20160321114521.GO2566@sirena.org.uk> <56EFF983.2050303@gmail.com> <20160321134515.GT2566@sirena.org.uk> <20160321145347.GA19391@earth> <56F04CC7.1040403@gmail.com> Cc: Mark Brown , Liam Girdwood , Peter Ujfalusi , Grygorii Strashko , =?UTF-8?Q?Pali_Roh=c3=a1r?= , Jarkko Nikula , Tony Lindgren , Lars-Peter Clausen , linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, Pavel Machek , Aaro Koskinen , Nishanth Menon , merlijn@wizzup.org From: Ivaylo Dimitrov Message-ID: <56F0FC0F.8000107@gmail.com> Date: Tue, 22 Mar 2016 10:02:23 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56F04CC7.1040403@gmail.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6254 Lines: 212 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 #include #include - +#include /* * 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