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

[me@herrie.org]

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@herrie.org>
>> ---
> 
> [...]
> 
>> +	.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