Messages in this thread | | | Subject | Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver | From | "Sandeep Maheswaram (Temp)" <> | Date | Thu, 4 Jun 2020 15:13:09 +0530 |
| |
On 6/3/2020 11:06 PM, Stephen Boyd wrote: > Quoting Sandeep Maheswaram (2020-03-31 22:15:43) >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 1dfd024..d33ae86 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -76,8 +85,13 @@ struct dwc3_qcom { >> enum usb_dr_mode mode; >> bool is_suspended; >> bool pm_suspended; >> + struct icc_path *usb_ddr_icc_path; >> + struct icc_path *apps_usb_icc_path; >> }; >> >> +static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom); >> +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom); > Please get rid of these. We shouldn't need forward declarations. Will do in next version. > >> + >> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) >> { >> u32 reg; >> @@ -285,6 +307,101 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) >> return 0; >> } >> >> + >> +/** >> + * dwc3_qcom_interconnect_init() - Get interconnect path handles >> + * @qcom: Pointer to the concerned usb core. >> + * >> + */ >> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom) >> +{ >> + struct device *dev = qcom->dev; >> + int ret; >> + >> + if (!device_is_bound(&qcom->dwc3->dev)) >> + return -EPROBE_DEFER; > How is this supposed to work? I see that this was added in an earlier > revision of this patch series but there isn't any mention of why > device_is_bound() is used here. It would be great if there was a comment > detailing why this is necessary. It sounds like maximum_speed is > important? > > Furthermore, dwc3_qcom_interconnect_init() is called by > dwc3_qcom_probe() which is the function that registers the device for > qcom->dwc3->dev. If that device doesn't probe between the time it is > registered by dwc3_qcom_probe() and this function is called then we'll > fail dwc3_qcom_probe() with -EPROBE_DEFER. And that will remove the > qcom->dwc3->dev device from the platform bus because we call > of_platform_depopulate() on the error path of dwc3_qcom_probe(). > > So isn't this whole thing racy and can potentially lead us to a driver > probe loop where the wrapper (dwc3_qcom) and the core (dwc3) are probing > and we're trying to time it just right so that driver for dwc3 binds > before we setup interconnects? I don't know if dwc3 can communicate to > the wrapper but that would be more of a direct way to do this. Or maybe > the wrapper should try to read the DT property for maximum speed and > fallback to a worst case high bandwidth value if it can't figure it out > itself without help from dwc3 core. > This was added in V4 to address comments from Matthias in V3
https://patchwork.kernel.org/patch/11148587/
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |