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

[me]

On 2026-06-09 15:44, Konrad Dybcio wrote:
> On 5/30/26 3:58 PM, Herman van Hazendonk wrote:
> > 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.
> >
> > 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 temporarily
> >     enables both parents during the glitch-free transition;
> >   - 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.
> >
> > 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.
> >
> > Signed-off-by: Herman van Hazendonk <github.com@xxxxxxxxxx>
> > ---
>
> [...]
>
> > +	.clkr.hw.init = &(struct clk_init_data){
> > +		.name = "pll2",
> > +		.parent_data = (const struct clk_parent_data[]){
> > +			{ .fw_name = "pxo", .name = "pxo_board" },
>
> Please drop .name (the kernel-global clock lookup), this is only a
> backwards compatiblity measure on existing drivers. For new entries,
> .index is the best (because well, it's the fastest)
>
> [...]
Ok will rework in the new version.
> > +static struct clk_branch camclk0_clk = {
> > +	.halt_reg = 0x01e8,
> > +	.halt_bit = 15,
> > +	/*
> > +	 * The legacy webOS kernel used halt_reg = NULL for this clock,
> > +	 * meaning it never checked the halt status. The hardware doesn't
> > +	 * properly report the clock state via the halt register. Use
> > +	 * BRANCH_HALT_SKIP to avoid the "status stuck at 'off'" warning.
>
> It may be that some piece of hw is holding this clock online behind the
> scenes. Is there perhaps a Qualcomm-authored commit that mentions the 
> hw
> bug, or is it a guess? Due to the age of this chip I would imagine I
> won't be find an answer if you don't have one..
Re: the halt-status decision, it's not a guess. Downstream 
Qualcomm-authored
BSP arch/arm/mach-msm/clock-8x60.c configures CAMCLK_CC_REG with 
halt_check
= DELAY, indicating Qualcomm engineering deliberately avoided polling 
the
halt register on this clock. The right mainline mapping
is BRANCH_HALT_DELAY rather than the BRANCH_HALT_SKIP I used (which 
works,
but seemingly incorrect); I'll switch it in the new version and apply 
the
same DELAY-vs-NOCHECK mapping to the other status-stuck clocks I 
flagged.

> [...]
>
> I see you have a lot of inline note-to-self comments, please strip some
> of them.
>
> Konrad
I'll also strip the inline note-to-self comments fair point, they 
belonged
in the commit message rather than in the code.

Thanks,
Herman