linux-kernel - Re: [PATCH v12 3/3] clk: aspeed: add AST2700 clock driver

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: <aMGHvHf6BPrJD1pC@x1>
Date: Wed, 10 Sep 2025 10:14:20 -0400
From: Brian Masney <bmasney@...hat.com>
To: Ryan Chen <ryan_chen@...eedtech.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-arm-kernel@...ts.infradead.org, linux-aspeed@...ts.ozlabs.org,
	devicetree@...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,
	leohu@...dia.com, dkodihalli@...dia.com, spuranik@...dia.com
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.

> +		}
> +	}
> +
> +	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);
> +			} else {
> +				mult = (m + 1) / (2 * (n + 1));
> +				div = (p + 1);

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:

Reviewed-by: Brian Masney <bmasney@...hat.com>