linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Felipe Balbi <balbi@kernel.org>
To: Sandeep Maheswaram <sanm@codeaurora.org>
Cc: Matthias Kaehlcke <mka@chromium.org>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Doug Anderson <dianders@chromium.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Pratham Pratap <prathampratap@codeaurora.org>
Subject: Re: [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup
Date: Wed, 18 Aug 2021 12:56:56 +0300	[thread overview]
Message-ID: <87sfz7f4w6.fsf@kernel.org> (raw)
In-Reply-To: <5ecee092-dba9-071c-940b-55e16f4d7a90@codeaurora.org>


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
>> This means that in order for glue_suspend() to run, dwc3 has to suspend
>> first and xhci has to suspend before dwc3.
>>
>> For example, in the suspend call above, qcom (the glue) is directly
>> accessing dwc3 core data, which is incorrect. It looks like we want to
>> know if the PHY is not powered off and if it isn't, then we want to
>> change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false
>> whenever any of xHCI's children enable USB wakeup.
>>
>> It seems like we need to way to generically propagate that knowledge up
>> the parent tree. I.e., a parent needs to know if its child is wakeup
>> capable, then dwc3 could, in its suspend routine:
>>
>> static int dwc3_suspend(struct device *dev)
>> {
>> 	/* ... */
>>
>> 	if (device_children_wakeup_capable(dev))
>>          	device_enable_wakeup(dev);
>>
>> 	/* ... */
>> }
>
> Can we use like  this device_may_wakeup(&dwc->xhci->dev) to check if
> children is wakeup capable like below ?

that really doesn't sound like a good idea, IMHO. We're still passing
through layers of abstraction without anyone's knowledge :-)

It looks to me like we're missing some infrastructure in the wakeup code
so parents can make decisions based on the state of their children.

>> and similarly for qcom glue:
>>
>> static int dwc3_qcom_suspend(struct device *dev)
>> {
>> 	/* ... */
>>
>>
>> 	if (device_children_wakeup_capable(dev)) {
>>          	device_enable_wakeup(dev);
>> 		genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;
>>          }
>>
>> 	/* ... */
>> }
>>
>> It also seems plausible that this could be done at driver core and
>> completely hidden away from drivers.
>
> And in qcom glue like this
>
> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> {
>
> /* ... */
>
>     struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);

you see, here there's an assumption that the platform data is still
valid and not some bogus dangling pointer. There's also an assumption
that the type is struct dwc3 (which is unlikely to change, but still).

-- 
balbi

  reply	other threads:[~2021-08-18 10:01 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-28 12:08 [PATCH v8 0/6] USB DWC3 host wake up support from system suspend Sandeep Maheswaram
2021-06-28 12:08 ` [PATCH v8 1/6] usb: dwc3: core: Add HS phy mode variable and phy poweroff flag Sandeep Maheswaram
2021-06-28 12:08 ` [PATCH v8 2/6] usb: host: xhci: plat: Add suspend quirk for dwc3 controller Sandeep Maheswaram
2021-07-12  9:31   ` Felipe Balbi
2021-09-28 23:08   ` Brian Norris
2021-06-28 12:08 ` [PATCH v8 3/6] usb: dwc3: core: Host wake up support from system suspend Sandeep Maheswaram
2021-06-28 12:08 ` [PATCH v8 4/6] usb: dwc3: qcom: Add helper functions to enable,disable wake irqs Sandeep Maheswaram
2021-06-28 12:08 ` [PATCH v8 5/6] usb: dwc3: qcom: Configure wakeup interrupts during suspend Sandeep Maheswaram
2021-06-28 12:08 ` [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup Sandeep Maheswaram
2021-06-28 21:23   ` Matthias Kaehlcke
2021-07-12  9:42     ` Felipe Balbi
2021-08-18  9:14       ` Sandeep Maheswaram
2021-08-18  9:56         ` Felipe Balbi [this message]
2021-09-15 14:05       ` Pavan Kondeti

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87sfz7f4w6.fsf@kernel.org \
    --to=balbi@kernel.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=mka@chromium.org \
    --cc=prathampratap@codeaurora.org \
    --cc=sanm@codeaurora.org \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).