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

[Reinhard Meyer]

Dear Remy Bohmer,
> 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.
> diff --git a/arch/arm/include/asm/arch-at91/at91_dbgu.h b/arch/arm/include/asm/arch-at91/at91_dbgu.h
> new file mode 100644

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

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

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

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

> diff --git a/arch/arm/include/asm/arch-at91/cpu.h b/arch/arm/include/asm/arch-at91/cpu.h
> new file mode 100644
<snip>
> +#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.

<snip>
> +#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>
> +//#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

> + * 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>
> +		hang();
Does this mean the system is dead and would need a watchdog or reset
button to come alive again?

<snip>

> +	// terminer chaque requete dans la queue
C++ comment in unknown language ;)?

<snip>

> +/* TODO:
> + *	BUG_ON(!list_empty(&req->queue));
> + *	kfree(req); */
Wrong multi line comment.

<snip>

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

> +		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);
> +		} 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.

> +	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?)

Best Regards,
Reinhard Meyer