Messages in this thread | | | Subject | Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings | From | Taniya Das <> | Date | Fri, 15 Jun 2018 23:01:58 +0530 |
| |
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.
--
| |