lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Zv0ijCB6GfJEEJID@pineapple>
Date: Wed, 2 Oct 2024 10:38:04 +0000
From: Yao Zi <ziyao@...root.org>
To: Heiko Stübner <heiko@...ech.de>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Celeste Liu <CoelacanthusHex@...il.com>
Subject: Re: [PATCH 6/8] clk: rockchip: Add clock controller driver for
 RK3528 SoC

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@...root.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)

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ