Hi,
2010/8/12 Reinhard Meyer u-boot@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