Hi Kim,
On Mon, Mar 11, 2013 at 5:44 PM, Kim Phillips kim.phillips@freescale.comwrote:
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