Re: [PATCH v6 8/9] dt-bindings: cpufreq: qcom-hw: Add bindings for 8998

From: AngeloGioacchino Del Regno  <angelogioacchino.delregno@somainline.org>
To: Rob Herring <robh@kernel.org>
Cc: bjorn.andersson@linaro.org, viresh.kumar@linaro.org,
	agross@kernel.org, rjw@rjwysocki.net, devicetree@vger.kernel.org,
	amit.kucheria@linaro.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, phone-devel@vger.kernel.org,
	konrad.dybcio@somainline.org, marijn.suijten@somainline.org,
	martin.botka@somainline.org, jami.kettunen@somainline.org,
	paul.bouchara@somainline.org,
	~postmarketos/upstreaming@lists.sr.ht, jeffrey.l.hugo@gmail.com
Subject: Re: [PATCH v6 8/9] dt-bindings: cpufreq: qcom-hw: Add bindings for 8998
Date: Wed, 21 Jul 2021 12:48:24 +0200	[thread overview]
Message-ID: <7d012e8c-4570-9d60-32c3-fb271ce636b8@somainline.org> (raw)
In-Reply-To: <20210714213946.GA3568065@robh.at.kernel.org>

Il 14/07/21 23:39, Rob Herring ha scritto:
> On Thu, Jul 01, 2021 at 12:57:29PM +0200, AngeloGioacchino Del Regno wrote:
>> The OSM programming addition has been done under the
>> qcom,cpufreq-hw-8998 compatible name: specify the requirement
>> of two additional register spaces for this functionality.
>> This implementation, with the same compatible, has been
>> tested on MSM8998 and SDM630.
> 
> Certainly we should be using the new binding for any new SoCs.
> 

Yes that's totally true, I should've probably added the new bindings directly
instead of making it implicit that the 8998 model is valid for the others.
Adding more bindings will explicitly clarify that the support is extended to
630/660 so yeah, I agree.

>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>> ---
>>   .../bindings/cpufreq/cpufreq-qcom-hw.yaml     | 67 ++++++++++++++-----
>>   1 file changed, 52 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
>> index bc81b6203e27..29b663321a0b 100644
>> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
>> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.yaml
>> @@ -18,6 +18,10 @@ description: |
>>   properties:
>>     compatible:
>>       oneOf:
>> +      - description: Non-secure v1 of CPUFREQ HW
>> +        items:
>> +          - const: qcom,cpufreq-hw-8998
>> +
>>         - description: v1 of CPUFREQ HW
>>           items:
>>             - const: qcom,cpufreq-hw
>> @@ -28,21 +32,9 @@ properties:
>>                 - qcom,sm8250-cpufreq-epss
>>             - const: qcom,cpufreq-epss
>>   
>> -  reg:
>> -    minItems: 2
>> -    maxItems: 3
>> -    items:
>> -      - description: Frequency domain 0 register region
>> -      - description: Frequency domain 1 register region
>> -      - description: Frequency domain 2 register region
>> +  reg: {}
>>   
>> -  reg-names:
>> -    minItems: 2
>> -    maxItems: 3
>> -    items:
>> -      - const: freq-domain0
>> -      - const: freq-domain1
>> -      - const: freq-domain2
>> +  reg-names: {}
>>   
>>     clocks:
>>       items:
>> @@ -57,10 +49,55 @@ properties:
>>     '#freq-domain-cells':
>>       const: 1
>>   
>> +if:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: qcom,cpufreq-hw-8998
>> +then:
>> +  properties:
>> +    reg:
>> +      minItems: 2
>> +      maxItems: 6
>> +      items:
>> +        - description: Frequency domain 0 register region
>> +        - description: Operating State Manager domain 0 register region
>> +        - description: Frequency domain 1 register region
>> +        - description: Operating State Manager domain 1 register region
>> +        - description: PLL ACD domain 0 register region (if ACD programming required)
>> +        - description: PLL ACD domain 1 register region (if ACD programming required)
>> +
>> +    reg-names:
>> +      minItems: 2
>> +      maxItems: 6
>> +      items:
>> +        - const: "osm-domain0"
>> +        - const: "freq-domain0"
>> +        - const: "osm-domain1"
>> +        - const: "freq-domain1"
>> +        - const: "osm-acd0"
>> +        - const: "osm-acd1"
> 
> This is different enough and there's not much else to this bindings, so
> I think you should do a separate schema doc.
> 
> BTW, Don't need quotes here.
> 

If you think that this would be appropriate, then I guess it's trivial to
do that and I will... though, I am 99% sure that these bindings will never
get updated, as Qualcomm has shifted to do the programming in TZ and there
surely will never be any new SoC requiring this kind of thing.

The ones that do require this should be around 6, if my memory isn't failing...

>> +
>> +else:
>> +  properties:
>> +    reg:
>> +      minItems: 2
>> +      maxItems: 3
>> +      items:
>> +        - description: Frequency domain 0 register region
>> +        - description: Frequency domain 1 register region
>> +        - description: Frequency domain 2 register region
>> +    reg-names:
>> +      minItems: 2
>> +      maxItems: 3
>> +      items:
>> +        - const: "freq-domain0"
>> +        - const: "freq-domain1"
>> +        - const: "freq-domain2"
>> +
>>   required:
>>     - compatible
>>     - reg
>> -  - reg-names
>>     - clocks
>>     - clock-names
>>     - '#freq-domain-cells'
>> -- 
>> 2.32.0
>>
>>