[PATCH 0/2] phy: qcom: usb-hs: add qcom,vendor-init-seq for raw ULPI writes

[PATCH 0/2] phy: qcom: usb-hs: add qcom,vendor-init-seq for raw ULPI writes

[Herman van Hazendonk]

The qcom,usb-hs-phy driver already supports qcom,init-seq for
board-specific ULPI initialisation, but the address field in that
property is always offset by ULPI_EXT_VENDOR_SPECIFIC.  That makes
it impossible to reach the standard ULPI vendor register range
(0x30-0x3f) where MSM8x60-class hardware keeps pre-emphasis, HS
driver slope, and CDR auto-reset bits.

PATCH 1/2 adds the DT binding: an optional "qcom,vendor-init-seq"
property carrying raw (addr, val) u8 pairs with no implicit offset.

PATCH 2/2 adds the driver support.  The new sequence is applied AFTER
reset_control_reset() so the values survive the register restore the
reset performs.  While refactoring the shared parse logic into a
single helper, two pre-existing bugs in the qcom,init-seq path are
also fixed: an odd byte count would previously silently drop the
trailing half-pair, and there was no upper bound on the
devm_kmalloc_array() allocation driven by the DT value.

Herman van Hazendonk (2):
  dt-bindings: phy: qcom,usb-hs-phy: add qcom,vendor-init-seq
  phy: qcom: usb-hs: honour qcom,vendor-init-seq raw ULPI writes

 .../bindings/phy/qcom,usb-hs-phy.yaml         |  14 +++
 drivers/phy/qualcomm/phy-qcom-usb-hs.c        | 109 +++++++++++++++---
 2 files changed, 104 insertions(+), 19 deletions(-)


base-commit: 944125b4c454b58d2fe6e35f1087a932b2050dff
-- 
2.43.0

[PATCH 1/2] dt-bindings: phy: qcom,usb-hs-phy: add qcom,vendor-init-seq

[Herman van Hazendonk]

Add an optional "qcom,vendor-init-seq" property carrying raw ULPI
(address, value) pairs that are written after PHY reset.

Unlike the existing "qcom,init-seq" property, the address field is
NOT offset by ULPI_EXT_VENDOR_SPECIFIC, so the new property can
reach the standard ULPI vendor register range (0x30-0x3f). MSM8x60-
class hardware needs this range to programme pre-emphasis, HS driver
slope and CDR auto-reset bits the legacy msm_otg driver used to set
via platform data.

The "qcom,init-seq" path is left unchanged.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 .../devicetree/bindings/phy/qcom,usb-hs-phy.yaml   | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
index e03b516c698c..b9eca670419a 100644
--- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
@@ -85,6 +85,20 @@ properties:
             the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address
         - description: value
 
+  qcom,vendor-init-seq:
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    description: >
+      Flat sequence of raw ULPI address/value byte pairs written after
+      the PHY reset. Each pair is two consecutive bytes:
+      [addr0, val0, addr1, val1, ...]. Total length must be even and
+      no more than 64 bytes (32 pairs). Unlike qcom,init-seq the
+      address field is not offset by ULPI_EXT_VENDOR_SPECIFIC, so this
+      property can reach the standard ULPI vendor range (0x30-0x3f) —
+      used on MSM8x60-class hardware to program pre-emphasis, HS
+      driver slope and CDR auto-reset bits the legacy msm_otg driver
+      used to set via platform data.
+    maxItems: 64
+
 required:
   - clocks
   - clock-names
-- 
2.43.0

Re: [PATCH 1/2] dt-bindings: phy: qcom,usb-hs-phy: add qcom,vendor-init-seq

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `qcom,vendor-init-seq` property uses a flat `uint8-array` type instead of `uint8-matrix`, preventing dt-schema from enforcing its required address-value pair structure.
--

commit c31501f8d171d100ea82c554966b30fc82a8bc2c
Author: Herman van Hazendonk <github.com@herrie.org>

dt-bindings: phy: qcom,usb-hs-phy: add qcom,vendor-init-seq

This commit introduces the qcom,vendor-init-seq property to the Qualcomm
USB HS PHY binding. It allows passing raw ULPI (address, value) pairs to
be written after PHY reset, targeting the standard ULPI vendor register
range for MSM8x60-class hardware configuration.

> diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> index e03b516c698cc..b9eca670419a8 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-phy.yaml
> @@ -85,6 +85,20 @@ properties:
>              the address is offset from the ULPI_EXT_VENDOR_SPECIFIC address
>          - description: value
>  
> +  qcom,vendor-init-seq:
> +    $ref: /schemas/types.yaml#/definitions/uint8-array

[Severity: Low]
Since the sequence consists of (address, value) pairs, should this use
/schemas/types.yaml#/definitions/uint8-matrix instead of uint8-array?

Using a uint8-matrix with an inner items list would allow the dt-schema
validation tool to statically catch malformed properties that do not have
an even number of bytes. This would structurally enforce the two-byte tuple
layout before runtime, similar to the existing qcom,init-seq property.

> +    description: >
> +      Flat sequence of raw ULPI address/value byte pairs written after
> +      the PHY reset. Each pair is two consecutive bytes:
> +      [addr0, val0, addr1, val1, ...]. Total length must be even and
> +      no more than 64 bytes (32 pairs). Unlike qcom,init-seq the
> +      address field is not offset by ULPI_EXT_VENDOR_SPECIFIC, so this
> +      property can reach the standard ULPI vendor range (0x30-0x3f) —
> +      used on MSM8x60-class hardware to program pre-emphasis, HS
> +      driver slope and CDR auto-reset bits the legacy msm_otg driver
> +      used to set via platform data.
> +    maxItems: 64
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603054809.565723-1-github.com@herrie.org?part=1

[PATCH 2/2] phy: qcom: usb-hs: honour qcom,vendor-init-seq raw ULPI writes

[Herman van Hazendonk]

Add support for the optional qcom,vendor-init-seq DT property: a
list of u8 (addr, val) pairs written verbatim to raw ULPI register
addresses, rather than to ULPI_EXT_VENDOR_SPECIFIC + addr like the
existing qcom,init-seq sequence reaches. This lets boards reach the
standard vendor register range 0x30-0x3F where MSM8x60-era hardware
keeps pre-emphasis level / HS driver slope / CDR auto-reset, etc.

The new sequence is applied AFTER reset_control_reset() in
qcom_usb_hs_phy_power_on() so the values survive the register
restore the reset performs. The pre-existing qcom,init-seq is left
applied BEFORE the reset to preserve current behaviour for the
APQ8064/MSM8916 boards that already depend on it; an earlier review flagged
this ordering as a likely pre-existing bug, but blindly reordering
established behaviour for those boards is out of scope for this
patch and would risk regressing them.

While at it, harden the parse logic shared between qcom,init-seq
and qcom,vendor-init-seq:

  - Reject an odd byte count up-front rather than silently dropping
    the trailing byte and producing a half-pair the rest of the
    driver cannot describe.
  - Reject more than 32 bytes (the binding's maxItems) rather than
    making an unbounded devm_kmalloc_array() allocation driven by
    an untrusted-shaped DT property.

Factor the duplicated parse-and-allocate sequence into
qcom_usb_hs_phy_parse_init_seq() so both properties get the same
validation. The validation also fixes the pre-existing same bugs in
the qcom,init-seq parse path uniformly with the new property.

Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
---
 drivers/phy/qualcomm/phy-qcom-usb-hs.c | 109 ++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 19 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
index 98a18987f1be..89fbe8f8d001 100644
--- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c
+++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
@@ -35,6 +35,13 @@ struct qcom_usb_hs_phy {
 	struct regulator *v3p3;
 	struct reset_control *reset;
 	struct ulpi_seq *init_seq;
+	/*
+	 * Optional sequence of writes targeting raw ULPI addresses (no
+	 * ULPI_EXT_VENDOR_SPECIFIC base added). Populated from the
+	 * "qcom,vendor-init-seq" DT property. Applied after PHY reset
+	 * so the values survive the reset that follows init_seq writes.
+	 */
+	struct ulpi_seq *vendor_init_seq;
 	struct extcon_dev *vbus_edev;
 	struct notifier_block vbus_notify;
 };
@@ -154,6 +161,19 @@ static int qcom_usb_hs_phy_power_on(struct phy *phy)
 			goto err_ulpi;
 	}
 
+	/*
+	 * Apply board-specific raw-address ULPI writes after the PHY reset
+	 * so they survive register restore. Used to reach the standard
+	 * vendor range 0x30-0x3F which qcom,init-seq (above) cannot —
+	 * pre-emphasis level / HS driver slope / CDR auto-reset etc. live
+	 * there on MSM8660-class hardware.
+	 */
+	for (seq = uphy->vendor_init_seq; seq->addr; seq++) {
+		ret = ulpi_write(ulpi, seq->addr, seq->val);
+		if (ret)
+			goto err_ulpi;
+	}
+
 	if (uphy->vbus_edev) {
 		state = extcon_get_state(uphy->vbus_edev, EXTCON_USB);
 		/* setup initial state */
@@ -199,6 +219,59 @@ static const struct phy_ops qcom_usb_hs_phy_ops = {
 	.owner = THIS_MODULE,
 };
 
+/*
+ * The binding caps both qcom,init-seq and qcom,vendor-init-seq at
+ * maxItems: 32 (addr, val) pairs, i.e. 64 bytes total. Enforce that
+ * limit here so a malformed DT cannot drive an unbounded
+ * devm_kmalloc_array() and so the misconfiguration is visible at
+ * probe time instead of silently truncated.
+ */
+#define QCOM_USB_HS_PHY_INIT_SEQ_MAX_PAIRS	32
+#define QCOM_USB_HS_PHY_INIT_SEQ_MAX_BYTES	\
+	(QCOM_USB_HS_PHY_INIT_SEQ_MAX_PAIRS * 2)
+
+static int qcom_usb_hs_phy_parse_init_seq(struct ulpi *ulpi,
+					  const char *propname,
+					  struct ulpi_seq **out)
+{
+	struct ulpi_seq *seq;
+	int size;
+
+	size = of_property_count_u8_elems(ulpi->dev.of_node, propname);
+	if (size < 0)
+		size = 0;
+	if (size > QCOM_USB_HS_PHY_INIT_SEQ_MAX_BYTES) {
+		dev_err(&ulpi->dev,
+			"%s: %d bytes exceeds %d-byte maximum\n",
+			propname, size, QCOM_USB_HS_PHY_INIT_SEQ_MAX_BYTES);
+		return -EINVAL;
+	}
+	if (size % 2) {
+		dev_err(&ulpi->dev,
+			"%s: %d bytes is not a whole number of (addr, val) pairs\n",
+			propname, size);
+		return -EINVAL;
+	}
+
+	seq = devm_kmalloc_array(&ulpi->dev, (size / 2) + 1, sizeof(*seq),
+				 GFP_KERNEL);
+	if (!seq)
+		return -ENOMEM;
+
+	if (size) {
+		int ret = of_property_read_u8_array(ulpi->dev.of_node,
+						    propname, (u8 *)seq, size);
+		if (ret)
+			return ret;
+	}
+	/* NUL-terminate so the power_on loop's seq->addr-as-sentinel works. */
+	seq[size / 2].addr = 0;
+	seq[size / 2].val = 0;
+
+	*out = seq;
+	return 0;
+}
+
 static int qcom_usb_hs_phy_probe(struct ulpi *ulpi)
 {
 	struct qcom_usb_hs_phy *uphy;
@@ -206,7 +279,6 @@ static int qcom_usb_hs_phy_probe(struct ulpi *ulpi)
 	struct clk *clk;
 	struct regulator *reg;
 	struct reset_control *reset;
-	int size;
 	int ret;
 
 	uphy = devm_kzalloc(&ulpi->dev, sizeof(*uphy), GFP_KERNEL);
@@ -215,19 +287,20 @@ static int qcom_usb_hs_phy_probe(struct ulpi *ulpi)
 	ulpi_set_drvdata(ulpi, uphy);
 	uphy->ulpi = ulpi;
 
-	size = of_property_count_u8_elems(ulpi->dev.of_node, "qcom,init-seq");
-	if (size < 0)
-		size = 0;
-	uphy->init_seq = devm_kmalloc_array(&ulpi->dev, (size / 2) + 1,
-					   sizeof(*uphy->init_seq), GFP_KERNEL);
-	if (!uphy->init_seq)
-		return -ENOMEM;
-	ret = of_property_read_u8_array(ulpi->dev.of_node, "qcom,init-seq",
-					(u8 *)uphy->init_seq, size);
-	if (ret && size)
+	ret = qcom_usb_hs_phy_parse_init_seq(ulpi, "qcom,init-seq",
+					     &uphy->init_seq);
+	if (ret)
+		return ret;
+	/*
+	 * Optional raw-address vendor init sequence — same encoding as
+	 * qcom,init-seq (u8 addr/val pairs) but each pair is written to
+	 * the raw ULPI address rather than to ULPI_EXT_VENDOR_SPECIFIC +
+	 * addr. Lets boards reach the standard vendor range 0x30-0x3F.
+	 */
+	ret = qcom_usb_hs_phy_parse_init_seq(ulpi, "qcom,vendor-init-seq",
+					     &uphy->vendor_init_seq);
+	if (ret)
 		return ret;
-	/* NUL terminate */
-	uphy->init_seq[size / 2].addr = uphy->init_seq[size / 2].val = 0;
 
 	uphy->ref_clk = clk = devm_clk_get(&ulpi->dev, "ref");
 	if (IS_ERR(clk))
@@ -245,12 +318,10 @@ static int qcom_usb_hs_phy_probe(struct ulpi *ulpi)
 	if (IS_ERR(reg))
 		return PTR_ERR(reg);
 
-	uphy->reset = reset = devm_reset_control_get(&ulpi->dev, "por");
-	if (IS_ERR(reset)) {
-		if (PTR_ERR(reset) == -EPROBE_DEFER)
-			return PTR_ERR(reset);
-		uphy->reset = NULL;
-	}
+	uphy->reset = reset = devm_reset_control_get_optional_exclusive(&ulpi->dev, "por");
+	if (IS_ERR(reset))
+		return dev_err_probe(&ulpi->dev, PTR_ERR(reset),
+				     "failed to get reset control\n");
 
 	uphy->phy = devm_phy_create(&ulpi->dev, ulpi->dev.of_node,
 				    &qcom_usb_hs_phy_ops);
-- 
2.43.0

Re: [PATCH 2/2] phy: qcom: usb-hs: honour qcom,vendor-init-seq raw ULPI writes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Using `seq->addr == 0` as a loop terminator prevents writing to offset 0 in `qcom,init-seq`.
--

commit 23768611bb7332cb583754c2260443d91ea51fb4
Author: Herman van Hazendonk <github.com@herrie.org>

phy: qcom: usb-hs: honour qcom,vendor-init-seq raw ULPI writes

This patch adds support for the optional qcom,vendor-init-seq DT property
to write directly to raw ULPI register addresses. It also hardens the parse
logic shared between qcom,init-seq and qcom,vendor-init-seq by factoring it
out into qcom_usb_hs_phy_parse_init_seq().

> diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs.c b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> index 98a18987f1bec..89fbe8f8d0015 100644
> --- a/drivers/phy/qualcomm/phy-qcom-usb-hs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs.c
[ ... ]
> @@ -199,6 +219,59 @@ static const struct phy_ops qcom_usb_hs_phy_ops = {
[ ... ]
> +static int qcom_usb_hs_phy_parse_init_seq(struct ulpi *ulpi,
> +					  const char *propname,
> +					  struct ulpi_seq **out)
> +{
[ ... ]
> +	if (size) {
> +		int ret = of_property_read_u8_array(ulpi->dev.of_node,
> +						    propname, (u8 *)seq, size);
> +		if (ret)
> +			return ret;
> +	}
> +	/* NUL-terminate so the power_on loop's seq->addr-as-sentinel works. */
> +	seq[size / 2].addr = 0;

[Severity: High]
This is a pre-existing issue, but does using seq->addr == 0 as a sentinel
prevent writing to offset 0 in qcom,init-seq?

In qcom_usb_hs_phy_power_on(), the initialization loop terminates when
seq->addr is 0:

	for (seq = uphy->init_seq; seq->addr; seq++) {
		ret = ulpi_write(ulpi, ULPI_EXT_VENDOR_SPECIFIC + seq->addr,
				 seq->val);

Since seq->addr represents an offset added to ULPI_EXT_VENDOR_SPECIFIC
for qcom,init-seq, an offset of 0 is valid and maps to the first vendor
specific register. If a device tree specifies a write to offset 0, won't
the loop prematurely terminate and silently truncate the remainder of the
initialization sequence?

> +	seq[size / 2].val = 0;
> +
> +	*out = seq;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603054809.565723-1-github.com@herrie.org?part=2