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

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

[Sandeep Maheswaram]

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

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 (3):
  dt-bindings: usb: qcom,dwc3: 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.yaml         |  16 +++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |  12 ++
 drivers/usb/dwc3/dwc3-qcom.c                       | 135 ++++++++++++++++++++-
 3 files changed, 161 insertions(+), 2 deletions(-)

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

[PATCH v5 1/3] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties for Qualcomm DWC3 driver

[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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index 4a36ab5..236877e 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -68,6 +68,22 @@ properties:
   resets:
     maxItems: 1
 
+  interconnects:
+    description:
+      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:
+    description:
+      List of interconnect path name strings sorted in the same
+      order as the interconnects property. Consumer drivers will use
+      interconnect-names to match interconnect paths with interconnect
+      specifiers. Please refer to Documentation/devicetree/bindings/
+      interconnect/ for more details.
+
   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

Re: [PATCH v5 1/3] dt-bindings: usb: qcom,dwc3: Introduce interconnect properties for Qualcomm DWC3 driver

[Rob Herring]

On Fri, Feb 14, 2020 at 01:54:42PM +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 | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index 4a36ab5..236877e 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -68,6 +68,22 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  interconnects:
> +    description:
> +      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.

No need to redefine a common property, but you do need to define how 
many entries (maxItems).

> +
> +  interconnect-names:
> +    description:
> +      List of interconnect path name strings sorted in the same
> +      order as the interconnects property. Consumer drivers will use
> +      interconnect-names to match interconnect paths with interconnect
> +      specifiers. Please refer to Documentation/devicetree/bindings/
> +      interconnect/ for more details.

Need to define what the names are.

Rob

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

[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>
---
 drivers/usb/dwc3/dwc3-qcom.c | 135 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 261af9e..2ed6c20 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,108 @@ 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)
+{
+	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;
@@ -648,6 +772,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 +785,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 +795,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 +827,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

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

[Matthias Kaehlcke]

Hi Sandeep,

On Fri, Feb 14, 2020 at 01:54:43PM +0530, Sandeep Maheswaram 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: Sandeep Maheswaram <sanm@codeaurora.org>
> Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 135 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 133 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 261af9e..2ed6c20 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);
> +

This assumes that all QCA systems with a DWC3 have an interconnect
configuration, however after applying this series SDM845 is the only
platform. You need to track somewhere if the controller in question has
an ICC config for not.

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

same as above

>  	/* 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,108 @@ 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);
> +	}

This will break all QCA platforms with DWC3, except SDM845. Instead of
failing you could interpret the basence of the 'usb-ddr' patch in the DT
as signal that the controller has no ICC configuration, and continue without
it (i.e. return 0 from here, don't print an error, at most a dev_info() log),
and track somewhere that the controller has no ICC config.

Alternatively you could check above with of_find_property() whether the
controller has an 'interconnects' property at all. If it doesn't exist
return zero, otherwise return an error if any of the paths doesn't exist,
as you do now.

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

Failing here is ok, if 'usb-ddr' exists, we expect the rest of the config
to be in place.

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

nit: curly braces are not needed here

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

instead of the above:

	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 +772,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 +785,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 +795,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 +827,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
> 

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

[Matthias Kaehlcke]

On Mon, Mar 16, 2020 at 03:11:32PM +0530, Sandeep Maheswaram (Temp) wrote:
> Hi Matthias
> 
> On 2/15/2020 1:41 AM, Matthias Kaehlcke wrote:
> > Hi Sandeep,
> > 
> > On Fri, Feb 14, 2020 at 01:54:43PM +0530, Sandeep Maheswaram 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: Sandeep Maheswaram <sanm@codeaurora.org>
> > > Signed-off-by: Chandana Kishori Chiluveru <cchiluve@codeaurora.org>
> > > ---
> > >   drivers/usb/dwc3/dwc3-qcom.c | 135 ++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 133 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index 261af9e..2ed6c20 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);
> > > +
> > This assumes that all QCA systems with a DWC3 have an interconnect
> > configuration, however after applying this series SDM845 is the only
> > platform. You need to track somewhere if the controller in question has
> > an ICC config for not.
> 
> This is handled in drivers <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/>/interconnect <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/interconnect/>/core.c <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/interconnect/core.c> 
> icc_set_bw function.

Thanks for the clarification!

> > 
> > >   	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);
> > > +
> > same as above
> This is handled in drivers <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/>/interconnect <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/interconnect/>/core.c <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/interconnect/core.c> 
> icc_set_bw function

ok

> > >   	/* 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,108 @@ 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);
> > > +	}
> > This will break all QCA platforms with DWC3, except SDM845. Instead of
> > failing you could interpret the basence of the 'usb-ddr' patch in the DT
> > as signal that the controller has no ICC configuration, and continue without
> > it (i.e. return 0 from here, don't print an error, at most a dev_info() log),
> > and track somewhere that the controller has no ICC config.
> > 
> > Alternatively you could check above with of_find_property() whether the
> > controller has an 'interconnects' property at all. If it doesn't exist
> > return zero, otherwise return an error if any of the paths doesn't exist,
> > as you do now.
> This is handled in drivers <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/>/interconnect <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/interconnect/>/core.c <https://opengrok.qualcomm.com/source/xref/LC.UM.3.0/src/third_party/kernel/v5.4/drivers/interconnect/core.c> 
> of_icc_get function.

You are right, of_icc_get() returns NULL if the property doesn't exist, and this
is handled gracefully by the other ICC functions.

> > > +
> > > +	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);
> > > +	}
> > Failing here is ok, if 'usb-ddr' exists, we expect the rest of the config
> > to be in place.
> This may be required for error handling

Agreed, my comment meant to say the above handling seems correct.

> > > +
> > > +	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);
> > > +	}
> > nit: curly braces are not needed here
> Will remove in next version.
> > 
> > > +
> > > +	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)
> > > +{
> > > +	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);
> > instead of the above:
> > 
> > 	dwc3_qcom_interconnect_enable(qcom);
> Will change in next version.

With that changed:

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

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

[Sandeep Maheswaram]

Populate USB DT nodes with interconnect properties.

Signed-off-by: Sandeep Maheswaram <sanm@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 ae7d661..6b59f42 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2563,6 +2563,12 @@
 
 			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>;
@@ -2607,6 +2613,12 @@
 
 			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