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

[Hector Martin]

On 05/05/2022 17.43, Krzysztof Kozlowski wrote:
> On 04/05/2022 09:51, Hector Martin wrote:
>> This binding represents the cpufreq/DVFS hardware present in Apple SoCs.
>> The hardware has an independent controller per CPU cluster, but we
>> represent them as a single cpufreq node since there can only be one
>> systemwide cpufreq device (and since in the future, interactions with
>> memory controller performance states will also involve cooperation
>> between multiple frequency domains).
>>
>> Signed-off-by: Hector Martin <marcan@xxxxxxxxx>
>> ---
>>  .../bindings/cpufreq/apple,soc-cpufreq.yaml   | 121 ++++++++++++++++++
>>  1 file changed, 121 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/cpufreq/apple,soc-cpufreq.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/apple,soc-cpufreq.yaml b/Documentation/devicetree/bindings/cpufreq/apple,soc-cpufreq.yaml
>> new file mode 100644
>> index 000000000000..f398c1bd5de5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/apple,soc-cpufreq.yaml
>> @@ -0,0 +1,121 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/cpufreq/apple,soc-cpufreq.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Apple SoC cpufreq device
>> +
>> +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:
>> +    items:
>> +      - enum:
>> +          - apple,t8103-soc-cpufreq
>> +          - apple,t6000-soc-cpufreq
>> +      - const: apple,soc-cpufreq
>> +
>> +  reg:
>> +    minItems: 1
>> +    maxItems: 6
> 
> Is the number of clusters fixed for t8103 and t6000? Are these
> compatibles strictly related to some specific M1 SoC? If yes, then you
> should have constraints in allOf:if:then.

No, t6000 includes t6002 which is a 2-die version and has 6 clusters,
t6001 and t6000 have 3. t8103 always has 2, but it's conceivable that
compatible could be used for other chips within the same generation with
a different number.

The general idea for these compats is as a fallback in case we need to
quirk something for individual SoCs or start using registers which
changed for some reason, but right now they are not used. But I think
given how closely related t6000/t6001/t6002 are (t6002 is literally two
t6001 dies, and t6000 is almost identical to t6001 with a chunk
missing), I think spelling those out as 3 separate compatibles is
overkill. In general we treat those 3 SoCs as identical in terms of
compatibles, but the one-cpufreq-node limitation means t6002 does have
twice the clusters.

It's also conceivable that Apple could start releasing CPUs with entire
clusters fused off, so then our bootloader would have to start mutating
the reg entry to represent that.

I can add constraints to express some of this if you really want me to,
but I'm not sure it's that useful. The way I see it, the compatibles
really *mean* "this soc has t8103 clusters" or "this soc has t6000
clusters" and the number of clusters is arbitrary and the driver will
never care about that. Honestly I'd rather have separate cpufreq nodes
for each cluster, but nobody else does it like that and there's only one
driver instance, so that gets complicated...

-- 
Hector Martin (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub