Re: [PATCH v4 1/2] Documentation: Documentation for qcom, llcc

[Channa <ckadabi@codeaurora.org>]

On 2018-04-18 11:11, Channa wrote:
> On 2018-04-18 07:52, Rob Herring wrote:
>> On Tue, Apr 17, 2018 at 5:12 PM,  <rishabhb@codeaurora.org> wrote:
>>> On 2018-04-17 10:43, rishabhb@codeaurora.org wrote:
>>>> 
>>>> On 2018-04-16 07:59, Rob Herring wrote:
>>>>> 
>>>>> On Tue, Apr 10, 2018 at 01:08:12PM -0700, Rishabh Bhatnagar wrote:
>>>>>> 
>>>>>> Documentation for last level cache controller device tree 
>>>>>> bindings,
>>>>>> client bindings usage examples.
>>>>> 
>>>>> 
>>>>> "Documentation: Documentation ..."? That wastes a lot of the 
>>>>> subject
>>>>> line... The preferred prefix is "dt-bindings: ..."
>>>>> 
>>>>>> 
>>>>>> Signed-off-by: Channagoud Kadabi <ckadabi@codeaurora.org>
>>>>>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>>>>>> ---
>>>>>>  .../devicetree/bindings/arm/msm/qcom,llcc.txt      | 58
>>>>>> ++++++++++++++++++++++
>>>>>>  1 file changed, 58 insertions(+)
>>>>>>  create mode 100644
>>>>>> Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
>>>>>> 
>>>>>> diff --git 
>>>>>> a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
>>>>>> b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..497cf0f
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
>>>>>> @@ -0,0 +1,58 @@
>>>>>> +== Introduction==
>>>>>> +
>>>>>> +LLCC (Last Level Cache Controller) provides last level of cache 
>>>>>> memory
>>>>>> in SOC,
>>>>>> +that can be shared by multiple clients. Clients here are 
>>>>>> different
>>>>>> cores in the
>>>>>> +SOC, the idea is to minimize the local caches at the clients and
>>>>>> migrate to
>>>>>> +common pool of memory
>>>>>> +
>>>>>> +Properties:
>>>>>> +- compatible:
>>>>>> +        Usage: required
>>>>>> +        Value type: <string>
>>>>>> +        Definition: must be "qcom,sdm845-llcc"
>>>>>> +
>>>>>> +- reg:
>>>>>> +        Usage: required
>>>>>> +        Value Type: <prop-encoded-array>
>>>>>> +        Definition: must be addresses and sizes of the LLCC 
>>>>>> registers
>>>>> 
>>>>> 
>>>>> How many address ranges?
>>>>> 
>>>> It consists of just one address range. I'll edit the definition to 
>>>> make
>>>> it more clear.
>>>>>> 
>>>>>> +
>>>>>> +- #cache-cells:
>>>>> 
>>>>> 
>>>>> This is all written as it is a common binding, but it is not one.
>>>>> 
>>>>> You already have most of the configuration data for each client in 
>>>>> the
>>>>> driver, I think I'd just put the client connection there too. Is 
>>>>> there
>>>>> any variation of this for a given SoC?
>>>>> 
>>>> #cache-cells and max-slices won't change for a given SOC. So you 
>>>> want me
>>>> to hard-code in the driver itself?
>>>> 
>>> I can use of_parse_phandle_with_fixed_args function and fix the 
>>> number of
>>> args as 1 instead of keeping #cache-cells here in DT. Does that look 
>>> fine?
>> 
>> No, I'm saying why even put cache-slices properties in DT to begin
>> with? You could just define client id's within the kernel and clients
>> can use those instead of getting the id from the DT.
> 
> The reason to add cache-slices here is to establish a connection 
> between
> client and system cache. For example if we have multiple instances of
> system cache blocks and client wants to choose a system cache instance
> based on the usecase then its easier to establish this connection using
> device tree than hard coding in the driver.
> 
>> 
>> I have a couple of hesitations with putting this into the DT. First, I
>> think a cache is just one aspect of describing the interconnect
>> between masters and memory (and there's been discussions on
>> interconnect bindings too) and any binding needs to consider all of
>> the aspects of the interconnect. Second, I'd expect this cache
>> architecture will change SoC to SoC and the binding here is pretty
>> closely tied to the current cache implementation (e.g. slices). If
>> there were a bunch of SoCs with the same design and just different
>> client IDs (like interrupt IDs), then I'd feel differently.
> 
> This is partially true, a bunch of SoCs would support this design but
> clients IDs are not expected to change. So Ideally client drivers could
> hard code these IDs.
> 
> However I have other concerns of moving the client Ids in the driver.
> The way the APIs implemented today are as follows:
> #1. Client calls into system cache driver to get cache slice handle
> with the usecase Id as input.
> #2. System cache driver gets the phandle of system cache instance from
> the client device to obtain the private data.
> #3. Based on the usecase Id perform look up in the private data to get
> cache slice handle.
> #4. Return the cache slice handle to client
> 
> If we don't have the connection between client & system cache then the
> private data needs to declared as static global in the system cache 
> driver,
> that limits us to have just once instance of system cache block.
> 
> 
>> 
>> Rob

Hi Rob:

Can you please provide your opinion on the approach here?

Thanks,
Channa

-- 
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project