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

[Marek Vasut]

On 12/16/23 17:55, Svyatoslav Ryhel wrote:
сб, 16 груд. 2023 р. о 18:52 Marek Vasut <ma...@denx.de> пише:

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 <clamo...@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+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 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.

It is not and this driver, as is, did not work on tegra since you
messed up headers.

This is not constructive feedback. Skip the assignment of blame and 
instead explain what the problem with headers is, that is the relevant 
part and that is missing here.