Re: [U-Boot] [PATCH 4/5 v4] gen: Add ACE acceleration to hash
Hi Kim,
On Tue, 5 Mar 2013 17:51:00 -0800 Simon Glass sjg@chromium.org wrote:
Hi Kim,
On Tue, Mar 5, 2013 at 2:43 PM, Kim Phillips kim.phillips@freescale.com wrote:
On Tue, 5 Mar 2013 08:19:59 -0500 Akshay Saraswat akshay.s@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
On Wed, 6 Mar 2013 10:29:46 -0500 Akshay Saraswat akshay.s@samsung.com wrote:
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.
thanks - the threshold looks good (although the two largest sizes looked a bit too close).
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.
no you don't, because no u-boot instance will contain support for others. SoC vendors don't put more than one crypto unit in their parts.
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.
I don't understand this - the question is whether the h/w can possibly experience an internal failure from submission time to result ready time.
Kim
participants (2)
-
Akshay Saraswat -
Kim Phillips