Re: [PATCH 01/14] bloblist: Update the tag numbering
> Ok, but if we do that we need to be careful with the standard. Things > like BLOBLISTT_U_BOOT_VIDEO dont feel U-Boot specific. The idea behind the Transfer List tag allocation policy is low friction allocation and organically emerging standards. You're not supposed to need to have big up-front debates about how exactly e.g. a video info descriptor should look like or whether it can be shared with other projects. Projects are just supposed to add what they need in the moment, in whatever format they prefer, and can call it something specific to that project to begin with (e.g. U_BOOT_VIDEO). Then, later, if other projects feel that this is a good format that would be worthwhile to reuse, they can just start using it. Just because it says U_BOOT or the ID number is close to the number for other U-Boot descriptors doesn't mean that it's not appropriate to be used anywhere else as long as the same structure with the same meaning makes sense there. If we eventually find that a bunch of different projects are all using this tag and it has become a de facto standard, we can also change the name to drop the U_BOOT (or different projects can even use different names for the same thing, that doesn't really matter as long as the ID and format matches). Or, if U-Boot eventually finds that this structure doesn't work well for them anymore they can just allocate a new U_BOOT_VIDEO2 and stop using the old one (or rename the old one to U_BOOT_VIDEO_DEPRECATED and call a new ID with a new format U_BOOT_VIDEO). Basically, the idea here is that upfront "perfect" design by committee tends to not work well for these things anyway and the friction it adds would be too much of a barrier to adoption. So it's better to not even try, just let everyone allocate what they want, and then later see what tends to work out well in practice and where there are opportunities for tag sharing between projects.
Re: [PATCH 14/14] bloblist: Update documentation and header comment
Here are a couple of other differences I have found between the bloblist code after applying your patches and the TL specification: * bloblist seems to explicitly disallow having the same tag more than once in the list (e.g. see documentation of bloblist_add()), whereas the TL specification explicitly allows that. You're of course free to impose this restriction on the way U-Boot uses the spec, but you may eventually run into compatibility issues if you hardcode that assumption and one day might ingest a TL from a different writer that doesn't adhere to it. * The bloblist_resize() function doesn't preserve alignment restrictions when relocating other TEs. I think the (only?) safe way to resize a TE in-place would be to align the distance that following TEs need to be moved up to (1 << hdr->align_log2), and then insert a void dummy TE to account for the additional added distance if necessary. * The comment at the top of bloblist.c should be updated to reflect how alignment and padding works in the TL spec. * The checksum algorithm seems incorrect. U-Boot's table_compute_checksum() subtracts bytes, but the TE spec says they need to be summed up. * The bloblist_reloc() function must be updated to follow the TL relocation mechanism (i.e. take hdr->align_log2 into account to make sure the new position preserves alignment requirements... I would say see https://github.com/FirmwareHandoff/firmware_handoff/blob/main/source/transfer_list.rst#relocating-a-tl for details, but I just noticed that we made a mistake there, so please check the version from https://github.com/FirmwareHandoff/firmware_handoff/pull/12 for the corrected algorithm).
Re: [PATCH 13/14] bloblist: Add alignment to bloblist_new()
I'm not sure why you would add an API to allow setting this explicitly, that's not really how it is supposed to work according to the spec. New TLs are always supposed to start with an alignment requirement of 8 (2^3) and then automatically increase that value if a TE gets added that has a larger requirement. There should be no use case where you'd want to initially create a TL that already has a larger number in that field (it doesn't make any difference... in particular, note that just because the alignment field has a larger value doesn't mean that the start of the TL must be aligned to that larger boundary; the field is only used to preserve the offset from the next alignment boundary during relocation).
Re: [PATCH 11/14] bloblist: Reduce blob-header size
> - rec->tag = tag; > - rec->hdr_size = sizeof(struct bloblist_rec); > + rec->tag_and_hdr_size = tag | sizeof(*rec) << > BLOBLISTR_HDR_SIZE_SHIFT; nit: Might be a good idea to mask the tag or double-check that it fits the field size here. > - * 5. Bloblist uses 16-byte alignment internally and is designed to start on > a > - * 16-byte boundary. Its headers are multiples of 16 bytes. This makes it > easier > - * to deal with data structures which need this level of alignment, such as > ACPI > - * tables. For use in SPL and TPL the alignment can be relaxed, since it can > be > - * relocated to an aligned address in U-Boot proper. > + * 5. Bloblist uses 8-byte alignment internally and is designed to start on a > + * 8-byte boundary. Its headers are 8 bytes long. It is possible to achieve > + * larger alignment (e.g. 16 bytes) by adding a dummy header, For use in SPL > and nit: I would call it a "dummy entry", it's not always just a header. > + BLOBLIST_BLOB_ALIGN_LOG2 = 3, > + BLOBLIST_BLOB_ALIGN = 1 << BLOBLIST_BLOB_ALIGN_LOG2, > + > BLOBLIST_ALIGN_LOG2 = 4, > BLOBLIST_ALIGN = 1 << BLOBLIST_ALIGN_LOG2, > }; > @@ -181,17 +184,25 @@ struct bloblist_hdr { Note that there's no specific requirement for the TL header to be aligned to 16 bytes. Even though it is 16 bytes long, 8 bytes alignment is enough (so you shouldn't really need a BLOBLIST_ALIGN that's different from BLOBLIST_BLOB_ALIGN). There's also some text above this in the docstring for this struct that refers to 16 bytes and should be updated. (I would recommend also updating the "it can be safely moved around" part to point to the instructions for relocation in the firmware_handoff spec, since while it can be relocated, special care must be taken to preserve alignment restrictions.
Re: [PATCH 10/14] bloblist: Handle alignment with a void entry
> /* Calculate the new allocated total */ > - new_alloced = data_start + ALIGN(size, 1U << align_log2); > + new_alloced = data_start - map_to_sysmem(hdr) + > + ALIGN(size, 1U << align_log2); I think this is incorrect. There's no requirement that the size of an entry must also be aligned as strictly as its start offset. So if someone calls this code as bloblist_addrec(tag, 16, 8, ptr), then it will try to create a blob at a 256 byte boundary with only 16 bytes of data size, which is perfectly legal, but this code here will set new_alloced as if the data size was also 256. That's not correct and would likely throw off calculations elsewhere later. The alignment to the start of the next entry is always just 8 bytes, so this line should use BLOBLIST_BLOB_ALIGN_LOG2 (or sizeof(*rec)) instead of align_log2. > if (new_alloced > hdr->size) { > log_err("Failed to allocate %x bytes size=%x, need size=%x\n", > @@ -153,7 +168,7 @@ static int bloblist_addrec(uint tag, int size, int > align_log2, > rec = (void *)hdr + hdr->alloced; > > rec->tag = tag; > - rec->hdr_size = data_start - hdr->alloced; > + rec->hdr_size = sizeof(struct bloblist_rec); > rec->size = size; You also need to update the TL header alignment field if the requested alignment here is greater, e.g. something like if (hdr->alignment < align_log2) hdr->alignment = align_log2;
Re: [PATCH 01/14] bloblist: Update the tag numbering
> diff --git a/include/bloblist.h b/include/bloblist.h > index 7ea72c6bd46..bad5fbbb889 100644 > --- a/include/bloblist.h > +++ b/include/bloblist.h nit: I would suggest also updating the documentation at the top of this file (point 7) to clarify that new standardized tags must be allocated in the firmware_handoff repo? Also, to point out the obvious, there are a bunch of tags in the standardized range in this file that don't match the firmware_handoff spec. I guess you could add them since 0x100 is still free. However, at least the BLOBLISTT_ACPI_TABLES tag would likely(?) be redundant with the existing XFERLIST_ACPI_AGGR tag. > + BLOBLISTT_U_BOOT_SPL_HANDOFF= 0xfff000, /* Hand-off info from SPL > */ > + BLOBLISTT_VBE = 0xfff001, /* VBE per-phase state */ > + BLOBLISTT_U_BOOT_VIDEO = 0xfff002, /* Video info from SPL */ FWIW, according to my view of the tag allocation philosophy, I think these kinds of tags should be allocated as official tags in the standardized range (e.g. in a cluster of U-Boot-specific tags). I would really recommend completely avoiding the non-standardized range for anything other than local experimentation or tags internal to closed-source code.
Re: [TF-A] Re: [RFC] Proposed location to host the firmware handoff specification.
> OK, this seems to be the crux of the problem. Is it possible for us say that > users are free to register new TLs, while at the same time recommending the > use of existing formats for complex structures (e.g. FDT, HOB)? I'm not really sure what qualifies as a "complex structure" in this discussion? I think each individual TE should deal with a single concern, and separate concerns should be implemented via separate TEs (IIRC I tried to propose language in that direction in my PR)... so I agree that TEs shouldn't be "complex" (if this is what you mean by that), but I think the solution to that is just to divide the complexity down into a larger number of more simple TEs. I also agree that usually each TE should be representable by a simple C structure like I think Simon mentioned somewhere (although I think that should be a recommendation instead of an absolute requirement in case projects have long-established data structures that they want to continue using... the FDT and HOB list TEs aren't representable by a C structure either, after all). > > On the other hand, if this is just supposed to be an early boot flow > > extension to FDTs, then that's very different from what I understood the > > goal > > to be and then I think the text of the spec should be honest about that > > front > > and center. In that case, I wouldn't expect much adoption beyond the > > ecosystem that's already deeply tied to FDT to begin with, though, and that > > would be far from "universal". > > That is another valid usage model. Some ecosystems are deeply wedded to FDT > or HOB/ACPI (there may be others) and this spec is not going to change that. > I don't think we're going to get something universal in the way you're > hoping. But we might be able to at least enable better > interoperability/reusability between fw components across different > ecosystems. Sure, I didn't mean to say this should be disallowed, but the question is whether that is the only allowed use and the FDT is required to pass certain data, or whether adopters are free to choose how they represent their data and the FDT tag is just one of many. Like you said there really needs to be maintainer consensus about this that must be written down somewhere. I don't believe there's going to be a lot of sharing between projects either to be honest, but ultimately that's what this whole thing was started for, right? I think in practice the most likely opportunities for sharing are going to be from small pieces that all projects need, like a TE defining which serial console to print debug output on. (That's another good reason to keep TEs simple, single-concern, and directly encoding data instead of wrapping another structure. In practice I assume that the projects that mostly rely on the wrapped HOB list or FDT will duplicate a few pieces of data into separate TEs to be able to share those with components that can't parse the larger structure.) > 1. A tag that is intended to be used for information passed between > different firmware projects. Such as from TF-A to EDK2 and u-boot. > > 2. A tag for _internal_ use within a firmware project. Here a > firmware project should be free to do whatever they want, but they > still will likely want to use tag IDs that will not conflict with > other uses. I don't see the value of cluttering the firmware > handoff spec with layouts internal to specific firmware projects. This solves the accidental overlap concern but not the organically emerging standard concern. I guess we could say that a tag from the "vendor" range can be promoted to "standard" at a later point by adding it to the global spec (with its existing ID in the vendor range). This does hurt discoverability though, i.e. it's harder for someone who is trying to implement a new TE to realize that the problem has already been solved by another project in a way that would also work out well for them. I still think it's important that tag layouts must be immutable and cannot change once allocated if they come out of that range, that's the basis for any interoperability and backwards-compatibility. And the best way to ensure that is to have them all listed in a global location. Otherwise, even if you write down somewhere that layouts mustn't change, tags shouldn't be reused after they get deprecated, etc., the fact that nobody would notice if you do will encourage people to silently do it anyway. I agree that we don't want to have all of them clutter a single .rst file once there are hundreds or thousands, but there are ways to organize that better which we can decide on once we get to that point.
Re: [TF-A] [RFC] Proposed location to host the firmware handoff specification.
It seems like some of us still have very different opinions about how this handoff structure should and shouldn't be used, which I really think need to be worked out and then codified in the spec before we can call the document final, because otherwise no firmware project can trust that it is safe to adopt this. My understanding was that this is supposed to be a universal handoff structure that's free to be used by any open source firmware project for any of its internal purposes at any stage of the boot flow as a standalone solution that doesn't force them to pull in dependencies to any other format. If that is the case then I think the spec should reflect this very clearly and should particularly not contain any language about deferring certain types of data to other handoff structures wrapped in the TL. It needs to be clear that projects will be allowed to freely register tags for their needs without the risk of suddenly getting told by the maintainers that they need to use an FDT for this or that instead. On the other hand, if this is just supposed to be an early boot flow extension to FDTs, then that's very different from what I understood the goal to be and then I think the text of the spec should be honest about that front and center. In that case, I wouldn't expect much adoption beyond the ecosystem that's already deeply tied to FDT to begin with, though, and that would be far from "universal".
Re: [TF-A] [RFC] Proposed location to host the firmware handoff specification.
Okay, FWIW I created a pull request with my suggestions here: https://github.com/FirmwareHandoff/firmware_handoff/pull/4 That should make it easier to discuss specific details, hopefully. As I was looking at the header size and format more closely I noticed that the checksum calculation and some details about what the size fields mean seems underspecified right now, so I suggested a change for that as well. Let me know what you think. > One way to enforce that is for firmware components to refuse to accept > a transfer list with unknown tags, assuming that all firmware is built > from source at the same time. It might be a pain, but I suspect it > could help avoid security problems, where bad actors use the structure > to pass code to a firmware blob, etc. I don't think that's feasible, particularly if tag IDs are not allocated in order (which I think is useful so that we can try to group similar tags by vendor, e.g. if say MediaTek wanted to add some tags they could just find some nice round number in the tag space, say 0x200, and then start allocating 0x201, 0x202, 0x203, etc. while the next vendor can do their own thing at 0x300 instead). It also creates automatic incompatibilities if you put newer and older components in the same image, which I think is something we explicitly don't want. I think the right policy to encourage compatibility is that unknown tags just get ignored. > I have to disagree here. The context is gone in this email, so I > cannot reply to the comments there. But I think it was talking about > using the transfer list for very tiny structures, just a few words. > That is not the intent. Firstly we should really have TF-A use the > device tree (embedded in the transfer list) instead of replicating > bl_params in this structure. Secondly, we should group things into a C > structure that is at least 16-32 bytes long. Uhh... okay, sorry, but then you're going against everything that was discussed in this thread and in the previous discussions last year. The whole idea of making this a tagged list came from the argument that other structures (like FDT) are unnecessarily bulky and complex for some of the intended use cases (like replacing TF-A bl_aux_params). I mean, what's the point of even having a transfer list if all it does is wrap an FDT? You could just use the FDT directly! I think all the 4 tags that are currently specified in that document are pathological edge cases that may be necessary for legacy applications but represent the opposite of how this structure *should* be used in the ideal case. We don't need just more wrappers for the stuff that we already have, that doesn't promote interoperability in any way. If project A only wants to use HOBs but not FDT, and project B only wants to use FDT but not HOBs, then creating a transfer list structure that can wrap both an FDT and a HOB in a TE is pointless and not going to fix anything (project A will still not be able to parse the FDT-wrapping TE, and project B will still not be able to parse the HOB-wrapping TE). The only way this whole exercise can actually be useful and improve anything is if we can use it to eventually (slowly) get away from both HOBs and FDTs towards a point where all data is encoded in TEs directly, without any extra layers. > So I don't see a need for a smaller header. The 16-byte alignment > works well with ACPI tables and we should not be using this structure > for tiny things where the 8-byte overhead is significant. FWIW, the compromise proposal now preserves the 16-byte TE alignment. (8 byte header, then small entries can use the next 8 bytes for data and the whole TE is only 16 bytes with only up to 7 bytes of forced padding. Once you need more than 8 bytes of data it gets a bit more ugly but at that point I guess you're not really a super small entry anymore.)
Re: [TF-A] [RFC] Proposed location to host the firmware handoff specification.
Hi Jose, Apologies for the late response, I had to find some time to dig back into this topic first. > The proposal is that the tag assignments are handled via a PR to [1]. > A PR should provide reasoning for the proposed entry layout as well as a > description of the use-case being serviced. > The community is invited to comment on the pull requests. We should aim to > have community consensus before merging any PR. I think this is a good approach if we understand "community consensus" to be a quick, couple-of-days review cycle like we have for TF-A patches that doesn't create a huge threshold for additions, and it's fine for entries to be very specific to the needs of a single platform when necessary. But I think the document should still have more guidance about when standard vs. non-standard tags should be used, and in particular just strongly discourage the use of non-standard tags in general (basically I think they should only be used for experiments that won't reach production code, or for cases where all code reading or writing that tag is closed source). I think the spec should work really hard in its language and guidance to discourage a situation where firmware components just assign a non-standard tag to something because it's easier and avoids the hassle of submitting something you don't think you'd want to share at the time, and then later it becomes entrenched and reused and eventually everyone has to pay attention to not reuse the non-standard tags that accidentally acquired a fixed meaning in enough implementations to become a problem. (It may also make sense to just make the standard tag range much bigger than the non-standard range, to make it clearer that the majority of things are expected to go in there.) > It seems sensible that data contained in an entry should not exceed 32MB. > Could we still accommodate the notion of a hdr_size in the TL entry header? > We'd have the following fields on an 8-byte TE header: > tag_id -- 4 bytes > hdr_size -- 1 byte > Data_size -- 3 bytes Yeah, that seems like a reasonable compromise, if you really think that variable header length is useful. I still think the table header could be smaller as well (down to 16 bytes from 32 by just avoiding so much reserved space). Do you want me to send those suggestions as pull requests to make them easier to discuss? > The document states that any tag in the standard range must be allocated in > the spec before being used in code [4] -- what this is really saying is: > until a tag id is allocated anyone can request it. So code written assuming a > particular standard tag id (absent from the spec) may have to change later if > someone in the meantime allocates that tag id for a different entry type. > I believe it’s a per-community policy to prevent code submissions containing > standard tag ids until these have been reserved in this spec. > This is the advised policy, but this spec cannot mandate it, rather we must > rely on the communities to enforce this. I would say the spec should mandate that, honestly. If there is a standardized range, no firmware implementing transfer lists according to this spec should use tags from that range until they have been defined in the global repository. On the flip side, we need to make the process of defining new tags fast and painless so that this requirement doesn't become a burden. > For the current set of entries, the data is included within the TL. Note that > this does not prevent newer entries from pointing to off-TL data. > Could this be handled on a case-by-case basis, via pull request, after an > initial full release of the spec? Yes, we can leave it like this and later add tags for the off-list equivalent of the tags defined there if necessary.
Re: [TF-A] [RFC] Proposed location to host the firmware handoff specification.
> That draft is the DEN0135 document [2]. > I realise that I haven’t made it sufficiently explicit in this thread that > the DEN0135 document [2] is still at draft quality (see ALP maturity called > out in the title page and footers). > Please do not consider this a finished document . We’ve been iterating on > this pdf as we gather feedback from the open-source communities. > It's becoming clear that we need to move this debate to a more suitable forum. > That’s why we’re proposing, in this thread, to move the document to > trustedfirmware.org. Understood. FWIW, I totally support hosting this on TF.org, I think that makes sense considering that it has good co-development infrastructure that is open to everyone (e.g. Gerrit) already set up and that it's easy to administer for you guys who are currently organizing this. I don't really see Simon's concern regarding "captive to one project" -- this whole spec is an attempt to cooperate better, not work against each other, so there shouldn't need to be fights about who "owns" it (just pick a place with good infrastructure and let everyone access it there). If perhaps the real concern is rather related to who controls future spec updates, tag assignments, etc. then I think we should address those directly and define clear processes that we can all feel comfortable with, rather than implying that the final word on these is left to whoever controls the hosting platform. > The requirement originated in feedback from the u-boot community, please see > [3]. > The argument for 16-byte alignment is that some data-structures have to be > 16-byte aligned. > We ought to meet that requirement if we ever want to carry those structures. I still really think that this is wasting space to the point where it defeats the purpose of this being a "lightweight" data handoff structure. Consider that all of the bl_aux_params tags in TF-A (which I described as one of the existing mechanisms for this kind of thing in last year's thread) are 8 bytes or less, which means that for these use cases the transfer list would have over 3 times as much overhead as actual data. There seems to be some disconnect here since the stated idea for this is to be a lightweight handoff mechanism to pass individual pieces of information directly, yet all the initially proposed tags are just for wrapping large existing hand off blobs that bundle lot of information in their own structures (giving it a very xkcd.com/927-like feel), and the spec design seems to be tuned toward the latter use cases. If individual tags have special alignment requirements, they could just deal with that internally (by including another offset field in the "data" part) without dragging every other tag in the structure down with them. Alternatively, if you really want to keep this I think an acceptable compromise could be to keep the 16-byte entry alignment but reduce the transfer entry header size to 8 bytes, and then allow tags that don't require 16 byte data alignment to immediately use the following 8 bytes for data. That way, entries with 8 bytes or less of data could fit both header and data into a single 16 byte block which drastically cuts down on overhead for small data. > > 2. The table entry header seems unnecessarily large. What's the point of > > including a "header length" field when that field will always contain a 16? > > The "header length" enables the table entry header to be extended if we ever > need that in the future. > Changes to the TE header are backwards compatible. > A consumer obtains the address of the data by adding the "header length" to > the entry's base address. I really don't see this as likely enough to be worth paying that big upfront cost of making the whole structure so much less efficient to start with. I think 8 bytes are already the most you'd reasonably want to pay for header overhead, I find it highly improbable that we'd want to introduce more fields later to make that even bigger. If there's desire for optional large annotations (e.g. per-entry cryptographic hashes) in the future, I'd rather suggest implementing that as "companion entries" with special tags that are written right behind the entry they refer to, rather than try squeezing it all into a single header structure. If you want to leave some room for simple bit flags, I would suggest masking out the high byte of the data length field instead (i.e. splitting it into 3 and 1) -- it doesn't seem like anyone would want to pass a data item larger than 32 MB in a firmware information structure anyway. > The TL header version is incremented when we do a new release of the document. > At a new document release, we can introduce one or more new standard tags and > also append fields to the TL header. FWIW I would suggest decoupling the data structure version from the document version and only increasing the data structure version when the actual structure changes in a meaningful way. Documents may be amended to
Re: [TF-A] [RFC] Proposed location to host the firmware handoff specification.
Hi Jose, Can you provide a bit more background on where this proposal suddenly came from and what considerations went into its design? The mailing list discussion from last year sort of petered out without much concrete agreement, and now you're here with a full specification. It's hard to provide feedback on a finished document that doesn't include any justification for the design choices it makes and without knowing what concrete requirements may have influenced it. With that caveat (of not knowing why you made the choices you did and what good reasons you may have had), here are the main concerns I came up with on a first read: 1. Where did the requirement for 16-byte alignment come from? It seems a bit excessive considering that no data field is actually larger than 4 bytes (and it looks like it influenced several other choices in the spec, e.g. unnecessarily large transfer entry header because the bytes to the alignment boundary have to be filled anyway). For something that's supposed to be the "lightweight" alternative to more elaborate formats, this design doesn't really feel that lightweight anymore. 2. The table entry header seems unnecessarily large. What's the point of including a "header length" field when that field will always contain a 16? I think you could easily get away with half the size here (just 4 bytes tag and 4 bytes data size). (While we're at it, the whole table header is also a bit large... do we really expect to need 2^32 possible versions? You could easily shrink that down to 16 bytes.) 3. It's not exactly clear what the table header's "version" field is supposed to mean. What exactly would cause us to increase the version? What should a reader do when it reads an unknown version field? It would probably be a good idea for the final document to specify that in more detail, and I'd maybe consider splitting the version number in a major version and minor version byte (major version = fully incompatible to older readers; minor version = added meaning to previously reserved fields, but older readers can continue to read the fields they know about). 4. I don't think a strict definition of entry type ranges is a good idea, particularly if "standard" and "OEM" are the only two options. In my experience such systems (e.g. same situation with ARM SMC FIDs) just mean that nothing ever gets added in the "standard" bucket because the barrier to inclusion is way too high, and so in practice everyone will throw every use case they have into the remaining free-for-all bucket ("OEM" in this case), which quickly leads to a mess of overlapping IDs and in the end nobody can ever share a tag between two different platforms even if they actually end up needing the same thing. I think you can't expect things like this to self-organize in advance, and the only way it can work in practice is that things which were originally added by one contributor become a de-facto standard over time as others adopt them. The way to allow and promote that would be to have a simple, low-barrier registration mechanism for new tags -- e.g. just by submitting a patch to the repository that also holds the spec itself which would just need the tag name, ID, and simple specification of the contained data, open for everyone and every use case they might have. With 4 billion tags, I think we wouldn't need to worry about running out any time soon. This would allow all the different firmware projects and hardware vendors using this standard to iterate quickly on their own needs, deprecate tags and add new versions as needed, and if over time specific tags become widely used/useful de-facto standards other stakeholders can start supporting them without having to worry about tag clashes. (If you want more organization than just one big free-for-all bucket you could still have that, of course... e.g. allocate smaller chunks of 1K tags each to each firmware project or hardware vendor or whatever as needed. But the important part is that the barrier to adding new tags should be low and the chunks should not overlap, so the 1K tags allocated to U-Boot should be different from the 1K tags allocated to TF-A, and if U-Boot comes up with a good tag definition where we end up seeing that it would be useful if TF-A reused the same format, we can just use that tag without risk of it already being defined as something else in TF-A context.) 5. Embedding large data structures like a full FDT or ACPI table directly by value in this structure seems like a bad idea and too inflexible in practice. These kinds of structures may be generated at different points in the boot process (some of which may or may not use the transfer list standard), they may need to be modified (including growing/shrinking in size) at various points, and they may need to be placed in special locations in memory. Besides, the transfer list design requires it to be copied in various cases (e.g. to expand beyond max_length), so it seems primarily suited for small
Re: [SPECIFICATION RFC v3] The firmware and bootloader log specification
> Since it doesn't seem possible to have each boot component using the same log > format, we added a log_format and log_phys_addr fields to give flexibility in > how logs are stored. An example of a different log format that can be used is > the cbmem_console log format used by coreboot: I am not exactly sure how you expect this interoperability you seem to be suggesting here to work. Are you saying that your bf_log_header can sometimes point to the bf_log_buffer structure you define, and sometimes to a coreboot CBMEM console buffer? But that's a completely different format that requires a different reader implementation, how is that supposed to work? If this proposal is just "we define a wrapper structure that points to everyone's custom firmware log implementation", then I don't really see the point (the only benefit still left then might be discovery of the log buffer, but that's the part you leave open in your design while all those other implementations already have working discovery mechanisms of their own anyway). For the other structures you have defined, the same feedback that I think was already mentioned on the last iteration of this thread still applies: it seems incredibly bloated for a simple firmware logging mechanism. You have a whooping 24+n bytes of overhead *per line* which probably comes out to somewhere between 30-50% of total space wasted on overhead for the average log buffer. I guess there are just fundamentally different opinions on how featureful a firmware log mechanism needs to be so we're probably not gonna find a format that makes everyone happy here, but at least for the coreboot project I see little reason for us to implement something like this when we already have a well-working existing solution with tooling and wideranged support.
Re: [TF-A] Proposal: TF-A to adopt hand-off blocks (HOBs) for information passing between boot stages
> - fuller implementation with more features Is that a good thing? Didn't we just have a long discussion eschewing a heavy-handed, bulky hand-off block design in favor of more simple and flexible structures? I think simplicity is key for this and the bl_aux_params are trying to be about as simple as they can possibly get. Particularly in a situation like this where many different projects will need to implement this independently, any extra clutter (like versioning, an apparently unused(?) flags field, checksumming) adds unnecessary extra friction that's best to avoid. (You can *do* all these things like adding an extra checksum over the whole thing with bl_aux_params by just defining an extra tag for it, of course, but none of them are required for platforms for which they don't make sense.) > - has comments / more documentation Is there anything in particular you feel is missing from the bl_aux_params comments? > - easily supports everything in one block instead of a linked list (easier to > allocate) I don't understand how this is easier, in fact I see it as a big drawback. The bl_aux_params design allows structures to be allocated wherever -- in the easiest case you can just define them as global variables in the earlier stage (in the respective files that deal with each tag) and then chain those together. You *can* of course also allocate them in a contiguous block from a special memory area, and if you're planning to pass this across more than one stage boundary that's probably the better choice. But not all platforms have that requirement, and this design allows them the flexibility to choose. The coreboot platforms only care about the BL2->BL31 transition, so why should they switch to a design that forces us to set up an extra contiguous-block allocator for no good reason? > - avoids 64-bit tags/size which seem quite unnecessary Well, earlier there were some concerns about possible tag collisions, so I think the extra space doesn't hurt to alleviate those fears. Having 64 bits allows us to split the tag space into many separate ranges that can then be allocated from independently -- for example, we could just say that the top 2^63 tags are available for private vendor tags generated as random numbers (like a GUID). (Your bloblist seems to waste space on things like a "spare" field instead, so in the end both implementations still come out to the same overhead per tag, and bloblist has additional overhead for the header at the front.) > - ability to link to another bloblist (do we really need this?) FWIW you can do this by default with bl_aux_params because they're not required to be contiguous.
Re: [TF-A] Proposal: TF-A to adopt hand-off blocks (HOBs) for information passing between boot stages
> - "bloblist" is a general term for concept of linked list and it's not > exactly U-boot implementation. The proposed solution will cause some degree > of changes in all the participating projects. For backward compatibility > issue, we have already though about it and proposed to have build configs > which will be enabled by platform integrators. Okay, let's gather the requirements and then decide how the existing system needs to be expanded to meet them. Please keep me in the loop on those discussions. > > U-Boot has an established mechanism which is used in particular on all > > chrome > > books in both x86 and Arm environments. I have the impression that > > U-Boot is the typical BL33 so I would import the mechanism into TFA, > > not the other way round. There has been no Chromebook (x86 or Arm) shipping with U-Boot since 2013 (before TF-A existed). In fact, Chromebooks are all running coreboot today and using the bl_aux_params mechanism (it's 3 SoC vendors, more than 3 SoCs, but the parameter code is shared at the vendor level), and one of my concerns here is trying to limit disruption to those existing setups. I don't have enough data to say what the "typical" boot flow among all TF-A devices in existence is, but at least for the millions of Chromebooks out there it is coreboot + TF-A, not U-Boot.
Re: [TF-A] Proposal: TF-A to adopt hand-off blocks (HOBs) for information passing between boot stages
> Of course every project would like not to change... > > For TF-A I wonder whether it will/should in fact use devicetree if there is a > lot of complex data? TBD, I suppose. Okay, sorry, now I'm a bit confused -- I thought the discussion in this thread was about which parameter hand-off mechanism to use *for TF-A*? Or did it shift to discuss other projects in the meantime (I didn't always follow it closely)? I think it started with the UEFI guys wanting to implement a HOB format to interface with TF-A, and I think we now concluded that they're okay with using a simple parameter list instead (and wrapping their custom HOBs into a parameter blob that contains their UUID and everything else in the data part). So for TF-A, if the decision is that we want a parameter list, I think it makes sense to keep using the format that has already been there and in use for several years, and define new tags for the UEFI HOB use case in that. I don't really see a reason to switch TF-A and all other projects currently interfacing with it that way (e.g. coreboot) to something only used by U-Boot right now, if they're practically identical in concept.
Re: [TF-A] Proposal: TF-A to adopt hand-off blocks (HOBs) for information passing between boot stages
> > Thank you all for your feedback. > > It appears that in theory we are all happy with using bloblist with few > implementation details which needs to be taken care of during > implementation. > Just to clarify: are you using "bloblist" as a general term for the concept of a simple linked list of tagged data blobs, or to refer specifically to the U-Boot implementation with that name? The existing TF-A implementation (bl_aux_params) is basically identical in concept to the U-Boot bloblist, but not binary compatible. Are we talking about just keeping that, or throwing it away in order to reimplement the exact structure U-Boot is using? (I would prefer to keep the bl_aux_params as they are to avoid disrupting existing uses, of course. Making backwards-incompatible changes to an interface that passes across multiple repos and firmware components is always a big pain.)
Re: [TF-A] Proposal: TF-A to adopt hand-off blocks (HOBs) for information passing between boot stages
> These structures we are talking about may not be defined by the SiP, and just saying we have SiP specific tags is not good enough. There is enough room in 64-bits to create separate tag spaces for every kind of category you can think of. (In fact, we could also just let every vendor allocate their globally unique tags instead if we prefer that model. We're not going to have more than 4 billion entities working on this together, and none of them will need more than 4 billion types of tags.) > If we use tags, then we must agree on who owns/dictates the tag list, and > how we add/claim tags for vendor specific purposes and avoid tag > collisions. > > > > I have not heard a solution to that without establishing some owner / > standards body to allocate tags. Until we do, tags are not going to work > for the types of platform/system agnostic features we must support. > Why not just define the tags in the TF-A upstream repository? Like Simon said, TF-A needs to maintain a list of them anyway to be able to interpret them, so why not just make that list the global authoritative source? This doesn't need to be some complicated bureaucracy. The TF-A headers defining this are written in a way that they can directly be included from other C code bases without include dependency issues (e.g. as a Git submodule, or by just copying them verbatim and resyncing regularly... this is the include/export thing Manish mentioned). I think that model actually makes coordination much easier than if a bunch of different projects needed to manually reimplement the same structure definitions tied to UUIDs.
Re: [TF-A] Proposal: TF-A to adopt hand-off blocks (HOBs) for information passing between boot stages
Hi Raghu, > It appears there are opinions you carry around UUID being complicated, > bloated, code being an eyesore, parsing these lists early with MMU/Caches > disabled, calculating checksums etc. While there is certainly a LOT of > truth to those statements, these concerns need to be put into context and > may not necessarily apply to all platforms running TF-A. Standardization > and interoperability may be valued more on some platforms and some of the > bloat and performance issues may be worth those tradeoffs. Some of these > concerns may not even apply on powerful CPU’s and SoC’s with lots of > memory. Also a quick grep for “uuid” in the TF-A repository shows that > there is significant use of UUID’s for FIP, some SMC’s, secure partitions > etc so use of UUID is not something new to TF-A. > Granted, I understand that different platforms have different requirements, and there may not be a one-size-fits-all solution for this. But if you have a system with a powerful CPU and lots of memory that doesn't care that much about boot speed, why wouldn't you just use FDT? I thought FDT is already established on many platforms to pass information between TF-A boot stages today, and it should certainly be flexible enough to fill your needs. Most of the earlier discussion in this thread seemed to focus on people wanting a different mechanism besides FDT because FDT is complex and not performant enough for all use cases, but then why would we replace it with a different mechanism that also has complexity and performance problems? Basically, as far as I'm aware there are two established mechanisms to pass data between boot stages in TF-A today (some platforms, like Rockchip's, even support both of them at once): passing an FDT and the bl_aux_params linked list. They are intentionally on opposite ends of the complexity spectrum because different platforms can have different needs. The question is really just whether we need to add a third one, and whether that offers anything that the other two can't already cover. I guess we can support the exact UEFI HOB format just to make things easier for UEFI users, and maybe that's okay, but it does add maintenance burden to TF-A to support so many different things and we should at least consider if the UEFI part couldn't just translate its stuff into an FDT instead. But mainly, the way this thread started it sounded more like "we want to standardize on a new HOB format that will be used by everyone", and for that goal, I think sticking with what we have would make a lot more sense for TF-A. If UEFI HOB support was added in addition then I think only UEFI users would really gain any benefit from that, and that mechanism would only end up getting used by UEFI platforms (which again, maybe that's okay, but I'm just saying I don't think this is going to become some great new "one HOB format to rule them all" design if it doesn't do anything better than the established FDT format already does). > Does the method mentioned below allow you to create a dynamic list of > arbitrary lengths and NOT lists of lengths decided at compile time? It > appears that it is mainly being used on lists fixed at compile time. > The bl_aux_params list elements don't need to have a fixed size. The only common thing about each element is the tag (and the next pointer), everything else (including size) is tag-specific. You can just define a new tag where the next uint64_t after the next pointer defines the size of the rest of the element or something like that. Heck, you can even have the list element contain pointers to other random stuff in memory that isn't directly part of the list. (The whole thing is not consecutive in memory anyway. It's not designed to be a serializable, persistent thing like an FDT blob, just something that a calling bootloader stage can quickly assemble in its own memory right before handoff and that will only stay valid until the next stage is done parsing it and starts reusing available memory for its own purposes.) > Clearly the solution below was tailored to coreboot. > > Not really, it was just the simplest flexible design I could come up with at the point. The APIs upstream is in the TF-A repo and coreboot (and other software) can include it from there.
Re: [TF-A] Proposal: TF-A to adopt hand-off blocks (HOBs) for information passing between boot stages
Just want to point out that TF-A currently already supports a (very simple) mechanism like this: https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/+/refs/heads/master/include/export/lib/bl_aux_params/bl_aux_params_exp.h https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/+/refs/heads/master/lib/bl_aux_params/bl_aux_params.c https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/+/refs/heads/master/plat/rockchip/common/params_setup.c It's just a linked list of tagged elements. The tag space is split into TF-A-wide generic tags and SiP-specific tags (with plenty of room to spare if more areas need to be defined -- a 64-bit tag can fit a lot). This is currently being used by some platforms that run coreboot in place of BL1/BL2, to pass information from coreboot (BL2) to BL31. I would echo Simon's sentiment of keeping this as simple as possible and avoiding complicated and bloated data structures with UUIDs. You usually want to parse something like this as early as possible in the passed-to firmware stage, particularly if the structure encodes information about the debug console (like it does for the platforms I mentioned above). For example, in BL31 this basically means doing it right after moving from assembly to C in bl31_early_platform_setup2() to get the console up before running anything else. At that point in the BL31 initialization, the MMU and caches are disabled, so data accesses are pretty expensive and you don't want to spend a lot of parsing effort or calculate complicated checksums or the like. You just want something extremely simple where you ideally have to touch every data word only once. On Wed, Mar 24, 2021 at 5:06 PM Simon Glass via TF-A < t...@lists.trustedfirmware.org> wrote: > Hi Harb, > > On Wed, 24 Mar 2021 at 11:39, Harb Abdulhamid OS < > abdulha...@os.amperecomputing.com> wrote: > >> Hello Folks, >> >> Appreciate the feedback and replies on this. Glad to see that there is >> interest in this topic. >> >> >> >> I try to address the comments/feedback from Francois and Simon below…. >> >> >> >> @François Ozog – happy to discuss this on a >> zoom call. I will make that time slot work, and will be available to >> attend April 8, 4pm CT. >> >> >> >> Note that I’m using the term “HOB” here more generically, as there are >> typically vendor specific structures beyond the resource descriptor HOB, >> which provides only a small subset of the information that needs to be >> passed between the boot phases. >> >> >> >> The whole point here is to provide mechanism to develop firmware that we >> can build ARM Server SoC’s that support **any** BL33 payload (e.g. EDK2, >> AptioV, CoreBoot, and maybe even directly boot strapping LinuxBoot at some >> point). In other-words, we are trying to come up with a TF-A that would >> be completely agnostic to the implementation of BL33 (i.e. BL33 is built >> completely independently by a separate entity – e.g. an ODM/OEM). >> >> >> >> Keep in mind, in the server/datacenter market segment we are not building >> vertically integrated systems with a single entity compiling >> firmware/software stacks like most folks in TF-A have become use to. There >> are two categories of higher level firmware code blobs in the >> server/datacenter model: >> >>1. “SoC” or “silicon” firmware – in TF-A this may map to BL1, BL2, >>BL31, and **possibly** one or more BL32 instances >>2. “Platform” or “board” firmware – in TF-A this may map to BL33 and * >>*possibly** one or more BL32 instances. >> >> >> >> Even the platform firmware stack could be further fragmented by having >> multiple entities involved in delivering the entire firmware stack: IBVs, >> ODMs, OEMs, CSPs, and possibly even device vendor code. >> >> >> >> To support a broad range of platform designs with a broad range of memory >> devices, we need a crisp and clear contract between the SoC firmware that >> initializes memory (e.g. BL2) and how that platform boot firmware (e.g. >> BL33) gathers information about what memory that was initialized, at what >> speeds, NUMA topology, and many other relevant information that needs to be >> known and comprehended by the platform firmware and eventually by the >> platform software. >> >> >> >> I understand the versatility of DT, but I see two major problems with DT: >> >>- DT requires more complicated parsing to get properties, and even >>more complex to dynamically set properties – this HOB structures may need >>to be generated in boot phases where DDR is not available, and therefore >> we >>will be extremely memory constrained. >>- DT is probably overkill for this purpose – We really just want a >>list of pointers to simple C structures that code cast (e.g. JEDEC SPD >> data >>blob) >> >> >> >> I think that we should not mix the efforts around DT/ACPI specs with what >> we are doing here, because those specs and concepts were developed for a >>
Re: [SPECIFICATION RFC] The firmware and bootloader log specification
Standardizing in-memory logging sounds like an interesting idea, especially with regards to components that can run on top of different firmware stacks (things like GRUB or TF-A). But I would be a bit wary of creating a "new standard to rule them all" and then expecting all projects to switch what they have over to that. I think we all know https://xkcd.com/927/. Have you looked around and evaluated existing solutions that already have some proliferation first? I think it would be much easier to convince people to standardize on something that already has existing users and drivers available in multiple projects. In coreboot we're using a very simple character ring buffer that only has two 4-byte header fields: total size and cursor (i.e. current position where you would write the next character). The top 4 bits of the cursor field are reserved for flags, one of which is the "overflow" flag that tells you whether the ring-buffer already overflowed or not (so readers know whether to print the whole ring buffer, or only from the start to the current cursor). We try to dimension the buffers so they don't overflow on a single boot, but this approach allows us to log multiple boots on platforms that can persist memory across reboots, which sometimes comes in handy. The disadvantages of that approach compared to your proposal are lack of some features, like the facilities field (although one can still just print a tag like "<0>" or "<4>" behind each newline) or timestamps (coreboot instead has separate timestamp logging). But I think a really big advantage is size: in early firmware environments before DDR training, space is often very precious and we struggle to find more than a couple of kilobytes for the log buffer. If I look at the structure you proposed, that's already 24 bytes of overhead per individual message. If we were hooking that up to our normal printk() facility in coreboot (such that each invocation creates a new message header), that would probably waste about a third of the whole log buffer on overhead. I think a complicated, syslog-style logging structure that stores individual message blocks instead of a continuous character string isn't really suitable for firmware logging. FWIW the coreboot console has existing support in Linux (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/google/memconsole-coreboot.c), SeaBIOS (https://github.com/coreboot/seabios/blob/master/src/fw/coreboot.c#L219), TF-A (https://github.com/ARM-software/arm-trusted-firmware/blob/master/drivers/coreboot/cbmem_console/aarch64/cbmem_console.S), GRUB (https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/term/i386/coreboot/cbmemc.c), U-Boot (https://github.com/u-boot/u-boot/blob/master/drivers/misc/cbmem_console.c) and probably a couple of others I'm not aware of. And the code to add support (especially when only appending) is so simple that it just takes a couple of lines to implement (binary code size to implement the format is also always a concern for firmware environments). On Wed, Nov 18, 2020 at 7:04 AM Heinrich Schuchardt wrote: > > On 14.11.20 00:52, Daniel Kiper wrote: > > Hey, > > > > This is next attempt to create firmware and bootloader log specification. > > Due to high interest among industry it is an extension to the initial > > bootloader log only specification. It takes into the account most of the > > comments which I got up until now. > > > > The goal is to pass all logs produced by various boot components to the > > running OS. The OS kernel should expose these logs to the user space > > and/or process them internally if needed. The content of these logs > > should be human readable. However, they should also contain the > > information which allows admins to do e.g. boot time analysis. > > > > The log specification should be as much as possible platform agnostic > > and self contained. The final version of this spec should be merged into > > existing specifications, e.g. UEFI, ACPI, Multiboot2, or be a standalone > > spec, e.g. as a part of OASIS Standards. The former seems better but is > > not perfect too... > > > > Here is the description (pseudocode) of the structures which will be > > used to store the log data. > > Hello Daniel, > > thanks for your suggestion which makes good sense to me. > > Why can't we simply use the message format defined in "The Syslog > Protocol", https://tools.ietf.org/html/rfc5424? > > > > > struct bf_log > > { > > uint32_t version; > > char producer[64]; > > uint64_t flags; > > uint64_t next_bf_log_addr; > > uint32_t next_msg_off; > > bf_log_msg msgs[]; > > As bf_log_msg is does not have defined length msgs[] cannot be an array. > > > } > > > > struct bf_log_msg > > { > > uint32_t size; > > uint64_t ts_nsec; > > uint32_t level; > > uint32_t facility; > > uint32_t msg_off; > > char strings[]; > > } > > > > The members of struct bf_log: > >
Re: [PATCH] lz4: fix decompressor on big-endian powerpc
> I'm a bit puzzled about the github.com/Cyan4973/lz4 > reference. Cyan4973 is Yann Collet, the author of lz4, and nowadays at > least that github url redirects to the upstream repo, > github.com/lz4/lz4. Grepping through all available tags and branches > there, the identifiers "has_content_checksum" and "lz4_frame_header" have > never appeared anywhere in the source (and he uses camelCase). That comment refers to the lz4.c file that's included by the line right below it. That was copied 1-to-1 from Yann's code. The lz4_wrapper.c file was written from scratch by me. Reviewed-by: Julius Werner
[U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images
The Linux ramdisk should always be decompressed by the kernel itself, not by U-Boot. Therefore, the 'compression' node in the FIT image should always be set to "none" for ramdisk images, since the only point of using that node is if you want U-Boot to do the decompression itself. Yet some systems populate the node to the compression algorithm used by the kernel instead. This used to be ignored, but now that we support decompression of all image types it becomes a problem. Since ramdisks should never be decompressed by U-Boot anyway, this patch adds a special exception for them to avoid these issues. Still, setting the 'compression' node like that is wrong in the first place, so we still want to print out a warning so that third-party distributions doing this can notice and fix it. Signed-off-by: Julius Werner --- common/image-fit.c| 13 + test/py/tests/test_fit.py | 10 +- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index e346fed550..5c63c769de 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1998,10 +1998,11 @@ int fit_image_load(bootm_headers_t *images, ulong addr, comp = IH_COMP_NONE; loadbuf = buf; /* Kernel images get decompressed later in bootm_load_os(). */ - if (!(image_type == IH_TYPE_KERNEL || - image_type == IH_TYPE_KERNEL_NOLOAD) && - !fit_image_get_comp(fit, noffset, ) && - comp != IH_COMP_NONE) { + if (!fit_image_get_comp(fit, noffset, ) && + comp != IH_COMP_NONE && + !(image_type == IH_TYPE_KERNEL || + image_type == IH_TYPE_KERNEL_NOLOAD || + image_type == IH_TYPE_RAMDISK)) { ulong max_decomp_len = len * 20; if (load == data) { loadbuf = malloc(max_decomp_len); @@ -2021,6 +2022,10 @@ int fit_image_load(bootm_headers_t *images, ulong addr, memcpy(loadbuf, buf, len); } + if (image_type == IH_TYPE_RAMDISK && comp != IH_COMP_NONE) + puts("WARNING: 'compression' nodes for ramdisks are deprecated," +" please fix your .its file!\n"); + /* verify that image data is a proper FDT blob */ if (image_type == IH_TYPE_FLATDT && fdt_check_header(loadbuf)) { puts("Subimage data is not a FDT"); diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py index 8009d2907b..e3210ed43f 100755 --- a/test/py/tests/test_fit.py +++ b/test/py/tests/test_fit.py @@ -269,6 +269,11 @@ def test_fit(u_boot_console): def check_equal(expected_fname, actual_fname, failure_msg): """Check that a file matches its expected contents +This is always used on out-buffers whose size is decided by the test +script anyway, which in some cases may be larger than what we're +actually looking for. So it's safe to truncate it to the size of the +expected data. + Args: expected_fname: Filename containing expected contents actual_fname: Filename containing actual contents @@ -276,6 +281,8 @@ def test_fit(u_boot_console): """ expected_data = read_file(expected_fname) actual_data = read_file(actual_fname) +if len(expected_data) < len(actual_data): +actual_data = actual_data[:len(expected_data)] assert expected_data == actual_data, failure_msg def check_not_equal(expected_fname, actual_fname, failure_msg): @@ -435,7 +442,8 @@ def test_fit(u_boot_console): output = cons.run_command_list(cmd.splitlines()) check_equal(kernel, kernel_out, 'Kernel not loaded') check_equal(control_dtb, fdt_out, 'FDT not loaded') -check_equal(ramdisk, ramdisk_out, 'Ramdisk not loaded') +check_not_equal(ramdisk, ramdisk_out, 'Ramdisk got decompressed?') +check_equal(ramdisk + '.gz', ramdisk_out, 'Ramdist not loaded') cons = u_boot_console -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] fit image: boot gzipped ramdisk does not work anymore
> OK. So lets special case ramdisk to just note that we see it's > compressed ("compression: %s, ignored" ?) and then we'll find out if > there's really a bunch of cases where people assumed it was decompressed > by us or not. Sounds good! I'll send a patch. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] fit image: boot gzipped ramdisk does not work anymore
> Well, I'm sure you'd find enough FIT images where the uncompression > might succeed but the target space for decompression is not big enough > (see bootm_size), would such an error be fixed as well? I think so? Assuming all the compression algorithms check for an output buffer overrun (looks like they do), that should also just lead to a compression error and go into the fallback path. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] fit image: boot gzipped ramdisk does not work anymore
> First, we have a problem as we need to support what's out in the world. How about I write a patch to change the decompression call in fit_image_load() to fall back to a memcpy() if the decompression fails (and print a big warning that the FIT image is wrong and should be updated)? That should keep those old installations working. > Second, what is intended with what's out in the world? Indeed before we > didn't do anything with the compressed data. But now what, we're saying > it's compressed, but not providing enough details to U-Boot to > uncompress it? Hmm... I'm not sure why it wouldn't just decompress correctly, actually. Is there anything special with how gzipped data is formatted for the kernel decompressor? Or is there any limitation in the U-Boot gzip implementation? ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v5 1/3] common: Move bootm_decomp_image() to image.c (as image_decomp())
> OK, this now causes 'make tests' to fail on the FIT image tests, please > look. Thanks! Whoops, sorry, should have run that again. This was just a printf() without a newline at the end, apparently pytest really doesn't like that. (I took out the "OK" at the end of the "Loading kernel..." message since with this patch decompression errors are supposed to be handled by the callers and there are separate error messages in those anyway.) Fixed in v6. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v6 2/3] fit: Support compression for non-kernel components (e.g. FDT)
This patch adds support for compressing non-kernel image nodes in a FIT image (kernel nodes could already be compressed previously). This can reduce the size of FIT images and therefore improve boot times (especially when an image bundles many different kernel FDTs). The images will automatically be decompressed on load. This patch does not support extracting compatible strings from compressed FDTs, so it's not very helpful in conjunction with CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments that select the configuration to load explicitly. Signed-off-by: Julius Werner Reviewed-by: Simon Glass --- - Changes for v2: - Changed from only supporting compressed FDTs to supporting all non-kernel image node types. - Changes for v3: - Fixed up some debug output that was still written for v1. - Fixed a mistake with handling FIT_LOAD_OPTIONAL_NON_ZERO when 'load' was 0 (i.e. unset). - Added compression test case to the test_fit pytest. - No changes for v4 - No changes for v5 - No changes for v6 common/image-fit.c| 86 +++ test/py/tests/test_fit.py | 29 +++-- 2 files changed, 77 insertions(+), 38 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index a74b44f298..c9ffc441aa 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -22,6 +22,7 @@ DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/ +#include #include #include #include @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void *fdt) kfdt_name); continue; } + + if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" (compressed)\n", + kfdt_name); + continue; + } + /* * Get a pointer to this configuration's fdt. */ @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr, const char *fit_uname_config; const char *fit_base_uname_config; const void *fit; - const void *buf; + void *buf; + void *loadbuf; size_t size; int type_ok, os_ok; - ulong load, data, len; - uint8_t os; + ulong load, load_end, data, len; + uint8_t os, comp; #ifndef USE_HOSTCC uint8_t os_arch; #endif @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, images->os.arch = os_arch; #endif - if (image_type == IH_TYPE_FLATDT && - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { - puts("FDT image is compressed"); - return -EPROTONOSUPPORT; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); type_ok = fit_image_check_type(fit, noffset, image_type) || fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); /* get image data address and length */ - if (fit_image_get_data_and_size(fit, noffset, , )) { + if (fit_image_get_data_and_size(fit, noffset, + (const void **), )) { printf("Could not find %s subimage data!\n", prop_name); bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); return -ENOENT; @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong addr, #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) /* perform any post-processing on the image data */ - board_fit_image_post_process((void **), ); + board_fit_image_post_process(, ); #endif len = (ulong)size; - /* verify that image data is a proper FDT blob */ - if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) { - puts("Subimage data is not a FDT"); - return -ENOEXEC; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK); - /* -* Work-around for eldk-4.2 which gives this warning if we try to -* cast in the unmap_sysmem() call: -* warning: initialization discards qualifiers from pointer target type -*/ - { - void *vbuf = (void *)buf; - - data = map_to_sysmem(vbuf); - } - + data = map_to_sysmem(buf); + load = data; if (load_op == FIT_LOAD_IGNORED) { /* Don't load */ } else if (fit_image_get_load(fit, noffset, )) { @@ -1974,8 +1963,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, } } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { ulong imag
[U-Boot] [PATCH v6 3/3] fit: Support compat string property in configuration node
This patch adds support for an optional optimization to compatible string matching where the compatible string property from the root node of the kernel FDT can be copied into the configuration node of the FIT image. This is most useful when using compressed FDTs or when using FDT overlays, where the traditional extraction of the compatible string from the kernel FDT itself is not easily possible. Signed-off-by: Julius Werner Reviewed-by: Simon Glass --- - No changes for v2 - No changes for v3 - Changes for v4: - Added documentation for compatible string in config node. - Added example .its file for compressed FDT with compat string in config node. - No changes for v5 - No changes for v6 common/image-fit.c| 67 - doc/uImage.FIT/kernel_fdts_compressed.its | 73 +++ doc/uImage.FIT/source_file_format.txt | 7 +++ 3 files changed, 119 insertions(+), 28 deletions(-) create mode 100644 doc/uImage.FIT/kernel_fdts_compressed.its diff --git a/common/image-fit.c b/common/image-fit.c index c9ffc441aa..e346fed550 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1522,6 +1522,10 @@ int fit_check_format(const void *fit) * compatible list, "foo,bar", matches a compatible string in the root of fdt1. * "bim,bam" in fdt2 matches the second string which isn't as good as fdt1. * + * As an optimization, the compatible property from the FDT's root node can be + * copied into the configuration node in the FIT image. This is required to + * match configurations with compressed FDTs. + * * returns: * offset to the configuration to use if one was found * -1 otherwise @@ -1554,55 +1558,62 @@ int fit_conf_find_compat(const void *fit, const void *fdt) for (noffset = fdt_next_node(fit, confs_noffset, ); (noffset >= 0) && (ndepth > 0); noffset = fdt_next_node(fit, noffset, )) { - const void *kfdt; + const void *fdt; const char *kfdt_name; - int kfdt_noffset; + int kfdt_noffset, compat_noffset; const char *cur_fdt_compat; int len; - size_t size; + size_t sz; int i; if (ndepth > 1) continue; - kfdt_name = fdt_getprop(fit, noffset, "fdt", ); - if (!kfdt_name) { - debug("No fdt property found.\n"); - continue; - } - kfdt_noffset = fdt_subnode_offset(fit, images_noffset, - kfdt_name); - if (kfdt_noffset < 0) { - debug("No image node named \"%s\" found.\n", - kfdt_name); - continue; - } + /* If there's a compat property in the config node, use that. */ + if (fdt_getprop(fit, noffset, "compatible", NULL)) { + fdt = fit;/* search in FIT image */ + compat_noffset = noffset; /* search under config node */ + } else {/* Otherwise extract it from the kernel FDT. */ + kfdt_name = fdt_getprop(fit, noffset, "fdt", ); + if (!kfdt_name) { + debug("No fdt property found.\n"); + continue; + } + kfdt_noffset = fdt_subnode_offset(fit, images_noffset, + kfdt_name); + if (kfdt_noffset < 0) { + debug("No image node named \"%s\" found.\n", + kfdt_name); + continue; + } - if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { - debug("Can't extract compat from \"%s\" (compressed)\n", - kfdt_name); - continue; - } + if (!fit_image_check_comp(fit, kfdt_noffset, + IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" " + "(compressed)\n", kfdt_name); + continue; + } - /* -* Get a pointer to this configuration's fdt. -*/ - if (fit_image_get_data(fit, kfdt_noffset, , )) { - debug("Failed to get fdt \"%s\".\n", kfdt_name); -
[U-Boot] [PATCH v6 0/3] fit: Image node compression
This patch series adds compression support for non-kernel FIT image nodes (e.g. FDTs). The first patch is a preparatory refactoring, the second adds the compression support itself, and the third adds a new feature to compatible string matching that allows it to be useful with compressed FDTs. Sandbox-tested with FIT images with and without compressed FDTs, with and without overlays, in both compatible string matching and direct config selection modes. Also expanded the test_fit pytest to include a case with compressed kernel, FDT and ramdisk. Julius Werner (3): common: Move bootm_decomp_image() to image.c (as image_decomp()) fit: Support compression for non-kernel components (e.g. FDT) fit: Support compat string property in configuration node - Changes for v6 - Fixed test_fit.py (had a printf() without a newline) - Changes for v5 - Added first patch to support tools-only_defconfig (which doesn't build bootm.c) - Changes for v4 - Added documentation - Changes for v3 - Added tests, fixed a few minor issues - Changes for v2 - Changed to support all non-kernel image types (not just FDT) common/bootm.c| 148 +++--- common/image-fit.c| 143 - common/image.c| 111 doc/uImage.FIT/kernel_fdts_compressed.its | 73 +++ doc/uImage.FIT/source_file_format.txt | 7 + include/bootm.h | 17 --- include/image.h | 17 +++ test/compression.c| 24 ++-- test/py/tests/test_fit.py | 29 - 9 files changed, 351 insertions(+), 218 deletions(-) create mode 100644 doc/uImage.FIT/kernel_fdts_compressed.its -- 2.22.0.657.g960e92d24f-goog ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v6 1/3] common: Move bootm_decomp_image() to image.c (as image_decomp())
Upcoming patches want to add decompression to use cases that are no longer directly related to booting. It makes sense to retain a single decompression routine, but it should no longer be in bootm.c (which is not compiled for all configurations). This patch moves bootm_decomp_image() to image.c and renames it to image_decomp() in preparation of those upcoming patches. Signed-off-by: Julius Werner --- - First version: v5 - Changes for v6 - Fixed test_fit.py (had a printf() without a newline) common/bootm.c | 148 ++--- common/image.c | 111 ++ include/bootm.h| 17 -- include/image.h| 17 ++ test/compression.c | 24 5 files changed, 160 insertions(+), 157 deletions(-) diff --git a/common/bootm.c b/common/bootm.c index bea516025f..5509b341ff 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -7,17 +7,12 @@ #ifndef USE_HOSTCC #include #include -#include #include #include #include #include #include #include -#include -#include -#include -#include #if defined(CONFIG_CMD_USB) #include #endif @@ -299,23 +294,6 @@ static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc, } #endif /* USE_HOSTC */ -/** - * print_decomp_msg() - Print a suitable decompression/loading message - * - * @type: OS type (IH_OS_...) - * @comp_type: Compression type being used (IH_COMP_...) - * @is_xip:true if the load address matches the image start - */ -static void print_decomp_msg(int comp_type, int type, bool is_xip) -{ - const char *name = genimg_get_type_name(type); - - if (comp_type == IH_COMP_NONE) - printf(" %s %s ... ", is_xip ? "XIP" : "Loading", name); - else - printf(" Uncompressing %s ... ", name); -} - /** * handle_decomp_error() - display a decompression error * @@ -325,16 +303,18 @@ static void print_decomp_msg(int comp_type, int type, bool is_xip) * * @comp_type: Compression type being used (IH_COMP_...) * @uncomp_size: Number of bytes uncompressed - * @unc_len: Amount of space available for decompression - * @ret: Error code to report - * @return BOOTM_ERR_RESET, indicating that the board must be reset + * @ret: errno error code received from compression library + * @return Appropriate BOOTM_ERR_ error code */ -static int handle_decomp_error(int comp_type, size_t uncomp_size, - size_t unc_len, int ret) +static int handle_decomp_error(int comp_type, size_t uncomp_size, int ret) { const char *name = genimg_get_comp_name(comp_type); - if (uncomp_size >= unc_len) + /* ENOSYS means unimplemented compression type, don't reset. */ + if (ret == -ENOSYS) + return BOOTM_ERR_UNIMPLEMENTED; + + if (uncomp_size >= CONFIG_SYS_BOOTM_LEN) printf("Image too large: increase CONFIG_SYS_BOOTM_LEN\n"); else printf("%s: uncompress error %d\n", name, ret); @@ -352,93 +332,6 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size, return BOOTM_ERR_RESET; } -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, - void *load_buf, void *image_buf, ulong image_len, - uint unc_len, ulong *load_end) -{ - int ret = 0; - - *load_end = load; - print_decomp_msg(comp, type, load == image_start); - - /* -* Load the image to the right place, decompressing if needed. After -* this, image_len will be set to the number of uncompressed bytes -* loaded, ret will be non-zero on error. -*/ - switch (comp) { - case IH_COMP_NONE: - if (load == image_start) - break; - if (image_len <= unc_len) - memmove_wd(load_buf, image_buf, image_len, CHUNKSZ); - else - ret = 1; - break; -#ifdef CONFIG_GZIP - case IH_COMP_GZIP: { - ret = gunzip(load_buf, unc_len, image_buf, _len); - break; - } -#endif /* CONFIG_GZIP */ -#ifdef CONFIG_BZIP2 - case IH_COMP_BZIP2: { - uint size = unc_len; - - /* -* If we've got less than 4 MB of malloc() space, -* use slower decompression algorithm which requires -* at most 2300 KB of memory. -*/ - ret = BZ2_bzBuffToBuffDecompress(load_buf, , - image_buf, image_len, - CONFIG_SYS_MALLOC_LEN < (4096 * 1024), 0); - image_len = size; - break; - } -#endif /* CONFIG_BZIP2 */ -#ifdef CONFIG_LZMA - case IH_COMP_LZMA: { - SizeT lzma_len =
Re: [U-Boot] [PATCH v5 1/3] common: Move bootm_decomp_image() to image.c (as image_decomp())
> I'd like to review this, but maybe you can help me speed up the process and > tell > us if the move has been a 1:1 code move or if you had to adapt things to the > new > location (other than the function being renamed)? print_decomp_msg() is a 1:1 move. bootm_decomp_image() is mostly 1:1 move, except that I changed the return code for unimplemented compression type (from BOOTM_ERR_UNIMPLEMENTED to -ENOSYS) and removed the call to handle_decomp_error() (handle_decomp_error() is specific to the BOOTM_ERR error types which are only used within bootm.c, so I decoupled it from image_decomp() and made all the call sites in bootm.c call it explicitly). ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v5 0/3] fit: Image node compression
> This kind of interleaved change log is kind of hard to read. A higher-level, > manually written changelog for the cover letter would be better I think. Sorry, will rewrite the cover letter next time. For v5 I just added the new patch and didn't change the other two. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v5 3/3] fit: Support compat string property in configuration node
> If there haven't been any changes from v4, why did you drop Simon Glass's > Reviewed-by tag? Same here: Reviewed-by: Simon Glass ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v5 2/3] fit: Support compression for non-kernel components (e.g. FDT)
> If there haven't been any changes from v4, why did you drop Simon Glass's > Reviewed-by tag? Sorry, I wasn't aware I was supposed to preserve those manually. Yes, there were no changes so this should still be considered Reviewed-by: Simon Glass ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4 1/2] fit: Support compression for non-kernel components (e.g. FDT)
Hi Tom, > This breaks tools-only_defconfig in non-trivial ways due to calling > bootm_decomp_image() which is not normally available to it. Can you > please update things (it may just end up being common/Makefile that > needs tweaking) so that tools-only builds? I'll then take a look and > make sure we don't also see unexpected size growth elsewhere. Thanks! Thanks for pointing that out! I think the best solution here would be to move bootm_decomp_image() out of bootm.c, if we now want to have it available in configurations that don't compile that file. I think image.c would be a better home for it. Added another patch to move the function and sent out v5 of the series, let me know what you think! ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v5 3/3] fit: Support compat string property in configuration node
This patch adds support for an optional optimization to compatible string matching where the compatible string property from the root node of the kernel FDT can be copied into the configuration node of the FIT image. This is most useful when using compressed FDTs or when using FDT overlays, where the traditional extraction of the compatible string from the kernel FDT itself is not easily possible. Signed-off-by: Julius Werner --- - No changes for v2 - No changes for v3 - Changes for v4: - Added documentation for compatible string in config node. - Added example .its file for compressed FDT with compat string in config node. - No changes for v5 common/image-fit.c| 67 - doc/uImage.FIT/kernel_fdts_compressed.its | 73 +++ doc/uImage.FIT/source_file_format.txt | 7 +++ 3 files changed, 119 insertions(+), 28 deletions(-) create mode 100644 doc/uImage.FIT/kernel_fdts_compressed.its diff --git a/common/image-fit.c b/common/image-fit.c index c9ffc441aa..e346fed550 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1522,6 +1522,10 @@ int fit_check_format(const void *fit) * compatible list, "foo,bar", matches a compatible string in the root of fdt1. * "bim,bam" in fdt2 matches the second string which isn't as good as fdt1. * + * As an optimization, the compatible property from the FDT's root node can be + * copied into the configuration node in the FIT image. This is required to + * match configurations with compressed FDTs. + * * returns: * offset to the configuration to use if one was found * -1 otherwise @@ -1554,55 +1558,62 @@ int fit_conf_find_compat(const void *fit, const void *fdt) for (noffset = fdt_next_node(fit, confs_noffset, ); (noffset >= 0) && (ndepth > 0); noffset = fdt_next_node(fit, noffset, )) { - const void *kfdt; + const void *fdt; const char *kfdt_name; - int kfdt_noffset; + int kfdt_noffset, compat_noffset; const char *cur_fdt_compat; int len; - size_t size; + size_t sz; int i; if (ndepth > 1) continue; - kfdt_name = fdt_getprop(fit, noffset, "fdt", ); - if (!kfdt_name) { - debug("No fdt property found.\n"); - continue; - } - kfdt_noffset = fdt_subnode_offset(fit, images_noffset, - kfdt_name); - if (kfdt_noffset < 0) { - debug("No image node named \"%s\" found.\n", - kfdt_name); - continue; - } + /* If there's a compat property in the config node, use that. */ + if (fdt_getprop(fit, noffset, "compatible", NULL)) { + fdt = fit;/* search in FIT image */ + compat_noffset = noffset; /* search under config node */ + } else {/* Otherwise extract it from the kernel FDT. */ + kfdt_name = fdt_getprop(fit, noffset, "fdt", ); + if (!kfdt_name) { + debug("No fdt property found.\n"); + continue; + } + kfdt_noffset = fdt_subnode_offset(fit, images_noffset, + kfdt_name); + if (kfdt_noffset < 0) { + debug("No image node named \"%s\" found.\n", + kfdt_name); + continue; + } - if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { - debug("Can't extract compat from \"%s\" (compressed)\n", - kfdt_name); - continue; - } + if (!fit_image_check_comp(fit, kfdt_noffset, + IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" " + "(compressed)\n", kfdt_name); + continue; + } - /* -* Get a pointer to this configuration's fdt. -*/ - if (fit_image_get_data(fit, kfdt_noffset, , )) { - debug("Failed to get fdt \"%s\".\n", kfdt_name); - continue; +
[U-Boot] [PATCH v5 2/3] fit: Support compression for non-kernel components (e.g. FDT)
This patch adds support for compressing non-kernel image nodes in a FIT image (kernel nodes could already be compressed previously). This can reduce the size of FIT images and therefore improve boot times (especially when an image bundles many different kernel FDTs). The images will automatically be decompressed on load. This patch does not support extracting compatible strings from compressed FDTs, so it's not very helpful in conjunction with CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments that select the configuration to load explicitly. Signed-off-by: Julius Werner --- - Changes for v2: - Changed from only supporting compressed FDTs to supporting all non-kernel image node types. - Changes for v3: - Fixed up some debug output that was still written for v1. - Fixed a mistake with handling FIT_LOAD_OPTIONAL_NON_ZERO when 'load' was 0 (i.e. unset). - Added compression test case to the test_fit pytest. - No changes for v4 - No changes for v5 common/image-fit.c| 86 +++ test/py/tests/test_fit.py | 29 +++-- 2 files changed, 77 insertions(+), 38 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index a74b44f298..c9ffc441aa 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -22,6 +22,7 @@ DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/ +#include #include #include #include @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void *fdt) kfdt_name); continue; } + + if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" (compressed)\n", + kfdt_name); + continue; + } + /* * Get a pointer to this configuration's fdt. */ @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr, const char *fit_uname_config; const char *fit_base_uname_config; const void *fit; - const void *buf; + void *buf; + void *loadbuf; size_t size; int type_ok, os_ok; - ulong load, data, len; - uint8_t os; + ulong load, load_end, data, len; + uint8_t os, comp; #ifndef USE_HOSTCC uint8_t os_arch; #endif @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, images->os.arch = os_arch; #endif - if (image_type == IH_TYPE_FLATDT && - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { - puts("FDT image is compressed"); - return -EPROTONOSUPPORT; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); type_ok = fit_image_check_type(fit, noffset, image_type) || fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); /* get image data address and length */ - if (fit_image_get_data_and_size(fit, noffset, , )) { + if (fit_image_get_data_and_size(fit, noffset, + (const void **), )) { printf("Could not find %s subimage data!\n", prop_name); bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); return -ENOENT; @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong addr, #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) /* perform any post-processing on the image data */ - board_fit_image_post_process((void **), ); + board_fit_image_post_process(, ); #endif len = (ulong)size; - /* verify that image data is a proper FDT blob */ - if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) { - puts("Subimage data is not a FDT"); - return -ENOEXEC; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK); - /* -* Work-around for eldk-4.2 which gives this warning if we try to -* cast in the unmap_sysmem() call: -* warning: initialization discards qualifiers from pointer target type -*/ - { - void *vbuf = (void *)buf; - - data = map_to_sysmem(vbuf); - } - + data = map_to_sysmem(buf); + load = data; if (load_op == FIT_LOAD_IGNORED) { /* Don't load */ } else if (fit_image_get_load(fit, noffset, )) { @@ -1974,8 +1963,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, } } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { ulong image_start, image_end; - ulong load
[U-Boot] [PATCH v5 1/3] common: Move bootm_decomp_image() to image.c (as image_decomp())
Upcoming patches want to add decompression to use cases that are no longer directly related to booting. It makes sense to retain a single decompression routine, but it should no longer be in bootm.c (which is not compiled for all configurations). This patch moves bootm_decomp_image() to image.c and renames it to image_decomp() in preparation of those upcoming patches. Signed-off-by: Julius Werner --- - First version: v5 common/bootm.c | 148 ++--- common/image.c | 111 ++ include/bootm.h| 17 -- include/image.h| 17 ++ test/compression.c | 24 5 files changed, 160 insertions(+), 157 deletions(-) diff --git a/common/bootm.c b/common/bootm.c index d193751647..2e64140df6 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -7,17 +7,12 @@ #ifndef USE_HOSTCC #include #include -#include #include #include #include #include #include #include -#include -#include -#include -#include #if defined(CONFIG_CMD_USB) #include #endif @@ -299,23 +294,6 @@ static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc, } #endif /* USE_HOSTC */ -/** - * print_decomp_msg() - Print a suitable decompression/loading message - * - * @type: OS type (IH_OS_...) - * @comp_type: Compression type being used (IH_COMP_...) - * @is_xip:true if the load address matches the image start - */ -static void print_decomp_msg(int comp_type, int type, bool is_xip) -{ - const char *name = genimg_get_type_name(type); - - if (comp_type == IH_COMP_NONE) - printf(" %s %s ... ", is_xip ? "XIP" : "Loading", name); - else - printf(" Uncompressing %s ... ", name); -} - /** * handle_decomp_error() - display a decompression error * @@ -325,16 +303,18 @@ static void print_decomp_msg(int comp_type, int type, bool is_xip) * * @comp_type: Compression type being used (IH_COMP_...) * @uncomp_size: Number of bytes uncompressed - * @unc_len: Amount of space available for decompression - * @ret: Error code to report - * @return BOOTM_ERR_RESET, indicating that the board must be reset + * @ret: errno error code received from compression library + * @return Appropriate BOOTM_ERR_ error code */ -static int handle_decomp_error(int comp_type, size_t uncomp_size, - size_t unc_len, int ret) +static int handle_decomp_error(int comp_type, size_t uncomp_size, int ret) { const char *name = genimg_get_comp_name(comp_type); - if (uncomp_size >= unc_len) + /* ENOSYS means unimplemented compression type, don't reset. */ + if (ret == -ENOSYS) + return BOOTM_ERR_UNIMPLEMENTED; + + if (uncomp_size >= CONFIG_SYS_BOOTM_LEN) printf("Image too large: increase CONFIG_SYS_BOOTM_LEN\n"); else printf("%s: uncompress error %d\n", name, ret); @@ -352,93 +332,6 @@ static int handle_decomp_error(int comp_type, size_t uncomp_size, return BOOTM_ERR_RESET; } -int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, - void *load_buf, void *image_buf, ulong image_len, - uint unc_len, ulong *load_end) -{ - int ret = 0; - - *load_end = load; - print_decomp_msg(comp, type, load == image_start); - - /* -* Load the image to the right place, decompressing if needed. After -* this, image_len will be set to the number of uncompressed bytes -* loaded, ret will be non-zero on error. -*/ - switch (comp) { - case IH_COMP_NONE: - if (load == image_start) - break; - if (image_len <= unc_len) - memmove_wd(load_buf, image_buf, image_len, CHUNKSZ); - else - ret = 1; - break; -#ifdef CONFIG_GZIP - case IH_COMP_GZIP: { - ret = gunzip(load_buf, unc_len, image_buf, _len); - break; - } -#endif /* CONFIG_GZIP */ -#ifdef CONFIG_BZIP2 - case IH_COMP_BZIP2: { - uint size = unc_len; - - /* -* If we've got less than 4 MB of malloc() space, -* use slower decompression algorithm which requires -* at most 2300 KB of memory. -*/ - ret = BZ2_bzBuffToBuffDecompress(load_buf, , - image_buf, image_len, - CONFIG_SYS_MALLOC_LEN < (4096 * 1024), 0); - image_len = size; - break; - } -#endif /* CONFIG_BZIP2 */ -#ifdef CONFIG_LZMA - case IH_COMP_LZMA: { - SizeT lzma_len = unc_len; - - ret = lzmaBuffToBuffDecompress(load_buf, _len, -
[U-Boot] [PATCH v5 0/3] fit: Image node compression
This patch series adds compression support for non-kernel FIT image nodes (e.g. FDTs). The first patch is a preparatory refactoring, the second adds the compression support itself, and the third adds a new feature to compatible string matching that allows it to be useful with compressed FDTs. Sandbox-tested with FIT images with and without compressed FDTs, with and without overlays, in both compatible string matching and direct config selection modes. Also expanded the test_fit pytest to include a case with compressed kernel, FDT and ramdisk. Julius Werner (3): common: Move bootm_decomp_image() to image.c (as image_decomp()) - First version: v5 fit: Support compression for non-kernel components (e.g. FDT) - Changes for v2: - Changed from only supporting compressed FDTs to supporting all non-kernel image node types. - Changes for v3: - Fixed up some debug output that was still written for v1. - Fixed a mistake with handling FIT_LOAD_OPTIONAL_NON_ZERO when 'load' was 0 (i.e. unset). - Added compression test case to the test_fit pytest. - No changes for v4 - No changes for v5 fit: Support compat string property in configuration node - No changes for v2 - No changes for v3 - Changes for v4: - Added documentation for compatible string in config node. - Added example .its file for compressed FDT with compat string in config node. - No changes for v5 common/bootm.c| 148 +++--- common/image-fit.c| 143 - common/image.c| 111 doc/uImage.FIT/kernel_fdts_compressed.its | 73 +++ doc/uImage.FIT/source_file_format.txt | 7 + include/bootm.h | 17 --- include/image.h | 17 +++ test/compression.c| 24 ++-- test/py/tests/test_fit.py | 29 - 9 files changed, 351 insertions(+), 218 deletions(-) create mode 100644 doc/uImage.FIT/kernel_fdts_compressed.its -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4 1/2] fit: Support compression for non-kernel components (e.g. FDT)
Hi Simon, I forgot about these for a while, but I believe I addressed all the feedback you still had in the latest version (v4). Looks like they didn't get picked up anywhere though. Can you help? On Thu, May 16, 2019 at 7:34 PM Simon Glass wrote: > > On Wed, 15 May 2019 at 16:30, Julius Werner wrote: > > > > This patch adds support for compressing non-kernel image nodes in a FIT > > image (kernel nodes could already be compressed previously). This can > > reduce the size of FIT images and therefore improve boot times > > (especially when an image bundles many different kernel FDTs). The > > images will automatically be decompressed on load. > > > > This patch does not support extracting compatible strings from > > compressed FDTs, so it's not very helpful in conjunction with > > CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments > > that select the configuration to load explicitly. > > > > Signed-off-by: Julius Werner > > --- > > - Changes for v2: > >- Changed from only supporting compressed FDTs to supporting all > > non-kernel image node types. > > - Changes for v3: > >- Fixed up some debug output that was still written for v1. > >- Fixed a mistake with handling FIT_LOAD_OPTIONAL_NON_ZERO when > > 'load' was 0 (i.e. unset). > >- Added compression test case to the test_fit pytest. > > - No changes for v4 > > > > common/image-fit.c| 86 +++ > > test/py/tests/test_fit.py | 29 +++-- > > 2 files changed, 77 insertions(+), 38 deletions(-) > > Reviewed-by: Simon Glass ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v4 2/2] fit: Support compat string property in configuration node
This patch adds support for an optional optimization to compatible string matching where the compatible string property from the root node of the kernel FDT can be copied into the configuration node of the FIT image. This is most useful when using compressed FDTs or when using FDT overlays, where the traditional extraction of the compatible string from the kernel FDT itself is not easily possible. Signed-off-by: Julius Werner --- - No changes for v2 - No changes for v3 - Changes for v4: - Added documentation for compatible string in config node. - Added example .its file for compressed FDT with compat string in config node. common/image-fit.c| 67 - doc/uImage.FIT/kernel_fdts_compressed.its | 73 +++ doc/uImage.FIT/source_file_format.txt | 7 +++ 3 files changed, 119 insertions(+), 28 deletions(-) create mode 100644 doc/uImage.FIT/kernel_fdts_compressed.its diff --git a/common/image-fit.c b/common/image-fit.c index 469c5c8f49..d32add6419 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1522,6 +1522,10 @@ int fit_check_format(const void *fit) * compatible list, "foo,bar", matches a compatible string in the root of fdt1. * "bim,bam" in fdt2 matches the second string which isn't as good as fdt1. * + * As an optimization, the compatible property from the FDT's root node can be + * copied into the configuration node in the FIT image. This is required to + * match configurations with compressed FDTs. + * * returns: * offset to the configuration to use if one was found * -1 otherwise @@ -1554,55 +1558,62 @@ int fit_conf_find_compat(const void *fit, const void *fdt) for (noffset = fdt_next_node(fit, confs_noffset, ); (noffset >= 0) && (ndepth > 0); noffset = fdt_next_node(fit, noffset, )) { - const void *kfdt; + const void *fdt; const char *kfdt_name; - int kfdt_noffset; + int kfdt_noffset, compat_noffset; const char *cur_fdt_compat; int len; - size_t size; + size_t sz; int i; if (ndepth > 1) continue; - kfdt_name = fdt_getprop(fit, noffset, "fdt", ); - if (!kfdt_name) { - debug("No fdt property found.\n"); - continue; - } - kfdt_noffset = fdt_subnode_offset(fit, images_noffset, - kfdt_name); - if (kfdt_noffset < 0) { - debug("No image node named \"%s\" found.\n", - kfdt_name); - continue; - } + /* If there's a compat property in the config node, use that. */ + if (fdt_getprop(fit, noffset, "compatible", NULL)) { + fdt = fit;/* search in FIT image */ + compat_noffset = noffset; /* search under config node */ + } else {/* Otherwise extract it from the kernel FDT. */ + kfdt_name = fdt_getprop(fit, noffset, "fdt", ); + if (!kfdt_name) { + debug("No fdt property found.\n"); + continue; + } + kfdt_noffset = fdt_subnode_offset(fit, images_noffset, + kfdt_name); + if (kfdt_noffset < 0) { + debug("No image node named \"%s\" found.\n", + kfdt_name); + continue; + } - if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { - debug("Can't extract compat from \"%s\" (compressed)\n", - kfdt_name); - continue; - } + if (!fit_image_check_comp(fit, kfdt_noffset, + IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" " + "(compressed)\n", kfdt_name); + continue; + } - /* -* Get a pointer to this configuration's fdt. -*/ - if (fit_image_get_data(fit, kfdt_noffset, , )) { - debug("Failed to get fdt \"%s\".\n", kfdt_name); - continue; +
[U-Boot] [PATCH v4 0/2] fit: Image node compression
This patch series adds compression support for non-kernel FIT image nodes (e.g. FDTs). The first patch adds the compression support itself, the second adds a new feature to compatible string matching that allows it to be useful with compressed FDTs. Sandbox-tested with FIT images with and without compressed FDTs, with and without overlays, in both compatible string matching and direct config selection modes. Also expanded the test_fit pytest to include a case with compressed kernel, FDT and ramdisk. Julius Werner (2): fit: Support compression for non-kernel components (e.g. FDT) - Changes for v2: - Changed from only supporting compressed FDTs to supporting all non-kernel image node types. - Changes for v3: - Fixed up some debug output that was still written for v1. - Fixed a mistake with handling FIT_LOAD_OPTIONAL_NON_ZERO when 'load' was 0 (i.e. unset). - Added compression test case to the test_fit pytest. - No changes for v4 fit: Support compat string property in configuration node - No changes for v2 - No changes for v3 - Changes for v4: - Added documentation for compatible string in config node. - Added example .its file for compressed FDT with compat string in config node. common/image-fit.c | 140 +++-- 1 file changed, 83 insertions(+), 57 deletions(-) -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v4 1/2] fit: Support compression for non-kernel components (e.g. FDT)
This patch adds support for compressing non-kernel image nodes in a FIT image (kernel nodes could already be compressed previously). This can reduce the size of FIT images and therefore improve boot times (especially when an image bundles many different kernel FDTs). The images will automatically be decompressed on load. This patch does not support extracting compatible strings from compressed FDTs, so it's not very helpful in conjunction with CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments that select the configuration to load explicitly. Signed-off-by: Julius Werner --- - Changes for v2: - Changed from only supporting compressed FDTs to supporting all non-kernel image node types. - Changes for v3: - Fixed up some debug output that was still written for v1. - Fixed a mistake with handling FIT_LOAD_OPTIONAL_NON_ZERO when 'load' was 0 (i.e. unset). - Added compression test case to the test_fit pytest. - No changes for v4 common/image-fit.c| 86 +++ test/py/tests/test_fit.py | 29 +++-- 2 files changed, 77 insertions(+), 38 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index ac901e131c..469c5c8f49 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -22,6 +22,7 @@ DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/ +#include #include #include #include @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void *fdt) kfdt_name); continue; } + + if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" (compressed)\n", + kfdt_name); + continue; + } + /* * Get a pointer to this configuration's fdt. */ @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr, const char *fit_uname_config; const char *fit_base_uname_config; const void *fit; - const void *buf; + void *buf; + void *loadbuf; size_t size; int type_ok, os_ok; - ulong load, data, len; - uint8_t os; + ulong load, load_end, data, len; + uint8_t os, comp; #ifndef USE_HOSTCC uint8_t os_arch; #endif @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, images->os.arch = os_arch; #endif - if (image_type == IH_TYPE_FLATDT && - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { - puts("FDT image is compressed"); - return -EPROTONOSUPPORT; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); type_ok = fit_image_check_type(fit, noffset, image_type) || fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); /* get image data address and length */ - if (fit_image_get_data_and_size(fit, noffset, , )) { + if (fit_image_get_data_and_size(fit, noffset, + (const void **), )) { printf("Could not find %s subimage data!\n", prop_name); bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); return -ENOENT; @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong addr, #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) /* perform any post-processing on the image data */ - board_fit_image_post_process((void **), ); + board_fit_image_post_process(, ); #endif len = (ulong)size; - /* verify that image data is a proper FDT blob */ - if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) { - puts("Subimage data is not a FDT"); - return -ENOEXEC; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK); - /* -* Work-around for eldk-4.2 which gives this warning if we try to -* cast in the unmap_sysmem() call: -* warning: initialization discards qualifiers from pointer target type -*/ - { - void *vbuf = (void *)buf; - - data = map_to_sysmem(vbuf); - } - + data = map_to_sysmem(buf); + load = data; if (load_op == FIT_LOAD_IGNORED) { /* Don't load */ } else if (fit_image_get_load(fit, noffset, )) { @@ -1974,8 +1963,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, } } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { ulong image_start, image_end; - ulong load_end; - v
Re: [U-Boot] [PATCH v3 1/2] fit: Support compression for non-kernel components (e.g. FDT)
Like Simon (Goldschmidt) said, the current documentation for the "compression" property in image nodes already matches this, so I don't think I need to add anything there. I'll update the other patch to add documentation for compatible strings in config nodes, and then upload an .its example for compressed FDTs there (because the example makes more sense when using both features together). On Tue, May 14, 2019 at 8:11 PM Simon Glass wrote: > > Hi Simon, > > On Tue, 14 May 2019 at 05:16, Simon Goldschmidt > wrote: > > > > On Tue, May 14, 2019 at 12:55 PM Simon Glass wrote: > > > > > > Hi Julius, > > > > > > On Mon, 13 May 2019 at 19:13, Julius Werner wrote: > > > > > > > > > Is there a change log for this patch?I think we discussed having a > > > > > test. > > > > > > > > Sorry, forgot that. Is it okay if I just put it here or do you need me > > > > to resend a v4? > > > > > > I think it should be a v4 so it looks right in patchwork. > > > > > > Also for this patch, can you please add documentation - see > > > source_file_format.txt > > > > I had submitted a patch for that file already last year but then got > > stuck in implementing just this, see commit fd15a9e2565f here: > > https://github.com/u-boot/u-boot/commit/fd15a9e2565f831bf95c2152d1966d068a642175#diff-8c4b5e332d50ba25248fd06d6b4eb026 > > > > I don't know if this compression support needs anything more > > specific in that doc file. If anything, we could explicitly state that > > compression is now supported for all nodes, but then again, the > > file didn't tell that it was only supported for Kernels... > > Now that gmail is fixed I can reply more easily... > > Fair enough. > > Compression for a DT node is a pain, since it must be decompressed > before being accessed and even reading the compatible string is hard. > This is the purpose of one of Julius' patches. > > The DT format is fairly well documented and I would like to keep it > that way, as well as extend the tests as we add new functions. > > Regards, > Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/2] fit: Support compat string property in configuration node
> Can you please add a change log, as this is a v3 patch. I didn't change anything in this one. (Should I have not called it v3 then? Sorry, I'm not familiar with the submission process here. I thought letting the individual patches in the series have different version numbers for the same set would be more confusing.) ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/2] fit: Support compression for non-kernel components (e.g. FDT)
> Is there a change log for this patch?I think we discussed having a test. Sorry, forgot that. Is it okay if I just put it here or do you need me to resend a v4? --- Changes for v2: - made decompression work for all non-kernel images, not just FDTs Changes for v3: - fixed up debug output that was still written for v1 - fixed a mistake with handling FIT_LOAD_OPTIONAL_NON_ZERO when 'load' was 0 (i.e. unset) - added compression test case to test_fit ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] fit: Support compression for non-kernel components (e.g. FDT)
> Oh, ok. Guess I was just surprised that it says "can't extract compat" > when it tries to check "comp"... Well, what I'm trying to say in that error is "can't extract the compatible string *because* the FDT is compressed", but without making it three lines long. > We should get a test together for this. There is an existing > test_fit.py which might be expanded, or perhaps create a new one and > share some code. Oh, neat, I didn't know that exists there! Added a test case with compressed images. > > @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > const char *fit_uname_config; > > const char *fit_base_uname_config; > > const void *fit; > > - const void *buf; > > + void *buf; > > + void *loadbuf; > > size_t size; > > int type_ok, os_ok; > > - ulong load, data, len; > > - uint8_t os; > > + ulong load, load_end, data, len; > > + uint8_t os, comp; > > #ifndef USE_HOSTCC > > uint8_t os_arch; > > #endif > > @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > images->os.arch = os_arch; > > #endif > > > > - if (image_type == IH_TYPE_FLATDT && > > - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { > > - puts("FDT image is compressed"); > > - return -EPROTONOSUPPORT; > > - } > > - > > bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); > > type_ok = fit_image_check_type(fit, noffset, image_type) || > > fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || > > @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); > > > > /* get image data address and length */ > > - if (fit_image_get_data_and_size(fit, noffset, , )) { > > + if (fit_image_get_data_and_size(fit, noffset, > > + (const void **), )) { > > printf("Could not find %s subimage data!\n", prop_name); > > bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); > > return -ENOENT; > > @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > > > #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) > > /* perform any post-processing on the image data */ > > - board_fit_image_post_process((void **), ); > > + board_fit_image_post_process(, ); > > #endif > > > > len = (ulong)size; > > > > - /* verify that image data is a proper FDT blob */ > > - if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) { > > - puts("Subimage data is not a FDT"); > > - return -ENOEXEC; > > - } > > - > > bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK); > > > > - /* > > -* Work-around for eldk-4.2 which gives this warning if we try to > > -* cast in the unmap_sysmem() call: > > -* warning: initialization discards qualifiers from pointer target > > type > > -*/ > > - { > > - void *vbuf = (void *)buf; > > - > > - data = map_to_sysmem(vbuf); > > - } > > - > > We don't support gcc 4.2 now so this code can be simplified to the > line you have below, but perhaps this should be in a separate patch? Well, I just rearranged the whole thing by removing the const qualifier from 'buf' itself, which I think makes this all way less confusing. So this block should become obsolete regardless of compiler version. That's somewhat intertwined with changing around all the load address handling so I hope keeping it all in one patch is okay. > > + comp = IH_COMP_NONE; > > + loadbuf = buf; > > + /* Kernel images get decompressed later in bootm_host_load_image(). > > */ > > But only for the host, right? Oh, right, good catch, looked up the wrong function name. The normal case goes through bootm_load_os(). ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v3 2/2] fit: Support compat string property in configuration node
This patch adds support for an optional optimization to compatible string matching where the compatible string property from the root node of the kernel FDT can be copied into the configuration node of the FIT image. This is most useful when using compressed FDTs or when using FDT overlays, where the traditional extraction of the compatible string from the kernel FDT itself is not easily possible. Signed-off-by: Julius Werner --- common/image-fit.c | 67 +++--- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index 469c5c8f49..d32add6419 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1522,6 +1522,10 @@ int fit_check_format(const void *fit) * compatible list, "foo,bar", matches a compatible string in the root of fdt1. * "bim,bam" in fdt2 matches the second string which isn't as good as fdt1. * + * As an optimization, the compatible property from the FDT's root node can be + * copied into the configuration node in the FIT image. This is required to + * match configurations with compressed FDTs. + * * returns: * offset to the configuration to use if one was found * -1 otherwise @@ -1554,55 +1558,62 @@ int fit_conf_find_compat(const void *fit, const void *fdt) for (noffset = fdt_next_node(fit, confs_noffset, ); (noffset >= 0) && (ndepth > 0); noffset = fdt_next_node(fit, noffset, )) { - const void *kfdt; + const void *fdt; const char *kfdt_name; - int kfdt_noffset; + int kfdt_noffset, compat_noffset; const char *cur_fdt_compat; int len; - size_t size; + size_t sz; int i; if (ndepth > 1) continue; - kfdt_name = fdt_getprop(fit, noffset, "fdt", ); - if (!kfdt_name) { - debug("No fdt property found.\n"); - continue; - } - kfdt_noffset = fdt_subnode_offset(fit, images_noffset, - kfdt_name); - if (kfdt_noffset < 0) { - debug("No image node named \"%s\" found.\n", - kfdt_name); - continue; - } + /* If there's a compat property in the config node, use that. */ + if (fdt_getprop(fit, noffset, "compatible", NULL)) { + fdt = fit;/* search in FIT image */ + compat_noffset = noffset; /* search under config node */ + } else {/* Otherwise extract it from the kernel FDT. */ + kfdt_name = fdt_getprop(fit, noffset, "fdt", ); + if (!kfdt_name) { + debug("No fdt property found.\n"); + continue; + } + kfdt_noffset = fdt_subnode_offset(fit, images_noffset, + kfdt_name); + if (kfdt_noffset < 0) { + debug("No image node named \"%s\" found.\n", + kfdt_name); + continue; + } - if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { - debug("Can't extract compat from \"%s\" (compressed)\n", - kfdt_name); - continue; - } + if (!fit_image_check_comp(fit, kfdt_noffset, + IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" " + "(compressed)\n", kfdt_name); + continue; + } - /* -* Get a pointer to this configuration's fdt. -*/ - if (fit_image_get_data(fit, kfdt_noffset, , )) { - debug("Failed to get fdt \"%s\".\n", kfdt_name); - continue; + /* search in this config's kernel FDT */ + if (fit_image_get_data(fit, kfdt_noffset, , )) { + debug("Failed to get fdt \"%s\".\n", kfdt_name); + continue; + } + + compat_noffset = 0; /* search kFDT under root node */ } len = fdt_compa
[U-Boot] [PATCH v3 1/2] fit: Support compression for non-kernel components (e.g. FDT)
This patch adds support for compressing non-kernel image nodes in a FIT image (kernel nodes could already be compressed previously). This can reduce the size of FIT images and therefore improve boot times (especially when an image bundles many different kernel FDTs). The images will automatically be decompressed on load. This patch does not support extracting compatible strings from compressed FDTs, so it's not very helpful in conjunction with CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments that select the configuration to load explicitly. Signed-off-by: Julius Werner --- common/image-fit.c| 86 +++ test/py/tests/test_fit.py | 29 +++-- 2 files changed, 77 insertions(+), 38 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index ac901e131c..469c5c8f49 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -22,6 +22,7 @@ DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/ +#include #include #include #include @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void *fdt) kfdt_name); continue; } + + if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" (compressed)\n", + kfdt_name); + continue; + } + /* * Get a pointer to this configuration's fdt. */ @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr, const char *fit_uname_config; const char *fit_base_uname_config; const void *fit; - const void *buf; + void *buf; + void *loadbuf; size_t size; int type_ok, os_ok; - ulong load, data, len; - uint8_t os; + ulong load, load_end, data, len; + uint8_t os, comp; #ifndef USE_HOSTCC uint8_t os_arch; #endif @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, images->os.arch = os_arch; #endif - if (image_type == IH_TYPE_FLATDT && - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { - puts("FDT image is compressed"); - return -EPROTONOSUPPORT; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); type_ok = fit_image_check_type(fit, noffset, image_type) || fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); /* get image data address and length */ - if (fit_image_get_data_and_size(fit, noffset, , )) { + if (fit_image_get_data_and_size(fit, noffset, + (const void **), )) { printf("Could not find %s subimage data!\n", prop_name); bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); return -ENOENT; @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong addr, #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) /* perform any post-processing on the image data */ - board_fit_image_post_process((void **), ); + board_fit_image_post_process(, ); #endif len = (ulong)size; - /* verify that image data is a proper FDT blob */ - if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) { - puts("Subimage data is not a FDT"); - return -ENOEXEC; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK); - /* -* Work-around for eldk-4.2 which gives this warning if we try to -* cast in the unmap_sysmem() call: -* warning: initialization discards qualifiers from pointer target type -*/ - { - void *vbuf = (void *)buf; - - data = map_to_sysmem(vbuf); - } - + data = map_to_sysmem(buf); + load = data; if (load_op == FIT_LOAD_IGNORED) { /* Don't load */ } else if (fit_image_get_load(fit, noffset, )) { @@ -1974,8 +1963,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, } } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { ulong image_start, image_end; - ulong load_end; - void *dst; /* * move image data to the load address, @@ -1993,14 +1980,45 @@ int fit_image_load(bootm_headers_t *images, ulong addr, printf(" Loading %s from 0x%08lx to 0x%08lx\n", prop_name, data, load); + } else { + load = data;/* No load address specified */ +
[U-Boot] [PATCH v3 0/2] fit: Image node compression
This patch series adds compression support for non-kernel FIT image nodes (e.g. FDTs). The first patch adds the compression support itself, the second adds a new feature to compatible string matching that allows it to be useful with compressed FDTs. Sandbox-tested with FIT images with and without compressed FDTs, with and without overlays, in both compatible string matching and direct config selection modes. Also expanded the test_fit pytest to include a case with compressed kernel, FDT and ramdisk. Julius Werner (2): fit: Support compression for non-kernel components (e.g. FDT) - Changes for v2: - Changed from only supporting compressed FDTs to supporting all non-kernel image node types. fit: Support compat string property in configuration node common/image-fit.c | 140 +++-- 1 file changed, 83 insertions(+), 57 deletions(-) -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2 v2] fit: Support compression for non-kernel components (e.g. FDT)
On Tue, Apr 30, 2019 at 11:25 AM Simon Goldschmidt wrote: > > Am 18.04.2019 um 23:08 schrieb Julius Werner: > > This patch adds support for compressing non-kernel image nodes in a FIT > > image (kernel nodes could already be compressed previously). This can > > reduce the size of FIT images and therefore improve boot times > > (especially when an image bundles many different kernel FDTs). The > > images will automatically be decompressed on load. > > > > This patch does not support extracting compatible strings from > > compressed FDTs, so it's not very helpful in conjunction with > > CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments > > that select the configuration to load explicitly. > > > > Signed-off-by: Julius Werner > > --- > > common/image-fit.c | 83 +++--- > > 1 file changed, 49 insertions(+), 34 deletions(-) > > > > diff --git a/common/image-fit.c b/common/image-fit.c > > index ac901e131c..006e828b79 100644 > > --- a/common/image-fit.c > > +++ b/common/image-fit.c > > @@ -22,6 +22,7 @@ > > DECLARE_GLOBAL_DATA_PTR; > > #endif /* !USE_HOSTCC*/ > > > > +#include > > #include > > #include > > #include > > @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void > > *fdt) > > kfdt_name); > > continue; > > } > > + > > + if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { > > + debug("Can't extract compat from \"%s\" > > (compressed)\n", > > + kfdt_name); > > + continue; > > + } > > + > > What's this block good for in this patch? Looks like it belongs to 2/2? This is necessary because I'm removing the (image_type == IH_TYPE_FLATDT && !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) check below. Previously, this function would just always hard fail with compressed FDTs. With this patch, compressed FDTs can be loaded, but they can't be used for compat string matching -- that's why I need to add this check here to prevent it. You can still use compressed FDTs when selecting a configuration explicitly (e.g. bootm 0x100#conf@1). The next patch will then add another feature that makes it possible to have compat string matching with compressed FDTs. > > > /* > >* Get a pointer to this configuration's fdt. > >*/ > > @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > const char *fit_uname_config; > > const char *fit_base_uname_config; > > const void *fit; > > - const void *buf; > > + void *buf; > > + void *loadbuf; > > size_t size; > > int type_ok, os_ok; > > - ulong load, data, len; > > - uint8_t os; > > + ulong load, load_end, data, len; > > + uint8_t os, comp; > > #ifndef USE_HOSTCC > > uint8_t os_arch; > > #endif > > @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > images->os.arch = os_arch; > > #endif > > > > - if (image_type == IH_TYPE_FLATDT && > > - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { > > - puts("FDT image is compressed"); > > - return -EPROTONOSUPPORT; > > - } > > - > > bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); > > type_ok = fit_image_check_type(fit, noffset, image_type) || > > fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || > > @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); > > > > /* get image data address and length */ > > - if (fit_image_get_data_and_size(fit, noffset, , )) { > > + if (fit_image_get_data_and_size(fit, noffset, > > + (const void **), )) { > > printf("Could not find %s subimage data!\n", prop_name); > > bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); > > return -ENOENT; > > @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong > > addr, > > > > #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) > > /* perform any post-processing on the image data */
Re: [U-Boot] [PATCH 1/2 v2] fit: Support compression for non-kernel components (e.g. FDT)
> However, compared to my above mentioned RFC patch, this here somehow > fails when loading something from the image, e.g. using the cmd 'fpga > loadmk'. Seems like this one doesn't transparently uncompress? It looks to me like do_fpga_loadmk() doesn't actually call fit_load_image()? It calls fit_image_get_data() directly, so it bypasses the code I'm adding. Maybe it should be using fit_load_image() instead, but that's moving pretty far away from what I was trying to do... it could be left for a later patch? ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 1/2 v2] fit: Support compression for non-kernel components (e.g. FDT)
This patch adds support for compressing non-kernel image nodes in a FIT image (kernel nodes could already be compressed previously). This can reduce the size of FIT images and therefore improve boot times (especially when an image bundles many different kernel FDTs). The images will automatically be decompressed on load. This patch does not support extracting compatible strings from compressed FDTs, so it's not very helpful in conjunction with CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments that select the configuration to load explicitly. Signed-off-by: Julius Werner --- common/image-fit.c | 83 +++--- 1 file changed, 49 insertions(+), 34 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index ac901e131c..006e828b79 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -22,6 +22,7 @@ DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/ +#include #include #include #include @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void *fdt) kfdt_name); continue; } + + if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" (compressed)\n", + kfdt_name); + continue; + } + /* * Get a pointer to this configuration's fdt. */ @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr, const char *fit_uname_config; const char *fit_base_uname_config; const void *fit; - const void *buf; + void *buf; + void *loadbuf; size_t size; int type_ok, os_ok; - ulong load, data, len; - uint8_t os; + ulong load, load_end, data, len; + uint8_t os, comp; #ifndef USE_HOSTCC uint8_t os_arch; #endif @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, images->os.arch = os_arch; #endif - if (image_type == IH_TYPE_FLATDT && - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { - puts("FDT image is compressed"); - return -EPROTONOSUPPORT; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); type_ok = fit_image_check_type(fit, noffset, image_type) || fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); /* get image data address and length */ - if (fit_image_get_data_and_size(fit, noffset, , )) { + if (fit_image_get_data_and_size(fit, noffset, + (const void **), )) { printf("Could not find %s subimage data!\n", prop_name); bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); return -ENOENT; @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong addr, #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) /* perform any post-processing on the image data */ - board_fit_image_post_process((void **), ); + board_fit_image_post_process(, ); #endif len = (ulong)size; - /* verify that image data is a proper FDT blob */ - if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) { - puts("Subimage data is not a FDT"); - return -ENOEXEC; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK); - /* -* Work-around for eldk-4.2 which gives this warning if we try to -* cast in the unmap_sysmem() call: -* warning: initialization discards qualifiers from pointer target type -*/ - { - void *vbuf = (void *)buf; - - data = map_to_sysmem(vbuf); - } - + data = map_to_sysmem(buf); + load = data; if (load_op == FIT_LOAD_IGNORED) { /* Don't load */ } else if (fit_image_get_load(fit, noffset, )) { @@ -1974,8 +1963,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, } } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { ulong image_start, image_end; - ulong load_end; - void *dst; /* * move image data to the load address, @@ -1993,14 +1980,42 @@ int fit_image_load(bootm_headers_t *images, ulong addr, printf(" Loading %s from 0x%08lx to 0x%08lx\n", prop_name, data, load); + } - dst = map_sysmem(load, len); - memmove(dst, buf, len); - data = load; +
Re: [U-Boot] [PATCH 1/2] fit: Support FDT compression
> Hmm, I think we might want to use the lmb functions here to allocate a > buffer instead of relyling on malloc? The malloc pool might be large > enough for an uncompressed devicetree, but not for an 8 MByte FPGA image... > > But starting with malloc might be ok. Okay, that sounds like a good possible improvement for later. For now, you can always just specify a load address for the FPGA image so that it doesn't need to allocate a separate buffer. > The reason I discontinued that patch was that I started adding a feature > to mkimage to add a property for the uncompressed size. This is still > pending work I haven't sent to the ML, but I do want to continue it once > I find the time. Yeah, that would make all of this a lot cleaner. > So maybe we could move on with a v2 of your patch that uncompresses > everything but the kernel? I'd like to test that with my compressed FPGA > images then. Sounds good, v2 sent out. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 2/2 v2] fit: Support compat string property in configuration node
This patch adds support for an optional optimization to compatible string matching where the compatible string property from the root node of the kernel FDT can be copied into the configuration node of the FIT image. This is most useful when using compressed FDTs or when using FDT overlays, where the traditional extraction of the compatible string from the kernel FDT itself is not easily possible. Signed-off-by: Julius Werner --- common/image-fit.c | 67 +++--- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index 006e828b79..4602db8414 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1522,6 +1522,10 @@ int fit_check_format(const void *fit) * compatible list, "foo,bar", matches a compatible string in the root of fdt1. * "bim,bam" in fdt2 matches the second string which isn't as good as fdt1. * + * As an optimization, the compatible property from the FDT's root node can be + * copied into the configuration node in the FIT image. This is required to + * match configurations with compressed FDTs. + * * returns: * offset to the configuration to use if one was found * -1 otherwise @@ -1554,55 +1558,62 @@ int fit_conf_find_compat(const void *fit, const void *fdt) for (noffset = fdt_next_node(fit, confs_noffset, ); (noffset >= 0) && (ndepth > 0); noffset = fdt_next_node(fit, noffset, )) { - const void *kfdt; + const void *fdt; const char *kfdt_name; - int kfdt_noffset; + int kfdt_noffset, compat_noffset; const char *cur_fdt_compat; int len; - size_t size; + size_t sz; int i; if (ndepth > 1) continue; - kfdt_name = fdt_getprop(fit, noffset, "fdt", ); - if (!kfdt_name) { - debug("No fdt property found.\n"); - continue; - } - kfdt_noffset = fdt_subnode_offset(fit, images_noffset, - kfdt_name); - if (kfdt_noffset < 0) { - debug("No image node named \"%s\" found.\n", - kfdt_name); - continue; - } + /* If there's a compat property in the config node, use that. */ + if (fdt_getprop(fit, noffset, "compatible", NULL)) { + fdt = fit;/* search in FIT image */ + compat_noffset = noffset; /* search under config node */ + } else {/* Otherwise extract it from the kernel FDT. */ + kfdt_name = fdt_getprop(fit, noffset, "fdt", ); + if (!kfdt_name) { + debug("No fdt property found.\n"); + continue; + } + kfdt_noffset = fdt_subnode_offset(fit, images_noffset, + kfdt_name); + if (kfdt_noffset < 0) { + debug("No image node named \"%s\" found.\n", + kfdt_name); + continue; + } - if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { - debug("Can't extract compat from \"%s\" (compressed)\n", - kfdt_name); - continue; - } + if (!fit_image_check_comp(fit, kfdt_noffset, + IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" " + "(compressed)\n", kfdt_name); + continue; + } - /* -* Get a pointer to this configuration's fdt. -*/ - if (fit_image_get_data(fit, kfdt_noffset, , )) { - debug("Failed to get fdt \"%s\".\n", kfdt_name); - continue; + /* search in this config's kernel FDT */ + if (fit_image_get_data(fit, kfdt_noffset, , )) { + debug("Failed to get fdt \"%s\".\n", kfdt_name); + continue; + } + + compat_noffset = 0; /* search kFDT under root node */ } len = fdt_compa
[U-Boot] [PATCH 0/2] fit: Image node compression
This patch series adds compression support for non-kernel FIT image nodes (e.g. FDTs). The first patch adds the compression support itself, the second adds a new feature to compatible string matching that allows it to be useful with compressed FDTs. Sandbox-tested with FIT images with and without compressed FDTs, with and without overlays, in both compatible string matching and direct config selection modes. Julius Werner (2): fit: Support compression for non-kernel components (e.g. FDT) - Changes for v2: - Changed from only supporting compressed FDTs to supporting all non-kernel image node types. fit: Support compat string property in configuration node common/image-fit.c | 140 +++-- 1 file changed, 83 insertions(+), 57 deletions(-) -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] fit: Support FDT compression
> My approach was to uncompress all compressed images on-the-fly in > fit_image_load(). Right, that's essentially what this patch is doing too. > Or I could dig up my patches from October and we'll see how far you get > with those? I think I found your patch: https://lists.denx.de/pipermail/u-boot/2018-October/344673.html It seems to be doing something very close to what my patch does anyway. My patch goes a little further by also solving the case when no load address is given (in that case it will malloc() a buffer to decompress into with an upper bound guess for the required size). If there is a load size given, the two should end up doing the same thing. Also your patch works on all image types, which as you said there breaks it for kernels. I think the option of doing it for all image types except kernels would be a good solution for everyone. (Ultimately, I think it might be nicer if the kernel decompression would also be handled there and not as a special case, but I'd rather not tackle everything at once. This can always be iterated upon in the future.) ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] fit: Support FDT compression
Hi Simon, Is your approach similar to what I did here (decompressing transparently as part of fit_image_load())? I think I could easily expand this to other image types, I just don't always know how to test those. Really, the only thing that can't be decompressed there is the kernel image (because that is done later), so rather than checking for IH_TYPE_FLATDT I could check for !(IH_TYPE_KERNEL || IH_TYPE_KERNEL_NOLOAD)? Alternatively, maybe we could take this as a first step and it can later be expanded to support other types when someone has time to work on that? ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 2/2] fit: Support compat string property in configuration node
This patch adds support for an optional optimization to compatible string matching where the compatible string property from the root node of the kernel FDT can be copied into the configuration node of the FIT image. This is most useful when using compressed FDTs or when using FDT overlays, where the traditional extraction of the compatible string from the kernel FDT itself is not easily possible. Signed-off-by: Julius Werner --- common/image-fit.c | 67 +++--- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index 20e02d240b..dec55ffaec 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1522,6 +1522,10 @@ int fit_check_format(const void *fit) * compatible list, "foo,bar", matches a compatible string in the root of fdt1. * "bim,bam" in fdt2 matches the second string which isn't as good as fdt1. * + * As an optimization, the compatible property from the FDT's root node can be + * copied into the configuration node in the FIT image. This is required to + * match configurations with compressed FDTs. + * * returns: * offset to the configuration to use if one was found * -1 otherwise @@ -1554,55 +1558,62 @@ int fit_conf_find_compat(const void *fit, const void *fdt) for (noffset = fdt_next_node(fit, confs_noffset, ); (noffset >= 0) && (ndepth > 0); noffset = fdt_next_node(fit, noffset, )) { - const void *kfdt; + const void *fdt; const char *kfdt_name; - int kfdt_noffset; + int kfdt_noffset, compat_noffset; const char *cur_fdt_compat; int len; - size_t size; + size_t sz; int i; if (ndepth > 1) continue; - kfdt_name = fdt_getprop(fit, noffset, "fdt", ); - if (!kfdt_name) { - debug("No fdt property found.\n"); - continue; - } - kfdt_noffset = fdt_subnode_offset(fit, images_noffset, - kfdt_name); - if (kfdt_noffset < 0) { - debug("No image node named \"%s\" found.\n", - kfdt_name); - continue; - } + /* If there's a compat property in the config node, use that. */ + if (fdt_getprop(fit, noffset, "compatible", NULL)) { + fdt = fit;/* search in FIT image */ + compat_noffset = noffset; /* search under config node */ + } else {/* Otherwise extract it from the kernel FDT. */ + kfdt_name = fdt_getprop(fit, noffset, "fdt", ); + if (!kfdt_name) { + debug("No fdt property found.\n"); + continue; + } + kfdt_noffset = fdt_subnode_offset(fit, images_noffset, + kfdt_name); + if (kfdt_noffset < 0) { + debug("No image node named \"%s\" found.\n", + kfdt_name); + continue; + } - if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { - debug("Can't extract compat from \"%s\" (compressed)\n", - kfdt_name); - continue; - } + if (!fit_image_check_comp(fit, kfdt_noffset, + IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" " + "(compressed)\n", kfdt_name); + continue; + } - /* -* Get a pointer to this configuration's fdt. -*/ - if (fit_image_get_data(fit, kfdt_noffset, , )) { - debug("Failed to get fdt \"%s\".\n", kfdt_name); - continue; + /* search in this config's kernel FDT */ + if (fit_image_get_data(fit, kfdt_noffset, , )) { + debug("Failed to get fdt \"%s\".\n", kfdt_name); + continue; + } + + compat_noffset = 0; /* search kFDT under root node */ } len = fdt_compa
[U-Boot] [PATCH 0/2] fit: FDT compression
This patch series adds FDT compression support. The first patch adds the compression support itself, the second adds a new feature to compatible string matching that allows it to be useful with compressed FDTs. Sandbox-tested with FIT images with and without compressed FDTs, with and without overlays, in both compatible string matching and direct config selection modes. Julius Werner (2): fit: Support FDT compression fit: Support compat string property in configuration node common/image-fit.c | 142 +++-- 1 file changed, 85 insertions(+), 57 deletions(-) -- 2.20.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 1/2] fit: Support FDT compression
This patch adds support for compressing FDT image nodes in a FIT image (equivalent to how kernel nodes can already be compressed). This can reduce the size of FIT images (and therefore improve boot times). FDTs will automatically get decompressed on load. This patch does not support extracting compatible strings from compressed FDTs, so it's not very helpful in conjunction with CONFIG_FIT_BEST_MATCH yet, but it can already be used in environments that select the configuration to load explicitly. Signed-off-by: Julius Werner --- common/image-fit.c | 85 +++--- 1 file changed, 51 insertions(+), 34 deletions(-) diff --git a/common/image-fit.c b/common/image-fit.c index ac901e131c..20e02d240b 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -22,6 +22,7 @@ DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/ +#include #include #include #include @@ -1576,6 +1577,13 @@ int fit_conf_find_compat(const void *fit, const void *fdt) kfdt_name); continue; } + + if (!fit_image_check_comp(fit, kfdt_noffset, IH_COMP_NONE)) { + debug("Can't extract compat from \"%s\" (compressed)\n", + kfdt_name); + continue; + } + /* * Get a pointer to this configuration's fdt. */ @@ -1795,11 +1803,12 @@ int fit_image_load(bootm_headers_t *images, ulong addr, const char *fit_uname_config; const char *fit_base_uname_config; const void *fit; - const void *buf; + void *buf; + void *loadbuf; size_t size; int type_ok, os_ok; - ulong load, data, len; - uint8_t os; + ulong load, load_end, data, len; + uint8_t os, comp; #ifndef USE_HOSTCC uint8_t os_arch; #endif @@ -1895,12 +1904,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, images->os.arch = os_arch; #endif - if (image_type == IH_TYPE_FLATDT && - !fit_image_check_comp(fit, noffset, IH_COMP_NONE)) { - puts("FDT image is compressed"); - return -EPROTONOSUPPORT; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL); type_ok = fit_image_check_type(fit, noffset, image_type) || fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE) || @@ -1931,7 +1934,8 @@ int fit_image_load(bootm_headers_t *images, ulong addr, bootstage_mark(bootstage_id + BOOTSTAGE_SUB_CHECK_ALL_OK); /* get image data address and length */ - if (fit_image_get_data_and_size(fit, noffset, , )) { + if (fit_image_get_data_and_size(fit, noffset, + (const void **), )) { printf("Could not find %s subimage data!\n", prop_name); bootstage_error(bootstage_id + BOOTSTAGE_SUB_GET_DATA); return -ENOENT; @@ -1939,30 +1943,15 @@ int fit_image_load(bootm_headers_t *images, ulong addr, #if !defined(USE_HOSTCC) && defined(CONFIG_FIT_IMAGE_POST_PROCESS) /* perform any post-processing on the image data */ - board_fit_image_post_process((void **), ); + board_fit_image_post_process(, ); #endif len = (ulong)size; - /* verify that image data is a proper FDT blob */ - if (image_type == IH_TYPE_FLATDT && fdt_check_header(buf)) { - puts("Subimage data is not a FDT"); - return -ENOEXEC; - } - bootstage_mark(bootstage_id + BOOTSTAGE_SUB_GET_DATA_OK); - /* -* Work-around for eldk-4.2 which gives this warning if we try to -* cast in the unmap_sysmem() call: -* warning: initialization discards qualifiers from pointer target type -*/ - { - void *vbuf = (void *)buf; - - data = map_to_sysmem(vbuf); - } - + data = map_to_sysmem(buf); + load = data; if (load_op == FIT_LOAD_IGNORED) { /* Don't load */ } else if (fit_image_get_load(fit, noffset, )) { @@ -1974,8 +1963,6 @@ int fit_image_load(bootm_headers_t *images, ulong addr, } } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { ulong image_start, image_end; - ulong load_end; - void *dst; /* * move image data to the load address, @@ -1993,14 +1980,44 @@ int fit_image_load(bootm_headers_t *images, ulong addr, printf(" Loading %s from 0x%08lx to 0x%08lx\n", prop_name, data, load); + } - dst = map_sysmem(load, len); - memmove(dst, buf, len); - data = load; + /* +* Decompress FDTs right here. Kernel images
[U-Boot] Questions/RFC about overlay compatible strings and device tree compression in uImage.FIT
Hi U-Boot developers, (I picked a few of you out that seem to be most related to this code, hope that's okay,) I am interested in booting device trees with overlays from a FIT image (actually for a different bootloader, but we're trying to stay compatible with U-Boot's FIT format so I'm asking here), and I came up with some questions about how this is supposed to work that I hope you can answer: I found the basic description of overlay support in FIT here: https://github.com/u-boot/u-boot/blob/master/doc/uImage.FIT/overlay-fdt-boot.txt However, what it doesn't really say is how compatible string matching works in the face of overlays. From my reading of the code it seems that U-Boot tries to match the compatible string in the FDT that is listed leftmost in the 'fdt' property of the config node (fit_conf_find_compat() just calls fdt_getprop(fit, "fdt") and uses the result as a string, so since the different FDT names in the property are separated by '\0' chars I assume that will act on the first name?), which according to the documentation should be the base device tree. Is that correct? I don't see how that would make sense... for overlays to make sense you would want to have multiple config nodes that all use the same base device tree and stack different overlays on top of it (the example in the docs also looks that way). But if the compatible string matcher only checks the base device tree, all of those config nodes will have the same strings to match -- how could it find the right node? I think the correct scheme would be to scan through all overlays from right to left (i.e. in reverse application order) until you find a compatible string somewhere. Was the current overlay support just only implemented with pre-selected config nodes (i.e. passing a fit_uname_configp to boot_get_fdt_fit()) in mind? On a somewhat related note, I was also thinking about allowing compression for the 'data' property of individual fdt@X nodes (similar to how you can already do it for kernel@X nodes), which can cut them down to about 15% of their size with LZ4 or even 5% with LZMA. The main problem with this is that you would have to decompress all device trees during the compatible string match to get at their embedded compatible strings. An obvious solution would be to copy the compatible string out into an explicit property in the config node at build time, so that the 'fdt' property (and the node it points to) doesn't have to be parsed before you're actually loading the full DT (and at that time you could decompress only the one(s) you're using). This would also help the compatible string problem with overlays, since then the config node has the compatible string right there and you don't need to worry about which overlay to extract it from anymore. Would it be an acceptable change if I added support for that to U-Boot? Of course, the code could gracefully fall back to scanning the whole FDT (decompressing if necessary) if the config node has no 'compatible' property. Please let me know what you think! Thanks, Julius ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCHv3] Add support for LZ4 decompression algorithm
This patch adds support for LZ4-compressed FIT image contents. This algorithm has a slightly worse compression ration than LZO while being nearly twice as fast to decompress. When loading images from a fast storage medium this usually results in a boot time win. Sandbox-tested only since I don't have a U-Boot development system set up right now. The code was imported unchanged from coreboot where it's proven to work, though. I'm mostly interested in getting this recognized by mkImage for use in a downstream project. Signed-off-by: Julius Werner <jwer...@chromium.org> --- common/bootm.c| 9 ++ common/image.c| 1 + configs/sandbox_defconfig | 1 + include/common.h | 3 + include/image.h | 1 + lib/Kconfig | 18 lib/Makefile | 1 + lib/lz4.c | 243 ++ lib/lz4_wrapper.c | 137 ++ test/compression.c| 57 +++ 10 files changed, 471 insertions(+) create mode 100644 lib/lz4.c create mode 100644 lib/lz4_wrapper.c diff --git a/common/bootm.c b/common/bootm.c index c0d0d09..58936ca 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -389,6 +389,15 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, break; } #endif /* CONFIG_LZO */ +#ifdef CONFIG_LZ4 + case IH_COMP_LZ4: { + size_t size = unc_len; + + ret = ulz4fn(image_buf, image_len, load_buf, ); + image_len = size; + break; + } +#endif /* CONFIG_LZ4 */ default: printf("Unimplemented compression type %d\n", comp); return BOOTM_ERR_UNIMPLEMENTED; diff --git a/common/image.c b/common/image.c index 1325e07..c33749d 100644 --- a/common/image.c +++ b/common/image.c @@ -167,6 +167,7 @@ static const table_entry_t uimage_comp[] = { { IH_COMP_GZIP, "gzip", "gzip compressed", }, { IH_COMP_LZMA, "lzma", "lzma compressed", }, { IH_COMP_LZO,"lzo", "lzo compressed", }, + { IH_COMP_LZ4,"lz4", "lz4 compressed", }, { -1, "", "", }, }; diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig index ae96b63..b2675c7 100644 --- a/configs/sandbox_defconfig +++ b/configs/sandbox_defconfig @@ -56,6 +56,7 @@ CONFIG_USB_STORAGE=y CONFIG_SYS_VSNPRINTF=y CONFIG_CMD_DHRYSTONE=y CONFIG_TPM=y +CONFIG_LZ4=y CONFIG_ERRNO_STR=y CONFIG_UNIT_TEST=y CONFIG_UT_TIME=y diff --git a/include/common.h b/include/common.h index 68b24d0..ecb1f06 100644 --- a/include/common.h +++ b/include/common.h @@ -826,6 +826,9 @@ int gzwrite(unsigned char *src, int len, u64 startoffs, u64 szexpected); +/* lib/lz4_wrapper.c */ +int ulz4fn(const void *src, size_t srcn, void *dst, size_t *dstn); + /* lib/qsort.c */ void qsort(void *base, size_t nmemb, size_t size, int(*compar)(const void *, const void *)); diff --git a/include/image.h b/include/image.h index 8a864ae..08ae24a 100644 --- a/include/image.h +++ b/include/image.h @@ -259,6 +259,7 @@ struct lmb; #define IH_COMP_BZIP2 2 /* bzip2 Compression Used */ #define IH_COMP_LZMA 3 /* lzma Compression Used */ #define IH_COMP_LZO4 /* lzo Compression Used */ +#define IH_COMP_LZ45 /* lz4 Compression Used */ #define IH_MAGIC 0x27051956 /* Image Magic Number */ #define IH_NMLEN 32 /* Image Name Length*/ diff --git a/lib/Kconfig b/lib/Kconfig index 0673072..a8f8460 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -100,6 +100,24 @@ config SHA_PROG_HW_ACCEL is performed in hardware. endmenu +menu "Compression Support" + +config LZ4 + bool "Enable LZ4 decompression support" + help + If this option is set, support for LZ4 compressed images + is included. The LZ4 algorithm can run in-place as long as the + compressed image is loaded to the end of the output buffer, and + trades lower compression ratios for much faster decompression. + + NOTE: This implements the release version of the LZ4 frame + format as generated by default by the 'lz4' command line tool. + This is not the same as the outdated, less efficient legacy + frame format currently (2015) implemented in the Linux kernel + (generated by 'lz4 -l'). The two formats are incompatible. + +endmenu + config ERRNO_STR bool "Enable function for getting errno-related string message" help diff --git a/lib/Makefile b/lib/Makefile index
Re: [U-Boot] [PATCHv2] Add support for LZ4 decompression algorithm
Well then... a few hours and a significant reduction in sanity later, I found that I'm shadowing the new 'ret' variable from changing the API around because I forgot to remove the declaration part from the 'ret' that already existed in the else block. It would be nice if U-Boot compiled with -Wshadow... This made the (outer) return variable uninitialized in the compression error case, which on my native x86_64 happened to produce a non-zero value... but with an i686 cross-compiler I could reproduce the error you saw. Fixed in the next version and moved the config to sandbox_defconfig as you suggested. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv2] Add support for LZ4 decompression algorithm
> You can build U-Boot with NO_SDL=1 Ah, thanks... that was the important magic flag I needed! > Actually you should enable the option in configs/sandbox_defconfig, > not lib/Kconfig. See the condition used by compression.c: > > obj-$(CONFIG_SANDBOX) += compression.o Okay, makes sense. I looked at the wrong line in the Makefile. > Also, for me the test fails: > > ./b/sandbox/u-boot -c "ut_compression" > ... > testing lz4 ... > orig_size:350 > compressed_size:276 > uncompressed_size:350 > compress does not overrun > Failed: ret != 0 > lz4: FAILED > ut_compression FAILED Huh... that's odd. When I run this now, I get: testing lz4 ... orig_size:350 compressed_size:276 uncompressed_size:350 compress does not overrun uncompress does not overrun lz4: ok And if I change a byte in the compressed test data, it fails on memcmp() as expected. Are you sure you have no local changes or anything (I based the patch off 1f8836396)? I don't see how this could give different results... ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCHv2] Add support for LZ4 decompression algorithm
This patch adds support for LZ4-compressed FIT image contents. This algorithm has a slightly worse compression ration than LZO while being nearly twice as fast to decompress. When loading images from a fast storage medium this usually results in a boot time win. Compile-tested only since I don't have a U-Boot development system set up right now. The code was imported unchanged from coreboot where it's proven to work, though. I'm mostly interested in getting this recognized by mkImage for use in a downstream project. Signed-off-by: Julius Werner <jwer...@chromium.org> --- common/bootm.c | 9 ++ common/image.c | 1 + include/common.h | 3 + include/image.h| 1 + lib/Kconfig| 18 lib/Makefile | 1 + lib/lz4.c | 243 + lib/lz4_wrapper.c | 137 ++ test/Kconfig | 1 + test/compression.c | 57 + 10 files changed, 471 insertions(+) create mode 100644 lib/lz4.c create mode 100644 lib/lz4_wrapper.c diff --git a/common/bootm.c b/common/bootm.c index 667c934..5f99ec8 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -389,6 +389,15 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, break; } #endif /* CONFIG_LZO */ +#ifdef CONFIG_LZ4 + case IH_COMP_LZ4: { + size_t size = unc_len; + + ret = ulz4fn(image_buf, image_len, load_buf, ); + image_len = size; + break; + } +#endif /* CONFIG_LZ4 */ default: printf("Unimplemented compression type %d\n", comp); return BOOTM_ERR_UNIMPLEMENTED; diff --git a/common/image.c b/common/image.c index 1325e07..c33749d 100644 --- a/common/image.c +++ b/common/image.c @@ -167,6 +167,7 @@ static const table_entry_t uimage_comp[] = { { IH_COMP_GZIP, "gzip", "gzip compressed", }, { IH_COMP_LZMA, "lzma", "lzma compressed", }, { IH_COMP_LZO,"lzo", "lzo compressed", }, + { IH_COMP_LZ4,"lz4", "lz4 compressed", }, { -1, "", "", }, }; diff --git a/include/common.h b/include/common.h index 68b24d0..ecb1f06 100644 --- a/include/common.h +++ b/include/common.h @@ -826,6 +826,9 @@ int gzwrite(unsigned char *src, int len, u64 startoffs, u64 szexpected); +/* lib/lz4_wrapper.c */ +int ulz4fn(const void *src, size_t srcn, void *dst, size_t *dstn); + /* lib/qsort.c */ void qsort(void *base, size_t nmemb, size_t size, int(*compar)(const void *, const void *)); diff --git a/include/image.h b/include/image.h index 8a864ae..08ae24a 100644 --- a/include/image.h +++ b/include/image.h @@ -259,6 +259,7 @@ struct lmb; #define IH_COMP_BZIP2 2 /* bzip2 Compression Used */ #define IH_COMP_LZMA 3 /* lzma Compression Used */ #define IH_COMP_LZO4 /* lzo Compression Used */ +#define IH_COMP_LZ45 /* lz4 Compression Used */ #define IH_MAGIC 0x27051956 /* Image Magic Number */ #define IH_NMLEN 32 /* Image Name Length*/ diff --git a/lib/Kconfig b/lib/Kconfig index 0673072..a8f8460 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -100,6 +100,24 @@ config SHA_PROG_HW_ACCEL is performed in hardware. endmenu +menu "Compression Support" + +config LZ4 + bool "Enable LZ4 decompression support" + help + If this option is set, support for LZ4 compressed images + is included. The LZ4 algorithm can run in-place as long as the + compressed image is loaded to the end of the output buffer, and + trades lower compression ratios for much faster decompression. + + NOTE: This implements the release version of the LZ4 frame + format as generated by default by the 'lz4' command line tool. + This is not the same as the outdated, less efficient legacy + frame format currently (2015) implemented in the Linux kernel + (generated by 'lz4 -l'). The two formats are incompatible. + +endmenu + config ERRNO_STR bool "Enable function for getting errno-related string message" help diff --git a/lib/Makefile b/lib/Makefile index 96f832e..3eecefa 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o obj-y += initcall.o obj-$(CONFIG_LMB) += lmb.o obj-y += ldiv.o +obj-$(CONFIG_LZ4) += lz4_wrapper.o obj-$(CONFIG_MD5) += md5.o obj-y += net_utils.o obj-$(CONFIG_PHYSMEM) += physmem.o diff --git a/lib/lz4.c b/lib/lz4.c new file mode 100644 index 000..f518341 --- /d
Re: [U-Boot] [PATCH] Add support for LZ4 decompression algorithm
> > I get this build error with sandbox. > > test/built-in.o: In function `uncompress_using_lz4': > /home/sjg/c/src/third_party/u-boot/files/test/compression.c:278: > undefined reference to `ulz4fn' > Yeah... that's because you didn't configure it in. I'm not really sure how this is supposed to work... there should really be some sort of dependency between selecting the compression tests and selecting the algorithm, or the algorithms should be #ifdefed out like in bootm.c. But neither of those seems to be in place right now... include/configs/sandbox.h just enables all the other compression algorithms, that's why it works for them. I could add a 'select LZ4' to config UNIT_TEST in the Kconfig to create such a dependency, I think that would be the best option? You should be able to run the tests using: > > make O=sandbox defconfig all > ./sandbox/u-boot -c "ut_compression" I tried, but I can't find myself through the SDL dependency hell... sorry. I installed Ubuntu's libsdl2-dev but apparently that wasn't enough... I still get "make[2]: sdl-config: Command not found" and "../arch/sandbox/cpu/sdl.c:9:21: fatal error: SDL/SDL.h: No such file or directory" when I'm trying to build. (I ran it with make -k now so I confirmed that it builds both lib/lz4_wrapper.o and test/compression.o without errors. I just can't test the linking step.) > Can you instead add this option to lib/Kconfig and put your help > there? We are moving away from the old CONFIGS. Done in next version. You should be able to replace this license with > > SPDX-License-Identifier: BSD-2-Clause > [...] > Should be able to use: > > SPDX-License-Identifier: GPL-2.0+ BSD-2-Clause Done in next version. > I can see why you want to keep lz4.c as is. But this file is written > by you, isn't it? If so, can you fix the checkpatch errors that are > fixable (e.g. run 'patman'). > > warning: lib/lz4_wrapper.c,41: do not add new typedefs > warning: lib/lz4_wrapper.c,42: do not add new typedefs > warning: lib/lz4_wrapper.c,43: do not add new typedefs > warning: lib/lz4_wrapper.c,44: do not add new typedefs > warning: lib/lz4_wrapper.c,45: do not add new typedefs > warning: lib/lz4_wrapper.c,47: storage class should be at the > beginning of the declaration > error: lib/lz4_wrapper.c,59: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,60: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,61: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,62: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,63: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,64: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,70: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,71: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,72: spaces prohibited around that ':' (ctx:WxW) > warning: lib/lz4_wrapper.c,77: __packed is preferred over > __attribute__((packed)) > warning: lib/lz4_wrapper.c,77: Adding new packed members is to be done > with care > error: lib/lz4_wrapper.c,83: spaces prohibited around that ':' (ctx:WxW) > error: lib/lz4_wrapper.c,84: spaces prohibited around that ':' (ctx:WxW) > warning: lib/lz4_wrapper.c,89: __packed is preferred over > __attribute__((packed)) > warning: lib/lz4_wrapper.c,89: Adding new packed members is to be done > with care > error: test/compression.c,282: return is not a function, parentheses > are not required Okay, I replaced __attribute__((packed)) with __packed and fixed the whitespace for bit fields. I think those are the only actionable ones... the camel case comes from names inside lz4.c, and I need the typedefs to map U-Boot's types to the ones lz4.c expects. > Could you return int here, and use proper error numbers below? Like > -EINVAL, -EPROTONOSUPPORT, -ENOSPC, etc. Okay, I switched it to the model where it returns an int and the size is an output pointer, like the other algorithms. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] Add support for LZ4 decompression algorithm
This patch adds support for LZ4-compressed FIT image contents. This algorithm has a slightly worse compression ration than LZO while being nearly twice as fast to decompress. When loading images from a fast storage medium this usually results in a boot time win. Compile-tested only since I don't have a U-Boot development system set up right now. The code was imported unchanged from coreboot where it's proven to work, though. I'm mostly interested in getting this recognized by mkImage for use in a downstream project. Signed-off-by: Julius Werner <jwer...@chromium.org> --- README | 13 +++ common/bootm.c | 10 ++ common/image.c | 1 + include/common.h | 3 + include/image.h| 1 + lib/Makefile | 1 + lib/lz4.c | 266 + lib/lz4_wrapper.c | 150 ++ test/compression.c | 57 9 files changed, 502 insertions(+) create mode 100644 lib/lz4.c create mode 100644 lib/lz4_wrapper.c diff --git a/README b/README index a13705a..4c98285 100644 --- a/README +++ b/README @@ -2035,6 +2035,19 @@ CBFS (Coreboot Filesystem) support the malloc area (as defined by CONFIG_SYS_MALLOC_LEN) should be at least 4MB. + CONFIG_LZ4 + + If this option is set, support for lz4 compressed images + is included. The LZ4 algorithm can run in-place as long as the + compressed image is loaded to the end of the output buffer, and + trades lower compression ratios for much faster decompression. + + NOTE: This implements the release version of the LZ4 frame + format as generated by default by the 'lz4' command line tool. + This is not the same as the outdated, less efficient legacy + frame format currently (2015) implemented in the Linux kernel + (generated by 'lz4 -l'). The two formats are incompatible. + CONFIG_LZMA If this option is set, support for lzma compressed diff --git a/common/bootm.c b/common/bootm.c index 667c934..0621363 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -389,6 +389,16 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, break; } #endif /* CONFIG_LZO */ +#ifdef CONFIG_LZ4 + case IH_COMP_LZ4: { + size_t size = ulz4fn(image_buf, image_len, load_buf, unc_len); + if (!size) + ret = -1; + else + image_len = size; + break; + } +#endif /* CONFIG_LZ4 */ default: printf("Unimplemented compression type %d\n", comp); return BOOTM_ERR_UNIMPLEMENTED; diff --git a/common/image.c b/common/image.c index 1325e07..c33749d 100644 --- a/common/image.c +++ b/common/image.c @@ -167,6 +167,7 @@ static const table_entry_t uimage_comp[] = { { IH_COMP_GZIP, "gzip", "gzip compressed", }, { IH_COMP_LZMA, "lzma", "lzma compressed", }, { IH_COMP_LZO,"lzo", "lzo compressed", }, + { IH_COMP_LZ4,"lz4", "lz4 compressed", }, { -1, "", "", }, }; diff --git a/include/common.h b/include/common.h index 68b24d0..82c75e2 100644 --- a/include/common.h +++ b/include/common.h @@ -826,6 +826,9 @@ int gzwrite(unsigned char *src, int len, u64 startoffs, u64 szexpected); +/* lib/lz4_wrapper.c */ +size_t ulz4fn(const void *src, size_t srcn, void *dst, size_t dstn); + /* lib/qsort.c */ void qsort(void *base, size_t nmemb, size_t size, int(*compar)(const void *, const void *)); diff --git a/include/image.h b/include/image.h index 8a864ae..08ae24a 100644 --- a/include/image.h +++ b/include/image.h @@ -259,6 +259,7 @@ struct lmb; #define IH_COMP_BZIP2 2 /* bzip2 Compression Used */ #define IH_COMP_LZMA 3 /* lzma Compression Used */ #define IH_COMP_LZO4 /* lzo Compression Used */ +#define IH_COMP_LZ45 /* lz4 Compression Used */ #define IH_MAGIC 0x27051956 /* Image Magic Number */ #define IH_NMLEN 32 /* Image Name Length*/ diff --git a/lib/Makefile b/lib/Makefile index 96f832e..3eecefa 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -34,6 +34,7 @@ obj-$(CONFIG_GZIP_COMPRESSED) += gzip.o obj-y += initcall.o obj-$(CONFIG_LMB) += lmb.o obj-y += ldiv.o +obj-$(CONFIG_LZ4) += lz4_wrapper.o obj-$(CONFIG_MD5) += md5.o obj-y += net_utils.o obj-$(CONFIG_PHYSMEM) += physmem.o diff --git a/lib/lz4.c b/lib/lz4.c new file mode 100644 index 00
[U-Boot] [PATCH v2] usb: ehci: Fix test mode for connected ports
The EHCI controller has some very specific requirements for the USB 2.0 port test modes, which were not closely followed in the initial test mode commit. It demands that the host controller is completely shut down (all ports suspended, Run/Stop bit unset) when activating test mode, and will not work on an already enumerated port. This patch fixes that by introducing a new ehci_shutdown() function that closely follows the procedure listed in EHCI 4.14. Also, when we have such a function anyway, we might as well also use it in usb_lowlevel_stop() to make the normal host controller shutdown cleaner. Signed-off-by: Julius Werner jwer...@chromium.org --- drivers/usb/host/ehci-hcd.c | 38 ++ 1 file changed, 38 insertions(+) diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index fdad739..3ae04c0 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -36,6 +36,12 @@ #define CONFIG_USB_MAX_CONTROLLER_COUNT 1 #endif +/* + * EHCI spec page 20 says that the HC may take up to 16 uFrames (= 4ms) to halt. + * Let's time out after 8 to have a little safety margin on top of that. + */ +#define HCHALT_TIMEOUT (8 * 1000) + static struct ehci_ctrl ehcic[CONFIG_USB_MAX_CONTROLLER_COUNT]; #define ALIGN_END_ADDR(type, ptr, size)\ @@ -190,6 +196,36 @@ out: return ret; } +static int ehci_shutdown(struct ehci_ctrl *ctrl) +{ + int i, ret = 0; + uint32_t cmd, reg; + + cmd = ehci_readl(ctrl-hcor-or_usbcmd); + cmd = ~(CMD_PSE | CMD_ASE); + ehci_writel(ctrl-hcor-or_usbcmd, cmd); + ret = handshake(ctrl-hcor-or_usbsts, STS_ASS | STS_PSS, 0, + 100 * 1000); + + if (!ret) { + for (i = 0; i CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS; i++) { + reg = ehci_readl(ctrl-hcor-or_portsc[i]); + reg |= EHCI_PS_SUSP; + ehci_writel(ctrl-hcor-or_portsc[i], reg); + } + + cmd = ~CMD_RUN; + ehci_writel(ctrl-hcor-or_usbcmd, cmd); + ret = handshake(ctrl-hcor-or_usbsts, STS_HALT, STS_HALT, + HCHALT_TIMEOUT); + } + + if (ret) + puts(EHCI failed to shut down host controller.\n); + + return ret; +} + static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz) { uint32_t delta, next; @@ -808,6 +844,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, } break; case USB_PORT_FEAT_TEST: + ehci_shutdown(ctrl); reg = ~(0xf 16); reg |= ((le16_to_cpu(req-index) 8) 0xf) 16; ehci_writel(status_reg, reg); @@ -878,6 +915,7 @@ unknown: int usb_lowlevel_stop(int index) { + ehci_shutdown(ehcic[index]); return ehci_hcd_stop(index); } -- 1.7.12.4 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports
Hi Simon, It seems like you could do something like /* * This is the delay for ...the spec requires a minimum of ... */ #define SOME_SUITABLE_NAME 8000 at the top of the file and then use it twice in your function. The file contains a dozen handshake() calls that have a literal 100 * 1000 in there, and none of them have any special meaning in regards to the spec... it's all just a random guess. I'd be happy to change the timeouts in my patch to 100 for consistency, but it really doesn't make sense to single this one out with a #define. (Maybe we should make it a #define DEFAULT_TIMEOUT 10 for all of them, but not in this patch.) ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports
Hello Marek, 8 seemed like a reasonable value. It is not the only possible one. As I said, the spec requires a minimum of 4.125 (125us to make sure the current microframe finishes, and 16 * 125us to wait for the 16 full microframes listed on page 20). 8 fulfills that constraint and leaves a little more tolerance, but you could also use 5, 16 or 100. I really don't care, so please just tell me what number you want to have if you don't like 8, because it just doesn't matter. It is no more or less magic than the 100ms used in most other handshake() calls (without any #define or explanation). ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports
Why 8 * 1000? It's not clear. I am not quite sure to be honest... it's been a while since I actually wrote this patch. The EHCI spec gives no clear max for the Periodic/Async Schedule disable (I assume not more than a frame, though), and says the Run/Stop bit must halt within 16 microframes (which would be 4.125ms). I guess 8 looked like a safe and not too large value that still makes the line fit on 80 characters (and it's not like most of the other timeouts in that file have a clear origin, so it's in good company). What do you want me to change it to? ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] usb: ehci: Fix test mode for connected ports
The EHCI controller has some very specific requirements for the USB 2.0 port test modes, which were not closely followed in the initial test mode commit. It demands that the host controller is completely shut down (all ports suspended, Run/Stop bit unset) when activating test mode, and will not work on an already enumerated port. This patch fixes that by introducing a new ehci_shutdown() function that closely follows the procedure listed in EHCI 4.14. Also, when we have such a function anyway, we might as well also use it in usb_lowlevel_stop() to make the normal host controller shutdown cleaner. Signed-off-by: Julius Werner jwer...@chromium.org --- drivers/usb/host/ehci-hcd.c | 31 +++ 1 file changed, 31 insertions(+) diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index fdad739..3143021 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -190,6 +190,35 @@ out: return ret; } +static int ehci_shutdown(struct ehci_ctrl *ctrl) +{ + int i, ret = 0; + uint32_t cmd, reg; + + cmd = ehci_readl(ctrl-hcor-or_usbcmd); + cmd = ~(CMD_PSE | CMD_ASE); + ehci_writel(ctrl-hcor-or_usbcmd, cmd); + ret = handshake(ctrl-hcor-or_usbsts, STS_ASS | STS_PSS, 0, 8 * 1000); + + if (!ret) { + for (i = 0; i CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS; i++) { + reg = ehci_readl(ctrl-hcor-or_portsc[i]); + reg |= EHCI_PS_SUSP; + ehci_writel(ctrl-hcor-or_portsc[i], reg); + } + + cmd = ~CMD_RUN; + ehci_writel(ctrl-hcor-or_usbcmd, cmd); + ret = handshake(ctrl-hcor-or_usbsts, STS_HALT, STS_HALT, + 8 * 1000); + } + + if (ret) + printf(EHCI failed to shut down host controller.\n); + + return ret; +} + static int ehci_td_buffer(struct qTD *td, void *buf, size_t sz) { uint32_t delta, next; @@ -808,6 +837,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, } break; case USB_PORT_FEAT_TEST: + ehci_shutdown(ctrl); reg = ~(0xf 16); reg |= ((le16_to_cpu(req-index) 8) 0xf) 16; ehci_writel(status_reg, reg); @@ -878,6 +908,7 @@ unknown: int usb_lowlevel_stop(int index) { + ehci_shutdown(ehcic[index]); return ehci_hcd_stop(index); } -- 1.7.12.4 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-Boot timeout
I saw that you had proposed a patch for increasing the U-Boot timeout to work with some USB flash devices: http://lists.denx.de/pipermail/u-boot/2010-December/083467.html Some devices do have strange characteristics, and increasing the timeout can make them work. However, it's tricky since it then makes every other device slower (if we are scanning for something that isn't there and waiting longer than we need to for 99% of the devices out there). Looks like that patch is just about the bulk transfer timeout after a failed TEST_UNIT_READY. Increasing that shouldn't really hurt, since if we get that far we already have a device for which basic enumeration worked (and then you probably want the code to do everything it can to get it to work). I have seen quite a few sticks that always fail their first TEST_UNIT_READY and then take very long to respond, though I'm not quite sure why (probably all using the same bad chipset/firmware). The highest I had took about 2.2 seconds, so I would set the timeout to at least 3 to be on the safe side. AFAIK Linux uses at least 5 seconds for most USB timeouts, so it's probably well-covered for this particular problem. I think it is a specific issue for some sticks that only concerns the response time after a failed TEST_UNIT_READY (which shouldn't be issued during data transfer), so if you see other errors during heavy activity on Linux they are probably unrelated. You should use a USB analyzer or the Linux usbmon framework to have a closer look at what transfers fail why if you want to get a better idea of what's wrong. On Mon, Aug 19, 2013 at 4:49 PM, Simon Glass s...@chromium.org wrote: +Julius, who may have more comment Hi Marshall, On Mon, Aug 19, 2013 at 4:41 PM, Marshall Ha mars...@gmail.com wrote: Hi Simon, I saw that you had proposed a patch for increasing the U-Boot timeout to work with some USB flash devices: http://lists.denx.de/pipermail/u-boot/2010-December/083467.html I have also ran into this problem in U-Boot, with the same root cause. I realize that this mailer is primarily for U-Boot, but I was curious whether this was an issue that you had encountered only in U-Boot, or whether you had also observed this problem after booting into an operating system. For example, I have observed that once booted into Linux, the USB devices will return I/O errors after heavy read/write activity, which I suspect to have the same root cause. I believe this code was merged into U-Boot - or at least in Chrome OS I'm not aware of any patches to this code that remain (on top of mainline) other than the USB3 work. Some devices do have strange characteristics, and increasing the timeout can make them work. However, it's tricky since it then makes every other device slower (if we are scanning for something that isn't there and waiting longer than we need to for 99% of the devices out there). So either you have found a real bug in U-Boot and Linux, or the device is broken. I'm not sure which, but it is worth investigating with an open mind. Regards, Simon Thanks, Marshall ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] usb: Use well-known descriptor sizes when parsing configuration
How would that make the code more consistent ? It seems if the device can not even provide valid config ep descriptor, the device is broken beyond salvation. Okay, sure, it's not important enough to argue about. Will resubmit it this way. The sizeof() thing is true for the configuration descriptor, but not for some others (e.g. endpoint) because U-Boot reserves fields for it's own stuff behind that. Urgh, then the structure defining the descriptor shall be separated out. Yes, maybe. But let's please not blow this patch up any more than it already is. Would be nice to clean this up into understandable format by defining a variable for the buffer[index] and than just simply comparing this var- bInterfaceNumber and curr_if_num . Agreed, but let's clean this up one patch at a time. Would you do a series on this maybe? On second thought, we already have the variable head (respectively head-bLength) to point there... I can just use that instead. So, let's just ignore broken descriptors. Done. Document this properly then. I'm already adding a comment to usb_parse_config() to point that out... I'll clarify that this includes sanitization in addition to byte swapping. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3] usb: Use well-known descriptor sizes when parsing configuration
The existing USB configuration parsing code relies on the descriptors' own length values when reading through the configuration blob. Since the size of those descriptors is always well-defined, we should rather use the known sizes instead of trusting device-provided values to be correct. Also adds some safety to potential out-of-order descriptors. Change-Id: I16f69dfdd6793aa0fe930b5148d4521f3e5c3090 Signed-off-by: Julius Werner jwer...@chromium.org --- common/usb.c | 87 +--- common/usb_hub.c | 14 +++-- 2 files changed, 75 insertions(+), 26 deletions(-) diff --git a/common/usb.c b/common/usb.c index 55fff5b..8a13f95 100644 --- a/common/usb.c +++ b/common/usb.c @@ -341,6 +341,7 @@ static int usb_set_maxpacket(struct usb_device *dev) /*** * Parse the config, located in buffer, and fills the dev-config structure. * Note that all little/big endian swapping are done automatically. + * (wTotalLength has already been swapped and sanitized when it was read.) */ static int usb_parse_config(struct usb_device *dev, unsigned char *buffer, int cfgno) @@ -361,24 +362,43 @@ static int usb_parse_config(struct usb_device *dev, head-bDescriptorType); return -1; } - memcpy(dev-config, buffer, buffer[0]); - le16_to_cpus((dev-config.desc.wTotalLength)); + if (head-bLength != USB_DT_CONFIG_SIZE) { + printf(ERROR: Invalid USB CFG length (%d)\n, head-bLength); + return -1; + } + memcpy(dev-config, head, USB_DT_CONFIG_SIZE); dev-config.no_of_if = 0; index = dev-config.desc.bLength; /* Ok the first entry must be a configuration entry, * now process the others */ head = (struct usb_descriptor_header *) buffer[index]; - while (index + 1 dev-config.desc.wTotalLength) { + while (index + 1 dev-config.desc.wTotalLength head-bLength) { switch (head-bDescriptorType) { case USB_DT_INTERFACE: + if (head-bLength != USB_DT_INTERFACE_SIZE) { + printf(ERROR: Invalid USB IF length (%d)\n, + head-bLength); + break; + } + if (index + USB_DT_INTERFACE_SIZE + dev-config.desc.wTotalLength) { + puts(USB IF descriptor overflowed buffer!\n); + break; + } if (((struct usb_interface_descriptor *) \ -buffer[index])-bInterfaceNumber != curr_if_num) { +head)-bInterfaceNumber != curr_if_num) { /* this is a new interface, copy new desc */ ifno = dev-config.no_of_if; + if (ifno = USB_MAXINTERFACES) { + puts(Too many USB interfaces!\n); + /* try to go on with what we have */ + return 1; + } if_desc = dev-config.if_desc[ifno]; dev-config.no_of_if++; - memcpy(if_desc, buffer[index], buffer[index]); + memcpy(if_desc, head, + USB_DT_INTERFACE_SIZE); if_desc-no_of_ep = 0; if_desc-num_altsetting = 1; curr_if_num = @@ -392,12 +412,31 @@ static int usb_parse_config(struct usb_device *dev, } break; case USB_DT_ENDPOINT: + if (head-bLength != USB_DT_ENDPOINT_SIZE) { + printf(ERROR: Invalid USB EP length (%d)\n, + head-bLength); + break; + } + if (index + USB_DT_ENDPOINT_SIZE + dev-config.desc.wTotalLength) { + puts(USB EP descriptor overflowed buffer!\n); + break; + } + if (ifno 0) { + puts(Endpoint descriptor out of order!\n); + break; + } epno = dev-config.if_desc[ifno].no_of_ep; if_desc = dev-config.if_desc[ifno]; + if (epno USB_MAXENDPOINTS) { + printf(Interface %d has too many endpoints!\n, + if_desc
Re: [U-Boot] [PATCH v2] usb: Use well-known descriptor sizes when parsing configuration
From a security / robustness standpoint, - if the descriptor length field is found to be abnormal, then the code should not process the packet at all. Here it seems it only warns then goes on to use the descriptor. Weren't you the guy who was so worried about poor Chinese devices who don't follow the spec just two mails ago? This code just tries to make the most of a broken response while keeping the host safe. Since the descriptors are just put into a structure and left there for the driver to decide which ones it's interested in anyway, more is usually better than less. - if for some reason (although I don't see any) the descriptor must be processed despite its wrong length, then only the bytes at positions 0..length-1 should be considered; bytes beyond the descriptor's announced length should be deemed undefined (and, I might add, should not be even be read from the device). This is not reading from the device, it's memcpy()'ing between an already read buffer and another data structure. memcpy()'ing one byte less into an uninitialized struct doesn't make the last byte any less undefined than copying garbage. Does this change fall into either using the well-defined length or adding safety to out-of-order descriptors category? If none, then there should be a third category of change documented in the commit message. It's another thing about the length... it prevents the code from running into an endless loop when it encounters a descriptor with length 0. - if (usb_get_hub_descriptor(dev, buffer, descriptor-bLength) 0) { + if (usb_get_hub_descriptor(dev, buffer, length) 0) { debug(usb_hub_configure: failed to get hub \ descriptor 2nd giving up %lX\n, dev-status); This message seems less clear than the one it replaces; I suspect failed to get hub descriptor 2nd giving up is not a valid sentence. I'm only touching the condition... if you don't like the message, I'd be happy to review your patch for it. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] usb: Use well-known descriptor sizes when parsing configuration
Mulling over this some more, I suspect if the device does have incorrect config descriptor, we should just ignore the device because it's broken piece of junk. I can change it if you insist, but I'd like to keep it to make the code look more consistent (since later on with the interface/endpoint descriptors, I think ignoring errors makes more sense). Looking at this, the memcpy is incorrect in the first place. It shouldn't memcpy into dev-config, but into dev-config.desc . And in turn, you an do memcpy(dev-config.desc, buffer, sizeof(dev-config.desc)); Which is even better, since you always take into account the size of the structure member. This would be worth fixing the right way. The sizeof() thing is true for the configuration descriptor, but not for some others (e.g. endpoint) because U-Boot reserves fields for it's own stuff behind that. So for consistency (and safety in case of future expansion) I went with the macros in all cases. Urgh, who the heck wrote this code. Damn that's a mess. Anyway, I suspect this might still explode if we go over USB_BUFSIZ in wTotalLength . Worth fixing. usb_get_configuration_no() now makes sure we don't read more than USB_BUFSIZ and stores the actually read amount in wTotalLength so we can use it without worrying here. Would be nice to clean this up into understandable format by defining a variable for the buffer[index] and than just simply comparing this var- bInterfaceNumber and curr_if_num . Agreed, but let's clean this up one patch at a time. if_desc = dev-config.if_desc[ifno]; dev-config.no_of_if++; - memcpy(if_desc, buffer[index], buffer[index]); + if (buffer[index] != USB_DT_INTERFACE_SIZE) + printf(Ignoring invalid USB IF length + (%d)\n, buffer[index]); Let's just ignore the descriptor if it's incorrect. Jokes about Chinese crap aside, I would try to err on the side of making it work if in any way possible, as long as the code stays safe. This is firmware so we often can't do much error recovery... either we can work with what we have, or we won't boot. Although I don't think these cases will happen in practice. - result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, tmp); - debug(get_conf_no %d Result %d, wLength %d\n, cfgno, result, tmp); + result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer, length); + debug(get_conf_no %d Result %d, wLength %d\n, cfgno, result, length); + config-wTotalLength = length; /* validated, with CPU byte order */ The above assignment is new, why ? This is the sanitization of wTotalLength I mentioned above. Maybe not the most obvious way to do it, but it's convenient since you have to pass the actually read length from here to usb_parse_config() somehow (to avoid things like reading into the leftover descriptors of a previously enumerated device). ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2] usb: Use well-known descriptor sizes when parsing configuration
The existing USB configuration parsing code relies on the descriptors' own length values when reading through the configuration blob. Since the size of those descriptors is always well-defined, we should rather use the known sizes instead of trusting device-provided values to be correct. Also adds some safety to potential out-of-order descriptors. Change-Id: I16f69dfdd6793aa0fe930b5148d4521f3e5c3090 Signed-off-by: Julius Werner jwer...@chromium.org --- common/usb.c | 73 ++-- common/usb_hub.c | 14 --- 2 files changed, 64 insertions(+), 23 deletions(-) diff --git a/common/usb.c b/common/usb.c index 55fff5b..ab7bafe 100644 --- a/common/usb.c +++ b/common/usb.c @@ -341,6 +341,7 @@ static int usb_set_maxpacket(struct usb_device *dev) /*** * Parse the config, located in buffer, and fills the dev-config structure. * Note that all little/big endian swapping are done automatically. + * (wTotalLength has already been swapped when it was read.) */ static int usb_parse_config(struct usb_device *dev, unsigned char *buffer, int cfgno) @@ -361,24 +362,39 @@ static int usb_parse_config(struct usb_device *dev, head-bDescriptorType); return -1; } - memcpy(dev-config, buffer, buffer[0]); - le16_to_cpus((dev-config.desc.wTotalLength)); + if (buffer[0] != USB_DT_CONFIG_SIZE) + printf(Ignoring invalid USB CFG length (%d)\n, buffer[0]); + memcpy(dev-config, buffer, USB_DT_CONFIG_SIZE); dev-config.no_of_if = 0; index = dev-config.desc.bLength; /* Ok the first entry must be a configuration entry, * now process the others */ head = (struct usb_descriptor_header *) buffer[index]; - while (index + 1 dev-config.desc.wTotalLength) { + while (index + 1 dev-config.desc.wTotalLength head-bLength) { switch (head-bDescriptorType) { case USB_DT_INTERFACE: + if (index + USB_DT_INTERFACE_SIZE + dev-config.desc.wTotalLength) { + puts(USB IF descriptor overflowed buffer!\n); + break; + } if (((struct usb_interface_descriptor *) \ buffer[index])-bInterfaceNumber != curr_if_num) { /* this is a new interface, copy new desc */ ifno = dev-config.no_of_if; + if (ifno = USB_MAXINTERFACES) { + puts(Too many USB interfaces!\n); + /* try to go on with what we have */ + return 1; + } if_desc = dev-config.if_desc[ifno]; dev-config.no_of_if++; - memcpy(if_desc, buffer[index], buffer[index]); + if (buffer[index] != USB_DT_INTERFACE_SIZE) + printf(Ignoring invalid USB IF length +(%d)\n, buffer[index]); + memcpy(if_desc, buffer[index], + USB_DT_INTERFACE_SIZE); if_desc-no_of_ep = 0; if_desc-num_altsetting = 1; curr_if_num = @@ -392,12 +408,29 @@ static int usb_parse_config(struct usb_device *dev, } break; case USB_DT_ENDPOINT: + if (index + USB_DT_ENDPOINT_SIZE + dev-config.desc.wTotalLength) { + puts(USB EP descriptor overflowed buffer!\n); + break; + } + if (ifno 0) { + puts(Endpoint descriptor out of order!\n); + break; + } epno = dev-config.if_desc[ifno].no_of_ep; if_desc = dev-config.if_desc[ifno]; + if (epno USB_MAXENDPOINTS) { + printf(Interface %d has too many endpoints!\n, + if_desc-desc.bInterfaceNumber); + return 1; + } /* found an endpoint */ if_desc-no_of_ep++; + if (buffer[index] != USB_DT_ENDPOINT_SIZE) + printf(Ignoring invalid USB EP length (%d)\n, + buffer[index]); memcpy
[U-Boot] [PATCH] usb: Use well-known descriptor sizes when parsing configuration
The existing USB configuration parsing code relies on the descriptors' own length values when reading through the configuration blob. Since the size of those descriptors is always well-defined, we should rather use the known sizes instead of trusting device-provided values to be correct. Also adds some safety to potential out-of-order descriptors. Change-Id: I16f69dfdd6793aa0fe930b5148d4521f3e5c3090 Signed-off-by: Julius Werner jwer...@chromium.org --- common/usb.c | 62 common/usb_hub.c | 14 - 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/common/usb.c b/common/usb.c index 55fff5b..4cf1d7a 100644 --- a/common/usb.c +++ b/common/usb.c @@ -341,6 +341,7 @@ static int usb_set_maxpacket(struct usb_device *dev) /*** * Parse the config, located in buffer, and fills the dev-config structure. * Note that all little/big endian swapping are done automatically. + * (wTotalLength has already been swapped when it was read.) */ static int usb_parse_config(struct usb_device *dev, unsigned char *buffer, int cfgno) @@ -361,24 +362,34 @@ static int usb_parse_config(struct usb_device *dev, head-bDescriptorType); return -1; } - memcpy(dev-config, buffer, buffer[0]); - le16_to_cpus((dev-config.desc.wTotalLength)); + memcpy(dev-config, buffer, USB_DT_CONFIG_SIZE); dev-config.no_of_if = 0; index = dev-config.desc.bLength; /* Ok the first entry must be a configuration entry, * now process the others */ head = (struct usb_descriptor_header *) buffer[index]; - while (index + 1 dev-config.desc.wTotalLength) { + while (index + 1 dev-config.desc.wTotalLength head-bLength) { switch (head-bDescriptorType) { case USB_DT_INTERFACE: + if (index + USB_DT_INTERFACE_SIZE + dev-config.desc.wTotalLength) { + puts(USB IF descriptor overflowed buffer!\n); + break; + } if (((struct usb_interface_descriptor *) \ buffer[index])-bInterfaceNumber != curr_if_num) { /* this is a new interface, copy new desc */ ifno = dev-config.no_of_if; + if (ifno = USB_MAXINTERFACES) { + puts(Too many USB interfaces!\n); + /* try to go on with what we have */ + return 1; + } if_desc = dev-config.if_desc[ifno]; dev-config.no_of_if++; - memcpy(if_desc, buffer[index], buffer[index]); + memcpy(if_desc, buffer[index], + USB_DT_INTERFACE_SIZE); if_desc-no_of_ep = 0; if_desc-num_altsetting = 1; curr_if_num = @@ -392,12 +403,26 @@ static int usb_parse_config(struct usb_device *dev, } break; case USB_DT_ENDPOINT: + if (index + USB_DT_ENDPOINT_SIZE + dev-config.desc.wTotalLength) { + puts(USB EP descriptor overflowed buffer!\n); + break; + } + if (ifno 0) { + puts(Endpoint descriptor out of order!\n); + break; + } epno = dev-config.if_desc[ifno].no_of_ep; if_desc = dev-config.if_desc[ifno]; + if (epno USB_MAXENDPOINTS) { + printf(Interface %d has too many endpoints!\n, + if_desc-desc.bInterfaceNumber); + return 1; + } /* found an endpoint */ if_desc-no_of_ep++; memcpy(if_desc-ep_desc[epno], - buffer[index], buffer[index]); + buffer[index], USB_DT_ENDPOINT_SIZE); ep_wMaxPacketSize = get_unaligned(dev-config.\ if_desc[ifno].\ ep_desc[epno].\ @@ -410,9 +435,18 @@ static int usb_parse_config(struct usb_device *dev, debug(if %d, ep %d\n, ifno, epno); break
Re: [U-Boot] [PATCH 3/3] usb: Correct CLEAR_FEATURE code in ehci-hcd
Thanks for taking charge of this Simon, looks good to me. Good catch with the missing break! Now that I look at it, the FEAT_C_ENABLE section seems to be broken as well, it should be setting EHCI_PS_PEC instead of EHCI_PS_PE. Maybe that code path is never used in practice. Acked-by: Julius Werner jwer...@chromium.org ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/3] usb: fix: Fixing Port status and feature number constants
On Wed, Apr 24, 2013 at 5:50 AM, Vivek Gautam gautam.vi...@samsung.com wrote: Fix the Port status bit constants and Port feature number constants as a part of USB 2.0 and USB 3.0 Hub class. Signed-off-by: Vivek Gautam gautam.vi...@samsung.com --- common/usb_hub.c | 40 include/usb_defs.h | 39 +++ 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/common/usb_hub.c b/common/usb_hub.c index dad0409..d9816e4 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -170,14 +170,24 @@ static struct usb_hub_device *usb_hub_allocate(void) static inline char *portspeed(int portstatus) { - if (portstatus (1 USB_PORT_FEAT_SUPERSPEED)) - return 5 Gb/s; - else if (portstatus (1 USB_PORT_FEAT_HIGHSPEED)) - return 480 Mb/s; - else if (portstatus (1 USB_PORT_FEAT_LOWSPEED)) - return 1.5 Mb/s; - else - return 12 Mb/s; + char *speed_str; + + switch (portstatus USB_PORT_STAT_SPEED_MASK) { + case USB_PORT_STAT_SUPER_SPEED: + speed_str = 5 Gb/s; + break; + case USB_PORT_STAT_HIGH_SPEED: + speed_str = 480 Mb/s; + break; + case USB_PORT_STAT_LOW_SPEED: + speed_str = 1.5 Mb/s; + break; + default: + speed_str = 12 Mb/s; + break; + } + + return speed_str; } Just a nitpick... you could save 8 lines here by moving the return into the case blocks. Rest of the patch set LGTM. int hub_port_reset(struct usb_device *dev, int port, @@ -275,14 +285,20 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) /* Allocate a new device struct for it */ usb = usb_alloc_new_device(dev-controller); - if (portstatus USB_PORT_STAT_SUPER_SPEED) + switch (portstatus USB_PORT_STAT_SPEED_MASK) { + case USB_PORT_STAT_SUPER_SPEED: usb-speed = USB_SPEED_SUPER; - else if (portstatus USB_PORT_STAT_HIGH_SPEED) + break; + case USB_PORT_STAT_HIGH_SPEED: usb-speed = USB_SPEED_HIGH; - else if (portstatus USB_PORT_STAT_LOW_SPEED) + break; + case USB_PORT_STAT_LOW_SPEED: usb-speed = USB_SPEED_LOW; - else + break; + default: usb-speed = USB_SPEED_FULL; + break; + } dev-children[port] = usb; usb-parent = dev; diff --git a/include/usb_defs.h b/include/usb_defs.h index 6ce6791..4f3601a 100644 --- a/include/usb_defs.h +++ b/include/usb_defs.h @@ -215,8 +215,6 @@ #define USB_PORT_FEAT_POWER 8 #define USB_PORT_FEAT_LOWSPEED 9 #define USB_PORT_FEAT_HIGHSPEED 10 -#define USB_PORT_FEAT_FULLSPEED 11 -#define USB_PORT_FEAT_SUPERSPEED 12 #define USB_PORT_FEAT_C_CONNECTION 16 #define USB_PORT_FEAT_C_ENABLE 17 #define USB_PORT_FEAT_C_SUSPEND 18 @@ -224,6 +222,17 @@ #define USB_PORT_FEAT_C_RESET20 #define USB_PORT_FEAT_TEST 21 +/* + * Changes to Port feature numbers for Super speed, + * from USB 3.0 spec Table 10-8 + */ +#define USB_SS_PORT_FEAT_U1_TIMEOUT23 +#define USB_SS_PORT_FEAT_U2_TIMEOUT24 +#define USB_SS_PORT_FEAT_C_LINK_STATE 25 +#define USB_SS_PORT_FEAT_C_CONFIG_ERROR26 +#define USB_SS_PORT_FEAT_BH_RESET 28 +#define USB_SS_PORT_FEAT_C_BH_RESET29 + /* wPortStatus bits */ #define USB_PORT_STAT_CONNECTION0x0001 #define USB_PORT_STAT_ENABLE0x0002 @@ -233,20 +242,18 @@ #define USB_PORT_STAT_POWER 0x0100 #define USB_PORT_STAT_LOW_SPEED 0x0200 #define USB_PORT_STAT_HIGH_SPEED0x0400 /* support for EHCI */ -#define USB_PORT_STAT_FULL_SPEED0x0800 -#define USB_PORT_STAT_SUPER_SPEED 0x1000 /* support for XHCI */ -#define USB_PORT_STAT_SPEED\ - (USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED | \ - USB_PORT_STAT_FULL_SPEED | USB_PORT_STAT_SUPER_SPEED) +#define USB_PORT_STAT_SUPER_SPEED 0x0600 /* faking support to XHCI */ +#define USB_PORT_STAT_SPEED_MASK \ + (USB_PORT_STAT_LOW_SPEED | USB_PORT_STAT_HIGH_SPEED) /* - * Additions to wPortStatus bit field from USB 3.0 - * See USB 3.0 spec Table 10-10 + * Changes to wPortStatus bit field in USB 3.0 + * See USB 3.0 spec Table 10-11 */ -#define USB_PORT_STAT_LINK_STATE 0x01e0 +#define USB_SS_PORT_STAT_LINK_STATE0x01e0 #define USB_SS_PORT_STAT_POWER 0x0200 #define USB_SS_PORT_STAT_SPEED 0x1c00 -#define USB_PORT_STAT_SPEED_5GBPS 0x +#define USB_SS_PORT_STAT_SPEED_5GBPS 0x /* wPortChange bits */ #define USB_PORT_STAT_C_CONNECTION 0x0001 @@ -256,12 +263,12 @@ #define USB_PORT_STAT_C_RESET 0x0010 /* - *
Re: [U-Boot] [U-Boot,v3,7/8] usb: hub: Reset only usb 2.0 ports
Sorry, forgot this one yesterday. I would consider to just drop/revert this patch entirely. It's not wrong, but it adds complexity where it is not needed. You don't have to reset SuperSpeed devices, but it shouldn't hurt either and from what I can tell Linux does it as well. As per XHCI specifications USB 3.0 protocol ports attempt to advance to 'Enabled' state; however USB 2.0 protocol ports require software reset to advance them to 'Enabled' state. Thereby, inferring that software need to reset USB 2.0 protocol ports invariably (as per EHCI spec or xHCI spec). Signed-off-by: Vivek Gautam gautam.vi...@samsung.com --- This patch added in V3(current-version) of this patch-series. common/usb_hub.c | 23 +++ 1 files changed, 19 insertions(+), 4 deletions(-) diff --git a/common/usb_hub.c b/common/usb_hub.c index 1e225e6..eedbcf2 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -233,6 +233,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) struct usb_device *usb; ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); unsigned short portstatus; + uint32_t do_port_reset = 1; /* Check status */ if (usb_get_port_status(dev, port + 1, portsts) 0) { @@ -246,6 +247,9 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) le16_to_cpu(portsts-wPortChange), portspeed(portstatus)); + if (portstatus (1 USB_PORT_FEAT_SUPERSPEED)) + do_port_reset = 0; + /* Clear the connection change status */ usb_clear_port_feature(dev, port + 1, USB_PORT_FEAT_C_CONNECTION); @@ -259,10 +263,21 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) } mdelay(200); - /* Reset the port */ - if (hub_port_reset(dev, port, portstatus) 0) { - printf(cannot reset port %i!?\n, port + 1); - return; + /* + * Reset the port: + * As per xHCI protocol, USB 3.0 devices do not require + * a port reset, however USB 2.0 device do require the same + * to let ports proceed to 'enabled' state + * + * XXX: Will this break EHCI ?? + * probably not, above condition for 'do_port_reset' checks for + * speed, and for EHCI it can't reach Super speed anyways. + */ + if (do_port_reset) { + if (hub_port_reset(dev, port, portstatus) 0) { + printf(cannot reset port %i!?\n, port + 1); + return; + } } mdelay(200); ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, v3, 3/8] usb: hub: Power-cycle on root-hub ports
This 20ms delay is truely being taken to be on safer side (twice of Power-on-to-power-good), its not observational. In my earlier patch-series, we were doing the same way as you are suggesting here (power-on ports only if they aren't), however Marek suggested to power-cycle the ports. This would ensure that we don't have any spurious Port status (telling us that port is powered up). Fair enough... I guess 20ms overall is not a big deal in the whole picture. I sometimes tend to overoptimize things... Actually i was seeing a strage behavior on USB 2.0 protocol ports available with XHCI. Since all ports with XHCI are powered-up after a Chip-reset, the instant we do a power-on on these ports (as with original code - simply setting the PORT_POWER feature), the Connect status change bit used to get cleared (however Current connect status bit was still set). This is a bug in your XHCI code I hadn't spotted before: in xhci_submit_root(SET_FEATURE) you read the PORTSC register, add a bit, and write it again... without calling xhci_port_state_to_neutral() inbetween. Thus you clear any pending change events when you set PORT_POWER. (Seems the EHCI code has a similar bug in CLEAR_FEATURE, now that I'm looking at it... someone should put a 'reg = ~EHCI_PS_CLEAR;' in there.) Hmm, so right now we are doing this for one port at a time. I am sure parallelising things as you suggested would be best to do here, but can you please explain, would handling one port at a time lead to unwanted behavior from Host's side somehow ? It doesn't affect correctness, just the amount of time wasted. Doing it one port at a time means you wait 100ms on a 5 port root hub, while you could get by with 20ms overall by parallelizing it. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, v3, 6/8] USB: SS: Add support for Super Speed USB interface
I would reword Additions to Changes and explicitly state that these fields concern the new USB 3.0 hub descriptor format, which is different and exclusively applies to SuperSpeed hubs. This bit fields are actually additions to USB 3.0 framework. These fields are not defined in USB 2.0 specs, or else they are reserved there. If you insist i can modify this as suggested. Yes, okay, I think we mean the same thing here. I just wanted to point out that the USB 3.0 port status is actually a completely different thing from the USB 2.0 port status (which is still valid for non-SuperSpeed hubs in USB 3.0 environments, even though the spec hasn't printed it out again). They did not just *add* a few bits to the old status bit field, they actually *changed* existing bits (not just reserved ones)... e.g. bit 9 means LOW_SPEED for USB 2.0 hubs but PORT_POWER for SuperSpeed hubs. I found this very confusing when I first read it, so I thought it might be worth making it extra obvious through comments. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, v3, 6/8] USB: SS: Add support for Super Speed USB interface
Migrating my comments here for public discussion. This adds usb framework support for super-speed usb, which will further facilitate to add stack support for xHCI. Signed-off-by: Vikas C Sajjan vikas.saj...@samsung.com Signed-off-by: Vivek Gautam gautam.vi...@samsung.com --- Changes from v2: - Replacing if-else with switch-case in portspeed() in cmd_usb.c common/cmd_usb.c | 24 ++-- common/usb.c |5 + common/usb_hub.c |8 ++-- include/usb.h |6 ++ include/usb_defs.h | 24 +++- 5 files changed, 58 insertions(+), 9 deletions(-) diff --git a/common/cmd_usb.c b/common/cmd_usb.c index dacdc2d..594385a 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -271,12 +271,24 @@ static void usb_display_config(struct usb_device *dev) static inline char *portspeed(int speed) { - if (speed == USB_SPEED_HIGH) - return 480 Mb/s; - else if (speed == USB_SPEED_LOW) - return 1.5 Mb/s; - else - return 12 Mb/s; + char *speed_str; + + switch (speed) { + case USB_SPEED_SUPER: + speed_str = 5 Gb/s; + break; + case USB_SPEED_HIGH: + speed_str = 480 Mb/s; + break; + case USB_SPEED_LOW: + speed_str = 1.5 Mb/s; + break; + default: + speed_str = 12 Mb/s; + break; + } + + return speed_str; } /* shows the device tree recursively */ diff --git a/common/usb.c b/common/usb.c index 3a96a34..55fff5b 100644 --- a/common/usb.c +++ b/common/usb.c @@ -409,6 +409,11 @@ static int usb_parse_config(struct usb_device *dev, wMaxPacketSize); debug(if %d, ep %d\n, ifno, epno); break; + case USB_DT_SS_ENDPOINT_COMP: + if_desc = dev-config.if_desc[ifno]; + memcpy(if_desc-ss_ep_comp_desc[epno], + buffer[index], buffer[index]); + break; default: if (head-bLength == 0) return 1; diff --git a/common/usb_hub.c b/common/usb_hub.c index ab41943..1e225e6 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -165,7 +165,9 @@ static struct usb_hub_device *usb_hub_allocate(void) static inline char *portspeed(int portstatus) { - if (portstatus (1 USB_PORT_FEAT_HIGHSPEED)) + if (portstatus (1 USB_PORT_FEAT_SUPERSPEED)) It doesn't make sense to use the USB_PORT_FEAT_ constants to read a port status here. These should be USB_PORT_STAT_ instead (you could use a switch statement if you mask and shift them correctly). + return 5 Gb/s; + else if (portstatus (1 USB_PORT_FEAT_HIGHSPEED)) return 480 Mb/s; else if (portstatus (1 USB_PORT_FEAT_LOWSPEED)) return 1.5 Mb/s; @@ -268,7 +270,9 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) /* Allocate a new device struct for it */ usb = usb_alloc_new_device(dev-controller); - if (portstatus USB_PORT_STAT_HIGH_SPEED) + if (portstatus USB_PORT_STAT_SUPER_SPEED) + usb-speed = USB_SPEED_SUPER; + else if (portstatus USB_PORT_STAT_HIGH_SPEED) usb-speed = USB_SPEED_HIGH; else if (portstatus USB_PORT_STAT_LOW_SPEED) usb-speed = USB_SPEED_LOW; Should change to switch statement after implementing my suggestion to usb_defs.h diff --git a/include/usb.h b/include/usb.h index d79c865..d7b082d 100644 --- a/include/usb.h +++ b/include/usb.h @@ -76,6 +76,12 @@ struct usb_interface { unsigned char act_altsetting; struct usb_endpoint_descriptor ep_desc[USB_MAXENDPOINTS]; + /* + * Super Speed Device will have Super Speed Endpoint + * Companion Descriptor (section 9.6.7 of usb 3.0 spec) + * Revision 1.0 June 6th 2011 + */ + struct usb_ss_ep_comp_descriptor ss_ep_comp_desc[USB_MAXENDPOINTS]; } __attribute__ ((packed)); /* Configuration information.. */ diff --git a/include/usb_defs.h b/include/usb_defs.h index 0c78d9d..e2aaef3 100644 --- a/include/usb_defs.h +++ b/include/usb_defs.h @@ -203,6 +203,8 @@ #define USB_PORT_FEAT_POWER 8 #define USB_PORT_FEAT_LOWSPEED 9 #define USB_PORT_FEAT_HIGHSPEED 10 +#define USB_PORT_FEAT_FULLSPEED 11 +#define USB_PORT_FEAT_SUPERSPEED 12 Speed is never set as a feature anyway. We should just get rid of all of these and always use USB_PORT_STAT_ for them. #define USB_PORT_FEAT_C_CONNECTION 16 #define USB_PORT_FEAT_C_ENABLE 17 #define USB_PORT_FEAT_C_SUSPEND 18 @@ -218,8 +220,20 @@ #define USB_PORT_STAT_POWER 0x0100 #define USB_PORT_STAT_LOW_SPEED 0x0200 #define USB_PORT_STAT_HIGH_SPEED0x0400
Re: [U-Boot] [U-Boot, v3, 4/8] usb: Update device class in usb device's descriptor
Fetch the device class into usb device's dwcriptors, so that the host controller's driver can use this info to differentiate between HUB and DEVICE. Signed-off-by: Amar amarendra...@samsung.com --- Changes from v2: - none common/usb.c |5 + 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/common/usb.c b/common/usb.c index 8407974..3a96a34 100644 --- a/common/usb.c +++ b/common/usb.c @@ -879,6 +879,11 @@ int usb_new_device(struct usb_device *dev) } dev-descriptor.bMaxPacketSize0 = desc-bMaxPacketSize0; + /* + * Fetch the device class, driver can use this info + * to differentiate between HUB and DEVICE. + */ + dev-descriptor.bDeviceClass = desc-bDeviceClass; There is nothing wrong with this, but I also don't think you need it. In those places where you check for the hub class in your XHCI stack, you actually want to check whether the device is the root hub... so you should do that differently to avoid breaking external hubs (e.g. by comparing the device address). I think we should drop this since there is no real use for it. /* find the port number we're at */ if (parent) { ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, v3, 3/8] usb: hub: Power-cycle on root-hub ports
XHCI ports are powered on after a H/W reset, however EHCI ports are not. So disabling and re-enabling power on all ports invariably. Signed-off-by: Amar amarendra...@samsung.com Signed-off-by: Vivek Gautam gautam.vi...@samsung.com --- Changes from v2: - Replaced USB_HUB_PRINTFs to debug() common/usb_hub.c | 34 ++ 1 files changed, 34 insertions(+), 0 deletions(-) diff --git a/common/usb_hub.c b/common/usb_hub.c index f2a0285..e4f4e3c 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -100,11 +100,45 @@ static void usb_hub_power_on(struct usb_hub_device *hub) int i; struct usb_device *dev; unsigned pgood_delay = hub-desc.bPwrOn2PwrGood * 2; + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); + unsigned short portstatus; + int ret; dev = hub-pusb_dev; /* Enable power to the ports */ debug(enabling power on all ports\n); for (i = 0; i dev-maxchild; i++) { + /* + * Power-cycle the ports here: aka, + * turning them off and turning on again. + */ + usb_clear_port_feature(dev, i + 1, USB_PORT_FEAT_POWER); + debug(port %d returns %lX\n, i + 1, dev-status); + + /* Wait at least 2*bPwrOn2PwrGood for PP to change */ + mdelay(pgood_delay); This adds 20ms per port to USB boot times... is it really necessary? Why do we need to powercycle XHCI ports? Couldn't we just check if the ports are powered and only power them on if they aren't? If you insist, please at least parallelize this. Disable power on all ports, wait, then check and reenable it. + + ret = usb_get_port_status(dev, i + 1, portsts); + if (ret 0) { + debug(port %d: get_port_status failed\n, i + 1); This is a severe error (because the ports won't come up again), so I think it should be a printf() instead of a debug(). + return; + } + + /* + * Check to confirm the state of Port Power: + * xHCI says After modifying PP, s/w shall read + * PP and confirm that it has reached the desired state + * before modifying it again, undefined behavior may occur + * if this procedure is not followed. + * EHCI doesn't say anything like this, but no harm in keeping + * this. + */ + portstatus = le16_to_cpu(portsts-wPortStatus); + if (portstatus (USB_PORT_STAT_POWER 1)) { + debug(port %d: Port power change failed\n, i + 1); Same as above. + return; + } + usb_set_port_feature(dev, i + 1, USB_PORT_FEAT_POWER); debug(port %d returns %lX\n, i + 1, dev-status); } ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [U-Boot, v3, 6/8] USB: SS: Add support for Super Speed USB interface
These patches haven't gone in yet, right? I think Vivek wants to discuss/update them himself, he just asked me to move my reviews to this thread. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 0/8] usb: ss: Some fixes and cleanup for USB super-speed support
Hi Marek, I'm sorry, that must have slipped by when I ported the change from my local fork. Your patch is correct, you just need to add the ctrl- there. On Thu, Apr 18, 2013 at 5:43 AM, Marek Vasut ma...@denx.de wrote: Dear Vivek Gautam, HI Marek, On Thu, Apr 18, 2013 at 4:29 PM, Vivek Gautam gautamvivek1...@gmail.com wrote: On Thu, Apr 18, 2013 at 11:54 AM, Vivek Gautam gautamvivek1...@gmail.com wrote: Hi Marek, On Sun, Apr 14, 2013 at 11:43 PM, Marek Vasut ma...@denx.de wrote: Dear Vivek Gautam, Based on 'u-boot-usb' master branch. This patch-series includes majorly some clean-up, few fixes and then some basic super-speed usb infrastructure addition, to help put support for XHCI in near future. btw can you test your patches with MAKEALL -a arm? I get this: - SUMMARY Boards compiled: 306 Boards with errors: 65 ( qong mx35pdk gplugd at91sam9m10g45ek_nandflash pogo_e02 dns325 iconnect lschlv2 lsxhl d2net_v2 inetspace_v2 net2big_v2 netspace_lite_v2 netspace_max_v2 netspace_v2 wireless_space dreamplug guruplug mv88f6281gtw_ge openrd_base openrd_client openrd_ultimate rd6281a sheevaplug ib62x0 dockstar tk71 zmx25 mx23_olinuxino apx4devkit mx23evk m28evk mx28evk sc_sps_1 edminiv2 mx51_efikamx mx51_efikasb mx51evk mx53loco mx6qsabreauto mx6qsabrelite nitrogen6dl nitrogen6dl2g nitrogen6q nitrogen6q2g nitrogen6s nitrogen6s1g cm_t35 mt_ventoux omap3_beagle mcx twister omap4_panda snow smdk5250 harmony seaboard ventana whistler colibri_t20_iris plutux medcom-wide tec paz00 trimslice ) -- Tried with MAKEALL got following result - SUMMARY Boards compiled: 306 Boards with errors: 1 ( omap3_evm ) Boards with warnings but no errors: 17 ( VCMA9 smdk2410 kzm9g balloon3 h2200 lubbock palmld palmtc polaris pxa255_idp trizepsiv vpac270_nor_128 vpac270_nor_256 vpac270_ond_256 xaeniax zipitz2 colibri_pxa270 ) -- I actually checked now just for omap3_evm configuration by trying out: make distclean make omap3_evm_config make But strangely i didn't get any build errros for omap3_evm board on explicitly compiling for it. Any clue ? **Without my patches i get following result - SUMMARY Boards compiled: 306 Boards with warnings but no errors: 17 ( VCMA9 smdk2410 kzm9g balloon3 h2200 lubbock palmld palmtc polaris pxa255_idp trizepsiv vpac270_nor_128 vpac270_nor_256 vpac270_ond_256 xaeniax zipitz2 colibri_pxa270 ) -- There's something to do with Cross Compiler version ?? btw what environment are you compiling the source with. I am using arm-2011.09 cross toolchain. I use ELDK 5.3 and Debian 4.7.2-5 to do by builds. But now that I'm looking at it, it's this patch that caused it, sorry. commit 28b31a5937b89528c40df24dd6c9122578880605 Author: Julius Werner jwer...@chromium.org Date: Thu Feb 28 18:08:40 2013 + usb: Add new command to set USB 2.0 port test modes diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 8464850..19d4352 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -630,7 +630,7 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, printf(The request port(%d) is not configured\n, port - 1); return -1; } - status_reg = (uint32_t *)hcor-or_portsc[port - 1]; + status_reg = (uint32_t *)ctrl-hcor-or_portsc[port - 1]; srclen = 0; debug(req=%u (%#x), type=%u (%#x), value=%u, index=%u\n, This change fixes is, right Julius ? Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] usb: Add new command to set USB 2.0 port test modes
Oops... I somehow pulled three lines of other local changes in there that don't belong when I cherry-picked this into my upstream branch. Sorry, will resubmit this once more. On Tue, Feb 19, 2013 at 5:14 PM, Julius Werner jwer...@chromium.org wrote: This patch adds a new 'usb test' command, that will set a port to a USB 2.0 test mode (see USB 2.0 spec 7.1.20). It supports all five test modes on both downstream hub ports and ordinary device's upstream ports. In addition, it supports EHCI root hub ports. Signed-off-by: Julius Werner jwer...@chromium.org --- common/cmd_usb.c| 121 +++- drivers/usb/host/ehci-hcd.c | 30 +++ include/usb_defs.h | 13 + 3 files changed, 130 insertions(+), 34 deletions(-) diff --git a/common/cmd_usb.c b/common/cmd_usb.c index dacdc2d..adc5f02 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -269,6 +269,22 @@ static void usb_display_config(struct usb_device *dev) printf(\n); } +static struct usb_device *usb_find_device(int devnum) +{ + struct usb_device *dev; + int d; + + for (d = 0; d USB_MAX_DEVICE; d++) { + dev = usb_get_dev_index(d); + if (dev == NULL) + return NULL; + if (dev-devnum == devnum) + return dev; + } + + return NULL; +} + static inline char *portspeed(int speed) { if (speed == USB_SPEED_HIGH) @@ -348,6 +364,66 @@ static void usb_show_tree(struct usb_device *dev) usb_show_tree_graph(dev, preamble[0]); } +static int usb_test(struct usb_device *dev, int port, char* arg) +{ + int mode; + + if (port dev-maxchild) { + printf(Device is no hub or does not have %d ports.\n, port); + return 1; + } + + switch (arg[0]) { + case 'J': + case 'j': + printf(Setting Test_J mode); + mode = USB_TEST_MODE_J; + break; + case 'K': + case 'k': + printf(Setting Test_K mode); + mode = USB_TEST_MODE_K; + break; + case 'S': + case 's': + printf(Setting Test_SE0_NAK mode); + mode = USB_TEST_MODE_SE0_NAK; + break; + case 'P': + case 'p': + printf(Setting Test_Packet mode); + mode = USB_TEST_MODE_PACKET; + break; + case 'F': + case 'f': + printf(Setting Test_Force_Enable mode); + mode = USB_TEST_MODE_FORCE_ENABLE; + break; + default: + printf(Unrecognized test mode: %s\nAvailable modes: + J, K, S[E0_NAK], P[acket], F[orce_Enable]\n, arg); + return 1; + } + + if (port) + printf( on downstream facing port %d...\n, port); + else + printf( on upstream facing port...\n); + + if (usb_control_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_FEATURE, + port ? USB_RT_PORT : USB_RECIP_DEVICE, + port ? USB_PORT_FEAT_TEST : USB_FEAT_TEST, + (mode 8) | port, + NULL, 0, USB_CNTL_TIMEOUT) == -1) { + printf(Error during SET_FEATURE.\n); + return 1; + } else { + printf(Test mode successfully set. Use 'usb start' + to return to normal operation.\n); + return 0; + } +} + /** * usb boot command intepreter. Derived from diskboot @@ -441,17 +517,9 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } return 0; } else { - int d; - - i = simple_strtoul(argv[2], NULL, 16); + i = simple_strtoul(argv[2], NULL, 10); printf(config for device %d\n, i); - for (d = 0; d USB_MAX_DEVICE; d++) { - dev = usb_get_dev_index(d); - if (dev == NULL) - break; - if (dev-devnum == i) - break; - } + dev = usb_find_device(i); if (dev == NULL) { printf(*** No device available ***\n); return 0; @@ -462,6 +530,18 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } return 0; } + if (strncmp(argv[1], test, 4) == 0
[U-Boot] [PATCH] usb: Add new command to set USB 2.0 port test modes
This patch adds a new 'usb test' command, that will set a port to a USB 2.0 test mode (see USB 2.0 spec 7.1.20). It supports all five test modes on both downstream hub ports and ordinary device's upstream ports. In addition, it supports EHCI root hub ports. Signed-off-by: Julius Werner jwer...@chromium.org --- common/cmd_usb.c| 121 +++- drivers/usb/host/ehci-hcd.c | 25 + include/usb_defs.h | 13 + 3 files changed, 125 insertions(+), 34 deletions(-) diff --git a/common/cmd_usb.c b/common/cmd_usb.c index dacdc2d..adc5f02 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -269,6 +269,22 @@ static void usb_display_config(struct usb_device *dev) printf(\n); } +static struct usb_device *usb_find_device(int devnum) +{ + struct usb_device *dev; + int d; + + for (d = 0; d USB_MAX_DEVICE; d++) { + dev = usb_get_dev_index(d); + if (dev == NULL) + return NULL; + if (dev-devnum == devnum) + return dev; + } + + return NULL; +} + static inline char *portspeed(int speed) { if (speed == USB_SPEED_HIGH) @@ -348,6 +364,66 @@ static void usb_show_tree(struct usb_device *dev) usb_show_tree_graph(dev, preamble[0]); } +static int usb_test(struct usb_device *dev, int port, char* arg) +{ + int mode; + + if (port dev-maxchild) { + printf(Device is no hub or does not have %d ports.\n, port); + return 1; + } + + switch (arg[0]) { + case 'J': + case 'j': + printf(Setting Test_J mode); + mode = USB_TEST_MODE_J; + break; + case 'K': + case 'k': + printf(Setting Test_K mode); + mode = USB_TEST_MODE_K; + break; + case 'S': + case 's': + printf(Setting Test_SE0_NAK mode); + mode = USB_TEST_MODE_SE0_NAK; + break; + case 'P': + case 'p': + printf(Setting Test_Packet mode); + mode = USB_TEST_MODE_PACKET; + break; + case 'F': + case 'f': + printf(Setting Test_Force_Enable mode); + mode = USB_TEST_MODE_FORCE_ENABLE; + break; + default: + printf(Unrecognized test mode: %s\nAvailable modes: + J, K, S[E0_NAK], P[acket], F[orce_Enable]\n, arg); + return 1; + } + + if (port) + printf( on downstream facing port %d...\n, port); + else + printf( on upstream facing port...\n); + + if (usb_control_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_FEATURE, + port ? USB_RT_PORT : USB_RECIP_DEVICE, + port ? USB_PORT_FEAT_TEST : USB_FEAT_TEST, + (mode 8) | port, + NULL, 0, USB_CNTL_TIMEOUT) == -1) { + printf(Error during SET_FEATURE.\n); + return 1; + } else { + printf(Test mode successfully set. Use 'usb start' + to return to normal operation.\n); + return 0; + } +} + /** * usb boot command intepreter. Derived from diskboot @@ -441,17 +517,9 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } return 0; } else { - int d; - - i = simple_strtoul(argv[2], NULL, 16); + i = simple_strtoul(argv[2], NULL, 10); printf(config for device %d\n, i); - for (d = 0; d USB_MAX_DEVICE; d++) { - dev = usb_get_dev_index(d); - if (dev == NULL) - break; - if (dev-devnum == i) - break; - } + dev = usb_find_device(i); if (dev == NULL) { printf(*** No device available ***\n); return 0; @@ -462,6 +530,18 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } return 0; } + if (strncmp(argv[1], test, 4) == 0) { + if (argc 5) + return CMD_RET_USAGE; + i = simple_strtoul(argv[2], NULL, 10); + dev = usb_find_device(i); + if (dev == NULL) { + printf(Device %d does not exist.\n, i); + return 1; + } + i = simple_strtoul(argv[3], NULL, 10
Re: [U-Boot] [PATCH] usb: Add new command to set USB 2.0 port test modes
Hi Tom, This patch is not really target specific, it works on all platforms that support USB 2.0. I just wrapped it in a new config option because most people probably won't need it… it is only really useful for hardware verification, so I figured whoever needs it can hack in the config option himself. However, I could also just remove the #ifdef and activate it by default if you prefer that. No new code will get executed unless you type the new command, so it should not have any negative effect besides a negligible code size increase. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] usb: Add new command to set USB 2.0 port test modes
I guess you are right about the bit rot, it makes more sense to keep this on so people notice when things break. I have compiled it for smdk5250 (though it's all common and EHCI code, so that should make no difference) and only measured a 1.1k increase in uncompressed code size (less than 0.3%). I will just resubmit this without the ifdefs. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] usb: Add new command to set USB 2.0 port test modes
This patch adds a new 'usb test' command, that will set a port to a USB 2.0 test mode (see USB 2.0 spec 7.1.20). It supports all five test modes on both downstream hub ports and ordinary device's upstream ports. In addition, it supports EHCI root hub ports. Signed-off-by: Julius Werner jwer...@chromium.org --- common/cmd_usb.c| 121 +++- drivers/usb/host/ehci-hcd.c | 30 +++ include/usb_defs.h | 13 + 3 files changed, 130 insertions(+), 34 deletions(-) diff --git a/common/cmd_usb.c b/common/cmd_usb.c index dacdc2d..adc5f02 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -269,6 +269,22 @@ static void usb_display_config(struct usb_device *dev) printf(\n); } +static struct usb_device *usb_find_device(int devnum) +{ + struct usb_device *dev; + int d; + + for (d = 0; d USB_MAX_DEVICE; d++) { + dev = usb_get_dev_index(d); + if (dev == NULL) + return NULL; + if (dev-devnum == devnum) + return dev; + } + + return NULL; +} + static inline char *portspeed(int speed) { if (speed == USB_SPEED_HIGH) @@ -348,6 +364,66 @@ static void usb_show_tree(struct usb_device *dev) usb_show_tree_graph(dev, preamble[0]); } +static int usb_test(struct usb_device *dev, int port, char* arg) +{ + int mode; + + if (port dev-maxchild) { + printf(Device is no hub or does not have %d ports.\n, port); + return 1; + } + + switch (arg[0]) { + case 'J': + case 'j': + printf(Setting Test_J mode); + mode = USB_TEST_MODE_J; + break; + case 'K': + case 'k': + printf(Setting Test_K mode); + mode = USB_TEST_MODE_K; + break; + case 'S': + case 's': + printf(Setting Test_SE0_NAK mode); + mode = USB_TEST_MODE_SE0_NAK; + break; + case 'P': + case 'p': + printf(Setting Test_Packet mode); + mode = USB_TEST_MODE_PACKET; + break; + case 'F': + case 'f': + printf(Setting Test_Force_Enable mode); + mode = USB_TEST_MODE_FORCE_ENABLE; + break; + default: + printf(Unrecognized test mode: %s\nAvailable modes: + J, K, S[E0_NAK], P[acket], F[orce_Enable]\n, arg); + return 1; + } + + if (port) + printf( on downstream facing port %d...\n, port); + else + printf( on upstream facing port...\n); + + if (usb_control_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_FEATURE, + port ? USB_RT_PORT : USB_RECIP_DEVICE, + port ? USB_PORT_FEAT_TEST : USB_FEAT_TEST, + (mode 8) | port, + NULL, 0, USB_CNTL_TIMEOUT) == -1) { + printf(Error during SET_FEATURE.\n); + return 1; + } else { + printf(Test mode successfully set. Use 'usb start' + to return to normal operation.\n); + return 0; + } +} + /** * usb boot command intepreter. Derived from diskboot @@ -441,17 +517,9 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } return 0; } else { - int d; - - i = simple_strtoul(argv[2], NULL, 16); + i = simple_strtoul(argv[2], NULL, 10); printf(config for device %d\n, i); - for (d = 0; d USB_MAX_DEVICE; d++) { - dev = usb_get_dev_index(d); - if (dev == NULL) - break; - if (dev-devnum == i) - break; - } + dev = usb_find_device(i); if (dev == NULL) { printf(*** No device available ***\n); return 0; @@ -462,6 +530,18 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } return 0; } + if (strncmp(argv[1], test, 4) == 0) { + if (argc 5) + return CMD_RET_USAGE; + i = simple_strtoul(argv[2], NULL, 10); + dev = usb_find_device(i); + if (dev == NULL) { + printf(Device %d does not exist.\n, i); + return 1; + } + i = simple_strtoul(argv[3], NULL, 10
[U-Boot] [PATCH] usb: Add new command to set USB 2.0 port test modes
This patch adds a new 'usb test' command, that will set a port to a USB 2.0 test mode (see USB 2.0 spec 7.1.20). It supports all five test modes on both downstream hub ports and ordinary device's upstream ports. In addition, it supports EHCI root hub ports. The command is guarded by the CONFIG_CMD_USB_TEST ifdef (disabled by default). Signed-off-by: Julius Werner jwer...@chromium.org --- README | 1 + common/cmd_usb.c| 127 doc/README.usb | 1 + drivers/usb/host/ehci-hcd.c | 32 +++ include/usb_defs.h | 13 + 5 files changed, 140 insertions(+), 34 deletions(-) diff --git a/README b/README index 2352e38..996a938 100644 --- a/README +++ b/README @@ -892,6 +892,7 @@ The following options need to be configured: CONFIG_CMD_TIME * run command and report execution time (ARM specific) CONFIG_CMD_TIMER* access to the system tick timer CONFIG_CMD_USB * USB support + CONFIG_CMD_USB_TEST * USB 2.0 hardware test mode support CONFIG_CMD_CDP * Cisco Discover Protocol support CONFIG_CMD_MFSL * Microblaze FSL support diff --git a/common/cmd_usb.c b/common/cmd_usb.c index dacdc2d..38dbea6 100644 --- a/common/cmd_usb.c +++ b/common/cmd_usb.c @@ -269,6 +269,22 @@ static void usb_display_config(struct usb_device *dev) printf(\n); } +static struct usb_device *usb_find_device(int devnum) +{ + struct usb_device *dev; + int d; + + for (d = 0; d USB_MAX_DEVICE; d++) { + dev = usb_get_dev_index(d); + if (dev == NULL) + return NULL; + if (dev-devnum == devnum) + return dev; + } + + return NULL; +} + static inline char *portspeed(int speed) { if (speed == USB_SPEED_HIGH) @@ -348,6 +364,68 @@ static void usb_show_tree(struct usb_device *dev) usb_show_tree_graph(dev, preamble[0]); } +#ifdef CONFIG_CMD_USB_TEST +static int usb_test(struct usb_device *dev, int port, char* arg) +{ + int mode; + + if (port dev-maxchild) { + printf(Device is no hub or does not have %d ports.\n, port); + return 1; + } + + switch (arg[0]) { + case 'J': + case 'j': + printf(Setting Test_J mode); + mode = USB_TEST_MODE_J; + break; + case 'K': + case 'k': + printf(Setting Test_K mode); + mode = USB_TEST_MODE_K; + break; + case 'S': + case 's': + printf(Setting Test_SE0_NAK mode); + mode = USB_TEST_MODE_SE0_NAK; + break; + case 'P': + case 'p': + printf(Setting Test_Packet mode); + mode = USB_TEST_MODE_PACKET; + break; + case 'F': + case 'f': + printf(Setting Test_Force_Enable mode); + mode = USB_TEST_MODE_FORCE_ENABLE; + break; + default: + printf(Unrecognized test mode: %s\nAvailable modes: + J, K, S[E0_NAK], P[acket], F[orce_Enable]\n, arg); + return 1; + } + + if (port) + printf( on downstream facing port %d...\n, port); + else + printf( on upstream facing port...\n); + + if (usb_control_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_FEATURE, + port ? USB_RT_PORT : USB_RECIP_DEVICE, + port ? USB_PORT_FEAT_TEST : USB_FEAT_TEST, + (mode 8) | port, + NULL, 0, USB_CNTL_TIMEOUT) == -1) { + printf(Error during SET_FEATURE.\n); + return 1; + } else { + printf(Test mode successfully set. Use 'usb start' + to return to normal operation.\n); + return 0; + } +} +#endif /* CONFIG_CMD_USB_TEST */ + /** * usb boot command intepreter. Derived from diskboot @@ -441,17 +519,9 @@ static int do_usb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } return 0; } else { - int d; - - i = simple_strtoul(argv[2], NULL, 16); + i = simple_strtoul(argv[2], NULL, 10); printf(config for device %d\n, i); - for (d = 0; d USB_MAX_DEVICE; d++) { - dev = usb_get_dev_index(d); - if (dev == NULL) - break; - if (dev-devnum == i) - break