dt-bindings: clock: qcom: add mmcc-msm8660 clock IDs
Subsystems (5)
PATCH 1/3 dt-bindings: clock: qcom: add mmcc-msm8660 clock IDs
In reply to a message not in the archiveAdd the dt-binding clock-ID header for the MSM8x60 family (MSM8260/MSM8660/APQ8060) Multimedia Clock Controller (MMCC). The header enumerates the clocks and power-domains consumed by the multimedia subsystem (MDP4 display, Adreno A220 GPU, CAMSS image pipeline, VFE, Gemini JPEG, video codec, rotator, VPE and the GFX2D Z180 cores).
IDs intentionally match the numeric values used by the original shared mmcc-msm8960.h so the driver's clk array indexing is preserved; only the clocks actually implemented by mmcc-msm8660.c are defined.
--- include/dt-bindings/clock/qcom,mmcc-msm8660.h | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 include/dt-bindings/clock/qcom,mmcc-msm8660.h
include/dt-bindings/clock/qcom,mmcc-msm8660.h new file +126 -0
@@ -0,0 +1,126 @@ +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ +/* +* Copyright (c) 2013, The Linux Foundation. All rights reserved. +* +* Clock and power-domain 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 bindings live in +* <dt-bindings/clock/qcom,mmcc-msm8960.h> and must not be reused here. +* +* IDs below intentionally match the numeric values used by the original +* shared mmcc-msm8960.h so the driver's clk array indexing is preserved; +* only the clocks actually implemented by mmcc-msm8660.c are defined. +*/ + +#ifndef _DT_BINDINGS_CLK_MSM_MMCC_8660_H +#define _DT_BINDINGS_CLK_MSM_MMCC_8660_H + +#define TV_ENC_AHB_CLK 3 +#define AMP_AHB_CLK 4 +#define JPEGD_AHB_CLK 6 +#define GFX2D0_AHB_CLK 7 +#define DSI_S_AHB_CLK 8 +#define VPE_AHB_CLK 10 +#define SMMU_AHB_CLK 11 +#define HDMI_M_AHB_CLK 12 +#define VFE_AHB_CLK 13 +#define ROT_AHB_CLK 14 +#define VCODEC_AHB_CLK 15 +#define MDP_AHB_CLK 16 +#define DSI_M_AHB_CLK 17 +#define CSI0_AHB_CLK 18 +#define MMSS_IMEM_AHB_CLK 19 +#define IJPEG_AHB_CLK 20 +#define HDMI_S_AHB_CLK 21 +#define GFX3D_AHB_CLK 22 +#define GFX2D1_AHB_CLK 23 +#define JPEGD_AXI_CLK 28 +#define GMEM_AXI_CLK 29 +#define MDP_AXI_CLK 30 +#define MMSS_IMEM_AXI_CLK 31 +#define IJPEG_AXI_CLK 32 +#define GFX3D_AXI_CLK 33 +#define VCODEC_AXI_CLK 34 +#define VFE_AXI_CLK 35 +#define VPE_AXI_CLK 36 +#define ROT_AXI_CLK 37 +#define VCODEC_AXI_A_CLK 38 +#define VCODEC_AXI_B_CLK 39 +#define CSI0_SRC 47 +#define CSI0_CLK 48 +#define CSI0_PHY_CLK 49 +#define CSI1_SRC 50 +#define CSI1_CLK 51 +#define CSI1_PHY_CLK 52 +#define DSI_SRC 56 +#define DSI_CLK 57 +#define CSI_PIX_CLK 58 +#define CSI_RDI_CLK 59 +#define MDP_VSYNC_CLK 60 +#define HDMI_APP_CLK 62 +#define GFX2D0_SRC 66 +#define GFX2D0_CLK 67 +#define GFX2D1_SRC 68 +#define GFX2D1_CLK 69 +#define GFX3D_SRC 70 +#define GFX3D_CLK 71 +#define IJPEG_SRC 72 +#define IJPEG_CLK 73 +#define JPEGD_SRC 74 +#define JPEGD_CLK 75 +#define MDP_SRC 76 +#define MDP_CLK 77 +#define MDP_LUT_CLK 78 +#define DSI1_BYTE_SRC 83 +#define DSI1_BYTE_CLK 84 +#define DSI1_ESC_SRC 87 +#define DSI1_ESC_CLK 88 +#define ROT_SRC 91 +#define ROT_CLK 92 +#define TV_ENC_CLK 93 +#define TV_DAC_CLK 94 +#define HDMI_TV_CLK 95 +#define MDP_TV_CLK 96 +#define TV_SRC 97 +#define VCODEC_SRC 98 +#define VCODEC_CLK 99 +#define VFE_SRC 100 +#define VFE_CLK 101 +#define VFE_CSI0_CLK 102 +#define VPE_SRC 103 +#define VPE_CLK 104 +#define DSI_PIXEL_SRC 105 +#define DSI_PIXEL_CLK 106 +#define CAMCLK0_SRC 107 +#define CAMCLK0_CLK 108 +#define CAMCLK1_SRC 109 +#define CAMCLK1_CLK 110 +#define CSIPHYTIMER_SRC 113 +#define CSIPHY1_TIMER_CLK 115 +#define CSIPHY0_TIMER_CLK 116 +#define PLL2 118 +#define MDP_PIXEL_SRC 129 +#define MDP_PIXEL_CLK 130 +#define MDP_LCDC_CLK 131 +#define VFE_CSI1_CLK 132 +#define CSI1_AHB_CLK 133 + +/* +* MSM8x60 legacy footswitch power domains. +* Used with the MMCC power-domain provider (#power-domain-cells = <1>). +* Numbering is independent of the clock ID space above. +*/ +#define GFX2D0_GDSC 0 +#define GFX2D1_GDSC 1 +#define GFX3D_GDSC 2 +#define IJPEG_GDSC 3 +#define MDP_GDSC 4 +#define ROT_GDSC 5 +#define VED_GDSC 6 +#define VFE_GDSC 7 +#define VPE_GDSC 8 + +#endif
Signature · 1 line
2.43.0
PATCH 2/3 dt-bindings: reset: qcom: add mmcc-msm8660 reset IDs
In reply to a message not in the archiveAdd the dt-binding reset-ID header for the MSM8x60 family (MSM8260/MSM8660/APQ8060) MMCC reset lines. The MMCC exposes per-IP software-reset bits used by MDP4 (display), CAMSS / VFE (camera), Gemini (JPEG), rotator, VPE and the GFX2D / GFX3D blocks; each gets a matching reset-controller ID here.
IDs match the numeric values used by the original shared mmcc-msm8960.h reset bindings; only the resets actually implemented by mmcc-msm8660.c are defined.
--- include/dt-bindings/reset/qcom,mmcc-msm8660.h | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 include/dt-bindings/reset/qcom,mmcc-msm8660.h
include/dt-bindings/reset/qcom,mmcc-msm8660.h new file +88 -0
@@ -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 +#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 +#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 +#define GFX3D_AHB_RESET 32 +#define HDMI_AHB_RESET 33 +#define MSSS_IMEM_AHB_RESET 34 +#define IJPEG_AHB_RESET 35 +#define DSI_M_AHB_RESET 36 +#define DSI_S_AHB_RESET 37 +#define JPEGD_AHB_RESET 38 +#define MDP_AHB_RESET 39 +#define ROT_AHB_RESET 40 +#define VCODEC_AHB_RESET 41 +#define VFE_AHB_RESET 42 +#define CSIPHY0_RESET 47 +#define CSIPHY1_RESET 48 +#define VFE_CSI_RESET 50 +#define MDP_RESET 51 +#define AMP_RESET 52 +#define JPEGD_RESET 53 +#define CSI1_RESET 54 +#define VPE_RESET 55 +#define MMSS_FABRIC_RESET 56 +#define VFE_RESET 57 +#define GFX3D_RESET 60 +#define HDMI_RESET 61 +#define MMSS_IMEM_RESET 62 +#define IJPEG_RESET 63 +#define CSI0_RESET 64 +#define DSI_RESET 65 +#define VCODEC_RESET 66 +#define MDP_TV_RESET 67 +#define MDP_VSYNC_RESET 68 +#define ROT_RESET 69 +#define TV_HDMI_RESET 70 +#define TV_ENC_RESET 71 +#define GFX3D_AXI_RESET 75 +#define CSI_RDI_RESET 79 +#define CSI_PIX_RESET 80 + +#endif
Signature · 1 line
2.43.0
PATCH 3/3 clk: qcom: add MSM8x60 MMCC driver
In reply to a message not in the archiveAdd a clock driver for the Multimedia Clock Controller (MMCC) on the MSM8x60 family (MSM8260/MSM8660/APQ8060) - the Scorpion-class generation that preceded MSM8960's Krait CPUs.
The MMCC layout on MSM8x60 differs from MSM8960 in several ways that make a separate driver cleaner than parameterising mmcc-msm8960.c:
- the pix_rdi mux requires a custom set_parent op that does a glitch-free switch between the two CSI sources (the common clock framework guarantees both parents are already enabled when the mux is switched on a running clock);
- the IJPEG GDSC requires releasing AXI, AHB and CORE resets;
- several rate-source pairs (MDP pixel, GFX2D/3D) only exist on
8x60 (e.g. PLL2-derived 228571000/266667000 for graphics);
- the camera CSI / VFE / JPEG / VPE / ROT clock topology lacks the later 8960 reorganisation.
On probe the driver unhalts the MMSS fabric AXI master ports via the qcom_rpm interface - downstream did this from the GDSC footswitch enable path, which mainline GDSC does not implement. The APPS and SYSTEM fabrics are left to their respective subsystems.
Used on the HP TouchPad (Tenderloin) for graphics (Adreno A220), display (MDP4), camera (CSI/VFE), JPEG (Gemini), VIDC, VPE and rotator. Reset IDs are exposed via a separate header so consumers can reset the GDSCs and individual blocks.
--- drivers/clk/qcom/Kconfig | 12 + drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/mmcc-msm8660.c | 3000 +++++++++++++++++++++++++++++++ 3 files changed, 3013 insertions(+) create mode 100644 drivers/clk/qcom/mmcc-msm8660.c
drivers/clk/qcom/Kconfig +12 -0
@@ -572,6 +572,18 @@ config MSM_MMCC_8960 Say Y if you want to support multimedia devices such as display, graphics, video encode/decode, camera, etc. +config MSM_MMCC_8660 + tristate "MSM8x60 Multimedia Clock Controller" + depends on ARM || COMPILE_TEST + select MSM_GCC_8660 + select QCOM_COMMON_CLK + select QCOM_GDSC + help + Support for the multimedia clock controller on the MSM8x60 family + (MSM8260/MSM8660/APQ8060). Say Y if you want to support multimedia + devices such as display, graphics, video encode/decode, camera, etc. + on these Scorpion-class SoCs (e.g. HP TouchPad). + config MSM_GCC_8953 tristate "MSM8953 Global Clock Controller" select QCOM_GDSC
drivers/clk/qcom/Makefile +1 -0
@@ -81,6 +81,7 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o obj-$(CONFIG_MSM_GCC_8998) += gcc-msm8998.o obj-$(CONFIG_MSM_GPUCC_8998) += gpucc-msm8998.o obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o +obj-$(CONFIG_MSM_MMCC_8660) += mmcc-msm8660.o obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o obj-$(CONFIG_MSM_MMCC_8994) += mmcc-msm8994.o obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
drivers/clk/qcom/mmcc-msm8660.c new file +3000 -0
2,975 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 1 line
2.43.0
Part of the diff is not shown here. View the raw message.
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Silent failure to unhalt MMSS fabric if qcom_rpm is not bound yet. --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Silent failure to unhalt MMSS fabric if qcom_rpm is not bound yet. --
commit 7a52c01ee4faffcac27f221b8547899fcd741b7b
Author: Herman van Hazendonk <[email protected]>
clk: qcom: add MSM8x60 MMCC driver
Add a clock driver for the Multimedia Clock Controller (MMCC) on the MSM8x60 family (MSM8260/MSM8660/APQ8060) - the Scorpion-class generation that preceded MSM8960's Krait CPUs.
Quoted 5 lines
drivers/clk/qcom/mmcc-msm8660.c new file +0
-0
[ ... ]
Quoted 9 lines
drivers/clk/qcom/mmcc-msm8660.c new file +9
-0+static void mmcc_msm8660_unhalt_fabric_ports (struct device *dev) +{ +struct device_node *rpm_node; +struct platform_device *rpm_pdev; +struct device_link *link; +struct qcom_rpm *rpm; +/* halt_data[0]=0 = CLK_UNHALT for all bits; halt_data[1] = port mask */ +u32 mmss_halt[2 ] = {0 ,GENMASK (13 ,0 )}; +int rc;
[ ... ]
drivers/clk/qcom/mmcc-msm8660.c
Quoted 10 lines
drivers/clk/qcom/mmcc-msm8660.c new file +10
-0+ rpm =dev_get_drvdata (&rpm_pdev->dev); +if (!rpm) { +/* +* Supplier exists but has not bound yet -- skip the +* unhalt rather than blocking mmcc probe; downstream +* MMSS clients will be enabled on demand. +*/ +put_device (&rpm_pdev->dev); +return ; + }
[Severity: High]
Does this code silently fail to unhalt the MMSS fabric if the qcom_rpm driver has not yet probed?
If dev_get_drvdata() returns NULL because the RPM driver hasn't bound yet, this function returns without an error. Since mmcc_msm8660_probe() calls this and ignores any failure, it will complete probe successfully but leave the MMSS fabric permanently halted.
The comment here states "downstream MMSS clients will be enabled on demand", but this contradicts the block comment above indicating that mainline GDSC does not implement on-demand unhalting. When multimedia hardware later powers on and attempts DMA, the hardware queues will freeze and stall the system.
Should mmcc_msm8660_unhalt_fabric_ports() return an int so we can pass -EPROBE_DEFER back to the probe function to wait for the RPM driver?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
[email protected]
AI reviewRe: [PATCH 2/3] dt-bindings: reset: qcom: add mmcc-msm8660 reset IDs
In reply to Herman van HazendonkThank 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…
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 <[email protected]>
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.
Quoted 19 lines
include/dt-bindings/reset/qcom,mmcc-msm8660.h new file +13
-0@@ -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
include/dt-bindings/reset/qcom,mmcc-msm8660.h line 23
Quoted 10 lines
include/dt-bindings/reset/qcom,mmcc-msm8660.h new file +10
-0+* 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.
include/dt-bindings/reset/qcom,mmcc-msm8660.h line 34
Quoted 11 lines
include/dt-bindings/reset/qcom,mmcc-msm8660.h new file +11
-0+#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?
Quoted 6 lines
include/dt-bindings/reset/qcom,mmcc-msm8660.h new file +6
-0+#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
include/dt-bindings/reset/qcom,mmcc-msm8660.h line 50
Quoted 10 lines
include/dt-bindings/reset/qcom,mmcc-msm8660.h new file +10
-0+#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?
include/dt-bindings/reset/qcom,mmcc-msm8660.h line 53
Quoted 3 lines
include/dt-bindings/reset/qcom,mmcc-msm8660.h new file +3
-0+#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?
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH 0/3] clk: qcom: add MSM8x60 Multimedia Clock Controller (MMCC) driver
In reply to a message not in the archiveHi,
Two things I need to flag against this v1 before further review:
1. Missed prerequisite (build break on a clean tree).
mmcc-msm8660.c references LEGACY_FOOTSWITCH (4 GDSCs: ROT, IJPEG, VFE, VPE) and RPM_ALWAYS_ON (GFX3D), both of which are not in mainline gdsc.{c,h} today. I was supposed to send the GDSC framework groundwork series first; I sent mmcc by mistake before it. The prereq is incoming on linux-clk in a separate thread:
Subject: [PATCH 0/2] clk: qcom: gdsc: groundwork for
MSM8x60 power domains
v2 of this MMCC series will declare the dependency explicitly in the cover letter and will not be sent before the gdsc series has review traction.
2. mmcc_msm8660_unhalt_fabric_ports() silent-fail bug.
The function returns silently on every "RPM supplier not ready" path (no qcom,rpm-msm8660 node, of_find_device_by_node() returns NULL, device_link_add() fails, dev_get_drvdata() returns NULL). probe() ignores it. The "downstream clients will be enabled on demand" comment is wrong: mainline GDSC does not re-issue the unhalt on power-domain enable, so a system where qcom_rpm has not yet bound when mmcc probes ends up with the MMSS AXI fabric permanently halted. First MMSS DMA (MDP / CAMSS / GFX / JPEG / VPE / HDCODEC) silently freezes.
v2 will:
- change unhalt_fabric_ports() to return int,
- return -EPROBE_DEFER on every "supplier not ready" path,
- return -ENODEV via dev_err_probe() when the DT node is absent entirely,
- propagate the result through probe() so the driver core retries when qcom_rpm finally binds.
The patch is in my local tree and on-device validated; I will roll it into v2 once initial review feedback on the rest of the series has had a chance to settle (and once the gdsc prereq is on-list).
Apologies for the noise. v2 is coming, just not immediately.
Thanks,
Herman
Re: [PATCH 3/3] clk: qcom: add MSM8x60 MMCC driver
In reply to Herman van HazendonkHi,
Confirmed -- thanks for catching this. mmcc_msm8660_resets[] jumps straight from FABRIC_AHB_RESET (29) to GFX3D_AHB_RESET (32). Array indices 30 (GFX2D0_AHB_RESET) and 31 (GFX2D1_AHB_RESET) were implicitly zero-initialised, so when gfx2d0_gdsc / gfx2d1_gdsc power-cycle the GDSC the qcom_reset_set_assert() path does a RMW on { .reg = 0, .bit = 0 } -- toggling bit 0 of register 0x0000 (MMSS PLL0 mode register) on every transition. Genuine silent clock-controller corruption, not just a missing reset toggle.
v2 will add:
[GFX2D0_AHB_RESET] = { 0x020c, 12 },
[GFX2D1_AHB_RESET] = { 0x020c, 11 },between the FABRIC_AHB_RESET and GFX3D_AHB_RESET entries. Bit positions match the sibling mmcc-msm8960.c driver -- same hardware IP, same MMSS_AHB_RESET register layout.
Fix is in my local tree and on-device validated; will land in v2 alongside the unhalt_fabric_ports() -EPROBE_DEFER fix and the cover-letter dependency on the gdsc framework series:
https://lore.kernel.org/linux-clk/[email protected]/
On the "MSSS vs MMSS" naming nit: agreed it should be MMSS_, but that one I'll leave for a separate cleanup so v2 doesn't churn DT ABI alongside the functional fixes.
Thanks,
Herman
Re: [PATCH 3/3] clk: qcom: add MSM8x60 MMCC driver
In reply to Herman van HazendonkHi,
Thanks for the thorough pass. All 5 items confirmed and queued for v2. Triage:
[Medium] Kconfig: select QCOM_COMMON_CLK is a bogus symbol.
Confirmed -- that symbol does not exist. The qcom clk infra is pulled in transitively via select QCOM_GDSC + select MSM_GCC_8660. The sibling MSM_MMCC_8960 stanza carries no analogous select either. v2 will drop the line.
[High] DSI src/byte/esc clocks use clk_rcg_bypass_ops with empty or missing freq_tbl.
Confirmed. clk_rcg_bypass_ops dereferences freq_tbl[0] for src andpre_div: with the empty/placeholder table, src resolves to P_PXO and pre_div to (0 - 1) = 255, shifting outside the bitmask and corrupting NS-register bits 14..21. dsi1_byte_src and dsi1_esc_src were even worse -- no freq_tbl at all, so the deref hits a NULL.
Fix matches the table-less ops mainline mmcc-msm8960.c uses for the same hardware (cross-checked against the legacy vendor 2.6.35-palm arch/arm/mach-msm/clock-8x60.c's clk_tbl_dsi_byte and the parent-PLL "src = SRC_NONE" comment that documents the divider-only intent):
dsi1_src -> clk_rcg_bypass2_ops + CLK_SET_RATE_PARENT dsi1_byte_src -> clk_rcg_bypass2_ops + CLK_SET_RATE_PARENT dsi1_esc_src -> clk_rcg_esc_ops + CLK_SET_RATE_PARENT
dsi1_pixel_src -> clk_rcg_pixel_ops + CLK_SET_RATE_PARENT
The clk_tbl_dsi placeholder is removed.
[High] vcodec_axi_clk / _a / _b missing BRANCH_HALT_SKIP.
Confirmed. The rot_axi_clk and gfx3d_axi_clk entries already note the MMSS-fabric-stuck-at-on case, and rot_axi_clk's comment even calls out vcodec_axi_b_clk by name as a peer -- I just forgot to actually carry the flag onto the three vcodec_axi branches. v2 will add .halt_check = BRANCH_HALT_SKIP to all three.
[High] mmcc_msm8660_unhalt_fabric_ports() UAF window during RPM probe failure.
Confirmed, and well caught. device_link_add() does not block on supplier->bound; it can succeed against a supplier mid-probe, with drvdata set early but a still-to-fail probe path that re-clears drvdata and frees the qcom_rpm structure via devres.
The earlier -EPROBE_DEFER fix I sent covers the "drvdata still NULL" case but does not close the "drvdata went non-NULL transiently and will be freed after we sampled it" window. Fix: take device_lock(&rpm_pdev->dev) and check device_is_bound() (exported by drivers/base/dd.c) before reading drvdata, and hold the lock across the single qcom_rpm_write() commit. The lock does not nest with anything qcom_rpm_write touches (rpm->lock + the mailbox subsystem; neither takes device_lock).
The legacy 2.6.35-palm vendor kernel does not exhibit this race because it accesses RPM as a global singleton via msm_rpm_set_* APIs -- no platform_device, no drvdata, no device_link. The race is intrinsic to the modern discrete-driver split, so the fix needs to live here.
All four are applied to my local tree and on-device validated. v2 reroll will batch:
- Kconfig drop
- DSI ops fix + clk_tbl_dsi removal
- vcodec_axi BRANCH_HALT_SKIP
- device_lock + device_is_bound around unhalt drvdata read
- (earlier rounds) GFX2D[01]_AHB_RESET entries
- (earlier rounds) unhalt -EPROBE_DEFER
plus cover-letter pointers to the gdsc framework prereq (LEGACY_FOOTSWITCH / RPM_ALWAYS_ON, sent separately as [email protected]).
Holding v2 until the first round of v1 feedback has had a chance to settle and the prereq series have review traction.
Thanks,
Herman