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

[Sudeep Holla]

On 07/06/18 08:20, Taniya Das wrote:
> Hello Sudeep,
> 
> Thanks for the review comments.
> 
> On 6/4/2018 4:25 PM, Sudeep Holla wrote:
>> On Mon, Jun 04, 2018 at 04:16:33PM +0530, 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 <tdas@xxxxxxxxxxxxxx>
>>> ---
>>> Â .../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.
>>> +
>>> +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.
>>> +- reg-names
>>> +ÂÂÂ Usage:ÂÂÂÂÂÂÂ required
>>> +ÂÂÂ Value type:ÂÂÂ <stringlist>
>>> +ÂÂÂ Definition:ÂÂÂ Address names. Must be "perf", "lut", "enable".
>>> +ÂÂÂÂÂÂÂÂÂÂÂ Must be specified in the same order as the reg property.
>>> +
>>
>> [...]
>>
>>> +
>>> +ÂÂÂ qcom,cpufreq-fw {
>>> +ÂÂÂÂÂÂÂ compatible = "qcom,cpufreq-fw";
>>> +
>>> +ÂÂÂÂÂÂÂ #address-cells = <1>;
>>> +ÂÂÂÂÂÂÂ #size-cells = <1>;
>>> +
>>> +ÂÂÂÂÂÂÂ freq_domain_table0 : freq_table0 {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ reg = <0x17d43920 0x4>, <0x17d43110 0x500>,
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ <0x17d41000 0x4>;
>>
>> Are "perf", "lut", "enable" registers part of single IP block / share
>> memory ?
>> I am just trying to understand the reason for separate entries in this
>> fashion
>> as part of DT register property. I am wondering if there will be multiple
>> entries that fall with the page size.
>>
> 
> They are part of the same IP block and these are the only register
> offsets which is required to be accessed by HLOS.
> 

HLOS ?

Anyways, 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 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.

-- 
Regards,
Sudeep