linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver
@ 2020-04-01  5:15 Sandeep Maheswaram
  2020-04-01  5:15 ` [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties " Sandeep Maheswaram
                   ` (4 more replies)
  0 siblings, 5 replies; 37+ messages in thread
From: Sandeep Maheswaram @ 2020-04-01  5:15 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Felipe Balbi, Stephen Boyd, Doug Anderson,
	Matthias Kaehlcke
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties for Qualcomm DWC3 driver
  2020-04-01  5:15 [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Sandeep Maheswaram
@ 2020-04-01  5:15 ` Sandeep Maheswaram
  2020-04-14 15:22   ` Rob Herring
  2020-04-14 20:30   ` Stephen Boyd
  2020-04-01  5:15 ` [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver Sandeep Maheswaram
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 37+ messages in thread
From: Sandeep Maheswaram @ 2020-04-01  5:15 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Felipe Balbi, Stephen Boyd, Doug Anderson,
	Matthias Kaehlcke
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

Add documentation for the interconnects and interconnect-names
properties for USB.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
 Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index 17e22ff..ec1ec47 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -70,6 +70,14 @@ properties:
   resets:
     maxItems: 1
 
+  interconnects:
+    maxItems: 2
+
+  interconnect-names:
+    items:
+      - const: usb-ddr
+      - const: apps-usb
+
   interrupts:
     items:
       - description: The interrupt that is asserted
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-04-01  5:15 [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Sandeep Maheswaram
  2020-04-01  5:15 ` [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties " Sandeep Maheswaram
@ 2020-04-01  5:15 ` Sandeep Maheswaram
  2020-05-14 11:29   ` Felipe Balbi
  2020-06-03 17:36   ` Stephen Boyd
  2020-04-01  5:15 ` [PATCH v7 3/4] arm64: dts: qcom: sdm845: Add interconnect properties for USB Sandeep Maheswaram
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 37+ messages in thread
From: Sandeep Maheswaram @ 2020-04-01  5:15 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Felipe Balbi, Stephen Boyd, Doug Anderson,
	Matthias Kaehlcke
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

Add interconnect support in dwc3-qcom driver to vote for bus
bandwidth.

This requires for two different paths - from USB master to
DDR slave. The other is from APPS master to USB slave.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 2 deletions(-)

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);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v7 3/4] arm64: dts: qcom: sdm845: Add interconnect properties for USB
  2020-04-01  5:15 [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Sandeep Maheswaram
  2020-04-01  5:15 ` [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties " Sandeep Maheswaram
  2020-04-01  5:15 ` [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver Sandeep Maheswaram
@ 2020-04-01  5:15 ` Sandeep Maheswaram
  2020-04-14 20:31   ` Stephen Boyd
  2020-04-01  5:15 ` [PATCH v7 4/4] arm64: dts: qcom: sc7180: " Sandeep Maheswaram
  2020-04-29 18:35 ` [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Matthias Kaehlcke
  4 siblings, 1 reply; 37+ messages in thread
From: Sandeep Maheswaram @ 2020-04-01  5:15 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Felipe Balbi, Stephen Boyd, Doug Anderson,
	Matthias Kaehlcke
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

Populate USB DT nodes with interconnect properties.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 8f926b5..860d5c2 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -3097,6 +3097,10 @@
 
 			resets = <&gcc GCC_USB30_PRIM_BCR>;
 
+			interconnects = <&aggre2_noc MASTER_USB3_0 &mem_noc SLAVE_EBI1>,
+					<&gladiator_noc MASTER_APPSS_PROC &config_noc SLAVE_USB3_0>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
 			usb_1_dwc3: dwc3@a600000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a600000 0 0xcd00>;
@@ -3141,6 +3145,10 @@
 
 			resets = <&gcc GCC_USB30_SEC_BCR>;
 
+			interconnects = <&aggre2_noc MASTER_USB3_1 &mem_noc SLAVE_EBI1>,
+					<&gladiator_noc MASTER_APPSS_PROC &config_noc SLAVE_USB3_1>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
 			usb_2_dwc3: dwc3@a800000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a800000 0 0xcd00>;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 37+ messages in thread

* [PATCH v7 4/4] arm64: dts: qcom: sc7180: Add interconnect properties for USB
  2020-04-01  5:15 [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Sandeep Maheswaram
                   ` (2 preceding siblings ...)
  2020-04-01  5:15 ` [PATCH v7 3/4] arm64: dts: qcom: sdm845: Add interconnect properties for USB Sandeep Maheswaram
@ 2020-04-01  5:15 ` Sandeep Maheswaram
  2020-04-14 20:32   ` Stephen Boyd
  2020-04-29 18:35 ` [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Matthias Kaehlcke
  4 siblings, 1 reply; 37+ messages in thread
From: Sandeep Maheswaram @ 2020-04-01  5:15 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Felipe Balbi, Stephen Boyd, Doug Anderson,
	Matthias Kaehlcke
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

Populate USB DT nodes with interconnect properties.

Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7180.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 998f101..bd0d3a7 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -1447,6 +1447,10 @@
 
 			resets = <&gcc GCC_USB30_PRIM_BCR>;
 
+			interconnects = <&aggre2_noc MASTER_USB3 &mc_virt SLAVE_EBI1>,
+					<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_USB3>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
 			usb_1_dwc3: dwc3@a600000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a600000 0 0xe000>;
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties for Qualcomm DWC3 driver
  2020-04-01  5:15 ` [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties " Sandeep Maheswaram
@ 2020-04-14 15:22   ` Rob Herring
  2020-04-14 20:30   ` Stephen Boyd
  1 sibling, 0 replies; 37+ messages in thread
From: Rob Herring @ 2020-04-14 15:22 UTC (permalink / raw)
  To: Sandeep Maheswaram
  Cc: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Stephen Boyd,
	Doug Anderson, Matthias Kaehlcke, linux-arm-msm, linux-usb,
	devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

On Wed,  1 Apr 2020 10:45:42 +0530, Sandeep Maheswaram wrote:
> Add documentation for the interconnects and interconnect-names
> properties for USB.
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties for Qualcomm DWC3 driver
  2020-04-01  5:15 ` [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties " Sandeep Maheswaram
  2020-04-14 15:22   ` Rob Herring
@ 2020-04-14 20:30   ` Stephen Boyd
  1 sibling, 0 replies; 37+ messages in thread
From: Stephen Boyd @ 2020-04-14 20:30 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Doug Anderson, Felipe Balbi,
	Greg Kroah-Hartman, Mark Rutland, Matthias Kaehlcke, Rob Herring,
	Sandeep Maheswaram
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

Quoting Sandeep Maheswaram (2020-03-31 22:15:42)
> Add documentation for the interconnects and interconnect-names
> properties for USB.
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 3/4] arm64: dts: qcom: sdm845: Add interconnect properties for USB
  2020-04-01  5:15 ` [PATCH v7 3/4] arm64: dts: qcom: sdm845: Add interconnect properties for USB Sandeep Maheswaram
@ 2020-04-14 20:31   ` Stephen Boyd
  0 siblings, 0 replies; 37+ messages in thread
From: Stephen Boyd @ 2020-04-14 20:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Doug Anderson, Felipe Balbi,
	Greg Kroah-Hartman, Mark Rutland, Matthias Kaehlcke, Rob Herring,
	Sandeep Maheswaram
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

Quoting Sandeep Maheswaram (2020-03-31 22:15:44)
> Populate USB DT nodes with interconnect properties.
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 4/4] arm64: dts: qcom: sc7180: Add interconnect properties for USB
  2020-04-01  5:15 ` [PATCH v7 4/4] arm64: dts: qcom: sc7180: " Sandeep Maheswaram
@ 2020-04-14 20:32   ` Stephen Boyd
  0 siblings, 0 replies; 37+ messages in thread
From: Stephen Boyd @ 2020-04-14 20:32 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Doug Anderson, Felipe Balbi,
	Greg Kroah-Hartman, Mark Rutland, Matthias Kaehlcke, Rob Herring,
	Sandeep Maheswaram
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

Quoting Sandeep Maheswaram (2020-03-31 22:15:45)
> Populate USB DT nodes with interconnect properties.
> 
> Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver
  2020-04-01  5:15 [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Sandeep Maheswaram
                   ` (3 preceding siblings ...)
  2020-04-01  5:15 ` [PATCH v7 4/4] arm64: dts: qcom: sc7180: " Sandeep Maheswaram
@ 2020-04-29 18:35 ` Matthias Kaehlcke
  2020-05-08  6:29   ` Sandeep Maheswaram (Temp)
  4 siblings, 1 reply; 37+ messages in thread
From: Matthias Kaehlcke @ 2020-04-29 18:35 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Felipe Balbi,
	Stephen Boyd, Doug Anderson, linux-arm-msm, linux-usb,
	devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

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
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver
  2020-04-29 18:35 ` [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Matthias Kaehlcke
@ 2020-05-08  6:29   ` Sandeep Maheswaram (Temp)
  2020-05-14 10:49     ` Felipe Balbi
  0 siblings, 1 reply; 37+ messages in thread
From: Sandeep Maheswaram (Temp) @ 2020-05-08  6:29 UTC (permalink / raw)
  To: Matthias Kaehlcke, Felipe Balbi
  Cc: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Stephen Boyd, Doug Anderson, linux-arm-msm,
	linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver
  2020-05-08  6:29   ` Sandeep Maheswaram (Temp)
@ 2020-05-14 10:49     ` Felipe Balbi
  0 siblings, 0 replies; 37+ messages in thread
From: Felipe Balbi @ 2020-05-14 10:49 UTC (permalink / raw)
  To: Sandeep Maheswaram (Temp), Matthias Kaehlcke
  Cc: Andy Gross, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Stephen Boyd, Doug Anderson, linux-arm-msm,
	linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

[-- Attachment #1: Type: text/plain, Size: 151 bytes --]

"Sandeep Maheswaram (Temp)" <sanm@codeaurora.org> writes:

> Hi Felipe,
>
> Any update about landing this series.

in my tree now

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-04-01  5:15 ` [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver Sandeep Maheswaram
@ 2020-05-14 11:29   ` Felipe Balbi
  2020-05-14 11:30     ` Felipe Balbi
  2020-06-03 17:36   ` Stephen Boyd
  1 sibling, 1 reply; 37+ messages in thread
From: Felipe Balbi @ 2020-05-14 11:29 UTC (permalink / raw)
  To: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, Matthias Kaehlcke
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]


Hi,

Sandeep Maheswaram <sanm@codeaurora.org> writes:
> +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;

this breaks allmodconfig. I'm dropping this series from my queue for
this merge window.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-14 11:29   ` Felipe Balbi
@ 2020-05-14 11:30     ` Felipe Balbi
  2020-05-14 17:13       ` Matthias Kaehlcke
  0 siblings, 1 reply; 37+ messages in thread
From: Felipe Balbi @ 2020-05-14 11:30 UTC (permalink / raw)
  To: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, Matthias Kaehlcke
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

Felipe Balbi <balbi@kernel.org> writes:

> Hi,
>
> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>> +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;
>
> this breaks allmodconfig. I'm dropping this series from my queue for
> this merge window.

Sorry, I meant this patch ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-14 11:30     ` Felipe Balbi
@ 2020-05-14 17:13       ` Matthias Kaehlcke
  2020-05-14 21:02         ` Georgi Djakov
  0 siblings, 1 reply; 37+ messages in thread
From: Matthias Kaehlcke @ 2020-05-14 17:13 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, linux-arm-msm, linux-usb, devicetree,
	linux-kernel, Manu Gautam, Chandana Kishori Chiluveru,
	Viresh Kumar, Georgi Djakov

On Thu, May 14, 2020 at 02:30:28PM +0300, Felipe Balbi wrote:
> Felipe Balbi <balbi@kernel.org> writes:
> 
> > Hi,
> >
> > Sandeep Maheswaram <sanm@codeaurora.org> writes:
> >> +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;
> >
> > 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
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.

Let's hold back with this patch/series then until that is sorted out.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-14 17:13       ` Matthias Kaehlcke
@ 2020-05-14 21:02         ` Georgi Djakov
  2020-05-15  3:57           ` Viresh Kumar
  2020-05-15  5:54           ` Felipe Balbi
  0 siblings, 2 replies; 37+ messages in thread
From: Georgi Djakov @ 2020-05-14 21:02 UTC (permalink / raw)
  To: Matthias Kaehlcke, Felipe Balbi
  Cc: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, linux-arm-msm, linux-usb, devicetree,
	linux-kernel, Manu Gautam, Chandana Kishori Chiluveru,
	Viresh Kumar

On 5/14/20 20:13, Matthias Kaehlcke wrote:
> On Thu, May 14, 2020 at 02:30:28PM +0300, Felipe Balbi wrote:
>> Felipe Balbi <balbi@kernel.org> writes:
>>
>>> Hi,
>>>
>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>> +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;
>>>
>>> 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
 	depends on (OF || ACPI)
 	default USB_DWC3
 	help


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-14 21:02         ` Georgi Djakov
@ 2020-05-15  3:57           ` Viresh Kumar
  2020-05-15  4:23             ` Manu Gautam
  2020-05-15  5:54           ` Felipe Balbi
  1 sibling, 1 reply; 37+ messages in thread
From: Viresh Kumar @ 2020-05-15  3:57 UTC (permalink / raw)
  To: Georgi Djakov
  Cc: Matthias Kaehlcke, Felipe Balbi, Sandeep Maheswaram, Andy Gross,
	Bjorn Andersson, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	Stephen Boyd, Doug Anderson, linux-arm-msm,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	devicetree, Linux Kernel Mailing List, Manu Gautam,
	Chandana Kishori Chiluveru

On Fri, 15 May 2020 at 02:33, Georgi Djakov <georgi.djakov@linaro.org> wrote:

> ---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

Again, as I said yesterday. This looks incorrect and may not fix the problem..

With this we will be able to select USB_DWC3_QCOM even when INTERCONNECT=m.

What we perhaps need here is:
depends on INTERCONNECT != m

--
viresh

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-15  3:57           ` Viresh Kumar
@ 2020-05-15  4:23             ` Manu Gautam
  2020-05-15  4:26               ` Viresh Kumar
  0 siblings, 1 reply; 37+ messages in thread
From: Manu Gautam @ 2020-05-15  4:23 UTC (permalink / raw)
  To: Viresh Kumar, Georgi Djakov
  Cc: Matthias Kaehlcke, Felipe Balbi, Sandeep Maheswaram, Andy Gross,
	Bjorn Andersson, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	Stephen Boyd, Doug Anderson, linux-arm-msm,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	devicetree, Linux Kernel Mailing List,
	Chandana Kishori Chiluveru

Hi,

On 5/15/2020 9:27 AM, Viresh Kumar wrote:
> On Fri, 15 May 2020 at 02:33, Georgi Djakov <georgi.djakov@linaro.org> wrote:
>
>> ---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
> Again, as I said yesterday. This looks incorrect and may not fix the problem..
>
> With this we will be able to select USB_DWC3_QCOM even when INTERCONNECT=m.

DWC3_QCOM in that case would be 'm' if INTERCONNECT =m and
that should be fine?


> What we perhaps need here is:
> depends on INTERCONNECT != m
>
> --
> viresh

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-15  4:23             ` Manu Gautam
@ 2020-05-15  4:26               ` Viresh Kumar
  0 siblings, 0 replies; 37+ messages in thread
From: Viresh Kumar @ 2020-05-15  4:26 UTC (permalink / raw)
  To: Manu Gautam
  Cc: Georgi Djakov, Matthias Kaehlcke, Felipe Balbi,
	Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, linux-arm-msm,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	devicetree, Linux Kernel Mailing List,
	Chandana Kishori Chiluveru

On 15-05-20, 09:53, Manu Gautam wrote:
> Hi,
> 
> On 5/15/2020 9:27 AM, Viresh Kumar wrote:
> > On Fri, 15 May 2020 at 02:33, Georgi Djakov <georgi.djakov@linaro.org> wrote:
> >
> >> ---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
> > Again, as I said yesterday. This looks incorrect and may not fix the problem..
> >
> > With this we will be able to select USB_DWC3_QCOM even when INTERCONNECT=m.
> 
> DWC3_QCOM in that case would be 'm' if INTERCONNECT =m and
> that should be fine?

Ahh, I was speaking in context of PM_OPP where it is a bool. Perhaps
this works just fine with DWC3_QCOM, sorry for the confusion.

-- 
viresh

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-14 21:02         ` Georgi Djakov
  2020-05-15  3:57           ` Viresh Kumar
@ 2020-05-15  5:54           ` Felipe Balbi
  2020-05-15  6:11             ` Georgi Djakov
  1 sibling, 1 reply; 37+ messages in thread
From: Felipe Balbi @ 2020-05-15  5:54 UTC (permalink / raw)
  To: Georgi Djakov, Matthias Kaehlcke
  Cc: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, linux-arm-msm, linux-usb, devicetree,
	linux-kernel, Manu Gautam, Chandana Kishori Chiluveru,
	Viresh Kumar

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]


Hi,

Georgi Djakov <georgi.djakov@linaro.org> writes:
> On 5/14/20 20:13, Matthias Kaehlcke wrote:
>> On Thu, May 14, 2020 at 02:30:28PM +0300, Felipe Balbi wrote:
>>> Felipe Balbi <balbi@kernel.org> writes:
>>>
>>>> Hi,
>>>>
>>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>>> +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;
>>>>
>>>> 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()

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-15  5:54           ` Felipe Balbi
@ 2020-05-15  6:11             ` Georgi Djakov
  2020-05-15  6:29               ` Felipe Balbi
  0 siblings, 1 reply; 37+ messages in thread
From: Georgi Djakov @ 2020-05-15  6:11 UTC (permalink / raw)
  To: Felipe Balbi, Matthias Kaehlcke
  Cc: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, linux-arm-msm, linux-usb, devicetree,
	linux-kernel, Manu Gautam, Chandana Kishori Chiluveru,
	Viresh Kumar

Hi,

On 5/15/20 08:54, Felipe Balbi wrote:
> 
> Hi,
> 
> Georgi Djakov <georgi.djakov@linaro.org> writes:
>> On 5/14/20 20:13, Matthias Kaehlcke wrote:
>>> On Thu, May 14, 2020 at 02:30:28PM +0300, Felipe Balbi wrote:
>>>> Felipe Balbi <balbi@kernel.org> writes:
>>>>
>>>>> Hi,
>>>>>
>>>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>>>> +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;
>>>>>
>>>>> 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.

Thanks,
Georgi

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-15  6:11             ` Georgi Djakov
@ 2020-05-15  6:29               ` Felipe Balbi
  2020-05-18 18:35                 ` Bjorn Andersson
  0 siblings, 1 reply; 37+ messages in thread
From: Felipe Balbi @ 2020-05-15  6:29 UTC (permalink / raw)
  To: Georgi Djakov, Matthias Kaehlcke
  Cc: Sandeep Maheswaram, Andy Gross, Bjorn Andersson,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, linux-arm-msm, linux-usb, devicetree,
	linux-kernel, Manu Gautam, Chandana Kishori Chiluveru,
	Viresh Kumar

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]


Hi,

Georgi Djakov <georgi.djakov@linaro.org> writes:
>>>>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>>>>> +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;
>>>>>>
>>>>>> 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

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-15  6:29               ` Felipe Balbi
@ 2020-05-18 18:35                 ` Bjorn Andersson
  2020-05-19  3:29                   ` Viresh Kumar
  2020-05-26 11:04                   ` Sandeep Maheswaram (Temp)
  0 siblings, 2 replies; 37+ messages in thread
From: Bjorn Andersson @ 2020-05-18 18:35 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Georgi Djakov, Matthias Kaehlcke, Sandeep Maheswaram, Andy Gross,
	Greg Kroah-Hartman, Rob Herring, Mark Rutland, Stephen Boyd,
	Doug Anderson, linux-arm-msm, linux-usb, devicetree,
	linux-kernel, Manu Gautam, Chandana Kishori Chiluveru,
	Viresh Kumar

On Thu 14 May 23:29 PDT 2020, Felipe Balbi wrote:

> 
> Hi,
> 
> Georgi Djakov <georgi.djakov@linaro.org> writes:
> >>>>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
> >>>>>>> +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;
> >>>>>>
> >>>>>> 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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-18 18:35                 ` Bjorn Andersson
@ 2020-05-19  3:29                   ` Viresh Kumar
  2020-05-26 11:04                   ` Sandeep Maheswaram (Temp)
  1 sibling, 0 replies; 37+ messages in thread
From: Viresh Kumar @ 2020-05-19  3:29 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Felipe Balbi, Georgi Djakov, Matthias Kaehlcke,
	Sandeep Maheswaram, Andy Gross, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Stephen Boyd, Doug Anderson, linux-arm-msm,
	linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

On 18-05-20, 11:35, Bjorn Andersson wrote:
> 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.

INTERCONNECT is a bool now and the patch has been pushed to linux-next
already.

-- 
viresh

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-18 18:35                 ` Bjorn Andersson
  2020-05-19  3:29                   ` Viresh Kumar
@ 2020-05-26 11:04                   ` Sandeep Maheswaram (Temp)
  2020-05-26 11:34                     ` Georgi Djakov
  1 sibling, 1 reply; 37+ messages in thread
From: Sandeep Maheswaram (Temp) @ 2020-05-26 11:04 UTC (permalink / raw)
  To: Bjorn Andersson, Felipe Balbi
  Cc: Georgi Djakov, Matthias Kaehlcke, Andy Gross, Greg Kroah-Hartman,
	Rob Herring, Mark Rutland, Stephen Boyd, Doug Anderson,
	linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Viresh Kumar

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 <georgi.djakov@linaro.org> writes:
>>>>>>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>>>>>>> +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;
>>>>>>>> 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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-26 11:04                   ` Sandeep Maheswaram (Temp)
@ 2020-05-26 11:34                     ` Georgi Djakov
  2020-05-26 11:43                       ` Felipe Balbi
  0 siblings, 1 reply; 37+ messages in thread
From: Georgi Djakov @ 2020-05-26 11:34 UTC (permalink / raw)
  To: Sandeep Maheswaram (Temp), Bjorn Andersson, Felipe Balbi
  Cc: Matthias Kaehlcke, Andy Gross, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Stephen Boyd, Doug Anderson, linux-arm-msm,
	linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Viresh Kumar

On 26.05.20 14:04, Sandeep Maheswaram (Temp) wrote:
> Hi Felipe,
> 
> Please let me know how to go forward with this patch
> 

Hi Sandeep,

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.

Thanks,
Georgi

> 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 <georgi.djakov@linaro.org> writes:
>>>>>>>>> Sandeep Maheswaram <sanm@codeaurora.org> writes:
>>>>>>>>>> +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;
>>>>>>>>> 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
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-26 11:34                     ` Georgi Djakov
@ 2020-05-26 11:43                       ` Felipe Balbi
  2020-05-28  9:24                         ` Sandeep Maheswaram (Temp)
  0 siblings, 1 reply; 37+ messages in thread
From: Felipe Balbi @ 2020-05-26 11:43 UTC (permalink / raw)
  To: Georgi Djakov, Sandeep Maheswaram (Temp), Bjorn Andersson
  Cc: Matthias Kaehlcke, Andy Gross, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Stephen Boyd, Doug Anderson, linux-arm-msm,
	linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Viresh Kumar

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]


Hi,

Georgi Djakov <georgi.djakov@linaro.org> 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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-05-26 11:43                       ` Felipe Balbi
@ 2020-05-28  9:24                         ` Sandeep Maheswaram (Temp)
  0 siblings, 0 replies; 37+ messages in thread
From: Sandeep Maheswaram (Temp) @ 2020-05-28  9:24 UTC (permalink / raw)
  To: Felipe Balbi, Georgi Djakov, Bjorn Andersson
  Cc: Matthias Kaehlcke, Andy Gross, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, Stephen Boyd, Doug Anderson, linux-arm-msm,
	linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Viresh Kumar


On 5/26/2020 5:13 PM, Felipe Balbi wrote:
> Hi,
>
> Georgi Djakov <georgi.djakov@linaro.org> 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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-04-01  5:15 ` [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver Sandeep Maheswaram
  2020-05-14 11:29   ` Felipe Balbi
@ 2020-06-03 17:36   ` Stephen Boyd
  2020-06-04  9:43     ` Sandeep Maheswaram (Temp)
  1 sibling, 1 reply; 37+ messages in thread
From: Stephen Boyd @ 2020-06-03 17:36 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Doug Anderson, Felipe Balbi,
	Greg Kroah-Hartman, Mark Rutland, Matthias Kaehlcke, Rob Herring,
	Sandeep Maheswaram
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru, Sandeep Maheswaram

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.

> +
>  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.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-06-03 17:36   ` Stephen Boyd
@ 2020-06-04  9:43     ` Sandeep Maheswaram (Temp)
  2020-06-04 11:16       ` Stephen Boyd
  0 siblings, 1 reply; 37+ messages in thread
From: Sandeep Maheswaram (Temp) @ 2020-06-04  9:43 UTC (permalink / raw)
  To: Stephen Boyd, Andy Gross, Bjorn Andersson, Doug Anderson,
	Felipe Balbi, Greg Kroah-Hartman, Mark Rutland,
	Matthias Kaehlcke, Rob Herring
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru


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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-06-04  9:43     ` Sandeep Maheswaram (Temp)
@ 2020-06-04 11:16       ` Stephen Boyd
  2020-06-15 19:42         ` Matthias Kaehlcke
  0 siblings, 1 reply; 37+ messages in thread
From: Stephen Boyd @ 2020-06-04 11:16 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Doug Anderson, Felipe Balbi,
	Greg Kroah-Hartman, Mark Rutland, Matthias Kaehlcke, Rob Herring,
	Sandeep Maheswaram
  Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

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?

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-06-04 11:16       ` Stephen Boyd
@ 2020-06-15 19:42         ` Matthias Kaehlcke
  2020-06-16  4:52           ` Sandeep Maheswaram (Temp)
  0 siblings, 1 reply; 37+ messages in thread
From: Matthias Kaehlcke @ 2020-06-15 19:42 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Doug Anderson, Felipe Balbi,
	Greg Kroah-Hartman, Mark Rutland, Rob Herring,
	Sandeep Maheswaram, linux-arm-msm, linux-usb, devicetree,
	linux-kernel, Manu Gautam, Chandana Kishori Chiluveru

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.

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-06-15 19:42         ` Matthias Kaehlcke
@ 2020-06-16  4:52           ` Sandeep Maheswaram (Temp)
  2020-06-16 20:38             ` Matthias Kaehlcke
  0 siblings, 1 reply; 37+ messages in thread
From: Sandeep Maheswaram (Temp) @ 2020-06-16  4:52 UTC (permalink / raw)
  To: Matthias Kaehlcke, Stephen Boyd
  Cc: Andy Gross, Bjorn Andersson, Doug Anderson, Felipe Balbi,
	Greg Kroah-Hartman, Mark Rutland, Rob Herring, linux-arm-msm,
	linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru


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?

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-06-16  4:52           ` Sandeep Maheswaram (Temp)
@ 2020-06-16 20:38             ` Matthias Kaehlcke
  2020-06-30 22:42               ` Matthias Kaehlcke
  0 siblings, 1 reply; 37+ messages in thread
From: Matthias Kaehlcke @ 2020-06-16 20:38 UTC (permalink / raw)
  To: Sandeep Maheswaram (Temp)
  Cc: Stephen Boyd, Andy Gross, Bjorn Andersson, Doug Anderson,
	Felipe Balbi, Greg Kroah-Hartman, Mark Rutland, Rob Herring,
	linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

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.

Thanks

Matthias

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-06-16 20:38             ` Matthias Kaehlcke
@ 2020-06-30 22:42               ` Matthias Kaehlcke
  2020-07-07  5:11                 ` Sandeep Maheswaram (Temp)
  0 siblings, 1 reply; 37+ messages in thread
From: Matthias Kaehlcke @ 2020-06-30 22:42 UTC (permalink / raw)
  To: Sandeep Maheswaram (Temp)
  Cc: Stephen Boyd, Andy Gross, Bjorn Andersson, Doug Anderson,
	Felipe Balbi, Greg Kroah-Hartman, Mark Rutland, Rob Herring,
	linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-06-30 22:42               ` Matthias Kaehlcke
@ 2020-07-07  5:11                 ` Sandeep Maheswaram (Temp)
  2020-07-07 15:52                   ` Matthias Kaehlcke
  0 siblings, 1 reply; 37+ messages in thread
From: Sandeep Maheswaram (Temp) @ 2020-07-07  5:11 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Stephen Boyd, Andy Gross, Bjorn Andersson, Doug Anderson,
	Felipe Balbi, Greg Kroah-Hartman, Mark Rutland, Rob Herring,
	linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru


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);

as they were doing similarly in below code to get mode

qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver
  2020-07-07  5:11                 ` Sandeep Maheswaram (Temp)
@ 2020-07-07 15:52                   ` Matthias Kaehlcke
  0 siblings, 0 replies; 37+ messages in thread
From: Matthias Kaehlcke @ 2020-07-07 15:52 UTC (permalink / raw)
  To: Sandeep Maheswaram (Temp)
  Cc: Stephen Boyd, Andy Gross, Bjorn Andersson, Doug Anderson,
	Felipe Balbi, Greg Kroah-Hartman, Mark Rutland, Rob Herring,
	linux-arm-msm, linux-usb, devicetree, linux-kernel, Manu Gautam,
	Chandana Kishori Chiluveru

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.

^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, other threads:[~2020-07-07 15:52 UTC | newest]

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01  5:15 [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Sandeep Maheswaram
2020-04-01  5:15 ` [PATCH v7 1/4] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties " Sandeep Maheswaram
2020-04-14 15:22   ` Rob Herring
2020-04-14 20:30   ` Stephen Boyd
2020-04-01  5:15 ` [PATCH v7 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver Sandeep Maheswaram
2020-05-14 11:29   ` Felipe Balbi
2020-05-14 11:30     ` Felipe Balbi
2020-05-14 17:13       ` Matthias Kaehlcke
2020-05-14 21:02         ` Georgi Djakov
2020-05-15  3:57           ` Viresh Kumar
2020-05-15  4:23             ` Manu Gautam
2020-05-15  4:26               ` Viresh Kumar
2020-05-15  5:54           ` Felipe Balbi
2020-05-15  6:11             ` Georgi Djakov
2020-05-15  6:29               ` Felipe Balbi
2020-05-18 18:35                 ` Bjorn Andersson
2020-05-19  3:29                   ` Viresh Kumar
2020-05-26 11:04                   ` Sandeep Maheswaram (Temp)
2020-05-26 11:34                     ` Georgi Djakov
2020-05-26 11:43                       ` Felipe Balbi
2020-05-28  9:24                         ` Sandeep Maheswaram (Temp)
2020-06-03 17:36   ` Stephen Boyd
2020-06-04  9:43     ` Sandeep Maheswaram (Temp)
2020-06-04 11:16       ` Stephen Boyd
2020-06-15 19:42         ` Matthias Kaehlcke
2020-06-16  4:52           ` Sandeep Maheswaram (Temp)
2020-06-16 20:38             ` Matthias Kaehlcke
2020-06-30 22:42               ` Matthias Kaehlcke
2020-07-07  5:11                 ` Sandeep Maheswaram (Temp)
2020-07-07 15:52                   ` Matthias Kaehlcke
2020-04-01  5:15 ` [PATCH v7 3/4] arm64: dts: qcom: sdm845: Add interconnect properties for USB Sandeep Maheswaram
2020-04-14 20:31   ` Stephen Boyd
2020-04-01  5:15 ` [PATCH v7 4/4] arm64: dts: qcom: sc7180: " Sandeep Maheswaram
2020-04-14 20:32   ` Stephen Boyd
2020-04-29 18:35 ` [PATCH v7 0/4] ADD interconnect support for Qualcomm DWC3 driver Matthias Kaehlcke
2020-05-08  6:29   ` Sandeep Maheswaram (Temp)
2020-05-14 10:49     ` Felipe Balbi

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).