| Message ID | 20130108203853.GB1876@animalcreek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 --
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?
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
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 --
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
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),