Re: [PATCH RFC 02/20] clk: qcom: Add global clock controller driver for MSM8952

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Duplicate CCF clock branches are registered for the same physical hardware registers (`0x58044` and `0x58048`), leading to state desynchronization.
- [High] Incorrect parent map for `gp1_clk_src`, `gp2_clk_src`, and `gp3_clk_src` causes rate setting to unconditionally fail.
- [High] Hardware resets are improperly mapped to Clock Branch Control Registers (CBCR) instead of Block Control Registers (BCR), causing them to toggle clock enablement instead of performing a hardware reset.
--

commit b4dadd423234c197ee41263628a67b773be1b25f
Author: Muzaffer Kadir <muzafferkadir@mainlining.org>

clk: qcom: Add global clock controller driver for MSM8952

This commit adds the global clock controller driver for MSM8952, providing
clocks, resets, and power domains for various SoC components. It ports the
GCC driver for the 8952 based on a downstream reference.

Link: https://android.googlesource.com/kernel/msm.git/+/refs/tags/android-wear-7.0.0_r0.3/drivers/clk/qcom/clock-gcc-8952.c

> diff --git a/drivers/clk/qcom/gcc-msm8952.c b/drivers/clk/qcom/gcc-msm8952.c
> new file mode 100644
> index 0000000000000..e75bf51ad4087
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-msm8952.c
[ ... ]
> +static const struct freq_tbl ftbl_gcc_gp1_3_clk[] = {
> +	F(19200000, P_XO, 1, 0, 0),
> +	{ }
> +};
> +
> +static struct clk_rcg2 gp1_clk_src = {
> +	.cmd_rcgr = 0x08004,
> +	.mnd_width = 8,
> +	.hid_width = 5,
> +	.parent_map = gcc_gpll0_map,

[Severity: High]
Will this cause rate setting to fail for the general-purpose clocks?
The frequency table ftbl_gcc_gp1_3_clk requests P_XO as the parent source,
but the parent map gcc_gpll0_map does not contain an entry for P_XO. When the
clock framework attempts to configure the parent, it will search for P_XO and
return -ENOENT. Does this parent map need to be updated to one that includes
P_XO?

> +	.freq_tbl = ftbl_gcc_gp1_3_clk,
[ ... ]
> +static struct clk_rcg2 gp2_clk_src = {
> +	.cmd_rcgr = 0x09004,
> +	.mnd_width = 8,
> +	.hid_width = 5,
> +	.parent_map = gcc_gpll0_map,

[Severity: High]
Does this have the same parent map issue as gp1_clk_src?

> +	.freq_tbl = ftbl_gcc_gp1_3_clk,
[ ... ]
> +static struct clk_rcg2 gp3_clk_src = {
> +	.cmd_rcgr = 0x0a004,
> +	.mnd_width = 8,
> +	.hid_width = 5,
> +	.parent_map = gcc_gpll0_map,

[Severity: High]
Does this also need to be updated to a parent map containing P_XO?

> +	.freq_tbl = ftbl_gcc_gp1_3_clk,
[ ... ]
> +static struct clk_branch gcc_camss_vfe0_ahb_clk = {
> +	.halt_reg = 0x58044,
[ ... ]
> +static struct clk_branch gcc_camss_vfe0_axi_clk = {
> +	.halt_reg = 0x58048,
[ ... ]
> +static struct clk_branch gcc_camss_vfe_ahb_clk = {
> +	.halt_reg = 0x58044,

[Severity: High]
Are these duplicate clock branches intentional? Both gcc_camss_vfe0_ahb_clk
and gcc_camss_vfe_ahb_clk map to the exact same physical hardware register
(0x58044). If different consumer drivers request these two different clock
abstractions for the same hardware bit, couldn't one driver disable the
clock while the other expects it to be on, causing unexpected hardware
failures?

> +	.clkr = {
> +		.enable_reg = 0x58044,
[ ... ]
> +static struct clk_branch gcc_camss_vfe_axi_clk = {
> +	.halt_reg = 0x58048,

[Severity: High]
Similarly, does this duplicate the gcc_camss_vfe0_axi_clk definition for
register 0x58048? This might lead to the same state desynchronization and
reference counting issues in the clock framework.

> +	.clkr = {
> +		.enable_reg = 0x58048,
[ ... ]
> +static const struct qcom_reset_map gcc_msm8952_resets[] = {
> +	[RST_CAMSS_MICRO_BCR]		= { 0x56008 },
> +	[RST_USB_HS_BCR]		= { 0x41000 },
> +	[RST_QUSB2_PHY_BCR]		= { 0x4103c },
> +	[RST_USB_HS_PHY_CFG_AHB_CBCR]	= { 0x41030 },

[Severity: High]
Could this reset mapping cause unintended behavior? It maps to a Clock
Branch Control Register (CBCR) instead of a Block Control Register (BCR).
Without specifying a bit, it defaults to bit 0, which is the CLK_ENABLE bit
for the CBCR. When a peripheral driver triggers this reset, will it
accidentally toggle the clock enable state rather than performing a true
hardware reset?

> +	[RST_USB2_HS_PHY_ONLY_BCR]	= { 0x41034 },
> +	[RST_USB_FS_BCR]		= { 0x3f000 },
> +	[RST_CAMSS_CSI1PIX_CBCR]	= { 0x4f058 },
> +	[RST_CAMSS_CSI_VFE1_CBCR]	= { 0x58074 },
> +	[RST_CAMSS_VFE1_CBCR]		= { 0x5805c },
> +	[RST_CAMSS_CPP_CBCR]		= { 0x5803c },

[Severity: High]
Do these entries also incorrectly map to CBCR registers rather than BCR
registers, potentially toggling clock enablement instead of performing a
hardware reset?

> +	[RST_MSS_BCR]			= { 0x71000 },
> +	[RST_MDSS_BCR]			= { 0x4d074 },
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-msm8952-initial-support-v1-0-0e742578b524@mainlining.org?part=2