| Message ID | 1362489600-20991-5-git-send-email-akshay.s@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
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
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
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
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
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
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
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 >
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
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 [...]
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
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
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
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
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
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
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
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 >
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",