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

[Remy Bohmer]

Hi,

2010/8/12 Reinhard Meyer <u-boot at emk-elektronik.de>:
> 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)

Hmm, this one does not seem to be in mainline yet. If it was, I
probably would have used that one ;-)

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

Most of the code is copied from Linux. Do you really think we should
rewrite it all?

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

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

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

Except for easy synching with Linux code?

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

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

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

See Linux...

>
> <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
>  */

OK. This is a left-over that I do not seem to have cleaned up.
I usually use // during development, then I can easily find what to
clean up later...

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

Even in code with Linux as origin?

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

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.

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

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

Linux code...

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

OK

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

That is doing things differently compared to Linux...

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

Agree, but that means doing things differently compared to Linux...
(the origin of this code)

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

See Linux

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

Kind regards,

Remy