[PATCH V3 0/3] ADD interconnect support for Qualcomm DWC3 driver

[PATCH V3 0/3] ADD interconnect support for Qualcomm DWC3 driver

[Chandana Kishori Chiluveru]

This path series aims to add interconnect support in
dwc3-qcom driver on SDM845 SoCs.

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

Chandana Kishori Chiluveru (3):
  dt-bindings: Introduce interconnect properties for Qualcomm DWC3
    driver
  usb: dwc3: qcom: Add interconnect support in dwc3 driver
  arm64: dts: sdm845: Add interconnect properties for USB

 .../devicetree/bindings/usb/qcom,dwc3.txt          |  13 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |  12 ++
 drivers/usb/dwc3/dwc3-qcom.c                       | 145 ++++++++++++++++++++-
 3 files changed, 168 insertions(+), 2 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V3 1/3] dt-bindings: Introduce interconnect properties for Qualcomm DWC3 driver

[Chandana Kishori Chiluveru]

Add documentation for the interconnects and interconnect-names
properties for USB as detailed by bindings/interconnect/interconnect.txt.

Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
---
 Documentation/devicetree/bindings/usb/qcom,dwc3.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt
index cb695aa..428565b 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt
@@ -33,6 +33,16 @@ Optional clocks:
 
 Optional properties:
 - resets:		Phandle to reset control that resets core and wrapper.
+- interconnects:	Pairs of phandles and interconnect provider specifiers
+			to denote the edge source and destination ports of
+			the interconnect path. Please refer to
+			Documentation/devicetree/bindings/interconnect/
+			for more details.
+- interconnect-names:	List of interconnect path name strings sorted in the same
+			order as the interconnects property. Consumers drivers will use
+			interconnect-names to match interconnect paths with interconnect
+			specifiers. Please refer to Documentation/devicetree/bindings/
+			interconnect/ for more details.
 - interrupts:		specifies interrupts from controller wrapper used
 			to wakeup from low power/susepnd state.	Must contain
 			one or more entry for interrupt-names property
@@ -74,6 +84,9 @@ Example device nodes:
 			#size-cells = <1>;
 			ranges;
 
+			interconnects = <&qnoc MASTER_USB3_0 &qnoc SLAVE_EBI1>,
+					<&qnoc MASTER_APPSS_PROC &qnoc SLAVE_USB3_0>;
+			interconnect-names = "usb-ddr", "apps-usb";
 			interrupts = <0 131 0>, <0 486 0>, <0 488 0>, <0 489 0>;
 			interrupt-names = "hs_phy_irq", "ss_phy_irq",
 				  "dm_hs_phy_irq", "dp_hs_phy_irq";
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Chandana Kishori Chiluveru]

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: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 143 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 184df4d..2a2f5af 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -12,6 +12,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>
@@ -59,8 +60,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;
@@ -222,7 +228,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;
@@ -234,6 +240,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);
 
@@ -259,6 +269,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);
@@ -268,6 +282,124 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
 	return 0;
 }
 
+/* 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)
+
+/**
+ * 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;
+
+	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;
+	int ret;
+
+	dwc = platform_get_drvdata(qcom->dwc3);
+	if (!dwc) {
+		dev_err(qcom->dev, "Failed to get dwc3 device\n");
+		return -EPROBE_DEFER;
+	}
+
+	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);
+		if (ret)
+			return ret;
+	} 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)
+		goto err_disable_mem_path;
+
+	return 0;
+
+err_disable_mem_path:
+	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)
+{
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+	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:
+	if (dwc->maximum_speed == USB_SPEED_SUPER)
+		icc_set_bw(qcom->usb_ddr_icc_path,
+			USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
+	else
+		icc_set_bw(qcom->usb_ddr_icc_path,
+			USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);
+
+	return ret;
+}
+
 static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
 {
 	struct dwc3_qcom *qcom = data;
@@ -494,6 +626,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 		goto depopulate;
 	}
 
+	ret = dwc3_qcom_interconnect_init(qcom);
+	if (ret) {
+		dev_err(dev, "failed to init interconnect handles %d\n", ret);
+		goto depopulate;
+	}
+
 	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
 
 	/* enable vbus override for device mode */
@@ -503,7 +641,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;
@@ -513,6 +651,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	return 0;
 
+interconnect_exit:
+	dwc3_qcom_interconnect_exit(qcom);
 depopulate:
 	of_platform_depopulate(&pdev->dev);
 clk_disable:
@@ -540,6 +680,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 Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH V3 3/3] arm64: dts: sdm845: Add interconnect properties for USB

[Chandana Kishori Chiluveru]

Populate USB DT nodes with interconnect properties.

Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index fcb9330..e4885f3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1837,6 +1837,12 @@
 
 			resets = <&gcc GCC_USB30_PRIM_BCR>;
 
+			interconnects = <&rsc_hlos MASTER_USB3_0
+					 &rsc_hlos SLAVE_EBI1>,
+					<&rsc_hlos MASTER_APPSS_PROC
+					 &rsc_hlos SLAVE_USB3_0>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
 			usb_1_dwc3: dwc3@a600000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a600000 0 0xcd00>;
@@ -1881,6 +1887,12 @@
 
 			resets = <&gcc GCC_USB30_SEC_BCR>;
 
+			interconnects = <&rsc_hlos MASTER_USB3_1
+					 &rsc_hlos SLAVE_EBI1>,
+					<&rsc_hlos MASTER_APPSS_PROC
+					 &rsc_hlos SLAVE_USB3_1>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
 			usb_2_dwc3: dwc3@a800000 {
 				compatible = "snps,dwc3";
 				reg = <0 0x0a800000 0 0xcd00>;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH V3 1/3] dt-bindings: Introduce interconnect properties for Qualcomm DWC3 driver

[Matthias Kaehlcke]

On Tue, Sep 17, 2019 at 04:40:47PM +0530, Chandana Kishori Chiluveru wrote:
> Add documentation for the interconnects and interconnect-names
> properties for USB as detailed by bindings/interconnect/interconnect.txt.
> 
> Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/usb/qcom,dwc3.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt
> index cb695aa..428565b 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.txt
> @@ -33,6 +33,16 @@ Optional clocks:
>  
>  Optional properties:
>  - resets:		Phandle to reset control that resets core and wrapper.
> +- interconnects:	Pairs of phandles and interconnect provider specifiers
> +			to denote the edge source and destination ports of
> +			the interconnect path. Please refer to
> +			Documentation/devicetree/bindings/interconnect/
> +			for more details.
> +- interconnect-names:	List of interconnect path name strings sorted in the same
> +			order as the interconnects property. Consumers drivers will use

nit: Consumer

no need to re-spin the series just for this.

> +			interconnect-names to match interconnect paths with interconnect
> +			specifiers. Please refer to Documentation/devicetree/bindings/
> +			interconnect/ for more details.
>  - interrupts:		specifies interrupts from controller wrapper used
>  			to wakeup from low power/susepnd state.	Must contain
>  			one or more entry for interrupt-names property
> @@ -74,6 +84,9 @@ Example device nodes:
>  			#size-cells = <1>;
>  			ranges;
>  
> +			interconnects = <&qnoc MASTER_USB3_0 &qnoc SLAVE_EBI1>,
> +					<&qnoc MASTER_APPSS_PROC &qnoc SLAVE_USB3_0>;
> +			interconnect-names = "usb-ddr", "apps-usb";
>  			interrupts = <0 131 0>, <0 486 0>, <0 488 0>, <0 489 0>;
>  			interrupt-names = "hs_phy_irq", "ss_phy_irq",
>  				  "dm_hs_phy_irq", "dp_hs_phy_irq";

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Matthias Kaehlcke]

On Tue, Sep 17, 2019 at 04:40:48PM +0530, Chandana Kishori Chiluveru wrote:
> 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: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 184df4d..2a2f5af 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>
> ...
>
> +/**
> + * dwc3_qcom_interconnect_init() - Get interconnect path handles

nit: "Initialize the interconnect" or similar?

> + * @qcom:			Pointer to the concerned usb core.
> + *
> + */
> +static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
> +{
> +	struct device *dev = qcom->dev;
> +	int ret;
> +
> +	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);

nit: 'interconnect: %d', otherwise the message could be read as
'failed to enable interconnect number N'.

> +		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;
> +	int ret;
> +
> +	dwc = platform_get_drvdata(qcom->dwc3);
> +	if (!dwc) {
> +		dev_err(qcom->dev, "Failed to get dwc3 device\n");
> +		return -EPROBE_DEFER;
> +	}

I understand the need for the/some check (see
https://patchwork.kernel.org/patch/11146903/#22885491 and my reply),
but I'm not convinced it should be done here. The function can be
called from other contexts than _probe(), so returning -EPROBE_DEFER
seems wrong, although I understand that you want _probe() to return
this value.

I would suggest to do this (or another) check early in
_probe(). Returning -EPROBE_DEFER from that context makes sense, and
it should be the only time the check is actually needed.

As commented on v2 I don't particularly like the idea of using a half
initialized struct (dwc3), even more when what is initialized or not
varies at runtime (since dwc3_probe() and qwc3_qcom_probe() appear to
run in parallel). IIUC device_is_bound() returns true when a device is
fully initialized (i.e. it's _probe() completed), I'd suggest to
evaluate to use it instead of checking the drvdata. In this case the
drvdata check should be ok, but in general these kind of races don't
seem a good idea, maybe tomorrow someone wants to access another
struct member, which isn't always initialized.

Others: please correct me if using half-initialized structs is
routine in kernel drivers or device_is_bound() is the wrong tool.

> +
> +	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);
> +		if (ret)
> +			return ret;
> +	} 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)
> +		goto err_disable_mem_path;
> +
> +	return 0;
> +
> +err_disable_mem_path:
> +	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)
> +{
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +	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:
> +	if (dwc->maximum_speed == USB_SPEED_SUPER)
> +		icc_set_bw(qcom->usb_ddr_icc_path,
> +			USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> +	else
> +		icc_set_bw(qcom->usb_ddr_icc_path,
> +			USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);

This is essentially the same as in _interconnect_enable(). You could
consider a helper function (inline?) for 'enabling' the memory path,
which would make things more compact and also allow to get rid of the
goto:

if (ret)
	qcom_dwc3_interconnect_enable_mempath(qcom);

return ret;

> +
> +	return ret;
> +}
> +
>  static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
>  {
>  	struct dwc3_qcom *qcom = data;
> @@ -494,6 +626,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  		goto depopulate;
>  	}
>  
> +	ret = dwc3_qcom_interconnect_init(qcom);
> +	if (ret) {
> +		dev_err(dev, "failed to init interconnect handles %d\n", ret);

nit: remove 'handles' (and add ':' before the error code), it's an
implementation detail, and now _icc_init() also calls _icc_enable(),
hence it's not only the handles.

Re: [PATCH V3 3/3] arm64: dts: sdm845: Add interconnect properties for USB

[Matthias Kaehlcke]

On Tue, Sep 17, 2019 at 04:40:49PM +0530, Chandana Kishori Chiluveru wrote:
> Populate USB DT nodes with interconnect properties.
> 
> Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index fcb9330..e4885f3 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1837,6 +1837,12 @@
>  
>  			resets = <&gcc GCC_USB30_PRIM_BCR>;
>  
> +			interconnects = <&rsc_hlos MASTER_USB3_0
> +					 &rsc_hlos SLAVE_EBI1>,
> +					<&rsc_hlos MASTER_APPSS_PROC
> +					 &rsc_hlos SLAVE_USB3_0>;
> +			interconnect-names = "usb-ddr", "apps-usb";
> +
>  			usb_1_dwc3: dwc3@a600000 {
>  				compatible = "snps,dwc3";
>  				reg = <0 0x0a600000 0 0xcd00>;
> @@ -1881,6 +1887,12 @@
>  
>  			resets = <&gcc GCC_USB30_SEC_BCR>;
>  
> +			interconnects = <&rsc_hlos MASTER_USB3_1
> +					 &rsc_hlos SLAVE_EBI1>,
> +					<&rsc_hlos MASTER_APPSS_PROC
> +					 &rsc_hlos SLAVE_USB3_1>;
> +			interconnect-names = "usb-ddr", "apps-usb";
> +
>  			usb_2_dwc3: dwc3@a800000 {
>  				compatible = "snps,dwc3";
>  				reg = <0 0x0a800000 0 0xcd00>;

Please collect tags from earlier versions, unless the new version has
'major' changes (which is a bit of a judgement call).

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Evan Green]

Hi Chandana,
Are you going to spin this series? My comments are below, I see
Matthias also has comments. Please CC us both on the next spin.
-Evan

On Tue, Sep 17, 2019 at 4:11 AM Chandana Kishori Chiluveru
<cchiluve@codeaurora.org> wrote:
>
> 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: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 184df4d..2a2f5af 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -12,6 +12,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>
> @@ -59,8 +60,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);

Is there any reason you didn't just define these functions earlier?

> +
>  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>  {
>         u32 reg;
> @@ -222,7 +228,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;
> @@ -234,6 +240,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);

Shouldn't you propagate the failure if this doesn't work?

> +
>         qcom->is_suspended = true;
>         dwc3_qcom_enable_interrupts(qcom);
>
> @@ -259,6 +269,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);

Same here, isn't this important? In theory, if you cannot enable
bandwidth to the device, you aren't really allowed to touch it (with
odd exceptions for boot proxy votes, which might now be gone).

> +
>         /* 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);
> @@ -268,6 +282,124 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>         return 0;
>  }
>
> +/* 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)

Can you share at all how these numbers were arrived at? I thought HS
for instance was 480MB/s.

> +
> +/**
> + * 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;
> +
> +       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));

You're leaking usb_ddr_icc_path here.

> +               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;
> +       int ret;
> +
> +       dwc = platform_get_drvdata(qcom->dwc3);
> +       if (!dwc) {
> +               dev_err(qcom->dev, "Failed to get dwc3 device\n");
> +               return -EPROBE_DEFER;
> +       }
> +
> +       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);
> +               if (ret)
> +                       return ret;
> +       } 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)
> +               goto err_disable_mem_path;
> +
> +       return 0;
> +
> +err_disable_mem_path:
> +       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)
> +{
> +       struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +       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:
> +       if (dwc->maximum_speed == USB_SPEED_SUPER)
> +               icc_set_bw(qcom->usb_ddr_icc_path,
> +                       USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> +       else
> +               icc_set_bw(qcom->usb_ddr_icc_path,
> +                       USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);

Yeah, this is weird. If you're failing to clear your votes, things are
in a pretty broken state, and these calls to re-enable bandwidth have
almost no chance of succeeding. Maybe just return failure here.

> +
> +       return ret;
> +}
> +
>  static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
>  {
>         struct dwc3_qcom *qcom = data;
> @@ -494,6 +626,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>                 goto depopulate;
>         }
>
> +       ret = dwc3_qcom_interconnect_init(qcom);
> +       if (ret) {
> +               dev_err(dev, "failed to init interconnect handles %d\n", ret);
> +               goto depopulate;
> +       }
> +
>         qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
>
>         /* enable vbus override for device mode */
> @@ -503,7 +641,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;
> @@ -513,6 +651,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>
>         return 0;
>
> +interconnect_exit:
> +       dwc3_qcom_interconnect_exit(qcom);
>  depopulate:
>         of_platform_depopulate(&pdev->dev);
>  clk_disable:
> @@ -540,6 +680,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 Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
>

Re: [PATCH V3 3/3] arm64: dts: sdm845: Add interconnect properties for USB

[Evan Green]

Hi Chandana,

On Tue, Sep 17, 2019 at 4:11 AM Chandana Kishori Chiluveru
<cchiluve@codeaurora.org> wrote:
>
> Populate USB DT nodes with interconnect properties.
>
> Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index fcb9330..e4885f3 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1837,6 +1837,12 @@
>
>                         resets = <&gcc GCC_USB30_PRIM_BCR>;
>
> +                       interconnects = <&rsc_hlos MASTER_USB3_0
> +                                        &rsc_hlos SLAVE_EBI1>,
> +                                       <&rsc_hlos MASTER_APPSS_PROC
> +                                        &rsc_hlos SLAVE_USB3_0>;

You'll be racing with David's series here to split rsc_hlos out into more nodes:
https://patchwork.kernel.org/patch/11326603/

Do you want to just base yours on top of his and list it as a dependency?

Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Matthias Kaehlcke]

On Tue, Feb 04, 2020 at 11:05:41AM -0800, Evan Green wrote:
> Hi Chandana,
> Are you going to spin this series? My comments are below, I see
> Matthias also has comments. Please CC us both on the next spin.

v4 was actually posted a few days later:

https://patchwork.kernel.org/project/linux-usb/list/?series=176783

I also missed it since neither linux-arm-msm@ nor myself are in cc.

> On Tue, Sep 17, 2019 at 4:11 AM Chandana Kishori Chiluveru
> <cchiluve@codeaurora.org> wrote:
> >
> > 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: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 143 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 184df4d..2a2f5af 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -12,6 +12,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>
> > @@ -59,8 +60,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);
> 
> Is there any reason you didn't just define these functions earlier?
> 
> > +
> >  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> >  {
> >         u32 reg;
> > @@ -222,7 +228,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;
> > @@ -234,6 +240,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);
> 
> Shouldn't you propagate the failure if this doesn't work?
> 
> > +
> >         qcom->is_suspended = true;
> >         dwc3_qcom_enable_interrupts(qcom);
> >
> > @@ -259,6 +269,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);
> 
> Same here, isn't this important? In theory, if you cannot enable
> bandwidth to the device, you aren't really allowed to touch it (with
> odd exceptions for boot proxy votes, which might now be gone).
> 
> > +
> >         /* 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);
> > @@ -268,6 +282,124 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> >         return 0;
> >  }
> >
> > +/* 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)
> 
> Can you share at all how these numbers were arrived at? I thought HS
> for instance was 480MB/s.
> 
> > +
> > +/**
> > + * 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;
> > +
> > +       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));
> 
> You're leaking usb_ddr_icc_path here.
> 
> > +               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;
> > +       int ret;
> > +
> > +       dwc = platform_get_drvdata(qcom->dwc3);
> > +       if (!dwc) {
> > +               dev_err(qcom->dev, "Failed to get dwc3 device\n");
> > +               return -EPROBE_DEFER;
> > +       }
> > +
> > +       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);
> > +               if (ret)
> > +                       return ret;
> > +       } 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)
> > +               goto err_disable_mem_path;
> > +
> > +       return 0;
> > +
> > +err_disable_mem_path:
> > +       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)
> > +{
> > +       struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> > +       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:
> > +       if (dwc->maximum_speed == USB_SPEED_SUPER)
> > +               icc_set_bw(qcom->usb_ddr_icc_path,
> > +                       USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> > +       else
> > +               icc_set_bw(qcom->usb_ddr_icc_path,
> > +                       USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);
> 
> Yeah, this is weird. If you're failing to clear your votes, things are
> in a pretty broken state, and these calls to re-enable bandwidth have
> almost no chance of succeeding. Maybe just return failure here.
> 
> > +
> > +       return ret;
> > +}
> > +
> >  static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
> >  {
> >         struct dwc3_qcom *qcom = data;
> > @@ -494,6 +626,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >                 goto depopulate;
> >         }
> >
> > +       ret = dwc3_qcom_interconnect_init(qcom);
> > +       if (ret) {
> > +               dev_err(dev, "failed to init interconnect handles %d\n", ret);
> > +               goto depopulate;
> > +       }
> > +
> >         qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
> >
> >         /* enable vbus override for device mode */
> > @@ -503,7 +641,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;
> > @@ -513,6 +651,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >
> >         return 0;
> >
> > +interconnect_exit:
> > +       dwc3_qcom_interconnect_exit(qcom);
> >  depopulate:
> >         of_platform_depopulate(&pdev->dev);
> >  clk_disable:
> > @@ -540,6 +680,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 Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
> > is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
> >

Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Evan Green]

On Tue, Feb 4, 2020 at 11:17 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Feb 04, 2020 at 11:05:41AM -0800, Evan Green wrote:
> > Hi Chandana,
> > Are you going to spin this series? My comments are below, I see
> > Matthias also has comments. Please CC us both on the next spin.
>
> v4 was actually posted a few days later:
>
> https://patchwork.kernel.org/project/linux-usb/list/?series=176783
>
> I also missed it since neither linux-arm-msm@ nor myself are in cc.

Ah, thanks for the pointer, Matthias.

Yes, please CC linux-arm-msm@ and linux-kernel@ as well as us.
-Evan

Re: [PATCH V3 2/3] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Evan Green]

On Tue, Sep 17, 2019 at 4:11 AM Chandana Kishori Chiluveru
<cchiluve@codeaurora.org> wrote:
>
> 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: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 145 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 143 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 184df4d..2a2f5af 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -12,6 +12,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>
> @@ -59,8 +60,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;
> @@ -222,7 +228,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;
> @@ -234,6 +240,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);
>
> @@ -259,6 +269,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);
> @@ -268,6 +282,124 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
>         return 0;
>  }
>
> +/* 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)
> +
> +/**
> + * 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;

I'm not subscribed to linux-usb, so I don't have the V4 edition. So
I'm commenting here. In the v4 version there's a line:
 struct dwc3 *dwc;

that results in an unused local variable compiler warning.
-Evan