Message-ID: <CAMz4kuLJaajsTtNF6MAJEBSBzR87P6ocvhr6=pPK2Uh=sH7m_g@mail.gmail.com>
Date:   Thu, 23 Mar 2017 10:17:49 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc:     Felipe Balbi <balbi@...nel.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Mark Brown <broonie@...nel.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Alan Stern <stern@...land.harvard.edu>
Subject: Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM

Hi,

On 22 March 2017 at 20:43, Mathias Nyman <mathias.nyman@...ux.intel.com> wrote:
> On 22.03.2017 12:40, Baolin Wang wrote:
>>
>> Hi,
>>
>> On 22 March 2017 at 17:00, Felipe Balbi <balbi@...nel.org> wrote:
>>>
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@...aro.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?
>>>>>
>>>>>
>>>>> 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

Powered by blists - more mailing lists