| Message ID | alpine.DEB.2.02.1501020654050.27058@utopia.booyaka.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Paul, On Saturday 03 January 2015 02:40 AM, Paul Walmsley wrote: > + Suman, lakml > > Hi Roger > > On Thu, 18 Dec 2014, Roger Quadros wrote: > >> Fixing up Paul's email id. >> >> cheers, >> -roger >> >> On 18/12/14 17:49, Roger Quadros wrote: >>> There are quite a few hwmods that don't have sysconfig register and so >>> _find_mpu_rt_port(oh) will return NULL thus preventing ready state check >>> on those modules after the module is enabled. > > Hmm. Any IP block that exposes registers that are accessible by the MPU > should have an MPU register target port, even if there's no SYSCONFIG > register. And if an IP block doesn't have registers that are accessible > from the MPU, then there shouldn't be much point to waiting for the module > to become ready. > > Looks like the real problem is the test for oh->class->sysc before the > call to _init_mpu_rt_base(). That was introduced by commit 6423d6df1440 > ("ARM: OMAP2+: hwmod: check for module address space during init"). It's > not clear to me why that test was added, since _init_mpu_rt_base() doesn't > do anything with oh->class->sysc or SYSCONFIG registers. This was introduced by commit 97597b962529 (ARM: OMAP2+: hwmod: Don't call _init_mpu_rt_base if no sysc) Patch description states that "there are few hwmod which doesn't have sysconfig registers and hence no need to ioremap() them in early init code". Isn't this correct? May be a dumb question: If IP doesn't have sysconfig, is there any case that hwmod does access the register address space of that IP? Why do we need to enable MPU register target port? Thanks and regards, Lokesh > > Could you please test the following patch? > I don't have an AM437x-gp-evm. > > > - Paul > > --- > arch/arm/mach-omap2/omap_hwmod.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > index cbb908dc5cf0..ce6d11f3eda7 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.c > +++ b/arch/arm/mach-omap2/omap_hwmod.c > @@ -2451,14 +2451,10 @@ static int __init _init(struct omap_hwmod *oh, void *data) > oh->name, np->name); > } > > - if (oh->class->sysc) { > - r = _init_mpu_rt_base(oh, NULL, index, np); > - if (r < 0) { > - WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n", > - oh->name); > - return 0; > - } > - } > + r = _init_mpu_rt_base(oh, NULL, index, np); > + if (r < 0) > + pr_debug("omap_hwmod: %s: doesn't have mpu register target base\n", > + oh->name); > > r = _init_clocks(oh, NULL); > if (r < 0) { >
On 01/05/2015 02:35 AM, Lokesh Vutla wrote: > Hi Paul, > On Saturday 03 January 2015 02:40 AM, Paul Walmsley wrote: >> + Suman, lakml >> >> Hi Roger >> >> On Thu, 18 Dec 2014, Roger Quadros wrote: >> >>> Fixing up Paul's email id. >>> >>> cheers, >>> -roger >>> >>> On 18/12/14 17:49, Roger Quadros wrote: >>>> There are quite a few hwmods that don't have sysconfig register and so >>>> _find_mpu_rt_port(oh) will return NULL thus preventing ready state check >>>> on those modules after the module is enabled. >> >> Hmm. Any IP block that exposes registers that are accessible by the MPU >> should have an MPU register target port, even if there's no SYSCONFIG >> register. And if an IP block doesn't have registers that are accessible >> from the MPU, then there shouldn't be much point to waiting for the module >> to become ready. >> >> Looks like the real problem is the test for oh->class->sysc before the >> call to _init_mpu_rt_base(). That was introduced by commit 6423d6df1440 >> ("ARM: OMAP2+: hwmod: check for module address space during init"). It's >> not clear to me why that test was added, since _init_mpu_rt_base() doesn't >> do anything with oh->class->sysc or SYSCONFIG registers. > This was introduced by commit > 97597b962529 (ARM: OMAP2+: hwmod: Don't call _init_mpu_rt_base if no sysc) That's right, the test was present even before 6423d6df1440, I merely made this a block. > Patch description states that "there are few hwmod which doesn't have sysconfig registers and hence > no need to ioremap() them in early init code". > Isn't this correct? > May be a dumb question: If IP doesn't have sysconfig, is there any case that hwmod does > access the register address space of that IP? Why do we need to enable MPU register target port? > > Thanks and regards, > Lokesh > >> >> Could you please test the following patch? >> I don't have an AM437x-gp-evm. >> >> >> - Paul >> >> --- >> arch/arm/mach-omap2/omap_hwmod.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c >> index cbb908dc5cf0..ce6d11f3eda7 100644 >> --- a/arch/arm/mach-omap2/omap_hwmod.c >> +++ b/arch/arm/mach-omap2/omap_hwmod.c >> @@ -2451,14 +2451,10 @@ static int __init _init(struct omap_hwmod *oh, void *data) >> oh->name, np->name); >> } >> >> - if (oh->class->sysc) { >> - r = _init_mpu_rt_base(oh, NULL, index, np); >> - if (r < 0) { >> - WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n", >> - oh->name); >> - return 0; >> - } >> - } >> + r = _init_mpu_rt_base(oh, NULL, index, np); >> + if (r < 0) >> + pr_debug("omap_hwmod: %s: doesn't have mpu register target base\n", >> + oh->name); >> >> r = _init_clocks(oh, NULL); >> if (r < 0) { >> >
+ Santosh Hi Lokesh On Mon, 5 Jan 2015, Lokesh Vutla wrote: > On Saturday 03 January 2015 02:40 AM, Paul Walmsley wrote: > > On Thu, 18 Dec 2014, Roger Quadros wrote: > > > >> On 18/12/14 17:49, Roger Quadros wrote: > >>> There are quite a few hwmods that don't have sysconfig register and so > >>> _find_mpu_rt_port(oh) will return NULL thus preventing ready state check > >>> on those modules after the module is enabled. > > > > Hmm. Any IP block that exposes registers that are accessible by the MPU > > should have an MPU register target port, even if there's no SYSCONFIG > > register. And if an IP block doesn't have registers that are accessible > > from the MPU, then there shouldn't be much point to waiting for the module > > to become ready. > > > > Looks like the real problem is the test for oh->class->sysc before the > > call to _init_mpu_rt_base(). That was introduced by commit 6423d6df1440 > > ("ARM: OMAP2+: hwmod: check for module address space during init"). It's > > not clear to me why that test was added, since _init_mpu_rt_base() doesn't > > do anything with oh->class->sysc or SYSCONFIG registers. > This was introduced by commit > 97597b962529 (ARM: OMAP2+: hwmod: Don't call _init_mpu_rt_base if no sysc) Yes, you're right. I misread commit 6423d6df1440. > Patch description states that "there are few hwmod which doesn't have sysconfig registers and hence > no need to ioremap() them in early init code". The MPU register target port code doesn't only determine whether the hwmod code should map the IP block's address space. It also determines whether or not the hwmod code needs to wait for the IP block to become ready after being enabled, so register accesses by non-hwmod code can succeed. > Isn't this correct? That commit 97597b962529 is bogus. If the goal was to skip the ioremap(), then the right fix would have been to just skip the ioremap(). The existing commit also skips the call to _save_mpu_port_index(), which caused the problem that Roger's patch is trying to fix. > May be a dumb question: If IP doesn't have sysconfig, is there any case > that hwmod does access the register address space of that IP? Why do we > need to enable MPU register target port? hwmod shouldn't touch the IP block registers if there are no SYSCONFIG, SYSSTATUS, etc. registers, and there's no IP block-specific reset code. But other code on the system (like the IP block's device driver) will, and the system needs to indicate that the IP block is ready before those accesses occur. - Paul
On Mon, 5 Jan 2015, Suman Anna wrote: > That's right, the test was present even before 6423d6df1440, I merely > made this a block. Indeed, I misread the commit. Sorry about ihat. - Paul
On 1/5/15 2:19 PM, Paul Walmsley wrote: > + Santosh > > Hi Lokesh > > On Mon, 5 Jan 2015, Lokesh Vutla wrote: >> On Saturday 03 January 2015 02:40 AM, Paul Walmsley wrote: >>> On Thu, 18 Dec 2014, Roger Quadros wrote: >>> >>>> On 18/12/14 17:49, Roger Quadros wrote: >>>>> There are quite a few hwmods that don't have sysconfig register and so >>>>> _find_mpu_rt_port(oh) will return NULL thus preventing ready state check >>>>> on those modules after the module is enabled. >>> >>> Hmm. Any IP block that exposes registers that are accessible by the MPU >>> should have an MPU register target port, even if there's no SYSCONFIG >>> register. And if an IP block doesn't have registers that are accessible >>> from the MPU, then there shouldn't be much point to waiting for the module >>> to become ready. >>> >>> Looks like the real problem is the test for oh->class->sysc before the >>> call to _init_mpu_rt_base(). That was introduced by commit 6423d6df1440 >>> ("ARM: OMAP2+: hwmod: check for module address space during init"). It's >>> not clear to me why that test was added, since _init_mpu_rt_base() doesn't >>> do anything with oh->class->sysc or SYSCONFIG registers. >> This was introduced by commit >> 97597b962529 (ARM: OMAP2+: hwmod: Don't call _init_mpu_rt_base if no sysc) > > Yes, you're right. I misread commit 6423d6df1440. > >> Patch description states that "there are few hwmod which doesn't have sysconfig registers and hence >> no need to ioremap() them in early init code". > > The MPU register target port code doesn't only determine whether the > hwmod code should map the IP block's address space. It also determines > whether or not the hwmod code needs to wait for the IP block to become > ready after being enabled, so register accesses by non-hwmod code can > succeed. > IIRC, Yes the intention was to skip the ioremap o.w there were some crashes seen. I am fine if that check is actually moved closer to iormap as it should have been first place. Regards, Santosh seen
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index cbb908dc5cf0..ce6d11f3eda7 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -2451,14 +2451,10 @@ static int __init _init(struct omap_hwmod *oh, void *data) oh->name, np->name); } - if (oh->class->sysc) { - r = _init_mpu_rt_base(oh, NULL, index, np); - if (r < 0) { - WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n", - oh->name); - return 0; - } - } + r = _init_mpu_rt_base(oh, NULL, index, np); + if (r < 0) + pr_debug("omap_hwmod: %s: doesn't have mpu register target base\n", + oh->name); r = _init_clocks(oh, NULL); if (r < 0) {