Re: [PATCH 3/3] clk: qcom: add MSM8x60 MMCC driver

Re: [PATCH 3/3] clk: qcom: add MSM8x60 MMCC driver

[Herman van Hazendonk]

Hi,

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/20260602050840.435933-1-github.com@herrie.org/

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

[Herman van Hazendonk]

Hi,

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 and
pre_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
20260602050840.435933-1-github.com@herrie.org).

Holding v2 until the first round of v1 feedback has had a chance
to settle and the prereq series have review traction.

Thanks,
Herman