[U-Boot] [PATCH v1 0/7] clk: at91: Improve the clock implementation
Add the clock ops for such as spi0_clk, which is the real clock provider, instead of periph32ck, which only recursively bind its children as clk devices. Also update the clocks called in the drivers.
Wenyou Yang (7): clk: clk-uclass: Assign clk->dev before call .of_xlate clk: at91: Improve the clock implementation gpio: atmel_pio4: Remove unneccessary clock calling i2c: at91_i2c: Remove unneccessary clock calling i2c: at91_i2c: Change error return -ENODEV to -EINVAL usb: ehci-atmel: Remove unneccessary clock calling mmc: atmel_sdhci: Remove unneccessary clock calling
drivers/clk/at91/clk-generated.c | 141 ++++++++++++++++++++++++++++---------- drivers/clk/at91/clk-peripheral.c | 139 ++++++++++++++++++++++++++++++------- drivers/clk/at91/clk-system.c | 117 +++++++++++++++++++++++++------ drivers/clk/at91/pmc.c | 28 -------- drivers/clk/at91/pmc.h | 1 - drivers/clk/clk-uclass.c | 3 + drivers/gpio/atmel_pio4.c | 12 ---- drivers/i2c/at91_i2c.c | 18 +---- drivers/mmc/atmel_sdhci.c | 27 +------- drivers/usb/host/ehci-atmel.c | 15 ---- 10 files changed, 322 insertions(+), 179 deletions(-)
In order to make clk->dev available in ops->of_xlate() to get the clock ID from the 'reg' property of the clock node, assign the clk->dev before calling ops->of_xlate().
Signed-off-by: Wenyou Yang wenyou.yang@atmel.com ---
drivers/clk/clk-uclass.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 4d78e3f..4a3248b 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -80,6 +80,9 @@ int clk_get_by_index(struct udevice *dev, int index, struct clk *clk) __func__, ret); return ret; } + + clk->dev = dev_clk; + ops = clk_dev_ops(dev_clk);
if (ops->of_xlate)
On 09/12/2016 07:54 PM, Wenyou Yang wrote:
In order to make clk->dev available in ops->of_xlate() to get the clock ID from the 'reg' property of the clock node, assign the clk->dev before calling ops->of_xlate().
It does seem reasonable to me to allow using the same of_xlate implementation across multiple similar-but-different clock providers, and this change is required to allow that. I note that the reset and power domain uclasses already work this way. So, from that perspective, you get an ack.
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
@@ -80,6 +80,9 @@ int clk_get_by_index(struct udevice *dev, int index, struct clk *clk) __func__, ret); return ret; }
- clk->dev = dev_clk;
That assignment also happens in clk_request() itself. I'm tempted to say that we should modify clk_request() to remove the dev parameter, and remove the assignment of clk->dev. clk_request()'s documentation says:
- @clock: A pointer to a clock struct to initialize. The caller must
have already initialized any field in this struct which theclock provider uses to identify the clock.
... and since clk->dev is a field that is used to identify the clock, it really should be set already, thus removing the need for this function to set clk->dev.
However, I suppose we can clean that up later, so, Acked-by: Stephen Warren swarren@nvidia.com
For the peripheral clock, provide the clock ops for the clock provider, such as spi0_clk. The .of_xlate is to get the clk->id, the .enable is to enable the spi0 peripheral clock, the .get_rate is to get the clock frequency.
The driver for periph32ck node is responsible for recursively binding its children as clk devices, not provide the clock ops.
So do the generated clock and system clock.
Signed-off-by: Wenyou Yang wenyou.yang@atmel.com ---
drivers/clk/at91/clk-generated.c | 141 ++++++++++++++++++++++++++++---------- drivers/clk/at91/clk-peripheral.c | 139 ++++++++++++++++++++++++++++++------- drivers/clk/at91/clk-system.c | 117 +++++++++++++++++++++++++------ drivers/clk/at91/pmc.c | 28 -------- drivers/clk/at91/pmc.h | 1 - 5 files changed, 316 insertions(+), 110 deletions(-)
diff --git a/drivers/clk/at91/clk-generated.c b/drivers/clk/at91/clk-generated.c index f6164cc..923ffbb 100644 --- a/drivers/clk/at91/clk-generated.c +++ b/drivers/clk/at91/clk-generated.c @@ -8,22 +8,97 @@ #include <common.h> #include <clk-uclass.h> #include <dm/device.h> +#include <dm/lists.h> #include <linux/io.h> #include <mach/at91_pmc.h> -#include "pmc.h"
DECLARE_GLOBAL_DATA_PTR;
#define GENERATED_SOURCE_MAX 6 #define GENERATED_MAX_DIV 255
-struct generated_clk_priv { +/** + * generated_clk_bind() - for the generated clock driver + * Recursively bind its children as clk devices. + * + * @return: 0 on success, or negative error code on failure + */ +static int generated_clk_bind(struct udevice *dev) +{ + const void *fdt = gd->fdt_blob; + int offset = dev->of_offset; + bool pre_reloc_only = !(gd->flags & GD_FLG_RELOC); + const char *name; + int ret; + + for (offset = fdt_first_subnode(fdt, offset); + offset > 0; + offset = fdt_next_subnode(fdt, offset)) { + if (pre_reloc_only && + !fdt_getprop(fdt, offset, "u-boot,dm-pre-reloc", NULL)) + continue; + /* + * If this node has "compatible" property, this is not + * a pin configuration node, but a normal device. skip. + */ + fdt_get_property(fdt, offset, "compatible", &ret); + if (ret >= 0) + continue; + + if (ret != -FDT_ERR_NOTFOUND) + return ret; + + name = fdt_get_name(fdt, offset, NULL); + if (!name) + return -EINVAL; + ret = device_bind_driver_to_node(dev, "generic-clk", name, + offset, NULL); + if (ret) + return ret; + } + + return 0; +} + +static const struct udevice_id generated_clk_match[] = { + { .compatible = "atmel,sama5d2-clk-generated" }, + {} +}; + +U_BOOT_DRIVER(generated_clk) = { + .name = "generated-clk", + .id = UCLASS_CLK, + .of_match = generated_clk_match, + .bind = generated_clk_bind, +}; + +/*-------------------------------------------------------------*/ + +struct generic_clk_platdata { + struct at91_pmc *reg_base; +}; + +struct generic_clk_priv { u32 num_parents; };
-static ulong generated_clk_get_rate(struct clk *clk) +static int generic_clk_of_xlate(struct clk *clk, + struct fdtdec_phandle_args *args) { - struct pmc_platdata *plat = dev_get_platdata(clk->dev); + int periph; + + periph = fdtdec_get_uint(gd->fdt_blob, clk->dev->of_offset, "reg", -1); + if (periph < 0) + return -EINVAL; + + clk->id = periph; + + return 0; +} + +static ulong generic_clk_get_rate(struct clk *clk) +{ + struct generic_clk_platdata *plat = dev_get_platdata(clk->dev); struct at91_pmc *pmc = plat->reg_base; struct clk parent; u32 tmp, gckdiv; @@ -36,18 +111,18 @@ static ulong generated_clk_get_rate(struct clk *clk) AT91_PMC_PCR_GCKCSS_MASK; gckdiv = (tmp >> AT91_PMC_PCR_GCKDIV_OFFSET) & AT91_PMC_PCR_GCKDIV_MASK;
- ret = clk_get_by_index(clk->dev, parent_id, &parent); + ret = clk_get_by_index(dev_get_parent(clk->dev), parent_id, &parent); if (ret) return 0;
return clk_get_rate(&parent) / (gckdiv + 1); }
-static ulong generated_clk_set_rate(struct clk *clk, ulong rate) +static ulong generic_clk_set_rate(struct clk *clk, ulong rate) { - struct pmc_platdata *plat = dev_get_platdata(clk->dev); + struct generic_clk_platdata *plat = dev_get_platdata(clk->dev); struct at91_pmc *pmc = plat->reg_base; - struct generated_clk_priv *priv = dev_get_priv(clk->dev); + struct generic_clk_priv *priv = dev_get_priv(clk->dev); struct clk parent, best_parent; ulong tmp_rate, best_rate = rate, parent_rate; int tmp_diff, best_diff = -1; @@ -58,7 +133,7 @@ static ulong generated_clk_set_rate(struct clk *clk, ulong rate) int ret;
for (i = 0; i < priv->num_parents; i++) { - ret = clk_get_by_index(clk->dev, i, &parent); + ret = clk_get_by_index(dev_get_parent(clk->dev), i, &parent); if (ret) return ret;
@@ -111,18 +186,20 @@ static ulong generated_clk_set_rate(struct clk *clk, ulong rate) return 0; }
-static struct clk_ops generated_clk_ops = { - .get_rate = generated_clk_get_rate, - .set_rate = generated_clk_set_rate, +static struct clk_ops generic_clk_ops = { + .of_xlate = generic_clk_of_xlate, + .get_rate = generic_clk_get_rate, + .set_rate = generic_clk_set_rate, };
-static int generated_clk_ofdata_to_platdata(struct udevice *dev) +static int generic_clk_ofdata_to_platdata(struct udevice *dev) { - struct generated_clk_priv *priv = dev_get_priv(dev); + struct generic_clk_priv *priv = dev_get_priv(dev); u32 cells[GENERATED_SOURCE_MAX]; u32 num_parents;
- num_parents = fdtdec_get_int_array_count(gd->fdt_blob, dev->of_offset, + num_parents = fdtdec_get_int_array_count(gd->fdt_blob, + dev_get_parent(dev)->of_offset, "clocks", cells, GENERATED_SOURCE_MAX);
@@ -134,29 +211,23 @@ static int generated_clk_ofdata_to_platdata(struct udevice *dev) return 0; }
-static int generated_clk_bind(struct udevice *dev) +static int generic_clk_probe(struct udevice *dev) { - return at91_pmc_clk_node_bind(dev); -} + struct generic_clk_platdata *plat = dev_get_platdata(dev);
-static int generated_clk_probe(struct udevice *dev) -{ - return at91_pmc_core_probe(dev); -} + dev = dev_get_parent(dev); + dev = dev_get_parent(dev); + plat->reg_base = (struct at91_pmc *)dev_get_addr_ptr(dev);
-static const struct udevice_id generated_clk_match[] = { - { .compatible = "atmel,sama5d2-clk-generated" }, - {} -}; + return 0; +}
-U_BOOT_DRIVER(generated_clk) = { - .name = "generated-clk", +U_BOOT_DRIVER(generic_clk) = { + .name = "generic-clk", .id = UCLASS_CLK, - .of_match = generated_clk_match, - .bind = generated_clk_bind, - .probe = generated_clk_probe, - .ofdata_to_platdata = generated_clk_ofdata_to_platdata, - .priv_auto_alloc_size = sizeof(struct generated_clk_priv), - .platdata_auto_alloc_size = sizeof(struct pmc_platdata), - .ops = &generated_clk_ops, + .probe = generic_clk_probe, + .ofdata_to_platdata = generic_clk_ofdata_to_platdata, + .priv_auto_alloc_size = sizeof(struct generic_clk_priv), + .platdata_auto_alloc_size = sizeof(struct generic_clk_platdata), + .ops = &generic_clk_ops, }; diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c index 16688e9..a7091c1 100644 --- a/drivers/clk/at91/clk-peripheral.c +++ b/drivers/clk/at91/clk-peripheral.c @@ -8,17 +8,94 @@ #include <common.h> #include <clk-uclass.h> #include <dm/device.h> +#include <dm/lists.h> #include <linux/io.h> #include <mach/at91_pmc.h> -#include "pmc.h" + +DECLARE_GLOBAL_DATA_PTR;
#define PERIPHERAL_ID_MIN 2 #define PERIPHERAL_ID_MAX 31 #define PERIPHERAL_MASK(id) (1 << ((id) & PERIPHERAL_ID_MAX))
-static int sam9x5_periph_clk_enable(struct clk *clk) +struct periph_clk_platdata { + struct at91_pmc *reg_base; +}; + +/** + * sam9x5_periph_clk_bind() - for the periph clock driver + * Recursively bind its children as clk devices. + * + * @return: 0 on success, or negative error code on failure + */ +static int sam9x5_periph_clk_bind(struct udevice *dev) +{ + const void *fdt = gd->fdt_blob; + int offset = dev->of_offset; + bool pre_reloc_only = !(gd->flags & GD_FLG_RELOC); + const char *name; + int ret; + + for (offset = fdt_first_subnode(fdt, offset); + offset > 0; + offset = fdt_next_subnode(fdt, offset)) { + if (pre_reloc_only && + !fdt_getprop(fdt, offset, "u-boot,dm-pre-reloc", NULL)) + continue; + /* + * If this node has "compatible" property, this is not + * a pin configuration node, but a normal device. skip. + */ + fdt_get_property(fdt, offset, "compatible", &ret); + if (ret >= 0) + continue; + + if (ret != -FDT_ERR_NOTFOUND) + return ret; + + name = fdt_get_name(fdt, offset, NULL); + if (!name) + return -EINVAL; + ret = device_bind_driver_to_node(dev, "periph-clk", name, + offset, NULL); + if (ret) + return ret; + } + + return 0; +} + +static const struct udevice_id sam9x5_periph_clk_match[] = { + { .compatible = "atmel,at91sam9x5-clk-peripheral" }, + {} +}; + +U_BOOT_DRIVER(sam9x5_periph_clk) = { + .name = "sam9x5-periph-clk", + .id = UCLASS_CLK, + .of_match = sam9x5_periph_clk_match, + .bind = sam9x5_periph_clk_bind, +}; + +/*---------------------------------------------------------*/ + +static int periph_clk_of_xlate(struct clk *clk, + struct fdtdec_phandle_args *args) { - struct pmc_platdata *plat = dev_get_platdata(clk->dev); + int periph; + + periph = fdtdec_get_uint(gd->fdt_blob, clk->dev->of_offset, "reg", -1); + if (periph < 0) + return -EINVAL; + + clk->id = periph; + + return 0; +} + +static int periph_clk_enable(struct clk *clk) +{ + struct periph_clk_platdata *plat = dev_get_platdata(clk->dev); struct at91_pmc *pmc = plat->reg_base;
if (clk->id < PERIPHERAL_ID_MIN) @@ -30,31 +107,45 @@ static int sam9x5_periph_clk_enable(struct clk *clk) return 0; }
-static struct clk_ops sam9x5_periph_clk_ops = { - .enable = sam9x5_periph_clk_enable, -}; - -static int sam9x5_periph_clk_bind(struct udevice *dev) +static ulong periph_get_rate(struct clk *clk) { - return at91_pmc_clk_node_bind(dev); -} + struct udevice *dev; + struct clk clk_dev; + int ret;
-static int sam9x5_periph_clk_probe(struct udevice *dev) -{ - return at91_pmc_core_probe(dev); + dev = dev_get_parent(clk->dev); + ret = clk_request(dev, &clk_dev); + if (ret) + return ret; + + ret = clk_get_by_index(dev, 0, &clk_dev); + if (ret) + return ret; + + return clk_get_rate(&clk_dev); }
-static const struct udevice_id sam9x5_periph_clk_match[] = { - { .compatible = "atmel,at91sam9x5-clk-peripheral" }, - {} +static struct clk_ops periph_clk_ops = { + .of_xlate = periph_clk_of_xlate, + .enable = periph_clk_enable, + .get_rate = periph_get_rate, };
-U_BOOT_DRIVER(sam9x5_periph_clk) = { - .name = "sam9x5-periph-clk", - .id = UCLASS_CLK, - .of_match = sam9x5_periph_clk_match, - .bind = sam9x5_periph_clk_bind, - .probe = sam9x5_periph_clk_probe, - .platdata_auto_alloc_size = sizeof(struct pmc_platdata), - .ops = &sam9x5_periph_clk_ops, +static int periph_clk_probe(struct udevice *dev) +{ + struct periph_clk_platdata *plat = dev_get_platdata(dev); + + dev = dev_get_parent(dev); + dev = dev_get_parent(dev); + plat->reg_base = (struct at91_pmc *)dev_get_addr_ptr(dev); + + return 0; +} + +U_BOOT_DRIVER(clk_periph) = { + .name = "periph-clk", + .id = UCLASS_CLK, + .platdata_auto_alloc_size = sizeof(struct periph_clk_platdata), + .probe = periph_clk_probe, + .ops = &periph_clk_ops, }; diff --git a/drivers/clk/at91/clk-system.c b/drivers/clk/at91/clk-system.c index fa80bad..149f45c 100644 --- a/drivers/clk/at91/clk-system.c +++ b/drivers/clk/at91/clk-system.c @@ -8,20 +8,98 @@ #include <common.h> #include <clk-uclass.h> #include <dm/device.h> +#include <dm/lists.h> #include <linux/io.h> #include <mach/at91_pmc.h> #include "pmc.h"
+DECLARE_GLOBAL_DATA_PTR; + #define SYSTEM_MAX_ID 31
+/** + * at91_system_clk_bind() - for the system clock driver + * Recursively bind its children as clk devices. + * + * @return: 0 on success, or negative error code on failure + */ +static int at91_system_clk_bind(struct udevice *dev) +{ + const void *fdt = gd->fdt_blob; + int offset = dev->of_offset; + bool pre_reloc_only = !(gd->flags & GD_FLG_RELOC); + const char *name; + int ret; + + for (offset = fdt_first_subnode(fdt, offset); + offset > 0; + offset = fdt_next_subnode(fdt, offset)) { + if (pre_reloc_only && + !fdt_getprop(fdt, offset, "u-boot,dm-pre-reloc", NULL)) + continue; + /* + * If this node has "compatible" property, this is not + * a pin configuration node, but a normal device. skip. + */ + fdt_get_property(fdt, offset, "compatible", &ret); + if (ret >= 0) + continue; + + if (ret != -FDT_ERR_NOTFOUND) + return ret; + + name = fdt_get_name(fdt, offset, NULL); + if (!name) + return -EINVAL; + ret = device_bind_driver_to_node(dev, "system-clk", name, + offset, NULL); + if (ret) + return ret; + } + + return 0; +} + +static const struct udevice_id at91_system_clk_match[] = { + { .compatible = "atmel,at91rm9200-clk-system" }, + {} +}; + +U_BOOT_DRIVER(at91_system_clk) = { + .name = "at91-system-clk", + .id = UCLASS_CLK, + .of_match = at91_system_clk_match, + .bind = at91_system_clk_bind, +}; + +/*----------------------------------------------------------*/ + +struct system_clk_platdata { + struct at91_pmc *reg_base; +}; + +static int system_clk_of_xlate(struct clk *clk, + struct fdtdec_phandle_args *args) +{ + int periph; + + periph = fdtdec_get_uint(gd->fdt_blob, clk->dev->of_offset, "reg", -1); + if (periph < 0) + return -EINVAL; + + clk->id = periph; + + return 0; +} + static inline int is_pck(int id) { return (id >= 8) && (id <= 15); }
-static int at91_system_clk_enable(struct clk *clk) +static int system_clk_enable(struct clk *clk) { - struct pmc_platdata *plat = dev_get_platdata(clk->dev); + struct system_clk_platdata *plat = dev_get_platdata(clk->dev); struct at91_pmc *pmc = plat->reg_base; u32 mask;
@@ -46,31 +124,26 @@ static int at91_system_clk_enable(struct clk *clk) return 0; }
-static struct clk_ops at91_system_clk_ops = { - .enable = at91_system_clk_enable, +static struct clk_ops system_clk_ops = { + .of_xlate = system_clk_of_xlate, + .enable = system_clk_enable, };
-static int at91_system_clk_bind(struct udevice *dev) +static int system_clk_probe(struct udevice *dev) { - return at91_pmc_clk_node_bind(dev); -} + struct system_clk_platdata *plat = dev_get_platdata(dev);
-static int at91_system_clk_probe(struct udevice *dev) -{ - return at91_pmc_core_probe(dev); -} + dev = dev_get_parent(dev); + dev = dev_get_parent(dev); + plat->reg_base = (struct at91_pmc *)dev_get_addr_ptr(dev);
-static const struct udevice_id at91_system_clk_match[] = { - { .compatible = "atmel,at91rm9200-clk-system" }, - {} -}; + return 0; +}
-U_BOOT_DRIVER(at91_system_clk) = { - .name = "at91-system-clk", +U_BOOT_DRIVER(system_clk) = { + .name = "system-clk", .id = UCLASS_CLK, - .of_match = at91_system_clk_match, - .bind = at91_system_clk_bind, - .probe = at91_system_clk_probe, - .platdata_auto_alloc_size = sizeof(struct pmc_platdata), - .ops = &at91_system_clk_ops, + .probe = system_clk_probe, + .platdata_auto_alloc_size = sizeof(struct system_clk_platdata), + .ops = &system_clk_ops, }; diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c index 76ff387..b666caf 100644 --- a/drivers/clk/at91/pmc.c +++ b/drivers/clk/at91/pmc.c @@ -35,31 +35,3 @@ int at91_pmc_core_probe(struct udevice *dev)
return 0; } - -int at91_pmc_clk_node_bind(struct udevice *dev) -{ - const void *fdt = gd->fdt_blob; - int offset = dev->of_offset; - const char *name; - int ret; - - for (offset = fdt_first_subnode(fdt, offset); - offset > 0; - offset = fdt_next_subnode(fdt, offset)) { - name = fdt_get_name(fdt, offset, NULL); - if (!name) - return -EINVAL; - - ret = device_bind_driver_to_node(dev, "clk", name, - offset, NULL); - if (ret) - return ret; - } - - return 0; -} - -U_BOOT_DRIVER(clk_generic) = { - .id = UCLASS_CLK, - .name = "clk", -}; diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h index 5444c84..201dad8 100644 --- a/drivers/clk/at91/pmc.h +++ b/drivers/clk/at91/pmc.h @@ -13,6 +13,5 @@ struct pmc_platdata { };
int at91_pmc_core_probe(struct udevice *dev); -int at91_pmc_clk_node_bind(struct udevice *dev);
#endif
On 09/12/2016 07:54 PM, Wenyou Yang wrote:
For the peripheral clock, provide the clock ops for the clock provider, such as spi0_clk. The .of_xlate is to get the clk->id, the .enable is to enable the spi0 peripheral clock, the .get_rate is to get the clock frequency.
The driver for periph32ck node is responsible for recursively binding its children as clk devices, not provide the clock ops.
So do the generated clock and system clock.
diff --git a/drivers/clk/at91/clk-generated.c b/drivers/clk/at91/clk-generated.c
+/**
- generated_clk_bind() - for the generated clock driver
- Recursively bind its children as clk devices.
- @return: 0 on success, or negative error code on failure
- */
+static int generated_clk_bind(struct udevice *dev)
... (code to enumerate/instantiate all child DT nodes)
+}
+static const struct udevice_id generated_clk_match[] = {
- { .compatible = "atmel,sama5d2-clk-generated" },
- {}
+};
+U_BOOT_DRIVER(generated_clk) = {
- .name = "generated-clk",
- .id = UCLASS_CLK,
- .of_match = generated_clk_match,
- .bind = generated_clk_bind,
+};
This driver shouldn't be UCLASS_CLK, and can't be without any clock ops implemented. It appears to be for another DT node that solely exists to house various sub-nodes which are the actual clock providers. I believe you should make this UCLASS_MISC. I guess you need to keep the custom bind function, since all the child nodes that it enumerates explicitly don't have a compatible value, so you can't rely on e.g. the default UCLASS_SIMPLE_BUS bind function to enumerate them.
BTW, while reading ./arch/arm/dts/sama5d2.dtsi, I noticed:
sdmmc0_gclk: sdmmc0_gclk { #clock-cells = <0>; reg = <31>; };
Doesn't dtc complain about that? The node has a reg property, yet there is no unit address in the node name to match it. Instead, that node should be:
sdmmc0_gclk: sdmmc0_gclk@31 { #clock-cells = <0>; reg = <31>; };
+static int generic_clk_of_xlate(struct clk *clk,
struct fdtdec_phandle_args *args){
- struct pmc_platdata *plat = dev_get_platdata(clk->dev);
- int periph;
The following should likely be added here:
if (args->args_count) { debug("Invaild args_count: %d\n", args->args_count); return -EINVAL; }
- periph = fdtdec_get_uint(gd->fdt_blob, clk->dev->of_offset, "reg", -1);
- if (periph < 0)
return -EINVAL;- clk->id = periph;
I guess that's OK. of_xlate is really intended to parse/process the clocks property in the client DT node. However, I suppose parsing the referenced DT node is probably OK too. Has this Atmel clock binding, and a similar clock driver implementation (including custom of_xlate) been reviewed for inclusion in the Linux kernel? I'd be curious if the same technique was/wasn't allowed there.
+static int generic_clk_probe(struct udevice *dev)
...
- dev = dev_get_parent(dev);
- dev = dev_get_parent(dev);
That line is duplicated.
diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
The same comments as above all apply to this file too.
+static ulong periph_get_rate(struct clk *clk) {
struct udevice *dev;
struct clk clk_dev;
int ret;
dev = dev_get_parent(clk->dev);
ret = clk_request(dev, &clk_dev);
You haven't filled in clk_dev.id here. That needs to be filled in before calling clk_request().
- if (ret)
return ret;- ret = clk_get_by_index(dev, 0, &clk_dev);
This over-writes everything in clk_dev, thus making the call to clk_request() above pointless.
- if (ret)
return ret;- return clk_get_rate(&clk_dev);
You need to call clk_free(&clk_dev) before returning. This comment applies to generated_clk_get_rate()/generic_clk_get_rate() too (in the previous file), although that problem existed before this patch.
+static int periph_clk_probe(struct udevice *dev) +{
- struct periph_clk_platdata *plat = dev_get_platdata(dev);
- dev = dev_get_parent(dev);
- dev = dev_get_parent(dev);
- plat->reg_base = (struct at91_pmc *)dev_get_addr_ptr(dev);
It would be helpful documentation-wise to use separate variables for those parents; make the variable name indicate what the parent is expected to be:
dev_periph_container = dev_get_parent(dev); dev_pmc = dev_get_parent(dev_periph_container);
diff --git a/drivers/clk/at91/clk-system.c b/drivers/clk/at91/clk-system.c
Likewise, all the same comments above apply to this file too.
+/**
- at91_system_clk_bind() - for the system clock driver
- Recursively bind its children as clk devices.
- @return: 0 on success, or negative error code on failure
- */
+static int at91_system_clk_bind(struct udevice *dev) +{
...
/** If this node has "compatible" property, this is not* a pin configuration node, but a normal device. skip.*/
This is nothing to do with pin configuration, but clocks.
This function is duplicated at least 3 times in this patch. It might be useful to implement it in some common location, and just call it from all the different clock drivers. You'd need to pass in a string for the driver name to bind to, but that's the only difference I see.
I wouldn't be surprised if other parts of the Atmel clock drivers could be shared too; of_xlate is probably common, and perhaps get_rate for some clocks that are derived from parent clocks. Still, cleaning that up might be a job for a separate series.
Hi Stephen,
Thank you for your review, and detailed comments.
The version 2 has been sent out, please give your advices.
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: 2016年9月14日 3:34 To: Wenyou Yang - A41535 Wenyou.Yang@microchip.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Stephen Warren swarren@nvidia.com Subject: Re: [U-Boot] [PATCH v1 2/7] clk: at91: Improve the clock implementation
On 09/12/2016 07:54 PM, Wenyou Yang wrote:
For the peripheral clock, provide the clock ops for the clock provider, such as spi0_clk. The .of_xlate is to get the clk->id, the .enable is to enable the spi0 peripheral clock, the .get_rate is to get the clock frequency.
The driver for periph32ck node is responsible for recursively binding its children as clk devices, not provide the clock ops.
So do the generated clock and system clock.
diff --git a/drivers/clk/at91/clk-generated.c b/drivers/clk/at91/clk-generated.c
+/**
- generated_clk_bind() - for the generated clock driver
- Recursively bind its children as clk devices.
- @return: 0 on success, or negative error code on failure */
+static int generated_clk_bind(struct udevice *dev)
... (code to enumerate/instantiate all child DT nodes)
+}
+static const struct udevice_id generated_clk_match[] = {
- { .compatible = "atmel,sama5d2-clk-generated" },
- {}
+};
+U_BOOT_DRIVER(generated_clk) = {
- .name = "generated-clk",
- .id = UCLASS_CLK,
- .of_match = generated_clk_match,
- .bind = generated_clk_bind,
+};
This driver shouldn't be UCLASS_CLK, and can't be without any clock ops implemented. It appears to be for another DT node that solely exists to house various sub-nodes which are the actual clock providers. I believe you should make this UCLASS_MISC. I guess you need to keep the custom bind function, since all the child nodes that it enumerates explicitly don't have a compatible value, so you can't rely on e.g. the default UCLASS_SIMPLE_BUS bind function to enumerate them.
BTW, while reading ./arch/arm/dts/sama5d2.dtsi, I noticed:
sdmmc0_gclk: sdmmc0_gclk { #clock-cells = <0>; reg = <31>; };
Doesn't dtc complain about that? The node has a reg property, yet there is no unit address in the node name to match it. Instead, that node should be:
Yes, there are dtc complains like this,
"Warning (unit_address_vs_reg): Node /ahb/apb/pmc@f0014000/periph64ck/sdmmc0_hclk has a reg or ranges property, but no unit name"
I will send a patches to fix this warning.
Others are accepted too.
sdmmc0_gclk: sdmmc0_gclk@31 { #clock-cells = <0>; reg = <31>; };
+static int generic_clk_of_xlate(struct clk *clk,
struct fdtdec_phandle_args *args){
- struct pmc_platdata *plat = dev_get_platdata(clk->dev);
- int periph;
The following should likely be added here:
if (args->args_count) { debug("Invaild args_count: %d\n", args->args_count); return -EINVAL; }
- periph = fdtdec_get_uint(gd->fdt_blob, clk->dev->of_offset, "reg", -1);
- if (periph < 0)
return -EINVAL;- clk->id = periph;
I guess that's OK. of_xlate is really intended to parse/process the clocks property in the client DT node. However, I suppose parsing the referenced DT node is probably OK too. Has this Atmel clock binding, and a similar clock driver implementation (including custom of_xlate) been reviewed for inclusion in the Linux kernel? I'd be curious if the same technique was/wasn't allowed there.
+static int generic_clk_probe(struct udevice *dev)
...
- dev = dev_get_parent(dev);
- dev = dev_get_parent(dev);
That line is duplicated.
diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
The same comments as above all apply to this file too.
+static ulong periph_get_rate(struct clk *clk) {
struct udevice *dev;
struct clk clk_dev;
int ret;
dev = dev_get_parent(clk->dev);
ret = clk_request(dev, &clk_dev);
You haven't filled in clk_dev.id here. That needs to be filled in before calling clk_request().
- if (ret)
return ret;- ret = clk_get_by_index(dev, 0, &clk_dev);
This over-writes everything in clk_dev, thus making the call to clk_request() above pointless.
- if (ret)
return ret;- return clk_get_rate(&clk_dev);
You need to call clk_free(&clk_dev) before returning. This comment applies to generated_clk_get_rate()/generic_clk_get_rate() too (in the previous file), although that problem existed before this patch.
+static int periph_clk_probe(struct udevice *dev) {
- struct periph_clk_platdata *plat = dev_get_platdata(dev);
- dev = dev_get_parent(dev);
- dev = dev_get_parent(dev);
- plat->reg_base = (struct at91_pmc *)dev_get_addr_ptr(dev);
It would be helpful documentation-wise to use separate variables for those parents; make the variable name indicate what the parent is expected to be:
dev_periph_container = dev_get_parent(dev); dev_pmc = dev_get_parent(dev_periph_container);
diff --git a/drivers/clk/at91/clk-system.c b/drivers/clk/at91/clk-system.c
Likewise, all the same comments above apply to this file too.
+/**
- at91_system_clk_bind() - for the system clock driver
- Recursively bind its children as clk devices.
- @return: 0 on success, or negative error code on failure */
+static int at91_system_clk_bind(struct udevice *dev) {
...
/** If this node has "compatible" property, this is not* a pin configuration node, but a normal device. skip.*/This is nothing to do with pin configuration, but clocks.
This function is duplicated at least 3 times in this patch. It might be useful to implement it in some common location, and just call it from all the different clock drivers. You'd need to pass in a string for the driver name to bind to, but that's the only difference I see.
I wouldn't be surprised if other parts of the Atmel clock drivers could be shared too; of_xlate is probably common, and perhaps get_rate for some clocks that are derived from parent clocks. Still, cleaning that up might be a job for a separate series.
Best Regards, Wenyou Yang
Due to the peripheral clock driver improvement, remove the unneccessary clock calling.
Signed-off-by: Wenyou Yang wenyou.yang@atmel.com ---
drivers/gpio/atmel_pio4.c | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/drivers/gpio/atmel_pio4.c b/drivers/gpio/atmel_pio4.c index 7adea88..cb90b02 100644 --- a/drivers/gpio/atmel_pio4.c +++ b/drivers/gpio/atmel_pio4.c @@ -284,27 +284,15 @@ static int atmel_pio4_probe(struct udevice *dev) struct atmel_pio4_platdata *plat = dev_get_platdata(dev); struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev); struct atmel_pioctrl_data *pioctrl_data; - struct udevice *dev_clk; struct clk clk; fdt_addr_t addr_base; u32 nbanks; - int periph; int ret;
ret = clk_get_by_index(dev, 0, &clk); if (ret) return ret;
- periph = fdtdec_get_uint(gd->fdt_blob, clk.dev->of_offset, "reg", -1); - if (periph < 0) - return -EINVAL; - - dev_clk = dev_get_parent(clk.dev); - ret = clk_request(dev_clk, &clk); - if (ret) - return ret; - - clk.id = periph; ret = clk_enable(&clk); if (ret) return ret;
On 09/12/2016 07:54 PM, Wenyou Yang wrote:
Due to the peripheral clock driver improvement, remove the unneccessary clock calling.
Patches 3-7, Acked-by: Stephen Warren swarren@nvidia.com
Due to the peripheral clock driver improvement, remove the unneccessary clock calling.
Signed-off-by: Wenyou Yang wenyou.yang@atmel.com ---
drivers/i2c/at91_i2c.c | 16 ---------------- 1 file changed, 16 deletions(-)
diff --git a/drivers/i2c/at91_i2c.c b/drivers/i2c/at91_i2c.c index 8e9c3ad..73f29e3 100644 --- a/drivers/i2c/at91_i2c.c +++ b/drivers/i2c/at91_i2c.c @@ -176,34 +176,18 @@ static void at91_calc_i2c_clock(struct udevice *dev, int i2c_clk) static int at91_i2c_enable_clk(struct udevice *dev) { struct at91_i2c_bus *bus = dev_get_priv(dev); - struct udevice *dev_clk; struct clk clk; ulong clk_rate; - int periph; int ret;
ret = clk_get_by_index(dev, 0, &clk); if (ret) return -EINVAL;
- periph = fdtdec_get_uint(gd->fdt_blob, clk.dev->of_offset, "reg", -1); - if (periph < 0) - return -EINVAL; - - dev_clk = dev_get_parent(clk.dev); - ret = clk_request(dev_clk, &clk); - if (ret) - return ret; - - clk.id = periph; ret = clk_enable(&clk); if (ret) return ret;
- ret = clk_get_by_index(dev_clk, 0, &clk); - if (ret) - return ret; - clk_rate = clk_get_rate(&clk); if (!clk_rate) return -ENODEV;
Change the error return value -ENODEV from to -EINVAL for more reasonable.
Signed-off-by: Wenyou Yang wenyou.yang@atmel.com ---
drivers/i2c/at91_i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/at91_i2c.c b/drivers/i2c/at91_i2c.c index 73f29e3..472420b 100644 --- a/drivers/i2c/at91_i2c.c +++ b/drivers/i2c/at91_i2c.c @@ -190,7 +190,7 @@ static int at91_i2c_enable_clk(struct udevice *dev)
clk_rate = clk_get_rate(&clk); if (!clk_rate) - return -ENODEV; + return -EINVAL;
bus->bus_clk_rate = clk_rate;
Due to the peripheral clock driver improvement, remove the unneccessary clock calling.
Signed-off-by: Wenyou Yang wenyou.yang@atmel.com ---
drivers/usb/host/ehci-atmel.c | 15 --------------- 1 file changed, 15 deletions(-)
diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c index d65bbe9..6cb5286 100644 --- a/drivers/usb/host/ehci-atmel.c +++ b/drivers/usb/host/ehci-atmel.c @@ -56,9 +56,7 @@ struct ehci_atmel_priv {
static int ehci_atmel_enable_clk(struct udevice *dev) { - struct udevice *dev_clk; struct clk clk; - int periph; int ret;
ret = clk_get_by_index(dev, 0, &clk); @@ -73,19 +71,6 @@ static int ehci_atmel_enable_clk(struct udevice *dev) if (ret) return -EINVAL;
- periph = fdtdec_get_uint(gd->fdt_blob, clk.dev->of_offset, "reg", -1); - if (periph < 0) - return -EINVAL; - - dev_clk = dev_get_parent(clk.dev); - if (!dev_clk) - return -ENODEV; - - ret = clk_request(dev_clk, &clk); - if (ret) - return ret; - - clk.id = periph; ret = clk_enable(&clk); if (ret) return ret;
Due to the peripheral and generated clock driver improvement, remove the unneccessary clock calling.
Signed-off-by: Wenyou Yang wenyou.yang@atmel.com ---
drivers/mmc/atmel_sdhci.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/mmc/atmel_sdhci.c b/drivers/mmc/atmel_sdhci.c index dd6bd33..437a652 100644 --- a/drivers/mmc/atmel_sdhci.c +++ b/drivers/mmc/atmel_sdhci.c @@ -51,29 +51,6 @@ struct atmel_sdhci_plat { struct mmc mmc; };
-static int atmel_sdhci_get_clk(struct udevice *dev, int index, struct clk *clk) -{ - struct udevice *dev_clk; - int periph, ret; - - ret = clk_get_by_index(dev, index, clk); - if (ret) - return ret; - - periph = fdtdec_get_uint(gd->fdt_blob, clk->dev->of_offset, "reg", -1); - if (periph < 0) - return -EINVAL; - - dev_clk = dev_get_parent(clk->dev); - ret = clk_request(dev_clk, clk); - if (ret) - return ret; - - clk->id = periph; - - return 0; -} - static int atmel_sdhci_probe(struct udevice *dev) { struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); @@ -86,7 +63,7 @@ static int atmel_sdhci_probe(struct udevice *dev) struct clk clk; int ret;
- ret = atmel_sdhci_get_clk(dev, 0, &clk); + ret = clk_get_by_index(dev, 0, &clk); if (ret) return ret;
@@ -107,7 +84,7 @@ static int atmel_sdhci_probe(struct udevice *dev) clk_mul = (caps_1 & SDHCI_CLOCK_MUL_MASK) >> SDHCI_CLOCK_MUL_SHIFT; gck_rate = clk_base * 1000000 * (clk_mul + 1);
- ret = atmel_sdhci_get_clk(dev, 1, &clk); + ret = clk_get_by_index(dev, 1, &clk); if (ret) return ret;
participants (3)
-
Stephen Warren -
Wenyou Yang -
Wenyou.Yang＠microchip.com