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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
