Re: [PATCH v11 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
Hi On 8/12/2020 12:27 PM, Felipe Balbi wrote: "Sandeep Maheswaram (Temp)" writes: Hi Felipe, On 7/28/2020 12:50 AM, Matthias Kaehlcke wrote: On Mon, Jul 27, 2020 at 10:36:36PM +0530, Sandeep Maheswaram wrote: Add interconnect support in dwc3-qcom driver to vote for bus bandwidth. This requires for two different paths - from USB to DDR. The other is from APPS to USB. Signed-off-by: Sandeep Maheswaram Signed-off-by: Chandana Kishori Chiluveru Reviewed-by: Matthias Kaehlcke Please ack if you are ok with this patch. What's the plan to get this upstream? Should I take dwc3-qcom patch and ignore the rest? Is there a hard-dependency on something else? Yes take dwc3-qcom patch only,the dt change is already in linux-next. -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 1/2] clk: qcom: gcc: Add genpd active wakeup flag for sc7180
Hi, On 8/8/2020 3:14 AM, Stephen Boyd wrote: Quoting Stephen Boyd (2020-06-11 15:46:58) Quoting Sandeep Maheswaram (2020-06-11 07:28:02) From: Taniya Das The USB client requires the usb30_prim gdsc to be kept active for certain use cases, thus add the GENPD_FLAG_ACTIVE_WAKEUP flag. Can you please describe more of what this is for? Once sentence doesn't tell me much at all. I guess that sometimes we want to wakeup from USB and so the usb gdsc should be marked as "maybe keep on for wakeups" with the GENPD_FLAG_ACTIVE_WAKEUP flag if the USB controller is wakeup enabled? Signed-off-by: Taniya Das --- Add a Fixes: tag too? And I assume we need to do this for all USB gdscs on various SoCs and maybe other GDSCs like PCIe. Any plans to fix those GDSCs? Any update here? I moved this change to usb driver code dwc3-qcom.c in v2 of this series https://patchwork.kernel.org/cover/11652281/ -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v11 1/2] usb: dwc3: qcom: Add interconnect support in dwc3 driver
Hi Felipe, On 7/28/2020 12:50 AM, Matthias Kaehlcke wrote: On Mon, Jul 27, 2020 at 10:36:36PM +0530, Sandeep Maheswaram wrote: Add interconnect support in dwc3-qcom driver to vote for bus bandwidth. This requires for two different paths - from USB to DDR. The other is from APPS to USB. Signed-off-by: Sandeep Maheswaram Signed-off-by: Chandana Kishori Chiluveru Reviewed-by: Matthias Kaehlcke Please ack if you are ok with this patch. -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
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(>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(>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.2
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
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(>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(>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?
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
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_modemode;
boolis_suspended;
boolpm_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(>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
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
On 5/26/2020 5:13 PM, Felipe Balbi wrote: Hi, Georgi Djakov writes: On 26.05.20 14:04, Sandeep Maheswaram (Temp) wrote: Hi Felipe, Please let me know how to go forward with this patch (don't top-post!) Please just add a patch to fix the allmodconfig error. Felipe has suggested to introduce a separate patch which exports the device_is_bound() function. This export should precede the addition of interconnect support. Also regarding the "depends on INTERCONNECT || !INTERCONNECT" change, no "depends on" would be needed, as we just made the interconnect framework bool. y'all have lost the current merge window, I guess. I'm not sure Greg will take last minute changes to drivers base and I have already sent him my pull request for v5.8. On the plus side, this gives you the chance to run hundreds of randbuilds with your patches. Hi Georgi, I am assuming that the patch which exports the device_is_bound() function will solve the allmodconfig error. Or do i need to change anything in dwc3 driver? -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
Hi Felipe,
Please let me know how to go forward with this patch
Regards
Sandeep
On 5/19/2020 12:05 AM, Bjorn Andersson wrote:
On Thu 14 May 23:29 PDT 2020, Felipe Balbi wrote:
Hi,
Georgi Djakov writes:
Sandeep Maheswaram writes:
+static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
+{
+ struct device *dev = qcom->dev;
+ int ret;
+
+ if (!device_is_bound(>dwc3->dev))
+ return -EPROBE_DEFER;
this breaks allmodconfig. I'm dropping this series from my queue for
this merge window.
Sorry, I meant this patch ;-)
I guess that's due to INTERCONNECT being a module. There is currently a
I believe it's because of this:
ERROR: modpost: "device_is_bound" [drivers/usb/dwc3/dwc3-qcom.ko] undefined!
discussion about this with Viresh and Georgi in response to another
automated build failure. Viresh suggests changing CONFIG_INTERCONNECT
from tristate to bool, which seems sensible to me given that interconnect
is a core subsystem.
The problem you are talking about would arise when INTERCONNECT=m and
USB_DWC3_QCOM=y and it definitely exists here and could be triggered with
randconfig build. So i suggest to squash also the diff below.
Thanks,
Georgi
---8<---
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 206caa0ea1c6..6661788b1a76 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -129,6 +129,7 @@ config USB_DWC3_QCOM
tristate "Qualcomm Platform"
depends on ARCH_QCOM || COMPILE_TEST
depends on EXTCON || !EXTCON
+ depends on INTERCONNECT || !INTERCONNECT
I would prefer to see a patch adding EXPORT_SYMBOL_GPL() to device_is_bound()
Agree, but just to clarify, that these are two separate issues that need to
be fixed. The device_is_bound() is the first one and USB_DWC3_QCOM=y combined
with INTERCONNECT=m is the second one.
If INTERCONNECT=m, QCOM3 shouldn't be y. I think the following is
enough:
depends on INTERCONNECT=y || INTERCONNECT=USB_DWC3_QCOM
This misses the case where INTERCONNECT=n and USB_DWC3_QCOM=[ym] which
I don't see a reason for breaking.
But if only INTERCONNECT where a bool, then we don't need to specify a
depends on, because it will either be there, or the stubs will.
We've come to this conclusion in a lot of different frameworks and I
don't see why we should do this differently with INTERCONNECT.
Regards,
Bjorn
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver
Hi Felipe, Any update about landing this series. Regards Sandeep On 4/30/2020 12:05 AM, Matthias Kaehlcke wrote: Hi Felipe, all patches of this series have been reviewed and there are no outstanding comments, so I guess it should be ready to land? Thanks Matthias On Wed, Apr 01, 2020 at 10:45:41AM +0530, Sandeep Maheswaram wrote: This path series aims to add interconnect support in dwc3-qcom driver on SDM845 and SC7180 SoCs. Changes from v6 -> v7 > [PATCH 2/4] Fixed review comments from Matthias in DWC3 driver. > Other patches remain unchanged. Changes from v5 -> v6 > [PATCH 1/4] Addressed comments from Rob. > [PATCH 2/4] Fixed review comments from Matthias in DWC3 driver. > [PATCH 3/4] Ignoring 80 char limit in defining interconnect paths. > Added [PATCH 4/4] in this series. Adding interconnect nodes for SC7180. Depends on patch https://patchwork.kernel.org/patch/11417989/. Changes from v4 -> v5 > [PATCH 1/3] Added the interconnect properties in yaml. This patch depends on series https://patchwork.kernel.org/cover/11372641/. > [PATCH 2/3] Fixed review comments from Matthias in DWC3 driver. > [PATCH 3/3] Modified as per the new interconnect nodes in sdm845. Depends on series https://patchwork.kernel.org/cover/11372211/. Changes from v3 -> v4 > Fixed review comments from Matthias > [PATCH 1/3] and [PATCH 3/3] remains unchanged Changes from v2 -> v3 > Fixed review comments from Matthias and Manu > changed the functions prefix from usb_* to dwc3_qcom_* Changes since V1: > Comments by Georgi Djakov on "[PATCH 2/3]" addressed > [PATCH 1/3] and [PATCH 3/3] remains unchanged Sandeep Maheswaram (4): dt-bindings: usb: qcom,dwc3: Introduce interconnect properties for Qualcomm DWC3 driver usb: dwc3: qcom: Add interconnect support in dwc3 driver arm64: dts: qcom: sdm845: Add interconnect properties for USB arm64: dts: qcom: sc7180: Add interconnect properties for USB .../devicetree/bindings/usb/qcom,dwc3.yaml | 8 ++ arch/arm64/boot/dts/qcom/sc7180.dtsi | 4 + arch/arm64/boot/dts/qcom/sdm845.dtsi | 8 ++ drivers/usb/dwc3/dwc3-qcom.c | 128 - 4 files changed, 146 insertions(+), 2 deletions(-) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
