Re: [RFC 7/7] arm64: dts: qcom: x1e80100: Enable LLCC/DDR dvfs

[Sibi Sankar <quic_sibis@quicinc.com>]

On 1/18/24 02:08, Konrad Dybcio wrote:
> 
> 
> On 1/17/24 18:34, Sibi Sankar wrote:
>> Enable LLCC/DDR dvfs through the Qualcomm's SCMI vendor protocol.
>>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/x1e80100.dtsi | 48 ++++++++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi 
>> b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> index 6856a206f7fc..3dc6f32fbb4c 100644
>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi
>> @@ -329,6 +329,54 @@ scmi_dvfs: protocol@13 {
>>                   reg = <0x13>;
>>                   #clock-cells = <1>;
>>               };
>> +
>> +            scmi_vendor: protocol@80 {
>> +                reg = <0x80>;
>> +
>> +                memlat {
>> +                    #address-cells = <1>;
>> +                    #size-cells = <0>;
>> +
>> +                    memory@0 {
>> +                        reg = <0x0>; /* Memory Type DDR */
> 
> I'm not sure reg is the best property to (ab)use..

I'm ok with introducing a custom property as well. I went
ahead with reg mainly because the overall structure looked
similar to audio apr.

> 
> You could very well define a new one, like qcom,memory type,
> then the subnodes could look like:
> 
> memory-0 {
>      qcom,memory-type = <QCOM_MEM_TYPE_DDR>;
>      [...]
> };
> 
>> +                        freq-table-khz = <200000 4224000>;
>> +
>> +                        monitor-0 {
>> +                            qcom,cpulist = <&CPU0 &CPU1 &CPU2 &CPU3 
>> &CPU4 &CPU5 &CPU6 &CPU7 &CPU8 &CPU9 &CPU10 &CPU11>;
> 
> I fail to see the usefulness in checking which CPUs make use of
> the same DRAM or LLC pool. If that's something that may not be
> obvious in future designs like on dual-socket x86 servers,
> I think it can be deferred until then and for now, AFAIU you
> can just unconditionally assume all CPUs count.

we list all the cpus here because on X1E they are identical
and have the same cpu frequency to memory frequency mapping.
But doesn't really apply to other SoCs in general. But dropping
this would mean that driver assumes a table applies to all
cpus by default.

> 
>> +                            qcom,cpufreq-memfreq-tbl = < 999000 
>> 547000 >,
>> +                                           < 1440000 768000 >,
>> +                                           < 1671000 1555000 >,
>> +                                           < 2189000 2092000 >,
>> +                                           < 2156000 3187000 >,
>> +                                           < 3860000 4224000 >;
> 
> I.. can't seem to think of a future where this doesn't explode.

Not really ... You can already see a more or less standard table
being used across various skus on older SoCs that uses memlat
running from the kernel downstream. So that should count for
something.

> 
> When you release a different bin/SKU/fuse config of this SoC where
> the CPU frequencies are different, this will likely also need to be
> updated. We don't want that manual cruft in the devicetree.

Also unlike cpufreq map, if you notice this table doesn't list all
possible cpu frequencies but list broad ranges instead. This way
the table rarely needs updates unless we want to scale to max llcc/ddr
at a lower CPU frequency for a particular SKU.

> 
> Since both previously cpufreq-hw and now cpufreq-scmi generally
> operate on levels that map to some frequencies in the firmware,
> could it be bound to that instead?

At this point the only decision is whether the table lies in dt
or in the driver. But driver wouldn't even have a way to distinguish
between various skus so the dt looks like the only option.

-Sibi

> 
> Konrad
>