Hi Kim,
On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips kim.phillips@freescale.com wrote:
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.
That is in the design of the hash.c module. It is intended to permit insertion of different algorithm implementations. We could perhaps introduce a hash_register() function to deal with this, but that's the way it is at present.
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.
Fair enough, and that might be a good idea, but that is an issue for the hash infrastructure. It would be good to get a second hardware accelerator upstream so we can judge where to take this.
If you have one in a Freescale SOC can I please request that you send some patches upstream and we can then evaluate how best to arrange the code.
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.
To be honest I think we are getting ahead of ourselves. We are dealing here with a patch to enable hardware acceleration in one SOC, with a few lines of changes to common code, changing it in a way that fits nicely with how hash.c was designed.
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)?
Yes I was thinking about that - probably something for a follow-on patch though. I have just finished getting crc32 in, so will add it to the queue to think about. Patches welcome.
Regards, Simon
[...]