| 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:
<OS8PR06MB7541F0274104DB687F6BDE51F209A@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Thu, 11 Sep 2025 09:02:15 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Brian Masney <bmasney@...hat.com>
CC: Michael Turquette <mturquette@...libre.com>, Stephen Boyd
<sboyd@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>, Joel Stanley
<joel@....id.au>, Andrew Jeffery <andrew@...econstruct.com.au>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, "linux-clk@...r.kernel.org"
<linux-clk@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
<linux-aspeed@...ts.ozlabs.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, Mo Elbadry <elbadrym@...gle.com>, Rom
Lemarchand <romlem@...gle.com>, William Kennington <wak@...gle.com>, Yuxiao
Zhang <yuxiaozhang@...gle.com>, "wthai@...dia.com" <wthai@...dia.com>,
"leohu@...dia.com" <leohu@...dia.com>, "dkodihalli@...dia.com"
<dkodihalli@...dia.com>, "spuranik@...dia.com" <spuranik@...dia.com>
Subject: RE: [PATCH v12 3/3] clk: aspeed: add AST2700 clock driver
Hello Brian,
Thanks a lot for review.
> Subject: Re: [PATCH v12 3/3] clk: aspeed: add AST2700 clock driver
>
> Hi Ryan,
>
> On Tue, Jul 08, 2025 at 01:29:09PM +0800, Ryan Chen wrote:
> > Add AST2700 clock controller driver and also use axiliary device
> > framework register the reset controller driver.
> > Due to clock and reset using the same register region.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@...eedtech.com>
>
> I just have a few very minor style comments below. Otherwise the driver looks
> good to me.
>
> > +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))) {
> > + 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:
> > + 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);
> > + default:
> > + return ERR_PTR(-EINVAL);
> > + }
> > + } else {
> > + 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;
> > +
> > + mult = (m + 1) / (2 * (n + 1));
> > + div = (p + 1);
>
> The ( ) is unnecessary here.
Will update div = p + 1; in next version.
>
> > + }
> > + }
> > +
> > + return devm_clk_hw_register_fixed_factor(clk_ctrl->dev, name,
> > +parent_name, 0, mult, div); }
> > +
> > +static struct clk_hw *ast2700_clk_hw_register_pll(int clk_idx, void
> __iomem *reg,
> > + const char *name, const char *parent_name,
> > + struct ast2700_clk_ctrl *clk_ctrl) {
> > + int scu = clk_ctrl->clk_data->scu;
> > + unsigned int mult, div;
> > + u32 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);
Will update in next version.
> > + } else {
> > + mult = (m + 1) / (2 * (n + 1));
> > + div = (p + 1);
Will update in next version.
>
> The ( ) is unnecessary on div on the three places above.
>
> > +static void ast2700_soc1_configure_i3c_clk(struct ast2700_clk_ctrl
> > +*clk_ctrl) {
> > + if (readl(clk_ctrl->base + SCU1_REVISION_ID) & REVISION_ID)
> > + /* I3C 250MHz = HPLL/4 */
> > + writel((readl(clk_ctrl->base + SCU1_CLK_SEL2) &
> > + ~SCU1_CLK_I3C_DIV_MASK) |
> > + FIELD_PREP(SCU1_CLK_I3C_DIV_MASK,
> > + SCU1_CLK_I3C_DIV(4)),
> > + clk_ctrl->base + SCU1_CLK_SEL2);
>
> This block is hard to read. What do you think about this instead?
>
> 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);
> }
>
> With those addressed:
Will update in next version.
>
> Reviewed-by: Brian Masney <bmasney@...hat.com>
Powered by blists - more mailing lists