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

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

[Khasim Syed Mohammed]

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

[Nishanth Menon]

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.

> +}
> 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?

> 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.
Regards,
Nishanth Menon

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

[Khasim Syed Mohammed]

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

We don't take care of failing systems, I would call them as hacks for
such devices.

>> +}
>> 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.

>> 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
the functionality that we are bringing in...

Regards,
Khasim

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

[Nishanth Menon]

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 :). 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
>
>   

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

[Khasim Syed Mohammed]

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
>>
>>
>
>

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

[Nishanth Menon]

Khasim Syed Mohammed said the following on 01/09/2010 09:02 PM:
> 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
>>>>>
[...]

>>>>> 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.
>>
[...]

>>>>> +#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.

no, I meant usage of twl4030_i2c_write_u8() without error check throughout the file.


Regards,
Nishanth Menon

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

[Khasim Syed Mohammed]

On Sun, Jan 10, 2010 at 9:14 PM, Nishanth Menon
<menon.nishanth@gmail.com> wrote:
> Khasim Syed Mohammed said the following on 01/09/2010 09:02 PM:
>>
>> 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
>>>>>>
> [...]
>
>>>>>> 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.
>>>
> [...]
>
>>>>>> +#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.
>
> no, I meant usage of twl4030_i2c_write_u8() without error check throughout
> the file.
>
Yeah, cleaning up the entire file might have to be a separate effort.
Nothing related to this patch.

I am wondering what one would do if I2C fails, an error message ???
Any way it means hardware is broken. Which will go through debugging
any way.

Regards,
Khasim