[PATCH v1 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.
Svyatoslav Ryhel (1): clk: clk-gpio: add actual gated clock
drivers/clk/clk-gpio.c | 47 +++++++++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 7 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 | 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; };
static int clk_gpio_enable(struct clk *clk) { struct clk_gpio_priv *priv = dev_get_priv(clk->dev);
+ clk_prepare_enable(priv->clk); 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; +} + 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_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); + 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,
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".
};
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.
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.
+}
- 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.
{ 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.
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
пт, 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.
};
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);elsereturn -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...
{ 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
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.
};
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.
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);elsereturn -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.
+}
- 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.
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
пт, 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);elsereturn -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
On 12/15/23 15:00, Svyatoslav Ryhel wrote:
пт, 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.
If you don't want to change the name, then add a doc comment.
}; 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.
There are many functions like this in U-Boot (e.g. kmalloc which takes an unused flags argument, or all the pr_* functions which are almost-but-not-quite the same as log_*). Not all of them are explicitly document as such, but generally you should use the native functions where it is not inconvenient.
--Sean
participants (2)
-
Sean Anderson -
Svyatoslav Ryhel