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

From: Marek Vasut <marex@nabladev.com>
To: Fabrice Gasnier <fabrice.gasnier@foss.st.com>, linux-usb@vger.kernel.org
Cc: Pankaj Dev <pankaj.dev@st.com>,
	Rahul Kumar <rahul.kumar05@st.com>,
	Christian Bruel <christian.bruel@foss.st.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Conor Dooley <conor+dt@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Rob Herring <robh@kernel.org>, Rosen Penev <rosenp@gmail.com>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	Vinod Koul <vkoul@kernel.org>,
	devicetree@vger.kernel.org, kernel@dh-electronics.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org,
	linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH v2 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY
Date: Tue, 18 Aug 2026 11:53:19 +0200	[thread overview]
Message-ID: <0198d074-6fc8-45d4-9ef0-eac299ec9d82@nabladev.com> (raw)
In-Reply-To: <ed1d6988-251a-43d3-ba0e-aaa3e54ad07b@foss.st.com>

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 !

[...]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy