[U-Boot] [RFC] fastboot gadget support

[Sebastian Andrzej Siewior]

On 08/24/2011 09:34 PM, Remy Bohmer wrote:
> Hi,
Hi,

thanks for your time.

> 2011/8/10 Sebastian Andrzej Siewior<bigeasy at linutronix.de>:
>>
>> It was tested before it has been re-written. It is possible that it is
>> broken. It is posted for reference only not for merging.
>
> Looking at this code, I can already say that the way it is written is
> not suitable for mainline acceptance at all. If you at least would run
> the Linux checkpatch over it, it would result in an almost endless
> list of style warnings and errors ;-)

Sure. I just wanted to avoid spending more time on it there are some
high level mistakes.

> I may be missing something, but it appears to me that this particular
> patch for the fastboot framework makes several assumptions about:
> * if MMC is available (max 2 controllers, namely 0 or 1). Let' s say
> my hardware doe not support it, and I do not want to enable it due to
> codesize reasons...
Why only two (0 / 1)?
"mmcwrite:X Y" where X is the number of the controller and Y the
partition name do support multiple controllers. The erase command does
not it is "erase:Y" where Y is the partition name.
If you want a ifdef around mmc code so you can disable it, this can be
arranged :)

> * if NAND is available and it is either YAFFS or raw, what if I have UBI?
NAND formats are not part of the fastboot protocol. For UBI you could 
use the raw format. If you want to rescue the erase counters than a 
per-partition flag has to be added (like it is the case for write.i and 
write.yaffs).

> * Or let's drop the MMC and NAND, and assume we only have a harddrive
> or USB storage on our board ;-)
My understanding is that the MMC / NAND partition table is comming from
EFI. So if you your requirement is usb disk media than you have to set
interface.storage_medium to USB_STORAGE (or whatever we define it) and
use the proper write command then.

> * Hardcoded NAND block sizes which are evil.
This should be moved to a per-board configuration.

> * storage_medium is hardcoded set to NAND, never to EMMC, so what is
> the EMMC code doing here?

most of fastboot_init() should be handled per-board. This includes the
name of the product, storage media & type and storage area for the
download command.

> Furthermore:
> * Board code is mixed up with generic code.
Yes, this has to be split up.

> * drivers/usb/dwc3/fastboot_oem.c,
is a hook for custom / vendor extensions to the fastboot protocol. It
will be most likely moved to board specific code.

> drivers/usb/dwc3/misc.c,
Contains currently a hacky snprintf() which is not used by the fastboot 
code and should be part of the patch. sorry.

> drivers/usb/dwc3/sparse.c
this contained custom unsparse code. It seemed to be used as some kind
of RLE compression which did not make much sense and I left it out.

> contain code that has _nothing_ to do with
> USB.
Yes, it will be moved to proper places.

> * generic files (for example like include/linux/usb/ch9.h) are adapted
> with changes not even used by the code.
I have also a USB3 gadget in the pipe. I removed it prior the post but
forgot about some changes.

> * Mix up of different licenses: U-boot is still GPLv2, while this
> patch contains Apache based licenses (Not sure if it conflicts with
> GPL, but it seems strange)

Fastboot are based on BSD license. The sparse header file is the only
part under the Apache v2 license. Since it is not compatible with v2 I
leave the sparse code completely out.

> * it makes a lot of assumptions how the hardware looks like.
This has to be defined somewhere and will be moved to board specific
implementation.

> * It is not properly separated across different subsystems. There need
> to be a proper separation between drivers, library code, U-boot core
> code and board files. Everything that is board specific should go in
> board files.
Okay, will split

> NAND-availability or partitioning is board specific, MMC
> as well, the only assumption you can make about hardware that should
> always be available is RAM, you can only not assume how much there is,
> and which address area is free.
The protocol requires that the complete data is loaded into ram before
anything is doen with it. So what options do I have to achieve this? The
old code hardcoded size to 512/256 - 16 MiB on panda/blaze board. The
buffer started physram + 16MiB. Should I continue this approach?

> NAND and MMC usage should be
> completely configurable per board.
This will be done.

> * There need to be proper Documentation in the doc directory.
I try to add something.

> * It would be great to have at least a demo tool in /tools or a
> commonly used OSS package that provides the tools, such that the
> mechanism can be tested as well.

Okay. There is a so called fastboot tool in the wide android
repository. I could copy it as-it or add a link to the git repository.
Any preferences? That folder can be compiled on windows, linux and mac
osx. The linux part requires libusb and is easy to use. The Windows
edition requires a bunch of Android's SDK to get access to the USB
device.

>> This is the faastboot gadget code based on
>> git://git.omapzoom.org/repo/u-boot.git as of commit 601ff71c8 including
>> a few changes. Some of them are:
>> - generic mmc framework
>
> I have not found a ' generic'  mmc framework, only a 'fastboot'
> dedicated mmc framework.

How so? What I meant is that it depends on CONFIG_GENERIC_MMC and uses
do_mmcops commands for its needs. Is this appropriate or is another 
interface preferred?


> So, I have no objections to the protocol or the mechanism itself, as
> long as it is properly implemented.

I see. So I guess I have to stick with it.

Thanks for the review.

> Kind regards,
>
> Remy

Sebastian