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

[Ryan Chen]

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 at 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.
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 at redhat.com>