Re: [PATCH v2 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY
From: Marek Vasut
Date: Mon Aug 17 2026 - 19:59:26 EST
On 8/17/26 6:22 PM, Fabrice Gasnier wrote:
Hello Fabrice,
+++ b/drivers/phy/st/phy-stm32-usb2phy.c
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * STMicroelectronics STM32 USB2 PHY Controller driver
+ * Currently Only supported for STM32MP25
+ *
+ * Copyright (C) 2022 STMicroelectronics
Hi Marek,
Could update to 2026 ?
Fixed in V3.
[...]
+static int stm32_usb2phy_enable(struct stm32_usb2phy *phy_dev)
+{
+ const struct stm32mp2_usb2phy_hw_data *phy_data = phy_dev->hw_data;
+ unsigned long rate;
+ int refsel, ret;
+
+ /* Check if a phy is already init or clk48 in use */
+ if (atomic_inc_return(&phy_dev->en_refcnt) > 1)
+ return 0;
+
+ rate = clk_get_rate(phy_dev->phyref);
+ if (rate == 19200000)
+ refsel = 0;
+ else if (rate == 20000000)
+ refsel = 1;
+ else if (rate == 24000000)
+ refsel = 2;
+ else
+ return -EINVAL;
+
+ ret = regmap_update_bits(phy_dev->regmap,
+ phy_dev->cr_offset,
+ phy_data->phyrefsel_mask,
+ field_prep(phy_data->phyrefsel_mask, refsel));
+ if (ret)
+ return ret;
+
Hi Marek,
Below condition
+ if (phy_data->is_usb2_host_only) {
+ /*
+ * The clock should default to active after standby, as it is
+ * needed when resuming OHCI to access its registers.
+ * CMN is default reset to 1, so enforce it is cleared, when the
+ * clock enable request from OHCI driver comes at resume time.
+ */
+ ret = regmap_clear_bits(phy_dev->regmap, phy_dev->cr_offset,
+ SYSCFG_USB2PHY2CR_USB2PHY2CMN);
+ if (ret)
+ return ret;
+ }
up to here, is a specific part to manage OHCI controller clock during
suspend modes (either bus suspend, or system-wide platform low power PM).
This should be moved to the clock provider api. E.g. to register a 2nd
clock.
Point here is the bit is always cleared, even if OHCI is
unused/disabled, but EHCI is. EHCI can be enabled w/o OHCI when there's
an on-board USB HUB (in such case only High Speed traffic is expected).
EHCI don't require to clear CMN for suspend states.
Please see at the end of this email.
+
+ ret = regulator_enable(phy_dev->vdd33);
+ if (ret)
+ return ret;
[...]
+static int stm32_usb2phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
+{
+ struct stm32_usb2phy *phy_dev = phy_get_drvdata(phy);
+ const struct stm32mp2_usb2phy_hw_data *phy_data = phy_dev->hw_data;
+ u32 val, mask = SYSCFG_USB2PHY2CR_USB2PHY2CMN;
+ int ret;
Then could simplify here directly for host only PHY : there's no point
in poking control register here.
(As mentioned above, control SYSCFG_USB2PHY2CR_USB2PHY2CMN with clock
provider API for host-only configuration.)
if (phy_data->is_usb2_host_only)
return 0;
I think it would be even better at this point to register separate PHY ops for each PHY, one with and one without .set_mode .
[...]
Is this what you have in mind ?+ ret = devm_clk_hw_register(phy_dev->dev, &phy_dev->clk48_hw);
+ if (ret)
+ return dev_err_probe(phy_dev->dev, ret, "Failed to register 48 MHz clock\n");
In v2, the #clock-cells has been updated to 1. This allow to manage
separately the OHCI clock bit ("...CMN") as a child clock of the 48MHz
clock.
Please register a 2nd clock, so the OHCI controller can take benefit of it.
As you mention the downstream driver, please see there a specific
comment regarding the 2nd clock for OHCI:
/*
* USB2PHY provides several clocks used either by either USHB
(EHCI/OHCI), OTG or USB3DR.
* In case of OHCI, CMN bit must be cleared (clkohci_hw). This clock is
required to access
* the registers, to resume the controller from suspended state.
* So declare two clocks, the PLL used in all case, and the OHCI clocks
used by OHCI
* controller.
*/
diff --git a/arch/arm64/boot/dts/st/stm32mp231.dtsi b/arch/arm64/boot/dts/st/stm32mp231.dtsi
index e22bb53c9748b..2fa03b059c71b 100644
--- a/arch/arm64/boot/dts/st/stm32mp231.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp231.dtsi
@@ -883,7 +883,7 @@ usbh_ohci: usb@482e0000 {
compatible = "generic-ohci";
reg = <0x482e0000 0x1000>;
access-controllers = <&rifsc 63>;
- clocks = <&usb2_phy1 0>, <&rcc CK_BUS_USB2OHCI>;
+ clocks = <&usb2_phy1 1>, <&rcc CK_BUS_USB2OHCI>;
interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
phys = <&usb2_phy1>;
phy-names = "usb";
diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index dbaac8a930fda..6e0f25e33f05f 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -1765,7 +1765,7 @@ usbh_ohci: usb@482e0000 {
compatible = "generic-ohci";
reg = <0x482e0000 0x1000>;
access-controllers = <&rifsc 63>;
- clocks = <&usb2_phy1 0>, <&rcc CK_BUS_USB2OHCI>;
+ clocks = <&usb2_phy1 1>, <&rcc CK_BUS_USB2OHCI>;
interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
phys = <&usb2_phy1>;
phy-names = "usb";
diff --git a/drivers/phy/st/phy-stm32-usb2phy.c b/drivers/phy/st/phy-stm32-usb2phy.c
index 1daaf2f440d04..1c4b37094591c 100644
--- a/drivers/phy/st/phy-stm32-usb2phy.c
+++ b/drivers/phy/st/phy-stm32-usb2phy.c
@@ -27,6 +27,8 @@
#define SYSCFG_USB2PHY2CR_VBUSVLDEXTSEL BIT(5)
#define SYSCFG_USB2PHY2CR_VBUSVLDEXT BIT(6)
+#define USBH_NUM_CLK 2
+
struct stm32_usb2phy {
struct phy *phy;
struct regmap *regmap;
@@ -34,12 +36,15 @@ struct stm32_usb2phy {
struct reset_control *rstc;
struct clk *phyref;
struct regulator *vdd33;
- struct clk_hw clk48_hw;
const struct stm32mp2_usb2phy_hw_data *hw_data;
atomic_t en_refcnt;
enum phy_mode mode;
u32 cr_offset;
bool is_init;
+ struct clk_hw clk48_hw;
+ struct clk_hw clkcmn_hw;
+ /* Must be last */
+ struct clk_hw_onecell_data onecell;
};
struct stm32mp2_usb2phy_hw_data {
@@ -74,19 +79,6 @@ static int stm32_usb2phy_enable(struct stm32_usb2phy *phy_dev)
if (ret)
return ret;
- if (phy_data->is_usb2_host_only) {
- /*
- * The clock should default to active after standby, as it is
- * needed when resuming OHCI to access its registers.
- * CMN is default reset to 1, so enforce it is cleared, when the
- * clock enable request from OHCI driver comes at resume time.
- */
- ret = regmap_clear_bits(phy_dev->regmap, phy_dev->cr_offset,
- SYSCFG_USB2PHY2CR_USB2PHY2CMN);
- if (ret)
- return ret;
- }
-
ret = regulator_enable(phy_dev->vdd33);
if (ret)
return ret;
@@ -233,9 +225,33 @@ static const struct clk_ops stm32_usb2phy_clk48_ops = {
.recalc_rate = stm32_usb2phy_clk48_recalc_rate,
};
+static int stm32_usb2phy_clkcmn_prepare(struct clk_hw *hw)
+{
+ struct stm32_usb2phy *phy_dev = container_of(hw, struct stm32_usb2phy,
+ clkcmn_hw);
+
+ return regmap_clear_bits(phy_dev->regmap, phy_dev->cr_offset,
+ SYSCFG_USB2PHY2CR_USB2PHY2CMN);
+}
+
+static void stm32_usb2phy_clkcmn_unprepare(struct clk_hw *hw)
+{
+ struct stm32_usb2phy *phy_dev = container_of(hw, struct stm32_usb2phy,
+ clkcmn_hw);
+
+ regmap_set_bits(phy_dev->regmap, phy_dev->cr_offset,
+ SYSCFG_USB2PHY2CR_USB2PHY2CMN);
+}
+
+static const struct clk_ops stm32_usb2phy_clkcmn_ops = {
+ .prepare = stm32_usb2phy_clkcmn_prepare,
+ .unprepare = stm32_usb2phy_clkcmn_unprepare,
+};
+
static int stm32_usb2phy_probe(struct platform_device *pdev)
{
- struct clk_init_data init = { .ops = &stm32_usb2phy_clk48_ops };
+ struct clk_init_data clk48init = { .ops = &stm32_usb2phy_clk48_ops };
+ struct clk_init_data clkcmninit = { .ops = &stm32_usb2phy_clkcmn_ops };
struct phy_provider *phy_provider;
struct device *dev = &pdev->dev;
struct stm32_usb2phy *phy_dev;
@@ -243,7 +259,7 @@ static int stm32_usb2phy_probe(struct platform_device *pdev)
struct phy *phy;
int ret;
- phy_dev = devm_kzalloc(dev, sizeof(*phy_dev), GFP_KERNEL);
+ phy_dev = devm_kzalloc(dev, struct_size(phy_dev, onecell.hws, USBH_NUM_CLK), GFP_KERNEL);
if (!phy_dev)
return -ENOMEM;
@@ -285,20 +301,33 @@ static int stm32_usb2phy_probe(struct platform_device *pdev)
if (IS_ERR(phy_provider))
return PTR_ERR(phy_provider);
- init.name = devm_kasprintf(dev, GFP_KERNEL, "clk_%s_48m",
+ clk48init.name = devm_kasprintf(dev, GFP_KERNEL, "clk_%s_48m",
of_node_full_name(dev->of_node));
- if (!init.name)
+ if (!clk48init.name)
return -ENOMEM;
- phy_dev->clk48_hw.init = &init;
+ clkcmninit.name = devm_kasprintf(dev, GFP_KERNEL, "clk_%s_cmn",
+ of_node_full_name(dev->of_node));
+ if (!clkcmninit.name)
+ return -ENOMEM;
+
+ phy_dev->onecell.num = USBH_NUM_CLK;
+ phy_dev->onecell.hws[0] = &phy_dev->clk48_hw;
+ phy_dev->onecell.hws[1] = &phy_dev->clkcmn_hw;
+ phy_dev->clk48_hw.init = &clk48init;
ret = devm_clk_hw_register(phy_dev->dev, &phy_dev->clk48_hw);
if (ret)
return dev_err_probe(phy_dev->dev, ret, "Failed to register 48 MHz clock\n");
- ret = devm_of_clk_add_hw_provider(phy_dev->dev, of_clk_hw_simple_get, &phy_dev->clk48_hw);
+ phy_dev->clkcmn_hw.init = &clkcmninit;
+ ret = devm_clk_hw_register(phy_dev->dev, &phy_dev->clkcmn_hw);
+ if (ret)
+ return dev_err_probe(phy_dev->dev, ret, "Failed to register CMN clock\n");
+
+ ret = devm_of_clk_add_hw_provider(phy_dev->dev, of_clk_hw_onecell_get, &phy_dev->onecell);
if (ret)
- return dev_err_probe(phy_dev->dev, ret, "Failed to add 48 MHz clock provider\n");
+ return dev_err_probe(phy_dev->dev, ret, "Failed to add clock provider\n");
return 0;
}