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

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

[Bhupesh Sharma]

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

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

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

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

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

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

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

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

 Documentation/ABI/testing/sysfs-driver-eud    |  2 +-
 .../bindings/soc/qcom/qcom,eud.yaml           | 16 ++++
 arch/arm64/boot/dts/qcom/qrb4210-rb2.dts      | 27 ++++++-
 arch/arm64/boot/dts/qcom/sm6115.dtsi          | 51 +++++++++++++
 drivers/usb/misc/Kconfig                      |  1 +
 drivers/usb/misc/qcom_eud.c                   | 74 +++++++++++++++++--
 6 files changed, 162 insertions(+), 9 deletions(-)

-- 
2.38.1

[PATCH v4 1/5] usb: misc: eud: Fix eud sysfs path (use 'qcom_eud')

[Bhupesh Sharma]

The eud sysfs enablement path is currently mentioned in the
Documentation as:
  /sys/bus/platform/drivers/eud/.../enable

Instead it should be:
  /sys/bus/platform/drivers/qcom_eud/.../enable

Fix the same.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 Documentation/ABI/testing/sysfs-driver-eud | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-eud b/Documentation/ABI/testing/sysfs-driver-eud
index 83f3872182a4..2bab0db2d2f0 100644
--- a/Documentation/ABI/testing/sysfs-driver-eud
+++ b/Documentation/ABI/testing/sysfs-driver-eud
@@ -1,4 +1,4 @@
-What:		/sys/bus/platform/drivers/eud/.../enable
+What:		/sys/bus/platform/drivers/qcom_eud/.../enable
 Date:           February 2022
 Contact:        Souradeep Chowdhury <quic_schowdhu@quicinc.com>
 Description:
-- 
2.38.1

[PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Bhupesh Sharma]

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

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

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

Also for these SoCs, introduce a new bool property
'qcom,secure-mode-enable', which indicates that the mode manager
needs to be accessed only via the secure world.

Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 .../devicetree/bindings/soc/qcom/qcom,eud.yaml   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
index f2c5ec7e6437..3b92cdf4e306 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
@@ -18,17 +18,33 @@ properties:
     items:
       - enum:
           - qcom,sc7280-eud
+          - qcom,sm6115-eud
       - const: qcom,eud
 
   reg:
+    minItems: 2
     items:
       - description: EUD Base Register Region
       - description: EUD Mode Manager Register
+      - description: TCSR Base Register Region
+
+  reg-names:
+    minItems: 2
+    items:
+      - const: eud-base
+      - const: eud-mode-mgr
+      - const: tcsr-base
 
   interrupts:
     description: EUD interrupt
     maxItems: 1
 
+  qcom,secure-mode-enable:
+    type: boolean
+    description:
+      Indicates that the mode manager needs to be accessed only via the secure
+      world (through 'scm' calls).
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
     description:
-- 
2.38.1

[PATCH v4 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

[Bhupesh Sharma]

Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.

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

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

Since this difference comes from how the firmware is configured, so
the driver now relies on the presence of an extra boolean DT property
to identify if secure access is needed.

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

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 99b15b77dfd5..fe1b5fec1dfc 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
 config USB_QCOM_EUD
 	tristate "QCOM Embedded USB Debugger(EUD) Driver"
 	depends on ARCH_QCOM || COMPILE_TEST
+	select QCOM_SCM
 	select USB_ROLE_SWITCH
 	help
 	  This module enables support for Qualcomm Technologies, Inc.
diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
index b7f13df00764..18a2dee3b4b9 100644
--- a/drivers/usb/misc/qcom_eud.c
+++ b/drivers/usb/misc/qcom_eud.c
@@ -5,12 +5,14 @@
 
 #include <linux/bitops.h>
 #include <linux/err.h>
+#include <linux/firmware/qcom/qcom_scm.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/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -22,23 +24,35 @@
 #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_REG_EUD_EN2		0x0000
 
 #define EUD_ENABLE		BIT(0)
-#define EUD_INT_PET_EUD	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)
 
+#define EUD_EN2_SECURE_EN	BIT(0)
+#define EUD_EN2_NONSECURE_EN	(1)
+#define EUD_EN2_DISABLE		(0)
+#define TCSR_CHECK_EN		BIT(0)
+
+struct eud_soc_cfg {
+	u32 tcsr_check_offset;
+};
+
 struct eud_chip {
 	struct device			*dev;
 	struct usb_role_switch		*role_sw;
+	const struct eud_soc_cfg	*eud_cfg;
 	void __iomem			*base;
 	void __iomem			*mode_mgr;
 	unsigned int			int_status;
 	int				irq;
 	bool				enabled;
 	bool				usb_attached;
+	bool				secure_mode_enable;
+	phys_addr_t			secure_mode_mgr;
 };
 
 static int enable_eud(struct eud_chip *priv)
@@ -46,7 +60,11 @@ static int enable_eud(struct eud_chip *priv)
 	writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
 	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
 			priv->base + EUD_REG_INT1_EN_MASK);
-	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
+
+	if (priv->secure_mode_mgr)
+		qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_SECURE_EN);
+	else
+		writel(EUD_EN2_NONSECURE_EN, priv->mode_mgr + EUD_REG_EUD_EN2);
 
 	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
 }
@@ -54,7 +72,11 @@ static int enable_eud(struct eud_chip *priv)
 static void disable_eud(struct eud_chip *priv)
 {
 	writel(0, priv->base + EUD_REG_CSR_EUD_EN);
-	writel(0, priv->mode_mgr + EUD_REG_EUD_EN2);
+
+	if (priv->secure_mode_mgr)
+		qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_DISABLE);
+	else
+		writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2);
 }
 
 static ssize_t enable_show(struct device *dev,
@@ -178,6 +200,8 @@ static void eud_role_switch_release(void *data)
 static int eud_probe(struct platform_device *pdev)
 {
 	struct eud_chip *chip;
+	struct resource *res;
+	phys_addr_t tcsr_base, tcsr_check;
 	int ret;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
@@ -200,9 +224,40 @@ static int eud_probe(struct platform_device *pdev)
 	if (IS_ERR(chip->base))
 		return PTR_ERR(chip->base);
 
-	chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
-	if (IS_ERR(chip->mode_mgr))
-		return PTR_ERR(chip->mode_mgr);
+	chip->secure_mode_enable = of_property_read_bool(chip->dev->of_node,
+						"qcom,secure-mode-enable");
+	/*
+	 * EUD block on a few Qualcomm SoCs need secure register access.
+	 * Check for the same.
+	 */
+	if (chip->secure_mode_enable) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!res)
+			return dev_err_probe(chip->dev, -ENODEV,
+					     "failed to get secure_mode_mgr reg base\n");
+
+		chip->secure_mode_mgr = res->start;
+	} else {
+		chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
+		if (IS_ERR(chip->mode_mgr))
+			return PTR_ERR(chip->mode_mgr);
+	}
+
+	/* Check for any SoC specific config data */
+	chip->eud_cfg = of_device_get_match_data(&pdev->dev);
+	if (chip->eud_cfg) {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr-base");
+		if (!res)
+			return dev_err_probe(chip->dev, -ENODEV,
+					     "failed to get tcsr reg base\n");
+
+		tcsr_base = res->start;
+		tcsr_check = tcsr_base + chip->eud_cfg->tcsr_check_offset;
+
+		ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN);
+		if (ret)
+			return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n");
+	}
 
 	chip->irq = platform_get_irq(pdev, 0);
 	ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq,
@@ -230,8 +285,13 @@ static int eud_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct eud_soc_cfg sm6115_eud_cfg = {
+	.tcsr_check_offset = 0x25018,
+};
+
 static const struct of_device_id eud_dt_match[] = {
 	{ .compatible = "qcom,sc7280-eud" },
+	{ .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_cfg },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, eud_dt_match);
-- 
2.38.1

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

[Bhupesh Sharma]

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

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

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

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

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

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

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

[PATCH v4 5/5] arm64: dts: qcom: qrb4210-rb2: Enable EUD debug peripheral

[Bhupesh Sharma]

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

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

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

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

Re: [PATCH v4 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

[Konrad Dybcio]

On 5.05.2023 08:40, Bhupesh Sharma wrote:
> Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.
> 
> On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
> needs to be accessed only via the secure world (through 'scm'
> calls).
> 
> Also, the enable bit inside 'tcsr_check_reg' needs to be set
> first to set the eud in 'enable' mode on these SoCs.
> 
> Since this difference comes from how the firmware is configured, so
> the driver now relies on the presence of an extra boolean DT property
> to identify if secure access is needed.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>  drivers/usb/misc/Kconfig    |  1 +
>  drivers/usb/misc/qcom_eud.c | 74 +++++++++++++++++++++++++++++++++----
>  2 files changed, 68 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 99b15b77dfd5..fe1b5fec1dfc 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
>  config USB_QCOM_EUD
>  	tristate "QCOM Embedded USB Debugger(EUD) Driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
> +	select QCOM_SCM
>  	select USB_ROLE_SWITCH
>  	help
>  	  This module enables support for Qualcomm Technologies, Inc.
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index b7f13df00764..18a2dee3b4b9 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -5,12 +5,14 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/err.h>
> +#include <linux/firmware/qcom/qcom_scm.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/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -22,23 +24,35 @@
>  #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_REG_EUD_EN2		0x0000
>  
>  #define EUD_ENABLE		BIT(0)
> -#define EUD_INT_PET_EUD	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)
>  
> +#define EUD_EN2_SECURE_EN	BIT(0)
> +#define EUD_EN2_NONSECURE_EN	(1)
BIT(0) == 1, is that actually a separate register or does it just
reflect whether scm_writel is used?

If the latter, perhaps it'd be okay to just call it EUD_EN2_EN or
something along those lines? Isn't that perhaps what the docs call it?


> +#define EUD_EN2_DISABLE		(0)
> +#define TCSR_CHECK_EN		BIT(0)
> +
> +struct eud_soc_cfg {
> +	u32 tcsr_check_offset;
> +};
> +
>  struct eud_chip {
>  	struct device			*dev;
>  	struct usb_role_switch		*role_sw;
> +	const struct eud_soc_cfg	*eud_cfg;
>  	void __iomem			*base;
>  	void __iomem			*mode_mgr;
>  	unsigned int			int_status;
>  	int				irq;
>  	bool				enabled;
>  	bool				usb_attached;
> +	bool				secure_mode_enable;
Since it's only used in the probe function now, we can get rid
of it!

> +	phys_addr_t			secure_mode_mgr;
>  };
>  
>  static int enable_eud(struct eud_chip *priv)
> @@ -46,7 +60,11 @@ static int enable_eud(struct eud_chip *priv)
>  	writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
>  	writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
>  			priv->base + EUD_REG_INT1_EN_MASK);
> -	writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
> +
> +	if (priv->secure_mode_mgr)
> +		qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_SECURE_EN);
> +	else
> +		writel(EUD_EN2_NONSECURE_EN, priv->mode_mgr + EUD_REG_EUD_EN2);
>  
>  	return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
>  }
> @@ -54,7 +72,11 @@ static int enable_eud(struct eud_chip *priv)
>  static void disable_eud(struct eud_chip *priv)
>  {
>  	writel(0, priv->base + EUD_REG_CSR_EUD_EN);
> -	writel(0, priv->mode_mgr + EUD_REG_EUD_EN2);
> +
> +	if (priv->secure_mode_mgr)
> +		qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_DISABLE);
> +	else
> +		writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2);
>  }
>  
>  static ssize_t enable_show(struct device *dev,
> @@ -178,6 +200,8 @@ static void eud_role_switch_release(void *data)
>  static int eud_probe(struct platform_device *pdev)
>  {
>  	struct eud_chip *chip;
> +	struct resource *res;
> +	phys_addr_t tcsr_base, tcsr_check;
>  	int ret;
>  
>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> @@ -200,9 +224,40 @@ static int eud_probe(struct platform_device *pdev)
>  	if (IS_ERR(chip->base))
>  		return PTR_ERR(chip->base);
>  
> -	chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
> -	if (IS_ERR(chip->mode_mgr))
> -		return PTR_ERR(chip->mode_mgr);
> +	chip->secure_mode_enable = of_property_read_bool(chip->dev->of_node,
> +						"qcom,secure-mode-enable");
> +	/*
> +	 * EUD block on a few Qualcomm SoCs need secure register access.
> +	 * Check for the same.
> +	 */
> +	if (chip->secure_mode_enable) {
if (of_property_read_bool...)

> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +		if (!res)
> +			return dev_err_probe(chip->dev, -ENODEV,
> +					     "failed to get secure_mode_mgr reg base\n");
> +
> +		chip->secure_mode_mgr = res->start;
> +	} else {
> +		chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
> +		if (IS_ERR(chip->mode_mgr))
> +			return PTR_ERR(chip->mode_mgr);
> +	}
> +
> +	/* Check for any SoC specific config data */
> +	chip->eud_cfg = of_device_get_match_data(&pdev->dev);
> +	if (chip->eud_cfg) {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr-base");
> +		if (!res)
> +			return dev_err_probe(chip->dev, -ENODEV,
> +					     "failed to get tcsr reg base\n");
> +
> +		tcsr_base = res->start;
This variable does not seem very useful, we can get rid of it.

> +		tcsr_check = tcsr_base + chip->eud_cfg->tcsr_check_offset;
> +
> +		ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN);
> +		if (ret)
> +			return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n");
> +	}
>  
>  	chip->irq = platform_get_irq(pdev, 0);
>  	ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq,
> @@ -230,8 +285,13 @@ static int eud_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct eud_soc_cfg sm6115_eud_cfg = {
This could be marked __initconst, but I'm not sure if future
additions won't need to be accessed after the driver has already
gone through its probe function.. Your call!


Konrad
> +	.tcsr_check_offset = 0x25018,
> +};
> +
>  static const struct of_device_id eud_dt_match[] = {
>  	{ .compatible = "qcom,sc7280-eud" },
> +	{ .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_cfg },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, eud_dt_match);

Re: [PATCH v4 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

[Dmitry Baryshkov]

On Fri, 5 May 2023 at 14:05, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
>
>
> On 5.05.2023 08:40, Bhupesh Sharma wrote:
> > Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.
> >
> > On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
> > needs to be accessed only via the secure world (through 'scm'
> > calls).
> >
> > Also, the enable bit inside 'tcsr_check_reg' needs to be set
> > first to set the eud in 'enable' mode on these SoCs.
> >
> > Since this difference comes from how the firmware is configured, so
> > the driver now relies on the presence of an extra boolean DT property
> > to identify if secure access is needed.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > ---
> >  drivers/usb/misc/Kconfig    |  1 +
> >  drivers/usb/misc/qcom_eud.c | 74 +++++++++++++++++++++++++++++++++----
> >  2 files changed, 68 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > index 99b15b77dfd5..fe1b5fec1dfc 100644
> > --- a/drivers/usb/misc/Kconfig
> > +++ b/drivers/usb/misc/Kconfig
> > @@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
> >  config USB_QCOM_EUD
> >       tristate "QCOM Embedded USB Debugger(EUD) Driver"
> >       depends on ARCH_QCOM || COMPILE_TEST
> > +     select QCOM_SCM
> >       select USB_ROLE_SWITCH
> >       help
> >         This module enables support for Qualcomm Technologies, Inc.
> > diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> > index b7f13df00764..18a2dee3b4b9 100644
> > --- a/drivers/usb/misc/qcom_eud.c
> > +++ b/drivers/usb/misc/qcom_eud.c
> > @@ -5,12 +5,14 @@
> >
> >  #include <linux/bitops.h>
> >  #include <linux/err.h>
> > +#include <linux/firmware/qcom/qcom_scm.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/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> > @@ -22,23 +24,35 @@
> >  #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_REG_EUD_EN2              0x0000
> >
> >  #define EUD_ENABLE           BIT(0)
> > -#define EUD_INT_PET_EUD      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)
> >
> > +#define EUD_EN2_SECURE_EN    BIT(0)
> > +#define EUD_EN2_NONSECURE_EN (1)
> BIT(0) == 1, is that actually a separate register or does it just
> reflect whether scm_writel is used?
>
> If the latter, perhaps it'd be okay to just call it EUD_EN2_EN or
> something along those lines? Isn't that perhaps what the docs call it?
>
>
> > +#define EUD_EN2_DISABLE              (0)
> > +#define TCSR_CHECK_EN                BIT(0)
> > +
> > +struct eud_soc_cfg {
> > +     u32 tcsr_check_offset;
> > +};
> > +
> >  struct eud_chip {
> >       struct device                   *dev;
> >       struct usb_role_switch          *role_sw;
> > +     const struct eud_soc_cfg        *eud_cfg;
> >       void __iomem                    *base;
> >       void __iomem                    *mode_mgr;
> >       unsigned int                    int_status;
> >       int                             irq;
> >       bool                            enabled;
> >       bool                            usb_attached;
> > +     bool                            secure_mode_enable;
> Since it's only used in the probe function now, we can get rid
> of it!
>
> > +     phys_addr_t                     secure_mode_mgr;
> >  };
> >
> >  static int enable_eud(struct eud_chip *priv)
> > @@ -46,7 +60,11 @@ static int enable_eud(struct eud_chip *priv)
> >       writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
> >       writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
> >                       priv->base + EUD_REG_INT1_EN_MASK);
> > -     writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
> > +
> > +     if (priv->secure_mode_mgr)
> > +             qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_SECURE_EN);
> > +     else
> > +             writel(EUD_EN2_NONSECURE_EN, priv->mode_mgr + EUD_REG_EUD_EN2);
> >
> >       return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
> >  }
> > @@ -54,7 +72,11 @@ static int enable_eud(struct eud_chip *priv)
> >  static void disable_eud(struct eud_chip *priv)
> >  {
> >       writel(0, priv->base + EUD_REG_CSR_EUD_EN);
> > -     writel(0, priv->mode_mgr + EUD_REG_EUD_EN2);
> > +
> > +     if (priv->secure_mode_mgr)
> > +             qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_DISABLE);
> > +     else
> > +             writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2);
> >  }
> >
> >  static ssize_t enable_show(struct device *dev,
> > @@ -178,6 +200,8 @@ static void eud_role_switch_release(void *data)
> >  static int eud_probe(struct platform_device *pdev)
> >  {
> >       struct eud_chip *chip;
> > +     struct resource *res;
> > +     phys_addr_t tcsr_base, tcsr_check;
> >       int ret;
> >
> >       chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > @@ -200,9 +224,40 @@ static int eud_probe(struct platform_device *pdev)
> >       if (IS_ERR(chip->base))
> >               return PTR_ERR(chip->base);
> >
> > -     chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
> > -     if (IS_ERR(chip->mode_mgr))
> > -             return PTR_ERR(chip->mode_mgr);
> > +     chip->secure_mode_enable = of_property_read_bool(chip->dev->of_node,
> > +                                             "qcom,secure-mode-enable");
> > +     /*
> > +      * EUD block on a few Qualcomm SoCs need secure register access.
> > +      * Check for the same.
> > +      */
> > +     if (chip->secure_mode_enable) {
> if (of_property_read_bool...)
>
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +             if (!res)
> > +                     return dev_err_probe(chip->dev, -ENODEV,
> > +                                          "failed to get secure_mode_mgr reg base\n");
> > +
> > +             chip->secure_mode_mgr = res->start;
> > +     } else {
> > +             chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
> > +             if (IS_ERR(chip->mode_mgr))
> > +                     return PTR_ERR(chip->mode_mgr);
> > +     }
> > +
> > +     /* Check for any SoC specific config data */
> > +     chip->eud_cfg = of_device_get_match_data(&pdev->dev);
> > +     if (chip->eud_cfg) {
> > +             res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr-base");
> > +             if (!res)
> > +                     return dev_err_probe(chip->dev, -ENODEV,
> > +                                          "failed to get tcsr reg base\n");
> > +
> > +             tcsr_base = res->start;
> This variable does not seem very useful, we can get rid of it.
>
> > +             tcsr_check = tcsr_base + chip->eud_cfg->tcsr_check_offset;
> > +
> > +             ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN);
> > +             if (ret)
> > +                     return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n");
> > +     }
> >
> >       chip->irq = platform_get_irq(pdev, 0);
> >       ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq,
> > @@ -230,8 +285,13 @@ static int eud_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static const struct eud_soc_cfg sm6115_eud_cfg = {
> This could be marked __initconst, but I'm not sure if future
> additions won't need to be accessed after the driver has already
> gone through its probe function.. Your call!

Please no. The device can be probed later or reprobed, etc.
We had __devinit* for this kind of data/functions, but it was removed
around 3.x as the HOTPLUG also got removed.

>
>
> Konrad
> > +     .tcsr_check_offset = 0x25018,
> > +};
> > +
> >  static const struct of_device_id eud_dt_match[] = {
> >       { .compatible = "qcom,sc7280-eud" },
> > +     { .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_cfg },
> >       { }
> >  };
> >  MODULE_DEVICE_TABLE(of, eud_dt_match);



-- 
With best wishes
Dmitry

Re: [PATCH v4 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

[Bhupesh Sharma]

On Fri, 5 May 2023 at 16:35, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 5.05.2023 08:40, Bhupesh Sharma wrote:
> > Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.
> >
> > On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
> > needs to be accessed only via the secure world (through 'scm'
> > calls).
> >
> > Also, the enable bit inside 'tcsr_check_reg' needs to be set
> > first to set the eud in 'enable' mode on these SoCs.
> >
> > Since this difference comes from how the firmware is configured, so
> > the driver now relies on the presence of an extra boolean DT property
> > to identify if secure access is needed.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > ---
> >  drivers/usb/misc/Kconfig    |  1 +
> >  drivers/usb/misc/qcom_eud.c | 74 +++++++++++++++++++++++++++++++++----
> >  2 files changed, 68 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> > index 99b15b77dfd5..fe1b5fec1dfc 100644
> > --- a/drivers/usb/misc/Kconfig
> > +++ b/drivers/usb/misc/Kconfig
> > @@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
> >  config USB_QCOM_EUD
> >       tristate "QCOM Embedded USB Debugger(EUD) Driver"
> >       depends on ARCH_QCOM || COMPILE_TEST
> > +     select QCOM_SCM
> >       select USB_ROLE_SWITCH
> >       help
> >         This module enables support for Qualcomm Technologies, Inc.
> > diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> > index b7f13df00764..18a2dee3b4b9 100644
> > --- a/drivers/usb/misc/qcom_eud.c
> > +++ b/drivers/usb/misc/qcom_eud.c
> > @@ -5,12 +5,14 @@
> >
> >  #include <linux/bitops.h>
> >  #include <linux/err.h>
> > +#include <linux/firmware/qcom/qcom_scm.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/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/sysfs.h>
> > @@ -22,23 +24,35 @@
> >  #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_REG_EUD_EN2              0x0000
> >
> >  #define EUD_ENABLE           BIT(0)
> > -#define EUD_INT_PET_EUD      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)
> >
> > +#define EUD_EN2_SECURE_EN    BIT(0)
> > +#define EUD_EN2_NONSECURE_EN (1)
> BIT(0) == 1, is that actually a separate register or does it just
> reflect whether scm_writel is used?
>
> If the latter, perhaps it'd be okay to just call it EUD_EN2_EN or
> something along those lines? Isn't that perhaps what the docs call it?

Ok, let's name it as EUD_EN2_ENABLE then.

> > +#define EUD_EN2_DISABLE              (0)
> > +#define TCSR_CHECK_EN                BIT(0)
> > +
> > +struct eud_soc_cfg {
> > +     u32 tcsr_check_offset;
> > +};
> > +
> >  struct eud_chip {
> >       struct device                   *dev;
> >       struct usb_role_switch          *role_sw;
> > +     const struct eud_soc_cfg        *eud_cfg;
> >       void __iomem                    *base;
> >       void __iomem                    *mode_mgr;
> >       unsigned int                    int_status;
> >       int                             irq;
> >       bool                            enabled;
> >       bool                            usb_attached;
> > +     bool                            secure_mode_enable;
> Since it's only used in the probe function now, we can get rid
> of it!

Ok.

> > +     phys_addr_t                     secure_mode_mgr;
> >  };
> >
> >  static int enable_eud(struct eud_chip *priv)
> > @@ -46,7 +60,11 @@ static int enable_eud(struct eud_chip *priv)
> >       writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
> >       writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
> >                       priv->base + EUD_REG_INT1_EN_MASK);
> > -     writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
> > +
> > +     if (priv->secure_mode_mgr)
> > +             qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_SECURE_EN);
> > +     else
> > +             writel(EUD_EN2_NONSECURE_EN, priv->mode_mgr + EUD_REG_EUD_EN2);
> >
> >       return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
> >  }
> > @@ -54,7 +72,11 @@ static int enable_eud(struct eud_chip *priv)
> >  static void disable_eud(struct eud_chip *priv)
> >  {
> >       writel(0, priv->base + EUD_REG_CSR_EUD_EN);
> > -     writel(0, priv->mode_mgr + EUD_REG_EUD_EN2);
> > +
> > +     if (priv->secure_mode_mgr)
> > +             qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_DISABLE);
> > +     else
> > +             writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2);
> >  }
> >
> >  static ssize_t enable_show(struct device *dev,
> > @@ -178,6 +200,8 @@ static void eud_role_switch_release(void *data)
> >  static int eud_probe(struct platform_device *pdev)
> >  {
> >       struct eud_chip *chip;
> > +     struct resource *res;
> > +     phys_addr_t tcsr_base, tcsr_check;
> >       int ret;
> >
> >       chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > @@ -200,9 +224,40 @@ static int eud_probe(struct platform_device *pdev)
> >       if (IS_ERR(chip->base))
> >               return PTR_ERR(chip->base);
> >
> > -     chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
> > -     if (IS_ERR(chip->mode_mgr))
> > -             return PTR_ERR(chip->mode_mgr);
> > +     chip->secure_mode_enable = of_property_read_bool(chip->dev->of_node,
> > +                                             "qcom,secure-mode-enable");
> > +     /*
> > +      * EUD block on a few Qualcomm SoCs need secure register access.
> > +      * Check for the same.
> > +      */
> > +     if (chip->secure_mode_enable) {
> if (of_property_read_bool...)

Sure.

> > +             res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +             if (!res)
> > +                     return dev_err_probe(chip->dev, -ENODEV,
> > +                                          "failed to get secure_mode_mgr reg base\n");
> > +
> > +             chip->secure_mode_mgr = res->start;
> > +     } else {
> > +             chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
> > +             if (IS_ERR(chip->mode_mgr))
> > +                     return PTR_ERR(chip->mode_mgr);
> > +     }
> > +
> > +     /* Check for any SoC specific config data */
> > +     chip->eud_cfg = of_device_get_match_data(&pdev->dev);
> > +     if (chip->eud_cfg) {
> > +             res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr-base");
> > +             if (!res)
> > +                     return dev_err_probe(chip->dev, -ENODEV,
> > +                                          "failed to get tcsr reg base\n");
> > +
> > +             tcsr_base = res->start;
> This variable does not seem very useful, we can get rid of it.

Ok.

> > +             tcsr_check = tcsr_base + chip->eud_cfg->tcsr_check_offset;
> > +
> > +             ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN);
> > +             if (ret)
> > +                     return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n");
> > +     }
> >
> >       chip->irq = platform_get_irq(pdev, 0);
> >       ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq,
> > @@ -230,8 +285,13 @@ static int eud_remove(struct platform_device *pdev)
> >       return 0;
> >  }
> >
> > +static const struct eud_soc_cfg sm6115_eud_cfg = {
> This could be marked __initconst, but I'm not sure if future
> additions won't need to be accessed after the driver has already
> gone through its probe function.. Your call!

Like Dmitry also mentioned, I have my apprehensions as well marking this
as __initconst, so let's not do that.

I will wait for a few more comments and then will send a new version across.

Thanks,
Bhupesh

> > +     .tcsr_check_offset = 0x25018,
> > +};
> > +
> >  static const struct of_device_id eud_dt_match[] = {
> >       { .compatible = "qcom,sc7280-eud" },
> > +     { .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_cfg },
> >       { }
> >  };
> >  MODULE_DEVICE_TABLE(of, eud_dt_match);

Re: [PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Krzysztof Kozlowski]

On 05/05/2023 08:40, Bhupesh Sharma wrote:
> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.
> 
> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
> needs to be set first to 'enable' the eud module.
> 
> So, update the dt-bindings to accommodate the third register
> property (TCSR Base) required by the driver on these SoCs.
> 
> Also for these SoCs, introduce a new bool property
> 'qcom,secure-mode-enable', which indicates that the mode manager
> needs to be accessed only via the secure world.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml   | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> index f2c5ec7e6437..3b92cdf4e306 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> @@ -18,17 +18,33 @@ properties:
>      items:
>        - enum:
>            - qcom,sc7280-eud
> +          - qcom,sm6115-eud
>        - const: qcom,eud
>  
>    reg:
> +    minItems: 2
>      items:
>        - description: EUD Base Register Region
>        - description: EUD Mode Manager Register
> +      - description: TCSR Base Register Region
> +
> +  reg-names:
> +    minItems: 2
> +    items:
> +      - const: eud-base
> +      - const: eud-mode-mgr
> +      - const: tcsr-base
>  
>    interrupts:
>      description: EUD interrupt
>      maxItems: 1
>  
> +  qcom,secure-mode-enable:
> +    type: boolean
> +    description:
> +      Indicates that the mode manager needs to be accessed only via the secure
> +      world (through 'scm' calls).

I understood tcsr-base aplies only to SM6115, so this should be further
constrained in allOf:if:then:.

Best regards,
Krzysztof

Re: [PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Krzysztof Kozlowski]

On 05/05/2023 08:40, Bhupesh Sharma wrote:
> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.
> 
> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
> needs to be set first to 'enable' the eud module.
> 
> So, update the dt-bindings to accommodate the third register
> property (TCSR Base) required by the driver on these SoCs.
> 
> Also for these SoCs, introduce a new bool property
> 'qcom,secure-mode-enable', which indicates that the mode manager
> needs to be accessed only via the secure world.

Cannot it be implied by compatible?

Best regards,
Krzysztof

Re: [PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Bhupesh Sharma]

Hi Krzysztof,

On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/05/2023 08:40, Bhupesh Sharma wrote:
> > Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.
> >
> > On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
> > needs to be set first to 'enable' the eud module.
> >
> > So, update the dt-bindings to accommodate the third register
> > property (TCSR Base) required by the driver on these SoCs.
> >
> > Also for these SoCs, introduce a new bool property
> > 'qcom,secure-mode-enable', which indicates that the mode manager
> > needs to be accessed only via the secure world.
>
> Cannot it be implied by compatible?

I can see this will be used by future SoCs as well from the available
EUD documentation.

So let's keep a dedicated dt property as suggested by Bjorn in earlier
reviews, as otherwise the compatible checks would start getting bigger
/ messier in the driver code, in my opinion, when we add EUD support
for other SoCs + boards.

Regards,
Bhupesh

Re: [PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Bhupesh Sharma]

HI Krzysztof,

On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/05/2023 08:40, Bhupesh Sharma wrote:
> > Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.
> >
> > On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
> > needs to be set first to 'enable' the eud module.
> >
> > So, update the dt-bindings to accommodate the third register
> > property (TCSR Base) required by the driver on these SoCs.
> >
> > Also for these SoCs, introduce a new bool property
> > 'qcom,secure-mode-enable', which indicates that the mode manager
> > needs to be accessed only via the secure world.
> >
> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > ---
> >  .../devicetree/bindings/soc/qcom/qcom,eud.yaml   | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> > index f2c5ec7e6437..3b92cdf4e306 100644
> > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> > @@ -18,17 +18,33 @@ properties:
> >      items:
> >        - enum:
> >            - qcom,sc7280-eud
> > +          - qcom,sm6115-eud
> >        - const: qcom,eud
> >
> >    reg:
> > +    minItems: 2
> >      items:
> >        - description: EUD Base Register Region
> >        - description: EUD Mode Manager Register
> > +      - description: TCSR Base Register Region
> > +
> > +  reg-names:
> > +    minItems: 2
> > +    items:
> > +      - const: eud-base
> > +      - const: eud-mode-mgr
> > +      - const: tcsr-base
> >
> >    interrupts:
> >      description: EUD interrupt
> >      maxItems: 1
> >
> > +  qcom,secure-mode-enable:
> > +    type: boolean
> > +    description:
> > +      Indicates that the mode manager needs to be accessed only via the secure
> > +      world (through 'scm' calls).
>
> I understood tcsr-base aplies only to SM6115, so this should be further
> constrained in allOf:if:then:.

Please refer to my reply to your query in another review.
I can see that secure access to mode_mgr register via TCSR will be
exposed by other Qualcomm SoCs as well (from the available EUD
documentation). So, maybe keeping it generic instead of limiting it to
sm6115 only would be more useful, for future changes.

Thanks,
Bhupesh

Re: [PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Krzysztof Kozlowski]

On 05/05/2023 18:27, Bhupesh Sharma wrote:
> Hi Krzysztof,
> 
> On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 05/05/2023 08:40, Bhupesh Sharma wrote:
>>> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.
>>>
>>> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
>>> needs to be set first to 'enable' the eud module.
>>>
>>> So, update the dt-bindings to accommodate the third register
>>> property (TCSR Base) required by the driver on these SoCs.
>>>
>>> Also for these SoCs, introduce a new bool property
>>> 'qcom,secure-mode-enable', which indicates that the mode manager
>>> needs to be accessed only via the secure world.
>>
>> Cannot it be implied by compatible?
> 
> I can see this will be used by future SoCs as well from the available
> EUD documentation.
> 
> So let's keep a dedicated dt property as suggested by Bjorn in earlier
> reviews, as otherwise the compatible checks would start getting bigger
> / messier in the driver code, in my opinion, when we add EUD support
> for other SoCs + boards.

I don't understand the last part about compatible checks would grow. Why
would you have any compatible check in the driver? This looks standard
as we do with all SoC properties, so via driver data.

Best regards,
Krzysztof

Re: [PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Krzysztof Kozlowski]

On 05/05/2023 18:31, Bhupesh Sharma wrote:
> HI Krzysztof,
> 
> On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 05/05/2023 08:40, Bhupesh Sharma wrote:
>>> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.
>>>
>>> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
>>> needs to be set first to 'enable' the eud module.
>>>
>>> So, update the dt-bindings to accommodate the third register
>>> property (TCSR Base) required by the driver on these SoCs.
>>>
>>> Also for these SoCs, introduce a new bool property
>>> 'qcom,secure-mode-enable', which indicates that the mode manager
>>> needs to be accessed only via the secure world.
>>>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>> ---
>>>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml   | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> index f2c5ec7e6437..3b92cdf4e306 100644
>>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
>>> @@ -18,17 +18,33 @@ properties:
>>>      items:
>>>        - enum:
>>>            - qcom,sc7280-eud
>>> +          - qcom,sm6115-eud
>>>        - const: qcom,eud
>>>
>>>    reg:
>>> +    minItems: 2
>>>      items:
>>>        - description: EUD Base Register Region
>>>        - description: EUD Mode Manager Register
>>> +      - description: TCSR Base Register Region
>>> +
>>> +  reg-names:
>>> +    minItems: 2
>>> +    items:
>>> +      - const: eud-base
>>> +      - const: eud-mode-mgr
>>> +      - const: tcsr-base
>>>
>>>    interrupts:
>>>      description: EUD interrupt
>>>      maxItems: 1
>>>
>>> +  qcom,secure-mode-enable:
>>> +    type: boolean
>>> +    description:
>>> +      Indicates that the mode manager needs to be accessed only via the secure
>>> +      world (through 'scm' calls).
>>
>> I understood tcsr-base aplies only to SM6115, so this should be further
>> constrained in allOf:if:then:.
> 
> Please refer to my reply to your query in another review.
> I can see that secure access to mode_mgr register via TCSR will be
> exposed by other Qualcomm SoCs as well (from the available EUD
> documentation). So, maybe keeping it generic instead of limiting it to
> sm6115 only would be more useful, for future changes.

Your reply does not say that qcom,sc7280-eud uses it, so I don't
understand. This is not valid to qcom,sc7280-eud, so allowing it there
is not correct.

Best regards,
Krzysztof

Re: [PATCH v4 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

[Konrad Dybcio]

On 5.05.2023 17:50, Bhupesh Sharma wrote:
> On Fri, 5 May 2023 at 16:35, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 5.05.2023 08:40, Bhupesh Sharma wrote:
>>> Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.
>>>
>>> On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
>>> needs to be accessed only via the secure world (through 'scm'
>>> calls).
>>>
>>> Also, the enable bit inside 'tcsr_check_reg' needs to be set
>>> first to set the eud in 'enable' mode on these SoCs.
>>>
>>> Since this difference comes from how the firmware is configured, so
>>> the driver now relies on the presence of an extra boolean DT property
>>> to identify if secure access is needed.
>>>
>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>> ---
>>>  drivers/usb/misc/Kconfig    |  1 +
>>>  drivers/usb/misc/qcom_eud.c | 74 +++++++++++++++++++++++++++++++++----
>>>  2 files changed, 68 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
>>> index 99b15b77dfd5..fe1b5fec1dfc 100644
>>> --- a/drivers/usb/misc/Kconfig
>>> +++ b/drivers/usb/misc/Kconfig
>>> @@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
>>>  config USB_QCOM_EUD
>>>       tristate "QCOM Embedded USB Debugger(EUD) Driver"
>>>       depends on ARCH_QCOM || COMPILE_TEST
>>> +     select QCOM_SCM
>>>       select USB_ROLE_SWITCH
>>>       help
>>>         This module enables support for Qualcomm Technologies, Inc.
>>> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
>>> index b7f13df00764..18a2dee3b4b9 100644
>>> --- a/drivers/usb/misc/qcom_eud.c
>>> +++ b/drivers/usb/misc/qcom_eud.c
>>> @@ -5,12 +5,14 @@
>>>
>>>  #include <linux/bitops.h>
>>>  #include <linux/err.h>
>>> +#include <linux/firmware/qcom/qcom_scm.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/of_device.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/sysfs.h>
>>> @@ -22,23 +24,35 @@
>>>  #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_REG_EUD_EN2              0x0000
>>>
>>>  #define EUD_ENABLE           BIT(0)
>>> -#define EUD_INT_PET_EUD      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)
>>>
>>> +#define EUD_EN2_SECURE_EN    BIT(0)
>>> +#define EUD_EN2_NONSECURE_EN (1)
>> BIT(0) == 1, is that actually a separate register or does it just
>> reflect whether scm_writel is used?
>>
>> If the latter, perhaps it'd be okay to just call it EUD_EN2_EN or
>> something along those lines? Isn't that perhaps what the docs call it?
> 
> Ok, let's name it as EUD_EN2_ENABLE then.
> 
>>> +#define EUD_EN2_DISABLE              (0)
>>> +#define TCSR_CHECK_EN                BIT(0)
>>> +
>>> +struct eud_soc_cfg {
>>> +     u32 tcsr_check_offset;
>>> +};
>>> +
>>>  struct eud_chip {
>>>       struct device                   *dev;
>>>       struct usb_role_switch          *role_sw;
>>> +     const struct eud_soc_cfg        *eud_cfg;
>>>       void __iomem                    *base;
>>>       void __iomem                    *mode_mgr;
>>>       unsigned int                    int_status;
>>>       int                             irq;
>>>       bool                            enabled;
>>>       bool                            usb_attached;
>>> +     bool                            secure_mode_enable;
>> Since it's only used in the probe function now, we can get rid
>> of it!
> 
> Ok.
> 
>>> +     phys_addr_t                     secure_mode_mgr;
>>>  };
>>>
>>>  static int enable_eud(struct eud_chip *priv)
>>> @@ -46,7 +60,11 @@ static int enable_eud(struct eud_chip *priv)
>>>       writel(EUD_ENABLE, priv->base + EUD_REG_CSR_EUD_EN);
>>>       writel(EUD_INT_VBUS | EUD_INT_SAFE_MODE,
>>>                       priv->base + EUD_REG_INT1_EN_MASK);
>>> -     writel(1, priv->mode_mgr + EUD_REG_EUD_EN2);
>>> +
>>> +     if (priv->secure_mode_mgr)
>>> +             qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_SECURE_EN);
>>> +     else
>>> +             writel(EUD_EN2_NONSECURE_EN, priv->mode_mgr + EUD_REG_EUD_EN2);
>>>
>>>       return usb_role_switch_set_role(priv->role_sw, USB_ROLE_DEVICE);
>>>  }
>>> @@ -54,7 +72,11 @@ static int enable_eud(struct eud_chip *priv)
>>>  static void disable_eud(struct eud_chip *priv)
>>>  {
>>>       writel(0, priv->base + EUD_REG_CSR_EUD_EN);
>>> -     writel(0, priv->mode_mgr + EUD_REG_EUD_EN2);
>>> +
>>> +     if (priv->secure_mode_mgr)
>>> +             qcom_scm_io_writel(priv->secure_mode_mgr + EUD_REG_EUD_EN2, EUD_EN2_DISABLE);
>>> +     else
>>> +             writel(EUD_EN2_DISABLE, priv->mode_mgr + EUD_REG_EUD_EN2);
>>>  }
>>>
>>>  static ssize_t enable_show(struct device *dev,
>>> @@ -178,6 +200,8 @@ static void eud_role_switch_release(void *data)
>>>  static int eud_probe(struct platform_device *pdev)
>>>  {
>>>       struct eud_chip *chip;
>>> +     struct resource *res;
>>> +     phys_addr_t tcsr_base, tcsr_check;
>>>       int ret;
>>>
>>>       chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>> @@ -200,9 +224,40 @@ static int eud_probe(struct platform_device *pdev)
>>>       if (IS_ERR(chip->base))
>>>               return PTR_ERR(chip->base);
>>>
>>> -     chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
>>> -     if (IS_ERR(chip->mode_mgr))
>>> -             return PTR_ERR(chip->mode_mgr);
>>> +     chip->secure_mode_enable = of_property_read_bool(chip->dev->of_node,
>>> +                                             "qcom,secure-mode-enable");
>>> +     /*
>>> +      * EUD block on a few Qualcomm SoCs need secure register access.
>>> +      * Check for the same.
>>> +      */
>>> +     if (chip->secure_mode_enable) {
>> if (of_property_read_bool...)
> 
> Sure.
> 
>>> +             res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> +             if (!res)
>>> +                     return dev_err_probe(chip->dev, -ENODEV,
>>> +                                          "failed to get secure_mode_mgr reg base\n");
>>> +
>>> +             chip->secure_mode_mgr = res->start;
>>> +     } else {
>>> +             chip->mode_mgr = devm_platform_ioremap_resource(pdev, 1);
>>> +             if (IS_ERR(chip->mode_mgr))
>>> +                     return PTR_ERR(chip->mode_mgr);
>>> +     }
>>> +
>>> +     /* Check for any SoC specific config data */
>>> +     chip->eud_cfg = of_device_get_match_data(&pdev->dev);
>>> +     if (chip->eud_cfg) {
>>> +             res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tcsr-base");
>>> +             if (!res)
>>> +                     return dev_err_probe(chip->dev, -ENODEV,
>>> +                                          "failed to get tcsr reg base\n");
>>> +
>>> +             tcsr_base = res->start;
>> This variable does not seem very useful, we can get rid of it.
> 
> Ok.
> 
>>> +             tcsr_check = tcsr_base + chip->eud_cfg->tcsr_check_offset;
>>> +
>>> +             ret = qcom_scm_io_writel(tcsr_check, TCSR_CHECK_EN);
>>> +             if (ret)
>>> +                     return dev_err_probe(chip->dev, ret, "failed to write tcsr check reg\n");
>>> +     }
>>>
>>>       chip->irq = platform_get_irq(pdev, 0);
>>>       ret = devm_request_threaded_irq(&pdev->dev, chip->irq, handle_eud_irq,
>>> @@ -230,8 +285,13 @@ static int eud_remove(struct platform_device *pdev)
>>>       return 0;
>>>  }
>>>
>>> +static const struct eud_soc_cfg sm6115_eud_cfg = {
>> This could be marked __initconst, but I'm not sure if future
>> additions won't need to be accessed after the driver has already
>> gone through its probe function.. Your call!
> 
> Like Dmitry also mentioned, I have my apprehensions as well marking this
> as __initconst, so let's not do that.
Right, thanks Dmitry for pointing this out, I didn't think of usecases
where a driver can be removed..

Konrad
> 
> I will wait for a few more comments and then will send a new version across.
> 
> Thanks,
> Bhupesh
> 
>>> +     .tcsr_check_offset = 0x25018,
>>> +};
>>> +
>>>  static const struct of_device_id eud_dt_match[] = {
>>>       { .compatible = "qcom,sc7280-eud" },
>>> +     { .compatible = "qcom,sm6115-eud", .data = &sm6115_eud_cfg },
>>>       { }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, eud_dt_match);

Re: [PATCH v4 2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

[Bhupesh Sharma]

Hi Krzysztof,

On Fri, 5 May 2023 at 23:27, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/05/2023 18:31, Bhupesh Sharma wrote:
> > HI Krzysztof,
> >
> > On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 05/05/2023 08:40, Bhupesh Sharma wrote:
> >>> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC.
> >>>
> >>> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg'
> >>> needs to be set first to 'enable' the eud module.
> >>>
> >>> So, update the dt-bindings to accommodate the third register
> >>> property (TCSR Base) required by the driver on these SoCs.
> >>>
> >>> Also for these SoCs, introduce a new bool property
> >>> 'qcom,secure-mode-enable', which indicates that the mode manager
> >>> needs to be accessed only via the secure world.
> >>>
> >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> >>> ---
> >>>  .../devicetree/bindings/soc/qcom/qcom,eud.yaml   | 16 ++++++++++++++++
> >>>  1 file changed, 16 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> >>> index f2c5ec7e6437..3b92cdf4e306 100644
> >>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> >>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml
> >>> @@ -18,17 +18,33 @@ properties:
> >>>      items:
> >>>        - enum:
> >>>            - qcom,sc7280-eud
> >>> +          - qcom,sm6115-eud
> >>>        - const: qcom,eud
> >>>
> >>>    reg:
> >>> +    minItems: 2
> >>>      items:
> >>>        - description: EUD Base Register Region
> >>>        - description: EUD Mode Manager Register
> >>> +      - description: TCSR Base Register Region
> >>> +
> >>> +  reg-names:
> >>> +    minItems: 2
> >>> +    items:
> >>> +      - const: eud-base
> >>> +      - const: eud-mode-mgr
> >>> +      - const: tcsr-base
> >>>
> >>>    interrupts:
> >>>      description: EUD interrupt
> >>>      maxItems: 1
> >>>
> >>> +  qcom,secure-mode-enable:
> >>> +    type: boolean
> >>> +    description:
> >>> +      Indicates that the mode manager needs to be accessed only via the secure
> >>> +      world (through 'scm' calls).
> >>
> >> I understood tcsr-base aplies only to SM6115, so this should be further
> >> constrained in allOf:if:then:.
> >
> > Please refer to my reply to your query in another review.
> > I can see that secure access to mode_mgr register via TCSR will be
> > exposed by other Qualcomm SoCs as well (from the available EUD
> > documentation). So, maybe keeping it generic instead of limiting it to
> > sm6115 only would be more useful, for future changes.
>
> Your reply does not say that qcom,sc7280-eud uses it, so I don't
> understand. This is not valid to qcom,sc7280-eud, so allowing it there
> is not correct.

Ok, I got your concern now. I will send a v5 with the change shortly.

Thanks.

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

[Konrad Dybcio]

On 5.05.2023 08:40, Bhupesh Sharma wrote:
> Add the Embedded USB Debugger(EUD) device tree node for
> SM6115 / SM4250 SoC.
> 
> The node contains EUD base register region, EUD mode manager
> register region and TCSR Base register region along with the
> interrupt entry.
> 
> Also add the typec connector node for EUD which is attached to
> EUD node via port. EUD is also attached to DWC3 node via port.
> 
> To enable the role switch, we need to set dr_mode = "otg" property
> for 'usb_dwc3' sub-node in the board dts file.
> 
> Also the EUD device can be enabled on a board once linux is boot'ed
> by setting:
>  $ echo 1 > /sys/bus/platform/drivers/qcom_eud/../enable
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 51 ++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> index f67863561f3f..61a0af33ca43 100644
> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> @@ -180,6 +180,18 @@ core3 {
>  		};
>  	};
>  
> +	eud_typec: connector {
> +		compatible = "usb-c-connector";
> +
> +		ports {
> +			port@0 {
> +				con_eud: endpoint {
> +					remote-endpoint = <&eud_con>;
> +				};
> +			};
> +		};
> +	};
> +
>  	firmware {
>  		scm: scm {
>  			compatible = "qcom,scm-sm6115", "qcom,scm";
> @@ -647,6 +659,38 @@ gcc: clock-controller@1400000 {
>  			#power-domain-cells = <1>;
>  		};
>  
> +		eud: eud@1610000 {
> +			compatible = "qcom,sm6115-eud", "qcom,eud";
> +			reg = <0x0 0x01610000 0x0 0x2000>,
> +			      <0x0 0x01612000 0x0 0x1000>,
> +			      <0x0 0x003c0000 0x0 0x40000>;
> +			reg-names = "eud-base", "eud-mode-mgr", "tcsr-base";
> +			interrupts = <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>;
> +			qcom,secure-mode-enable;
> +			status = "disabled";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					eud_ep: endpoint {
> +						remote-endpoint = <&usb2_role_switch>;
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					eud_con: endpoint {
> +						remote-endpoint = <&con_eud>;
> +					};
> +				};
> +			};
> +		};
> +
>  		usb_hsphy: phy@1613000 {
>  			compatible = "qcom,sm6115-qusb2-phy";
>  			reg = <0x0 0x01613000 0x0 0x180>;
> @@ -1144,6 +1188,13 @@ usb_dwc3: usb@4e00000 {
>  				snps,has-lpm-erratum;
>  				snps,hird-threshold = /bits/ 8 <0x10>;
>  				snps,usb3_lpm_capable;
> +				usb-role-switch;
> +
> +				port {
> +					usb2_role_switch: endpoint {
> +						remote-endpoint = <&eud_ep>;
> +					};
> +				};
>  			};
>  		};
>  

Re: [PATCH v4 5/5] arm64: dts: qcom: qrb4210-rb2: Enable EUD debug peripheral

[Konrad Dybcio]

On 5.05.2023 08:40, Bhupesh Sharma wrote:
> Since the USB-C type port on the Qualcomm QRB4210-RB2 board
> can be set primarily in a 'device' configuration (with the default
> DIP switch settings), it makes sense to enable the EUD debug
> peripheral on the board by default by setting the USB 'dr_mode' property
> as 'otg'.
> 
> Now, the EUD debug peripheral can be enabled by executing:
>  $ echo 1 > /sys/bus/platform/drivers/qcom_eud/1610000.eud/enable
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

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