From mboxrd@z Thu Jan 1 00:00:00 1970 From: Khasim Syed Mohammed Date: Sun, 10 Jan 2010 08:32:24 +0530 Subject: [U-Boot] [beagleboard] TI:OMAP: [PATCH 3/4] Support 720Mhz configuration for OMAP35xx In-Reply-To: <4B489977.6010808@gmail.com> References: <782515bb1001081152t569a3e5end7fa4ba535c9c354@mail.gmail.com> <4B489977.6010808@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Sat, Jan 9, 2010 at 8:27 PM, Nishanth Menon wrote: > Khasim Syed Mohammed said the following on 01/08/2010 09:21 PM: >> >> On Sat, Jan 9, 2010 at 1:22 AM, Nishanth Menon >> wrote: >> >>> >>> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed >>> wrote: >>> >>>> >>>> From bba669562fa208d12f4c7cd8188446e8576cd6ee Mon Sep 17 00:00:00 2001 >>>> From: Syed Mohammed Khasim >>>> Date: Fri, 8 Jan 2010 20:34:37 +0530 >>>> Subject: [PATCH] Support 720Mhz configuration for OMAP35xx >>>> >>>> Adds a new API "twl4030_pmrecv_vsel_cfg" to select voltage and group >>>> Adds support for 720Mhz in clock.c >>>> Board file modified to use these new APIs and boot at 720Mhz >>>> >>> >>> Could you split this into three patches please? easier to track >>> changes at a later date. >>> >>> a) introducing generic voltage setting API for twl >>> b) introduce 720mhz >>> c) beagle support for C4 with 720Mhz. >>> >>> >>>> >>>> Signed-off-by: Syed Mohammed Khasim >>>> --- >>>> ?board/ti/beagle/beagle.c ? ? ? | ? 20 ++++++++++++++++++-- >>>> ?cpu/arm_cortexa8/omap3/clock.c | ? 21 +++++++++++++++++++++ >>>> ?drivers/power/twl4030.c ? ? ? ?| ? 24 +++++++++++++++--------- >>>> ?include/twl4030.h ? ? ? ? ? ? ?| ? 16 ++++++++++++++++ >>>> ?4 files changed, 70 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c >>>> index 0def5a6..7985ee9 100644 >>>> --- a/board/ti/beagle/beagle.c >>>> +++ b/board/ti/beagle/beagle.c >>>> @@ -122,9 +122,27 @@ int misc_init_r(void) >>>> ? ? ? struct gpio *gpio5_base = (struct gpio *)OMAP34XX_GPIO5_BASE; >>>> ? ? ? struct gpio *gpio6_base = (struct gpio *)OMAP34XX_GPIO6_BASE; >>>> >>>> + ? ? ? beagle_identify(); >>>> + >>>> ? ? ? ? ? ? twl4030_power_init(); >>>> ? ? ? twl4030_led_init(); >>>> >>>> + ? ? ? if (beagle_revision == REVISION_C4) { >>>> + >>>> + ? ? ? ? ? ? ? /* Select TWL4030 VSEL to support 720Mhz */ >>>> + >>>> twl4030_pmrecv_vsel_cfg(TWL4030_PM_RECEIVER_VAUX2_DEDICATED, >>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VAUX2_VSEL_18, >>>> + >>>> TWL4030_PM_RECEIVER_VAUX2_DEV_GRP, >>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DEV_GRP_P1); >>>> + >>>> + ? ? ? ? ? ? ? twl4030_pmrecv_vsel_cfg(TWL4030_PM_RECEIVER_VDD1_VSEL, >>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VDD1_VSEL_14, >>>> + >>>> TWL4030_PM_RECEIVER_VDD1_DEV_GRP, >>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DEV_GRP_P1); >>>> + >>>> + ? ? ? ? ? ? ? prcm_config_720mhz(); >>>> + ? ? ? } >>>> + >>>> ? ? ? /* Configure GPIOs to output */ >>>> ? ? ? writel(~(GPIO23 | GPIO10 | GPIO8 | GPIO2 | GPIO1), >>>> &gpio6_base->oe); >>>> ? ? ? writel(~(GPIO31 | GPIO30 | GPIO29 | GPIO28 | GPIO22 | GPIO21 | >>>> @@ -136,8 +154,6 @@ int misc_init_r(void) >>>> ? ? ? writel(GPIO31 | GPIO30 | GPIO29 | GPIO28 | GPIO22 | GPIO21 | >>>> ? ? ? ? ? ? ? GPIO15 | GPIO14 | GPIO13 | GPIO12, >>>> &gpio5_base->setdataout); >>>> >>>> - ? ? ? beagle_identify(); >>>> - >>>> ? ? ? dieid_num_r(); >>>> >>>> ? ? ? return 0; >>>> diff --git a/cpu/arm_cortexa8/omap3/clock.c >>>> b/cpu/arm_cortexa8/omap3/clock.c >>>> index 174c453..d67517a 100644 >>>> --- a/cpu/arm_cortexa8/omap3/clock.c >>>> +++ b/cpu/arm_cortexa8/omap3/clock.c >>>> @@ -402,3 +402,24 @@ void per_clocks_enable(void) >>>> >>>> ? ? ? sdelay(1000); >>>> ?} >>>> + >>>> +/* >>>> + * Configure PRCM registers to get 720 Mhz >>>> + * >>>> + * NOTE: N value doesn't change, only M gets affected >>>> + */ >>>> +void prcm_config_720mhz(void) >>>> +{ >>>> + ? ? ? struct prcm *prcm_base = (struct prcm *)PRCM_BASE; >>>> + >>>> + ? ? ? /* Unlock MPU DPLL (slows things down, and needed later) */ >>>> + ? ? ? sr32(&prcm_base->clken_pll_mpu, 0, 3, PLL_LOW_POWER_BYPASS); >>>> + ? ? ? wait_on_value(ST_MPU_CLK, 0, &prcm_base->idlest_pll_mpu, >>>> LDELAY); >>>> + >>>> + ? ? ? /* Set M */ >>>> + ? ? ? sr32(&prcm_base->clksel1_pll_mpu, 8, 11, 0x2D0); >>>> + >>>> + ? ? ? /* lock mode */ >>>> + ? ? ? sr32(&prcm_base->clken_pll_mpu, 0, 3, PLL_LOCK); >>>> + ? ? ? wait_on_value(ST_MPU_CLK, 1, &prcm_base->idlest_pll_mpu, >>>> LDELAY); >>>> >>> >>> I know of dll lock infinite loops in some other system.. but that is a >>> different topic needing a different patch anyways. >>> >> >> Have you seen cpu/arm_cortexa8/omap3/syslib.c, wait_on_value does >> handle such an instance, where your hardware is broken >> > > I was wondering about the timeout LDELAY inwhich case wait_on_value returns > 0? >> >> We don't take care of failing systems, I would call them as hacks for >> such devices. >> > > this is a sad statement :( I call them error recovery unfortunately. > >> >>>> >>>> +} >>>> diff --git a/drivers/power/twl4030.c b/drivers/power/twl4030.c >>>> index eb066cb..d68e515 100644 >>>> --- a/drivers/power/twl4030.c >>>> +++ b/drivers/power/twl4030.c >>>> @@ -59,16 +59,9 @@ void twl4030_power_reset_init(void) >>>> ? ? ? } >>>> ?} >>>> >>>> - >>>> ?/* >>>> ?* Power Init >>>> ?*/ >>>> -#define DEV_GRP_P1 ? ? ? ? ? ? 0x20 >>>> -#define VAUX3_VSEL_28 ? ? ? ? ?0x03 >>>> -#define DEV_GRP_ALL ? ? ? ? ? ?0xE0 >>>> -#define VPLL2_VSEL_18 ? ? ? ? ?0x05 >>>> -#define VDAC_VSEL_18 ? ? ? ? ? 0x03 >>>> - >>>> ?void twl4030_power_init(void) >>>> ?{ >>>> ? ? ? unsigned char byte; >>>> @@ -98,8 +91,6 @@ void twl4030_power_init(void) >>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ?TWL4030_PM_RECEIVER_VDAC_DEDICATED); >>>> ?} >>>> >>>> -#define VMMC1_VSEL_30 ? ? ? ? ?0x02 >>>> - >>>> ?void twl4030_power_mmc_init(void) >>>> ?{ >>>> ? ? ? unsigned char byte; >>>> @@ -113,3 +104,18 @@ void twl4030_power_mmc_init(void) >>>> ? ? ? twl4030_i2c_write_u8(TWL4030_CHIP_PM_RECEIVER, byte, >>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ?TWL4030_PM_RECEIVER_VMMC1_DEDICATED); >>>> ?} >>>> + >>>> +/* >>>> + * Generic function to select Device Group and Voltage >>>> + */ >>>> +void twl4030_pmrecv_vsel_cfg(u8 vsel_reg, u8 vsel_val, >>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? u8 dev_grp, u8 dev_grp_sel) >>>> +{ >>>> + ? ? ? /* Select the Device Group */ >>>> + ? ? ? twl4030_i2c_write_u8(TWL4030_CHIP_PM_RECEIVER, dev_grp_sel, >>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_grp); >>>> + >>>> + ? ? ? /* Select the Voltage */ >>>> + ? ? ? twl4030_i2c_write_u8(TWL4030_CHIP_PM_RECEIVER, vsel_val, >>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? vsel_reg); >>>> +} >>>> >>> >>> Assumption that i2c operations work 100% successfully! is'nt serial >>> bus subject to noise? and cant' i2c ops fail? >>> >> >> May be, ?such cases will be treated as system fail. Should be handled >> separately for "broken platforms". >> >> In beagleboard and EVMs atleast in last 4 revs we have never >> encountered such problems. >> >> > > I mean never seen an i2c read/write failure? I have seen at least a couple > unfortunately when one of the SDP3430's had some one solder a wrong pull up > resistor and another where a pull up resistor was torn off by accident. > > these are broken platforms ofcourse :). Yeah, > sigh, seeing that the rest of the > file is messed up in this regards, I leave it for the community to further > comment on this. > >>>> diff --git a/include/twl4030.h b/include/twl4030.h >>>> index f260ecb..b96c96c 100644 >>>> --- a/include/twl4030.h >>>> +++ b/include/twl4030.h >>>> @@ -359,6 +359,22 @@ >>>> ?#define TWL4030_USB_PHY_DPLL_CLK ? ? ? ? ? ? ? ? ? ? ? (1 << 0) >>>> >>>> ?/* >>>> + * Voltage Selection in PM Receiver Module >>>> + */ >>>> +#define VAUX2_VSEL_18 ? ? ? ? ?0x05 >>>> +#define VDD1_VSEL_14 ? ? ? ? ? 0x40 >>>> +#define VAUX3_VSEL_28 ? ? ? ? ?0x03 >>>> +#define VPLL2_VSEL_18 ? ? ? ? ?0x05 >>>> +#define VDAC_VSEL_18 ? ? ? ? ? 0x03 >>>> +#define VMMC1_VSEL_30 ? ? ? ? ?0x02 >>>> + Did you mean these lines ? When I apply the patch I don't see these kind of lines, they are properly arranged in TABs. I have also checked every patch with checkpatch.pl (from Linux). There are no such alignment issues. >>>> +/* >>>> + * Device Selection >>>> + */ >>>> +#define DEV_GRP_P1 ? ? ? ? ? ? 0x20 >>>> +#define DEV_GRP_ALL ? ? ? ? ? ?0xE0 >>>> + >>>> +/* >>>> ?* Convience functions to read and write from TWL4030 >>>> ?* >>>> ?* chip_no is the i2c address, it must be one of the chip addresses >>>> -- >>>> 1.5.6.3 >>>> >>> >>> we should try review ?again after you have split the series up. >>> >> >> I don't mind generating another patch series, but make sure you give >> as much comments as possible with given patch set (this is fourth try >> for the patch set), this level of discussion doesn't make sense for >> > > Hmm.. Please do not get frustrated, at least I am trying to make the code > generic and flexible enough for usable in other platforms. oh, I am not frustrated. I can correct the changes any number of times. They are not that big. My only concern was it was not worth as the functionality getting into u-boot was very small. > your work is > good, and we are going in the right direction, we will have a solution soon > I hope.Could I request a [V4] inside your patch subject to keep the reviewer > aware of this as well as in the --- diffstat section add link to previous > discussions for us to refer back on? >> Let me see how I can do that. >> the functionality that we are bringing in... >> >> Regards, >> Khasim >> >> > >