[PATCH v1 0/2] usb: tegra: separate schema and adjust OTG logic

[PATCH v1 0/2] usb: tegra: separate schema and adjust OTG logic

[Svyatoslav Ryhel]

Separate Tegra specific USB controller props into a dedicated schema.
Adjust USB OTG mode behavior with extcon use.

Jordan Viknar (1):
  usb: phy: tegra: Disable internal USB sensing if extcon is used

Svyatoslav Ryhel (1):
  dt-bindings: usb: Move Tegra-specific Chipidea USB properties into a
    dedicated schema

 .../bindings/usb/chipidea,usb2-tegra.yaml     | 88 +++++++++++++++++++
 .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 22 -----
 drivers/usb/phy/phy-tegra-usb.c               | 55 ++++++++++++
 include/linux/usb/tegra_usb_phy.h             |  1 +
 4 files changed, 144 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/chipidea,usb2-tegra.yaml

-- 
2.53.0

[PATCH v1 1/2] dt-bindings: usb: Move Tegra-specific Chipidea USB properties into a dedicated schema

[Svyatoslav Ryhel]

Move all Tegra-specific Chipidea devices and their properties into a
dedicated schema file, by analogy with i.MX.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/usb/chipidea,usb2-tegra.yaml     | 88 +++++++++++++++++++
 .../devicetree/bindings/usb/ci-hdrc-usb2.yaml | 22 -----
 2 files changed, 88 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/chipidea,usb2-tegra.yaml

diff --git a/Documentation/devicetree/bindings/usb/chipidea,usb2-tegra.yaml b/Documentation/devicetree/bindings/usb/chipidea,usb2-tegra.yaml
new file mode 100644
index 0000000000000..78046f8a63ad5
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/chipidea,usb2-tegra.yaml
@@ -0,0 +1,88 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/chipidea,usb2-tegra.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tegra USB2 ChipIdea USB controller
+
+maintainers:
+  - Svyatoslav Ryhel <clamor95@gmail.com>
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - nvidia,tegra20-ehci
+          - nvidia,tegra20-udc
+          - nvidia,tegra30-ehci
+          - nvidia,tegra30-udc
+          - nvidia,tegra114-udc
+          - nvidia,tegra124-udc
+      - items:
+          - enum:
+              - nvidia,tegra114-ehci
+              - nvidia,tegra124-ehci
+              - nvidia,tegra210-ehci
+          - const: nvidia,tegra30-ehci
+
+  operating-points-v2:
+    description: A phandle to the OPP table containing the performance states.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
+  nvidia,phy:
+    description: phandle of usb phy that connects to the port. Use "phys" instead.
+    $ref: /schemas/types.yaml#/definitions/phandle
+    deprecated: true
+
+  nvidia,needs-double-reset:
+    description: Indicates double reset or not.
+    type: boolean
+    deprecated: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - resets
+
+allOf:
+  - $ref: chipidea,usb2-common.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/tegra30-car.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    usb@7d000000 {
+        compatible = "nvidia,tegra30-udc";
+        reg = <0x7d000000 0x4000>;
+        interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>;
+        phy_type = "utmi";
+        clocks = <&tegra_car TEGRA30_CLK_USBD>;
+        resets = <&tegra_car 22>;
+        reset-names = "usb";
+        phys = <&phy1>;
+        power-domains = <&pd_core>;
+        operating-points-v2 = <&usbd_dvfs_opp_table>;
+
+        dr_mode = "otg";
+
+        hnp-disable;
+        srp-disable;
+        adp-disable;
+
+        usb-role-switch;
+        extcon = <&charger>, <&extcon>; /* vbus, id */
+        vbus-supply = <&usb_otg_vbus>;
+
+        port {
+            usb_in: endpoint {
+                remote-endpoint = <&connector_out>;
+            };
+        };
+    };
+...
diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
index 620b564914d43..b73bea61dcac0 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
@@ -17,20 +17,8 @@ properties:
           - chipidea,usb2
           - lsi,zevio-usb
           - nuvoton,npcm750-udc
-          - nvidia,tegra20-ehci
-          - nvidia,tegra20-udc
-          - nvidia,tegra30-ehci
-          - nvidia,tegra30-udc
-          - nvidia,tegra114-udc
-          - nvidia,tegra124-udc
           - nxp,s32g2-usb
           - qcom,ci-hdrc
-      - items:
-          - enum:
-              - nvidia,tegra114-ehci
-              - nvidia,tegra124-ehci
-              - nvidia,tegra210-ehci
-          - const: nvidia,tegra30-ehci
       - items:
           - const: xlnx,zynq-usb-2.20a
           - const: chipidea,usb2
@@ -66,16 +54,6 @@ properties:
           - description: register offset
           - description: phy index
 
-  nvidia,phy:
-    description: phandle of usb phy that connects to the port. Use "phys" instead.
-    $ref: /schemas/types.yaml#/definitions/phandle
-    deprecated: true
-
-  nvidia,needs-double-reset:
-    description: Indicates double reset or not.
-    type: boolean
-    deprecated: true
-
   ulpi:
     type: object
     additionalProperties: false
-- 
2.53.0

[PATCH v1 2/2] usb: phy: tegra: Disable internal USB sensing if extcon is used

[Svyatoslav Ryhel]

From: Jordan Viknar <jordanviknar@gmail.com>

The Tegra USB PHY features four VBUS sensors: A_VBUS_VLD, A_SESS_VLD,
B_SESS_VLD, and B_SESS_END. This allows the SoC to detect mode switches
and natively work with OTG. On some devices, instead of relying on Tegra's
own mechanism, some type of extcon detection chip may be used, which
conflicts with the native Tegra implementation.

Add extcon parsing from the linked USB controller. If such a property
exists, switch VBUS sensors to software mode and set their values to SET.
This indicates that the VBUS sensors are not required by the USB
controller, which uses extcon for detection instead.

This fixes all devices that have OTG mode set with extcon for Tegra20
(LG Optimus 2X P990), Tegra30 (LG Optimus 4X P880 and Vu P895),
Tegra114 (Nvidia Tegra Note 7), and Tegra124 (Xiaomi Mi Pad). All listed
devices were tested.

Assisted-by: Claude Sonnet 5
Signed-off-by: Jordan Viknar <jordanviknar@gmail.com>
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/usb/phy/phy-tegra-usb.c   | 55 +++++++++++++++++++++++++++++++
 include/linux/usb/tegra_usb_phy.h |  1 +
 2 files changed, 56 insertions(+)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 00443a7beaeb0..d3faeb6619454 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -16,6 +16,7 @@
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/resource.h>
@@ -68,9 +69,18 @@
 #define   UHSIC_PHY_ENABLE			BIT(19)
 
 #define USB_PHY_VBUS_SENSORS			0x404
+#define   B_SESS_VLD_SW_EN			BIT(11)
+#define   B_SESS_VLD_SW_VALUE			BIT(12)
 #define   B_SESS_VLD_WAKEUP_EN			BIT(14)
+#define   A_SESS_VLD_SW_EN			BIT(19)
+#define   A_SESS_VLD_SW_VALUE			BIT(20)
 #define   A_SESS_VLD_WAKEUP_EN			BIT(22)
+#define   A_VBUS_VLD_SW_EN			BIT(27)
+#define   A_VBUS_VLD_SW_VALUE			BIT(28)
 #define   A_VBUS_VLD_WAKEUP_EN			BIT(30)
+#define VBUS_SENSORS_SW_OVERRIDE		(B_SESS_VLD_SW_EN | B_SESS_VLD_SW_VALUE | \
+						 A_SESS_VLD_SW_EN | A_SESS_VLD_SW_VALUE | \
+						 A_VBUS_VLD_SW_EN | A_VBUS_VLD_SW_VALUE)
 
 #define USB_PHY_VBUS_WAKEUP_ID			0x408
 #define   ID_INT_EN				BIT(0)
@@ -621,6 +631,10 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 		val = readl_relaxed(base + USB_PHY_VBUS_SENSORS);
 		val &= ~(A_VBUS_VLD_WAKEUP_EN | A_SESS_VLD_WAKEUP_EN);
 		val &= ~(B_SESS_VLD_WAKEUP_EN);
+
+		if (phy->has_extcon)
+			val |= VBUS_SENSORS_SW_OVERRIDE;
+
 		writel_relaxed(val, base + USB_PHY_VBUS_SENSORS);
 
 		val = readl_relaxed(base + UTMIP_BAT_CHRG_CFG0);
@@ -727,6 +741,12 @@ static int utmi_phy_power_off(struct tegra_usb_phy *phy)
 					   val, !(val & VBUS_WAKEUP_STS),
 					   5000, 100000);
 
+	if (phy->has_extcon) {
+		val = readl_relaxed(base + USB_PHY_VBUS_SENSORS);
+		val &= ~VBUS_SENSORS_SW_OVERRIDE;
+		writel_relaxed(val, base + USB_PHY_VBUS_SENSORS);
+	}
+
 	utmi_phy_clk_disable(phy);
 
 	/* PHY won't resume if reset is asserted */
@@ -1456,6 +1476,34 @@ static int tegra_usb_phy_parse_pmc(struct device *dev,
 	return 0;
 }
 
+static int tegra_usb_phy_parse_controller(struct device *dev,
+					  struct tegra_usb_phy *phy)
+{
+	struct device_node *usb_node;
+	char *node_path;
+	u64 addr;
+	int ret;
+
+	ret = of_property_read_reg(dev->of_node, 0, &addr, NULL);
+	if (ret) {
+		dev_err(dev, "Failed to read reg entry 0: %d\n", ret);
+		return ret;
+	}
+
+	node_path = devm_kasprintf(dev, GFP_KERNEL, "/usb@%08llx", addr);
+	if (!node_path)
+		return -ENOMEM;
+
+	usb_node = of_find_node_by_path(node_path);
+	if (!usb_node)
+		return -ENODEV;
+
+	phy->has_extcon = of_property_present(usb_node, "extcon");
+	of_node_put(usb_node);
+
+	return 0;
+}
+
 static const struct tegra_phy_soc_config tegra20_soc_config = {
 	.utmi_pll_config_in_car_module = false,
 	.has_hostpc = false,
@@ -1552,6 +1600,13 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	if (tegra_phy->mode == USB_DR_MODE_OTG) {
+		err = tegra_usb_phy_parse_controller(&pdev->dev, tegra_phy);
+		if (err)
+			return dev_err_probe(&pdev->dev, err,
+					     "Failed to get controller\n");
+	}
+
 	tegra_phy->phy_type = of_usb_get_phy_mode(np);
 	switch (tegra_phy->phy_type) {
 	case USBPHY_INTERFACE_MODE_UTMI:
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index 6343f88df5de8..4bb20e39003ab 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -87,6 +87,7 @@ struct tegra_usb_phy {
 	bool wakeup_enabled;
 	bool pad_wakeup;
 	bool powered_on;
+	bool has_extcon;
 };
 
 #endif /* __TEGRA_USB_PHY_H */
-- 
2.53.0

Re: [PATCH v1 2/2] usb: phy: tegra: Disable internal USB sensing if extcon is used

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] usb: phy: tegra: spurious wakeups due to re-enabled floating VBUS sensor
- [High] usb: phy: tegra: unhandled extcon parsing for peripheral-only mode
- [Medium] usb: phy: tegra: brittle DT parsing via hardcoded absolute path
- [Medium] usb: phy: tegra: unconditional 100ms timeout delay on power-off due to active SW override
- [Low] usb: phy: tegra: unnecessary devm allocation for temporary string
--

commit 4cd162cd45681e0cd98b5676b09fbcbe3368c0d1
Author: Jordan Viknar <jordanviknar@gmail.com>

usb: phy: tegra: Disable internal USB sensing if extcon is used

This commit adds extcon parsing from the linked USB controller. If an extcon
property exists, it switches VBUS sensors to software mode and sets their
values to SET, allowing boards with extcon chips to function properly without
conflicting with Tegra's native mechanism.

> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index 00443a7beaeb0..b5660910dfc7b 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c

[ ... ]

> @@ -727,6 +741,12 @@ static int utmi_phy_power_off(struct tegra_usb_phy *phy)
>  					   val, !(val & VBUS_WAKEUP_STS),
>  					   5000, 100000);
>  

[Severity: Medium]
Does this code introduce an unconditional 100ms delay during power-off?

Since readl_relaxed_poll_timeout() executes before VBUS_SENSORS_SW_OVERRIDE
is disabled in the code below, the software override forces VBUS to 1. Will
this cause the status bit to never clear and the loop to hit the 100ms
timeout limit on extcon-enabled boards?

> +	if (phy->has_extcon) {
> +		val = readl_relaxed(base + USB_PHY_VBUS_SENSORS);
> +		val &= ~VBUS_SENSORS_SW_OVERRIDE;
> +		writel_relaxed(val, base + USB_PHY_VBUS_SENSORS);
> +	}

[Severity: High]
Can this clearing of the software override cause spurious system wakeups?

By clearing VBUS_SENSORS_SW_OVERRIDE here, the disconnected hardware sensors
are exposed. Later in utmi_phy_power_off(), if wakeup_enabled is true, the
code sets A_VBUS_VLD_WAKEUP_EN:

    val = readl_relaxed(base + USB_PHY_VBUS_SENSORS);
    val |= A_VBUS_VLD_WAKEUP_EN;
    writel_relaxed(val, base + USB_PHY_VBUS_SENSORS);

Because the physical pin is disconnected or floating on extcon-based designs,
will re-enabling this hardware sensor trigger spurious wakeups during system
suspend?

> +
>  	utmi_phy_clk_disable(phy);
>  
>  	/* PHY won't resume if reset is asserted */

[ ... ]

> @@ -1456,6 +1476,34 @@ static int tegra_usb_phy_parse_pmc(struct device *dev,
>  	return 0;
>  }
>  
> +static int tegra_usb_phy_parse_controller(struct device *dev,
> +					  struct tegra_usb_phy *phy)
> +{
> +	struct device_node *usb_node;
> +	char *node_path;
> +	u64 addr;
> +	int ret;
> +
> +	ret = of_property_read_reg(dev->of_node, 0, &addr, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to read reg entry 0: %d\n", ret);
> +		return ret;
> +	}
> +
> +	node_path = devm_kasprintf(dev, GFP_KERNEL, "/usb@%08llx", addr);

[Severity: Low]
Is it necessary to use devm_kasprintf() for this temporary string?

Since node_path is only used once for of_find_node_by_path(), allocating it
with devm_kasprintf() binds the string to the device lifetime, unnecessarily
keeping it in heap memory until the device is unbound.

[Severity: Medium]
Will this hardcoded absolute path break if the device tree topology changes?

Using a hardcoded format string "/usb@%08llx" requires the controller node
to be named exactly "usb", placed at the root of the device tree, and its
address zero-padded to exactly 8 characters. If the node is moved under
"/soc/" or "/amba/", or if the address requires fewer than 8 hex digits
(as leading zeroes are prohibited for smaller addresses by the DT spec),
will this fail to find the node and break USB functionality?

> +	if (!node_path)
> +		return -ENOMEM;
> +
> +	usb_node = of_find_node_by_path(node_path);
> +	if (!usb_node)
> +		return -ENODEV;
> +
> +	phy->has_extcon = of_property_present(usb_node, "extcon");
> +	of_node_put(usb_node);
> +
> +	return 0;
> +}

[ ... ]

> @@ -1552,6 +1600,13 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> +	if (tegra_phy->mode == USB_DR_MODE_OTG) {
> +		err = tegra_usb_phy_parse_controller(&pdev->dev, tegra_phy);
> +		if (err)
> +			return dev_err_probe(&pdev->dev, err,
> +					     "Failed to get controller\n");
> +	}

[Severity: High]
Does this prevent extcon parsing for USB_DR_MODE_PERIPHERAL?

If dr_mode is "peripheral", the parser never runs and has_extcon remains
false. Since the software override (VBUS_SENSORS_SW_OVERRIDE) is not applied
during utmi_phy_power_on() without has_extcon, will peripheral-only
extcon-based designs fail to detect connections because they read the
disconnected hardware sensor instead?

> +
>  	tegra_phy->phy_type = of_usb_get_phy_mode(np);
>  	switch (tegra_phy->phy_type) {
>  	case USBPHY_INTERFACE_MODE_UTMI:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004165204.205660-1-clamor95@gmail.com?part=2