[U-Boot] [PATCH] dfu: initial implementation

[Stefan Schmidt]

Hello.

On Thu, 2011-11-03 at 11:33, Andrzej Pietrasiewicz wrote:
> On Wednesday, November 02, 2011 9:07 PM Stefan Schmidt wrote:
> > On Wed, 2011-11-02 at 11:00, Andrzej Pietrasiewicz wrote:
> > 
> > > diff --git a/common/Makefile b/common/Makefile
> > > index ae795e0..de926d9 100644
> > > --- a/common/Makefile
> > > +++ b/common/Makefile
> > > @@ -162,6 +162,7 @@ COBJS-y += cmd_usb.o
> > >  COBJS-y += usb.o
> > >  COBJS-$(CONFIG_USB_STORAGE) += usb_storage.o
> > >  endif
> > > +COBJS-$(CONFIG_CMD_DEVICE_FIRMWARE_UPGRADE) += cmd_dfu.o
> > 
> > CONFIG_CMD_DFU instead? Looks a bit long to me.
> 
> ACK. No problem with changing that.

OK

> > I already wrote a bit about my approach here. At OpenMoko we used a
> > button to enter u-boot during startup when pressed. This offered a
> > usbtty interface as usb gadget as well as the runtime descripto for
> > DFU. With dfu-util it was possible to iniate the DFU download or
> > upload procedure while being in the mode. Another option would be to
> > directly jump into DFU mode when a button is pressed.
> 
> Please see my comment in the other mail. A generic way to handle
> "a button pressed" seems necessary. Actually in my opinion just
> providing the "dfu" command is enough. If, in some hardware,
> initiating the runtime mode is desired by pressing some keys, then
> the board initialization code should check for such an event and
> if it happens then just programmatically run the dfu command.
> What do you think?

Lets go with your dfu implementation and the dfu command. I will
prepare patches against is if I see something missing from my patch.

> > > +/* #include "usbstring.c" */
> > 
> > Not needed, can be removed.
> 
> I agree to coding style/reduntant code comments; will not
> specifically comment on them unless I disagree.

Thanks.

> > > +static const struct dfu_function_descriptor dfu_func = {
> > > +	.bLength =		sizeof dfu_func,
> > > +	.bDescriptorType =	DFU_DT_FUNC,
> > > +	.bmAttributes =		DFU_BIT_WILL_DETACH |
> > 
> > Here are you setting the WILL_DETACH bit. Is the device really
> > detaching itself? Its not obvious to me from the code that it does.
> 
> It seems it does not. The dfu-util session says "Resetting USB", so I
> assume that resetting is initiated by the host, not by the device.
> To be removed.

So far I also put this aside. Its only a 1.1 feature which can be
added with another patch when the basic stuff is working and merged.

> > > +static char manufacturer[50];
> > > +static const char longname[] = "DFU Gadget";
> > > +/* default serial number takes at least two packets */
> > > +static const char serial[] = "0123456789.0123456789.012345678";
> > > +static const char dfu_name[] = "Device Firmware Upgrade";
> > > +static const char shortname[] = "dfu";
> > 
> > This surely should be come from the board configuration?
> 
> Yes and no - depends on whether we want to version the gadget itself
> or the flashing backend, or both. As probably the last option
> is best, the answer is probably "yes". We need some interface then.

As I already stated this part is not worked out in my head yet. Feel
free to propose something or we will fix it while working on it.

> > > +static bool is_runtime(struct dfu_dev *dev)
> > > +{
> > > +	return dev->dfu_state == DFU_STATE_appIDLE ||
> > > +		dev->dfu_state == DFU_STATE_appDETACH;
> > > +}
> > 
> > Hmm, here you are checking if you are in run-time mode. In the
> > introduction you wrote that it is in DFU mode when the dfu command is
> > executed. When does it enter the run-time mode? After the first
> > flashing?
> 
> I think this is over-interpretation of what I wrote in introduction.
> I gave an example dfu-util session when the device is already in DFU
> mode. I didn't say that it only has DFU mode. It has both.

Yeah, sorry for that. I indeed misread this part of your
implementation.

> > > +	/* send status response */
> > > +	dstat->bStatus = dev->dfu_status;
> > > +	/* FIXME: set dstat->bwPollTimeout */
> > 
> > This really needs to be set. It was a nightmare with dfu-util not
> > having it set with the initial u-boot implementation. setting the
> > correct values here is not that easy though. It depends on the flash
> > layer and if can get the information about the maximal time a flash
> > write can take.
> 
> Not sure what to do at this moment, either.

If we don't have a back channel from the actual flashing subsystem we
need to oriantate the values on the hardware spec on own measurements
I fear. Soemthing that need to be set in the board file then.

> > > +		case USB_REQ_DFU_DETACH:
> > > +			/* Proprietary extension: 'detach' from idle mode
> and
> > > +			 * get back to runtime mode in case of USB Reset.
> As
> > > +			 * much as I dislike this, we just can't use every
> > USB
> > > +			 * bus reset to switch back to runtime mode, since
> at
> > > +			 * least the Linux USB stack likes to send a number
> > of
> > > +			 * resets in a row :(
> > > +			 */
> > 
> > OK, this makes it clear that you took the DFU state machine from
> > Haralds implementation I'm also using. :)
> > 
> > It would be nice if the related copyright holder would be stated in
> > the header of the file.
> 
> You are absolutely right. My gadget implementation is also based on
> Gadget Zero from Linux and I did mention that, but missed including
> appropriate information regarding the DFU state machine.

OK. Would be good to add this for the next round.

> > > +int __init dfu_init(void)
> > > +{
> > > +	return usb_gadget_register_driver(&dfu_driver);
> > > +}
> > 
> > All this is absed on the re-written gadget layer? It loks as if all
> > kind of kernel stuff is brought over here. Is this gadget layer
> > already merged into u-boot mainline?
> 
> I think that the gadget framework works well and can be successfully
> reused in u-boot. What other interface would you see to talk to UDC?
> Also, USB mass storage implementation can be easy done with usb gadget.

I don't wanted to argue against a good gadget framework. I was just
wondering if it already is in mainline and if not what is blocking it.

My patch has some additions to usbtty for the runtime descriptor  to
show up and all other parts are handled over ep0. The gadget framework
would make this easier and better abstracted to work toegther with
different usb gadget drivers. The only fear I have is to depend on
soemthing not yet in u-boot mainline will make it harder to get the
dfu implementation merged.

> > > +#define DRIVER_VERSION			"Marzanna"
> > 
> > Maybe that helps you internal but it does not make much sense for
> > people outside Samsung. :)
> > 
> 
> The code I started to work on had an equally nice name here, which
> was like "Victory Day" or something. "Marzanna" is one of female
> names in Polish and it was Marzanna's name day when I started work
> on dfu. Self explanatory ;)

Hehe :)

> > How would you like to proceed in getting our stuff merged and finally
> > integrated into mainline?
> 
> You mentioned that your DFU implementation does things which mine doesn't.
> Can you please tell something more? As far as I understand what you wrote,
> there is something in your DFU implementation which does more, and also we
> use different media as backends.

One thin was the runtime and dfu mode thing. I was wrong there as
yours also do both. For the flashing backend I have the alt settings
dynamically generated from available mtdparts and use the new nand
write functions skipping over bad blocks. For the core DFU protocol
I'm not sure about the differences yet. But as I said, prepare I stand
alone patch of yours and I will add send patches to add missing stuff
of the DFU implementation, if any.

My patch should be on the list later today or tomorrow to let you see
what I have.

regards
Stefan Schmidt