Re: [PATCH 01/14] bloblist: Update the tag numbering

2023-07-28 Thread Julius Werner
> 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

2023-07-26 Thread Julius Werner
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()

2023-07-26 Thread Julius Werner
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

2023-07-26 Thread Julius Werner
> -   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

2023-07-26 Thread Julius Werner
> /* 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

2023-07-26 Thread Julius Werner
> 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.

2022-12-06 Thread Julius Werner
> 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.

2022-12-05 Thread Julius Werner
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.

2022-11-29 Thread Julius Werner
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.

2022-11-29 Thread Julius Werner
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.

2022-07-11 Thread Julius Werner
> 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.

2022-07-08 Thread Julius Werner
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

2021-09-21 Thread Julius Werner
> 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

2021-07-15 Thread Julius Werner
> - 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

2021-07-09 Thread Julius Werner
> - "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

2021-07-08 Thread Julius Werner
> 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

2021-07-08 Thread Julius Werner
>
> 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

2021-04-08 Thread Julius Werner
> 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

2021-03-29 Thread Julius Werner
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

2021-03-24 Thread Julius Werner
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

2020-12-02 Thread Julius Werner
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

2020-06-05 Thread Julius Werner
> 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

2019-08-02 Thread Julius Werner
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

2019-08-02 Thread Julius Werner
> 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

2019-08-01 Thread Julius Werner
> 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

2019-08-01 Thread Julius Werner
> 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())

2019-07-24 Thread Julius Werner
> 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)

2019-07-24 Thread 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 
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

2019-07-24 Thread Julius Werner
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

2019-07-24 Thread Julius Werner
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())

2019-07-24 Thread Julius Werner
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())

2019-07-12 Thread Julius Werner
> 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

2019-07-12 Thread Julius Werner
> 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

2019-07-12 Thread Julius Werner
> 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)

2019-07-12 Thread Julius Werner
> 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)

2019-07-11 Thread Julius Werner
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

2019-07-11 Thread Julius Werner
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)

2019-07-11 Thread 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 
---
 - 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())

2019-07-11 Thread Julius Werner
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

2019-07-11 Thread Julius Werner
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)

2019-06-24 Thread Julius Werner
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

2019-05-15 Thread Julius Werner
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

2019-05-15 Thread Julius Werner
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)

2019-05-15 Thread 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 
---
 - 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)

2019-05-15 Thread Julius Werner
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

2019-05-13 Thread Julius Werner
> 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)

2019-05-13 Thread Julius Werner
> 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)

2019-05-03 Thread Julius Werner
> 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

2019-05-03 Thread Julius Werner
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)

2019-05-03 Thread 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| 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

2019-05-03 Thread Julius Werner
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)

2019-04-30 Thread Julius Werner
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)

2019-04-29 Thread Julius Werner
> 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)

2019-04-18 Thread 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;
+   }
+
/*
 * 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

2019-04-18 Thread Julius Werner
> 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

2019-04-18 Thread Julius Werner
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

2019-04-18 Thread Julius Werner
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

2019-04-18 Thread Julius Werner
> 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

2019-04-18 Thread Julius Werner
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

2019-04-18 Thread Julius Werner
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

2019-04-18 Thread Julius Werner
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

2019-04-18 Thread Julius Werner
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

2019-03-27 Thread Julius Werner
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

2015-10-07 Thread Julius Werner
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

2015-10-07 Thread Julius Werner
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

2015-10-06 Thread Julius Werner
> 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

2015-10-03 Thread Julius Werner
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

2015-10-03 Thread Julius Werner
>
> 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

2015-09-26 Thread Julius Werner
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

2013-09-24 Thread Julius Werner
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

2013-09-23 Thread Julius Werner
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

2013-09-19 Thread Julius Werner
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

2013-09-18 Thread Julius Werner
 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

2013-09-17 Thread Julius Werner
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

2013-08-19 Thread Julius Werner
 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

2013-07-19 Thread Julius Werner
 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

2013-07-19 Thread Julius Werner
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

2013-07-18 Thread Julius Werner
 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

2013-07-18 Thread Julius Werner
 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

2013-07-17 Thread Julius Werner
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

2013-07-12 Thread Julius Werner
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

2013-05-13 Thread Julius Werner
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

2013-04-24 Thread Julius Werner
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

2013-04-23 Thread Julius Werner
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

2013-04-22 Thread Julius Werner
 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

2013-04-22 Thread Julius Werner
 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

2013-04-19 Thread Julius Werner
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

2013-04-19 Thread Julius Werner
 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

2013-04-19 Thread Julius Werner
 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

2013-04-19 Thread Julius Werner
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

2013-04-18 Thread Julius Werner
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

2013-02-28 Thread Julius Werner
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

2013-02-28 Thread Julius Werner
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

2013-02-20 Thread Julius Werner
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

2013-02-20 Thread Julius Werner
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

2013-02-20 Thread Julius Werner
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

2013-02-16 Thread Julius Werner
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