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

[Svyatoslav Ryhel]

пт, 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.

> >   };
> >
> >   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.

> >       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?

> > +}
> > +
> >   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

> >   {
> >       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.

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

> > +             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