Re: [PATCH 2/3] dt-bindings: reset: qcom: add mmcc-msm8660 reset IDs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unmapped GDSC reset IDs (GFX2D0_AHB_RESET, GFX2D1_AHB_RESET) cause memory corruption at register 0x0.
- [Low] Typographical errors in newly defined reset ID macro names.
--

commit 318183bccf1215fa02d8a5a344be5c0173d40944
Author: Herman van Hazendonk <github.com@herrie.org>

dt-bindings: reset: qcom: add mmcc-msm8660 reset IDs

This commit adds the dt-binding reset-ID header for the MSM8x60 family
(MSM8260/MSM8660/APQ8060) MMCC reset lines. It exposes per-IP software-reset
bits with IDs matching the numeric values from the mmcc-msm8960.h bindings.

> diff --git a/include/dt-bindings/reset/qcom,mmcc-msm8660.h b/include/dt-bindings/reset/qcom,mmcc-msm8660.h
> new file mode 100644
> index 0000000000000..c3ffd57834c97
> --- /dev/null
> +++ b/include/dt-bindings/reset/qcom,mmcc-msm8660.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> + *
> + * Reset bindings for the MSM8x60 family (MSM8260/MSM8660/APQ8060) Multimedia Clock
> + * Controller (MMCC).
> + *
> + * MSM8260, MSM8660 and APQ8060 are the same Scorpion-class MSM8x60 SoC
> + * with different bin/feature labels. MSM8960 is a newer generation (Krait)
> + * — its reset bindings live in
> + * <dt-bindings/reset/qcom,mmcc-msm8960.h> and must not be reused here.
> + *
> + * IDs intentionally match the numeric values used by the original shared
> + * mmcc-msm8960.h so the driver's qcom_reset_map array indexing is preserved;
> + * only the resets actually implemented by mmcc-msm8660.c are defined.
> + */
> +
> +#ifndef _DT_BINDINGS_RESET_MSM_MMCC_8660_H
> +#define _DT_BINDINGS_RESET_MSM_MMCC_8660_H
> +
> +#define VPE_AXI_RESET					0
> +#define IJPEG_AXI_RESET					1
> +#define MPD_AXI_RESET					2

[Severity: Low]
This isn't a bug, but should this be MDP_AXI_RESET instead of MPD? Propagating
misspelled macro names from the older platform might create confusion without
providing any ABI benefits.

> +#define VFE_AXI_RESET					3
> +#define SP_AXI_RESET					4
> +#define VCODEC_AXI_RESET				5
> +#define ROT_AXI_RESET					6
> +#define VCODEC_AXI_A_RESET				7
> +#define VCODEC_AXI_B_RESET				8
> +#define FAB_S3_AXI_RESET				9
> +#define FAB_S2_AXI_RESET				10
> +#define FAB_S1_AXI_RESET				11
> +#define FAB_S0_AXI_RESET				12
> +#define SMMU_GFX3D_ABH_RESET				13

[Severity: Low]
This isn't a bug, but should this be SMMU_GFX3D_AHB_RESET instead of ABH?

> +#define SMMU_VPE_AHB_RESET				14
> +#define SMMU_VFE_AHB_RESET				15
> +#define SMMU_ROT_AHB_RESET				16
> +#define SMMU_VCODEC_B_AHB_RESET				17
> +#define SMMU_VCODEC_A_AHB_RESET				18
> +#define SMMU_MDP1_AHB_RESET				19
> +#define SMMU_MDP0_AHB_RESET				20
> +#define SMMU_JPEGD_AHB_RESET				21
> +#define SMMU_IJPEG_AHB_RESET				22
> +#define APU_AHB_RESET					25
> +#define CSI_AHB_RESET					26
> +#define TV_ENC_AHB_RESET				27
> +#define VPE_AHB_RESET					28
> +#define FABRIC_AHB_RESET				29
> +#define GFX2D0_AHB_RESET				30
> +#define GFX2D1_AHB_RESET				31

[Severity: High]
Are these reset IDs missing from the mmcc_msm8660_resets array in the driver?

Looking at drivers/clk/qcom/mmcc-msm8660.c, gfx2d0_gdsc and gfx2d1_gdsc specify
.resets using GFX2D0_AHB_RESET (30) and GFX2D1_AHB_RESET (31):

drivers/clk/qcom/mmcc-msm8660.c:gfx2d0_gdsc
    .resets = (unsigned int []){ GFX2D0_AHB_RESET },

However, indices 30 and 31 appear to be skipped in the mmcc_msm8660_resets array:

drivers/clk/qcom/mmcc-msm8660.c:mmcc_msm8660_resets() {
    ...
    [FABRIC_AHB_RESET] = { 0x020c, 13 },
    [GFX3D_AHB_RESET] = { 0x020c, 10 },
    ...
}

Because omitted indices in a static array are implicitly zero-initialized,
accesses to rst->reset_map[30] and rst->reset_map[31] would yield { .reg = 0,
.bit = 0 }.

During GDSC power-on/off, qcom_reset_set_assert() uses these zeroed entries to
perform a read-modify-write that erroneously toggles bit 0 of register 0x0000.
Could this regression silently corrupt clock controller state instead of
resetting the intended block?

> +#define GFX3D_AHB_RESET					32
> +#define HDMI_AHB_RESET					33
> +#define MSSS_IMEM_AHB_RESET				34

[Severity: Low]
This isn't a bug, but would it make sense to name this MMSS_IMEM_AHB_RESET
instead of MSSS?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602043623.285901-1-github.com@herrie.org?part=2