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

[Marek Vasut]

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 ?