Re: [PATCH 5/9] clk: google: Add CPM clock driver

[Jerome Brunet]

> Google Tensor SoCs (used on Pixel phones) manage many resources
> (including most clocks) using a Central Power Manager (CPM)
> co-processor. We communicate with CPM using mailboxes, abstracted in the
> cpm_request_client(), etc., API.
> 
> The clock tree is largely not represented fully by this driver, because
> CPM hides most of the divider, mux, and parent details, instead
> maintaining its own internal table of mappings for divider and mux
> values. The one exception is for some of the DPU clocks, where several
> clocks and dividers are exposed directly, and the firmware API works on
> divider values instead of clock rates. I attempt to represent this
> properly with dividers and parents, although I don't have the full
> display stack running to verify everything yet.
>

As you say, the clock tree is not fully represented here. Beyond that, CPM
owns the clocks themselves, as the driver acknowledges by setting
CLK_GET_RATE_NOCACHE.

The driver keeps a partial copy of CPM's state in the kernel:

- Rate tables below hard-code what the FW has (or should have) instead
  of rounding the rate, even though there seems to be a ROUND_RATE_OP_ID.

- kernel handles PLL parenting and divider math for the DP clocks, while the
  firmware can also switch them to what looks like the oscillator, out of
  the kernel's view. So the math is likely duplicated as well

This middle ground cannot be kept consistent. CCF assumes it knows the
topology and computes rates from it, but the firmware makes the actual
decisions. The DP clocks already show it: the tree can report a parent the
hardware does not use, and .recalc_rate()/.determine_rate() can disagree
with what CPM programmed.

I would advise to follow a model closer SCMI. What you are doing here looks
a lot like it.
(see drivers/firmware/arm_scmi/clock.c and drivers/clk/clk-scmi.c)

 - Put the LPCM service protocol (request word layout, dst_id, timeout,
   error translation) in the CPM firmware/soc driver, behind a small API
   shared by clk and reset. Philipp asked for the error helper on 6/9; the
   whole request encoding is common, not just the errors.

 - Make the clock driver a thin CCF adapter: each CPM clock is an opaque
   endpoint, and gate/rate requests go to the firmware. Get rates from the
   firmware (round-rate op) instead of local tables. Model no topology the
   kernel does not own. If parents must be exposed, the firmware should
   describe them, as SCMI does, rather than the kernel hard-coding them.

-- 
Jerome