Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> I don't yet understand why we can't just keep runtime pm disabled as a
>>>>> default for xhci platform devices.
>>>>> It could be enabled by whatever creates the platform device by setting some
>>>>> device property
>>>>> (or equivalent), which would be checked in xhci_plat_probe() before enabling
>>>>> runtime pm. It
>>>>> could then optionally be set in sysfs via power/control entry.
>>>>
>>>> I think runtime pm is not one hardware property, is it suitable if we
>>>> introduce one device property to enable/disable runtime pm?
>>>
>>> As I said, runtime pm is not one hardware property, I think it is not
>>> suitable if we introduce one device property to enable/disable runtime
>>> pm.
>>
>> we already this functionality exposed on sysfs.
>
> From my understanding, Mathias suggested me to add one device property
> (name like "usb-host-runtimePM") by platform_device_add_properties()
> to enable/disable runtime PM when creating platform device, like
> usb3_lpm_capable:
>
> if (dwc->usb3_lpm_capable)
>       props[prop_idx++].name = "usb3-lpm-capable";
>
> ret = platform_device_add_properties(xhci, props);
> if (ret) {
>       dev_err(dwc->dev, "failed to add properties to xHCI\n");
>       goto err1;
> }
>
> What I think It is not suitable to introduce one device property like
> above to enable/disable runtime PM, it is not one hardware attribute.

yeah, that's silly. We already have means for doing that:

	my_probe()
        {
		[...]

		pm_runtime_enable(dev);
                pm_runtime_forbid(dev);

		[...]

		return 0;
	}

>>> Secondly, we only can resume the xhci platform device by getting the
>>> xhci usage counter from gadget driver, since the cable plug in/out
>>> events only can be notified to glue layer of gadget driver(like dwc3
>>> glue layer). That means if we want to suspend xhci platform device, we
>>
>> this is a problem with the glue layer, IMO. It should be something like
>> so:
>>
>> static irqreturn_t dwc3_foobar_wakeup(int irq, void *_glue)
>> {
>>         struct dwc3_foobar_glue *glue = _glue;
>>         u32 reg;
>>
>>         reg = real(glue->base, OFFSET);
>>         if (reg & CONNECT)
>>                 pm_runtime_resume(&glue->dwc3);
>>
>>         return IRQ_HANDLED;
>> }
>>
>> then dwc3's ->runtime_resume() should check if the event is supposed to
>> be handled by host or peripheral by checking which mode it was before
>> suspend and making the assumption that we don't change modes while
>> suspended. Something like:
>>
>> static int dwc3_runtime_resume(struct device *dev)
>> {
>>         struct dwc3 *dwc = dev_get_drvdata(dev);
>>
>>         [...]
>>
>>         if (dwc->is_host)
>>                 pm_runtime_resume(dwc->xhci.dev);
>>
>>         [...]
>>
>>         return 0;
>> }
>
> Yeah, if we don't need to get xhci usage counter in xhci_plat_probe()
> to avoid affecting other controller's runtime PM, we can do like this
> and do not need to get/put counter.

why do you need to get xhci's usage counter in xhci_plat_probe() ?

And why would one xhci affect the other? They are different struct
device instances and, thus, have different pm usage counter. How would
one xhci's pm_runtime affect another?

PM runtime is not per driver, it's per device.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]