[PATCH 0/2] Add charger detection support for Shikra

[PATCH 0/2] Add charger detection support for Shikra

[Krishna Kurapati]

QUSB2 Phy HW is capable of doing BC.1.2 charger detection. On most of the
platforms, the charger detection is done by PMIC. But on Shikra CQM/CQS
platforms, the PM4125 doesn't have ability to do charger detection and
that responsibility is delegated to QUSB2 Phy which acts on the DP/DM
lines in its control to get the job done.

During process of charger detection, if the controller is supposed to
enter device mode, the charger is detected as SDP/CDP/DCP. The controller
must enter device mode only if the detected charger is SDP/CDP. In case of
DCP, the phy can be turned off after detection. If the controller must
enter host mode, charger detection can be skipped and hence the phy must
know the role before the controller so that appropriate decision can be
taken whether the controller must enter device/host mode or not.

Intention of adding remote endpoints and usb-role-switch property binding
support is as follows:

a) The DP/DM lines do go through the phy to the controller i.e., the phy
exists between connector and controller. Hence adding remote endpoints
depicting the same.

b) Although the usb2 phy is not the entity doing role switching, it still
does need to decide which mode the controller must move into and hence
adding usb-role-switch property to allow role notifications from charger
to go to controller via phy.

Also the driver changes do the following:

a) Refactor phy enable/disable calls to control regulators and clocks.

b) Let the usb-role information coming from PM4125 to pass through the phy
so that when a vbus active signal is detected, the phy can check if its
SDP/CDP/DCP and pass on the role to controller only when the charger is
SDP/CDP. In case of host mode, the role is forwarded without any charger
detection.

c) Since the controller probe relies on phy probe to be completed first,
during phy probe, it won't be possible to get role-switch handle of the
controller. Hence check for controller role switch whenever a role info
is obtained from the connector node.

Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
---
Krishna Kurapati (2):
      dt-bindings: phy: qcom,qusb2: Add support for remote endpoints
      phy: qcom: qusb2: Add charger detection support

 .../devicetree/bindings/phy/qcom,qusb2-phy.yaml    |  23 +
 drivers/phy/qualcomm/phy-qcom-qusb2.c              | 606 +++++++++++++++++++--
 2 files changed, 597 insertions(+), 32 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261111-shikra-qusb2-charger-det-de6a4299f6c2

Best regards,
--  
Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>

[PATCH 1/2] dt-bindings: phy: qcom,qusb2: Add support for remote endpoints

[Krishna Kurapati]

QUSB2 Phy HW is capable of doing BC.1.2 charger detection. On most of the
platforms, the charger detection is done by PMIC. But on Shikra CQM/CQS
platforms, the PM4125 doesn't have ability to do charger detection and
that responsibility is delegated to QUSB2 Phy which acts on the DP/DM
lines in its control to get the job done.

During process of charger detection, if the controller is supposed to
enter device mode, the charger is detected as SDP/CDP/DCP. The controller
must enter device mode only if the detected charger is SDP/CDP. In case of
DCP, the phy can be turned off after detection. If the controller must
enter host mode, charger detection can be skipped and hence the phy must
know the role before the controller so that appropriate decision can be
taken whether the controller must enter device/host mode or not.

Intention of adding remote endpoints and usb-role-switch property binding
support is as follows:

a) The DP/DM lines do go through the phy to the controller i.e., the phy
exists between connector and controller. Hence adding remote endpoints
depicting the same.

b) Although the usb2 phy is not the entity doing role switching, it still
does need to decide which mode the controller must move into and hence
adding usb-role-switch property to allow role notifications from charger
to go to controller via phy.

Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
---
 .../devicetree/bindings/phy/qcom,qusb2-phy.yaml    | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
index 001fd0ccc985..0b6b2933dd98 100644
--- a/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
@@ -159,6 +159,29 @@ properties:
     maximum: 3
     default: 0
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    description:
+      Those ports can be used with any connector/pmic node that is able to
+      provide role switch notifications for charger detection purposes.
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Connector Data bus
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description: Controller data bus.
+
+  usb-role-switch:
+    description:
+      In the context of usb-phy, it denotes the capability to perform charger
+      detection. Although phy is not the one responsible for switching mode, it
+      is the one responsible to take a call on whether controller must switch
+      mode or not. In case of SDP/CDT, the phy would inform the controller to
+      switch to device and in case of DCP, role switch doesn't need to happen.
+
 required:
   - compatible
   - reg

-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: phy: qcom,qusb2: Add support for remote endpoints

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dt-bindings: phy: qcom,qusb2: usb-role-switch overloaded for
  BC1.2 detection
- [Low] dt-bindings: phy: qcom,qusb2: incomplete usb-role-switch and
  ports schema
--

commit fd796c127a8188028d89719a01ee3f717ede3073
Author: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>

dt-bindings: phy: qcom,qusb2: Add support for remote endpoints

This commit adds OF graph ports and the usb-role-switch property to the
Qualcomm QUSB2 PHY Devicetree binding to support BC1.2 charger detection
and role notification routing.

> diff --git a/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
> index 001fd0ccc9852..0b6b2933dd98b 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
> @@ -159,6 +159,29 @@ properties:
>      maximum: 3
>      default: 0
>  
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description:
> +      Those ports can be used with any connector/pmic node that is able to
> +      provide role switch notifications for charger detection purposes.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Connector Data bus
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: Controller data bus.
> +
> +  usb-role-switch:
> +    description:
> +      In the context of usb-phy, it denotes the capability to perform charger
> +      detection. Although phy is not the one responsible for switching mode, it
> +      is the one responsible to take a call on whether controller must switch
> +      mode or not. In case of SDP/CDT, the phy would inform the controller to
> +      switch to device and in case of DCP, role switch doesn't need to happen.
> +

[Severity: Medium]
Should usb-role-switch be used on the QUSB2 PHY node to represent BC1.2
charger detection and role notification forwarding?

In Documentation/devicetree/bindings/usb/usb-drd.yaml, usb-role-switch
indicates that a device is capable of assigning the USB data role, which
on these platforms is the USB controller rather than the PHY.  Using it
here to make fwnode_usb_role_switch_get() match the PHY endpoint encodes
Linux driver notification routing into the Devicetree ABI, while BC1.2
detection capability is already implied by the PHY compatible string and
the DP/DM routing is already described by ports.

[Severity: Low]
Does this schema need type and port constraints for dtbs_check validation?

Because usb-role-switch is defined here without referencing usb-drd.yaml,
it is missing type: boolean.  In addition, since the PHY does not permit a
child usb-connector node and requires both port@0 (incoming notification)
and port@1 (outgoing notification to the controller) when intercepting
role switches, ports should list port@0 and port@1 under required and a
dependencies entry should require ports when usb-role-switch is present.
Also, the description has a typo ("SDP/CDT" instead of "SDP/CDP").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-shikra-qusb2-charger-det-v1-0-5a9d919c66b5@oss.qualcomm.com?part=1

[PATCH 2/2] phy: qcom: qusb2: Add charger detection support

[Krishna Kurapati]

QUSB2 Phy is capable of performing BC.1.2 Charger detection by controlling
the dp/dm lines. On platforms where the PMIC isn't capable of performing
charger detection, the responsibility is delegated to the phy.

On Shikra CQM/CQS platforms, the PM4125 relies on QUSB2 Phy to perform
charger detection. As part of this, the following changes are being done:

a) Refactor phy enable/disable calls to control regulators and clocks.

b) Let the usb-role information coming from PM4125 to pass through the phy
so that when a vbus active signal is detected, the phy can check if its
SDP/CDP/DCP and pass on the role to controller only when the charger is
SDP/CDP. In case of host mode, the role is forwarded without any charger
detection.

c) Since the controller probe relies on phy probe to be completed first,
during phy probe, it won't be possible to get role-switch handle of the
controller. Hence check for controller role switch whenever a role info
is obtained from the connector node.

Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
---
 drivers/phy/qualcomm/phy-qcom-qusb2.c | 606 ++++++++++++++++++++++++++++++++--
 1 file changed, 574 insertions(+), 32 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 2e5fdb620cb2..549dc8d1378a 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -18,9 +18,31 @@
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
+#include <linux/power_supply.h>
+#include <linux/usb/role.h>
 
 #include <dt-bindings/phy/phy-qcom-qusb2.h>
 
+#define QUSB2PHY_PORT_QC1	0x70
+#define VDM_SRC_EN		BIT(4)
+#define VDP_SRC_EN		BIT(2)
+
+#define QUSB2PHY_PORT_UTMI_CTRL1	0xC0
+#define SUSPEND_N			BIT(5)
+#define TERM_SELECT			BIT(4)
+#define XCVR_SELECT_FS			BIT(2)
+#define OP_MODE_NON_DRIVE		BIT(0)
+
+#define QUSB2PHY_PORT_UTMI_CTRL2	0xC4
+#define UTMI_ULPI_SEL			BIT(7)
+#define UTMI_TEST_MUX_SEL		BIT(6)
+
+#define QUSB2PHY_PORT_QC2	0x74
+#define RDM_UP_EN		BIT(1)
+#define RDP_UP_EN		BIT(3)
+#define RPUM_LOW_EN		BIT(4)
+#define RPUP_LOW_EN		BIT(5)
+
 #define QUSB2PHY_PLL			0x0
 #define QUSB2PHY_PLL_TEST		0x04
 #define CLK_REF_SEL			BIT(7)
@@ -30,10 +52,19 @@
 #define QUSB2PHY_PLL_USER_CTL2		0x10
 #define QUSB2PHY_PLL_AUTOPGM_CTL1	0x1c
 #define QUSB2PHY_PLL_PWR_CTRL		0x18
+#define PLL_RESET_N_CNT_5		0x5
+#define PLL_RESET_N			BIT(4)
+#define PLL_AUTOPGM_EN			BIT(7)
 
 /* QUSB2PHY_PLL_STATUS register bits */
 #define PLL_LOCKED			BIT(5)
 
+/* QUSB2PHY_PLL_PWR_CTRL register bits */
+#define REF_BUF_EN			BIT(6)
+#define REXT_EN				BIT(4)
+#define PLL_BYPASSNL			BIT(1)
+#define REXT_TRIM_0			BIT(0)
+
 /* QUSB2PHY_PLL_COMMON_STATUS_ONE register bits */
 #define CORE_READY_STATUS		BIT(0)
 
@@ -57,6 +88,10 @@
 #define DMSE_INTR_EN				BIT(1)
 #define DPSE_INTR_EN				BIT(0)
 
+#define QUSB2PHY_PORT_INTR_STATUS	0xF0
+#define DMSE_INTR_HIGH			BIT(2)
+#define DPSE_INTR_HIGH			BIT(0)
+
 /* QUSB2PHY_PLL_CORE_INPUT_OVERRIDE register bits */
 #define CORE_PLL_EN_FROM_RESET			BIT(4)
 #define CORE_RESET				BIT(5)
@@ -97,6 +132,29 @@
 #define QUSB2PHY_IMP_CTRL2			0x224
 #define QUSB2PHY_CHG_CTRL2			0x23c
 
+#define CHG_DCD_TIMEOUT_MSEC			750
+#define CHG_DCD_POLL_TIME_MSEC			50
+#define CHG_PRIMARY_DET_TIME_MSEC		100
+#define CHG_SECONDARY_DET_TIME_MSEC		100
+#define DP_PULSE_WIDTH_MSEC			200
+
+enum port_state {
+	PORT_UNKNOWN,
+	PORT_DISCONNECTED,
+	PORT_DCD_IN_PROGRESS,
+	PORT_PRIMARY_IN_PROGRESS,
+	PORT_SECONDARY_IN_PROGRESS,
+	PORT_CHG_DET_DONE,
+	PORT_HOST_MODE,
+};
+
+enum chg_det_state {
+	STATE_UNKNOWN,
+	STATE_DCD,
+	STATE_PRIMARY,
+	STATE_SECONDARY,
+};
+
 struct qusb2_phy_init_tbl {
 	unsigned int offset;
 	unsigned int val;
@@ -478,9 +536,425 @@ struct qusb2_phy {
 	const struct qusb2_phy_cfg *cfg;
 	bool has_se_clk_scheme;
 	bool phy_initialized;
+	bool is_phy_enabled;
 	enum phy_mode mode;
+	int vbus_active;
+	int id_state;
+	struct power_supply	*usb_psy;
+	struct delayed_work	port_det_w;
+	enum port_state		port_state;
+	unsigned int		dcd_timeout;
+	bool			dcp_charger;
+
+	struct usb_role_switch	*connector_role_sw;
+	struct usb_role_switch	*controller_role_sw;
+	enum usb_role		current_role;
+	struct mutex		role_lock;
 };
 
+static int qusb_phy_notify_charger(struct qusb2_phy *qphy,
+					enum power_supply_usb_type charger_type)
+{
+	union power_supply_propval pval = {0};
+
+	if (!qphy->usb_psy) {
+		qphy->usb_psy = power_supply_get_by_name("pm4125-charger");
+		if (!qphy->usb_psy) {
+			dev_err(&qphy->phy->dev, "could not get usb power supply\n");
+			return -ENODEV;
+		}
+	}
+
+	dev_dbg(&qphy->phy->dev, "notifying charger type: %d\n", charger_type);
+
+	pval.intval = charger_type;
+	power_supply_set_property(qphy->usb_psy, POWER_SUPPLY_PROP_USB_TYPE,
+									&pval);
+	return 0;
+}
+
+static bool qusb_phy_chg_det_status(struct qusb2_phy *qphy,
+						enum chg_det_state state)
+{
+	u32 reg, status;
+
+	reg = readl_relaxed(qphy->base + QUSB2PHY_PORT_INTR_STATUS);
+	dev_dbg(&qphy->phy->dev, "chg_det_status: state=%d intr_status=0x%02x\n",
+		state, reg);
+
+	status = reg & 0xff;
+
+	switch (state) {
+	case STATE_DCD:
+		return (status != DPSE_INTR_HIGH);
+	case STATE_PRIMARY:
+		return (status && (status != DPSE_INTR_HIGH));
+	case STATE_SECONDARY:
+		return status;
+	case STATE_UNKNOWN:
+	default:
+		break;
+	}
+
+	return false;
+}
+
+/*
+ * Different circuit blocks are enabled on DP and DM lines as part
+ * of different phases of charger detection. Then the state of
+ * DP and DM lines are monitored to identify different type of
+ * chargers.
+ * These circuit blocks can be enabled with the configuration of
+ * the QUICKCHARGE1 and QUICKCHARGE2 registers and the DP/DM lines
+ * can be monitored with the status of the INTR_STATUS register.
+ */
+static void qusb_phy_chg_det_enable_seq(struct qusb2_phy *qphy, int state)
+{
+	dev_dbg(&qphy->phy->dev, "chg_det_enable_seq: state=%d base: %pK\n", state, qphy->base);
+
+	writel_relaxed(0x23, qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
+	writel_relaxed(0x35, qphy->base + QUSB2PHY_PORT_UTMI_CTRL1);
+	writel_relaxed(0xC0, qphy->base + QUSB2PHY_PORT_UTMI_CTRL2);
+	writel_relaxed(0x05, qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
+	writel_relaxed(0x22, qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
+	writel_relaxed(0x17, qphy->base + QUSB2PHY_PLL_PWR_CTRL);
+
+	usleep_range(5, 10);
+
+	writel_relaxed(0x15, qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
+	writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC1);
+	writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC2);
+
+	usleep_range(50, 60);
+
+	switch (state) {
+	case STATE_DCD:
+		writel_relaxed(0x08, qphy->base + QUSB2PHY_PORT_QC1);
+		writel_relaxed(0x01, qphy->base + QUSB2PHY_PORT_QC2);
+		writel_relaxed(0x1F, qphy->base + qphy->cfg->regs[QUSB2PHY_INTR_CTRL]);
+		break;
+	case STATE_PRIMARY:
+		writel_relaxed(0x25, qphy->base + QUSB2PHY_PORT_QC1);
+		writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC2);
+		writel_relaxed(0x1F, qphy->base + qphy->cfg->regs[QUSB2PHY_INTR_CTRL]);
+		break;
+	case STATE_SECONDARY:
+		writel_relaxed(0x72, qphy->base + QUSB2PHY_PORT_QC1);
+		writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC2);
+		writel_relaxed(0x1F, qphy->base + qphy->cfg->regs[QUSB2PHY_INTR_CTRL]);
+		break;
+	case STATE_UNKNOWN:
+	default:
+		break;
+	};
+}
+
+/*
+ * qusb2_phy_enable_phy - power up regulators and enable clocks for the PHY.
+ *
+ * Used by the charger-detection path to bring the PHY up before programming
+ * the QC1/QC2 detection registers, and by qusb2_phy_init().
+ * No-op if the PHY is already enabled (is_phy_enabled is true).
+ */
+static int qusb2_phy_enable_phy(struct qusb2_phy *qphy)
+{
+	struct device *dev = &qphy->phy->dev;
+	int ret;
+
+	if (qphy->is_phy_enabled) {
+		dev_dbg(dev, "PHY already enabled, skipping\n");
+		return 0;
+	}
+
+	dev_vdbg(dev, "enabling PHY clocks and regulators\n");
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
+	if (ret) {
+		dev_err(dev, "failed to enable regulators, %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(qphy->iface_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable iface_clk, %d\n", ret);
+		goto disable_regs;
+	}
+
+	ret = clk_prepare_enable(qphy->cfg_ahb_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable cfg_ahb_clk, %d\n", ret);
+		goto disable_iface_clk;
+	}
+
+	if (!qphy->has_se_clk_scheme) {
+		ret = clk_prepare_enable(qphy->ref_clk);
+		if (ret) {
+			dev_err(dev, "failed to enable ref_clk, %d\n", ret);
+			goto disable_ahb_clk;
+		}
+	}
+
+	qphy->is_phy_enabled = true;
+
+	return 0;
+
+disable_ahb_clk:
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+disable_iface_clk:
+	clk_disable_unprepare(qphy->iface_clk);
+disable_regs:
+	regulator_bulk_disable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
+	return ret;
+}
+
+/*
+ * qusb2_phy_disable_phy - disable clocks and power down regulators for the PHY.
+ */
+static void qusb2_phy_disable_phy(struct qusb2_phy *qphy)
+{
+	if (!qphy->is_phy_enabled) {
+		dev_dbg(&qphy->phy->dev, "PHY already disabled, skipping\n");
+		return;
+	}
+
+	dev_vdbg(&qphy->phy->dev, "disabling PHY clocks and regulators\n");
+
+	if (!qphy->has_se_clk_scheme)
+		clk_disable_unprepare(qphy->ref_clk);
+
+	clk_disable_unprepare(qphy->cfg_ahb_clk);
+	clk_disable_unprepare(qphy->iface_clk);
+	regulator_bulk_disable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
+	qphy->is_phy_enabled = false;
+}
+
+/*
+ * qusb2_phy_forward_role - forward a USB role to the controller.
+ *
+ * Only called after charger detection confirms the port is SDP or CDP, so
+ * the controller knows it should operate in device mode.
+ */
+static void qusb2_phy_forward_role(struct qusb2_phy *qphy, enum usb_role role)
+{
+	if (!qphy->controller_role_sw)
+		return;
+
+	dev_dbg(&qphy->phy->dev, "forwarding role %s to controller\n",
+		usb_role_string(role));
+
+	usb_role_switch_set_role(qphy->controller_role_sw, role);
+}
+
+static int qusb_phy_drive_dp_pulse(struct qusb2_phy *qphy)
+{
+	int ret;
+
+	dev_dbg(&qphy->phy->dev, "connected to a CDP, drive DP up\n");
+
+	ret = qusb2_phy_enable_phy(qphy);
+	if (ret)
+		return ret;
+
+	ret = reset_control_assert(qphy->phy_reset);
+	if (ret)
+		dev_err(&qphy->phy->dev, "phy assert failed\n");
+	usleep_range(100, 150);
+	ret = reset_control_deassert(qphy->phy_reset);
+	if (ret)
+		dev_err(&qphy->phy->dev, "phy deassert failed\n");
+
+	writel_relaxed(CLAMP_N_EN | FREEZIO_N | POWER_DOWN,
+			qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
+	writel_relaxed(TERM_SELECT | XCVR_SELECT_FS | OP_MODE_NON_DRIVE |
+			SUSPEND_N, qphy->base + QUSB2PHY_PORT_UTMI_CTRL1);
+	writel_relaxed(UTMI_ULPI_SEL | UTMI_TEST_MUX_SEL,
+			qphy->base + QUSB2PHY_PORT_UTMI_CTRL2);
+	writel_relaxed(PLL_RESET_N_CNT_5,
+			qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
+	writel_relaxed(CLAMP_N_EN | FREEZIO_N,
+			qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
+	writel_relaxed(REF_BUF_EN | REXT_EN | PLL_BYPASSNL | REXT_TRIM_0,
+			qphy->base + QUSB2PHY_PLL_PWR_CTRL);
+
+	usleep_range(5, 10);
+
+	writel_relaxed(0x15, qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
+	writel_relaxed(PLL_RESET_N | PLL_RESET_N_CNT_5,
+			qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
+	writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC1);
+	writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC2);
+
+	usleep_range(50, 60);
+
+	writel_relaxed(RDP_UP_EN, qphy->base + QUSB2PHY_PORT_QC2);
+
+	msleep(DP_PULSE_WIDTH_MSEC);
+
+	writel_relaxed(CLAMP_N_EN | FREEZIO_N | POWER_DOWN,
+			qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
+	writel_relaxed(PLL_AUTOPGM_EN | PLL_RESET_N | PLL_RESET_N_CNT_5,
+			qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
+	writel_relaxed(UTMI_ULPI_SEL, qphy->base + QUSB2PHY_PORT_UTMI_CTRL2);
+	writel_relaxed(TERM_SELECT, qphy->base + QUSB2PHY_PORT_UTMI_CTRL1);
+
+	qusb2_phy_disable_phy(qphy);
+
+	return 0;
+}
+
+static void qusb_phy_port_state_work(struct work_struct *w)
+{
+	struct qusb2_phy *qphy = container_of(w, struct qusb2_phy,
+							port_det_w.work);
+	unsigned long delay = 0;
+	int status, ret = 0;
+
+	dev_dbg(&qphy->phy->dev, "port_state_work: state=%d vbus_active=%d id_state=%d\n",
+		qphy->port_state, qphy->vbus_active, qphy->id_state);
+
+	switch (qphy->port_state) {
+	case PORT_UNKNOWN:
+		if (!qphy->id_state) {
+			dev_dbg(&qphy->phy->dev, "ID grounded, entering host mode\n");
+			qphy->port_state = PORT_HOST_MODE;
+			qusb2_phy_forward_role(qphy, USB_ROLE_HOST);
+			return;
+		}
+
+		if (qphy->vbus_active) {
+			dev_dbg(&qphy->phy->dev, "VBUS active, starting DCD\n");
+
+			/* Enable DCD sequence */
+			ret = qusb2_phy_enable_phy(qphy);
+			if (ret)
+				return;
+
+			qusb_phy_chg_det_enable_seq(qphy, STATE_DCD);
+			qphy->port_state = PORT_DCD_IN_PROGRESS;
+			qphy->dcd_timeout = 0;
+			delay = CHG_DCD_POLL_TIME_MSEC;
+			break;
+		}
+		return;
+	case PORT_DISCONNECTED:
+		dev_dbg(&qphy->phy->dev, "port disconnected, resetting state\n");
+		qusb2_phy_disable_phy(qphy);
+		qphy->port_state = PORT_UNKNOWN;
+		qphy->dcp_charger = false;
+		break;
+	case PORT_DCD_IN_PROGRESS:
+		if (!qphy->vbus_active) {
+			dev_dbg(&qphy->phy->dev, "VBUS lost during DCD, aborting\n");
+
+			/* Disable PHY sequence */
+			qusb2_phy_disable_phy(qphy);
+			qphy->port_state = PORT_DISCONNECTED;
+			break;
+		}
+
+		status = qusb_phy_chg_det_status(qphy, STATE_DCD);
+		if (!status && qphy->dcd_timeout < CHG_DCD_TIMEOUT_MSEC) {
+			dev_dbg(&qphy->phy->dev, "DCD polling: timeout=%u ms\n",
+				qphy->dcd_timeout);
+			delay = CHG_DCD_POLL_TIME_MSEC;
+			qphy->dcd_timeout += delay;
+		} else if (status) {
+			dev_dbg(&qphy->phy->dev, "DCD complete, starting primary detection\n");
+			qusb_phy_chg_det_enable_seq(qphy, STATE_PRIMARY);
+			qphy->port_state = PORT_PRIMARY_IN_PROGRESS;
+			delay = CHG_PRIMARY_DET_TIME_MSEC;
+		} else if (qphy->dcd_timeout >= CHG_DCD_TIMEOUT_MSEC) {
+			dev_dbg(&qphy->phy->dev,
+				 "charger detected: DCP (DCD timeout after %u ms)\n",
+				 qphy->dcd_timeout);
+			qusb_phy_notify_charger(qphy,
+						POWER_SUPPLY_USB_TYPE_DCP);
+			qusb2_phy_disable_phy(qphy);
+			qphy->port_state = PORT_CHG_DET_DONE;
+		}
+		break;
+	case PORT_PRIMARY_IN_PROGRESS:
+		if (!qphy->vbus_active) {
+			dev_dbg(&qphy->phy->dev, "VBUS lost during primary detection, aborting\n");
+			qusb2_phy_disable_phy(qphy);
+			qphy->port_state = PORT_DISCONNECTED;
+			break;
+		}
+
+		status = qusb_phy_chg_det_status(qphy, STATE_PRIMARY);
+		if (status) {
+			dev_dbg(&qphy->phy->dev,
+				"primary detection positive, starting secondary detection\n");
+			qusb_phy_chg_det_enable_seq(qphy, STATE_SECONDARY);
+			qphy->port_state = PORT_SECONDARY_IN_PROGRESS;
+			delay = CHG_SECONDARY_DET_TIME_MSEC;
+
+		} else {
+			dev_dbg(&qphy->phy->dev, "charger detected: SDP (standard USB host)\n");
+			qusb2_phy_disable_phy(qphy);
+			qusb_phy_notify_charger(qphy, POWER_SUPPLY_USB_TYPE_SDP);
+			qusb2_phy_forward_role(qphy, USB_ROLE_DEVICE);
+			qphy->port_state = PORT_CHG_DET_DONE;
+		}
+		break;
+	case PORT_SECONDARY_IN_PROGRESS:
+		if (!qphy->vbus_active) {
+			dev_dbg(&qphy->phy->dev, "VBUS lost during secondary detection, aborting\n");
+			qusb2_phy_disable_phy(qphy);
+			qphy->port_state = PORT_DISCONNECTED;
+			break;
+		}
+
+		status = qusb_phy_chg_det_status(qphy, STATE_SECONDARY);
+		if (status) {
+			dev_dbg(&qphy->phy->dev, "charger detected: DCP (dedicated charging port)\n");
+			qusb_phy_notify_charger(qphy,
+						POWER_SUPPLY_USB_TYPE_DCP);
+			qphy->dcp_charger = true;
+		} else {
+			dev_dbg(&qphy->phy->dev, "charger detected: CDP (charging downstream port)\n");
+			qusb_phy_notify_charger(qphy,
+						POWER_SUPPLY_USB_TYPE_CDP);
+			qusb_phy_drive_dp_pulse(qphy);
+			qusb2_phy_disable_phy(qphy);
+			qusb2_phy_forward_role(qphy, USB_ROLE_DEVICE);
+		}
+
+		qphy->port_state = PORT_CHG_DET_DONE;
+		break;
+	case PORT_CHG_DET_DONE:
+		if (!qphy->vbus_active) {
+			dev_dbg(&qphy->phy->dev, "VBUS removed, resetting from CHG_DET_DONE\n");
+
+			if (qphy->dcp_charger) {
+				qphy->dcp_charger = false;
+				qusb2_phy_disable_phy(qphy);
+			} else {
+				qusb2_phy_forward_role(qphy, USB_ROLE_NONE);
+			}
+			qphy->port_state = PORT_UNKNOWN;
+		}
+
+		return;
+	case PORT_HOST_MODE:
+		if (qphy->id_state) {
+			dev_dbg(&qphy->phy->dev, "ID released, leaving host mode\n");
+			qphy->port_state = PORT_UNKNOWN;
+			qusb2_phy_forward_role(qphy, USB_ROLE_NONE);
+		}
+
+		if (!qphy->vbus_active)
+			return;
+
+		break;
+	default:
+		return;
+	}
+
+	queue_delayed_work(system_freezable_wq,
+			&qphy->port_det_w, msecs_to_jiffies(delay));
+}
+
 static inline void qusb2_write_mask(void __iomem *base, u32 offset,
 				    u32 val, u32 mask)
 {
@@ -767,29 +1241,16 @@ static int qusb2_phy_init(struct phy *phy)
 
 	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
 
-	/* turn on regulator supplies */
-	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
+	/* Power up regulators and enable clocks (including ref_clk if needed). */
+	ret = qusb2_phy_enable_phy(qphy);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(qphy->iface_clk);
-	if (ret) {
-		dev_err(&phy->dev, "failed to enable iface_clk, %d\n", ret);
-		goto poweroff_phy;
-	}
-
-	/* enable ahb interface clock to program phy */
-	ret = clk_prepare_enable(qphy->cfg_ahb_clk);
-	if (ret) {
-		dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret);
-		goto disable_iface_clk;
-	}
-
 	/* Perform phy reset */
 	ret = reset_control_assert(qphy->phy_reset);
 	if (ret) {
 		dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
-		goto disable_ahb_clk;
+		return ret;
 	}
 
 	/* 100 us delay to keep PHY in reset mode */
@@ -798,7 +1259,7 @@ static int qusb2_phy_init(struct phy *phy)
 	ret = reset_control_deassert(qphy->phy_reset);
 	if (ret) {
 		dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret);
-		goto disable_ahb_clk;
+		goto assert_phy_reset;
 	}
 
 	/* Disable the PHY */
@@ -887,23 +1348,15 @@ static int qusb2_phy_init(struct phy *phy)
 		dev_err(&phy->dev,
 			"QUSB2PHY pll lock failed: status reg = %x\n", val);
 		ret = -EBUSY;
-		goto disable_ref_clk;
+		goto assert_phy_reset;
 	}
 	qphy->phy_initialized = true;
 
 	return 0;
 
-disable_ref_clk:
-	if (!qphy->has_se_clk_scheme)
-		clk_disable_unprepare(qphy->ref_clk);
 assert_phy_reset:
 	reset_control_assert(qphy->phy_reset);
-disable_ahb_clk:
-	clk_disable_unprepare(qphy->cfg_ahb_clk);
-disable_iface_clk:
-	clk_disable_unprepare(qphy->iface_clk);
-poweroff_phy:
-	regulator_bulk_disable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
+	qusb2_phy_disable_phy(qphy);
 
 	return ret;
 }
@@ -921,16 +1374,83 @@ static int qusb2_phy_exit(struct phy *phy)
 
 	reset_control_assert(qphy->phy_reset);
 
-	clk_disable_unprepare(qphy->cfg_ahb_clk);
-	clk_disable_unprepare(qphy->iface_clk);
-
-	regulator_bulk_disable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
+	qusb2_phy_disable_phy(qphy);
 
 	qphy->phy_initialized = false;
 
 	return 0;
 }
 
+/*
+ * qusb2_role_switch_set - USB role switch callback.
+ *
+ * Invoked by the role-switch framework when an upstream user (e.g. a Type-C
+ * port manager) requests a role change.  For USB_ROLE_DEVICE we kick off
+ * charger detection; the state machine will forward the role to the
+ * controller switch only after confirming SDP or CDP.  For USB_ROLE_HOST
+ * and USB_ROLE_NONE we forward immediately.
+ */
+static int qusb2_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
+{
+	struct qusb2_phy *qphy = usb_role_switch_get_drvdata(sw);
+
+	dev_dbg(&qphy->phy->dev, "role switch set: %s\n", usb_role_string(role));
+
+	mutex_lock(&qphy->role_lock);
+	qphy->current_role = role;
+
+	/*
+	 * If the controller role switch was not available at probe time (e.g.
+	 * the USB controller driver registered its role switch after us), try
+	 * to look it up now.  A return of NULL or -ENODEV simply means there
+	 * is no controller switch to forward to, which is fine.
+	 */
+	if (!qphy->controller_role_sw) {
+		struct usb_role_switch *dsw = usb_role_switch_get(&qphy->phy->dev);
+
+		if (IS_ERR_OR_NULL(dsw)) {
+			if (!IS_ERR(dsw) || PTR_ERR(dsw) == -ENODEV)
+				dev_dbg(&qphy->phy->dev,
+					"no controller role switch available\n");
+			else
+				dev_err(&qphy->phy->dev,
+					"failed to get controller role switch: %ld\n",
+					PTR_ERR(dsw));
+		} else {
+			qphy->controller_role_sw = dsw;
+		}
+	}
+
+	switch (role) {
+	case USB_ROLE_DEVICE:
+		/*
+		 * VBUS is active; start charger detection.  The state machine
+		 * will forward USB_ROLE_DEVICE controller only for SDP/CDP.
+		 */
+		qphy->vbus_active = 1;
+		qphy->id_state = 1;
+		qphy->port_state = PORT_UNKNOWN;
+		queue_delayed_work(system_freezable_wq, &qphy->port_det_w, 0);
+		break;
+	case USB_ROLE_HOST:
+		qphy->id_state = 0;
+		qphy->vbus_active = 0;
+		qphy->port_state = PORT_UNKNOWN;
+		queue_delayed_work(system_freezable_wq, &qphy->port_det_w, 0);
+		break;
+	case USB_ROLE_NONE:
+	default:
+		qphy->vbus_active = 0;
+		qphy->id_state = 1;
+		cancel_delayed_work(&qphy->port_det_w);
+		qusb2_phy_forward_role(qphy, USB_ROLE_NONE);
+		break;
+	}
+	mutex_unlock(&qphy->role_lock);
+
+	return 0;
+}
+
 static const struct phy_ops qusb2_phy_gen_ops = {
 	.init		= qusb2_phy_init,
 	.exit		= qusb2_phy_exit,
@@ -1131,7 +1651,29 @@ static int qusb2_phy_probe(struct platform_device *pdev)
 
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
 
-	return PTR_ERR_OR_ZERO(phy_provider);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	mutex_init(&qphy->role_lock);
+	INIT_DELAYED_WORK(&qphy->port_det_w, qusb_phy_port_state_work);
+
+	if (device_property_read_bool(dev, "usb-role-switch")) {
+		struct usb_role_switch_desc role_sw_desc = {};
+
+		role_sw_desc.fwnode = dev->fwnode;
+		role_sw_desc.set = qusb2_role_switch_set;
+		role_sw_desc.driver_data = qphy;
+
+		qphy->connector_role_sw = usb_role_switch_register(dev, &role_sw_desc);
+		if (IS_ERR(qphy->connector_role_sw)) {
+			ret = PTR_ERR(qphy->connector_role_sw);
+			dev_err(dev, "failed to register role switch, %d\n", ret);
+			return ret;
+		}
+	}
+	qphy->controller_role_sw = NULL;
+
+	return 0;
 }
 
 static struct platform_driver qusb2_phy_driver = {

-- 
2.34.1

Re: [PATCH 2/2] phy: qcom: qusb2: Add charger detection support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 10 potential
issues to consider:
- [High] phy: qcom: qusb2: unbalanced ref_clk accounting in
  qusb2_phy_init()
- [High] phy: qcom: qusb2: charger detection disables initialized PHY
- [High] phy: qcom: qusb2: PHY left enabled on USB_ROLE_NONE disconnect
- [High] phy: qcom: qusb2: missing teardown of work and role switches on
  unbind
- [High] phy: qcom: qusb2: missing Kconfig dependency on USB_ROLE_SWITCH
- [Medium] phy: qcom: qusb2: clock and regulator leak in
  qusb2_phy_init()
- [Medium] phy: qcom: qusb2: race between qusb2_role_switch_set() and
  work
- [Medium] phy: qcom: qusb2: mismatched register writes in
  qusb_phy_drive_dp_pulse()
- [Low] phy: qcom: qusb2: undocumented new members in struct qusb2_phy
  kernel-doc
- [Low] phy: qcom: qusb2: hardcoded charger name in
  qusb_phy_notify_charger()
--

commit 24edde049bf5427e2a864a633231dd8850451f54
Author: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>

phy: qcom: qusb2: Add charger detection support

This commit adds BC 1.2 charger detection to the QUSB2 PHY driver and
registers a USB role switch to intercept role transitions and forward
device mode to the controller only after detecting SDP or CDP.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index 2e5fdb620cb2d..549dc8d1378a4 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
[ ... ]
> @@ -478,9 +536,425 @@ struct qusb2_phy {
>  	const struct qusb2_phy_cfg *cfg;
>  	bool has_se_clk_scheme;
>  	bool phy_initialized;
> +	bool is_phy_enabled;
>  	enum phy_mode mode;
> +	int vbus_active;
> +	int id_state;
> +	struct power_supply	*usb_psy;
> +	struct delayed_work	port_det_w;
> +	enum port_state		port_state;
> +	unsigned int		dcd_timeout;
> +	bool			dcp_charger;
> +
> +	struct usb_role_switch	*connector_role_sw;
> +	struct usb_role_switch	*controller_role_sw;
> +	enum usb_role		current_role;
> +	struct mutex		role_lock;
>  };

[Severity: Low]
struct qusb2_phy has a kernel-doc comment above it, so adding these 12
members without @member descriptions will trigger scripts/kernel-doc
warnings under W=1.

> +static int qusb_phy_notify_charger(struct qusb2_phy *qphy,
> +					enum power_supply_usb_type charger_type)
> +{
> +	union power_supply_propval pval = {0};
> +
> +	if (!qphy->usb_psy) {
> +		qphy->usb_psy = power_supply_get_by_name("pm4125-charger");
> +		if (!qphy->usb_psy) {
> +			dev_err(&qphy->phy->dev, "could not get usb power supply\n");
> +			return -ENODEV;
> +		}
> +	}

[Severity: Low]
Should this look up the charger power supply via a firmware reference
(such as devm_power_supply_get_by_reference()) instead of hardcoding
"pm4125-charger" in the generic QUSB2 PHY driver?  Any platform with
usb-role-switch enabled whose charger power supply uses a different name
will fail with -ENODEV here.

[ ... ]

> +static int qusb_phy_drive_dp_pulse(struct qusb2_phy *qphy)
> +{
> +	int ret;
> +
> +	dev_dbg(&qphy->phy->dev, "connected to a CDP, drive DP up\n");
> +
> +	ret = qusb2_phy_enable_phy(qphy);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_assert(qphy->phy_reset);
> +	if (ret)
> +		dev_err(&qphy->phy->dev, "phy assert failed\n");
> +	usleep_range(100, 150);
> +	ret = reset_control_deassert(qphy->phy_reset);
> +	if (ret)
> +		dev_err(&qphy->phy->dev, "phy deassert failed\n");
> +
> +	writel_relaxed(CLAMP_N_EN | FREEZIO_N | POWER_DOWN,
> +			qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
> +	writel_relaxed(TERM_SELECT | XCVR_SELECT_FS | OP_MODE_NON_DRIVE |
> +			SUSPEND_N, qphy->base + QUSB2PHY_PORT_UTMI_CTRL1);
> +	writel_relaxed(UTMI_ULPI_SEL | UTMI_TEST_MUX_SEL,
> +			qphy->base + QUSB2PHY_PORT_UTMI_CTRL2);
> +	writel_relaxed(PLL_RESET_N_CNT_5,
> +			qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
> +	writel_relaxed(CLAMP_N_EN | FREEZIO_N,
> +			qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
> +	writel_relaxed(REF_BUF_EN | REXT_EN | PLL_BYPASSNL | REXT_TRIM_0,
> +			qphy->base + QUSB2PHY_PLL_PWR_CTRL);
> +
> +	usleep_range(5, 10);
> +
> +	writel_relaxed(0x15, qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
> +	writel_relaxed(PLL_RESET_N | PLL_RESET_N_CNT_5,
> +			qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
> +	writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC1);
> +	writel_relaxed(0x00, qphy->base + QUSB2PHY_PORT_QC2);
> +
> +	usleep_range(50, 60);
> +
> +	writel_relaxed(RDP_UP_EN, qphy->base + QUSB2PHY_PORT_QC2);
> +
> +	msleep(DP_PULSE_WIDTH_MSEC);
> +
> +	writel_relaxed(CLAMP_N_EN | FREEZIO_N | POWER_DOWN,
> +			qphy->base + qphy->cfg->regs[QUSB2PHY_PORT_POWERDOWN]);
> +	writel_relaxed(PLL_AUTOPGM_EN | PLL_RESET_N | PLL_RESET_N_CNT_5,
> +			qphy->base + QUSB2PHY_PLL_AUTOPGM_CTL1);
> +	writel_relaxed(UTMI_ULPI_SEL, qphy->base + QUSB2PHY_PORT_UTMI_CTRL2);
> +	writel_relaxed(TERM_SELECT, qphy->base + QUSB2PHY_PORT_UTMI_CTRL1);
> +
> +	qusb2_phy_disable_phy(qphy);
> +
> +	return 0;
> +}

[Severity: Medium]
Are the register values and relaxed writes in qusb_phy_drive_dp_pulse()
intended?

The bring-up sequence in qusb_phy_chg_det_enable_seq() writes 0x17 to
QUSB2PHY_PLL_PWR_CTRL, whereas REF_BUF_EN | REXT_EN | PLL_BYPASSNL |
REXT_TRIM_0 evaluates to 0x53 because REF_BUF_EN is defined as BIT(6)
rather than BIT(2).  Below that, QUSB2PHY_PLL_AUTOPGM_CTL1 is written
twice with 0x15 (raw 0x15 followed by PLL_RESET_N | PLL_RESET_N_CNT_5),
and the four teardown writes at the end use writel_relaxed() without a
readback flush right before qusb2_phy_disable_phy() gates cfg_ahb_clk.

[ ... ]

> +	case PORT_PRIMARY_IN_PROGRESS:
> +		if (!qphy->vbus_active) {
> +			dev_dbg(&qphy->phy->dev, "VBUS lost during primary detection, aborting\n");
> +			qusb2_phy_disable_phy(qphy);
> +			qphy->port_state = PORT_DISCONNECTED;
> +			break;
> +		}
> +
> +		status = qusb_phy_chg_det_status(qphy, STATE_PRIMARY);
> +		if (status) {
> +			dev_dbg(&qphy->phy->dev,
> +				"primary detection positive, starting secondary detection\n");
> +			qusb_phy_chg_det_enable_seq(qphy, STATE_SECONDARY);
> +			qphy->port_state = PORT_SECONDARY_IN_PROGRESS;
> +			delay = CHG_SECONDARY_DET_TIME_MSEC;
> +
> +		} else {
> +			dev_dbg(&qphy->phy->dev, "charger detected: SDP (standard USB host)\n");
> +			qusb2_phy_disable_phy(qphy);
> +			qusb_phy_notify_charger(qphy, POWER_SUPPLY_USB_TYPE_SDP);
> +			qusb2_phy_forward_role(qphy, USB_ROLE_DEVICE);
> +			qphy->port_state = PORT_CHG_DET_DONE;
> +		}
> +		break;

[Severity: High]
What happens when charger detection runs after the USB controller has
already called phy_init()?

Controller drivers such as dwc3 call phy_init() -> qusb2_phy_init() during
probe, which sets qphy->is_phy_enabled = true and qphy->phy_initialized =
true.  When USB_ROLE_DEVICE later triggers qusb_phy_port_state_work(),
qusb_phy_chg_det_enable_seq() overwrites the operational PHY registers (and
qusb_phy_drive_dp_pulse() resets the PHY on CDP), and then
qusb2_phy_disable_phy() disables all clocks and regulators while
qphy->phy_initialized remains true.  Forwarding USB_ROLE_DEVICE then puts
the controller into device mode with a powered-down, unconfigured PHY, and
a subsequent qusb2_phy_runtime_suspend() or qusb2_phy_exit() will access
MMIO registers with cfg_ahb_clk and regulators disabled.

[ ... ]

> @@ -767,29 +1241,16 @@ static int qusb2_phy_init(struct phy *phy)
>  
>  	dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__);
>  
> -	/* turn on regulator supplies */
> -	ret = regulator_bulk_enable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
> +	/* Power up regulators and enable clocks (including ref_clk if needed). */
> +	ret = qusb2_phy_enable_phy(qphy);
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_prepare_enable(qphy->iface_clk);
> -	if (ret) {
> -		dev_err(&phy->dev, "failed to enable iface_clk, %d\n", ret);
> -		goto poweroff_phy;
> -	}
> -
> -	/* enable ahb interface clock to program phy */
> -	ret = clk_prepare_enable(qphy->cfg_ahb_clk);
> -	if (ret) {
> -		dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret);
> -		goto disable_iface_clk;
> -	}
> -
>  	/* Perform phy reset */
>  	ret = reset_control_assert(qphy->phy_reset);
>  	if (ret) {
>  		dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret);
> -		goto disable_ahb_clk;
> +		return ret;
>  	}

[Severity: Medium]
Does returning directly when reset_control_assert() fails leak the clocks
and regulators enabled by qusb2_phy_enable_phy() and leave
qphy->is_phy_enabled set to true?

>  	/* 100 us delay to keep PHY in reset mode */
>  	usleep_range(100, 150);
>  
>  	ret = reset_control_deassert(qphy->phy_reset);
>  	if (ret) {
>  		dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret);
> -		goto disable_ahb_clk;
> +		goto assert_phy_reset;
>  	}
>  
>  	/* Disable the PHY */
> @@ -887,23 +1348,15 @@ static int qusb2_phy_init(struct phy *phy)
>  		dev_err(&phy->dev,
>  			"QUSB2PHY pll lock failed: status reg = %x\n", val);
>  		ret = -EBUSY;
> -		goto disable_ref_clk;
> +		goto assert_phy_reset;
>  	}
>  	qphy->phy_initialized = true;
>  
>  	return 0;
>  
> -disable_ref_clk:
> -	if (!qphy->has_se_clk_scheme)
> -		clk_disable_unprepare(qphy->ref_clk);
>  assert_phy_reset:
>  	reset_control_assert(qphy->phy_reset);
> -disable_ahb_clk:
> -	clk_disable_unprepare(qphy->cfg_ahb_clk);
> -disable_iface_clk:
> -	clk_disable_unprepare(qphy->iface_clk);
> -poweroff_phy:
> -	regulator_bulk_disable(ARRAY_SIZE(qphy->vregs), qphy->vregs);
> +	qusb2_phy_disable_phy(qphy);
>  
>  	return ret;
>  }

[Severity: High]
Does moving clk_prepare_enable(qphy->ref_clk) into qusb2_phy_enable_phy()
unbalance ref_clk across both single-ended and differential clock
platforms?

qphy->has_se_clk_scheme starts as false and is only determined halfway
through qusb2_phy_init() from cfg->se_clk_scheme_default and qphy->tcsr:

drivers/phy/qualcomm/phy-qcom-qusb2.c:qusb2_phy_init() {
    ...
	qphy->has_se_clk_scheme = cfg->se_clk_scheme_default;
    ...
	if (!qphy->has_se_clk_scheme) {
		ret = clk_prepare_enable(qphy->ref_clk);
    ...
}

Because qusb2_phy_enable_phy() runs at the start of qusb2_phy_init() (or
earlier during charger detection) while qphy->has_se_clk_scheme is still
false, it always enables ref_clk.  On single-ended platforms,
qusb2_phy_init() then sets qphy->has_se_clk_scheme = true, so
qusb2_phy_exit() and qusb2_phy_disable_phy() both skip disabling ref_clk
and leak the enable reference.  On differential platforms,
qusb2_phy_init() still calls clk_prepare_enable(qphy->ref_clk) a second
time, leaving ref_clk enabled twice so qusb2_phy_runtime_suspend() cannot
gate it and the assert_phy_reset error path leaks one reference.

[ ... ]

> +static int qusb2_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
> +{
[ ... ]
> +	switch (role) {
> +	case USB_ROLE_DEVICE:
> +		/*
> +		 * VBUS is active; start charger detection.  The state machine
> +		 * will forward USB_ROLE_DEVICE controller only for SDP/CDP.
> +		 */
> +		qphy->vbus_active = 1;
> +		qphy->id_state = 1;
> +		qphy->port_state = PORT_UNKNOWN;
> +		queue_delayed_work(system_freezable_wq, &qphy->port_det_w, 0);
> +		break;
> +	case USB_ROLE_HOST:
> +		qphy->id_state = 0;
> +		qphy->vbus_active = 0;
> +		qphy->port_state = PORT_UNKNOWN;
> +		queue_delayed_work(system_freezable_wq, &qphy->port_det_w, 0);
> +		break;
> +	case USB_ROLE_NONE:
> +	default:
> +		qphy->vbus_active = 0;
> +		qphy->id_state = 1;
> +		cancel_delayed_work(&qphy->port_det_w);
> +		qusb2_phy_forward_role(qphy, USB_ROLE_NONE);
> +		break;
> +	}
> +	mutex_unlock(&qphy->role_lock);
> +
> +	return 0;
> +}

[Severity: High]
Does canceling port_det_w on USB_ROLE_NONE prevent the disconnect cleanup
in qusb_phy_port_state_work() from ever running?

When a DCP charger is detected in PORT_SECONDARY_IN_PROGRESS, the PHY is
left enabled with qphy->dcp_charger = true, and PORT_CHG_DET_DONE returns
once while qphy->vbus_active is 1 without re-queuing port_det_w.  When the
cable is unplugged (either after DCP detection or mid-detection in
PORT_DCD_IN_PROGRESS / PORT_PRIMARY_IN_PROGRESS /
PORT_SECONDARY_IN_PROGRESS), USB_ROLE_NONE sets qphy->vbus_active = 0 and
calls cancel_delayed_work(&qphy->port_det_w) instead of running the state
machine or calling qusb2_phy_disable_phy(), leaving the PHY clocks,
regulators, and DP/DM detection circuits enabled and qphy->dcp_charger set
to true.

[Severity: Medium]
Can qusb2_role_switch_set() also race with an already-running
qusb_phy_port_state_work()?

CPU0                                      CPU1 (kworker)
qusb_phy_port_state_work()
  case PORT_SECONDARY_IN_PROGRESS:
    qusb_phy_drive_dp_pulse()
      msleep(DP_PULSE_WIDTH_MSEC)
qusb2_role_switch_set(USB_ROLE_NONE)
  mutex_lock(&qphy->role_lock)
  qphy->vbus_active = 0
  cancel_delayed_work(&qphy->port_det_w)
  qusb2_phy_forward_role(USB_ROLE_NONE)
  mutex_unlock(&qphy->role_lock)
                                          qusb2_phy_forward_role(USB_ROLE_DEVICE)
                                            <-- stale role forwarded after NONE
                                          qphy->port_state = PORT_CHG_DET_DONE

qusb_phy_port_state_work() does not acquire qphy->role_lock, and
cancel_delayed_work() does not wait for an executing work item to finish,
so a disconnect during the 200 ms sleep in qusb_phy_drive_dp_pulse() (or
in qusb_phy_chg_det_enable_seq()) allows the worker to overwrite
qphy->port_state and forward a stale USB_ROLE_DEVICE after USB_ROLE_NONE.

[ ... ]

> @@ -1131,7 +1651,29 @@ static int qusb2_phy_probe(struct platform_device *pdev)
>  
>  	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>  
> -	return PTR_ERR_OR_ZERO(phy_provider);
> +	if (IS_ERR(phy_provider))
> +		return PTR_ERR(phy_provider);
> +
> +	mutex_init(&qphy->role_lock);
> +	INIT_DELAYED_WORK(&qphy->port_det_w, qusb_phy_port_state_work);
> +
> +	if (device_property_read_bool(dev, "usb-role-switch")) {
> +		struct usb_role_switch_desc role_sw_desc = {};
> +
> +		role_sw_desc.fwnode = dev->fwnode;
> +		role_sw_desc.set = qusb2_role_switch_set;
> +		role_sw_desc.driver_data = qphy;
> +
> +		qphy->connector_role_sw = usb_role_switch_register(dev, &role_sw_desc);
> +		if (IS_ERR(qphy->connector_role_sw)) {
> +			ret = PTR_ERR(qphy->connector_role_sw);
> +			dev_err(dev, "failed to register role switch, %d\n", ret);
> +			return ret;
> +		}
> +	}
> +	qphy->controller_role_sw = NULL;
> +
> +	return 0;
>  }

[Severity: High]
Does PHY_QCOM_QUSB2 in drivers/phy/qualcomm/Kconfig need a dependency on
or selection of USB_ROLE_SWITCH?

With CONFIG_PHY_QCOM_QUSB2=y and CONFIG_USB_ROLE_SWITCH=m, vmlinux fails
to link with undefined references to usb_role_switch_register(),
usb_role_switch_get(), usb_role_switch_set_role(),
usb_role_switch_get_drvdata(), and usb_role_string().  In addition, when
CONFIG_USB_ROLE_SWITCH=n, the usb_role_switch_register() stub returns
ERR_PTR(-ENODEV), failing probe for any QUSB2 PHY node with the
usb-role-switch property.

[Severity: High]
What cleans up connector_role_sw, port_det_w, controller_role_sw, and
usb_psy when the driver is unbound?

qusb2_phy_driver has no .remove callback and registers no devm cleanup
actions for these resources.  Unbinding the device leaves
qphy->connector_role_sw registered and qphy->port_det_w queued or running
after devm frees qphy and unmaps qphy->base (causing a use-after-free),
and leaks the references acquired via usb_role_switch_get() and
power_supply_get_by_name().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-shikra-qusb2-charger-det-v1-0-5a9d919c66b5@oss.qualcomm.com?part=2

Re: [PATCH 2/2] phy: qcom: qusb2: Add charger detection support

[kernel test robot]

Hi Krishna,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4]

url:    https://github.com/intel-lab-lkp/linux/commits/Krishna-Kurapati/dt-bindings-phy-qcom-qusb2-Add-support-for-remote-endpoints/20261111-230126
base:   6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
patch link:    https://lore.kernel.org/r/20261111-shikra-qusb2-charger-det-v1-2-5a9d919c66b5%40oss.qualcomm.com
patch subject: [PATCH 2/2] phy: qcom: qusb2: Add charger detection support
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20261112/202611120841.GECRO9c3-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 16.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261112/202611120841.GECRO9c3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202611120841.GECRO9c3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'is_phy_enabled' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'vbus_active' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'id_state' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'usb_psy' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'port_det_w' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'port_state' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'dcd_timeout' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'dcp_charger' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'connector_role_sw' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'controller_role_sw' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'current_role' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'role_lock' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'is_phy_enabled' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'vbus_active' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'id_state' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'usb_psy' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'port_det_w' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'port_state' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'dcd_timeout' not described in 'qusb2_phy'
>> Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'dcp_charger' not described in 'qusb2_phy'
   Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'connector_role_sw' not described in 'qusb2_phy'
   Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'controller_role_sw' not described in 'qusb2_phy'
   Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'current_role' not described in 'qusb2_phy'
   Warning: drivers/phy/qualcomm/phy-qcom-qusb2.c:552 struct member 'role_lock' not described in 'qusb2_phy'

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki