Message-ID: <452f80bb-34fb-e27e-d743-43cf30453f4e@ti.com>
Date:   Thu, 6 Jul 2017 14:08:07 -0500
From:   Dave Gerlach <d-gerlach@...com>
To:     Johan Hovold <johan@...nel.org>
CC:     Tony Lindgren <tony@...mide.com>,
        Santosh Shilimkar <ssantosh@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Keerthy J <j-keerthy@...com>
Subject: Re: [PATCH v2 4/5] soc: ti: Add pm33xx driver for basic suspend
 support

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
> 

Powered by blists - more mailing lists