Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros <rogerq@kernel.org>]

On 15/11/2023 17:02, Théo Lebrun wrote:
> Hi Roger,
> 
> On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote:
>> On 13/11/2023 16:26, Théo Lebrun wrote:
>>> Hardware initialisation is only done at probe. The J7200 USB controller
>>> is reset at resume because of power-domains toggling off & on. We
>>> therefore (1) toggle PM runtime at suspend/resume & (2) reconfigure the
>>> hardware at resume.
>>
>> at probe we are doing a pm_runtime_get() and never doing a put thus
>> preventing any runtime PM.
> 
> Indeed. The get() from probe/resume are in symmetry with the put() from
> suspend. Is this wrong in some manner?
> 
>>> index c331bcd2faeb..50b38c4b9c87 100644
>>> --- a/drivers/usb/cdns3/cdns3-ti.c
>>> +++ b/drivers/usb/cdns3/cdns3-ti.c
>>> @@ -197,6 +197,50 @@ static int cdns_ti_probe(struct platform_device *pdev)
>>>  	return error;
>>>  }
>>>  
>>> +#ifdef CONFIG_PM
>>> +
>>> +static int cdns_ti_suspend(struct device *dev)
>>> +{
>>> +	struct cdns_ti *data = dev_get_drvdata(dev);
>>> +
>>> +	if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb"))
>>> +		return 0;
>>> +
>>> +	pm_runtime_put_sync(data->dev);
>>> +
>>> +	return 0;
>>
>> You might want to check suspend/resume ops in cdns3-plat and
>> do something similar here.
> 
> I'm unsure what you are referring to specifically in cdns3-plat?

What I meant is, calling pm_runtime_get/put() from system suspend/resume
hooks doesn't seem right.

How about using something like pm_runtime_forbid(dev) on devices which
loose USB context on runtime suspend e.g. J7200.
So at probe we can get rid of the pm_runtime_get_sync() call.
e.g.

        pm_runtime_set_active(dev);
        pm_runtime_enable(dev);
        if (cnds_ti->can_loose_context)
                pm_runtime_forbid(dev);

        pm_runtime_set_autosuspend_delay(dev, CNDS_TI_AUTOSUSPEND_DELAY);	/* could be 20ms? */
        pm_runtime_mark_last_busy(dev);
        pm_runtime_use_autosuspend(dev);

You will need to modify the suspend/resume handlers accordingly.
https://docs.kernel.org/power/runtime_pm.html#runtime-pm-and-system-sleep

What I'm not sure of is if there are any TI platforms that retain USB context
on power domain off. Let me get back on this. Till then we can assume that
all platforms loose USB context on power domain off.

One comment below.

> +	return ret;
> +}


> 
>  - Using pm_runtime_status_suspended() to get the current PM runtime
>    state & act on it? I don't see why because we know the pm_runtime
>    state is a single put() at probe.
> 
>  - Having a `in_lpm` flag to track low-power mode state? I wouldn't see
>    why we'd want that as we don't register runtime_suspend &
>    runtime_resume callbacks and system syspend/resume can be assumed to
>    be called in the right order.
> 
>  - Checking the `device_may_wakeup()`? That doesn't apply to this driver
>    which cannot be a wakeup source.
> 
> Thanks for your review!
> Regards,
> 
> --> +static int cdns_ti_resume(struct device *dev)
> +{
> +	struct cdns_ti *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb"))
> +		return 0;
> +
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret);
> +		goto err;
> +	}
> +
> +	cdns_ti_init_hw(data);
> +
> +	return 0;
> +
> +err:
> +	pm_runtime_put_sync(data->dev);
> +	pm_runtime_disable(data->dev);

Why do you do a pm_runtime_disable() here?

> +	return ret;
> +}


> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
> ------------------------------------------------------------------------
> 
> 

-- 
cheers,
-roger