Integrate USB gadget layer and USB CDC driver layer

arch/arm/include/asm/arch-at91/cpu.h new file +0 -0

new file mode 100644

+ * This driver expects the board has been wired with two GPIOs suppporting
+ * a VBUS sensing IRQ, and a D+ pullup. (They may be omitted, but the
+ * testing hasn't covered such cases.)

+/* TODO:
+ * BUG_ON(!list_empty(&req->queue));
+ * kfree(req); */

+static void pullup(struct at91_udc *udc, int is_on)

Dear Remy Bohmer,

arch/arm/include/asm/arch-at91/at91_dbgu.h

Quoted 6 lines

This patch implements the low-level part of the USB CDC layer for AT91 Other boards can use this patch as an example to connect their own.

arch/arm/include/asm/arch-at91/at91_dbgu.h new file +0 -0

new file mode 100644

Please see my patch introducing at91_dbgu.h (4/5.July 2010)

arch/arm/include/asm/arch-at91/at91_dbgu.h

Quoted 3 lines

arch/arm/include/asm/arch-at91/at91_dbgu.h new file +3 -0

+#define AT91_DBGU_CR ? ? ? ? ? (AT91_DBGU + 0x00) /* Control Register */
+#define AT91_DBGU_MR ? ? ? ? ? (AT91_DBGU + 0x04) /* Mode Register */
+#define AT91_DBGU_IER ? ? ? ? ?(AT91_DBGU + 0x08) /* Interrupt Enable */

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.

Quoted 4 lines

+#define ? ? ? ? ? ? ? ?AT91_DBGU_TXRDY ? ? ? ? (1 << 1) ? /* Transmitter Ready */
+#define ? ? ? ? ? ? ? ?AT91_DBGU_TXEMPTY ? ? ? (1 << 9) ? /* Transmitter Empty */

Don't define this here, UART handling is in drivers/serial/atmel_usart.? The first 9 registers in the DBGU are identical to the normal USARTS.

arch/arm/include/asm/arch-at91/at91_dbgu.h

Quoted 5 lines

arch/arm/include/asm/arch-at91/at91_dbgu.h new file +5 -0

+#define ? ? ? ? ? ? ? ? ? ? ? ?AT91_CIDR_SRAMSIZ_1K ? ?(1 << 16)
+#define ? ? ? ? ? ? ? ? ? ? ? ?AT91_CIDR_SRAMSIZ_2K ? ?(2 << 16)
+#define ? ? ? ? ? ? ? ? ? ? ? ?AT91_CIDR_SRAMSIZ_112K ?(4 << 16)
+#define ? ? ? ? ? ? ? ? ? ? ? ?AT91_CIDR_SRAMSIZ_4K ? ?(5 << 16)
+#define ? ? ? ? ? ? ? ? ? ? ? ?AT91_CIDR_SRAMSIZ_80K ? (6 << 16)

I think we should not define stuff that is nowhere used, and if one space or tab will do.

Quoted 3 lines

arch/arm/include/asm/arch-at91/cpu.h new file +0 -0

new file mode 100644

<snip>

Quoted 1 line

arch/arm/include/asm/arch-at91/cpu.h line 18

Quoted 3 lines

arch/arm/include/asm/arch-at91/cpu.h new file +3 -0

+#include <asm/hardware.h>
+#include <asm/arch/at91_dbgu.h>
+#include <asm/arch/io.h>

Don't include other header files at this point. If a c source requires both DBGU and CPU headers, it shall include both.

arch/arm/include/asm/arch-at91/cpu.h

Quoted 6 lines

arch/arm/include/asm/arch-at91/cpu.h new file +6 -0

+#define ARCH_ID_AT91RM9200 ? ? 0x09290780
+#define ARCH_ID_AT91SAM9260 ? ?0x019803a0
+#define ARCH_ID_AT91SAM9261 ? ?0x019703a0
+#define ARCH_ID_AT91SAM9263 ? ?0x019607a0
+#define ARCH_ID_AT91SAM9RL64 ? 0x019b03a0
+#define ARCH_ID_AT91CAP9 ? ? ? 0x039A03A0

Nice to have, but determining at runtime which variant we are on makes only sense to me for pin-compatible variants, e.g. 9260, 9xe, 9g20. Then you will notice that their hardware is not different much, if at all.
Non pin-compatible variants are #defined in their board specific header file. Use #ifdef in the source to handle necessary variant specific stuff...

<snip>

arch/arm/include/asm/arch-at91/cpu.h

Quoted 4 lines

arch/arm/include/asm/arch-at91/cpu.h new file +3 -0

+//#define DEBUG 1
+//#define VERBOSE 1
+//#define PACKET_TRACE 1

NO C++ comments allowed. And no commented away defines in general. Maybe say it like this:
/*
?* to switch on debug output define any of those:
?* DEBUG - general debug
?* VERBOSE - more debug
?* PACKET_TRACE - also print packets
?*/

wherever possible use the debug() macro defines in common.h

Quoted 4 lines

+ * This driver expects the board has been wired with two GPIOs suppporting
+ * a VBUS sensing IRQ, and a D+ pullup. ?(They may be omitted, but the
+ * testing hasn't covered such cases.)

It should. Besides AT91 currently cannot handle IRQs. Also I think it should read D- pullup.

<snip>

Quoted 2 lines

+ ? ? ? ? ? ? ? hang();

Does this mean the system is dead and would need a watchdog or reset button to come alive again?

<snip>

Quoted 1 line

+ ? ? ? // terminer chaque requete dans la queue

C++ comment in unknown language ;)?

<snip>

Quoted 3 lines

+/* TODO:
+ * ? ? BUG_ON(!list_empty(&req->queue));
+ * ? ? kfree(req); */

Wrong multi line comment.

<snip>

Quoted 1 line

+static void pullup(struct at91_udc *udc, int is_on)

<snip>

I suggest to define and use a weak function to dummy on/off the pullup.
Board specific source can implement a function then to really switch on/off a pullup in whatever way the hardware has provided that. That need not necessarily be a GPIO pin...

Quoted 7 lines

+ ? ? ? ? ? ? ? if (cpu_is_at91rm9200())
+ ? ? ? ? ? ? ? ? ? ? ? at91_set_gpio_value(udc->board.pullup_pin, 1);
+ ? ? ? ? ? ? ? else if (cpu_is_at91sam9260() || cpu_is_at91sam9263()) {
+ ? ? ? ? ? ? ? ? ? ? ? u32 ? ? txvc = at91_udp_read(udc, AT91_UDP_TXVC); +
+ ? ? ? ? ? ? ? ? ? ? ? txvc |= AT91_UDP_TXVC_PUON;
+ ? ? ? ? ? ? ? ? ? ? ? at91_udp_write(udc, AT91_UDP_TXVC, txvc);

Quoted 6 lines

+ ? ? ? ? ? ? ? } else if (cpu_is_at91sam9261()) {
+ ? ? ? ? ? ? ? ? ? ? ? u32 ? ? usbpucr;
+
+ ? ? ? ? ? ? ? ? ? ? ? usbpucr = at91_sys_read(AT91_MATRIX_USBPUCR);
+ ? ? ? ? ? ? ? ? ? ? ? usbpucr |= AT91_MATRIX_USBPUCR_PUON;
+ ? ? ? ? ? ? ? ? ? ? ? at91_sys_write(AT91_MATRIX_USBPUCR, usbpucr);

Using a board specific function will rid you of that cascade as well.

Quoted 3 lines

+ ? ? ? u32 __iomem ? ? *creg = ep->creg;
+ ? ? ? u8 __iomem ? ? ?*dreg = ep->creg + (AT91_UDP_FDR(0) - AT91_UDP_CSR(0));

Always use readl/writel to access hardware.

There are more coding style problems in the source, but for source pulled from the kernel that might be ok (Wolfgang?)

Quoted 4 lines

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?

Quoted 2 lines

Don't define this here, UART handling is in drivers/serial/atmel_usart.? The first 9 registers in the DBGU are identical to the normal USARTS.

Indeed, but again, it is copied from Linux...

Quoted 2 lines

Don't include other header files at this point. If a c source requires both DBGU and CPU headers, it shall include both.

This is how the file looks like in Linux...

See Linux...

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.

Quoted 4 lines

Quoted 1 line

+ hang();

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

Quoted 3 lines

Quoted 1 line

+ // terminer chaque requete dans la queue

C++ comment in unknown language ;)?

Linux code...

Quoted 2 lines

There are more coding style problems in the source, but for source pulled from the kernel that might be ok (Wolfgang?)

I understand your remarks, and for new written code they are 100% valid to me (I would even not have posted it at all like this), however, most of this code comes from Linux. It is a port to U-boot. True, things can be done differently, but that would make future syncing and integration of new drivers much more difficult.

3. I did not follow the whole USB patches, my concern is what touches AT91, but please explain to me what for we need a board under u-boot to be a USB device. Since u-boot per se is not multitasking how is the traffic handled while at the command prompt?

Which leads to:
4. Which hardware did you test that on, and what device functionality?

Quoted 5 lines

3. I did not follow the whole USB patches, my concern is what touches AT91, but please explain to me what for we need a board under u-boot to be a USB device. Since u-boot per se is not multitasking how is the traffic handled while at the command prompt?

There is no traffic handled while at the command prompt. It behaves just like any other 'normal' network adapter.
So, it brings up a USB connection once you start a network action like a ping or tftp command. Once that completes the link is brought down again.

Dear Remy Bohmer,

Quoted 7 lines

Quoted 6 lines

3. I did not follow the whole USB patches, my concern is what touches AT91, but please explain to me what for we need a board under u-boot to be a USB device. Since u-boot per se is not multitasking how is the traffic handled while at the command prompt?

Quoted 5 lines

There is no traffic handled while at the command prompt. It behaves just like any other 'normal' network adapter.
So, it brings up a USB connection once you start a network action like a ping or tftp command. Once that completes the link is brought down again.

If I understand the procedure correctly now, its as follows:

Any client network command on u-boot's command prompt like ping, dhcp, tftp etc. will 'up' the USB Device...
The host to which it is connected to will happily discover and enumerate a network adapter with MAC and IP addresses...

And the host will be the server for that command...

Dear Remy Bohmer,

Quoted 1 line · In message <[email protected]> you…

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

Indeed, but again, it is copied from Linux...

Maybe we can do it better?

Quoted 5 lines

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.

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;

Quoted 4 lines

Quoted 2 lines

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?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ ? ? ? // terminer chaque requete dans la queue

C++ comment in unknown language ;)?

Linux code...

And that justifies it all.

Quoted 3 lines

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

Quoted 4 lines

Quoted 1 line

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

Quoted 1 line

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.

Dear Remy Bohmer,

Well, if your argument is true that the current code can easily be kept in sync with Linux, then why is it already 8 kernel releases old?

How about syncing it with Linux, before adding it to U-Boot?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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

That's not a good idea. ?Please fix.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

So broken that you cannot get back to the command line interpreter? Really?

Quoted 3 lines

Quoted 1 line

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

That's not a good idea. =A0Please fix.

Fix what?
If runtime a condition is detected that should _never_ _ever_ happen (like malloc does not work properly for some reason), which would even make Linux OOPS or panic may not result in a hang() on U-boot?

In that case I can remove the hang() and consider if it never happened. Maybe even the code will reach the prompt, but there will be no guarantees...

I personally consider that worse compared to tell the user that something serious has gone wrong...