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

[Akshay Saraswat]

Hi Kim,

>On Tue, 5 Mar 2013 17:51:00 -0800
>Simon Glass <sjg at chromium.org> wrote:
>
>> Hi Kim,
>> 
>> On Tue, Mar 5, 2013 at 2:43 PM, Kim Phillips <kim.phillips at freescale.com> wrote:
>> > On Tue, 5 Mar 2013 08:19:59 -0500
>> > Akshay Saraswat <akshay.s at samsung.com> wrote:
>> >
>> >> Tested with command "hash sha256 0x40008000 0x2B 0x40009000".
>> >> Used mm and md to write a standard string to memory location
>> >> 0x40008000 and ran the above command to verify the output.
>> >
>> > patches 1,2,4,5 all contain this "tested with" text, yet obviously
>> > were not.  It also indicates that a data buffer that's > 8MB was not
>> > tested?
>> 
>> Would be useful to test a larger transfer.
>
>esp. since it's now advertised.

I have removed "tested with" in the new set of patches. And I tested those patches with that command
before mailing for review. I have tested them for various sizes this time which includes 8 MB as well.
I have shared benchmark results in another mail.

>
>> > I also asked about speed relative to software running on the core
>> > and didn't get a response.  Software should be faster up to
>> > a certain buffer size: what is that threshold?
>> 
>> You can fairly easily do that by temporarily modifying your patch to
>> use "acesha1" for the name, enable CONFIG_CMD_TIME, then you can
>> compare the two with:
>> 
>> time hash sha1 <addr> <size>
>> time hash acesha1 <addr> <size>
>
>sure, but I don't have an ACE - that's why I asked.
>
>> >> 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.
>
>> 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.
>
>Something like CONFIG_HW_SHA{1,256} ought to do it.
>

These instances of struct algo were created specifically for ace because
we need function name different for ace to distinguish it from others.
Hence, config name includes "ace" as well.

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

Regarding documentation, I have replied to that mail itself. Sorry for the delay.

Since it is obvious that in case of h/w fault all readl and writel's would result
incorrectly and since we know that status register should change it's value quickly,
we have a good option to depend upon 100 ms as the time more than enough for wait.
And I tried to handle our concern over frequency change and early timeout
with the proportional timeout calculation in the new patch. Please have a look.

>
>Kim

Regards,
Akshay Saraswat