On Thu, 7 Mar 2013 19:11:16 -0800 Simon Glass <s...@chromium.org> wrote:
> Hi Kim, > > On Thu, Mar 7, 2013 at 6:18 PM, Kim Phillips <kim.phill...@freescale.com> > wrote: > > On Thu, 7 Mar 2013 17:05:15 -0800 > > Simon Glass <s...@chromium.org> wrote: > > > >> On Thu, Mar 7, 2013 at 4:25 PM, Kim Phillips <kim.phill...@freescale.com> > >> wrote: > >> > On Wed, 6 Mar 2013 18:08:21 -0800 > >> > Simon Glass <s...@chromium.org> wrote: > >> > > >> >> On Wed, Mar 6, 2013 at 5:22 PM, Kim Phillips > >> >> <kim.phill...@freescale.com> wrote: > >> >> > On Tue, 5 Mar 2013 22:22:09 -0800 > >> >> > Simon Glass <s...@chromium.org> wrote: > >> >> > > >> >> >> On Tue, Mar 5, 2013 at 9:04 PM, Kim Phillips > >> >> >> <kim.phill...@freescale.com> wrote: > >> >> >> > On Tue, 5 Mar 2013 17:51:00 -0800 > >> >> >> > Simon Glass <s...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
