From mboxrd@z Thu Jan 1 00:00:00 1970 From: Remy Bohmer Date: Fri, 13 Aug 2010 11:16:38 +0200 Subject: [U-Boot] [PATCH 2/3] Connect the AT91 UDC to USB CDC-ethernet support In-Reply-To: <20100812205602.46E911536EF@gemini.denx.de> References: <1281634613-2707-1-git-send-email-linux@bohmer.net> <1281634613-2707-2-git-send-email-linux@bohmer.net> <4C644411.6030608@emk-elektronik.de> <20100812205602.46E911536EF@gemini.denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Wolfgang, 2010/8/12 Wolfgang Denk : > Dear Remy Bohmer, > > In message 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