[PATCH V4 0/6] Add Embedded USB Debugger (EUD) driver

[PATCH V4 0/6] Add Embedded USB Debugger (EUD) driver

[Souradeep Chowdhury]

This is a series of patches that implements a driver for the control
peripheral, EUD (Embedded USB Debugger). The EUD is a mini-USB hub
implemented on chip to support the USB-based debug and trace capabilities.
Apart from debug capabilities, EUD has a control peripheral. Control
Peripheral is on when EUD is on and gets signals like USB attach, pet
EUD etc. EUD driver listens to events like USB attach or detach and then
informs the USB about these events via ROLE-SWITCH. At regular intervals,
the EUD driver receives an interrupt to pet the driver indicating that
the software is functional.

Changes in V4

* Aligned the device tree node structure of EUD as per discussion.

* Changes to usb-connector.yaml is no longer required and is not 
  included in the patch series.
  
* Implemented the rest of the comments on Version 3 of the patch.  

Changes in V3

* Removed the patch for registration of EUD connector as it is no longer
  required.
  
* Added the description to include EUD in usb-connector.yaml  

* Implemented comments on V2 of the patch.

Changes in V2

* Fixed the yaml issue and also implemented comments on yaml in V1.

Changes in V1

* EUD has now been mapped as a separate DT node as it is an independent QCOM IP.

* EUD is attached to the connector child of dwc3 via port end point since EUD
  driver needs the connector for role-switching.

* EUD driver has been moved now to drivers/soc/qcom/qcom_eud.c.

* All the comments from version 0 of the patch has been implemented.

Souradeep Chowdhury (6):
  dt-bindings: Add the yaml bindings for EUD
  bindings: usb: dwc3: Update dwc3 properties for EUD connector
  soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)
  arm64: dts: qcom: sc7280: Add EUD dt node and dwc3 connector
  arm64: dts: qcom: sc7280: Set the default dr_mode for usb2
  MAINTAINERS: Add maintainer entry for EUD

 Documentation/ABI/testing/sysfs-driver-eud         |   9 +
 .../devicetree/bindings/soc/qcom/qcom,eud.yaml     |  77 +++++++
 .../devicetree/bindings/usb/snps,dwc3.yaml         |  10 +
 MAINTAINERS                                        |   8 +
 arch/arm64/boot/dts/qcom/sc7280-idp.dts            |   4 +
 arch/arm64/boot/dts/qcom/sc7280.dtsi               |  39 ++++
 drivers/soc/qcom/Kconfig                           |  10 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/qcom_eud.c                        | 250 +++++++++++++++++++++
 9 files changed, 408 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-eud
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
 create mode 100644 drivers/soc/qcom/qcom_eud.c

-- 
2.7.4

[PATCH V4 1/6] dt-bindings: Add the yaml bindings for EUD

[Souradeep Chowdhury]

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

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 .../devicetree/bindings/soc/qcom/qcom,eud.yaml     | 77 ++++++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
new file mode 100644
index 0000000..c98aab2
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
@@ -0,0 +1,77 @@
+# 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 <quic_schowdhu@quicinc.com>
+
+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:
+    items:
+      - description: EUD Base Register Region
+      - description: EUD Mode Manager Register
+
+  interrupts:
+    description: EUD interrupt
+    maxItems: 1
+
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      These ports are to be attached to the endpoint of the DWC3 controller node
+      and type C connector node. The controller has the "usb-role-switch"
+      property.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: This port is to be attached to the DWC3 controller.
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: This port is to be attached to the type C connector.
+
+required:
+  - compatible
+  - reg
+  - ports
+
+additionalProperties: false
+
+examples:
+  - |
+    eud@88e0000 {
+           compatible = "qcom,sc7280-eud","qcom,eud";
+           reg = <0x88e0000 0x2000>,
+                 <0x88e2000 0x1000>;
+           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>;
+                           };
+                   };
+           };
+    };
--
2.7.4

[PATCH V4 2/6] bindings: usb: dwc3: Update dwc3 properties for EUD connector

[Souradeep Chowdhury]

Add the ports property for dwc3 node. This port can be used
by the Embedded USB Debugger for role switching the controller
from device to host mode and vice versa.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index d29ffcd..ccb1236 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -332,6 +332,16 @@ properties:
     items:
       enum: [1, 4, 8, 16, 32, 64, 128, 256]
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      This port is to be attached to the endpoint of the Embedded USB Debugger.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Connector endpoint of Embedded USB debugger.
+
 unevaluatedProperties: false
 
 required:
-- 
2.7.4

[PATCH V4 3/6] soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)

[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 <quic_schowdhu@quicinc.com>
---
 Documentation/ABI/testing/sysfs-driver-eud |   9 ++
 drivers/soc/qcom/Kconfig                   |  10 ++
 drivers/soc/qcom/Makefile                  |   1 +
 drivers/soc/qcom/qcom_eud.c                | 250 +++++++++++++++++++++++++++++
 4 files changed, 270 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-eud
 create mode 100644 drivers/soc/qcom/qcom_eud.c

diff --git a/Documentation/ABI/testing/sysfs-driver-eud b/Documentation/ABI/testing/sysfs-driver-eud
new file mode 100644
index 0000000..2381552
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-eud
@@ -0,0 +1,9 @@
+What:		/sys/bus/platform/drivers/eud/.../enable
+Date:           January 2022
+Contact:        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
+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.
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e718b87..abc6be0 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -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
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 70d5de6..e0c7d2d 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -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
diff --git a/drivers/soc/qcom/qcom_eud.c b/drivers/soc/qcom/qcom_eud.c
new file mode 100644
index 0000000..a538645
--- /dev/null
+++ b/drivers/soc/qcom/qcom_eud.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/bitops.h>
+#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)
+#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			*base;
+	void __iomem			*mode_mgr;
+	unsigned int			int_status;
+	int				irq;
+	bool				enabled;
+	bool				usb_attached;
+};
+
+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);
+
+	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
+}
+
+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);
+}
+
+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->enabled);
+}
+
+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->enabled = enable;
+		else
+			disable_eud(chip);
+	} 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;
+
+	/* read ctl_out_1[4] to find USB attach or detach event */
+	reg = readl(chip->base + EUD_REG_CTL_OUT_1);
+	chip->usb_attached = reg & EUD_INT_SAFE_MODE;
+}
+
+static void pet_eud(struct eud_chip *chip)
+{
+	u32 reg;
+	int ret;
+
+	/* When the EUD_INT_PET_EUD in SW_ATTACH_DET is set, the cable has been
+	 * disconnected and we need to detach the pet to check if EUD is in safe
+	 * mode before attaching again.
+	 */
+	reg = readl(chip->base + EUD_REG_SW_ATTACH_DET);
+	if (reg & EUD_INT_PET_EUD) {
+		/* Detach & Attach pet for EUD */
+		writel(0, chip->base + EUD_REG_SW_ATTACH_DET);
+		/* Delay to make sure detach pet is done before attach pet */
+		ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
+					reg, (reg == 0), 1, 100);
+		if (ret) {
+			dev_err(chip->dev, "Detach pet failed\n");
+			return;
+		}
+	}
+	/* Attach pet for EUD */
+	writel(EUD_INT_PET_EUD, chip->base +EUD_REG_SW_ATTACH_DET);
+}
+
+static irqreturn_t handle_eud_irq(int irq, void *data)
+{
+	struct eud_chip *chip = data;
+	u32 reg;
+
+	reg = readl(chip->base + EUD_REG_INT_STATUS_1);
+	switch (reg & EUD_INT_ALL) {
+	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);
+		return IRQ_HANDLED;
+	default:
+		return IRQ_NONE;
+	}
+}
+
+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) {
+		if (chip->usb_attached)
+			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");
+	}
+
+	/* set and clear vbus_int_clr[0] to clear interrupt */
+	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
+	writel(0, chip->base + EUD_REG_VBUS_INT_CLR);
+
+	return IRQ_HANDLED;
+}
+
+static int eud_probe(struct platform_device *pdev)
+{
+	struct eud_chip *chip;
+	struct fwnode_handle *fwnode = pdev->dev.fwnode, *dwc3;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &pdev->dev;
+
+	dwc3 = fwnode_graph_get_next_endpoint(fwnode, NULL);
+	if (!dwc3)
+		return -ENODEV;
+
+	chip->role_sw = fwnode_usb_role_switch_get(dwc3);
+	if (IS_ERR(chip->role_sw)) {
+		ret = PTR_ERR(chip->role_sw);
+		usb_role_switch_put(chip->role_sw);
+		return dev_err_probe(chip->dev, ret,
+					"failed to get role switch\n");
+	}
+
+	chip->base = devm_platform_ioremap_resource(pdev, 0);
+	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);
+
+	chip->irq = platform_get_irq(pdev, 0);
+	ret = devm_request_threaded_irq(&pdev->dev, chip->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->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->enabled)
+		disable_eud(chip);
+
+	device_init_wakeup(&pdev->dev, false);
+	disable_irq_wake(chip->irq);
+
+	return 0;
+}
+
+static const struct of_device_id eud_dt_match[] = {
+	{ .compatible = "qcom,sc7280-eud" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, eud_dt_match);
+
+static struct platform_driver eud_driver = {
+	.probe	= eud_probe,
+	.remove	= eud_remove,
+	.driver	= {
+		.name = "qcom_eud",
+		.dev_groups = eud_groups,
+		.of_match_table = eud_dt_match,
+	},
+};
+module_platform_driver(eud_driver);
+
+MODULE_DESCRIPTION("QTI EUD driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4

[PATCH V4 4/6] arm64: dts: qcom: sc7280: Add EUD dt node and dwc3 connector

[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 typec connector node for EUD which is attached to
EUD node via port. EUD is also attached to DWC3 node via port.
Also add the role-switch property to dwc3 node.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 39 ++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 937c2e0..daac831 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2583,6 +2583,14 @@
 				phys = <&usb_2_hsphy>;
 				phy-names = "usb2-phy";
 				maximum-speed = "high-speed";
+				usb-role-switch;
+				ports {
+					port@0 {
+						usb2_role_switch: endpoint {
+							remote-endpoint = <&eud_ep>;
+						};
+					};
+				};
 			};
 		};
 
@@ -2624,6 +2632,37 @@
 			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>;
+			ports {
+				port@0 {
+					eud_ep: endpoint {
+						remote-endpoint = <&usb2_role_switch>;
+					};
+				};
+				port@1 {
+					eud_con: endpoint {
+						remote-endpoint = <&con_eud>;
+					};
+				};
+			};
+		};
+
+		eud_typec: connector {
+			compatible = "usb-c-connector";
+			ports {
+				port@0 {
+					con_eud: endpoint {
+						remote-endpoint = <&eud_con>;
+					};
+				};
+			};
+		};
+
 		nsp_noc: interconnect@a0c0000 {
 			reg = <0 0x0a0c0000 0 0x10000>;
 			compatible = "qcom,sc7280-nsp-noc";
-- 
2.7.4

[PATCH V4 5/6] arm64: dts: qcom: sc7280: Set the default dr_mode for usb2

[Souradeep Chowdhury]

Set the default dr_mode for usb2 node to "otg" to enable
role-switch for EUD(Embedded USB Debugger) connector node.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 9b991ba..f40eaa5 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
@@ -61,6 +61,10 @@
 	modem-init;
 };
 
+&usb_2_dwc3 {
+	dr_mode = "otg";
+};
+
 &pmk8350_rtc {
 	status = "okay";
 };
-- 
2.7.4

[PATCH V4 6/6] MAINTAINERS: Add maintainer entry for EUD

[Souradeep Chowdhury]

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

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b84e2d5..0fa9d54 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7227,6 +7227,14 @@ F:	include/uapi/linux/mdio.h
 F:	include/uapi/linux/mii.h
 F:	net/core/of_net.c
 
+QCOM EMBEDDED USB DEBUGGER(EUD)
+M:	Souradeep Chowdhury <quic_schowdhu@quicinc.com>
+L:	linux-arm-msm@vger.kernel.org
+S:	Maintained
+F:	Documentation/ABI/testing/sysfs-driver-eud
+F:	Documentation/devicetree/bindings/arm/msm/qcom,eud.yaml
+F:	drivers/usb/common/qcom_eud.c
+
 EXEC & BINFMT API
 R:	Eric Biederman <ebiederm@xmission.com>
 R:	Kees Cook <keescook@chromium.org>
-- 
2.7.4

Re: [PATCH V4 2/6] bindings: usb: dwc3: Update dwc3 properties for EUD connector

[Bjorn Andersson]

On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:

> Add the ports property for dwc3 node. This port can be used
> by the Embedded USB Debugger for role switching the controller
> from device to host mode and vice versa.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index d29ffcd..ccb1236 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -332,6 +332,16 @@ properties:
>      items:
>        enum: [1, 4, 8, 16, 32, 64, 128, 256]
>  
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      This port is to be attached to the endpoint of the Embedded USB Debugger.

More generally this is used together with the already documented
usb-role-switch property to connect the dwc3 to the Type-C connector.

Which makes me feel that we don't actually need ports/port, but could do
with just port? Perhaps I'm missing some usecase?


I'm somewhat confused to why this isn't already documented...

Regards,
Bjorn

> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Connector endpoint of Embedded USB debugger.
> +
>  unevaluatedProperties: false
>  
>  required:
> -- 
> 2.7.4
> 

Re: [PATCH V4 1/6] dt-bindings: Add the yaml bindings for EUD

[Bjorn Andersson]

On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml     | 77 ++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> new file mode 100644
> index 0000000..c98aab2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> @@ -0,0 +1,77 @@
> +# 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 <quic_schowdhu@quicinc.com>
> +
> +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:
> +    items:
> +      - description: EUD Base Register Region
> +      - description: EUD Mode Manager Register
> +
> +  interrupts:
> +    description: EUD interrupt
> +    maxItems: 1
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      These ports are to be attached to the endpoint of the DWC3 controller node
> +      and type C connector node. The controller has the "usb-role-switch"
> +      property.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: This port is to be attached to the DWC3 controller.
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: This port is to be attached to the type C connector.
> +
> +required:
> +  - compatible
> +  - reg
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    eud@88e0000 {
> +           compatible = "qcom,sc7280-eud","qcom,eud";
> +           reg = <0x88e0000 0x2000>,
> +                 <0x88e2000 0x1000>;
> +           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>;
> +                           };
> +                   };
> +           };
> +    };
> --
> 2.7.4
> 

Re: [PATCH V4 4/6] arm64: dts: qcom: sc7280: Add EUD dt node and dwc3 connector

[Bjorn Andersson]

On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:

> 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 typec connector node for EUD which is attached to
> EUD node via port. EUD is also attached to DWC3 node via port.
> Also add the role-switch property to dwc3 node.
> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 39 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 937c2e0..daac831 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2583,6 +2583,14 @@
>  				phys = <&usb_2_hsphy>;
>  				phy-names = "usb2-phy";
>  				maximum-speed = "high-speed";
> +				usb-role-switch;
> +				ports {
> +					port@0 {
> +						usb2_role_switch: endpoint {
> +							remote-endpoint = <&eud_ep>;
> +						};
> +					};
> +				};
>  			};
>  		};
>  
> @@ -2624,6 +2632,37 @@
>  			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>;

I find "interrupts-extended = <&pdc 11 IRQ_TYPE_LEVEL_HIGH>;" cleaner
than having to specify both parent and interrupts.

> +			ports {
> +				port@0 {
> +					eud_ep: endpoint {
> +						remote-endpoint = <&usb2_role_switch>;
> +					};
> +				};
> +				port@1 {
> +					eud_con: endpoint {
> +						remote-endpoint = <&con_eud>;
> +					};
> +				};
> +			};
> +		};
> +
> +		eud_typec: connector {

The connector should be a child of the Type-C controller, which I know
differs between the various devices on this platform. So you should
leave &eud_con without a remote-endpoint and then fill that in for each
device, based on respective Type-C controller.


But beyond that, I think this design looks good now!

Regards,
Bjorn

> +			compatible = "usb-c-connector";
> +			ports {
> +				port@0 {
> +					con_eud: endpoint {
> +						remote-endpoint = <&eud_con>;
> +					};
> +				};
> +			};
> +		};
> +
>  		nsp_noc: interconnect@a0c0000 {
>  			reg = <0 0x0a0c0000 0 0x10000>;
>  			compatible = "qcom,sc7280-nsp-noc";
> -- 
> 2.7.4
> 

Re: [PATCH V4 5/6] arm64: dts: qcom: sc7280: Set the default dr_mode for usb2

[Bjorn Andersson]

On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:

> Set the default dr_mode for usb2 node to "otg" to enable
> role-switch for EUD(Embedded USB Debugger) connector node.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> index 9b991ba..f40eaa5 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> @@ -61,6 +61,10 @@
>  	modem-init;
>  };
>  
> +&usb_2_dwc3 {
> +	dr_mode = "otg";
> +};
> +
>  &pmk8350_rtc {
>  	status = "okay";
>  };
> -- 
> 2.7.4
> 

Re: [PATCH V4 6/6] MAINTAINERS: Add maintainer entry for EUD

[Bjorn Andersson]

On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  MAINTAINERS | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b84e2d5..0fa9d54 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7227,6 +7227,14 @@ F:	include/uapi/linux/mdio.h
>  F:	include/uapi/linux/mii.h
>  F:	net/core/of_net.c
>  
> +QCOM EMBEDDED USB DEBUGGER(EUD)
> +M:	Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +L:	linux-arm-msm@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-driver-eud
> +F:	Documentation/devicetree/bindings/arm/msm/qcom,eud.yaml
> +F:	drivers/usb/common/qcom_eud.c
> +
>  EXEC & BINFMT API
>  R:	Eric Biederman <ebiederm@xmission.com>
>  R:	Kees Cook <keescook@chromium.org>
> -- 
> 2.7.4
> 

Re: [PATCH V4 3/6] soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)

[Bjorn Andersson]

On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:

> 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 <quic_schowdhu@quicinc.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-eud |   9 ++
>  drivers/soc/qcom/Kconfig                   |  10 ++
>  drivers/soc/qcom/Makefile                  |   1 +
>  drivers/soc/qcom/qcom_eud.c                | 250 +++++++++++++++++++++++++++++
>  4 files changed, 270 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-eud
>  create mode 100644 drivers/soc/qcom/qcom_eud.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-eud b/Documentation/ABI/testing/sysfs-driver-eud
> new file mode 100644
> index 0000000..2381552
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-eud
> @@ -0,0 +1,9 @@
> +What:		/sys/bus/platform/drivers/eud/.../enable
> +Date:           January 2022
> +Contact:        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> +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.
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index e718b87..abc6be0 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -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"

The indentation looks off here.

> +	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
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 70d5de6..e0c7d2d 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -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
> diff --git a/drivers/soc/qcom/qcom_eud.c b/drivers/soc/qcom/qcom_eud.c
> new file mode 100644
> index 0000000..a538645
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom_eud.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/bitops.h>
> +#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)
> +#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			*base;
> +	void __iomem			*mode_mgr;
> +	unsigned int			int_status;
> +	int				irq;
> +	bool				enabled;
> +	bool				usb_attached;
> +};
> +
> +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);
> +
> +	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);

So we won't get EUD_INT_VBUS when we enable the EUD and can rely on the
irq handler to set the role?

> +}
> +
> +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);
> +}
> +
> +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->enabled);
> +}
> +
> +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->enabled = enable;
> +		else
> +			disable_eud(chip);
> +	} 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;
> +
> +	/* read ctl_out_1[4] to find USB attach or detach event */
> +	reg = readl(chip->base + EUD_REG_CTL_OUT_1);
> +	chip->usb_attached = reg & EUD_INT_SAFE_MODE;
> +}
> +
> +static void pet_eud(struct eud_chip *chip)
> +{
> +	u32 reg;
> +	int ret;
> +
> +	/* When the EUD_INT_PET_EUD in SW_ATTACH_DET is set, the cable has been
> +	 * disconnected and we need to detach the pet to check if EUD is in safe
> +	 * mode before attaching again.
> +	 */
> +	reg = readl(chip->base + EUD_REG_SW_ATTACH_DET);
> +	if (reg & EUD_INT_PET_EUD) {
> +		/* Detach & Attach pet for EUD */
> +		writel(0, chip->base + EUD_REG_SW_ATTACH_DET);
> +		/* Delay to make sure detach pet is done before attach pet */
> +		ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
> +					reg, (reg == 0), 1, 100);
> +		if (ret) {
> +			dev_err(chip->dev, "Detach pet failed\n");
> +			return;
> +		}
> +	}
> +	/* Attach pet for EUD */
> +	writel(EUD_INT_PET_EUD, chip->base +EUD_REG_SW_ATTACH_DET);
> +}
> +
> +static irqreturn_t handle_eud_irq(int irq, void *data)
> +{
> +	struct eud_chip *chip = data;
> +	u32 reg;
> +
> +	reg = readl(chip->base + EUD_REG_INT_STATUS_1);
> +	switch (reg & EUD_INT_ALL) {
> +	case EUD_INT_VBUS:
> +		chip->int_status = EUD_INT_VBUS;

The first time that reg & EUD_INT_VBUS is set, you assign int_status
EUD_INT_VBUS, you never clear it again.

This is also the only path where you wake up the thread, so int_status
will always be EUD_INT_VBUS when you reach handle_eud_irq_thread().

Which means that int_status serves no purpose and if you're happy with
how this implementation currently works you can just drop "int_status"
and the conditional below.

> +		usb_attach_detach(chip);
> +		return IRQ_WAKE_THREAD;
> +	case EUD_INT_SAFE_MODE:
> +		pet_eud(chip);
> +		return IRQ_HANDLED;
> +	default:
> +		return IRQ_NONE;
> +	}
> +}
> +
> +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) {
> +		if (chip->usb_attached)
> +			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");
> +	}
> +
> +	/* set and clear vbus_int_clr[0] to clear interrupt */
> +	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
> +	writel(0, chip->base + EUD_REG_VBUS_INT_CLR);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int eud_probe(struct platform_device *pdev)
> +{
> +	struct eud_chip *chip;
> +	struct fwnode_handle *fwnode = pdev->dev.fwnode, *dwc3;
> +	int ret;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->dev = &pdev->dev;
> +
> +	dwc3 = fwnode_graph_get_next_endpoint(fwnode, NULL);

This will pick the first endpoint, but if you instead use

    chip->role_sw = usb_role_switch_get(&pdev->dev);

you should get whichever port that points to a usb-role-switch node,
without having to do the fwnode dance (and refcounting, which you forgot
to release).

> +	if (!dwc3)
> +		return -ENODEV;
> +
> +	chip->role_sw = fwnode_usb_role_switch_get(dwc3);
> +	if (IS_ERR(chip->role_sw)) {
> +		ret = PTR_ERR(chip->role_sw);
> +		usb_role_switch_put(chip->role_sw);

You don't need to return the role_sw if it's IS_ERR().

> +		return dev_err_probe(chip->dev, ret,
> +					"failed to get role switch\n");
> +	}
> +
> +	chip->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(chip->base))

You're not usb_role_switch_put() your role_sw here, or below return
cases. I would recommend devm_add_action_or_reset() to avoid the hassle
of adding the necessary cleanup logic.

> +		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);
> +
> +	chip->irq = platform_get_irq(pdev, 0);
> +	ret = devm_request_threaded_irq(&pdev->dev, chip->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->irq);
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	return 0;

Per the updated binding, the EUD would now be a usb-role-switch as well
and when not enabled should simply propagate the incoming requests. So I
was expecting this to register as a usb_role_switch as well...

> +}
> +
> +static int eud_remove(struct platform_device *pdev)
> +{
> +	struct eud_chip *chip = platform_get_drvdata(pdev);
> +
> +	if (chip->enabled)
> +		disable_eud(chip);
> +
> +	device_init_wakeup(&pdev->dev, false);
> +	disable_irq_wake(chip->irq);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id eud_dt_match[] = {
> +	{ .compatible = "qcom,sc7280-eud" },

Do you see any reason for not just adding qcom,eud here? Are there any
differences from other platforms that has this block that means that we
need per-platform driver support (the dts should have both
compatibles still)?

Regards,
Bjorn

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, eud_dt_match);
> +
> +static struct platform_driver eud_driver = {
> +	.probe	= eud_probe,
> +	.remove	= eud_remove,
> +	.driver	= {
> +		.name = "qcom_eud",
> +		.dev_groups = eud_groups,
> +		.of_match_table = eud_dt_match,
> +	},
> +};
> +module_platform_driver(eud_driver);
> +
> +MODULE_DESCRIPTION("QTI EUD driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
> 

Re: [PATCH V4 3/6] soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)

[Greg KH]

On Fri, Jan 21, 2022 at 07:23:48PM +0530, Souradeep Chowdhury wrote:
> 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 <quic_schowdhu@quicinc.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-eud |   9 ++
>  drivers/soc/qcom/Kconfig                   |  10 ++
>  drivers/soc/qcom/Makefile                  |   1 +
>  drivers/soc/qcom/qcom_eud.c                | 250 +++++++++++++++++++++++++++++

This should go under drivers/usb/ as it's creating a USB generic
user/kernel api that all future devices of this type must follow.

thanks,

greg k-h

Re: [PATCH V4 2/6] bindings: usb: dwc3: Update dwc3 properties for EUD connector

[Souradeep Chowdhury]

On 1/26/2022 9:51 AM, Bjorn Andersson wrote:
> On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:
>
>> Add the ports property for dwc3 node. This port can be used
>> by the Embedded USB Debugger for role switching the controller
>> from device to host mode and vice versa.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index d29ffcd..ccb1236 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -332,6 +332,16 @@ properties:
>>       items:
>>         enum: [1, 4, 8, 16, 32, 64, 128, 256]
>>   
>> +  ports:
>> +    $ref: /schemas/graph.yaml#/properties/ports
>> +    description:
>> +      This port is to be attached to the endpoint of the Embedded USB Debugger.
> More generally this is used together with the already documented
> usb-role-switch property to connect the dwc3 to the Type-C connector.
>
> Which makes me feel that we don't actually need ports/port, but could do
> with just port? Perhaps I'm missing some usecase?
>
>
> I'm somewhat confused to why this isn't already documented...

Ack. Will document this as a port.

>
> Regards,
> Bjorn
>
>> +
>> +    properties:
>> +      port@0:
>> +        $ref: /schemas/graph.yaml#/properties/port
>> +        description: Connector endpoint of Embedded USB debugger.
>> +
>>   unevaluatedProperties: false
>>   
>>   required:
>> -- 
>> 2.7.4
>>

Re: [PATCH V4 4/6] arm64: dts: qcom: sc7280: Add EUD dt node and dwc3 connector

[Souradeep Chowdhury]

On 1/26/2022 9:58 AM, Bjorn Andersson wrote:
> On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:
>
>> 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 typec connector node for EUD which is attached to
>> EUD node via port. EUD is also attached to DWC3 node via port.
>> Also add the role-switch property to dwc3 node.
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sc7280.dtsi | 39 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 937c2e0..daac831 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -2583,6 +2583,14 @@
>>   				phys = <&usb_2_hsphy>;
>>   				phy-names = "usb2-phy";
>>   				maximum-speed = "high-speed";
>> +				usb-role-switch;
>> +				ports {
>> +					port@0 {
>> +						usb2_role_switch: endpoint {
>> +							remote-endpoint = <&eud_ep>;
>> +						};
>> +					};
>> +				};
>>   			};
>>   		};
>>   
>> @@ -2624,6 +2632,37 @@
>>   			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>;
> I find "interrupts-extended = <&pdc 11 IRQ_TYPE_LEVEL_HIGH>;" cleaner
> than having to specify both parent and interrupts.
Ack
>
>> +			ports {
>> +				port@0 {
>> +					eud_ep: endpoint {
>> +						remote-endpoint = <&usb2_role_switch>;
>> +					};
>> +				};
>> +				port@1 {
>> +					eud_con: endpoint {
>> +						remote-endpoint = <&con_eud>;
>> +					};
>> +				};
>> +			};
>> +		};
>> +
>> +		eud_typec: connector {
> The connector should be a child of the Type-C controller, which I know
> differs between the various devices on this platform. So you should
> leave &eud_con without a remote-endpoint and then fill that in for each
> device, based on respective Type-C controller.
>
>
> But beyond that, I think this design looks good now!
Ack
>
> Regards,
> Bjorn
>
>> +			compatible = "usb-c-connector";
>> +			ports {
>> +				port@0 {
>> +					con_eud: endpoint {
>> +						remote-endpoint = <&eud_con>;
>> +					};
>> +				};
>> +			};
>> +		};
>> +
>>   		nsp_noc: interconnect@a0c0000 {
>>   			reg = <0 0x0a0c0000 0 0x10000>;
>>   			compatible = "qcom,sc7280-nsp-noc";
>> -- 
>> 2.7.4
>>

Re: [PATCH V4 3/6] soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)

[Souradeep Chowdhury]

On 1/26/2022 10:17 AM, Bjorn Andersson wrote:
> On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:
>
>> 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 <quic_schowdhu@quicinc.com>
>> ---
>>   Documentation/ABI/testing/sysfs-driver-eud |   9 ++
>>   drivers/soc/qcom/Kconfig                   |  10 ++
>>   drivers/soc/qcom/Makefile                  |   1 +
>>   drivers/soc/qcom/qcom_eud.c                | 250 +++++++++++++++++++++++++++++
>>   4 files changed, 270 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-driver-eud
>>   create mode 100644 drivers/soc/qcom/qcom_eud.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-eud b/Documentation/ABI/testing/sysfs-driver-eud
>> new file mode 100644
>> index 0000000..2381552
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-eud
>> @@ -0,0 +1,9 @@
>> +What:		/sys/bus/platform/drivers/eud/.../enable
>> +Date:           January 2022
>> +Contact:        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> +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.
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index e718b87..abc6be0 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -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"
> The indentation looks off here.
Ack
>
>> +	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
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 70d5de6..e0c7d2d 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -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
>> diff --git a/drivers/soc/qcom/qcom_eud.c b/drivers/soc/qcom/qcom_eud.c
>> new file mode 100644
>> index 0000000..a538645
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom_eud.c
>> @@ -0,0 +1,250 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#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)
>> +#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			*base;
>> +	void __iomem			*mode_mgr;
>> +	unsigned int			int_status;
>> +	int				irq;
>> +	bool				enabled;
>> +	bool				usb_attached;
>> +};
>> +
>> +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);
>> +
>> +	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
> So we won't get EUD_INT_VBUS when we enable the EUD and can rely on the
> irq handler to set the role?

Yes, in this case it is being explicitly set based on the user input 
rather than through irq handler.

In this way we are able to detect if the role is set properly and take 
the next steps from the

enable_store method.

>
>> +}
>> +
>> +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);
>> +}
>> +
>> +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->enabled);
>> +}
>> +
>> +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->enabled = enable;
>> +		else
>> +			disable_eud(chip);
>> +	} 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;
>> +
>> +	/* read ctl_out_1[4] to find USB attach or detach event */
>> +	reg = readl(chip->base + EUD_REG_CTL_OUT_1);
>> +	chip->usb_attached = reg & EUD_INT_SAFE_MODE;
>> +}
>> +
>> +static void pet_eud(struct eud_chip *chip)
>> +{
>> +	u32 reg;
>> +	int ret;
>> +
>> +	/* When the EUD_INT_PET_EUD in SW_ATTACH_DET is set, the cable has been
>> +	 * disconnected and we need to detach the pet to check if EUD is in safe
>> +	 * mode before attaching again.
>> +	 */
>> +	reg = readl(chip->base + EUD_REG_SW_ATTACH_DET);
>> +	if (reg & EUD_INT_PET_EUD) {
>> +		/* Detach & Attach pet for EUD */
>> +		writel(0, chip->base + EUD_REG_SW_ATTACH_DET);
>> +		/* Delay to make sure detach pet is done before attach pet */
>> +		ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
>> +					reg, (reg == 0), 1, 100);
>> +		if (ret) {
>> +			dev_err(chip->dev, "Detach pet failed\n");
>> +			return;
>> +		}
>> +	}
>> +	/* Attach pet for EUD */
>> +	writel(EUD_INT_PET_EUD, chip->base +EUD_REG_SW_ATTACH_DET);
>> +}
>> +
>> +static irqreturn_t handle_eud_irq(int irq, void *data)
>> +{
>> +	struct eud_chip *chip = data;
>> +	u32 reg;
>> +
>> +	reg = readl(chip->base + EUD_REG_INT_STATUS_1);
>> +	switch (reg & EUD_INT_ALL) {
>> +	case EUD_INT_VBUS:
>> +		chip->int_status = EUD_INT_VBUS;
> The first time that reg & EUD_INT_VBUS is set, you assign int_status
> EUD_INT_VBUS, you never clear it again.
>
> This is also the only path where you wake up the thread, so int_status
> will always be EUD_INT_VBUS when you reach handle_eud_irq_thread().
>
> Which means that int_status serves no purpose and if you're happy with
> how this implementation currently works you can just drop "int_status"
> and the conditional below.
Ack. Will remove the int_status flag as it is not needed.
>
>> +		usb_attach_detach(chip);
>> +		return IRQ_WAKE_THREAD;
>> +	case EUD_INT_SAFE_MODE:
>> +		pet_eud(chip);
>> +		return IRQ_HANDLED;
>> +	default:
>> +		return IRQ_NONE;
>> +	}
>> +}
>> +
>> +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) {
>> +		if (chip->usb_attached)
>> +			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");
>> +	}
>> +
>> +	/* set and clear vbus_int_clr[0] to clear interrupt */
>> +	writel(BIT(0), chip->base + EUD_REG_VBUS_INT_CLR);
>> +	writel(0, chip->base + EUD_REG_VBUS_INT_CLR);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int eud_probe(struct platform_device *pdev)
>> +{
>> +	struct eud_chip *chip;
>> +	struct fwnode_handle *fwnode = pdev->dev.fwnode, *dwc3;
>> +	int ret;
>> +
>> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> +	if (!chip)
>> +		return -ENOMEM;
>> +
>> +	chip->dev = &pdev->dev;
>> +
>> +	dwc3 = fwnode_graph_get_next_endpoint(fwnode, NULL);
> This will pick the first endpoint, but if you instead use
>
>      chip->role_sw = usb_role_switch_get(&pdev->dev);
>
> you should get whichever port that points to a usb-role-switch node,
> without having to do the fwnode dance (and refcounting, which you forgot
> to release).
Ack
>
>> +	if (!dwc3)
>> +		return -ENODEV;
>> +
>> +	chip->role_sw = fwnode_usb_role_switch_get(dwc3);
>> +	if (IS_ERR(chip->role_sw)) {
>> +		ret = PTR_ERR(chip->role_sw);
>> +		usb_role_switch_put(chip->role_sw);
> You don't need to return the role_sw if it's IS_ERR().
Ack
>
>> +		return dev_err_probe(chip->dev, ret,
>> +					"failed to get role switch\n");
>> +	}
>> +
>> +	chip->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(chip->base))
> You're not usb_role_switch_put() your role_sw here, or below return
> cases. I would recommend devm_add_action_or_reset() to avoid the hassle
> of adding the necessary cleanup logic.
Ack
>
>> +		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);
>> +
>> +	chip->irq = platform_get_irq(pdev, 0);
>> +	ret = devm_request_threaded_irq(&pdev->dev, chip->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->irq);
>> +
>> +	platform_set_drvdata(pdev, chip);
>> +
>> +	return 0;
> Per the updated binding, the EUD would now be a usb-role-switch as well
> and when not enabled should simply propagate the incoming requests. So I
> was expecting this to register as a usb_role_switch as well...

Can you please elaborate on this?

Do I need to define a separate 'usb_role_switch_desc' here and register 
using 'usb_role_switch_register'?

Also what should be the set method in this case for usb_role_switch_desc?

>
>> +}
>> +
>> +static int eud_remove(struct platform_device *pdev)
>> +{
>> +	struct eud_chip *chip = platform_get_drvdata(pdev);
>> +
>> +	if (chip->enabled)
>> +		disable_eud(chip);
>> +
>> +	device_init_wakeup(&pdev->dev, false);
>> +	disable_irq_wake(chip->irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id eud_dt_match[] = {
>> +	{ .compatible = "qcom,sc7280-eud" },
> Do you see any reason for not just adding qcom,eud here? Are there any
> differences from other platforms that has this block that means that we
> need per-platform driver support (the dts should have both
> compatibles still)?
That is correct, this might need driver support for multiple platforms.
>
> Regards,
> Bjorn
>
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, eud_dt_match);
>> +
>> +static struct platform_driver eud_driver = {
>> +	.probe	= eud_probe,
>> +	.remove	= eud_remove,
>> +	.driver	= {
>> +		.name = "qcom_eud",
>> +		.dev_groups = eud_groups,
>> +		.of_match_table = eud_dt_match,
>> +	},
>> +};
>> +module_platform_driver(eud_driver);
>> +
>> +MODULE_DESCRIPTION("QTI EUD driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.7.4
>>

Re: [PATCH V4 3/6] soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)

[Souradeep Chowdhury]

On 1/26/2022 3:00 PM, Greg KH wrote:
> On Fri, Jan 21, 2022 at 07:23:48PM +0530, Souradeep Chowdhury wrote:
>> 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 <quic_schowdhu@quicinc.com>
>> ---
>>   Documentation/ABI/testing/sysfs-driver-eud |   9 ++
>>   drivers/soc/qcom/Kconfig                   |  10 ++
>>   drivers/soc/qcom/Makefile                  |   1 +
>>   drivers/soc/qcom/qcom_eud.c                | 250 +++++++++++++++++++++++++++++
> This should go under drivers/usb/ as it's creating a USB generic
> user/kernel api that all future devices of this type must follow.
Ack. Will move this to drivers/usb.
>
> thanks,
>
> greg k-h

Re: [PATCH V4 3/6] soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)

[Bjorn Andersson]

On Thu 27 Jan 04:01 PST 2022, Souradeep Chowdhury wrote:

> 
> On 1/26/2022 10:17 AM, Bjorn Andersson wrote:
> > On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:
[..]
> > > +		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);
> > > +
> > > +	chip->irq = platform_get_irq(pdev, 0);
> > > +	ret = devm_request_threaded_irq(&pdev->dev, chip->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->irq);
> > > +
> > > +	platform_set_drvdata(pdev, chip);
> > > +
> > > +	return 0;
> > Per the updated binding, the EUD would now be a usb-role-switch as well
> > and when not enabled should simply propagate the incoming requests. So I
> > was expecting this to register as a usb_role_switch as well...
> 
> Can you please elaborate on this?
> 
> Do I need to define a separate 'usb_role_switch_desc' here and register
> using 'usb_role_switch_register'?
> 
> Also what should be the set method in this case for usb_role_switch_desc?
> 

My expectation is that in normal operation pmic_glink will provide role
switching requests and then as you enable the EUD it will force the role
to gadget.

So my suggestion was that you make eud a role-switch and as long as EUD
is disabled you just pass through the role-switch vote from pmic_glink
onto the dwc3.

Perhaps I'm misunderstanding how this is really working.

Regards,
Bjorn

Re: [PATCH V4 3/6] soc: qcom: eud: Add driver support for Embedded USB Debugger(EUD)

[Souradeep Chowdhury]

On 1/28/2022 9:41 PM, Bjorn Andersson wrote:
> On Thu 27 Jan 04:01 PST 2022, Souradeep Chowdhury wrote:
>
>> On 1/26/2022 10:17 AM, Bjorn Andersson wrote:
>>> On Fri 21 Jan 07:53 CST 2022, Souradeep Chowdhury wrote:
> [..]
>>>> +		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);
>>>> +
>>>> +	chip->irq = platform_get_irq(pdev, 0);
>>>> +	ret = devm_request_threaded_irq(&pdev->dev, chip->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->irq);
>>>> +
>>>> +	platform_set_drvdata(pdev, chip);
>>>> +
>>>> +	return 0;
>>> Per the updated binding, the EUD would now be a usb-role-switch as well
>>> and when not enabled should simply propagate the incoming requests. So I
>>> was expecting this to register as a usb_role_switch as well...
>> Can you please elaborate on this?
>>
>> Do I need to define a separate 'usb_role_switch_desc' here and register
>> using 'usb_role_switch_register'?
>>
>> Also what should be the set method in this case for usb_role_switch_desc?
>>
> My expectation is that in normal operation pmic_glink will provide role
> switching requests and then as you enable the EUD it will force the role
> to gadget.
>
> So my suggestion was that you make eud a role-switch and as long as EUD
> is disabled you just pass through the role-switch vote from pmic_glink
> onto the dwc3.
>
> Perhaps I'm misunderstanding how this is really working.

As per the code, the role-switching on dwc3 is enabled from 
drivers/usb/dwc3/drd.c on setting the usb mode to "otg " ,

that is being done on the 5th patch of this series. There is no other 
entity that is switching role for dwc3 other than EUD.

Also the role-switch registration happens from drivers/usb/dwc3/drd.c as 
follows

static int dwc3_setup_role_switch(struct dwc3 *dwc)
{
         struct usb_role_switch_desc dwc3_role_switch = {NULL};
         u32 mode;

         dwc->role_switch_default_mode = 
usb_get_role_switch_default_mode(dwc->dev);
         if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) {
                 mode = DWC3_GCTL_PRTCAP_HOST;
         } else {
                 dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
                 mode = DWC3_GCTL_PRTCAP_DEVICE;
         }

         dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
         dwc3_role_switch.set = dwc3_usb_role_switch_set;
         dwc3_role_switch.get = dwc3_usb_role_switch_get;
         dwc3_role_switch.driver_data = dwc;
         dwc->role_sw = usb_role_switch_register(dwc->dev, 
&dwc3_role_switch);
         if (IS_ERR(dwc->role_sw))
                 return PTR_ERR(dwc->role_sw);

         dwc3_set_mode(dwc, mode);
         return 0;
}

Can you please point out the pmic_glink driver upstream and how it is 
related in this case?

>
> Regards,
> Bjorn