diff mbox

[00/15] OMAP SHAM & AES Crypto Updates

Message ID 20130108203853.GB1876@animalcreek.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Greer Jan. 8, 2013, 8:38 p.m. UTC 
On Sun, Dec 23, 2012 at 08:40:43AM +0000, Paul Walmsley wrote:
> Hi Mark,

Hi Paul.

> On Fri, 21 Dec 2012, Mark A. Greer wrote:
> 
> > From: "Mark A. Greer" <mgreer@animalcreek.com>
> > 
> > [This series supersedes the hwmod related patches sent in the
> >  "crypto: omap-sham updates" and "crypto: omap-aes updates"
> >  series a few weeks ago.]
> > 
> > This series adds hwmod support for the OMAP SHAM and AES
> > modules on OMAP2, OMAP3, and OMAP4/AM33XX SoCs.  It also
> > adds device tree info for those modules.
> 
> Thanks for working on this, this will get us much closer to being able to 
> convert the hwmod code into an OMAP bus.  I haven't looked closely at 
> these patches yet, but a few comments/questions:

> - The patch series causes AM3517/3505 to crash.  I'd guess this is due to 
> the SHAM/AES modules being initialized on those chips, but they probably 
> don't exist there.  Can you change the initialization for those on OMAP3 
> to only take place on OMAP34xx/36xx GP?  I guess you'd need to create new 
> lists for those in the hwmod init.

All am35xx GPs have the SHAM and AES modules except some very old ones.
I've been told that there should be very few of the "old" ones around
(I don't know how to differentiate them).  We're likely safe since the
SHAM & AES modules are not enabled in omap2plus_defconfig so nobody
should be enabling them on an am35xx unless they know that they have
the modules.  Do you agree?

The issue that you're likely running into is that 'CK_AM35XX' needs to be
added for aes2_ick & sha12_ick in cclock3xxx_data.c.   The following
patch should fix it (applies to my submitted/crypto/hwmod branch):



Please let me know if this patch works for you and, if it does, I'll respin
my patches to add those changes.

Thanks,

Mark
--
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Paul Walmsley Jan. 17, 2013, 7:13 p.m. UTC | #1 
Hi Mark,

I regret the delay,

On Tue, 8 Jan 2013, Mark A. Greer wrote:

> On Sun, Dec 23, 2012 at 08:40:43AM +0000, Paul Walmsley wrote:
>
> > - The patch series causes AM3517/3505 to crash.  I'd guess this is due to 
> > the SHAM/AES modules being initialized on those chips, but they probably 
> > don't exist there.  Can you change the initialization for those on OMAP3 
> > to only take place on OMAP34xx/36xx GP?  I guess you'd need to create new 
> > lists for those in the hwmod init.
> 
> All am35xx GPs have the SHAM and AES modules except some very old ones.
> I've been told that there should be very few of the "old" ones around
> (I don't know how to differentiate them).  We're likely safe since the
> SHAM & AES modules are not enabled in omap2plus_defconfig so nobody
> should be enabling them on an am35xx unless they know that they have
> the modules.  Do you agree?

Those will presumably only enable or disable the device drivers.  The 
hwmod code will probably still try to write to those IP blocks if they are 
listed as present in the hwmod data, during the initial reset-and-idle 
phase.

What do you think about adding an am35xx_es11plus_hwmod_ocp_ifs[] array to 
omap_hwmod_3xxx_data.c for these secure hwmods?  That carries the implicit 
and possibly wrong assumption that it's likely to be ES1.0 devices that 
are missing the SHAM/AES, but it seems unlikely that TI would have 
multiple silicon revs running around claiming to be ES1.1?  Or maybe I'm 
just being naïve.

> The issue that you're likely running into is that 'CK_AM35XX' needs to be
> added for aes2_ick & sha12_ick in cclock3xxx_data.c.   The following
> patch should fix it (applies to my submitted/crypto/hwmod branch):
> 
> diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c
> index 582b055..aa5bdf6 100644
> --- a/arch/arm/mach-omap2/cclock3xxx_data.c
> +++ b/arch/arm/mach-omap2/cclock3xxx_data.c
> @@ -3332,10 +3332,10 @@ static struct omap_clk omap3xxx_clks[] = {
>  	CLK("omap_hsmmc.2",	"ick",	&mmchs3_ick,	CK_3430ES2PLUS | CK_AM35XX | CK_36XX),
>  	CLK(NULL,	"mmchs3_ick",	&mmchs3_ick,	CK_3430ES2PLUS | CK_AM35XX | CK_36XX),
>  	CLK(NULL,	"icr_ick",	&icr_ick,	CK_34XX | CK_36XX),
> -	CLK("omap-aes",	"ick",		&aes2_ick,	CK_34XX | CK_36XX),
> -	CLK(NULL,	"aes2_ick",	&aes2_ick,	CK_34XX | CK_36XX),
> -	CLK("omap-sham",	"ick",	&sha12_ick,	CK_34XX | CK_36XX),
> -	CLK(NULL,	"sha12_ick",	&sha12_ick,	CK_34XX | CK_36XX),
> +	CLK("omap-aes",	"ick",		&aes2_ick,	CK_34XX | CK_AM35XX | CK_36XX),
> +	CLK(NULL,	"aes2_ick",	&aes2_ick,	CK_34XX | CK_AM35XX | CK_36XX),
> +	CLK("omap-sham",	"ick",	&sha12_ick,	CK_34XX | CK_AM35XX | CK_36XX),
> +	CLK(NULL,	"sha12_ick",	&sha12_ick,	CK_34XX | CK_AM35XX | CK_36XX),
>  	CLK(NULL,	"des2_ick",	&des2_ick,	CK_34XX | CK_36XX),
>  	CLK("omap_hsmmc.1",	"ick",	&mmchs2_ick,	CK_3XXX),
>  	CLK("omap_hsmmc.0",	"ick",	&mmchs1_ick,	CK_3XXX),
> 
> 
> Please let me know if this patch works for you and, if it does, I'll respin
> my patches to add those changes.

If those clocks are referenced by the hwmods, that that patch makes sense 
to me.  Haven't had the chance to test it yet but maybe tomorrow.  On the 
other hand it looks 'obviously correct' so maybe just add that change to 
your patches and repost that one?


- Paul
Mark Greer Jan. 17, 2013, 10:27 p.m. UTC | #2 
On Thu, Jan 17, 2013 at 07:13:36PM +0000, Paul Walmsley wrote:
> Hi Mark,

Hi Paul.

> I regret the delay,
> 
> On Tue, 8 Jan 2013, Mark A. Greer wrote:
> 
> > On Sun, Dec 23, 2012 at 08:40:43AM +0000, Paul Walmsley wrote:
> >
> > > - The patch series causes AM3517/3505 to crash.  I'd guess this is due to 
> > > the SHAM/AES modules being initialized on those chips, but they probably 
> > > don't exist there.  Can you change the initialization for those on OMAP3 
> > > to only take place on OMAP34xx/36xx GP?  I guess you'd need to create new 
> > > lists for those in the hwmod init.
> > 
> > All am35xx GPs have the SHAM and AES modules except some very old ones.
> > I've been told that there should be very few of the "old" ones around
> > (I don't know how to differentiate them).  We're likely safe since the
> > SHAM & AES modules are not enabled in omap2plus_defconfig so nobody
> > should be enabling them on an am35xx unless they know that they have
> > the modules.  Do you agree?
> 
> Those will presumably only enable or disable the device drivers.  The 
> hwmod code will probably still try to write to those IP blocks if they are 
> listed as present in the hwmod data, during the initial reset-and-idle 
> phase.

Um, yeah, good point. :)

> What do you think about adding an am35xx_es11plus_hwmod_ocp_ifs[] array to 
> omap_hwmod_3xxx_data.c for these secure hwmods?  That carries the implicit 
> and possibly wrong assumption that it's likely to be ES1.0 devices that 
> are missing the SHAM/AES, but it seems unlikely that TI would have 
> multiple silicon revs running around claiming to be ES1.1?  Or maybe I'm 
> just being naïve.

Something like that makes sense to me.  I'll re-read my email, etc. and
see if I can find something to help us figure it out.

> > The issue that you're likely running into is that 'CK_AM35XX' needs to be
> > added for aes2_ick & sha12_ick in cclock3xxx_data.c.   The following
> > patch should fix it (applies to my submitted/crypto/hwmod branch):
> > 
> > diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c
> > index 582b055..aa5bdf6 100644
> > --- a/arch/arm/mach-omap2/cclock3xxx_data.c
> > +++ b/arch/arm/mach-omap2/cclock3xxx_data.c
> > @@ -3332,10 +3332,10 @@ static struct omap_clk omap3xxx_clks[] = {
> >  	CLK("omap_hsmmc.2",	"ick",	&mmchs3_ick,	CK_3430ES2PLUS | CK_AM35XX | CK_36XX),
> >  	CLK(NULL,	"mmchs3_ick",	&mmchs3_ick,	CK_3430ES2PLUS | CK_AM35XX | CK_36XX),
> >  	CLK(NULL,	"icr_ick",	&icr_ick,	CK_34XX | CK_36XX),
> > -	CLK("omap-aes",	"ick",		&aes2_ick,	CK_34XX | CK_36XX),
> > -	CLK(NULL,	"aes2_ick",	&aes2_ick,	CK_34XX | CK_36XX),
> > -	CLK("omap-sham",	"ick",	&sha12_ick,	CK_34XX | CK_36XX),
> > -	CLK(NULL,	"sha12_ick",	&sha12_ick,	CK_34XX | CK_36XX),
> > +	CLK("omap-aes",	"ick",		&aes2_ick,	CK_34XX | CK_AM35XX | CK_36XX),
> > +	CLK(NULL,	"aes2_ick",	&aes2_ick,	CK_34XX | CK_AM35XX | CK_36XX),
> > +	CLK("omap-sham",	"ick",	&sha12_ick,	CK_34XX | CK_AM35XX | CK_36XX),
> > +	CLK(NULL,	"sha12_ick",	&sha12_ick,	CK_34XX | CK_AM35XX | CK_36XX),
> >  	CLK(NULL,	"des2_ick",	&des2_ick,	CK_34XX | CK_36XX),
> >  	CLK("omap_hsmmc.1",	"ick",	&mmchs2_ick,	CK_3XXX),
> >  	CLK("omap_hsmmc.0",	"ick",	&mmchs1_ick,	CK_3XXX),
> > 
> > 
> > Please let me know if this patch works for you and, if it does, I'll respin
> > my patches to add those changes.
> 
> If those clocks are referenced by the hwmods, that that patch makes sense 
> to me.  Haven't had the chance to test it yet but maybe tomorrow.  On the 
> other hand it looks 'obviously correct' so maybe just add that change to 
> your patches and repost that one?

Will do.

Mark
--
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Greer Jan. 28, 2013, 7:16 p.m. UTC | #3 
On Thu, Jan 17, 2013 at 03:27:28PM -0700, Mark A. Greer wrote:
> On Thu, Jan 17, 2013 at 07:13:36PM +0000, Paul Walmsley wrote:
> > Hi Mark,
> 
> Hi Paul.

Hi again, Paul.  Sorry for the delay, I've been under the weather.

> > I regret the delay,
> > 
> > On Tue, 8 Jan 2013, Mark A. Greer wrote:
> > 
> > > On Sun, Dec 23, 2012 at 08:40:43AM +0000, Paul Walmsley wrote:

> > What do you think about adding an am35xx_es11plus_hwmod_ocp_ifs[] array to 
> > omap_hwmod_3xxx_data.c for these secure hwmods?  That carries the implicit 
> > and possibly wrong assumption that it's likely to be ES1.0 devices that 
> > are missing the SHAM/AES, but it seems unlikely that TI would have 
> > multiple silicon revs running around claiming to be ES1.1?  Or maybe I'm 
> > just being naïve.
> 
> Something like that makes sense to me.  I'll re-read my email, etc. and
> see if I can find something to help us figure it out.

I couldn't find any information that helped with this so AFAIK there is no
good way to tell if a particular am35xx has the crypto hardware available
or not.  At this point, I vote for moving 'omap3xxx_l4_core__sham' and
'omap3xxx_l4_core__aes' from omap3xxx_gp_hwmod_ocp_ifs[] and putting them
in omap34xx_hwmod_ocp_ifs[] and omap36xx_hwmod_ocp_ifs[].  That should be
safe in general and if someone with an am35xx wants to use those modules,
they can edit am35xx_hwmod_ocp_ifs[] locally.

What do you think?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Walmsley Feb. 1, 2013, 5:35 p.m. UTC | #4 
Hi Mark

On Mon, 28 Jan 2013, Mark A. Greer wrote:

> On Thu, Jan 17, 2013 at 03:27:28PM -0700, Mark A. Greer wrote:
> > On Thu, Jan 17, 2013 at 07:13:36PM +0000, Paul Walmsley wrote:
> > > On Tue, 8 Jan 2013, Mark A. Greer wrote:
> > > 
> > > > On Sun, Dec 23, 2012 at 08:40:43AM +0000, Paul Walmsley wrote:
> 
> > > What do you think about adding an am35xx_es11plus_hwmod_ocp_ifs[] array to 
> > > omap_hwmod_3xxx_data.c for these secure hwmods?  That carries the implicit 
> > > and possibly wrong assumption that it's likely to be ES1.0 devices that 
> > > are missing the SHAM/AES, but it seems unlikely that TI would have 
> > > multiple silicon revs running around claiming to be ES1.1?  Or maybe I'm 
> > > just being naïve.
> > 
> > Something like that makes sense to me.  I'll re-read my email, etc. and
> > see if I can find something to help us figure it out.
> 
> I couldn't find any information that helped with this so AFAIK there is no
> good way to tell if a particular am35xx has the crypto hardware available
> or not.

I was thinking that we might assume that they are present on AM35xx 
ES1.1+.  If the TI folks are saying that they aren't available on only a 
few early devices, I'd guess that means ES1.0.  I personally have never 
seen an ES1.0 AM35xx device... 

Discriminating between ES1.0 and ES1.1+ should be pretty easy in the hwmod 
init...

>  At this point, I vote for moving 'omap3xxx_l4_core__sham' and
> 'omap3xxx_l4_core__aes' from omap3xxx_gp_hwmod_ocp_ifs[] and putting them
> in omap34xx_hwmod_ocp_ifs[] and omap36xx_hwmod_ocp_ifs[].  

I'm pretty sure that's going to break on HS OMAPs, like the HS OMAP3430 in 
the N900.  I don't think those IP blocks are directly accessible from 
Linux on most HS setups, although this might vary by device.  I'd feel 
more comfortable if you created an omap34xx_gp_hwmod_ocp_ifs[] list and an 
omap36xx_gp_hwmod_ocp_ifs[] list.  We should probably get rid of 
omap3xxx_gp_hwmod_ocp_ifs[] altogether.

> That should be safe in general and if someone with an am35xx wants to 
> use those modules, they can edit am35xx_hwmod_ocp_ifs[] locally.

If you want to just leave them commented in am35xx_hwmod_ocp_ifs[], rather 
than enabling them for ES1.1+ AM35xx, that's fine with me too, since we 
don't know that they are ES-level-based.  Maybe put a comment there that 
says that these are likely to be present, but no one seems to know for 
certain?  Seems ludicrous, but I guess that's what we're reduced to!


- Paul
Mark Greer Feb. 1, 2013, 8:18 p.m. UTC | #5 
On Fri, Feb 01, 2013 at 05:35:05PM +0000, Paul Walmsley wrote:
> Hi Mark
> 
> On Mon, 28 Jan 2013, Mark A. Greer wrote:
> 
> > On Thu, Jan 17, 2013 at 03:27:28PM -0700, Mark A. Greer wrote:
> > > On Thu, Jan 17, 2013 at 07:13:36PM +0000, Paul Walmsley wrote:
> > > > On Tue, 8 Jan 2013, Mark A. Greer wrote:
> > > > 
> > > > > On Sun, Dec 23, 2012 at 08:40:43AM +0000, Paul Walmsley wrote:
> > 
> > > > What do you think about adding an am35xx_es11plus_hwmod_ocp_ifs[] array to 
> > > > omap_hwmod_3xxx_data.c for these secure hwmods?  That carries the implicit 
> > > > and possibly wrong assumption that it's likely to be ES1.0 devices that 
> > > > are missing the SHAM/AES, but it seems unlikely that TI would have 
> > > > multiple silicon revs running around claiming to be ES1.1?  Or maybe I'm 
> > > > just being naïve.
> > > 
> > > Something like that makes sense to me.  I'll re-read my email, etc. and
> > > see if I can find something to help us figure it out.
> > 
> > I couldn't find any information that helped with this so AFAIK there is no
> > good way to tell if a particular am35xx has the crypto hardware available
> > or not.
> 
> I was thinking that we might assume that they are present on AM35xx 
> ES1.1+.  If the TI folks are saying that they aren't available on only a 
> few early devices, I'd guess that means ES1.0.  I personally have never 
> seen an ES1.0 AM35xx device... 
> 
> Discriminating between ES1.0 and ES1.1+ should be pretty easy in the hwmod 
> init...
> 
> >  At this point, I vote for moving 'omap3xxx_l4_core__sham' and
> > 'omap3xxx_l4_core__aes' from omap3xxx_gp_hwmod_ocp_ifs[] and putting them
> > in omap34xx_hwmod_ocp_ifs[] and omap36xx_hwmod_ocp_ifs[].  
> 
> I'm pretty sure that's going to break on HS OMAPs, like the HS OMAP3430 in 
> the N900.  I don't think those IP blocks are directly accessible from 
> Linux on most HS setups, although this might vary by device.  I'd feel 
> more comfortable if you created an omap34xx_gp_hwmod_ocp_ifs[] list and an 
> omap36xx_gp_hwmod_ocp_ifs[] list.  We should probably get rid of 
> omap3xxx_gp_hwmod_ocp_ifs[] altogether.
> 
> > That should be safe in general and if someone with an am35xx wants to 
> > use those modules, they can edit am35xx_hwmod_ocp_ifs[] locally.
> 
> If you want to just leave them commented in am35xx_hwmod_ocp_ifs[], rather 
> than enabling them for ES1.1+ AM35xx, that's fine with me too, since we 
> don't know that they are ES-level-based.  Maybe put a comment there that 
> says that these are likely to be present, but no one seems to know for 
> certain?  Seems ludicrous, but I guess that's what we're reduced to!

Thanks Paul.  I will have some patches early next week.

Mark
--
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Walmsley Feb. 8, 2013, 5:45 p.m. UTC | #6 
Hi Mark,

just FYI, these patches caused several warnings from sparse:

> arch/arm/mach-omap2/omap_hwmod_2xxx_interconnect_data.c:141:30: warning: 
symbol 'omap2xxx_sham_addrs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_2xxx_interconnect_data.c:150:30: warning: 
symbol 'omap2xxx_aes_addrs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c:884:28: warning: 
symbol 'omap2_sham_mpu_irqs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c:889:28: warning: 
symbol 'omap2_sham_sdma_chs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c:927:28: warning: 
symbol 'omap2_aes_sdma_chs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_33xx_data.c:540:28: warning: symbol 
'am33xx_aes0_edma_reqs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_33xx_data.c:579:28: warning: symbol 
'am33xx_sha0_edma_reqs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c:3564:28: warning: symbol 
'omap3_sham_mpu_irqs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c:3569:28: warning: symbol 
'omap3_sham_sdma_reqs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c:3574:19: warning: symbol 
'omap3xxx_sham_hwmod' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c:3630:28: warning: symbol 
'omap3_aes_sdma_reqs' was not declared. Should it be static?
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c:3636:19: warning: symbol 
'omap3xxx_aes_hwmod' was not declared. Should it be static?


I've fixed them up here, but please don't forget to make sure that patches 
don't add any sparse warnings.  sparse info is here:

https://sparse.wiki.kernel.org/index.php/Main_Page

regards,

- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/mach-omap2/cclock3xxx_data.c b/arch/arm/mach-omap2/cclock3xxx_data.c
index 582b055..aa5bdf6 100644
--- a/arch/arm/mach-omap2/cclock3xxx_data.c
+++ b/arch/arm/mach-omap2/cclock3xxx_data.c
@@ -3332,10 +3332,10 @@  static struct omap_clk omap3xxx_clks[] = {
 	CLK("omap_hsmmc.2",	"ick",	&mmchs3_ick,	CK_3430ES2PLUS | CK_AM35XX | CK_36XX),
 	CLK(NULL,	"mmchs3_ick",	&mmchs3_ick,	CK_3430ES2PLUS | CK_AM35XX | CK_36XX),
 	CLK(NULL,	"icr_ick",	&icr_ick,	CK_34XX | CK_36XX),
-	CLK("omap-aes",	"ick",		&aes2_ick,	CK_34XX | CK_36XX),
-	CLK(NULL,	"aes2_ick",	&aes2_ick,	CK_34XX | CK_36XX),
-	CLK("omap-sham",	"ick",	&sha12_ick,	CK_34XX | CK_36XX),
-	CLK(NULL,	"sha12_ick",	&sha12_ick,	CK_34XX | CK_36XX),
+	CLK("omap-aes",	"ick",		&aes2_ick,	CK_34XX | CK_AM35XX | CK_36XX),
+	CLK(NULL,	"aes2_ick",	&aes2_ick,	CK_34XX | CK_AM35XX | CK_36XX),
+	CLK("omap-sham",	"ick",	&sha12_ick,	CK_34XX | CK_AM35XX | CK_36XX),
+	CLK(NULL,	"sha12_ick",	&sha12_ick,	CK_34XX | CK_AM35XX | CK_36XX),
 	CLK(NULL,	"des2_ick",	&des2_ick,	CK_34XX | CK_36XX),
 	CLK("omap_hsmmc.1",	"ick",	&mmchs2_ick,	CK_3XXX),
 	CLK("omap_hsmmc.0",	"ick",	&mmchs1_ick,	CK_3XXX),