Re: [PATCH 6/8] clk: rockchip: Add clock controller driver for RK3528 SoC

["Heiko Stübner" <heiko@sntech.de>]

Am Dienstag, 1. Oktober 2024, 06:24:00 CEST schrieb Yao Zi:
> Add clock tree definition for RK3528. Similar to previous Rockchip
> SoCs, clock controller shares MMIO region with reset controller and
> they are probed together.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---

[...]

> +	GATE(ACLK_DDR_UPCTL, "aclk_ddr_upctl", "clk_ddrc_src", CLK_IS_CRITICAL,
> +	     RK3528_CLKGATE_CON(45), 11, GFLAGS),
> +	GATE(CLK_DDR_UPCTL, "clk_ddr_upctl", "clk_ddrc_src", CLK_IS_CRITICAL,
> +	     RK3528_CLKGATE_CON(45), 12, GFLAGS),
> +	GATE(CLK_DDRMON, "clk_ddrmon", "clk_ddrc_src", CLK_IS_CRITICAL,
> +	     RK3528_CLKGATE_CON(45), 13, GFLAGS),
> +	GATE(ACLK_DDR_SCRAMBLE, "aclk_ddr_scramble", "clk_ddrc_src",
> +	     CLK_IS_CRITICAL, RK3528_CLKGATE_CON(45), 14, GFLAGS),
> +	GATE(ACLK_SPLIT, "aclk_split", "clk_ddrc_src", CLK_IS_CRITICAL,
> +	     RK3528_CLKGATE_CON(45), 15, GFLAGS),
> +
> +	/* gpu */
> +	COMPOSITE_NODIV(ACLK_GPU_ROOT, "aclk_gpu_root",
> +			mux_500m_300m_100m_24m_p, CLK_IS_CRITICAL,
> +			RK3528_CLKSEL_CON(76), 0, 2, MFLAGS,
> +			RK3528_CLKGATE_CON(34), 0, GFLAGS),

Please keep the styling intact for all branch definitions.
(this one taken as an example, but applies to all)

I.e. if you look at the rk3588/rk3576/and everything else, you'll see
subsequent lines getting indented by 3 tabs all the time. For a large
set of definitions this makes it way easier to parse for the eye, than
having ever shifting offsets, when things get aligned to opening
parentheses.

Similarly, please also keep elements in their position, i.e. for the
aclk_gpu_root above, this would mean moving parents and CLK_IS_CRITICAL
up to the parent line.

(lines according to coding style are allowed up to 100 chars, and Rockchip
clock drivers sometimes exceed even that, because it makes handling the
clock drivers a lot easier)

> +};
> +
> +static int __init clk_rk3528_probe(struct platform_device *pdev)
> +{
> +	struct rockchip_clk_provider *ctx;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	unsigned long nr_branches = ARRAY_SIZE(rk3528_clk_branches);
> +	unsigned long nr_clks;
> +	void __iomem *reg_base;
> +
> +	nr_clks = rockchip_clk_find_max_clk_id(rk3528_clk_branches,
> +					       nr_branches) + 1;
> +
> +	pr_warn("%s: nr_clks = %lu\n", __func__, nr_clks);
> +
> +	reg_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(reg_base))
> +		return dev_err_probe(dev, PTR_ERR(reg_base),
> +				     "could not map cru region");
> +
> +	ctx = rockchip_clk_init(np, reg_base, nr_clks);
> +	if (IS_ERR(ctx))
> +		return dev_err_probe(dev, PTR_ERR(ctx),
> +				     "rockchip clk init failed");
> +
> +	rockchip_clk_register_plls(ctx, rk3528_pll_clks,
> +				   ARRAY_SIZE(rk3528_pll_clks),
> +				   RK3528_GRF_SOC_STATUS0);
> +	rockchip_clk_register_armclk(ctx, ARMCLK, "armclk",
> +				     mux_armclk, ARRAY_SIZE(mux_armclk),
> +				     &rk3528_cpuclk_data, rk3528_cpuclk_rates,
> +				     ARRAY_SIZE(rk3528_cpuclk_rates));
> +	rockchip_clk_register_branches(ctx, rk3528_clk_branches, nr_branches);
> +
> +	rockchip_register_softrst(np, 47, reg_base + RK3528_SOFTRST_CON(0),
> +				  ROCKCHIP_SOFTRST_HIWORD_MASK);

here you'll like also want to check how rk3576 + rk3588 handle how the reset-ids
are not matched to the register offsets anymore.
(see rst-rk3588.c for example)


Thanks a lot
Heiko