[PATCH v5 0/8] clk: Switch from soc_clk_dump to clk_ops function

Currently clock providers may override default implementation of soc_clk_dump function to replace clk dump command output. This causes confusing behaviour when u-boot is built with one of such drivers enabled but still has clocks defined using CCF. For example, enabling CMD_CLK and using clk dump on sandbox target will not show CCF clocks because k210 driver overrides common soc_clk_dump.
Changelog: v1 -> v2: - Add missing static to dump functions
v2 -> v3: - Make soc_clk_dump in cmd/clk.c static instead of removing __weak
v3 -> v4: - Rebase and refactor dump for new Amlogic A1 clock controller driver
v4 -> v5: - Add docs for dump() function in clk_ops - Print driver and device names before calling corresponding dump()
Igor Prusov (8): clk: zynq: Move soc_clk_dump to Zynq clock driver clk: ast2600: Move soc_clk_dump function clk: k210: Move soc_clk_dump function clk: amlogic: Move driver and ops structs clk: Add dump operation to clk_ops cmd: clk: Use dump function from clk_ops clk: treewide: switch to clock dump from clk_ops cmd: clk: Make soc_clk_dump static
arch/arm/mach-zynq/clk.c | 57 -------------- arch/mips/mach-pic32/cpu.c | 23 ------ cmd/clk.c | 13 +++- drivers/clk/aspeed/clk_ast2600.c | 83 ++++++++++---------- drivers/clk/clk_k210.c | 103 ++++++++++++------------- drivers/clk/clk_pic32.c | 39 ++++++++++ drivers/clk/clk_versal.c | 7 +- drivers/clk/clk_zynq.c | 51 ++++++++++++ drivers/clk/clk_zynqmp.c | 13 ++-- drivers/clk/imx/clk-imx8.c | 11 +-- drivers/clk/meson/a1.c | 54 +++++-------- drivers/clk/mvebu/armada-37xx-periph.c | 5 +- drivers/clk/stm32/clk-stm32mp1.c | 29 ++----- include/clk-uclass.h | 15 ++++ include/clk.h | 2 - 15 files changed, 256 insertions(+), 249 deletions(-)

Move clock dump function in preparation for switching to dump function in clk_ops.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Acked-by: Michal Simek michal.simek@amd.com --- arch/arm/mach-zynq/clk.c | 57 --------------------------------------- drivers/clk/clk_zynq.c | 58 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 57 deletions(-)
diff --git a/arch/arm/mach-zynq/clk.c b/arch/arm/mach-zynq/clk.c index 1945f60e08..e6a67326dd 100644 --- a/arch/arm/mach-zynq/clk.c +++ b/arch/arm/mach-zynq/clk.c @@ -13,20 +13,6 @@
DECLARE_GLOBAL_DATA_PTR;
-static const char * const clk_names[clk_max] = { - "armpll", "ddrpll", "iopll", - "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", - "ddr2x", "ddr3x", "dci", - "lqspi", "smc", "pcap", "gem0", "gem1", - "fclk0", "fclk1", "fclk2", "fclk3", "can0", "can1", - "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1", "dma", - "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper", - "sdio0_aper", "sdio1_aper", "spi0_aper", "spi1_aper", - "can0_aper", "can1_aper", "i2c0_aper", "i2c1_aper", - "uart0_aper", "uart1_aper", "gpio_aper", "lqspi_aper", - "smc_aper", "swdt", "dbg_trc", "dbg_apb" -}; - /** * set_cpu_clk_info() - Setup clock information * @@ -65,46 +51,3 @@ int set_cpu_clk_info(void)
return 0; } - -/** - * soc_clk_dump() - Print clock frequencies - * Returns zero on success - * - * Implementation for the clk dump command. - */ -int soc_clk_dump(void) -{ - struct udevice *dev; - int i, ret; - - ret = uclass_get_device_by_driver(UCLASS_CLK, - DM_DRIVER_GET(zynq_clk), &dev); - if (ret) - return ret; - - printf("clk\t\tfrequency\n"); - for (i = 0; i < clk_max; i++) { - const char *name = clk_names[i]; - if (name) { - struct clk clk; - unsigned long rate; - - clk.id = i; - ret = clk_request(dev, &clk); - if (ret < 0) - return ret; - - rate = clk_get_rate(&clk); - - clk_free(&clk); - - if ((rate == (unsigned long)-ENOSYS) || - (rate == (unsigned long)-ENXIO)) - printf("%10s%20s\n", name, "unknown"); - else - printf("%10s%20lu\n", name, rate); - } - } - - return 0; -} diff --git a/drivers/clk/clk_zynq.c b/drivers/clk/clk_zynq.c index e80500e382..be5226175f 100644 --- a/drivers/clk/clk_zynq.c +++ b/drivers/clk/clk_zynq.c @@ -454,6 +454,64 @@ static int dummy_enable(struct clk *clk) return 0; }
+static const char * const clk_names[clk_max] = { + "armpll", "ddrpll", "iopll", + "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", + "ddr2x", "ddr3x", "dci", + "lqspi", "smc", "pcap", "gem0", "gem1", + "fclk0", "fclk1", "fclk2", "fclk3", "can0", "can1", + "sdio0", "sdio1", "uart0", "uart1", "spi0", "spi1", "dma", + "usb0_aper", "usb1_aper", "gem0_aper", "gem1_aper", + "sdio0_aper", "sdio1_aper", "spi0_aper", "spi1_aper", + "can0_aper", "can1_aper", "i2c0_aper", "i2c1_aper", + "uart0_aper", "uart1_aper", "gpio_aper", "lqspi_aper", + "smc_aper", "swdt", "dbg_trc", "dbg_apb" +}; + +/** + * soc_clk_dump() - Print clock frequencies + * Returns zero on success + * + * Implementation for the clk dump command. + */ +int soc_clk_dump(void) +{ + struct udevice *dev; + int i, ret; + + ret = uclass_get_device_by_driver(UCLASS_CLK, + DM_DRIVER_GET(zynq_clk), &dev); + if (ret) + return ret; + + printf("clk\t\tfrequency\n"); + for (i = 0; i < clk_max; i++) { + const char *name = clk_names[i]; + + if (name) { + struct clk clk; + unsigned long rate; + + clk.id = i; + ret = clk_request(dev, &clk); + if (ret < 0) + return ret; + + rate = clk_get_rate(&clk); + + clk_free(&clk); + + if ((rate == (unsigned long)-ENOSYS) || + (rate == (unsigned long)-ENXIO)) + printf("%10s%20s\n", name, "unknown"); + else + printf("%10s%20lu\n", name, rate); + } + } + + return 0; +} + static struct clk_ops zynq_clk_ops = { .get_rate = zynq_clk_get_rate, #ifndef CONFIG_SPL_BUILD

Move clock dump function to avoid forward declaration after switching to dump in clk_ops.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru --- drivers/clk/aspeed/clk_ast2600.c | 70 ++++++++++++++++---------------- 1 file changed, 35 insertions(+), 35 deletions(-)
diff --git a/drivers/clk/aspeed/clk_ast2600.c b/drivers/clk/aspeed/clk_ast2600.c index e5ada5b6d4..b3cc8392fa 100644 --- a/drivers/clk/aspeed/clk_ast2600.c +++ b/drivers/clk/aspeed/clk_ast2600.c @@ -1104,41 +1104,6 @@ static int ast2600_clk_enable(struct clk *clk) return 0; }
-struct clk_ops ast2600_clk_ops = { - .get_rate = ast2600_clk_get_rate, - .set_rate = ast2600_clk_set_rate, - .enable = ast2600_clk_enable, -}; - -static int ast2600_clk_probe(struct udevice *dev) -{ - struct ast2600_clk_priv *priv = dev_get_priv(dev); - - priv->scu = devfdt_get_addr_ptr(dev); - if (IS_ERR(priv->scu)) - return PTR_ERR(priv->scu); - - ast2600_init_rgmii_clk(priv->scu, &rgmii_clk_defconfig); - ast2600_init_rmii_clk(priv->scu, &rmii_clk_defconfig); - ast2600_configure_mac12_clk(priv->scu); - ast2600_configure_mac34_clk(priv->scu); - ast2600_configure_rsa_ecc_clk(priv->scu); - - return 0; -} - -static int ast2600_clk_bind(struct udevice *dev) -{ - int ret; - - /* The reset driver does not have a device node, so bind it here */ - ret = device_bind_driver(gd->dm_root, "ast_sysreset", "reset", &dev); - if (ret) - debug("Warning: No reset driver: ret=%d\n", ret); - - return 0; -} - struct aspeed_clks { ulong id; const char *name; @@ -1203,6 +1168,41 @@ int soc_clk_dump(void) return 0; }
+struct clk_ops ast2600_clk_ops = { + .get_rate = ast2600_clk_get_rate, + .set_rate = ast2600_clk_set_rate, + .enable = ast2600_clk_enable, +}; + +static int ast2600_clk_probe(struct udevice *dev) +{ + struct ast2600_clk_priv *priv = dev_get_priv(dev); + + priv->scu = devfdt_get_addr_ptr(dev); + if (IS_ERR(priv->scu)) + return PTR_ERR(priv->scu); + + ast2600_init_rgmii_clk(priv->scu, &rgmii_clk_defconfig); + ast2600_init_rmii_clk(priv->scu, &rmii_clk_defconfig); + ast2600_configure_mac12_clk(priv->scu); + ast2600_configure_mac34_clk(priv->scu); + ast2600_configure_rsa_ecc_clk(priv->scu); + + return 0; +} + +static int ast2600_clk_bind(struct udevice *dev) +{ + int ret; + + /* The reset driver does not have a device node, so bind it here */ + ret = device_bind_driver(gd->dm_root, "ast_sysreset", "reset", &dev); + if (ret) + debug("Warning: No reset driver: ret=%d\n", ret); + + return 0; +} + static const struct udevice_id ast2600_clk_ids[] = { { .compatible = "aspeed,ast2600-scu", }, { },

Move clock dump function to avoid forward declaration after switching to dump in clk_ops.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Sean Anderson seanga2@gmail.com --- drivers/clk/clk_k210.c | 92 +++++++++++++++++++++--------------------- 1 file changed, 46 insertions(+), 46 deletions(-)
diff --git a/drivers/clk/clk_k210.c b/drivers/clk/clk_k210.c index c534cc07e0..2f17152021 100644 --- a/drivers/clk/clk_k210.c +++ b/drivers/clk/clk_k210.c @@ -1238,52 +1238,6 @@ static int k210_clk_request(struct clk *clk) return 0; }
-static const struct clk_ops k210_clk_ops = { - .request = k210_clk_request, - .set_rate = k210_clk_set_rate, - .get_rate = k210_clk_get_rate, - .set_parent = k210_clk_set_parent, - .enable = k210_clk_enable, - .disable = k210_clk_disable, -}; - -static int k210_clk_probe(struct udevice *dev) -{ - int ret; - struct k210_clk_priv *priv = dev_get_priv(dev); - - priv->base = dev_read_addr_ptr(dev_get_parent(dev)); - if (!priv->base) - return -EINVAL; - - ret = clk_get_by_index(dev, 0, &priv->in0); - if (ret) - return ret; - - /* - * Force setting defaults, even before relocation. This is so we can - * set the clock rate for PLL1 before we relocate into aisram. - */ - if (!(gd->flags & GD_FLG_RELOC)) - clk_set_defaults(dev, CLK_DEFAULTS_POST_FORCE); - - return 0; -} - -static const struct udevice_id k210_clk_ids[] = { - { .compatible = "canaan,k210-clk" }, - { }, -}; - -U_BOOT_DRIVER(k210_clk) = { - .name = "k210_clk", - .id = UCLASS_CLK, - .of_match = k210_clk_ids, - .ops = &k210_clk_ops, - .probe = k210_clk_probe, - .priv_auto = sizeof(struct k210_clk_priv), -}; - #if IS_ENABLED(CONFIG_CMD_CLK) static char show_enabled(struct k210_clk_priv *priv, int id) { @@ -1342,3 +1296,49 @@ int soc_clk_dump(void) return 0; } #endif + +static const struct clk_ops k210_clk_ops = { + .request = k210_clk_request, + .set_rate = k210_clk_set_rate, + .get_rate = k210_clk_get_rate, + .set_parent = k210_clk_set_parent, + .enable = k210_clk_enable, + .disable = k210_clk_disable, +}; + +static int k210_clk_probe(struct udevice *dev) +{ + int ret; + struct k210_clk_priv *priv = dev_get_priv(dev); + + priv->base = dev_read_addr_ptr(dev_get_parent(dev)); + if (!priv->base) + return -EINVAL; + + ret = clk_get_by_index(dev, 0, &priv->in0); + if (ret) + return ret; + + /* + * Force setting defaults, even before relocation. This is so we can + * set the clock rate for PLL1 before we relocate into aisram. + */ + if (!(gd->flags & GD_FLG_RELOC)) + clk_set_defaults(dev, CLK_DEFAULTS_POST_FORCE); + + return 0; +} + +static const struct udevice_id k210_clk_ids[] = { + { .compatible = "canaan,k210-clk" }, + { }, +}; + +U_BOOT_DRIVER(k210_clk) = { + .name = "k210_clk", + .id = UCLASS_CLK, + .of_match = k210_clk_ids, + .ops = &k210_clk_ops, + .probe = k210_clk_probe, + .priv_auto = sizeof(struct k210_clk_priv), +};

Move driver and ops structs to avoid forward declaration after switching to dump in clk_ops.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Neil Armstrong neil.armstrong@linaro.org --- drivers/clk/meson/a1.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/clk/meson/a1.c b/drivers/clk/meson/a1.c index 1075ba7333..e3fa9db7d0 100644 --- a/drivers/clk/meson/a1.c +++ b/drivers/clk/meson/a1.c @@ -601,14 +601,6 @@ static int meson_clk_set_parent(struct clk *clk, struct clk *parent_clk) return meson_mux_set_parent_by_id(clk, parent_clk->id); }
-static struct clk_ops meson_clk_ops = { - .disable = meson_clk_disable, - .enable = meson_clk_enable, - .get_rate = meson_clk_get_rate, - .set_rate = meson_clk_set_rate, - .set_parent = meson_clk_set_parent, -}; - static int meson_clk_probe(struct udevice *dev) { struct meson_clk *priv = dev_get_priv(dev); @@ -638,15 +630,6 @@ static const struct udevice_id meson_clk_ids[] = { { } };
-U_BOOT_DRIVER(meson_clk) = { - .name = "meson-clk-a1", - .id = UCLASS_CLK, - .of_match = meson_clk_ids, - .priv_auto = sizeof(struct meson_clk), - .ops = &meson_clk_ops, - .probe = meson_clk_probe, -}; - static const char *meson_clk_get_name(struct clk *clk, int id) { const struct meson_clk_info *info; @@ -727,3 +710,20 @@ int soc_clk_dump(void)
return 0; } + +static struct clk_ops meson_clk_ops = { + .disable = meson_clk_disable, + .enable = meson_clk_enable, + .get_rate = meson_clk_get_rate, + .set_rate = meson_clk_set_rate, + .set_parent = meson_clk_set_parent, +}; + +U_BOOT_DRIVER(meson_clk) = { + .name = "meson-clk-a1", + .id = UCLASS_CLK, + .of_match = meson_clk_ids, + .priv_auto = sizeof(struct meson_clk), + .ops = &meson_clk_ops, + .probe = meson_clk_probe, +};

This adds dump function to struct clk_ops which should replace soc_clk_dump. It allows clock drivers to provide custom dump implementation without overriding generic CCF dump function.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com --- include/clk-uclass.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/clk-uclass.h b/include/clk-uclass.h index a22f1a5d84..793bf14160 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -25,6 +25,7 @@ struct ofnode_phandle_args; * @set_parent: Set current clock parent * @enable: Enable a clock. * @disable: Disable a clock. + * @dump: Print clock information. * * The individual methods are described more fully below. */ @@ -39,6 +40,9 @@ struct clk_ops { int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk); +#if IS_ENABLED(CONFIG_CMD_CLK) + int (*dump)(struct udevice *dev); +#endif };
#if 0 /* For documentation only */ @@ -135,6 +139,17 @@ int enable(struct clk *clk); * Return: zero on success, or -ve error code. */ int disable(struct clk *clk); + +/** + * dump() - Print clock information. + * @clk: The clock device to dump. + * + * If present, this function is called by "clk dump" command for each + * bound device. + * + * Return: zero on success, or -ve error code. + */ +int dump(struct udevice *dev); #endif
#endif

On 11/2/23 08:20, Igor Prusov wrote:
This adds dump function to struct clk_ops which should replace soc_clk_dump. It allows clock drivers to provide custom dump implementation without overriding generic CCF dump function.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com
include/clk-uclass.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/clk-uclass.h b/include/clk-uclass.h index a22f1a5d84..793bf14160 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -25,6 +25,7 @@ struct ofnode_phandle_args;
- @set_parent: Set current clock parent
- @enable: Enable a clock.
- @disable: Disable a clock.
*/
- @dump: Print clock information.
- The individual methods are described more fully below.
@@ -39,6 +40,9 @@ struct clk_ops { int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk); +#if IS_ENABLED(CONFIG_CMD_CLK)
- int (*dump)(struct udevice *dev);
+#endif };
#if 0 /* For documentation only */ @@ -135,6 +139,17 @@ int enable(struct clk *clk);
- Return: zero on success, or -ve error code.
*/ int disable(struct clk *clk);
+/**
- dump() - Print clock information.
- @clk: The clock device to dump.
- If present, this function is called by "clk dump" command for each
- bound device.
- Return: zero on success, or -ve error code.
- */
+int dump(struct udevice *dev);
Actually, this should return void, since we don't do anything with the return code.
--Sean
#endif
#endif

On Sat, Nov 04, 2023 at 11:24:32AM -0400, Sean Anderson wrote:
On 11/2/23 08:20, Igor Prusov wrote:
This adds dump function to struct clk_ops which should replace soc_clk_dump. It allows clock drivers to provide custom dump implementation without overriding generic CCF dump function.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com
include/clk-uclass.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/clk-uclass.h b/include/clk-uclass.h index a22f1a5d84..793bf14160 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -25,6 +25,7 @@ struct ofnode_phandle_args;
- @set_parent: Set current clock parent
- @enable: Enable a clock.
- @disable: Disable a clock.
*/
- @dump: Print clock information.
- The individual methods are described more fully below.
@@ -39,6 +40,9 @@ struct clk_ops { int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk); +#if IS_ENABLED(CONFIG_CMD_CLK)
- int (*dump)(struct udevice *dev);
+#endif }; #if 0 /* For documentation only */ @@ -135,6 +139,17 @@ int enable(struct clk *clk);
- Return: zero on success, or -ve error code.
*/ int disable(struct clk *clk);
+/**
- dump() - Print clock information.
- @clk: The clock device to dump.
- If present, this function is called by "clk dump" command for each
- bound device.
- Return: zero on success, or -ve error code.
- */
+int dump(struct udevice *dev);
Actually, this should return void, since we don't do anything with the return code.
Good catch! Though there is, for example, zynqmp_clk_dump() that may return an error code. Wouldn't it be better to print an error message with the code in soc_clk_dump()? It might be convinient to have common code handling unexpected errors during dump.
--Sean
#endif #endif

On 11/4/23 14:09, Igor Prusov wrote:
On Sat, Nov 04, 2023 at 11:24:32AM -0400, Sean Anderson wrote:
On 11/2/23 08:20, Igor Prusov wrote:
This adds dump function to struct clk_ops which should replace soc_clk_dump. It allows clock drivers to provide custom dump implementation without overriding generic CCF dump function.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com
include/clk-uclass.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/clk-uclass.h b/include/clk-uclass.h index a22f1a5d84..793bf14160 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -25,6 +25,7 @@ struct ofnode_phandle_args; * @set_parent: Set current clock parent * @enable: Enable a clock. * @disable: Disable a clock.
- @dump: Print clock information.
*/
- The individual methods are described more fully below.
@@ -39,6 +40,9 @@ struct clk_ops { int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk); +#if IS_ENABLED(CONFIG_CMD_CLK)
- int (*dump)(struct udevice *dev);
+#endif }; #if 0 /* For documentation only */ @@ -135,6 +139,17 @@ int enable(struct clk *clk); * Return: zero on success, or -ve error code. */ int disable(struct clk *clk);
+/**
- dump() - Print clock information.
- @clk: The clock device to dump.
- If present, this function is called by "clk dump" command for each
- bound device.
- Return: zero on success, or -ve error code.
- */
+int dump(struct udevice *dev);
Actually, this should return void, since we don't do anything with the return code.
Good catch! Though there is, for example, zynqmp_clk_dump() that may return an error code. Wouldn't it be better to print an error message with the code in soc_clk_dump()? It might be convinient to have common code handling unexpected errors during dump.
Since this function is for printing, if the driver gets an error it should just print the error itself. It can probably provide a better error message than we can. And this command is mainly informational anyway, so we don't really need to set the return code (e.g. $?).
--Sean

On Sat, Nov 04, 2023 at 02:40:34PM -0400, Sean Anderson wrote:
On 11/4/23 14:09, Igor Prusov wrote:
On Sat, Nov 04, 2023 at 11:24:32AM -0400, Sean Anderson wrote:
On 11/2/23 08:20, Igor Prusov wrote:
This adds dump function to struct clk_ops which should replace soc_clk_dump. It allows clock drivers to provide custom dump implementation without overriding generic CCF dump function.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com
include/clk-uclass.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/include/clk-uclass.h b/include/clk-uclass.h index a22f1a5d84..793bf14160 100644 --- a/include/clk-uclass.h +++ b/include/clk-uclass.h @@ -25,6 +25,7 @@ struct ofnode_phandle_args; * @set_parent: Set current clock parent * @enable: Enable a clock. * @disable: Disable a clock.
- @dump: Print clock information.
*/
- The individual methods are described more fully below.
@@ -39,6 +40,9 @@ struct clk_ops { int (*set_parent)(struct clk *clk, struct clk *parent); int (*enable)(struct clk *clk); int (*disable)(struct clk *clk); +#if IS_ENABLED(CONFIG_CMD_CLK)
- int (*dump)(struct udevice *dev);
+#endif }; #if 0 /* For documentation only */ @@ -135,6 +139,17 @@ int enable(struct clk *clk); * Return: zero on success, or -ve error code. */ int disable(struct clk *clk);
+/**
- dump() - Print clock information.
- @clk: The clock device to dump.
- If present, this function is called by "clk dump" command for each
- bound device.
- Return: zero on success, or -ve error code.
- */
+int dump(struct udevice *dev);
Actually, this should return void, since we don't do anything with the return code.
Good catch! Though there is, for example, zynqmp_clk_dump() that may return an error code. Wouldn't it be better to print an error message with the code in soc_clk_dump()? It might be convinient to have common code handling unexpected errors during dump.
Since this function is for printing, if the driver gets an error it should just print the error itself. It can probably provide a better error message than we can. And this command is mainly informational anyway, so we don't really need to set the return code (e.g. $?).
Got it, will fix in v6.

Add another loop to dump additional info from clock providers that implement dump operation.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com --- cmd/clk.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/cmd/clk.c b/cmd/clk.c index c7c379d7a6..4b9709d3ff 100644 --- a/cmd/clk.c +++ b/cmd/clk.c @@ -62,6 +62,7 @@ static void show_clks(struct udevice *dev, int depth, int last_flag) int __weak soc_clk_dump(void) { struct udevice *dev; + const struct clk_ops *ops;
printf(" Rate Usecnt Name\n"); printf("------------------------------------------\n"); @@ -69,6 +70,14 @@ int __weak soc_clk_dump(void) uclass_foreach_dev_probe(UCLASS_CLK, dev) show_clks(dev, -1, 0);
+ uclass_foreach_dev_probe(UCLASS_CLK, dev) { + ops = dev_get_driver_ops(dev); + if (ops && ops->dump) { + printf("\n%s %s:\n", dev->driver->name, dev->name); + ops->dump(dev); + } + } + return 0; } #else

On 11/2/23 08:20, Igor Prusov wrote:
Add another loop to dump additional info from clock providers that implement dump operation.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com
cmd/clk.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/cmd/clk.c b/cmd/clk.c index c7c379d7a6..4b9709d3ff 100644 --- a/cmd/clk.c +++ b/cmd/clk.c @@ -62,6 +62,7 @@ static void show_clks(struct udevice *dev, int depth, int last_flag) int __weak soc_clk_dump(void) { struct udevice *dev;
const struct clk_ops *ops;
printf(" Rate Usecnt Name\n"); printf("------------------------------------------\n");
@@ -69,6 +70,14 @@ int __weak soc_clk_dump(void) uclass_foreach_dev_probe(UCLASS_CLK, dev) show_clks(dev, -1, 0);
- uclass_foreach_dev_probe(UCLASS_CLK, dev) {
ops = dev_get_driver_ops(dev);
if (ops && ops->dump) {
printf("\n%s %s:\n", dev->driver->name, dev->name);
ops->dump(dev);
}
- }
- return 0; } #else
Reviewed-by: Sean Anderson seanga2@gmail.com

Switch to using new dump operation in clock provider drivers instead of overriding soc_clk_dump.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Tested-by: Patrice Chotard patrice.chotard@foss.st.com Reviewed-by: Sean Anderson seanga2@gmail.com --- arch/mips/mach-pic32/cpu.c | 23 --------------- drivers/clk/aspeed/clk_ast2600.c | 13 ++++----- drivers/clk/clk_k210.c | 11 +++----- drivers/clk/clk_pic32.c | 39 ++++++++++++++++++++++++++ drivers/clk/clk_versal.c | 7 ++++- drivers/clk/clk_zynq.c | 19 ++++--------- drivers/clk/clk_zynqmp.c | 13 ++++----- drivers/clk/imx/clk-imx8.c | 11 +++----- drivers/clk/meson/a1.c | 24 ++++------------ drivers/clk/mvebu/armada-37xx-periph.c | 5 +++- drivers/clk/stm32/clk-stm32mp1.c | 29 ++++++------------- 11 files changed, 89 insertions(+), 105 deletions(-)
diff --git a/arch/mips/mach-pic32/cpu.c b/arch/mips/mach-pic32/cpu.c index dbf8c9cd22..3181a946a2 100644 --- a/arch/mips/mach-pic32/cpu.c +++ b/arch/mips/mach-pic32/cpu.c @@ -143,26 +143,3 @@ const char *get_core_name(void) return str; } #endif -#ifdef CONFIG_CMD_CLK - -int soc_clk_dump(void) -{ - int i; - - printf("PLL Speed: %lu MHz\n", - CLK_MHZ(rate(PLLCLK))); - - printf("CPU Speed: %lu MHz\n", CLK_MHZ(rate(PB7CLK))); - - printf("MPLL Speed: %lu MHz\n", CLK_MHZ(rate(MPLL))); - - for (i = PB1CLK; i <= PB7CLK; i++) - printf("PB%d Clock Speed: %lu MHz\n", i - PB1CLK + 1, - CLK_MHZ(rate(i))); - - for (i = REF1CLK; i <= REF5CLK; i++) - printf("REFO%d Clock Speed: %lu MHz\n", i - REF1CLK + 1, - CLK_MHZ(rate(i))); - return 0; -} -#endif diff --git a/drivers/clk/aspeed/clk_ast2600.c b/drivers/clk/aspeed/clk_ast2600.c index b3cc8392fa..e1365d3f81 100644 --- a/drivers/clk/aspeed/clk_ast2600.c +++ b/drivers/clk/aspeed/clk_ast2600.c @@ -1109,6 +1109,7 @@ struct aspeed_clks { const char *name; };
+#if IS_ENABLED(CONFIG_CMD_CLK) static struct aspeed_clks aspeed_clk_names[] = { { ASPEED_CLK_HPLL, "hpll" }, { ASPEED_CLK_MPLL, "mpll" }, @@ -1123,18 +1124,12 @@ static struct aspeed_clks aspeed_clk_names[] = { { ASPEED_CLK_HUARTX, "huxclk" }, };
-int soc_clk_dump(void) +static int ast2600_clk_dump(struct udevice *dev) { - struct udevice *dev; struct clk clk; unsigned long rate; int i, ret;
- ret = uclass_get_device_by_driver(UCLASS_CLK, DM_DRIVER_GET(aspeed_scu), - &dev); - if (ret) - return ret; - printf("Clk\t\tHz\n");
for (i = 0; i < ARRAY_SIZE(aspeed_clk_names); i++) { @@ -1167,11 +1162,15 @@ int soc_clk_dump(void)
return 0; } +#endif
struct clk_ops ast2600_clk_ops = { .get_rate = ast2600_clk_get_rate, .set_rate = ast2600_clk_set_rate, .enable = ast2600_clk_enable, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = ast2600_clk_dump, +#endif };
static int ast2600_clk_probe(struct udevice *dev) diff --git a/drivers/clk/clk_k210.c b/drivers/clk/clk_k210.c index 2f17152021..058940b828 100644 --- a/drivers/clk/clk_k210.c +++ b/drivers/clk/clk_k210.c @@ -1276,16 +1276,10 @@ static void show_clks(struct k210_clk_priv *priv, int id, int depth) } }
-int soc_clk_dump(void) +static int k210_clk_dump(struct udevice *dev) { - int ret; - struct udevice *dev; struct k210_clk_priv *priv;
- ret = uclass_get_device_by_driver(UCLASS_CLK, DM_DRIVER_GET(k210_clk), - &dev); - if (ret) - return ret; priv = dev_get_priv(dev);
puts(" Rate Enabled Name\n"); @@ -1304,6 +1298,9 @@ static const struct clk_ops k210_clk_ops = { .set_parent = k210_clk_set_parent, .enable = k210_clk_enable, .disable = k210_clk_disable, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = k210_clk_dump, +#endif };
static int k210_clk_probe(struct udevice *dev) diff --git a/drivers/clk/clk_pic32.c b/drivers/clk/clk_pic32.c index ef06a7fb9f..f756fc88f0 100644 --- a/drivers/clk/clk_pic32.c +++ b/drivers/clk/clk_pic32.c @@ -20,6 +20,8 @@
DECLARE_GLOBAL_DATA_PTR;
+#define CLK_MHZ(x) ((x) / 1000000) + /* Primary oscillator */ #define SYS_POSC_CLK_HZ 24000000
@@ -385,9 +387,46 @@ static ulong pic32_set_rate(struct clk *clk, ulong rate) return rate; }
+#if IS_ENABLED(CONFIG_CMD_CLK) +static int pic32_dump(struct udevice *dev) +{ + int i; + struct clk clk; + + clk.dev = dev; + + clk.id = PLLCLK; + printf("PLL Speed: %lu MHz\n", + CLK_MHZ(pic32_get_rate(&clk))); + + clk.id = PB7CLK; + printf("CPU Speed: %lu MHz\n", CLK_MHZ(pic32_get_rate(&clk))); + + clk.id = MPLL; + printf("MPLL Speed: %lu MHz\n", CLK_MHZ(pic32_get_rate(&clk))); + + for (i = PB1CLK; i <= PB7CLK; i++) { + clk.id = i; + printf("PB%d Clock Speed: %lu MHz\n", i - PB1CLK + 1, + CLK_MHZ(pic32_get_rate(&clk))); + } + + for (i = REF1CLK; i <= REF5CLK; i++) { + clk.id = i; + printf("REFO%d Clock Speed: %lu MHz\n", i - REF1CLK + 1, + CLK_MHZ(pic32_get_rate(&clk))); + } + + return 0; +} +#endif + static struct clk_ops pic32_pic32_clk_ops = { .set_rate = pic32_set_rate, .get_rate = pic32_get_rate, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = pic32_dump, +#endif };
static int pic32_clk_probe(struct udevice *dev) diff --git a/drivers/clk/clk_versal.c b/drivers/clk/clk_versal.c index c473643603..f3fafec7f7 100644 --- a/drivers/clk/clk_versal.c +++ b/drivers/clk/clk_versal.c @@ -555,7 +555,8 @@ static int versal_clock_get_rate(u32 clk_id, u64 *clk_rate) return 0; }
-int soc_clk_dump(void) +#if IS_ENABLED(CONFIG_CMD_CLK) +static int versal_clk_dump(struct udevice __always_unused *dev) { u64 clk_rate = 0; u32 type, ret, i = 0; @@ -578,6 +579,7 @@ int soc_clk_dump(void)
return 0; } +#endif
static void versal_get_clock_info(void) { @@ -769,6 +771,9 @@ static struct clk_ops versal_clk_ops = { .set_rate = versal_clk_set_rate, .get_rate = versal_clk_get_rate, .enable = versal_clk_enable, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = versal_clk_dump, +#endif };
static const struct udevice_id versal_clk_ids[] = { diff --git a/drivers/clk/clk_zynq.c b/drivers/clk/clk_zynq.c index be5226175f..d492eea3b0 100644 --- a/drivers/clk/clk_zynq.c +++ b/drivers/clk/clk_zynq.c @@ -454,6 +454,7 @@ static int dummy_enable(struct clk *clk) return 0; }
+#if IS_ENABLED(CONFIG_CMD_CLK) static const char * const clk_names[clk_max] = { "armpll", "ddrpll", "iopll", "cpu_6or4x", "cpu_3or2x", "cpu_2x", "cpu_1x", @@ -468,22 +469,10 @@ static const char * const clk_names[clk_max] = { "smc_aper", "swdt", "dbg_trc", "dbg_apb" };
-/** - * soc_clk_dump() - Print clock frequencies - * Returns zero on success - * - * Implementation for the clk dump command. - */ -int soc_clk_dump(void) +static int zynq_clk_dump(struct udevice *dev) { - struct udevice *dev; int i, ret;
- ret = uclass_get_device_by_driver(UCLASS_CLK, - DM_DRIVER_GET(zynq_clk), &dev); - if (ret) - return ret; - printf("clk\t\tfrequency\n"); for (i = 0; i < clk_max; i++) { const char *name = clk_names[i]; @@ -511,6 +500,7 @@ int soc_clk_dump(void)
return 0; } +#endif
static struct clk_ops zynq_clk_ops = { .get_rate = zynq_clk_get_rate, @@ -518,6 +508,9 @@ static struct clk_ops zynq_clk_ops = { .set_rate = zynq_clk_set_rate, #endif .enable = dummy_enable, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = zynq_clk_dump, +#endif };
static int zynq_clk_probe(struct udevice *dev) diff --git a/drivers/clk/clk_zynqmp.c b/drivers/clk/clk_zynqmp.c index 1cfe0e25b1..dfcda429c7 100644 --- a/drivers/clk/clk_zynqmp.c +++ b/drivers/clk/clk_zynqmp.c @@ -735,16 +735,11 @@ static ulong zynqmp_clk_set_rate(struct clk *clk, ulong rate) } }
-int soc_clk_dump(void) +#if IS_ENABLED(CONFIG_CMD_CLK) +static int zynqmp_clk_dump(struct udevice *dev) { - struct udevice *dev; int i, ret;
- ret = uclass_get_device_by_driver(UCLASS_CLK, - DM_DRIVER_GET(zynqmp_clk), &dev); - if (ret) - return ret; - printf("clk\t\tfrequency\n"); for (i = 0; i < clk_max; i++) { const char *name = clk_names[i]; @@ -772,6 +767,7 @@ int soc_clk_dump(void)
return 0; } +#endif
static int zynqmp_get_freq_by_name(char *name, struct udevice *dev, ulong *freq) { @@ -871,6 +867,9 @@ static struct clk_ops zynqmp_clk_ops = { .set_rate = zynqmp_clk_set_rate, .get_rate = zynqmp_clk_get_rate, .enable = zynqmp_clk_enable, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = zynqmp_clk_dump, +#endif };
static const struct udevice_id zynqmp_clk_ids[] = { diff --git a/drivers/clk/imx/clk-imx8.c b/drivers/clk/imx/clk-imx8.c index ceeead3434..6cd55f4369 100644 --- a/drivers/clk/imx/clk-imx8.c +++ b/drivers/clk/imx/clk-imx8.c @@ -43,18 +43,12 @@ static int imx8_clk_enable(struct clk *clk) }
#if IS_ENABLED(CONFIG_CMD_CLK) -int soc_clk_dump(void) +static int imx8_clk_dump(struct udevice *dev) { - struct udevice *dev; struct clk clk; unsigned long rate; int i, ret;
- ret = uclass_get_device_by_driver(UCLASS_CLK, - DM_DRIVER_GET(imx8_clk), &dev); - if (ret) - return ret; - printf("Clk\t\tHz\n");
for (i = 0; i < num_clks; i++) { @@ -94,6 +88,9 @@ static struct clk_ops imx8_clk_ops = { .get_rate = imx8_clk_get_rate, .enable = imx8_clk_enable, .disable = imx8_clk_disable, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = imx8_clk_dump, +#endif };
static int imx8_clk_probe(struct udevice *dev) diff --git a/drivers/clk/meson/a1.c b/drivers/clk/meson/a1.c index e3fa9db7d0..1820b31ff7 100644 --- a/drivers/clk/meson/a1.c +++ b/drivers/clk/meson/a1.c @@ -639,7 +639,7 @@ static const char *meson_clk_get_name(struct clk *clk, int id) return IS_ERR(info) ? "unknown" : info->name; }
-static int meson_clk_dump(struct clk *clk) +static int meson_clk_dump_single(struct clk *clk) { const struct meson_clk_info *info; struct meson_clk *priv; @@ -674,7 +674,7 @@ static int meson_clk_dump(struct clk *clk) return 0; }
-static int meson_clk_dump_dev(struct udevice *dev) +static int meson_clk_dump(struct udevice *dev) { int i; struct meson_clk_data *data; @@ -687,7 +687,7 @@ static int meson_clk_dump_dev(struct udevice *dev)
data = (struct meson_clk_data *)dev_get_driver_data(dev); for (i = 0; i < data->num_clocks; i++) { - meson_clk_dump(&(struct clk){ + meson_clk_dump_single(&(struct clk){ .dev = dev, .id = i }); @@ -696,27 +696,15 @@ static int meson_clk_dump_dev(struct udevice *dev) return 0; }
-int soc_clk_dump(void) -{ - struct udevice *dev; - int i = 0; - - while (!uclass_get_device(UCLASS_CLK, i++, &dev)) { - if (dev->driver == DM_DRIVER_GET(meson_clk)) { - meson_clk_dump_dev(dev); - printf("\n"); - } - } - - return 0; -} - static struct clk_ops meson_clk_ops = { .disable = meson_clk_disable, .enable = meson_clk_enable, .get_rate = meson_clk_get_rate, .set_rate = meson_clk_set_rate, .set_parent = meson_clk_set_parent, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = meson_clk_dump, +#endif };
U_BOOT_DRIVER(meson_clk) = { diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c index e75052f383..faf75c8792 100644 --- a/drivers/clk/mvebu/armada-37xx-periph.c +++ b/drivers/clk/mvebu/armada-37xx-periph.c @@ -499,7 +499,7 @@ static int clk_dump(const char *name, int (*func)(struct udevice *))
int armada_37xx_tbg_clk_dump(struct udevice *);
-int soc_clk_dump(void) +static int armada37xx_clk_dump(struct udevice __always_unused *dev) { printf(" xtal at %u000000 Hz\n\n", get_ref_clk());
@@ -605,6 +605,9 @@ static const struct clk_ops armada_37xx_periph_clk_ops = { .set_parent = armada_37xx_periph_clk_set_parent, .enable = armada_37xx_periph_clk_enable, .disable = armada_37xx_periph_clk_disable, +#if IS_ENABLED(CONFIG_CMD_CLK) + .dump = armada37xx_clk_dump, +#endif };
static const struct udevice_id armada_37xx_periph_clk_ids[] = { diff --git a/drivers/clk/stm32/clk-stm32mp1.c b/drivers/clk/stm32/clk-stm32mp1.c index f3ac8c7583..f78540ad1d 100644 --- a/drivers/clk/stm32/clk-stm32mp1.c +++ b/drivers/clk/stm32/clk-stm32mp1.c @@ -2225,10 +2225,13 @@ static void stm32mp1_osc_init(struct udevice *dev) } }
-static void __maybe_unused stm32mp1_clk_dump(struct stm32mp1_clk_priv *priv) +static int __maybe_unused stm32mp1_clk_dump(struct udevice *dev) { char buf[32]; int i, s, p; + struct stm32mp1_clk_priv *priv; + + priv = dev_get_priv(dev);
printf("Clocks:\n"); for (i = 0; i < _PARENT_NB; i++) { @@ -2250,28 +2253,9 @@ static void __maybe_unused stm32mp1_clk_dump(struct stm32mp1_clk_priv *priv) stm32mp1_clk_parent_sel_name[i], i, p); } } -} - -#ifdef CONFIG_CMD_CLK -int soc_clk_dump(void) -{ - struct udevice *dev; - struct stm32mp1_clk_priv *priv; - int ret; - - ret = uclass_get_device_by_driver(UCLASS_CLK, - DM_DRIVER_GET(stm32mp1_clock), - &dev); - if (ret) - return ret; - - priv = dev_get_priv(dev); - - stm32mp1_clk_dump(priv);
return 0; } -#endif
static int stm32mp1_clk_probe(struct udevice *dev) { @@ -2302,7 +2286,7 @@ static int stm32mp1_clk_probe(struct udevice *dev) #if defined(VERBOSE_DEBUG) /* display debug information for probe after relocation */ if (gd->flags & GD_FLG_RELOC) - stm32mp1_clk_dump(priv); + stm32mp1_clk_dump(dev); #endif
gd->cpu_clk = stm32mp1_clk_get(priv, _CK_MPU); @@ -2333,6 +2317,9 @@ static const struct clk_ops stm32mp1_clk_ops = { .disable = stm32mp1_clk_disable, .get_rate = stm32mp1_clk_get_rate, .set_rate = stm32mp1_clk_set_rate, +#if IS_ENABLED(CONFIG_CMD_CLK) && !IS_ENABLED(CONFIG_SPL_BUILD) + .dump = stm32mp1_clk_dump, +#endif };
U_BOOT_DRIVER(stm32mp1_clock) = {

After introducing dump to clk_ops there is no need to override or expose this symbol anymore.
Signed-off-by: Igor Prusov ivprusov@sberdevices.ru Reviewed-by: Patrice Chotard patrice.chotard@foss.st.com Tested-by: Patrice Chotard patrice.chotard@foss.st.com Reviewed-by: Sean Anderson seanga2@gmail.com --- cmd/clk.c | 4 ++-- include/clk.h | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/cmd/clk.c b/cmd/clk.c index 4b9709d3ff..7bbcbfeda3 100644 --- a/cmd/clk.c +++ b/cmd/clk.c @@ -59,7 +59,7 @@ static void show_clks(struct udevice *dev, int depth, int last_flag) } }
-int __weak soc_clk_dump(void) +static int soc_clk_dump(void) { struct udevice *dev; const struct clk_ops *ops; @@ -81,7 +81,7 @@ int __weak soc_clk_dump(void) return 0; } #else -int __weak soc_clk_dump(void) +static int soc_clk_dump(void) { puts("Not implemented\n"); return 1; diff --git a/include/clk.h b/include/clk.h index d91285235f..bf0d9c9d7f 100644 --- a/include/clk.h +++ b/include/clk.h @@ -674,8 +674,6 @@ static inline bool clk_valid(struct clk *clk) return clk && !!clk->dev; }
-int soc_clk_dump(void); - #endif
#define clk_prepare_enable(clk) clk_enable(clk)
participants (3)
-
Igor Prusov
-
Igor Prusov
-
Sean Anderson