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

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

[Roger Quadros]

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.

This can potentially cause a bus access error if the module is accessed
before the module is ready.

Get rid of the redundant _find_mpu_rt_port() check from the _wait_target_ready()
funcion for all the SoCs. The following PRCM register access that checks the
module ready state has nothing to do with module's SYSCONFIG or mpu_rt_port.

e.g. this fixes the below DCAN bus access error on AM437x-gp-evm.

[   16.672978] ------------[ cut here ]------------
[   16.677885] WARNING: CPU: 0 PID: 1580 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x234/0x35c()
[   16.687946] 44000000.ocp:L3 Custom Error: MASTER M2 (64-bit) TARGET L4_PER_0 (Read): Data Access in User mode during Functional access
[   16.700654] Modules linked in: xhci_hcd btwilink ti_vpfe dwc3 videobuf2_core ov2659 bluetooth v4l2_common videodev ti_am335x_adc kfifo_buf industrialio c_can_platform videobuf2_dma_contig media snd_soc_tlv320aic3x pixcir_i2c_ts c_can dc
[   16.731144] CPU: 0 PID: 1580 Comm: rpc.statd Not tainted 3.14.26-02561-gf733aa036398 #180
[   16.739747] Backtrace:
[   16.742336] [<c0011108>] (dump_backtrace) from [<c00112a4>] (show_stack+0x18/0x1c)
[   16.750285]  r6:00000093 r5:00000009 r4:eab5b8a8 r3:00000000
[   16.756252] [<c001128c>] (show_stack) from [<c05a4418>] (dump_stack+0x20/0x28)
[   16.763870] [<c05a43f8>] (dump_stack) from [<c0037120>] (warn_slowpath_common+0x6c/0x8c)
[   16.772408] [<c00370b4>] (warn_slowpath_common) from [<c00371e4>] (warn_slowpath_fmt+0x38/0x40)
[   16.781550]  r8:c05d1f90 r7:c0730844 r6:c0730448 r5:80080003 r4:ed0cd210
[   16.788626] [<c00371b0>] (warn_slowpath_fmt) from [<c027fa94>] (l3_interrupt_handler+0x234/0x35c)
[   16.797968]  r3:ed0cd480 r2:c0730508
[   16.801747] [<c027f860>] (l3_interrupt_handler) from [<c0063758>] (handle_irq_event_percpu+0x54/0x1bc)
[   16.811533]  r10:ed005600 r9:c084855b r8:0000002a r7:00000000 r6:00000000 r5:0000002a
[   16.819780]  r4:ed0e6d80
[   16.822453] [<c0063704>] (handle_irq_event_percpu) from [<c00638f0>] (handle_irq_event+0x30/0x40)
[   16.831789]  r10:eb2b6938 r9:eb2b6960 r8:bf011420 r7:fa240100 r6:00000000 r5:0000002a
[   16.840052]  r4:ed005600
[   16.842744] [<c00638c0>] (handle_irq_event) from [<c00661d8>] (handle_fasteoi_irq+0x74/0x128)
[   16.851702]  r4:ed005600 r3:00000000
[   16.855479] [<c0066164>] (handle_fasteoi_irq) from [<c0063068>] (generic_handle_irq+0x28/0x38)
[   16.864523]  r4:0000002a r3:c0066164
[   16.868294] [<c0063040>] (generic_handle_irq) from [<c000ef60>] (handle_IRQ+0x38/0x8c)
[   16.876612]  r4:c081c640 r3:00000202
[   16.880380] [<c000ef28>] (handle_IRQ) from [<c00084f0>] (gic_handle_irq+0x30/0x5c)
[   16.888328]  r6:eab5ba38 r5:c0804460 r4:fa24010c r3:00000100
[   16.894303] [<c00084c0>] (gic_handle_irq) from [<c05a8d80>] (__irq_svc+0x40/0x50)
[   16.902193] Exception stack(0xeab5ba38 to 0xeab5ba80)
[   16.907499] ba20:                                                       00000000 00000006
[   16.916108] ba40: fa1d0000 fa1d0008 ed3d3000 eab5bab4 ed3d3460 c0842af4 bf011420 eb2b6960
[   16.924716] ba60: eb2b6938 eab5ba8c eab5ba90 eab5ba80 bf035220 bf07702c 600f0013 ffffffff
[   16.933317]  r7:eab5ba6c r6:ffffffff r5:600f0013 r4:bf07702c
[   16.939317] [<bf077000>] (c_can_plat_read_reg_aligned_to_16bit [c_can_platform]) from [<bf035220>] (c_can_get_berr_counter+0x38/0x64 [c_can])
[   16.952696] [<bf0351e8>] (c_can_get_berr_counter [c_can]) from [<bf010294>] (can_fill_info+0x124/0x15c [can_dev])
[   16.963480]  r5:ec8c9740 r4:ed3d3000
[   16.967253] [<bf010170>] (can_fill_info [can_dev]) from [<c0502fa8>] (rtnl_fill_ifinfo+0x58c/0x8fc)
[   16.976749]  r6:ec8c9740 r5:ed3d3000 r4:eb2b6780
[   16.981613] [<c0502a1c>] (rtnl_fill_ifinfo) from [<c0503408>] (rtnl_dump_ifinfo+0xf0/0x1dc)
[   16.990401]  r10:ec8c9740 r9:00000000 r8:00000000 r7:00000000 r6:ebd4d1b4 r5:ed3d3000
[   16.998671]  r4:00000000
[   17.001342] [<c0503318>] (rtnl_dump_ifinfo) from [<c050e6e4>] (netlink_dump+0xa8/0x1e0)
[   17.009772]  r10:00000000 r9:00000000 r8:c0503318 r7:ebf3e6c0 r6:ebd4d1b4 r5:ec8c9740
[   17.018050]  r4:ebd4d000
[   17.020714] [<c050e63c>] (netlink_dump) from [<c050ec10>] (__netlink_dump_start+0x104/0x154)
[   17.029591]  r6:eab5bd34 r5:ec8c9980 r4:ebd4d000
[   17.034454] [<c050eb0c>] (__netlink_dump_start) from [<c0505604>] (rtnetlink_rcv_msg+0x110/0x1f4)
[   17.043778]  r7:00000000 r6:ec8c9980 r5:00000f40 r4:ebf3e6c0
[   17.049743] [<c05054f4>] (rtnetlink_rcv_msg) from [<c05108e8>] (netlink_rcv_skb+0xb4/0xc8)
[   17.058449]  r8:eab5bdac r7:ec8c9980 r6:c05054f4 r5:ec8c9980 r4:ebf3e6c0
[   17.065534] [<c0510834>] (netlink_rcv_skb) from [<c0504134>] (rtnetlink_rcv+0x24/0x2c)
[   17.073854]  r6:ebd4d000 r5:00000014 r4:ec8c9980 r3:c0504110
[   17.079846] [<c0504110>] (rtnetlink_rcv) from [<c05102ac>] (netlink_unicast+0x180/0x1ec)
[   17.088363]  r4:ed0c6800 r3:c0504110
[   17.092113] [<c051012c>] (netlink_unicast) from [<c0510670>] (netlink_sendmsg+0x2ac/0x380)
[   17.100813]  r10:00000000 r8:00000008 r7:ec8c9980 r6:ebd4d000 r5:eab5be70 r4:eab5bee4
[   17.109083] [<c05103c4>] (netlink_sendmsg) from [<c04dfdb4>] (sock_sendmsg+0x90/0xb0)
[   17.117305]  r10:00000000 r9:eab5a000 r8:becdda3c r7:0000000c r6:ea978400 r5:eab5be70
[   17.125563]  r4:c05103c4
[   17.128225] [<c04dfd24>] (sock_sendmsg) from [<c04e1c28>] (SyS_sendto+0xb8/0xdc)
[   17.136001]  r6:becdda5c r5:00000014 r4:ecd37040
[   17.140876] [<c04e1b70>] (SyS_sendto) from [<c000e680>] (ret_fast_syscall+0x0/0x30)
[   17.148923]  r10:00000000 r8:c000e804 r7:00000122 r6:becdda5c r5:0000000c r4:becdda5c
[   17.157169] ---[ end trace 2b71e15b38f58bad ]---

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 716247e..3afcc65 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2943,9 +2943,6 @@ static int _omap2xxx_wait_target_ready(struct omap_hwmod *oh)
 	if (oh->flags & HWMOD_NO_IDLEST)
 		return 0;
 
-	if (!_find_mpu_rt_port(oh))
-		return 0;
-
 	/* XXX check module SIDLEMODE, hardreset status, enabled clocks */
 
 	return omap2xxx_cm_wait_module_ready(oh->prcm.omap2.module_offs,
@@ -2970,9 +2967,6 @@ static int _omap3xxx_wait_target_ready(struct omap_hwmod *oh)
 	if (oh->flags & HWMOD_NO_IDLEST)
 		return 0;
 
-	if (!_find_mpu_rt_port(oh))
-		return 0;
-
 	/* XXX check module SIDLEMODE, hardreset status, enabled clocks */
 
 	return omap3xxx_cm_wait_module_ready(oh->prcm.omap2.module_offs,
@@ -2997,9 +2991,6 @@ static int _omap4_wait_target_ready(struct omap_hwmod *oh)
 	if (oh->flags & HWMOD_NO_IDLEST || !oh->clkdm)
 		return 0;
 
-	if (!_find_mpu_rt_port(oh))
-		return 0;
-
 	/* XXX check module SIDLEMODE, hardreset status */
 
 	return omap4_cminst_wait_module_ready(oh->clkdm->prcm_partition,
@@ -3025,9 +3016,6 @@ static int _am33xx_wait_target_ready(struct omap_hwmod *oh)
 	if (oh->flags & HWMOD_NO_IDLEST)
 		return 0;
 
-	if (!_find_mpu_rt_port(oh))
-		return 0;
-
 	/* XXX check module SIDLEMODE, hardreset status */
 
 	return am33xx_cm_wait_module_ready(oh->clkdm->cm_inst,
-- 
2.1.0

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

[Roger Quadros]

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.
> 
> This can potentially cause a bus access error if the module is accessed
> before the module is ready.
> 
> Get rid of the redundant _find_mpu_rt_port() check from the _wait_target_ready()
> funcion for all the SoCs. The following PRCM register access that checks the
> module ready state has nothing to do with module's SYSCONFIG or mpu_rt_port.
> 
> e.g. this fixes the below DCAN bus access error on AM437x-gp-evm.
> 
> [   16.672978] ------------[ cut here ]------------
> [   16.677885] WARNING: CPU: 0 PID: 1580 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x234/0x35c()
> [   16.687946] 44000000.ocp:L3 Custom Error: MASTER M2 (64-bit) TARGET L4_PER_0 (Read): Data Access in User mode during Functional access
> [   16.700654] Modules linked in: xhci_hcd btwilink ti_vpfe dwc3 videobuf2_core ov2659 bluetooth v4l2_common videodev ti_am335x_adc kfifo_buf industrialio c_can_platform videobuf2_dma_contig media snd_soc_tlv320aic3x pixcir_i2c_ts c_can dc
> [   16.731144] CPU: 0 PID: 1580 Comm: rpc.statd Not tainted 3.14.26-02561-gf733aa036398 #180
> [   16.739747] Backtrace:
> [   16.742336] [<c0011108>] (dump_backtrace) from [<c00112a4>] (show_stack+0x18/0x1c)
> [   16.750285]  r6:00000093 r5:00000009 r4:eab5b8a8 r3:00000000
> [   16.756252] [<c001128c>] (show_stack) from [<c05a4418>] (dump_stack+0x20/0x28)
> [   16.763870] [<c05a43f8>] (dump_stack) from [<c0037120>] (warn_slowpath_common+0x6c/0x8c)
> [   16.772408] [<c00370b4>] (warn_slowpath_common) from [<c00371e4>] (warn_slowpath_fmt+0x38/0x40)
> [   16.781550]  r8:c05d1f90 r7:c0730844 r6:c0730448 r5:80080003 r4:ed0cd210
> [   16.788626] [<c00371b0>] (warn_slowpath_fmt) from [<c027fa94>] (l3_interrupt_handler+0x234/0x35c)
> [   16.797968]  r3:ed0cd480 r2:c0730508
> [   16.801747] [<c027f860>] (l3_interrupt_handler) from [<c0063758>] (handle_irq_event_percpu+0x54/0x1bc)
> [   16.811533]  r10:ed005600 r9:c084855b r8:0000002a r7:00000000 r6:00000000 r5:0000002a
> [   16.819780]  r4:ed0e6d80
> [   16.822453] [<c0063704>] (handle_irq_event_percpu) from [<c00638f0>] (handle_irq_event+0x30/0x40)
> [   16.831789]  r10:eb2b6938 r9:eb2b6960 r8:bf011420 r7:fa240100 r6:00000000 r5:0000002a
> [   16.840052]  r4:ed005600
> [   16.842744] [<c00638c0>] (handle_irq_event) from [<c00661d8>] (handle_fasteoi_irq+0x74/0x128)
> [   16.851702]  r4:ed005600 r3:00000000
> [   16.855479] [<c0066164>] (handle_fasteoi_irq) from [<c0063068>] (generic_handle_irq+0x28/0x38)
> [   16.864523]  r4:0000002a r3:c0066164
> [   16.868294] [<c0063040>] (generic_handle_irq) from [<c000ef60>] (handle_IRQ+0x38/0x8c)
> [   16.876612]  r4:c081c640 r3:00000202
> [   16.880380] [<c000ef28>] (handle_IRQ) from [<c00084f0>] (gic_handle_irq+0x30/0x5c)
> [   16.888328]  r6:eab5ba38 r5:c0804460 r4:fa24010c r3:00000100
> [   16.894303] [<c00084c0>] (gic_handle_irq) from [<c05a8d80>] (__irq_svc+0x40/0x50)
> [   16.902193] Exception stack(0xeab5ba38 to 0xeab5ba80)
> [   16.907499] ba20:                                                       00000000 00000006
> [   16.916108] ba40: fa1d0000 fa1d0008 ed3d3000 eab5bab4 ed3d3460 c0842af4 bf011420 eb2b6960
> [   16.924716] ba60: eb2b6938 eab5ba8c eab5ba90 eab5ba80 bf035220 bf07702c 600f0013 ffffffff
> [   16.933317]  r7:eab5ba6c r6:ffffffff r5:600f0013 r4:bf07702c
> [   16.939317] [<bf077000>] (c_can_plat_read_reg_aligned_to_16bit [c_can_platform]) from [<bf035220>] (c_can_get_berr_counter+0x38/0x64 [c_can])
> [   16.952696] [<bf0351e8>] (c_can_get_berr_counter [c_can]) from [<bf010294>] (can_fill_info+0x124/0x15c [can_dev])
> [   16.963480]  r5:ec8c9740 r4:ed3d3000
> [   16.967253] [<bf010170>] (can_fill_info [can_dev]) from [<c0502fa8>] (rtnl_fill_ifinfo+0x58c/0x8fc)
> [   16.976749]  r6:ec8c9740 r5:ed3d3000 r4:eb2b6780
> [   16.981613] [<c0502a1c>] (rtnl_fill_ifinfo) from [<c0503408>] (rtnl_dump_ifinfo+0xf0/0x1dc)
> [   16.990401]  r10:ec8c9740 r9:00000000 r8:00000000 r7:00000000 r6:ebd4d1b4 r5:ed3d3000
> [   16.998671]  r4:00000000
> [   17.001342] [<c0503318>] (rtnl_dump_ifinfo) from [<c050e6e4>] (netlink_dump+0xa8/0x1e0)
> [   17.009772]  r10:00000000 r9:00000000 r8:c0503318 r7:ebf3e6c0 r6:ebd4d1b4 r5:ec8c9740
> [   17.018050]  r4:ebd4d000
> [   17.020714] [<c050e63c>] (netlink_dump) from [<c050ec10>] (__netlink_dump_start+0x104/0x154)
> [   17.029591]  r6:eab5bd34 r5:ec8c9980 r4:ebd4d000
> [   17.034454] [<c050eb0c>] (__netlink_dump_start) from [<c0505604>] (rtnetlink_rcv_msg+0x110/0x1f4)
> [   17.043778]  r7:00000000 r6:ec8c9980 r5:00000f40 r4:ebf3e6c0
> [   17.049743] [<c05054f4>] (rtnetlink_rcv_msg) from [<c05108e8>] (netlink_rcv_skb+0xb4/0xc8)
> [   17.058449]  r8:eab5bdac r7:ec8c9980 r6:c05054f4 r5:ec8c9980 r4:ebf3e6c0
> [   17.065534] [<c0510834>] (netlink_rcv_skb) from [<c0504134>] (rtnetlink_rcv+0x24/0x2c)
> [   17.073854]  r6:ebd4d000 r5:00000014 r4:ec8c9980 r3:c0504110
> [   17.079846] [<c0504110>] (rtnetlink_rcv) from [<c05102ac>] (netlink_unicast+0x180/0x1ec)
> [   17.088363]  r4:ed0c6800 r3:c0504110
> [   17.092113] [<c051012c>] (netlink_unicast) from [<c0510670>] (netlink_sendmsg+0x2ac/0x380)
> [   17.100813]  r10:00000000 r8:00000008 r7:ec8c9980 r6:ebd4d000 r5:eab5be70 r4:eab5bee4
> [   17.109083] [<c05103c4>] (netlink_sendmsg) from [<c04dfdb4>] (sock_sendmsg+0x90/0xb0)
> [   17.117305]  r10:00000000 r9:eab5a000 r8:becdda3c r7:0000000c r6:ea978400 r5:eab5be70
> [   17.125563]  r4:c05103c4
> [   17.128225] [<c04dfd24>] (sock_sendmsg) from [<c04e1c28>] (SyS_sendto+0xb8/0xdc)
> [   17.136001]  r6:becdda5c r5:00000014 r4:ecd37040
> [   17.140876] [<c04e1b70>] (SyS_sendto) from [<c000e680>] (ret_fast_syscall+0x0/0x30)
> [   17.148923]  r10:00000000 r8:c000e804 r7:00000122 r6:becdda5c r5:0000000c r4:becdda5c
> [   17.157169] ---[ end trace 2b71e15b38f58bad ]---
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 716247e..3afcc65 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2943,9 +2943,6 @@ static int _omap2xxx_wait_target_ready(struct omap_hwmod *oh)
>  	if (oh->flags & HWMOD_NO_IDLEST)
>  		return 0;
>  
> -	if (!_find_mpu_rt_port(oh))
> -		return 0;
> -
>  	/* XXX check module SIDLEMODE, hardreset status, enabled clocks */
>  
>  	return omap2xxx_cm_wait_module_ready(oh->prcm.omap2.module_offs,
> @@ -2970,9 +2967,6 @@ static int _omap3xxx_wait_target_ready(struct omap_hwmod *oh)
>  	if (oh->flags & HWMOD_NO_IDLEST)
>  		return 0;
>  
> -	if (!_find_mpu_rt_port(oh))
> -		return 0;
> -
>  	/* XXX check module SIDLEMODE, hardreset status, enabled clocks */
>  
>  	return omap3xxx_cm_wait_module_ready(oh->prcm.omap2.module_offs,
> @@ -2997,9 +2991,6 @@ static int _omap4_wait_target_ready(struct omap_hwmod *oh)
>  	if (oh->flags & HWMOD_NO_IDLEST || !oh->clkdm)
>  		return 0;
>  
> -	if (!_find_mpu_rt_port(oh))
> -		return 0;
> -
>  	/* XXX check module SIDLEMODE, hardreset status */
>  
>  	return omap4_cminst_wait_module_ready(oh->clkdm->prcm_partition,
> @@ -3025,9 +3016,6 @@ static int _am33xx_wait_target_ready(struct omap_hwmod *oh)
>  	if (oh->flags & HWMOD_NO_IDLEST)
>  		return 0;
>  
> -	if (!_find_mpu_rt_port(oh))
> -		return 0;
> -
>  	/* XXX check module SIDLEMODE, hardreset status */
>  
>  	return am33xx_cm_wait_module_ready(oh->clkdm->cm_inst,
> 

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

[Lokesh Vutla]

Hi Roger,
On Thursday 18 December 2014 09:22 PM, 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.
>>
>> This can potentially cause a bus access error if the module is accessed
>> before the module is ready.
>>
>> Get rid of the redundant _find_mpu_rt_port() check from the _wait_target_ready()
>> funcion for all the SoCs. The following PRCM register access that checks the
>> module ready state has nothing to do with module's SYSCONFIG or mpu_rt_port.
Yes, makes sense. This patch looks good to me.
Tested this on AM437x-gp-evm.

Tested-by: Lokesh Vutla <lokeshvutla@ti.com>

May be good idea to warn every time if enabling of module is failed?
Unrelated to this patch though.

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 4c6b7b2..db67b66 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2243,7 +2243,7 @@ static int _enable(struct omap_hwmod *oh)
 		if (soc_ops.disable_module)
 			soc_ops.disable_module(oh);
 		_disable_clocks(oh);
-		pr_debug("omap_hwmod: %s: _wait_target_ready: %d\n",
+		pr_warn("omap_hwmod: %s: _wait_target_ready failed : %d\n",
 			 oh->name, r);
 
 		if (oh->clkdm)


Thanks and regards,
Lokesh

>>
>> e.g. this fixes the below DCAN bus access error on AM437x-gp-evm.
>>
>> [   16.672978] ------------[ cut here ]------------
>> [   16.677885] WARNING: CPU: 0 PID: 1580 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x234/0x35c()
>> [   16.687946] 44000000.ocp:L3 Custom Error: MASTER M2 (64-bit) TARGET L4_PER_0 (Read): Data Access in User mode during Functional access
>> [   16.700654] Modules linked in: xhci_hcd btwilink ti_vpfe dwc3 videobuf2_core ov2659 bluetooth v4l2_common videodev ti_am335x_adc kfifo_buf industrialio c_can_platform videobuf2_dma_contig media snd_soc_tlv320aic3x pixcir_i2c_ts c_can dc
>> [   16.731144] CPU: 0 PID: 1580 Comm: rpc.statd Not tainted 3.14.26-02561-gf733aa036398 #180
>> [   16.739747] Backtrace:
>> [   16.742336] [<c0011108>] (dump_backtrace) from [<c00112a4>] (show_stack+0x18/0x1c)
>> [   16.750285]  r6:00000093 r5:00000009 r4:eab5b8a8 r3:00000000
>> [   16.756252] [<c001128c>] (show_stack) from [<c05a4418>] (dump_stack+0x20/0x28)
>> [   16.763870] [<c05a43f8>] (dump_stack) from [<c0037120>] (warn_slowpath_common+0x6c/0x8c)
>> [   16.772408] [<c00370b4>] (warn_slowpath_common) from [<c00371e4>] (warn_slowpath_fmt+0x38/0x40)
>> [   16.781550]  r8:c05d1f90 r7:c0730844 r6:c0730448 r5:80080003 r4:ed0cd210
>> [   16.788626] [<c00371b0>] (warn_slowpath_fmt) from [<c027fa94>] (l3_interrupt_handler+0x234/0x35c)
>> [   16.797968]  r3:ed0cd480 r2:c0730508
>> [   16.801747] [<c027f860>] (l3_interrupt_handler) from [<c0063758>] (handle_irq_event_percpu+0x54/0x1bc)
>> [   16.811533]  r10:ed005600 r9:c084855b r8:0000002a r7:00000000 r6:00000000 r5:0000002a
>> [   16.819780]  r4:ed0e6d80
>> [   16.822453] [<c0063704>] (handle_irq_event_percpu) from [<c00638f0>] (handle_irq_event+0x30/0x40)
>> [   16.831789]  r10:eb2b6938 r9:eb2b6960 r8:bf011420 r7:fa240100 r6:00000000 r5:0000002a
>> [   16.840052]  r4:ed005600
>> [   16.842744] [<c00638c0>] (handle_irq_event) from [<c00661d8>] (handle_fasteoi_irq+0x74/0x128)
>> [   16.851702]  r4:ed005600 r3:00000000
>> [   16.855479] [<c0066164>] (handle_fasteoi_irq) from [<c0063068>] (generic_handle_irq+0x28/0x38)
>> [   16.864523]  r4:0000002a r3:c0066164
>> [   16.868294] [<c0063040>] (generic_handle_irq) from [<c000ef60>] (handle_IRQ+0x38/0x8c)
>> [   16.876612]  r4:c081c640 r3:00000202
>> [   16.880380] [<c000ef28>] (handle_IRQ) from [<c00084f0>] (gic_handle_irq+0x30/0x5c)
>> [   16.888328]  r6:eab5ba38 r5:c0804460 r4:fa24010c r3:00000100
>> [   16.894303] [<c00084c0>] (gic_handle_irq) from [<c05a8d80>] (__irq_svc+0x40/0x50)
>> [   16.902193] Exception stack(0xeab5ba38 to 0xeab5ba80)
>> [   16.907499] ba20:                                                       00000000 00000006
>> [   16.916108] ba40: fa1d0000 fa1d0008 ed3d3000 eab5bab4 ed3d3460 c0842af4 bf011420 eb2b6960
>> [   16.924716] ba60: eb2b6938 eab5ba8c eab5ba90 eab5ba80 bf035220 bf07702c 600f0013 ffffffff
>> [   16.933317]  r7:eab5ba6c r6:ffffffff r5:600f0013 r4:bf07702c
>> [   16.939317] [<bf077000>] (c_can_plat_read_reg_aligned_to_16bit [c_can_platform]) from [<bf035220>] (c_can_get_berr_counter+0x38/0x64 [c_can])
>> [   16.952696] [<bf0351e8>] (c_can_get_berr_counter [c_can]) from [<bf010294>] (can_fill_info+0x124/0x15c [can_dev])
>> [   16.963480]  r5:ec8c9740 r4:ed3d3000
>> [   16.967253] [<bf010170>] (can_fill_info [can_dev]) from [<c0502fa8>] (rtnl_fill_ifinfo+0x58c/0x8fc)
>> [   16.976749]  r6:ec8c9740 r5:ed3d3000 r4:eb2b6780
>> [   16.981613] [<c0502a1c>] (rtnl_fill_ifinfo) from [<c0503408>] (rtnl_dump_ifinfo+0xf0/0x1dc)
>> [   16.990401]  r10:ec8c9740 r9:00000000 r8:00000000 r7:00000000 r6:ebd4d1b4 r5:ed3d3000
>> [   16.998671]  r4:00000000
>> [   17.001342] [<c0503318>] (rtnl_dump_ifinfo) from [<c050e6e4>] (netlink_dump+0xa8/0x1e0)
>> [   17.009772]  r10:00000000 r9:00000000 r8:c0503318 r7:ebf3e6c0 r6:ebd4d1b4 r5:ec8c9740
>> [   17.018050]  r4:ebd4d000
>> [   17.020714] [<c050e63c>] (netlink_dump) from [<c050ec10>] (__netlink_dump_start+0x104/0x154)
>> [   17.029591]  r6:eab5bd34 r5:ec8c9980 r4:ebd4d000
>> [   17.034454] [<c050eb0c>] (__netlink_dump_start) from [<c0505604>] (rtnetlink_rcv_msg+0x110/0x1f4)
>> [   17.043778]  r7:00000000 r6:ec8c9980 r5:00000f40 r4:ebf3e6c0
>> [   17.049743] [<c05054f4>] (rtnetlink_rcv_msg) from [<c05108e8>] (netlink_rcv_skb+0xb4/0xc8)
>> [   17.058449]  r8:eab5bdac r7:ec8c9980 r6:c05054f4 r5:ec8c9980 r4:ebf3e6c0
>> [   17.065534] [<c0510834>] (netlink_rcv_skb) from [<c0504134>] (rtnetlink_rcv+0x24/0x2c)
>> [   17.073854]  r6:ebd4d000 r5:00000014 r4:ec8c9980 r3:c0504110
>> [   17.079846] [<c0504110>] (rtnetlink_rcv) from [<c05102ac>] (netlink_unicast+0x180/0x1ec)
>> [   17.088363]  r4:ed0c6800 r3:c0504110
>> [   17.092113] [<c051012c>] (netlink_unicast) from [<c0510670>] (netlink_sendmsg+0x2ac/0x380)
>> [   17.100813]  r10:00000000 r8:00000008 r7:ec8c9980 r6:ebd4d000 r5:eab5be70 r4:eab5bee4
>> [   17.109083] [<c05103c4>] (netlink_sendmsg) from [<c04dfdb4>] (sock_sendmsg+0x90/0xb0)
>> [   17.117305]  r10:00000000 r9:eab5a000 r8:becdda3c r7:0000000c r6:ea978400 r5:eab5be70
>> [   17.125563]  r4:c05103c4
>> [   17.128225] [<c04dfd24>] (sock_sendmsg) from [<c04e1c28>] (SyS_sendto+0xb8/0xdc)
>> [   17.136001]  r6:becdda5c r5:00000014 r4:ecd37040
>> [   17.140876] [<c04e1b70>] (SyS_sendto) from [<c000e680>] (ret_fast_syscall+0x0/0x30)
>> [   17.148923]  r10:00000000 r8:c000e804 r7:00000122 r6:becdda5c r5:0000000c r4:becdda5c
>> [   17.157169] ---[ end trace 2b71e15b38f58bad ]---
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>  arch/arm/mach-omap2/omap_hwmod.c | 12 ------------
>>  1 file changed, 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>> index 716247e..3afcc65 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>> @@ -2943,9 +2943,6 @@ static int _omap2xxx_wait_target_ready(struct omap_hwmod *oh)
>>  	if (oh->flags & HWMOD_NO_IDLEST)
>>  		return 0;
>>  
>> -	if (!_find_mpu_rt_port(oh))
>> -		return 0;
>> -
>>  	/* XXX check module SIDLEMODE, hardreset status, enabled clocks */
>>  
>>  	return omap2xxx_cm_wait_module_ready(oh->prcm.omap2.module_offs,
>> @@ -2970,9 +2967,6 @@ static int _omap3xxx_wait_target_ready(struct omap_hwmod *oh)
>>  	if (oh->flags & HWMOD_NO_IDLEST)
>>  		return 0;
>>  
>> -	if (!_find_mpu_rt_port(oh))
>> -		return 0;
>> -
>>  	/* XXX check module SIDLEMODE, hardreset status, enabled clocks */
>>  
>>  	return omap3xxx_cm_wait_module_ready(oh->prcm.omap2.module_offs,
>> @@ -2997,9 +2991,6 @@ static int _omap4_wait_target_ready(struct omap_hwmod *oh)
>>  	if (oh->flags & HWMOD_NO_IDLEST || !oh->clkdm)
>>  		return 0;
>>  
>> -	if (!_find_mpu_rt_port(oh))
>> -		return 0;
>> -
>>  	/* XXX check module SIDLEMODE, hardreset status */
>>  
>>  	return omap4_cminst_wait_module_ready(oh->clkdm->prcm_partition,
>> @@ -3025,9 +3016,6 @@ static int _am33xx_wait_target_ready(struct omap_hwmod *oh)
>>  	if (oh->flags & HWMOD_NO_IDLEST)
>>  		return 0;
>>  
>> -	if (!_find_mpu_rt_port(oh))
>> -		return 0;
>> -
>>  	/* XXX check module SIDLEMODE, hardreset status */
>>  
>>  	return am33xx_cm_wait_module_ready(oh->clkdm->cm_inst,
>>
> 
> --
> 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
> 

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

[Roger Quadros]

Lokesh,

On 19/12/14 07:21, Lokesh Vutla wrote:
> Hi Roger,
> On Thursday 18 December 2014 09:22 PM, 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.
>>>
>>> This can potentially cause a bus access error if the module is accessed
>>> before the module is ready.
>>>
>>> Get rid of the redundant _find_mpu_rt_port() check from the _wait_target_ready()
>>> funcion for all the SoCs. The following PRCM register access that checks the
>>> module ready state has nothing to do with module's SYSCONFIG or mpu_rt_port.
> Yes, makes sense. This patch looks good to me.
> Tested this on AM437x-gp-evm.
> 
> Tested-by: Lokesh Vutla <lokeshvutla@ti.com>

Thanks.

> 
> May be good idea to warn every time if enabling of module is failed?
> Unrelated to this patch though.

Yes, failing to be ready is serious enough for a warning. Care to send a separate patch for that?

cheers,
-roger

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

[Tony Lindgren]

* Roger Quadros <rogerq@ti.com> [141219 01:08]:
> Lokesh,
> 
> On 19/12/14 07:21, Lokesh Vutla wrote:
> > Hi Roger,
> > On Thursday 18 December 2014 09:22 PM, 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.
> >>>
> >>> This can potentially cause a bus access error if the module is accessed
> >>> before the module is ready.
> >>>
> >>> Get rid of the redundant _find_mpu_rt_port() check from the _wait_target_ready()
> >>> funcion for all the SoCs. The following PRCM register access that checks the
> >>> module ready state has nothing to do with module's SYSCONFIG or mpu_rt_port.
> > Yes, makes sense. This patch looks good to me.
> > Tested this on AM437x-gp-evm.

Roger, if the modules don't have sysconfig registers, care to check
if we actually really need hwmod for those modules then?

I know hwmod is managing runtime PM gate clocks for devices with
clkctrl_offs. But if that's all we need hwmod for in the non-sysc
cases, then it might make sense to manage the gate clocks in the
clock framework directly instead for those devices.

Of course that's more of a long term project, but at least we should
be aware of the dependencies here :)

> > May be good idea to warn every time if enabling of module is failed?
> > Unrelated to this patch though.
> 
> Yes, failing to be ready is serious enough for a warning. Care to send a separate patch for that?

Yeah that sounds like a separate patch.

Regards,

Tony

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

[Paul Walmsley]

+ 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.

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

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

[Lokesh Vutla]

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

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

[Suman Anna]

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

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

[Paul Walmsley]

+ 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

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

[Paul Walmsley]

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

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

[santosh.shilimkar]

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

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

[Paul Walmsley]

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);
 		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) {
-- 
2.1.4

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

[Lokesh Vutla]

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

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

[Suman Anna]

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

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

[Suman Anna]

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

Re: [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@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) {
>>>>
>>>
>>
> 

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

[Roger Quadros]

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

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

[Paul Walmsley]

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

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

[Paul Walmsley]

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

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

[Paul Walmsley]

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

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

[Suman Anna]

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

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

[Roger Quadros]

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