cpufreq: qcom-fw: Add support for QCOM cpufreq FW driver

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 <[email protected]>

---
 .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 173 +++++++++++++++++++++
 1 file changed, 173 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +0 -0

new file mode 100644
index 0000000..e3087ec

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +15 -0

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

Quoted 10 lines · Taniya Das

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 <[email protected]>

---
  .../bindings/cpufreq/cpufreq-qcom-fw.txt           | 173 +++++++++++++++++++++
  1 file changed, 173 insertions(+)

create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 6

Quoted 12 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +6 -0

new file mode 100644
index 0000000..e3087ec
@@ -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.

Quoted 15 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +15 -0

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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 31

Quoted 10 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +10 -0

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

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.

Hello Sudeep,

Thanks for review comments.

Quoted 2 lines · Sudeep Holla

Quoted 4 lines

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.

Quoted 30 lines

Quoted 16 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +8 -0

+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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +7 -0

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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 31

Quoted 10 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +10 -0

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

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 7 lines · Taniya Das

Hello Sudeep,

Thanks for review comments.

Quoted 2 lines · Sudeep Holla

[...]

Quoted 9 lines

Quoted 4 lines

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.

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

Quoted 41 lines

Quoted 30 lines

Quoted 16 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +8 -0

+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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +7 -0

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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 31

Quoted 10 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +10 -0

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

Quoted 10 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

Quoted 4 lines

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 ?

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 ?

Quoted 10 lines

Quoted 9 lines

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.

Quoted 9 lines

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.

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 6 lines

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.

On Thu, Jun 14, 2018 at 9:24 PM, Taniya Das <[email protected]> wrote:

Quoted 5 lines

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.

"For now" - I think this is exactly the point that Sudeep is trying to make.

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

#define ENABLE_OFFSET 0x0
#define LUT_OFFSET 0x110
#define PERF_DESIRED_OFFSET 0x920

This will allow you add any new registers in the future w/o modifying the DT binding and reduce qcom_cpu_resources_init() to a handful of lines since you no longer need so many OF string matches, and devm_ioremap()s.

Quoted 44 lines · Taniya Das

Quoted 10 lines

Quoted 9 lines

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.

Quoted 9 lines

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.

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

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.

Quoted 6 lines

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 ?

Quoted 4 lines

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.

"For now" - I think this is exactly the point that Sudeep is trying to make.

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

#define ENABLE_OFFSET 0x0
#define LUT_OFFSET 0x110
#define PERF_DESIRED_OFFSET 0x920

This will allow you add any new registers in the future w/o modifying the DT binding and reduce qcom_cpu_resources_init() to a handful of lines since you no longer need so many OF string matches, and devm_ioremap()s.

Regards,
Amit

Quoted 73 lines · Amit Kucheria

Quoted 47 lines · Taniya Das

Quoted 16 lines

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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 ?

Quoted 6 lines

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.

"For now" - I think this is exactly the point that Sudeep is trying to make.

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

Quoted 4 lines

#define ENABLE_OFFSET               0x0
#define LUT_OFFSET                      0x110
#define PERF_DESIRED_OFFSET 0x920

The offsets could vary across versions of this IP and that is the reason to provide them through the DT and not define any such offsets.

Quoted 75 lines · Amit Kucheria

Quoted 47 lines · Taniya Das

Quoted 10 lines

Quoted 9 lines

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.

Quoted 9 lines

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.

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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 ?

Quoted 6 lines

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.

"For now" - I think this is exactly the point that Sudeep is trying to make.

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

Quoted 4 lines

#define ENABLE_OFFSET 0x0
#define LUT_OFFSET 0x110
#define PERF_DESIRED_OFFSET 0x920

The offsets could vary across versions of this IP and that is the reason to provide them through the DT and not define any such offsets.

On 6/15/2018 5:29 PM, Amit Kucheria wrote:

Quoted 14 lines

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

Quoted 4 lines

#define ENABLE_OFFSET               0x0
#define LUT_OFFSET                      0x110
#define PERF_DESIRED_OFFSET 0x920

The offsets could vary across versions of this IP and that is the reason to provide them through the DT and not define any such offsets.

Quoted 3 lines · Taniya Das

On 6/15/2018 5:29 PM, Amit Kucheria wrote:

[...]

Quoted 14 lines

Quoted 14 lines

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

Quoted 8 lines

Quoted 4 lines

#define ENABLE_OFFSET               0x0
#define LUT_OFFSET                      0x110
#define PERF_DESIRED_OFFSET 0x920

The offsets could vary across versions of this IP and that is the reason to provide them through the DT and not define any such offsets.

Just get compatibles to identify the version of the hardware if it can't be probed and detected. Please don't use DT to get the addresses of each register you use in the driver. That's neither scalable nor nice solution to the problem.
Hello Sudeep and Amit,

Driver code (The below representation is just for example).
=============

V1
#define ENABLE 0x0
#define LUT_V1 0x110
#define PERF_V1 0x920

V2
#define LUT_V2 0x150
#define PERF_V2 0x980

V3
#define LUT_V3 0x120
....

Do you want me to use "compatible" flag to

if (compatible == v1)
enable = readl_relaxed(X + LUT_V1);
else if (compatible == v2)
enable = readl_relaxed(X + LUT_V2);
else if (compatible == v3)
enable = readl_relaxed(X + LUT_V2);

Quoted 32 lines · Sudeep Holla

Quoted 3 lines · Taniya Das

On 6/15/2018 5:29 PM, Amit Kucheria wrote:

[...]

Quoted 14 lines

Quoted 14 lines

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

Quoted 8 lines

Quoted 4 lines

#define ENABLE_OFFSET               0x0
#define LUT_OFFSET                      0x110
#define PERF_DESIRED_OFFSET 0x920

The offsets could vary across versions of this IP and that is the reason to provide them through the DT and not define any such offsets.

Quoted 5 lines

Just get compatibles to identify the version of the hardware if it can't be probed and detected. Please don't use DT to get the addresses of each register you use in the driver. That's neither scalable nor nice solution to the problem.
Hello Sudeep and Amit,

Thanks for the comments, I am consolidating the understanding from the other emails in a single one.

I understand that you are looking for this IP to map the full region and define offsets according to access them.

But I still not sure how do you want this common driver to scale in the cases where the offsets could vary across version change.

 DT
====
  freq-node {
    reg = < X x_size>;   Where X is the start of the IP address. }

Driver code (The below representation is just for example).
=============

V1
#define ENABLE    0x0
#define LUT_V1    0x110
#define PERF_V1    0x920

V2
#define LUT_V2    0x150
#define PERF_V2    0x980

V3
#define LUT_V3    0x120
....

Do you want me to use "compatible" flag to

if (compatible == v1)
 enable =  readl_relaxed(X + LUT_V1);
else if (compatible == v2)
 enable = readl_relaxed(X + LUT_V2);
else if (compatible == v3)
 enable = readl_relaxed(X + LUT_V2);

With the current design I do not need such compatible checks and unmap the ones which are not required after probe.

Please let me know your comments.

Quoted 41 lines · Taniya Das

Quoted 32 lines · Sudeep Holla

Quoted 3 lines · Taniya Das

On 6/15/2018 5:29 PM, Amit Kucheria wrote:

[...]

Quoted 14 lines

Quoted 14 lines

A future version of the HW engine, or more likely, a firmware revision, will make more functionality available. Say, this needs access to another register or two. This will require changing the DT bindings. Instead, if you map the entire address space, you can just add offsets to the new registers.

So in this case, I think you should define the following addresses (size 0x1400) for the two frequency domains

0x17d43000, 0x1400 (power cluster)
0x17d45800, 0x1400 (perf cluster)

And in the driver simply add offsets as follows:

Quoted 8 lines

Quoted 4 lines

#define ENABLE_OFFSET               0x0
#define LUT_OFFSET                      0x110
#define PERF_DESIRED_OFFSET 0x920

The offsets could vary across versions of this IP and that is the reason to provide them through the DT and not define any such offsets.

Quoted 5 lines

Just get compatibles to identify the version of the hardware if it can't be probed and detected. Please don't use DT to get the addresses of each register you use in the driver. That's neither scalable nor nice solution to the problem.
Hello Sudeep and Amit,

Quoted 9 lines

Thanks for the comments, I am consolidating the understanding from the other emails in a single one.

I understand that you are looking for this IP to map the full region and define offsets according to access them.

But I still not sure how do you want this common driver to scale in the cases where the offsets could vary across version change.

There are plenty of drivers that you can look at as example. TBH most of the drivers implementing support for multiple versions of IP do something on the similar lines.

Quoted 22 lines

 DT
====
  freq-node {
    reg = < X x_size>;   Where X is the start of the IP address. }

Driver code (The below representation is just for example).
=============

V1
#define ENABLE    0x0
#define LUT_V1    0x110
#define PERF_V1    0x920

V2
#define LUT_V2    0x150
#define PERF_V2    0x980

V3
#define LUT_V3    0x120
....

Quoted 9 lines

Do you want me to use "compatible" flag to

if (compatible == v1)
 enable =  readl_relaxed(X + LUT_V1);
else if (compatible == v2)
 enable = readl_relaxed(X + LUT_V2);
else if (compatible == v3)
 enable = readl_relaxed(X + LUT_V2);

These are implementation details. But you should try to use compatibles only in probe and just record the version in some variable or update the offsets in some device specific structure so that you can use that unconditionally for any access you make on that device.

Quoted 2 lines

With the current design I do not need such compatible checks and unmap the ones which are not required after probe.

I am not sure what you mean by unmap after probe.

Quoted 2 lines

Please let me know your comments.

Please look at some drivers in the Linux tree for examples. Infact there may be few drivers on QCOM SoC itself. What I am suggesting is the normal practice in the drivers and you should see plenty of examples. Since I was looking at some serial port patch, I can say you can have a look at drivers/tty/serial/amba-pl011.c which supports multiple versions from different vendors. I am sure there are many simpler examples but AMBA PL011 just stood out.

Hello Sudeep,

Thanks for your comments.

Quoted 23 lines · Sudeep Holla

Quoted 7 lines · Taniya Das

Hello Sudeep,

Thanks for review comments.

Quoted 2 lines · Sudeep Holla

[...]

Quoted 9 lines

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

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

Quoted 57 lines

Quoted 42 lines

Quoted 30 lines

Quoted 16 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +8 -0

+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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +7 -0

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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 31

Quoted 10 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +10 -0

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

Quoted 11 lines

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.

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 34 lines · Taniya Das

Hello Sudeep,

Thanks for your comments.

Quoted 23 lines · Sudeep Holla

Quoted 7 lines · Taniya Das

Hello Sudeep,

Thanks for review comments.

Quoted 2 lines · Sudeep Holla

[...]

Quoted 9 lines

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

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

Quoted 57 lines

Quoted 57 lines

Quoted 42 lines

Quoted 30 lines

Quoted 16 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +8 -0

+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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +7 -0

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

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt line 31

Quoted 10 lines

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +10 -0

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

Quoted 11 lines

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.

Quoted 9 lines

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.

Quoted 8 lines

Quoted 4 lines

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"

Quoted 21 lines

Quoted 10 lines

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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 ?

Quoted 5 lines

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.

Quoted 1 line · Sudeep Holla

Quoted 14 lines

Quoted 13 lines

Quoted 7 lines

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.

Quoted 3 lines

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.

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 <[email protected]>

Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-fw.txt new file +9 -0

+                       reg-names = "perf", "lut", "enable";
+               };
+
+               freq_domain_table1 : freq_table1 {
+                       reg = <0x17d46120 0x4>, <0x17d45910 0x500>,
+                               <0x17d45800 0x4> ;
+                       reg-names = "perf", "lut", "enable";
+               };
+       };

The CPUfreq FW present in some QCOM chipsets offloads the steps necessary for changing the frequency of CPUs. The driver implements the cpufreq driver interface for this firmware.

Signed-off-by Saravana Kannan <[email protected]>

Signed-off-by Taniya Das <[email protected]>

---
 drivers/cpufreq/Kconfig.arm       |   9 +
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-fw.c | 336 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 346 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

drivers/cpufreq/Kconfig.arm +0 -0

index 52f5f1a..2683716 100644

drivers/cpufreq/Kconfig.arm +2 -0

+        implements the cpufreq driver interface for this firmware.
+        Say Y if you want to support CPUFreq FW.

drivers/cpufreq/Makefile +1 -0

index fb4a2ec..34691a2 100644
@@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)    += tegra124-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)     += tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)           += ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ) += vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)      += qcom-cpufreq-fw.o
 

drivers/cpufreq/Makefile +11 -0

+static const struct of_device_id match_table[] = {
+       { .compatible = "qcom,cpufreq-fw" },
+       {}
+};
+
+static struct platform_driver qcom_cpufreq_fw_driver = {
+       .probe = qcom_cpufreq_fw_driver_probe,
+       .driver = {
+               .name = "qcom-cpufreq-fw",
+               .of_match_table = match_table,
+               .owner = THIS_MODULE,

+MODULE_LICENSE("GPL v2");
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

drivers/cpufreq/qcom-cpufreq-fw.c new file +85 -0

+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
+{
+	struct device_node *cpu_np, *freq_np;
+	int cpu;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np)
+			continue;
+		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		if (!freq_np)
+			continue;
+		if (freq_np == np)
+			cpumask_set_cpu(cpu, m);
+	}
+
+	return 0;
+}
+
+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np, unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	void __iomem *en_base;
+	int index, ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	index = of_property_match_string(np, "reg-names", "enable");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	en_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!en_base) {
+		dev_err(dev, "Unable to map %s enable-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	/* FW should be in enabled state to proceed */
+	if (!(readl_relaxed(en_base) & 0x1)) {
+		dev_err(dev, "%s firmware not enabled\n", np->name);
+		return -ENODEV;
+	}
+	devm_iounmap(&pdev->dev, en_base);
+
+	index = of_property_match_string(np, "reg-names", "perf");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->perf_base) {
+		dev_err(dev, "Unable to map %s perf-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	index = of_property_match_string(np, "reg-names", "lut");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->lut_base) {
+		dev_err(dev, "Unable to map %s lut-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	ret = qcom_get_related_cpus(np, &c->related_cpus);
+	if (ret) {
+		dev_err(dev, "%s failed to get related CPUs\n", np->name);
+		return ret;
+	}
+
+	c->max_cores = cpumask_weight(&c->related_cpus);

drivers/cpufreq/qcom-cpufreq-fw.c new file +33 -0

+
+	return 0;
+}
+
+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np, *cpu_np;
+	unsigned int cpu;
+	int ret;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np) {
+			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
+				cpu);
+			continue;
+		}
+
+		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Failed to get freq-domain device\n");
+			return -EINVAL;
+		}
+
+		of_node_put(cpu_np);
+
+		ret = qcom_cpu_resources_init(pdev, np, cpu);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}

The CPUfreq FW present in some QCOM chipsets offloads the steps necessary for changing the frequency of CPUs. The driver implements the cpufreq driver interface for this firmware.

Signed-off-by Saravana Kannan <[email protected]>

Signed-off-by Taniya Das <[email protected]>

---
 drivers/cpufreq/Kconfig.arm       |   9 +
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/qcom-cpufreq-fw.c | 336 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 346 insertions(+)
 create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

drivers/cpufreq/Kconfig.arm +9 -0

index 52f5f1a..2683716 100644
@@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
 	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
 
 	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_FW
+	bool "QCOM CPUFreq FW driver"
+	help
+	 Support for the CPUFreq FW driver.
+	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
+	 necessary for changing the frequency of CPUs. The driver
+	 implements the cpufreq driver interface for this firmware.
+	 Say Y if you want to support CPUFreq FW.

drivers/cpufreq/Makefile +1 -0

index fb4a2ec..34691a2 100644
@@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o
 
 
 ##################################################################################

drivers/cpufreq/qcom-cpufreq-fw.c new file +15 -0

new file mode 100644
index 0000000..62f4452
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define INIT_RATE			300000000UL
+#define XO_RATE				19200000UL
+#define LUT_MAX_ENTRIES			40U

drivers/cpufreq/qcom-cpufreq-fw.c new file +11 -0

+static int qcom_read_lut(struct platform_device *pdev,
+			 struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+
+	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*c->table), GFP_KERNEL);
+	if (!c->table)
+		return -ENOMEM;
+

drivers/cpufreq/qcom-cpufreq-fw.c new file +4 -0

+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
+{
+	struct device_node *cpu_np, *freq_np;
+	int cpu;

drivers/cpufreq/qcom-cpufreq-fw.c new file +48 -0

+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np, unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	void __iomem *en_base;
+	int index, ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	index = of_property_match_string(np, "reg-names", "enable");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	en_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!en_base) {
+		dev_err(dev, "Unable to map %s enable-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	/* FW should be in enabled state to proceed */
+	if (!(readl_relaxed(en_base) & 0x1)) {
+		dev_err(dev, "%s firmware not enabled\n", np->name);
+		return -ENODEV;
+	}
+	devm_iounmap(&pdev->dev, en_base);
+
+	index = of_property_match_string(np, "reg-names", "perf");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->perf_base) {
+		dev_err(dev, "Unable to map %s perf-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	index = of_property_match_string(np, "reg-names", "lut");
+	if (index < 0)

drivers/cpufreq/qcom-cpufreq-fw.c new file +7 -0

+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np, *cpu_np;
+	unsigned int cpu;
+	int ret;
+
+	for_each_possible_cpu(cpu) {

Hi,

Quoted 63 lines · Taniya Das

The CPUfreq FW present in some QCOM chipsets offloads the steps necessary for changing the frequency of CPUs. The driver implements the cpufreq driver interface for this firmware.

Signed-off-by Saravana Kannan <[email protected]>

Signed-off-by Taniya Das <[email protected]>

---
  drivers/cpufreq/Kconfig.arm       |   9 +
  drivers/cpufreq/Makefile          |   1 +
  drivers/cpufreq/qcom-cpufreq-fw.c | 336 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 346 insertions(+)

create mode 100644 drivers/cpufreq/qcom-cpufreq-fw.c

drivers/cpufreq/Kconfig.arm +9 -0

index 52f5f1a..2683716 100644
@@ -312,3 +312,12 @@ config ARM_PXA2xx_CPUFREQ
  	  This add the CPUFreq driver support for Intel PXA2xx SOCs.
 
  	  If in doubt, say N.
+
+config ARM_QCOM_CPUFREQ_FW
+	bool "QCOM CPUFreq FW driver"
+	help
+	 Support for the CPUFreq FW driver.
+	 The CPUfreq FW preset in some QCOM chipsets offloads the steps
+	 necessary for changing the frequency of CPUs. The driver
+	 implements the cpufreq driver interface for this firmware.
+	 Say Y if you want to support CPUFreq FW.

drivers/cpufreq/Makefile +1 -0

index fb4a2ec..34691a2 100644
@@ -86,6 +86,7 @@ obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
  obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
+obj-$(CONFIG_ARM_QCOM_CPUFREQ_FW)	+= qcom-cpufreq-fw.o
 
 
  ##################################################################################

drivers/cpufreq/qcom-cpufreq-fw.c new file +15 -0

new file mode 100644
index 0000000..62f4452
@@ -0,0 +1,336 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+
+#define INIT_RATE			300000000UL
+#define XO_RATE				19200000UL
+#define LUT_MAX_ENTRIES			40U

drivers/cpufreq/qcom-cpufreq-fw.c line 25

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+#define CORE_COUNT_VAL(val)		(((val) & (GENMASK(18, 16))) >> 16)
+#define LUT_ROW_SIZE			32
+
+struct cpufreq_qcom {
+	struct cpufreq_frequency_table *table;
+	struct device *dev;
+	void __iomem *perf_base;
+	void __iomem *lut_base;
+	cpumask_t related_cpus;
+	unsigned int max_cores;

Why *max*_cores? This seems to be the number of CPUs in a cluster and qcom_read_lut() expects the core count read from the LUT to match exactly.

Quoted 11 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +11 -0

+static int qcom_read_lut(struct platform_device *pdev,
+			 struct cpufreq_qcom *c)
+{
+	struct device *dev = &pdev->dev;
+	u32 data, src, lval, i, core_count, prev_cc, prev_freq, cur_freq;
+
+	c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
+				sizeof(*c->table), GFP_KERNEL);
+	if (!c->table)
+		return -ENOMEM;
+

drivers/cpufreq/qcom-cpufreq-fw.c line 133

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(c->lut_base + i * LUT_ROW_SIZE);
+		src = ((data & GENMASK(31, 30)) >> 30);
+		lval = (data & GENMASK(7, 0));
+		core_count = CORE_COUNT_VAL(data);
+
+		if (!src)
+			c->table[i].frequency = INIT_RATE / 1000;
+		else
+			c->table[i].frequency = XO_RATE * lval / 1000;

nit: any particular reason to use negative logic here? Why not check for 'src[ != NULL]', which also seems to be the more common case.

Quoted 4 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +4 -0

+static int qcom_get_related_cpus(struct device_node *np, struct cpumask *m)
+{
+	struct device_node *cpu_np, *freq_np;
+	int cpu;

drivers/cpufreq/qcom-cpufreq-fw.c line 177

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np)
+			continue;
+		freq_np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		if (!freq_np)
+			continue;
+		if (freq_np == np)
+			cpumask_set_cpu(cpu, m);

missing 'of_node_put(cpu_np)'. You might want to do it at the end of the loop and use a 'goto' above instead of 'continue'.

Quoted 48 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +48 -0

+static int qcom_cpu_resources_init(struct platform_device *pdev,
+				   struct device_node *np, unsigned int cpu)
+{
+	struct cpufreq_qcom *c;
+	struct resource res;
+	struct device *dev = &pdev->dev;
+	void __iomem *en_base;
+	int index, ret;
+
+	c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	index = of_property_match_string(np, "reg-names", "enable");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	en_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!en_base) {
+		dev_err(dev, "Unable to map %s enable-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	/* FW should be in enabled state to proceed */
+	if (!(readl_relaxed(en_base) & 0x1)) {
+		dev_err(dev, "%s firmware not enabled\n", np->name);
+		return -ENODEV;
+	}
+	devm_iounmap(&pdev->dev, en_base);
+
+	index = of_property_match_string(np, "reg-names", "perf");
+	if (index < 0)
+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->perf_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->perf_base) {
+		dev_err(dev, "Unable to map %s perf-base\n", np->name);
+		return -ENOMEM;
+	}
+
+	index = of_property_match_string(np, "reg-names", "lut");
+	if (index < 0)

drivers/cpufreq/qcom-cpufreq-fw.c line 240

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+		return index;
+
+	if (of_address_to_resource(np, index, &res))
+		return -ENOMEM;
+
+	c->lut_base = devm_ioremap(dev, res.start, resource_size(&res));
+	if (!c->lut_base) {
+		dev_err(dev, "Unable to map %s lut-base\n", np->name);
+		return -ENOMEM;
+	}

The of_property_match_string() - of_address_to_resource() - devm_ioremap() pattern is repeated 3x. In case the binding doesn't change (there is discussion on the DT patch) you might want to move this to a helper.

Quoted 7 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +7 -0

+static int qcom_resources_init(struct platform_device *pdev)
+{
+	struct device_node *np, *cpu_np;
+	unsigned int cpu;
+	int ret;
+
+	for_each_possible_cpu(cpu) {

drivers/cpufreq/qcom-cpufreq-fw.c line 279

Quoted 10 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +10 -0

+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np) {
+			dev_err(&pdev->dev, "Failed to get cpu %d device\n",
+				cpu);
+			continue;
+		}
+
+		np = of_parse_phandle(cpu_np, "qcom,freq-domain", 0);
+		if (!np) {
+			dev_err(&pdev->dev, "Failed to get freq-domain device\n");

			of_node_put(cpu_np);

drivers/cpufreq/qcom-cpufreq-fw.c line 290

Quoted 11 lines

drivers/cpufreq/qcom-cpufreq-fw.c new file +11 -0

+			return -EINVAL;
+		}
+
+		of_node_put(cpu_np);
+
+		ret = qcom_cpu_resources_init(pdev, np, cpu);
+		if (ret)
+			return ret;
+	}
+
+	return 0;

Cheers

Matthias