| Message ID | 1585718145-29537-3-git-send-email-sanm@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | ADD interconnect support for Qualcomm DWC3 driver | expand |
On Tue, Jul 07, 2020 at 10:41:24AM +0530, Sandeep Maheswaram (Temp) wrote: > > On 7/1/2020 4:12 AM, Matthias Kaehlcke wrote: > > On Tue, Jun 16, 2020 at 01:38:49PM -0700, Matthias Kaehlcke wrote: > > > On Tue, Jun 16, 2020 at 10:22:47AM +0530, Sandeep Maheswaram (Temp) wrote: > > > > On 6/16/2020 1:12 AM, Matthias Kaehlcke wrote: > > > > > On Thu, Jun 04, 2020 at 04:16:31AM -0700, Stephen Boyd wrote: > > > > > > Quoting Sandeep Maheswaram (Temp) (2020-06-04 02:43:09) > > > > > > > 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 > > > > > > > > > @@ -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/ > > > > > > > > > > > > > Yes, that why I said: > > > > > > > > > > > > "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?" > > > > > > > > > > > > Can you please respond to the rest of my email? > > > > > I agree with Stephen that using device_is_bound() isn't a good option > > > > > in this case, when I suggested it I wasn't looking at the big picture > > > > > of how probing the core driver is triggered, sorry about that. > > > > > > > > > > Reading the speed from the DT with usb_get_maximum_speed() as Stephen > > > > > suggests would be an option, the inconvenient is that we then > > > > > essentially require the property to be defined, while the core driver > > > > > gets a suitable value from hardware registers. Not sure if the wrapper > > > > > driver could read from the same registers. > > > > > > > > > > One option could be to poll device_is_bound() for 100 ms (or so), with > > > > > sleeps between polls. It's not elegant but would probably work if we > > > > > don't find a better solution. > > > > if (np) > > > > ret = dwc3_qcom_of_register_core(pdev); > > > > else > > > > ret = dwc3_qcom_acpi_register_core(pdev); > > > > > > > > if (ret) { > > > > dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret); > > > > goto depopulate; > > > > } > > > > > > > > ret = dwc3_qcom_interconnect_init(qcom); > > > > if (ret) > > > > goto depopulate; > > > > > > > > qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); > > > > > > > > Before calling dwc3_qcom_interconnect_init we are checking > > > > > > > > if (ret) { > > > > dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret); > > > > goto depopulate; > > > > } > > > > > > > > Doesn't this condition confirm the core driver is probed? > > > Not really: > > > > > > // called under the hood by of_platform_populate() > > > static int really_probe(struct device *dev, struct device_driver *drv) > > > { > > > ... > > > > > > if (dev->bus->probe) { > > > ret = dev->bus->probe(dev); > > > if (ret) > > > goto probe_failed; > > > } else if (drv->probe) { > > > ret = drv->probe(dev); > > > if (ret) > > > goto probe_failed; > > > } > > > > > > ... > > > > > > probe_failed: > > > ... > > > > > > /* > > > * Ignore errors returned by ->probe so that the next driver can try > > > * its luck. > > > */ > > > ret = 0; > > > > > > ... > > > > > > return ret; > > > } > > > > > > As a result of_platform_populate() in dwc3_qcom_of_register_core() > > > returns 0 even when probing the device failed: > > > > > > [ 0.244339] dwc3-qcom a6f8800.usb: DBG: populate > > > [ 0.244772] dwc3 a600000.dwc3: DBG: dwc3_probe > > > [ 0.245237] dwc3 a600000.dwc3: DBG: dwc3_probe err: -517 > > > [ 0.245264] dwc3-qcom a6f8800.usb: DBG: populate (done) > > > [ 0.245317] dwc3-qcom a6f8800.usb: DBG: dwc3_qcom_interconnect_init() failed: -517 > > > > > > Probe fails because the interconnect stuff isn't ready yet, otherwise > > > it could access invalid data. > > > > > > A later _populate() is successful and the probing of the core is done > > > synchronously, i.e. after _populate() the core driver is fully > > > initialized: > > > > > > [ 3.898106] dwc3-qcom a6f8800.usb: DBG: populate > > > [ 3.908356] dwc3 a600000.dwc3: DBG: dwc3_probe > > > [ 4.205104] dwc3 a600000.dwc3: DBG: dwc3_probe (done) > > > [ 4.210305] dwc3-qcom a6f8800.usb: DBG: populate (done) > > > > > > The synchronous probing in _populate() suggests that using device_is_bound() > > > would actually be a valid option, either the core device was successfully > > > probed or not, there should be no race. > > > > > > I sent a patch that adds this check to dwc3_qcom_of_register_core(), which > > > is less confusing and makes clear that the core device is valid unless > > > this function returns an error: > > > > > > https://lore.kernel.org/patchwork/patch/1257279/ > > > > > > It might make sense to add your "driver core:Export the symbol > > > device_is_bound" patch, mine and this one to a single series. > > From the discussion on "driver core:Export the symbol device_is_bound" > > (https://patchwork.kernel.org/patch/11584225/) it is clear that > > this won't fly. The split dwc3 driver is considered a broken > > design. > > > > This is what Rob Herring said: > > > > We never should have had this split either in the DT binding nor > > driver(s) as if the SoC wrapper crap and licensed IP block are > > independent things. The thing to do here is either make the DWC3 code > > a library which drivers call (e.g. SDHCI) or add hooks into the DWC3 > > driver for platform specifics (e.g. Designware PCI). Neither is a > > simple solution though. > > > > That seems to be the desirable solution in the longer term, but it > > doesn't seem reasonable to me to expect you to fix this design issue > > to add interconnect support. > > > > Some possible options to move forward: > > > > - try to determine the max speed without involving the core device > > - select a reasonable default when 'maximum-speed' is not specified > > - use the core device to determine the max speed and pray > > Can we do as below to get speed > > qcom->max_speed = usb_get_maximum_speed(&qcom->dwc3->dev); Yes, that would get the maximum speed from the DT if it is specified. In case of USB_SPEED_UNKNOWN you probably want to assume it's USB_SPEED_SUPER, which in the worst case would result in the ICC running at a higher speed than needed.
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 @@ -13,6 +13,7 @@ #include <linux/module.h> #include <linux/kernel.h> #include <linux/extcon.h> +#include <linux/interconnect.h> #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/phy/phy.h> @@ -43,6 +44,14 @@ #define SDM845_QSCRATCH_SIZE 0x400 #define SDM845_DWC3_CORE_SIZE 0xcd00 +/* Interconnect path bandwidths in MBps */ +#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240) +#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700) +#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000) +#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500) +#define APPS_USB_AVG_BW 0 +#define APPS_USB_PEAK_BW MBps_to_icc(40) + struct dwc3_acpi_pdata { u32 qscratch_base_offset; u32 qscratch_base_size; @@ -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); + static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) { u32 reg; @@ -239,7 +253,7 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) { u32 val; - int i; + int i, ret; if (qcom->is_suspended) return 0; @@ -251,6 +265,10 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) for (i = qcom->num_clocks - 1; i >= 0; i--) clk_disable_unprepare(qcom->clks[i]); + ret = dwc3_qcom_interconnect_disable(qcom); + if (ret) + dev_warn(qcom->dev, "failed to disable interconnect %d\n", ret); + qcom->is_suspended = true; dwc3_qcom_enable_interrupts(qcom); @@ -276,6 +294,10 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) } } + ret = dwc3_qcom_interconnect_enable(qcom); + if (ret) + dev_warn(qcom->dev, "failed to enable interconnect %d\n", ret); + /* Clear existing events from PHY related to L2 in/out */ dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); @@ -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; + + qcom->usb_ddr_icc_path = of_icc_get(dev, "usb-ddr"); + if (IS_ERR(qcom->usb_ddr_icc_path)) { + dev_err(dev, "Error: (%ld) failed getting usb-ddr path\n", + PTR_ERR(qcom->usb_ddr_icc_path)); + return PTR_ERR(qcom->usb_ddr_icc_path); + } + + qcom->apps_usb_icc_path = of_icc_get(dev, "apps-usb"); + if (IS_ERR(qcom->apps_usb_icc_path)) { + dev_err(dev, "Error: (%ld) failed getting apps-usb path\n", + PTR_ERR(qcom->apps_usb_icc_path)); + return PTR_ERR(qcom->apps_usb_icc_path); + } + + ret = dwc3_qcom_interconnect_enable(qcom); + if (ret) { + dev_err(dev, "failed to enable interconnect %d\n", ret); + return ret; + } + + return 0; +} + +/** + * dwc3_qcom_interconnect_exit() - Release interconnect path handles + * @qcom: Pointer to the concerned usb core. + * + * This function is used to release interconnect path handle. + */ +static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) +{ + icc_put(qcom->usb_ddr_icc_path); + icc_put(qcom->apps_usb_icc_path); +} + +/* Currently we only use bandwidth level, so just "enable" interconnects */ +static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom) +{ + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); + int ret; + + if (dwc->maximum_speed == USB_SPEED_SUPER) + ret = icc_set_bw(qcom->usb_ddr_icc_path, + USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW); + else + ret = icc_set_bw(qcom->usb_ddr_icc_path, + USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW); + + if (ret) + return ret; + + ret = icc_set_bw(qcom->apps_usb_icc_path, + APPS_USB_AVG_BW, APPS_USB_PEAK_BW); + if (ret) + icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); + + return ret; +} + +/* To disable an interconnect, we just set its bandwidth to 0 */ +static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom) +{ + int ret; + + ret = icc_set_bw(qcom->usb_ddr_icc_path, 0, 0); + if (ret) + return ret; + + ret = icc_set_bw(qcom->apps_usb_icc_path, 0, 0); + if (ret) + goto err_reenable_memory_path; + + return 0; + + /* Re-enable things in the event of an error */ +err_reenable_memory_path: + dwc3_qcom_interconnect_enable(qcom); + + return ret; +} + static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) { struct dwc3_qcom *qcom = data; @@ -648,6 +765,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) goto depopulate; } + ret = dwc3_qcom_interconnect_init(qcom); + if (ret) + goto depopulate; + qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev); /* enable vbus override for device mode */ @@ -657,7 +778,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) /* register extcon to override sw_vbus on Vbus change later */ ret = dwc3_qcom_register_extcon(qcom); if (ret) - goto depopulate; + goto interconnect_exit; device_init_wakeup(&pdev->dev, 1); qcom->is_suspended = false; @@ -667,6 +788,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev) return 0; +interconnect_exit: + dwc3_qcom_interconnect_exit(qcom); depopulate: if (np) of_platform_depopulate(&pdev->dev); @@ -697,6 +820,7 @@ static int dwc3_qcom_remove(struct platform_device *pdev) } qcom->num_clocks = 0; + dwc3_qcom_interconnect_exit(qcom); reset_control_assert(qcom->resets); pm_runtime_allow(dev);