[U-Boot] [PATCH 4/5 v4] gen: Add ACE acceleration to hash

[Simon Glass]

Hi Kim,

On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <kim.phillips at freescale.com> wrote:
> On Tue, 5 Mar 2013 22:22:09 -0800
> Simon Glass <sjg at chromium.org> wrote:
>
>> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips at freescale.com> wrote:
>> > On Tue, 5 Mar 2013 17:51:00 -0800
>> > Simon Glass <sjg at chromium.org> wrote:
>> >
>> [snip for Kim]
>
> and others too, I hope.
>
>> >> >> 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.

>
>> 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
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.

>
>> >> CONFIG_EXYNOS_ACE_SHA? Will the ACE unit not appear on any other SOC?
>> >
>> > my point is other SoCs can use the same entry in the array - there's
>> > nothing h/w-vendor or model-specific about it.
>>
>> OK, know you of such an SOC?
>
> I'm not familiar with Samsung crypto products, and I can't become
> familiar either, judging by the state of their public
> documentation (if a company doesn't make their crypto unit
> documentation public, that only has to mean something's insecure -
> and not just through obscurity :).
>
> A large majority of Freescale's PowerQUICC & QorIQ chips
> have crypto units.  A lot of them have crypto as an option, so
> discovery has to be done at runtime (but we can add that
> later).
>
> 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?

>
>> > 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
>