Hi, On 22 March 2017 at 17:00, Felipe Balbi <ba...@kernel.org> wrote: > > Hi, > > Baolin Wang <baolin.w...@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);
That's same with getting the usage counter. > > [...] > > 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? What I mean is if another USB controller's driver did not implement runtime pm callbacks but system enables CONFIG_PM, that will make xhci device auto-suspended when after probing xhci-plat if we did not get xhci device usage counter, but gadget driver can not resume xhci without implementing runtime PM callbacks. If we want to implement xhci-plat runtime resume/suspend without getting usage counter, we should assume every driver using xhci-plat should implement their runtime PM callbacks. Is this right? > > PM runtime is not per driver, it's per device. Correct. -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html