Re: [PATCH v3 2/5] dt-bindings: cpufreq: apple,soc-cpufreq: Add binding for Apple SoC cpufreq

[Hector Martin]

On 26/10/2022 08.12, Rob Herring wrote:
> On Wed, Oct 26, 2022 at 02:22:40AM +0900, Hector Martin wrote:
>> On 26/10/2022 01.01, Krzysztof Kozlowski wrote:
>>> On 24/10/2022 00:39, Hector Martin wrote:
>>>> This binding represents the cpufreq/DVFS hardware present in Apple SoCs.
>>>> The hardware has an independent controller per CPU cluster, and we
>>>> represent them as unique nodes in order to accurately describe the
>>>> hardware. The driver is responsible for binding them as a single cpufreq
>>>> device (in the Linux cpufreq model).
>>>>
>>>> Signed-off-by: Hector Martin <marcan@xxxxxxxxx>
>>>> ---
>>>>  .../cpufreq/apple,cluster-cpufreq.yaml        | 119 ++++++++++++++++++
>>>>  1 file changed, 119 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/apple,cluster-cpufreq.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/cpufreq/apple,cluster-cpufreq.yaml b/Documentation/devicetree/bindings/cpufreq/apple,cluster-cpufreq.yaml
>>>> new file mode 100644
>>>> index 000000000000..b11452f91468
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/cpufreq/apple,cluster-cpufreq.yaml
>>>> @@ -0,0 +1,119 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/cpufreq/apple,cluster-cpufreq.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Apple SoC cluster cpufreq device
>>>
>>> Few nits, in general looks fine to me.
>>>
>>>> +
>>>> +maintainers:
>>>> +  - Hector Martin <marcan@xxxxxxxxx>
>>>> +
>>>> +description: |
>>>> +  Apple SoCs (e.g. M1) have a per-cpu-cluster DVFS controller that is part of
>>>> +  the cluster management register block. This binding uses the standard
>>>> +  operating-points-v2 table to define the CPU performance states, with the
>>>> +  opp-level property specifying the hardware p-state index for that level.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - const: apple,t8103-cluster-cpufreq
>>>> +          - const: apple,cluster-cpufreq
>>>> +      - items:
>>>> +          - const: apple,t6000-cluster-cpufreq
>>>> +          - const: apple,t8103-cluster-cpufreq
>>>> +          - const: apple,cluster-cpufreq
>>>> +      - items:
>>>> +          - const: apple,t8112-cluster-cpufreq
>>>
>>> With the first one (t8103) - it's an enum.
>>
>> This is deliberate. t6000 is compatible with t8103, but t8112 is not
>> (though all are compatible with what the generic apple,cluster-cpufreq
>> compatible implies).
> 
> What does compatible mean here? IOW, what can a client do with 
> 'apple,cluster-cpufreq' alone? It's one thing for self-contained blocks 
> to remain unchanged from chip to chip, but things like this tend to 
> change frequently. It looks like for 4 chips we have 3 different 
> versions.

This is described in the cover letter. The actual cpufreq control is
identical for all shipping SoCs right now (that's 5 SoCs, since t6000 is
actually also t6001 and t6002) and will work with just that generic
compatible (and almost certainly quite a few SoC generations going back
too). It's just that I found a useful register that gives you feedback
on the *actual* pstate, and that register field shifted one bit on t8112
because they ran out of bits. If the driver finds a t8103 or t8112
compatible, it will use that register to accurately report the current
frequency (subject to boost frequency restrictions). If it doesn't, it
will just report the requested frequency as actual. t6000 is compatible
with t8103 in this regard, hence the tiering. I expect lots of future
SoCs to be compatible with t8112, since although they exceeded 16
pstates there, I doubt they'll push beyond 32 and have to move it
another bit any time soon.

Right *now*, since boost frequencies are unachievable and disabled due
to reasons unrelated to this driver, all compatibles are, in fact,
completely equivalent in functionality for end users, and nothing would
change if we just had `apple,cluster-cpufreq` in the DT. This will
change once we get cpuidle support, which unlocks boost frequencies as a
side effect, but that will require no changes to this driver/series
(other than uncommenting the extra OPPs in the DT).

- Hector