Re: [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 !

[...]