| 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: <287924eed186e3b6b52cd13bcf939ab6.sboyd@kernel.org>
Date: Wed, 30 Oct 2024 14:01:43 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: andrew@...econstruct.com.au, conor+dt@...nel.org, devicetree@...r.kernel.org, dmitry.baryshkov@...aro.org, joel@....id.au, krzk+dt@...nel.org, lee@...nel.org, linux-arm-kernel@...ts.infradead.org, linux-aspeed@...ts.ozlabs.org, linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, mturquette@...libre.com, p.zabel@...gutronix.de, robh@...nel.org, ryan_chen@...eedtech.com
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@...eedtech.com>
> + */
> +
> +#include <linux/auxiliary_bus.h>
Is this include used?
> +#include <linux/clk-provider.h>
> +#include <linux/of_address.h>
Is this include used?
> +#include <linux/of_device.h>
Probably should be mod_devicetable.h here
> +#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.
> +
> +#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
> +
> +/* 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.
> + 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?
> + { 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?
> + .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?
> + }
> +
> + 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);
> + if (!clk_data)
> + return devm_of_platform_populate(dev);
What is being populated? Isn't there always clk_data?
> +
> + 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.
> + 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.
Powered by blists - more mailing lists