| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<OS8PR06MB754123A3C254D9ABD70A5117F21FA@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Thu, 25 Sep 2025 03:15:53 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Stephen Boyd <sboyd@...nel.org>, Andrew Jeffery
<andrew@...econstruct.com.au>, Conor Dooley <conor+dt@...nel.org>, Joel
Stanley <joel@....id.au>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Michael
Turquette <mturquette@...libre.com>, Mo Elbadry <elbadrym@...gle.com>,
Philipp Zabel <p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>, Rom
Lemarchand <romlem@...gle.com>, William Kennington <wak@...gle.com>, Yuxiao
Zhang <yuxiaozhang@...gle.com>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "dkodihalli@...dia.com"
<dkodihalli@...dia.com>, "leohu@...dia.com" <leohu@...dia.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "linux-clk@...r.kernel.org"
<linux-clk@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "spuranik@...dia.com" <spuranik@...dia.com>,
"wthai@...dia.com" <wthai@...dia.com>
CC: Brian Masney <bmasney@...hat.com>
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@...eedtech.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?
> > +
> > +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?
Powered by blists - more mailing lists