> Subject: Re: [PATCH v7 3/3] clk: aspeed: add AST2700 clock driver. > > Quoting Ryan Chen (2024-10-27 22:30:18) > > diff --git a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c new > > file mode 100644 index 000000000000..db9ee5031b7c > > --- /dev/null > > +++ b/drivers/clk/clk-ast2700.c > > @@ -0,0 +1,1513 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2024 ASPEED Technology Inc. > > + * Author: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx> */ > > + > > +#include <linux/auxiliary_bus.h> > > Is this include used? Will remove it. > > > +#include <linux/clk-provider.h> > > +#include <linux/of_address.h> > > Is this include used? Will remove, and add #include <linux/io.h> fore readl/writel > > > +#include <linux/of_device.h> > > Probably should be mod_devicetable.h here Yes, will add #include <linux/mod_devicetable.h> > > > +#include <linux/of_platform.h> > > +#include <linux/platform_device.h> > > +#include <linux/slab.h> > > +#include <linux/units.h> > > + > > +#include <dt-bindings/clock/aspeed,ast2700-scu.h> > > +#include <soc/aspeed/reset-aspeed.h> > > This include can go before dt-bindings. Will update following. #include <soc/aspeed/reset-aspeed.h> #include <dt-bindings/clock/aspeed,ast2700-scu.h> > > > + > > +#define SCU_CLK_12MHZ (12 * HZ_PER_MHZ) #define SCU_CLK_24MHZ > (24 * > > +HZ_PER_MHZ) #define SCU_CLK_25MHZ (25 * HZ_PER_MHZ) #define > > +SCU_CLK_192MHZ (192 * HZ_PER_MHZ) > > + > > +/* SOC0 */ > > +#define SCU0_HWSTRAP1 0x010 > > +#define SCU0_CLK_STOP 0x240 > > +#define SCU0_CLK_SEL1 0x280 > > +#define SCU0_CLK_SEL2 0x284 > > +#define GET_USB_REFCLK_DIV(x) ((GENMASK(23, 20) & (x)) >> 20) #define > > +UART_DIV13_EN BIT(30) #define SCU0_HPLL_PARAM 0x300 #define > > +SCU0_DPLL_PARAM 0x308 #define SCU0_MPLL_PARAM 0x310 #define > > +SCU0_D0CLK_PARAM 0x320 #define SCU0_D1CLK_PARAM 0x330 #define > > +SCU0_CRT0CLK_PARAM 0x340 #define SCU0_CRT1CLK_PARAM 0x350 > #define > > +SCU0_MPHYCLK_PARAM 0x360 > > It would be easier to read if these things were tabbed out a little. > > #define SCU0_MPHYCLK_PARAM 0x360 Thank, all those will update by tabbed. > > > + > > +/* SOC1 */ > > +#define SCU1_REVISION_ID 0x0 > > +#define REVISION_ID GENMASK(23, 16) > > +#define SCU1_CLK_STOP 0x240 > > +#define SCU1_CLK_STOP2 0x260 > > +#define SCU1_CLK_SEL1 0x280 > > +#define SCU1_CLK_SEL2 0x284 > > +#define UXCLK_MASK GENMASK(1, 0) > > +#define HUXCLK_MASK GENMASK(4, 3) > > +#define SCU1_HPLL_PARAM 0x300 > > +#define SCU1_APLL_PARAM 0x310 > > +#define SCU1_DPLL_PARAM 0x320 > > +#define SCU1_UXCLK_CTRL 0x330 > > +#define SCU1_HUXCLK_CTRL 0x334 > > +#define SCU1_MAC12_CLK_DLY 0x390 > > +#define SCU1_MAC12_CLK_DLY_100M 0x394 #define > SCU1_MAC12_CLK_DLY_10M > > +0x398 > > + > > +enum { > > + CLK_MUX, > > + CLK_PLL, > > + CLK_GATE, > > + CLK_MISC, > > + CLK_FIXED, > > + CLK_DIVIDER, > > + CLK_UART_PLL, > > + CLK_DIV_TABLE, > > + CLK_FIXED_FACTOR, > > + CLK_GATE_ASPEED, > > +}; > > + > > +struct ast2700_clk_info { > > + const char *name; > > + const char * const *parent_names; > > Please don't use strings for parent names. Sorry, do you mean use clk_parent_data struct for parent? +const struct clk_parent_data parent; /* For gate */ +const struct clk_parent_data *parents; /* For mux */ > > > + const struct clk_div_table *div_table; > > + unsigned long fixed_rate; > > + unsigned int mult; > > + unsigned int div; > > + u32 reg; > > + u32 flags; > > + u32 type; > > + u8 clk_idx; > > + u8 bit_shift; > > + u8 bit_width; > > + u8 num_parents; > > +}; > > + > [...] > > + > > +static const struct clk_div_table ast2700_clk_div_table2[] = { > > + { 0x0, 2 }, > > + { 0x1, 4 }, > > + { 0x2, 6 }, > > + { 0x3, 8 }, > > + { 0x4, 10 }, > > + { 0x5, 12 }, > > + { 0x6, 14 }, > > + { 0x7, 16 }, > > Isn't this the default divider setting for struct clk_divider? Sorry, I don't catch your point. the SoC do have default divider setting. But it can be modified. And also have different divider table setting. > > > + { 0 } > > +}; > > + > > +static const struct clk_div_table ast2700_clk_uart_div_table[] = { > > + { 0x0, 1 }, > > + { 0x1, 13 }, > > + { 0 } > [...] > > + .bit_shift = 23, > > + .bit_width = 3, > > + .div_table = ast2700_clk_div_table2, > > + }, > > + [SCU0_CLK_GATE_MCLK] = { > > + .type = CLK_GATE_ASPEED, > > + .name = "mclk-gate", > > + .parent_names = (const char *[]){ "soc0-mpll", }, > > + .reg = SCU0_CLK_STOP, > > + .clk_idx = 0, > > + .flags = CLK_IS_CRITICAL, > > + }, > > + [SCU0_CLK_GATE_ECLK] = { > > + .type = CLK_GATE_ASPEED, > > + .name = "eclk-gate", > > + .parent_names = (const char *[]){ }, > > + .reg = SCU0_CLK_STOP, > > + .clk_idx = 1, > > + }, > > + [SCU0_CLK_GATE_2DCLK] = { > > + .type = CLK_GATE_ASPEED, > > + .name = "gclk-gate", > > + .parent_names = (const char *[]){ }, > > This has no parent? Why is parent_names set to an empty array? Due to I use clk->parent_names[0] for clk_hw_register_gate, const char *name parameter input. If null, that will cause panic for NULL point. > > > + .reg = SCU0_CLK_STOP, > > + .clk_idx = 2, > > + }, > > + [SCU0_CLK_GATE_VCLK] = { > [...] > > + > > +static struct clk_hw *ast2700_clk_hw_register_pll(int clk_idx, void > __iomem *reg, > > + const struct > ast2700_clk_info *clk, > > + struct > > +ast2700_clk_ctrl *clk_ctrl) { > > + int scu = clk_ctrl->clk_data->scu; > > + unsigned int mult, div; > > + u32 val; > > + > > + if (!scu && clk_idx == SCU0_CLK_HPLL) { > > + val = readl(clk_ctrl->base + SCU0_HWSTRAP1); > > + if ((val & GENMASK(3, 2)) != 0) { > > + switch ((val & GENMASK(3, 2)) >> 2) { > > + case 1: > > + return > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, "soc0-hpll", > > + > NULL, 0, 1900000000); > > + case 2: > > + return > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, "soc0-hpll", > > + > NULL, 0, 1800000000); > > + case 3: > > + return > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, "soc0-hpll", > > + > NULL, 0, 1700000000); > > + default: > > + return ERR_PTR(-EINVAL); > > + } > > + } > > What if it is 0? Were we supposed to return an error? Why isn't HPLL a > different type of PLL so that this function can be broken up into two PLL > registration functions? Sorry, I miss 0: It will be another fix clk. It need add. And also I will separate another PLL registration function, add new enum clk type : CLK_HPLL_SOC0 > > > + } > > + > > + val = readl(reg); > > + > > + if (val & BIT(24)) { > > + /* Pass through mode */ > > + mult = 1; > > + div = 1; > > + } else { > > + u32 m = val & 0x1fff; > > + u32 n = (val >> 13) & 0x3f; > > + u32 p = (val >> 19) & 0xf; > > + > > + if (scu) { > > + mult = (m + 1) / (n + 1); > > + div = (p + 1); > > + } else { > > + if (clk_idx == SCU0_CLK_MPLL) { > > + mult = m / (n + 1); > > + div = (p + 1); > > + } else { > > + mult = (m + 1) / (2 * (n + 1)); > > + div = (p + 1); > > + } > > + } > > + } > > + > > + return devm_clk_hw_register_fixed_factor(clk_ctrl->dev, > clk->name, > > + > clk->parent_names[0], > > +0, mult, div); } > > + > [...] > > + > > +static int ast2700_soc_clk_probe(struct platform_device *pdev) { > > + struct ast2700_clk_data *clk_data; > > + struct ast2700_clk_ctrl *clk_ctrl; > > + struct clk_hw_onecell_data *clk_hw_data; > > + struct device *dev = &pdev->dev; > > + void __iomem *clk_base; > > + struct clk_hw **hws; > > + char *reset_name; > > + int ret; > > + int i; > > + > > + clk_ctrl = devm_kzalloc(dev, sizeof(*clk_ctrl), GFP_KERNEL); > > + if (!clk_ctrl) > > + return -ENOMEM; > > + clk_ctrl->dev = dev; > > + dev_set_drvdata(&pdev->dev, clk_ctrl); > > + > > + spin_lock_init(&clk_ctrl->lock); > > + > > + clk_base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(clk_base)) > > + return PTR_ERR(clk_base); > > + > > + clk_ctrl->base = clk_base; > > + > > + clk_data = (struct ast2700_clk_data > > + *)of_device_get_match_data(dev); > > Just > clk_data = device_get_match_data(dev); Will update. > > > + if (!clk_data) > > + return devm_of_platform_populate(dev); > > What is being populated? Isn't there always clk_data? Yes, it is always clk_data, I will modify to be following, is it ok? If(!clk_data) Return -ENODEV; > > > + > > + clk_ctrl->clk_data = clk_data; > > + reset_name = devm_kasprintf(dev, GFP_KERNEL, "reset%d", > > + clk_data->scu); > > + > > + clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws, > clk_data->nr_clks), > > + GFP_KERNEL); > > + if (!clk_hw_data) > > + return -ENOMEM; > > + > > + clk_hw_data->num = clk_data->nr_clks; > > + hws = clk_hw_data->hws; > > + > > + for (i = 0; i < clk_data->nr_clks; i++) { > > + const struct ast2700_clk_info *clk = > &clk_data->clk_info[i]; > > + void __iomem *reg = clk_ctrl->base + clk->reg; > > + > > + if (clk->type == CLK_FIXED) { > > + hws[i] = devm_clk_hw_register_fixed_rate(dev, > clk->name, NULL, > > + > clk->flags, clk->fixed_rate); > > + } else if (clk->type == CLK_FIXED_FACTOR) { > > + hws[i] = > devm_clk_hw_register_fixed_factor(dev, clk->name, > > + > clk->parent_names[0], clk->flags, > > + > clk->mult, clk->div); > > + } else if (clk->type == CLK_PLL) { > > + hws[i] = ast2700_clk_hw_register_pll(i, reg, > clk, clk_ctrl); > > + } else if (clk->type == CLK_UART_PLL) { > > + hws[i] = ast2700_clk_hw_register_uartpll(i, > reg, clk, clk_ctrl); > > + } else if (clk->type == CLK_MUX) { > > + hws[i] = devm_clk_hw_register_mux(dev, > > + clk->name, clk->parent_names, > > Please don't use strings for parent_names. Use clk_hw pointers or DT indices. Use clk_pareent_data is it ok ? +const struct clk_parent_data parent; /* For gate */ +const struct clk_parent_data *parents; /* For mux */ > > > + > clk->num_parents, clk->flags, reg, > > + > clk->bit_shift, clk->bit_width, > > + 0, > &clk_ctrl->lock); > > + } else if (clk->type == CLK_MISC) { > > + hws[i] = ast2700_clk_hw_register_misc(i, reg, > clk, clk_ctrl); > > + } else if (clk->type == CLK_DIVIDER) { > > + hws[i] = devm_clk_hw_register_divider(dev, > clk->name, clk->parent_names[0], > > + > clk->flags, reg, clk->bit_shift, > > + > clk->bit_width, 0, > > + > &clk_ctrl->lock); > > + } else if (clk->type == CLK_DIV_TABLE) { > > + hws[i] = clk_hw_register_divider_table(dev, > clk->name, clk->parent_names[0], > > + > clk->flags, reg, clk->bit_shift, > > + > clk->bit_width, 0, > > + > clk->div_table, &clk_ctrl->lock); > > + } else if (clk->type == CLK_GATE_ASPEED) { > > + hws[i] = ast2700_clk_hw_register_gate(dev, > clk->name, clk->parent_names[0], > > + > clk->flags, reg, clk->clk_idx, > > + > clk->flags, &clk_ctrl->lock); > > + } else { > > + hws[i] = clk_hw_register_gate(dev, clk->name, > clk->parent_names[0], > > + > clk->flags, reg, clk->clk_idx, clk->flags, > > + > &clk_ctrl->lock); > > + } > > + > > + if (IS_ERR(hws[i])) > > + return PTR_ERR(hws[i]); > > + } > > + > > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > clk_hw_data); > > + if (ret) > > + return ret; > > + > > + return aspeed_reset_controller_register(dev, clk_base, > > +reset_name); } > > + > > +static const struct ast2700_clk_data ast2700_clk0_data = { > > + .scu = 0, > > + .nr_clks = ARRAY_SIZE(ast2700_scu0_clk_info), > > + .clk_info = ast2700_scu0_clk_info, }; > > + > > +static const struct ast2700_clk_data ast2700_clk1_data = { > > + .scu = 1, > > + .nr_clks = ARRAY_SIZE(ast2700_scu1_clk_info), > > + .clk_info = ast2700_scu1_clk_info, }; > > + > > +static const struct of_device_id ast2700_scu_match[] = { > > + { .compatible = "aspeed,ast2700-scu0", .data = > &ast2700_clk0_data }, > > + { .compatible = "aspeed,ast2700-scu1", .data = > &ast2700_clk1_data }, > > + { /* sentinel */ } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, ast2700_scu_match); > > + > > +static struct platform_driver ast2700_scu_driver = { > > + .driver = { > > + .name = "clk-ast2700", > > + .of_match_table = ast2700_scu_match, > > + }, > > +}; > > + > > +builtin_platform_driver_probe(ast2700_scu_driver, > > +ast2700_soc_clk_probe); > > Use module_platform_driver_probe() and make the config tristate. I don't see > what's preventing this from being a module. Will update.