[PATCH v2 0/1] Fix clk-gpio driver
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.
Testing current driver implementation shows that it is not fully capable and lacks gated clock manipulations.
--- Changes from v1 - added comments for priv struct members - return gatet clock rate unconditionally - switch log_err > log_debug on of_to_plat ---
Svyatoslav Ryhel (1): clk: clk-gpio: add actual gated clock
drivers/clk/clk-gpio.c | 44 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)
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 | 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+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 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; }
/* @@ -59,7 +87,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,
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 clamor95@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+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 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; }
/*
@@ -59,7 +87,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,
+CC Marek
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 clamor95@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+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 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.
сб, 16 груд. 2023 р. о 18:52 Marek Vasut marex@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 clamor95@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+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 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.
On 12/16/23 17:55, Svyatoslav Ryhel wrote:
сб, 16 груд. 2023 р. о 18:52 Marek Vasut marex@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 clamor95@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+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 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.
On 12/16/23 11:52, Marek Vasut wrote:
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 clamor95@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+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 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.
Only CCF forwards things. It's up to the driver to do it in the non-CCF case.
--Sean
On 12/16/23 17:56, Sean Anderson wrote:
On 12/16/23 11:52, Marek Vasut wrote:
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 clamor95@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+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 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.
Only CCF forwards things. It's up to the driver to do it in the non-CCF case.
Shall we add dependency on CCF instead of duplicating code in drivers ?
On 12/16/23 19:19, Marek Vasut wrote:
On 12/16/23 17:56, Sean Anderson wrote:
On 12/16/23 11:52, Marek Vasut wrote:
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 clamor95@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+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 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.
Only CCF forwards things. It's up to the driver to do it in the non-CCF case.
Shall we add dependency on CCF instead of duplicating code in drivers ?
I'd rather not. This will be useful on non-CCF systems.
--Sean
On 12/17/23 02:20, Sean Anderson wrote:
On 12/16/23 19:19, Marek Vasut wrote:
On 12/16/23 17:56, Sean Anderson wrote:
On 12/16/23 11:52, Marek Vasut wrote:
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 clamor95@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+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 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.
Only CCF forwards things. It's up to the driver to do it in the non-CCF case.
Shall we add dependency on CCF instead of duplicating code in drivers ?
I'd rather not. This will be useful on non-CCF systems.
Then the forwarding functionality should be in the clock core, basically if a driver does not implement callback <something>, call parent clock callback <something>, and repeat until you reach the top of the clock tree. If you do reach the top, fail with some error code. Isn't that how clock enable already works anyway ?
On 12/17/23 10:19, Marek Vasut wrote:
On 12/17/23 02:20, Sean Anderson wrote:
On 12/16/23 19:19, Marek Vasut wrote:
On 12/16/23 17:56, Sean Anderson wrote:
On 12/16/23 11:52, Marek Vasut wrote:
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 clamor95@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+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 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.
Only CCF forwards things. It's up to the driver to do it in the non-CCF case.
Shall we add dependency on CCF instead of duplicating code in drivers ?
I'd rather not. This will be useful on non-CCF systems.
Then the forwarding functionality should be in the clock core, basically if a driver does not implement callback <something>, call parent clock callback <something>, and repeat until you reach the top of the clock tree. If you do reach the top, fail with some error code. Isn't that how clock enable already works anyway ?
No. In the non-CCF case, we have no idea what the parent of any given clock is. So there's no framework we can provide.
Now, you might say "why not add a get_parent op?" Well, that will work fine for enabling things, but it is a bit hairy on the disable side. You really need to have refcounting to protect parent clocks (which can have many children). But we don't really have anywhere to store refcounts. struct clk has a member for it, but that's only because it's abused as private data by CCF. Non-CCF clocks have no clock subsystem data allocated at all. Another alternative is to just only disable the leaf node, which is what we currently do. That's OK I think.
But that solution requires some more-intensive effort which I don't want to impose on random contributors. So for the time being, all non-CCF clock drivers must handle enabling their own parent clocks.
--Sean
On 12/17/23 16:37, Sean Anderson wrote:
On 12/17/23 10:19, Marek Vasut wrote:
On 12/17/23 02:20, Sean Anderson wrote:
On 12/16/23 19:19, Marek Vasut wrote:
On 12/16/23 17:56, Sean Anderson wrote:
On 12/16/23 11:52, Marek Vasut wrote:
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 clamor95@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+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 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.
Only CCF forwards things. It's up to the driver to do it in the non-CCF case.
Shall we add dependency on CCF instead of duplicating code in drivers ?
I'd rather not. This will be useful on non-CCF systems.
Then the forwarding functionality should be in the clock core, basically if a driver does not implement callback <something>, call parent clock callback <something>, and repeat until you reach the top of the clock tree. If you do reach the top, fail with some error code. Isn't that how clock enable already works anyway ?
No. In the non-CCF case, we have no idea what the parent of any given clock is. So there's no framework we can provide.
Huh, but this driver is attempting to grab some sort of parent clock and enable them, so how does that work ?
Now, you might say "why not add a get_parent op?" Well, that will work fine for enabling things, but it is a bit hairy on the disable side. You really need to have refcounting to protect parent clocks (which can have many children). But we don't really have anywhere to store refcounts. struct clk has a member for it, but that's only because it's abused as private data by CCF. Non-CCF clocks have no clock subsystem data allocated at all. Another alternative is to just only disable the leaf node, which is what we currently do. That's OK I think.
But that solution requires some more-intensive effort which I don't want to impose on random contributors. So for the time being, all non-CCF clock drivers must handle enabling their own parent clocks.
So, in light of this, why not push for use of CCF as much as possible and deprecate the current non-CCF clock support ? It seems like the right thing to do, and avoids growing ad-hoc workarounds for missing core functionality in drivers, which I really want to avoid .
On 12/17/23 11:45, Marek Vasut wrote:
On 12/17/23 16:37, Sean Anderson wrote:
On 12/17/23 10:19, Marek Vasut wrote:
On 12/17/23 02:20, Sean Anderson wrote:
On 12/16/23 19:19, Marek Vasut wrote:
On 12/16/23 17:56, Sean Anderson wrote:
On 12/16/23 11:52, Marek Vasut wrote: > 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 clamor95@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+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 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.
Only CCF forwards things. It's up to the driver to do it in the non-CCF case.
Shall we add dependency on CCF instead of duplicating code in drivers ?
I'd rather not. This will be useful on non-CCF systems.
Then the forwarding functionality should be in the clock core, basically if a driver does not implement callback <something>, call parent clock callback <something>, and repeat until you reach the top of the clock tree. If you do reach the top, fail with some error code. Isn't that how clock enable already works anyway ?
No. In the non-CCF case, we have no idea what the parent of any given clock is. So there's no framework we can provide.
Huh, but this driver is attempting to grab some sort of parent clock and enable them, so how does that work ?
The driver knows what its parent is, so it can enable it just fine.
Now, you might say "why not add a get_parent op?" Well, that will work fine for enabling things, but it is a bit hairy on the disable side. You really need to have refcounting to protect parent clocks (which can have many children). But we don't really have anywhere to store refcounts. struct clk has a member for it, but that's only because it's abused as private data by CCF. Non-CCF clocks have no clock subsystem data allocated at all. Another alternative is to just only disable the leaf node, which is what we currently do. That's OK I think.
But that solution requires some more-intensive effort which I don't want to impose on random contributors. So for the time being, all non-CCF clock drivers must handle enabling their own parent clocks.
So, in light of this, why not push for use of CCF as much as possible and deprecate the current non-CCF clock support ? It seems like the right thing to do, and avoids growing ad-hoc workarounds for missing core functionality in drivers, which I really want to avoid .
Because the CCF framework abuses struct clk for private data, uses strings everywhere, and dynamically allocates all its clocks (which are known at compile-time). You can easily save 4K (or more) by moving away from CCF, which can be especially important in SPL. It's a necessary evil because people want to copy their clock drivers straight from Linux, but IMO it is a step in the wrong direction right now. Especially due to the dynamic allocation and strings I think it is not fixable in the general sense. But the "core" also needs some refactoring (in particular to fix the abuse of struct clk).
Working on this has been on my TODO for a long time, but I have not gotten around to it.
--Sean
On 12/16/23 10: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 clamor95@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+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 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)
Same comment as the first time:
Why the conversion from probe to of_to_plat?
--Sean
{ 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; } /* @@ -59,7 +87,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,
+CC Marek
10 січня 2024 р. 17:45:57 GMT+02:00, Sean Anderson seanga2@gmail.com написав(-ла):
On 12/16/23 10: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 clamor95@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+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 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)
Same comment as the first time:
Why the conversion from probe to of_to_plat?
Same answer as the first time. You propose to spam commits for this small adjustment?
--Sean
{ 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; } /* @@ -59,7 +87,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,
+CC Marek
On 1/10/24 10:53, Svyatoslav wrote:
10 січня 2024 р. 17:45:57 GMT+02:00, Sean Anderson seanga2@gmail.com написав(-ла):
On 12/16/23 10: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 clamor95@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+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 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)
Same comment as the first time:
Why the conversion from probe to of_to_plat?
Same answer as the first time.
Last time you said
-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...
and I thought "This actually should not be accepted first place" was referring to the conversion (i.e. that you would be removing it next time).
This sort of conversion should be mentioned in the commit message.
And the weird thing to me is that I would expect of_to_plat to fill in a platdata struct. If you are not doing that, why use this function?
You propose to spam commits for this small adjustment?
Yes. One change per commit.
--Sean
participants (4)
-
Marek Vasut -
Sean Anderson -
Svyatoslav -
Svyatoslav Ryhel