| Message ID | 20170519200438.9502-5-d-gerlach@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote: > AM335x and AM437x support various low power modes as documented > in section 8.1.4.3 of the AM335x Technical Reference Manual and > section 6.4.3 of the AM437x Technical Reference Manual. > > DeepSleep0 mode offers the lowest power mode with limited > wakeup sources without a system reboot and is mapped as > the suspend state in the kernel. In this state, MPU and > PER domains are turned off with the internal RAM held in > retention to facilitate the resume process. As part of > the boot process, the assembly code is copied over to OCMCRAM > so it can be executed to turn of the EMIF and put DDR into self > refresh. > > Both platforms have a Cortex-M3 (WKUP_M3) which assists the MPU > in DeepSleep0 entry and exit. WKUP_M3 takes care > of the clockdomain and powerdomain transitions based on the > intended low power state. MPU needs to load the appropriate > WKUP_M3 binary onto the WKUP_M3 memory space before it can > leverage any of the PM features like DeepSleep. This loading > is handled by the remoteproc driver wkup_m3_rproc. > > Communication with the WKUP_M3 is handled by a wkup_m3_ipc > driver that exposes the specific PM functionality to be used > the PM code. > +static void am33xx_pm_free_sram(void) > +{ > + gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz); > + gen_pool_free(sram_pool_data, ocmcram_location_data, > + sizeof(struct am33xx_pm_ro_sram_data)); > +} > + > +/* > + * Push the minimal suspend-resume code to SRAM > + */ > +static int am33xx_prepare_push_sram_idle(void) > +{ > + struct device_node *np; > + > + np = of_find_compatible_node(NULL, NULL, "ti,omap3-mpu"); > + Stray newline. > + if (!np) { > + np = of_find_compatible_node(NULL, NULL, "ti,omap4-mpu"); > + if (!np) { > + pr_warn("PM: %s: Unable to find device node for mpu\n", > + __func__); > + return -ENODEV; > + } > + } You never put the reference to np you acquire here. [snip] > +static int am33xx_push_sram_idle(void) > +{ > + struct am33xx_pm_ro_sram_data ro_sram_data; > + int ret; > + void *copy_addr; > + > + ro_sram_data.amx3_pm_sram_data_virt = ocmcram_location_data; > + ro_sram_data.amx3_pm_sram_data_phys = > + gen_pool_virt_to_phys(sram_pool_data, ocmcram_location_data); > + > + /* Save physical address to calculate resume offset during pm init */ > + am33xx_do_wfi_sram_phys = gen_pool_virt_to_phys(sram_pool, > + ocmcram_location); > + > + am33xx_do_wfi_sram = sram_exec_copy(sram_pool, (void *)ocmcram_location, > + pm_sram->do_wfi, > + *pm_sram->do_wfi_sz); > + if (!am33xx_do_wfi_sram) { > + pr_err("PM: %s: am33xx_do_wfi copy to sram failed\n", __func__); > + return -ENODEV; > + } > + > + ret = ti_emif_copy_pm_function_table(sram_pool, > + (void *)sram_suspend_address((unsigned long)pm_sram->emif_sram_table)); > + if (ret) { > + pr_warn("PM: %s: EMIF function copy failed\n", __func__); > + return -EPROBE_DEFER; > + } Here's the dependency to the emif device I commented on earlier (and below). > + > + copy_addr = sram_exec_copy(sram_pool, > + (void *)sram_suspend_address((unsigned long)pm_sram->ro_sram_data), > + &ro_sram_data, > + sizeof(ro_sram_data)); > + if (!copy_addr) { > + pr_err("PM: %s: ro_sram_data copy to sram failed\n", __func__); > + return -ENODEV; > + } > + > + return 0; > +} > + > +static int am33xx_pm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + int ret; > + > + if (!of_machine_is_compatible("ti,am33xx") && > + !of_machine_is_compatible("ti,am43")) > + return -ENODEV; > + > + pm_ops = dev->platform_data; > + if (!pm_ops) { > + pr_err("PM: Cannot get core PM ops!\n"); > + return -ENODEV; > + } > + > + pm_sram = pm_ops->get_sram_addrs(); > + if (!pm_sram) { > + pr_err("PM: Cannot get PM asm function addresses!!\n"); > + return -ENODEV; > + } > + > + ret = am33xx_prepare_push_sram_idle(); Perhaps calling this one am33xx_pm_alloc_sram() would be more descriptive (and match the release function)? > + if (ret) > + return ret; > + > + ret = am33xx_push_sram_idle(); > + if (ret) > + goto err_free_sram; As I mentioned in my comments to the emif-sram driver, you may need to create device link to the emif-sram device to prevent it from going away under you here. > + > + m3_ipc = wkup_m3_ipc_get(); > + if (!m3_ipc) { > + pr_err("PM: Cannot get wkup_m3_ipc handle\n"); You shouldn't log this as an error when probe is being deferred. Why not use dev_err and friends for logging now that you have a struct device? And similarly to the emif-sram device, you may need to create a device-link also to the ipc device to prevent its driver from being unbound. > + ret = -EPROBE_DEFER; > + goto err_free_sram; > + } > + > + am33xx_pm_set_ipc_ops(); > + > +#ifdef CONFIG_SUSPEND > + suspend_set_ops(&am33xx_pm_ops); > +#endif /* CONFIG_SUSPEND */ This renders a lockdep splash about a circular locking dependency when suspending since we're taking the pm_mutex in suspend_set_ops here, and during suspend we flush any deferred probes while already holding the mutex: ====================================================== WARNING: possible circular locking dependency detected 4.12.0-rc7 #11 Not tainted ------------------------------------------------------ bash/404 is trying to acquire lock: (deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c but task is already holding lock: (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (pm_mutex){+.+...}: __mutex_lock+0x80/0x694 mutex_lock_nested+0x2c/0x34 suspend_set_ops+0x4c/0x128 am33xx_pm_probe+0x1fc/0x3a8 platform_drv_probe+0x5c/0xc0 driver_probe_device+0x37c/0x490 __device_attach_driver+0xac/0x128 bus_for_each_drv+0x74/0xa8 __device_attach+0xc4/0x154 device_initial_probe+0x1c/0x20 bus_probe_device+0x98/0xa0 deferred_probe_work_func+0x4c/0xe4 process_one_work+0x1f4/0x758 worker_thread+0x1e0/0x514 kthread+0x128/0x158 ret_from_fork+0x14/0x24 -> #0 (deferred_probe_work){+.+.+.}: lock_acquire+0x108/0x264 flush_work+0x60/0x27c wait_for_device_probe+0x24/0xa4 dpm_prepare+0xd0/0x91c dpm_suspend_start+0x1c/0x70 suspend_devices_and_enter+0xc4/0xeac pm_suspend+0x890/0xc94 state_store+0x80/0xdc kobj_attr_store+0x1c/0x28 sysfs_kf_write+0x5c/0x60 kernfs_fop_write+0x128/0x254 __vfs_write+0x38/0x128 vfs_write+0xb4/0x174 SyS_write+0x54/0xb0 ret_fast_syscall+0x0/0x1c Johan -- 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
On Mon, Jul 03, 2017 at 06:54:19PM +0200, Johan Hovold wrote: > On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote: > > +static int am33xx_pm_probe(struct platform_device *pdev) > > +#ifdef CONFIG_SUSPEND > > + suspend_set_ops(&am33xx_pm_ops); > > +#endif /* CONFIG_SUSPEND */ > > This renders a lockdep splash about a circular locking dependency when > suspending since we're taking the pm_mutex in suspend_set_ops here, and > during suspend we flush any deferred probes while already holding the > mutex: Here's the full splat against 4.12: ====================================================== WARNING: possible circular locking dependency detected 4.12.0 #30 Not tainted ------------------------------------------------------ bash/404 is trying to acquire lock: (deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c but task is already holding lock: (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (pm_mutex){+.+...}: __mutex_lock+0x80/0x694 mutex_lock_nested+0x2c/0x34 suspend_set_ops+0x4c/0x128 am33xx_pm_probe+0x1fc/0x3a8 platform_drv_probe+0x5c/0xc0 driver_probe_device+0x37c/0x490 __device_attach_driver+0xac/0x128 bus_for_each_drv+0x74/0xa8 __device_attach+0xc4/0x154 device_initial_probe+0x1c/0x20 bus_probe_device+0x98/0xa0 deferred_probe_work_func+0x4c/0xe4 process_one_work+0x1f4/0x758 worker_thread+0x1e0/0x514 kthread+0x128/0x158 ret_from_fork+0x14/0x24 -> #0 (deferred_probe_work){+.+.+.}: lock_acquire+0x108/0x264 flush_work+0x60/0x27c wait_for_device_probe+0x24/0xa4 dpm_prepare+0xd0/0x91c dpm_suspend_start+0x1c/0x70 suspend_devices_and_enter+0xc4/0xeac pm_suspend+0x890/0xc94 state_store+0x80/0xdc kobj_attr_store+0x1c/0x28 sysfs_kf_write+0x5c/0x60 kernfs_fop_write+0x128/0x254 __vfs_write+0x38/0x128 vfs_write+0xb4/0x174 SyS_write+0x54/0xb0 ret_fast_syscall+0x0/0x1c other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(pm_mutex); lock(deferred_probe_work); lock(pm_mutex); lock(deferred_probe_work); *** DEADLOCK *** 4 locks held by bash/404: #0: (sb_writers#6){.+.+.+}, at: [<c0244fac>] vfs_write+0x160/0x174 #1: (&of->mutex){+.+.+.}, at: [<c02bab64>] kernfs_fop_write+0xe4/0x254 #2: (s_active#99){.+.+.+}, at: [<c02bab6c>] kernfs_fop_write+0xec/0x254 #3: (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94 stack backtrace: CPU: 0 PID: 404 Comm: bash Not tainted 4.12.0 #30 Hardware name: Generic AM33XX (Flattened Device Tree) [<c011192c>] (unwind_backtrace) from [<c010e104>] (show_stack+0x20/0x24) [<c010e104>] (show_stack) from [<c03f2da0>] (dump_stack+0x24/0x28) [<c03f2da0>] (dump_stack) from [<c016e070>] (print_circular_bug+0x20c/0x334) [<c016e070>] (print_circular_bug) from [<c0171660>] (__lock_acquire+0x1bf4/0x1c08) [<c0171660>] (__lock_acquire) from [<c0172000>] (lock_acquire+0x108/0x264) [<c0172000>] (lock_acquire) from [<c014cf6c>] (flush_work+0x60/0x27c) [<c014cf6c>] (flush_work) from [<c04a7524>] (wait_for_device_probe+0x24/0xa4) [<c04a7524>] (wait_for_device_probe) from [<c04bae20>] (dpm_prepare+0xd0/0x91c) [<c04bae20>] (dpm_prepare) from [<c04bb688>] (dpm_suspend_start+0x1c/0x70) [<c04bb688>] (dpm_suspend_start) from [<c0178364>] (suspend_devices_and_enter+0xc4/0xeac) [<c0178364>] (suspend_devices_and_enter) from [<c01799dc>] (pm_suspend+0x890/0xc94) [<c01799dc>] (pm_suspend) from [<c01773f0>] (state_store+0x80/0xdc) [<c01773f0>] (state_store) from [<c03f442c>] (kobj_attr_store+0x1c/0x28) [<c03f442c>] (kobj_attr_store) from [<c02bb858>] (sysfs_kf_write+0x5c/0x60) [<c02bb858>] (sysfs_kf_write) from [<c02baba8>] (kernfs_fop_write+0x128/0x254) [<c02baba8>] (kernfs_fop_write) from [<c0243830>] (__vfs_write+0x38/0x128) [<c0243830>] (__vfs_write) from [<c0244f00>] (vfs_write+0xb4/0x174) [<c0244f00>] (vfs_write) from [<c0245e08>] (SyS_write+0x54/0xb0) [<c0245e08>] (SyS_write) from [<c01092e0>] (ret_fast_syscall+0x0/0x1c) Johan -- 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
On 07/03/2017 11:54 AM, Johan Hovold wrote: > On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote: >> AM335x and AM437x support various low power modes as documented >> in section 8.1.4.3 of the AM335x Technical Reference Manual and >> section 6.4.3 of the AM437x Technical Reference Manual. >> >> DeepSleep0 mode offers the lowest power mode with limited >> wakeup sources without a system reboot and is mapped as >> the suspend state in the kernel. In this state, MPU and >> PER domains are turned off with the internal RAM held in >> retention to facilitate the resume process. As part of >> the boot process, the assembly code is copied over to OCMCRAM >> so it can be executed to turn of the EMIF and put DDR into self >> refresh. >> >> Both platforms have a Cortex-M3 (WKUP_M3) which assists the MPU >> in DeepSleep0 entry and exit. WKUP_M3 takes care >> of the clockdomain and powerdomain transitions based on the >> intended low power state. MPU needs to load the appropriate >> WKUP_M3 binary onto the WKUP_M3 memory space before it can >> leverage any of the PM features like DeepSleep. This loading >> is handled by the remoteproc driver wkup_m3_rproc. >> >> Communication with the WKUP_M3 is handled by a wkup_m3_ipc >> driver that exposes the specific PM functionality to be used >> the PM code. > >> +static void am33xx_pm_free_sram(void) >> +{ >> + gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz); >> + gen_pool_free(sram_pool_data, ocmcram_location_data, >> + sizeof(struct am33xx_pm_ro_sram_data)); >> +} >> + >> +/* >> + * Push the minimal suspend-resume code to SRAM >> + */ >> +static int am33xx_prepare_push_sram_idle(void) >> +{ >> + struct device_node *np; >> + >> + np = of_find_compatible_node(NULL, NULL, "ti,omap3-mpu"); >> + > > Stray newline. > Yes thanks. >> + if (!np) { >> + np = of_find_compatible_node(NULL, NULL, "ti,omap4-mpu"); >> + if (!np) { >> + pr_warn("PM: %s: Unable to find device node for mpu\n", >> + __func__); >> + return -ENODEV; >> + } >> + } > > You never put the reference to np you acquire here. Whoops, it seems I did not, will fix. > > [snip] > >> +static int am33xx_push_sram_idle(void) >> +{ >> + struct am33xx_pm_ro_sram_data ro_sram_data; >> + int ret; >> + void *copy_addr; >> + >> + ro_sram_data.amx3_pm_sram_data_virt = ocmcram_location_data; >> + ro_sram_data.amx3_pm_sram_data_phys = >> + gen_pool_virt_to_phys(sram_pool_data, ocmcram_location_data); >> + >> + /* Save physical address to calculate resume offset during pm init */ >> + am33xx_do_wfi_sram_phys = gen_pool_virt_to_phys(sram_pool, >> + ocmcram_location); >> + >> + am33xx_do_wfi_sram = sram_exec_copy(sram_pool, (void *)ocmcram_location, >> + pm_sram->do_wfi, >> + *pm_sram->do_wfi_sz); >> + if (!am33xx_do_wfi_sram) { >> + pr_err("PM: %s: am33xx_do_wfi copy to sram failed\n", __func__); >> + return -ENODEV; >> + } >> + >> + ret = ti_emif_copy_pm_function_table(sram_pool, >> + (void *)sram_suspend_address((unsigned long)pm_sram->emif_sram_table)); >> + if (ret) { >> + pr_warn("PM: %s: EMIF function copy failed\n", __func__); >> + return -EPROBE_DEFER; >> + } > > Here's the dependency to the emif device I commented on earlier (and > below). > I commented on this in the ti-emif-pm thread but we should be ok, we can't remove that driver while pm33xx is loaded because of the direct call of exported symbols, pm33xx holds a reference to ti-emif-pm until it is unloaded. I will make sure the confirmation that these functions are valid is solid though. >> + >> + copy_addr = sram_exec_copy(sram_pool, >> + (void *)sram_suspend_address((unsigned long)pm_sram->ro_sram_data), >> + &ro_sram_data, >> + sizeof(ro_sram_data)); >> + if (!copy_addr) { >> + pr_err("PM: %s: ro_sram_data copy to sram failed\n", __func__); >> + return -ENODEV; >> + } >> + >> + return 0; >> +} >> + >> +static int am33xx_pm_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + int ret; >> + >> + if (!of_machine_is_compatible("ti,am33xx") && >> + !of_machine_is_compatible("ti,am43")) >> + return -ENODEV; >> + >> + pm_ops = dev->platform_data; >> + if (!pm_ops) { >> + pr_err("PM: Cannot get core PM ops!\n"); >> + return -ENODEV; >> + } >> + >> + pm_sram = pm_ops->get_sram_addrs(); >> + if (!pm_sram) { >> + pr_err("PM: Cannot get PM asm function addresses!!\n"); >> + return -ENODEV; >> + } >> + >> + ret = am33xx_prepare_push_sram_idle(); > > Perhaps calling this one am33xx_pm_alloc_sram() would be more > descriptive (and match the release function)? Not a bad point. > >> + if (ret) >> + return ret; >> + >> + ret = am33xx_push_sram_idle(); >> + if (ret) >> + goto err_free_sram; > > As I mentioned in my comments to the emif-sram driver, you may need to > create device link to the emif-sram device to prevent it from going away > under you here. Addressed in ti-emif-pm thread. > >> + >> + m3_ipc = wkup_m3_ipc_get(); >> + if (!m3_ipc) { >> + pr_err("PM: Cannot get wkup_m3_ipc handle\n"); > > You shouldn't log this as an error when probe is being deferred. Yes, good point, just noise. > > Why not use dev_err and friends for logging now that you have a struct > device? I suppose I could now. > > And similarly to the emif-sram device, you may need to create a > device-link also to the ipc device to prevent its driver from being > unbound. As described in the ti-emif-pm thread for that driver, we also call exported symbols directly from wkup_m3_ipc driver, so pm33xx cannot probe at all if wkup_m3_ipc is not loaded, and wkup_m3_ipc cannot be removed once pm33xx has been loaded on top. > >> + ret = -EPROBE_DEFER; >> + goto err_free_sram; >> + } >> + >> + am33xx_pm_set_ipc_ops(); >> + >> +#ifdef CONFIG_SUSPEND >> + suspend_set_ops(&am33xx_pm_ops); >> +#endif /* CONFIG_SUSPEND */ > > This renders a lockdep splash about a circular locking dependency when > suspending since we're taking the pm_mutex in suspend_set_ops here, and > during suspend we flush any deferred probes while already holding the > mutex: > > ====================================================== > WARNING: possible circular locking dependency detected > 4.12.0-rc7 #11 Not tainted > ------------------------------------------------------ > bash/404 is trying to acquire lock: > (deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c > > but task is already holding lock: > (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94 > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #1 (pm_mutex){+.+...}: > __mutex_lock+0x80/0x694 > mutex_lock_nested+0x2c/0x34 > suspend_set_ops+0x4c/0x128 > am33xx_pm_probe+0x1fc/0x3a8 > platform_drv_probe+0x5c/0xc0 > driver_probe_device+0x37c/0x490 > __device_attach_driver+0xac/0x128 > bus_for_each_drv+0x74/0xa8 > __device_attach+0xc4/0x154 > device_initial_probe+0x1c/0x20 > bus_probe_device+0x98/0xa0 > deferred_probe_work_func+0x4c/0xe4 > process_one_work+0x1f4/0x758 > worker_thread+0x1e0/0x514 > kthread+0x128/0x158 > ret_from_fork+0x14/0x24 > > -> #0 (deferred_probe_work){+.+.+.}: > lock_acquire+0x108/0x264 > flush_work+0x60/0x27c > wait_for_device_probe+0x24/0xa4 > dpm_prepare+0xd0/0x91c > dpm_suspend_start+0x1c/0x70 > suspend_devices_and_enter+0xc4/0xeac > pm_suspend+0x890/0xc94 > state_store+0x80/0xdc > kobj_attr_store+0x1c/0x28 > sysfs_kf_write+0x5c/0x60 > kernfs_fop_write+0x128/0x254 > __vfs_write+0x38/0x128 > vfs_write+0xb4/0x174 > SyS_write+0x54/0xb0 > ret_fast_syscall+0x0/0x1c > Yes thanks, I have seen this before myself now. I will need to look closer into eliminating this. I am not sure how it is happening, pm_suspend should not be able to be called if suspend_set_ops has not completed, at which point it should have released the mutex. Regards, Dave > Johan > -- 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
On Thu, Jul 06, 2017 at 02:08:07PM -0500, Dave Gerlach wrote: > On 07/03/2017 11:54 AM, Johan Hovold wrote: > > On Fri, May 19, 2017 at 03:04:37PM -0500, Dave Gerlach wrote: > >> AM335x and AM437x support various low power modes as documented > >> in section 8.1.4.3 of the AM335x Technical Reference Manual and > >> section 6.4.3 of the AM437x Technical Reference Manual. > >> > >> DeepSleep0 mode offers the lowest power mode with limited > >> wakeup sources without a system reboot and is mapped as > >> the suspend state in the kernel. In this state, MPU and > >> PER domains are turned off with the internal RAM held in > >> retention to facilitate the resume process. As part of > >> the boot process, the assembly code is copied over to OCMCRAM > >> so it can be executed to turn of the EMIF and put DDR into self > >> refresh. > >> > >> Both platforms have a Cortex-M3 (WKUP_M3) which assists the MPU > >> in DeepSleep0 entry and exit. WKUP_M3 takes care > >> of the clockdomain and powerdomain transitions based on the > >> intended low power state. MPU needs to load the appropriate > >> WKUP_M3 binary onto the WKUP_M3 memory space before it can > >> leverage any of the PM features like DeepSleep. This loading > >> is handled by the remoteproc driver wkup_m3_rproc. > >> > >> Communication with the WKUP_M3 is handled by a wkup_m3_ipc > >> driver that exposes the specific PM functionality to be used > >> the PM code. > > And similarly to the emif-sram device, you may need to create a > > device-link also to the ipc device to prevent its driver from being > > unbound. > > As described in the ti-emif-pm thread for that driver, we also call exported > symbols directly from wkup_m3_ipc driver, so pm33xx cannot probe at all if > wkup_m3_ipc is not loaded, and wkup_m3_ipc cannot be removed once pm33xx has > been loaded on top. As discussed in the other thread, the ipc driver can be unbound from its device even if the module remains loaded. > > > >> + ret = -EPROBE_DEFER; > >> + goto err_free_sram; > >> + } > >> + > >> + am33xx_pm_set_ipc_ops(); > >> + > >> +#ifdef CONFIG_SUSPEND > >> + suspend_set_ops(&am33xx_pm_ops); > >> +#endif /* CONFIG_SUSPEND */ > > > > This renders a lockdep splash about a circular locking dependency when > > suspending since we're taking the pm_mutex in suspend_set_ops here, and > > during suspend we flush any deferred probes while already holding the > > mutex: > > > > ====================================================== > > WARNING: possible circular locking dependency detected > > 4.12.0-rc7 #11 Not tainted > > ------------------------------------------------------ > > bash/404 is trying to acquire lock: > > (deferred_probe_work){+.+.+.}, at: [<c014cf3c>] flush_work+0x30/0x27c > > > > but task is already holding lock: > > (pm_mutex){+.+...}, at: [<c01792dc>] pm_suspend+0x190/0xc94 > > > > which lock already depends on the new lock. > > > > > > the existing dependency chain (in reverse order) is: > > > > -> #1 (pm_mutex){+.+...}: > > __mutex_lock+0x80/0x694 > > mutex_lock_nested+0x2c/0x34 > > suspend_set_ops+0x4c/0x128 > > am33xx_pm_probe+0x1fc/0x3a8 > > platform_drv_probe+0x5c/0xc0 > > driver_probe_device+0x37c/0x490 > > __device_attach_driver+0xac/0x128 > > bus_for_each_drv+0x74/0xa8 > > __device_attach+0xc4/0x154 > > device_initial_probe+0x1c/0x20 > > bus_probe_device+0x98/0xa0 > > deferred_probe_work_func+0x4c/0xe4 > > process_one_work+0x1f4/0x758 > > worker_thread+0x1e0/0x514 > > kthread+0x128/0x158 > > ret_from_fork+0x14/0x24 > > > > -> #0 (deferred_probe_work){+.+.+.}: > > lock_acquire+0x108/0x264 > > flush_work+0x60/0x27c > > wait_for_device_probe+0x24/0xa4 > > dpm_prepare+0xd0/0x91c > > dpm_suspend_start+0x1c/0x70 > > suspend_devices_and_enter+0xc4/0xeac > > pm_suspend+0x890/0xc94 > > state_store+0x80/0xdc > > kobj_attr_store+0x1c/0x28 > > sysfs_kf_write+0x5c/0x60 > > kernfs_fop_write+0x128/0x254 > > __vfs_write+0x38/0x128 > > vfs_write+0xb4/0x174 > > SyS_write+0x54/0xb0 > > ret_fast_syscall+0x0/0x1c > > > > Yes thanks, I have seen this before myself now. I will need to look closer into > eliminating this. I am not sure how it is happening, pm_suspend should not be > able to be called if suspend_set_ops has not completed, at which point it should > have released the mutex. So perhaps the deadlock cannot happen in practise then even if both paths can indeed be taken (which triggers the lockdep warning). Johan -- 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
diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig index 39e152abe6b9..92770d84a288 100644 --- a/drivers/soc/ti/Kconfig +++ b/drivers/soc/ti/Kconfig @@ -28,6 +28,15 @@ config KEYSTONE_NAVIGATOR_DMA If unsure, say N. +config AMX3_PM + tristate "AMx3 Power Management" + depends on SOC_AM33XX || SOC_AM43XX + depends on WKUP_M3_IPC && TI_EMIF_SRAM && SRAM + help + Enable power management on AM335x and AM437x. Required for suspend to mem + and standby states on both AM335x and AM437x platforms and for deeper cpuidle + c-states on AM335x. + config WKUP_M3_IPC tristate "TI AMx3 Wkup-M3 IPC Driver" depends on WKUP_M3_RPROC diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile index 7d572736c86e..d9bd4ba424be 100644 --- a/drivers/soc/ti/Makefile +++ b/drivers/soc/ti/Makefile @@ -4,5 +4,6 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o +obj-$(CONFIG_AMX3_PM) += pm33xx.o obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o diff --git a/drivers/soc/ti/pm33xx.c b/drivers/soc/ti/pm33xx.c new file mode 100644 index 000000000000..cfeb8df7e82a --- /dev/null +++ b/drivers/soc/ti/pm33xx.c @@ -0,0 +1,337 @@ +/* + * AM33XX Power Management Routines + * + * Copyright (C) 2012-2017 Texas Instruments Incorporated - http://www.ti.com/ + * Vaibhav Bedia, Dave Gerlach + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/cpu.h> +#include <linux/err.h> +#include <linux/genalloc.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_data/pm33xx.h> +#include <linux/platform_device.h> +#include <linux/sizes.h> +#include <linux/sram.h> +#include <linux/suspend.h> +#include <linux/ti-emif-sram.h> +#include <linux/wkup_m3_ipc.h> + +#include <asm/proc-fns.h> +#include <asm/suspend.h> +#include <asm/system_misc.h> + +#define AMX3_PM_SRAM_SYMBOL_OFFSET(sym) ((unsigned long)(sym) - \ + (unsigned long)pm_sram->do_wfi) + +static int (*am33xx_do_wfi_sram)(unsigned long unused); +static phys_addr_t am33xx_do_wfi_sram_phys; + +static struct gen_pool *sram_pool, *sram_pool_data; +static unsigned long ocmcram_location, ocmcram_location_data; + +static struct am33xx_pm_platform_data *pm_ops; +static struct am33xx_pm_sram_addr *pm_sram; + +static struct wkup_m3_ipc *m3_ipc; + +static u32 sram_suspend_address(unsigned long addr) +{ + return ((unsigned long)am33xx_do_wfi_sram + + AMX3_PM_SRAM_SYMBOL_OFFSET(addr)); +} + +#ifdef CONFIG_SUSPEND +static int am33xx_pm_suspend(suspend_state_t suspend_state) +{ + int i, ret = 0; + + ret = pm_ops->soc_suspend((unsigned long)suspend_state, + am33xx_do_wfi_sram); + + if (ret) { + pr_err("PM: Kernel suspend failure\n"); + } else { + i = m3_ipc->ops->request_pm_status(m3_ipc); + + switch (i) { + case 0: + pr_info("PM: Successfully put all powerdomains to target state\n"); + break; + case 1: + pr_err("PM: Could not transition all powerdomains to target state\n"); + ret = -1; + break; + default: + pr_err("PM: CM3 returned unknown result = %d\n", i); + ret = -1; + } + } + + return ret; +} + +static int am33xx_pm_enter(suspend_state_t suspend_state) +{ + int ret = 0; + + switch (suspend_state) { + case PM_SUSPEND_MEM: + case PM_SUSPEND_STANDBY: + ret = am33xx_pm_suspend(suspend_state); + break; + default: + ret = -EINVAL; + } + + return ret; +} + +static int am33xx_pm_begin(suspend_state_t state) +{ + int ret = -EINVAL; + + switch (state) { + case PM_SUSPEND_MEM: + ret = m3_ipc->ops->prepare_low_power(m3_ipc, WKUP_M3_DEEPSLEEP); + break; + case PM_SUSPEND_STANDBY: + ret = m3_ipc->ops->prepare_low_power(m3_ipc, WKUP_M3_STANDBY); + break; + } + + return ret; +} + +static void am33xx_pm_end(void) +{ + m3_ipc->ops->finish_low_power(m3_ipc); +} + +static int am33xx_pm_valid(suspend_state_t state) +{ + switch (state) { + case PM_SUSPEND_STANDBY: + case PM_SUSPEND_MEM: + return 1; + default: + return 0; + } +} + +static const struct platform_suspend_ops am33xx_pm_ops = { + .begin = am33xx_pm_begin, + .end = am33xx_pm_end, + .enter = am33xx_pm_enter, + .valid = am33xx_pm_valid, +}; +#endif /* CONFIG_SUSPEND */ + +static void am33xx_pm_set_ipc_ops(void) +{ + u32 resume_address; + int temp; + + temp = ti_emif_get_mem_type(); + if (temp < 0) { + pr_err("PM: Cannot determine memory type, no PM available\n"); + return; + } + m3_ipc->ops->set_mem_type(m3_ipc, temp); + + /* Physical resume address to be used by ROM code */ + resume_address = am33xx_do_wfi_sram_phys + + *pm_sram->resume_offset + 0x4; + + m3_ipc->ops->set_resume_address(m3_ipc, (void *)resume_address); +} + +static void am33xx_pm_free_sram(void) +{ + gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz); + gen_pool_free(sram_pool_data, ocmcram_location_data, + sizeof(struct am33xx_pm_ro_sram_data)); +} + +/* + * Push the minimal suspend-resume code to SRAM + */ +static int am33xx_prepare_push_sram_idle(void) +{ + struct device_node *np; + + np = of_find_compatible_node(NULL, NULL, "ti,omap3-mpu"); + + if (!np) { + np = of_find_compatible_node(NULL, NULL, "ti,omap4-mpu"); + if (!np) { + pr_warn("PM: %s: Unable to find device node for mpu\n", + __func__); + return -ENODEV; + } + } + + sram_pool = of_gen_pool_get(np, "pm-sram", 0); + if (!sram_pool) { + pr_warn("PM: %s: Unable to get sram pool for ocmcram\n", + __func__); + return -ENODEV; + } + + sram_pool_data = of_gen_pool_get(np, "pm-sram", 1); + if (!sram_pool_data) { + pr_warn("PM: %s: Unable to get sram data pool for ocmcram\n", + __func__); + return -ENODEV; + } + + ocmcram_location = gen_pool_alloc(sram_pool, *pm_sram->do_wfi_sz); + if (!ocmcram_location) { + pr_warn("PM: %s: Unable to allocate memory from ocmcram\n", + __func__); + return -ENOMEM; + } + + ocmcram_location_data = gen_pool_alloc(sram_pool_data, + sizeof(struct emif_regs_amx3)); + if (!ocmcram_location_data) { + pr_err("PM: Unable to allocate memory from ocmcram\n"); + gen_pool_free(sram_pool, ocmcram_location, *pm_sram->do_wfi_sz); + return -ENOMEM; + } + + return 0; +} + +static int am33xx_push_sram_idle(void) +{ + struct am33xx_pm_ro_sram_data ro_sram_data; + int ret; + void *copy_addr; + + ro_sram_data.amx3_pm_sram_data_virt = ocmcram_location_data; + ro_sram_data.amx3_pm_sram_data_phys = + gen_pool_virt_to_phys(sram_pool_data, ocmcram_location_data); + + /* Save physical address to calculate resume offset during pm init */ + am33xx_do_wfi_sram_phys = gen_pool_virt_to_phys(sram_pool, + ocmcram_location); + + am33xx_do_wfi_sram = sram_exec_copy(sram_pool, (void *)ocmcram_location, + pm_sram->do_wfi, + *pm_sram->do_wfi_sz); + if (!am33xx_do_wfi_sram) { + pr_err("PM: %s: am33xx_do_wfi copy to sram failed\n", __func__); + return -ENODEV; + } + + ret = ti_emif_copy_pm_function_table(sram_pool, + (void *)sram_suspend_address((unsigned long)pm_sram->emif_sram_table)); + if (ret) { + pr_warn("PM: %s: EMIF function copy failed\n", __func__); + return -EPROBE_DEFER; + } + + copy_addr = sram_exec_copy(sram_pool, + (void *)sram_suspend_address((unsigned long)pm_sram->ro_sram_data), + &ro_sram_data, + sizeof(ro_sram_data)); + if (!copy_addr) { + pr_err("PM: %s: ro_sram_data copy to sram failed\n", __func__); + return -ENODEV; + } + + return 0; +} + +static int am33xx_pm_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + int ret; + + if (!of_machine_is_compatible("ti,am33xx") && + !of_machine_is_compatible("ti,am43")) + return -ENODEV; + + pm_ops = dev->platform_data; + if (!pm_ops) { + pr_err("PM: Cannot get core PM ops!\n"); + return -ENODEV; + } + + pm_sram = pm_ops->get_sram_addrs(); + if (!pm_sram) { + pr_err("PM: Cannot get PM asm function addresses!!\n"); + return -ENODEV; + } + + ret = am33xx_prepare_push_sram_idle(); + if (ret) + return ret; + + ret = am33xx_push_sram_idle(); + if (ret) + goto err_free_sram; + + m3_ipc = wkup_m3_ipc_get(); + if (!m3_ipc) { + pr_err("PM: Cannot get wkup_m3_ipc handle\n"); + ret = -EPROBE_DEFER; + goto err_free_sram; + } + + am33xx_pm_set_ipc_ops(); + +#ifdef CONFIG_SUSPEND + suspend_set_ops(&am33xx_pm_ops); +#endif /* CONFIG_SUSPEND */ + + ret = pm_ops->init(); + if (ret) { + pr_err("Unable to call core pm init!\n"); + ret = -ENODEV; + goto err_put_wkup_m3_ipc; + } + + return 0; + +err_put_wkup_m3_ipc: + wkup_m3_ipc_put(m3_ipc); +err_free_sram: + am33xx_pm_free_sram(); + return ret; +} + +static int am33xx_pm_remove(struct platform_device *pdev) +{ + suspend_set_ops(NULL); + wkup_m3_ipc_put(m3_ipc); + am33xx_pm_free_sram(); + return 0; +} + +static struct platform_driver am33xx_pm_driver = { + .driver = { + .name = "pm33xx", + }, + .probe = am33xx_pm_probe, + .remove = am33xx_pm_remove, +}; +module_platform_driver(am33xx_pm_driver); + +MODULE_ALIAS("platform:pm33xx"); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("am33xx power management driver"); diff --git a/include/linux/platform_data/pm33xx.h b/include/linux/platform_data/pm33xx.h index c191ab681093..b8d6c3a9b46c 100644 --- a/include/linux/platform_data/pm33xx.h +++ b/include/linux/platform_data/pm33xx.h @@ -38,12 +38,12 @@ struct am33xx_pm_sram_data { u32 wfi_flags; u32 l2_aux_ctrl_val; u32 l2_prefetch_ctrl_val; -}; +} __packed __aligned(8); struct am33xx_pm_ro_sram_data { u32 amx3_pm_sram_data_virt; u32 amx3_pm_sram_data_phys; -}; +} __packed __aligned(8); extern inline void amx3_pm_asm_offsets(void) {
AM335x and AM437x support various low power modes as documented in section 8.1.4.3 of the AM335x Technical Reference Manual and section 6.4.3 of the AM437x Technical Reference Manual. DeepSleep0 mode offers the lowest power mode with limited wakeup sources without a system reboot and is mapped as the suspend state in the kernel. In this state, MPU and PER domains are turned off with the internal RAM held in retention to facilitate the resume process. As part of the boot process, the assembly code is copied over to OCMCRAM so it can be executed to turn of the EMIF and put DDR into self refresh. Both platforms have a Cortex-M3 (WKUP_M3) which assists the MPU in DeepSleep0 entry and exit. WKUP_M3 takes care of the clockdomain and powerdomain transitions based on the intended low power state. MPU needs to load the appropriate WKUP_M3 binary onto the WKUP_M3 memory space before it can leverage any of the PM features like DeepSleep. This loading is handled by the remoteproc driver wkup_m3_rproc. Communication with the WKUP_M3 is handled by a wkup_m3_ipc driver that exposes the specific PM functionality to be used the PM code. In the current implementation when the suspend process is initiated, MPU interrupts the WKUP_M3 to let it know about the intent of entering DeepSleep0 and waits for an ACK. When the ACK is received MPU continues with its suspend process to suspend all the drivers and then jumps to assembly in OCMC RAM. The assembly code puts the external RAM in self-refresh mode, gates the MPU clock, and then finally executes the WFI instruction. Execution of the WFI instruction with MPU clock gated triggers another interrupt to the WKUP_M3 which then continues with the power down sequence wherein the clockdomain and powerdomain transition takes place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt lines for the wakeup sources. WFI execution on WKUP_M3 causes the hardware to disable the main oscillator of the SoC and from here system remains in sleep state until a wake source brings the system into resume path. When a wakeup event occurs, WKUP_M3 starts the power-up sequence by switching on the power domains and finally enabling the clock to MPU. Since the MPU gets powered down as part of the sleep sequence in the resume path ROM code starts executing. The ROM code detects a wakeup from sleep and then jumps to the resume location in OCMC which was populated in one of the IPC registers as part of the suspend sequence. Code is based on work by Vaibhav Bedia. Signed-off-by: Dave Gerlach <d-gerlach@ti.com> --- drivers/soc/ti/Kconfig | 9 + drivers/soc/ti/Makefile | 1 + drivers/soc/ti/pm33xx.c | 337 +++++++++++++++++++++++++++++++++++ include/linux/platform_data/pm33xx.h | 4 +- 4 files changed, 349 insertions(+), 2 deletions(-) create mode 100644 drivers/soc/ti/pm33xx.c