[PATCH v2 1/1] clk: clk-gpio: add actual gated clock

[Marek Vasut]

On 12/17/23 16:37, Sean Anderson wrote:
> On 12/17/23 10:19, Marek Vasut wrote:
>> On 12/17/23 02:20, Sean Anderson wrote:
>>> On 12/16/23 19:19, Marek Vasut wrote:
>>>> On 12/16/23 17:56, Sean Anderson wrote:
>>>>> On 12/16/23 11:52, Marek Vasut wrote:
>>>>>> On 12/16/23 16:37, Sean Anderson wrote:
>>>>>>> On 12/16/23 03:48, Svyatoslav Ryhel wrote:
>>>>>>>> Existing gpio-gate-clock driver acts like a simple GPIO switch 
>>>>>>>> without any
>>>>>>>> effect on gated clock. Add actual clock actions into 
>>>>>>>> enable/disable ops and
>>>>>>>> implement get_rate op by passing gated clock if it is enabled.
>>>>>>>>
>>>>>>>> Signed-off-by: Svyatoslav Ryhel <clamor95 at gmail.com>
>>>>>>>> ---
>>>>>>>>   drivers/clk/clk-gpio.c | 44 
>>>>>>>> ++++++++++++++++++++++++++++++++++--------
>>>>>>>>   1 file changed, 36 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
>>>>>>>> index 26d795b978..72d9747a47 100644
>>>>>>>> --- a/drivers/clk/clk-gpio.c
>>>>>>>> +++ b/drivers/clk/clk-gpio.c
>>>>>>>> @@ -3,19 +3,23 @@
>>>>>>>>    * Copyright (C) 2023 Marek Vasut 
>>>>>>>> <marek.vasut+renesas at mailbox.org>
>>>>>>>>    */
>>>>>>>> -#include <asm/gpio.h>
>>>>>>>> -#include <common.h>
>>>>>>>> -#include <clk-uclass.h>
>>>>>>>> +#include <clk.h>
>>>>>>>>   #include <dm.h>
>>>>>>>> +#include <clk-uclass.h>
>>>>>>>> +#include <linux/clk-provider.h>
>>>>>>>> +
>>>>>>>> +#include <asm/gpio.h>
>>>>>>>>   struct clk_gpio_priv {
>>>>>>>> -    struct gpio_desc    enable;
>>>>>>>> +    struct gpio_desc    enable;    /* GPIO, controlling the 
>>>>>>>> gate */
>>>>>>>> +    struct clk        *clk;    /* Gated clock */
>>>>>>>>   };
>>>>>>>>   static int clk_gpio_enable(struct clk *clk)
>>>>>>>>   {
>>>>>>>>       struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>>>>>> +    clk_enable(priv->clk);
>>>>>>>>       dm_gpio_set_value(&priv->enable, 1);
>>>>>>>>       return 0;
>>>>>>>> @@ -26,21 +30,45 @@ static int clk_gpio_disable(struct clk *clk)
>>>>>>>>       struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>>>>>>       dm_gpio_set_value(&priv->enable, 0);
>>>>>>>> +    clk_disable(priv->clk);
>>>>>>>>       return 0;
>>>>>>>>   }
>>>>>>>> +static ulong clk_gpio_get_rate(struct clk *clk)
>>>>>>>> +{
>>>>>>>> +    struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>>>>>> +
>>>>>>>> +    return clk_get_rate(priv->clk);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   const struct clk_ops clk_gpio_ops = {
>>>>>>>>       .enable        = clk_gpio_enable,
>>>>>>>>       .disable    = clk_gpio_disable,
>>>>>>>> +    .get_rate    = clk_gpio_get_rate,
>>>>>>>>   };
>>>>>>>> -static int clk_gpio_probe(struct udevice *dev)
>>>>>>>> +static int clk_gpio_of_to_plat(struct udevice *dev)
>>>>>>>>   {
>>>>>>>>       struct clk_gpio_priv *priv = dev_get_priv(dev);
>>>>>>>> +    int ret;
>>>>>>>> -    return gpio_request_by_name(dev, "enable-gpios", 0,
>>>>>>>> -                    &priv->enable, GPIOD_IS_OUT);
>>>>>>>> +    priv->clk = devm_clk_get(dev, NULL);
>>>>>>>> +    if (IS_ERR(priv->clk)) {
>>>>>>>> +        log_debug("%s: Could not get gated clock: %ld\n",
>>>>>>>> +              __func__, PTR_ERR(priv->clk));
>>>>>>>> +        return PTR_ERR(priv->clk);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    ret = gpio_request_by_name(dev, "enable-gpios", 0,
>>>>>>>> +                   &priv->enable, GPIOD_IS_OUT);
>>>>>>>> +    if (ret) {
>>>>>>>> +        log_debug("%s: Could not decode enable-gpios (%d)\n",
>>>>>>>> +              __func__, ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>
>>>>>> All this forwarding of clock enable/disable/get_rate for parent 
>>>>>> clock should already be done in clock core. If it is not, then it 
>>>>>> should be fixed in clock core.
>>>>>
>>>>> Only CCF forwards things. It's up to the driver to do it in the 
>>>>> non-CCF case.
>>>>
>>>> Shall we add dependency on CCF instead of duplicating code in drivers ?
>>>
>>> I'd rather not. This will be useful on non-CCF systems.
>>
>> Then the forwarding functionality should be in the clock core, 
>> basically if a driver does not implement callback <something>, call 
>> parent clock callback <something>, and repeat until you reach the top 
>> of the clock tree. If you do reach the top, fail with some error code. 
>> Isn't that how clock enable already works anyway ?
> 
> No. In the non-CCF case, we have no idea what the parent of any given 
> clock is. So
> there's no framework we can provide.

Huh, but this driver is attempting to grab some sort of parent clock and 
enable them, so how does that work ?

> Now, you might say "why not add a get_parent op?" Well, that will work 
> fine for
> enabling things, but it is a bit hairy on the disable side. You really 
> need to have
> refcounting to protect parent clocks (which can have many children). But 
> we don't
> really have anywhere to store refcounts. struct clk has a member for it, 
> but that's
> only because it's abused as private data by CCF. Non-CCF clocks have no 
> clock
> subsystem data allocated at all. Another alternative is to just only 
> disable the leaf
> node, which is what we currently do. That's OK I think.
> 
> But that solution requires some more-intensive effort which I don't want 
> to impose on
> random contributors. So for the time being, all non-CCF clock drivers 
> must handle
> enabling their own parent clocks.

So, in light of this, why not push for use of CCF as much as possible 
and deprecate the current non-CCF clock support ? It seems like the 
right thing to do, and avoids growing ad-hoc workarounds for missing 
core functionality in drivers, which I really want to avoid .