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

[Linus Walleij]

On Wed, Jun 10, 2026 at 3:39 PM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxxxxxxxx> wrote:
> On Tue, Jun 09, 2026 at 03:44:39PM +0200, 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)
> >
> > [...]
> >
> > > +static struct clk_branch camclk0_clk = {
> > > +   .halt_reg = 0x01e8,
>
> From msm-3.0:
>
> +#define DBG_BUS_VEC_I_REG                      REG_MM(0x01E8)
>
>
> So, it seems, it is a debug reg, not an actual halt reg (but I might be
> mistaken here, I haven't looked into the details).

I think the define just has a bad name. You have to look
at how it's used.

This define is in msm-3.4 for msm8960, clock-msm8960:
#define DBG_BUS_VEC_I_REG                       REG_MM(0x01E8)
... and then it is used in 10 different places as a halt reg:

$ git grep DBG_BUS_VEC_I_REG *
clock-8960.c:#define DBG_BUS_VEC_I_REG                  REG_MM(0x01E8)
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:                   .halt_reg = DBG_BUS_VEC_I_REG, \
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,
clock-8960.c:           .halt_reg = DBG_BUS_VEC_I_REG,

and in the mainline msm8960 driver 01e8 is used as a halt reg
in the as well, see drivers/clk/qcom/mmcc-msm8960.c.

Some qualcomm clock registers being named DBG_* has no
semantic meaning, one has to look at the usage. Maybe at
some point some engineer thought it's a debug feature to
be able to halt a clock.

Yours,
Linus Walleij