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

login
register
mail settings
Submitter Akshay Saraswat
Date March 5, 2013, 1:19 p.m.
Message ID <1362489600-20991-5-git-send-email-akshay.s@samsung.com>
Download mbox | patch
Permalink /patch/225021/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Akshay Saraswat - March 5, 2013, 1:19 p.m.
ACE H/W acceleration support is added to hash
which can be used to test SHA 256 hash algorithm.

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.

Signed-off-by: ARUN MANKUZHI <arun.m@samsung.com>
Signed-off-by: Akshay Saraswat <akshay.s@samsung.com>
---
Changes since v1:
	- Added sha256 support to "hash" command instead of new sha256 command.

Changes sice v2:
	- Added new nodes for SHA1 and SHA256 in struct hash_algo for the case when ACE is enabled.
	- Added new declaration for function pointer hash_func_ws with different return type.

Changes sice v3:
	- Changed command names to lower case in algo struct.
	- Added generic ace_sha config.

 common/hash.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Kim Phillips - March 5, 2013, 10:43 p.m.
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?

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?

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

Kim
Simon Glass - March 6, 2013, 1:46 a.m.
Hi Akshay,

On Tue, Mar 5, 2013 at 5:19 AM, Akshay Saraswat <akshay.s@samsung.com> wrote:
> ACE H/W acceleration support is added to hash
> which can be used to test SHA 256 hash algorithm.
>
> 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.
>
> Signed-off-by: ARUN MANKUZHI <arun.m@samsung.com>
> Signed-off-by: Akshay Saraswat <akshay.s@samsung.com>
> ---
> Changes since v1:
>         - Added sha256 support to "hash" command instead of new sha256 command.
>
> Changes sice v2:
>         - Added new nodes for SHA1 and SHA256 in struct hash_algo for the case when ACE is enabled.
>         - Added new declaration for function pointer hash_func_ws with different return type.
>
> Changes sice v3:
>         - Changed command names to lower case in algo struct.
>         - Added generic ace_sha config.
>
>  common/hash.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/common/hash.c b/common/hash.c
> index e3a6e43..10da26d 100644
> --- a/common/hash.c
> +++ b/common/hash.c
> @@ -28,12 +28,26 @@
>  #include <hash.h>
>  #include <sha1.h>
>  #include <sha256.h>
> +#include <ace_sha.h>
>
>  /*
>   * These are the hash algorithms we support. Chips which support accelerated
>   * crypto could perhaps add named version of these algorithms here.
>   */
>  static struct hash_algo hash_algo[] = {
> +#ifdef CONFIG_ACE_SHA
> +       {
> +               "sha1",
> +               SHA1_SUM_LEN,
> +               ace_sha_hash_digest,
> +               ACE_SHA_TYPE_SHA1,

This should be CHUNKSZ_SHA1 I think. You can't reuse this field for
anything you want :-)

> +       }, {
> +               "sha256",
> +               SHA256_SUM_LEN,
> +               ace_sha_hash_digest,
> +               ACE_SHA_TYPE_SHA256,

Similar here.

> +       },
> +#endif
>  #ifdef CONFIG_SHA1
>         {
>                 "SHA1",
> --
> 1.8.0
>

Regards,
Simon
Simon Glass - March 6, 2013, 1:51 a.m.
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.

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

>
>> 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
CONFIG_EXYNOS_ACE_SHA? Will the ACE unit not appear on any other SOC?

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.

Regards,
Simon

>
> Kim
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Kim Phillips - March 6, 2013, 5:04 a.m.
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 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.

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

Kim

> Regards,
> Simon
> 
> >
> > Kim
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot

oh, and please stop full-quoting - I'm tired of looking at my
scrollbar go by with no inline content.

Thanks,

Kim
Simon Glass - March 6, 2013, 6:22 a.m.
Hi Kim,

On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> On Tue, 5 Mar 2013 17:51:00 -0800
> Simon Glass <sjg@chromium.org> wrote:
>
[snip for Kim]

>> >> 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
extended later when someone else has one.

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

>
> 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?
>
> Kim
>
> oh, and please stop full-quoting - I'm tired of looking at my
> scrollbar go by with no inline content.

I will try harder. You could look at trying another mailer :-)

>
> Thanks,
>
> Kim
>

Regards,
Simon
Kim Phillips - March 7, 2013, 1:22 a.m.
On Tue, 5 Mar 2013 22:22:09 -0800
Simon Glass <sjg@chromium.org> wrote:

> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> > On Tue, 5 Mar 2013 17:51:00 -0800
> > Simon Glass <sjg@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?

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

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

> > 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?
> >
> > Kim
> >
> > oh, and please stop full-quoting - I'm tired of looking at my
> > scrollbar go by with no inline content.
> 
> I will try harder. You could look at trying another mailer :-)

Thanks, I appreciate it.  Gmail?  just more clicking, no?

Kim
Simon Glass - March 7, 2013, 2:08 a.m.
Hi Kim,

On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> On Tue, 5 Mar 2013 22:22:09 -0800
> Simon Glass <sjg@chromium.org> wrote:
>
>> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>> > On Tue, 5 Mar 2013 17:51:00 -0800
>> > Simon Glass <sjg@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
>
Kim Phillips - March 8, 2013, 12:25 a.m.
On Wed, 6 Mar 2013 18:08:21 -0800
Simon Glass <sjg@chromium.org> wrote:

> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> > On Tue, 5 Mar 2013 22:22:09 -0800
> > Simon Glass <sjg@chromium.org> wrote:
> >
> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> >> > On Tue, 5 Mar 2013 17:51:00 -0800
> >> > Simon Glass <sjg@chromium.org> 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
Simon Glass - March 8, 2013, 1:05 a.m.
Hi Kim,

On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> On Wed, 6 Mar 2013 18:08:21 -0800
> Simon Glass <sjg@chromium.org> wrote:
>
>> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>> > On Tue, 5 Mar 2013 22:22:09 -0800
>> > Simon Glass <sjg@chromium.org> wrote:
>> >
>> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>> >> > On Tue, 5 Mar 2013 17:51:00 -0800
>> >> > Simon Glass <sjg@chromium.org> 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

[...]
Kim Phillips - March 8, 2013, 2:18 a.m.
On Thu, 7 Mar 2013 17:05:15 -0800
Simon Glass <sjg@chromium.org> wrote:

> On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> > On Wed, 6 Mar 2013 18:08:21 -0800
> > Simon Glass <sjg@chromium.org> wrote:
> >
> >> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> >> > On Tue, 5 Mar 2013 22:22:09 -0800
> >> > Simon Glass <sjg@chromium.org> wrote:
> >> >
> >> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> >> >> > On Tue, 5 Mar 2013 17:51:00 -0800
> >> >> > Simon Glass <sjg@chromium.org> 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.

ok, well this is the first time a new alternate algorithm
implementation is being posted, and it's bringing up a flaw in the
design - no vendor-specific stuff in common areas.

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

well right now as it stands the 2nd h/w accelerator would have to
duplicate hash entry point function signatures, just changing the
ACE in the name to that of its h/w, and then spin on a tough
decision: what priority does my h/w have vs. Samsung's ACE??

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

they'll come, eventually I hope.  Other than the driver internals,
the only thing different from the ACE functional capacity provided
in this patchseries for the analogous Freescale parts is that the
hash infrastructure would need to be adapted for runtime detection
of the crypto unit's existence.

But let's not use this as an excuse to let things slide, please.

this is new infrastructure, right?  It's common to make mistakes
without seeing the entire picture, and this patchset represents the
next piece.

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

sorry, I disagree:  This is a brand new driver class that's being
added, and the design enforces adding h/w vendor specific symbols in
common code.  This tells me the design is broken.  Net doesn't do
this - why should crypto get away with it?  Plus, as I explain
above, it's not that hard to fix (well, for now at least): just
change the common ace parts to have a generic name.

Kim
Simon Glass - March 8, 2013, 3:11 a.m.
Hi Kim,

On Thu, Mar 7, 2013 at 6:18 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> On Thu, 7 Mar 2013 17:05:15 -0800
> Simon Glass <sjg@chromium.org> wrote:
>
>> On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>> > On Wed, 6 Mar 2013 18:08:21 -0800
>> > Simon Glass <sjg@chromium.org> wrote:
>> >
>> >> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>> >> > On Tue, 5 Mar 2013 22:22:09 -0800
>> >> > Simon Glass <sjg@chromium.org> wrote:
>> >> >
>> >> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
>> >> >> > On Tue, 5 Mar 2013 17:51:00 -0800
>> >> >> > Simon Glass <sjg@chromium.org> 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.
>
> ok, well this is the first time a new alternate algorithm
> implementation is being posted, and it's bringing up a flaw in the
> design - no vendor-specific stuff in common areas.

OK so let's look at adding the hash_register() idea. But not in this
series. That should come later in a revision of the hash.c
infrastructure, since it needs to adjust sha1, sha255 and crc32.

>
>> > 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.
>
> well right now as it stands the 2nd h/w accelerator would have to
> duplicate hash entry point function signatures, just changing the
> ACE in the name to that of its h/w, and then spin on a tough
> decision: what priority does my h/w have vs. Samsung's ACE??

I thought you said that only one h/w accelerator would be used on a board?

>
>> 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.
>
> they'll come, eventually I hope.  Other than the driver internals,
> the only thing different from the ACE functional capacity provided
> in this patchseries for the analogous Freescale parts is that the
> hash infrastructure would need to be adapted for runtime detection
> of the crypto unit's existence.
>
> But let's not use this as an excuse to let things slide, please.
>
> this is new infrastructure, right?  It's common to make mistakes
> without seeing the entire picture, and this patchset represents the
> next piece.

I would prefer to invent new infrastructure when we have two users,
not one, otherwise we run the risk of over-engineering. I already hit
a code size snag with crc32 integration. This is just the first user
and there is not yet a clear picture of what other users will want. If
you are saying that Freescale will need things to be done a particular
way, please post the patches and we can take a look at something that
fits both SOCs.

>
>> >> > 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.
>
> sorry, I disagree:  This is a brand new driver class that's being
> added, and the design enforces adding h/w vendor specific symbols in
> common code.  This tells me the design is broken.  Net doesn't do
> this - why should crypto get away with it?  Plus, as I explain
> above, it's not that hard to fix (well, for now at least): just
> change the common ace parts to have a generic name.

I'm willing to come up with patches to move to a hash_register() type
implementation which will allow us to address this shortcoming - but
people will need to understand that it will increase code size a
little more. I was intending to wait for device model, but I don't
mind doing something in the interim. Then I will happily adjust this
driver to use that new mechanism. In the meantime, it would help if
you could post some Freescale patches for us to compare / review.

Regards.
Simon
Kim Phillips - March 12, 2013, 12:44 a.m.
On Thu, 7 Mar 2013 19:11:16 -0800
Simon Glass <sjg@chromium.org> wrote:

> Hi Kim,
> 
> On Thu, Mar 7, 2013 at 6:18 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> > On Thu, 7 Mar 2013 17:05:15 -0800
> > Simon Glass <sjg@chromium.org> wrote:
> >
> >> On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> >> > On Wed, 6 Mar 2013 18:08:21 -0800
> >> > Simon Glass <sjg@chromium.org> wrote:
> >> >
> >> >> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> >> >> > On Tue, 5 Mar 2013 22:22:09 -0800
> >> >> > Simon Glass <sjg@chromium.org> wrote:
> >> >> >
> >> >> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <kim.phillips@freescale.com> wrote:
> >> >> >> > On Tue, 5 Mar 2013 17:51:00 -0800
> >> >> >> > Simon Glass <sjg@chromium.org> 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.
> >
> > ok, well this is the first time a new alternate algorithm
> > implementation is being posted, and it's bringing up a flaw in the
> > design - no vendor-specific stuff in common areas.
> 
> OK so let's look at adding the hash_register() idea. But not in this
> series. That should come later in a revision of the hash.c
> infrastructure, since it needs to adjust sha1, sha255 and crc32.

I don't understand: why not s/ace/hw/g in common/ and include/ on
this patchseries, then move straight to the device model at some
later point?  It's a compromise, but it works fine for the time
being - other vendors can add their hash support without having to
touch common code, code size is not affected, etc.

> >> > 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.
> >
> > well right now as it stands the 2nd h/w accelerator would have to
> > duplicate hash entry point function signatures, just changing the
> > ACE in the name to that of its h/w, and then spin on a tough
> > decision: what priority does my h/w have vs. Samsung's ACE??
> 
> I thought you said that only one h/w accelerator would be used on a board?

yes, I was trying to be funny, but as usual, it didn't work.

> >> 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.
> >
> > they'll come, eventually I hope.  Other than the driver internals,
> > the only thing different from the ACE functional capacity provided
> > in this patchseries for the analogous Freescale parts is that the
> > hash infrastructure would need to be adapted for runtime detection
> > of the crypto unit's existence.
> >
> > But let's not use this as an excuse to let things slide, please.
> >
> > this is new infrastructure, right?  It's common to make mistakes
> > without seeing the entire picture, and this patchset represents the
> > next piece.
> 
> I would prefer to invent new infrastructure when we have two users,
> not one, otherwise we run the risk of over-engineering. I already hit
> a code size snag with crc32 integration. This is just the first user
> and there is not yet a clear picture of what other users will want. If
> you are saying that Freescale will need things to be done a particular
> way, please post the patches and we can take a look at something that
> fits both SOCs.

I'm saying that - at least with the current patchseries as-is -
other crypto vendors would have to touch common code.

Freescale would need runtime device detection, but that's completely
orthogonal to this patchseries.

> >> >> > 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.
> >
> > sorry, I disagree:  This is a brand new driver class that's being
> > added, and the design enforces adding h/w vendor specific symbols in
> > common code.  This tells me the design is broken.  Net doesn't do
> > this - why should crypto get away with it?  Plus, as I explain
> > above, it's not that hard to fix (well, for now at least): just
> > change the common ace parts to have a generic name.
> 
> I'm willing to come up with patches to move to a hash_register() type
> implementation which will allow us to address this shortcoming - but
> people will need to understand that it will increase code size a
> little more. I was intending to wait for device model, but I don't
> mind doing something in the interim. Then I will happily adjust this
> driver to use that new mechanism. In the meantime, it would help if
> you could post some Freescale patches for us to compare / review.

see above.

Kim
Simon Glass - March 12, 2013, 12:53 a.m.
Hi Kim,

On Mon, Mar 11, 2013 at 5:44 PM, Kim Phillips <kim.phillips@freescale.com>wrote:

> On Thu, 7 Mar 2013 19:11:16 -0800
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Kim,
> >
> > On Thu, Mar 7, 2013 at 6:18 PM, Kim Phillips <kim.phillips@freescale.com>
> wrote:
> > > On Thu, 7 Mar 2013 17:05:15 -0800
> > > Simon Glass <sjg@chromium.org> wrote:
> > >
> > >> On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <
> kim.phillips@freescale.com> wrote:
> > >> > On Wed, 6 Mar 2013 18:08:21 -0800
> > >> > Simon Glass <sjg@chromium.org> wrote:
> > >> >
> > >> >> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips <
> kim.phillips@freescale.com> wrote:
> > >> >> > On Tue, 5 Mar 2013 22:22:09 -0800
> > >> >> > Simon Glass <sjg@chromium.org> wrote:
> > >> >> >
> > >> >> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips <
> kim.phillips@freescale.com> wrote:
> > >> >> >> > On Tue, 5 Mar 2013 17:51:00 -0800
> > >> >> >> > Simon Glass <sjg@chromium.org> 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.
> > >
> > > ok, well this is the first time a new alternate algorithm
> > > implementation is being posted, and it's bringing up a flaw in the
> > > design - no vendor-specific stuff in common areas.
> >
> > OK so let's look at adding the hash_register() idea. But not in this
> > series. That should come later in a revision of the hash.c
> > infrastructure, since it needs to adjust sha1, sha255 and crc32.
>
> I don't understand: why not s/ace/hw/g in common/ and include/ on
> this patchseries, then move straight to the device model at some
> later point?  It's a compromise, but it works fine for the time
> being - other vendors can add their hash support without having to
> touch common code, code size is not affected, etc.
>

Fine with me. The effect is the same - this is just a rename. Should not be
done in the ace.h file though, only in the naming of the functions called
from hash.c, right?


>
> > >> > 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.
> > >
> > > well right now as it stands the 2nd h/w accelerator would have to
> > > duplicate hash entry point function signatures, just changing the
> > > ACE in the name to that of its h/w, and then spin on a tough
> > > decision: what priority does my h/w have vs. Samsung's ACE??
> >
> > I thought you said that only one h/w accelerator would be used on a
> board?
>
> yes, I was trying to be funny, but as usual, it didn't work.
>

OK, sorry I missed it.


>
> > >> 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.
> > >
> > > they'll come, eventually I hope.  Other than the driver internals,
> > > the only thing different from the ACE functional capacity provided
> > > in this patchseries for the analogous Freescale parts is that the
> > > hash infrastructure would need to be adapted for runtime detection
> > > of the crypto unit's existence.
> > >
> > > But let's not use this as an excuse to let things slide, please.
> > >
> > > this is new infrastructure, right?  It's common to make mistakes
> > > without seeing the entire picture, and this patchset represents the
> > > next piece.
> >
> > I would prefer to invent new infrastructure when we have two users,
> > not one, otherwise we run the risk of over-engineering. I already hit
> > a code size snag with crc32 integration. This is just the first user
> > and there is not yet a clear picture of what other users will want. If
> > you are saying that Freescale will need things to be done a particular
> > way, please post the patches and we can take a look at something that
> > fits both SOCs.
>
> I'm saying that - at least with the current patchseries as-is -
> other crypto vendors would have to touch common code.
>

Yes.


>
> Freescale would need runtime device detection, but that's completely
> orthogonal to this patchseries.
>

You can use device tree - CONFIG_OF_CONTROL - in fact ACE could move to
that also.

[snip]

Regards,
Simon
Kim Phillips - March 12, 2013, 7:32 p.m.
On Mon, 11 Mar 2013 17:53:37 -0700
Simon Glass <sjg@chromium.org> wrote:

> On Mon, Mar 11, 2013 at 5:44 PM, Kim Phillips <kim.phillips@freescale.com>wrote:
> > On Thu, 7 Mar 2013 19:11:16 -0800
> > Simon Glass <sjg@chromium.org> wrote:
> > > OK so let's look at adding the hash_register() idea. But not in this
> > > series. That should come later in a revision of the hash.c
> > > infrastructure, since it needs to adjust sha1, sha255 and crc32.
> >
> > I don't understand: why not s/ace/hw/g in common/ and include/ on
> > this patchseries, then move straight to the device model at some
> > later point?  It's a compromise, but it works fine for the time
> > being - other vendors can add their hash support without having to
> > touch common code, code size is not affected, etc.
> 
> Fine with me. The effect is the same - this is just a rename. Should not be
> done in the ace.h file though, only in the naming of the functions called
> from hash.c, right?

the ace_sha_hash_digest() declaration should be removed from
ace_sha.h (it's only needed by the driver, which is ok without it
being there).  ACE_SHA_TYPE_SHA* definitions belong in the driver
too - they're ACE h/w-specific.  Rename the filename ace_sha.h to
hw_sha.h, and all remaining ACE references contained in it.

If the 'hw_' nomenclature is undesired, other possibilities are
'accel_', 'hw_accel_', 'alt_'...

Kim
Simon Glass - March 12, 2013, 11:40 p.m.
Hi Kim,

On Tue, Mar 12, 2013 at 12:32 PM, Kim Phillips
<kim.phillips@freescale.com> wrote:
> On Mon, 11 Mar 2013 17:53:37 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
>> On Mon, Mar 11, 2013 at 5:44 PM, Kim Phillips <kim.phillips@freescale.com>wrote:
>> > On Thu, 7 Mar 2013 19:11:16 -0800
>> > Simon Glass <sjg@chromium.org> wrote:
>> > > OK so let's look at adding the hash_register() idea. But not in this
>> > > series. That should come later in a revision of the hash.c
>> > > infrastructure, since it needs to adjust sha1, sha255 and crc32.
>> >
>> > I don't understand: why not s/ace/hw/g in common/ and include/ on
>> > this patchseries, then move straight to the device model at some
>> > later point?  It's a compromise, but it works fine for the time
>> > being - other vendors can add their hash support without having to
>> > touch common code, code size is not affected, etc.
>>
>> Fine with me. The effect is the same - this is just a rename. Should not be
>> done in the ace.h file though, only in the naming of the functions called
>> from hash.c, right?
>
> the ace_sha_hash_digest() declaration should be removed from
> ace_sha.h (it's only needed by the driver, which is ok without it
> being there).  ACE_SHA_TYPE_SHA* definitions belong in the driver
> too - they're ACE h/w-specific.  Rename the filename ace_sha.h to
> hw_sha.h, and all remaining ACE references contained in it.

Maybe I misunderstand - are you saying that that all the ACE symbols
in the driver should be renamed to hw? That doesn't make a lot of
sense to me - why is this needed?
>
> If the 'hw_' nomenclature is undesired, other possibilities are
> 'accel_', 'hw_accel_', 'alt_'...
>
> Kim
>

Regards,
Simon
Kim Phillips - March 13, 2013, 12:03 a.m.
On Tue, 12 Mar 2013 16:40:38 -0700
Simon Glass <sjg@chromium.org> wrote:

> Hi Kim,
> 
> On Tue, Mar 12, 2013 at 12:32 PM, Kim Phillips
> <kim.phillips@freescale.com> wrote:
> > On Mon, 11 Mar 2013 17:53:37 -0700
> > Simon Glass <sjg@chromium.org> wrote:
> >
> >> On Mon, Mar 11, 2013 at 5:44 PM, Kim Phillips <kim.phillips@freescale.com>wrote:
> >> > On Thu, 7 Mar 2013 19:11:16 -0800
> >> > Simon Glass <sjg@chromium.org> wrote:
> >> > > OK so let's look at adding the hash_register() idea. But not in this
> >> > > series. That should come later in a revision of the hash.c
> >> > > infrastructure, since it needs to adjust sha1, sha255 and crc32.
> >> >
> >> > I don't understand: why not s/ace/hw/g in common/ and include/ on
> >> > this patchseries, then move straight to the device model at some
> >> > later point?  It's a compromise, but it works fine for the time
> >> > being - other vendors can add their hash support without having to
> >> > touch common code, code size is not affected, etc.
> >>
> >> Fine with me. The effect is the same - this is just a rename. Should not be
> >> done in the ace.h file though, only in the naming of the functions called
> >> from hash.c, right?
> >
> > the ace_sha_hash_digest() declaration should be removed from
> > ace_sha.h (it's only needed by the driver, which is ok without it
> > being there).  ACE_SHA_TYPE_SHA* definitions belong in the driver
> > too - they're ACE h/w-specific.  Rename the filename ace_sha.h to
> > hw_sha.h, and all remaining ACE references contained in it.
> 
> Maybe I misunderstand - are you saying that that all the ACE symbols
> in the driver should be renamed to hw? That doesn't make a lot of
> sense to me - why is this needed?

no, only the entry points to the driver should be renamed - they are
currently called ace_sha{1,256}().  The include/ace_sha.h file as it
is currently can be made completely h/w agnostic.

Kim
Simon Glass - March 13, 2013, 12:20 a.m.
Hi Kim,

On Tue, Mar 12, 2013 at 5:03 PM, Kim Phillips
<kim.phillips@freescale.com> wrote:
> On Tue, 12 Mar 2013 16:40:38 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Kim,
>>
>> On Tue, Mar 12, 2013 at 12:32 PM, Kim Phillips
>> <kim.phillips@freescale.com> wrote:
>> > On Mon, 11 Mar 2013 17:53:37 -0700
>> > Simon Glass <sjg@chromium.org> wrote:
>> >
>> >> On Mon, Mar 11, 2013 at 5:44 PM, Kim Phillips <kim.phillips@freescale.com>wrote:
>> >> > On Thu, 7 Mar 2013 19:11:16 -0800
>> >> > Simon Glass <sjg@chromium.org> wrote:
>> >> > > OK so let's look at adding the hash_register() idea. But not in this
>> >> > > series. That should come later in a revision of the hash.c
>> >> > > infrastructure, since it needs to adjust sha1, sha255 and crc32.
>> >> >
>> >> > I don't understand: why not s/ace/hw/g in common/ and include/ on
>> >> > this patchseries, then move straight to the device model at some
>> >> > later point?  It's a compromise, but it works fine for the time
>> >> > being - other vendors can add their hash support without having to
>> >> > touch common code, code size is not affected, etc.
>> >>
>> >> Fine with me. The effect is the same - this is just a rename. Should not be
>> >> done in the ace.h file though, only in the naming of the functions called
>> >> from hash.c, right?
>> >
>> > the ace_sha_hash_digest() declaration should be removed from
>> > ace_sha.h (it's only needed by the driver, which is ok without it
>> > being there).  ACE_SHA_TYPE_SHA* definitions belong in the driver
>> > too - they're ACE h/w-specific.  Rename the filename ace_sha.h to
>> > hw_sha.h, and all remaining ACE references contained in it.
>>
>> Maybe I misunderstand - are you saying that that all the ACE symbols
>> in the driver should be renamed to hw? That doesn't make a lot of
>> sense to me - why is this needed?
>
> no, only the entry points to the driver should be renamed - they are
> currently called ace_sha{1,256}().  The include/ace_sha.h file as it
> is currently can be made completely h/w agnostic.

OK, that sounds better.

Regards,
Simon

>
> Kim
>

Patch

diff --git a/common/hash.c b/common/hash.c
index e3a6e43..10da26d 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -28,12 +28,26 @@ 
 #include <hash.h>
 #include <sha1.h>
 #include <sha256.h>
+#include <ace_sha.h>
 
 /*
  * These are the hash algorithms we support. Chips which support accelerated
  * crypto could perhaps add named version of these algorithms here.
  */
 static struct hash_algo hash_algo[] = {
+#ifdef CONFIG_ACE_SHA
+	{
+		"sha1",
+		SHA1_SUM_LEN,
+		ace_sha_hash_digest,
+		ACE_SHA_TYPE_SHA1,
+	}, {
+		"sha256",
+		SHA256_SUM_LEN,
+		ace_sha_hash_digest,
+		ACE_SHA_TYPE_SHA256,
+	},
+#endif
 #ifdef CONFIG_SHA1
 	{
 		"SHA1",