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

[Taniya Das]

On 6/15/2018 5:29 PM, Amit Kucheria wrote:
> On Thu, Jun 14, 2018 at 9:24 PM, Taniya Das <tdas@xxxxxxxxxxxxxx> wrote:
>
> > > > 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.
> >
> > > > > 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.
>
> "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

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.

> 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

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

--