Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] clk: qcom: gp1/2/3_clk_src CCF parent incorrectly mapped to gpll0_vote instead of xo - [High] clk: qcom: reset map targets CBCR enable bits instead of BCR reset registers --
commit f6425874e2d9adc87ee0fd906bb71cd217405222 Author: Muzaffer Kadir <[email protected]> clk: qcom: Add global clock controller driver for MSM8952 This commit adds the global clock controller (GCC) driver for the Qualcomm MSM8952 SoC. It provides clocks, resets, and power domains needed for various hardware components on the device. 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 --- a/drivers/clk/qcom/gcc-msm8952.c +++ b/drivers/clk/qcom/gcc-msm8952.c [ ... ] > static struct clk_rcg2 gp1_clk_src = { > .cmd_rcgr = 0x08004, > .mnd_width = 8, > .hid_width = 5, > .parent_map = gcc_xo_map, > .freq_tbl = ftbl_gcc_gp1_3_clk, > .clkr.hw.init = &(struct clk_init_data){ > .name = "gp1_clk_src", > .parent_hws = (const struct clk_hw *[]) { > &gpll0_vote.hw, > }, [Severity: High] Does this code map to the wrong parent hardware clock? The parent_map is set to gcc_xo_map (which implies hardware source 0, typically XO), but the CCF parent_hws array points to &gpll0_vote.hw (800MHz) instead of the XO reference. This causes the Common Clock Framework to report the parent rate as 800MHz while the hardware operates at 19.2MHz. Consequently, downstream drivers configuring these clocks may compute incorrect hardware dividers, leading to severe underclocking, and it will keep gpll0_vote enabled unnecessarily. > .num_parents = 1, > .ops = &clk_rcg2_ops, > }, > }; > > static struct clk_rcg2 gp2_clk_src = { > .cmd_rcgr = 0x09004, > .mnd_width = 8, > .hid_width = 5, > .parent_map = gcc_xo_map, > .freq_tbl = ftbl_gcc_gp1_3_clk, > .clkr.hw.init = &(struct clk_init_data){ > .name = "gp2_clk_src", > .parent_hws = (const struct clk_hw *[]) { > &gpll0_vote.hw, > }, [Severity: High] Does the same parent mismatch apply here to gp2_clk_src as well? > .num_parents = 1, > .ops = &clk_rcg2_ops, > }, > }; > > static struct clk_rcg2 gp3_clk_src = { > .cmd_rcgr = 0x0a004, > .mnd_width = 8, > .hid_width = 5, > .parent_map = gcc_xo_map, > .freq_tbl = ftbl_gcc_gp1_3_clk, > .clkr.hw.init = &(struct clk_init_data){ > .name = "gp3_clk_src", > .parent_hws = (const struct clk_hw *[]) { > &gpll0_vote.hw, > }, [Severity: High] And similarly, does gp3_clk_src also point to gpll0_vote.hw instead of xo? [ ... ] > 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] Are we targeting the Clock Branch Control Register (CBCR) here instead of the Block Control Register (BCR)? By omitting .bit, the reset framework defaults to bit 0, which is the clock enable bit for CBCRs. When a subsystem requests a block reset, the driver will toggle the clock enable bit instead of actually resetting the hardware block. This corrupts the clock's enabled/disabled state behind the framework's back and fails to reset the hardware. > [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 CAMSS entries also incorrectly target CBCR registers instead of their respective BCR hardware reset registers? Similar to the USB reset above, this could break hardware state recovery for fundamental blocks by toggling clock enablements instead. > [RST_MSS_BCR] = { 0x71000 }, > [RST_MDSS_BCR] = { 0x4d074 }, > }; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260916-msm8952-initial-support-v2-0-798c2602d...@mainlining.org?part=2
