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

[Baolin Wang]

Hi,

On 21 March 2017 at 16:07, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@xxxxxxxxxx> 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.

>
>> 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.

>> must put xhci usage counter (so we can not keep their parent-child
>> relationship intact). That is why we need
>> pm_suspend_ignore_children(dev, true).
>
> you really shouldn't need that and it's still unclear why you think you
> do.
>
> --
> balbi



-- 
Baolin.wang
Best Regards