пт, 15 груд. 2023 р. о 20:45 Sean Anderson <sean...@gmail.com> пише:
>
> On 12/15/23 13:26, Svyatoslav Ryhel wrote:
> > пт, 15 груд. 2023 р. о 19:25 Sean Anderson <sean...@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 <clamo...@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+rene...@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.
>
This driver uses a single (1) clock with a single purpose.
Why do you think you need a clock in a clock-gpio driver? Hm, I don't
know. This will remain a mystery for future archeo-coders yeah.
> >>> };
> >>>
> >>> 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.
>
Then document that its use is discouraged. Until then it is not restricted.
> >>> 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.
>
I don't quite understand the logic behind this but ok.
> >>> +}
> >>> +
> >>> 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.
>
Alright
> >>> + 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
>
