Messages in this thread | | | Subject | Re: [PATCH v4 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ FW bindings | From | Taniya Das <> | Date | Tue, 19 Jun 2018 16:14:50 +0530 |
| |
On 6/19/2018 3:04 PM, Sudeep Holla wrote: > > > On 19/06/18 08:53, Taniya Das wrote: >> >> >> On 6/18/2018 2:51 PM, Sudeep Holla wrote: >>> >>> >>> On 15/06/18 18:40, Taniya Das wrote: >>>> >>>> >>>> On 6/15/2018 5:29 PM, Amit Kucheria wrote: >>> >>> [...] >>> >>>>> 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. >>>> >>> >>> 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. >> > > 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. > >> 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); >> > > 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. > >> 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. > >> 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. >
Thanks Sudeep, let me take a look at the driver to see how I can associate data (offsets) based on compatible.
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation.
--
| |