Hello Sudeep,

Thanks for review comments.

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


On 12/06/18 12:02, Taniya Das wrote:
Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
SoCs. This is required for managing the cpu frequency transitions which are
controlled by firmware.

Signed-off-by: Taniya Das <t...@codeaurora.org>
---
  .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 173 +++++++++++++++++++++
  1 file changed, 173 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt 
b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
new file mode 100644
index 0000000..e3087ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt
@@ -0,0 +1,173 @@
+Qualcomm Technologies, Inc. CPUFREQ Bindings
+
+CPUFREQ FW is a hardware engine used by some Qualcomm Technologies, Inc. (QTI)
+SoCs to manage frequency in hardware. It is capable of controlling frequency
+for multiple clusters.
+

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.

+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.

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.

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

--

Reply via email to