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

Christian Bruel <christian.bruel@xxxxxxxxxxx> · Mon, 9 Sep 2024 10:40:44 +0200

Hi,

On 9/6/24 15:44, Philipp Zabel wrote:
Hi,

On Di, 2024-09-03 at 14:13 +0200, 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@xxxxxxxxxxx>
---
  drivers/phy/st/Kconfig              |  11 +
  drivers/phy/st/Makefile             |   1 +
  drivers/phy/st/phy-stm32-combophy.c | 590 ++++++++++++++++++++++++++++
  3 files changed, 602 insertions(+)
  create mode 100644 drivers/phy/st/phy-stm32-combophy.c

[...]

diff --git a/drivers/phy/st/phy-stm32-combophy.c b/drivers/phy/st/phy-stm32-combophy.c
new file mode 100644
index 000000000000..305ba124d092
--- /dev/null
+++ b/drivers/phy/st/phy-stm32-combophy.c
@@ -0,0 +1,590 @@
[...]
+static int stm32_combophy_pll_init(struct stm32_combophy *combophy)
+{
+	int ret;
+	u32 refclksel, pllmult, propcntrl, val;
+	u32 clk_rate;
+	struct clk *clk;
+
+	if (combophy->have_pad_clk)
+		clk = combophy->clks[PAD_CLK].clk;
+	else
+		clk = combophy->clks[KER_CLK].clk;
+
+	clk_rate = clk_get_rate(clk);
+
+	reset_control_assert(combophy->phy_reset);
+
+	dev_dbg(combophy->dev, "%s pll init rate %d\n",
+		combophy->have_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);

Could the multiple accesses to SYSCFG_COMBOPHY_CR1 be consolidated into
a single regmap_update_bits()?

yes, thanks


+
+	if (combophy->type == PHY_TYPE_PCIE && !combophy->have_pad_clk)
+		regmap_update_bits(combophy->regmap, SYSCFG_PCIEPRGCR,
+				   STM32MP25_PCIEPRGCR_EN, STM32MP25_PCIEPRGCR_EN);
+
+	if (of_property_present(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);

This path looks like it should deassert the phy_reset as well.

Ack


+			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->have_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->have_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 int stm32_combophy_probe(struct platform_device *pdev)
+{
+	struct stm32_combophy *combophy;
+	struct device *dev = &pdev->dev;
+	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);
+
+	if (stm32_combophy_get_clocks(combophy))
+		return ret;
+
+	combophy->phy_reset = devm_reset_control_get(dev, "phy");

Please use devm_reset_control_get_exclusive() directly.

OK,

thanks,
Christian

regards
Philipp







