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. > >> 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
