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>