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