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
