Re: [PATCH v2 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY

On 8/17/26 21:43, Marek Vasut wrote:
> 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;

Hello Marek,

Just noticed refsel should be unsigned ?

>>> +
>>> +    /* 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 .

Indeed, nice thought

> 
> [...]
> 
>>> +    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.
>> */
> Is this what you have in mind ?

Yes, with one addition, please see next comment

> 
> 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 };

clkcmninit should be a child of clk48 which basically represent the PLL
(480MHz) as it is still needed as parent. See below.

BTW, mainly a nit: could rename clk48 to clkpll and update frequency to
480M.

>      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;

Would initialize with (to adapt) :

+ phy_dev->clkcmn_hw.init = CLK_HW_INIT_HW(name, &phy_dev->clk48_hw,
+					 &stm32_usb2phy_clkcmn_ops, 0);
+

Thanks & BR,
Fabrice

> +    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;
>  }