Hi, Baolin Wang <[email protected]> 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
signature.asc
Description: PGP signature
