| Message ID | 1297633963-15736-1-git-send-email-korgull@home.nl |
|---|---|
| State | Changes Requested |
| Delegated to: | Remy Bohmer |
| Headers | show |
Hi, 2011/2/13 Marcel Janssen <korgull@home.nl>: > From: Marcel <korgull@home.nl> > > Signed-off-by: Marcel <korgull@home.nl> > --- > arch/arm/cpu/arm926ejs/at91/led.c | 119 +++++++++++++++++++++++++++++++++- Why is this part if this patch? It does not seem to be related to USB stuff. Please make it a seperate patch. > common/Makefile | 1 + > common/update_dfu.c | 2 - > drivers/usb/gadget/atmel_usba_udc.c | 8 +- > drivers/usb/gadget/usbdfu.c | 14 +++-- > 5 files changed, 128 insertions(+), 16 deletions(-) > > diff --git a/arch/arm/cpu/arm926ejs/at91/led.c b/arch/arm/cpu/arm926ejs/at91/led.c > index 0a315c4..0638a2e 100644 > --- a/arch/arm/cpu/arm926ejs/at91/led.c > +++ b/arch/arm/cpu/arm926ejs/at91/led.c > @@ -28,38 +28,149 @@ > #include <asm/arch/gpio.h> > #include <asm/arch/io.h> > > +static unsigned int saved_state[4] = {STATUS_LED_OFF, STATUS_LED_OFF, > + STATUS_LED_OFF, STATUS_LED_OFF}; > + > +void coloured_LED_init(void) > +{ > + /* Enable clock */ > + at91_sys_write(AT91_PMC_PCER, 1 << AT91SAM9G45_ID_PIODE); at91_sys_write is deprecated, please use register access by struct Why is modification of the generic at91 led code required? It is not clear. Please, only make a patch series with only USB-DFU stuff in it, drop the add-board code from this series. The board code is not acceptable for mainstream since it does not follow the new u-boot-atmel->rework110202 tree style of adding at91 boards. It will take you a huge amount of effort to make it suitable. Further, both parts of the patch series are not related and you are now creating a link between the Atmel tree and the USB tree, which makes it even more difficult. Kind regards, Remy
On Tuesday, February 15, 2011 07:43:34 pm Remy Bohmer wrote: > Hi, > > 2011/2/13 Marcel Janssen <korgull@home.nl>: > > From: Marcel <korgull@home.nl> > > > > Signed-off-by: Marcel <korgull@home.nl> > > --- > > arch/arm/cpu/arm926ejs/at91/led.c | 119 > > +++++++++++++++++++++++++++++++++- > > Why is this part if this patch? > It does not seem to be related to USB stuff. Please make it a seperate > patch. I optionally use the LED's in DFU mode so that there's interaction while upgrading the board. You might believe the host could handle this, but I like the device to indicate activity as well. Somehow I couldn't get it working without changing led.c I think, but I may have missed something. > > common/Makefile | 1 + > > common/update_dfu.c | 2 - > > drivers/usb/gadget/atmel_usba_udc.c | 8 +- > > drivers/usb/gadget/usbdfu.c | 14 +++-- > > 5 files changed, 128 insertions(+), 16 deletions(-) > > > > diff --git a/arch/arm/cpu/arm926ejs/at91/led.c > > b/arch/arm/cpu/arm926ejs/at91/led.c index 0a315c4..0638a2e 100644 > > --- a/arch/arm/cpu/arm926ejs/at91/led.c > > +++ b/arch/arm/cpu/arm926ejs/at91/led.c > > @@ -28,38 +28,149 @@ > > #include <asm/arch/gpio.h> > > #include <asm/arch/io.h> > > > > +static unsigned int saved_state[4] = {STATUS_LED_OFF, STATUS_LED_OFF, > > + STATUS_LED_OFF, STATUS_LED_OFF}; > > + > > +void coloured_LED_init(void) > > +{ > > + /* Enable clock */ > > + at91_sys_write(AT91_PMC_PCER, 1 << AT91SAM9G45_ID_PIODE); > > at91_sys_write is deprecated, please use register access by struct Already noticed that. > Why is modification of the generic at91 led code required? It is not clear. > > Please, only make a patch series with only USB-DFU stuff in it, drop > the add-board code from this series. The board code is not acceptable > for mainstream since it does not follow the new > u-boot-atmel->rework110202 tree style of adding at91 boards. It will > take you a huge amount of effort to make it suitable. > Further, both parts of the patch series are not related and you are > now creating a link between the Atmel tree and the USB tree, which > makes it even more difficult. I'm actually busy fixing the board code for u-boot-atmel->rework110202 Most of it is working so I hope I can create the patches as you like them. best regards, Marcel
Hi, 2011/2/15 Marcel Janssen <korgull@home.nl>: > On Tuesday, February 15, 2011 07:43:34 pm Remy Bohmer wrote: >> Hi, >> >> 2011/2/13 Marcel Janssen <korgull@home.nl>: >> > From: Marcel <korgull@home.nl> >> > >> > Signed-off-by: Marcel <korgull@home.nl> >> > --- >> > arch/arm/cpu/arm926ejs/at91/led.c | 119 >> > +++++++++++++++++++++++++++++++++- >> >> Why is this part if this patch? >> It does not seem to be related to USB stuff. Please make it a seperate >> patch. > > I optionally use the LED's in DFU mode so that there's interaction while > upgrading the board. U-boot has a terminal/monitor. It is not up to the driver to control LEDs that might or might not be on a board. > You might believe the host could handle this, but I like > the device to indicate activity as well. > Somehow I couldn't get it working without changing led.c I think, but I may > have missed something. The problem is here that you mix up sub-systems. Modifying LED code should be independent from USB driver code, and really not in the same patch. >> > common/Makefile | 1 + >> > common/update_dfu.c | 2 - >> > drivers/usb/gadget/usbdfu.c | 14 +++-- These files should be completely generic code, that even would work on X86, PowerPC and so on. >> > drivers/usb/gadget/atmel_usba_udc.c | 8 +- Only this driver file should be Atmel USBA specific, but NOT SoC specific, and absolutely not board or board config related. >> Please, only make a patch series with only USB-DFU stuff in it, drop >> the add-board code from this series. The board code is not acceptable >> for mainstream since it does not follow the new >> u-boot-atmel->rework110202 tree style of adding at91 boards. It will >> take you a huge amount of effort to make it suitable. >> Further, both parts of the patch series are not related and you are >> now creating a link between the Atmel tree and the USB tree, which >> makes it even more difficult. > > I'm actually busy fixing the board code for u-boot-atmel->rework110202 > > Most of it is working so I hope I can create the patches as you like them. Hmm, Let's make it even more black/white: I do not have to like the board code. ;-) Reinhard is the Atmel maintainer. He needs to pull in the Board code. I only care about generic USB code... ;-))) Please make 2 unrelated patch series (1 series for USB DFU support, 1 series for board support), or it will never hit mainline... AND: I would really recommend to build a decent USB/DFU patch series first. Forget the board for now. The board seems to depend on DFU support, not the other way around. If generic DFU code is really generic and acceptable, I will pull it in my tree. I am willing to fix small issues in the series to assist you, but I will not do major redesigns... Kind regards, Remy
Hi Remy, > 2011/2/15 Marcel Janssen <korgull@home.nl>: > > On Tuesday, February 15, 2011 07:43:34 pm Remy Bohmer wrote: > >> Hi, > >> > >> 2011/2/13 Marcel Janssen <korgull@home.nl>: > >> > From: Marcel <korgull@home.nl> > >> > > >> > Signed-off-by: Marcel <korgull@home.nl> > >> > --- > >> > arch/arm/cpu/arm926ejs/at91/led.c | 119 > >> > +++++++++++++++++++++++++++++++++- > >> > >> Why is this part if this patch? > >> It does not seem to be related to USB stuff. Please make it a seperate > >> patch. > > > > I optionally use the LED's in DFU mode so that there's interaction while > > upgrading the board. > > U-boot has a terminal/monitor. It is not up to the driver to control > LEDs that might or might not be on a board. OK, good to know. I'll check that out, but not today. For now I can remove the LED code I think. Than in a couple of months I can can check how to use the monitor code. > > You might believe the host could handle this, but I like > > the device to indicate activity as well. > > Somehow I couldn't get it working without changing led.c I think, but I > > may have missed something. > > The problem is here that you mix up sub-systems. > Modifying LED code should be independent from USB driver code, and > really not in the same patch. > > >> > common/Makefile | 1 + > >> > common/update_dfu.c | 2 - > >> > drivers/usb/gadget/usbdfu.c | 14 +++-- > > These files should be completely generic code, that even would work on > X86, PowerPC and so on. Agree on that. It would be optiomal. Not everything is, the first time :-) > >> > drivers/usb/gadget/atmel_usba_udc.c | 8 +- > > Only this driver file should be Atmel USBA specific, but NOT SoC > specific, and absolutely not board or board config related. ok. > >> Please, only make a patch series with only USB-DFU stuff in it, drop > >> the add-board code from this series. The board code is not acceptable > >> for mainstream since it does not follow the new > >> u-boot-atmel->rework110202 tree style of adding at91 boards. It will > >> take you a huge amount of effort to make it suitable. > >> Further, both parts of the patch series are not related and you are > >> now creating a link between the Atmel tree and the USB tree, which > >> makes it even more difficult. > > > > I'm actually busy fixing the board code for u-boot-atmel->rework110202 > > > > Most of it is working so I hope I can create the patches as you like > > them. > > Hmm, Let's make it even more black/white: I do not have to like the > board code. ;-) > Reinhard is the Atmel maintainer. He needs to pull in the Board code. > I only care about generic USB code... ;-))) hmmm. The black and white this is that after today I don't have a single hour to spend on this code :-) > Please make 2 unrelated patch series (1 series for USB DFU support, 1 > series for board support), or it will never hit mainline... > AND: I would really recommend to build a decent USB/DFU patch series > first. Forget the board for now. The board seems to depend on DFU > support, not the other way around. > If generic DFU code is really generic and acceptable, I will pull it > in my tree. I am willing to fix small issues in the series to assist > you, but I will not do major redesigns... Well, the board does not depend on DFU. Not even the USBD controller is activated in the board config. It is left up to a script to handle that though update_dfu.c which defines a command for that. I may have left some parts in that don't really belong there, I haven't check it yet. Best regards, Marcel
Dear Remy and Reinhard, > Hmm, Let's make it even more black/white: I do not have to like the > board code. ;-) > Reinhard is the Atmel maintainer. He needs to pull in the Board code. > I only care about generic USB code... ;-))) > > Please make 2 unrelated patch series (1 series for USB DFU support, 1 > series for board support), or it will never hit mainline... > AND: I would really recommend to build a decent USB/DFU patch series > first. Forget the board for now. The board seems to depend on DFU > support, not the other way around. > If generic DFU code is really generic and acceptable, I will pull it > in my tree. I am willing to fix small issues in the series to assist > you, but I will not do major redesigns... I tried compiling everything against the rework tree. Unfortunately I don't know how to solve the reset_timer issue and my NAND won't work. Although Reinhard just commented on that, I have no clue yet how to fix it. I can't finish this work today and really have no time left for it, so I have to quit on the code for now. I will have to thank you for you assistance for now and have to get back on this in a couple of months if I do find the time again. Hopefully someone picks up the v2 patch and continues from there. It's not very difficult to make it work in Reinhard's tree I think. Best regards, Marcel
diff --git a/arch/arm/cpu/arm926ejs/at91/led.c b/arch/arm/cpu/arm926ejs/at91/led.c index 0a315c4..0638a2e 100644 --- a/arch/arm/cpu/arm926ejs/at91/led.c +++ b/arch/arm/cpu/arm926ejs/at91/led.c @@ -28,38 +28,149 @@ #include <asm/arch/gpio.h> #include <asm/arch/io.h> +static unsigned int saved_state[4] = {STATUS_LED_OFF, STATUS_LED_OFF, + STATUS_LED_OFF, STATUS_LED_OFF}; + +void coloured_LED_init(void) +{ + /* Enable clock */ + at91_sys_write(AT91_PMC_PCER, 1 << AT91SAM9G45_ID_PIODE); + +#ifdef CONFIG_RED_LED + at91_set_gpio_output(CONFIG_RED_LED, 1); + at91_set_gpio_value(CONFIG_RED_LED, 1); +#endif +#ifdef CONFIG_GREEN_LED + at91_set_gpio_output(CONFIG_GREEN_LED, 1); + at91_set_gpio_value(CONFIG_GREEN_LED, 1); +#endif +#ifdef CONFIG_YELLOW_LED + at91_set_gpio_output(CONFIG_YELLOW_LED, 1); + at91_set_gpio_value(CONFIG_YELLOW_LED, 1); +#endif +#ifdef CONFIG_BLUE_LED + at91_set_gpio_output(CONFIG_BLUE_LED, 1); + at91_set_gpio_value(CONFIG_BLUE_LED, 1); +#endif +} + #ifdef CONFIG_RED_LED void red_LED_on(void) { at91_set_gpio_value(CONFIG_RED_LED, 1); + saved_state[STATUS_LED_RED] = STATUS_LED_ON; } void red_LED_off(void) { at91_set_gpio_value(CONFIG_RED_LED, 0); + saved_state[STATUS_LED_RED] = STATUS_LED_OFF; } #endif #ifdef CONFIG_GREEN_LED void green_LED_on(void) { - at91_set_gpio_value(CONFIG_GREEN_LED, 0); + at91_set_gpio_value(CONFIG_GREEN_LED, 1); + saved_state[STATUS_LED_GREEN] = STATUS_LED_ON; } void green_LED_off(void) { - at91_set_gpio_value(CONFIG_GREEN_LED, 1); + at91_set_gpio_value(CONFIG_GREEN_LED, 0); + saved_state[STATUS_LED_GREEN] = STATUS_LED_OFF; } #endif #ifdef CONFIG_YELLOW_LED void yellow_LED_on(void) { - at91_set_gpio_value(CONFIG_YELLOW_LED, 0); + at91_set_gpio_value(CONFIG_YELLOW_LED, 1); + saved_state[STATUS_LED_YELLOW] = STATUS_LED_ON; } void yellow_LED_off(void) { - at91_set_gpio_value(CONFIG_YELLOW_LED, 1); + at91_set_gpio_value(CONFIG_YELLOW_LED, 0); + saved_state[STATUS_LED_YELLOW] = STATUS_LED_OFF; } #endif + +void __led_init(led_id_t mask, int state) +{ + __led_set(mask, state); +} + +void __led_toggle(led_id_t mask) +{ + +#ifdef CONFIG_RED_LED + if (STATUS_LED_RED == mask) { + if (STATUS_LED_ON == saved_state[STATUS_LED_RED]) + red_LED_off(); + else + red_LED_on(); + } +#endif +#ifdef CONFIG_BLUE_LED + else if (STATUS_LED_BLUE == mask) { + if (STATUS_LED_ON == saved_state[STATUS_LED_BLUE]) + blue_LED_off(); + else + blue_LED_on(); + } +#endif +#ifdef CONFIG_GREEN_LED + else if (STATUS_LED_GREEN == mask) { + if (STATUS_LED_ON == saved_state[STATUS_LED_GREEN]) + green_LED_off(); + else + green_LED_on(); + } +#endif +#ifdef CONFIG_YELLOW_LED + else if (STATUS_LED_YELLOW == mask) { + if (STATUS_LED_ON == saved_state[STATUS_LED_YELLOW]) + yellow_LED_off(); + else + yellow_LED_on(); + } +#endif +} + +void __led_set(led_id_t mask, int state) +{ +#ifdef CONFIG_RED_LED + if (STATUS_LED_RED == mask) { + if (STATUS_LED_ON == state) + red_LED_on(); + else + red_LED_off(); + } +#endif +#ifdef CONFIG_BLUE_LED + else if (STATUS_LED_BLUE == mask) { + if (STATUS_LED_ON == state) + blue_LED_on(); + else + blue_LED_off(); + } +#endif +#ifdef CONFIG_GREEN_LED + else if (STATUS_LED_GREEN == mask) { + if (STATUS_LED_ON == state) + green_LED_on(); + else + green_LED_off(); + } +#endif +#ifdef CONFIG_YELLOW_LED + else if (STATUS_LED_YELLOW == mask) { + if (STATUS_LED_ON == state) + yellow_LED_on(); + else + yellow_LED_off(); + } +#endif +} + diff --git a/common/Makefile b/common/Makefile index 048df0c..a653c1e 100644 --- a/common/Makefile +++ b/common/Makefile @@ -163,6 +163,7 @@ COBJS-$(CONFIG_LCD) += lcd.o COBJS-$(CONFIG_LYNXKDI) += lynxkdi.o COBJS-$(CONFIG_MODEM_SUPPORT) += modem.o COBJS-$(CONFIG_UPDATE_TFTP) += update.o +COBJS-$(CONFIG_USBD_DFU)+= update_dfu.o COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o diff --git a/common/update_dfu.c b/common/update_dfu.c index f1ceccf..ca2240b 100644 --- a/common/update_dfu.c +++ b/common/update_dfu.c @@ -56,8 +56,6 @@ int dfu_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { int rcv; - dfu_finished = 0; - /* initialize the USBD controller */ #ifdef CONFIG_USB_GADGET_ATMEL_USBA usba_udc_probe(&dfubrd); diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c index 6d02de6..45227c4 100644 --- a/drivers/usb/gadget/atmel_usba_udc.c +++ b/drivers/usb/gadget/atmel_usba_udc.c @@ -349,12 +349,12 @@ static struct usba_request *alloc_request(void) if (!req_pool[i].in_use) { ptr = &req_pool[i]; req_pool[i].in_use = 1; - DBG("alloc_req: request[%d]\n", i); + debug("alloc_req: request[%d]\n", i); break; } } if (!ptr) - DBG("panic: no more free req buffers\n"); + debug("panic: no more free req buffers\n"); return ptr; } @@ -412,7 +412,7 @@ usba_ep_queue(struct usb_ep *_ep, } if (!_ep || (!ep->desc && ep->ep.name != ep0name)) { - DBG("invalid ep\n"); + debug("invalid ep\n"); return -EINVAL; } @@ -457,7 +457,7 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) ep->ep.name, req); if (!_ep || ep->ep.name == ep0name) { - DBG("invalid dequeue request\n"); + debug("invalid dequeue request\n"); if (!_ep) debug("NO ENDPOINT\n"); if (ep->ep.name == ep0name) diff --git a/drivers/usb/gadget/usbdfu.c b/drivers/usb/gadget/usbdfu.c index 65f334a..a1e7316 100644 --- a/drivers/usb/gadget/usbdfu.c +++ b/drivers/usb/gadget/usbdfu.c @@ -398,9 +398,10 @@ static int handle_dnload(struct usb_gadget *gadget, char *mtdp = getenv("mtdparts"); if (mtdp) printf("Valid MTD partitions found\n"); - /*this used to be in the Openmoko driver */ - /*if (!mtdp) - /*run_command("dynpart", 0); */ + /*this used to be in the Openmoko driver + *if (!mtdp) + *run_command("dynpart", 0); + */ else { dev->dfu_state = DFU_STATE_dfuERROR; dev->dfu_status = DFU_STATUS_errADDRESS; @@ -477,9 +478,10 @@ static int handle_dnload(struct usb_gadget *gadget, red_LED_off(); #endif rc = nand_write_skip_bad(ds->nand, - ds->part->offset, - &rwsize, - (u_char *)addr); + ds->part->offset, + &rwsize, + (u_char *)addr, + 0); if (rc) { printf("NAND write failed\n"); break;