RE: [PATCH v14 3/3] clk: aspeed: add AST2700 clock driver

Ryan Chen
2025-10-01T09:35:08Z

> Subject: RE: [PATCH v14 3/3] clk: aspeed: add AST2700 clock driver
>
> > Subject: Re: [PATCH v14 3/3] clk: aspeed: add AST2700 clock driver
> >
> > Quoting Ryan Chen (2025-09-16 19:05:39)
> > > diff --git a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c
> > > new file mode 100644 index 000000000000..7766bc17876f
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-ast2700.c
> > > @@ -0,0 +1,1139 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2024 ASPEED Technology Inc.
> > > + * Author: Ryan Chen <ryan_chen@aspeedtech.com> */ #include
> > > +<linux/auxiliary_bus.h> #include <linux/bitfield.h> #include
> > > +<linux/clk-provider.h> #include <linux/io.h> #include
> > > +<linux/mod_devicetable.h> #include <linux/of_platform.h>
> >
> > Is this include used?
>
> Will remove in next version.
>
> >
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/units.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)
> >
> > Please inline these where they're used to make it more readable and shorter.
> > We don't have to look at the definition of 192MHZ to know that it is
> > 192 MHz
>
> OK, I will remove it to inline.
> >
> > > +
> > > +static const struct clk_div_table ast2700_clk_uart_div_table[] = {
> > > + { 0x0, 1 },
> > > + { 0x1, 13 },
> > > + { 0 }
> > > +};
> > > +
> > > +static const struct clk_parent_data soc0_clkin[] = {
> > > + { .fw_name = "soc0-clkin", .name = "soc0-clkin" },
> >
> > This is wrong. Please use clk_hw pointers, or the .index member of
> > 'struct clk_parent_data' instead of having both the .fw_name and .name
> > members set in the parent data. The .fw_name should match some string
> > in the DT binding for this device. The .name shouldn't be used unless
> > we're migrating existing code from string based parent descriptions to
> > clk_hw or clk_parent_data structures.
> >
> Understood your concern, do you have some reference that I can refer?

Hello Stephen,
After looking deeper, the devm_clk_hw_register_fixed_factor use parent_name
https://github.com/torvalds/linux/blob/master/drivers/clk/clk-fixed-factor.c#L293-L302
Do you mean no longer use this function anymore?

Ryan

>
> > > +
> > > +static const struct clk_parent_data uart13clk[] = {
> >
> > This isn't used.
>
> Will remove
>
> >
> > > + { .fw_name = "uart13clk", .name = "uart13clk" }, };
> > > +
> > > +static const struct clk_parent_data uart14clk[] = {
> >
> > This isn't used.
>
> Will remove
> >
> > > + { .fw_name = "uart14clk", .name = "uart14clk" }, };
> > > +
> > > +static const struct clk_parent_data soc1_i3c[] = {
> > > + { .fw_name = "soc1-i3c", .name = "soc1-i3c" }, };
> > > +
> > > +
> > > +static struct clk_hw *ast2700_clk_hw_register_hpll(void __iomem *reg,
> > > + const char
> > *name, const char *parent_name,
> > > + struct
> > > +ast2700_clk_ctrl *clk_ctrl) {
> > > + unsigned int mult, div;
> > > + u32 val;
> > > +
> > > + val = readl(clk_ctrl->base + SCU0_HWSTRAP1);
> > > + if ((readl(clk_ctrl->base) & REVISION_ID) && (val & BIT(3)))
> > > + {
> >
> > This can be read once during probe and then passed as an argument?
>
> My basic idea is keeping argument simple, that will add 2 new arguments in
> function.
> Does you intend me to modify?
> Otherwise, the struct ast2700_clk_ctrl have base, that should be ok?
>
> Or, could I just add into the struct ast2700_clk_ctrl?
>
> struct ast2700_clk_ctrl {
> ...
> + u32 scu0_hwstrap1_val;
> + u32 revision_id_val;
> };
>
> >
> > > + switch ((val & GENMASK(4, 2)) >> 2) {
> > > + case 2:
> > > + return
> > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, name, NULL,
> > > +
> > 0, 1800 * HZ_PER_MHZ);
> > > + case 3:
> > > + return
> > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, name, NULL,
> > > +
> > 0, 1700 * HZ_PER_MHZ);
> > > + case 6:
> > > + return
> > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, name, NULL,
> > > +
> > 0, 1200 * HZ_PER_MHZ);
> > > + case 7:
> > > + return
> > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, name, NULL,
> > > +
> > 0, 800 * HZ_PER_MHZ);
> > > + default:
> > > + return ERR_PTR(-EINVAL);
> > > + }
> > > + } else if ((val & GENMASK(3, 2)) != 0) {
> > > + switch ((val & GENMASK(3, 2)) >> 2) {
> > > + case 1:
> > > + return
> > devm_clk_hw_register_fixed_rate(clk_ctrl->dev, name, NULL,
> > > +
> > 0, 1900 * HZ_PER_MHZ);
> > > + case 2:
> > [...]
> > > +static int ast2700_clk_enable(struct clk_hw *hw) {
> > > + struct clk_gate *gate = to_clk_gate(hw);
> > > + u32 clk = BIT(gate->bit_idx);
> > > +
> > > + if (readl(gate->reg) & clk)
> > > + writel(clk, gate->reg + 0x04);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void ast2700_clk_disable(struct clk_hw *hw) {
> > > + struct clk_gate *gate = to_clk_gate(hw);
> > > + u32 clk = BIT(gate->bit_idx);
> > > +
> > > + /* Clock is set to enable, so use write to set register */
> > > + writel(clk, gate->reg);
> > > +}
> > > +
> > > +static const struct clk_ops ast2700_clk_gate_ops = {
> > > + .enable = ast2700_clk_enable,
> > > + .disable = ast2700_clk_disable,
> > > + .is_enabled = ast2700_clk_is_enabled, };
> > > +
> > > +static struct clk_hw *ast2700_clk_hw_register_gate(struct device
> > > +*dev,
> > const char *name,
> > > + const struct
> > clk_parent_data *parent,
> > > + void
> __iomem
> > *reg, u8 clock_idx,
> > > + unsigned
> long
> > > +clk_gate_flags, spinlock_t *lock) {
> > > + struct clk_gate *gate;
> > > + struct clk_hw *hw;
> > > + struct clk_init_data init;
> > > + int ret = -EINVAL;
> > > +
> > > + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> > > + if (!gate)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + init.name = name;
> > > + init.ops = &ast2700_clk_gate_ops;
> > > + init.flags = clk_gate_flags;
> > > + init.parent_names = parent ? &parent->name : NULL;
> >
> > Don't use string names to describe parent child relationships.
> >
> > > + init.num_parents = parent ? 1 : 0;
> > > +
> > > + gate->reg = reg;
> > > + gate->bit_idx = clock_idx;
> > > + gate->flags = 0;
> > > + gate->lock = lock;
> > > + gate->hw.init = &init;
> > > +
> > > + hw = &gate->hw;
> > > + ret = clk_hw_register(dev, hw);
> > > + if (ret) {
> > > + kfree(gate);
> > > + hw = ERR_PTR(ret);
> > > + }
> > > +
> > > + return hw;
> > > +}
> > > +
> > > +static void ast2700_soc1_configure_i3c_clk(struct ast2700_clk_ctrl
> > > +*clk_ctrl) {
> > > + if (readl(clk_ctrl->base + SCU1_REVISION_ID) & REVISION_ID) {
> > > + u32 val;
> > > +
> > > + /* I3C 250MHz = HPLL/4 */
> > > + val = readl(clk_ctrl->base + SCU1_CLK_SEL2) &
> > ~SCU1_CLK_I3C_DIV_MASK;
> > > + val |= FIELD_PREP(SCU1_CLK_I3C_DIV_MASK,
> > SCU1_CLK_I3C_DIV(4));
> > > + writel(val, clk_ctrl->base + SCU1_CLK_SEL2);
> > > + }
> > > +}
> > > +
> > > +static int ast2700_soc_clk_probe(struct platform_device *pdev) {
> > > + const struct ast2700_clk_data *clk_data;
> > > + struct clk_hw_onecell_data *clk_hw_data;
> > > + struct ast2700_clk_ctrl *clk_ctrl;
> > > + struct device *dev = &pdev->dev;
> > > + struct auxiliary_device *adev;
> > > + 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 = device_get_match_data(dev);
> > > + 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;
> > > +
> > > + if (clk_data->scu)
> > > + ast2700_soc1_configure_i3c_clk(clk_ctrl);
> > > +
> > > + for (i = 0; i < clk_data->nr_clks; i++) {
> > > + const struct ast2700_clk_info *clk =
> > &clk_data->clk_info[i];
> > > + void __iomem *reg;
> > > +
> > > + if (clk->type == CLK_FIXED) {
> > > + const struct ast2700_clk_fixed_rate_data
> > > + *fixed_rate = &clk->data.rate;
> > > +
> > > + hws[i] =
> > > + devm_clk_hw_register_fixed_rate(dev,
> > clk->name, NULL, 0,
> > > +
> > fixed_rate->fixed_rate);
> > > + } else if (clk->type == CLK_FIXED_FACTOR) {
> > > + const struct ast2700_clk_fixed_factor_data
> > > + *factor = &clk->data.factor;
> > > +
> > > + hws[i] =
> > devm_clk_hw_register_fixed_factor(dev, clk->name,
> > > +
> > factor->parent->name,
> > > +
> > 0, factor->mult, factor->div);
> > > + } else if (clk->type == DCLK_FIXED) {
> > > + const struct ast2700_clk_pll_data *pll =
> > > + &clk->data.pll;
> > > +
> > > + reg = clk_ctrl->base + pll->reg;
> > > + hws[i] = ast2700_clk_hw_register_dclk(reg,
> > clk->name, clk_ctrl);
> > > + } else if (clk->type == CLK_HPLL) {
> > > + const struct ast2700_clk_pll_data *pll =
> > > + &clk->data.pll;
> > > +
> > > + reg = clk_ctrl->base + pll->reg;
> > > + hws[i] = ast2700_clk_hw_register_hpll(reg,
> > clk->name,
> > > +
> > pll->parent->name, clk_ctrl);
> > > + } else if (clk->type == CLK_PLL) {
> > > + const struct ast2700_clk_pll_data *pll =
> > > + &clk->data.pll;
> > > +
> > > + reg = clk_ctrl->base + pll->reg;
> > > + hws[i] = ast2700_clk_hw_register_pll(i, reg,
> > clk->name,
> > > +
> > pll->parent->name, clk_ctrl);
> > > + } else if (clk->type == CLK_UART_PLL) {
> > > + const struct ast2700_clk_pll_data *pll =
> > > + &clk->data.pll;
> > > +
> > > + reg = clk_ctrl->base + pll->reg;
> > > + hws[i] =
> > > + ast2700_clk_hw_register_uartpll(reg,
> > clk->name,
> > > +
> > pll->parent->name, clk_ctrl);
> > > + } else if (clk->type == CLK_MUX) {
> > > + const struct ast2700_clk_mux_data *mux =
> > > + &clk->data.mux;
> > > +
> > > + reg = clk_ctrl->base + mux->reg;
> > > + hws[i] =
> > devm_clk_hw_register_mux_parent_data_table(dev, clk->name,
> > > +
> > mux->parents,
> > > +
> > mux->num_parents, 0,
> > > +
> > reg, mux->bit_shift,
> > > +
> > mux->bit_width, 0,
> > > +
> > NULL, &clk_ctrl->lock);
> > > + } else if (clk->type == CLK_MISC) {
> > > + const struct ast2700_clk_pll_data *misc =
> > > + &clk->data.pll;
> > > +
> > > + reg = clk_ctrl->base + misc->reg;
> > > + hws[i] = ast2700_clk_hw_register_misc(i,
> > > + reg,
> > clk->name,
> > > +
> > misc->parent->name, clk_ctrl);
> > > + } else if (clk->type == CLK_DIVIDER) {
> > > + const struct ast2700_clk_div_data *div =
> > > + &clk->data.div;
> > > +
> > > + reg = clk_ctrl->base + div->reg;
> > > + hws[i] =
> > devm_clk_hw_register_divider_table(dev, clk->name,
> > > +
> > div->parent->name, 0,
> > > +
> > reg, div->bit_shift,
> > > +
> > div->bit_width, 0,
> > > +
> > div->div_table,
> > > +
> > &clk_ctrl->lock);
> > > + } else if (clk->type == CLK_GATE_ASPEED) {
> > > + const struct ast2700_clk_gate_data *gate =
> > > + &clk->data.gate;
> > > +
> > > + reg = clk_ctrl->base + gate->reg;
> > > + hws[i] = ast2700_clk_hw_register_gate(dev,
> > clk->name, gate->parent,
> > > +
> > reg, gate->bit, gate->flags,
> > > +
> > > + &clk_ctrl->lock);
> > > +
> > > + } else {
> > > + const struct ast2700_clk_gate_data *gate =
> > > + &clk->data.gate;
> > > +
> > > + reg = clk_ctrl->base + gate->reg;
> > > + hws[i] =
> > devm_clk_hw_register_gate_parent_data(dev, clk->name, gate->parent,
> > > +
> > 0, reg, clk->clk_idx, 0,
> > > +
> > &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;
> > > +
> > > + adev = devm_auxiliary_device_create(dev, reset_name,
> > > + (__force
> > void *)clk_base);
> > > + if (!adev)
> > > + return -ENODEV;
> > > +
> > > + return 0;
> > > +}
> > > +
> > [...]
> > > +static int __init clk_ast2700_init(void) {
> > > + return platform_driver_register(&ast2700_scu_driver);
> > > +}
> > > +arch_initcall(clk_ast2700_init);
> >
> > Just use module_platform_driver()?
> The clock driver should be core in the SoC platform. Should I modify it to
> platform module?