From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758361AbdCVFH4 (ORCPT ); Wed, 22 Mar 2017 01:07:56 -0400 Received: from mail-ot0-f173.google.com ([74.125.82.173]:36296 "EHLO mail-ot0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750731AbdCVFHs (ORCPT ); Wed, 22 Mar 2017 01:07:48 -0400 MIME-Version: 1.0 In-Reply-To: <87d1dbuk73.fsf@linux.intel.com> References: <58908DBC.5010208@linux.intel.com> <58AB06EA.7040401@linux.intel.com> <87d1dbuk73.fsf@linux.intel.com> From: Baolin Wang Date: Wed, 22 Mar 2017 13:07:46 +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 21 March 2017 at 16:07, 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. > >> 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