[PATCH v8 0/4] Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support

[PATCH v8 0/4] Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support

[Bhupesh Sharma]

Changes since v6/v7:
-------------------
- v6 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230517211756.2483552-1-bhupesh.sharma@linaro.org/
- Konrad and Krzysztof had different suggestions on how to tackle
  different SoCs inside the eud driver which require access to secure mode
  manager register space. While Konrad's suggestion was to use a dt property,
  other comments suggested using optional platform data for determining
  the same. Modified [PATCH 2/4] accordingly to use the optional
  platform data for now.
- Added Krzysztof's RB for [PATCH 1/4] and also addressed his review comments
  received on v5.
- Dropped eud cleanup patches (which were sent a v7) as they have been accepted in linux-next.
- Rebased on latest linux-next/master.

Changes since v5:
----------------
- v5 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230516213308.2432018-1-bhupesh.sharma@linaro.org/
- Addressed Mani's comment and added Fixes tag for [PATCH 1/6].
  Also collected his Ack for this patch.
- Fixed [PATCH 4/6] as per Greg's comments and added a separate patch
  for identation issues -> [PATCH 3/6].

Changes since v4:
----------------
- v4 can be viewed here: https://lore.kernel.org/linux-arm-msm/20230505064039.1630025-1-bhupesh.sharma@linaro.org/
- Addressed Konrad's review comments regarding EUD driver code.
- Also collected his R-B for [PATCH 4/5 and 5/5].
- Fixed the dt-bindings as per Krzysztof's comments.

Changes since v3:
----------------
- v3 can be viewed here: https://www.spinics.net/lists/linux-arm-msm/msg137025.html 
- Addressed Konrad's review comments regarding mainly the driver code.
  Also fixed the .dtsi as per his comments.
- Also collected his R-B for [PATCH 1/5].

Changes since v2:
----------------
- v2 can be viewed here: https://www.spinics.net/lists/linux-arm-msm/msg137025.html 
- Addressed Bjorn and Krzysztof's comments.
- Added [PATCH 1/5] which fixes the 'qcom_eud' sysfs path. 
- Added [PATCH 5/5] to enable EUD for Qualcomm QRB4210-RB2 boards.

Changes since v1:
----------------
- v1 can be viewed here: https://lore.kernel.org/linux-arm-msm/20221231130743.3285664-1-bhupesh.sharma@linaro.org
- Added Krzysztof in Cc list.
- Fixed the following issue reported by kernel test bot:
  >> ERROR: modpost: "qcom_scm_io_writel" [drivers/usb/misc/qcom_eud.ko] undefined!

This series adds the dt-binding and driver support for SM6115 / SM4250
EUD (Embedded USB Debugger) block available on Qualcomm SoCs.

It also enables the same for QRB4210-RB2 boards by default (the user
still needs to enable the same via sysfs).

The EUD is a mini-USB hub implemented on chip to support the USB-based debug
and trace capabilities.

EUD driver listens to events like USB attach or detach and then
informs the USB about these events via ROLE-SWITCH.

Bhupesh Sharma (4):
  dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support
  usb: misc: eud: Add driver support for SM6115 / SM4250
  arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector
  arm64: dts: qcom: qrb4210-rb2: Enable EUD debug peripheral

 .../bindings/soc/qcom/qcom,eud.yaml           | 42 ++++++++++++-
 arch/arm64/boot/dts/qcom/qrb4210-rb2.dts      | 27 +++++++-
 arch/arm64/boot/dts/qcom/sm6115.dtsi          | 50 +++++++++++++++
 drivers/usb/misc/Kconfig                      |  2 +-
 drivers/usb/misc/qcom_eud.c                   | 62 +++++++++++++++++--
 5 files changed, 173 insertions(+), 10 deletions(-)

-- 
2.38.1

[PATCH v8 1/4] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Bhupesh Sharma]

Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.

On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
needs to be set first to 'enable' the eud module.

So, update the dt-bindings to accommodate the third register
property (TCSR Base) required by the driver on these SoCs.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 .../bindings/soc/qcom/qcom,eud.yaml           | 42 +++++++++++++++++--
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
index f2c5ec7e6437b..71274bc978584 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
@@ -18,12 +18,16 @@ properties:
     items:
       - enum:
           - qcom,sc7280-eud
+          - qcom,sm6115-eud
       - const: qcom,eud
 
   reg:
-    items:
-      - description: EUD Base Register Region
-      - description: EUD Mode Manager Register
+    minItems: 2
+    maxItems: 3
+
+  reg-names:
+    minItems: 2
+    maxItems: 3
 
   interrupts:
     description: EUD interrupt
@@ -50,6 +54,38 @@ required:
   - reg
   - ports
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc7280-eud
+    then:
+      properties:
+        reg:
+          maxItems: 2
+        reg-names:
+          items:
+            - const: eud-base
+            - const: eud-mode-mgr
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sm6115-eud
+    then:
+      properties:
+        reg:
+          maxItems: 3
+        reg-names:
+          items:
+            - const: eud-base
+            - const: eud-mode-mgr
+            - const: tcsr-base
+
 additionalProperties: false
 
 examples:
-- 
2.38.1

[PATCH v8 2/4] usb: misc: eud: Add driver support for SM6115 / SM4250

[Bhupesh Sharma]

Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.

On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
needs to be accessed only via the secure world (through 'scm'
calls).

Also, the enable bit inside 'tcsr_check_reg' needs to be set
first to set the eud in 'enable' mode on these SoCs.

Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 drivers/usb/misc/Kconfig    |  2 +-
 drivers/usb/misc/qcom_eud.c | 62 ++++++++++++++++++++++++++++++++++---
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 99b15b77dfd57..51eb5140caa14 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -146,7 +146,7 @@ config USB_APPLEDISPLAY
 
 config USB_QCOM_EUD
 	tristate "QCOM Embedded USB Debugger(EUD) Driver"
-	depends on ARCH_QCOM || COMPILE_TEST
+	depends on (ARCH_QCOM && QCOM_SCM) || COMPILE_TEST
 	select USB_ROLE_SWITCH
 	help
 	  This module enables support for Qualcomm Technologies, Inc.
diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index 7f371ea1248c3..136cac90228a0 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -11,9 +11,11 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
+#include <linux/firmware/qcom/qcom_scm.h>
 #include <linux/usb/role.h>
 
 #define EUD_REG_INT1_EN_MASK	0x0024
@@ -30,15 +32,25 @@
 #define EUD_INT_SAFE_MODE	BIT(4)
 #define EUD_INT_ALL		(EUD_INT_VBUS | EUD_INT_SAFE_MODE)
 
+#define EUD_EN2_EN		BIT(0)
+#define EUD_EN2_DISABLE		(0)
+#define TCSR_CHECK_EN		BIT(0)
+
+struct eud_soc_cfg {
+	u32 tcsr_check_offset;
+};
+
 struct eud_chip {
 	struct device			*dev;
 	struct usb_role_switch		*role_sw;
+	const struct eud_soc_cfg	*eud_cfg;
 	void __iomem			*base;
 	void __iomem			*mode_mgr;
 	unsigned int			int_status;
 	int				irq;
 	bool				enabled;
 	bool				usb_attached;
+	phys_addr_t			secure_mode_mgr;
 };
 
 static int enable_eud(struct eud_chip *priv)
@@ -46,7 +58,11 @@ static int enable_eud(struct eud_chip *priv)
 	writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
 	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
 			priv->base + EUD_REG_INT1_EN_MASK);
-	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
+
+	if (priv->secure_mode_mgr)
+		qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_EN);
+	else
+		writel(EUD_EN2_EN, priv->mode_mgr + EUD_REG_EUD_EN2);
 
 	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
 }
@@ -54,7 +70,11 @@ static int enable_eud(struct eud_chip *priv)
 static void disable_eud(struct eud_chip *priv)
 {
 	writel(0, priv->base + EUD_REG_CSR_EUD_EN);
-	writel(0, priv->mode_mgr + EUD_REG_EUD_EN2);
+
+	if (priv->secure_mode_mgr)
+		qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_DISABLE);
+	else
+		writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2);
 }
 
 static ssize_t enable_show(struct device *dev,
@@ -178,6 +198,8 @@ static void eud_role_switch_release(void *data)
 static int eud_probe(struct platform_device *pdev)
 {
 	struct eud_chip *chip;
+	struct resource *res;
+	phys_addr_t tcsr_check;
 	int ret;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
@@ -200,9 +222,34 @@ static int eud_probe(struct platform_device *pdev)
 	if (IS_ERR(chip->base))
 		return PTR_ERR(chip->base);
 
-	chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
-	if (IS_ERR(chip->mode_mgr))
-		return PTR_ERR(chip->mode_mgr);
+	/*
+	 * EUD block on a few Qualcomm SoCs needs secure register access.
+	 * Check for the same via SoC specific config data.
+	 */
+	chip->eud_cfg = of_device_get_match_data(&pdev->dev);
+	if (chip->eud_cfg) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!res)
+			return dev_err_probe(chip->dev, -ENODEV,
+					     "failed to get secure_mode_mgr reg base\n");
+
+		chip->secure_mode_mgr = res->start;
+
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr-base");
+		if (!res)
+			return dev_err_probe(chip->dev, -ENODEV,
+					     "failed to get tcsr reg base\n");
+
+		tcsr_check = res->start + chip->eud_cfg->tcsr_check_offset;
+
+		ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN);
+		if (ret)
+			return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n");
+	} else {
+		chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
+		if (IS_ERR(chip->mode_mgr))
+			return PTR_ERR(chip->mode_mgr);
+	}
 
 	chip->irq = platform_get_irq(pdev, 0);
 	ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq,
@@ -228,8 +275,13 @@ static void eud_remove(struct platform_device *pdev)
 	disable_irq_wake(chip->irq);
 }
 
+static const struct eud_soc_cfg sm6115_eud_cfg = {
+	.tcsr_check_offset = 0x25018,
+};
+
 static const struct of_device_id eud_dt_match[] = {
 	{ .compatible = "qcom,sc7280-eud" },
+	{ .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_cfg },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, eud_dt_match);
-- 
2.38.1

[PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Bhupesh Sharma]

Add the Embedded USB Debugger(EUD) device tree node for
SM6115 / SM4250 SoC.

The node contains EUD base register region, EUD mode manager
register region and TCSR Base register region along with the
interrupt entry.

Also add the typec connector node for EUD which is attached to
EUD node via port. EUD is also attached to DWC3 node via port.

To enable the role switch, we need to set dr_mode = "otg" property
for 'usb_dwc3' sub-node in the board dts file.

Also the EUD device can be enabled on a board once linux is boot'ed
by setting:
 $ echo 1 > /sys/bus/platform/drivers/qcom_eud/../enable

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
index 839c603512403..db45337c1082c 100644
--- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
@@ -260,6 +260,18 @@ CLUSTER_1_SLEEP_1: cluster-sleep-1-1 {
 		};
 	};
 
+	eud_typec: connector {
+		compatible = "usb-c-connector";
+
+		ports {
+			port@0 {
+				con_eud: endpoint {
+					remote-endpoint = <&eud_con>;
+				};
+			};
+		};
+	};
+
 	firmware {
 		scm: scm {
 			compatible = "qcom,scm-sm6115", "qcom,scm";
@@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
 			#power-domain-cells = <1>;
 		};
 
+		eud: eud@1610000 {
+			compatible = "qcom,sm6115-eud", "qcom,eud";
+			reg = <0x0 0x01610000 0x0 0x2000>,
+			      <0x0 0x01612000 0x0 0x1000>,
+			      <0x0 0x003c0000 0x0 0x40000>;
+			reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
+			interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+
+					eud_ep: endpoint {
+						remote-endpoint = <&usb2_role_switch>;
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+
+					eud_con: endpoint {
+						remote-endpoint = <&con_eud>;
+					};
+				};
+			};
+		};
+
 		usb_hsphy: phy@1613000 {
 			compatible = "qcom,sm6115-qusb2-phy";
 			reg = <0x0 0x01613000 0x0 0x180>;
@@ -1322,6 +1365,13 @@ usb_dwc3: usb@4e00000 {
 				snps,has-lpm-erratum;
 				snps,hird-threshold = /bits/ 8 <0x10>;
 				snps,usb3_lpm_capable;
+				usb-role-switch;
+
+				port {
+					usb2_role_switch: endpoint {
+						remote-endpoint = <&eud_ep>;
+					};
+				};
 			};
 		};
 
-- 
2.38.1

Re: [PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Bhupesh Sharma]

On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> > Add the Embedded USB Debugger(EUD) device tree node for
> > SM6115 / SM4250 SoC.
> >
> > The node contains EUD base register region, EUD mode manager
> > register region and TCSR Base register region along with the
> > interrupt entry.
> >
> > [...]
> >
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > index 839c603512403..db45337c1082c 100644
> > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > [...]
> > @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> >                       #power-domain-cells = <1>;
> >               };
> >
> > +             eud: eud@1610000 {
> > +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> > +                     reg = <0x0 0x01610000 0x0 0x2000>,
> > +                           <0x0 0x01612000 0x0 0x1000>,
> > +                           <0x0 0x003c0000 0x0 0x40000>;
> > +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
>
> TCSR is a separate hardware block unrelated to the EUD. IMHO it
> shouldn't be listed as "reg" here.
>
> Typically we describe it as syscon and then reference it from other
> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> same use case as you have. It also uses this to write something with
> qcom_scm_io_writel() at the end.

That was discussed a bit during v1 patchset review. Basically, if we
use a tcsr syscon approach here, we will need to define a 'qcom,xx'
vendor specific dt-property and use something like this in the eud
node:

qcom,eud-sec-reg = <&tcsr_reg yyyy>

which would be then used by the eud driver (via
syscon_regmap_lookup_by_phandle()).

But for sm6115 / qcm2290 this would be an over complicated solution as
normally the eud driver (say sc7280) doesn't need tcsr based secure
mode manager access. So defining a new soc / vendor specific
dt-property might be an overkill.

Thanks,
Bhupesh

Re: [PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Stephan Gerhold]

On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> > > Add the Embedded USB Debugger(EUD) device tree node for
> > > SM6115 / SM4250 SoC.
> > >
> > > The node contains EUD base register region, EUD mode manager
> > > register region and TCSR Base register region along with the
> > > interrupt entry.
> > >
> > > [...]
> > >
> > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> > >  1 file changed, 50 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > index 839c603512403..db45337c1082c 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > [...]
> > > @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> > >                       #power-domain-cells = <1>;
> > >               };
> > >
> > > +             eud: eud@1610000 {
> > > +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> > > +                     reg = <0x0 0x01610000 0x0 0x2000>,
> > > +                           <0x0 0x01612000 0x0 0x1000>,
> > > +                           <0x0 0x003c0000 0x0 0x40000>;
> > > +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> >
> > TCSR is a separate hardware block unrelated to the EUD. IMHO it
> > shouldn't be listed as "reg" here.
> >
> > Typically we describe it as syscon and then reference it from other
> > nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> > "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> > same use case as you have. It also uses this to write something with
> > qcom_scm_io_writel() at the end.
> 
> That was discussed a bit during v1 patchset review. Basically, if we
> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> vendor specific dt-property and use something like this in the eud
> node:
> 
> qcom,eud-sec-reg = <&tcsr_reg yyyy>
> 
> which would be then used by the eud driver (via
> syscon_regmap_lookup_by_phandle()).
> 
> But for sm6115 / qcm2290 this would be an over complicated solution as
> normally the eud driver (say sc7280) doesn't need tcsr based secure
> mode manager access. So defining a new soc / vendor specific
> dt-property might be an overkill.
> 

IMO a vendor-specific DT property is still better than messing up the
device separation in the device tree. The same "tcsr-base" reg would
also appear on the actual tcsr syscon device tree node. Having two
device tree nodes with the same reg region is generally not valid.

Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
clear that this points into a region that is shared between multiple
different devices, while adding it as reg suggests that TCSR belongs
exclusively to EUD.

Thanks,
Stephan

Re: [PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Bhupesh Sharma]

On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> > On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> > >
> > > On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> > > > Add the Embedded USB Debugger(EUD) device tree node for
> > > > SM6115 / SM4250 SoC.
> > > >
> > > > The node contains EUD base register region, EUD mode manager
> > > > register region and TCSR Base register region along with the
> > > > interrupt entry.
> > > >
> > > > [...]
> > > >
> > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> > > >  1 file changed, 50 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > > index 839c603512403..db45337c1082c 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> > > > [...]
> > > > @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> > > >                       #power-domain-cells = <1>;
> > > >               };
> > > >
> > > > +             eud: eud@1610000 {
> > > > +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> > > > +                     reg = <0x0 0x01610000 0x0 0x2000>,
> > > > +                           <0x0 0x01612000 0x0 0x1000>,
> > > > +                           <0x0 0x003c0000 0x0 0x40000>;
> > > > +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> > >
> > > TCSR is a separate hardware block unrelated to the EUD. IMHO it
> > > shouldn't be listed as "reg" here.
> > >
> > > Typically we describe it as syscon and then reference it from other
> > > nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> > > "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> > > same use case as you have. It also uses this to write something with
> > > qcom_scm_io_writel() at the end.
> >
> > That was discussed a bit during v1 patchset review. Basically, if we
> > use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> > vendor specific dt-property and use something like this in the eud
> > node:
> >
> > qcom,eud-sec-reg = <&tcsr_reg yyyy>
> >
> > which would be then used by the eud driver (via
> > syscon_regmap_lookup_by_phandle()).
> >
> > But for sm6115 / qcm2290 this would be an over complicated solution as
> > normally the eud driver (say sc7280) doesn't need tcsr based secure
> > mode manager access. So defining a new soc / vendor specific
> > dt-property might be an overkill.
> >
>
> IMO a vendor-specific DT property is still better than messing up the
> device separation in the device tree. The same "tcsr-base" reg would
> also appear on the actual tcsr syscon device tree node. Having two
> device tree nodes with the same reg region is generally not valid.
>
> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
> clear that this points into a region that is shared between multiple
> different devices, while adding it as reg suggests that TCSR belongs
> exclusively to EUD.

I understand your point but since for sm6115 / qcm2290 devices TCSR is
not used for any other purpose than EUD, I still think introducing a
new soc / vendor specific dt-property might be an overkill for this
changeset.

Thanks,
Bhupesh

Re: [PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Konrad Dybcio]

On 17.07.2023 22:09, Bhupesh Sharma wrote:
> On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
>>
>> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
>>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
>>>>
>>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
>>>>> Add the Embedded USB Debugger(EUD) device tree node for
>>>>> SM6115 / SM4250 SoC.
>>>>>
>>>>> The node contains EUD base register region, EUD mode manager
>>>>> register region and TCSR Base register region along with the
>>>>> interrupt entry.
>>>>>
>>>>> [...]
>>>>>
>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
>>>>>  1 file changed, 50 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> index 839c603512403..db45337c1082c 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> [...]
>>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
>>>>>                       #power-domain-cells = <1>;
>>>>>               };
>>>>>
>>>>> +             eud: eud@1610000 {
>>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
>>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
>>>>> +                           <0x0 0x01612000 0x0 0x1000>,
>>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
>>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
>>>>
>>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
>>>> shouldn't be listed as "reg" here.
>>>>
>>>> Typically we describe it as syscon and then reference it from other
>>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
>>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
>>>> same use case as you have. It also uses this to write something with
>>>> qcom_scm_io_writel() at the end.
>>>
>>> That was discussed a bit during v1 patchset review. Basically, if we
>>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
>>> vendor specific dt-property and use something like this in the eud
>>> node:
>>>
>>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
>>>
>>> which would be then used by the eud driver (via
>>> syscon_regmap_lookup_by_phandle()).
>>>
>>> But for sm6115 / qcm2290 this would be an over complicated solution as
>>> normally the eud driver (say sc7280) doesn't need tcsr based secure
>>> mode manager access. So defining a new soc / vendor specific
>>> dt-property might be an overkill.
>>>
>>
>> IMO a vendor-specific DT property is still better than messing up the
>> device separation in the device tree. The same "tcsr-base" reg would
>> also appear on the actual tcsr syscon device tree node. Having two
>> device tree nodes with the same reg region is generally not valid.
>>
>> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
>> clear that this points into a region that is shared between multiple
>> different devices, while adding it as reg suggests that TCSR belongs
>> exclusively to EUD.
> 
> I understand your point but since for sm6115 / qcm2290 devices TCSR is
> not used for any other purpose than EUD, I still think introducing a
> new soc / vendor specific dt-property might be an overkill for this
> changeset.
Untrue, there's some mumblings around the PHY properties and PSHOLD.
I think Stephan may be correct here.

Konrad
> 
> Thanks,
> Bhupesh

Re: [PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Bhupesh Sharma]

On Tue, 18 Jul 2023 at 01:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 17.07.2023 22:09, Bhupesh Sharma wrote:
> > On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>
> >> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> >>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>>>
> >>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> >>>>> Add the Embedded USB Debugger(EUD) device tree node for
> >>>>> SM6115 / SM4250 SoC.
> >>>>>
> >>>>> The node contains EUD base register region, EUD mode manager
> >>>>> register region and TCSR Base register region along with the
> >>>>> interrupt entry.
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> >>>>> ---
> >>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> >>>>>  1 file changed, 50 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>> index 839c603512403..db45337c1082c 100644
> >>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>> [...]
> >>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> >>>>>                       #power-domain-cells = <1>;
> >>>>>               };
> >>>>>
> >>>>> +             eud: eud@1610000 {
> >>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> >>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
> >>>>> +                           <0x0 0x01612000 0x0 0x1000>,
> >>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
> >>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> >>>>
> >>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
> >>>> shouldn't be listed as "reg" here.
> >>>>
> >>>> Typically we describe it as syscon and then reference it from other
> >>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> >>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> >>>> same use case as you have. It also uses this to write something with
> >>>> qcom_scm_io_writel() at the end.
> >>>
> >>> That was discussed a bit during v1 patchset review. Basically, if we
> >>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> >>> vendor specific dt-property and use something like this in the eud
> >>> node:
> >>>
> >>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
> >>>
> >>> which would be then used by the eud driver (via
> >>> syscon_regmap_lookup_by_phandle()).
> >>>
> >>> But for sm6115 / qcm2290 this would be an over complicated solution as
> >>> normally the eud driver (say sc7280) doesn't need tcsr based secure
> >>> mode manager access. So defining a new soc / vendor specific
> >>> dt-property might be an overkill.
> >>>
> >>
> >> IMO a vendor-specific DT property is still better than messing up the
> >> device separation in the device tree. The same "tcsr-base" reg would
> >> also appear on the actual tcsr syscon device tree node. Having two
> >> device tree nodes with the same reg region is generally not valid.
> >>
> >> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
> >> clear that this points into a region that is shared between multiple
> >> different devices, while adding it as reg suggests that TCSR belongs
> >> exclusively to EUD.
> >
> > I understand your point but since for sm6115 / qcm2290 devices TCSR is
> > not used for any other purpose than EUD, I still think introducing a
> > new soc / vendor specific dt-property might be an overkill for this
> > changeset.
> Untrue, there's some mumblings around the PHY properties and PSHOLD.
> I think Stephan may be correct here.

Can you share the links to those discussions?

Thanks,
Bhupesh

Re: [PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Konrad Dybcio]

On 17.07.2023 22:22, Bhupesh Sharma wrote:
> On Tue, 18 Jul 2023 at 01:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 17.07.2023 22:09, Bhupesh Sharma wrote:
>>> On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
>>>>
>>>> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
>>>>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
>>>>>>
>>>>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
>>>>>>> Add the Embedded USB Debugger(EUD) device tree node for
>>>>>>> SM6115 / SM4250 SoC.
>>>>>>>
>>>>>>> The node contains EUD base register region, EUD mode manager
>>>>>>> register region and TCSR Base register region along with the
>>>>>>> interrupt entry.
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>>>> ---
>>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 50 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>> index 839c603512403..db45337c1082c 100644
>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>>>> [...]
>>>>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
>>>>>>>                       #power-domain-cells = <1>;
>>>>>>>               };
>>>>>>>
>>>>>>> +             eud: eud@1610000 {
>>>>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
>>>>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
>>>>>>> +                           <0x0 0x01612000 0x0 0x1000>,
>>>>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
>>>>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
>>>>>>
>>>>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
>>>>>> shouldn't be listed as "reg" here.
>>>>>>
>>>>>> Typically we describe it as syscon and then reference it from other
>>>>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
>>>>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
>>>>>> same use case as you have. It also uses this to write something with
>>>>>> qcom_scm_io_writel() at the end.
>>>>>
>>>>> That was discussed a bit during v1 patchset review. Basically, if we
>>>>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
>>>>> vendor specific dt-property and use something like this in the eud
>>>>> node:
>>>>>
>>>>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
>>>>>
>>>>> which would be then used by the eud driver (via
>>>>> syscon_regmap_lookup_by_phandle()).
>>>>>
>>>>> But for sm6115 / qcm2290 this would be an over complicated solution as
>>>>> normally the eud driver (say sc7280) doesn't need tcsr based secure
>>>>> mode manager access. So defining a new soc / vendor specific
>>>>> dt-property might be an overkill.
>>>>>
>>>>
>>>> IMO a vendor-specific DT property is still better than messing up the
>>>> device separation in the device tree. The same "tcsr-base" reg would
>>>> also appear on the actual tcsr syscon device tree node. Having two
>>>> device tree nodes with the same reg region is generally not valid.
>>>>
>>>> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
>>>> clear that this points into a region that is shared between multiple
>>>> different devices, while adding it as reg suggests that TCSR belongs
>>>> exclusively to EUD.
>>>
>>> I understand your point but since for sm6115 / qcm2290 devices TCSR is
>>> not used for any other purpose than EUD, I still think introducing a
>>> new soc / vendor specific dt-property might be an overkill for this
>>> changeset.
>> Untrue, there's some mumblings around the PHY properties and PSHOLD.
>> I think Stephan may be correct here.
> 
> Can you share the links to those discussions?
It just seemed off to me that TCSR was not used by anything else (even
from Linux, it would obviously be used by something else higher up in
the boot chain as it contains various configuration registers), so I
took a glance at the downstream device tree and I noticed there are
more users.

Konrad

Re: [PATCH v8 3/4] arm64: dts: qcom: sm6115: Add EUD dt node and dwc3 connector

[Bhupesh Sharma]

On Tue, 18 Jul 2023 at 01:54, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 17.07.2023 22:22, Bhupesh Sharma wrote:
> > On Tue, 18 Jul 2023 at 01:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 17.07.2023 22:09, Bhupesh Sharma wrote:
> >>> On Mon, 17 Jul 2023 at 23:58, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>>>
> >>>> On Mon, Jul 17, 2023 at 11:33:40PM +0530, Bhupesh Sharma wrote:
> >>>>> On Mon, 17 Jul 2023 at 16:15, Stephan Gerhold <stephan@gerhold.net> wrote:
> >>>>>>
> >>>>>> On Mon, Jul 17, 2023 at 04:02:35PM +0530, Bhupesh Sharma wrote:
> >>>>>>> Add the Embedded USB Debugger(EUD) device tree node for
> >>>>>>> SM6115 / SM4250 SoC.
> >>>>>>>
> >>>>>>> The node contains EUD base register region, EUD mode manager
> >>>>>>> register region and TCSR Base register region along with the
> >>>>>>> interrupt entry.
> >>>>>>>
> >>>>>>> [...]
> >>>>>>>
> >>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> >>>>>>> ---
> >>>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 50 ++++++++++++++++++++++++++++
> >>>>>>>  1 file changed, 50 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>> index 839c603512403..db45337c1082c 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> >>>>>>> [...]
> >>>>>>> @@ -789,6 +801,37 @@ gcc: clock-controller@1400000 {
> >>>>>>>                       #power-domain-cells = <1>;
> >>>>>>>               };
> >>>>>>>
> >>>>>>> +             eud: eud@1610000 {
> >>>>>>> +                     compatible = "qcom,sm6115-eud", "qcom,eud";
> >>>>>>> +                     reg = <0x0 0x01610000 0x0 0x2000>,
> >>>>>>> +                           <0x0 0x01612000 0x0 0x1000>,
> >>>>>>> +                           <0x0 0x003c0000 0x0 0x40000>;
> >>>>>>> +                     reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> >>>>>>
> >>>>>> TCSR is a separate hardware block unrelated to the EUD. IMHO it
> >>>>>> shouldn't be listed as "reg" here.
> >>>>>>
> >>>>>> Typically we describe it as syscon and then reference it from other
> >>>>>> nodes. See e.g. sm8450.dtsi "tcsr: syscon@1fc0000" referenced in &scm
> >>>>>> "qcom,dload-mode = <&tcsr 0x13000>". This is pretty much exactly the
> >>>>>> same use case as you have. It also uses this to write something with
> >>>>>> qcom_scm_io_writel() at the end.
> >>>>>
> >>>>> That was discussed a bit during v1 patchset review. Basically, if we
> >>>>> use a tcsr syscon approach here, we will need to define a 'qcom,xx'
> >>>>> vendor specific dt-property and use something like this in the eud
> >>>>> node:
> >>>>>
> >>>>> qcom,eud-sec-reg = <&tcsr_reg yyyy>
> >>>>>
> >>>>> which would be then used by the eud driver (via
> >>>>> syscon_regmap_lookup_by_phandle()).
> >>>>>
> >>>>> But for sm6115 / qcm2290 this would be an over complicated solution as
> >>>>> normally the eud driver (say sc7280) doesn't need tcsr based secure
> >>>>> mode manager access. So defining a new soc / vendor specific
> >>>>> dt-property might be an overkill.
> >>>>>
> >>>>
> >>>> IMO a vendor-specific DT property is still better than messing up the
> >>>> device separation in the device tree. The same "tcsr-base" reg would
> >>>> also appear on the actual tcsr syscon device tree node. Having two
> >>>> device tree nodes with the same reg region is generally not valid.
> >>>>
> >>>> Something like qcom,eud-sec-reg = <&tcsr_reg yyyy> would at least make
> >>>> clear that this points into a region that is shared between multiple
> >>>> different devices, while adding it as reg suggests that TCSR belongs
> >>>> exclusively to EUD.
> >>>
> >>> I understand your point but since for sm6115 / qcm2290 devices TCSR is
> >>> not used for any other purpose than EUD, I still think introducing a
> >>> new soc / vendor specific dt-property might be an overkill for this
> >>> changeset.
> >> Untrue, there's some mumblings around the PHY properties and PSHOLD.
> >> I think Stephan may be correct here.
> >
> > Can you share the links to those discussions?
> It just seemed off to me that TCSR was not used by anything else (even
> from Linux, it would obviously be used by something else higher up in
> the boot chain as it contains various configuration registers), so I
> took a glance at the downstream device tree and I noticed there are
> more users.

Ok, let me recheck the downstream code and come back.

Thanks,
Bhupesh

[PATCH v8 4/4] arm64: dts: qcom: qrb4210-rb2: Enable EUD debug peripheral

[Bhupesh Sharma]

Since the USB-C type port on the Qualcomm QRB4210-RB2 board
can be set primarily in a 'device' configuration (with the default
DIP switch settings), it makes sense to enable the EUD debug
peripheral on the board by default by setting the USB 'dr_mode' property
as 'otg'.

Now, the EUD debug peripheral can be enabled by executing:
 $ echo 1 > /sys/bus/platform/drivers/qcom_eud/1610000.eud/enable

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 arch/arm64/boot/dts/qcom/qrb4210-rb2.dts | 27 +++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts
index a7278a9472ed9..640668960deb0 100644
--- a/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts
+++ b/arch/arm64/boot/dts/qcom/qrb4210-rb2.dts
@@ -264,6 +264,10 @@ &pon_resin {
 	status = "okay";
 };
 
+&eud {
+	status = "okay";
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
@@ -518,7 +522,28 @@ &usb {
 
 &usb_dwc3 {
 	maximum-speed = "super-speed";
-	dr_mode = "peripheral";
+
+	/*
+	 * There is only one USB DWC3 controller on QRB4210 board and it is connected
+	 * via a DIP Switch:
+	 * - to either an USB - C type connector or an USB - A type connector
+	 *   (via a GL3590-S hub), and
+	 * - to either an USB - A type connector (via a GL3590-S hub) or a connector
+	 *   for further connection with a mezzanine board.
+	 *
+	 * All of the above hardware muxes would allow us to hook things up in
+	 * different ways to some potential benefit for static configurations (for e.g.
+	 * on one hand we can have two USB - A type connectors and a USB - Ethernet
+	 * connection available and on the other we can use the USB - C type in
+	 * peripheral mode).
+	 *
+	 * Note that since the USB - C type can be used only in peripehral mode,
+	 * so hardcoding the mode to 'peripheral' here makes sense.
+	 *
+	 * However since we want to use the EUD debug device, we set the mode as
+	 * 'otg' here.
+	 */
+	dr_mode = "otg";
 };
 
 &usb_hsphy {
-- 
2.38.1