| 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: <175848665572.4354.4488545150485789314@lazor>
Date: Sun, 21 Sep 2025 13:30:55 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: 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, dkodihalli@...dia.com, leohu@...dia.com, linux-arm-kernel@...ts.infradead.org, linux-aspeed@...ts.ozlabs.org, linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, ryan_chen <ryan_chen@...eedtech.com>, spuranik@...dia.com, wthai@...dia.com
Cc: Brian Masney <bmasney@...hat.com>
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?
> +#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
> +
> +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.
> +
> +static const struct clk_parent_data uart13clk[] = {
This isn't used.
> + { .fw_name = "uart13clk", .name = "uart13clk" },
> +};
> +
> +static const struct clk_parent_data uart14clk[] = {
This isn't used.
> + { .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?
> + 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()?
Powered by blists - more mailing lists