[PATCH v4 0/5] Add STM32MP25 USB3/PCIE COMBOPHY driver

[PATCH v4 0/5] Add STM32MP25 USB3/PCIE COMBOPHY driver

[Christian Bruel]

Address comments from Rob and Krzysztof

Changes in v4:
   - "#phy-cells": Drop type item description since it is specified
     by user node phandle.
   - Rename stm32-combophy.yaml to match compatible
   - Drop wakeup-source from bindings (should be generic)
   - Alphabetically reorder required: list.
   - Drop "Reviewed-by" since those previous changes

Changes in v3:
   - Reorder MAINTAINERS patch

Changes in v2:
   - Reorder entries
   - Rename clock_names and reset_names bindings
   - Rename and clarify rx-equalizer binding 

Christian Bruel (5):
  dt-bindings: phy: Add STM32MP25 COMBOPHY bindings
  phy: stm32: Add support for STM32MP25 COMBOPHY.
  MAINTAINERS: add entry for ST STM32MP25 COMBOPHY driver
  arm64: dts: st: Add combophy node on stm32mp251
  arm64: dts: st: Enable COMBOPHY on the stm32mp257f-ev1 board

 .../bindings/phy/st,stm32mp25-combophy.yaml   | 128 ++++
 MAINTAINERS                                   |   6 +
 arch/arm64/boot/dts/st/stm32mp251.dtsi        |  17 +
 arch/arm64/boot/dts/st/stm32mp257f-ev1.dts    |  14 +
 drivers/phy/st/Kconfig                        |  11 +
 drivers/phy/st/Makefile                       |   1 +
 drivers/phy/st/phy-stm32-combophy.c           | 607 ++++++++++++++++++
 7 files changed, 784 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
 create mode 100644 drivers/phy/st/phy-stm32-combophy.c

-- 
2.34.1

[PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Christian Bruel]

Document the bindings for STM32 COMBOPHY interface, used to support
the PCIe and USB3 stm32mp25 drivers.
Following entries can be used to tune caracterisation parameters
 - st,output-micro-ohms and st,output-vswing-microvolt bindings entries
to tune the impedance and voltage swing using discrete simulation results
 - st,rx-equalizer register to set the internal rx equalizer filter value.

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 .../bindings/phy/st,stm32mp25-combophy.yaml   | 128 ++++++++++++++++++
 1 file changed, 128 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml

diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
new file mode 100644
index 000000000000..8d4a40b94507
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
@@ -0,0 +1,128 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY
+
+maintainers:
+  - Christian Bruel <christian.bruel@foss.st.com>
+
+description:
+  Single lane PHY shared (exclusive) between the USB3 and PCIe controllers.
+  Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1.
+
+properties:
+  compatible:
+    const: st,stm32mp25-combophy
+
+  reg:
+    maxItems: 1
+
+  "#phy-cells":
+    const: 1
+
+  clocks:
+    minItems: 2
+    items:
+      - description: apb Bus clock mandatory to access registers.
+      - description: ker Internal RCC reference clock for USB3 or PCIe
+      - description: pad Optional on board clock input for PCIe only. Typically an
+                     external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference
+                     clock input instead of the ker
+
+  clock-names:
+    minItems: 2
+    items:
+      - const: apb
+      - const: ker
+      - const: pad
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: phy
+
+  power-domains:
+    maxItems: 1
+
+  wakeup-source: true
+
+  interrupts:
+    maxItems: 1
+    description: interrupt used for wakeup
+
+  access-controllers:
+    minItems: 1
+    maxItems: 2
+
+  st,syscfg:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle to the SYSCON entry required for configuring PCIe
+      or USB3.
+
+  st,ssc-on:
+    type: boolean
+    description:
+      A boolean property whose presence indicates that the SSC for common clock
+      needs to be set.
+
+  st,rx-equalizer:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 7
+    default: 2
+    description:
+      A 3 bit value to tune the RX fixed equalizer setting for optimal eye compliance
+
+  st,output-micro-ohms:
+    minimum: 3999000
+    maximum: 6090000
+    default: 4968000
+    description:
+      A value property to tune the Single Ended Output Impedance, simulations results
+      at 25C for a VDDP=0.8V. The hardware accepts discrete values in this range.
+
+  st,output-vswing-microvolt:
+    minimum: 442000
+    maximum: 803000
+    default: 803000
+    description:
+      A value property in microvolt to tune the Single Ended Output Voltage Swing to change the
+      Vlo, Vhi for a VDDP = 0.8V. The hardware accepts discrete values in this range.
+
+required:
+  - "#phy-cells"
+  - compatible
+  - clocks
+  - clock-names
+  - reg
+  - resets
+  - reset-names
+  - st,syscfg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
+
+    combophy: phy@480c0000 {
+        compatible = "st,stm32mp25-combophy";
+        reg = <0x480c0000 0x1000>;
+        #phy-cells = <1>;
+        clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>;
+        clock-names = "apb", "ker";
+        resets = <&rcc USB3PCIEPHY_R>;
+        reset-names = "phy";
+        st,syscfg = <&syscfg>;
+        access-controllers = <&rifsc 67>;
+        power-domains = <&CLUSTER_PD>;
+        wakeup-source;
+        interrupts-extended = <&exti1 45 IRQ_TYPE_EDGE_FALLING>;
+    };
+...
-- 
2.34.1

[PATCH v4 2/5] phy: stm32: Add support for STM32MP25 COMBOPHY.

[Christian Bruel]

Addition of the COMBOPHY driver found on STM32MP25 platforms

This single lane PHY is shared (exclusive) between the USB3 and PCIE
controllers.
Supports 5Gbit/s for PCIE gen2 or 2.5Gbit/s for PCIE gen1.

Supports wakeup-source capability to wakeup system using remote-wakeup
capable USB device

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 drivers/phy/st/Kconfig              |  11 +
 drivers/phy/st/Makefile             |   1 +
 drivers/phy/st/phy-stm32-combophy.c | 607 ++++++++++++++++++++++++++++
 3 files changed, 619 insertions(+)
 create mode 100644 drivers/phy/st/phy-stm32-combophy.c

diff --git a/drivers/phy/st/Kconfig b/drivers/phy/st/Kconfig
index 3fc3d0781fb8..304614b6dabf 100644
--- a/drivers/phy/st/Kconfig
+++ b/drivers/phy/st/Kconfig
@@ -33,6 +33,17 @@ config PHY_STIH407_USB
 	  Enable this support to enable the picoPHY device used by USB2
 	  and USB3 controllers on STMicroelectronics STiH407 SoC families.
 
+config PHY_STM32_COMBOPHY
+	tristate "STMicroelectronics COMBOPHY driver for STM32MP25"
+	depends on ARCH_STM32 || COMPILE_TEST
+	select GENERIC_PHY
+	help
+	  Enable this to support the COMBOPHY device used by USB3 or PCIe
+	  controllers on STMicroelectronics STM32MP25 SoC.
+	  This driver controls the COMBOPHY block to generate the PCIe 100Mhz
+	  reference clock from either the external clock generator or HSE
+	  internal SoC clock source.
+
 config PHY_STM32_USBPHYC
 	tristate "STMicroelectronics STM32 USB HS PHY Controller driver"
 	depends on ARCH_STM32 || COMPILE_TEST
diff --git a/drivers/phy/st/Makefile b/drivers/phy/st/Makefile
index c862dd937b64..cb80e954ea9f 100644
--- a/drivers/phy/st/Makefile
+++ b/drivers/phy/st/Makefile
@@ -3,4 +3,5 @@ obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
 obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
 obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
 obj-$(CONFIG_PHY_STIH407_USB)		+= phy-stih407-usb.o
+obj-$(CONFIG_PHY_STM32_COMBOPHY)	+= phy-stm32-combophy.o
 obj-$(CONFIG_PHY_STM32_USBPHYC) 	+= phy-stm32-usbphyc.o
diff --git a/drivers/phy/st/phy-stm32-combophy.c b/drivers/phy/st/phy-stm32-combophy.c
new file mode 100644
index 000000000000..7cd4193b0277
--- /dev/null
+++ b/drivers/phy/st/phy-stm32-combophy.c
@@ -0,0 +1,607 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * STMicroelectronics COMBOPHY STM32MP25 Controller driver.
+ *
+ * Copyright (C) 2024 STMicroelectronics
+ * Author: Christian Bruel <christian.bruel@foss.st.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <dt-bindings/phy/phy.h>
+
+#define SYSCFG_COMBOPHY_CR1 0x4C00
+#define SYSCFG_COMBOPHY_CR2 0x4C04
+#define SYSCFG_COMBOPHY_CR4 0x4C0C
+#define SYSCFG_COMBOPHY_CR5 0x4C10
+#define SYSCFG_COMBOPHY_SR  0x4C14
+#define SYSCFG_PCIEPRGCR    0x6080
+
+/* SYSCFG PCIEPRGCR */
+#define STM32MP25_PCIEPRGCR_EN	  BIT(0)
+#define STM32MP25_PCIEPRG_IMPCTRL_OHM     GENMASK(3, 1)
+#define STM32MP25_PCIEPRG_IMPCTRL_VSWING  GENMASK(5, 4)
+
+/* SYSCFG SYSCFG_COMBOPHY_SR */
+#define STM32MP25_PIPE0_PHYSTATUS BIT(1)
+
+/* SYSCFG CR1 */
+#define SYSCFG_COMBOPHY_CR1_REFUSEPAD BIT(0)
+#define SYSCFG_COMBOPHY_CR1_MPLLMULT GENMASK(7, 1)
+#define SYSCFG_COMBOPHY_CR1_REFCLKSEL GENMASK(16, 8)
+#define SYSCFG_COMBOPHY_CR1_REFCLKDIV2 BIT(17)
+#define SYSCFG_COMBOPHY_CR1_REFSSPEN BIT(18)
+#define SYSCFG_COMBOPHY_CR1_SSCEN BIT(19)
+
+/* SYSCFG CR4 */
+#define SYSCFG_COMBOPHY_CR4_RX0_EQ GENMASK(2, 0)
+
+#define MPLLMULT_19_2 (0x02u << 1)
+#define MPLLMULT_20   (0x7Du << 1)
+#define MPLLMULT_24   (0x68u << 1)
+#define MPLLMULT_25   (0x64u << 1)
+#define MPLLMULT_26   (0x60u << 1)
+#define MPLLMULT_38_4 (0x41u << 1)
+#define MPLLMULT_48   (0x6Cu << 1)
+#define MPLLMULT_50   (0x32u << 1)
+#define MPLLMULT_52   (0x30u << 1)
+#define MPLLMULT_100  (0x19u << 1)
+
+#define REFCLKSEL_0   0
+#define REFCLKSEL_1   (0x108u << 8)
+
+#define REFCLDIV_0    0
+
+/* SYSCFG CR2 */
+#define SYSCFG_COMBOPHY_CR2_MODESEL GENMASK(1, 0)
+#define SYSCFG_COMBOPHY_CR2_ISO_DIS BIT(15)
+
+#define COMBOPHY_MODESEL_PCIE 0
+#define COMBOPHY_MODESEL_USB  3
+
+/* SYSCFG CR5 */
+#define SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS BIT(12)
+
+#define COMBOPHY_SUP_ANA_MPLL_LOOP_CTL 0xC0
+#define COMBOPHY_PROP_CNTRL GENMASK(7, 4)
+
+struct stm32_combophy {
+	struct phy *phy;
+	struct regmap *regmap;
+	struct device *dev;
+	void __iomem *base;
+	struct reset_control *phy_reset;
+	struct clk *phy_clk;
+	struct clk *pad_clk;
+	struct clk *ker_clk;
+	unsigned int type;
+	bool is_init;
+	int irq_wakeup;
+};
+
+struct clk_impedance  {
+	u32 microohm;
+	u32 vswing[4];
+};
+
+/*
+ * lookup table to hold the settings needed for a ref clock frequency
+ * impedance, the offset is used to set the IMP_CTL and DE_EMP bit of the
+ * PRG_IMP_CTRL register
+ */
+static const struct clk_impedance imp_lookup[] = {
+	{ 6090000, { 442000, 564000, 684000, 802000 } },
+	{ 5662000, { 528000, 621000, 712000, 803000 } },
+	{ 5292000, { 491000, 596000, 700000, 802000 } },
+	{ 4968000, { 558000, 640000, 722000, 803000 } },
+	{ 4684000, { 468000, 581000, 692000, 802000 } },
+	{ 4429000, { 554000, 613000, 717000, 803000 } },
+	{ 4204000, { 511000, 609000, 706000, 802000 } },
+	{ 3999000, { 571000, 648000, 726000, 803000 } }
+};
+
+static int stm32_impedance_tune(struct stm32_combophy *combophy)
+{
+	u8 imp_size = ARRAY_SIZE(imp_lookup);
+	u8 vswing_size = ARRAY_SIZE(imp_lookup[0].vswing);
+	u8 imp_of, vswing_of;
+	u32 max_imp = imp_lookup[0].microohm;
+	u32 min_imp = imp_lookup[imp_size - 1].microohm;
+	u32 max_vswing = imp_lookup[imp_size - 1].vswing[vswing_size - 1];
+	u32 min_vswing = imp_lookup[0].vswing[0];
+	u32 val;
+
+	if (!of_property_read_u32(combophy->dev->of_node, "st,output-micro-ohms", &val)) {
+		if (val < min_imp || val > max_imp) {
+			dev_err(combophy->dev, "Invalid value %u for output ohm\n", val);
+			return -EINVAL;
+		}
+
+		for (imp_of = 0 ; imp_of < ARRAY_SIZE(imp_lookup); imp_of++)
+			if (imp_lookup[imp_of].microohm <= val)
+				break;
+
+		dev_dbg(combophy->dev, "Set %u micro-ohms output impedance\n",
+			imp_lookup[imp_of].microohm);
+
+		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
+				   STM32MP25_PCIEPRG_IMPCTRL_OHM,
+				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_OHM, imp_of));
+	} else {
+		regmap_read(combophy->regmap, SYSCFG_PCIEPRGCR, &val);
+		imp_of = FIELD_GET(STM32MP25_PCIEPRG_IMPCTRL_OHM, val);
+	}
+
+	if (!of_property_read_u32(combophy->dev->of_node, "st,output-vswing-microvolt", &val)) {
+		if (val < min_vswing || val > max_vswing) {
+			dev_err(combophy->dev, "Invalid value %u for output vswing\n", val);
+			return -EINVAL;
+		}
+
+		for (vswing_of = 0 ; vswing_of < ARRAY_SIZE(imp_lookup[imp_of].vswing); vswing_of++)
+			if (imp_lookup[imp_of].vswing[vswing_of] >= val)
+				break;
+
+		dev_dbg(combophy->dev, "Set %u microvolt swing\n",
+			 imp_lookup[imp_of].vswing[vswing_of]);
+
+		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
+				   STM32MP25_PCIEPRG_IMPCTRL_VSWING,
+				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_VSWING, vswing_of));
+	}
+
+	return 0;
+}
+
+static int stm32_combophy_pll_init(struct stm32_combophy *combophy)
+{
+	int ret;
+	u32 refclksel, pllmult, propcntrl, val;
+	u32 clk_rate;
+
+	if (combophy->pad_clk)
+		clk_rate = clk_get_rate(combophy->pad_clk);
+	else
+		clk_rate = clk_get_rate(combophy->ker_clk);
+
+	reset_control_assert(combophy->phy_reset);
+
+	dev_dbg(combophy->dev, "%s pll init rate %d\n",
+		combophy->pad_clk ? "External" : "Ker", clk_rate);
+
+	/*
+	 * vddcombophy is interconnected with vddcore. Isolation bit should be unset
+	 * before using the ComboPHY.
+	 */
+	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
+			   SYSCFG_COMBOPHY_CR2_ISO_DIS, SYSCFG_COMBOPHY_CR2_ISO_DIS);
+
+	if (combophy->type != PHY_TYPE_PCIE)
+		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+				   SYSCFG_COMBOPHY_CR1_REFSSPEN, SYSCFG_COMBOPHY_CR1_REFSSPEN);
+
+	if (combophy->type == PHY_TYPE_PCIE && !combophy->pad_clk)
+		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
+				   STM32MP25_PCIEPRGCR_EN, STM32MP25_PCIEPRGCR_EN);
+
+	if (of_property_read_bool(combophy->dev->of_node, "st,ssc-on")) {
+		dev_dbg(combophy->dev, "Enabling clock with SSC\n");
+		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+				   SYSCFG_COMBOPHY_CR1_SSCEN, SYSCFG_COMBOPHY_CR1_SSCEN);
+	}
+
+	if (!of_property_read_u32(combophy->dev->of_node, "st,rx-equalizer", &val)) {
+		dev_dbg(combophy->dev, "Set RX equalizer %u\n", val);
+		if (val > SYSCFG_COMBOPHY_CR4_RX0_EQ) {
+			dev_err(combophy->dev, "Invalid value %u for rx0 equalizer\n", val);
+			return -EINVAL;
+		}
+
+		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR4,
+			   SYSCFG_COMBOPHY_CR4_RX0_EQ, val);
+	}
+
+	if (combophy->type == PHY_TYPE_PCIE) {
+		ret = stm32_impedance_tune(combophy);
+		if (ret) {
+			reset_control_deassert(combophy->phy_reset);
+			goto out;
+		}
+
+		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+				   SYSCFG_COMBOPHY_CR1_REFUSEPAD,
+				   combophy->pad_clk ? SYSCFG_COMBOPHY_CR1_REFUSEPAD : 0);
+	}
+
+	switch (clk_rate) {
+	case 100000000:
+		pllmult = MPLLMULT_100;
+		refclksel = REFCLKSEL_0;
+		propcntrl = 0x8u << 4;
+		break;
+	case 19200000:
+		pllmult = MPLLMULT_19_2;
+		refclksel = REFCLKSEL_1;
+		propcntrl = 0x8u << 4;
+		break;
+	case 25000000:
+		pllmult = MPLLMULT_25;
+		refclksel = REFCLKSEL_0;
+		propcntrl = 0xEu << 4;
+		break;
+	case 24000000:
+		pllmult = MPLLMULT_24;
+		refclksel = REFCLKSEL_1;
+		propcntrl = 0xEu << 4;
+		break;
+	case 20000000:
+		pllmult = MPLLMULT_20;
+		refclksel = REFCLKSEL_0;
+		propcntrl = 0xEu << 4;
+		break;
+	default:
+		dev_err(combophy->dev, "Invalid rate 0x%x\n", clk_rate);
+		reset_control_deassert(combophy->phy_reset);
+		ret = -EINVAL;
+		goto out;
+	};
+
+	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+			   SYSCFG_COMBOPHY_CR1_REFCLKDIV2, REFCLDIV_0);
+	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+			   SYSCFG_COMBOPHY_CR1_REFCLKSEL, refclksel);
+	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+			   SYSCFG_COMBOPHY_CR1_MPLLMULT, pllmult);
+
+	/*
+	 * Force elasticity buffer to be tuned for the reference clock as
+	 * the separated clock model is not supported
+	 */
+	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR5,
+			   SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS, SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS);
+
+	reset_control_deassert(combophy->phy_reset);
+
+	ret = regmap_read_poll_timeout(combophy->regmap, SYSCFG_COMBOPHY_SR, val,
+				       !(val & STM32MP25_PIPE0_PHYSTATUS),
+				       10, 1000);
+	if (ret) {
+		dev_err(combophy->dev, "timeout, cannot lock PLL\n");
+		goto out;
+	}
+
+	if (combophy->type == PHY_TYPE_PCIE) {
+		val = readl_relaxed(combophy->base + COMBOPHY_SUP_ANA_MPLL_LOOP_CTL);
+		val &= ~COMBOPHY_PROP_CNTRL;
+		val |= propcntrl;
+		writel_relaxed(val, combophy->base + COMBOPHY_SUP_ANA_MPLL_LOOP_CTL);
+	}
+
+	return 0;
+
+out:
+	if (combophy->type == PHY_TYPE_PCIE && !combophy->pad_clk)
+		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
+				   STM32MP25_PCIEPRGCR_EN, 0);
+
+	if (combophy->type != PHY_TYPE_PCIE)
+		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+				   SYSCFG_COMBOPHY_CR1_REFSSPEN, 0);
+
+	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
+			   SYSCFG_COMBOPHY_CR2_ISO_DIS, 0);
+
+	return ret;
+}
+
+static struct phy *stm32_combophy_xlate(struct device *dev,
+					const struct of_phandle_args *args)
+{
+	struct stm32_combophy *combophy = dev_get_drvdata(dev);
+	unsigned int type;
+
+	if (args->args_count != 1) {
+		dev_err(dev, "invalid number of cells in 'phy' property\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	type = args->args[0];
+	if (type != PHY_TYPE_USB3 && type != PHY_TYPE_PCIE) {
+		dev_err(dev, "unsupported device type: %d\n", type);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (combophy->pad_clk && type != PHY_TYPE_PCIE) {
+		dev_err(dev, "Invalid use of clk_pad for USB3 mode\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	combophy->type = type;
+
+	return combophy->phy;
+}
+
+static int stm32_combophy_set_mode(struct stm32_combophy *combophy)
+{
+	int type = combophy->type;
+	u32 val;
+
+	switch (type) {
+	case PHY_TYPE_PCIE:
+		dev_dbg(combophy->dev, "setting PCIe ComboPHY\n");
+		val = COMBOPHY_MODESEL_PCIE;
+		break;
+	case PHY_TYPE_USB3:
+		dev_dbg(combophy->dev, "setting USB3 ComboPHY\n");
+		val = COMBOPHY_MODESEL_USB;
+		break;
+	default:
+		dev_err(combophy->dev, "Invalid PHY mode %d\n", type);
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
+				  SYSCFG_COMBOPHY_CR2_MODESEL, val);
+}
+
+static int stm32_combophy_enable_clocks(struct stm32_combophy *combophy)
+{
+	int ret;
+
+	ret = clk_prepare_enable(combophy->phy_clk);
+	if (ret) {
+		dev_err(combophy->dev, "Core clock enable failed %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(combophy->ker_clk);
+	if (ret) {
+		dev_err(combophy->dev, "ker_usb3pcie clock enable failed %d\n", ret);
+		clk_disable_unprepare(combophy->phy_clk);
+		return ret;
+	}
+
+	if (combophy->pad_clk) {
+		ret = clk_prepare_enable(combophy->pad_clk);
+		if (ret) {
+			dev_err(combophy->dev, "External clock enable failed %d\n", ret);
+			clk_disable_unprepare(combophy->ker_clk);
+			clk_disable_unprepare(combophy->phy_clk);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static void stm32_combophy_disable_clocks(struct stm32_combophy *combophy)
+{
+	if (combophy->pad_clk)
+		clk_disable_unprepare(combophy->pad_clk);
+	clk_disable_unprepare(combophy->ker_clk);
+	clk_disable_unprepare(combophy->phy_clk);
+}
+
+static int stm32_combophy_suspend_noirq(struct device *dev)
+{
+	struct stm32_combophy *combophy = dev_get_drvdata(dev);
+
+	/*
+	 * Clocks should be turned off since it is not needed for
+	 * wakeup capability. In case usb-remote wakeup is not enabled,
+	 * combo-phy is already turned off by HCD driver using exit callback
+	 */
+	if (combophy->is_init) {
+		stm32_combophy_disable_clocks(combophy);
+
+		/* since wakeup is enabled for ctrl */
+		enable_irq_wake(combophy->irq_wakeup);
+	}
+
+	return 0;
+}
+
+static int stm32_combophy_resume_noirq(struct device *dev)
+{
+	struct stm32_combophy *combophy = dev_get_drvdata(dev);
+	int ret;
+
+	/*
+	 * If clocks was turned off by suspend call for wakeup then needs
+	 * to be turned back ON in resume. In case usb-remote wakeup is not
+	 * enabled, clocks already turned ON by HCD driver using init callback
+	 */
+	if (combophy->is_init) {
+		/* since wakeup was enabled for ctrl */
+		disable_irq_wake(combophy->irq_wakeup);
+
+		ret = stm32_combophy_enable_clocks(combophy);
+		if (ret) {
+			dev_err(dev, "can't enable clocks (%d)\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int stm32_combophy_exit(struct phy *phy)
+{
+	struct stm32_combophy *combophy = phy_get_drvdata(phy);
+	struct device *dev = combophy->dev;
+
+	combophy->is_init = false;
+
+	if (combophy->type == PHY_TYPE_PCIE && !combophy->pad_clk)
+		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
+				   STM32MP25_PCIEPRGCR_EN, 0);
+
+	if (combophy->type != PHY_TYPE_PCIE)
+		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
+				   SYSCFG_COMBOPHY_CR1_REFSSPEN, 0);
+
+	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
+			   SYSCFG_COMBOPHY_CR2_ISO_DIS, 0);
+
+	stm32_combophy_disable_clocks(combophy);
+
+	pm_runtime_put_noidle(dev);
+
+	return 0;
+}
+
+static int stm32_combophy_init(struct phy *phy)
+{
+	struct stm32_combophy *combophy = phy_get_drvdata(phy);
+	struct device *dev = combophy->dev;
+	int ret;
+
+	pm_runtime_get_noresume(dev);
+
+	ret = stm32_combophy_enable_clocks(combophy);
+	if (ret) {
+		dev_err(dev, "Clock enable failed %d\n", ret);
+		pm_runtime_put_noidle(dev);
+		return ret;
+	}
+
+	ret = stm32_combophy_set_mode(combophy);
+	if (ret) {
+		dev_err(dev, "combophy mode not set\n");
+		stm32_combophy_disable_clocks(combophy);
+		pm_runtime_put_noidle(dev);
+		return ret;
+	}
+
+	ret = stm32_combophy_pll_init(combophy);
+	if (ret) {
+		stm32_combophy_disable_clocks(combophy);
+		pm_runtime_put_noidle(dev);
+		return ret;
+	}
+
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	combophy->is_init = true;
+
+	return ret;
+}
+
+static const struct phy_ops stm32_combophy_phy_data = {
+	.init = stm32_combophy_init,
+	.exit = stm32_combophy_exit,
+	.owner = THIS_MODULE
+};
+
+static irqreturn_t stm32_combophy_irq_wakeup_handler(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+
+static int stm32_combophy_probe(struct platform_device *pdev)
+{
+	struct stm32_combophy *combophy;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct phy_provider *phy_provider;
+	int ret, irq;
+
+	combophy = devm_kzalloc(dev, sizeof(*combophy), GFP_KERNEL);
+	if (!combophy)
+		return -ENOMEM;
+
+	combophy->dev = dev;
+
+	dev_set_drvdata(dev, combophy);
+
+	combophy->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(combophy->base))
+		return PTR_ERR(combophy->base);
+
+	combophy->phy_clk = devm_clk_get(dev, "apb");
+	if (IS_ERR(combophy->phy_clk))
+		return dev_err_probe(dev, PTR_ERR(combophy->phy_clk),
+				     "Failed to get PHY clock source\n");
+
+	combophy->ker_clk = devm_clk_get(dev, "ker");
+	if (IS_ERR(combophy->ker_clk))
+		return dev_err_probe(dev, PTR_ERR(combophy->ker_clk),
+				     "Failed to get PHY internal clock source\n");
+
+	combophy->pad_clk = devm_clk_get_optional(dev, "pad");
+	if (IS_ERR(combophy->pad_clk))
+		return dev_err_probe(dev, PTR_ERR(combophy->pad_clk),
+				     "Failed to get PHY external clock source\n");
+
+	combophy->phy_reset = devm_reset_control_get(dev, "phy");
+	if (IS_ERR(combophy->phy_reset))
+		return dev_err_probe(dev, PTR_ERR(combophy->phy_reset),
+				     "Failed to get PHY reset\n");
+
+	combophy->regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
+	if (IS_ERR(combophy->regmap))
+		return dev_err_probe(dev, PTR_ERR(combophy->regmap),
+				     "No syscfg phandle specified\n");
+
+	combophy->phy = devm_phy_create(dev, NULL, &stm32_combophy_phy_data);
+	if (IS_ERR(combophy->phy))
+		return dev_err_probe(dev, PTR_ERR(combophy->phy),
+				     "failed to create PCIe/USB3 ComboPHY\n");
+
+	if (device_property_read_bool(dev, "wakeup-source")) {
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0)
+			return dev_err_probe(dev, irq, "failed to get IRQ\n");
+		combophy->irq_wakeup = irq;
+
+		ret = devm_request_threaded_irq(dev, combophy->irq_wakeup, NULL,
+						stm32_combophy_irq_wakeup_handler, IRQF_ONESHOT,
+						NULL, NULL);
+		if (ret)
+			return dev_err_probe(dev, ret, "unable to request wake IRQ %d\n",
+						 combophy->irq_wakeup);
+	}
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable pm runtime\n");
+
+	phy_set_drvdata(combophy->phy, combophy);
+
+	phy_provider = devm_of_phy_provider_register(dev, stm32_combophy_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct dev_pm_ops stm32_combophy_pm_ops = {
+	NOIRQ_SYSTEM_SLEEP_PM_OPS(stm32_combophy_suspend_noirq,
+				  stm32_combophy_resume_noirq)
+};
+
+static const struct of_device_id stm32_combophy_of_match[] = {
+	{ .compatible = "st,stm32mp25-combophy", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, stm32_combophy_of_match);
+
+static struct platform_driver stm32_combophy_driver = {
+	.probe = stm32_combophy_probe,
+	.driver = {
+		   .name = "stm32-combophy",
+		   .of_match_table = stm32_combophy_of_match,
+		   .pm = pm_sleep_ptr(&stm32_combophy_pm_ops)
+	}
+};
+
+module_platform_driver(stm32_combophy_driver);
+
+MODULE_AUTHOR("Christian Bruel <christian.bruel@foss.st.com>");
+MODULE_DESCRIPTION("STM32MP25 Combophy USB3/PCIe controller driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v4 3/5] MAINTAINERS: add entry for ST STM32MP25 COMBOPHY driver

[Christian Bruel]

Add myself as STM32MP25 COMBOPHY maintainer

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index f328373463b0..4dc304ba6f90 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21910,6 +21910,12 @@ F:	arch/m68k/kernel/*sun3*
 F:	arch/m68k/sun3*/
 F:	drivers/net/ethernet/i825xx/sun3*
 
+STMICROELECTRONICS STMP32MP25 USB3/PCIE COMBOPHY DRIVER
+M:	Christian Bruel <christian.bruel@foss.st.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
+F:	drivers/phy/st/phy-stm32-combophy.c
+
 SUN4I LOW RES ADC ATTACHED TABLET KEYS DRIVER
 M:	Hans de Goede <hdegoede@redhat.com>
 L:	linux-input@vger.kernel.org
-- 
2.34.1

[PATCH v4 4/5] arm64: dts: st: Add combophy node on stm32mp251

[Christian Bruel]

Add support for COMBOPHY which is used either by the USB3 and PCIe
controller.
USB3 or PCIe mode is done with phy_set_mode().
PCIe internal reference clock can be generated from the internal clock
source or optionnaly from an external 100Mhz pad.

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 arch/arm64/boot/dts/st/stm32mp251.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index 1167cf63d7e8..0d38763d9317 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -7,6 +7,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/reset/st,stm32mp25-rcc.h>
 #include <dt-bindings/regulator/st,stm32mp25-regulator.h>
+#include <dt-bindings/phy/phy.h>
 
 / {
 	#address-cells = <2>;
@@ -518,6 +519,22 @@ i2c8: i2c@46040000 {
 				status = "disabled";
 			};
 
+			combophy: phy@480c0000 {
+				compatible = "st,stm32mp25-combophy";
+				reg = <0x480c0000 0x1000>;
+				#phy-cells = <1>;
+				clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>;
+				clock-names = "apb", "ker";
+				resets = <&rcc USB3PCIEPHY_R>;
+				reset-names = "phy";
+				st,syscfg = <&syscfg>;
+				access-controllers = <&rifsc 67>;
+				power-domains = <&CLUSTER_PD>;
+				wakeup-source;
+				interrupts-extended = <&exti1 45 IRQ_TYPE_EDGE_FALLING>;
+				status = "disabled";
+			};
+
 			sdmmc1: mmc@48220000 {
 				compatible = "st,stm32mp25-sdmmc2", "arm,pl18x", "arm,primecell";
 				arm,primecell-periphid = <0x00353180>;
-- 
2.34.1

[PATCH v4 5/5] arm64: dts: st: Enable COMBOPHY on the stm32mp257f-ev1 board

[Christian Bruel]

Enable the COMBOPHY with external pad clock on stm32mp257f-ev1
board, to be used for the PCIe clock provider.

Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
---
 arch/arm64/boot/dts/st/stm32mp257f-ev1.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
index 214191a8322b..bcf84d533cb2 100644
--- a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
+++ b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
@@ -27,6 +27,14 @@ chosen {
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		pad_clk: pad-clk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <100000000>;
+		};
+	};
+
 	memory@80000000 {
 		device_type = "memory";
 		reg = <0x0 0x80000000 0x1 0x0>;
@@ -50,6 +58,12 @@ &arm_wdt {
 	status = "okay";
 };
 
+&combophy {
+	clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>, <&pad_clk>;
+	clock-names = "apb", "ker", "pad";
+	status = "okay";
+};
+
 &ethernet2 {
 	pinctrl-names = "default", "sleep";
 	pinctrl-0 = <&eth2_rgmii_pins_a>;
-- 
2.34.1

Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5467 bytes --]

On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote:
> Document the bindings for STM32 COMBOPHY interface, used to support
> the PCIe and USB3 stm32mp25 drivers.
> Following entries can be used to tune caracterisation parameters
>  - st,output-micro-ohms and st,output-vswing-microvolt bindings entries
> to tune the impedance and voltage swing using discrete simulation results
>  - st,rx-equalizer register to set the internal rx equalizer filter value.
> 
> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
>  .../bindings/phy/st,stm32mp25-combophy.yaml   | 128 ++++++++++++++++++
>  1 file changed, 128 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
> new file mode 100644
> index 000000000000..8d4a40b94507
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
> @@ -0,0 +1,128 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY
> +
> +maintainers:
> +  - Christian Bruel <christian.bruel@foss.st.com>
> +
> +description:
> +  Single lane PHY shared (exclusive) between the USB3 and PCIe controllers.
> +  Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1.
> +
> +properties:
> +  compatible:
> +    const: st,stm32mp25-combophy
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#phy-cells":
> +    const: 1
> +
> +  clocks:
> +    minItems: 2
> +    items:
> +      - description: apb Bus clock mandatory to access registers.
> +      - description: ker Internal RCC reference clock for USB3 or PCIe
> +      - description: pad Optional on board clock input for PCIe only. Typically an
> +                     external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference
> +                     clock input instead of the ker
> +
> +  clock-names:
> +    minItems: 2
> +    items:
> +      - const: apb
> +      - const: ker
> +      - const: pad
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: phy
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  wakeup-source: true
> +
> +  interrupts:
> +    maxItems: 1
> +    description: interrupt used for wakeup
> +
> +  access-controllers:
> +    minItems: 1
> +    maxItems: 2

Can you please describe the items here?

> +  st,syscfg:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the SYSCON entry required for configuring PCIe
> +      or USB3.

Why is a phandle required for this lookup, rather than doing it by
compatible?

> +
> +  st,ssc-on:
> +    type: boolean

flag, not boolean, for presence based stuff. And in the driver,
s/of_property_read_bool/of_property_present/.

> +    description:
> +      A boolean property whose presence indicates that the SSC for common clock
> +      needs to be set.

And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a
"linuxism", what does this actually do in the hardware block?

> +
> +  st,rx-equalizer:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 7
> +    default: 2
> +    description:
> +      A 3 bit value to tune the RX fixed equalizer setting for optimal eye compliance
> +
> +  st,output-micro-ohms:
> +    minimum: 3999000
> +    maximum: 6090000
> +    default: 4968000
> +    description:
> +      A value property to tune the Single Ended Output Impedance, simulations results
> +      at 25C for a VDDP=0.8V. The hardware accepts discrete values in this range.
> +
> +  st,output-vswing-microvolt:
> +    minimum: 442000
> +    maximum: 803000
> +    default: 803000
> +    description:
> +      A value property in microvolt to tune the Single Ended Output Voltage Swing to change the
> +      Vlo, Vhi for a VDDP = 0.8V. The hardware accepts discrete values in this range.
> +
> +required:
> +  - "#phy-cells"
> +  - compatible
> +  - clocks
> +  - clock-names
> +  - reg
> +  - resets
> +  - reset-names
> +  - st,syscfg

The order here should reflect the ordering in a node, so compatible and
reg first, rather than sorted alphanumerically. 

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
> +
> +    combophy: phy@480c0000 {

You can drop the label here, it ain't used by anything.

Cheers,
Conor.

> +        compatible = "st,stm32mp25-combophy";
> +        reg = <0x480c0000 0x1000>;
> +        #phy-cells = <1>;
> +        clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>;
> +        clock-names = "apb", "ker";
> +        resets = <&rcc USB3PCIEPHY_R>;
> +        reset-names = "phy";
> +        st,syscfg = <&syscfg>;
> +        access-controllers = <&rifsc 67>;
> +        power-domains = <&CLUSTER_PD>;
> +        wakeup-source;
> +        interrupts-extended = <&exti1 45 IRQ_TYPE_EDGE_FALLING>;
> +    };
> +...
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Christian Bruel]

On 8/28/24 18:11, Conor Dooley wrote:
> On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote:
>> Document the bindings for STM32 COMBOPHY interface, used to support
>> the PCIe and USB3 stm32mp25 drivers.
>> Following entries can be used to tune caracterisation parameters
>>   - st,output-micro-ohms and st,output-vswing-microvolt bindings entries
>> to tune the impedance and voltage swing using discrete simulation results
>>   - st,rx-equalizer register to set the internal rx equalizer filter value.
>>
>> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
>> ---
>>   .../bindings/phy/st,stm32mp25-combophy.yaml   | 128 ++++++++++++++++++
>>   1 file changed, 128 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
>> new file mode 100644
>> index 000000000000..8d4a40b94507
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
>> @@ -0,0 +1,128 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY
>> +
>> +maintainers:
>> +  - Christian Bruel <christian.bruel@foss.st.com>
>> +
>> +description:
>> +  Single lane PHY shared (exclusive) between the USB3 and PCIe controllers.
>> +  Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1.
>> +
>> +properties:
>> +  compatible:
>> +    const: st,stm32mp25-combophy
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#phy-cells":
>> +    const: 1
>> +
>> +  clocks:
>> +    minItems: 2
>> +    items:
>> +      - description: apb Bus clock mandatory to access registers.
>> +      - description: ker Internal RCC reference clock for USB3 or PCIe
>> +      - description: pad Optional on board clock input for PCIe only. Typically an
>> +                     external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference
>> +                     clock input instead of the ker
>> +
>> +  clock-names:
>> +    minItems: 2
>> +    items:
>> +      - const: apb
>> +      - const: ker
>> +      - const: pad
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  reset-names:
>> +    const: phy
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  wakeup-source: true
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: interrupt used for wakeup
>> +
>> +  access-controllers:
>> +    minItems: 1
>> +    maxItems: 2
> Can you please describe the items here?

I can specialize the description: "Phandle to the rifsc firewall device to check access right."

otherwise described in access-controllers/access-controllers.yaml, see also bindings/bus/st,stm32mp25-rifsc.yaml

>
>> +  st,syscfg:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description: Phandle to the SYSCON entry required for configuring PCIe
>> +      or USB3.
> Why is a phandle required for this lookup, rather than doing it by
> compatible?

the phandle is used to select the sysconf SoC configuration register 
depending on the PCIe/USB3 mode (selected by xlate function), so it's 
not like a lookup here. This sysconf register is also used for other 
settings such as the PLL, Reference clock selection, ...

>
>> +
>> +  st,ssc-on:
>> +    type: boolean
> flag, not boolean, for presence based stuff. And in the driver,
> s/of_property_read_bool/of_property_present/.

ok

>
>> +    description:
>> +      A boolean property whose presence indicates that the SSC for common clock
>> +      needs to be set.
> And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a
> "linuxism", what does this actually do in the hardware block?

SSC for Spread Spectrum Clocking. It is an hardware setting for the 100Mhz PCIe reference common clock,
I will rephrase the description

>
>> +
>> +  st,rx-equalizer:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 7
>> +    default: 2
>> +    description:
>> +      A 3 bit value to tune the RX fixed equalizer setting for optimal eye compliance
>> +
>> +  st,output-micro-ohms:
>> +    minimum: 3999000
>> +    maximum: 6090000
>> +    default: 4968000
>> +    description:
>> +      A value property to tune the Single Ended Output Impedance, simulations results
>> +      at 25C for a VDDP=0.8V. The hardware accepts discrete values in this range.
>> +
>> +  st,output-vswing-microvolt:
>> +    minimum: 442000
>> +    maximum: 803000
>> +    default: 803000
>> +    description:
>> +      A value property in microvolt to tune the Single Ended Output Voltage Swing to change the
>> +      Vlo, Vhi for a VDDP = 0.8V. The hardware accepts discrete values in this range.
>> +
>> +required:
>> +  - "#phy-cells"
>> +  - compatible
>> +  - clocks
>> +  - clock-names
>> +  - reg
>> +  - resets
>> +  - reset-names
>> +  - st,syscfg
> The order here should reflect the ordering in a node, so compatible and
> reg first, rather than sorted alphanumerically.

ok

>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/reset/st,stm32mp25-rcc.h>
>> +
>> +    combophy: phy@480c0000 {
> You can drop the label here, it ain't used by anything.

ok

thanks,
Christian

>
> Cheers,
> Conor.
>
>> +        compatible = "st,stm32mp25-combophy";
>> +        reg = <0x480c0000 0x1000>;
>> +        #phy-cells = <1>;
>> +        clocks = <&rcc CK_BUS_USB3PCIEPHY>, <&rcc CK_KER_USB3PCIEPHY>;
>> +        clock-names = "apb", "ker";
>> +        resets = <&rcc USB3PCIEPHY_R>;
>> +        reset-names = "phy";
>> +        st,syscfg = <&syscfg>;
>> +        access-controllers = <&rifsc 67>;
>> +        power-domains = <&CLUSTER_PD>;
>> +        wakeup-source;
>> +        interrupts-extended = <&exti1 45 IRQ_TYPE_EDGE_FALLING>;
>> +    };
>> +...
>> -- 
>> 2.34.1
>>

Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5371 bytes --]

On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote:
> On 8/28/24 18:11, Conor Dooley wrote:
> > On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote:
> > > Document the bindings for STM32 COMBOPHY interface, used to support
> > > the PCIe and USB3 stm32mp25 drivers.
> > > Following entries can be used to tune caracterisation parameters
> > >   - st,output-micro-ohms and st,output-vswing-microvolt bindings entries
> > > to tune the impedance and voltage swing using discrete simulation results
> > >   - st,rx-equalizer register to set the internal rx equalizer filter value.
> > > 
> > > Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> > > ---
> > >   .../bindings/phy/st,stm32mp25-combophy.yaml   | 128 ++++++++++++++++++
> > >   1 file changed, 128 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
> > > new file mode 100644
> > > index 000000000000..8d4a40b94507
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
> > > @@ -0,0 +1,128 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY
> > > +
> > > +maintainers:
> > > +  - Christian Bruel <christian.bruel@foss.st.com>
> > > +
> > > +description:
> > > +  Single lane PHY shared (exclusive) between the USB3 and PCIe controllers.
> > > +  Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: st,stm32mp25-combophy
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  "#phy-cells":
> > > +    const: 1
> > > +
> > > +  clocks:
> > > +    minItems: 2
> > > +    items:
> > > +      - description: apb Bus clock mandatory to access registers.
> > > +      - description: ker Internal RCC reference clock for USB3 or PCIe
> > > +      - description: pad Optional on board clock input for PCIe only. Typically an
> > > +                     external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference
> > > +                     clock input instead of the ker
> > > +
> > > +  clock-names:
> > > +    minItems: 2
> > > +    items:
> > > +      - const: apb
> > > +      - const: ker
> > > +      - const: pad
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  reset-names:
> > > +    const: phy
> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +
> > > +  wakeup-source: true
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +    description: interrupt used for wakeup
> > > +
> > > +  access-controllers:
> > > +    minItems: 1
> > > +    maxItems: 2
> > Can you please describe the items here?
> 
> I can specialize the description: "Phandle to the rifsc firewall device to check access right."

Right, but there are potentially two access controllers here. You need
to describe which is which, so that people can hook them up in the
correct order. In what case are there two? Your dts patch only has one.

> otherwise described in access-controllers/access-controllers.yaml, see also bindings/bus/st,stm32mp25-rifsc.yaml
> 
> > 
> > > +  st,syscfg:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description: Phandle to the SYSCON entry required for configuring PCIe
> > > +      or USB3.
> > Why is a phandle required for this lookup, rather than doing it by
> > compatible?
> 
> the phandle is used to select the sysconf SoC configuration register
> depending on the PCIe/USB3 mode (selected by xlate function), so it's not
> like a lookup here.

If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not
know what is. An example justification for it would be that there are
multiple combophys on the same soc, each using a different sysconf
region. Your dts suggests that is not the case though, since you have
st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;.

> This sysconf register is also used for other settings
> such as the PLL, Reference clock selection, ...
> 
> > 
> > > +
> > > +  st,ssc-on:
> > > +    type: boolean
> > flag, not boolean, for presence based stuff. And in the driver,
> > s/of_property_read_bool/of_property_present/.
> 
> ok
> 
> > 
> > > +    description:
> > > +      A boolean property whose presence indicates that the SSC for common clock
> > > +      needs to be set.
> > And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a
> > "linuxism", what does this actually do in the hardware block?
> 
> SSC for Spread Spectrum Clocking. It is an hardware setting for the 100Mhz PCIe reference common clock,

Ah, so not really a "common clock" linuxism at all.

> I will rephrase the description

How is someone supposed to decide between on and off? Is it always on
for PCIe, or only on in some configurations? Or maybe only (or always?) on
if the pad clock is provided?

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/5] phy: stm32: Add support for STM32MP25 COMBOPHY.

[Vinod Koul]

On 28-08-24, 16:34, Christian Bruel wrote:
> Addition of the COMBOPHY driver found on STM32MP25 platforms
> 
> This single lane PHY is shared (exclusive) between the USB3 and PCIE
> controllers.
> Supports 5Gbit/s for PCIE gen2 or 2.5Gbit/s for PCIE gen1.
> 
> Supports wakeup-source capability to wakeup system using remote-wakeup
> capable USB device
> 
> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
>  drivers/phy/st/Kconfig              |  11 +
>  drivers/phy/st/Makefile             |   1 +
>  drivers/phy/st/phy-stm32-combophy.c | 607 ++++++++++++++++++++++++++++
>  3 files changed, 619 insertions(+)
>  create mode 100644 drivers/phy/st/phy-stm32-combophy.c
> 
> diff --git a/drivers/phy/st/Kconfig b/drivers/phy/st/Kconfig
> index 3fc3d0781fb8..304614b6dabf 100644
> --- a/drivers/phy/st/Kconfig
> +++ b/drivers/phy/st/Kconfig
> @@ -33,6 +33,17 @@ config PHY_STIH407_USB
>  	  Enable this support to enable the picoPHY device used by USB2
>  	  and USB3 controllers on STMicroelectronics STiH407 SoC families.
>  
> +config PHY_STM32_COMBOPHY
> +	tristate "STMicroelectronics COMBOPHY driver for STM32MP25"
> +	depends on ARCH_STM32 || COMPILE_TEST
> +	select GENERIC_PHY
> +	help
> +	  Enable this to support the COMBOPHY device used by USB3 or PCIe
> +	  controllers on STMicroelectronics STM32MP25 SoC.
> +	  This driver controls the COMBOPHY block to generate the PCIe 100Mhz
> +	  reference clock from either the external clock generator or HSE
> +	  internal SoC clock source.
> +
>  config PHY_STM32_USBPHYC
>  	tristate "STMicroelectronics STM32 USB HS PHY Controller driver"
>  	depends on ARCH_STM32 || COMPILE_TEST
> diff --git a/drivers/phy/st/Makefile b/drivers/phy/st/Makefile
> index c862dd937b64..cb80e954ea9f 100644
> --- a/drivers/phy/st/Makefile
> +++ b/drivers/phy/st/Makefile
> @@ -3,4 +3,5 @@ obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
>  obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
>  obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
>  obj-$(CONFIG_PHY_STIH407_USB)		+= phy-stih407-usb.o
> +obj-$(CONFIG_PHY_STM32_COMBOPHY)	+= phy-stm32-combophy.o
>  obj-$(CONFIG_PHY_STM32_USBPHYC) 	+= phy-stm32-usbphyc.o
> diff --git a/drivers/phy/st/phy-stm32-combophy.c b/drivers/phy/st/phy-stm32-combophy.c
> new file mode 100644
> index 000000000000..7cd4193b0277
> --- /dev/null
> +++ b/drivers/phy/st/phy-stm32-combophy.c
> @@ -0,0 +1,607 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * STMicroelectronics COMBOPHY STM32MP25 Controller driver.
> + *
> + * Copyright (C) 2024 STMicroelectronics
> + * Author: Christian Bruel <christian.bruel@foss.st.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <dt-bindings/phy/phy.h>
> +
> +#define SYSCFG_COMBOPHY_CR1 0x4C00
> +#define SYSCFG_COMBOPHY_CR2 0x4C04
> +#define SYSCFG_COMBOPHY_CR4 0x4C0C
> +#define SYSCFG_COMBOPHY_CR5 0x4C10
> +#define SYSCFG_COMBOPHY_SR  0x4C14

Lowercase hex values please

> +#define SYSCFG_PCIEPRGCR    0x6080
> +
> +/* SYSCFG PCIEPRGCR */
> +#define STM32MP25_PCIEPRGCR_EN	  BIT(0)
> +#define STM32MP25_PCIEPRG_IMPCTRL_OHM     GENMASK(3, 1)
> +#define STM32MP25_PCIEPRG_IMPCTRL_VSWING  GENMASK(5, 4)
> +
> +/* SYSCFG SYSCFG_COMBOPHY_SR */
> +#define STM32MP25_PIPE0_PHYSTATUS BIT(1)
> +
> +/* SYSCFG CR1 */
> +#define SYSCFG_COMBOPHY_CR1_REFUSEPAD BIT(0)
> +#define SYSCFG_COMBOPHY_CR1_MPLLMULT GENMASK(7, 1)
> +#define SYSCFG_COMBOPHY_CR1_REFCLKSEL GENMASK(16, 8)
> +#define SYSCFG_COMBOPHY_CR1_REFCLKDIV2 BIT(17)
> +#define SYSCFG_COMBOPHY_CR1_REFSSPEN BIT(18)
> +#define SYSCFG_COMBOPHY_CR1_SSCEN BIT(19)
> +
> +/* SYSCFG CR4 */
> +#define SYSCFG_COMBOPHY_CR4_RX0_EQ GENMASK(2, 0)
> +
> +#define MPLLMULT_19_2 (0x02u << 1)
> +#define MPLLMULT_20   (0x7Du << 1)
> +#define MPLLMULT_24   (0x68u << 1)
> +#define MPLLMULT_25   (0x64u << 1)
> +#define MPLLMULT_26   (0x60u << 1)
> +#define MPLLMULT_38_4 (0x41u << 1)
> +#define MPLLMULT_48   (0x6Cu << 1)
> +#define MPLLMULT_50   (0x32u << 1)
> +#define MPLLMULT_52   (0x30u << 1)
> +#define MPLLMULT_100  (0x19u << 1)
> +
> +#define REFCLKSEL_0   0
> +#define REFCLKSEL_1   (0x108u << 8)
> +
> +#define REFCLDIV_0    0
> +
> +/* SYSCFG CR2 */
> +#define SYSCFG_COMBOPHY_CR2_MODESEL GENMASK(1, 0)
> +#define SYSCFG_COMBOPHY_CR2_ISO_DIS BIT(15)
> +
> +#define COMBOPHY_MODESEL_PCIE 0
> +#define COMBOPHY_MODESEL_USB  3
> +
> +/* SYSCFG CR5 */
> +#define SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS BIT(12)
> +
> +#define COMBOPHY_SUP_ANA_MPLL_LOOP_CTL 0xC0
> +#define COMBOPHY_PROP_CNTRL GENMASK(7, 4)
> +
> +struct stm32_combophy {
> +	struct phy *phy;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	void __iomem *base;
> +	struct reset_control *phy_reset;
> +	struct clk *phy_clk;
> +	struct clk *pad_clk;
> +	struct clk *ker_clk;
> +	unsigned int type;
> +	bool is_init;
> +	int irq_wakeup;
> +};
> +
> +struct clk_impedance  {
> +	u32 microohm;
> +	u32 vswing[4];
> +};
> +
> +/*
> + * lookup table to hold the settings needed for a ref clock frequency
> + * impedance, the offset is used to set the IMP_CTL and DE_EMP bit of the
> + * PRG_IMP_CTRL register
> + */
> +static const struct clk_impedance imp_lookup[] = {
> +	{ 6090000, { 442000, 564000, 684000, 802000 } },
> +	{ 5662000, { 528000, 621000, 712000, 803000 } },
> +	{ 5292000, { 491000, 596000, 700000, 802000 } },
> +	{ 4968000, { 558000, 640000, 722000, 803000 } },
> +	{ 4684000, { 468000, 581000, 692000, 802000 } },
> +	{ 4429000, { 554000, 613000, 717000, 803000 } },
> +	{ 4204000, { 511000, 609000, 706000, 802000 } },
> +	{ 3999000, { 571000, 648000, 726000, 803000 } }
> +};
> +
> +static int stm32_impedance_tune(struct stm32_combophy *combophy)
> +{
> +	u8 imp_size = ARRAY_SIZE(imp_lookup);
> +	u8 vswing_size = ARRAY_SIZE(imp_lookup[0].vswing);
> +	u8 imp_of, vswing_of;
> +	u32 max_imp = imp_lookup[0].microohm;
> +	u32 min_imp = imp_lookup[imp_size - 1].microohm;

table is ordered, pls mention that in comments somewhere

> +	u32 max_vswing = imp_lookup[imp_size - 1].vswing[vswing_size - 1];
> +	u32 min_vswing = imp_lookup[0].vswing[0];
> +	u32 val;
> +
> +	if (!of_property_read_u32(combophy->dev->of_node, "st,output-micro-ohms", &val)) {
> +		if (val < min_imp || val > max_imp) {
> +			dev_err(combophy->dev, "Invalid value %u for output ohm\n", val);
> +			return -EINVAL;
> +		}
> +
> +		for (imp_of = 0 ; imp_of < ARRAY_SIZE(imp_lookup); imp_of++)
> +			if (imp_lookup[imp_of].microohm <= val)
> +				break;
> +
> +		dev_dbg(combophy->dev, "Set %u micro-ohms output impedance\n",
> +			imp_lookup[imp_of].microohm);
> +
> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
> +				   STM32MP25_PCIEPRG_IMPCTRL_OHM,
> +				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_OHM, imp_of));
> +	} else {
> +		regmap_read(combophy->regmap, SYSCFG_PCIEPRGCR, &val);
> +		imp_of = FIELD_GET(STM32MP25_PCIEPRG_IMPCTRL_OHM, val);
> +	}
> +
> +	if (!of_property_read_u32(combophy->dev->of_node, "st,output-vswing-microvolt", &val)) {
> +		if (val < min_vswing || val > max_vswing) {
> +			dev_err(combophy->dev, "Invalid value %u for output vswing\n", val);
> +			return -EINVAL;
> +		}
> +
> +		for (vswing_of = 0 ; vswing_of < ARRAY_SIZE(imp_lookup[imp_of].vswing); vswing_of++)
> +			if (imp_lookup[imp_of].vswing[vswing_of] >= val)
> +				break;
> +
> +		dev_dbg(combophy->dev, "Set %u microvolt swing\n",
> +			 imp_lookup[imp_of].vswing[vswing_of]);
> +
> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
> +				   STM32MP25_PCIEPRG_IMPCTRL_VSWING,
> +				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_VSWING, vswing_of));
> +	}
> +
> +	return 0;
> +}
> +
> +static int stm32_combophy_pll_init(struct stm32_combophy *combophy)
> +{
> +	int ret;
> +	u32 refclksel, pllmult, propcntrl, val;
> +	u32 clk_rate;
> +
> +	if (combophy->pad_clk)
> +		clk_rate = clk_get_rate(combophy->pad_clk);
> +	else
> +		clk_rate = clk_get_rate(combophy->ker_clk);
> +
> +	reset_control_assert(combophy->phy_reset);
> +
> +	dev_dbg(combophy->dev, "%s pll init rate %d\n",
> +		combophy->pad_clk ? "External" : "Ker", clk_rate);
> +
> +	/*
> +	 * vddcombophy is interconnected with vddcore. Isolation bit should be unset
> +	 * before using the ComboPHY.
> +	 */
> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
> +			   SYSCFG_COMBOPHY_CR2_ISO_DIS, SYSCFG_COMBOPHY_CR2_ISO_DIS);
> +
> +	if (combophy->type != PHY_TYPE_PCIE)
> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
> +				   SYSCFG_COMBOPHY_CR1_REFSSPEN, SYSCFG_COMBOPHY_CR1_REFSSPEN);
> +
> +	if (combophy->type == PHY_TYPE_PCIE && !combophy->pad_clk)
> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
> +				   STM32MP25_PCIEPRGCR_EN, STM32MP25_PCIEPRGCR_EN);
> +
> +	if (of_property_read_bool(combophy->dev->of_node, "st,ssc-on")) {
> +		dev_dbg(combophy->dev, "Enabling clock with SSC\n");
> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
> +				   SYSCFG_COMBOPHY_CR1_SSCEN, SYSCFG_COMBOPHY_CR1_SSCEN);
> +	}
> +
> +	if (!of_property_read_u32(combophy->dev->of_node, "st,rx-equalizer", &val)) {
> +		dev_dbg(combophy->dev, "Set RX equalizer %u\n", val);
> +		if (val > SYSCFG_COMBOPHY_CR4_RX0_EQ) {
> +			dev_err(combophy->dev, "Invalid value %u for rx0 equalizer\n", val);
> +			return -EINVAL;
> +		}
> +
> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR4,
> +			   SYSCFG_COMBOPHY_CR4_RX0_EQ, val);
> +	}
> +
> +	if (combophy->type == PHY_TYPE_PCIE) {
> +		ret = stm32_impedance_tune(combophy);
> +		if (ret) {
> +			reset_control_deassert(combophy->phy_reset);
> +			goto out;
> +		}
> +
> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
> +				   SYSCFG_COMBOPHY_CR1_REFUSEPAD,
> +				   combophy->pad_clk ? SYSCFG_COMBOPHY_CR1_REFUSEPAD : 0);
> +	}
> +
> +	switch (clk_rate) {
> +	case 100000000:
> +		pllmult = MPLLMULT_100;
> +		refclksel = REFCLKSEL_0;
> +		propcntrl = 0x8u << 4;
> +		break;
> +	case 19200000:
> +		pllmult = MPLLMULT_19_2;
> +		refclksel = REFCLKSEL_1;
> +		propcntrl = 0x8u << 4;
> +		break;
> +	case 25000000:
> +		pllmult = MPLLMULT_25;
> +		refclksel = REFCLKSEL_0;
> +		propcntrl = 0xEu << 4;
> +		break;
> +	case 24000000:
> +		pllmult = MPLLMULT_24;
> +		refclksel = REFCLKSEL_1;
> +		propcntrl = 0xEu << 4;
> +		break;
> +	case 20000000:
> +		pllmult = MPLLMULT_20;
> +		refclksel = REFCLKSEL_0;
> +		propcntrl = 0xEu << 4;
> +		break;
> +	default:
> +		dev_err(combophy->dev, "Invalid rate 0x%x\n", clk_rate);
> +		reset_control_deassert(combophy->phy_reset);
> +		ret = -EINVAL;
> +		goto out;
> +	};
> +
> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
> +			   SYSCFG_COMBOPHY_CR1_REFCLKDIV2, REFCLDIV_0);
> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
> +			   SYSCFG_COMBOPHY_CR1_REFCLKSEL, refclksel);
> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
> +			   SYSCFG_COMBOPHY_CR1_MPLLMULT, pllmult);
> +
> +	/*
> +	 * Force elasticity buffer to be tuned for the reference clock as
> +	 * the separated clock model is not supported
> +	 */
> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR5,
> +			   SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS, SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS);
> +
> +	reset_control_deassert(combophy->phy_reset);
> +
> +	ret = regmap_read_poll_timeout(combophy->regmap, SYSCFG_COMBOPHY_SR, val,
> +				       !(val & STM32MP25_PIPE0_PHYSTATUS),
> +				       10, 1000);
> +	if (ret) {
> +		dev_err(combophy->dev, "timeout, cannot lock PLL\n");
> +		goto out;
> +	}
> +
> +	if (combophy->type == PHY_TYPE_PCIE) {
> +		val = readl_relaxed(combophy->base + COMBOPHY_SUP_ANA_MPLL_LOOP_CTL);
> +		val &= ~COMBOPHY_PROP_CNTRL;
> +		val |= propcntrl;
> +		writel_relaxed(val, combophy->base + COMBOPHY_SUP_ANA_MPLL_LOOP_CTL);
> +	}
> +
> +	return 0;
> +
> +out:
> +	if (combophy->type == PHY_TYPE_PCIE && !combophy->pad_clk)
> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
> +				   STM32MP25_PCIEPRGCR_EN, 0);
> +
> +	if (combophy->type != PHY_TYPE_PCIE)
> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
> +				   SYSCFG_COMBOPHY_CR1_REFSSPEN, 0);
> +
> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
> +			   SYSCFG_COMBOPHY_CR2_ISO_DIS, 0);
> +
> +	return ret;
> +}
> +
> +static struct phy *stm32_combophy_xlate(struct device *dev,
> +					const struct of_phandle_args *args)
> +{
> +	struct stm32_combophy *combophy = dev_get_drvdata(dev);
> +	unsigned int type;
> +
> +	if (args->args_count != 1) {
> +		dev_err(dev, "invalid number of cells in 'phy' property\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	type = args->args[0];
> +	if (type != PHY_TYPE_USB3 && type != PHY_TYPE_PCIE) {
> +		dev_err(dev, "unsupported device type: %d\n", type);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (combophy->pad_clk && type != PHY_TYPE_PCIE) {
> +		dev_err(dev, "Invalid use of clk_pad for USB3 mode\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	combophy->type = type;
> +
> +	return combophy->phy;
> +}
> +
> +static int stm32_combophy_set_mode(struct stm32_combophy *combophy)
> +{
> +	int type = combophy->type;
> +	u32 val;
> +
> +	switch (type) {
> +	case PHY_TYPE_PCIE:
> +		dev_dbg(combophy->dev, "setting PCIe ComboPHY\n");
> +		val = COMBOPHY_MODESEL_PCIE;
> +		break;
> +	case PHY_TYPE_USB3:
> +		dev_dbg(combophy->dev, "setting USB3 ComboPHY\n");
> +		val = COMBOPHY_MODESEL_USB;
> +		break;
> +	default:
> +		dev_err(combophy->dev, "Invalid PHY mode %d\n", type);
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
> +				  SYSCFG_COMBOPHY_CR2_MODESEL, val);
> +}
> +
> +static int stm32_combophy_enable_clocks(struct stm32_combophy *combophy)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(combophy->phy_clk);
> +	if (ret) {
> +		dev_err(combophy->dev, "Core clock enable failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(combophy->ker_clk);
> +	if (ret) {
> +		dev_err(combophy->dev, "ker_usb3pcie clock enable failed %d\n", ret);
> +		clk_disable_unprepare(combophy->phy_clk);
> +		return ret;
> +	}
> +
> +	if (combophy->pad_clk) {
> +		ret = clk_prepare_enable(combophy->pad_clk);
> +		if (ret) {
> +			dev_err(combophy->dev, "External clock enable failed %d\n", ret);
> +			clk_disable_unprepare(combophy->ker_clk);
> +			clk_disable_unprepare(combophy->phy_clk);
> +			return ret;
> +		}
> +	}

Can you use bulk_prepare for this?

-- 
~Vinod

Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Christian Bruel]

On 8/29/24 18:44, Conor Dooley wrote:
> On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote:
>> On 8/28/24 18:11, Conor Dooley wrote:
>>> On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote:
>>>> Document the bindings for STM32 COMBOPHY interface, used to support
>>>> the PCIe and USB3 stm32mp25 drivers.
>>>> Following entries can be used to tune caracterisation parameters
>>>>    - st,output-micro-ohms and st,output-vswing-microvolt bindings entries
>>>> to tune the impedance and voltage swing using discrete simulation results
>>>>    - st,rx-equalizer register to set the internal rx equalizer filter value.
>>>>
>>>> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
>>>> ---
>>>>    .../bindings/phy/st,stm32mp25-combophy.yaml   | 128 ++++++++++++++++++
>>>>    1 file changed, 128 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
>>>> new file mode 100644
>>>> index 000000000000..8d4a40b94507
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/phy/st,stm32mp25-combophy.yaml
>>>> @@ -0,0 +1,128 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/phy/st,stm32mp25-combophy.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: STMicroelectronics STM32MP25 USB3/PCIe COMBOPHY
>>>> +
>>>> +maintainers:
>>>> +  - Christian Bruel <christian.bruel@foss.st.com>
>>>> +
>>>> +description:
>>>> +  Single lane PHY shared (exclusive) between the USB3 and PCIe controllers.
>>>> +  Supports 5Gbit/s for USB3 and PCIe gen2 or 2.5Gbit/s for PCIe gen1.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: st,stm32mp25-combophy
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  "#phy-cells":
>>>> +    const: 1
>>>> +
>>>> +  clocks:
>>>> +    minItems: 2
>>>> +    items:
>>>> +      - description: apb Bus clock mandatory to access registers.
>>>> +      - description: ker Internal RCC reference clock for USB3 or PCIe
>>>> +      - description: pad Optional on board clock input for PCIe only. Typically an
>>>> +                     external 100Mhz oscillator wired on dedicated CLKIN pad. Used as reference
>>>> +                     clock input instead of the ker
>>>> +
>>>> +  clock-names:
>>>> +    minItems: 2
>>>> +    items:
>>>> +      - const: apb
>>>> +      - const: ker
>>>> +      - const: pad
>>>> +
>>>> +  resets:
>>>> +    maxItems: 1
>>>> +
>>>> +  reset-names:
>>>> +    const: phy
>>>> +
>>>> +  power-domains:
>>>> +    maxItems: 1
>>>> +
>>>> +  wakeup-source: true
>>>> +
>>>> +  interrupts:
>>>> +    maxItems: 1
>>>> +    description: interrupt used for wakeup
>>>> +
>>>> +  access-controllers:
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>> Can you please describe the items here?
>> I can specialize the description: "Phandle to the rifsc firewall device to check access right."
> Right, but there are potentially two access controllers here. You need
> to describe which is which, so that people can hook them up in the
> correct order. In what case are there two? Your dts patch only has one.

(sorry, resent in plain test)

yes, we don't have more than 1 controller in the current setup,

I'll use maxItems: 1

>> otherwise described in access-controllers/access-controllers.yaml, see also bindings/bus/st,stm32mp25-rifsc.yaml
>>
>>>> +  st,syscfg:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +    description: Phandle to the SYSCON entry required for configuring PCIe
>>>> +      or USB3.
>>> Why is a phandle required for this lookup, rather than doing it by
>>> compatible?
>> the phandle is used to select the sysconf SoC configuration register
>> depending on the PCIe/USB3 mode (selected by xlate function), so it's not
>> like a lookup here.
> If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not
> know what is. An example justification for it would be that there are
> multiple combophys on the same soc, each using a different sysconf
> region. Your dts suggests that is not the case though, since you have
> st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;.

I didn't get your suggestion earlier to use "syscon_regmap_lookup_by_compatible()".

We have several other syscon in the other. That's why we choose a direct syscfg phandle

>
>> This sysconf register is also used for other settings
>> such as the PLL, Reference clock selection, ...
>>
>>>> +
>>>> +  st,ssc-on:
>>>> +    type: boolean
>>> flag, not boolean, for presence based stuff. And in the driver,
>>> s/of_property_read_bool/of_property_present/.
>> ok
>>
>>>> +    description:
>>>> +      A boolean property whose presence indicates that the SSC for common clock
>>>> +      needs to be set.
>>> And what, may I ask, does "SSC" mean? "Common clock" is also a bit of a
>>> "linuxism", what does this actually do in the hardware block?
>> SSC for Spread Spectrum Clocking. It is an hardware setting for the 100Mhz PCIe reference common clock,
> Ah, so not really a "common clock" linuxism at all.
>
>> I will rephrase the description
> How is someone supposed to decide between on and off? Is it always on
> for PCIe, or only on in some configurations? Or maybe only (or always?) on
> if the pad clock is provided?

SSC is off by default and available for the PCIe pad clock. it must be defined for USB3

thanks

Christian

>
> Cheers,
> Conor.

Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2694 bytes --]

On Fri, Aug 30, 2024 at 02:53:15PM +0200, Christian Bruel wrote:
> 
> On 8/29/24 18:44, Conor Dooley wrote:
> > On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote:
> > > On 8/28/24 18:11, Conor Dooley wrote:
> > > > On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote:

> > > > > +  st,syscfg:
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > +    description: Phandle to the SYSCON entry required for configuring PCIe
> > > > > +      or USB3.
> > > > Why is a phandle required for this lookup, rather than doing it by
> > > > compatible?
> > > the phandle is used to select the sysconf SoC configuration register
> > > depending on the PCIe/USB3 mode (selected by xlate function), so it's not
> > > like a lookup here.
> > If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not
> > know what is. An example justification for it would be that there are
> > multiple combophys on the same soc, each using a different sysconf
> > region. Your dts suggests that is not the case though, since you have
> > st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;.
> 
> I didn't get your suggestion earlier to use "syscon_regmap_lookup_by_compatible()".
> 
> We have several other syscon in the other. That's why we choose a direct syscfg phandle

In the other what? SoCs?

Way I see it, if you're going to support different socs in the same
driver, it's almost a certainty that the offsets within a syscon that
particular features lie at are going to change between socs, so even if
you have a phandle you're going to need to have the offsets in your
match data. And if you're going to have offsets in match data, you may
as well have the compatibles for the syscon in match data too.
If the layout of the syscon hasn't changed between devices, then you
should have a fallback compatible for the syscon too, making
syscon_regmap_lookup_by_compatible() function without changes to the
driver.

If you do have multiple syscons, but they do different things, they
should have different compatibles, so having multiple syscons doesn't
justify using a property for this either in and of itself. If you have
multiple syscons with the same layout (and therefore the same
compatible) then a phandle makes sense, but if that's the case then you
almost certainly have multiple combophys too! Otherwise, if you have one
syscon, but the controls for more than one combophy are in it, then
having a phandle _with an offset_ makes sense.

If you know there are other SoCs with more than one combo phy, do they
use different syscons, or is the same syscon used for more than one
combophy?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 1/5] dt-bindings: phy: Add STM32MP25 COMBOPHY bindings

[Christian Bruel]

On 8/30/24 16:55, Conor Dooley wrote:
> On Fri, Aug 30, 2024 at 02:53:15PM +0200, Christian Bruel wrote:
>> On 8/29/24 18:44, Conor Dooley wrote:
>>> On Thu, Aug 29, 2024 at 01:06:53PM +0200, Christian Bruel wrote:
>>>> On 8/28/24 18:11, Conor Dooley wrote:
>>>>> On Wed, Aug 28, 2024 at 04:34:48PM +0200, Christian Bruel wrote:
>>>>>> +  st,syscfg:
>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>>> +    description: Phandle to the SYSCON entry required for configuring PCIe
>>>>>> +      or USB3.
>>>>> Why is a phandle required for this lookup, rather than doing it by
>>>>> compatible?
>>>> the phandle is used to select the sysconf SoC configuration register
>>>> depending on the PCIe/USB3 mode (selected by xlate function), so it's not
>>>> like a lookup here.
>>> If "syscon_regmap_lookup_by_phandle()" is not a lookup, then I do not
>>> know what is. An example justification for it would be that there are
>>> multiple combophys on the same soc, each using a different sysconf
>>> region. Your dts suggests that is not the case though, since you have
>>> st,syscfg = <&syscfg>; in it, rather than st,syscfg = <&syscfg0>;.
>> I didn't get your suggestion earlier to use "syscon_regmap_lookup_by_compatible()".
>>
>> We have several other syscon in the other. That's why we choose a direct syscfg phandle
> In the other what? SoCs?
>
> Way I see it, if you're going to support different socs in the same
> driver, it's almost a certainty that the offsets within a syscon that
> particular features lie at are going to change between socs, so even if
> you have a phandle you're going to need to have the offsets in your
> match data. And if you're going to have offsets in match data, you may
> as well have the compatibles for the syscon in match data too.
> If the layout of the syscon hasn't changed between devices, then you
> should have a fallback compatible for the syscon too, making
> syscon_regmap_lookup_by_compatible() function without changes to the
> driver.
>
> If you do have multiple syscons, but they do different things, they
> should have different compatibles, so having multiple syscons doesn't
> justify using a property for this either in and of itself. If you have
> multiple syscons with the same layout (and therefore the same
> compatible) then a phandle makes sense, but if that's the case then you
> almost certainly have multiple combophys too! Otherwise, if you have one
> syscon, but the controls for more than one combophy are in it, then
> having a phandle _with an offset_ makes sense.
>
> If you know there are other SoCs with more than one combo phy, do they
> use different syscons, or is the same syscon used for more than one
> combophy?

we have other syscon for other subsystem in the same SoC, but I not the same layout

We indeed have a different compatible for the syscfg top (not the COMBOPHY registers), I can use
"syscon_regmap_lookup_by_compatible(st,stm32mp25-syscfg)" effectively

one justification I had in mind for using a phandle is that we can use an argument to the
COMBOPHY registers offset in the syscfg. Having this DT flexibility to adjust the offset
for new SoC revisions using the same driver looked like a nice to have.
For the time being the lookup_by_compatible pointing the syscfg syscon is OK

thanks for the clarification.

Christian

Re: [PATCH v4 2/5] phy: stm32: Add support for STM32MP25 COMBOPHY.

[Christian Bruel]

On 8/29/24 19:39, Vinod Koul wrote:
> On 28-08-24, 16:34, Christian Bruel wrote:
>> Addition of the COMBOPHY driver found on STM32MP25 platforms
>>
>> This single lane PHY is shared (exclusive) between the USB3 and PCIE
>> controllers.
>> Supports 5Gbit/s for PCIE gen2 or 2.5Gbit/s for PCIE gen1.
>>
>> Supports wakeup-source capability to wakeup system using remote-wakeup
>> capable USB device
>>
>> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
>> ---
>>   drivers/phy/st/Kconfig              |  11 +
>>   drivers/phy/st/Makefile             |   1 +
>>   drivers/phy/st/phy-stm32-combophy.c | 607 ++++++++++++++++++++++++++++
>>   3 files changed, 619 insertions(+)
>>   create mode 100644 drivers/phy/st/phy-stm32-combophy.c
>>
>> diff --git a/drivers/phy/st/Kconfig b/drivers/phy/st/Kconfig
>> index 3fc3d0781fb8..304614b6dabf 100644
>> --- a/drivers/phy/st/Kconfig
>> +++ b/drivers/phy/st/Kconfig
>> @@ -33,6 +33,17 @@ config PHY_STIH407_USB
>>   	  Enable this support to enable the picoPHY device used by USB2
>>   	  and USB3 controllers on STMicroelectronics STiH407 SoC families.
>>   
>> +config PHY_STM32_COMBOPHY
>> +	tristate "STMicroelectronics COMBOPHY driver for STM32MP25"
>> +	depends on ARCH_STM32 || COMPILE_TEST
>> +	select GENERIC_PHY
>> +	help
>> +	  Enable this to support the COMBOPHY device used by USB3 or PCIe
>> +	  controllers on STMicroelectronics STM32MP25 SoC.
>> +	  This driver controls the COMBOPHY block to generate the PCIe 100Mhz
>> +	  reference clock from either the external clock generator or HSE
>> +	  internal SoC clock source.
>> +
>>   config PHY_STM32_USBPHYC
>>   	tristate "STMicroelectronics STM32 USB HS PHY Controller driver"
>>   	depends on ARCH_STM32 || COMPILE_TEST
>> diff --git a/drivers/phy/st/Makefile b/drivers/phy/st/Makefile
>> index c862dd937b64..cb80e954ea9f 100644
>> --- a/drivers/phy/st/Makefile
>> +++ b/drivers/phy/st/Makefile
>> @@ -3,4 +3,5 @@ obj-$(CONFIG_PHY_MIPHY28LP) 		+= phy-miphy28lp.o
>>   obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY)	+= phy-spear1310-miphy.o
>>   obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY)	+= phy-spear1340-miphy.o
>>   obj-$(CONFIG_PHY_STIH407_USB)		+= phy-stih407-usb.o
>> +obj-$(CONFIG_PHY_STM32_COMBOPHY)	+= phy-stm32-combophy.o
>>   obj-$(CONFIG_PHY_STM32_USBPHYC) 	+= phy-stm32-usbphyc.o
>> diff --git a/drivers/phy/st/phy-stm32-combophy.c b/drivers/phy/st/phy-stm32-combophy.c
>> new file mode 100644
>> index 000000000000..7cd4193b0277
>> --- /dev/null
>> +++ b/drivers/phy/st/phy-stm32-combophy.c
>> @@ -0,0 +1,607 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * STMicroelectronics COMBOPHY STM32MP25 Controller driver.
>> + *
>> + * Copyright (C) 2024 STMicroelectronics
>> + * Author: Christian Bruel <christian.bruel@foss.st.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <dt-bindings/phy/phy.h>
>> +
>> +#define SYSCFG_COMBOPHY_CR1 0x4C00
>> +#define SYSCFG_COMBOPHY_CR2 0x4C04
>> +#define SYSCFG_COMBOPHY_CR4 0x4C0C
>> +#define SYSCFG_COMBOPHY_CR5 0x4C10
>> +#define SYSCFG_COMBOPHY_SR  0x4C14
> 
> Lowercase hex values please
> 
>> +#define SYSCFG_PCIEPRGCR    0x6080
>> +
>> +/* SYSCFG PCIEPRGCR */
>> +#define STM32MP25_PCIEPRGCR_EN	  BIT(0)
>> +#define STM32MP25_PCIEPRG_IMPCTRL_OHM     GENMASK(3, 1)
>> +#define STM32MP25_PCIEPRG_IMPCTRL_VSWING  GENMASK(5, 4)
>> +
>> +/* SYSCFG SYSCFG_COMBOPHY_SR */
>> +#define STM32MP25_PIPE0_PHYSTATUS BIT(1)
>> +
>> +/* SYSCFG CR1 */
>> +#define SYSCFG_COMBOPHY_CR1_REFUSEPAD BIT(0)
>> +#define SYSCFG_COMBOPHY_CR1_MPLLMULT GENMASK(7, 1)
>> +#define SYSCFG_COMBOPHY_CR1_REFCLKSEL GENMASK(16, 8)
>> +#define SYSCFG_COMBOPHY_CR1_REFCLKDIV2 BIT(17)
>> +#define SYSCFG_COMBOPHY_CR1_REFSSPEN BIT(18)
>> +#define SYSCFG_COMBOPHY_CR1_SSCEN BIT(19)
>> +
>> +/* SYSCFG CR4 */
>> +#define SYSCFG_COMBOPHY_CR4_RX0_EQ GENMASK(2, 0)
>> +
>> +#define MPLLMULT_19_2 (0x02u << 1)
>> +#define MPLLMULT_20   (0x7Du << 1)
>> +#define MPLLMULT_24   (0x68u << 1)
>> +#define MPLLMULT_25   (0x64u << 1)
>> +#define MPLLMULT_26   (0x60u << 1)
>> +#define MPLLMULT_38_4 (0x41u << 1)
>> +#define MPLLMULT_48   (0x6Cu << 1)
>> +#define MPLLMULT_50   (0x32u << 1)
>> +#define MPLLMULT_52   (0x30u << 1)
>> +#define MPLLMULT_100  (0x19u << 1)
>> +
>> +#define REFCLKSEL_0   0
>> +#define REFCLKSEL_1   (0x108u << 8)
>> +
>> +#define REFCLDIV_0    0
>> +
>> +/* SYSCFG CR2 */
>> +#define SYSCFG_COMBOPHY_CR2_MODESEL GENMASK(1, 0)
>> +#define SYSCFG_COMBOPHY_CR2_ISO_DIS BIT(15)
>> +
>> +#define COMBOPHY_MODESEL_PCIE 0
>> +#define COMBOPHY_MODESEL_USB  3
>> +
>> +/* SYSCFG CR5 */
>> +#define SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS BIT(12)
>> +
>> +#define COMBOPHY_SUP_ANA_MPLL_LOOP_CTL 0xC0
>> +#define COMBOPHY_PROP_CNTRL GENMASK(7, 4)
>> +
>> +struct stm32_combophy {
>> +	struct phy *phy;
>> +	struct regmap *regmap;
>> +	struct device *dev;
>> +	void __iomem *base;
>> +	struct reset_control *phy_reset;
>> +	struct clk *phy_clk;
>> +	struct clk *pad_clk;
>> +	struct clk *ker_clk;
>> +	unsigned int type;
>> +	bool is_init;
>> +	int irq_wakeup;
>> +};
>> +
>> +struct clk_impedance  {
>> +	u32 microohm;
>> +	u32 vswing[4];
>> +};
>> +
>> +/*
>> + * lookup table to hold the settings needed for a ref clock frequency
>> + * impedance, the offset is used to set the IMP_CTL and DE_EMP bit of the
>> + * PRG_IMP_CTRL register
>> + */
>> +static const struct clk_impedance imp_lookup[] = {
>> +	{ 6090000, { 442000, 564000, 684000, 802000 } },
>> +	{ 5662000, { 528000, 621000, 712000, 803000 } },
>> +	{ 5292000, { 491000, 596000, 700000, 802000 } },
>> +	{ 4968000, { 558000, 640000, 722000, 803000 } },
>> +	{ 4684000, { 468000, 581000, 692000, 802000 } },
>> +	{ 4429000, { 554000, 613000, 717000, 803000 } },
>> +	{ 4204000, { 511000, 609000, 706000, 802000 } },
>> +	{ 3999000, { 571000, 648000, 726000, 803000 } }
>> +};
>> +
>> +static int stm32_impedance_tune(struct stm32_combophy *combophy)
>> +{
>> +	u8 imp_size = ARRAY_SIZE(imp_lookup);
>> +	u8 vswing_size = ARRAY_SIZE(imp_lookup[0].vswing);
>> +	u8 imp_of, vswing_of;
>> +	u32 max_imp = imp_lookup[0].microohm;
>> +	u32 min_imp = imp_lookup[imp_size - 1].microohm;
> 
> table is ordered, pls mention that in comments somewhere
> 
>> +	u32 max_vswing = imp_lookup[imp_size - 1].vswing[vswing_size - 1];
>> +	u32 min_vswing = imp_lookup[0].vswing[0];
>> +	u32 val;
>> +
>> +	if (!of_property_read_u32(combophy->dev->of_node, "st,output-micro-ohms", &val)) {
>> +		if (val < min_imp || val > max_imp) {
>> +			dev_err(combophy->dev, "Invalid value %u for output ohm\n", val);
>> +			return -EINVAL;
>> +		}
>> +
>> +		for (imp_of = 0 ; imp_of < ARRAY_SIZE(imp_lookup); imp_of++)
>> +			if (imp_lookup[imp_of].microohm <= val)
>> +				break;
>> +
>> +		dev_dbg(combophy->dev, "Set %u micro-ohms output impedance\n",
>> +			imp_lookup[imp_of].microohm);
>> +
>> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
>> +				   STM32MP25_PCIEPRG_IMPCTRL_OHM,
>> +				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_OHM, imp_of));
>> +	} else {
>> +		regmap_read(combophy->regmap, SYSCFG_PCIEPRGCR, &val);
>> +		imp_of = FIELD_GET(STM32MP25_PCIEPRG_IMPCTRL_OHM, val);
>> +	}
>> +
>> +	if (!of_property_read_u32(combophy->dev->of_node, "st,output-vswing-microvolt", &val)) {
>> +		if (val < min_vswing || val > max_vswing) {
>> +			dev_err(combophy->dev, "Invalid value %u for output vswing\n", val);
>> +			return -EINVAL;
>> +		}
>> +
>> +		for (vswing_of = 0 ; vswing_of < ARRAY_SIZE(imp_lookup[imp_of].vswing); vswing_of++)
>> +			if (imp_lookup[imp_of].vswing[vswing_of] >= val)
>> +				break;
>> +
>> +		dev_dbg(combophy->dev, "Set %u microvolt swing\n",
>> +			 imp_lookup[imp_of].vswing[vswing_of]);
>> +
>> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
>> +				   STM32MP25_PCIEPRG_IMPCTRL_VSWING,
>> +				   FIELD_PREP(STM32MP25_PCIEPRG_IMPCTRL_VSWING, vswing_of));
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int stm32_combophy_pll_init(struct stm32_combophy *combophy)
>> +{
>> +	int ret;
>> +	u32 refclksel, pllmult, propcntrl, val;
>> +	u32 clk_rate;
>> +
>> +	if (combophy->pad_clk)
>> +		clk_rate = clk_get_rate(combophy->pad_clk);
>> +	else
>> +		clk_rate = clk_get_rate(combophy->ker_clk);
>> +
>> +	reset_control_assert(combophy->phy_reset);
>> +
>> +	dev_dbg(combophy->dev, "%s pll init rate %d\n",
>> +		combophy->pad_clk ? "External" : "Ker", clk_rate);
>> +
>> +	/*
>> +	 * vddcombophy is interconnected with vddcore. Isolation bit should be unset
>> +	 * before using the ComboPHY.
>> +	 */
>> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
>> +			   SYSCFG_COMBOPHY_CR2_ISO_DIS, SYSCFG_COMBOPHY_CR2_ISO_DIS);
>> +
>> +	if (combophy->type != PHY_TYPE_PCIE)
>> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
>> +				   SYSCFG_COMBOPHY_CR1_REFSSPEN, SYSCFG_COMBOPHY_CR1_REFSSPEN);
>> +
>> +	if (combophy->type == PHY_TYPE_PCIE && !combophy->pad_clk)
>> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
>> +				   STM32MP25_PCIEPRGCR_EN, STM32MP25_PCIEPRGCR_EN);
>> +
>> +	if (of_property_read_bool(combophy->dev->of_node, "st,ssc-on")) {
>> +		dev_dbg(combophy->dev, "Enabling clock with SSC\n");
>> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
>> +				   SYSCFG_COMBOPHY_CR1_SSCEN, SYSCFG_COMBOPHY_CR1_SSCEN);
>> +	}
>> +
>> +	if (!of_property_read_u32(combophy->dev->of_node, "st,rx-equalizer", &val)) {
>> +		dev_dbg(combophy->dev, "Set RX equalizer %u\n", val);
>> +		if (val > SYSCFG_COMBOPHY_CR4_RX0_EQ) {
>> +			dev_err(combophy->dev, "Invalid value %u for rx0 equalizer\n", val);
>> +			return -EINVAL;
>> +		}
>> +
>> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR4,
>> +			   SYSCFG_COMBOPHY_CR4_RX0_EQ, val);
>> +	}
>> +
>> +	if (combophy->type == PHY_TYPE_PCIE) {
>> +		ret = stm32_impedance_tune(combophy);
>> +		if (ret) {
>> +			reset_control_deassert(combophy->phy_reset);
>> +			goto out;
>> +		}
>> +
>> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
>> +				   SYSCFG_COMBOPHY_CR1_REFUSEPAD,
>> +				   combophy->pad_clk ? SYSCFG_COMBOPHY_CR1_REFUSEPAD : 0);
>> +	}
>> +
>> +	switch (clk_rate) {
>> +	case 100000000:
>> +		pllmult = MPLLMULT_100;
>> +		refclksel = REFCLKSEL_0;
>> +		propcntrl = 0x8u << 4;
>> +		break;
>> +	case 19200000:
>> +		pllmult = MPLLMULT_19_2;
>> +		refclksel = REFCLKSEL_1;
>> +		propcntrl = 0x8u << 4;
>> +		break;
>> +	case 25000000:
>> +		pllmult = MPLLMULT_25;
>> +		refclksel = REFCLKSEL_0;
>> +		propcntrl = 0xEu << 4;
>> +		break;
>> +	case 24000000:
>> +		pllmult = MPLLMULT_24;
>> +		refclksel = REFCLKSEL_1;
>> +		propcntrl = 0xEu << 4;
>> +		break;
>> +	case 20000000:
>> +		pllmult = MPLLMULT_20;
>> +		refclksel = REFCLKSEL_0;
>> +		propcntrl = 0xEu << 4;
>> +		break;
>> +	default:
>> +		dev_err(combophy->dev, "Invalid rate 0x%x\n", clk_rate);
>> +		reset_control_deassert(combophy->phy_reset);
>> +		ret = -EINVAL;
>> +		goto out;
>> +	};
>> +
>> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
>> +			   SYSCFG_COMBOPHY_CR1_REFCLKDIV2, REFCLDIV_0);
>> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
>> +			   SYSCFG_COMBOPHY_CR1_REFCLKSEL, refclksel);
>> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
>> +			   SYSCFG_COMBOPHY_CR1_MPLLMULT, pllmult);
>> +
>> +	/*
>> +	 * Force elasticity buffer to be tuned for the reference clock as
>> +	 * the separated clock model is not supported
>> +	 */
>> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR5,
>> +			   SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS, SYSCFG_COMBOPHY_CR5_COMMON_CLOCKS);
>> +
>> +	reset_control_deassert(combophy->phy_reset);
>> +
>> +	ret = regmap_read_poll_timeout(combophy->regmap, SYSCFG_COMBOPHY_SR, val,
>> +				       !(val & STM32MP25_PIPE0_PHYSTATUS),
>> +				       10, 1000);
>> +	if (ret) {
>> +		dev_err(combophy->dev, "timeout, cannot lock PLL\n");
>> +		goto out;
>> +	}
>> +
>> +	if (combophy->type == PHY_TYPE_PCIE) {
>> +		val = readl_relaxed(combophy->base + COMBOPHY_SUP_ANA_MPLL_LOOP_CTL);
>> +		val &= ~COMBOPHY_PROP_CNTRL;
>> +		val |= propcntrl;
>> +		writel_relaxed(val, combophy->base + COMBOPHY_SUP_ANA_MPLL_LOOP_CTL);
>> +	}
>> +
>> +	return 0;
>> +
>> +out:
>> +	if (combophy->type == PHY_TYPE_PCIE && !combophy->pad_clk)
>> +		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
>> +				   STM32MP25_PCIEPRGCR_EN, 0);
>> +
>> +	if (combophy->type != PHY_TYPE_PCIE)
>> +		regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR1,
>> +				   SYSCFG_COMBOPHY_CR1_REFSSPEN, 0);
>> +
>> +	regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
>> +			   SYSCFG_COMBOPHY_CR2_ISO_DIS, 0);
>> +
>> +	return ret;
>> +}
>> +
>> +static struct phy *stm32_combophy_xlate(struct device *dev,
>> +					const struct of_phandle_args *args)
>> +{
>> +	struct stm32_combophy *combophy = dev_get_drvdata(dev);
>> +	unsigned int type;
>> +
>> +	if (args->args_count != 1) {
>> +		dev_err(dev, "invalid number of cells in 'phy' property\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	type = args->args[0];
>> +	if (type != PHY_TYPE_USB3 && type != PHY_TYPE_PCIE) {
>> +		dev_err(dev, "unsupported device type: %d\n", type);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (combophy->pad_clk && type != PHY_TYPE_PCIE) {
>> +		dev_err(dev, "Invalid use of clk_pad for USB3 mode\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	combophy->type = type;
>> +
>> +	return combophy->phy;
>> +}
>> +
>> +static int stm32_combophy_set_mode(struct stm32_combophy *combophy)
>> +{
>> +	int type = combophy->type;
>> +	u32 val;
>> +
>> +	switch (type) {
>> +	case PHY_TYPE_PCIE:
>> +		dev_dbg(combophy->dev, "setting PCIe ComboPHY\n");
>> +		val = COMBOPHY_MODESEL_PCIE;
>> +		break;
>> +	case PHY_TYPE_USB3:
>> +		dev_dbg(combophy->dev, "setting USB3 ComboPHY\n");
>> +		val = COMBOPHY_MODESEL_USB;
>> +		break;
>> +	default:
>> +		dev_err(combophy->dev, "Invalid PHY mode %d\n", type);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return regmap_update_bits(combophy->regmap, SYSCFG_COMBOPHY_CR2,
>> +				  SYSCFG_COMBOPHY_CR2_MODESEL, val);
>> +}
>> +
>> +static int stm32_combophy_enable_clocks(struct stm32_combophy *combophy)
>> +{
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(combophy->phy_clk);
>> +	if (ret) {
>> +		dev_err(combophy->dev, "Core clock enable failed %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(combophy->ker_clk);
>> +	if (ret) {
>> +		dev_err(combophy->dev, "ker_usb3pcie clock enable failed %d\n", ret);
>> +		clk_disable_unprepare(combophy->phy_clk);
>> +		return ret;
>> +	}
>> +
>> +	if (combophy->pad_clk) {
>> +		ret = clk_prepare_enable(combophy->pad_clk);
>> +		if (ret) {
>> +			dev_err(combophy->dev, "External clock enable failed %d\n", ret);
>> +			clk_disable_unprepare(combophy->ker_clk);
>> +			clk_disable_unprepare(combophy->phy_clk);
>> +			return ret;
>> +		}
>> +	}
> 
> Can you use bulk_prepare for this?

Yes, need a twist a little bit to handle optional and required clocks 
together but that's better.

thanks,

Christian
>