[PATCH v8 0/8] Enable IPQ9754 USB

[PATCH v8 0/8] Enable IPQ9754 USB

[Varadarajan Narayanan]

This patch series adds the relevant phy and controller
configurations for enabling USB on IPQ9754

Depends on:
https://lore.kernel.org/all/20230217142030.16012-1-quic_devipriy@quicinc.com/

[v8]:
	- Update bindings for the clock name change
[v7]:
	- com_aux -> cfg_ahb in patch 7

[v6]:
        - Incorporated review comments
	- Resolve make dtbs_check messages
	- Fixed pcs_usb offset
	- Board dts file name changed

[v5]:
        - Incorporated review comments
	- 'make dtbs_check' giving the following messages since
	  ipq9574 doesn't have power domains. Hope this is ok

		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
        	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
        	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml

	- Move qmp phy DT node to newer specification

[v4]:
        - Incorporated review comments
	- Address all 'make dtbs_check' errors

[v3]:
        - Incorporated review comments regarding coding style

[v2]:
        - Incorporated review comments regarding coding style,
          maintaining sorted order of entries and unused phy register
          offsets
        - Removed NOC clock entries from DT node (will be implemented
          later with interconnect support)
        - Fixed 'make dtbs_check' errors/warnings

[v1]:
        https://lore.kernel.org/linux-arm-msm/5dac3aa4-8dc7-f9eb-5cf3-b361efdc9494@linaro.org/T/

Varadarajan Narayanan (8):
  dt-bindings: phy: qcom,qusb2: Document IPQ9574 compatible
  dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY
  dt-bindings: usb: dwc3: Add IPQ9574 compatible
  clk: qcom: gcc-ipq9574: Add USB related clocks
  phy: qcom-qusb2: add QUSB2 support for IPQ9574
  phy: qcom: qmp: Update IPQ9574 USB Phy initialization Sequence
  arm64: dts: qcom: ipq9574: Add USB related nodes
  arm64: dts: qcom: ipq9574: Enable USB

 .../devicetree/bindings/phy/qcom,qusb2-phy.yaml    |   3 +-
 .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        |  43 ++++++--
 .../devicetree/bindings/usb/qcom,dwc3.yaml         |  22 +++-
 arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts        |  16 +++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi              | 120 +++++++++++++++++++++
 drivers/clk/qcom/gcc-ipq9574.c                     |  37 +++++++
 drivers/phy/qualcomm/phy-qcom-qmp-usb.c            | 115 ++++++++++++++++++++
 drivers/phy/qualcomm/phy-qcom-qusb2.c              |   3 +
 include/dt-bindings/clock/qcom,ipq9574-gcc.h       |   2 +
 9 files changed, 353 insertions(+), 8 deletions(-)

-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 1/8] dt-bindings: phy: qcom,qusb2: Document IPQ9574 compatible

[Varadarajan Narayanan]

Document the compatible string used for the qusb2 phy in IPQ9574.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>

---
 Changes in v5:
	- Undo v4 changes

 Changes in v4:
	- Remove constraints not applicable to IPQ9574

 Changes in v3:
	- Maintain the proper sorted order

 Changes in v2:
	- Moved ipq6018 to the proper place and placed ipq9574
	  next to it as suggested by Dmitry
---
 Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
index 543c1a2..95eecba 100644
--- a/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,qusb2-phy.yaml
@@ -18,13 +18,14 @@ properties:
     oneOf:
       - items:
           - enum:
+              - qcom,ipq6018-qusb2-phy
               - qcom,ipq8074-qusb2-phy
+              - qcom,ipq9574-qusb2-phy
               - qcom,msm8953-qusb2-phy
               - qcom,msm8996-qusb2-phy
               - qcom,msm8998-qusb2-phy
               - qcom,qcm2290-qusb2-phy
               - qcom,sdm660-qusb2-phy
-              - qcom,ipq6018-qusb2-phy
               - qcom,sm4250-qusb2-phy
               - qcom,sm6115-qusb2-phy
       - items:
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Varadarajan Narayanan]

Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 Changes in v8:
	- Update clock names for ipq9574

 Changes in v6:
	- Made power-domains optional

Note: In the earlier patch sets, had used the (legacy)
specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
---
 .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
index 16fce10..e902a0d 100644
--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
@@ -16,6 +16,7 @@ description:
 properties:
   compatible:
     enum:
+      - qcom,ipq9574-qmp-usb3-phy
       - qcom,sc8280xp-qmp-usb3-uni-phy
 
   reg:
@@ -25,11 +26,7 @@ properties:
     maxItems: 4
 
   clock-names:
-    items:
-      - const: aux
-      - const: ref
-      - const: com_aux
-      - const: pipe
+    maxItems: 4
 
   power-domains:
     maxItems: 1
@@ -60,7 +57,6 @@ required:
   - reg
   - clocks
   - clock-names
-  - power-domains
   - resets
   - reset-names
   - vdda-phy-supply
@@ -71,6 +67,41 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq9574-qmp-usb3-phy
+    then:
+      properties:
+        clocks:
+          maxItems: 4
+        clock-names:
+          items:
+            - const: aux
+            - const: ref
+            - const: cfg_ahb
+            - const: pipe
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc8280xp-qmp-usb3-uni-phy
+    then:
+      properties:
+        clocks:
+          maxItems: 4
+        clock-names:
+          items:
+            - const: aux
+            - const: ref
+            - const: com_aux
+            - const: pipe
+
 examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-sc8280xp.h>
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 3/8] dt-bindings: usb: dwc3: Add IPQ9574 compatible

[Varadarajan Narayanan]

Document the IPQ9574 dwc3 compatible.

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 Changes in v6:
	- Made power-domains as optional
	- Resolved all 'make dtbs_check' messages

 Changes in v5:
	- Restore removed constraints

 Changes in v4:
	- Update other relevant sections
	- Remove constraints not applicable to IPQ9574
---
 .../devicetree/bindings/usb/qcom,dwc3.yaml         | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index d842819..f5dd268 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -17,6 +17,7 @@ properties:
           - qcom,ipq6018-dwc3
           - qcom,ipq8064-dwc3
           - qcom,ipq8074-dwc3
+          - qcom,ipq9574-dwc3
           - qcom,msm8953-dwc3
           - qcom,msm8994-dwc3
           - qcom,msm8996-dwc3
@@ -133,7 +134,6 @@ required:
   - "#address-cells"
   - "#size-cells"
   - ranges
-  - power-domains
   - clocks
   - clock-names
   - interrupts
@@ -197,6 +197,26 @@ allOf:
             - const: iface
             - const: sleep
             - const: mock_utmi
+      required:
+        - power-domains
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq9574-dwc3
+    then:
+      properties:
+        clocks:
+          maxItems: 5
+        clock-names:
+          items:
+            - const: cfg_noc
+            - const: core
+            - const: iface
+            - const: sleep
+            - const: mock_utmi
 
   - if:
       properties:
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 4/8] clk: qcom: gcc-ipq9574: Add USB related clocks

[Varadarajan Narayanan]

Add the clocks needed for enabling USB in IPQ9574

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 Changes in v2:
	- Fixed coding style issues
---
 drivers/clk/qcom/gcc-ipq9574.c               | 37 ++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,ipq9574-gcc.h |  2 ++
 2 files changed, 39 insertions(+)

diff --git a/drivers/clk/qcom/gcc-ipq9574.c b/drivers/clk/qcom/gcc-ipq9574.c
index c855954..f19e9cb 100644
--- a/drivers/clk/qcom/gcc-ipq9574.c
+++ b/drivers/clk/qcom/gcc-ipq9574.c
@@ -2041,6 +2041,41 @@ static struct clk_regmap_mux usb0_pipe_clk_src = {
 	},
 };
 
+static struct clk_branch gcc_usb0_pipe_clk = {
+	.halt_reg = 0x2c054,
+	.halt_check = BRANCH_HALT_DELAY,
+	.clkr = {
+		.enable_reg = 0x2c054,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_usb0_pipe_clk",
+			.parent_hws = (const struct clk_hw *[]) {
+				&usb0_pipe_clk_src.clkr.hw
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
+static struct clk_branch gcc_usb0_sleep_clk = {
+	.halt_reg = 0x2c058,
+	.clkr = {
+		.enable_reg = 0x2c058,
+		.enable_mask = BIT(0),
+		.hw.init = &(struct clk_init_data){
+			.name = "gcc_usb0_sleep_clk",
+			.parent_hws = (const struct clk_hw *[]) {
+				&gcc_sleep_clk_src.clkr.hw
+			},
+			.num_parents = 1,
+			.flags = CLK_SET_RATE_PARENT,
+			.ops = &clk_branch2_ops,
+		},
+	},
+};
+
 static const struct freq_tbl ftbl_sdcc_apps_clk_src[] = {
 	F(144000, P_XO, 16, 12, 125),
 	F(400000, P_XO, 12, 1, 5),
@@ -4008,6 +4043,8 @@ static struct clk_regmap *gcc_ipq9574_clks[] = {
 	[GCC_USB0_MOCK_UTMI_CLK] = &gcc_usb0_mock_utmi_clk.clkr,
 	[USB0_PIPE_CLK_SRC] = &usb0_pipe_clk_src.clkr,
 	[GCC_USB0_PHY_CFG_AHB_CLK] = &gcc_usb0_phy_cfg_ahb_clk.clkr,
+	[GCC_USB0_PIPE_CLK] = &gcc_usb0_pipe_clk.clkr,
+	[GCC_USB0_SLEEP_CLK] = &gcc_usb0_sleep_clk.clkr,
 	[SDCC1_APPS_CLK_SRC] = &sdcc1_apps_clk_src.clkr,
 	[GCC_SDCC1_APPS_CLK] = &gcc_sdcc1_apps_clk.clkr,
 	[SDCC1_ICE_CORE_CLK_SRC] = &sdcc1_ice_core_clk_src.clkr,
diff --git a/include/dt-bindings/clock/qcom,ipq9574-gcc.h b/include/dt-bindings/clock/qcom,ipq9574-gcc.h
index 2d7b460..2cb02f7 100644
--- a/include/dt-bindings/clock/qcom,ipq9574-gcc.h
+++ b/include/dt-bindings/clock/qcom,ipq9574-gcc.h
@@ -214,4 +214,6 @@
 #define GCC_PCIE1_PIPE_CLK				205
 #define GCC_PCIE2_PIPE_CLK				206
 #define GCC_PCIE3_PIPE_CLK				207
+#define GCC_USB0_PIPE_CLK				208
+#define GCC_USB0_SLEEP_CLK				209
 #endif
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 5/8] phy: qcom-qusb2: add QUSB2 support for IPQ9574

[Varadarajan Narayanan]

Add the phy init sequence for the Super Speed ports found
on IPQ9574.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
  Changes in v2:
	- Place the entry such that the list continues to be sorted
---
 drivers/phy/qualcomm/phy-qcom-qusb2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 2ef638b..bec6e40 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -912,6 +912,9 @@ static const struct of_device_id qusb2_phy_of_match_table[] = {
 		.compatible	= "qcom,ipq8074-qusb2-phy",
 		.data		= &msm8996_phy_cfg,
 	}, {
+		.compatible	= "qcom,ipq9574-qusb2-phy",
+		.data		= &ipq6018_phy_cfg,
+	}, {
 		.compatible	= "qcom,msm8953-qusb2-phy",
 		.data		= &msm8996_phy_cfg,
 	}, {
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 6/8] phy: qcom: qmp: Update IPQ9574 USB Phy initialization Sequence

[Varadarajan Narayanan]

Updated USB QMP PHY Init sequence based on HPG for IPQ9574.
Reused clock and reset list from existing targets.

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 Changes in v6:
	- Fix pcs_usb offset
	- Use correct clock names array
 Changes in v5:
	- Fix additional review comments
	- Use V3 register offsets
 Changes in v4:
	- Use qmp_usb_offsets for register space access
 Changes in v3:
	- Fix hex captitalization
 Changes in v2:
	- Removed unused phy register offsets
	- Moved the clock entries to the correct place
	- Maintain sorted order
---
 drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 115 ++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
index a49711c..11a76a4 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
@@ -139,6 +139,88 @@ static const unsigned int qmp_v5_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR] = QPHY_V5_PCS_USB3_LFPS_RXTERM_IRQ_CLEAR,
 };
 
+static const struct qmp_phy_init_tbl ipq9574_usb3_serdes_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x1a),
+	QMP_PHY_INIT_CFG(QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x08),
+	QMP_PHY_INIT_CFG(QSERDES_COM_CLK_SELECT, 0x30),
+	QMP_PHY_INIT_CFG(QSERDES_COM_BG_TRIM, 0x0f),
+	QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_FASTLOCK_FO_GAIN, 0x0b),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SVS_MODE_CLK_SEL, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_COM_HSCLK_SEL, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_COM_CMN_CONFIG, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_COM_PLL_IVCO, 0x0f),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SYS_CLK_CTRL, 0x06),
+	/* PLL and Loop filter settings */
+	QMP_PHY_INIT_CFG(QSERDES_COM_DEC_START_MODE0, 0x68),
+	QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START1_MODE0, 0xab),
+	QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START2_MODE0, 0xaa),
+	QMP_PHY_INIT_CFG(QSERDES_COM_DIV_FRAC_START3_MODE0, 0x02),
+	QMP_PHY_INIT_CFG(QSERDES_COM_CP_CTRL_MODE0, 0x09),
+	QMP_PHY_INIT_CFG(QSERDES_COM_PLL_RCTRL_MODE0, 0x16),
+	QMP_PHY_INIT_CFG(QSERDES_COM_PLL_CCTRL_MODE0, 0x28),
+	QMP_PHY_INIT_CFG(QSERDES_COM_INTEGLOOP_GAIN0_MODE0, 0xa0),
+	QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP1_MODE0, 0xaa),
+	QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP2_MODE0, 0x29),
+	QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP3_MODE0, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_COM_CORE_CLK_EN, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_COM_LOCK_CMP_CFG, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_COM_VCO_TUNE_MAP, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_COM_BG_TIMER, 0x0a),
+	/* SSC settings */
+	QMP_PHY_INIT_CFG(QSERDES_COM_SSC_EN_CENTER, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER1, 0x7d),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SSC_PER2, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER1, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SSC_ADJ_PER2, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE1, 0x0a),
+	QMP_PHY_INIT_CFG(QSERDES_COM_SSC_STEP_SIZE2, 0x05),
+};
+
+static const struct qmp_phy_init_tbl ipq9574_usb3_tx_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_TX_HIGHZ_TRANSCEIVEREN_BIAS_DRVR_EN, 0x45),
+	QMP_PHY_INIT_CFG(QSERDES_TX_RCV_DETECT_LVL_2, 0x12),
+	QMP_PHY_INIT_CFG(QSERDES_TX_LANE_MODE, 0x06),
+};
+
+static const struct qmp_phy_init_tbl ipq9574_usb3_rx_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_RX_UCDR_SO_GAIN, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL2, 0x02),
+	QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL3, 0x6c),
+	QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL3, 0x4c),
+	QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQU_ADAPTOR_CNTRL4, 0xb8),
+	QMP_PHY_INIT_CFG(QSERDES_RX_RX_EQ_OFFSET_ADAPTOR_CNTRL1, 0x77),
+	QMP_PHY_INIT_CFG(QSERDES_RX_RX_OFFSET_ADAPTOR_CNTRL2, 0x80),
+	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_CNTRL, 0x03),
+	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_DEGLITCH_CNTRL, 0x16),
+	QMP_PHY_INIT_CFG(QSERDES_RX_SIGDET_ENABLES, 0x0c),
+};
+
+static const struct qmp_phy_init_tbl ipq9574_usb3_pcs_tbl[] = {
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V0, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V0, 0x0e),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNTRL2, 0x83),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNTRL1, 0x02),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNT_VAL_L, 0x09),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNT_VAL_H_TOL, 0xa2),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_MAN_CODE, 0x85),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG1, 0xd1),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG2, 0x1f),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG3, 0x47),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_POWER_STATE_CONFIG2, 0x1b),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_WAIT_TIME, 0x75),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_RUN_TIME, 0x13),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LFPS_TX_ECSTART_EQTLOCK, 0x86),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_PWRUP_RESET_DLY_TIME_AUXCLK, 0x04),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TSYNC_RSYNC_TIME, 0x44),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_P1U2_L, 0xe7),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_P1U2_H, 0x03),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_U3_L, 0x40),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_U3_H, 0x00),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RX_SIGDET_LVL, 0x88),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V0, 0x17),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V0, 0x0f),
+};
+
 static const struct qmp_phy_init_tbl ipq8074_usb3_serdes_tbl[] = {
 	QMP_PHY_INIT_CFG(QSERDES_COM_SYSCLK_EN_SEL, 0x1a),
 	QMP_PHY_INIT_CFG(QSERDES_COM_BIAS_EN_CLKBUFLR_EN, 0x08),
@@ -1558,6 +1640,14 @@ static const char * const qmp_phy_vreg_l[] = {
 	"vdda-phy", "vdda-pll",
 };
 
+static const struct qmp_usb_offsets qmp_usb_offsets_ipq9574 = {
+	.serdes		= 0,
+	.pcs		= 0x800,
+	.pcs_usb	= 0x800,
+	.tx		= 0x200,
+	.rx		= 0x400,
+};
+
 static const struct qmp_usb_offsets qmp_usb_offsets_v5 = {
 	.serdes		= 0,
 	.pcs		= 0x0200,
@@ -1586,6 +1676,28 @@ static const struct qmp_phy_cfg ipq8074_usb3phy_cfg = {
 	.regs			= qmp_v3_usb3phy_regs_layout,
 };
 
+static const struct qmp_phy_cfg ipq9574_usb3phy_cfg = {
+	.lanes			= 1,
+
+	.offsets		= &qmp_usb_offsets_ipq9574,
+
+	.serdes_tbl		= ipq9574_usb3_serdes_tbl,
+	.serdes_tbl_num		= ARRAY_SIZE(ipq9574_usb3_serdes_tbl),
+	.tx_tbl			= ipq9574_usb3_tx_tbl,
+	.tx_tbl_num		= ARRAY_SIZE(ipq9574_usb3_tx_tbl),
+	.rx_tbl			= ipq9574_usb3_rx_tbl,
+	.rx_tbl_num		= ARRAY_SIZE(ipq9574_usb3_rx_tbl),
+	.pcs_tbl		= ipq9574_usb3_pcs_tbl,
+	.pcs_tbl_num		= ARRAY_SIZE(ipq9574_usb3_pcs_tbl),
+	.clk_list		= msm8996_phy_clk_l,
+	.num_clks		= ARRAY_SIZE(msm8996_phy_clk_l),
+	.reset_list		= qcm2290_usb3phy_reset_l,
+	.num_resets		= ARRAY_SIZE(qcm2290_usb3phy_reset_l),
+	.vreg_list		= qmp_phy_vreg_l,
+	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
+	.regs			= qmp_v3_usb3phy_regs_layout,
+};
+
 static const struct qmp_phy_cfg msm8996_usb3phy_cfg = {
 	.lanes			= 1,
 
@@ -2589,6 +2701,9 @@ static const struct of_device_id qmp_usb_of_match_table[] = {
 		.compatible = "qcom,ipq8074-qmp-usb3-phy",
 		.data = &ipq8074_usb3phy_cfg,
 	}, {
+		.compatible = "qcom,ipq9574-qmp-usb3-phy",
+		.data = &ipq9574_usb3phy_cfg,
+	}, {
 		.compatible = "qcom,msm8996-qmp-usb3-phy",
 		.data = &msm8996_usb3phy_cfg,
 	}, {
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes

[Varadarajan Narayanan]

Add USB phy and controller related nodes

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 Changes in v8:
	- Change clocks order to match the bindings
 Changes in v7:
	- Change com_aux -> cfg_ahb
 Changes in v6:
	- Introduce fixed regulators for the phy
	- Resolved all 'make dtbs_check' messages

 Changes in v5:
	- Fix additional comments
	- Edit nodes to match with qcom,sc8280xp-qmp-usb3-uni-phy.yaml
	- 'make dtbs_check' giving the following messages since
	  ipq9574 doesn't have power domains. Hope this is ok

		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
        	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
        	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml

 Changes in v4:
	- Use newer bindings without subnodes
	- Fix coding style issues

 Changes in v3:
	- Insert the nodes at proper location

 Changes in v2:
	- Fixed issues flagged by Krzysztof
	- Fix issues reported by make dtbs_check
	- Remove NOC related clocks (to be added with proper
	  interconnect support)
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 120 ++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 43a3dbe..1242382 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -150,6 +150,33 @@
 		method = "smc";
 	};
 
+	reg_usb_3p3: s3300 {
+		compatible = "regulator-fixed";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+		regulator-always-on;
+		regulator-name = "usb-phy-vdd-dummy";
+	};
+
+	reg_usb_1p8: s1800 {
+		compatible = "regulator-fixed";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		regulator-boot-on;
+		regulator-always-on;
+		regulator-name = "usb-phy-pll-dummy";
+	};
+
+	reg_usb_0p925: s0925 {
+		compatible = "regulator-fixed";
+		regulator-min-microvolt = <925000>;
+		regulator-max-microvolt = <925000>;
+		regulator-boot-on;
+		regulator-always-on;
+		regulator-name = "usb-phy-dummy";
+	};
+
 	reserved-memory {
 		#address-cells = <2>;
 		#size-cells = <2>;
@@ -179,6 +206,52 @@
 		#size-cells = <1>;
 		ranges = <0 0 0 0xffffffff>;
 
+		usb_0_qusbphy: phy@7b000 {
+			compatible = "qcom,ipq9574-qusb2-phy";
+			reg = <0x0007b000 0x180>;
+			#phy-cells = <0>;
+
+			clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
+				 <&xo_board_clk>;
+			clock-names = "cfg_ahb",
+				      "ref";
+
+			vdd-supply = <&reg_usb_0p925>;
+			vdda-pll-supply = <&reg_usb_1p8>;
+			vdda-phy-dpdm-supply = <&reg_usb_3p3>;
+
+			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
+			status = "disabled";
+		};
+
+		usb_0_qmpphy: phy@7d000 {
+			compatible = "qcom,ipq9574-qmp-usb3-phy";
+			reg = <0x0007d000 0xa00>;
+			#phy-cells = <0>;
+
+			clocks = <&gcc GCC_USB0_AUX_CLK>,
+				 <&xo_board_clk>,
+				 <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
+				 <&gcc GCC_USB0_PIPE_CLK>;
+			clock-names = "aux",
+				      "ref",
+				      "cfg_ahb",
+				      "pipe";
+
+			resets = <&gcc GCC_USB0_PHY_BCR>,
+				 <&gcc GCC_USB3PHY_0_PHY_BCR>;
+			reset-names = "phy",
+				      "phy_phy";
+
+			vdda-pll-supply = <&reg_usb_1p8>;
+			vdda-phy-supply = <&reg_usb_0p925>;
+
+			status = "disabled";
+
+			#clock-cells = <0>;
+			clock-output-names = "usb0_pipe_clk";
+		};
+
 		pcie0_phy: phy@84000 {
 			compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy";
 			reg = <0x00084000 0x1000>;
@@ -548,6 +621,53 @@
 			status = "disabled";
 		};
 
+		usb3: usb@8a00000 {
+			compatible = "qcom,ipq9574-dwc3", "qcom,dwc3";
+			reg = <0x08af8800 0x400>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			clocks = <&gcc GCC_SNOC_USB_CLK>,
+				 <&gcc GCC_USB0_MASTER_CLK>,
+				 <&gcc GCC_ANOC_USB_AXI_CLK>,
+				 <&gcc GCC_USB0_SLEEP_CLK>,
+				 <&gcc GCC_USB0_MOCK_UTMI_CLK>;
+
+			clock-names = "cfg_noc",
+				      "core",
+				      "iface",
+				      "sleep",
+				      "mock_utmi";
+
+			assigned-clocks = <&gcc GCC_USB0_MASTER_CLK>,
+					  <&gcc GCC_USB0_MOCK_UTMI_CLK>;
+			assigned-clock-rates = <200000000>,
+					       <24000000>;
+
+			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "pwr_event";
+
+			resets = <&gcc GCC_USB_BCR>;
+			status = "disabled";
+
+			dwc_0: usb@8a00000 {
+				compatible = "snps,dwc3";
+				reg = <0x8a00000 0xcd00>;
+				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
+				clock-names = "ref";
+				interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
+				phys = <&usb_0_qusbphy>, <&usb_0_qmpphy>;
+				phy-names = "usb2-phy", "usb3-phy";
+				tx-fifo-resize;
+				snps,is-utmi-l1-suspend;
+				snps,hird-threshold = /bits/ 8 <0x0>;
+				snps,dis_u2_susphy_quirk;
+				snps,dis_u3_susphy_quirk;
+				dr_mode = "host";
+			};
+		};
+
 		intc: interrupt-controller@b000000 {
 			compatible = "qcom,msm-qgic2";
 			reg = <0x0b000000 0x1000>,  /* GICD */
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v8 8/8] arm64: dts: qcom: ipq9574: Enable USB

[Varadarajan Narayanan]

Turn on USB related nodes

Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
---
 Changes in v5:
	- Move "host" mode specification to board dts
	- Due to dependency with earlier patches board dts
	  filename changed ipq9574-al02-c7.dts -> ipq9574-rdp433.dts

 Changes in v2:
	- Fix node placement and coding style
	- "ok" -> "okay"
---
 arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
index 859973b..92ff7ee 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
+++ b/arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts
@@ -44,6 +44,10 @@
 	};
 };
 
+&dwc_0 {
+	dr_mode = "host";
+};
+
 &pcie1_phy {
 	status = "okay";
 };
@@ -107,6 +111,14 @@
 	clock-frequency = <32000>;
 };
 
+&usb_0_qmpphy {
+	status = "okay";
+};
+
+&usb_0_qusbphy {
+	status = "okay";
+};
+
 &tlmm {
 
 	pcie_1_pin: pcie-1-state {
@@ -173,6 +185,10 @@
 	};
 };
 
+&usb3 {
+	status = "okay";
+};
+
 &xo_board_clk {
 	clock-frequency = <24000000>;
 };
-- 
2.7.4


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 0/8] Enable IPQ9754 USB

[Krzysztof Kozlowski]

On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> This patch series adds the relevant phy and controller
> configurations for enabling USB on IPQ9754
> 
> Depends on:
> https://lore.kernel.org/all/20230217142030.16012-1-quic_devipriy@quicinc.com/
> 
> [v8]:
> 	- Update bindings for the clock name change
> [v7]:
> 	- com_aux -> cfg_ahb in patch 7
> 
> [v6]:
>         - Incorporated review comments
> 	- Resolve make dtbs_check messages
> 	- Fixed pcs_usb offset
> 	- Board dts file name changed
> 

Thanks for the patches. Three submissions within few hours it is a bit
too many. Please slow down a bit, give reviewers chance to respond and
wait with new versions at least one day.
Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Krzysztof Kozlowski]

On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  Changes in v8:
> 	- Update clock names for ipq9574
> 
>  Changes in v6:
> 	- Made power-domains optional
> 
> Note: In the earlier patch sets, had used the (legacy)
> specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
> to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> ---
>  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> index 16fce10..e902a0d 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> @@ -16,6 +16,7 @@ description:
>  properties:
>    compatible:
>      enum:
> +      - qcom,ipq9574-qmp-usb3-phy
>        - qcom,sc8280xp-qmp-usb3-uni-phy
>  
>    reg:
> @@ -25,11 +26,7 @@ properties:
>      maxItems: 4
>  
>    clock-names:
> -    items:
> -      - const: aux
> -      - const: ref
> -      - const: com_aux
> -      - const: pipe
> +    maxItems: 4
>  
>    power-domains:
>      maxItems: 1
> @@ -60,7 +57,6 @@ required:
>    - reg
>    - clocks
>    - clock-names
> -  - power-domains
>    - resets
>    - reset-names
>    - vdda-phy-supply
> @@ -71,6 +67,41 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:

As you can see in example-schema, allOf goes before
additionalProperties: false.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq9574-qmp-usb3-phy
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 4

Don't need clocks here.

> +        clock-names:
> +          items:
> +            - const: aux
> +            - const: ref
> +            - const: cfg_ahb
> +            - const: pipe
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sc8280xp-qmp-usb3-uni-phy
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 4

Neither here.

> +        clock-names:
> +          items:
> +            - const: aux
> +            - const: ref
> +            - const: com_aux

Can anyone explain me why do we name these (here and other Qualcomm
bindings) based on clock name, not input? Just because different clock
is fed to the block, does not necessarily mean the input should be named
differently.

> +            - const: pipe
> +
>  examples:
>    - |
>      #include <dt-bindings/clock/qcom,gcc-sc8280xp.h>

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Krzysztof Kozlowski]

On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  Changes in v8:
> 	- Update clock names for ipq9574
> 
>  Changes in v6:
> 	- Made power-domains optional
> 
> Note: In the earlier patch sets, had used the (legacy)
> specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
> to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> ---
>  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> index 16fce10..e902a0d 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> @@ -16,6 +16,7 @@ description:
>  properties:
>    compatible:
>      enum:
> +      - qcom,ipq9574-qmp-usb3-phy
>        - qcom,sc8280xp-qmp-usb3-uni-phy
>  
>    reg:
> @@ -25,11 +26,7 @@ properties:
>      maxItems: 4
>  
>    clock-names:
> -    items:
> -      - const: aux
> -      - const: ref
> -      - const: com_aux
> -      - const: pipe
> +    maxItems: 4
>  
>    power-domains:
>      maxItems: 1
> @@ -60,7 +57,6 @@ required:
>    - reg
>    - clocks
>    - clock-names
> -  - power-domains

Power domains are required. Commit msg does not explain why this should
be now optional.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 3/8] dt-bindings: usb: dwc3: Add IPQ9574 compatible

[Krzysztof Kozlowski]

On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> Document the IPQ9574 dwc3 compatible.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>  Changes in v6:
> 	- Made power-domains as optional
> 	- Resolved all 'make dtbs_check' messages
> 
>  Changes in v5:
> 	- Restore removed constraints
> 
>  Changes in v4:
> 	- Update other relevant sections
> 	- Remove constraints not applicable to IPQ9574
> ---
>  .../devicetree/bindings/usb/qcom,dwc3.yaml         | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index d842819..f5dd268 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -17,6 +17,7 @@ properties:
>            - qcom,ipq6018-dwc3
>            - qcom,ipq8064-dwc3
>            - qcom,ipq8074-dwc3
> +          - qcom,ipq9574-dwc3
>            - qcom,msm8953-dwc3
>            - qcom,msm8994-dwc3
>            - qcom,msm8996-dwc3
> @@ -133,7 +134,6 @@ required:
>    - "#address-cells"
>    - "#size-cells"
>    - ranges
> -  - power-domains


Power domains are required. Commit msg does not explain why this should
be now optional.

>    - clocks
>    - clock-names
>    - interrupts
> @@ -197,6 +197,26 @@ allOf:
>              - const: iface
>              - const: sleep
>              - const: mock_utmi
> +      required:
> +        - power-domains
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,ipq9574-dwc3

You do not need new entry. Just open the file and file respective
existing if.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Johan Hovold]

On Thu, Apr 06, 2023 at 09:41:49AM +0200, Krzysztof Kozlowski wrote:
> On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> > Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
> > 
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  Changes in v8:
> > 	- Update clock names for ipq9574
> > 
> >  Changes in v6:
> > 	- Made power-domains optional
> > 
> > Note: In the earlier patch sets, had used the (legacy)
> > specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
> > to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > ---
> >  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
> >  1 file changed, 37 insertions(+), 6 deletions(-)

> > +        clock-names:
> > +          items:
> > +            - const: aux
> > +            - const: ref
> > +            - const: com_aux
> 
> Can anyone explain me why do we name these (here and other Qualcomm
> bindings) based on clock name, not input? Just because different clock
> is fed to the block, does not necessarily mean the input should be named
> differently.

I guess part of the answer is that this has just been copied from the
vendor dts and (almost) no one but Qualcomm has access to the
documentation. What would the input names be here?

Also note that there are SoCs that enable both 'cfg_ahb' and 'com_aux'
(e.g. sc7180).

> > +            - const: pipe
> > +
> >  examples:
> >    - |
> >      #include <dt-bindings/clock/qcom,gcc-sc8280xp.h>

Johan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Varadarajan Narayanan]

On Mon, Apr 17, 2023 at 10:05:11AM +0200, Johan Hovold wrote:
> On Thu, Apr 06, 2023 at 09:41:49AM +0200, Krzysztof Kozlowski wrote:
> > On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> > > Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
> > >
> > > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > > ---
> > >  Changes in v8:
> > > 	- Update clock names for ipq9574
> > >
> > >  Changes in v6:
> > > 	- Made power-domains optional
> > >
> > > Note: In the earlier patch sets, had used the (legacy)
> > > specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
> > > to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > > ---
> > >  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
> > >  1 file changed, 37 insertions(+), 6 deletions(-)
>
> > > +        clock-names:
> > > +          items:
> > > +            - const: aux
> > > +            - const: ref
> > > +            - const: com_aux
> >
> > Can anyone explain me why do we name these (here and other Qualcomm
> > bindings) based on clock name, not input? Just because different clock
> > is fed to the block, does not necessarily mean the input should be named
> > differently.
>
> I guess part of the answer is that this has just been copied from the
> vendor dts and (almost) no one but Qualcomm has access to the
> documentation. What would the input names be here?
>
> Also note that there are SoCs that enable both 'cfg_ahb' and 'com_aux'
> (e.g. sc7180).

The clock name definitions are auto-generated based on the clock
tree definitions provided by the h/w team. We followed the naming
pattern done in the previous SoCs.

Thanks
Varada

>
> > > +            - const: pipe
> > > +
> > >  examples:
> > >    - |
> > >      #include <dt-bindings/clock/qcom,gcc-sc8280xp.h>
>
> Johan

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Varadarajan Narayanan]

On Thu, Apr 06, 2023 at 09:42:31AM +0200, Krzysztof Kozlowski wrote:
> On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> > Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  Changes in v8:
> > 	- Update clock names for ipq9574
> >
> >  Changes in v6:
> > 	- Made power-domains optional
> >
> > Note: In the earlier patch sets, had used the (legacy)
> > specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
> > to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > ---
> >  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
> >  1 file changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > index 16fce10..e902a0d 100644
> > --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > @@ -16,6 +16,7 @@ description:
> >  properties:
> >    compatible:
> >      enum:
> > +      - qcom,ipq9574-qmp-usb3-phy
> >        - qcom,sc8280xp-qmp-usb3-uni-phy
> >
> >    reg:
> > @@ -25,11 +26,7 @@ properties:
> >      maxItems: 4
> >
> >    clock-names:
> > -    items:
> > -      - const: aux
> > -      - const: ref
> > -      - const: com_aux
> > -      - const: pipe
> > +    maxItems: 4
> >
> >    power-domains:
> >      maxItems: 1
> > @@ -60,7 +57,6 @@ required:
> >    - reg
> >    - clocks
> >    - clock-names
> > -  - power-domains
>
> Power domains are required. Commit msg does not explain why this should
> be now optional.

Since IPQ9574 doesn't have power switches couldn't provide power-domains details.
So, had to make it optional to pass 'make dtbs_check'.

Thanks
Varada

> Best regards,
> Krzysztof
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 3/8] dt-bindings: usb: dwc3: Add IPQ9574 compatible

[Varadarajan Narayanan]

On Thu, Apr 06, 2023 at 09:43:48AM +0200, Krzysztof Kozlowski wrote:
> On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> > Document the IPQ9574 dwc3 compatible.
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > ---
> >  Changes in v6:
> > 	- Made power-domains as optional
> > 	- Resolved all 'make dtbs_check' messages
> >
> >  Changes in v5:
> > 	- Restore removed constraints
> >
> >  Changes in v4:
> > 	- Update other relevant sections
> > 	- Remove constraints not applicable to IPQ9574
> > ---
> >  .../devicetree/bindings/usb/qcom,dwc3.yaml         | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > index d842819..f5dd268 100644
> > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > @@ -17,6 +17,7 @@ properties:
> >            - qcom,ipq6018-dwc3
> >            - qcom,ipq8064-dwc3
> >            - qcom,ipq8074-dwc3
> > +          - qcom,ipq9574-dwc3
> >            - qcom,msm8953-dwc3
> >            - qcom,msm8994-dwc3
> >            - qcom,msm8996-dwc3
> > @@ -133,7 +134,6 @@ required:
> >    - "#address-cells"
> >    - "#size-cells"
> >    - ranges
> > -  - power-domains
>
>
> Power domains are required. Commit msg does not explain why this should
> be now optional.

Since IPQ9574 doesn't have power switches, couldn't provide power-domains info.
So, had to make it optional to pass 'make dtbs_check'.

Will post patch with updated commit msg.

> >    - clocks
> >    - clock-names
> >    - interrupts
> > @@ -197,6 +197,26 @@ allOf:
> >              - const: iface
> >              - const: sleep
> >              - const: mock_utmi
> > +      required:
> > +        - power-domains
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,ipq9574-dwc3
>
> You do not need new entry. Just open the file and file respective
> existing if.

Sure. Will fix this and post.

Thanks
Varada

> Best regards,
> Krzysztof

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Dmitry Baryshkov]

On 21/04/2023 13:13, Varadarajan Narayanan wrote:
> On Thu, Apr 06, 2023 at 09:42:31AM +0200, Krzysztof Kozlowski wrote:
>> On 05/04/2023 13:41, Varadarajan Narayanan wrote:
>>> Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
>>>
>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>> ---
>>>   Changes in v8:
>>> 	- Update clock names for ipq9574
>>>
>>>   Changes in v6:
>>> 	- Made power-domains optional
>>>
>>> Note: In the earlier patch sets, had used the (legacy)
>>> specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
>>> to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>> ---
>>>   .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
>>>   1 file changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>> index 16fce10..e902a0d 100644
>>> --- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>> @@ -16,6 +16,7 @@ description:
>>>   properties:
>>>     compatible:
>>>       enum:
>>> +      - qcom,ipq9574-qmp-usb3-phy
>>>         - qcom,sc8280xp-qmp-usb3-uni-phy
>>>
>>>     reg:
>>> @@ -25,11 +26,7 @@ properties:
>>>       maxItems: 4
>>>
>>>     clock-names:
>>> -    items:
>>> -      - const: aux
>>> -      - const: ref
>>> -      - const: com_aux
>>> -      - const: pipe
>>> +    maxItems: 4
>>>
>>>     power-domains:
>>>       maxItems: 1
>>> @@ -60,7 +57,6 @@ required:
>>>     - reg
>>>     - clocks
>>>     - clock-names
>>> -  - power-domains
>>
>> Power domains are required. Commit msg does not explain why this should
>> be now optional.
> 
> Since IPQ9574 doesn't have power switches couldn't provide power-domains details.
> So, had to make it optional to pass 'make dtbs_check'.

This should be a part of the commit message, so that the next developer 
understands your intentions without going to mail archives.

> 
> Thanks
> Varada
> 
>> Best regards,
>> Krzysztof
>>

-- 
With best wishes
Dmitry


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Krzysztof Kozlowski]

On 21/04/2023 11:58, Varadarajan Narayanan wrote:
> On Mon, Apr 17, 2023 at 10:05:11AM +0200, Johan Hovold wrote:
>> On Thu, Apr 06, 2023 at 09:41:49AM +0200, Krzysztof Kozlowski wrote:
>>> On 05/04/2023 13:41, Varadarajan Narayanan wrote:
>>>> Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
>>>>
>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>> ---
>>>>  Changes in v8:
>>>> 	- Update clock names for ipq9574
>>>>
>>>>  Changes in v6:
>>>> 	- Made power-domains optional
>>>>
>>>> Note: In the earlier patch sets, had used the (legacy)
>>>> specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
>>>> to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>> ---
>>>>  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
>>>>  1 file changed, 37 insertions(+), 6 deletions(-)
>>
>>>> +        clock-names:
>>>> +          items:
>>>> +            - const: aux
>>>> +            - const: ref
>>>> +            - const: com_aux
>>>
>>> Can anyone explain me why do we name these (here and other Qualcomm
>>> bindings) based on clock name, not input? Just because different clock
>>> is fed to the block, does not necessarily mean the input should be named
>>> differently.
>>
>> I guess part of the answer is that this has just been copied from the
>> vendor dts and (almost) no one but Qualcomm has access to the
>> documentation. What would the input names be here?
>>
>> Also note that there are SoCs that enable both 'cfg_ahb' and 'com_aux'
>> (e.g. sc7180).
> 
> The clock name definitions are auto-generated based on the clock
> tree definitions provided by the h/w team. We followed the naming
> pattern done in the previous SoCs.

Are you sure? We talk about clock inputs here.

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes

[Dmitry Baryshkov]

On 05/04/2023 14:41, Varadarajan Narayanan wrote:
> Add USB phy and controller related nodes
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>   Changes in v8:
> 	- Change clocks order to match the bindings
>   Changes in v7:
> 	- Change com_aux -> cfg_ahb
>   Changes in v6:
> 	- Introduce fixed regulators for the phy
> 	- Resolved all 'make dtbs_check' messages
> 
>   Changes in v5:
> 	- Fix additional comments
> 	- Edit nodes to match with qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> 	- 'make dtbs_check' giving the following messages since
> 	  ipq9574 doesn't have power domains. Hope this is ok
> 
> 		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
>          	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> 		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
>          	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> 
>   Changes in v4:
> 	- Use newer bindings without subnodes
> 	- Fix coding style issues
> 
>   Changes in v3:
> 	- Insert the nodes at proper location
> 
>   Changes in v2:
> 	- Fixed issues flagged by Krzysztof
> 	- Fix issues reported by make dtbs_check
> 	- Remove NOC related clocks (to be added with proper
> 	  interconnect support)
> ---
>   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 120 ++++++++++++++++++++++++++++++++++
>   1 file changed, 120 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> index 43a3dbe..1242382 100644
> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> @@ -150,6 +150,33 @@
>   		method = "smc";
>   	};
>   
> +	reg_usb_3p3: s3300 {

The node names do not look generic enough. Please take a look at other 
platforms.

> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		regulator-name = "usb-phy-vdd-dummy";

This also doesn't look correct. This regulator should not just fill the 
gap. Does it represent a generic voltage network on the board?

Please do not add 'dummy' voltage regulators if there is no real voltage 
wire.

> +	};
> +
> +	reg_usb_1p8: s1800 {
> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		regulator-name = "usb-phy-pll-dummy";
> +	};
> +
> +	reg_usb_0p925: s0925 {
> +		compatible = "regulator-fixed";
> +		regulator-min-microvolt = <925000>;
> +		regulator-max-microvolt = <925000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +		regulator-name = "usb-phy-dummy";
> +	};
> +
>   	reserved-memory {
>   		#address-cells = <2>;
>   		#size-cells = <2>;
> @@ -179,6 +206,52 @@
>   		#size-cells = <1>;
>   		ranges = <0 0 0 0xffffffff>;
>   
> +		usb_0_qusbphy: phy@7b000 {
> +			compatible = "qcom,ipq9574-qusb2-phy";
> +			reg = <0x0007b000 0x180>;
> +			#phy-cells = <0>;
> +
> +			clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> +				 <&xo_board_clk>;
> +			clock-names = "cfg_ahb",
> +				      "ref";
> +
> +			vdd-supply = <&reg_usb_0p925>;
> +			vdda-pll-supply = <&reg_usb_1p8>;
> +			vdda-phy-dpdm-supply = <&reg_usb_3p3>;
> +
> +			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> +			status = "disabled";
> +		};
> +
> +		usb_0_qmpphy: phy@7d000 {
> +			compatible = "qcom,ipq9574-qmp-usb3-phy";
> +			reg = <0x0007d000 0xa00>;
> +			#phy-cells = <0>;
> +
> +			clocks = <&gcc GCC_USB0_AUX_CLK>,
> +				 <&xo_board_clk>,
> +				 <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> +				 <&gcc GCC_USB0_PIPE_CLK>;
> +			clock-names = "aux",
> +				      "ref",
> +				      "cfg_ahb",
> +				      "pipe";
> +
> +			resets = <&gcc GCC_USB0_PHY_BCR>,
> +				 <&gcc GCC_USB3PHY_0_PHY_BCR>;
> +			reset-names = "phy",
> +				      "phy_phy";
> +
> +			vdda-pll-supply = <&reg_usb_1p8>;
> +			vdda-phy-supply = <&reg_usb_0p925>;
> +
> +			status = "disabled";
> +
> +			#clock-cells = <0>;
> +			clock-output-names = "usb0_pipe_clk";
> +		};
> +
>   		pcie0_phy: phy@84000 {
>   			compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy";
>   			reg = <0x00084000 0x1000>;
> @@ -548,6 +621,53 @@
>   			status = "disabled";
>   		};
>   
> +		usb3: usb@8a00000 {
> +			compatible = "qcom,ipq9574-dwc3", "qcom,dwc3";
> +			reg = <0x08af8800 0x400>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +
> +			clocks = <&gcc GCC_SNOC_USB_CLK>,
> +				 <&gcc GCC_USB0_MASTER_CLK>,
> +				 <&gcc GCC_ANOC_USB_AXI_CLK>,
> +				 <&gcc GCC_USB0_SLEEP_CLK>,
> +				 <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> +
> +			clock-names = "cfg_noc",
> +				      "core",
> +				      "iface",
> +				      "sleep",
> +				      "mock_utmi";
> +
> +			assigned-clocks = <&gcc GCC_USB0_MASTER_CLK>,
> +					  <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> +			assigned-clock-rates = <200000000>,
> +					       <24000000>;
> +
> +			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "pwr_event";
> +
> +			resets = <&gcc GCC_USB_BCR>;
> +			status = "disabled";
> +
> +			dwc_0: usb@8a00000 {
> +				compatible = "snps,dwc3";
> +				reg = <0x8a00000 0xcd00>;
> +				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> +				clock-names = "ref";
> +				interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> +				phys = <&usb_0_qusbphy>, <&usb_0_qmpphy>;
> +				phy-names = "usb2-phy", "usb3-phy";
> +				tx-fifo-resize;
> +				snps,is-utmi-l1-suspend;
> +				snps,hird-threshold = /bits/ 8 <0x0>;
> +				snps,dis_u2_susphy_quirk;
> +				snps,dis_u3_susphy_quirk;
> +				dr_mode = "host";
> +			};
> +		};
> +
>   		intc: interrupt-controller@b000000 {
>   			compatible = "qcom,msm-qgic2";
>   			reg = <0x0b000000 0x1000>,  /* GICD */

-- 
With best wishes
Dmitry


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 5/8] phy: qcom-qusb2: add QUSB2 support for IPQ9574

[Dmitry Baryshkov]

On 05/04/2023 14:41, Varadarajan Narayanan wrote:
> Add the phy init sequence for the Super Speed ports found
> on IPQ9574.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
>    Changes in v2:
> 	- Place the entry such that the list continues to be sorted

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 2/8] dt-bindings: phy: qcom,qmp-usb: Add IPQ9574 USB3 PHY

[Varadarajan Narayanan]

On Fri, Apr 21, 2023 at 05:19:58PM +0300, Dmitry Baryshkov wrote:
> On 21/04/2023 13:13, Varadarajan Narayanan wrote:
> >On Thu, Apr 06, 2023 at 09:42:31AM +0200, Krzysztof Kozlowski wrote:
> >>On 05/04/2023 13:41, Varadarajan Narayanan wrote:
> >>>Add dt-bindings for USB3 PHY found on Qualcomm IPQ9574
> >>>
> >>>Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>>---
> >>>  Changes in v8:
> >>>	- Update clock names for ipq9574
> >>>
> >>>  Changes in v6:
> >>>	- Made power-domains optional
> >>>
> >>>Note: In the earlier patch sets, had used the (legacy)
> >>>specification available in qcom,msm8996-qmp-usb3-phy.yaml. Moved
> >>>to newer specification in qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >>>---
> >>>  .../phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml        | 43 +++++++++++++++++++---
> >>>  1 file changed, 37 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >>>index 16fce10..e902a0d 100644
> >>>--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >>>+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >>>@@ -16,6 +16,7 @@ description:
> >>>  properties:
> >>>    compatible:
> >>>      enum:
> >>>+      - qcom,ipq9574-qmp-usb3-phy
> >>>        - qcom,sc8280xp-qmp-usb3-uni-phy
> >>>
> >>>    reg:
> >>>@@ -25,11 +26,7 @@ properties:
> >>>      maxItems: 4
> >>>
> >>>    clock-names:
> >>>-    items:
> >>>-      - const: aux
> >>>-      - const: ref
> >>>-      - const: com_aux
> >>>-      - const: pipe
> >>>+    maxItems: 4
> >>>
> >>>    power-domains:
> >>>      maxItems: 1
> >>>@@ -60,7 +57,6 @@ required:
> >>>    - reg
> >>>    - clocks
> >>>    - clock-names
> >>>-  - power-domains
> >>
> >>Power domains are required. Commit msg does not explain why this should
> >>be now optional.
> >
> >Since IPQ9574 doesn't have power switches couldn't provide power-domains details.
> >So, had to make it optional to pass 'make dtbs_check'.
>
> This should be a part of the commit message, so that the next developer
> understands your intentions without going to mail archives.

Thanks for the feedback. Have posted v9 that includes the above
in commit message.

https://lore.kernel.org/lkml/b00042df41420ac337703ca99ac7876c46552946.1682092324.git.quic_varada@quicinc.com/

Thanks
Varada

> >>Best regards,
> >>Krzysztof
> >>
>
> --
> With best wishes
> Dmitry
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes

[Varadarajan Narayanan]

On Sat, Apr 22, 2023 at 12:07:01AM +0300, Dmitry Baryshkov wrote:
> On 05/04/2023 14:41, Varadarajan Narayanan wrote:
> >Add USB phy and controller related nodes
> >
> >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >---
> >  Changes in v8:
> >	- Change clocks order to match the bindings
> >  Changes in v7:
> >	- Change com_aux -> cfg_ahb
> >  Changes in v6:
> >	- Introduce fixed regulators for the phy
> >	- Resolved all 'make dtbs_check' messages
> >
> >  Changes in v5:
> >	- Fix additional comments
> >	- Edit nodes to match with qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >	- 'make dtbs_check' giving the following messages since
> >	  ipq9574 doesn't have power domains. Hope this is ok
> >
> >		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
> >         	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >		/local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
> >         	From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> >
> >  Changes in v4:
> >	- Use newer bindings without subnodes
> >	- Fix coding style issues
> >
> >  Changes in v3:
> >	- Insert the nodes at proper location
> >
> >  Changes in v2:
> >	- Fixed issues flagged by Krzysztof
> >	- Fix issues reported by make dtbs_check
> >	- Remove NOC related clocks (to be added with proper
> >	  interconnect support)
> >---
> >  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 120 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 120 insertions(+)
> >
> >diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >index 43a3dbe..1242382 100644
> >--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >@@ -150,6 +150,33 @@
> >  		method = "smc";
> >  	};
> >+	reg_usb_3p3: s3300 {
>
> The node names do not look generic enough. Please take a look at other
> platforms.

Please see below.

> >+		compatible = "regulator-fixed";
> >+		regulator-min-microvolt = <3300000>;
> >+		regulator-max-microvolt = <3300000>;
> >+		regulator-boot-on;
> >+		regulator-always-on;
> >+		regulator-name = "usb-phy-vdd-dummy";
>
> This also doesn't look correct. This regulator should not just fill the gap.
> Does it represent a generic voltage network on the board?
>
> Please do not add 'dummy' voltage regulators if there is no real voltage
> wire.

These are real voltage wires. I used dummy since they are
always-on and cannot be increased/decreased (i.e. fixed).
Would something along the following lines be appropriate?

	vreg_ae10_3p3: s3300 {
		compatible = "regulator-fixed";
		regulator-min-microvolt = <3300000>;
		regulator-max-microvolt = <3300000>;
		regulator-boot-on;
		regulator-always-on;
		regulator-name = "usb-phy-vdd";
	};

	vreg_ad8_1p8: s1800 {
		compatible = "regulator-fixed";
		regulator-min-microvolt = <1800000>;
		regulator-max-microvolt = <1800000>;
		regulator-boot-on;
		regulator-always-on;
		regulator-name = "usb-phy-pll";
	};

	vreg_ad9_0p925: s0925 {
		compatible = "regulator-fixed";
		regulator-min-microvolt = <925000>;
		regulator-max-microvolt = <925000>;
		regulator-boot-on;
		regulator-always-on;
		regulator-name = "usb-phy";
	};

Thanks
Varada

> >+	};
> >+
> >+	reg_usb_1p8: s1800 {
> >+		compatible = "regulator-fixed";
> >+		regulator-min-microvolt = <1800000>;
> >+		regulator-max-microvolt = <1800000>;
> >+		regulator-boot-on;
> >+		regulator-always-on;
> >+		regulator-name = "usb-phy-pll-dummy";
> >+	};
> >+
> >+	reg_usb_0p925: s0925 {
> >+		compatible = "regulator-fixed";
> >+		regulator-min-microvolt = <925000>;
> >+		regulator-max-microvolt = <925000>;
> >+		regulator-boot-on;
> >+		regulator-always-on;
> >+		regulator-name = "usb-phy-dummy";
> >+	};
> >+
> >  	reserved-memory {
> >  		#address-cells = <2>;
> >  		#size-cells = <2>;
> >@@ -179,6 +206,52 @@
> >  		#size-cells = <1>;
> >  		ranges = <0 0 0 0xffffffff>;
> >+		usb_0_qusbphy: phy@7b000 {
> >+			compatible = "qcom,ipq9574-qusb2-phy";
> >+			reg = <0x0007b000 0x180>;
> >+			#phy-cells = <0>;
> >+
> >+			clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> >+				 <&xo_board_clk>;
> >+			clock-names = "cfg_ahb",
> >+				      "ref";
> >+
> >+			vdd-supply = <&reg_usb_0p925>;
> >+			vdda-pll-supply = <&reg_usb_1p8>;
> >+			vdda-phy-dpdm-supply = <&reg_usb_3p3>;
> >+
> >+			resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> >+			status = "disabled";
> >+		};
> >+
> >+		usb_0_qmpphy: phy@7d000 {
> >+			compatible = "qcom,ipq9574-qmp-usb3-phy";
> >+			reg = <0x0007d000 0xa00>;
> >+			#phy-cells = <0>;
> >+
> >+			clocks = <&gcc GCC_USB0_AUX_CLK>,
> >+				 <&xo_board_clk>,
> >+				 <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> >+				 <&gcc GCC_USB0_PIPE_CLK>;
> >+			clock-names = "aux",
> >+				      "ref",
> >+				      "cfg_ahb",
> >+				      "pipe";
> >+
> >+			resets = <&gcc GCC_USB0_PHY_BCR>,
> >+				 <&gcc GCC_USB3PHY_0_PHY_BCR>;
> >+			reset-names = "phy",
> >+				      "phy_phy";
> >+
> >+			vdda-pll-supply = <&reg_usb_1p8>;
> >+			vdda-phy-supply = <&reg_usb_0p925>;
> >+
> >+			status = "disabled";
> >+
> >+			#clock-cells = <0>;
> >+			clock-output-names = "usb0_pipe_clk";
> >+		};
> >+
> >  		pcie0_phy: phy@84000 {
> >  			compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy";
> >  			reg = <0x00084000 0x1000>;
> >@@ -548,6 +621,53 @@
> >  			status = "disabled";
> >  		};
> >+		usb3: usb@8a00000 {
> >+			compatible = "qcom,ipq9574-dwc3", "qcom,dwc3";
> >+			reg = <0x08af8800 0x400>;
> >+			#address-cells = <1>;
> >+			#size-cells = <1>;
> >+			ranges;
> >+
> >+			clocks = <&gcc GCC_SNOC_USB_CLK>,
> >+				 <&gcc GCC_USB0_MASTER_CLK>,
> >+				 <&gcc GCC_ANOC_USB_AXI_CLK>,
> >+				 <&gcc GCC_USB0_SLEEP_CLK>,
> >+				 <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> >+
> >+			clock-names = "cfg_noc",
> >+				      "core",
> >+				      "iface",
> >+				      "sleep",
> >+				      "mock_utmi";
> >+
> >+			assigned-clocks = <&gcc GCC_USB0_MASTER_CLK>,
> >+					  <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> >+			assigned-clock-rates = <200000000>,
> >+					       <24000000>;
> >+
> >+			interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> >+			interrupt-names = "pwr_event";
> >+
> >+			resets = <&gcc GCC_USB_BCR>;
> >+			status = "disabled";
> >+
> >+			dwc_0: usb@8a00000 {
> >+				compatible = "snps,dwc3";
> >+				reg = <0x8a00000 0xcd00>;
> >+				clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> >+				clock-names = "ref";
> >+				interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> >+				phys = <&usb_0_qusbphy>, <&usb_0_qmpphy>;
> >+				phy-names = "usb2-phy", "usb3-phy";
> >+				tx-fifo-resize;
> >+				snps,is-utmi-l1-suspend;
> >+				snps,hird-threshold = /bits/ 8 <0x0>;
> >+				snps,dis_u2_susphy_quirk;
> >+				snps,dis_u3_susphy_quirk;
> >+				dr_mode = "host";
> >+			};
> >+		};
> >+
> >  		intc: interrupt-controller@b000000 {
> >  			compatible = "qcom,msm-qgic2";
> >  			reg = <0x0b000000 0x1000>,  /* GICD */
>
> --
> With best wishes
> Dmitry
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes

[Dmitry Baryshkov]

On Mon, 24 Apr 2023 at 12:04, Varadarajan Narayanan
<quic_varada@quicinc.com> wrote:
>
> On Sat, Apr 22, 2023 at 12:07:01AM +0300, Dmitry Baryshkov wrote:
> > On 05/04/2023 14:41, Varadarajan Narayanan wrote:
> > >Add USB phy and controller related nodes
> > >
> > >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > >---
> > >  Changes in v8:
> > >     - Change clocks order to match the bindings
> > >  Changes in v7:
> > >     - Change com_aux -> cfg_ahb
> > >  Changes in v6:
> > >     - Introduce fixed regulators for the phy
> > >     - Resolved all 'make dtbs_check' messages
> > >
> > >  Changes in v5:
> > >     - Fix additional comments
> > >     - Edit nodes to match with qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > >     - 'make dtbs_check' giving the following messages since
> > >       ipq9574 doesn't have power domains. Hope this is ok
> > >
> > >             /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
> > >             From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > >             /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
> > >             From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > >
> > >  Changes in v4:
> > >     - Use newer bindings without subnodes
> > >     - Fix coding style issues
> > >
> > >  Changes in v3:
> > >     - Insert the nodes at proper location
> > >
> > >  Changes in v2:
> > >     - Fixed issues flagged by Krzysztof
> > >     - Fix issues reported by make dtbs_check
> > >     - Remove NOC related clocks (to be added with proper
> > >       interconnect support)
> > >---
> > >  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 120 ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 120 insertions(+)
> > >
> > >diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > >index 43a3dbe..1242382 100644
> > >--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > >+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > >@@ -150,6 +150,33 @@
> > >             method = "smc";
> > >     };
> > >+    reg_usb_3p3: s3300 {
> >
> > The node names do not look generic enough. Please take a look at other
> > platforms.
>
> Please see below.
>
> > >+            compatible = "regulator-fixed";
> > >+            regulator-min-microvolt = <3300000>;
> > >+            regulator-max-microvolt = <3300000>;
> > >+            regulator-boot-on;
> > >+            regulator-always-on;
> > >+            regulator-name = "usb-phy-vdd-dummy";
> >
> > This also doesn't look correct. This regulator should not just fill the gap.
> > Does it represent a generic voltage network on the board?
> >
> > Please do not add 'dummy' voltage regulators if there is no real voltage
> > wire.
>
> These are real voltage wires. I used dummy since they are
> always-on and cannot be increased/decreased (i.e. fixed).
> Would something along the following lines be appropriate?

Still not fully correct. Please use regulator name that corresponds to
the power grid on the board schematics. I don't think that you have a
separate power grids for USB PHY.

>
>         vreg_ae10_3p3: s3300 {

Naming suggests that these voltages are generated by some PMIC. Is
this correct? If so, please describe the PMIC instead.

>                 compatible = "regulator-fixed";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>                 regulator-boot-on;
>                 regulator-always-on;
>                 regulator-name = "usb-phy-vdd";
>         };
>
>         vreg_ad8_1p8: s1800 {
>                 compatible = "regulator-fixed";
>                 regulator-min-microvolt = <1800000>;
>                 regulator-max-microvolt = <1800000>;
>                 regulator-boot-on;
>                 regulator-always-on;
>                 regulator-name = "usb-phy-pll";
>         };
>
>         vreg_ad9_0p925: s0925 {
>                 compatible = "regulator-fixed";
>                 regulator-min-microvolt = <925000>;
>                 regulator-max-microvolt = <925000>;
>                 regulator-boot-on;
>                 regulator-always-on;
>                 regulator-name = "usb-phy";
>         };
>
> Thanks
> Varada
>
> > >+    };
> > >+
> > >+    reg_usb_1p8: s1800 {
> > >+            compatible = "regulator-fixed";
> > >+            regulator-min-microvolt = <1800000>;
> > >+            regulator-max-microvolt = <1800000>;
> > >+            regulator-boot-on;
> > >+            regulator-always-on;
> > >+            regulator-name = "usb-phy-pll-dummy";
> > >+    };
> > >+
> > >+    reg_usb_0p925: s0925 {
> > >+            compatible = "regulator-fixed";
> > >+            regulator-min-microvolt = <925000>;
> > >+            regulator-max-microvolt = <925000>;
> > >+            regulator-boot-on;
> > >+            regulator-always-on;
> > >+            regulator-name = "usb-phy-dummy";
> > >+    };
> > >+
> > >     reserved-memory {
> > >             #address-cells = <2>;
> > >             #size-cells = <2>;
> > >@@ -179,6 +206,52 @@
> > >             #size-cells = <1>;
> > >             ranges = <0 0 0 0xffffffff>;
> > >+            usb_0_qusbphy: phy@7b000 {
> > >+                    compatible = "qcom,ipq9574-qusb2-phy";
> > >+                    reg = <0x0007b000 0x180>;
> > >+                    #phy-cells = <0>;
> > >+
> > >+                    clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> > >+                             <&xo_board_clk>;
> > >+                    clock-names = "cfg_ahb",
> > >+                                  "ref";
> > >+
> > >+                    vdd-supply = <&reg_usb_0p925>;
> > >+                    vdda-pll-supply = <&reg_usb_1p8>;
> > >+                    vdda-phy-dpdm-supply = <&reg_usb_3p3>;
> > >+
> > >+                    resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> > >+                    status = "disabled";
> > >+            };
> > >+
> > >+            usb_0_qmpphy: phy@7d000 {
> > >+                    compatible = "qcom,ipq9574-qmp-usb3-phy";
> > >+                    reg = <0x0007d000 0xa00>;
> > >+                    #phy-cells = <0>;
> > >+
> > >+                    clocks = <&gcc GCC_USB0_AUX_CLK>,
> > >+                             <&xo_board_clk>,
> > >+                             <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> > >+                             <&gcc GCC_USB0_PIPE_CLK>;
> > >+                    clock-names = "aux",
> > >+                                  "ref",
> > >+                                  "cfg_ahb",
> > >+                                  "pipe";
> > >+
> > >+                    resets = <&gcc GCC_USB0_PHY_BCR>,
> > >+                             <&gcc GCC_USB3PHY_0_PHY_BCR>;
> > >+                    reset-names = "phy",
> > >+                                  "phy_phy";
> > >+
> > >+                    vdda-pll-supply = <&reg_usb_1p8>;
> > >+                    vdda-phy-supply = <&reg_usb_0p925>;
> > >+
> > >+                    status = "disabled";
> > >+
> > >+                    #clock-cells = <0>;
> > >+                    clock-output-names = "usb0_pipe_clk";
> > >+            };
> > >+
> > >             pcie0_phy: phy@84000 {
> > >                     compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy";
> > >                     reg = <0x00084000 0x1000>;
> > >@@ -548,6 +621,53 @@
> > >                     status = "disabled";
> > >             };
> > >+            usb3: usb@8a00000 {
> > >+                    compatible = "qcom,ipq9574-dwc3", "qcom,dwc3";
> > >+                    reg = <0x08af8800 0x400>;
> > >+                    #address-cells = <1>;
> > >+                    #size-cells = <1>;
> > >+                    ranges;
> > >+
> > >+                    clocks = <&gcc GCC_SNOC_USB_CLK>,
> > >+                             <&gcc GCC_USB0_MASTER_CLK>,
> > >+                             <&gcc GCC_ANOC_USB_AXI_CLK>,
> > >+                             <&gcc GCC_USB0_SLEEP_CLK>,
> > >+                             <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > >+
> > >+                    clock-names = "cfg_noc",
> > >+                                  "core",
> > >+                                  "iface",
> > >+                                  "sleep",
> > >+                                  "mock_utmi";
> > >+
> > >+                    assigned-clocks = <&gcc GCC_USB0_MASTER_CLK>,
> > >+                                      <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > >+                    assigned-clock-rates = <200000000>,
> > >+                                           <24000000>;
> > >+
> > >+                    interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> > >+                    interrupt-names = "pwr_event";
> > >+
> > >+                    resets = <&gcc GCC_USB_BCR>;
> > >+                    status = "disabled";
> > >+
> > >+                    dwc_0: usb@8a00000 {
> > >+                            compatible = "snps,dwc3";
> > >+                            reg = <0x8a00000 0xcd00>;
> > >+                            clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > >+                            clock-names = "ref";
> > >+                            interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> > >+                            phys = <&usb_0_qusbphy>, <&usb_0_qmpphy>;
> > >+                            phy-names = "usb2-phy", "usb3-phy";
> > >+                            tx-fifo-resize;
> > >+                            snps,is-utmi-l1-suspend;
> > >+                            snps,hird-threshold = /bits/ 8 <0x0>;
> > >+                            snps,dis_u2_susphy_quirk;
> > >+                            snps,dis_u3_susphy_quirk;
> > >+                            dr_mode = "host";
> > >+                    };
> > >+            };
> > >+
> > >             intc: interrupt-controller@b000000 {
> > >                     compatible = "qcom,msm-qgic2";
> > >                     reg = <0x0b000000 0x1000>,  /* GICD */
> >
> > --
> > With best wishes
> > Dmitry
> >



-- 
With best wishes
Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes

[Varadarajan Narayanan]

On Mon, Apr 24, 2023 at 02:17:06PM +0300, Dmitry Baryshkov wrote:
> On Mon, 24 Apr 2023 at 12:04, Varadarajan Narayanan
> <quic_varada@quicinc.com> wrote:
> >
> > On Sat, Apr 22, 2023 at 12:07:01AM +0300, Dmitry Baryshkov wrote:
> > > On 05/04/2023 14:41, Varadarajan Narayanan wrote:
> > > >Add USB phy and controller related nodes
> > > >
> > > >Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> > > >---
> > > >  Changes in v8:
> > > >     - Change clocks order to match the bindings
> > > >  Changes in v7:
> > > >     - Change com_aux -> cfg_ahb
> > > >  Changes in v6:
> > > >     - Introduce fixed regulators for the phy
> > > >     - Resolved all 'make dtbs_check' messages
> > > >
> > > >  Changes in v5:
> > > >     - Fix additional comments
> > > >     - Edit nodes to match with qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > > >     - 'make dtbs_check' giving the following messages since
> > > >       ipq9574 doesn't have power domains. Hope this is ok
> > > >
> > > >             /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
> > > >             From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> > > >             /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
> > > >             From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > > >
> > > >  Changes in v4:
> > > >     - Use newer bindings without subnodes
> > > >     - Fix coding style issues
> > > >
> > > >  Changes in v3:
> > > >     - Insert the nodes at proper location
> > > >
> > > >  Changes in v2:
> > > >     - Fixed issues flagged by Krzysztof
> > > >     - Fix issues reported by make dtbs_check
> > > >     - Remove NOC related clocks (to be added with proper
> > > >       interconnect support)
> > > >---
> > > >  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 120 ++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 120 insertions(+)
> > > >
> > > >diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > >index 43a3dbe..1242382 100644
> > > >--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > >+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> > > >@@ -150,6 +150,33 @@
> > > >             method = "smc";
> > > >     };
> > > >+    reg_usb_3p3: s3300 {
> > >
> > > The node names do not look generic enough. Please take a look at other
> > > platforms.
> >
> > Please see below.
> >
> > > >+            compatible = "regulator-fixed";
> > > >+            regulator-min-microvolt = <3300000>;
> > > >+            regulator-max-microvolt = <3300000>;
> > > >+            regulator-boot-on;
> > > >+            regulator-always-on;
> > > >+            regulator-name = "usb-phy-vdd-dummy";
> > >
> > > This also doesn't look correct. This regulator should not just fill the gap.
> > > Does it represent a generic voltage network on the board?
> > >
> > > Please do not add 'dummy' voltage regulators if there is no real voltage
> > > wire.
> >
> > These are real voltage wires. I used dummy since they are
> > always-on and cannot be increased/decreased (i.e. fixed).
> > Would something along the following lines be appropriate?
>
> Still not fully correct. Please use regulator name that corresponds to
> the power grid on the board schematics. I don't think that you have a
> separate power grids for USB PHY.
>
> >
> >         vreg_ae10_3p3: s3300 {
>
> Naming suggests that these voltages are generated by some PMIC. Is
> this correct? If so, please describe the PMIC instead.

SS PHY needs two supplies and HS PHY needs three supplies. 3.3V
and 0.925V are from fixed DC - DC regulators and 1.8V is
generated from MP5496 PMIC. Would the following node definitions
be ok?

usb_hs_vreg0: usb_hs_vreg0 {
	compatible = "regulator-fixed";
	regulator-min-microvolt = <3300000>;
	regulator-max-microvolt = <3300000>;
	regulator-boot-on;
	regulator-always-on;
	regulator-name = "usb-phy-vdd";
};

usb_hs_vreg1: usb_hs_vreg1 {
	compatible = "regulator-fixed";
	regulator-min-microvolt = <925000>;
	regulator-max-microvolt = <925000>;
	regulator-boot-on;
	regulator-always-on;
	regulator-name = "usb-phy";
};

&rpm_requests {
	regulators {
		compatible = "qcom,rpm-mp5496-regulators";
		.
		.
		.
		ipq9574_l2: l2 {
			regulator-min-microvolt = <1800000>;
			regulator-max-microvolt = <1800000>;
			regulator-boot-on;
			regulator-always-on;
		};
	};
};

Thanks
Varada

> >                 compatible = "regulator-fixed";
> >                 regulator-min-microvolt = <3300000>;
> >                 regulator-max-microvolt = <3300000>;
> >                 regulator-boot-on;
> >                 regulator-always-on;
> >                 regulator-name = "usb-phy-vdd";
> >         };
> >
> >         vreg_ad8_1p8: s1800 {
> >                 compatible = "regulator-fixed";
> >                 regulator-min-microvolt = <1800000>;
> >                 regulator-max-microvolt = <1800000>;
> >                 regulator-boot-on;
> >                 regulator-always-on;
> >                 regulator-name = "usb-phy-pll";
> >         };
> >
> >         vreg_ad9_0p925: s0925 {
> >                 compatible = "regulator-fixed";
> >                 regulator-min-microvolt = <925000>;
> >                 regulator-max-microvolt = <925000>;
> >                 regulator-boot-on;
> >                 regulator-always-on;
> >                 regulator-name = "usb-phy";
> >         };
> >
> > Thanks
> > Varada
> >
> > > >+    };
> > > >+
> > > >+    reg_usb_1p8: s1800 {
> > > >+            compatible = "regulator-fixed";
> > > >+            regulator-min-microvolt = <1800000>;
> > > >+            regulator-max-microvolt = <1800000>;
> > > >+            regulator-boot-on;
> > > >+            regulator-always-on;
> > > >+            regulator-name = "usb-phy-pll-dummy";
> > > >+    };
> > > >+
> > > >+    reg_usb_0p925: s0925 {
> > > >+            compatible = "regulator-fixed";
> > > >+            regulator-min-microvolt = <925000>;
> > > >+            regulator-max-microvolt = <925000>;
> > > >+            regulator-boot-on;
> > > >+            regulator-always-on;
> > > >+            regulator-name = "usb-phy-dummy";
> > > >+    };
> > > >+
> > > >     reserved-memory {
> > > >             #address-cells = <2>;
> > > >             #size-cells = <2>;
> > > >@@ -179,6 +206,52 @@
> > > >             #size-cells = <1>;
> > > >             ranges = <0 0 0 0xffffffff>;
> > > >+            usb_0_qusbphy: phy@7b000 {
> > > >+                    compatible = "qcom,ipq9574-qusb2-phy";
> > > >+                    reg = <0x0007b000 0x180>;
> > > >+                    #phy-cells = <0>;
> > > >+
> > > >+                    clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> > > >+                             <&xo_board_clk>;
> > > >+                    clock-names = "cfg_ahb",
> > > >+                                  "ref";
> > > >+
> > > >+                    vdd-supply = <&reg_usb_0p925>;
> > > >+                    vdda-pll-supply = <&reg_usb_1p8>;
> > > >+                    vdda-phy-dpdm-supply = <&reg_usb_3p3>;
> > > >+
> > > >+                    resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> > > >+                    status = "disabled";
> > > >+            };
> > > >+
> > > >+            usb_0_qmpphy: phy@7d000 {
> > > >+                    compatible = "qcom,ipq9574-qmp-usb3-phy";
> > > >+                    reg = <0x0007d000 0xa00>;
> > > >+                    #phy-cells = <0>;
> > > >+
> > > >+                    clocks = <&gcc GCC_USB0_AUX_CLK>,
> > > >+                             <&xo_board_clk>,
> > > >+                             <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> > > >+                             <&gcc GCC_USB0_PIPE_CLK>;
> > > >+                    clock-names = "aux",
> > > >+                                  "ref",
> > > >+                                  "cfg_ahb",
> > > >+                                  "pipe";
> > > >+
> > > >+                    resets = <&gcc GCC_USB0_PHY_BCR>,
> > > >+                             <&gcc GCC_USB3PHY_0_PHY_BCR>;
> > > >+                    reset-names = "phy",
> > > >+                                  "phy_phy";
> > > >+
> > > >+                    vdda-pll-supply = <&reg_usb_1p8>;
> > > >+                    vdda-phy-supply = <&reg_usb_0p925>;
> > > >+
> > > >+                    status = "disabled";
> > > >+
> > > >+                    #clock-cells = <0>;
> > > >+                    clock-output-names = "usb0_pipe_clk";
> > > >+            };
> > > >+
> > > >             pcie0_phy: phy@84000 {
> > > >                     compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy";
> > > >                     reg = <0x00084000 0x1000>;
> > > >@@ -548,6 +621,53 @@
> > > >                     status = "disabled";
> > > >             };
> > > >+            usb3: usb@8a00000 {
> > > >+                    compatible = "qcom,ipq9574-dwc3", "qcom,dwc3";
> > > >+                    reg = <0x08af8800 0x400>;
> > > >+                    #address-cells = <1>;
> > > >+                    #size-cells = <1>;
> > > >+                    ranges;
> > > >+
> > > >+                    clocks = <&gcc GCC_SNOC_USB_CLK>,
> > > >+                             <&gcc GCC_USB0_MASTER_CLK>,
> > > >+                             <&gcc GCC_ANOC_USB_AXI_CLK>,
> > > >+                             <&gcc GCC_USB0_SLEEP_CLK>,
> > > >+                             <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > > >+
> > > >+                    clock-names = "cfg_noc",
> > > >+                                  "core",
> > > >+                                  "iface",
> > > >+                                  "sleep",
> > > >+                                  "mock_utmi";
> > > >+
> > > >+                    assigned-clocks = <&gcc GCC_USB0_MASTER_CLK>,
> > > >+                                      <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > > >+                    assigned-clock-rates = <200000000>,
> > > >+                                           <24000000>;
> > > >+
> > > >+                    interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> > > >+                    interrupt-names = "pwr_event";
> > > >+
> > > >+                    resets = <&gcc GCC_USB_BCR>;
> > > >+                    status = "disabled";
> > > >+
> > > >+                    dwc_0: usb@8a00000 {
> > > >+                            compatible = "snps,dwc3";
> > > >+                            reg = <0x8a00000 0xcd00>;
> > > >+                            clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> > > >+                            clock-names = "ref";
> > > >+                            interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> > > >+                            phys = <&usb_0_qusbphy>, <&usb_0_qmpphy>;
> > > >+                            phy-names = "usb2-phy", "usb3-phy";
> > > >+                            tx-fifo-resize;
> > > >+                            snps,is-utmi-l1-suspend;
> > > >+                            snps,hird-threshold = /bits/ 8 <0x0>;
> > > >+                            snps,dis_u2_susphy_quirk;
> > > >+                            snps,dis_u3_susphy_quirk;
> > > >+                            dr_mode = "host";
> > > >+                    };
> > > >+            };
> > > >+
> > > >             intc: interrupt-controller@b000000 {
> > > >                     compatible = "qcom,msm-qgic2";
> > > >                     reg = <0x0b000000 0x1000>,  /* GICD */
> > >
> > > --
> > > With best wishes
> > > Dmitry
> > >
>
>
>
> --
> With best wishes
> Dmitry

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes

[Dmitry Baryshkov]

On 26/04/2023 12:51, Varadarajan Narayanan wrote:
> On Mon, Apr 24, 2023 at 02:17:06PM +0300, Dmitry Baryshkov wrote:
>> On Mon, 24 Apr 2023 at 12:04, Varadarajan Narayanan
>> <quic_varada@quicinc.com> wrote:
>>>
>>> On Sat, Apr 22, 2023 at 12:07:01AM +0300, Dmitry Baryshkov wrote:
>>>> On 05/04/2023 14:41, Varadarajan Narayanan wrote:
>>>>> Add USB phy and controller related nodes
>>>>>
>>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>>> ---
>>>>>   Changes in v8:
>>>>>      - Change clocks order to match the bindings
>>>>>   Changes in v7:
>>>>>      - Change com_aux -> cfg_ahb
>>>>>   Changes in v6:
>>>>>      - Introduce fixed regulators for the phy
>>>>>      - Resolved all 'make dtbs_check' messages
>>>>>
>>>>>   Changes in v5:
>>>>>      - Fix additional comments
>>>>>      - Edit nodes to match with qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>>>      - 'make dtbs_check' giving the following messages since
>>>>>        ipq9574 doesn't have power domains. Hope this is ok
>>>>>
>>>>>              /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
>>>>>              From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
>>>>>              /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
>>>>>              From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>>>>
>>>>>   Changes in v4:
>>>>>      - Use newer bindings without subnodes
>>>>>      - Fix coding style issues
>>>>>
>>>>>   Changes in v3:
>>>>>      - Insert the nodes at proper location
>>>>>
>>>>>   Changes in v2:
>>>>>      - Fixed issues flagged by Krzysztof
>>>>>      - Fix issues reported by make dtbs_check
>>>>>      - Remove NOC related clocks (to be added with proper
>>>>>        interconnect support)
>>>>> ---
>>>>>   arch/arm64/boot/dts/qcom/ipq9574.dtsi | 120 ++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 120 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>> index 43a3dbe..1242382 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
>>>>> @@ -150,6 +150,33 @@
>>>>>              method = "smc";
>>>>>      };
>>>>> +    reg_usb_3p3: s3300 {
>>>>
>>>> The node names do not look generic enough. Please take a look at other
>>>> platforms.
>>>
>>> Please see below.
>>>
>>>>> +            compatible = "regulator-fixed";
>>>>> +            regulator-min-microvolt = <3300000>;
>>>>> +            regulator-max-microvolt = <3300000>;
>>>>> +            regulator-boot-on;
>>>>> +            regulator-always-on;
>>>>> +            regulator-name = "usb-phy-vdd-dummy";
>>>>
>>>> This also doesn't look correct. This regulator should not just fill the gap.
>>>> Does it represent a generic voltage network on the board?
>>>>
>>>> Please do not add 'dummy' voltage regulators if there is no real voltage
>>>> wire.
>>>
>>> These are real voltage wires. I used dummy since they are
>>> always-on and cannot be increased/decreased (i.e. fixed).
>>> Would something along the following lines be appropriate?
>>
>> Still not fully correct. Please use regulator name that corresponds to
>> the power grid on the board schematics. I don't think that you have a
>> separate power grids for USB PHY.
>>
>>>
>>>          vreg_ae10_3p3: s3300 {
>>
>> Naming suggests that these voltages are generated by some PMIC. Is
>> this correct? If so, please describe the PMIC instead.
> 
> SS PHY needs two supplies and HS PHY needs three supplies. 3.3V
> and 0.925V are from fixed DC - DC regulators and 1.8V is
> generated from MP5496 PMIC. Would the following node definitions
> be ok?
> 
> usb_hs_vreg0: usb_hs_vreg0 {
> 	compatible = "regulator-fixed";
> 	regulator-min-microvolt = <3300000>;
> 	regulator-max-microvolt = <3300000>;
> 	regulator-boot-on;
> 	regulator-always-on;
> 	regulator-name = "usb-phy-vdd";
> };
> 
> usb_hs_vreg1: usb_hs_vreg1 {
> 	compatible = "regulator-fixed";
> 	regulator-min-microvolt = <925000>;
> 	regulator-max-microvolt = <925000>;
> 	regulator-boot-on;
> 	regulator-always-on;
> 	regulator-name = "usb-phy";
> };

Again. The voltage rails on the board are not USB-specific, are they? So 
why are you declaring usb-phy regulators? Would another consumer of 3.3V 
rail use the same usb-phy-vdd regulator?

> 
> &rpm_requests {
> 	regulators {
> 		compatible = "qcom,rpm-mp5496-regulators";
> 		.
> 		.
> 		.
> 		ipq9574_l2: l2 {

mp5496_l2

> 			regulator-min-microvolt = <1800000>;
> 			regulator-max-microvolt = <1800000>;
> 			regulator-boot-on;
> 			regulator-always-on;
> 		};
> 	};
> };
> 
> Thanks
> Varada
> 
>>>                  compatible = "regulator-fixed";
>>>                  regulator-min-microvolt = <3300000>;
>>>                  regulator-max-microvolt = <3300000>;
>>>                  regulator-boot-on;
>>>                  regulator-always-on;
>>>                  regulator-name = "usb-phy-vdd";
>>>          };
>>>
>>>          vreg_ad8_1p8: s1800 {
>>>                  compatible = "regulator-fixed";
>>>                  regulator-min-microvolt = <1800000>;
>>>                  regulator-max-microvolt = <1800000>;
>>>                  regulator-boot-on;
>>>                  regulator-always-on;
>>>                  regulator-name = "usb-phy-pll";
>>>          };
>>>
>>>          vreg_ad9_0p925: s0925 {
>>>                  compatible = "regulator-fixed";
>>>                  regulator-min-microvolt = <925000>;
>>>                  regulator-max-microvolt = <925000>;
>>>                  regulator-boot-on;
>>>                  regulator-always-on;
>>>                  regulator-name = "usb-phy";
>>>          };
>>>
>>> Thanks
>>> Varada
>>>
>>>>> +    };
>>>>> +
>>>>> +    reg_usb_1p8: s1800 {
>>>>> +            compatible = "regulator-fixed";
>>>>> +            regulator-min-microvolt = <1800000>;
>>>>> +            regulator-max-microvolt = <1800000>;
>>>>> +            regulator-boot-on;
>>>>> +            regulator-always-on;
>>>>> +            regulator-name = "usb-phy-pll-dummy";
>>>>> +    };
>>>>> +
>>>>> +    reg_usb_0p925: s0925 {
>>>>> +            compatible = "regulator-fixed";
>>>>> +            regulator-min-microvolt = <925000>;
>>>>> +            regulator-max-microvolt = <925000>;
>>>>> +            regulator-boot-on;
>>>>> +            regulator-always-on;
>>>>> +            regulator-name = "usb-phy-dummy";
>>>>> +    };
>>>>> +
>>>>>      reserved-memory {
>>>>>              #address-cells = <2>;
>>>>>              #size-cells = <2>;
>>>>> @@ -179,6 +206,52 @@
>>>>>              #size-cells = <1>;
>>>>>              ranges = <0 0 0 0xffffffff>;
>>>>> +            usb_0_qusbphy: phy@7b000 {
>>>>> +                    compatible = "qcom,ipq9574-qusb2-phy";
>>>>> +                    reg = <0x0007b000 0x180>;
>>>>> +                    #phy-cells = <0>;
>>>>> +
>>>>> +                    clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
>>>>> +                             <&xo_board_clk>;
>>>>> +                    clock-names = "cfg_ahb",
>>>>> +                                  "ref";
>>>>> +
>>>>> +                    vdd-supply = <&reg_usb_0p925>;
>>>>> +                    vdda-pll-supply = <&reg_usb_1p8>;
>>>>> +                    vdda-phy-dpdm-supply = <&reg_usb_3p3>;
>>>>> +
>>>>> +                    resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
>>>>> +                    status = "disabled";
>>>>> +            };
>>>>> +
>>>>> +            usb_0_qmpphy: phy@7d000 {
>>>>> +                    compatible = "qcom,ipq9574-qmp-usb3-phy";
>>>>> +                    reg = <0x0007d000 0xa00>;
>>>>> +                    #phy-cells = <0>;
>>>>> +
>>>>> +                    clocks = <&gcc GCC_USB0_AUX_CLK>,
>>>>> +                             <&xo_board_clk>,
>>>>> +                             <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
>>>>> +                             <&gcc GCC_USB0_PIPE_CLK>;
>>>>> +                    clock-names = "aux",
>>>>> +                                  "ref",
>>>>> +                                  "cfg_ahb",
>>>>> +                                  "pipe";
>>>>> +
>>>>> +                    resets = <&gcc GCC_USB0_PHY_BCR>,
>>>>> +                             <&gcc GCC_USB3PHY_0_PHY_BCR>;
>>>>> +                    reset-names = "phy",
>>>>> +                                  "phy_phy";
>>>>> +
>>>>> +                    vdda-pll-supply = <&reg_usb_1p8>;
>>>>> +                    vdda-phy-supply = <&reg_usb_0p925>;
>>>>> +
>>>>> +                    status = "disabled";
>>>>> +
>>>>> +                    #clock-cells = <0>;
>>>>> +                    clock-output-names = "usb0_pipe_clk";
>>>>> +            };
>>>>> +
>>>>>              pcie0_phy: phy@84000 {
>>>>>                      compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy";
>>>>>                      reg = <0x00084000 0x1000>;
>>>>> @@ -548,6 +621,53 @@
>>>>>                      status = "disabled";
>>>>>              };
>>>>> +            usb3: usb@8a00000 {
>>>>> +                    compatible = "qcom,ipq9574-dwc3", "qcom,dwc3";
>>>>> +                    reg = <0x08af8800 0x400>;
>>>>> +                    #address-cells = <1>;
>>>>> +                    #size-cells = <1>;
>>>>> +                    ranges;
>>>>> +
>>>>> +                    clocks = <&gcc GCC_SNOC_USB_CLK>,
>>>>> +                             <&gcc GCC_USB0_MASTER_CLK>,
>>>>> +                             <&gcc GCC_ANOC_USB_AXI_CLK>,
>>>>> +                             <&gcc GCC_USB0_SLEEP_CLK>,
>>>>> +                             <&gcc GCC_USB0_MOCK_UTMI_CLK>;
>>>>> +
>>>>> +                    clock-names = "cfg_noc",
>>>>> +                                  "core",
>>>>> +                                  "iface",
>>>>> +                                  "sleep",
>>>>> +                                  "mock_utmi";
>>>>> +
>>>>> +                    assigned-clocks = <&gcc GCC_USB0_MASTER_CLK>,
>>>>> +                                      <&gcc GCC_USB0_MOCK_UTMI_CLK>;
>>>>> +                    assigned-clock-rates = <200000000>,
>>>>> +                                           <24000000>;
>>>>> +
>>>>> +                    interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                    interrupt-names = "pwr_event";
>>>>> +
>>>>> +                    resets = <&gcc GCC_USB_BCR>;
>>>>> +                    status = "disabled";
>>>>> +
>>>>> +                    dwc_0: usb@8a00000 {
>>>>> +                            compatible = "snps,dwc3";
>>>>> +                            reg = <0x8a00000 0xcd00>;
>>>>> +                            clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
>>>>> +                            clock-names = "ref";
>>>>> +                            interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +                            phys = <&usb_0_qusbphy>, <&usb_0_qmpphy>;
>>>>> +                            phy-names = "usb2-phy", "usb3-phy";
>>>>> +                            tx-fifo-resize;
>>>>> +                            snps,is-utmi-l1-suspend;
>>>>> +                            snps,hird-threshold = /bits/ 8 <0x0>;
>>>>> +                            snps,dis_u2_susphy_quirk;
>>>>> +                            snps,dis_u3_susphy_quirk;
>>>>> +                            dr_mode = "host";
>>>>> +                    };
>>>>> +            };
>>>>> +
>>>>>              intc: interrupt-controller@b000000 {
>>>>>                      compatible = "qcom,msm-qgic2";
>>>>>                      reg = <0x0b000000 0x1000>,  /* GICD */
>>>>
>>>> --
>>>> With best wishes
>>>> Dmitry
>>>>
>>
>>
>>
>> --
>> With best wishes
>> Dmitry

-- 
With best wishes
Dmitry


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v8 7/8] arm64: dts: qcom: ipq9574: Add USB related nodes

[Varadarajan Narayanan]

On Thu, Apr 27, 2023 at 08:50:39PM +0300, Dmitry Baryshkov wrote:
> On 26/04/2023 12:51, Varadarajan Narayanan wrote:
> >On Mon, Apr 24, 2023 at 02:17:06PM +0300, Dmitry Baryshkov wrote:
> >>On Mon, 24 Apr 2023 at 12:04, Varadarajan Narayanan
> >><quic_varada@quicinc.com> wrote:
> >>>
> >>>On Sat, Apr 22, 2023 at 12:07:01AM +0300, Dmitry Baryshkov wrote:
> >>>>On 05/04/2023 14:41, Varadarajan Narayanan wrote:
> >>>>>Add USB phy and controller related nodes
> >>>>>
> >>>>>Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> >>>>>---
> >>>>>  Changes in v8:
> >>>>>     - Change clocks order to match the bindings
> >>>>>  Changes in v7:
> >>>>>     - Change com_aux -> cfg_ahb
> >>>>>  Changes in v6:
> >>>>>     - Introduce fixed regulators for the phy
> >>>>>     - Resolved all 'make dtbs_check' messages
> >>>>>
> >>>>>  Changes in v5:
> >>>>>     - Fix additional comments
> >>>>>     - Edit nodes to match with qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >>>>>     - 'make dtbs_check' giving the following messages since
> >>>>>       ipq9574 doesn't have power domains. Hope this is ok
> >>>>>
> >>>>>             /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: phy@7d000: 'power-domains' is a required property
> >>>>>             From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-usb3-uni-phy.yaml
> >>>>>             /local/mnt/workspace/varada/varda-linux/arch/arm64/boot/dts/qcom/ipq9574-al02-c7.dtb: usb@8a00000: 'power-domains' is a required property
> >>>>>             From schema: /local/mnt/workspace/varada/varda-linux/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> >>>>>
> >>>>>  Changes in v4:
> >>>>>     - Use newer bindings without subnodes
> >>>>>     - Fix coding style issues
> >>>>>
> >>>>>  Changes in v3:
> >>>>>     - Insert the nodes at proper location
> >>>>>
> >>>>>  Changes in v2:
> >>>>>     - Fixed issues flagged by Krzysztof
> >>>>>     - Fix issues reported by make dtbs_check
> >>>>>     - Remove NOC related clocks (to be added with proper
> >>>>>       interconnect support)
> >>>>>---
> >>>>>  arch/arm64/boot/dts/qcom/ipq9574.dtsi | 120 ++++++++++++++++++++++++++++++++++
> >>>>>  1 file changed, 120 insertions(+)
> >>>>>
> >>>>>diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >>>>>index 43a3dbe..1242382 100644
> >>>>>--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >>>>>+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
> >>>>>@@ -150,6 +150,33 @@
> >>>>>             method = "smc";
> >>>>>     };
> >>>>>+    reg_usb_3p3: s3300 {
> >>>>
> >>>>The node names do not look generic enough. Please take a look at other
> >>>>platforms.
> >>>
> >>>Please see below.
> >>>
> >>>>>+            compatible = "regulator-fixed";
> >>>>>+            regulator-min-microvolt = <3300000>;
> >>>>>+            regulator-max-microvolt = <3300000>;
> >>>>>+            regulator-boot-on;
> >>>>>+            regulator-always-on;
> >>>>>+            regulator-name = "usb-phy-vdd-dummy";
> >>>>
> >>>>This also doesn't look correct. This regulator should not just fill the gap.
> >>>>Does it represent a generic voltage network on the board?
> >>>>
> >>>>Please do not add 'dummy' voltage regulators if there is no real voltage
> >>>>wire.
> >>>
> >>>These are real voltage wires. I used dummy since they are
> >>>always-on and cannot be increased/decreased (i.e. fixed).
> >>>Would something along the following lines be appropriate?
> >>
> >>Still not fully correct. Please use regulator name that corresponds to
> >>the power grid on the board schematics. I don't think that you have a
> >>separate power grids for USB PHY.
> >>
> >>>
> >>>         vreg_ae10_3p3: s3300 {
> >>
> >>Naming suggests that these voltages are generated by some PMIC. Is
> >>this correct? If so, please describe the PMIC instead.
> >
> >SS PHY needs two supplies and HS PHY needs three supplies. 3.3V
> >and 0.925V are from fixed DC - DC regulators and 1.8V is
> >generated from MP5496 PMIC. Would the following node definitions
> >be ok?
> >
> >usb_hs_vreg0: usb_hs_vreg0 {
> >	compatible = "regulator-fixed";
> >	regulator-min-microvolt = <3300000>;
> >	regulator-max-microvolt = <3300000>;
> >	regulator-boot-on;
> >	regulator-always-on;
> >	regulator-name = "usb-phy-vdd";
> >};
> >
> >usb_hs_vreg1: usb_hs_vreg1 {
> >	compatible = "regulator-fixed";
> >	regulator-min-microvolt = <925000>;
> >	regulator-max-microvolt = <925000>;
> >	regulator-boot-on;
> >	regulator-always-on;
> >	regulator-name = "usb-phy";
> >};
>
> Again. The voltage rails on the board are not USB-specific, are they? So why
> are you declaring usb-phy regulators? Would another consumer of 3.3V rail
> use the same usb-phy-vdd regulator?

Ok. Will rename them as follows

	usb_hs_vreg0 -> fixed_3p3
	usb_hs_vreg1 -> fixed_0p925

	regulator-name = "usb-phy-vdd";	-> fixed_3p3
	regulator-name = "usb-phy";	-> fixed_0p925

> >&rpm_requests {
> >	regulators {
> >		compatible = "qcom,rpm-mp5496-regulators";
> >		.
> >		.
> >		.
> >		ipq9574_l2: l2 {
>
> mp5496_l2

Ok.

Thanks
Varada

> >			regulator-min-microvolt = <1800000>;
> >			regulator-max-microvolt = <1800000>;
> >			regulator-boot-on;
> >			regulator-always-on;
> >		};
> >	};
> >};
> >
> >Thanks
> >Varada
> >
> >>>                 compatible = "regulator-fixed";
> >>>                 regulator-min-microvolt = <3300000>;
> >>>                 regulator-max-microvolt = <3300000>;
> >>>                 regulator-boot-on;
> >>>                 regulator-always-on;
> >>>                 regulator-name = "usb-phy-vdd";
> >>>         };
> >>>
> >>>         vreg_ad8_1p8: s1800 {
> >>>                 compatible = "regulator-fixed";
> >>>                 regulator-min-microvolt = <1800000>;
> >>>                 regulator-max-microvolt = <1800000>;
> >>>                 regulator-boot-on;
> >>>                 regulator-always-on;
> >>>                 regulator-name = "usb-phy-pll";
> >>>         };
> >>>
> >>>         vreg_ad9_0p925: s0925 {
> >>>                 compatible = "regulator-fixed";
> >>>                 regulator-min-microvolt = <925000>;
> >>>                 regulator-max-microvolt = <925000>;
> >>>                 regulator-boot-on;
> >>>                 regulator-always-on;
> >>>                 regulator-name = "usb-phy";
> >>>         };
> >>>
> >>>Thanks
> >>>Varada
> >>>
> >>>>>+    };
> >>>>>+
> >>>>>+    reg_usb_1p8: s1800 {
> >>>>>+            compatible = "regulator-fixed";
> >>>>>+            regulator-min-microvolt = <1800000>;
> >>>>>+            regulator-max-microvolt = <1800000>;
> >>>>>+            regulator-boot-on;
> >>>>>+            regulator-always-on;
> >>>>>+            regulator-name = "usb-phy-pll-dummy";
> >>>>>+    };
> >>>>>+
> >>>>>+    reg_usb_0p925: s0925 {
> >>>>>+            compatible = "regulator-fixed";
> >>>>>+            regulator-min-microvolt = <925000>;
> >>>>>+            regulator-max-microvolt = <925000>;
> >>>>>+            regulator-boot-on;
> >>>>>+            regulator-always-on;
> >>>>>+            regulator-name = "usb-phy-dummy";
> >>>>>+    };
> >>>>>+
> >>>>>     reserved-memory {
> >>>>>             #address-cells = <2>;
> >>>>>             #size-cells = <2>;
> >>>>>@@ -179,6 +206,52 @@
> >>>>>             #size-cells = <1>;
> >>>>>             ranges = <0 0 0 0xffffffff>;
> >>>>>+            usb_0_qusbphy: phy@7b000 {
> >>>>>+                    compatible = "qcom,ipq9574-qusb2-phy";
> >>>>>+                    reg = <0x0007b000 0x180>;
> >>>>>+                    #phy-cells = <0>;
> >>>>>+
> >>>>>+                    clocks = <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> >>>>>+                             <&xo_board_clk>;
> >>>>>+                    clock-names = "cfg_ahb",
> >>>>>+                                  "ref";
> >>>>>+
> >>>>>+                    vdd-supply = <&reg_usb_0p925>;
> >>>>>+                    vdda-pll-supply = <&reg_usb_1p8>;
> >>>>>+                    vdda-phy-dpdm-supply = <&reg_usb_3p3>;
> >>>>>+
> >>>>>+                    resets = <&gcc GCC_QUSB2_0_PHY_BCR>;
> >>>>>+                    status = "disabled";
> >>>>>+            };
> >>>>>+
> >>>>>+            usb_0_qmpphy: phy@7d000 {
> >>>>>+                    compatible = "qcom,ipq9574-qmp-usb3-phy";
> >>>>>+                    reg = <0x0007d000 0xa00>;
> >>>>>+                    #phy-cells = <0>;
> >>>>>+
> >>>>>+                    clocks = <&gcc GCC_USB0_AUX_CLK>,
> >>>>>+                             <&xo_board_clk>,
> >>>>>+                             <&gcc GCC_USB0_PHY_CFG_AHB_CLK>,
> >>>>>+                             <&gcc GCC_USB0_PIPE_CLK>;
> >>>>>+                    clock-names = "aux",
> >>>>>+                                  "ref",
> >>>>>+                                  "cfg_ahb",
> >>>>>+                                  "pipe";
> >>>>>+
> >>>>>+                    resets = <&gcc GCC_USB0_PHY_BCR>,
> >>>>>+                             <&gcc GCC_USB3PHY_0_PHY_BCR>;
> >>>>>+                    reset-names = "phy",
> >>>>>+                                  "phy_phy";
> >>>>>+
> >>>>>+                    vdda-pll-supply = <&reg_usb_1p8>;
> >>>>>+                    vdda-phy-supply = <&reg_usb_0p925>;
> >>>>>+
> >>>>>+                    status = "disabled";
> >>>>>+
> >>>>>+                    #clock-cells = <0>;
> >>>>>+                    clock-output-names = "usb0_pipe_clk";
> >>>>>+            };
> >>>>>+
> >>>>>             pcie0_phy: phy@84000 {
> >>>>>                     compatible = "qcom,ipq9574-qmp-gen3x1-pcie-phy";
> >>>>>                     reg = <0x00084000 0x1000>;
> >>>>>@@ -548,6 +621,53 @@
> >>>>>                     status = "disabled";
> >>>>>             };
> >>>>>+            usb3: usb@8a00000 {
> >>>>>+                    compatible = "qcom,ipq9574-dwc3", "qcom,dwc3";
> >>>>>+                    reg = <0x08af8800 0x400>;
> >>>>>+                    #address-cells = <1>;
> >>>>>+                    #size-cells = <1>;
> >>>>>+                    ranges;
> >>>>>+
> >>>>>+                    clocks = <&gcc GCC_SNOC_USB_CLK>,
> >>>>>+                             <&gcc GCC_USB0_MASTER_CLK>,
> >>>>>+                             <&gcc GCC_ANOC_USB_AXI_CLK>,
> >>>>>+                             <&gcc GCC_USB0_SLEEP_CLK>,
> >>>>>+                             <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> >>>>>+
> >>>>>+                    clock-names = "cfg_noc",
> >>>>>+                                  "core",
> >>>>>+                                  "iface",
> >>>>>+                                  "sleep",
> >>>>>+                                  "mock_utmi";
> >>>>>+
> >>>>>+                    assigned-clocks = <&gcc GCC_USB0_MASTER_CLK>,
> >>>>>+                                      <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> >>>>>+                    assigned-clock-rates = <200000000>,
> >>>>>+                                           <24000000>;
> >>>>>+
> >>>>>+                    interrupts-extended = <&intc GIC_SPI 134 IRQ_TYPE_LEVEL_HIGH>;
> >>>>>+                    interrupt-names = "pwr_event";
> >>>>>+
> >>>>>+                    resets = <&gcc GCC_USB_BCR>;
> >>>>>+                    status = "disabled";
> >>>>>+
> >>>>>+                    dwc_0: usb@8a00000 {
> >>>>>+                            compatible = "snps,dwc3";
> >>>>>+                            reg = <0x8a00000 0xcd00>;
> >>>>>+                            clocks = <&gcc GCC_USB0_MOCK_UTMI_CLK>;
> >>>>>+                            clock-names = "ref";
> >>>>>+                            interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> >>>>>+                            phys = <&usb_0_qusbphy>, <&usb_0_qmpphy>;
> >>>>>+                            phy-names = "usb2-phy", "usb3-phy";
> >>>>>+                            tx-fifo-resize;
> >>>>>+                            snps,is-utmi-l1-suspend;
> >>>>>+                            snps,hird-threshold = /bits/ 8 <0x0>;
> >>>>>+                            snps,dis_u2_susphy_quirk;
> >>>>>+                            snps,dis_u3_susphy_quirk;
> >>>>>+                            dr_mode = "host";
> >>>>>+                    };
> >>>>>+            };
> >>>>>+
> >>>>>             intc: interrupt-controller@b000000 {
> >>>>>                     compatible = "qcom,msm-qgic2";
> >>>>>                     reg = <0x0b000000 0x1000>,  /* GICD */
> >>>>
> >>>>--
> >>>>With best wishes
> >>>>Dmitry
> >>>>
> >>
> >>
> >>
> >>--
> >>With best wishes
> >>Dmitry
>
> --
> With best wishes
> Dmitry
>

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy