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

2011-11-08 Thread Detlev Zundel
Hi Andrzej,


[...]

   If my assumption is correct, then what would it take to split off
   protocol part and make it independent of the actual driver
 interface?
 
  I guess that in the situation given it would be of little use.
 
 What do you think would be of little use?
 

 As far as I understand you correctly, you would like the state
 machine code extracted from the initial DFU implementation posted
 on this mailing list. And then you would like the DFU implemented
 in terms of this abstract state machine. This means actually
 more code, probably not a desired thing. The generic state machine
 would also require some accompanying generic data structures,
 and this would also mean code to translate back and forth between
 USB/DFU (DFU _is_ USB) and state machine's data structures.
 All in all, this means adding more complexity and code. This looks
 like a premature optimization (which is widely known to be the root
 of all evil), while at the moment, there are no use cases
 to justify it. Should the use cases appear, the code can be reworked
 based on the needs of both USB/DFU and new state machine users.

Just to add my personal opinion here - I am occassionally lobbying for a
long time on this mailing list that mainline U-Boot gains DFU support,
so please let's get this working with the existing tools and in the
existing contexts, i.e. USB.  Personally I consider it to be completely
out of scope to dis-entangle DFU from its specification and abstract it
into something which it _could_ be, but really _isn't_.

When the implementation for USB is here, we all have much better grounds
to discuss possible generalizations (although I doubt that there are
many).

Cheers
  Detlev

-- 
Perfecting oneself is as much unlearning as it is learning. 
-- Edsger Dijkstra 
--
DENX Software Engineering GmbH,  MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-07 Thread Andrzej Pietrasiewicz
Dear Wolfgang Denk,

Please see my comments inline.

 
  DFU is part of USB; an extension to be precise, but an extension
 bound
  so tightly to the design and philosophy of USB that it is rather
  inconceivable to separate the two.
 
 Could you please be so kind and explain which exact issues you see for
 such a separation?

First of all, I think that it is more proper to speak of communications
part and state machine part. They are bound together by the standard.
For example, in the dfuIDLE state, the receipt of DFU_DNLOAD request
with wLength == 0 triggers a device stalls the control pipe action.
This kind of stuff is very USB specific - I am not sure what would
the control pipe be for serial/Ethernet, nor what stall the
control pipe would mean for either of them.
Another example, in the dfuMANIFEST state, when the status poll
timeout occurs and bitManifestationTolerant == 1 then device can
still communicate via the USB.

The communications part is of course the very USB itself, so the only
thing which could be extracted is the state machine part. But, then,
in order not to be bound to USB but reusable, it should be abstract.
Being abstract, it would not be DFU anymore, but some
Generic Update Protocol that you have on your mind,
but we don't know of. Is there any standard for such a protocol?
 
 
  Seriously speaking, in view of ties between DFU and USB
  IMHO it is impossible, or, at least, highly impractical.
 
 Can you please support this statement with a few facts?

Please see my comment above.
 
   If my assumption is correct, then what would it take to split off
   protocol part and make it independent of the actual driver
 interface?
 
  I guess that in the situation given it would be of little use.
 
 What do you think would be of little use?
 

As far as I understand you correctly, you would like the state
machine code extracted from the initial DFU implementation posted
on this mailing list. And then you would like the DFU implemented
in terms of this abstract state machine. This means actually
more code, probably not a desired thing. The generic state machine
would also require some accompanying generic data structures,
and this would also mean code to translate back and forth between
USB/DFU (DFU _is_ USB) and state machine's data structures.
All in all, this means adding more complexity and code. This looks
like a premature optimization (which is widely known to be the root
of all evil), while at the moment, there are no use cases
to justify it. Should the use cases appear, the code can be reworked
based on the needs of both USB/DFU and new state machine users.

Regards,

Andrzej



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-06 Thread Stefan Schmidt
Hello.

On Sat, 2011-11-05 at 16:31, Wolfgang Denk wrote:
 In message 2002200717.GP17069@excalibur.local you wrote:
 
  While I think a dfu command is usefull I don't like the need to
  execute it before any DFU interaction can happen. That may be an
  option during development but for field upgrades or receovery it is
  not.
 
 Yes, a command is the natural way in U-Boot to start any activities.
 
  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
 
 This is just another way to start a command.

So bounding a key press to the command would be the way to achieve
what some people want. Fine with me.

  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.
 
 NAK.  Let's stick to standard interfaces.

What do you mean here? You don't want the DFU descripto available with
USBTTY or you don't want to start DFU directly in DFU mode?

regards
Stefan Schmidt
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-06 Thread Stefan Schmidt
Hello.

On Sat, 2011-11-05 at 16:33, Wolfgang Denk wrote:
 In message 000601cc9abe$4f544bd0$edfce370$%p...@samsung.com you wrote:
  
http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
  
  DFU is part of USB; an extension to be precise, but an extension bound
  so tightly to the design and philosophy of USB that it is rather
  inconceivable to separate the two.
 
 Could you please be so kind and explain which exact issues you see for
 such a separation?

As Andrzej pointed out the DFU spec is written by the USB forum and
one can see that there target are USB devices. Let me bring in some
facts from the spec.

One part that is not covered here is the DFU suffix mandatory for all
firmware files. It gets stripped from the flashing tool before sending
it to the device code and thus it was not covered here so far. This
suffix holds, among others, fields with the PID of VID of the USB
device. Only allowing to be flashed if the device has the correct IDs.
How would assign the USB IDs to a network based approach for example?

The core part, I understand, is the DFU state machine you are referring
to. This sate machine relies on the USB descriptors in run-time or DFU
mode. It is hardcoded wot USB descriptors using bDeviceClass,
bDeviceSubClass, idVendor and idProduct to name a few. (See p. 14, 15
of the 1.1 spec).

In the reconfiguration phase the spec mandates the host to issue a
DFU_DETACH request on the USB EP0 and then issuing an USB reset. (p.
17)

All bound to the USB spec and hardware for using the spec in a
standard compliant way.

   Eventually it should be possible to run this protocol over Ethernet or
   even over a serial line?
  
  Of course there is no such a reason, provided we lay USB over Ethernet
  or serial line first ;)
 
 This is of course not intended.  I was thinking about a plain standard
 UDP based link.

As always in computers one could try to come up with solutions to
replace the USB hardcoded parts with something else for a UDP based
solution. Its just now longer compliant with the DFU spec. All DFU
flashing utils out there are only working over USB. All the windows
flash utils as well as dfu-programmer, a small dfu util from the bluez
project and also dfu-util. The last ones are using libusb for this.

The main point is that if you replace the USB related parts with
something else not much of DFU stays anymore. It would be a different
flashing procedure. And for systems with a network interface available
there are way better ways to flash it then DFU. Even TFP would be more
convenient for them. You are way more flexible with Ethernet here.

DFU was designed for really small devices with minimal software
originally. Best examples are Bluetooth and WiFi USB dongles which can
be flashed this way if they. Its just that it becomes more popular as
many consumer electronics device are coming with a USB port but no
Ethernet these days.

regards
Stefan Schmidt
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-06 Thread Wolfgang Denk
Dear Stefan Schmidt,

In message 2006163605.GA20104@excalibur.local you wrote:
 
   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.
  
  NAK.  Let's stick to standard interfaces.
 
 What do you mean here? You don't want the DFU descripto available with
 USBTTY or you don't want to start DFU directly in DFU mode?

I don't want random transfers of control between unrelated
sub-systems.  If you want to run a command, then run a command.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
In the beginning, there was nothing, which exploded.
- Terry Pratchett, _Lords and Ladies_
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-06 Thread Wolfgang Denk
Dear Stefan Schmidt,

In message 2006170219.GB20104@excalibur.local you wrote:
 
  Could you please be so kind and explain which exact issues you see for
  such a separation?
 
 As Andrzej pointed out the DFU spec is written by the USB forum and
 one can see that there target are USB devices. Let me bring in some
 facts from the spec.
...
 All bound to the USB spec and hardware for using the spec in a
 standard compliant way.

Yes, indeed.

But I feel you stick way too close to this spec, and are not trying to
abstract awway the low level details a bit.

Yes, this spec was written by the USB forum, and they did not even
attempt to look over the rim of their plates.  But does that mean we
all have to do the same?  I'm not willing to accept such a narrow
view.

Let's - just as a gedankenexperiment - assume our task was to
implement a solution that provides similar capabilities as DFU, but
must work over serial line, Ethernet, Bluetooth, Infrared, whatever.
Assume we thing DFU could be used as base, but we need to find a way
to move the USB related things into the USB layer, while things like
the state machine, the requests, device status, state transitions,
etc. are generic enough.

Yes, the DFU File Suffix contains fields that map nicely to USB
devices - but what prevents us to provide similar data on non-USB
media, too?  We just need to make sure that th code which handles this
allows to plug in handlers for other transfer media.


 As always in computers one could try to come up with solutions to
 replace the USB hardcoded parts with something else for a UDP based
 solution. Its just now longer compliant with the DFU spec. All DFU

I'm not sure what exactly you want to tell me here.

My idea is that of an implementation that is split into a generic and
a device specific part.  If implemented corectly, the combination of
the generic part with the USB specific part would be strictly
conformant to the DFU spec.

Of course, the combination of for example the DFU generic part with an
Ethernet network interface would not be conformant to the DFU spec,
but it would be something that works in a very similar way.  And that
would most probably be better than inventing yet another download
method.

 flashing utils out there are only working over USB. All the windows
 flash utils as well as dfu-programmer, a small dfu util from the bluez
 project and also dfu-util. The last ones are using libusb for this.

That's the status quo.  Agreed.  Bit this can be changed, extended.
Or not?

 The main point is that if you replace the USB related parts with
 something else not much of DFU stays anymore. It would be a different

I disagree here.  DFU is different in a number of aspects, and you
know this very well.

 flashing procedure. And for systems with a network interface available
 there are way better ways to flash it then DFU. Even TFP would be more
 convenient for them. You are way more flexible with Ethernet here.

I'm not sure what your exact argument is here. I'm not the one asking
for DFU support. I would be happy to use TFTP over Ethenret over USB.
We're actually doing this in some products.

 DFU was designed for really small devices with minimal software
 originally. Best examples are Bluetooth and WiFi USB dongles which can
 be flashed this way if they. Its just that it becomes more popular as
 many consumer electronics device are coming with a USB port but no
 Ethernet these days.

Agreed. And if it's becoming more popular, it might even be a good
idea to think about a less restricted implementation, which opens new
oportunites.


Yes, I think we should strive for such a separation of USB transport
layer and protocol implementation / state machine / command interface.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
While money can't buy happiness, it certainly lets  you  choose  your
own form of misery.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-05 Thread Wolfgang Denk
Dear Andrzej Pietrasiewicz,

In message 000601cc9abe$4f544bd0$edfce370$%p...@samsung.com you wrote:
 
   http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
 
 DFU is part of USB; an extension to be precise, but an extension bound
 so tightly to the design and philosophy of USB that it is rather
 inconceivable to separate the two.

Could you please be so kind and explain which exact issues you see for
such a separation?

  Eventually it should be possible to run this protocol over Ethernet or
  even over a serial line?
 
 Of course there is no such a reason, provided we lay USB over Ethernet
 or serial line first ;)

This is of course not intended.  I was thinking about a plain standard
UDP based link.

 Seriously speaking, in view of ties between DFU and USB
 IMHO it is impossible, or, at least, highly impractical.

Can you please support this statement with a few facts?

  If my assumption is correct, then what would it take to split off
  protocol part and make it independent of the actual driver interface?
 
 I guess that in the situation given it would be of little use.

What do you think would be of little use?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
This restaurant was advertising breakfast  any  time.  So  I  ordered
french toast in the renaissance.- Steven Wright, comedian
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-05 Thread Wolfgang Denk
Dear Stefan Schmidt,

In message 2002200717.GP17069@excalibur.local you wrote:

 While I think a dfu command is usefull I don't like the need to
 execute it before any DFU interaction can happen. That may be an
 option during development but for field upgrades or receovery it is
 not.

Yes, a command is the natural way in U-Boot to start any activities.

 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

This is just another way to start a command.

 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.

NAK.  Let's stick to standard interfaces.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Each honest calling, each walk of life, has its own  elite,  its  own
aristocracy based on excellence of performance. - James Bryant Conant
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-04 Thread Andrzej Pietrasiewicz
Dear Wolfgang Denk,

Please see my comments inline.

On Thursday, November 03, 2011 7:14 PM Wolfgang Denk wrote:

 
  http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
 ...

DFU is part of USB; an extension to be precise, but an extension bound
so tightly to the design and philosophy of USB that it is rather
inconceivable to separate the two.

 
 If I understand correctly, there is no fundamental reason why the DFU
 protocol would only be possible to implement over a USB link.  Or am I
 missing something here?
 
 Eventually it should be possible to run this protocol over Ethernet or
 even over a serial line?

Of course there is no such a reason, provided we lay USB over Ethernet
or serial line first ;)
Seriously speaking, in view of ties between DFU and USB
IMHO it is impossible, or, at least, highly impractical.

 
 If my assumption is correct, then what would it take to split off
 protocol part and make it independent of the actual driver interface?
 

I guess that in the situation given it would be of little use.

Regards,

Andrzej


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-04 Thread Andrzej Pietrasiewicz
Hello,

On Thursday, November 03, 2011 3:01 PM Stefan Schmidt wrote:

 
 o I will send out my not ready for mainline patch to give you and
   others an impression how it is tackled in my patch.
 
 o I like your split between dfu and flashing and also the addition of
   the dfu command. Could you split this part from the rest and send it
   as self conatining patch without other dependencies?
 
 o And then one patch with the mmc/fat flashing backend in moved out of
   the board file (no idea what the best place is, maybe just another
   file for now) and a last patch with all the board specific changes?


I assume splitting into 4 parts: (1) dfu gadget, (2) board specific code,
(3) flashing backend generic code, (4) dfu command.

As I today discovered, after latest merges in u-boot master the mmc
subsystem in Goni does not work properly - the detected capacity is 0,
and the number of available partitions is 0.
This took me some time to discover. I hope the responsible person will
fix this soon. So, as far as the splitting of my dfu implementation
into separate patches is concerned, I think I will come up with it
early next week.

 
 o I will then rebase my code on yours and prepare patches against the
   dfu implementations as needed and also flashing backend for NAND.
 
 o After this we need to review was is missing and bring it over.
 
 Not all details covered in the plan but a start. Comments?

In the beginning I intend to split the code into respective parts,
to give you something to work with. Then I intend to clean the code up,
taking into account your and Mike's review.

Regards,

Andrzej



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread Andrzej Pietrasiewicz
Hello Stefan,

On Wednesday, November 02, 2011 3:29 PM Stefan Shmidt wrote:

 
 Have you fully implemented 1.1? With the detahc logic inside the
 device implementation?

As you noticed in another post, the state machine is reused.

 Just curious. What version of dfu-util your are using for your tests?
 I released 0.5 earlier today.

I used what was available in my desktop's Ubuntu 10.04 LTS, and
it happens to be version 0.1+svn :O It works, though.

 Hmm, that sounds like you only implemented the DFU mode but not the
 run-time mode yet?

This conclusion is not correct. After issuing the dfu command
my implementation begins in runtime mode. Only after using dfu-util
from the host is the mode changed to DFU mode.
Actually, as far as usage of dfu-util is concerned, I find it
inconvenient when the device is in runtime mode, because there
(at least with my version of dfu-util) is no way for the user
to guess what altsettings are actually available. I personally
do dfu-util -U file -a 0 or a similar command to kick the device
into DFU mode, then dfu-util -l to discover what altsettings there are.
So, it could be tempting to omit the runtime mode at all and start
in DFU mode, but, to be standard compliant, I start in runtime mode.
How do you discover with dfu-util what altsettings there are in the
device?

 While the original purpose
 of DFU was to make updating USB device easy enough for end-users as
 well.

True, that's what the standard says about the very purpose of DFU.
However, pressing keys to bring the device into a specific mode is
probably very much device-specific. It would be nice if we
are able to come up with a generic solution which only delegates
the necessary details to board-specific files.

 In the end I would like to see different options available:
 
 1) Entering DFU mode directly when a button is pressed on startup. An
 update mode if you want to call it like this.

Please see my comment above.

 
 2) Normal startup, but with DFU descripton to allow dfu-util detaching
 and entering the DFU mode for flashing.
 
 3) Starting dfu mode form the command line as you did implement it.
 

I think 2 and 3 don't differ too much - it seems that enriching a
setup without DFU with additional USB descriptors will do the job.
But 2 and 3 probably should not be merged, though.

  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.
 
 What is your reasoning behind this split? I have it working here with
 the normal nand_flashing routines taking care of bad blocks, etc.

The reason is to factor out board-specific stuff from the dfu gadget
implementation. However, in what I factored out probably there are
parts which are generic enough not to be board-specific. Since there
seems to be considerable interest in DFU implementation for u-boot,
this could be reworked.

 To make it clear, I'm happy to see somebody else working on this

Whap! --- Oh, man, we are not alone! ;) I am happy to
hear from someone else who works on this, too.

Regards,

Andrzej


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread Andrzej Pietrasiewicz
Hello Mike,

Thank you for your review. Please see my comments inline.

On Wednesday, November 02, 2011 4:16 PM Mike Frysinger wrote:

 
  Dear All,
 
  This is Device Firmware Upgrade (DFU) implementation which supports
  data upload and download function to devices which are equipped with
 a UDC.
 
 this information belongs in the changelog (above the --- marker)
 
I generally agree to remarks related to coding style and implementation's
distribution into respective patches.

 are you working with the elinux.org guys ?
   http://elinux.org/Merge_DFU_support_into_mainline_U-Boot

That's not me.

 
 this should be split up into at least the dfu core and board-specific
 changes.
 although i'd wonder how much of the board/samsung/ stuff is really
 board specific and couldn't be generalized ...

You are right, probably the flashing backend part contains
some code which can be generalized.

Regards,

Andrzej


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread Andrzej Pietrasiewicz
Hello Stefan,

On Wednesday, November 02, 2011 8:30 PM Stefan Schmidt writes:

 
 Agreed. The eMMC flashing with files on FAT is nothing goni specific.
 Others should be able to use this as well. I see three different parts
 here that can be separated:

I agree. Since there is interest in DFU implementation this can and should
be generalized.

 
 1) The DFU protocol implementation which lives in usb gadget with an
 interface for flashing backends
 
 2) The flashing backends (no idea where to put them best). At the
 moment that would be the implementation here with eMMC and files on FAT,
 mine with raw NAND devices and in the future maybe others. Each
 flashing backend should be generic enough to allow different boards
 using it.
 
 3) Board specific information like partitions or hints for the flashing
 backend. The information itself should be in the board file here and
 only used by the flashing backends.
 
 How does this sound to you? Andrzej?

Sounds good to me. In my implementation the interface between dfu gadget
and flashing backend is around the struct flash_entity.
It contains a character string intended to provide a human-readable name,
a void * context which is not interpreted by the gadget code,
but is passed to the flashing backend and understood by it.
The struct flash_entity also contains prepare-finish methods
to be called before and after read/write operations, and the read_block-
write_block pair. What do you think?

As far as generalization is concerned, in my flashing backend
implementation I see these parts as candidates for generalization:

1) mbr/ebr reading
2) reading/writing mmc
3) read/write fat
4) generic prepare/finish; not sure if fat-specific prepare can be
generalized
5) read/write_block
6) some more work should be put to create an interface between the board
initialization routine, the flashing backend and the DFU gadget
implementation.
In my implementation the board initialization routine calls board-specific
register_flash_areas, which, in turn, calls DFU gadget's
register_flash_entities.
What's your opinion?

Thanks,

Andrzej


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread 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
  +  

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

2011-11-03 Thread Andrzej Pietrasiewicz
On Thursday, November 03, 2011 11:33 AM Andrzej Pietrasiewicz wrote:

 
 Sorry about that. I forgot to mention the reference. It is
 http://patchwork.ozlabs.org/patch/122080
 
Sorry again. I meant http://patchwork.ozlabs.org/patch/122079

Andrzej


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread Stefan Schmidt
Hello.

On Thu, 2011-11-03 at 13:47, Andrzej Pietrasiewicz wrote:
 On Thursday, November 03, 2011 11:33 AM Andrzej Pietrasiewicz wrote:
 
  Sorry about that. I forgot to mention the reference. It is
  http://patchwork.ozlabs.org/patch/122080
  
 Sorry again. I meant http://patchwork.ozlabs.org/patch/122079

Hmm, applied this one but the DFU patch stills fails to apply in
goni.c and goni.h. Seems there is something else applied that is not
in mainline yet.

I can't test the goni part anyway as I don't have the hardware so I
will split it out of the patch and test the dfu command and its
implementation.

regards
Stefan Schmidt
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread Stefan Schmidt
Hello.

On Thu, 2011-11-03 at 09:12, Andrzej Pietrasiewicz wrote:
 On Wednesday, November 02, 2011 3:29 PM Stefan Shmidt wrote:
  
  Have you fully implemented 1.1? With the detahc logic inside the
  device implementation?
 
 As you noticed in another post, the state machine is reused.

Yes, sorry about this. I was fouled into thinking you did not as it
was splitted out and no mentioning of Harald copyright. Having the
same state machine is a good base for further cooperation on this. :)

  Just curious. What version of dfu-util your are using for your tests?
  I released 0.5 earlier today.
 
 I used what was available in my desktop's Ubuntu 10.04 LTS, and
 it happens to be version 0.1+svn :O It works, though.

Ah, latest Ubuntu has 0.3 and hopefully they will update as well.
Already pinged the maintainer.

  Hmm, that sounds like you only implemented the DFU mode but not the
  run-time mode yet?
 
 This conclusion is not correct.

Indeed. I stand corrected.

 After issuing the dfu command
 my implementation begins in runtime mode. Only after using dfu-util
 from the host is the mode changed to DFU mode.
 Actually, as far as usage of dfu-util is concerned, I find it
 inconvenient when the device is in runtime mode, because there
 (at least with my version of dfu-util) is no way for the user
 to guess what altsettings are actually available.

Correct. Sadly this is due to the standard only allowing the
functional descriptor in runtime mode. On the other hand the exposed
alt settings in run-time mode would clutter the USB device settings.

 I personally
 do dfu-util -U file -a 0 or a similar command to kick the device
 into DFU mode, then dfu-util -l to discover what altsettings there are.

I did run exactly in the same problem. :) I added a detach command to
dfu-util to switch between the different modes. From run-time to dfu
and from dfu to run-time. I wanted to get the release out before
bringing it in. Pushed it to the master branch of dfu-util now. Feel
free to test and let me know if it works for you.

 So, it could be tempting to omit the runtime mode at all and start
 in DFU mode, but, to be standard compliant, I start in runtime mode.
 How do you discover with dfu-util what altsettings there are in the
 device?

See above. Not available in run-time mode. :/

  While the original purpose
  of DFU was to make updating USB device easy enough for end-users as
  well.
 
 True, that's what the standard says about the very purpose of DFU.
 However, pressing keys to bring the device into a specific mode is
 probably very much device-specific. It would be nice if we
 are able to come up with a generic solution which only delegates
 the necessary details to board-specific files.

Agreed. This should not be handled in the dfu implementation at all.
Your split between dfu and flashing backend is a good step in the
right direction here. Invoking the dfu mode through a command is also
a nice addition.

   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.
  
  What is your reasoning behind this split? I have it working here with
  the normal nand_flashing routines taking care of bad blocks, etc.
 
 The reason is to factor out board-specific stuff from the dfu gadget
 implementation. However, in what I factored out probably there are
 parts which are generic enough not to be board-specific. Since there
 seems to be considerable interest in DFU implementation for u-boot,
 this could be reworked.

After thinking more about it I agree with you here. The split makes
sense and it will allow for more flexibility when adding more flashing
backend to the implementation.

  To make it clear, I'm happy to see somebody else working on this
 
 Whap! --- Oh, man, we are not alone! ;) I am happy to
 hear from someone else who works on this, too.

Good to know. :)

Brainstorming about a way going forward. Please comment if with your
view on this.

o I will send out my not ready for mainline patch to give you and
  others an impression how it is tackled in my patch.

o I like your split between dfu and flashing and also the addition of
  the dfu command. Could you split this part from the rest and send it
  as self conatining patch without other dependencies?

o And then one patch with the mmc/fat flashing backend in moved out of
  the board file (no idea what the best place is, maybe just another
  file for now) and a last patch with all the board specific changes?

o I will then rebase my code on yours and prepare patches against the
  dfu implementations as needed and also flashing backend for NAND.

o After this we need to review was is missing and bring it over.

Not all details covered in the plan but a start. Comments?

regards
Stefan Schmidt
___
U-Boot mailing 

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

2011-11-03 Thread Stefan Schmidt
Hello.

On Thu, 2011-11-03 at 09:44, Andrzej Pietrasiewicz wrote:
 On Wednesday, November 02, 2011 8:30 PM Stefan Schmidt writes:
  
  Agreed. The eMMC flashing with files on FAT is nothing goni specific.
  Others should be able to use this as well. I see three different parts
  here that can be separated:
 
 I agree. Since there is interest in DFU implementation this can and should
 be generalized.

Great!

  1) The DFU protocol implementation which lives in usb gadget with an
  interface for flashing backends
  
  2) The flashing backends (no idea where to put them best). At the
  moment that would be the implementation here with eMMC and files on FAT,
  mine with raw NAND devices and in the future maybe others. Each
  flashing backend should be generic enough to allow different boards
  using it.
  
  3) Board specific information like partitions or hints for the flashing
  backend. The information itself should be in the board file here and
  only used by the flashing backends.
  
  How does this sound to you? Andrzej?
 
 Sounds good to me. In my implementation the interface between dfu gadget
 and flashing backend is around the struct flash_entity.
 It contains a character string intended to provide a human-readable name,
 a void * context which is not interpreted by the gadget code,
 but is passed to the flashing backend and understood by it.
 The struct flash_entity also contains prepare-finish methods
 to be called before and after read/write operations, and the read_block-
 write_block pair. What do you think?

I would need to use it for my NAND backend before I really can comment
on it. I can only tell if I'm happy with and interface if I actually
used it. :)

Will do this when you send it a separate patch with only the DFU
implementation with the flash entity as interface. See my roadmap
proposal in the other mail.

 As far as generalization is concerned, in my flashing backend
 implementation I see these parts as candidates for generalization:
 
 1) mbr/ebr reading
 2) reading/writing mmc
 3) read/write fat

Agreed. That can be used by all kind of devices.

 4) generic prepare/finish; not sure if fat-specific prepare can be
 generalized

Would be good to get soem comments on the fs custodians around here.

 5) read/write_block

Agreed.

 6) some more work should be put to create an interface between the board
 initialization routine, the flashing backend and the DFU gadget
 implementation.
 In my implementation the board initialization routine calls board-specific
 register_flash_areas, which, in turn, calls DFU gadget's
 register_flash_entities.
 What's your opinion?

The interface between the configuration in the board file and the
actual flashing backend is something I haven't wraped my mind around
yet. Need to think about it.

regards
Stefan Schmidt
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread Andrzej Pietrasiewicz
Hello Stefan,

On Thursday, November 03, 2011 2:32 PM Stefan Schmidt wrote:

 
   Sorry about that. I forgot to mention the reference. It is
   http://patchwork.ozlabs.org/patch/122080
  
  Sorry again. I meant http://patchwork.ozlabs.org/patch/11122079
 
 Hmm, applied this one but the DFU patch stills fails to apply in
 goni.c and goni.h. Seems there is something else applied that is not
 in mainline yet.

Ok, here is the full story: this http://patchwork.ozlabs.org/patch/11122079
is the second patch in a series, the first being
http://patchwork.ozlabs.org/patch/122080 and you actually need both.

I pulled the latest git.denx.de tree and had to also apply a patch which
Defines CONFIG_SYS_CACHELINE_SIZE for Goni target. It can be found
here: http://article.gmane.org/gmane.comp.boot-loaders.u-boot/112755/match=
I know this is confusing. Sorry about that.

Andrzej



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-03 Thread 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. :)
  
  

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

2011-11-03 Thread Wolfgang Denk
Dear Andrzej Pietrasiewicz,

In message 1320228007-8947-1-git-send-email-andrze...@samsung.com you wrote:

 Device Firmware Upgrade (DFU) is an extension to the USB specification.
 As of the time of this writing it is documented at
 
 http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf
...
 Please refer to dfu-util documentation for details.
 The below ASCII-art presents a connection between the host and the device.
 
   +-+--- DFU ---+-+
   |  e.g. dfu-util  |=== USB ===|  / altsetting 0 |
   |  host   +-+   device - altsetting 1 |
   |   file /| |  \ ...  |
   +-+ +-+


Sorry for asking some probably really dumb questions...


If I understand correctly, there is no fundamental reason why the DFU
protocol would only be possible to implement over a USB link.  Or am I
missing something here?

Eventually it should be possible to run this protocol over Ethernet or
even over a serial line?

If my assumption is correct, then what would it take to split off
protocol part and make it independent of the actual driver interface?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Today is the yesterday you worried about tomorrow.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-02 Thread Stefan Schmidt
Hello.

This really comes as surprise to me as I'm working on exactly the same
at the moment. I just realised that I only mentioned it here inside the
fastboot patches thread.

I started from the old patches Harald Welte wrote for u-boot during
his work for OpenMoko. I worked with him during this time at OpenMoko
and I'm the current maintainer of dfu-util. (Just as background).

You seem to have started from scratch with this DFU implementation
instead of using the older but available patches. Was there a specific
reason for this?

I found it not that complicated to update them on a current u-boot and
adjust it accordingly. I have it working here with a beagleboard. Was
about to send them out next week but you have been a small tick faster
here. :)

From a quick look I see some stuff missing in your implementation that
is working in mine so there is some ground for working together on
this. The main question is which of both implementation to use to
build up from. I will give you some feedback on the design high level
aspects further below. A complete review of the code and testing on
the beagleboard will hopefully happen over the next days. Testing
might be problematic as you flashing part is forced to be rewritten
for every board here it seems. Need to investigate further before I
can really comment on this. More comment inline.

On Wed, 2011-11-02 at 11:00, Andrzej Pietrasiewicz wrote:

 http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf

Have you fully implemented 1.1? With the detahc logic inside the
device implementation?

 The aim of DFU is to allow transferring data to/from selected areas of
 interest within a compliant device. In the DFU nomenclature the host-device
 direction is called download and the device-host direction is called
 upload. The areas are defined by the compliant device. In the DFU
 nomenclature the area of interest within the device is called an altsetting.
 The device is connected to the host through USB. On the host side compliant
 software must be used to initiate the data transfer. Example piece of such
 software is dfu-util package from the OpenMoko project:
 
 http://wiki.openmoko.org/wiki/Dfu-util.

Better to use the official homepage here:
http://dfu-util.gnumonks.org/

It misses some examples but has the current informations while the
wiki is outdated.

 The DFU implementation on the device side remains in one of the two modes:
 the Run-Time mode and the DFU mode. The USB descriptor sets exported by
 the device depend on which mode is in force. While in DFU mode the device
 exports the descriptors corresponding to each available altsetting. An
 example dfu-util session when the device is in the DFU mode looks similar
 to this:
 
 # dfu-util -l
 dfu-util - (C) 2007-2008 by OpenMoko Inc.
 This program is Free Software and has ABSOLUTELY NO WARRANTY
 
 Found DFU: [0x0525:0x] devnum=46, cfg=0, intf=0, alt=0, name=bootldr
 Found DFU: [0x0525:0x] devnum=46, cfg=0, intf=0, alt=1, name=kernel

Just curious. What version of dfu-util your are using for your tests?
I released 0.5 earlier today.

 This u-boot implementation introduces a parameter-less dfu command.
 After the user finishes with dfu they can Ctrl-C to return to u-boot main
 menu.

Hmm, that sounds like you only implemented the DFU mode but not the
run-time mode yet? No addtional DFU descripto in the normal run-time
operation mode like usbtty? If yes this is a real limitation as the
update would only be possible when the user has serial access to start
the dfu command and then flash the device. While the original purpose
of DFU was to make updating USB device easy enough for end-users as
well. The implementation from OpenMoko i forward ported and cleanup
has this functionality and it works well enough. Did so on the
Freerunner from OM already.

In the end I would like to see different options available:

1) Entering DFU mode directly when a button is pressed on startup. An
update mode if you want to call it like this.

2) Normal startup, but with DFU descripton to allow dfu-util detaching
and entering the DFU mode for flashing.

3) Starting dfu mode form the command line as you did implement it.

 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.

What is your reasoning behind this split? I have it working here with
the normal nand_flashing routines taking care of bad blocks, etc.

I agree that there are several aspects that are board specific.
Partions can be read out dynamically and the alt setting generated
though. Soemthing what bothers me is the different underlaying medias
that can be flashed. Right now I only cover nand, but eMMC, USB and
other are valid as well. That is not covered in my implementation at
all right now.

To make it clear, I'm happy to see somebody else working on this as
well 

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

2011-11-02 Thread Mike Frysinger
On Wednesday 02 November 2011 06:00:07 Andrzej Pietrasiewicz wrote:
 Signed-off-by: Andrzej Pietrasiewicz andrze...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 
 Dear All,
 
 This is Device Firmware Upgrade (DFU) implementation which supports data
 upload and download function to devices which are equipped with a UDC.

this information belongs in the changelog (above the --- marker)

are you working with the elinux.org guys ?
http://elinux.org/Merge_DFU_support_into_mainline_U-Boot

  board/samsung/goni/Makefile |2 +
  board/samsung/goni/flash.c  |  634
  board/samsung/goni/flash.h  |   28 ++
  board/samsung/goni/goni.c   |   17 +
  common/Makefile |1 +
  common/cmd_dfu.c|   50 +++
  drivers/usb/gadget/Makefile |1 +
  drivers/usb/gadget/dfu.c|  920
  drivers/usb/gadget/dfu.h|  171
  include/configs/s5p_goni.h  |6 +
  include/dfu.h   |   31 ++
  include/flash_entity.h  |   39 ++
  include/mbr.h   |   49 +++

this should be split up into at least the dfu core and board-specific changes.  
although i'd wonder how much of the board/samsung/ stuff is really board 
specific and couldn't be generalized ...

 --- /dev/null
 +++ b/include/dfu.h

 +extern int usb_gadget_handle_interrupts(void);

this doesn't belong in the dfu header

 --- /dev/null
 +++ b/include/flash_entity.h

 +struct flash_entity {
 + char*name;

should probably be const

 --- /dev/null
 +++ b/include/mbr.h

 +/*
 + * Copyright (C) 2010 Samsung Electrnoics
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA
 + */
 +
 +#include linux/compiler.h

missing ifdef protection against multiple inclusion

this also should be split out of the dfu core ... although it seems weird that 
u-boot doesn't already have mbr parsing code ...
-mike


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-02 Thread Stefan Schmidt
Hello.

On Wed, 2011-11-02 at 11:16, Mike Frysinger wrote:
 On Wednesday 02 November 2011 06:00:07 Andrzej Pietrasiewicz wrote:
 
 are you working with the elinux.org guys ?
   http://elinux.org/Merge_DFU_support_into_mainline_U-Boot

That would be me. As I stated in my other mail I was surprised seeing
this patch coming in. I did not communicate my work on this broadly
either. Wanted to show up with soem real code not only promises. Same
as Andrzej it seems. :)

A quick comparison shows me that there are areas that don't touch each
other at all (mmc and images on fat as backend vs. raw nand flash as
backend) and areas that are duplicated and needed to get sorted out.
The DFU protocol implementation in this case. I hope Andrzej is
willing to work toegtehr with me on a final implementation that
contains pieces of both versions. Mine is missing a code split between
DFU protocol and flashing backend for example but feature other parts
missing here.

   board/samsung/goni/Makefile |2 +
   board/samsung/goni/flash.c  |  634
   board/samsung/goni/flash.h  |   28 ++
   board/samsung/goni/goni.c   |   17 +
   common/Makefile |1 +
   common/cmd_dfu.c|   50 +++
   drivers/usb/gadget/Makefile |1 +
   drivers/usb/gadget/dfu.c|  920
   drivers/usb/gadget/dfu.h|  171
   include/configs/s5p_goni.h  |6 +
   include/dfu.h   |   31 ++
   include/flash_entity.h  |   39 ++
   include/mbr.h   |   49 +++
 
 this should be split up into at least the dfu core and board-specific 
 changes.  
 although i'd wonder how much of the board/samsung/ stuff is really board 
 specific and couldn't be generalized ...

Agreed. The eMMC flashing with files on FAT is nothing goni specific.
Others should be able to use this as well. I see three different parts
here that can be separated:

1) The DFU protocol implementation which lives in usb gadget with an
interface for flashing backends

2) The flashing backends (no idea where to put them best). At the
moment that would be the implementation here with eMMC and files on
FAT, mine with raw NAND devices and in the future maybe others. Each
flashing backend should be generic enough to allow different boards
using it.

3) Board specific information like partitions or hints for the
flashing backend. The information itself should be in the board file
here and only used by the flashing backends.

How does this sound to you? Andrzej?

regards
Stefan Schmidt


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2011-11-02 Thread Stefan Schmidt
Hello.

@Remy: One question I have for you is if the DFU implementation should
be based on the re-written gadget layer from samsung or based on the
current one?

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?

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!

 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.

  COBJS-$(CONFIG_CMD_XIMG) += cmd_ximg.o
  COBJS-$(CONFIG_YAFFS2) += cmd_yaffs2.o
  
 diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
V new file mode 100644
 index 000..c85fbb1
 --- /dev/null
 +++ b/common/cmd_dfu.c
 @@ -0,0 +1,50 @@
 +/*
 + * Copyright (C) 2011 Samsung Electrnoics
 + * author: Andrzej Pietrasiewicz andrze...@samsung.com
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation; either version 2 of
 + * the License, or (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
 + * MA 02111-1307 USA
 + */
 +
 +#include common.h
 +#include command.h
 +#include dfu.h
 +#include flash_entity.h
 +
 +static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 +{
 + board_dfu_init();
 + dfu_init();
 + while (1) {
 + int irq_res;
 + /* Handle control-c and timeouts */
 + if (ctrlc()) {
 + printf(The remote end did not respond in time.\n);
 + goto fail;
 + }
 +
 + irq_res = usb_gadget_handle_interrupts();
 + }
 +fail:
 + dfu_cleanup();
 + board_dfu_cleanup();
 + return -1;
 +}
 +
 +U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
 + Use the DFU [Device Firmware Upgrade],
 + dfu - device firmware upgrade
 +);
 +

While I think a dfu command is usefull I don't like the need to
execute it before any DFU interaction can happen. That may be an
option during development but for field upgrades or receovery it is
not.

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.

 diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
 new file mode 100644
 index 000..535e194
 --- /dev/null
 +++ b/drivers/usb/gadget/dfu.c
 @@ -0,0 +1,920 @@
 +/*
 + * dfu.c -- Device Firmware Update gadget
 + *
 + * Copyright (C) 2011 Samsung Electronics
 + * author: Andrzej Pietrasiewicz andrze...@samsung.com
 + *
 + * Based on gadget zero:
 + * Copyright (C) 2003-2007 David Brownell
 + * All rights reserved.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program; if not, write to the Free Software
 + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 + */
 +