Re: [PATCH 2/2] arm64: dts: qcom: sc7280: Add cpu OPP tables

On 2021-05-05 14:19, Sudeep Holla wrote:

Hi Sibi,

On Tue, May 04, 2021 at 11:55:10PM +0530, Sibi Sankar wrote:

quoted

Hey Sudeep,

Thanks for the review!

On 2021-05-04 20:12, Sudeep Holla wrote:

[...]

quoted

quoted

NACK, this breaks if there is a mismatch from what is read from the
hardware and what is presented in this table above. Either add it from the
some bootloader or other boot code to this table reading from the
hardware/firmware or find a way to link them without this.

Sorry I had warned long back about this when such links were discussed
as part of interconnect binding.

Not sure why this warrants a NACK, as this was consensus for mapping 
cpu
freq to DDR/L3 bandwidth votes. (We use the same solution on SDM845 
and
SC7180). The opp tables are optional and when specified puts in votes 
for
DDR/L3. In the future the table can be safely dropped when more useful
devfreq governors are upstreamed.
cpufreq: qcom: Don't add frequencies without an OPP

(You can always add commit sha to make it easy to search)

But I am not sure how this is related to the above commit anyways.

quoted

I guess your main concern for breakage is ^^ commit? The original 
design is
to list a super set of frequencies supported by all variants of the 
SoC
along with the required DDR/L3 bandwidth values. When we run into
non-documented frequency we just wouldn't put in bw votes for it which
should be fine since the entire opp_table is optional. If this is the 
reason
for the NACK I can try get it reverted with Matthias's ack.

No my main concern is this platform uses "qcom-cpufreq-hw" driver and 
the
fact that the OPPs are retrieved from the hardware lookup table 
invalidates
whatever we have in DT. In short it will be junk and becomes obsolete.

The table provides mapping to bandwidths
which aren't available in the firmware
though. In short we do have to store the
mapping somewhere i.e. a mapping that
lists all possible frequencies to its
bandwidth requirements needs to be present
and using a opp table with the interconnect
bw bindings was the consensus reached.

Given that a duplicate mapping that lists
all possible frequencies to bw is inevitable
and Qualcomm has a way of listing all the
supported frequencies for the SoC, I feel
that dt breakage in the future should be
a non-concern. Not sure why you call it
junk since it solves the perf/power
requirements on SDM845/SC7180 SoCs. When
it becomes obsolete it would mean that
they are better devfreq governors available
upstream and that's a good reason for the
opp tables to go away.

So what I suggested before is still valid. You simply can't have static
OPP tables in the DT for this platform. Do get some boot code to fetch 
the
same from the h/w LUT and patch to the DT or figure out any other way 
to
manage dynamically.

moving the logic to boot loader doesn't
magically fix your concerns though (since
it would also need a superset of available
frequencies). It will suffer from the same
problems with an additional dependency on
firmware propagation in case of breakages
which is something you can avoid for the
simple cpu based scaling solution.

So NACK still stands for static addition of OPPs to the DT as in this 
patch.

I'll let Viresh take the call since this
solution is already used on older SoCs.

--
Regards,
Sudeep

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.