[PATCH v6 0/4] ADD interconnect support for Qualcomm DWC3 driver

[PATCH v6 0/4] ADD interconnect support for Qualcomm DWC3 driver

[Sandeep Maheswaram]

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

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

[PATCH v6 1/4] 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 | 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

[PATCH v6 2/4] 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>
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..7e85fe6 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:
+	ret = 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

[PATCH v6 3/4] arm64: dts: qcom: 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 | 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

[PATCH v6 4/4] arm64: dts: qcom: sc7180: 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/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

Re: [PATCH v6 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Matthias Kaehlcke]

Hi,

On Fri, Mar 27, 2020 at 03:13:21PM +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>
> 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..7e85fe6 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>
> ...
>
> +/* 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:
> +	ret = dwc3_qcom_interconnect_enable(qcom);

This overwrites the error that led to the execution of this code path.
The function should return original error, not the result of the
_interconnect_enable() call.

I saw Felipe queued the patch for v5.8. I think the main options to fix this
are:

- a v6 of this patch to replace v5 in Felipe's tree (which IIUC will be rebased
  anyway once there is a v5.7-rc)
- send the fix as a separate patch
- Felipe amends the patch in his tree

Felipe, what would work best for you?

Thanks

Matthias

Re: [PATCH v6 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Felipe Balbi]

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


Hi,

Matthias Kaehlcke <mka@chromium.org> writes:
>> 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..7e85fe6 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>
>> ...
>>
>> +/* 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:
>> +	ret = dwc3_qcom_interconnect_enable(qcom);
>
> This overwrites the error that led to the execution of this code path.
> The function should return original error, not the result of the
> _interconnect_enable() call.
>
> I saw Felipe queued the patch for v5.8. I think the main options to fix this
> are:
>
> - a v6 of this patch to replace v5 in Felipe's tree (which IIUC will be rebased
>   anyway once there is a v5.7-rc)
> - send the fix as a separate patch
> - Felipe amends the patch in his tree
>
> Felipe, what would work best for you?

Let's go for a v6, which commits should I drop? I can't find anything
related to $subject in my queue:

$ git --no-pager log --oneline HEAD ^linus/master -- drivers/usb/dwc3/dwc3-qcom.c
201c26c08db4 usb: dwc3: qcom: Replace <linux/clk-provider.h> by <linux/of_clk.h>

-- 
balbi

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

Re: [PATCH v6 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Matthias Kaehlcke]

On Mon, Mar 30, 2020 at 11:19:11AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Matthias Kaehlcke <mka@chromium.org> writes:
> >> 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..7e85fe6 100644
> >> --- a/drivers/usb/dwc3/dwc3-qcom.c
> >> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> >>
> >> ...
> >>
> >> +/* 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:
> >> +	ret = dwc3_qcom_interconnect_enable(qcom);
> >
> > This overwrites the error that led to the execution of this code path.
> > The function should return original error, not the result of the
> > _interconnect_enable() call.
> >
> > I saw Felipe queued the patch for v5.8. I think the main options to fix this
> > are:
> >
> > - a v6 of this patch to replace v5 in Felipe's tree (which IIUC will be rebased
> >   anyway once there is a v5.7-rc)
> > - send the fix as a separate patch
> > - Felipe amends the patch in his tree
> >
> > Felipe, what would work best for you?
> 
> Let's go for a v6, which commits should I drop? I can't find anything
> related to $subject in my queue:
> 
> $ git --no-pager log --oneline HEAD ^linus/master -- drivers/usb/dwc3/dwc3-qcom.c
> 201c26c08db4 usb: dwc3: qcom: Replace <linux/clk-provider.h> by <linux/of_clk.h>

I thought I saw a "queued for v5.8" message from you, but can't find that back.
I guess I saw the "queued" message for the "Add USB DWC3 support for SC7180"
series and thought it was for this one. Sorry for the confusion.

Re: [PATCH v6 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Felipe Balbi]

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


Hi,

Matthias Kaehlcke <mka@chromium.org> writes:
>> Matthias Kaehlcke <mka@chromium.org> writes:
>> >> 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..7e85fe6 100644
>> >> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> >> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> >>
>> >> ...
>> >>
>> >> +/* 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:
>> >> +	ret = dwc3_qcom_interconnect_enable(qcom);
>> >
>> > This overwrites the error that led to the execution of this code path.
>> > The function should return original error, not the result of the
>> > _interconnect_enable() call.
>> >
>> > I saw Felipe queued the patch for v5.8. I think the main options to fix this
>> > are:
>> >
>> > - a v6 of this patch to replace v5 in Felipe's tree (which IIUC will be rebased
>> >   anyway once there is a v5.7-rc)
>> > - send the fix as a separate patch
>> > - Felipe amends the patch in his tree
>> >
>> > Felipe, what would work best for you?
>> 
>> Let's go for a v6, which commits should I drop? I can't find anything
>> related to $subject in my queue:
>> 
>> $ git --no-pager log --oneline HEAD ^linus/master -- drivers/usb/dwc3/dwc3-qcom.c
>> 201c26c08db4 usb: dwc3: qcom: Replace <linux/clk-provider.h> by <linux/of_clk.h>
>
> I thought I saw a "queued for v5.8" message from you, but can't find that back.
> I guess I saw the "queued" message for the "Add USB DWC3 support for SC7180"
> series and thought it was for this one. Sorry for the confusion.

no worries :-)

-- 
balbi

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

Re: [PATCH v6 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Sandeep Maheswaram (Temp)]

Hi,

On 3/31/2020 3:05 AM, Felipe Balbi wrote:
> Hi,
>
> Matthias Kaehlcke <mka@chromium.org> writes:
>>> Matthias Kaehlcke <mka@chromium.org> writes:
>>>>> 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..7e85fe6 100644
>>>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>>>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>>>>
>>>>> ...
>>>>>
>>>>> +/* 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:
>>>>> +	ret = dwc3_qcom_interconnect_enable(qcom);
>>>> This overwrites the error that led to the execution of this code path.
>>>> The function should return original error, not the result of the
>>>> _interconnect_enable() call.
>>>>
>>>> I saw Felipe queued the patch for v5.8. I think the main options to fix this
>>>> are:
>>>>
>>>> - a v6 of this patch to replace v5 in Felipe's tree (which IIUC will be rebased
>>>>    anyway once there is a v5.7-rc)
>>>> - send the fix as a separate patch
>>>> - Felipe amends the patch in his tree
>>>>
>>>> Felipe, what would work best for you?
>>> Let's go for a v6, which commits should I drop? I can't find anything
>>> related to $subject in my queue:
>>>
>>> $ git --no-pager log --oneline HEAD ^linus/master -- drivers/usb/dwc3/dwc3-qcom.c
>>> 201c26c08db4 usb: dwc3: qcom: Replace <linux/clk-provider.h> by <linux/of_clk.h>
>> I thought I saw a "queued for v5.8" message from you, but can't find that back.
>> I guess I saw the "queued" message for the "Add USB DWC3 support for SC7180"
>> series and thought it was for this one. Sorry for the confusion.
> no worries :-)
>
Should I remove the ret from below line and send a new version?
+	ret = dwc3_qcom_interconnect_enable(qcom);

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

Re: [PATCH v6 2/4] usb: dwc3: qcom: Add interconnect support in dwc3 driver

[Matthias Kaehlcke]

On Tue, Mar 31, 2020 at 10:45:19AM +0530, Sandeep Maheswaram (Temp) wrote:
> Hi,
> 
> On 3/31/2020 3:05 AM, Felipe Balbi wrote:
> > Hi,
> > 
> > Matthias Kaehlcke <mka@chromium.org> writes:
> > > > Matthias Kaehlcke <mka@chromium.org> writes:
> > > > > > 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..7e85fe6 100644
> > > > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > +/* 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:
> > > > > > +	ret = dwc3_qcom_interconnect_enable(qcom);
> > > > > This overwrites the error that led to the execution of this code path.
> > > > > The function should return original error, not the result of the
> > > > > _interconnect_enable() call.
> > > > > 
> > > > > I saw Felipe queued the patch for v5.8. I think the main options to fix this
> > > > > are:
> > > > > 
> > > > > - a v6 of this patch to replace v5 in Felipe's tree (which IIUC will be rebased
> > > > >    anyway once there is a v5.7-rc)
> > > > > - send the fix as a separate patch
> > > > > - Felipe amends the patch in his tree
> > > > > 
> > > > > Felipe, what would work best for you?
> > > > Let's go for a v6, which commits should I drop? I can't find anything
> > > > related to $subject in my queue:
> > > > 
> > > > $ git --no-pager log --oneline HEAD ^linus/master -- drivers/usb/dwc3/dwc3-qcom.c
> > > > 201c26c08db4 usb: dwc3: qcom: Replace <linux/clk-provider.h> by <linux/of_clk.h>
> > > I thought I saw a "queued for v5.8" message from you, but can't find that back.
> > > I guess I saw the "queued" message for the "Add USB DWC3 support for SC7180"
> > > series and thought it was for this one. Sorry for the confusion.
> > no worries :-)
> > 
> Should I remove the ret from below line and send a new version?
> +	ret = dwc3_qcom_interconnect_enable(qcom);

Yes, please!