[PATCH] ARM: OMAP2+: hwmod: Fix _wait_target_ready() for hwmods without sysc

[Suman Anna]

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 at 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) {
>>>>
>>>
>>
>