Re: [PATCH 1/3] phy: stm32-usbphyc: add counter of PLL consumer

[Sean Anderson <seanga2@gmail.com>]

On 5/10/22 5:51 AM, Amelie Delaunay wrote:
> Hi Patrick,
> Hi Sean,
> 
> On 5/9/22 16:37, Patrick DELAUNAY wrote:
>> Hi Sean,
>>
>> On 5/8/22 20:21, Sean Anderson wrote:
>>> On 4/26/22 8:37 AM, Patrick Delaunay wrote:
>>>> Add the counter of the PLL user n_pll_cons managed by the 2 functions
>>>> stm32_usbphyc_pll_enable / stm32_usbphyc_pll_disable.
>>>>
>>>> This counter allow to remove the function stm32_usbphyc_is_init
>>>> and it is a preliminary step for ck_usbo_48m introduction.
>>>
>>> Is it necessary to disable this clock before booting to Linux? If it isn't,
>>> then perhaps it is simpler to just not disable the clock.
>>>
>>> --Sean
>>
>>
>> No, it is not necessary, we only need to enable the clock for the first user.
>>
>> I copy the clock behavior from kernel,
>>
>> but I agree that can be simpler.
>>
>>
>> Amelie any notice about this point ?
>>
>> Do you prefer that I kept the behavior - same as kernel driver - or I simplify the U-Boot driver ?
> 
> In case the PLL has not been disabled before Kernel boot, usbphyc Kernel driver will wait for the PLL pwerdown.
> USB could also not being used in Kernel, so PLL would remain enabled, and would waste power.
> I am rather in favor of disabling the PLL.

It should be disabled if clk_ignore_unused is not in the kernel parameters,
as long as Linux is also aware of the clock.

Generally, I would like to avoid refcounting if possible. Many U-Boot
drivers do not disable their clocks (because they don't do any cleanup),
so you can end up with the clock staying on anyway.

--Sean

> Regards,
> Amelie
> 
>>
>>
>> Patrick
>>
>>
>>>
>>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>>> ---
>>>>
>>>>   drivers/phy/phy-stm32-usbphyc.c | 76 +++++++++++++++++++++------------
>>>>   1 file changed, 48 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/phy-stm32-usbphyc.c b/drivers/phy/phy-stm32-usbphyc.c
>>>> index 9c1dcfae52..16c8799eca 100644
>>>> --- a/drivers/phy/phy-stm32-usbphyc.c
>>>> +++ b/drivers/phy/phy-stm32-usbphyc.c
>>>> @@ -65,6 +65,7 @@ struct stm32_usbphyc {
>>>>           bool init;
>>>>           bool powered;
>>>>       } phys[MAX_PHYS];
>>>> +    int n_pll_cons;
>>>>   };
>>>>     static void stm32_usbphyc_get_pll_params(u32 clk_rate,
>>>> @@ -124,18 +125,6 @@ static int stm32_usbphyc_pll_init(struct stm32_usbphyc *usbphyc)
>>>>       return 0;
>>>>   }
>>>>   -static bool stm32_usbphyc_is_init(struct stm32_usbphyc *usbphyc)
>>>> -{
>>>> -    int i;
>>>> -
>>>> -    for (i = 0; i < MAX_PHYS; i++) {
>>>> -        if (usbphyc->phys[i].init)
>>>> -            return true;
>>>> -    }
>>>> -
>>>> -    return false;
>>>> -}
>>>> -
>>>>   static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc)
>>>>   {
>>>>       int i;
>>>> @@ -148,18 +137,17 @@ static bool stm32_usbphyc_is_powered(struct stm32_usbphyc *usbphyc)
>>>>       return false;
>>>>   }
>>>>   -static int stm32_usbphyc_phy_init(struct phy *phy)
>>>> +static int stm32_usbphyc_pll_enable(struct stm32_usbphyc *usbphyc)
>>>>   {
>>>> -    struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev);
>>>> -    struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id;
>>>>       bool pllen = readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN ?
>>>>                true : false;
>>>>       int ret;
>>>>   -    dev_dbg(phy->dev, "phy ID = %lu\n", phy->id);
>>>> -    /* Check if one phy port has already configured the pll */
>>>> -    if (pllen && stm32_usbphyc_is_init(usbphyc))
>>>> -        goto initialized;
>>>> +    /* Check if one consumer has already configured the pll */
>>>> +    if (pllen && usbphyc->n_pll_cons) {
>>>> +        usbphyc->n_pll_cons++;
>>>> +        return 0;
>>>> +    }
>>>>         if (usbphyc->vdda1v1) {
>>>>           ret = regulator_set_enable(usbphyc->vdda1v1, true);
>>>> @@ -190,23 +178,19 @@ static int stm32_usbphyc_phy_init(struct phy *phy)
>>>>       if (!(readl(usbphyc->base + STM32_USBPHYC_PLL) & PLLEN))
>>>>           return -EIO;
>>>>   -initialized:
>>>> -    usbphyc_phy->init = true;
>>>> +    usbphyc->n_pll_cons++;
>>>>         return 0;
>>>>   }
>>>>   -static int stm32_usbphyc_phy_exit(struct phy *phy)
>>>> +static int stm32_usbphyc_pll_disable(struct stm32_usbphyc *usbphyc)
>>>>   {
>>>> -    struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev);
>>>> -    struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id;
>>>>       int ret;
>>>>   -    dev_dbg(phy->dev, "phy ID = %lu\n", phy->id);
>>>> -    usbphyc_phy->init = false;
>>>> +    usbphyc->n_pll_cons--;
>>>>   -    /* Check if other phy port requires pllen */
>>>> -    if (stm32_usbphyc_is_init(usbphyc))
>>>> +    /* Check if other consumer requires pllen */
>>>> +    if (usbphyc->n_pll_cons)
>>>>           return 0;
>>>>         clrbits_le32(usbphyc->base + STM32_USBPHYC_PLL, PLLEN);
>>>> @@ -235,6 +219,42 @@ static int stm32_usbphyc_phy_exit(struct phy *phy)
>>>>       return 0;
>>>>   }
>>>>   +static int stm32_usbphyc_phy_init(struct phy *phy)
>>>> +{
>>>> +    struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev);
>>>> +    struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id;
>>>> +    int ret;
>>>> +
>>>> +    dev_dbg(phy->dev, "phy ID = %lu\n", phy->id);
>>>> +    if (usbphyc_phy->init)
>>>> +        return 0;
>>>> +
>>>> +    ret = stm32_usbphyc_pll_enable(usbphyc);
>>>> +    if (ret)
>>>> +        return log_ret(ret);
>>>> +
>>>> +    usbphyc_phy->init = true;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int stm32_usbphyc_phy_exit(struct phy *phy)
>>>> +{
>>>> +    struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev);
>>>> +    struct stm32_usbphyc_phy *usbphyc_phy = usbphyc->phys + phy->id;
>>>> +    int ret;
>>>> +
>>>> +    dev_dbg(phy->dev, "phy ID = %lu\n", phy->id);
>>>> +    if (!usbphyc_phy->init)
>>>> +        return 0;
>>>> +
>>>> +    ret = stm32_usbphyc_pll_disable(usbphyc);
>>>> +
>>>> +    usbphyc_phy->init = false;
>>>> +
>>>> +    return log_ret(ret);
>>>> +}
>>>> +
>>>>   static int stm32_usbphyc_phy_power_on(struct phy *phy)
>>>>   {
>>>>       struct stm32_usbphyc *usbphyc = dev_get_priv(phy->dev);
>>>>
>>>