On 08/24/2011 09:34 PM, Remy Bohmer wrote:
Hi,
Hi,
thanks for your time.
2011/8/10 Sebastian Andrzej Siewiorbigeasy@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