From: Peter Chen <peter.chen@kernel.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
balbi@kernel.org, agross@kernel.org, gregkh@linuxfoundation.org,
jackp@codeaurora.org, wcheng@codeaurora.org,
linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom
Date: Thu, 8 Jul 2021 11:06:31 +0800 [thread overview]
Message-ID: <20210708030631.GA22420@nchen> (raw)
In-Reply-To: <YOX6d+sBEJMP4V3q@yoga>
On 21-07-07 14:03:19, Bjorn Andersson wrote:
> On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
>
> Allow me to reorder your two questions:
>
> > And why using a notifier need to concern core's deferral probe?
>
> The problem at hand calls for the core for somehow invoking
> dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
>
> This means that dwc3-qcom somehow needs to inform the dwc3-core about
> this (and stash the pointer). And this can't be done until dwc3-core
> actually exist, which it won't until dwc3_probe() has completed
> successfully (or in particular allocated struct dwc).
Maybe you misunderstood the notifier I meant previous, my pointer was
calling glue layer API directly.
Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
allocated successfully, you could call glue layer notifier at function
dwc3_usb_role_switch_set directly.
Some references of my idea [1] [2]
[1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
[2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
>
> > Why do you think we need to retry the parent's probe again?
>
> There's four options here:
>
> 0) Hope that of_platform_populate() always succeeds in invoking
> dwc3_probe() on the first attempt, so that it is available when
> of_find_device_by_node() is invoked in dwc3_qcom_probe() (and a few of
> the other platform's drivers).
>
> 1) Ensure that the operations performed by dwc3_probe() happens
> synchronously and return a failure to dwc3-qcom, which depending on how
> dwc3_probe() failed can propagate that failure - i.e. either probe defer
> or clean up its resources if the failure from dwc3-core is permanent.
>
> 2) Register the dwc3-core and then return from dwc3_qcom_probe() in some
> half-initialized state and through some yet to be invented notification
> mechanism continue the tail of dwc3_qcom_probe() once dwc3_probe() has
> finished. But I know of no such notification mechanism in place today
> and we can just register a callback, because of 1).
> Furthermore we'd leave dwc3-qcom half-initialized until the dwc3-core is
> done probing - which might never happen.
>
> 3) Make drvdata of all the platform drivers be some known struct that
> dwc3-core can retrieve and dereference - containing the optional
> callback to the role_switch callback.
>
>
> We've tried the option 0) for a few years now. Option 2) is a variant of
> what we have today, where we patch one problem up and hope that nothing
> unexpected happens until things has fully probed. We're doing 3) in
> various other places, but in my view it's abusing a void * and has to be
> kept synchronized between all the possible parent drivers.
>
> Left is 1), which will take some refactoring, but will leave the
> interaction between the two drivers in a state that's very easy to
> reason about.
Function of_find_device_by_node() invoked at glue layer is usually successfully,
The dwc3_probe failure doesn't affect it, unless you enable auto-suspend,
and glue layer's runtime suspend routine depends on dwc3 core's runtime suspend
routine. Would you please describe more about dwc3-core probe failure causes
dwc3-qcom's probe has failed or in half-initialized state you said?
>
> > I know there are some downstream code which using this way, I would
> > like to know the shortcoming for it.
> >
>
> The shortcoming of having dwc3_qcom_probe() invoke dwc3_probe()
> "manually" and then returning -EPROBE_DEFER if the dwc3-core's resources
> aren't yet available is that we're wasting some time tearing down the
> dwc3-qcom state and then re-initialize it next time this is attempted.
Like above, would you explain more about it?
--
Thanks,
Peter Chen
next prev parent reply other threads:[~2021-07-08 3:06 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-04 1:33 [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom Bryan O'Donoghue
2021-07-04 1:33 ` [PATCH 1/3] usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API Bryan O'Donoghue
2021-07-07 5:06 ` Bjorn Andersson
2021-07-04 1:33 ` [PATCH 2/3] usb: dwc3: Add role switch relay support Bryan O'Donoghue
2021-07-06 2:51 ` Peter Chen
2021-07-06 10:07 ` Bryan O'Donoghue
2021-07-07 5:14 ` Bjorn Andersson
2021-07-07 9:49 ` Bryan O'Donoghue
2021-07-07 9:51 ` Bryan O'Donoghue
2021-07-04 1:33 ` [PATCH 3/3] usb: dwc3: dwc3-qcom: Make dwc3-qcom a role-switch signal recipient Bryan O'Donoghue
2021-07-07 1:57 ` [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom Peter Chen
2021-07-07 19:03 ` Bjorn Andersson
2021-07-08 3:06 ` Peter Chen [this message]
2021-07-08 3:54 ` Bjorn Andersson
2021-07-08 10:17 ` Bryan O'Donoghue
2021-08-24 23:52 ` Bjorn Andersson
2021-08-24 23:58 ` Bryan O'Donoghue
2021-08-25 0:01 ` Bjorn Andersson
2021-08-25 0:17 ` Bryan O'Donoghue
2021-08-24 23:37 ` Bjorn Andersson
2021-08-25 5:51 ` Felipe Balbi
2021-08-25 8:18 ` Bryan O'Donoghue
2021-08-25 15:53 ` Bjorn Andersson
2021-08-25 16:43 ` Heikki Krogerus
2021-08-25 17:04 ` Bjorn Andersson
2021-08-25 17:59 ` Bryan O'Donoghue
2021-08-25 20:06 ` Bjorn Andersson
2021-08-26 6:15 ` Felipe Balbi
2021-09-15 13:53 ` Bjorn Andersson
2021-09-17 12:33 ` Heikki Krogerus
2021-08-25 20:11 ` Dmitry Baryshkov
2021-08-25 12:12 ` Rob Herring
2021-08-25 15:20 ` Felipe Balbi
2021-08-25 13:16 ` Bjorn Andersson
2021-08-25 15:22 ` Felipe Balbi
2021-08-25 16:33 ` Bjorn Andersson
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=20210708030631.GA22420@nchen \
--to=peter.chen@kernel.org \
--cc=agross@kernel.org \
--cc=balbi@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=bryan.odonoghue@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=jackp@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=wcheng@codeaurora.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).