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

[Brian Masney <bmasney@redhat.com>]

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@aspeedtech.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@redhat.com>