From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nishanth Menon Date: Sat, 09 Jan 2010 08:57:59 -0600 Subject: [U-Boot] [beagleboard] TI:OMAP: [PATCH 3/4] Support 720Mhz configuration for OMAP35xx In-Reply-To: References: <782515bb1001081152t569a3e5end7fa4ba535c9c354@mail.gmail.com> Message-ID: <4B489977.6010808@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 :). 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 >>> + >>> +/* >>> + * 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. 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? > the functionality that we are bringing in... > > Regards, > Khasim > >