On Wed, 6 Mar 2013 18:08:21 -0800 Simon Glass sjg@chromium.org wrote:
On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips kim.phillips@freescale.com wrote:
On Tue, 5 Mar 2013 22:22:09 -0800 Simon Glass sjg@chromium.org wrote:
On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips kim.phillips@freescale.com wrote:
On Tue, 5 Mar 2013 17:51:00 -0800 Simon Glass sjg@chromium.org wrote:
> Changes sice v3: > - Changed command names to lower case in algo struct. > - Added generic ace_sha config.
I wouldn't call "ace" a generic name - crypto units other than ACE should be able to use this code.
I don't really understand this comment. A new CONFIG has been added, and this is specific to that unit. Are you suggesting that it be
right, and that's the problem - it needn't be specific to that unit.
Really? I think here we have a patch for an ACE unit, and currently the only implementation is in an Exynos chip. It can easily be
so make the ace_sha.o build depend on whichever level of chip config applies - CONFIG_S5P, CONFIG_EXYNOS5, or CONFIG_SMDK5250. No need for a new define specifically for ACE, right?
No, the ACE may appear in multiple chips, and anyway we may want to enable it or disable it. Drivers tend to have their own configs since some boards want to use (for example) USB, crypto, mmc, and some don't.
ok, if you really need the ACE define, restrict it to platform code and the driver, but not common code.
extended later when someone else has one.
maybe, but we can try to avoid people copying existing code patterns, i.e. polluting common code when adding crypto routines for their h/w which are basically the same function declarations but with different names.
Are you referring to adding code in into the hash algorithm table in hash.c? I specifically designed it so that people could add their
yes.
algorithms in there. Once we have device model in place then I'm sure we can do better, but for now that's the U-Boot way.
the problem is there are only two algorithms for all - the accelerated, and the non-. Presumably we get the acceleration for free, so we always will prefer to use the accelerated version, and if it doesn't work, then the core s/w implementation. The common/hash.c infrastructure currently doesn't support that: it assumes the existence of multiple heterogeneous crypto units will exist on a single u-boot instance, each used depending on its priority, which is not the case.
The primary objective here is to not add h/w vendor dependencies in common areas when they can easily be avoided.
Please can you point specifically to the lines of code you are wanting changed?
common/hash.c and include/ace_sha.h should have all occurrences of 'ace' and 'ACE' replaced with something like 'hw' or 'accel'...including the ace_sha.h filename itself.
Since it's probably common enough, the 0-length handler could move into the common code, and be enabled iff the hw-accelerated config is set.
Also, can we try to leave the existing void return type for the hash functions for the rest of u-boot. What do you think about changing common/hash.c to do the s/w fallback if h/w failed instead of in the driver(s)?
Something like CONFIG_HW_SHA{1,256} ought to do it.
But I don't think crypto units other than ACE will use the code in this patch - it is intended to implement ACE support, and put it ahead of software support in terms of priority.
the same priority that any h/w accelerated device would get - higher than that of software crypto.
Another question for Akshay wrt the timeout (since I never got a reply re: documentation): how can the h/w fault?
[...]
Regards, Simon
Kim
you're doing it again :)
Kim