diff mbox

[U-Boot] beagleboard: enable HUB power on all variants of the BeagleBoard

Message ID 1314226061-6933-1-git-send-email-agnel.joel@gmail.com
State Changes Requested
Delegated to: Tom Rini
Headers show

Commit Message

Joel Fernandes Aug. 24, 2011, 10:47 p.m. UTC 
From: Christian Spielberger <c.spielberger@bct-electronic.com>

Signed-off-by: Joel A Fernandes <joelagnel@ti.com>
Cc: "Eric Bénard" <eric@eukrea.com>
Cc: "Christian Spielberger" <c.spielberger@bct-electronic.com>
---
Pushing this patch myself as no one else did.

Previous discussions:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/105663/
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101540/

 board/ti/beagle/beagle.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

Comments

Marek Vasut Aug. 25, 2011, 3:22 a.m. UTC | #1 
On Thursday, August 25, 2011 12:47:41 AM Joel A Fernandes wrote:
> From: Christian Spielberger <c.spielberger@bct-electronic.com>
> 
> Signed-off-by: Joel A Fernandes <joelagnel@ti.com>
> Cc: "Eric Bأ�nard" <eric@eukrea.com>
> Cc: "Christian Spielberger" <c.spielberger@bct-electronic.com>
> ---
> Pushing this patch myself as no one else did.
> 
> Previous discussions:
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/105663/
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101540/
> 
>  board/ti/beagle/beagle.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c
> index 13fe39b..9d65e9e 100644
> --- a/board/ti/beagle/beagle.c
> +++ b/board/ti/beagle/beagle.c
> @@ -332,7 +332,15 @@ int misc_init_r(void)
>  		setenv(expansion_config.env_var, expansion_config.env_setting);
> 
>  	twl4030_power_init();
> -	twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
> +	switch (get_board_revision()) {
> +	case REVISION_XM_C:
> +	case REVISION_C4:
> +		twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
> +		break;
> +	default:
> +		twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
> +		break;
> +	}

Am I just too sleepy or the Subject doesn't correlate with what the patch does 
...

My understanding of Subject is that it enables power on HUB ... USB HUB ? But 
the patch enables some LEDs ?

I might be wrong.

CHeers
> 
>  	/* Set GPIO states before they are made outputs */
>  	writel(GPIO23 | GPIO10 | GPIO8 | GPIO2 | GPIO1,
Jason Kridner Aug. 25, 2011, 3:30 a.m. UTC | #2 
On Wed, Aug 24, 2011 at 11:22 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Thursday, August 25, 2011 12:47:41 AM Joel A Fernandes wrote:
>> From: Christian Spielberger <c.spielberger@bct-electronic.com>
>>
>> Signed-off-by: Joel A Fernandes <joelagnel@ti.com>
>> Cc: "Eric Bأ�nard" <eric@eukrea.com>
>> Cc: "Christian Spielberger" <c.spielberger@bct-electronic.com>
>> ---
>> Pushing this patch myself as no one else did.
>>
>> Previous discussions:
>> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/105663/
>> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101540/
>>
>>  board/ti/beagle/beagle.c |   10 +++++++++-
>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c
>> index 13fe39b..9d65e9e 100644
>> --- a/board/ti/beagle/beagle.c
>> +++ b/board/ti/beagle/beagle.c
>> @@ -332,7 +332,15 @@ int misc_init_r(void)
>>               setenv(expansion_config.env_var, expansion_config.env_setting);
>>
>>       twl4030_power_init();
>> -     twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
>> +     switch (get_board_revision()) {
>> +     case REVISION_XM_C:
>> +     case REVISION_C4:
>> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
>> +             break;
>> +     default:
>> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
>> +             break;
>> +     }

Please excuse if this is a duplicate, but I'm configuring a new mail
tool and I don't think my other message got sent.

It is XM_A and XM_B that are odd-balls, so the correct code should be
something like:

+     switch (get_board_revision()) {
+     case REVISION_XM_A:
+     case REVISION_XM_B:
+             twl4030_led_init(TWL4030_LED_LEDEN_LEDBON); /* LEDA
signal set low */
+             break;
+     default:
+             twl4030_led_init(TWL4030_LED_LEDEN_LEDAON |
TWL4030_LED_LEDEN_LEDBON);
+             break;
>> +     }

>
> Am I just too sleepy or the Subject doesn't correlate with what the patch does
> ...
>
> My understanding of Subject is that it enables power on HUB ... USB HUB ? But
> the patch enables some LEDs ?

The LED drivers on the power management device (TWL4030-compatible)
are used to enable the hub power.

>
> I might be wrong.
>
> CHeers
>>
>>       /* Set GPIO states before they are made outputs */
>>       writel(GPIO23 | GPIO10 | GPIO8 | GPIO2 | GPIO1,
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Marek Vasut Aug. 25, 2011, 3:38 a.m. UTC | #3 
On Thursday, August 25, 2011 05:30:59 AM Jason Kridner wrote:
> On Wed, Aug 24, 2011 at 11:22 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Thursday, August 25, 2011 12:47:41 AM Joel A Fernandes wrote:
> >> From: Christian Spielberger <c.spielberger@bct-electronic.com>
> >> 
> >> Signed-off-by: Joel A Fernandes <joelagnel@ti.com>
> >> Cc: "Eric Bأ�nard" <eric@eukrea.com>
> >> Cc: "Christian Spielberger" <c.spielberger@bct-electronic.com>
> >> ---
> >> Pushing this patch myself as no one else did.
> >> 
> >> Previous discussions:
> >> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/105663/
> >> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101540/
> >> 
> >>  board/ti/beagle/beagle.c |   10 +++++++++-
> >>  1 files changed, 9 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c
> >> index 13fe39b..9d65e9e 100644
> >> --- a/board/ti/beagle/beagle.c
> >> +++ b/board/ti/beagle/beagle.c
> >> @@ -332,7 +332,15 @@ int misc_init_r(void)
> >>               setenv(expansion_config.env_var,
> >> expansion_config.env_setting);
> >> 
> >>       twl4030_power_init();
> >> -     twl4030_led_init(TWL4030_LED_LEDEN_LEDAON |
> >> TWL4030_LED_LEDEN_LEDBON); +     switch (get_board_revision()) {
> >> +     case REVISION_XM_C:
> >> +     case REVISION_C4:
> >> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDAON |
> >> TWL4030_LED_LEDEN_LEDBON); +             break;
> >> +     default:
> >> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
> >> +             break;
> >> +     }
> 
> Please excuse if this is a duplicate, but I'm configuring a new mail
> tool and I don't think my other message got sent.
> 
> It is XM_A and XM_B that are odd-balls, so the correct code should be
> something like:
> 
> +     switch (get_board_revision()) {
> +     case REVISION_XM_A:
> +     case REVISION_XM_B:
> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDBON); /* LEDA
> signal set low */
> +             break;
> +     default:
> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDAON |
> TWL4030_LED_LEDEN_LEDBON);
> +             break;
> 
> >> +     }
> > 
> > Am I just too sleepy or the Subject doesn't correlate with what the patch
> > does ...
> > 
> > My understanding of Subject is that it enables power on HUB ... USB HUB ?
> > But the patch enables some LEDs ?
> 
> The LED drivers on the power management device (TWL4030-compatible)
> are used to enable the hub power.

Oh uh ... ok, now I understand. Please add a comment explaining it.

Also, change the subject se it's clear you're operating with USB HUB here.

Thanks!

> 
> > I might be wrong.
> > 
> > CHeers
> > 
> >>       /* Set GPIO states before they are made outputs */
> >>       writel(GPIO23 | GPIO10 | GPIO8 | GPIO2 | GPIO1,
> > 
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
Joel Fernandes Aug. 26, 2011, 6:44 p.m. UTC | #4 
On Wed, Aug 24, 2011 at 10:30 PM, Jason Kridner
<jkridner@beagleboard.org> wrote:
> On Wed, Aug 24, 2011 at 11:22 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On Thursday, August 25, 2011 12:47:41 AM Joel A Fernandes wrote:
>>> From: Christian Spielberger <c.spielberger@bct-electronic.com>
>>>
>>> Signed-off-by: Joel A Fernandes <joelagnel@ti.com>
>>> Cc: "Eric Bأ�nard" <eric@eukrea.com>
>>> Cc: "Christian Spielberger" <c.spielberger@bct-electronic.com>
>>> ---
>>> Pushing this patch myself as no one else did.
>>>
>>> Previous discussions:
>>> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/105663/
>>> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/101540/
>>>
>>>  board/ti/beagle/beagle.c |   10 +++++++++-
>>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c
>>> index 13fe39b..9d65e9e 100644
>>> --- a/board/ti/beagle/beagle.c
>>> +++ b/board/ti/beagle/beagle.c
>>> @@ -332,7 +332,15 @@ int misc_init_r(void)
>>>               setenv(expansion_config.env_var, expansion_config.env_setting);
>>>
>>>       twl4030_power_init();
>>> -     twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
>>> +     switch (get_board_revision()) {
>>> +     case REVISION_XM_C:
>>> +     case REVISION_C4:
>>> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
>>> +             break;
>>> +     default:
>>> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
>>> +             break;
>>> +     }
>
> Please excuse if this is a duplicate, but I'm configuring a new mail
> tool and I don't think my other message got sent.
>
> It is XM_A and XM_B that are odd-balls, so the correct code should be
> something like:
>
> +     switch (get_board_revision()) {
> +     case REVISION_XM_A:
> +     case REVISION_XM_B:
> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDBON); /* LEDA
> signal set low */
> +             break;
> +     default:
> +             twl4030_led_init(TWL4030_LED_LEDEN_LEDAON |
> TWL4030_LED_LEDEN_LEDBON);
> +             break;


Hi Jason, I think it should be:


 +     switch (get_board_revision()) {
 +     case REVISION_XM_A:
 +     case REVISION_XM_B:
 +             twl4030_led_init(TWL4030_LED_LEDEN_LEDAON |
TWL4030_LED_LEDEN_LEDBON); /* LEDA
 signal set low */
 +             break;
 +     default:
 +             twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
 +             break;


Here's the equivalent in the linux kernel board file (default is
GPIOF_OUT_INIT_LOW)

        printk(KERN_INFO "OMAP3 Beagle Rev: xM Ax/Bx\n");
        omap3_beagle_version = OMAP3BEAGLE_BOARD_XM;
        beagle_config.usb_pwr_level = GPIOF_OUT_INIT_HIGH;


Let me know your comments.

thanks,
Joel
Joel Fernandes Aug. 27, 2011, 4:46 a.m. UTC | #5 
+U-boot list


---------- Forwarded message ----------
From: Joel A Fernandes <agnel.joel@gmail.com>
Date: Fri, Aug 26, 2011 at 11:45 PM
Subject: Re: [PATCH] beagleboard: enable HUB power on all variants of
the BeagleBoard
To: Jason Kridner <jkridner@beagleboard.org>


>
> The logic is wrong here.  I don't remember the polarity, but I do remember that it is
> xM-A and xM-B that are the oddballs.  I believe this is the correct change:

Hi Jason,

LEDAON should be high for HUB power up on xMA and xMB, so in your code snip:

>
> +       switch (get_board_revision()) {
> +       case REVISION_XM_A:
> +       case REVISION_XM_B:
> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);

So this should be TWL4030_LED_LEDEN_LEDBON | TWL4030_LED_LEDEN_LEDAON

> +               break;
> +       default:
> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);

and, this should be TWL4030_LED_LEDEN_LEDBON

Am I correct?

Thanks
Joel
Eric Benard Aug. 27, 2011, 1:51 p.m. UTC | #6 
Hi Joel,

Le 27/08/2011 06:46, Joel A Fernandes a écrit :
>> The logic is wrong here.  I don't remember the polarity, but I do remember that it is
>> xM-A and xM-B that are the oddballs.  I believe this is the correct change:
>
> Hi Jason,
>
> LEDAON should be high for HUB power up on xMA and xMB, so in your code snip:
>
>>
>> +       switch (get_board_revision()) {
>> +       case REVISION_XM_A:
>> +       case REVISION_XM_B:
>> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
>
> So this should be TWL4030_LED_LEDEN_LEDBON | TWL4030_LED_LEDEN_LEDAON
>
>> +               break;
>> +       default:
>> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
>
> and, this should be TWL4030_LED_LEDEN_LEDBON
>
> Am I correct?
>
LEDA & LEDB are active low Open Drain outputs so enabling LEDAON bit in LEDEN 
means LEDA output is at low level.

Eric
Jason Kridner Aug. 27, 2011, 4:19 p.m. UTC | #7 
On Sat, Aug 27, 2011 at 9:51 AM, Eric Bénard <eric@eukrea.com> wrote:
> Hi Joel,
>
> Le 27/08/2011 06:46, Joel A Fernandes a écrit :
>>> The logic is wrong here.  I don't remember the polarity, but I do remember that it is
>>> xM-A and xM-B that are the oddballs.  I believe this is the correct change:
>>
>> Hi Jason,
>>
>> LEDAON should be high for HUB power up on xMA and xMB, so in your code snip:
>>
>>>
>>> +       switch (get_board_revision()) {
>>> +       case REVISION_XM_A:
>>> +       case REVISION_XM_B:
>>> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
>>
>> So this should be TWL4030_LED_LEDEN_LEDBON | TWL4030_LED_LEDEN_LEDAON
>>
>>> +               break;
>>> +       default:
>>> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
>>
>> and, this should be TWL4030_LED_LEDEN_LEDBON
>>
>> Am I correct?
>>
> LEDA & LEDB are active low Open Drain outputs so enabling LEDAON bit in LEDEN
> means LEDA output is at low level.

Thanks for resolving that mystery.  I just figured out what I thought
it should be by looking at the previous default, which should be the
new default as well with only XM_A and XM_B are different.


>
> Eric
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Joel Fernandes Aug. 27, 2011, 7:16 p.m. UTC | #8 
All,

I'm post a v2 now, please give ACKs, thanks :)

On Sat, Aug 27, 2011 at 11:19 AM, Jason Kridner
<jkridner@beagleboard.org> wrote:
> On Sat, Aug 27, 2011 at 9:51 AM, Eric Bénard <eric@eukrea.com> wrote:
>> Hi Joel,
>>
>> Le 27/08/2011 06:46, Joel A Fernandes a écrit :
>>>> The logic is wrong here.  I don't remember the polarity, but I do remember that it is
>>>> xM-A and xM-B that are the oddballs.  I believe this is the correct change:
>>>
>>> Hi Jason,
>>>
>>> LEDAON should be high for HUB power up on xMA and xMB, so in your code snip:
>>>
>>>>
>>>> +       switch (get_board_revision()) {
>>>> +       case REVISION_XM_A:
>>>> +       case REVISION_XM_B:
>>>> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
>>>
>>> So this should be TWL4030_LED_LEDEN_LEDBON | TWL4030_LED_LEDEN_LEDAON
>>>
>>>> +               break;
>>>> +       default:
>>>> +               twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
>>>
>>> and, this should be TWL4030_LED_LEDEN_LEDBON
>>>
>>> Am I correct?
>>>
>> LEDA & LEDB are active low Open Drain outputs so enabling LEDAON bit in LEDEN
>> means LEDA output is at low level.
>
> Thanks for resolving that mystery.  I just figured out what I thought
> it should be by looking at the previous default, which should be the
> new default as well with only XM_A and XM_B are different.
>
>
>>
>> Eric
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
diff mbox

Patch

diff --git a/board/ti/beagle/beagle.c b/board/ti/beagle/beagle.c
index 13fe39b..9d65e9e 100644
--- a/board/ti/beagle/beagle.c
+++ b/board/ti/beagle/beagle.c
@@ -332,7 +332,15 @@  int misc_init_r(void)
 		setenv(expansion_config.env_var, expansion_config.env_setting);
 
 	twl4030_power_init();
-	twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
+	switch (get_board_revision()) {
+	case REVISION_XM_C:
+	case REVISION_C4:
+		twl4030_led_init(TWL4030_LED_LEDEN_LEDAON | TWL4030_LED_LEDEN_LEDBON);
+		break;
+	default:
+		twl4030_led_init(TWL4030_LED_LEDEN_LEDBON);
+		break;
+	}
 
 	/* Set GPIO states before they are made outputs */
 	writel(GPIO23 | GPIO10 | GPIO8 | GPIO2 | GPIO1,