From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755774AbdCWCSC (ORCPT ); Wed, 22 Mar 2017 22:18:02 -0400 Received: from mail-ot0-f179.google.com ([74.125.82.179]:33317 "EHLO mail-ot0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755696AbdCWCRv (ORCPT ); Wed, 22 Mar 2017 22:17:51 -0400 MIME-Version: 1.0 In-Reply-To: <58D27178.8090008@linux.intel.com> References: <58908DBC.5010208@linux.intel.com> <58AB06EA.7040401@linux.intel.com> <87d1dbuk73.fsf@linux.intel.com> <871stpvg7a.fsf@linux.intel.com> <58D27178.8090008@linux.intel.com> From: Baolin Wang Date: Thu, 23 Mar 2017 10:17:49 +0800 Message-ID: Subject: Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM To: Mathias Nyman Cc: Felipe Balbi , 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 20:43, Mathias Nyman wrote: > On 22.03.2017 12:40, Baolin Wang wrote: >> >> 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? >>>>> >>>>> >>>>> 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: >>>> > > It was more of generic pondering how to automatically enable runtime PM for > platforms > that know their xhci can runtime suspend/resume. But all this can be skipped > for now and just > force users to manually enable xhci platform runtime pm in sysfs for now. > > For me, and for xhci point of view only, patch 1/2 is quite ok. > Only some minor adjustments like calling runtime_get in the beginning of > probe, and runtime_put > at the end end after both hcd's are added. > > Probe could additionally call pm_runtime_forbid() to prevent runtime pm frpm > being on as > default, (increases usage count, modifies runtime_auto) > > This would force the user to explicitly enable runtime pm usin power/control > in sysfs. Fair enough. > > In my opinion that patch could be a separate one, and how dwc3 deals with it > can be a separate > topic. OK. > >>> >>> 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? > > > That is my understanding as well, Basically the xhci parents driver > should end up calling xhci_plat_runtime_resume() one way or another. > Just like Felipe's glue driver fix above does. > > So lets keep pm_runtime_forbid() as default for xhci-plat for now. I agreed. I will create new patches. Thanks Mathias and Felipe's suggestion. -- Baolin.wang Best Regards