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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <SI6PR06MB7535F5D22E3FCCF5C610B307F2552@SI6PR06MB7535.apcprd06.prod.outlook.com>
Date: Thu, 31 Oct 2024 07:24:39 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Stephen Boyd <sboyd@...nel.org>, "andrew@...econstruct.com.au"
	<andrew@...econstruct.com.au>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.org>, "joel@....id.au"
	<joel@....id.au>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>, "lee@...nel.org"
	<lee@...nel.org>, "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>, "mturquette@...libre.com"
	<mturquette@...libre.com>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"robh@...nel.org" <robh@...nel.org>
Subject: RE: [PATCH v7 3/3] clk: aspeed: add AST2700 clock driver.

> 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?

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. 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ