Add Embedded USB Debugger (EUD) driver

Documentation for Embedded USB Debugger(EUD) device tree bindings in yaml format.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 .../devicetree/bindings/soc/qcom/qcom,eud.yaml     | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +20 -0

new file mode 100644
index 0000000..3523932
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/soc/qcom/qcom,eud.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Qualcomm Embedded USB Debugger
+
+maintainers:
+  - Souradeep Chowdhury <[email protected]>
+
+description:
+  This binding is used to describe the Qualcomm Embedded USB Debugger, which is
+  mini USB-hub implemented on chip to support USB-based debug capabilities.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,sc7280-eud

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +7 -0

+    description:
+      This port is to be attached to the endpoint of the connector child of
+      DWC3 controller node. The controller has the "usb-role-switch" property.
+    $ref: /schemas/graph.yaml#/properties/port
+
+required:
+  - compatible

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +1 -0

+    };

Signature · 1 line

2.7.4

Quoted 2 lines · Souradeep Chowdhury

Documentation for Embedded USB Debugger(EUD) device tree bindings in yaml format.

I agree with Bjorn's comments.

Quoted 36 lines

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  .../devicetree/bindings/soc/qcom/qcom,eud.yaml     | 50 ++++++++++++++++++++++
  1 file changed, 50 insertions(+)

create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +23 -0

new file mode 100644
index 0000000..3523932
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/soc/qcom/qcom,eud.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Qualcomm Embedded USB Debugger
+
+maintainers:
+  - Souradeep Chowdhury <[email protected]>
+
+description:
+  This binding is used to describe the Qualcomm Embedded USB Debugger, which is
+  mini USB-hub implemented on chip to support USB-based debug capabilities.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,sc7280-eud
+      - const: qcom,eud
+
+  reg:

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml line 30

Quoted 7 lines

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +7 -0

+    items:
+      - description: EUD Base Register Region
+      - description: EUD Mode Manager Register
+
+  interrupts:
+    description:
+      EUD interrupt

Just 'maxItems: 1' is sufficient here.

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml line 32

Quoted 3 lines

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +2 -0

+
+  port:

Based on Bjorn's comment, you will need 2 port nodes.

Quoted 11 lines

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +10 -0

+    description:
+      This port is to be attached to the endpoint of the connector child of
+      DWC3 controller node. The controller has the "usb-role-switch" property.
+    $ref: /schemas/graph.yaml#/properties/port
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml line 49

Quoted 7 lines

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +7 -0

+
+examples:
+  - |
+    eud@88e0000 {
+           compatible = "qcom,sc7280-eud","qcom,eud";
+           reg = <0x88e0000 0x2000>,
+                 <0x88e2000 0x1000>;

Are the ports really optional?

Quoted 6 lines

Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml new file +1 -0

+    };

Signature · 1 line

2.7.4

Added the property for EUD(Embedded USB Debugger) connector. EUD is a mini usb hub with debug and trace capabilities and it has a type C connector attached to it for role-switching. This connector is attached to EUD via port.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 Documentation/devicetree/bindings/connector/usb-connector.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/connector/usb-connector.yaml +6 -1

index 7eb8659..417d39a 100644
@@ -30,6 +30,10 @@ properties:
           - const: samsung,usb-connector-11pin
           - const: usb-b-connector
 
+      - items:
+          - const: qcom,usb-connector-eud
+          - const: usb-c-connector
+
   label:
     description: Symbolic name for the connector.
 

@@ -179,7 +183,8 @@ properties:
     properties:
       port@0:
         $ref: /schemas/graph.yaml#/properties/port
-        description: High Speed (HS), present in all connectors.
+        description: High Speed (HS), present in all connectors. Also used as a
+                     port to connect QCOM Embedded USB Debugger(EUD).
 
       port@1:
         $ref: /schemas/graph.yaml#/properties/port

Signature · 1 line

2.7.4

Quoted 5 lines · Souradeep Chowdhury

Added the property for EUD(Embedded USB Debugger) connector. EUD is a mini usb hub with debug and trace capabilities and it has a type C connector attached to it for role-switching. This connector is attached to EUD via port.

Per my feedback on the dts patch, I don't think this binding is needed.

Regards,
Bjorn

Quoted 33 lines

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  Documentation/devicetree/bindings/connector/usb-connector.yaml | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/connector/usb-connector.yaml +6 -1

index 7eb8659..417d39a 100644
@@ -30,6 +30,10 @@ properties:
            - const: samsung,usb-connector-11pin
            - const: usb-b-connector
 
+      - items:
+          - const: qcom,usb-connector-eud
+          - const: usb-c-connector
+
    label:
      description: Symbolic name for the connector.
 

@@ -179,7 +183,8 @@ properties:
      properties:
        port@0:
          $ref: /schemas/graph.yaml#/properties/port
-        description: High Speed (HS), present in all connectors.
+        description: High Speed (HS), present in all connectors. Also used as a
+                     port to connect QCOM Embedded USB Debugger(EUD).
 
        port@1:
          $ref: /schemas/graph.yaml#/properties/port

Signature · 1 line

2.7.4

Add the connector property for dwc3 node. This connector can be used to role-switch the controller from device to host and vice versa.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

Documentation/devicetree/bindings/usb/snps,dwc3.yaml +0 -0

index 078fb78..9382168 100644

Documentation/devicetree/bindings/usb/snps,dwc3.yaml +5 -0

+    type: object
+    $ref: /connector/usb-connector.yaml#
+    description:
+      Connector for dual role switch, especially for "eud-usb-c-connector"
+
 unevaluatedProperties: false
 
 required:

Signature · 1 line

2.7.4

Quoted 17 lines · Souradeep Chowdhury

Add the connector property for dwc3 node. This connector can be used to role-switch the controller from device to host and vice versa.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 6 ++++++
  1 file changed, 6 insertions(+)

Documentation/devicetree/bindings/usb/snps,dwc3.yaml +0 -0

index 078fb78..9382168 100644
@@ -318,6 +318,12 @@ properties:
      items:
        enum: [1, 4, 8, 16, 32, 64, 128, 256]
 

Documentation/devicetree/bindings/usb/snps,dwc3.yaml properties: · line 321

Quoted 1 line

Documentation/devicetree/bindings/usb/snps,dwc3.yaml +1 -0

+  connector:

The dwc3 isn't the connector, so I think you should put a ports {} in the dwc3 and link that to the connector that is described elsewhere.

Regards,
Bjorn

Quoted 11 lines

Documentation/devicetree/bindings/usb/snps,dwc3.yaml +5 -0

+    type: object
+    $ref: /connector/usb-connector.yaml#
+    description:
+      Connector for dual role switch, especially for "eud-usb-c-connector"
+
  unevaluatedProperties: false
 
  required:

Signature · 1 line

2.7.4

Add support for control peripheral of EUD (Embedded USB Debugger) to listen to events such as USB attach/detach, pet EUD to indicate software is functional.Reusing the platform device kobj, sysfs entry 'enable' is created to enable or disable EUD.

To enable the eud the following needs to be done
echo 1 > /sys/bus/platform/.../enable

To disable eud, following is the command
echo 0 > /sys/bus/platform/.../enable

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 Documentation/ABI/testing/sysfs-driver-eud |   9 +
 drivers/soc/qcom/Kconfig                   |  10 ++
 drivers/soc/qcom/Makefile                  |   1 +
 drivers/soc/qcom/qcom_eud.c                | 260 +++++++++++++++++++++++++++++
 4 files changed, 280 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-eud
 create mode 100644 drivers/soc/qcom/qcom_eud.c

Documentation/ABI/testing/sysfs-driver-eud new file +9 -0

new file mode 100644
index 0000000..2381552
@@ -0,0 +1,9 @@
+What:		/sys/bus/platform/drivers/eud/.../enable
+Date:           January 2022
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		The Enable/Disable sysfs interface for Embedded
+		USB Debugger(EUD). This enables and disables the
+		EUD based on a 1 or a 0 value. By enabling EUD,
+		the user is able to activate the mini-usb hub of
+		EUD for debug and trace capabilities.

drivers/soc/qcom/Kconfig +10 -0

index 79b568f..a4db41b 100644
@@ -42,6 +42,16 @@ config QCOM_CPR
 	  To compile this driver as a module, choose M here: the module will
 	  be called qcom-cpr
 
+config QCOM_EUD
+	tristate "QCOM Embedded USB Debugger(EUD) Driver"
+	select USB_ROLE_SWITCH
+	help
+	  This module enables support for Qualcomm Technologies, Inc.
+	  Embedded USB Debugger (EUD). The EUD is a control peripheral
+	  which reports VBUS attach/detach events and has USB-based
+	  debug and trace capabilities. On selecting m, the module name
+	  that is built is qcom_eud.ko
+
 config QCOM_GENI_SE
 	tristate "QCOM GENI Serial Engine Driver"
 	depends on ARCH_QCOM || COMPILE_TEST

drivers/soc/qcom/Makefile +1 -0

index ad675a6..3331a40 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
 obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
 obj-$(CONFIG_QCOM_CPR)		+= cpr.o
+obj-$(CONFIG_QCOM_EUD)          += qcom_eud.o
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
 obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o

drivers/soc/qcom/qcom_eud.c new file +0 -0

new file mode 100644
index 0000000..7c4cc7d

drivers/soc/qcom/qcom_eud.c new file +20 -0

+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/usb/role.h>
+
+#define EUD_REG_INT1_EN_MASK	0x0024
+#define EUD_REG_INT_STATUS_1	0x0044
+#define EUD_REG_CTL_OUT_1	0x0074
+#define EUD_REG_VBUS_INT_CLR	0x0080
+#define EUD_REG_CSR_EUD_EN	0x1014
+#define EUD_REG_SW_ATTACH_DET	0x1018
+#define EUD_REG_EUD_EN2         0x0000
+

drivers/soc/qcom/qcom_eud.c new file +46 -0

+}
+
+static void disable_eud(struct eud_chip *priv)
+{
+	writel(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
+	writel(0, priv->eud_mode_mgr2_phys_base + EUD_REG_EUD_EN2);
+}
+
+static ssize_t enable_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct eud_chip *chip = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", chip->enable);
+}
+
+static ssize_t enable_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct eud_chip *chip = dev_get_drvdata(dev);
+	bool enable;
+	int ret;
+
+	if (kstrtobool(buf, &enable))
+		return -EINVAL;
+
+	if (enable) {
+		ret = enable_eud(chip);
+		if (!ret)
+			chip->enable = enable;
+	} else {
+		disable_eud(chip);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(enable);
+
+static struct attribute *eud_attrs[] = {
+	&dev_attr_enable.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(eud);
+

drivers/soc/qcom/qcom_eud.c new file +23 -0

+		if (chip->usb_attach)
+			ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
+		else
+			ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
+		if (ret)
+			dev_err(chip->dev, "failed to set role switch\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int eud_probe(struct platform_device *pdev)
+{
+	struct eud_chip *chip;
+	struct fwnode_handle *fwnode = pdev->dev.fwnode, *ep, *connector;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &pdev->dev;
+

drivers/soc/qcom/qcom_eud.c new file +33 -0

+
+	chip->eud_reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(chip->eud_reg_base))
+		return PTR_ERR(chip->eud_reg_base);
+
+	chip->eud_mode_mgr2_phys_base = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(chip->eud_mode_mgr2_phys_base))
+		return PTR_ERR(chip->eud_mode_mgr2_phys_base);
+
+	chip->eud_irq = platform_get_irq(pdev, 0);
+	ret = devm_request_threaded_irq(&pdev->dev, chip->eud_irq, handle_eud_irq,
+			handle_eud_irq_thread, IRQF_ONESHOT, NULL, chip);
+	if (ret)
+		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
+
+	enable_irq_wake(chip->eud_irq);
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+}
+
+static int eud_remove(struct platform_device *pdev)
+{
+	struct eud_chip *chip = platform_get_drvdata(pdev);
+
+	if (chip->enable)
+		disable_eud(chip);
+
+	device_init_wakeup(&pdev->dev, false);
+	disable_irq_wake(chip->eud_irq);
+
+	return 0;

drivers/soc/qcom/qcom_eud.c new file +8 -0

+		.dev_groups	= eud_groups,
+		.of_match_table = eud_dt_match,
+	},
+};
+module_platform_driver(eud_driver);
+
+MODULE_DESCRIPTION("QTI EUD driver");
+MODULE_LICENSE("GPL v2");

Signature · 1 line

2.7.4

Quoted 75 lines · Souradeep Chowdhury

Add support for control peripheral of EUD (Embedded USB Debugger) to listen to events such as USB attach/detach, pet EUD to indicate software is functional.Reusing the platform device kobj, sysfs entry 'enable' is created to enable or disable EUD.

To enable the eud the following needs to be done
echo 1 > /sys/bus/platform/.../enable

To disable eud, following is the command
echo 0 > /sys/bus/platform/.../enable

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  Documentation/ABI/testing/sysfs-driver-eud |   9 +
  drivers/soc/qcom/Kconfig                   |  10 ++
  drivers/soc/qcom/Makefile                  |   1 +
  drivers/soc/qcom/qcom_eud.c                | 260 +++++++++++++++++++++++++++++
  4 files changed, 280 insertions(+)

create mode 100644 Documentation/ABI/testing/sysfs-driver-eud create mode 100644 drivers/soc/qcom/qcom_eud.c

Documentation/ABI/testing/sysfs-driver-eud new file +9 -0

new file mode 100644
index 0000000..2381552
@@ -0,0 +1,9 @@
+What:		/sys/bus/platform/drivers/eud/.../enable
+Date:           January 2022
+Contact:        Souradeep Chowdhury <[email protected]>
+Description:
+		The Enable/Disable sysfs interface for Embedded
+		USB Debugger(EUD). This enables and disables the
+		EUD based on a 1 or a 0 value. By enabling EUD,
+		the user is able to activate the mini-usb hub of
+		EUD for debug and trace capabilities.

drivers/soc/qcom/Kconfig +10 -0

index 79b568f..a4db41b 100644
@@ -42,6 +42,16 @@ config QCOM_CPR
  	  To compile this driver as a module, choose M here: the module will
  	  be called qcom-cpr
 
+config QCOM_EUD
+	tristate "QCOM Embedded USB Debugger(EUD) Driver"
+	select USB_ROLE_SWITCH
+	help
+	  This module enables support for Qualcomm Technologies, Inc.
+	  Embedded USB Debugger (EUD). The EUD is a control peripheral
+	  which reports VBUS attach/detach events and has USB-based
+	  debug and trace capabilities. On selecting m, the module name
+	  that is built is qcom_eud.ko
+
  config QCOM_GENI_SE
  	tristate "QCOM GENI Serial Engine Driver"
  	depends on ARCH_QCOM || COMPILE_TEST

drivers/soc/qcom/Makefile +1 -0

index ad675a6..3331a40 100644
@@ -4,6 +4,7 @@ obj-$(CONFIG_QCOM_AOSS_QMP) +=	qcom_aoss.o
  obj-$(CONFIG_QCOM_GENI_SE) +=	qcom-geni-se.o
  obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
  obj-$(CONFIG_QCOM_CPR)		+= cpr.o
+obj-$(CONFIG_QCOM_EUD)          += qcom_eud.o
  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
  obj-$(CONFIG_QCOM_MDT_LOADER)	+= mdt_loader.o
  obj-$(CONFIG_QCOM_OCMEM)	+= ocmem.o

drivers/soc/qcom/qcom_eud.c new file +0 -0

new file mode 100644
index 0000000..7c4cc7d
@@ -0,0 +1,260 @@

drivers/soc/qcom/qcom_eud.c line 7

Quoted 7 lines

drivers/soc/qcom/qcom_eud.c new file +7 -0

+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/bitops.h>
+#include <linux/delay.h>

Is this needed?

Quoted 24 lines

drivers/soc/qcom/qcom_eud.c new file +23 -0

+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/usb/role.h>
+
+#define EUD_REG_INT1_EN_MASK	0x0024
+#define EUD_REG_INT_STATUS_1	0x0044
+#define EUD_REG_CTL_OUT_1	0x0074
+#define EUD_REG_VBUS_INT_CLR	0x0080
+#define EUD_REG_CSR_EUD_EN	0x1014
+#define EUD_REG_SW_ATTACH_DET	0x1018
+#define EUD_REG_EUD_EN2         0x0000
+
+#define EUD_ENABLE		BIT(0)
+#define EUD_INT_PET_EUD		BIT(0)
+#define EUD_INT_VBUS		BIT(2)

drivers/soc/qcom/qcom_eud.c line 37

Quoted 7 lines

drivers/soc/qcom/qcom_eud.c new file +7 -0

+#define EUD_INT_SAFE_MODE	BIT(4)
+#define EUD_INT_ALL		(EUD_INT_VBUS|EUD_INT_SAFE_MODE)
+
+struct eud_chip {
+	struct device			*dev;
+	struct usb_role_switch		*role_sw;
+	void __iomem			*eud_reg_base;

This is unnecessarily long, "base" or "eud" seems sufficient.

drivers/soc/qcom/qcom_eud.c line 38

Quoted 2 lines

drivers/soc/qcom/qcom_eud.c new file +1 -0

+	void __iomem			*eud_mode_mgr2_phys_base;

"mode_mgr"

drivers/soc/qcom/qcom_eud.c line 40

Quoted 3 lines

drivers/soc/qcom/qcom_eud.c new file +2 -0

+	unsigned int			int_status;
+	int				eud_irq;

"irq"

drivers/soc/qcom/qcom_eud.c line 41

Quoted 2 lines

drivers/soc/qcom/qcom_eud.c new file +1 -0

+	bool				enable;

"enable" is an action, but you use it to track if the state of the EUD, i.e if it's "enabled".

drivers/soc/qcom/qcom_eud.c line 42

Quoted 2 lines

drivers/soc/qcom/qcom_eud.c new file +1 -0

+	bool				usb_attach;

"attach" vs "attached"

drivers/soc/qcom/qcom_eud.c line 43

Quoted 2 lines

drivers/soc/qcom/qcom_eud.c new file +1 -0

+

Empty line.

drivers/soc/qcom/qcom_eud.c line 53

Quoted 11 lines

drivers/soc/qcom/qcom_eud.c new file +10 -0

+};
+
+static int enable_eud(struct eud_chip *priv)
+{
+	writel(EUD_ENABLE, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
+	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
+			priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
+	writel(1, priv->eud_mode_mgr2_phys_base + EUD_REG_EUD_EN2);
+
+	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);

When this fails, don't you want to disable the EUD again?

Quoted 50 lines

drivers/soc/qcom/qcom_eud.c new file +49 -0

+}
+
+static void disable_eud(struct eud_chip *priv)
+{
+	writel(0, priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
+	writel(0, priv->eud_mode_mgr2_phys_base + EUD_REG_EUD_EN2);
+}
+
+static ssize_t enable_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct eud_chip *chip = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%d\n", chip->enable);
+}
+
+static ssize_t enable_store(struct device *dev,
+		struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct eud_chip *chip = dev_get_drvdata(dev);
+	bool enable;
+	int ret;
+
+	if (kstrtobool(buf, &enable))
+		return -EINVAL;
+
+	if (enable) {
+		ret = enable_eud(chip);
+		if (!ret)
+			chip->enable = enable;
+	} else {
+		disable_eud(chip);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(enable);
+
+static struct attribute *eud_attrs[] = {
+	&dev_attr_enable.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(eud);
+
+static void usb_attach_detach(struct eud_chip *chip)
+{
+	u32 reg;

drivers/soc/qcom/qcom_eud.c line 109

Quoted 7 lines

drivers/soc/qcom/qcom_eud.c new file +7 -0

+
+	/* read ctl_out_1[4] to find USB attach or detach event */
+	reg = readl(chip->eud_reg_base + EUD_REG_CTL_OUT_1);
+	if (reg & EUD_INT_SAFE_MODE)
+		chip->usb_attach = true;
+	else
+		chip->usb_attach = false;

chip->usb_attach = reg & EUD_INT_SAFE_MODE;

drivers/soc/qcom/qcom_eud.c line 113

Quoted 4 lines

drivers/soc/qcom/qcom_eud.c new file +4 -0

+
+	/* set and clear vbus_int_clr[0] to clear interrupt */
+	writel(BIT(0), chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);
+	writel(0, chip->eud_reg_base + EUD_REG_VBUS_INT_CLR);

How come you do this before waking up the threaded handler?

drivers/soc/qcom/qcom_eud.c line 121

Quoted 9 lines

drivers/soc/qcom/qcom_eud.c new file +8 -0

+}
+
+static void pet_eud(struct eud_chip *chip)
+{
+	u32 reg;
+	int ret;
+
+	/* read sw_attach_det[0] to find attach/detach event */

Please instead describe what this means, like:

/*
* When the EUD_INT_PET_EUD in SW_ATTACH_DET is set, the cable has been
* disconnected and we need to detach the something because something. */

drivers/soc/qcom/qcom_eud.c line 122

Quoted 2 lines

drivers/soc/qcom/qcom_eud.c new file +1 -0

+	reg = readl(chip->eud_reg_base +  EUD_REG_SW_ATTACH_DET);

Please drop the double space.

Quoted 4 lines

drivers/soc/qcom/qcom_eud.c new file +3 -0

+	if (reg & EUD_INT_PET_EUD) {
+		/* Detach & Attach pet for EUD */
+		writel(0, chip->eud_reg_base + EUD_REG_SW_ATTACH_DET);

drivers/soc/qcom/qcom_eud.c line 135

Quoted 10 lines

drivers/soc/qcom/qcom_eud.c new file +10 -0

+		/* Delay to make sure detach pet is done before attach pet */
+		ret = readl_poll_timeout(chip->eud_reg_base + EUD_REG_SW_ATTACH_DET,
+					reg, (reg == 0), 1, 100);
+		if (ret) {
+			dev_err(chip->dev, "Detach pet failed\n");
+			return;
+		}
+
+		writel(EUD_INT_PET_EUD, chip->eud_reg_base +
+				EUD_REG_SW_ATTACH_DET);

Can you please help me see what differs between this and the write in the else statement?

Quoted 4 lines

drivers/soc/qcom/qcom_eud.c new file +3 -0

+	} else {
+		/* Attach pet for EUD */
+		writel(EUD_INT_PET_EUD, chip->eud_reg_base +

drivers/soc/qcom/qcom_eud.c line 148

Quoted 10 lines

drivers/soc/qcom/qcom_eud.c new file +10 -0

+				EUD_REG_SW_ATTACH_DET);
+	}
+}
+
+static irqreturn_t handle_eud_irq(int irq, void *data)
+{
+	struct eud_chip *chip = data;
+	u32 reg;
+
+	/* read status register and find out which interrupt triggered */

That's what the next line says as well, please drop this comment.

drivers/soc/qcom/qcom_eud.c line 149

Quoted 2 lines

drivers/soc/qcom/qcom_eud.c new file +1 -0

+	reg = readl(chip->eud_reg_base +  EUD_REG_INT_STATUS_1);

Please drop the double space.

drivers/soc/qcom/qcom_eud.c line 150

Quoted 2 lines

drivers/soc/qcom/qcom_eud.c new file +1 -0

+	switch (reg & EUD_INT_ALL) {

What if both of these bits are set? Won't you just end up with an interrupt storm until someone disables the EUD again?

drivers/soc/qcom/qcom_eud.c line 157

Quoted 8 lines

drivers/soc/qcom/qcom_eud.c new file +7 -0

+	case EUD_INT_VBUS:
+		chip->int_status = EUD_INT_VBUS;
+		usb_attach_detach(chip);
+		return IRQ_WAKE_THREAD;
+	case EUD_INT_SAFE_MODE:
+		pet_eud(chip);
+		break;

This is the only case where you break to return IRQ_HANDLED. Please be consistent.

drivers/soc/qcom/qcom_eud.c line 169

Quoted 13 lines

drivers/soc/qcom/qcom_eud.c new file +12 -0

+	default:
+		return IRQ_NONE;
+	}
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t handle_eud_irq_thread(int irq, void *data)
+{
+	struct eud_chip *chip = data;
+	int ret;
+
+	if (chip->int_status == EUD_INT_VBUS) {

I presume you have this here to signal that EUD_INT_VBUS was set in EUD_REG_INT_STATUS_1 in handle_eud_irq().

But 1) unless this was true you wouldn't have woken up the threaded handler and 2) you never clear int_status, so once you've received the EUD_INT_VBUS it seems to forever be true.

Quoted 28 lines

drivers/soc/qcom/qcom_eud.c new file +27 -0

+		if (chip->usb_attach)
+			ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_DEVICE);
+		else
+			ret = usb_role_switch_set_role(chip->role_sw, USB_ROLE_HOST);
+		if (ret)
+			dev_err(chip->dev, "failed to set role switch\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int eud_probe(struct platform_device *pdev)
+{
+	struct eud_chip *chip;
+	struct fwnode_handle *fwnode = pdev->dev.fwnode, *ep, *connector;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &pdev->dev;
+
+	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!ep)
+		return -ENODEV;
+

drivers/soc/qcom/qcom_eud.c line 202

Quoted 6 lines

drivers/soc/qcom/qcom_eud.c new file +6 -0

+	connector = fwnode_graph_get_remote_port_parent(ep);
+	fwnode_handle_put(ep);
+	if (!connector)
+		return -ENODEV;
+
+	chip->role_sw = fwnode_usb_role_switch_get(connector);

Can't you just call usb_role_switch_get(&pdev->dev) to find the "usb-role-switch", without having to traverse the fwnode_graph manually?

drivers/soc/qcom/qcom_eud.c line 206

Quoted 5 lines

drivers/soc/qcom/qcom_eud.c new file +4 -0

+	if (IS_ERR(chip->role_sw)) {
+		return dev_err_probe(chip->dev, PTR_ERR(chip->role_sw),
+					"failed to get role switch\n");
+	}

Note that from now on you must usb_role_switch_put(chip->role_sw);

Quoted 37 lines

drivers/soc/qcom/qcom_eud.c new file +36 -0

+
+	chip->eud_reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(chip->eud_reg_base))
+		return PTR_ERR(chip->eud_reg_base);
+
+	chip->eud_mode_mgr2_phys_base = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(chip->eud_mode_mgr2_phys_base))
+		return PTR_ERR(chip->eud_mode_mgr2_phys_base);
+
+	chip->eud_irq = platform_get_irq(pdev, 0);
+	ret = devm_request_threaded_irq(&pdev->dev, chip->eud_irq, handle_eud_irq,
+			handle_eud_irq_thread, IRQF_ONESHOT, NULL, chip);
+	if (ret)
+		return dev_err_probe(chip->dev, ret, "failed to allocate irq\n");
+
+	enable_irq_wake(chip->eud_irq);
+
+	platform_set_drvdata(pdev, chip);
+
+	return 0;
+}
+
+static int eud_remove(struct platform_device *pdev)
+{
+	struct eud_chip *chip = platform_get_drvdata(pdev);
+
+	if (chip->enable)
+		disable_eud(chip);
+
+	device_init_wakeup(&pdev->dev, false);
+	disable_irq_wake(chip->eud_irq);
+
+	return 0;
+}
+
+static const struct of_device_id eud_dt_match[] = {

drivers/soc/qcom/qcom_eud.c line 249

Quoted 7 lines

drivers/soc/qcom/qcom_eud.c new file +7 -0

+	{ .compatible = "qcom,sc7280-eud" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, eud_dt_match);
+
+static struct platform_driver eud_driver = {
+	.probe		= eud_probe,

I would prefer if you didn't indent the assignments.

drivers/soc/qcom/qcom_eud.c line 252

Quoted 4 lines

drivers/soc/qcom/qcom_eud.c new file +3 -0

+	.remove		= eud_remove,
+	.driver		= {
+		.name		= "eud",

"qcom_eud" please.

Regards,
Bjorn

Quoted 11 lines

drivers/soc/qcom/qcom_eud.c new file +8 -0

+		.dev_groups	= eud_groups,
+		.of_match_table = eud_dt_match,
+	},
+};
+module_platform_driver(eud_driver);
+
+MODULE_DESCRIPTION("QTI EUD driver");
+MODULE_LICENSE("GPL v2");

Signature · 1 line

2.7.4

Add the Embedded USB Debugger(EUD) device tree node. The node contains EUD base register region and EUD mode manager register regions along with the interrupt entry. Also add the connector to EUD which is mapped as the child of dwc3. The connector is attached to EUD via port. Also add the role-switch property to dwc3 node.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

arch/arm64/boot/dts/qcom/sc7280.dtsi +0 -0

index 53a21d0..2d14e5c 100644

arch/arm64/boot/dts/qcom/sc7280.dtsi +23 -1

+                                       compatible = "qcom,usb-connector-eud",
+                                                    "usb-c-connector";
+                                       ports {
+                                               port@0 {
+                                                       usb2_role_switch: endpoint {
+                                                               remote-endpoint = <&eud_ep>;
+                                                       };
+                                               };
+                                       };
+                               };
                        };
                };
 

@@ -1339,6 +1351,19 @@
                        interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
                };
 
+               eud:  eud@88e0000 {
+                       compatible = "qcom,sc7280-eud","qcom,eud";
+                       reg = <0 0x88e0000 0 0x2000>,
+                             <0 0x88e2000 0 0x1000>;
+                       interrupt-parent = <&pdc>;
+                       interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
+                       port {
+                               eud_ep: endpoint {
+                                       remote-endpoint = <&usb2_role_switch>;
+                               };
+                       };
+               };
+
                nsp_noc: interconnect@a0c0000 {
                        reg = <0 0x0a0c0000 0 0x10000>;
                        compatible = "qcom,sc7280-nsp-noc";
--

2.7.4

Add the Embedded USB Debugger(EUD) device tree node. The node contains EUD base register region and EUD mode manager register regions along with the interrupt entry. Also add the connector to EUD which is mapped as the child of dwc3. The connector is attached to EUD via port. Also add the role-switch property to dwc3 node.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

arch/arm64/boot/dts/qcom/sc7280.dtsi +0 -0

index 53a21d0..2d14e5c 100644

arch/arm64/boot/dts/qcom/sc7280.dtsi +23 -0

+					compatible = "qcom,usb-connector-eud",
+						     "usb-c-connector";
+					ports {
+						port@0 {
+							usb2_role_switch: endpoint {
+								remote-endpoint = <&eud_ep>;
+							};
+						};
+					};
+				};
 			};
 		};
 

@@ -1339,6 +1351,19 @@
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		eud:  eud@88e0000 {
+			compatible = "qcom,sc7280-eud","qcom,eud";
+			reg = <0 0x88e0000 0 0x2000>,
+			      <0 0x88e2000 0 0x1000>;
+			interrupt-parent = <&pdc>;
+			interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
+			port {
+				eud_ep: endpoint {
+					remote-endpoint = <&usb2_role_switch>;
+				};
+			};
+		};
+
 		nsp_noc: interconnect@a0c0000 {
 			reg = <0 0x0a0c0000 0 0x10000>;
 			compatible = "qcom,sc7280-nsp-noc";

Signature · 1 line

2.7.4

Quoted 17 lines · Souradeep Chowdhury

Add the Embedded USB Debugger(EUD) device tree node. The node contains EUD base register region and EUD mode manager register regions along with the interrupt entry. Also add the connector to EUD which is mapped as the child of dwc3. The connector is attached to EUD via port. Also add the role-switch property to dwc3 node.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
  arch/arm64/boot/dts/qcom/sc7280.dtsi | 25 +++++++++++++++++++++++++
  1 file changed, 25 insertions(+)

arch/arm64/boot/dts/qcom/sc7280.dtsi +0 -0

index 53a21d0..2d14e5c 100644

arch/arm64/boot/dts/qcom/sc7280.dtsi line 1319

Quoted 5 lines

arch/arm64/boot/dts/qcom/sc7280.dtsi +2 -0

@@ -1315,6 +1315,18 @@
  				phys = <&usb_2_hsphy>;
  				phy-names = "usb2-phy";
  				maximum-speed = "high-speed";
+				usb-role-switch;
+				usb_con: eud_usb_connector {

I think this is backwards.

Iiuc you have dwc3 <-> EUD <-> connector, where the connector typically would be represented in relation to a Type-C controller.

So I think this graph should be looking something like:

dwc3 {
     ports {
         port {
             dwc3_eud: endpoint {
                 remote-endpoint = <&eud_ep>;
             };
         };
     };
};

eud {
     ports {
         port {
             eud_dwc3: endpoint {
                 remote-endpoint = <&dwc3_eud>;
             };
         };

         port {
             eud_con: endpoint {
                 remote-endpoint = <&con_eud>;
             };
         };
     };
};

connector {
     compatible = "usb-c-connector";

     ports {
         port {
             con_eud: endpoint {
                 remote-endpoint = <&eud_con>;
             };
         };
     };
};

With EUD "forwarding" the role switching requests from the connector, or overriding them if the EUD is enabled.

Alternatively the dwc3 would be in a graph with both the eud and the connector, but I don't know how to ensure that the EUD decision takes precedence over the type-c controllers role request...

In the case of us having a type-c controller, such as the pmic_glink, the connector node would then be a child of the controller.

Regards,
Bjorn

Quoted 36 lines

arch/arm64/boot/dts/qcom/sc7280.dtsi +23 -0

+					compatible = "qcom,usb-connector-eud",
+						     "usb-c-connector";
+					ports {
+						port@0 {
+							usb2_role_switch: endpoint {
+								remote-endpoint = <&eud_ep>;
+							};
+						};
+					};
+				};
  			};
  		};
 

@@ -1339,6 +1351,19 @@
  			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
  		};
 
+		eud:  eud@88e0000 {
+			compatible = "qcom,sc7280-eud","qcom,eud";
+			reg = <0 0x88e0000 0 0x2000>,
+			      <0 0x88e2000 0 0x1000>;
+			interrupt-parent = <&pdc>;
+			interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
+			port {
+				eud_ep: endpoint {
+					remote-endpoint = <&usb2_role_switch>;
+				};
+			};
+		};
+
  		nsp_noc: interconnect@a0c0000 {
  			reg = <0 0x0a0c0000 0 0x10000>;
  			compatible = "qcom,sc7280-nsp-noc";

Signature · 1 line

2.7.4

Add the entry for maintainer for EUD driver
and other associated files.

Signed-off-by Souradeep Chowdhury <[email protected]>

---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

MAINTAINERS +0 -0

index 0227e15..54f616a 100644