[U-Boot] [beagleboard] TI:OMAP: [PATCH 3/4] Support 720Mhz configuration for OMAP35xx

[Khasim Syed Mohammed <khasim@beagleboard.org>]

On Sat, Jan 9, 2010 at 8:27 PM, Nishanth Menon <menon.nishanth@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.nishanth@gmail.com>
>> wrote:
>>
>>>
>>> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed
>>> <khasim@beagleboard.org> wrote:
>>>
>>>>
>>>> From bba669562fa208d12f4c7cd8188446e8576cd6ee Mon Sep 17 00:00:00 2001
>>>> From: Syed Mohammed Khasim <khasim@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 <khasim@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
>>
>>
>
>