On Sat, Jan 9, 2010 at 8:27 PM, Nishanth Menon <menon.nisha...@gmail.com> 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 <menon.nisha...@gmail.com> >> wrote: >> >>> >>> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed >>> <kha...@beagleboard.org> wrote: >>> >>>> >>>> From bba669562fa208d12f4c7cd8188446e8576cd6ee Mon Sep 17 00:00:00 2001 >>>> From: Syed Mohammed Khasim <kha...@ti.com> >>>> 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 <kha...@ti.com> >>>> --- >>>> 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 >> >> > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot