usb: dwc2: Add AHB burst configuration

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

Hi,

Quoted 10 lines · John Youn

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Quoted 5 lines

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

it's getting rather late for this merge window. I still need an ack by Rob or any of the devicetree folks.

Hi John,

Am 17.11.2016 um 00:47 schrieb John Youn:

Quoted 21 lines

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
 drivers/usb/dwc2/core.h                        |  9 +++++
 drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt

Quoted 4 lines

Documentation/devicetree/bindings/usb/dwc2.txt +0 -0

index 6c7c2bce..9e7b4b4 100644

according to Documentation/devicetree/bindings/submitting-patches.txt this change should be a separate patch.

Documentation/devicetree/bindings/usb/dwc2.txt Optional properties: · line 30

Quoted 7 lines

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

@@ -26,6 +26,8 @@ Optional properties:
 Refer to phy/phy-bindings.txt for generic phy consumer properties
 - dr_mode: shall be one of "host", "peripheral" and "otg"
   Refer to usb/generic.txt
+- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
+  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".

This doesn't apply in case of the bcm2835. I would prefer this option is ignored in that case with a dev_warn("snps,ahb-burst is not supported on this platform").

Hi John,

Am 17.11.2016 um 00:47 schrieb John Youn:

Documentation/devicetree/bindings/usb/dwc2.txt

Quoted 25 lines

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
 drivers/usb/dwc2/core.h                        |  9 +++++
 drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +0 -0

index 6c7c2bce..9e7b4b4 100644

according to Documentation/devicetree/bindings/submitting-patches.txt this change should be a separate patch.

Documentation/devicetree/bindings/usb/dwc2.txt Optional properties: · line 30

Quoted 6 lines

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

@@ -26,6 +26,8 @@ Optional properties:
 Refer to phy/phy-bindings.txt for generic phy consumer properties
 - dr_mode: shall be one of "host", "peripheral" and "otg"
   Refer to usb/generic.txt
+- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
+  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".

This doesn't apply in case of the bcm2835. I would prefer this option is ignored in that case with a dev_warn("snps,ahb-burst is not supported on this platform").

Quoted 32 lines · Stefan Wahren

Hi John,

Am 17.11.2016 um 00:47 schrieb John Youn:

Documentation/devicetree/bindings/usb/dwc2.txt

Quoted 25 lines

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
 drivers/usb/dwc2/core.h                        |  9 +++++
 drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +0 -0

index 6c7c2bce..9e7b4b4 100644

according to Documentation/devicetree/bindings/submitting-patches.txt this change should be a separate patch.

Quoted 10 lines

Documentation/devicetree/bindings/usb/dwc2.txt Optional properties: · line 30

Quoted 6 lines

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

@@ -26,6 +26,8 @@ Optional properties:
 Refer to phy/phy-bindings.txt for generic phy consumer properties
 - dr_mode: shall be one of "host", "peripheral" and "otg"
   Refer to usb/generic.txt
+- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
+  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".

This doesn't apply in case of the bcm2835. I would prefer this option is ignored in that case with a dev_warn("snps,ahb-burst is not supported on this platform").

Also, perhaps you should allow that the compatible string can define the default.

From 4c31a029dde714828810b1c3e61a5b1412ac939a Mon Sep 17 00:00:00 2001

Quoted 44 lines · CET Rob Herring

Quoted 32 lines · Stefan Wahren

Hi John,

Am 17.11.2016 um 00:47 schrieb John Youn:

Documentation/devicetree/bindings/usb/dwc2.txt

Quoted 25 lines

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
 drivers/usb/dwc2/core.h                        |  9 +++++
 drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +0 -0

index 6c7c2bce..9e7b4b4 100644

according to Documentation/devicetree/bindings/submitting-patches.txt this change should be a separate patch.

Quoted 10 lines

Documentation/devicetree/bindings/usb/dwc2.txt Optional properties: · line 30

Quoted 6 lines

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

@@ -26,6 +26,8 @@ Optional properties:
 Refer to phy/phy-bindings.txt for generic phy consumer properties
 - dr_mode: shall be one of "host", "peripheral" and "otg"
   Refer to usb/generic.txt
+- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
+  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".

This doesn't apply in case of the bcm2835. I would prefer this option is ignored in that case with a dev_warn("snps,ahb-burst is not supported on this platform").

Quoted 3 lines

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) that does just that for the amcc,dwc-otg. I put the GAHBCFG_HBSTLEN_INCR value into the .data, if that's a problem, I can certainly respin the patch and put it in a dedicated struct.

Regards

Christian
---
From 4c31a029dde714828810b1c3e61a5b1412ac939a Mon Sep 17 00:00:00 2001
From: Christian Lamparter <[email protected]>
Date: Fri, 18 Nov 2016 21:03:19 +0100
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg

This patch adds a of_device_id table which can be used by existing devices to supply a ahb-burst value for the platform without having to add a "snps,ahb-burst" entry to the dts.

Note: Adding new devices to this table is discouraged.
please consider adding the "snps,ahb-burst" property with the correct configuration to your device tree file instead.

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/usb/dwc2/params.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/dwc2/params.c +16 -0

index e0fc9aa..51be266 100644
@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
 	[GAHBCFG_HBSTLEN_INCR16]	= "INCR16",
 };
 
+/*
+ * This table provides AHB burst configuration for existing
+ * device tree bindings that work poorly with the default setting.
+ *
+ * Note: Adding new devices to this table is discouraged.
+ *	 please consider adding the "snps,ahb-burst" property
+ *	 with the correct configuration to your device tree
+ *	 file instead.
+ */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+	{
+		.compatible = "amcc,dwc-otg",
+		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
+	},
+};
+
 static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
 {
 	struct device_node *node = hsotg->dev->of_node;

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
 	ret = device_property_read_string(hsotg->dev,
 					  "snps,ahb-burst", &str);

Quoted 113 lines · Christian Lamparter

Quoted 44 lines · CET Rob Herring

Quoted 32 lines · Stefan Wahren

Hi John,

Am 17.11.2016 um 00:47 schrieb John Youn:

Documentation/devicetree/bindings/usb/dwc2.txt

Quoted 25 lines

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
 drivers/usb/dwc2/core.h                        |  9 +++++
 drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +0 -0

index 6c7c2bce..9e7b4b4 100644

according to Documentation/devicetree/bindings/submitting-patches.txt this change should be a separate patch.

Quoted 10 lines

Documentation/devicetree/bindings/usb/dwc2.txt Optional properties: · line 30

Quoted 6 lines

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

@@ -26,6 +26,8 @@ Optional properties:
 Refer to phy/phy-bindings.txt for generic phy consumer properties
 - dr_mode: shall be one of "host", "peripheral" and "otg"
   Refer to usb/generic.txt
+- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
+  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".

This doesn't apply in case of the bcm2835. I would prefer this option is ignored in that case with a dev_warn("snps,ahb-burst is not supported on this platform").

Quoted 3 lines

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) that does just that for the amcc,dwc-otg. I put the GAHBCFG_HBSTLEN_INCR value into the .data, if that's a problem, I can certainly respin the patch and put it in a dedicated struct.

Regards

Christian
---
From 4c31a029dde714828810b1c3e61a5b1412ac939a Mon Sep 17 00:00:00 2001
From: Christian Lamparter <[email protected]>
Date: Fri, 18 Nov 2016 21:03:19 +0100
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg

This patch adds a of_device_id table which can be used by existing devices to supply a ahb-burst value for the platform without having to add a "snps,ahb-burst" entry to the dts.

Note: Adding new devices to this table is discouraged.
please consider adding the "snps,ahb-burst" property with the correct configuration to your device tree file instead.

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/usb/dwc2/params.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/dwc2/params.c +20 -0

index e0fc9aa..51be266 100644
@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
 	[GAHBCFG_HBSTLEN_INCR16]	= "INCR16",
 };
 
+/*
+ * This table provides AHB burst configuration for existing
+ * device tree bindings that work poorly with the default setting.
+ *
+ * Note: Adding new devices to this table is discouraged.
+ *	 please consider adding the "snps,ahb-burst" property
+ *	 with the correct configuration to your device tree
+ *	 file instead.
+ */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+	{
+		.compatible = "amcc,dwc-otg",
+		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
+	},
+};
+
 static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
 {
 	struct device_node *node = hsotg->dev->of_node;

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
 	ret = device_property_read_string(hsotg->dev,
 					  "snps,ahb-burst", &str);
 	if (ret < 0) {
+		const struct of_device_id *match;
+
+		match = of_match_node(dwc2_compat_ahb_bursts, node);
+		if (match)

drivers/usb/dwc2/params.c static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg) · line 1134

Quoted 6 lines

drivers/usb/dwc2/params.c +2 -0

+			ret = (int)match->data;
+
 		return ret;
 	} else if (of_device_is_compatible(node, "brcm,bcm2835-usb")) {
 		dev_warn(hsotg->dev,

I'd prefer if you use the binding which requires no extra code in dwc2.

From e78604cb0b8ea8db277ef9bf321a613f8e0c7129 Mon Sep 17 00:00:00 2001

Hello John,

Quoted 114 lines · CET John Youn

Quoted 113 lines · Christian Lamparter

Quoted 44 lines · CET Rob Herring

Quoted 32 lines · Stefan Wahren

Hi John,

Am 17.11.2016 um 00:47 schrieb John Youn:

Documentation/devicetree/bindings/usb/dwc2.txt

Quoted 25 lines

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 +
 drivers/usb/dwc2/core.h                        |  9 +++++
 drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

Documentation/devicetree/bindings/usb/dwc2.txt +0 -0

index 6c7c2bce..9e7b4b4 100644

according to Documentation/devicetree/bindings/submitting-patches.txt this change should be a separate patch.

Quoted 10 lines

Documentation/devicetree/bindings/usb/dwc2.txt Optional properties: · line 30

Quoted 6 lines

Documentation/devicetree/bindings/usb/dwc2.txt +2 -0

@@ -26,6 +26,8 @@ Optional properties:
 Refer to phy/phy-bindings.txt for generic phy consumer properties
 - dr_mode: shall be one of "host", "peripheral" and "otg"
   Refer to usb/generic.txt
+- snps,ahb-burst: specifies the ahb burst length. Valid arguments are:
+  "SINGLE", "INCR", "INCR4", "INCR8", "INCR16". Defaults to "INCR4".

This doesn't apply in case of the bcm2835. I would prefer this option is ignored in that case with a dev_warn("snps,ahb-burst is not supported on this platform").

Quoted 3 lines

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) that does just that for the amcc,dwc-otg. I put the GAHBCFG_HBSTLEN_INCR value into the .data, if that's a problem, I can certainly respin the patch and put it in a dedicated struct.

Regards

Christian
---
From 4c31a029dde714828810b1c3e61a5b1412ac939a Mon Sep 17 00:00:00 2001
From: Christian Lamparter <[email protected]>
Date: Fri, 18 Nov 2016 21:03:19 +0100
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg

This patch adds a of_device_id table which can be used by existing devices to supply a ahb-burst value for the platform without having to add a "snps,ahb-burst" entry to the dts.

Note: Adding new devices to this table is discouraged.
please consider adding the "snps,ahb-burst" property with the correct configuration to your device tree file instead.

Signed-off-by Christian Lamparter <[email protected]>

---
 drivers/usb/dwc2/params.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

drivers/usb/dwc2/params.c +20 -0

index e0fc9aa..51be266 100644
@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
 	[GAHBCFG_HBSTLEN_INCR16]	= "INCR16",
 };
 
+/*
+ * This table provides AHB burst configuration for existing
+ * device tree bindings that work poorly with the default setting.
+ *
+ * Note: Adding new devices to this table is discouraged.
+ *	 please consider adding the "snps,ahb-burst" property
+ *	 with the correct configuration to your device tree
+ *	 file instead.
+ */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+	{
+		.compatible = "amcc,dwc-otg",
+		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
+	},
+};
+
 static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
 {
 	struct device_node *node = hsotg->dev->of_node;

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)
 	ret = device_property_read_string(hsotg->dev,
 					  "snps,ahb-burst", &str);
 	if (ret < 0) {
+		const struct of_device_id *match;
+
+		match = of_match_node(dwc2_compat_ahb_bursts, node);
+		if (match)

Quoted 8 lines

drivers/usb/dwc2/params.c static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg) · line 1134

Quoted 6 lines

drivers/usb/dwc2/params.c +2 -0

+			ret = (int)match->data;
+
 		return ret;
 	} else if (of_device_is_compatible(node, "brcm,bcm2835-usb")) {
 		dev_warn(hsotg->dev,

I'd prefer if you use the binding which requires no extra code in dwc2.

I'm fine with either option. However it think that this would require that either Mark or Rob would allow an exception to the "keep existing dts the way they are) and ack the following change to the canyonlands.dts.

In that case I wouldn't need the overwrite in dwc2_get_property_ahb_burst.

Regards,
Christian
---
From e78604cb0b8ea8db277ef9bf321a613f8e0c7129 Mon Sep 17 00:00:00 2001
From: Christian Lamparter <[email protected]>
Date: Mon, 21 Nov 2016 21:46:19 +0100
Subject: [PATCH] powerpc/dts: set snps,ahb-burst to INCR16

The dwc2 driver defaults to INCR4 which can cause a system hang when the USB and SATA is used concurrently.

Note: This patch requires:
"usb: dwc2: add amcc,dwc-otg support" (which already landed in the usb subsystem queue) and "usb: dwc2: Add AHB burst configuration"

Signed-off-by Christian Lamparter <[email protected]>

---
 arch/powerpc/boot/dts/canyonlands.dts | 1 +
 1 file changed, 1 insertion(+)

arch/powerpc/boot/dts/canyonlands.dts +0 -0

index 0d6ac92..90db712 100644

Quoted 20 lines · Christian Lamparter

Quoted 19 lines · CET John Youn

Quoted 18 lines · Christian Lamparter

Quoted 3 lines · CET Rob Herring

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) [...]
---
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg [...]

(quoted diff) +7 -0

@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
+/* [...] */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+	{
+		.compatible = "amcc,dwc-otg",
+		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
+	},
+};

[...]

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 1 line

(quoted diff) +0 -0

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)

(quoted diff) +6 -0

 	ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", &str);
 	if (ret < 0) {
+		const struct of_device_id *match;
+
+		match = of_match_node(dwc2_compat_ahb_bursts, node);
+		if (match)
+			ret = (int)match->data;
+

[...]

Quoted 5 lines

Quoted 2 lines

I'd prefer if you use the binding which requires no extra code in dwc2.

I'm fine with either option. However it think that this would require that either Mark or Rob would allow an exception to the "keep existing dts the way they are) and ack the following change to the canyonlands.dts.

I don't know about that. Under what circumstance can the dts change?

The canyonlands dts was binding to an external vendor driver. So it wasn't documented nor expected to work with dwc2 until your recent patch adding the compatible string.

Systems that use the vendor driver will still work with the dts. If you remove the vendor driver and configure it to use dwc2, it won't work due to a quirk of the canyonlands hardware, for which you need to add a dts property.

I think this is reasonable. Rob or Mark, any feedback?

One of the reasons I don't want to add the code in dwc2 is because I'm trying to make dwc2 a generic IP driver like dwc3.

Quoted 32 lines · CET John Youn

Quoted 20 lines · Christian Lamparter

Quoted 19 lines · CET John Youn

Quoted 18 lines · Christian Lamparter

Quoted 3 lines · CET Rob Herring

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) [...]
---
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg [...]

(quoted diff) +7 -0

@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
+/* [...] */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+ {
+         .compatible = "amcc,dwc-otg",
+         .data = (void *) GAHBCFG_HBSTLEN_INCR16,
+ },
+};

[...]

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Review static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)

Quoted 1 line

(quoted diff) +0 -0

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)

ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", &str); if (ret < 0) {

(quoted diff) +6 -0

+         const struct of_device_id *match;
+
+         match = of_match_node(dwc2_compat_ahb_bursts, node);
+         if (match)
+                 ret = (int)match->data;
+

[...]

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

I'd prefer if you use the binding which requires no extra code in dwc2.

I'm fine with either option. However it think that this would require that either Mark or Rob would allow an exception to the "keep existing dts the way they are) and ack the following change to the canyonlands.dts.

I don't know about that. Under what circumstance can the dts change?

As far as I know, the justification for not changing the DTS is that a compiled DTB might be stored in an read-only ROM on a board. So it would be impossible to update it. Hence, the driver have work with the existing (and sometimes buggy or incomplete) information to stay compatible.

(Note: Thankfully, the canyonlands dtb is stored in flash, it's possible to update it. But it is an extra step that's not done automatically with make install).

Quoted 3 lines

The canyonlands dts was binding to an external vendor driver. So it wasn't documented nor expected to work with dwc2 until your recent patch adding the compatible string.

Oh, no that's not what happend. Let me explain why there was no "external vendor driver": AMCC/APM were planing to upstream their hole platform. And in fact, the devs tried very hard to include their driver back in 2011 [0]. But this driver was denied inclusion back then due to:

"[...]
I would also like to point out that the same Synopsys USB controller is used in a number of other SoCs (especially ARM chips), and supported by other drivers, some of these even in mainline.

See http://thread.gmane.org/gmane.linux.usb.general/61714/focus=62139
for a related thread.

Instead of trying to add a completely new driver to mainline (and one which has been repeatedly been rejected), I vote for focusing on the existing driver code that is already in mainline, and testing and improving this so we can use a single implementation of this driver code for all SoCs that use the same IP block." [1]

Of course: The listed link goes the "USB Host driver for i.MX28" driver. And this is an ehci-hcd like driver... Which is as you are well aware not that similar to the dwc2 OTG. And as far as I can tell: AMCC abandoned the patch series right there.

Note: AMCC did however succeed in pushing your employer's Synopsys DesignWare SATA and DMA drivers to the kernel back then. And I'm happy to report that both drivers are still around and working fine for the 460EX (sata_dwc_460ex.c[2] and the DW AHB DMA [3]). (The drivers also work for different platforms than the original PPC. I know that because I helped Andy Shevchenko with testing and pushing some fixes to it when he was adding support for the Intel Quark SoC, which uses the DWC SATA and DMA).

So Please?

Quoted 4 lines

Systems that use the vendor driver will still work with the dts. If you remove the vendor driver and configure it to use dwc2, it won't work due to a quirk of the canyonlands hardware, for which you need to add a dts property.

Sadly, there is no up to date vendor driver. The canyonlands.dts binding is still in place and the hardware works fine. I'm interested in this platform since it is a cheap BigEndian system which is useful for usb driver development (carl9170 and rtl8192su)... and I would like to have out-of-the-box support.

Quoted 1 line

I think this is reasonable. Rob or Mark, any feedback?

I recall that Rob has already voiced his opinion about the ahb-burst setting:
"Also, perhaps you should allow that the compatible string can define the default."

And based on that, I made the "add a default ahb-burst setting for amcc,dwc-otg" patch above. Of course, it would be nice to have any feedback too. But unless I hear otherwise, I'll continue with posting patches to the dwc2 driver :).

Quoted 32 lines · CET John Youn

Quoted 20 lines · Christian Lamparter

Quoted 19 lines · CET John Youn

Quoted 18 lines · Christian Lamparter

Quoted 3 lines · CET Rob Herring

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) [...]
---
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg [...]

(quoted diff) +7 -0

@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
+/* [...] */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+	{
+		.compatible = "amcc,dwc-otg",
+		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
+	},
+};

[...]

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 1 line

(quoted diff) +0 -0

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)

(quoted diff) +6 -0

 	ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", &str);
 	if (ret < 0) {
+		const struct of_device_id *match;
+
+		match = of_match_node(dwc2_compat_ahb_bursts, node);
+		if (match)
+			ret = (int)match->data;
+

[...]

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

I'd prefer if you use the binding which requires no extra code in dwc2.

I'm fine with either option. However it think that this would require that either Mark or Rob would allow an exception to the "keep existing dts the way they are) and ack the following change to the canyonlands.dts.

I don't know about that. Under what circumstance can the dts change?

As far as I know, the justification for not changing the DTS is that a compiled DTB might be stored in an read-only ROM on a board. So it would be impossible to update it. Hence, the driver have work with the existing (and sometimes buggy or incomplete) information to stay compatible.

(Note: Thankfully, the canyonlands dtb is stored in flash, it's possible to update it. But it is an extra step that's not done automatically with make install).

Quoted 3 lines

The canyonlands dts was binding to an external vendor driver. So it wasn't documented nor expected to work with dwc2 until your recent patch adding the compatible string.

Oh, no that's not what happend. Let me explain why there was no "external vendor driver": AMCC/APM were planing to upstream their hole platform. And in fact, the devs tried very hard to include their driver back in 2011 [0]. But this driver was denied inclusion back then due to:

"[...]
I would also like to point out that the same Synopsys USB controller is used in a number of other SoCs (especially ARM chips), and supported by other drivers, some of these even in mainline.

See http://thread.gmane.org/gmane.linux.usb.general/61714/focus=62139
for a related thread.

Instead of trying to add a completely new driver to mainline (and one which has been repeatedly been rejected), I vote for focusing on the existing driver code that is already in mainline, and testing and improving this so we can use a single implementation of this driver code for all SoCs that use the same IP block." [1]

Of course: The listed link goes the "USB Host driver for i.MX28" driver. And this is an ehci-hcd like driver... Which is as you are well aware not that similar to the dwc2 OTG. And as far as I can tell: AMCC abandoned the patch series right there.

Note: AMCC did however succeed in pushing your employer's Synopsys DesignWare SATA and DMA drivers to the kernel back then. And I'm happy to report that both drivers are still around and working fine for the 460EX (sata_dwc_460ex.c[2] and the DW AHB DMA [3]). (The drivers also work for different platforms than the original PPC. I know that because I helped Andy Shevchenko with testing and pushing some fixes to it when he was adding support for the Intel Quark SoC, which uses the DWC SATA and DMA).

Quoted 33 lines · Christian Lamparter

Quoted 32 lines · CET John Youn

Quoted 20 lines · Christian Lamparter

Quoted 19 lines · CET John Youn

Quoted 18 lines · Christian Lamparter

Quoted 3 lines · CET Rob Herring

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) [...]
---
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg [...]

(quoted diff) +7 -0

@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
+/* [...] */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+	{
+		.compatible = "amcc,dwc-otg",
+		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
+	},
+};

[...]

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 1 line

(quoted diff) +0 -0

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)

(quoted diff) +6 -0

 	ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", &str);
 	if (ret < 0) {
+		const struct of_device_id *match;
+
+		match = of_match_node(dwc2_compat_ahb_bursts, node);
+		if (match)
+			ret = (int)match->data;
+

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I'd prefer if you use the binding which requires no extra code in dwc2.

I'm fine with either option. However it think that this would require that either Mark or Rob would allow an exception to the "keep existing dts the way they are) and ack the following change to the canyonlands.dts.

[...]

Ok thanks for clearing that up. I understand.

For now we can just set the property to "INCR16" based on the compatible string. Perhaps in the future do this from a glue-layer driver which binds to all compatible strings other than "snps,dwc2".

I won't be able to do anything with this until next week though.

Hello John, hello Felipe

Quoted 41 lines · CET John Youn

Quoted 33 lines · Christian Lamparter

Quoted 32 lines · CET John Youn

Quoted 20 lines · Christian Lamparter

Quoted 19 lines · CET John Youn

Quoted 18 lines · Christian Lamparter

Quoted 3 lines · CET Rob Herring

Also, perhaps you should allow that the compatible string can define the default.

I hoped you would say that :).

I've attached a patch (on top of John Youn changes) [...]
---
Subject: [PATCH] usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg [...]

(quoted diff) +7 -0

@@ -1097,6 +1097,22 @@ static const char *const ahb_bursts[] = {
+/* [...] */
+static const struct of_device_id dwc2_compat_ahb_bursts[] = {
+	{
+		.compatible = "amcc,dwc-otg",
+		.data = (void *) GAHBCFG_HBSTLEN_INCR16,
+	},
+};

[...]

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 1 line

(quoted diff) +0 -0

@@ -1107,6 +1123,12 @@ static int dwc2_get_property_ahb_burst(struct dwc2_hsotg *hsotg)

(quoted diff) +6 -0

 	ret = device_property_read_string(hsotg->dev, "snps,ahb-burst", &str);
 	if (ret < 0) {
+		const struct of_device_id *match;
+
+		match = of_match_node(dwc2_compat_ahb_bursts, node);
+		if (match)
+			ret = (int)match->data;
+

[...]

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

I'd prefer if you use the binding which requires no extra code in dwc2.

I'm fine with either option. However it think that this would require that either Mark or Rob would allow an exception to the "keep existing dts the way they are) and ack the following change to the canyonlands.dts.

[...]

Quoted 7 lines

Ok thanks for clearing that up. I understand.

For now we can just set the property to "INCR16" based on the compatible string. Perhaps in the future do this from a glue-layer driver which binds to all compatible strings other than "snps,dwc2".

I won't be able to do anything with this until next week though.

Ok, I think enough time has passed. I would like to see this patch series (v3 [0]) being queued for 4.11+ together with "usb: dwc2: add a default ahb-burst setting for amcc,dwc-otg" [1].

Felipe, if you want I can resend the series and add the "amcc,dwc-otg" patch to it as well. Just let me know what you prefer here.

Regards,
Christian

[0] <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mail-2Darchive.com_linux-2Dusb-40vger.kernel.org_msg83401.html&d=DgICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=0VsY7UdB1d6feXmjAhNrI_qghu91wtaLLnO3I3Uw2os&m=9a_ZbkfhbOdNjPstQXQq2hm157AzXQX03qKZIddHarU&s=zb0Zx7ZwvnKxcbpCJ_RlLrt1JtzhwUm9DffIAn9oR3k&e= >
[1] <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_linux-2Dusb_msg149663.html&d=DgICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=0VsY7UdB1d6feXmjAhNrI_qghu91wtaLLnO3I3Uw2os&m=9a_ZbkfhbOdNjPstQXQq2hm157AzXQX03qKZIddHarU&s=23LsbEs2DjUVLrj4ifAU2LCzEi-U3wn1G1Nx9FBIxvw&e= >

Quoted 1 line · Christian Lamparter

(Lot's of old stuff, that doesn't matter anymore)

This should be fixed against the latest dwc2 param rework series [1] which i hope to get queued for 4.11. If you can give it a test, that would be great.

Quoted 2 lines · CET John Youn

Quoted 1 line · Christian Lamparter

(Lot's of old stuff, that doesn't matter anymore)

Hello John,

Quoted 3 lines

This should be fixed against the latest dwc2 param rework series [1] which i hope to get queued for 4.11. If you can give it a test, that would be great.

oh Ok. I see you added it to
"[PATCH 16/21] usb: dwc2: Remove platform static params" [0]. Yes, I think this should work nicely. Thank you very much for your time, even though it's just for like "one old board" :-).

Do you have a public git tree with your patches that I can clone/checkout? If not, I'll take some time on the weekend for this and write back on monday. But yeah, this should work.

Quoted 6 lines · Christian Lamparter

Quoted 2 lines · CET John Youn

Quoted 1 line · Christian Lamparter

(Lot's of old stuff, that doesn't matter anymore)

Hello John,

Quoted 8 lines

Quoted 3 lines

This should be fixed against the latest dwc2 param rework series [1] which i hope to get queued for 4.11. If you can give it a test, that would be great.

oh Ok. I see you added it to
"[PATCH 16/21] usb: dwc2: Remove platform static params" [0]. Yes, I think this should work nicely. Thank you very much for your time, even though it's just for like "one old board" :-).

No problem. Sorry for the delay getting the param stuff sorted.

Quoted 1 line

Quoted 4 lines

Do you have a public git tree with your patches that I can clone/checkout? If not, I'll take some time on the weekend for this and write back on monday. But yeah, this should work.

Yes check here on branch 'next'

https://github.com/synopsys-usb/linux.git

From the (attached) bootlog:

Add the "snps,ahb-burst" binding and read it in.

This property controls which burst type to perform on the AHB bus as a master in internal DMA mode. This overrides the legacy param value, which we need to keep around for now since several platforms use it.

Some platforms may see better or worse performance based on this value. The HAPS platform is one example where all INCRx have worse performance than INCR.

Other platforms (such as the Canyonlands board) report that the default value causes system hangs.

Signed-off-by John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]>

Cc Christian Lamparter <chunkeey-gM/[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc2.txt |  2 +

 drivers/usb/dwc2/core.h                        |  9 +++++
 drivers/usb/dwc2/params.c                      | 56 ++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)