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

[Andrzej Pietrasiewicz]

Hello Stefan,

Thank you for your review. Please see comments inline.

On Wednesday, November 02, 2011 9:07 PM Stefan Schmidt wrote:

> 
> First, and only high level, review for the DFU part.
> 
> Against which u-boot tree/branch/version is this patch? I tried to
> apply it against HEAD and it failed for me. Anything I miss?
> 
Sorry about that. I forgot to mention the reference. It is
http://patchwork.ozlabs.org/patch/122080

> On Wed, 2011-11-02 at 11:00, Andrzej Pietrasiewicz wrote:
> >
> > The implementation is split into two parts: the dfu gadget
> implementation
> > and a "flashing backend", the interface between the two being the
> > struct flash_entity. The "flashing backend"'s implementation is
> > board-specific and is implemented on the Goni target.
> 
> I replied to Mike's mail with my ideas on this. Split between dfu and
> flashing backends is good and missng in my approach currently. I would
> not put it into the board file though but make it generic for other
> boards as well and only define the needed informations in the board
> file. Please see my other mail for details. Comments appreciated!

Comments sent in other mails;) I agree that since there is interest
in implementing DFU in u-boot the code which in fact is not board
specific can and should be generalized. We need to think of an
appropriate place in the tree to store it, though.

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

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

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

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

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

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

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

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

Actually, the original intention was to implement the dfu both in u-boot
and in kernel. I did post my kernel implementation to linux-usb and
did give example of being useful. Even though this kind of a driver
could seem controversial, there are other drivers based on similar
philosophy
which are in the mainline kernel. Anyway, Felipe Balbi, the usb maintainer
in
Linux, did not approve because he says this kind of things belongs
to userspace. Still, I see the gadget framework suitable
to do the job for us here.

> > +/*
> > + * end compatibility layer
> > + */
> > 
> Urgs, is this all needed? Do we really want to have all this around.
> This is not the kernel. U-Boots debug infrastrcuture should be used.

Please see the comment above. In such circumstances, we probably don't.

> > +#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 ;)

> It was good to see that the DFU state machine was taken from Haralds
> patches. That should make it a lot easier to find a common ground
> between the implementations. And I agree that splitting out the DFU
> protocol from the flashing parts makes sense.
> 

Absolutely yes.

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

Thank you,

Andrzej