[PATCH 0/9] Enable IPQ5332 USB2

[PATCH 0/9] Enable IPQ5332 USB2

[Varadarajan Narayanan]

This patch series adds the relevant phy and controller
configurations for enabling USB2 on IPQ5332

Depends On: https://lore.kernel.org/linux-arm-msm/cover.1686045347.git.quic_varada@quicinc.com/

Varadarajan Narayanan (9):
  dt-bindings: usb: dwc3: Add IPQ5332 compatible
  dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy
  phy: qcom-m31: Introduce qcom,m31 USB phy driver
  clk: qcom: ipq5332: Fix USB related clock defines
  phy: qcom-m31: Introduce qcom,m31 USB phy
  phy: qcom: Add qcom,m31 USB phy driver
  arm64: dts: qcom: ipq5332: Add USB related nodes
  arm64: dts: qcom: ipq5332: Enable USB
  arm64: defconfig: Enable QCOM M31 USB phy driver

 .../devicetree/bindings/phy/qcom,m31.yaml          |  69 ++++
 .../devicetree/bindings/usb/qcom,dwc3.yaml         |   2 +
 arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts        |   8 +
 arch/arm64/boot/dts/qcom/ipq5332.dtsi              |  55 ++++
 arch/arm64/configs/defconfig                       |   1 +
 drivers/clk/qcom/gcc-ipq5332.c                     |  34 +-
 drivers/phy/qualcomm/Kconfig                       |  11 +
 drivers/phy/qualcomm/Makefile                      |   1 +
 drivers/phy/qualcomm/phy-qcom-m31.c                | 360 +++++++++++++++++++++
 9 files changed, 530 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
 create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c

-- 
2.7.4

[PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible

[Varadarajan Narayanan]

Document the IPQ5332 dwc3 compatible

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index ae24dac..9c3d6f4 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -14,6 +14,7 @@ properties:
     items:
       - enum:
           - qcom,ipq4019-dwc3
+          - qcom,ipq5332-dwc3
           - qcom,ipq6018-dwc3
           - qcom,ipq8064-dwc3
           - qcom,ipq8074-dwc3
@@ -246,6 +247,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,ipq5332-dwc3
               - qcom,msm8994-dwc3
               - qcom,qcs404-dwc3
     then:
-- 
2.7.4

[PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Varadarajan Narayanan]

Document the M31 USB2 phy present in IPQ5332

Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml

diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
new file mode 100644
index 0000000..8ad4ba4
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm M31 USB PHY
+
+maintainers:
+  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
+  - Varadarajan Narayanan <quic_varada@quicinc.org>
+
+description:
+  This file contains documentation for the USB M31 PHY found in Qualcomm
+  IPQ5018, IPQ5332 SoCs.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - qcom,m31-usb-hsphy
+              - qcom,ipq5332-m31-usb-hsphy
+
+  reg:
+    description:
+      Offset and length of the M31 PHY register set
+    maxItems: 2
+
+  reg-names:
+    items:
+      - const: m31usb_phy_base
+      - const: qscratch_base
+
+  phy_type:
+    oneOf:
+      - items:
+          - enum:
+              - utmi
+              - ulpi
+
+  resets:
+    maxItems: 1
+    description:
+      List of phandles and reset pairs, one for each entry in reset-names.
+
+  reset-names:
+    items:
+      - const:
+          usb2_phy_reset
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+    hs_m31phy_0: hs_m31phy@5b00 {
+        compatible = "qcom,m31-usb-hsphy";
+        reg = <0x5b000 0x3000>,
+            <0x08af8800 0x400>;
+        reg-names = "m31usb_phy_base",
+                "qscratch_base";
+        phy_type = "utmi";
+        resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
+        reset-names = "usb2_phy_reset";
+
+        status = "ok";
+    };
-- 
2.7.4

[PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver

[Varadarajan Narayanan]

Add the M31 USB2 phy driver

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
 1 file changed, 360 insertions(+)
 create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c

diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
new file mode 100644
index 0000000..d29a91e
--- /dev/null
+++ b/drivers/phy/qualcomm/phy-qcom-m31.c
@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/usb/phy.h>
+#include <linux/reset.h>
+#include <linux/of_device.h>
+
+enum clk_reset_action {
+	CLK_RESET_DEASSERT	= 0,
+	CLK_RESET_ASSERT	= 1
+};
+
+#define USB2PHY_PORT_POWERDOWN		0xA4
+#define POWER_UP			BIT(0)
+#define POWER_DOWN			0
+
+#define USB2PHY_PORT_UTMI_CTRL1	0x40
+
+#define USB2PHY_PORT_UTMI_CTRL2	0x44
+#define UTMI_ULPI_SEL			BIT(7)
+#define UTMI_TEST_MUX_SEL		BIT(6)
+
+#define HS_PHY_CTRL_REG			0x10
+#define UTMI_OTG_VBUS_VALID             BIT(20)
+#define SW_SESSVLD_SEL                  BIT(28)
+
+#define USB_PHY_CFG0			0x94
+#define USB_PHY_UTMI_CTRL5		0x50
+#define USB_PHY_FSEL_SEL		0xB8
+#define USB_PHY_HS_PHY_CTRL_COMMON0	0x54
+#define USB_PHY_REFCLK_CTRL		0xA0
+#define USB_PHY_HS_PHY_CTRL2		0x64
+#define USB_PHY_UTMI_CTRL0		0x3c
+#define USB2PHY_USB_PHY_M31_XCFGI_1	0xBC
+#define USB2PHY_USB_PHY_M31_XCFGI_4	0xC8
+#define USB2PHY_USB_PHY_M31_XCFGI_5	0xCC
+#define USB2PHY_USB_PHY_M31_XCFGI_11	0xE4
+
+#define USB2_0_TX_ENABLE		BIT(2)
+#define HSTX_SLEW_RATE_565PS		3
+#define PLL_CHARGING_PUMP_CURRENT_35UA	(3 << 3)
+#define ODT_VALUE_38_02_OHM		(3 << 6)
+#define ODT_VALUE_45_02_OHM		BIT(2)
+#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA	(1)
+
+#define UTMI_PHY_OVERRIDE_EN		BIT(1)
+#define POR_EN				BIT(1)
+#define FREQ_SEL			BIT(0)
+#define COMMONONN			BIT(7)
+#define FSEL				BIT(4)
+#define RETENABLEN			BIT(3)
+#define USB2_SUSPEND_N_SEL		BIT(3)
+#define USB2_SUSPEND_N			BIT(2)
+#define USB2_UTMI_CLK_EN		BIT(1)
+#define CLKCORE				BIT(1)
+#define ATERESET			~BIT(0)
+#define FREQ_24MHZ			(5 << 4)
+#define XCFG_COARSE_TUNE_NUM		(2 << 0)
+#define XCFG_FINE_TUNE_NUM		(1 << 3)
+
+static void m31usb_write_readback(void *base, u32 offset,
+					const u32 mask, u32 val);
+
+struct m31usb_phy {
+	struct usb_phy		phy;
+	void __iomem		*base;
+	void __iomem		*qscratch_base;
+
+	struct reset_control	*phy_reset;
+
+	bool			cable_connected;
+	bool			suspended;
+	bool			ulpi_mode;
+};
+
+static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
+{
+	if (action == CLK_RESET_ASSERT)
+		reset_control_assert(qphy->phy_reset);
+	else
+		reset_control_deassert(qphy->phy_reset);
+	wmb(); /* ensure data is written to hw register */
+}
+
+static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
+{
+	/* Enable override ctrl */
+	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
+	/* Enable POR*/
+	writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
+	udelay(15);
+	/* Configure frequency select value*/
+	writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
+	/* Configure refclk frequency */
+	writel(COMMONONN | FSEL | RETENABLEN,
+		qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
+	/* select refclk source */
+	writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL);
+	/* select ATERESET*/
+	writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
+	writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+		qphy->base + USB_PHY_HS_PHY_CTRL2);
+	writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
+	writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+		qphy->base + USB_PHY_HS_PHY_CTRL2);
+	/* Disable override ctrl */
+	writel(0x0, qphy->base + USB_PHY_CFG0);
+}
+
+static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy)
+{
+	/* Enable override ctrl */
+	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
+	/* Enable POR*/
+	writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
+	udelay(15);
+	/* Configure frequency select value*/
+	writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
+	/* Configure refclk frequency */
+	writel(COMMONONN | FREQ_24MHZ | RETENABLEN,
+		qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
+	/* select ATERESET*/
+	writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
+	writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+		qphy->base + USB_PHY_HS_PHY_CTRL2);
+	writel(XCFG_COARSE_TUNE_NUM  | XCFG_FINE_TUNE_NUM,
+				qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11);
+	/* Adjust HSTX slew rate to 565 ps*/
+	/* Adjust PLL lock Time counter for release clock to 35uA */
+	/* Adjust Manual control ODT value to 38.02 Ohm */
+	writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA |
+		ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4);
+	/*
+	 * Enable to always turn on USB 2.0 TX driver
+	 * when system is in USB 2.0 HS mode
+	 */
+	writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1);
+	/* Adjust Manual control ODT value to 45.02 Ohm */
+	/* Adjust HSTX Pre-emphasis level to 0.55mA */
+	writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA,
+		qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5);
+	udelay(4);
+	writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
+	writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
+		qphy->base + USB_PHY_HS_PHY_CTRL2);
+}
+
+static int m31usb_phy_init(struct usb_phy *phy)
+{
+	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+	/* Perform phy reset */
+	m31usb_reset(qphy, CLK_RESET_ASSERT);
+	usleep_range(1, 5);
+	m31usb_reset(qphy, CLK_RESET_DEASSERT);
+
+	/* configure for ULPI mode if requested */
+	if (qphy->ulpi_mode)
+		writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2);
+
+	/* Enable the PHY */
+	writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN);
+
+	/* Make sure above write completed */
+	wmb();
+
+	/* Turn on phy ref clock*/
+	if (of_device_is_compatible(phy->dev->of_node,
+					"qcom,ipq5332-m31-usb-hsphy"))
+		ipq5332_m31usb_phy_enable_clock(qphy);
+	else
+		m31usb_phy_enable_clock(qphy);
+
+	/* Set OTG VBUS Valid from HSPHY to controller */
+	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+				UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID);
+	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
+	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+				SW_SESSVLD_SEL, SW_SESSVLD_SEL);
+
+	return 0;
+}
+
+static void m31usb_phy_shutdown(struct usb_phy *phy)
+{
+	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+	/* Disable the PHY */
+	writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN);
+	/* Make sure above write completed */
+	wmb();
+}
+
+static void m31usb_write_readback(void *base, u32 offset,
+					const u32 mask, u32 val)
+{
+	u32 write_val, tmp = readl_relaxed(base + offset);
+
+	tmp &= ~mask; /* retain other bits */
+	write_val = tmp | val;
+
+	writel_relaxed(write_val, base + offset);
+	/* Make sure above write completed */
+	wmb();
+
+	/* Read back to see if val was written */
+	tmp = readl_relaxed(base + offset);
+	tmp &= mask; /* clear other bits */
+
+	if (tmp != val)
+		pr_err("%s: write: %x to QSCRATCH: %x FAILED\n",
+			__func__, val, offset);
+}
+
+static int m31usb_phy_notify_connect(struct usb_phy *phy,
+					enum usb_device_speed speed)
+{
+	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+	qphy->cable_connected = true;
+
+	dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
+
+	/* Set OTG VBUS Valid from HSPHY to controller */
+	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+				UTMI_OTG_VBUS_VALID,
+				UTMI_OTG_VBUS_VALID);
+
+	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
+	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+				SW_SESSVLD_SEL, SW_SESSVLD_SEL);
+
+	dev_dbg(phy->dev, "M31USB2 phy connect notification\n");
+	return 0;
+}
+
+static int m31usb_phy_notify_disconnect(struct usb_phy *phy,
+					enum usb_device_speed speed)
+{
+	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
+
+	qphy->cable_connected = false;
+
+	dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
+
+	/* Set OTG VBUS Valid from HSPHY to controller */
+	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+				UTMI_OTG_VBUS_VALID, 0);
+
+	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
+	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
+				SW_SESSVLD_SEL, 0);
+
+	dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n");
+	return 0;
+}
+
+static int m31usb_phy_probe(struct platform_device *pdev)
+{
+	struct m31usb_phy *qphy;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	int ret;
+	const char *phy_type;
+
+	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
+	if (!qphy)
+		return -ENOMEM;
+
+	qphy->phy.dev = dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+							"m31usb_phy_base");
+	qphy->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(qphy->base))
+		return PTR_ERR(qphy->base);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+							"qscratch_base");
+	if (res) {
+		qphy->qscratch_base = devm_ioremap(dev, res->start,
+							resource_size(res));
+		if (IS_ERR(qphy->qscratch_base)) {
+			dev_dbg(dev, "couldn't ioremap qscratch_base\n");
+			qphy->qscratch_base = NULL;
+		}
+	}
+
+	qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset");
+	if (IS_ERR(qphy->phy_reset))
+		return PTR_ERR(qphy->phy_reset);
+
+	qphy->ulpi_mode = false;
+	ret = of_property_read_string(dev->of_node, "phy_type", &phy_type);
+
+	if (!ret) {
+		if (!strcasecmp(phy_type, "ulpi"))
+			qphy->ulpi_mode = true;
+	} else {
+		dev_err(dev, "error reading phy_type property\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, qphy);
+
+	qphy->phy.label			= "qcom-m31usb-phy";
+	qphy->phy.init			= m31usb_phy_init;
+	qphy->phy.shutdown		= m31usb_phy_shutdown;
+	qphy->phy.type			= USB_PHY_TYPE_USB2;
+
+	if (qphy->qscratch_base) {
+		qphy->phy.notify_connect        = m31usb_phy_notify_connect;
+		qphy->phy.notify_disconnect     = m31usb_phy_notify_disconnect;
+	}
+
+	ret = usb_add_phy_dev(&qphy->phy);
+
+	return ret;
+}
+
+static int m31usb_phy_remove(struct platform_device *pdev)
+{
+	struct m31usb_phy *qphy = platform_get_drvdata(pdev);
+
+	usb_remove_phy(&qphy->phy);
+
+	return 0;
+}
+
+static const struct of_device_id m31usb_phy_id_table[] = {
+	{ .compatible = "qcom,m31-usb-hsphy",},
+	{ .compatible = "qcom,ipq5332-m31-usb-hsphy",},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, m31usb_phy_id_table);
+
+static struct platform_driver m31usb_phy_driver = {
+	.probe		= m31usb_phy_probe,
+	.remove		= m31usb_phy_remove,
+	.driver = {
+		.name	= "qcom-m31usb-phy",
+		.of_match_table = of_match_ptr(m31usb_phy_id_table),
+	},
+};
+
+module_platform_driver(m31usb_phy_driver);
+
+MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines

[Varadarajan Narayanan]

Fix the USB related clock defines and add details
referenced by them

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 drivers/clk/qcom/gcc-ipq5332.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
index a75ab88..2b58558 100644
--- a/drivers/clk/qcom/gcc-ipq5332.c
+++ b/drivers/clk/qcom/gcc-ipq5332.c
@@ -351,6 +351,16 @@ static const struct freq_tbl ftbl_gcc_adss_pwm_clk_src[] = {
 	{ }
 };
 
+static const struct clk_parent_data gcc_usb3phy_0_cc_pipe_clk_xo[] = {
+	{ .fw_name = "usb3phy_0_cc_pipe_clk" },
+	{ .fw_name = "xo" },
+};
+
+static const struct parent_map gcc_usb3phy_0_cc_pipe_clk_xo_map[] = {
+	{ P_USB3PHY_0_PIPE, 0 },
+	{ P_XO, 2 },
+};
+
 static struct clk_rcg2 gcc_adss_pwm_clk_src = {
 	.cmd_rcgr = 0x1c004,
 	.mnd_width = 0,
@@ -1101,16 +1111,18 @@ static struct clk_rcg2 gcc_usb0_mock_utmi_clk_src = {
 	},
 };
 
-static struct clk_regmap_phy_mux gcc_usb0_pipe_clk_src = {
+static struct clk_regmap_mux usb0_pipe_clk_src = {
 	.reg = 0x2c074,
+	.shift = 8,
+	.width = 2,
+	.parent_map = gcc_usb3phy_0_cc_pipe_clk_xo_map,
 	.clkr = {
-		.hw.init = &(struct clk_init_data) {
-			.name = "gcc_usb0_pipe_clk_src",
-			.parent_data = &(const struct clk_parent_data) {
-				.index = DT_USB_PCIE_WRAPPER_PIPE_CLK,
-			},
-			.num_parents = 1,
-			.ops = &clk_regmap_phy_mux_ops,
+		.hw.init = &(const struct clk_init_data){
+			.name = "usb0phy_0_cc_pipe_clk_src",
+			.parent_data = gcc_usb3phy_0_cc_pipe_clk_xo,
+			.num_parents = 2,
+			.ops = &clk_regmap_mux_closest_ops,
+			.flags = CLK_SET_RATE_PARENT,
 		},
 	},
 };
@@ -3041,8 +3053,8 @@ static struct clk_branch gcc_usb0_pipe_clk = {
 		.enable_mask = BIT(0),
 		.hw.init = &(const struct clk_init_data) {
 			.name = "gcc_usb0_pipe_clk",
-			.parent_hws = (const struct clk_hw*[]) {
-				&gcc_usb0_pipe_clk_src.clkr.hw,
+			.parent_names = (const char *[]){
+				"usb0_pipe_clk_src"
 			},
 			.num_parents = 1,
 			.flags = CLK_SET_RATE_PARENT,
@@ -3580,7 +3592,7 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = {
 	[GCC_PCIE3X2_PIPE_CLK_SRC] = &gcc_pcie3x2_pipe_clk_src.clkr,
 	[GCC_PCIE3X1_0_PIPE_CLK_SRC] = &gcc_pcie3x1_0_pipe_clk_src.clkr,
 	[GCC_PCIE3X1_1_PIPE_CLK_SRC] = &gcc_pcie3x1_1_pipe_clk_src.clkr,
-	[GCC_USB0_PIPE_CLK_SRC] = &gcc_usb0_pipe_clk_src.clkr,
+	[GCC_USB0_PIPE_CLK_SRC] = &usb0_pipe_clk_src.clkr,
 };
 
 static const struct qcom_reset_map gcc_ipq5332_resets[] = {
-- 
2.7.4

[PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy

[Varadarajan Narayanan]

Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 drivers/phy/qualcomm/Kconfig | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index 67a45d9..8a363dd 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -188,3 +188,14 @@ config PHY_QCOM_IPQ806X_USB
 	  This option enables support for the Synopsis PHYs present inside the
 	  Qualcomm USB3.0 DWC3 controller on ipq806x SoC. This driver supports
 	  both HS and SS PHY controllers.
+
+config PHY_QCOM_M31_USB
+	tristate "Qualcomm M31 HS PHY driver support"
+	depends on (USB || USB_GADGET) && ARCH_QCOM
+	select USB_PHY
+	help
+	  Enable this to support M31 HS PHY transceivers on Qualcomm chips
+	  with DWC3 USB core. It handles PHY initialization, clock
+	  management required after resetting the hardware and power
+	  management. This driver is required even for peripheral only or
+	  host only mode configurations.
-- 
2.7.4

[PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver

[Varadarajan Narayanan]

Include M31 phy driver if CONFIG_PHY_QCOM_M31_USB is enabled

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 drivers/phy/qualcomm/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
index de3dc9c..79a6e75 100644
--- a/drivers/phy/qualcomm/Makefile
+++ b/drivers/phy/qualcomm/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_PHY_QCOM_USB_HS_28NM)	+= phy-qcom-usb-hs-28nm.o
 obj-$(CONFIG_PHY_QCOM_USB_SS)		+= phy-qcom-usb-ss.o
 obj-$(CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2)+= phy-qcom-snps-femto-v2.o
 obj-$(CONFIG_PHY_QCOM_IPQ806X_USB)		+= phy-qcom-ipq806x-usb.o
+obj-$(CONFIG_PHY_QCOM_M31_USB)		+= phy-qcom-m31.o
-- 
2.7.4

[PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes

[Varadarajan Narayanan]

Add USB phy and controller nodes

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index c2d6cc65..3183357 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -383,6 +383,61 @@
 				status = "disabled";
 			};
 		};
+
+		usb_0_m31phy: hs_m31phy@7b000 {
+			compatible = "qcom,ipq5332-m31-usb-hsphy";
+			reg = <0x0007b000 0x12C>,
+			      <0x08af8800 0x400>;
+			reg-names = "m31usb_phy_base",
+				    "qscratch_base";
+			phy_type= "utmi";
+
+			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
+			reset-names = "usb2_phy_reset";
+
+			status = "okay";
+		};
+
+		usb2: usb2@8a00000 {
+			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			reg = <0x08af8800 0x100>;
+
+			clocks = <&gcc GCC_USB0_MASTER_CLK>,
+				<&gcc GCC_SNOC_USB_CLK>,
+				<&gcc GCC_USB0_SLEEP_CLK>,
+				<&gcc GCC_USB0_MOCK_UTMI_CLK>;
+
+			clock-names = "core",
+				"iface",
+				"sleep",
+				"mock_utmi";
+
+			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "pwr_event";
+
+			resets = <&gcc GCC_USB_BCR>;
+
+			qcom,select-utmi-as-pipe-clk;
+
+			usb2_0_dwc: usb@8a00000 {
+				compatible = "snps,dwc3";
+				reg = <0x08a00000 0xe000>;
+				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
+				clock-names = "ref";
+				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+				usb-phy = <&usb_0_m31phy>;
+				tx-fifo-resize;
+				snps,is-utmi-l1-suspend;
+				snps,hird-threshold = /bits/ 8 <0x0>;
+				snps,dis_u2_susphy_quirk;
+				snps,dis_u3_susphy_quirk;
+				snps,ref-clock-period-ns = <21>;
+			};
+		};
 	};
 
 	timer {
-- 
2.7.4

[PATCH 8/9] arm64: dts: qcom: ipq5332: Enable USB

[Varadarajan Narayanan]

Enable USB2 in host mode

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts b/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts
index 3b6a5cb..303e796 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts
+++ b/arch/arm64/boot/dts/qcom/ipq5332-rdp468.dts
@@ -101,3 +101,11 @@
 		bias-pull-up;
 	};
 };
+
+&usb2 {
+	status = "okay";
+};
+
+&usb2_0_dwc {
+	dr_mode = "host";
+};
-- 
2.7.4

[PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver

[Varadarajan Narayanan]

Enable QCOM M31 USB phy driver present in IPQ5332

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index afb63e3..f1e8669 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1362,6 +1362,7 @@ CONFIG_PHY_QCOM_USB_HS=m
 CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2=m
 CONFIG_PHY_QCOM_USB_HS_28NM=m
 CONFIG_PHY_QCOM_USB_SS=m
+CONFIG_PHY_QCOM_M31_USB=m
 CONFIG_PHY_R8A779F0_ETHERNET_SERDES=y
 CONFIG_PHY_RCAR_GEN3_PCIE=y
 CONFIG_PHY_RCAR_GEN3_USB2=y
-- 
2.7.4

Re: [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines

[Dmitry Baryshkov]

On 07/06/2023 13:56, Varadarajan Narayanan wrote:
> Fix the USB related clock defines and add details
> referenced by them
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>   drivers/clk/qcom/gcc-ipq5332.c | 34 +++++++++++++++++++++++-----------
>   1 file changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
> index a75ab88..2b58558 100644
> --- a/drivers/clk/qcom/gcc-ipq5332.c
> +++ b/drivers/clk/qcom/gcc-ipq5332.c
> @@ -351,6 +351,16 @@ static const struct freq_tbl ftbl_gcc_adss_pwm_clk_src[] = {
>   	{ }
>   };
>   
> +static const struct clk_parent_data gcc_usb3phy_0_cc_pipe_clk_xo[] = {
> +	{ .fw_name = "usb3phy_0_cc_pipe_clk" },
> +	{ .fw_name = "xo" },

gcc-ipq5332 uses DT indices, please don't mix that with .fw_name.

> +};
> +
> +static const struct parent_map gcc_usb3phy_0_cc_pipe_clk_xo_map[] = {
> +	{ P_USB3PHY_0_PIPE, 0 },
> +	{ P_XO, 2 },
> +};
> +
>   static struct clk_rcg2 gcc_adss_pwm_clk_src = {
>   	.cmd_rcgr = 0x1c004,
>   	.mnd_width = 0,
> @@ -1101,16 +1111,18 @@ static struct clk_rcg2 gcc_usb0_mock_utmi_clk_src = {
>   	},
>   };
>   
> -static struct clk_regmap_phy_mux gcc_usb0_pipe_clk_src = {
> +static struct clk_regmap_mux usb0_pipe_clk_src = {
>   	.reg = 0x2c074,
> +	.shift = 8,
> +	.width = 2,
> +	.parent_map = gcc_usb3phy_0_cc_pipe_clk_xo_map,
>   	.clkr = {
> -		.hw.init = &(struct clk_init_data) {
> -			.name = "gcc_usb0_pipe_clk_src",
> -			.parent_data = &(const struct clk_parent_data) {
> -				.index = DT_USB_PCIE_WRAPPER_PIPE_CLK,
> -			},
> -			.num_parents = 1,
> -			.ops = &clk_regmap_phy_mux_ops,
> +		.hw.init = &(const struct clk_init_data){
> +			.name = "usb0phy_0_cc_pipe_clk_src",
> +			.parent_data = gcc_usb3phy_0_cc_pipe_clk_xo,
> +			.num_parents = 2,
> +			.ops = &clk_regmap_mux_closest_ops,
> +			.flags = CLK_SET_RATE_PARENT,
>   		},

Soo... As you are reverting this. Is USB0 PIPE clock required to be 
parked to the XO? I was going to write 'before turning USB0_GDSC' off, 
but then I noticed that gcc-ipq5332 doesn't declare GDSCs. Does this 
platform have GDSCs?

>   	},
>   };
> @@ -3041,8 +3053,8 @@ static struct clk_branch gcc_usb0_pipe_clk = {
>   		.enable_mask = BIT(0),
>   		.hw.init = &(const struct clk_init_data) {
>   			.name = "gcc_usb0_pipe_clk",
> -			.parent_hws = (const struct clk_hw*[]) {
> -				&gcc_usb0_pipe_clk_src.clkr.hw,
> +			.parent_names = (const char *[]){
> +				"usb0_pipe_clk_src"

complete and definitive NAK. Do not use parent_names, we have just 
stopped migrating from them.

>   			},
>   			.num_parents = 1,
>   			.flags = CLK_SET_RATE_PARENT,
> @@ -3580,7 +3592,7 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = {
>   	[GCC_PCIE3X2_PIPE_CLK_SRC] = &gcc_pcie3x2_pipe_clk_src.clkr,
>   	[GCC_PCIE3X1_0_PIPE_CLK_SRC] = &gcc_pcie3x1_0_pipe_clk_src.clkr,
>   	[GCC_PCIE3X1_1_PIPE_CLK_SRC] = &gcc_pcie3x1_1_pipe_clk_src.clkr,
> -	[GCC_USB0_PIPE_CLK_SRC] = &gcc_usb0_pipe_clk_src.clkr,
> +	[GCC_USB0_PIPE_CLK_SRC] = &usb0_pipe_clk_src.clkr,
>   };
>   
>   static const struct qcom_reset_map gcc_ipq5332_resets[] = {

-- 
With best wishes
Dmitry

Re: [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy

[Dmitry Baryshkov]

On 07/06/2023 13:56, Varadarajan Narayanan wrote:
> Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>

Is there any reason to keep Kconfig, Makefile and driver in different 
commits?

> ---
>   drivers/phy/qualcomm/Kconfig | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index 67a45d9..8a363dd 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -188,3 +188,14 @@ config PHY_QCOM_IPQ806X_USB
>   	  This option enables support for the Synopsis PHYs present inside the
>   	  Qualcomm USB3.0 DWC3 controller on ipq806x SoC. This driver supports
>   	  both HS and SS PHY controllers.
> +
> +config PHY_QCOM_M31_USB
> +	tristate "Qualcomm M31 HS PHY driver support"
> +	depends on (USB || USB_GADGET) && ARCH_QCOM
> +	select USB_PHY
> +	help
> +	  Enable this to support M31 HS PHY transceivers on Qualcomm chips
> +	  with DWC3 USB core. It handles PHY initialization, clock
> +	  management required after resetting the hardware and power
> +	  management. This driver is required even for peripheral only or
> +	  host only mode configurations.

-- 
With best wishes
Dmitry

Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes

[Dmitry Baryshkov]

On 07/06/2023 13:56, Varadarajan Narayanan wrote:
> Add USB phy and controller nodes
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index c2d6cc65..3183357 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -383,6 +383,61 @@
>   				status = "disabled";
>   			};
>   		};
> +
> +		usb_0_m31phy: hs_m31phy@7b000 {
> +			compatible = "qcom,ipq5332-m31-usb-hsphy";
> +			reg = <0x0007b000 0x12C>,
> +			      <0x08af8800 0x400>;
> +			reg-names = "m31usb_phy_base",
> +				    "qscratch_base";
> +			phy_type= "utmi";
> +
> +			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> +			reset-names = "usb2_phy_reset";
> +
> +			status = "okay";
> +		};
> +
> +		usb2: usb2@8a00000 {
> +			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			reg = <0x08af8800 0x100>;
> +
> +			clocks = <&gcc GCC_USB0_MASTER_CLK>,
> +				<&gcc GCC_SNOC_USB_CLK>,
> +				<&gcc GCC_USB0_SLEEP_CLK>,
> +				<&gcc GCC_USB0_MOCK_UTMI_CLK>;

Please indent these values.

> +
> +			clock-names = "core",
> +				"iface",
> +				"sleep",
> +				"mock_utmi";

Please indent these values.

> +
> +			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;

No PHY IRQs?

> +			interrupt-names = "pwr_event";
> +
> +			resets = <&gcc GCC_USB_BCR>;
> +
> +			qcom,select-utmi-as-pipe-clk;
> +
> +			usb2_0_dwc: usb@8a00000 {
> +				compatible = "snps,dwc3";
> +				reg = <0x08a00000 0xe000>;
> +				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> +				clock-names = "ref";
> +				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +				usb-phy = <&usb_0_m31phy>;
> +				tx-fifo-resize;
> +				snps,is-utmi-l1-suspend;
> +				snps,hird-threshold = /bits/ 8 <0x0>;
> +				snps,dis_u2_susphy_quirk;
> +				snps,dis_u3_susphy_quirk;
> +				snps,ref-clock-period-ns = <21>;
> +			};
> +		};
>   	};
>   
>   	timer {

-- 
With best wishes
Dmitry

Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Rob Herring]

On Wed, 07 Jun 2023 16:26:06 +0530, Varadarajan Narayanan wrote:
> Document the M31 USB2 phy present in IPQ5332
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,m31.example.dtb: hs_m31phy@5b00: status:0: 'ok' is not one of ['okay', 'disabled', 'reserved']
	From schema: /usr/local/lib/python3.10/dist-packages/dtschema/schemas/dt-core.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/14f60578e2935c0844537eab162af3afa52ffe39.1686126439.git.quic_varada@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver

[Konrad Dybcio]

On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> Add the M31 USB2 phy driver
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 360 insertions(+)
>  create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> new file mode 100644
> index 0000000..d29a91e
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> @@ -0,0 +1,360 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/phy.h>
> +#include <linux/reset.h>
> +#include <linux/of_device.h>
Please sort these

> +
> +enum clk_reset_action {
> +	CLK_RESET_DEASSERT	= 0,
> +	CLK_RESET_ASSERT	= 1
> +};
> +
> +#define USB2PHY_PORT_POWERDOWN		0xA4
> +#define POWER_UP			BIT(0)
> +#define POWER_DOWN			0
> +
> +#define USB2PHY_PORT_UTMI_CTRL1	0x40
> +
> +#define USB2PHY_PORT_UTMI_CTRL2	0x44
> +#define UTMI_ULPI_SEL			BIT(7)
> +#define UTMI_TEST_MUX_SEL		BIT(6)
> +
> +#define HS_PHY_CTRL_REG			0x10
> +#define UTMI_OTG_VBUS_VALID             BIT(20)
> +#define SW_SESSVLD_SEL                  BIT(28)
> +
> +#define USB_PHY_CFG0			0x94
> +#define USB_PHY_UTMI_CTRL5		0x50
> +#define USB_PHY_FSEL_SEL		0xB8
> +#define USB_PHY_HS_PHY_CTRL_COMMON0	0x54
> +#define USB_PHY_REFCLK_CTRL		0xA0
> +#define USB_PHY_HS_PHY_CTRL2		0x64
> +#define USB_PHY_UTMI_CTRL0		0x3c
> +#define USB2PHY_USB_PHY_M31_XCFGI_1	0xBC
> +#define USB2PHY_USB_PHY_M31_XCFGI_4	0xC8
> +#define USB2PHY_USB_PHY_M31_XCFGI_5	0xCC
> +#define USB2PHY_USB_PHY_M31_XCFGI_11	0xE4
Could you sort them address-wise?

> +
> +#define USB2_0_TX_ENABLE		BIT(2)
> +#define HSTX_SLEW_RATE_565PS		3
> +#define PLL_CHARGING_PUMP_CURRENT_35UA	(3 << 3)
> +#define ODT_VALUE_38_02_OHM		(3 << 6)
> +#define ODT_VALUE_45_02_OHM		BIT(2)
> +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA	(1)
Weird mix of values, bits, bitfields.. perhaps BIT(n) and
GENMASK() (+ FIELD_PREP) would be more suitable?

> +
> +#define UTMI_PHY_OVERRIDE_EN		BIT(1)
> +#define POR_EN				BIT(1)
Please associate these with their registers, like

#define FOO_REG		0xf00
 #define POR_EN		BIT(1)

> +#define FREQ_SEL			BIT(0)
> +#define COMMONONN			BIT(7)
> +#define FSEL				BIT(4)
> +#define RETENABLEN			BIT(3)
> +#define USB2_SUSPEND_N_SEL		BIT(3)
> +#define USB2_SUSPEND_N			BIT(2)
> +#define USB2_UTMI_CLK_EN		BIT(1)
> +#define CLKCORE				BIT(1)
> +#define ATERESET			~BIT(0)
> +#define FREQ_24MHZ			(5 << 4)
> +#define XCFG_COARSE_TUNE_NUM		(2 << 0)
> +#define XCFG_FINE_TUNE_NUM		(1 << 3)
same comment

> +
> +static void m31usb_write_readback(void *base, u32 offset,
> +					const u32 mask, u32 val);
We don't need this forward-definition, just move the function up.

> +
> +struct m31usb_phy {
> +	struct usb_phy		phy;
> +	void __iomem		*base;
> +	void __iomem		*qscratch_base;
> +
> +	struct reset_control	*phy_reset;
> +
> +	bool			cable_connected;
> +	bool			suspended;
> +	bool			ulpi_mode;
> +};
> +
> +static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
> +{
> +	if (action == CLK_RESET_ASSERT)
> +		reset_control_assert(qphy->phy_reset);
> +	else
> +		reset_control_deassert(qphy->phy_reset);
> +	wmb(); /* ensure data is written to hw register */
Please move the comment above the call.

> +}
> +
> +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> +{
> +	/* Enable override ctrl */
> +	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
Some of the comments are missing a space before '*/'

Also, please consider adding some newlines to logically split the
actions.

> +	/* Enable POR*/
> +	writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> +	udelay(15);
> +	/* Configure frequency select value*/
> +	writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> +	/* Configure refclk frequency */
> +	writel(COMMONONN | FSEL | RETENABLEN,
> +		qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> +	/* select refclk source */
> +	writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL);
> +	/* select ATERESET*/
> +	writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> +	writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> +	writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> +	writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> +	/* Disable override ctrl */
> +	writel(0x0, qphy->base + USB_PHY_CFG0);
> +}
> +
> +static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> +{
> +	/* Enable override ctrl */
> +	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> +	/* Enable POR*/
ditto

> +	writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> +	udelay(15);
> +	/* Configure frequency select value*/
> +	writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> +	/* Configure refclk frequency */
> +	writel(COMMONONN | FREQ_24MHZ | RETENABLEN,
> +		qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> +	/* select ATERESET*/
> +	writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> +	writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> +	writel(XCFG_COARSE_TUNE_NUM  | XCFG_FINE_TUNE_NUM,
> +				qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11);
> +	/* Adjust HSTX slew rate to 565 ps*/
> +	/* Adjust PLL lock Time counter for release clock to 35uA */
> +	/* Adjust Manual control ODT value to 38.02 Ohm */
> +	writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA |
> +		ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4);
These functions seem very similar, with the main difference being that
IPQ5332 adds some more writes at the end. Perhaps some commonizing could
be done?

> +	/*
> +	 * Enable to always turn on USB 2.0 TX driver
> +	 * when system is in USB 2.0 HS mode
> +	 */
> +	writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1);
> +	/* Adjust Manual control ODT value to 45.02 Ohm */
> +	/* Adjust HSTX Pre-emphasis level to 0.55mA */
> +	writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA,
> +		qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5);
> +	udelay(4);
> +	writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> +	writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> +}
> +
> +static int m31usb_phy_init(struct usb_phy *phy)
> +{
> +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> +	/* Perform phy reset */
> +	m31usb_reset(qphy, CLK_RESET_ASSERT);
> +	usleep_range(1, 5);
https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

this may not work as intended

> +	m31usb_reset(qphy, CLK_RESET_DEASSERT);
> +
> +	/* configure for ULPI mode if requested */
> +	if (qphy->ulpi_mode)
> +		writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2);
> +
> +	/* Enable the PHY */
> +	writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN);
> +
> +	/* Make sure above write completed */
> +	wmb();
As you're calling wmb in the reset func, dropping _relaxed from the
ULPI and PORT_POWERDOWN writes should work the same

> +
> +	/* Turn on phy ref clock*/
> +	if (of_device_is_compatible(phy->dev->of_node,
> +					"qcom,ipq5332-m31-usb-hsphy"))
> +		ipq5332_m31usb_phy_enable_clock(qphy);
> +	else
> +		m31usb_phy_enable_clock(qphy);
This should be done using OF match data.

> +
> +	/* Set OTG VBUS Valid from HSPHY to controller */
> +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> +				UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID);
> +	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> +				SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> +
> +	return 0;
> +}
> +
> +static void m31usb_phy_shutdown(struct usb_phy *phy)
> +{
> +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> +	/* Disable the PHY */
> +	writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN);
> +	/* Make sure above write completed */
> +	wmb();
> +}
> +
> +static void m31usb_write_readback(void *base, u32 offset,
> +					const u32 mask, u32 val)
The indentation here makes `const u32..` misaligned.

> +{
> +	u32 write_val, tmp = readl_relaxed(base + offset);
> +
> +	tmp &= ~mask; /* retain other bits */
> +	write_val = tmp | val;
> +
> +	writel_relaxed(write_val, base + offset);
> +	/* Make sure above write completed */
> +	wmb();
> +
> +	/* Read back to see if val was written */
> +	tmp = readl_relaxed(base + offset);
> +	tmp &= mask; /* clear other bits */
> +
> +	if (tmp != val)
> +		pr_err("%s: write: %x to QSCRATCH: %x FAILED\n",
> +			__func__, val, offset);
> +}
> +
> +static int m31usb_phy_notify_connect(struct usb_phy *phy,
> +					enum usb_device_speed speed)
> +{
> +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> +	qphy->cable_connected = true;
> +
> +	dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
spurious space at the beginning of the string

> +
> +	/* Set OTG VBUS Valid from HSPHY to controller */
> +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> +				UTMI_OTG_VBUS_VALID,
> +				UTMI_OTG_VBUS_VALID);
Please align the lines with the previous opening bracket

> +
> +	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> +				SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> +
> +	dev_dbg(phy->dev, "M31USB2 phy connect notification\n");
> +	return 0;
> +}
> +
> +static int m31usb_phy_notify_disconnect(struct usb_phy *phy,
> +					enum usb_device_speed speed)
> +{
> +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> +
> +	qphy->cable_connected = false;
> +
> +	dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
> +
> +	/* Set OTG VBUS Valid from HSPHY to controller */
> +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> +				UTMI_OTG_VBUS_VALID, 0);
> +
> +	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> +				SW_SESSVLD_SEL, 0);
> +
> +	dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n");
> +	return 0;
> +}
> +
> +static int m31usb_phy_probe(struct platform_device *pdev)
> +{
> +	struct m31usb_phy *qphy;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int ret;
> +	const char *phy_type;
Please sort these in a reverse-Christmas-tree order.

> +
> +	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> +	if (!qphy)
> +		return -ENOMEM;
> +
> +	qphy->phy.dev = dev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +							"m31usb_phy_base");
> +	qphy->base = devm_ioremap_resource(dev, res);
devm_platform_get_and_ioremap_resource?

> +	if (IS_ERR(qphy->base))
> +		return PTR_ERR(qphy->base);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +							"qscratch_base");
> +	if (res) {
Do we expect platforms without this register space?

> +		qphy->qscratch_base = devm_ioremap(dev, res->start,
> +							resource_size(res));
> +		if (IS_ERR(qphy->qscratch_base)) {
> +			dev_dbg(dev, "couldn't ioremap qscratch_base\n");
> +			qphy->qscratch_base = NULL;
> +		}
> +	}
> +
> +	qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset");
not _exclusive?

> +	if (IS_ERR(qphy->phy_reset))
> +		return PTR_ERR(qphy->phy_reset);
> +
> +	qphy->ulpi_mode = false;
> +	ret = of_property_read_string(dev->of_node, "phy_type", &phy_type);
of_usb_get_phy_mode?

> +
> +	if (!ret) {
> +		if (!strcasecmp(phy_type, "ulpi"))
> +			qphy->ulpi_mode = true;
> +	} else {
> +		dev_err(dev, "error reading phy_type property\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, qphy);
> +
> +	qphy->phy.label			= "qcom-m31usb-phy";
> +	qphy->phy.init			= m31usb_phy_init;
> +	qphy->phy.shutdown		= m31usb_phy_shutdown;
> +	qphy->phy.type			= USB_PHY_TYPE_USB2;
> +
> +	if (qphy->qscratch_base) {
> +		qphy->phy.notify_connect        = m31usb_phy_notify_connect;
> +		qphy->phy.notify_disconnect     = m31usb_phy_notify_disconnect;
> +	}
> +
> +	ret = usb_add_phy_dev(&qphy->phy);
> +
> +	return ret;
> +}
> +
> +static int m31usb_phy_remove(struct platform_device *pdev)
Please make this return void and pass it to .remove_new

> +{
> +	struct m31usb_phy *qphy = platform_get_drvdata(pdev);
> +
> +	usb_remove_phy(&qphy->phy);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id m31usb_phy_id_table[] = {
> +	{ .compatible = "qcom,m31-usb-hsphy",},
> +	{ .compatible = "qcom,ipq5332-m31-usb-hsphy",},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, m31usb_phy_id_table);
> +
> +static struct platform_driver m31usb_phy_driver = {
> +	.probe		= m31usb_phy_probe,
> +	.remove		= m31usb_phy_remove,
> +	.driver = {
> +		.name	= "qcom-m31usb-phy",
> +		.of_match_table = of_match_ptr(m31usb_phy_id_table),
of_match_ptr is unnecessary, this driver depends on OF.

Konrad
> +	},
> +};
> +
> +module_platform_driver(m31usb_phy_driver);
> +
> +MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver

[Dmitry Baryshkov]

Two minor nits on top of the review:

On 07/06/2023 14:54, Konrad Dybcio wrote:
> On 7.06.2023 12:56, Varadarajan Narayanan wrote:
>> Add the M31 USB2 phy driver
>>
>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 360 insertions(+)
>>   create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
>> new file mode 100644
>> index 0000000..d29a91e
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
>> @@ -0,0 +1,360 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/err.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/usb/phy.h>
>> +#include <linux/reset.h>
>> +#include <linux/of_device.h>
> Please sort these
> 
>> +
>> +enum clk_reset_action {
>> +	CLK_RESET_DEASSERT	= 0,
>> +	CLK_RESET_ASSERT	= 1
>> +};
>> +
>> +#define USB2PHY_PORT_POWERDOWN		0xA4
>> +#define POWER_UP			BIT(0)
>> +#define POWER_DOWN			0
>> +
>> +#define USB2PHY_PORT_UTMI_CTRL1	0x40
>> +
>> +#define USB2PHY_PORT_UTMI_CTRL2	0x44
>> +#define UTMI_ULPI_SEL			BIT(7)
>> +#define UTMI_TEST_MUX_SEL		BIT(6)
>> +
>> +#define HS_PHY_CTRL_REG			0x10
>> +#define UTMI_OTG_VBUS_VALID             BIT(20)
>> +#define SW_SESSVLD_SEL                  BIT(28)
>> +
>> +#define USB_PHY_CFG0			0x94
>> +#define USB_PHY_UTMI_CTRL5		0x50
>> +#define USB_PHY_FSEL_SEL		0xB8
>> +#define USB_PHY_HS_PHY_CTRL_COMMON0	0x54
>> +#define USB_PHY_REFCLK_CTRL		0xA0
>> +#define USB_PHY_HS_PHY_CTRL2		0x64
>> +#define USB_PHY_UTMI_CTRL0		0x3c
>> +#define USB2PHY_USB_PHY_M31_XCFGI_1	0xBC
>> +#define USB2PHY_USB_PHY_M31_XCFGI_4	0xC8
>> +#define USB2PHY_USB_PHY_M31_XCFGI_5	0xCC
>> +#define USB2PHY_USB_PHY_M31_XCFGI_11	0xE4
> Could you sort them address-wise?

... and lowercase the hex values, please.

> 
>> +
>> +#define USB2_0_TX_ENABLE		BIT(2)
>> +#define HSTX_SLEW_RATE_565PS		3
>> +#define PLL_CHARGING_PUMP_CURRENT_35UA	(3 << 3)
>> +#define ODT_VALUE_38_02_OHM		(3 << 6)
>> +#define ODT_VALUE_45_02_OHM		BIT(2)
>> +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA	(1)
> Weird mix of values, bits, bitfields.. perhaps BIT(n) and
> GENMASK() (+ FIELD_PREP) would be more suitable?
> 
>> +
>> +#define UTMI_PHY_OVERRIDE_EN		BIT(1)
>> +#define POR_EN				BIT(1)
> Please associate these with their registers, like
> 
> #define FOO_REG		0xf00
>   #define POR_EN		BIT(1)
> 
>> +#define FREQ_SEL			BIT(0)
>> +#define COMMONONN			BIT(7)
>> +#define FSEL				BIT(4)
>> +#define RETENABLEN			BIT(3)
>> +#define USB2_SUSPEND_N_SEL		BIT(3)
>> +#define USB2_SUSPEND_N			BIT(2)
>> +#define USB2_UTMI_CLK_EN		BIT(1)
>> +#define CLKCORE				BIT(1)
>> +#define ATERESET			~BIT(0)
>> +#define FREQ_24MHZ			(5 << 4)
>> +#define XCFG_COARSE_TUNE_NUM		(2 << 0)
>> +#define XCFG_FINE_TUNE_NUM		(1 << 3)
> same comment
> 
>> +
>> +static void m31usb_write_readback(void *base, u32 offset,
>> +					const u32 mask, u32 val);
> We don't need this forward-definition, just move the function up.
> 
>> +
>> +struct m31usb_phy {
>> +	struct usb_phy		phy;
>> +	void __iomem		*base;
>> +	void __iomem		*qscratch_base;
>> +
>> +	struct reset_control	*phy_reset;
>> +
>> +	bool			cable_connected;
>> +	bool			suspended;
>> +	bool			ulpi_mode;
>> +};
>> +
>> +static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
>> +{
>> +	if (action == CLK_RESET_ASSERT)
>> +		reset_control_assert(qphy->phy_reset);
>> +	else
>> +		reset_control_deassert(qphy->phy_reset);
>> +	wmb(); /* ensure data is written to hw register */
> Please move the comment above the call.
> 
>> +}

Or even better just inline the function. I was never a fan of such 
multiplexers.

Also does wmb() make sense here? Doesn't regmap (which is used by reset 
controller) remove the need for it?

>> +
>> +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
>> +{
>> +	/* Enable override ctrl */
>> +	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> Some of the comments are missing a space before '*/'
> 
> Also, please consider adding some newlines to logically split the
> actions.


-- 
With best wishes
Dmitry

Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes

[Konrad Dybcio]

On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> Add USB phy and controller nodes
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index c2d6cc65..3183357 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -383,6 +383,61 @@
>  				status = "disabled";
>  			};
>  		};
> +
> +		usb_0_m31phy: hs_m31phy@7b000 {
> +			compatible = "qcom,ipq5332-m31-usb-hsphy";
> +			reg = <0x0007b000 0x12C>,
random uppercase hex

> +			      <0x08af8800 0x400>;
> +			reg-names = "m31usb_phy_base",
> +				    "qscratch_base";
> +			phy_type= "utmi";
Missing space before '='

> +
> +			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> +			reset-names = "usb2_phy_reset";
> +
> +			status = "okay";
If you're only defining the node, it's enabled by default

In this case, you'd probably want to disable it by default.

> +		};
> +
> +		usb2: usb2@8a00000 {
> +			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
Please push these 3 properties to the end

And add status = "disabled" below them.

> +
> +			reg = <0x08af8800 0x100>;
> +
> +			clocks = <&gcc GCC_USB0_MASTER_CLK>,
> +				<&gcc GCC_SNOC_USB_CLK>,
> +				<&gcc GCC_USB0_SLEEP_CLK>,
> +				<&gcc GCC_USB0_MOCK_UTMI_CLK>;
> +
Please remove this newline.

> +			clock-names = "core",
> +				"iface",
> +				"sleep",
> +				"mock_utmi";
Please align this, and all other similar lists.

> +
> +			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
interrupts-extended is unnecessary with just a single interrupt
source.. you can make it `interrupts` and drop the GIC reference.

It would also be nice to push the interrupt properties below 'reg'.
We're working on documenting and automating checking the preferred
property order.

> +			interrupt-names = "pwr_event";
> +
> +			resets = <&gcc GCC_USB_BCR>;
> +
> +			qcom,select-utmi-as-pipe-clk;
> +
> +			usb2_0_dwc: usb@8a00000 {
> +				compatible = "snps,dwc3";
> +				reg = <0x08a00000 0xe000>;
> +				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> +				clock-names = "ref";
> +				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> +				usb-phy = <&usb_0_m31phy>;
> +				tx-fifo-resize;
> +				snps,is-utmi-l1-suspend;
> +				snps,hird-threshold = /bits/ 8 <0x0>;
> +				snps,dis_u2_susphy_quirk;
> +				snps,dis_u3_susphy_quirk;
> +				snps,ref-clock-period-ns = <21>;
1/21 is 0.0476..  that doesn't seem to correspond to the ref
clk frequency?

Konrad
> +			};
> +		};
>  	};
>  
>  	timer {

Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Krzysztof Kozlowski]

On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Document the M31 USB2 phy present in IPQ5332

Full stop.

> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> new file mode 100644
> index 0000000..8ad4ba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +

Drop stray blank lines.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm M31 USB PHY

What is M31?

> +
> +maintainers:
> +  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
> +  - Varadarajan Narayanan <quic_varada@quicinc.org>
> +
> +description:
> +  This file contains documentation for the USB M31 PHY found in Qualcomm

Drop redundant parts ("This file contains documentation for the").

> +  IPQ5018, IPQ5332 SoCs.
> +
> +properties:
> +  compatible:
> +    oneOf:

That's not oneOf.

> +      - items:

No items.

> +          - enum:
> +              - qcom,m31-usb-hsphy

I am confused what's this. If m31 is coming from some IP block provider,
then you are using wrong vendor prefix.
https://www.m31tech.com/download_file/M31_USB.pdf


> +              - qcom,ipq5332-m31-usb-hsphy

This confuses me even more. IPQ m31?

> +
> +  reg:
> +    description:
> +      Offset and length of the M31 PHY register set

Drop description, obvious.

> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: m31usb_phy_base
> +      - const: qscratch_base

Drop "_base" from both.

> +
> +  phy_type:
> +    oneOf:
> +      - items:
> +          - enum:
> +              - utmi
> +              - ulpi

This does not belong to phy, but to USB node.

> +
> +  resets:
> +    maxItems: 1
> +    description:
> +      List of phandles and reset pairs, one for each entry in reset-names.

Drop useless description.

> +
> +  reset-names:
> +    items:
> +      - const:
> +          usb2_phy_reset

Drop entire reset-names.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> +    hs_m31phy_0: hs_m31phy@5b00 {

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Also, no underscores in node names.

> +        compatible = "qcom,m31-usb-hsphy";
> +        reg = <0x5b000 0x3000>,

This is not what your unit address is saying.

> +            <0x08af8800 0x400>;

Align it.

> +        reg-names = "m31usb_phy_base",
> +                "qscratch_base";

Align it.

> +        phy_type = "utmi";
> +        resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> +        reset-names = "usb2_phy_reset";
> +
> +        status = "ok";

Drop.

> +    };

Best regards,
Krzysztof

Re: [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible

[Krzysztof Kozlowski]

On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Document the IPQ5332 dwc3 compatible

Full stop.

> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index ae24dac..9c3d6f4 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -14,6 +14,7 @@ properties:
>      items:
>        - enum:
>            - qcom,ipq4019-dwc3
> +          - qcom,ipq5332-dwc3
>            - qcom,ipq6018-dwc3
>            - qcom,ipq8064-dwc3
>            - qcom,ipq8074-dwc3
> @@ -246,6 +247,7 @@ allOf:
>          compatible:
>            contains:
>              enum:
> +              - qcom,ipq5332-dwc3
>                - qcom,msm8994-dwc3
>                - qcom,qcs404-dwc3

What about interrupts?

Best regards,
Krzysztof

Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes

[Krzysztof Kozlowski]

On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Add USB phy and controller nodes
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index c2d6cc65..3183357 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -383,6 +383,61 @@
>  				status = "disabled";
>  			};
>  		};
> +
> +		usb_0_m31phy: hs_m31phy@7b000 {

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +			compatible = "qcom,ipq5332-m31-usb-hsphy";
> +			reg = <0x0007b000 0x12C>,
> +			      <0x08af8800 0x400>;

Lowercase hex only.

> +			reg-names = "m31usb_phy_base",
> +				    "qscratch_base";
> +			phy_type= "utmi";
> +
> +			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> +			reset-names = "usb2_phy_reset";
> +
> +			status = "okay";

It's by default. Drop.

> +		};
> +
> +		usb2: usb2@8a00000 {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			reg = <0x08af8800 0x100>;

reg is always after compatible. Ranges is third. Then you will spot that
address is wrong.

> +
> +			clocks = <&gcc GCC_USB0_MASTER_CLK>,
> +				<&gcc GCC_SNOC_USB_CLK>,
> +				<&gcc GCC_USB0_SLEEP_CLK>,
> +				<&gcc GCC_USB0_MOCK_UTMI_CLK>;

Fix alignment.

> +
> +			clock-names = "core",
> +				"iface",
> +				"sleep",
> +				"mock_utmi";

Fix alignment.

> +
> +			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "pwr_event";
> +


Best regards,
Krzysztof

Re: [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver

[Krzysztof Kozlowski]

On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Enable QCOM M31 USB phy driver present in IPQ5332

What is "QCOM"? If acronym, extend. IPQ5332 - provide full name, so
"Qualcomm IPQ....".

Best regards,
Krzysztof

Re: [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver

[Krzysztof Kozlowski]

On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> Include M31 phy driver if CONFIG_PHY_QCOM_M31_USB is enabled
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>

That's not a separate commit.

Best regards,
Krzysztof

Re: [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy

[Krzysztof Kozlowski]

On 07/06/2023 13:20, Dmitry Baryshkov wrote:
> On 07/06/2023 13:56, Varadarajan Narayanan wrote:
>> Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver
>>
>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> 
> Is there any reason to keep Kconfig, Makefile and driver in different 
> commits?

KPI? Yearly objectives/goals?

Best regards,
Krzysztof

Re: [PATCH 1/9] dt-bindings: usb: dwc3: Add IPQ5332 compatible

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 08:33:26PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Document the IPQ5332 dwc3 compatible
>
> Full stop.

Ok.

> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > index ae24dac..9c3d6f4 100644
> > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > @@ -14,6 +14,7 @@ properties:
> >      items:
> >        - enum:
> >            - qcom,ipq4019-dwc3
> > +          - qcom,ipq5332-dwc3
> >            - qcom,ipq6018-dwc3
> >            - qcom,ipq8064-dwc3
> >            - qcom,ipq8074-dwc3
> > @@ -246,6 +247,7 @@ allOf:
> >          compatible:
> >            contains:
> >              enum:
> > +              - qcom,ipq5332-dwc3
> >                - qcom,msm8994-dwc3
> >                - qcom,qcs404-dwc3
>
> What about interrupts?

Will fix and post next version.

Thanks
Varada

Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 08:31:50PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Document the M31 USB2 phy present in IPQ5332
>
> Full stop.

Ok.

> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  .../devicetree/bindings/phy/qcom,m31.yaml          | 69 ++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/qcom,m31.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/qcom,m31.yaml b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> > new file mode 100644
> > index 0000000..8ad4ba4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/qcom,m31.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +
>
> Drop stray blank lines.

Ok.

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/qcom,m31.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm M31 USB PHY
>
> What is M31?

M31 is an external IP integrated into IPQ5332 SoC.

> > +
> > +maintainers:
> > +  - Sricharan Ramabadhran <quic_srichara@quicinc.com>
> > +  - Varadarajan Narayanan <quic_varada@quicinc.org>
> > +
> > +description:
> > +  This file contains documentation for the USB M31 PHY found in Qualcomm
>
> Drop redundant parts ("This file contains documentation for the").

Ok.

> > +  IPQ5018, IPQ5332 SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
>
> That's not oneOf.

Ok.

> > +      - items:
>
> No items.

Ok.

> > +          - enum:
> > +              - qcom,m31-usb-hsphy
>
> I am confused what's this. If m31 is coming from some IP block provider,
> then you are using wrong vendor prefix.
> https://www.m31tech.com/download_file/M31_USB.pdf
>
>
> > +              - qcom,ipq5332-m31-usb-hsphy
>
> This confuses me even more. IPQ m31?

Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
Will that be acceptable?

> > +
> > +  reg:
> > +    description:
> > +      Offset and length of the M31 PHY register set
>
> Drop description, obvious.

Ok.

> > +    maxItems: 2
> > +
> > +  reg-names:
> > +    items:
> > +      - const: m31usb_phy_base
> > +      - const: qscratch_base
>
> Drop "_base" from both.

Ok. Will drop qscratch_base. This is in the controller space.
Should not come here.

> > +
> > +  phy_type:
> > +    oneOf:
> > +      - items:
> > +          - enum:
> > +              - utmi
> > +              - ulpi
>
> This does not belong to phy, but to USB node.

This is used by the driver to set a bit during phy init. Hence
have it as a replication of the USB node's entry. If this is not
permissible, is there some way to get this from the USB node,
or any other alternative mechanism?

> > +
> > +  resets:
> > +    maxItems: 1
> > +    description:
> > +      List of phandles and reset pairs, one for each entry in reset-names.
>
> Drop useless description.

Ok.

> > +
> > +  reset-names:
> > +    items:
> > +      - const:
> > +          usb2_phy_reset
>
> Drop entire reset-names.

Ok.

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> > +    hs_m31phy_0: hs_m31phy@5b00 {
>
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> Also, no underscores in node names.

Will change this as usbphy0:hs_m31phy@7b000

> > +        compatible = "qcom,m31-usb-hsphy";
> > +        reg = <0x5b000 0x3000>,
>
> This is not what your unit address is saying.

Will change to 0x0007b000.

> > +            <0x08af8800 0x400>;
>
> Align it.

Will drop this. This is in the controller space.

> > +        reg-names = "m31usb_phy_base",
> > +                "qscratch_base";
>
> Align it.

Ok.

> > +        phy_type = "utmi";
> > +        resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> > +        reset-names = "usb2_phy_reset";
> > +
> > +        status = "ok";
>
> Drop.

Ok. Hopefully will get an alternative for the phy_type.

Thanks
Varada

> > +    };
>
> Best regards,
> Krzysztof
>

Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 01:54:23PM +0200, Konrad Dybcio wrote:
>
>
> On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> > Add the M31 USB2 phy driver
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 360 insertions(+)
> >  create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> > new file mode 100644
> > index 0000000..d29a91e
> > --- /dev/null
> > +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> > @@ -0,0 +1,360 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/phy.h>
> > +#include <linux/reset.h>
> > +#include <linux/of_device.h>
> Please sort these

Ok.

> > +
> > +enum clk_reset_action {
> > +	CLK_RESET_DEASSERT	= 0,
> > +	CLK_RESET_ASSERT	= 1
> > +};
> > +
> > +#define USB2PHY_PORT_POWERDOWN		0xA4
> > +#define POWER_UP			BIT(0)
> > +#define POWER_DOWN			0
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL1	0x40
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL2	0x44
> > +#define UTMI_ULPI_SEL			BIT(7)
> > +#define UTMI_TEST_MUX_SEL		BIT(6)
> > +
> > +#define HS_PHY_CTRL_REG			0x10
> > +#define UTMI_OTG_VBUS_VALID             BIT(20)
> > +#define SW_SESSVLD_SEL                  BIT(28)
> > +
> > +#define USB_PHY_CFG0			0x94
> > +#define USB_PHY_UTMI_CTRL5		0x50
> > +#define USB_PHY_FSEL_SEL		0xB8
> > +#define USB_PHY_HS_PHY_CTRL_COMMON0	0x54
> > +#define USB_PHY_REFCLK_CTRL		0xA0
> > +#define USB_PHY_HS_PHY_CTRL2		0x64
> > +#define USB_PHY_UTMI_CTRL0		0x3c
> > +#define USB2PHY_USB_PHY_M31_XCFGI_1	0xBC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_4	0xC8
> > +#define USB2PHY_USB_PHY_M31_XCFGI_5	0xCC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_11	0xE4
> Could you sort them address-wise?

Ok.

> > +
> > +#define USB2_0_TX_ENABLE		BIT(2)
> > +#define HSTX_SLEW_RATE_565PS		3
> > +#define PLL_CHARGING_PUMP_CURRENT_35UA	(3 << 3)
> > +#define ODT_VALUE_38_02_OHM		(3 << 6)
> > +#define ODT_VALUE_45_02_OHM		BIT(2)
> > +#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA	(1)
> Weird mix of values, bits, bitfields.. perhaps BIT(n) and
> GENMASK() (+ FIELD_PREP) would be more suitable?

Ok.

> > +
> > +#define UTMI_PHY_OVERRIDE_EN		BIT(1)
> > +#define POR_EN				BIT(1)
> Please associate these with their registers, like

Ok.

> #define FOO_REG		0xf00
>  #define POR_EN		BIT(1)
>
> > +#define FREQ_SEL			BIT(0)
> > +#define COMMONONN			BIT(7)
> > +#define FSEL				BIT(4)
> > +#define RETENABLEN			BIT(3)
> > +#define USB2_SUSPEND_N_SEL		BIT(3)
> > +#define USB2_SUSPEND_N			BIT(2)
> > +#define USB2_UTMI_CLK_EN		BIT(1)
> > +#define CLKCORE				BIT(1)
> > +#define ATERESET			~BIT(0)
> > +#define FREQ_24MHZ			(5 << 4)
> > +#define XCFG_COARSE_TUNE_NUM		(2 << 0)
> > +#define XCFG_FINE_TUNE_NUM		(1 << 3)
> same comment

Ok.

> > +
> > +static void m31usb_write_readback(void *base, u32 offset,
> > +					const u32 mask, u32 val);
> We don't need this forward-definition, just move the function up.
>
> > +
> > +struct m31usb_phy {
> > +	struct usb_phy		phy;
> > +	void __iomem		*base;
> > +	void __iomem		*qscratch_base;
> > +
> > +	struct reset_control	*phy_reset;
> > +
> > +	bool			cable_connected;
> > +	bool			suspended;
> > +	bool			ulpi_mode;
> > +};
> > +
> > +static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
> > +{
> > +	if (action == CLK_RESET_ASSERT)
> > +		reset_control_assert(qphy->phy_reset);
> > +	else
> > +		reset_control_deassert(qphy->phy_reset);
> > +	wmb(); /* ensure data is written to hw register */
> Please move the comment above the call.

This is used only once. Hence, will move it to the calling location itself.

> > +}
> > +
> > +static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> > +{
> > +	/* Enable override ctrl */
> > +	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> Some of the comments are missing a space before '*/'
>
> Also, please consider adding some newlines to logically split the
> actions.
>
> > +	/* Enable POR*/
> > +	writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> > +	udelay(15);
> > +	/* Configure frequency select value*/
> > +	writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> > +	/* Configure refclk frequency */
> > +	writel(COMMONONN | FSEL | RETENABLEN,
> > +		qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> > +	/* select refclk source */
> > +	writel(CLKCORE, qphy->base + USB_PHY_REFCLK_CTRL);
> > +	/* select ATERESET*/
> > +	writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> > +	writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> > +	writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> > +	writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> > +	/* Disable override ctrl */
> > +	writel(0x0, qphy->base + USB_PHY_CFG0);
> > +}
> > +
> > +static void ipq5332_m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> > +{
> > +	/* Enable override ctrl */
> > +	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> > +	/* Enable POR*/
> ditto
>
> > +	writel(POR_EN, qphy->base + USB_PHY_UTMI_CTRL5);
> > +	udelay(15);
> > +	/* Configure frequency select value*/
> > +	writel(FREQ_SEL, qphy->base + USB_PHY_FSEL_SEL);
> > +	/* Configure refclk frequency */
> > +	writel(COMMONONN | FREQ_24MHZ | RETENABLEN,
> > +		qphy->base + USB_PHY_HS_PHY_CTRL_COMMON0);
> > +	/* select ATERESET*/
> > +	writel(POR_EN & ATERESET, qphy->base + USB_PHY_UTMI_CTRL5);
> > +	writel(USB2_SUSPEND_N_SEL | USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> > +	writel(XCFG_COARSE_TUNE_NUM  | XCFG_FINE_TUNE_NUM,
> > +				qphy->base + USB2PHY_USB_PHY_M31_XCFGI_11);
> > +	/* Adjust HSTX slew rate to 565 ps*/
> > +	/* Adjust PLL lock Time counter for release clock to 35uA */
> > +	/* Adjust Manual control ODT value to 38.02 Ohm */
> > +	writel(HSTX_SLEW_RATE_565PS | PLL_CHARGING_PUMP_CURRENT_35UA |
> > +		ODT_VALUE_38_02_OHM, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_4);
> These functions seem very similar, with the main difference being that
> IPQ5332 adds some more writes at the end. Perhaps some commonizing could
> be done?
>
> > +	/*
> > +	 * Enable to always turn on USB 2.0 TX driver
> > +	 * when system is in USB 2.0 HS mode
> > +	 */
> > +	writel(USB2_0_TX_ENABLE, qphy->base + USB2PHY_USB_PHY_M31_XCFGI_1);
> > +	/* Adjust Manual control ODT value to 45.02 Ohm */
> > +	/* Adjust HSTX Pre-emphasis level to 0.55mA */
> > +	writel(ODT_VALUE_45_02_OHM | HSTX_PRE_EMPHASIS_LEVEL_0_55MA,
> > +		qphy->base + USB2PHY_USB_PHY_M31_XCFGI_5);
> > +	udelay(4);
> > +	writel(0x0, qphy->base + USB_PHY_UTMI_CTRL5);
> > +	writel(USB2_SUSPEND_N | USB2_UTMI_CLK_EN,
> > +		qphy->base + USB_PHY_HS_PHY_CTRL2);
> > +}

Will change the above to table based register init, based on
compatible/data.

> > +
> > +static int m31usb_phy_init(struct usb_phy *phy)
> > +{
> > +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > +	/* Perform phy reset */
> > +	m31usb_reset(qphy, CLK_RESET_ASSERT);
> > +	usleep_range(1, 5);
> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>
> this may not work as intended

Will change it to udelay(5).

> > +	m31usb_reset(qphy, CLK_RESET_DEASSERT);
> > +
> > +	/* configure for ULPI mode if requested */
> > +	if (qphy->ulpi_mode)
> > +		writel_relaxed(0x0, qphy->base + USB2PHY_PORT_UTMI_CTRL2);
> > +
> > +	/* Enable the PHY */
> > +	writel_relaxed(POWER_UP, qphy->base + USB2PHY_PORT_POWERDOWN);
> > +
> > +	/* Make sure above write completed */
> > +	wmb();
> As you're calling wmb in the reset func, dropping _relaxed from the
> ULPI and PORT_POWERDOWN writes should work the same

Ok.

> > +
> > +	/* Turn on phy ref clock*/
> > +	if (of_device_is_compatible(phy->dev->of_node,
> > +					"qcom,ipq5332-m31-usb-hsphy"))
> > +		ipq5332_m31usb_phy_enable_clock(qphy);
> > +	else
> > +		m31usb_phy_enable_clock(qphy);
> This should be done using OF match data.

Ok.

> > +
> > +	/* Set OTG VBUS Valid from HSPHY to controller */
> > +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > +				UTMI_OTG_VBUS_VALID, UTMI_OTG_VBUS_VALID);
> > +	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> > +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > +				SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> > +
> > +	return 0;
> > +}
> > +
> > +static void m31usb_phy_shutdown(struct usb_phy *phy)
> > +{
> > +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > +	/* Disable the PHY */
> > +	writel_relaxed(POWER_DOWN, qphy->base + USB2PHY_PORT_POWERDOWN);
> > +	/* Make sure above write completed */
> > +	wmb();
> > +}
> > +
> > +static void m31usb_write_readback(void *base, u32 offset,
> > +					const u32 mask, u32 val)
> The indentation here makes `const u32..` misaligned.
>
> > +{
> > +	u32 write_val, tmp = readl_relaxed(base + offset);
> > +
> > +	tmp &= ~mask; /* retain other bits */
> > +	write_val = tmp | val;
> > +
> > +	writel_relaxed(write_val, base + offset);
> > +	/* Make sure above write completed */
> > +	wmb();
> > +
> > +	/* Read back to see if val was written */
> > +	tmp = readl_relaxed(base + offset);
> > +	tmp &= mask; /* clear other bits */
> > +
> > +	if (tmp != val)
> > +		pr_err("%s: write: %x to QSCRATCH: %x FAILED\n",
> > +			__func__, val, offset);
> > +}
> > +
> > +static int m31usb_phy_notify_connect(struct usb_phy *phy,
> > +					enum usb_device_speed speed)
> > +{
> > +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > +	qphy->cable_connected = true;
> > +
> > +	dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
> spurious space at the beginning of the string
>
> > +
> > +	/* Set OTG VBUS Valid from HSPHY to controller */
> > +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > +				UTMI_OTG_VBUS_VALID,
> > +				UTMI_OTG_VBUS_VALID);
> Please align the lines with the previous opening bracket
>
> > +
> > +	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> > +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > +				SW_SESSVLD_SEL, SW_SESSVLD_SEL);
> > +
> > +	dev_dbg(phy->dev, "M31USB2 phy connect notification\n");
> > +	return 0;
> > +}
> > +
> > +static int m31usb_phy_notify_disconnect(struct usb_phy *phy,
> > +					enum usb_device_speed speed)
> > +{
> > +	struct m31usb_phy *qphy = container_of(phy, struct m31usb_phy, phy);
> > +
> > +	qphy->cable_connected = false;
> > +
> > +	dev_dbg(phy->dev, " cable_connected=%d\n", qphy->cable_connected);
> > +
> > +	/* Set OTG VBUS Valid from HSPHY to controller */
> > +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > +				UTMI_OTG_VBUS_VALID, 0);
> > +
> > +	/* Indicate value is driven by UTMI_OTG_VBUS_VALID bit */
> > +	m31usb_write_readback(qphy->qscratch_base, HS_PHY_CTRL_REG,
> > +				SW_SESSVLD_SEL, 0);
> > +
> > +	dev_dbg(phy->dev, "M31USB2 phy disconnect notification\n");
> > +	return 0;
> > +}

Will remove these functions. They are accessing 'qscratch' area that
is part of the controller space. It doesn't belong in this driver.

> > +static int m31usb_phy_probe(struct platform_device *pdev)
> > +{
> > +	struct m31usb_phy *qphy;
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *res;
> > +	int ret;
> > +	const char *phy_type;
> Please sort these in a reverse-Christmas-tree order.

ok.

> > +
> > +	qphy = devm_kzalloc(dev, sizeof(*qphy), GFP_KERNEL);
> > +	if (!qphy)
> > +		return -ENOMEM;
> > +
> > +	qphy->phy.dev = dev;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +							"m31usb_phy_base");
> > +	qphy->base = devm_ioremap_resource(dev, res);
> devm_platform_get_and_ioremap_resource?

ok.

> > +	if (IS_ERR(qphy->base))
> > +		return PTR_ERR(qphy->base);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +							"qscratch_base");
> > +	if (res) {
> Do we expect platforms without this register space?
>
> > +		qphy->qscratch_base = devm_ioremap(dev, res->start,
> > +							resource_size(res));
> > +		if (IS_ERR(qphy->qscratch_base)) {
> > +			dev_dbg(dev, "couldn't ioremap qscratch_base\n");
> > +			qphy->qscratch_base = NULL;
> > +		}
> > +	}

Will remove this qscratch code.

> > +	qphy->phy_reset = devm_reset_control_get(dev, "usb2_phy_reset");
> not _exclusive?

Ok.

> > +	if (IS_ERR(qphy->phy_reset))
> > +		return PTR_ERR(qphy->phy_reset);
> > +
> > +	qphy->ulpi_mode = false;
> > +	ret = of_property_read_string(dev->of_node, "phy_type", &phy_type);
> of_usb_get_phy_mode?

Ok.

> > +
> > +	if (!ret) {
> > +		if (!strcasecmp(phy_type, "ulpi"))
> > +			qphy->ulpi_mode = true;
> > +	} else {
> > +		dev_err(dev, "error reading phy_type property\n");
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, qphy);
> > +
> > +	qphy->phy.label			= "qcom-m31usb-phy";
> > +	qphy->phy.init			= m31usb_phy_init;
> > +	qphy->phy.shutdown		= m31usb_phy_shutdown;
> > +	qphy->phy.type			= USB_PHY_TYPE_USB2;
> > +
> > +	if (qphy->qscratch_base) {
> > +		qphy->phy.notify_connect        = m31usb_phy_notify_connect;
> > +		qphy->phy.notify_disconnect     = m31usb_phy_notify_disconnect;
> > +	}
> > +
> > +	ret = usb_add_phy_dev(&qphy->phy);
> > +
> > +	return ret;
> > +}
> > +
> > +static int m31usb_phy_remove(struct platform_device *pdev)
> Please make this return void and pass it to .remove_new

Ok.

> > +{
> > +	struct m31usb_phy *qphy = platform_get_drvdata(pdev);
> > +
> > +	usb_remove_phy(&qphy->phy);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id m31usb_phy_id_table[] = {
> > +	{ .compatible = "qcom,m31-usb-hsphy",},
> > +	{ .compatible = "qcom,ipq5332-m31-usb-hsphy",},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, m31usb_phy_id_table);
> > +
> > +static struct platform_driver m31usb_phy_driver = {
> > +	.probe		= m31usb_phy_probe,
> > +	.remove		= m31usb_phy_remove,
> > +	.driver = {
> > +		.name	= "qcom-m31usb-phy",
> > +		.of_match_table = of_match_ptr(m31usb_phy_id_table),
> of_match_ptr is unnecessary, this driver depends on OF.

Ok.

Thanks
Varada

> Konrad
> > +	},
> > +};
> > +
> > +module_platform_driver(m31usb_phy_driver);
> > +
> > +MODULE_DESCRIPTION("USB2 Qualcomm M31 HSPHY driver");
> > +MODULE_LICENSE("GPL");

Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 03:29:18PM +0300, Dmitry Baryshkov wrote:
> Two minor nits on top of the review:
>
> On 07/06/2023 14:54, Konrad Dybcio wrote:
> >On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> >>Add the M31 USB2 phy driver
> >>
> >>Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>---
> >>  drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 360 insertions(+)
> >>  create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
> >>
> >>diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> >>new file mode 100644
> >>index 0000000..d29a91e
> >>--- /dev/null
> >>+++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> >>@@ -0,0 +1,360 @@
> >>+// SPDX-License-Identifier: GPL-2.0+
> >>+/*
> >>+ * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> >>+ */
> >>+
> >>+#include <linux/module.h>
> >>+#include <linux/kernel.h>
> >>+#include <linux/err.h>
> >>+#include <linux/slab.h>
> >>+#include <linux/clk.h>
> >>+#include <linux/delay.h>
> >>+#include <linux/io.h>
> >>+#include <linux/of.h>
> >>+#include <linux/platform_device.h>
> >>+#include <linux/usb/phy.h>
> >>+#include <linux/reset.h>
> >>+#include <linux/of_device.h>
> >Please sort these
> >
> >>+
> >>+enum clk_reset_action {
> >>+	CLK_RESET_DEASSERT	= 0,
> >>+	CLK_RESET_ASSERT	= 1
> >>+};
> >>+
> >>+#define USB2PHY_PORT_POWERDOWN		0xA4
> >>+#define POWER_UP			BIT(0)
> >>+#define POWER_DOWN			0
> >>+
> >>+#define USB2PHY_PORT_UTMI_CTRL1	0x40
> >>+
> >>+#define USB2PHY_PORT_UTMI_CTRL2	0x44
> >>+#define UTMI_ULPI_SEL			BIT(7)
> >>+#define UTMI_TEST_MUX_SEL		BIT(6)
> >>+
> >>+#define HS_PHY_CTRL_REG			0x10
> >>+#define UTMI_OTG_VBUS_VALID             BIT(20)
> >>+#define SW_SESSVLD_SEL                  BIT(28)
> >>+
> >>+#define USB_PHY_CFG0			0x94
> >>+#define USB_PHY_UTMI_CTRL5		0x50
> >>+#define USB_PHY_FSEL_SEL		0xB8
> >>+#define USB_PHY_HS_PHY_CTRL_COMMON0	0x54
> >>+#define USB_PHY_REFCLK_CTRL		0xA0
> >>+#define USB_PHY_HS_PHY_CTRL2		0x64
> >>+#define USB_PHY_UTMI_CTRL0		0x3c
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_1	0xBC
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_4	0xC8
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_5	0xCC
> >>+#define USB2PHY_USB_PHY_M31_XCFGI_11	0xE4
> >Could you sort them address-wise?
>
> ... and lowercase the hex values, please.

Ok.

> >>+
> >>+#define USB2_0_TX_ENABLE		BIT(2)
> >>+#define HSTX_SLEW_RATE_565PS		3
> >>+#define PLL_CHARGING_PUMP_CURRENT_35UA	(3 << 3)
> >>+#define ODT_VALUE_38_02_OHM		(3 << 6)
> >>+#define ODT_VALUE_45_02_OHM		BIT(2)
> >>+#define HSTX_PRE_EMPHASIS_LEVEL_0_55MA	(1)
> >Weird mix of values, bits, bitfields.. perhaps BIT(n) and
> >GENMASK() (+ FIELD_PREP) would be more suitable?
> >
> >>+
> >>+#define UTMI_PHY_OVERRIDE_EN		BIT(1)
> >>+#define POR_EN				BIT(1)
> >Please associate these with their registers, like
> >
> >#define FOO_REG		0xf00
> >  #define POR_EN		BIT(1)
> >
> >>+#define FREQ_SEL			BIT(0)
> >>+#define COMMONONN			BIT(7)
> >>+#define FSEL				BIT(4)
> >>+#define RETENABLEN			BIT(3)
> >>+#define USB2_SUSPEND_N_SEL		BIT(3)
> >>+#define USB2_SUSPEND_N			BIT(2)
> >>+#define USB2_UTMI_CLK_EN		BIT(1)
> >>+#define CLKCORE				BIT(1)
> >>+#define ATERESET			~BIT(0)
> >>+#define FREQ_24MHZ			(5 << 4)
> >>+#define XCFG_COARSE_TUNE_NUM		(2 << 0)
> >>+#define XCFG_FINE_TUNE_NUM		(1 << 3)
> >same comment
> >
> >>+
> >>+static void m31usb_write_readback(void *base, u32 offset,
> >>+					const u32 mask, u32 val);
> >We don't need this forward-definition, just move the function up.
> >
> >>+
> >>+struct m31usb_phy {
> >>+	struct usb_phy		phy;
> >>+	void __iomem		*base;
> >>+	void __iomem		*qscratch_base;
> >>+
> >>+	struct reset_control	*phy_reset;
> >>+
> >>+	bool			cable_connected;
> >>+	bool			suspended;
> >>+	bool			ulpi_mode;
> >>+};
> >>+
> >>+static void m31usb_reset(struct m31usb_phy *qphy, u32 action)
> >>+{
> >>+	if (action == CLK_RESET_ASSERT)
> >>+		reset_control_assert(qphy->phy_reset);
> >>+	else
> >>+		reset_control_deassert(qphy->phy_reset);
> >>+	wmb(); /* ensure data is written to hw register */
> >Please move the comment above the call.
> >
> >>+}
>
> Or even better just inline the function. I was never a fan of such
> multiplexers.
>
> Also does wmb() make sense here? Doesn't regmap (which is used by reset
> controller) remove the need for it?

Will inline and remove the wmb.

Thanks
Varada

> >>+
> >>+static void m31usb_phy_enable_clock(struct m31usb_phy *qphy)
> >>+{
> >>+	/* Enable override ctrl */
> >>+	writel(UTMI_PHY_OVERRIDE_EN, qphy->base + USB_PHY_CFG0);
> >Some of the comments are missing a space before '*/'
> >
> >Also, please consider adding some newlines to logically split the
> >actions.
>
>
> --
> With best wishes
> Dmitry
>

Re: [PATCH 4/9] clk: qcom: ipq5332: Fix USB related clock defines

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 02:19:09PM +0300, Dmitry Baryshkov wrote:
> On 07/06/2023 13:56, Varadarajan Narayanan wrote:
> >Fix the USB related clock defines and add details
> >referenced by them
> >
> >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >---
> >  drivers/clk/qcom/gcc-ipq5332.c | 34 +++++++++++++++++++++++-----------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> >
> >diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
> >index a75ab88..2b58558 100644
> >--- a/drivers/clk/qcom/gcc-ipq5332.c
> >+++ b/drivers/clk/qcom/gcc-ipq5332.c
> >@@ -351,6 +351,16 @@ static const struct freq_tbl ftbl_gcc_adss_pwm_clk_src[] = {
> >  	{ }
> >  };
> >+static const struct clk_parent_data gcc_usb3phy_0_cc_pipe_clk_xo[] = {
> >+	{ .fw_name = "usb3phy_0_cc_pipe_clk" },
> >+	{ .fw_name = "xo" },
>
> gcc-ipq5332 uses DT indices, please don't mix that with .fw_name.
>
> >+};
> >+
> >+static const struct parent_map gcc_usb3phy_0_cc_pipe_clk_xo_map[] = {
> >+	{ P_USB3PHY_0_PIPE, 0 },
> >+	{ P_XO, 2 },
> >+};
> >+
> >  static struct clk_rcg2 gcc_adss_pwm_clk_src = {
> >  	.cmd_rcgr = 0x1c004,
> >  	.mnd_width = 0,
> >@@ -1101,16 +1111,18 @@ static struct clk_rcg2 gcc_usb0_mock_utmi_clk_src = {
> >  	},
> >  };
> >-static struct clk_regmap_phy_mux gcc_usb0_pipe_clk_src = {
> >+static struct clk_regmap_mux usb0_pipe_clk_src = {
> >  	.reg = 0x2c074,
> >+	.shift = 8,
> >+	.width = 2,
> >+	.parent_map = gcc_usb3phy_0_cc_pipe_clk_xo_map,
> >  	.clkr = {
> >-		.hw.init = &(struct clk_init_data) {
> >-			.name = "gcc_usb0_pipe_clk_src",
> >-			.parent_data = &(const struct clk_parent_data) {
> >-				.index = DT_USB_PCIE_WRAPPER_PIPE_CLK,
> >-			},
> >-			.num_parents = 1,
> >-			.ops = &clk_regmap_phy_mux_ops,
> >+		.hw.init = &(const struct clk_init_data){
> >+			.name = "usb0phy_0_cc_pipe_clk_src",
> >+			.parent_data = gcc_usb3phy_0_cc_pipe_clk_xo,
> >+			.num_parents = 2,
> >+			.ops = &clk_regmap_mux_closest_ops,
> >+			.flags = CLK_SET_RATE_PARENT,
> >  		},
>
> Soo... As you are reverting this. Is USB0 PIPE clock required to be parked
> to the XO? I was going to write 'before turning USB0_GDSC' off, but then I
> noticed that gcc-ipq5332 doesn't declare GDSCs. Does this platform have
> GDSCs?
>
> >  	},
> >  };
> >@@ -3041,8 +3053,8 @@ static struct clk_branch gcc_usb0_pipe_clk = {
> >  		.enable_mask = BIT(0),
> >  		.hw.init = &(const struct clk_init_data) {
> >  			.name = "gcc_usb0_pipe_clk",
> >-			.parent_hws = (const struct clk_hw*[]) {
> >-				&gcc_usb0_pipe_clk_src.clkr.hw,
> >+			.parent_names = (const char *[]){
> >+				"usb0_pipe_clk_src"
>
> complete and definitive NAK. Do not use parent_names, we have just stopped
> migrating from them.

Will drop changes to this file and post a new patch.

Thanks
Varada
> >  			},
> >  			.num_parents = 1,
> >  			.flags = CLK_SET_RATE_PARENT,
> >@@ -3580,7 +3592,7 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = {
> >  	[GCC_PCIE3X2_PIPE_CLK_SRC] = &gcc_pcie3x2_pipe_clk_src.clkr,
> >  	[GCC_PCIE3X1_0_PIPE_CLK_SRC] = &gcc_pcie3x1_0_pipe_clk_src.clkr,
> >  	[GCC_PCIE3X1_1_PIPE_CLK_SRC] = &gcc_pcie3x1_1_pipe_clk_src.clkr,
> >-	[GCC_USB0_PIPE_CLK_SRC] = &gcc_usb0_pipe_clk_src.clkr,
> >+	[GCC_USB0_PIPE_CLK_SRC] = &usb0_pipe_clk_src.clkr,
> >  };
> >  static const struct qcom_reset_map gcc_ipq5332_resets[] = {
>
> --
> With best wishes
> Dmitry
>

Re: [PATCH 5/9] phy: qcom-m31: Introduce qcom,m31 USB phy

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 08:37:05PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 13:20, Dmitry Baryshkov wrote:
> > On 07/06/2023 13:56, Varadarajan Narayanan wrote:
> >> Introduce CONFIG_PHY_QCOM_M31_USB for including the M31 phy driver
> >>
> >> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >
> > Is there any reason to keep Kconfig, Makefile and driver in different
> > commits?
>
> KPI? Yearly objectives/goals?

No :-). Was not sure, hence split them.
Will combine in the next version.

Thanks
Varada

Re: [PATCH 6/9] phy: qcom: Add qcom,m31 USB phy driver

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 08:36:45PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Include M31 phy driver if CONFIG_PHY_QCOM_M31_USB is enabled
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>
> That's not a separate commit.

Will combine with the driver and post new version.

Thanks
Varada

> Best regards,
> Krzysztof
>

Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 02:23:20PM +0300, Dmitry Baryshkov wrote:
> On 07/06/2023 13:56, Varadarajan Narayanan wrote:
> >Add USB phy and controller nodes
> >
> >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >---
> >  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> >diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> >index c2d6cc65..3183357 100644
> >--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> >+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> >@@ -383,6 +383,61 @@
> >  				status = "disabled";
> >  			};
> >  		};
> >+
> >+		usb_0_m31phy: hs_m31phy@7b000 {
> >+			compatible = "qcom,ipq5332-m31-usb-hsphy";
> >+			reg = <0x0007b000 0x12C>,
> >+			      <0x08af8800 0x400>;
> >+			reg-names = "m31usb_phy_base",
> >+				    "qscratch_base";
> >+			phy_type= "utmi";
> >+
> >+			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> >+			reset-names = "usb2_phy_reset";
> >+
> >+			status = "okay";
> >+		};
> >+
> >+		usb2: usb2@8a00000 {
> >+			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
> >+			#address-cells = <1>;
> >+			#size-cells = <1>;
> >+			ranges;
> >+
> >+			reg = <0x08af8800 0x100>;
> >+
> >+			clocks = <&gcc GCC_USB0_MASTER_CLK>,
> >+				<&gcc GCC_SNOC_USB_CLK>,
> >+				<&gcc GCC_USB0_SLEEP_CLK>,
> >+				<&gcc GCC_USB0_MOCK_UTMI_CLK>;
>
> Please indent these values.

Ok.

> >+
> >+			clock-names = "core",
> >+				"iface",
> >+				"sleep",
> >+				"mock_utmi";
>
> Please indent these values.

Ok.

> >+
> >+			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
>
> No PHY IRQs?

Will add.


Thanks
Varada

> >+			interrupt-names = "pwr_event";
> >+
> >+			resets = <&gcc GCC_USB_BCR>;
> >+
> >+			qcom,select-utmi-as-pipe-clk;
> >+
> >+			usb2_0_dwc: usb@8a00000 {
> >+				compatible = "snps,dwc3";
> >+				reg = <0x08a00000 0xe000>;
> >+				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> >+				clock-names = "ref";
> >+				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> >+				usb-phy = <&usb_0_m31phy>;
> >+				tx-fifo-resize;
> >+				snps,is-utmi-l1-suspend;
> >+				snps,hird-threshold = /bits/ 8 <0x0>;
> >+				snps,dis_u2_susphy_quirk;
> >+				snps,dis_u3_susphy_quirk;
> >+				snps,ref-clock-period-ns = <21>;
> >+			};
> >+		};
> >  	};
> >  	timer {
>
> --
> With best wishes
> Dmitry
>

Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 08:24:04PM +0200, Konrad Dybcio wrote:
>
>
> On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> > Add USB phy and controller nodes
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > index c2d6cc65..3183357 100644
> > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > @@ -383,6 +383,61 @@
> >  				status = "disabled";
> >  			};
> >  		};
> > +
> > +		usb_0_m31phy: hs_m31phy@7b000 {
> > +			compatible = "qcom,ipq5332-m31-usb-hsphy";
> > +			reg = <0x0007b000 0x12C>,
> random uppercase hex

Ok.

> > +			      <0x08af8800 0x400>;
> > +			reg-names = "m31usb_phy_base",
> > +				    "qscratch_base";
> > +			phy_type= "utmi";
> Missing space before '='

Ok.

> > +
> > +			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> > +			reset-names = "usb2_phy_reset";
> > +
> > +			status = "okay";
> If you're only defining the node, it's enabled by default
>
> In this case, you'd probably want to disable it by default.

Ok.

> > +		};
> > +
> > +		usb2: usb2@8a00000 {
> > +			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			ranges;
> Please push these 3 properties to the end
>
> And add status = "disabled" below them.

Ok.

> > +
> > +			reg = <0x08af8800 0x100>;
> > +
> > +			clocks = <&gcc GCC_USB0_MASTER_CLK>,
> > +				<&gcc GCC_SNOC_USB_CLK>,
> > +				<&gcc GCC_USB0_SLEEP_CLK>,
> > +				<&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > +
> Please remove this newline.
>
> > +			clock-names = "core",
> > +				"iface",
> > +				"sleep",
> > +				"mock_utmi";
> Please align this, and all other similar lists.

Ok.

> > +
> > +			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> interrupts-extended is unnecessary with just a single interrupt
> source.. you can make it `interrupts` and drop the GIC reference.
>
> It would also be nice to push the interrupt properties below 'reg'.
> We're working on documenting and automating checking the preferred
> property order.

Ok.

> > +			interrupt-names = "pwr_event";
> > +
> > +			resets = <&gcc GCC_USB_BCR>;
> > +
> > +			qcom,select-utmi-as-pipe-clk;
> > +
> > +			usb2_0_dwc: usb@8a00000 {
> > +				compatible = "snps,dwc3";
> > +				reg = <0x08a00000 0xe000>;
> > +				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > +				clock-names = "ref";
> > +				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> > +				usb-phy = <&usb_0_m31phy>;
> > +				tx-fifo-resize;
> > +				snps,is-utmi-l1-suspend;
> > +				snps,hird-threshold = /bits/ 8 <0x0>;
> > +				snps,dis_u2_susphy_quirk;
> > +				snps,dis_u3_susphy_quirk;
> > +				snps,ref-clock-period-ns = <21>;
> 1/21 is 0.0476..  that doesn't seem to correspond to the ref
> clk frequency?

dwc3_ref_clk_period() prefers ref clock's rate over ref-clock-period-ns.
Since ref clock is available this is not used. Will remove this.

Thanks
Varada

> Konrad
> > +			};
> > +		};
> >  	};
> >
> >  	timer {

Re: [PATCH 7/9] arm64: dts: qcom: ipq5332: Add USB related nodes

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 08:35:09PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Add USB phy and controller nodes
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 55 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > index c2d6cc65..3183357 100644
> > --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> > @@ -383,6 +383,61 @@
> >  				status = "disabled";
> >  			};
> >  		};
> > +
> > +		usb_0_m31phy: hs_m31phy@7b000 {
>
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Ok.

> > +			compatible = "qcom,ipq5332-m31-usb-hsphy";
> > +			reg = <0x0007b000 0x12C>,
> > +			      <0x08af8800 0x400>;
>
> Lowercase hex only.

Ok.

> > +			reg-names = "m31usb_phy_base",
> > +				    "qscratch_base";
> > +			phy_type= "utmi";
> > +
> > +			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> > +			reset-names = "usb2_phy_reset";
> > +
> > +			status = "okay";
>
> It's by default. Drop.

Ok.

> > +		};
> > +
> > +		usb2: usb2@8a00000 {
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Ok.

> > +			compatible = "qcom,ipq5332-dwc3", "qcom,dwc3";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			ranges;
> > +
> > +			reg = <0x08af8800 0x100>;
>
> reg is always after compatible. Ranges is third. Then you will spot that
> address is wrong.

Ok.

> > +
> > +			clocks = <&gcc GCC_USB0_MASTER_CLK>,
> > +				<&gcc GCC_SNOC_USB_CLK>,
> > +				<&gcc GCC_USB0_SLEEP_CLK>,
> > +				<&gcc GCC_USB0_MOCK_UTMI_CLK>;
>
> Fix alignment.

Ok.

> > +
> > +			clock-names = "core",
> > +				"iface",
> > +				"sleep",
> > +				"mock_utmi";
>
> Fix alignment.

Ok.

> > +
> > +			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "pwr_event";
> > +

Thanks
Varada

> Best regards,
> Krzysztof
>

Re: [PATCH 9/9] arm64: defconfig: Enable QCOM M31 USB phy driver

[Varadarajan Narayanan]

On Wed, Jun 07, 2023 at 08:36:21PM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2023 12:56, Varadarajan Narayanan wrote:
> > Enable QCOM M31 USB phy driver present in IPQ5332
>
> What is "QCOM"? If acronym, extend. IPQ5332 - provide full name, so
> "Qualcomm IPQ....".

Will remove 'QCOM'.

Thanks
Varada

> Best regards,
> Krzysztof
>

Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Krzysztof Kozlowski]

On 15/06/2023 07:27, Varadarajan Narayanan wrote:
>>> +          - enum:
>>> +              - qcom,m31-usb-hsphy
>>
>> I am confused what's this. If m31 is coming from some IP block provider,
>> then you are using wrong vendor prefix.
>> https://www.m31tech.com/download_file/M31_USB.pdf
>>
>>
>>> +              - qcom,ipq5332-m31-usb-hsphy
>>
>> This confuses me even more. IPQ m31?
> 
> Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
> Will that be acceptable?

m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31
device have some name/version/model? If it is not really known, then I
would just propose to go with qcom,ipq5332-usb-hsphy.

Skip generic compatible ("usb-hsphy") entirely.

And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
something similar with difference in the hyphen. Just use device
specific compatible thus device specific filename.

> 
>>> +
>>> +  reg:
>>> +    description:
>>> +      Offset and length of the M31 PHY register set
>>
>> Drop description, obvious.
> 
> Ok.
> 
>>> +    maxItems: 2
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: m31usb_phy_base
>>> +      - const: qscratch_base
>>
>> Drop "_base" from both.
> 
> Ok. Will drop qscratch_base. This is in the controller space.
> Should not come here.

Then drop reg-names entirely.

> 
>>> +
>>> +  phy_type:
>>> +    oneOf:
>>> +      - items:
>>> +          - enum:
>>> +              - utmi
>>> +              - ulpi
>>
>> This does not belong to phy, but to USB node.
> 
> This is used by the driver to set a bit during phy init. Hence
> have it as a replication of the USB node's entry. If this is not
> permissible, is there some way to get this from the USB node,
> or any other alternative mechanism?

Shouldn't USB controller choose what type of PHY type it wants?

> 
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
>>> +    hs_m31phy_0: hs_m31phy@5b00 {
>>
>> Node names should be generic. See also explanation and list of examples
>> in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
>> Also, no underscores in node names.
> 
> Will change this as usbphy0:hs_m31phy@7b000

This does not solve my comments. I did not write "label" but "node name".

Best regards,
Krzysztof

Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Varadarajan Narayanan]

On Sat, Jun 17, 2023 at 10:48:41AM +0200, Krzysztof Kozlowski wrote:
> On 15/06/2023 07:27, Varadarajan Narayanan wrote:
> >>> +          - enum:
> >>> +              - qcom,m31-usb-hsphy
> >>
> >> I am confused what's this. If m31 is coming from some IP block provider,
> >> then you are using wrong vendor prefix.
> >> https://www.m31tech.com/download_file/M31_USB.pdf
> >>
> >>
> >>> +              - qcom,ipq5332-m31-usb-hsphy
> >>
> >> This confuses me even more. IPQ m31?
> >
> > Will change this to m31,usb-hsphy and m31,ipq5332-usb-hsphy respectively.
> > Will that be acceptable?
>
> m31,ipq5332 seems wrong, as m31 did not create ipq5332. Does the m31
> device have some name/version/model? If it is not really known, then I
> would just propose to go with qcom,ipq5332-usb-hsphy.
>
> Skip generic compatible ("usb-hsphy") entirely.

Ok.

> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
> something similar with difference in the hyphen. Just use device
> specific compatible thus device specific filename.

qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
driver we are introducing is for UTMI. We would have to
modify phy-qcom-usb-hs.c to accomodate M31. Will that be
acceptable to phy-qcom-usb-hs.c owners/maintainers?

> >>> +
> >>> +  reg:
> >>> +    description:
> >>> +      Offset and length of the M31 PHY register set
> >>
> >> Drop description, obvious.
> >
> > Ok.
> >
> >>> +    maxItems: 2
> >>> +
> >>> +  reg-names:
> >>> +    items:
> >>> +      - const: m31usb_phy_base
> >>> +      - const: qscratch_base
> >>
> >> Drop "_base" from both.
> >
> > Ok. Will drop qscratch_base. This is in the controller space.
> > Should not come here.
>
> Then drop reg-names entirely.

Ok.

> >>> +
> >>> +  phy_type:
> >>> +    oneOf:
> >>> +      - items:
> >>> +          - enum:
> >>> +              - utmi
> >>> +              - ulpi
> >>
> >> This does not belong to phy, but to USB node.
> >
> > This is used by the driver to set a bit during phy init. Hence
> > have it as a replication of the USB node's entry. If this is not
> > permissible, is there some way to get this from the USB node,
> > or any other alternative mechanism?
>
> Shouldn't USB controller choose what type of PHY type it wants?

Will remove this. IPQ5332 uses it in UTMI mode only.

> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> >>> +    hs_m31phy_0: hs_m31phy@5b00 {
> >>
> >> Node names should be generic. See also explanation and list of examples
> >> in DT specification:
> >> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> >>
> >> Also, no underscores in node names.
> >
> > Will change this as usbphy0:hs_m31phy@7b000
>
> This does not solve my comments. I did not write "label" but "node name".

Sorry. will fix it.

Thanks
Varada

Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Krzysztof Kozlowski]

On 20/06/2023 11:32, Varadarajan Narayanan wrote:

> 
>> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
>> something similar with difference in the hyphen. Just use device
>> specific compatible thus device specific filename.
> 
> qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
> driver we are introducing is for UTMI. We would have to
> modify phy-qcom-usb-hs.c to accomodate M31. Will that be
> acceptable to phy-qcom-usb-hs.c owners/maintainers?

We don't talk about drivers here but bindings. Why would you need to
modify the driver when introducing new binding for different device?



Best regards,
Krzysztof

Re: [PATCH 2/9] dt-bindings: phy: qcom,m31: Document qcom,m31 USB phy

[Varadarajan Narayanan]

On Wed, Jun 21, 2023 at 10:43:38AM +0200, Krzysztof Kozlowski wrote:
> On 20/06/2023 11:32, Varadarajan Narayanan wrote:
>
> >
> >> And then we have... existing bindings qcom,usb-hs-phy.yaml. Don't create
> >> something similar with difference in the hyphen. Just use device
> >> specific compatible thus device specific filename.
> >
> > qcom,usb-hs-phy.yaml seems to be for ULPI mode phy and the
> > driver we are introducing is for UTMI. We would have to
> > modify phy-qcom-usb-hs.c to accomodate M31. Will that be
> > acceptable to phy-qcom-usb-hs.c owners/maintainers?
>
> We don't talk about drivers here but bindings. Why would you need to
> modify the driver when introducing new binding for different device?

Sorry. I misunderstood your feedback as "use the existing bindings".

Will name the bindings file as qcom,ipq5332-usb-hsphy.yaml and post
the next version.

Thanks
Varada

Re: [PATCH 3/9] phy: qcom-m31: Introduce qcom,m31 USB phy driver

[Vinod Koul]

On 07-06-23, 13:54, Konrad Dybcio wrote:
> 
> 
> On 7.06.2023 12:56, Varadarajan Narayanan wrote:
> > Add the M31 USB2 phy driver
> > 
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-m31.c | 360 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 360 insertions(+)
> >  create mode 100644 drivers/phy/qualcomm/phy-qcom-m31.c
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-m31.c b/drivers/phy/qualcomm/phy-qcom-m31.c
> > new file mode 100644
> > index 0000000..d29a91e
> > --- /dev/null
> > +++ b/drivers/phy/qualcomm/phy-qcom-m31.c
> > @@ -0,0 +1,360 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2014-2016, 2020, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/phy.h>
> > +#include <linux/reset.h>
> > +#include <linux/of_device.h>
> Please sort these
> 
> > +
> > +enum clk_reset_action {
> > +	CLK_RESET_DEASSERT	= 0,
> > +	CLK_RESET_ASSERT	= 1
> > +};
> > +
> > +#define USB2PHY_PORT_POWERDOWN		0xA4
> > +#define POWER_UP			BIT(0)
> > +#define POWER_DOWN			0
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL1	0x40
> > +
> > +#define USB2PHY_PORT_UTMI_CTRL2	0x44
> > +#define UTMI_ULPI_SEL			BIT(7)
> > +#define UTMI_TEST_MUX_SEL		BIT(6)
> > +
> > +#define HS_PHY_CTRL_REG			0x10
> > +#define UTMI_OTG_VBUS_VALID             BIT(20)
> > +#define SW_SESSVLD_SEL                  BIT(28)
> > +
> > +#define USB_PHY_CFG0			0x94
> > +#define USB_PHY_UTMI_CTRL5		0x50
> > +#define USB_PHY_FSEL_SEL		0xB8
> > +#define USB_PHY_HS_PHY_CTRL_COMMON0	0x54
> > +#define USB_PHY_REFCLK_CTRL		0xA0
> > +#define USB_PHY_HS_PHY_CTRL2		0x64
> > +#define USB_PHY_UTMI_CTRL0		0x3c
> > +#define USB2PHY_USB_PHY_M31_XCFGI_1	0xBC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_4	0xC8
> > +#define USB2PHY_USB_PHY_M31_XCFGI_5	0xCC
> > +#define USB2PHY_USB_PHY_M31_XCFGI_11	0xE4
> Could you sort them address-wise?

and lower case hex values as well please

-- 
~Vinod