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

[Marek Vasut]

On 8/18/26 11:28 AM, Fabrice Gasnier wrote:

Hello Fabrice,

>>>> +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 ?

It makes no difference in this case, since the value can be either 
0/1/2, but fixed.

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

[...]

>>> 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
[...]

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

Fixed in V3.

>> +    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);
> +
That's nice, also added to V3, thanks !

[...]