From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933802AbdCVKk2 (ORCPT ); Wed, 22 Mar 2017 06:40:28 -0400 Received: from mail-oi0-f41.google.com ([209.85.218.41]:35638 "EHLO mail-oi0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933987AbdCVKkX (ORCPT ); Wed, 22 Mar 2017 06:40:23 -0400 MIME-Version: 1.0 In-Reply-To: <871stpvg7a.fsf@linux.intel.com> References: <58908DBC.5010208@linux.intel.com> <58AB06EA.7040401@linux.intel.com> <87d1dbuk73.fsf@linux.intel.com> <871stpvg7a.fsf@linux.intel.com> From: Baolin Wang Date: Wed, 22 Mar 2017 18:40:21 +0800 Message-ID: Subject: Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM To: Felipe Balbi Cc: Mathias Nyman , Mathias Nyman , Greg KH , Mark Brown , USB , LKML , Alan Stern Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 22 March 2017 at 17:00, Felipe Balbi wrote: > > Hi, > > Baolin Wang 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