Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings

From: Taniya Das
Date: Fri Jun 15 2018 - 13:32:14 EST



On 6/15/2018 6:53 PM, Sudeep Holla wrote:


On 14/06/18 19:24, Taniya Das wrote:
Hello Sudeep,

Thanks for your comments.

On 6/14/2018 4:17 PM, Sudeep Holla wrote:


On 13/06/18 19:13, Taniya Das wrote:
Hello Sudeep,

Thanks for review comments.

On 6/13/2018 4:56 PM, Sudeep Holla wrote:



[...]

You are bit inconsistent on the wordings. Some places you refer this as
hardware engine. If so, please drop all references to firmware/FW. If
it's firmware then update accordingly.


It is a hardware engine which has a firmware to take care of the
managing the frequency request from OS. That is reason to refer it as a
firmware.


Yes I did guess that initially, but I failed to understand when
different bindings were posted to deal with devfreq and cpufreq with the
same firmware. Ideally if it's the firmware you are talking to, place
all these under /firmware node and align all those with single binding.


The OS is not aware of the firmware and OS only knows about the hardware
engine and has to put forward it's request to the hardware engine using
the "Perf" state register in both devfreq & cpufreq. So would it be
still required to put under the /firmware node?


Ah ok, then remove any references to firmware other than stating its
presence in the introduction. E.g. you have "Add cpufreq firmware device
bindings ...". So this is definitely not firmware binding. You are just
presenting the h/w as is and you need to deal with change of firmware in
DT and OS agnostic way.


Sure Sudeep, I will update the references of firmware.

Is there anything else that this firmware deals with ? If so all those
need to be put in one place.


We deal only with the CPU frequency and L3 frequency(devfreq).


OK

+Properties:
+- compatible
+ÂÂÂ Usage:ÂÂÂÂÂÂÂ required
+ÂÂÂ Value type:ÂÂÂ <string>
+ÂÂÂ Definition:ÂÂÂ must be "qcom,cpufreq-fw".
+
+* Property qcom,freq-domain
+Devices supporting freq-domain must set their "qcom,freq-domain"
property with
+phandle to a freq_domain_table in their DT node.
+
+* Frequency Domain Table Node
+
+This describes the frequency domain belonging to a device.
+This node can have following properties:
+
+- reg
+ÂÂÂ Usage:ÂÂÂÂÂÂÂ required
+ÂÂÂ Value type:ÂÂÂ <prop-encoded-array>
+ÂÂÂ Definition:ÂÂÂ Addresses and sizes for the memory of the perf
+ÂÂÂÂÂÂÂÂÂÂÂ , lut and enable bases.
+ÂÂÂÂÂÂÂÂÂÂÂ perf - indicates the base address for the desired
+ÂÂÂÂÂÂÂÂÂÂÂ performance state to be set.
+ÂÂÂÂÂÂÂÂÂÂÂ lut - indicates the look up table base address for the
+ÂÂÂÂÂÂÂÂÂÂÂ cpufreqÂÂÂ driver to read frequencies.
+ÂÂÂÂÂÂÂÂÂÂÂ enable - indicates the enable register for firmware.


You still didn't answer my earlier question.

OS might touch one or 2 registers in lots of IP blocks. I am not sure
why those are any different from these. Are you trying to align with
any
other bindings or specification. Are you trying to make this binding
generic here ? I understand if it was trying to generalize the firmware
interface, but you also state it's a hardware engine. So I fail to see
the need for such specificity here. Why not define the whole IP block
and the driver knows where to access these specific ones as they are
specific to this hardware block. In that way if you decide to add more
data, it's extensible easily without the need for patching DT.


Sorry Sudeep I missed replying to your earlier query.
The High level OS(HLOS) would require to access only these specific
registers from this IP block and just mapping the whole block(huge
region) is unnecessary from the OS point of View. As of now it is a
generic binding for all using this IP block to manage frequency
requests. The OS would only have to know the frequencies supported i.e
to read the lookup table registers and put across the OS request using
the performance state register.


I am not sure if you need to defining bindings to save OSPM IO mapping.
In-fact you may be adding more mapping unnecessarily. The mappings are
page aligned and spiting the registers and mapping them individually may
result in more mappings.

I just need to know the rational for such specific choice of registers.
I assume it's aligned to some other standard specifications like CPPC
though not identical.


I am not sure of the query but there is no other register that the OS is
required to use other than the ones defined here.


The point is ever IP on the SoC may have 100s to 1000s of registers that
may or may not be used by OS. That's about to the OS to decide and you
just need to provide the hardware view to anyone using the device tree.
It *should not* _just_ represent what you think OS(Linux in particular)
"for now"

Eg. Suppose you need some information on power curve for EAS energy
model, I really hate to update DT for that or even do a mix with DT
just
because f/w is no longer modifiable.


For now we are safe.


What do you mean by that ?

I meant here was currently there is no such known case where the f/w is
no longer modifiable and we need to extend device tree bindings.

It should be easily extensible is what I am
trying to say. You can add more info and alter the information in the
driver with compatibles if you keep the register info as minimum as
possible. For now, you have enable, set and lut registers. What if you
want to provide power numbers ?


Yes I do understand the intent of mapping the whole register space, but
as per the HW specs these 3 registers would be the only ones required
for now. I do not think this hardware engine has any information on the
power numbers.


That's fine. So on this platform DT, will you list only the registers
touched by the OS for all the IP ? I am sure that will not be the case.


Yes, registers list those would be touched by OS only.

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--