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

15 Dec 2023

пт, 15 груд. 2023 р. о 20:45 Sean Anderson seanga2@gmail.com пише:
...
On 12/15/23 13:26, Svyatoslav Ryhel wrote:
...
пт, 15 груд. 2023 р. о 19:25 Sean Anderson seanga2@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@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@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...
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-cod...
| 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

    