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

[Yao Zi]

On Wed, Oct 02, 2024 at 12:21:29PM +0200, Heiko Stübner wrote:
> 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@xxxxxxxxxxx>
> > ---
> 
> [...]
> 
> > +	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)

I'm not sure whether it is okay so wrapped these lines. Thanks for
clarification.

> > +};
> > +
> > +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)

Have checked them when replying to the former mails. Reset code will be
largely refacted according to the recommended style in next revision.

Best regards,
Yao Zi