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

[Sean Anderson]

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.

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.

--Sean