| Message ID | alpine.DEB.2.02.1501060202540.28771@utopia.booyaka.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Paul, On Tuesday 06 January 2015 07:34 AM, Paul Walmsley wrote: > > Roger, Lokesh, could you try this one instead? Yep, the below patch works on AM437x. Boot logs here: http://paste.ubuntu.com/9680938/ Thanks and regards, Lokesh > > It passes all the basic tests here except it does not boot on the 4460 > VAR-SOM-OM - unclear why at this point - but it would be good to see if it > works on your AM4372 boards, since I don't have that one. > > Test logs are here: > > http://www.pwsan.com/omap/testlogs/hwmod_skip_only_remap_v3.19-rc/20150105171744/ > > > - Paul > > > From 4f2e13bd2181e0ebede3aabc484aa2339830748a Mon Sep 17 00:00:00 2001 > From: Paul Walmsley <paul@pwsan.com> > Date: Mon, 5 Jan 2015 15:49:57 -0700 > Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header > registers. Experimental. > > --- > arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > index cbb908dc5cf0..03df8833d399 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.c > +++ b/arch/arm/mach-omap2/omap_hwmod.c > @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) > pr_debug("omap_hwmod: %s: resetting\n", oh->name); > > if (oh->class->reset) { > + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", > + oh->name); > r = oh->class->reset(oh); > } else { > if (oh->rst_lines_cnt > 0) { > @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, > } > > /** > - * _init_mpu_rt_base - populate the virtual address for a hwmod > + * _init_mpu_rt_base - populate the MPU port and virtual address > * @oh: struct omap_hwmod * to locate the virtual address > * @data: (unused, caller should pass NULL) > * @index: index of the reg entry iospace in device tree > * @np: struct device_node * of the IP block's device node in the DT data > * > - * Cache the virtual address used by the MPU to access this IP block's > - * registers. This address is needed early so the OCP registers that > - * are part of the device's address space can be ioremapped properly. > + * Cache the interconnect target port and the virtual address used by > + * the MPU to access this IP block's registers. The address is needed > + * early so the OCP registers that are part of the device's address > + * space can be ioremapped properly. The presence or absence of the > + * interconnect target port also indicates whether the hwmod code > + * should wait for the IP block to indicate readiness after it is > + * enabled. > * > * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and > * -ENXIO on absent or invalid register target address space. > @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, > if (oh->_int_flags & _HWMOD_NO_MPU_PORT) > return -ENXIO; > > + /* > + * If there's no need for the hwmod code to read or write to > + * the IP block registers, bail out early before the ioremap() > + */ > + if (!oh->class->sysc) > + return 0; > + > mem = _find_mpu_rt_addr_space(oh); > if (!mem) { > pr_debug("omap_hwmod: %s: no MPU register target found\n", > @@ -2451,14 +2464,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) { >
Hi Paul, On 01/06/2015 02:14 AM, Lokesh Vutla wrote: > Hi Paul, > On Tuesday 06 January 2015 07:34 AM, Paul Walmsley wrote: >> >> Roger, Lokesh, could you try this one instead? > Yep, the below patch works on AM437x. > Boot logs here: http://paste.ubuntu.com/9680938/ > > Thanks and regards, > Lokesh >> >> It passes all the basic tests here except it does not boot on the 4460 >> VAR-SOM-OM - unclear why at this point - but it would be good to see if it >> works on your AM4372 boards, since I don't have that one. >> >> Test logs are here: >> >> http://www.pwsan.com/omap/testlogs/hwmod_skip_only_remap_v3.19-rc/20150105171744/ >> >> >> - Paul >> >> >> From 4f2e13bd2181e0ebede3aabc484aa2339830748a Mon Sep 17 00:00:00 2001 >> From: Paul Walmsley <paul@pwsan.com> >> Date: Mon, 5 Jan 2015 15:49:57 -0700 >> Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header >> registers. Experimental. >> >> --- >> arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ >> 1 file changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c >> index cbb908dc5cf0..03df8833d399 100644 >> --- a/arch/arm/mach-omap2/omap_hwmod.c >> +++ b/arch/arm/mach-omap2/omap_hwmod.c >> @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) >> pr_debug("omap_hwmod: %s: resetting\n", oh->name); >> >> if (oh->class->reset) { >> + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", >> + oh->name); >> r = oh->class->reset(oh); >> } else { >> if (oh->rst_lines_cnt > 0) { >> @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, >> } >> >> /** >> - * _init_mpu_rt_base - populate the virtual address for a hwmod >> + * _init_mpu_rt_base - populate the MPU port and virtual address >> * @oh: struct omap_hwmod * to locate the virtual address >> * @data: (unused, caller should pass NULL) >> * @index: index of the reg entry iospace in device tree >> * @np: struct device_node * of the IP block's device node in the DT data >> * >> - * Cache the virtual address used by the MPU to access this IP block's >> - * registers. This address is needed early so the OCP registers that >> - * are part of the device's address space can be ioremapped properly. >> + * Cache the interconnect target port and the virtual address used by >> + * the MPU to access this IP block's registers. The address is needed >> + * early so the OCP registers that are part of the device's address >> + * space can be ioremapped properly. The presence or absence of the >> + * interconnect target port also indicates whether the hwmod code >> + * should wait for the IP block to indicate readiness after it is >> + * enabled. >> * >> * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and >> * -ENXIO on absent or invalid register target address space. >> @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, >> if (oh->_int_flags & _HWMOD_NO_MPU_PORT) >> return -ENXIO; >> >> + /* >> + * If there's no need for the hwmod code to read or write to >> + * the IP block registers, bail out early before the ioremap() >> + */ >> + if (!oh->class->sysc) >> + return 0; >> + >> mem = _find_mpu_rt_addr_space(oh); >> if (!mem) { >> pr_debug("omap_hwmod: %s: no MPU register target found\n", >> @@ -2451,14 +2464,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); You have removed the return from the above block on failure. If any DT entry doesn't have the reg property, this will hang the kernel boot. Just remove the "reg" entry from any of the existing DT, and you will run into the issue, this is what 6423d6df1440 ("ARM: OMAP2+: hwmod: check for module address space during init") fixed. Also, are you sure you want to turn the WARN into a pr_debug, it won't even show during the kernel boot log if the reg base is missing. regards Suman >> >> r = _init_clocks(oh, NULL); >> if (r < 0) { >> >
Hi Paul, On 01/06/2015 11:14 AM, Suman Anna wrote: > Hi Paul, > > On 01/06/2015 02:14 AM, Lokesh Vutla wrote: >> Hi Paul, >> On Tuesday 06 January 2015 07:34 AM, Paul Walmsley wrote: >>> >>> Roger, Lokesh, could you try this one instead? >> Yep, the below patch works on AM437x. >> Boot logs here: http://paste.ubuntu.com/9680938/ >> >> Thanks and regards, >> Lokesh >>> >>> It passes all the basic tests here except it does not boot on the 4460 >>> VAR-SOM-OM - unclear why at this point - but it would be good to see if it >>> works on your AM4372 boards, since I don't have that one. Looking at your rc3 log @ http://www.pwsan.com/omap/testlogs/test_v3.19-rc3/20150105224749/boot/4460varsomom/, I do see a missing reg entry for mailbox, and that's the reason for your hang because of the missing bail out in your new patch. [ 0.261932] ------------[ cut here ]------------ [ 0.261962] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/omap_hwmod.c:2458 _init+0x3a0/0x3f0() [ 0.261962] omap_hwmod: mailbox: doesn't have mpu register target base ... ... [ 0.262329] ---[ end trace a1be72591db4662e ]--- Now that said, this is weird, since the reg property for mailbox is defined in the base omap4.dtsi and should be inherited by the 4460 VAR-SOM-OM, unless you are booting with an old dtb. regards Suman >>> >>> Test logs are here: >>> >>> http://www.pwsan.com/omap/testlogs/hwmod_skip_only_remap_v3.19-rc/20150105171744/ >>> >>> >>> - Paul >>> >>> >>> From 4f2e13bd2181e0ebede3aabc484aa2339830748a Mon Sep 17 00:00:00 2001 >>> From: Paul Walmsley <paul@pwsan.com> >>> Date: Mon, 5 Jan 2015 15:49:57 -0700 >>> Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header >>> registers. Experimental. >>> >>> --- >>> arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ >>> 1 file changed, 21 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c >>> index cbb908dc5cf0..03df8833d399 100644 >>> --- a/arch/arm/mach-omap2/omap_hwmod.c >>> +++ b/arch/arm/mach-omap2/omap_hwmod.c >>> @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) >>> pr_debug("omap_hwmod: %s: resetting\n", oh->name); >>> >>> if (oh->class->reset) { >>> + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", >>> + oh->name); >>> r = oh->class->reset(oh); >>> } else { >>> if (oh->rst_lines_cnt > 0) { >>> @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, >>> } >>> >>> /** >>> - * _init_mpu_rt_base - populate the virtual address for a hwmod >>> + * _init_mpu_rt_base - populate the MPU port and virtual address >>> * @oh: struct omap_hwmod * to locate the virtual address >>> * @data: (unused, caller should pass NULL) >>> * @index: index of the reg entry iospace in device tree >>> * @np: struct device_node * of the IP block's device node in the DT data >>> * >>> - * Cache the virtual address used by the MPU to access this IP block's >>> - * registers. This address is needed early so the OCP registers that >>> - * are part of the device's address space can be ioremapped properly. >>> + * Cache the interconnect target port and the virtual address used by >>> + * the MPU to access this IP block's registers. The address is needed >>> + * early so the OCP registers that are part of the device's address >>> + * space can be ioremapped properly. The presence or absence of the >>> + * interconnect target port also indicates whether the hwmod code >>> + * should wait for the IP block to indicate readiness after it is >>> + * enabled. >>> * >>> * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and >>> * -ENXIO on absent or invalid register target address space. >>> @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, >>> if (oh->_int_flags & _HWMOD_NO_MPU_PORT) >>> return -ENXIO; >>> >>> + /* >>> + * If there's no need for the hwmod code to read or write to >>> + * the IP block registers, bail out early before the ioremap() >>> + */ >>> + if (!oh->class->sysc) >>> + return 0; >>> + >>> mem = _find_mpu_rt_addr_space(oh); >>> if (!mem) { >>> pr_debug("omap_hwmod: %s: no MPU register target found\n", >>> @@ -2451,14 +2464,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); > > You have removed the return from the above block on failure. If any DT > entry doesn't have the reg property, this will hang the kernel boot. > Just remove the "reg" entry from any of the existing DT, and you will > run into the issue, this is what 6423d6df1440 ("ARM: OMAP2+: hwmod: > check for module address space during init") fixed. Also, are you sure > you want to turn the WARN into a pr_debug, it won't even show during the > kernel boot log if the reg base is missing. > > regards > Suman > >>> >>> r = _init_clocks(oh, NULL); >>> if (r < 0) { >>> >> >
On 01/06/2015 11:27 AM, Suman Anna wrote: > Hi Paul, > > On 01/06/2015 11:14 AM, Suman Anna wrote: >> Hi Paul, >> >> On 01/06/2015 02:14 AM, Lokesh Vutla wrote: >>> Hi Paul, >>> On Tuesday 06 January 2015 07:34 AM, Paul Walmsley wrote: >>>> >>>> Roger, Lokesh, could you try this one instead? >>> Yep, the below patch works on AM437x. >>> Boot logs here: http://paste.ubuntu.com/9680938/ >>> >>> Thanks and regards, >>> Lokesh >>>> >>>> It passes all the basic tests here except it does not boot on the 4460 >>>> VAR-SOM-OM - unclear why at this point - but it would be good to see if it >>>> works on your AM4372 boards, since I don't have that one. > > Looking at your rc3 log @ > http://www.pwsan.com/omap/testlogs/test_v3.19-rc3/20150105224749/boot/4460varsomom/, > I do see a missing reg entry for mailbox, and that's the reason for your > hang because of the missing bail out in your new patch. > > [ 0.261932] ------------[ cut here ]------------ > [ 0.261962] WARNING: CPU: 0 PID: 1 at > arch/arm/mach-omap2/omap_hwmod.c:2458 _init+0x3a0/0x3f0() > [ 0.261962] omap_hwmod: mailbox: doesn't have mpu register target base > ... > ... > [ 0.262329] ---[ end trace a1be72591db4662e ]--- > > Now that said, this is weird, since the reg property for mailbox is > defined in the base omap4.dtsi and should be inherited by the 4460 > VAR-SOM-OM, unless you are booting with an old dtb. > > regards > Suman > >>>> >>>> Test logs are here: >>>> >>>> http://www.pwsan.com/omap/testlogs/hwmod_skip_only_remap_v3.19-rc/20150105171744/ >>>> >>>> >>>> - Paul >>>> >>>> >>>> From 4f2e13bd2181e0ebede3aabc484aa2339830748a Mon Sep 17 00:00:00 2001 >>>> From: Paul Walmsley <paul@pwsan.com> >>>> Date: Mon, 5 Jan 2015 15:49:57 -0700 >>>> Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header >>>> registers. Experimental. >>>> >>>> --- >>>> arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ >>>> 1 file changed, 21 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c >>>> index cbb908dc5cf0..03df8833d399 100644 >>>> --- a/arch/arm/mach-omap2/omap_hwmod.c >>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c >>>> @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) >>>> pr_debug("omap_hwmod: %s: resetting\n", oh->name); >>>> >>>> if (oh->class->reset) { >>>> + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", >>>> + oh->name); >>>> r = oh->class->reset(oh); >>>> } else { >>>> if (oh->rst_lines_cnt > 0) { >>>> @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, >>>> } >>>> >>>> /** >>>> - * _init_mpu_rt_base - populate the virtual address for a hwmod >>>> + * _init_mpu_rt_base - populate the MPU port and virtual address >>>> * @oh: struct omap_hwmod * to locate the virtual address >>>> * @data: (unused, caller should pass NULL) >>>> * @index: index of the reg entry iospace in device tree >>>> * @np: struct device_node * of the IP block's device node in the DT data >>>> * >>>> - * Cache the virtual address used by the MPU to access this IP block's >>>> - * registers. This address is needed early so the OCP registers that >>>> - * are part of the device's address space can be ioremapped properly. >>>> + * Cache the interconnect target port and the virtual address used by >>>> + * the MPU to access this IP block's registers. The address is needed >>>> + * early so the OCP registers that are part of the device's address >>>> + * space can be ioremapped properly. The presence or absence of the >>>> + * interconnect target port also indicates whether the hwmod code >>>> + * should wait for the IP block to indicate readiness after it is >>>> + * enabled. >>>> * >>>> * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and >>>> * -ENXIO on absent or invalid register target address space. >>>> @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, >>>> if (oh->_int_flags & _HWMOD_NO_MPU_PORT) >>>> return -ENXIO; >>>> >>>> + /* >>>> + * If there's no need for the hwmod code to read or write to >>>> + * the IP block registers, bail out early before the ioremap() >>>> + */ >>>> + if (!oh->class->sysc) >>>> + return 0; >>>> + Also, I am getting some additional warnings [1] w.r.t MPU hwmod when I changed the pr_debug on the check on _init_mpu_rt_base returned value to a WARN and return like before. These warnings seem to be because -ENXIO is returned as MPU hwmod will be setup with _HWMOD_NO_MPU_PORT. By moving the !oh->class->sysc check prior to the _int_flags check and after the _save_mpu_port_index(oh) call, I get the log similar to 3.19-rc3. Below test log is on BeagleBone black, and I have removed mailbox reg from am33xx.dtsi as well for testing the patch. regards Suman [1] http://slexy.org/view/s2bWsGSV1Y >>>> mem = _find_mpu_rt_addr_space(oh); >>>> if (!mem) { >>>> pr_debug("omap_hwmod: %s: no MPU register target found\n", >>>> @@ -2451,14 +2464,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); >> >> You have removed the return from the above block on failure. If any DT >> entry doesn't have the reg property, this will hang the kernel boot. >> Just remove the "reg" entry from any of the existing DT, and you will >> run into the issue, this is what 6423d6df1440 ("ARM: OMAP2+: hwmod: >> check for module address space during init") fixed. Also, are you sure >> you want to turn the WARN into a pr_debug, it won't even show during the >> kernel boot log if the reg base is missing. >> >> regards >> Suman >> >>>> >>>> r = _init_clocks(oh, NULL); >>>> if (r < 0) { >>>> >>> >> >
On 06/01/15 04:04, Paul Walmsley wrote: > > Roger, Lokesh, could you try this one instead? > > It passes all the basic tests here except it does not boot on the 4460 > VAR-SOM-OM - unclear why at this point - but it would be good to see if it > works on your AM4372 boards, since I don't have that one. > > Test logs are here: > > http://www.pwsan.com/omap/testlogs/hwmod_skip_only_remap_v3.19-rc/20150105171744/ > > > - Paul > > > From 4f2e13bd2181e0ebede3aabc484aa2339830748a Mon Sep 17 00:00:00 2001 > From: Paul Walmsley <paul@pwsan.com> > Date: Mon, 5 Jan 2015 15:49:57 -0700 > Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header > registers. Experimental. > > --- > arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > index cbb908dc5cf0..03df8833d399 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.c > +++ b/arch/arm/mach-omap2/omap_hwmod.c > @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) > pr_debug("omap_hwmod: %s: resetting\n", oh->name); > > if (oh->class->reset) { > + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", > + oh->name); Not part of $subject. > r = oh->class->reset(oh); > } else { > if (oh->rst_lines_cnt > 0) { > @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, > } > > /** > - * _init_mpu_rt_base - populate the virtual address for a hwmod > + * _init_mpu_rt_base - populate the MPU port and virtual address > * @oh: struct omap_hwmod * to locate the virtual address > * @data: (unused, caller should pass NULL) > * @index: index of the reg entry iospace in device tree > * @np: struct device_node * of the IP block's device node in the DT data > * > - * Cache the virtual address used by the MPU to access this IP block's > - * registers. This address is needed early so the OCP registers that > - * are part of the device's address space can be ioremapped properly. > + * Cache the interconnect target port and the virtual address used by > + * the MPU to access this IP block's registers. The address is needed > + * early so the OCP registers that are part of the device's address > + * space can be ioremapped properly. The presence or absence of the > + * interconnect target port also indicates whether the hwmod code > + * should wait for the IP block to indicate readiness after it is > + * enabled. > * > * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and > * -ENXIO on absent or invalid register target address space. > @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, > if (oh->_int_flags & _HWMOD_NO_MPU_PORT) > return -ENXIO; > > + /* > + * If there's no need for the hwmod code to read or write to > + * the IP block registers, bail out early before the ioremap() > + */ > + if (!oh->class->sysc) > + return 0; > + > mem = _find_mpu_rt_addr_space(oh); > if (!mem) { > pr_debug("omap_hwmod: %s: no MPU register target found\n", > @@ -2451,14 +2464,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); This is the real piece that fixes the issue. > > r = _init_clocks(oh, NULL); > if (r < 0) { > I've tested this patch on am43x-gp-evm, and it seems to fix the issue although with some unpleasant warning messages. So if we can get rid of the WARN() you can put my Acked-by: Roger Quadros <rogerq@ti.com> cheers, -roger
Hi Suman, thanks for pitching in on this! On Tue, 6 Jan 2015, Suman Anna wrote: > You have removed the return from the above block on failure. If any DT > entry doesn't have the reg property, this will hang the kernel boot. > Just remove the "reg" entry from any of the existing DT, and you will > run into the issue, this is what 6423d6df1440 ("ARM: OMAP2+: hwmod: > check for module address space during init") fixed. Seems like that's the problem that we need to track down, then. If a hwmod has no MPU-accessible registers, it should still be possible to init the clocks for the device, etc. ... > Also, are you sure you want to turn the WARN into a pr_debug, it won't > even show during the kernel boot log if the reg base is missing. No, I'm not sure :-) I guess it depends how many hwmods we'll have with no MPU-accessible registers. We don't seem to have address ranges for the interconnects defined; we could fix that fairly easily. - Paul
Hi Suman On Tue, 6 Jan 2015, Suman Anna wrote: > Looking at your rc3 log @ > http://www.pwsan.com/omap/testlogs/test_v3.19-rc3/20150105224749/boot/4460varsomom/, > I do see a missing reg entry for mailbox, and that's the reason for your > hang because of the missing bail out in your new patch. > > [ 0.261932] ------------[ cut here ]------------ > [ 0.261962] WARNING: CPU: 0 PID: 1 at > arch/arm/mach-omap2/omap_hwmod.c:2458 _init+0x3a0/0x3f0() > [ 0.261962] omap_hwmod: mailbox: doesn't have mpu register target base > ... > ... > [ 0.262329] ---[ end trace a1be72591db4662e ]--- > > Now that said, this is weird, since the reg property for mailbox is > defined in the base omap4.dtsi and should be inherited by the 4460 > VAR-SOM-OM, unless you are booting with an old dtb. Yeah, it doesn't make sense. I'm pretty sure I'm using the right DTB here, since that board is booting with a concatenated uImage + DTB. Well, at least there's a good test platform here. - Paul
On Wed, 7 Jan 2015, Roger Quadros wrote: > > From: Paul Walmsley <paul@pwsan.com> > > Date: Mon, 5 Jan 2015 15:49:57 -0700 > > Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header > > registers. Experimental. > > > > --- > > arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ > > 1 file changed, 21 insertions(+), 12 deletions(-) > > > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > > index cbb908dc5cf0..03df8833d399 100644 > > --- a/arch/arm/mach-omap2/omap_hwmod.c > > +++ b/arch/arm/mach-omap2/omap_hwmod.c > > @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) > > pr_debug("omap_hwmod: %s: resetting\n", oh->name); > > > > if (oh->class->reset) { > > + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", > > + oh->name); > > Not part of $subject. Hmm, how do you mean? If we skip the ioremap(), wouldn't you like to know before some custom reset code gets called that pretty much always depends on the ioremap() succeeding? :-) > > r = oh->class->reset(oh); > > } else { > > if (oh->rst_lines_cnt > 0) { > > @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, > > } > > > > /** > > - * _init_mpu_rt_base - populate the virtual address for a hwmod > > + * _init_mpu_rt_base - populate the MPU port and virtual address > > * @oh: struct omap_hwmod * to locate the virtual address > > * @data: (unused, caller should pass NULL) > > * @index: index of the reg entry iospace in device tree > > * @np: struct device_node * of the IP block's device node in the DT data > > * > > - * Cache the virtual address used by the MPU to access this IP block's > > - * registers. This address is needed early so the OCP registers that > > - * are part of the device's address space can be ioremapped properly. > > + * Cache the interconnect target port and the virtual address used by > > + * the MPU to access this IP block's registers. The address is needed > > + * early so the OCP registers that are part of the device's address > > + * space can be ioremapped properly. The presence or absence of the > > + * interconnect target port also indicates whether the hwmod code > > + * should wait for the IP block to indicate readiness after it is > > + * enabled. > > * > > * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and > > * -ENXIO on absent or invalid register target address space. > > @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, > > if (oh->_int_flags & _HWMOD_NO_MPU_PORT) > > return -ENXIO; > > > > + /* > > + * If there's no need for the hwmod code to read or write to > > + * the IP block registers, bail out early before the ioremap() > > + */ > > + if (!oh->class->sysc) > > + return 0; > > + > > mem = _find_mpu_rt_addr_space(oh); > > if (!mem) { > > pr_debug("omap_hwmod: %s: no MPU register target found\n", > > @@ -2451,14 +2464,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); > > This is the real piece that fixes the issue. > > > > > r = _init_clocks(oh, NULL); > > if (r < 0) { > > > > I've tested this patch on am43x-gp-evm, and it seems to fix the issue > although with some unpleasant warning messages. Could you paste those in? - Paul
Hi Paul, On 01/13/2015 05:29 PM, Paul Walmsley wrote: > Hi Suman, > > thanks for pitching in on this! > > On Tue, 6 Jan 2015, Suman Anna wrote: > >> You have removed the return from the above block on failure. If any DT >> entry doesn't have the reg property, this will hang the kernel boot. >> Just remove the "reg" entry from any of the existing DT, and you will >> run into the issue, this is what 6423d6df1440 ("ARM: OMAP2+: hwmod: >> check for module address space during init") fixed. > > Seems like that's the problem that we need to track down, then. If a > hwmod has no MPU-accessible registers, it should still be possible to > init the clocks for the device, etc. ... Yes true, and I should have rephrased above statement a little better - its for modules with sysc but with no reg property to supply the base for the module's SYSCONFIG or SYSSTATUS registers. The commit 6423d6df1440 has the explanation for the hang. > >> Also, are you sure you want to turn the WARN into a pr_debug, it won't >> even show during the kernel boot log if the reg base is missing. > > No, I'm not sure :-) I guess it depends how many hwmods we'll have with > no MPU-accessible registers. We don't seem to have address ranges for the > interconnects defined; we could fix that fairly easily. The WARN_ON previously was to throw a eye-catchy print for the case where hwmods have sysc but no address space defined (is an error usually, but this is what we run into during the DT conversion of a device as the hwmod and DTS changes come in through separate topic branches). I still think that the sysc check should be before the check for _HWMOD_NO_MPU_PORT, a module with sysc mandates it has an MPU port. regards Suman
On 14/01/15 01:46, Paul Walmsley wrote: > On Wed, 7 Jan 2015, Roger Quadros wrote: > >>> From: Paul Walmsley <paul@pwsan.com> >>> Date: Mon, 5 Jan 2015 15:49:57 -0700 >>> Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header >>> registers. Experimental. >>> >>> --- >>> arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ >>> 1 file changed, 21 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c >>> index cbb908dc5cf0..03df8833d399 100644 >>> --- a/arch/arm/mach-omap2/omap_hwmod.c >>> +++ b/arch/arm/mach-omap2/omap_hwmod.c >>> @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) >>> pr_debug("omap_hwmod: %s: resetting\n", oh->name); >>> >>> if (oh->class->reset) { >>> + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", >>> + oh->name); >> >> Not part of $subject. > > Hmm, how do you mean? If we skip the ioremap(), wouldn't you like to know > before some custom reset code gets called that pretty much always depends > on the ioremap() succeeding? :-) Ah yes. you are right. > >>> r = oh->class->reset(oh); >>> } else { >>> if (oh->rst_lines_cnt > 0) { >>> @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, >>> } >>> >>> /** >>> - * _init_mpu_rt_base - populate the virtual address for a hwmod >>> + * _init_mpu_rt_base - populate the MPU port and virtual address >>> * @oh: struct omap_hwmod * to locate the virtual address >>> * @data: (unused, caller should pass NULL) >>> * @index: index of the reg entry iospace in device tree >>> * @np: struct device_node * of the IP block's device node in the DT data >>> * >>> - * Cache the virtual address used by the MPU to access this IP block's >>> - * registers. This address is needed early so the OCP registers that >>> - * are part of the device's address space can be ioremapped properly. >>> + * Cache the interconnect target port and the virtual address used by >>> + * the MPU to access this IP block's registers. The address is needed >>> + * early so the OCP registers that are part of the device's address >>> + * space can be ioremapped properly. The presence or absence of the >>> + * interconnect target port also indicates whether the hwmod code >>> + * should wait for the IP block to indicate readiness after it is >>> + * enabled. >>> * >>> * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and >>> * -ENXIO on absent or invalid register target address space. >>> @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, >>> if (oh->_int_flags & _HWMOD_NO_MPU_PORT) >>> return -ENXIO; >>> >>> + /* >>> + * If there's no need for the hwmod code to read or write to >>> + * the IP block registers, bail out early before the ioremap() >>> + */ >>> + if (!oh->class->sysc) >>> + return 0; >>> + >>> mem = _find_mpu_rt_addr_space(oh); >>> if (!mem) { >>> pr_debug("omap_hwmod: %s: no MPU register target found\n", >>> @@ -2451,14 +2464,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); >> >> This is the real piece that fixes the issue. >> >>> >>> r = _init_clocks(oh, NULL); >>> if (r < 0) { >>> >> >> I've tested this patch on am43x-gp-evm, and it seems to fix the issue >> although with some unpleasant warning messages. > > Could you paste those in? > I can't reproduce what I saw earlier. Sorry for the noise. cheers, -roger
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index cbb908dc5cf0..03df8833d399 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -1938,6 +1938,8 @@ static int _reset(struct omap_hwmod *oh) pr_debug("omap_hwmod: %s: resetting\n", oh->name); if (oh->class->reset) { + WARN(!oh->_mpu_rt_va, "Attempt to call custom reset with no MPU register target ioremapped: %s", + oh->name); r = oh->class->reset(oh); } else { if (oh->rst_lines_cnt > 0) { @@ -2358,15 +2360,19 @@ static int of_dev_hwmod_lookup(struct device_node *np, } /** - * _init_mpu_rt_base - populate the virtual address for a hwmod + * _init_mpu_rt_base - populate the MPU port and virtual address * @oh: struct omap_hwmod * to locate the virtual address * @data: (unused, caller should pass NULL) * @index: index of the reg entry iospace in device tree * @np: struct device_node * of the IP block's device node in the DT data * - * Cache the virtual address used by the MPU to access this IP block's - * registers. This address is needed early so the OCP registers that - * are part of the device's address space can be ioremapped properly. + * Cache the interconnect target port and the virtual address used by + * the MPU to access this IP block's registers. The address is needed + * early so the OCP registers that are part of the device's address + * space can be ioremapped properly. The presence or absence of the + * interconnect target port also indicates whether the hwmod code + * should wait for the IP block to indicate readiness after it is + * enabled. * * Returns 0 on success, -EINVAL if an invalid hwmod is passed, and * -ENXIO on absent or invalid register target address space. @@ -2385,6 +2391,13 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data, if (oh->_int_flags & _HWMOD_NO_MPU_PORT) return -ENXIO; + /* + * If there's no need for the hwmod code to read or write to + * the IP block registers, bail out early before the ioremap() + */ + if (!oh->class->sysc) + return 0; + mem = _find_mpu_rt_addr_space(oh); if (!mem) { pr_debug("omap_hwmod: %s: no MPU register target found\n", @@ -2451,14 +2464,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) {
Roger, Lokesh, could you try this one instead? It passes all the basic tests here except it does not boot on the 4460 VAR-SOM-OM - unclear why at this point - but it would be good to see if it works on your AM4372 boards, since I don't have that one. Test logs are here: http://www.pwsan.com/omap/testlogs/hwmod_skip_only_remap_v3.19-rc/20150105171744/ - Paul From 4f2e13bd2181e0ebede3aabc484aa2339830748a Mon Sep 17 00:00:00 2001 From: Paul Walmsley <paul@pwsan.com> Date: Mon, 5 Jan 2015 15:49:57 -0700 Subject: [PATCH] Only skip ioremap() if IP block does not have OCP header registers. Experimental. --- arch/arm/mach-omap2/omap_hwmod.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)