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

[Sean Anderson]

On 12/15/23 13:26, Svyatoslav Ryhel wrote:
> пт, 15 груд. 2023 р. о 19:25 Sean Anderson <seanga2 at gmail.com> пише:
>>
>> On 11/17/23 02:52, 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 | 47 +++++++++++++++++++++++++++++++++++-------
>>>    1 file changed, 40 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
>>> index 26d795b978..a55470cfbf 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 clk              *clk;
>>
>> Please name this "parent".
>>
> I see no need in this since clk is generic name and this driver can
> accept only one clock with any name.

It's good documentation. It makes it clear what the purpose of the member is.

>>>    };
>>>
>>>    static int clk_gpio_enable(struct clk *clk)
>>>    {
>>>        struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>>
>>> +     clk_prepare_enable(priv->clk);
>>
>> Just clk_enable is fine. We don't have a separate prepare stage line Linux.
>>
> U-Boot supports clk_prepare_enable and since this support is present I
> see no contradiction in using it.

These functions are only there for to make it easier to port code from Linux.
Since this code was written for U-Boot, just use the actual function.

>>>        dm_gpio_set_value(&priv->enable, 1);
>>>
>>>        return 0;
>>> @@ -26,21 +30,50 @@ 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_unprepare(priv->clk);
>>>
>>>        return 0;
>>>    }
>>>
>>> +static ulong clk_gpio_get_rate(struct clk *clk)
>>> +{
>>> +     struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
>>> +     int ret;
>>> +
>>> +     ret = dm_gpio_get_value(&priv->enable);
>>> +     if (ret)
>>> +             return clk_get_rate(priv->clk);
>>> +     else
>>> +             return -EINVAL;
>>
>> This should always return the parent rate.
>>
> How can you return clock rate if the gate is disabled?

This allows consumers to know what the rate will be before they
enable the clock.

>>> +}
>>> +
>>>    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)
>>
>> This change should be a separate commit.
>>
> This actually should not be accepted first place
> 
> .of_to_plat - convert device tree data to plat
> .probe - make a device ready for use
> 
> https://github.com/u-boot/u-boot/blob/master/doc/develop/driver-model/design.rst

Yes, this is why I was a bit confused :)

>>>    {
>>>        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_err("%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_err("%s: Could not decode enable-gpios (%d)\n",
>>> +                     __func__, ret);
>>
>> Use dev_dbg for probe errors to reduce binary size.
>>
> These errors provide intel about probing 2 key parts of the driver.
> How would you know it fails in case you have no devices which face
> this error? Iis there any convention or restriction which prohibits
> this? If yes, then provide a document please.

https://docs.u-boot.org/en/latest/develop/driver-model/design.html#error-codes

| Ideally, messages in drivers should only be displayed when debugging,
| e.g. by using log_debug() although in extreme cases log_warning() or
| log_error() may be used.

> All other general clock drivers use log_err/dev_err versions btw.

Yeah, it's something we have not been so consistent with enforcing.

>>> +             return ret;
>>> +     }
>>> +
>>> +     return 0;
>>>    }
>>>
>>>    /*
>>> @@ -59,7 +92,7 @@ U_BOOT_DRIVER(gpio_gate_clock) = {
>>>        .name           = "gpio_clock",
>>>        .id             = UCLASS_CLK,
>>>        .of_match       = clk_gpio_match,
>>> -     .probe          = clk_gpio_probe,
>>> +     .of_to_plat     = clk_gpio_of_to_plat,
>>>        .priv_auto      = sizeof(struct clk_gpio_priv),
>>>        .ops            = &clk_gpio_ops,
>>>        .flags          = DM_FLAG_PRE_RELOC,
>>
>> --Sean