[U-Boot] [PATCH v2 2/4] USB DFU driver added
Remy Bohmer
linux at bohmer.net
Tue Feb 15 20:34:22 CET 2011
Hi,
2011/2/13 Marcel Janssen <korgull at home.nl>:
> From: Marcel <korgull at home.nl>
>
> USB DFU driver cleaning phase1
>
> USB DFU driver cleaning phase2
>
> USB DFU driver cleaning phase3
>
> USB DFU driver cleaning phase4
Not a very descriptive patch header. Please fix this.
> Signed-off-by: Marcel <korgull at home.nl>
> ---
> common/update_dfu.c | 89 +++
> doc/README.dfu | 129 ++++
> drivers/usb/gadget/Makefile | 9 +
> drivers/usb/gadget/usbdfu.c | 1336 +++++++++++++++++++++++++++++++++++++++++
> include/usb_dfu.h | 128 ++++
> include/usb_dfu_descriptors.h | 100 +++
> 6 files changed, 1791 insertions(+), 0 deletions(-)
> create mode 100644 common/update_dfu.c
> create mode 100644 doc/README.dfu
> create mode 100644 drivers/usb/gadget/usbdfu.c
> create mode 100644 include/usb_dfu.h
> create mode 100644 include/usb_dfu_descriptors.h
> diff --git a/doc/README.dfu b/doc/README.dfu
> new file mode 100644
> index 0000000..363c7a2
> --- /dev/null
> +++ b/doc/README.dfu
> @@ -0,0 +1,129 @@
> +USBD DFU mode
> +
> +Initially written by Marcel Janssen, Admesy B.V.
> +Based on parts from OpenMoko, ether.c and update.c
> +
> +
No useless double empty lines (globally)
> +========================================
> +
> +This describes the DFU implementation in u-boot.
> +
> +The implementation works with dfu-utils to upgrade NAND partitions defined by
> +mtdparts.
> +The board configuration file needs serveral CONFIG options to be set.
> +DFU is implemented to be executed as a command "dfu" (common/update_dfu.c).
> +This command should start the USB device controller and the DFU driver.
> +A typical implementation would be that a script is executed, that will check
> +whether DFU should be started. If so, it can execute 'dfu" and the device will
> +announce itself to the host as a DFU capable device.
> +dfu-util can than be used to upgrade the partitions defined by mtdparts.
> +
> +Description of flow :
> +dfu-utils sets the alternate interface which corresponds to the selected
> +partition.
> +The file (uImage, rootfs.arm.jffs2) is loaded fully to RAM first.
> +U-boot nand routines are used to write from RAM to NAND.
> +
> +LED usage :
> +Status LED's can be defined to show DFU action.
> +Define the RED and GREEN leds to make this happen.
LEDs are board specific, please do not use that in generic driver code.
> +
> +Initial testing example :
> +This was done on the in-circuit icnova_sam9g45 board.
> +This board uses atmel_usbd_udc.c
> +
> +========================================
> +
> +
> +
useles empty lines
> +To make DFU work you need a working USB controller, for example at91_udc or
> +atmel_usba_udc. Make sure to set it in the board config file.
> +
> +========================================
> +USBD CONFIG options
> +----------------------------------------
> +
> +#define CONFIG_USB_GADGET
> +#define CONFIG_USB_GADGET_ATMEL_USBA (or #define CONFIG_USB_GADGET_AT91_UDC )
> +#define CONFIG_USB_GADGET_DUALSPEED
> +
> +----------------------------------------
> +USBD CONFIG options end
> +========================================
> +
> +
empty lines
> +========================================
> +DFU CONFIG options
> +----------------------------------------
> +
> +#define CONFIG_USBD_DFU 1
> +#ifdef CONFIG_USBD_DFU
> +#define CONFIG_USBD_VENDORID 0x23CF /* Admesy */
> +#define CONFIG_USBD_PRODUCTID_DFU 0x0100 /* donated number */
> +#define CONFIG_USBD_MANUFACTURER "Admesy"
> +#define CONFIG_USBD_PRODUCT_NAME "Admesy DFU 001"
> +#define CONFIG_USBD_DFU_XFER_SIZE 4096 /* Buffer size */
> +#define CONFIG_USBD_DFU_INTERFACE 0
> +#define DFU_NUM_ALTERNATES 3 /* 3 partitions */
> +#define LOAD_ADDR ((unsigned char *)0x70400000) /* RAM address to use */
> +#endif
> +
> +----------------------------------------
> +DFU CONFIG options end
> +========================================
> +
> +
> +
empty lines
> +In order to make DFU work with dfu-utils, mtdparts need to be defined.
> +See the example futher below on how to do this.
> +
> +========================================
> +mtdparts example with dfu-utils
> +----------------------------------------
> +
> +mtdparts add nand0 0x2000000 kernel
> +mtdparts add nand0 0x1000000 at 0x00200000 root
> +mtdparts add nand0 0xEE00000 at 0x01200000 data
> +saveenv
> +
> +Your mtdparts than should look like this :
> +
> +board> mtdparts
> +device nand0 <nand.0>, # parts = 3
> + #: name size offset mask_flags
> + 0: kernel 0x00200000 0x00000000 0
> + 1: root 0x01000000 0x00200000 0
> + 2: data 0x0ee00000 0x01200000 0
> +
> +active partition: nand0,0 - (kernel) 0x00200000 @ 0x00000000
> +
> +
> +After the mtdparts have been defined, dfu-utils can be used to upgrade the
> +kernel and root partition.
> +Make sure you have read/write access to the DFU device !
> +
> +cd dfu-utils/src
> +./dfu-util -a0 -D uImage
> +./dfu-util -a1 -D rootfs.arm.jffs2 -R
> +
> +----------------------------------------
> +mtdparts example end
> +========================================
> +
> +
> +
empty lines
> +Possible changes for the near future :
> +1) integrate DFU and Ethernet and/or tty by making it a composite driver.
> +2) allow partitions larger than RAM by implementing per-buffer NAND writing.
> + Openmoko does this, but I didn't need it and liked to write to RAM first.
> +3) Allow DFU to flash NOR (could be added to handle_dnload )
> +
> +
> +
> +
> +
> +
> +
> +
> +
> +
enough said....
> diff --git a/drivers/usb/gadget/usbdfu.c b/drivers/usb/gadget/usbdfu.c
> new file mode 100644
> index 0000000..65f334a
> --- /dev/null
> +++ b/drivers/usb/gadget/usbdfu.c
> +#include <linux/ctype.h>
> +#include <linux/porting-compat.h>
> +
> +#include <nand.h>
> +#include <jffs2/load_kernel.h>
> +
> +extern struct list_head devices;
No extern's please
> +
> +#define RET_NOTHING 0
> +#define RET_ZLP 1
> +#define RET_STALL 2
> +
> +#ifndef CONFIG_USBD_DFU_XFER_SIZE
> +#define CONFIG_USBD_DFU_XFER_SIZE 4096
> +#endif
> +
> +/* this should be done differently */
> +#define EP0_MAX_PACKET_SIZE 64 /* MUSB_EP0_FIFOSIZE */
> +
> +unsigned char ledcount;
Drop this led support
> +#define USB_CONNECT_TIMEOUT (30 * CONFIG_SYS_HZ)
> +enum dfu_state *system_dfu_state; /* for 3rd parties */
> +
> +char rtm_dfu;
If used only locally n this file, please make it 'static'
> +#define LOAD_ADDR ((unsigned char *)0x70400000)
My board does not have RAM on this location...
And even if there is RAM, How do you know it is not occupied by something else?
> +struct dnload_state {
> + nand_info_t *nand;
> + struct part_info *part;
> + unsigned int part_net_size; /* net size (excl. bad blocks) */
> + nand_erase_options_t erase_opts;
> + nand_write_options_t write_opts;
> + nand_read_options_t read_opts;
> + unsigned char *ptr; /* pointer to next empty byte in buffer */
> + unsigned int off; /* offset of current erase page in flash chip*/
> + unsigned char *buf; /* pointer to allocated erase page buffer */
> + /* unless doing an atomic transfer, we use the static buffer below.
> + * This saves us from having to clean up dynamic allications in the
> + * various error paths of the code. Also, it will always work, no
> + * matter what the memory situation is. */
> + unsigned char _buf[0x20000]; /*FIXME : depends on flash page size */
Then use malloc for this
> + if (first) {
> + /* Make sure that we have a valid mtd partition table */
> + char *mtdp = getenv("mtdparts");
Why directly write to flash?
Just load the image first to RAM, from which the user can store it to
flash. (And if the user can script that it would work on more
configurations)
You seem to mix up driver code with some type of application.
> + if (mtdp)
> + printf("Valid MTD partitions found\n");
> + /*this used to be in the Openmoko driver */
> + /*if (!mtdp)
> + /*run_command("dynpart", 0); */
No dead code please.
> + else {
> + dev->dfu_state = DFU_STATE_dfuERROR;
> + dev->dfu_status = DFU_STATUS_errADDRESS;
Const defines in capitals please.
> + return RET_STALL;
> + }
> + }
> +
> + if (len == 0) {
> + debug("zero-size write -> MANIFEST_SYNC ");
> + dev->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
> +
> + /* cleanup */
> + switch (dev->alternate) {
> + char buf[12];
> + /* we don't actually handle partitions differently
Bad multiline comment
> + * the original Openmoko driver did use the alternate
> + * setting to do different things we have no need for
> + * that a big difference is that our driver first reads
> + * the full firmware to RAM and than writes it to flash
> + */
> + default:
> + sprintf(buf, "0x%08x", ds->ptr - ds->buf);
> + addr = (ulong)LOAD_ADDR;
> + rwsize = ds->ptr - ds->buf;
> + printf("Filesize = %s\n", buf);
> + setenv("filesize", buf);
> + rc = 0;
> + rc = mtdparts_init();
> + if (rc) {
> + printf("Error initilizing mtdparts\n");
> + break;
> + }
> + ds->part = get_partition_nand(dev->alternate);
> + if (ds->part == NULL) {
> + printf("Error reading partition info\n");
> + break;
> + }
What if I do not have NAND on my board...
Remove this mtd related code, use store in RAM, let the user decide
how to handle it from there.
> +
> + ds->erase_opts.offset = ds->part->offset;
> + ds->erase_opts.length = ds->part->size;
> + if (!rc && !strcmp(ds->part->name, "root"))
> + ds->erase_opts.jffs2 = 1;
> + else
> + ds->erase_opts.jffs2 = 0;
> + ds->erase_opts.quiet = 0;
> + ds->erase_opts.spread = 1;
> + ds->erase_opts.scrub = 0; /* do not allow this */
> + ds->nand = &nand_info[ds->part->dev->id->num];
> +
> + printf("Using partition : %s ,", ds->part->name);
The partitioning of my board is probably different than you expect.
> + if (ds->erase_opts.jffs2)
> + printf(" with jffs2 option\n");
> + else
> + printf(" without jffs2 option\n");
> + printf("Partition size=%lx offset=%lx\n",
> + (unsigned long)ds->part->size,
> + (unsigned long)ds->part->offset);
> +#ifdef CONFIG_GREEN_LED
> + green_LED_off();
> +#endif
> +#ifdef CONFIG_RED_LED
> + red_LED_off();
> +#endif
No LED control please. My board could contain a single led which my be
used for different purposes.
> + rc = nand_erase_opts(ds->nand, &ds->erase_opts);
> + if (rc) {
> + printf("NAND erase failed\n");
> + break;
> + } else
> + printf("NAND erased succesfully\n");
> +#ifdef CONFIG_GREEN_LED
> + green_LED_on();
> +#endif
> +#ifdef CONFIG_RED_LED
> + red_LED_off();
> +#endif
> + rc = nand_write_skip_bad(ds->nand,
> + ds->part->offset,
> + &rwsize,
> + (u_char *)addr);
Hmm, I might want it in a UBIFS or Yaffs partition... Get the picture?
> + /* Handle vendor specific code
Bad multiline comment
> + * this is an example how to reset u-boot by sending a
An example does not belong in generic driver code.
> + * control message. This can be useful in case
> + * the USB reset fails for whatever reason.
> + */
> + if ((requesttype == 0x41) && (request == 0x08)) {
> + printf("Device reset called\n");
> + do_reset(NULL, 0, 0, NULL);
> + }
> + switch (request) {
> +
> + case USB_REQ_GET_DESCRIPTOR:
No empty line.
> + if (ctrl->bRequestType != USB_DIR_IN)
> + break;
> +
> + switch (wValue >> 8) {
> +
> + case USB_DT_DEVICE:
Apparently globally: no empty lines
> + default:
> + printf("unknown control req%02x.%02x v%04x i%04x l%d\n",
> + ctrl->bRequestType, ctrl->bRequest,
> + wValue, wIndex, wLength);
> + return DFU_EP0_UNHANDLED;
> + break;
> + }
> +
> + }
Indentation garbage... (2 closing brackets on the same indentation)
> diff --git a/include/usb_dfu.h b/include/usb_dfu.h
> new file mode 100644
> index 0000000..e232a7e
> --- /dev/null
> +++ b/include/usb_dfu.h
> +extern enum dfu_state *system_dfu_state; /* for 3rd parties */
Why globally export data structures?
> +#define DFU_STATUS_OK 0x00
> +#define DFU_STATUS_errTARGET 0x01
> +#define DFU_STATUS_errFILE 0x02
> +#define DFU_STATUS_errWRITE 0x03
> +#define DFU_STATUS_errERASE 0x04
> +#define DFU_STATUS_errCHECK_ERASED 0x05
> +#define DFU_STATUS_errPROG 0x06
> +#define DFU_STATUS_errVERIFY 0x07
> +#define DFU_STATUS_errADDRESS 0x08
> +#define DFU_STATUS_errNOTDONE 0x09
> +#define DFU_STATUS_errFIRMWARE 0x0a
> +#define DFU_STATUS_errVENDOR 0x0b
> +#define DFU_STATUS_errUSBR 0x0c
> +#define DFU_STATUS_errPOR 0x0d
> +#define DFU_STATUS_errUNKNOWN 0x0e
> +#define DFU_STATUS_errSTALLEDPKT 0x0f
Constant defines in all capitals please
> +
> +enum dfu_state {
> + DFU_STATE_appIDLE = 0,
> + DFU_STATE_appDETACH = 1,
> + DFU_STATE_dfuIDLE = 2,
> + DFU_STATE_dfuDNLOAD_SYNC = 3,
> + DFU_STATE_dfuDNBUSY = 4,
> + DFU_STATE_dfuDNLOAD_IDLE = 5,
> + DFU_STATE_dfuMANIFEST_SYNC = 6,
> + DFU_STATE_dfuMANIFEST = 7,
> + DFU_STATE_dfuMANIFEST_WAIT_RST = 8,
> + DFU_STATE_dfuUPLOAD_IDLE = 9,
> + DFU_STATE_dfuERROR = 10,
Same here.
I am sorry, but I have too many review remarks here, among which some
really fundamental design flaws (Like assumption on NAND availability
and partitioning, memory addresses). This code makes a lot of
assumptions on a certain type of application, and it even contains
board knowledge inside a generic DFU-USB device driver. Therefore this
code is non portable across different architectures, and it is not
even portable between 2 different AT91 boards. So, I have to give it a
full NAK at this time.
Kind regards,
Remy
More information about the U-Boot
mailing list