[U-Boot] [PATCH 2/3] Connect the AT91 UDC to USB CDC-ethernet support

[Remy Bohmer]

Hi Wolfgang,

2010/8/12 Wolfgang Denk <wd at denx.de>:
> Dear Remy Bohmer,
>
> In message <AANLkTi=z0SX9YrXgH-_ogcPdUXhyrHADNErHVVBoNW7L at mail.gmail.com> you wrote:
>>
>> > Use struct access for SoC registers. Offset adressing is depreciated. See
>> > the
>> > struct in my at91_dbgu.h. Use readl/writel to access the hardware.
>>
>> Most of the code is copied from Linux. Do you really think we should
>> rewrite it all?
>
> Indeed. Using I/O accessors is mandatory in U-Boot.

Ok, but here ends the goal for easy synching with the kernel. In that
case we can write our own USB gadget layer

>> Indeed, but again, it is copied from Linux...
>
> Maybe we can do it better?

As mentioned: we can, but I did not want to at the time of writing that code.

>> This file is copied from Linux. That was the goal of this port; to be
>> able to integrate new devices and fixes easier in the future. It was
>> not our goal to make it nicer compared to Linux. If the comments are
>> wrong, I would suggest to fix it in Linux first after which we
>> integrate it in U-boot.
>
> Come on.  Here a bit statistics:
>
> -> diff -u linux/drivers/usb/gadget/at91_udc.c u-boot/drivers/usb/gadget/at91_udc.c | wc -l
> 1057
> -> wc -l linux/drivers/usb/gadget/at91_udc.c u_boot/drivers/usb/gadget/at91_udc.c
>  1908 linux/drivers/usb/gadget/at91_udc.c
>  1553 u-boot/drivers/usb/gadget/at91_udc.c
>
> There are so many lines changed already in the code that claoming
> "This file is copied from Linux." does not make much sense any more.

Well, it is derived from 2.6.27, not latest git.
True, there are differences, but mostly it is a strip down of the
original code( /proc support is removed for example)
And I also noticed that the file can be synced to Linux more than it
currently is...

> And many of the code changes are to the worse actually.
>
> For example:
>
>        -       ep->is_in = usb_endpoint_dir_in(desc);
>        +       ep->is_in = (desc->bEndpointAddress & USB_DIR_IN) != 0;

This was NOT the case in 2.6.27. So, it is not worse compared to 2.6.27...

>> > Does this mean the system is dead and would need a watchdog or reset
>> > button to come alive again?
>>
>> Yep, if that happens things are really broken...
>
> So broken that you cannot get back to the command line interpreter?
> Really?

If malloc is broken, I consider that reason enough for hang/reboot...

>> >> +       // terminer chaque requete dans la queue
>> >
>> > C++ comment in unknown language ;)?
>>
>> Linux code...
>
> And that justifies it all.

No, but please look at the overall goal here. It was the goal to use
the Linux code as much as-is, and to allow easy merging new driver
code in the future. Everything can be made better, but in that case I
would prefer to first make the origin better, and port that back to
U-boot.


Further, I am not planning to rewrite the code to the struct based
register access _right now_, since the struct based access mechanism
is somehow work in progress that is not finalised yet
(CONFIG_AT91_LEGACY is still defined for at91sam9261ek). I am not
going to shoot on a moving duck. Also because you mention that
at91_dbgu.h patches float around that  are not in mainline yet. I will
probably clean it up later when things are stabilised.

So, if you prefer I can drop patch 2/3 and 3/3 from the series, since
those are only needed to prove that patch 1/3 works and to have at
least 1 board in U-boot that make use of the USB-CDC-ECM gadget
layer... I expect that there will be other boards soon that will use
it, since others are working on it right now.

Kind regards,

Remy