Re: [U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport

2011-12-07 Thread Igor Grinberg
On 12/07/11 20:36, Marek Vasut wrote:
>> Hi Marek,
>>
>> On 12/07/11 19:27, Marek Vasut wrote:
 On 12/07/11 03:42, Simon Glass wrote:
> Hi Igor,
>
> Looks good - a few comments from me.
>
> On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg 
>>>
>>> wrote:
>> From: Jana Rapava 
>>
>> Add partial ULPI specification implementation that should be enough to
>> interface the ULPI PHYs in the boot loader context.
>> Add a viewport implementation for Chipidea/ARC based controllers.
>>
>> Signed-off-by: Jana Rapava 
>> Signed-off-by: Igor Grinberg 
>> Cc: Remy Bohmer 
>> Cc: Stefano Babic 
>> Cc: Wolfgang Grandegger 
>> Cc: Simon Glass 
>> ---
>>
>> [...]
>>
> Just out of interest, is it possible to test this? How would I plumb it
> in?

 Well, from my experience with ULPI hardware,
 I think the controller specific glue looks like the right place
 for putting the ULPI layer calls in.

 In general, the controller code knows which PHYs it supports
 and board code knows which PHY is assembled on the board,
 so it is not that straight simple to find the right place.

 I think, Marek has patches that supposed to use this framework on
 efikamx board.
>>>
>>> I tried using the interface, but the design seems completely wrong :-(
>>> Jana was supposed to design it mainly for the efikamx board, but this
>>> interface is unusable there.
>>
>> May I ask you why?
>> Isn't it because of that nasty VBUS bug efikamx has?
>> You can't say the design is wrong if it is more generic then you want...
> 
> I think it's overengineered. Basically, to achieve what I needed, I either 
> didn't find the right function or I had to use multiple functions. Therefore 
> I 
> had to fall back to plain simple ulpi_read/write().

So now it is plain simple multiple ulpi_read/write() calls?
This is no more as simple as ulpi_() calls...
Instead of forging masks and writing arbitrary values, use
the functional API.

I don't think is is over engineered, because it is simple
and in the same time helps prevent mistakes and most of the cases,
you don't need to be familiar with all the ULPI bits.

>>
>>> I had to fall back to basic ulpi_read()/ulpi_write() calls :-(
>>
>> That's too bad.
>> Because ulpi_{read|write}() is only a viewport implementation and
>> it is not following the ULPI spec.
> 
> Well ... we'll need to rethink this.
>>
>>> I'm afraid we won't make it for .12 release window with this patches ...
>>> very bad :-( I'll try talking to WD if he can push the release window to
>>> allow this
>>
>> Good.
>>
>>> (or redesigned version) in, but I don't know if that's a good idea.
>>
>> I don't think it should be redesigned.
>> Currently, it is generic and abstracts the ULPI specification nicely.
> 
> Nicely maybe, but try using it on top of some hardware that has ULPI chip.

What's the problem? It is better then just using ulpi_read/write() calls...
May be you are just lazy to read the documentation...

> 
>> It can be used on any architecture.
>> I have already stated in the cover letter,
>> what IMO is missing to improve usability, but that will not be a problem.
>>
>> Do you have the efikamx patches somewhere I can look at?
> 
> Submitted to ML just a while ago.
> 
> M
> 

-- 
Regards,
Igor.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport

2011-12-07 Thread Marek Vasut
> Hi Marek,
> 
> On 12/07/11 19:27, Marek Vasut wrote:
> >> On 12/07/11 03:42, Simon Glass wrote:
> >>> Hi Igor,
> >>> 
> >>> Looks good - a few comments from me.
> >>> 
> >>> On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg 
> > 
> > wrote:
>  From: Jana Rapava 
>  
>  Add partial ULPI specification implementation that should be enough to
>  interface the ULPI PHYs in the boot loader context.
>  Add a viewport implementation for Chipidea/ARC based controllers.
>  
>  Signed-off-by: Jana Rapava 
>  Signed-off-by: Igor Grinberg 
>  Cc: Remy Bohmer 
>  Cc: Stefano Babic 
>  Cc: Wolfgang Grandegger 
>  Cc: Simon Glass 
>  ---
> 
> [...]
> 
> >>> Just out of interest, is it possible to test this? How would I plumb it
> >>> in?
> >> 
> >> Well, from my experience with ULPI hardware,
> >> I think the controller specific glue looks like the right place
> >> for putting the ULPI layer calls in.
> >> 
> >> In general, the controller code knows which PHYs it supports
> >> and board code knows which PHY is assembled on the board,
> >> so it is not that straight simple to find the right place.
> >> 
> >> I think, Marek has patches that supposed to use this framework on
> >> efikamx board.
> > 
> > I tried using the interface, but the design seems completely wrong :-(
> > Jana was supposed to design it mainly for the efikamx board, but this
> > interface is unusable there.
> 
> May I ask you why?
> Isn't it because of that nasty VBUS bug efikamx has?
> You can't say the design is wrong if it is more generic then you want...

I think it's overengineered. Basically, to achieve what I needed, I either 
didn't find the right function or I had to use multiple functions. Therefore I 
had to fall back to plain simple ulpi_read/write().
> 
> > I had to fall back to basic ulpi_read()/ulpi_write() calls :-(
> 
> That's too bad.
> Because ulpi_{read|write}() is only a viewport implementation and
> it is not following the ULPI spec.

Well ... we'll need to rethink this.
> 
> > I'm afraid we won't make it for .12 release window with this patches ...
> > very bad :-( I'll try talking to WD if he can push the release window to
> > allow this
> 
> Good.
> 
> > (or redesigned version) in, but I don't know if that's a good idea.
> 
> I don't think it should be redesigned.
> Currently, it is generic and abstracts the ULPI specification nicely.

Nicely maybe, but try using it on top of some hardware that has ULPI chip.

> It can be used on any architecture.
> I have already stated in the cover letter,
> what IMO is missing to improve usability, but that will not be a problem.
> 
> Do you have the efikamx patches somewhere I can look at?

Submitted to ML just a while ago.

M
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport

2011-12-07 Thread Igor Grinberg
Hi Marek,

On 12/07/11 19:27, Marek Vasut wrote:
>> On 12/07/11 03:42, Simon Glass wrote:
>>> Hi Igor,
>>>
>>> Looks good - a few comments from me.
>>>
>>> On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg  
> wrote:
 From: Jana Rapava 

 Add partial ULPI specification implementation that should be enough to
 interface the ULPI PHYs in the boot loader context.
 Add a viewport implementation for Chipidea/ARC based controllers.

 Signed-off-by: Jana Rapava 
 Signed-off-by: Igor Grinberg 
 Cc: Remy Bohmer 
 Cc: Stefano Babic 
 Cc: Wolfgang Grandegger 
 Cc: Simon Glass 
 ---

[...]

>>> Just out of interest, is it possible to test this? How would I plumb it
>>> in?
>>
>> Well, from my experience with ULPI hardware,
>> I think the controller specific glue looks like the right place
>> for putting the ULPI layer calls in.
>>
>> In general, the controller code knows which PHYs it supports
>> and board code knows which PHY is assembled on the board,
>> so it is not that straight simple to find the right place.
>>
>> I think, Marek has patches that supposed to use this framework on efikamx
>> board.
> 
> I tried using the interface, but the design seems completely wrong :-( Jana 
> was 
> supposed to design it mainly for the efikamx board, but this interface is 
> unusable there.

May I ask you why?
Isn't it because of that nasty VBUS bug efikamx has?
You can't say the design is wrong if it is more generic then you want...

> I had to fall back to basic ulpi_read()/ulpi_write() calls :-( 

That's too bad.
Because ulpi_{read|write}() is only a viewport implementation and
it is not following the ULPI spec.

> I'm afraid we won't make it for .12 release window with this patches ... very 
> bad :-( I'll try talking to WD if he can push the release window to allow 
> this 

Good.

> (or redesigned version) in, but I don't know if that's a good idea.

I don't think it should be redesigned.
Currently, it is generic and abstracts the ULPI specification nicely.
It can be used on any architecture.
I have already stated in the cover letter,
what IMO is missing to improve usability, but that will not be a problem.

Do you have the efikamx patches somewhere I can look at?


-- 
Regards,
Igor.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport

2011-12-07 Thread Marek Vasut
> On 12/07/11 03:42, Simon Glass wrote:
> > Hi Igor,
> > 
> > Looks good - a few comments from me.
> > 
> > On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg  
wrote:
> >> From: Jana Rapava 
> >> 
> >> Add partial ULPI specification implementation that should be enough to
> >> interface the ULPI PHYs in the boot loader context.
> >> Add a viewport implementation for Chipidea/ARC based controllers.
> >> 
> >> Signed-off-by: Jana Rapava 
> >> Signed-off-by: Igor Grinberg 
> >> Cc: Remy Bohmer 
> >> Cc: Stefano Babic 
> >> Cc: Wolfgang Grandegger 
> >> Cc: Simon Glass 
> >> ---
> >> 
> >>  Makefile |1 +
> >>  drivers/usb/ulpi/Makefile|   44 ++
> >>  drivers/usb/ulpi/ulpi-viewport.c |  118 +++
> >>  drivers/usb/ulpi/ulpi.c  |  227 +
> >>  include/usb/ulpi.h   |  298
> >>  ++
> > 
> > This would benefit from additions to the README describing the two new
> > CONFIGs you add.
> > 
> >>  5 files changed, 688 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/usb/ulpi/Makefile
> >>  create mode 100644 drivers/usb/ulpi/ulpi-viewport.c
> >>  create mode 100644 drivers/usb/ulpi/ulpi.c
> >>  create mode 100644 include/usb/ulpi.h
> >> 
> >> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> >> new file mode 100644
> >> index 000..805e29d
> >> --- /dev/null
> >> +++ b/drivers/usb/ulpi/ulpi.c
> >> @@ -0,0 +1,227 @@
> >> +/*
> >> + * Copyright (C) 2011 Jana Rapava 
> >> + * Copyright (C) 2011 CompuLab, Ltd. 
> >> + *
> >> + * Authors: Jana Rapava 
> >> + * Igor Grinberg 
> >> + *
> >> + * Based on:
> >> + * linux/drivers/usb/otg/ulpi.c
> >> + * Generic ULPI USB transceiver support
> >> + *
> >> + * Original Copyright follow:
> >> + * Copyright (C) 2009 Daniel Mack 
> >> + *
> >> + * Based on sources from
> >> + *
> >> + *   Sascha Hauer 
> >> + *   Freescale Semiconductors
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +#define ULPI_ID_REGS_COUNT 4
> >> +#define ULPI_TEST_VALUE0x55/* 0x55 == 0b01010101 */
> >> +
> >> +static struct ulpi_regs *ulpi = (struct ulpi_regs *)0;
> >> +
> >> +static int ulpi_integrity_check(u32 ulpi_viewport)
> >> +{
> >> +   u32 err, val, tval = ULPI_TEST_VALUE;
> >> +   int i;
> >> +
> >> +   /* Use the 'special' test value to check all bits */
> >> +   for (i = 0; i < 2; i++, tval <<= 1) {
> >> +   err = ulpi_write(ulpi_viewport, &ulpi->scratch, tval);
> >> +   if (err)
> >> +   return err;
> >> +
> >> +   val = ulpi_read(ulpi_viewport, &ulpi->scratch);
> >> +   if (val != tval) {
> >> +   printf("ULPI integrity check failed\n");
> >> +   return val;
> >> +   }
> >> +   }
> >> +
> >> +   return 0;
> >> +}
> >> +
> >> +int ulpi_init(u32 ulpi_viewport)
> >> +{
> >> +   u32 val, id = 0;
> >> +   u8 *reg = &ulpi->product_id_high;
> >> +   int i;
> >> +
> >> +   /* Assemble ID from four ULPI ID registers (8 bits each). */
> >> +   for (i = 0; i < ULPI_ID_REGS_COUNT; i++) {
> >> +   val = ulpi_read(ulpi_viewport, reg - i);
> >> +   if (val == ULPI_ERROR)
> >> +   return val;
> >> +
> >> +   id = (id << 8) | val;
> >> +   }
> >> +
> >> +   /* Split ID into vendor and product ID. */
> >> +   debug("ULPI transceiver ID 0x%04x:0x%04x\n", id >> 16, id &
> >> 0x); +
> >> +   return ulpi_integrity_check(ulpi_viewport);
> >> +}
> >> +
> >> +int ulpi_select_transceiver(u32 ulpi_viewport, u8 speed)
> > 
> > Is there any point in making the argument u8?
> 
> Not really...
> 
> > How about just unsigned?
> 
> Sounds sane
> 
> > I think this adds a mask to the call = larger code size. You check the
> > arg with the switch() below anyway.
> 
> Haven't thought about that...
> I don't know how is this exactly works, but I don't see
> any reason why shouldn't I use unsigned here.
> 
> >> +{
> >> +   u8 tspeed = ULPI_FC_FULL_SPEED;
> >> +   u32 val;
> >> +
> >> +   switch (speed) {
> >> +   case ULPI_FC_HIGH_SPEED:
> >> +   case ULPI_FC_FULL_SPEED:
> >> +   case ULPI_FC_LOW_SPEED:
> >> +   case ULPI_FC_FS4LS:
> >> +   tspeed = speed;
> >> +   break;
> >> +  

Re: [U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport

2011-12-07 Thread Igor Grinberg
On 12/07/11 03:42, Simon Glass wrote:
> Hi Igor,
> 
> Looks good - a few comments from me.
> 
> On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg  wrote:
>> From: Jana Rapava 
>>
>> Add partial ULPI specification implementation that should be enough to
>> interface the ULPI PHYs in the boot loader context.
>> Add a viewport implementation for Chipidea/ARC based controllers.
>>
>> Signed-off-by: Jana Rapava 
>> Signed-off-by: Igor Grinberg 
>> Cc: Remy Bohmer 
>> Cc: Stefano Babic 
>> Cc: Wolfgang Grandegger 
>> Cc: Simon Glass 
>> ---
>>  Makefile |1 +
>>  drivers/usb/ulpi/Makefile|   44 ++
>>  drivers/usb/ulpi/ulpi-viewport.c |  118 +++
>>  drivers/usb/ulpi/ulpi.c  |  227 +
>>  include/usb/ulpi.h   |  298 
>> ++
> 
> This would benefit from additions to the README describing the two new
> CONFIGs you add.
> 
>>  5 files changed, 688 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/usb/ulpi/Makefile
>>  create mode 100644 drivers/usb/ulpi/ulpi-viewport.c
>>  create mode 100644 drivers/usb/ulpi/ulpi.c
>>  create mode 100644 include/usb/ulpi.h
>>
> 
>> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
>> new file mode 100644
>> index 000..805e29d
>> --- /dev/null
>> +++ b/drivers/usb/ulpi/ulpi.c
>> @@ -0,0 +1,227 @@
>> +/*
>> + * Copyright (C) 2011 Jana Rapava 
>> + * Copyright (C) 2011 CompuLab, Ltd. 
>> + *
>> + * Authors: Jana Rapava 
>> + * Igor Grinberg 
>> + *
>> + * Based on:
>> + * linux/drivers/usb/otg/ulpi.c
>> + * Generic ULPI USB transceiver support
>> + *
>> + * Original Copyright follow:
>> + * Copyright (C) 2009 Daniel Mack 
>> + *
>> + * Based on sources from
>> + *
>> + *   Sascha Hauer 
>> + *   Freescale Semiconductors
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define ULPI_ID_REGS_COUNT 4
>> +#define ULPI_TEST_VALUE0x55/* 0x55 == 0b01010101 */
>> +
>> +static struct ulpi_regs *ulpi = (struct ulpi_regs *)0;
>> +
>> +static int ulpi_integrity_check(u32 ulpi_viewport)
>> +{
>> +   u32 err, val, tval = ULPI_TEST_VALUE;
>> +   int i;
>> +
>> +   /* Use the 'special' test value to check all bits */
>> +   for (i = 0; i < 2; i++, tval <<= 1) {
>> +   err = ulpi_write(ulpi_viewport, &ulpi->scratch, tval);
>> +   if (err)
>> +   return err;
>> +
>> +   val = ulpi_read(ulpi_viewport, &ulpi->scratch);
>> +   if (val != tval) {
>> +   printf("ULPI integrity check failed\n");
>> +   return val;
>> +   }
>> +   }
>> +
>> +   return 0;
>> +}
>> +
>> +int ulpi_init(u32 ulpi_viewport)
>> +{
>> +   u32 val, id = 0;
>> +   u8 *reg = &ulpi->product_id_high;
>> +   int i;
>> +
>> +   /* Assemble ID from four ULPI ID registers (8 bits each). */
>> +   for (i = 0; i < ULPI_ID_REGS_COUNT; i++) {
>> +   val = ulpi_read(ulpi_viewport, reg - i);
>> +   if (val == ULPI_ERROR)
>> +   return val;
>> +
>> +   id = (id << 8) | val;
>> +   }
>> +
>> +   /* Split ID into vendor and product ID. */
>> +   debug("ULPI transceiver ID 0x%04x:0x%04x\n", id >> 16, id & 0x);
>> +
>> +   return ulpi_integrity_check(ulpi_viewport);
>> +}
>> +
>> +int ulpi_select_transceiver(u32 ulpi_viewport, u8 speed)
> 
> Is there any point in making the argument u8?

Not really...

> How about just unsigned?

Sounds sane

> I think this adds a mask to the call = larger code size. You check the
> arg with the switch() below anyway.

Haven't thought about that...
I don't know how is this exactly works, but I don't see
any reason why shouldn't I use unsigned here.

> 
>> +{
>> +   u8 tspeed = ULPI_FC_FULL_SPEED;
>> +   u32 val;
>> +
>> +   switch (speed) {
>> +   case ULPI_FC_HIGH_SPEED:
>> +   case ULPI_FC_FULL_SPEED:
>> +   case ULPI_FC_LOW_SPEED:
>> +   case ULPI_FC_FS4LS:
>> +   tspeed = speed;
>> +   break;
>> +   default:
>> +   printf("ULPI: %s: wrong transceiver speed specified, "
>> +   "falling back to full speed\n", __func__);
>> +   }
>> +
>> +   val = ulpi_read(ulpi_viewport, &ulpi->function_ctrl);
>> +   if (val == ULPI_ERROR)
>> +   return val;

Re: [U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport

2011-12-06 Thread Simon Glass
Hi Igor,

Looks good - a few comments from me.

On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg  wrote:
> From: Jana Rapava 
>
> Add partial ULPI specification implementation that should be enough to
> interface the ULPI PHYs in the boot loader context.
> Add a viewport implementation for Chipidea/ARC based controllers.
>
> Signed-off-by: Jana Rapava 
> Signed-off-by: Igor Grinberg 
> Cc: Remy Bohmer 
> Cc: Stefano Babic 
> Cc: Wolfgang Grandegger 
> Cc: Simon Glass 
> ---
>  Makefile                         |    1 +
>  drivers/usb/ulpi/Makefile        |   44 ++
>  drivers/usb/ulpi/ulpi-viewport.c |  118 +++
>  drivers/usb/ulpi/ulpi.c          |  227 +
>  include/usb/ulpi.h               |  298 
> ++

This would benefit from additions to the README describing the two new
CONFIGs you add.

>  5 files changed, 688 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/usb/ulpi/Makefile
>  create mode 100644 drivers/usb/ulpi/ulpi-viewport.c
>  create mode 100644 drivers/usb/ulpi/ulpi.c
>  create mode 100644 include/usb/ulpi.h
>

> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> new file mode 100644
> index 000..805e29d
> --- /dev/null
> +++ b/drivers/usb/ulpi/ulpi.c
> @@ -0,0 +1,227 @@
> +/*
> + * Copyright (C) 2011 Jana Rapava 
> + * Copyright (C) 2011 CompuLab, Ltd. 
> + *
> + * Authors: Jana Rapava 
> + *         Igor Grinberg 
> + *
> + * Based on:
> + * linux/drivers/usb/otg/ulpi.c
> + * Generic ULPI USB transceiver support
> + *
> + * Original Copyright follow:
> + * Copyright (C) 2009 Daniel Mack 
> + *
> + * Based on sources from
> + *
> + *   Sascha Hauer 
> + *   Freescale Semiconductors
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#define ULPI_ID_REGS_COUNT     4
> +#define ULPI_TEST_VALUE                0x55    /* 0x55 == 0b01010101 */
> +
> +static struct ulpi_regs *ulpi = (struct ulpi_regs *)0;
> +
> +static int ulpi_integrity_check(u32 ulpi_viewport)
> +{
> +       u32 err, val, tval = ULPI_TEST_VALUE;
> +       int i;
> +
> +       /* Use the 'special' test value to check all bits */
> +       for (i = 0; i < 2; i++, tval <<= 1) {
> +               err = ulpi_write(ulpi_viewport, &ulpi->scratch, tval);
> +               if (err)
> +                       return err;
> +
> +               val = ulpi_read(ulpi_viewport, &ulpi->scratch);
> +               if (val != tval) {
> +                       printf("ULPI integrity check failed\n");
> +                       return val;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +int ulpi_init(u32 ulpi_viewport)
> +{
> +       u32 val, id = 0;
> +       u8 *reg = &ulpi->product_id_high;
> +       int i;
> +
> +       /* Assemble ID from four ULPI ID registers (8 bits each). */
> +       for (i = 0; i < ULPI_ID_REGS_COUNT; i++) {
> +               val = ulpi_read(ulpi_viewport, reg - i);
> +               if (val == ULPI_ERROR)
> +                       return val;
> +
> +               id = (id << 8) | val;
> +       }
> +
> +       /* Split ID into vendor and product ID. */
> +       debug("ULPI transceiver ID 0x%04x:0x%04x\n", id >> 16, id & 0x);
> +
> +       return ulpi_integrity_check(ulpi_viewport);
> +}
> +
> +int ulpi_select_transceiver(u32 ulpi_viewport, u8 speed)

Is there any point in making the argument u8? How about just unsigned?
I think this adds a mask to the call = larger code size. You check the
arg with the switch() below anyway.

> +{
> +       u8 tspeed = ULPI_FC_FULL_SPEED;
> +       u32 val;
> +
> +       switch (speed) {
> +       case ULPI_FC_HIGH_SPEED:
> +       case ULPI_FC_FULL_SPEED:
> +       case ULPI_FC_LOW_SPEED:
> +       case ULPI_FC_FS4LS:
> +               tspeed = speed;
> +               break;
> +       default:
> +               printf("ULPI: %s: wrong transceiver speed specified, "
> +                       "falling back to full speed\n", __func__);
> +       }
> +
> +       val = ulpi_read(ulpi_viewport, &ulpi->function_ctrl);
> +       if (val == ULPI_ERROR)
> +               return val;
> +
> +       /* clear the previous speed setting */
> +       val = (val & ~ULPI_FC_XCVRSEL_MASK) | tspeed;
> +
> +       return ulpi_write(ulpi_viewport, &ulpi->function_ctrl, val);
> +}
> +
> +int ulpi_set_vbus(u32 ulpi_viewport, int on, int ext_power, int ext_ind)
> +{
> +       u32 flags = ULPI_OTG_DRVVBUS;
> +       u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi

Re: [U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport

2011-12-06 Thread Marek Vasut
> From: Jana Rapava 
> 
> Add partial ULPI specification implementation that should be enough to
> interface the ULPI PHYs in the boot loader context.
> Add a viewport implementation for Chipidea/ARC based controllers.
> 
> Signed-off-by: Jana Rapava 
> Signed-off-by: Igor Grinberg 
> Cc: Remy Bohmer 
> Cc: Stefano Babic 
> Cc: Wolfgang Grandegger 
> Cc: Simon Glass 

For some reason, I don't see Remy in the Cc of the mail (in the email headers). 
Detlev, can you comment on that please?
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot