Hi Kim, On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <[email protected]> wrote: > On Wed, 6 Mar 2013 18:08:21 -0800 > Simon Glass <[email protected]> wrote: > >> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <[email protected]> >> wrote: >> > On Tue, 5 Mar 2013 22:22:09 -0800 >> > Simon Glass <[email protected]> wrote: >> > >> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <[email protected]> >> >> wrote: >> >> > On Tue, 5 Mar 2013 17:51:00 -0800 >> >> > Simon Glass <[email protected]> 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 [...] _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
