Re: [PATCH 1/2] dt-bindings: Documentation for qcom,llcc

[Channa <ckadabi@codeaurora.org>]

On 2018-02-13 06:37, Mark Rutland wrote:
> On Tue, Feb 06, 2018 at 11:56:50AM -0800, Channa wrote:
>> On 2018-02-02 03:05, Mark Rutland wrote:
>> > On Thu, Feb 01, 2018 at 12:39:09PM -0800, Channa wrote:
>> > > On 2018-02-01 02:44, Mark Rutland wrote:
>> > > > On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote:
>> > > > > +- llcc-bank-off:
>> > > > > +	Usage: required
>> > > > > +	Value Type: <u32 array>
>> > > > > +	Definition: Offsets of llcc banks from llcc base address starting
>> > > > > from
>> > > > > +		    LLCC bank0.
>> > > > > +
>> > > > > +- llcc-broadcast-off:
>> > > > > +	Usage: required
>> > > > > +	Value Type: <u32>
>> > > > > +	Definition: Offset of broadcast register from LLCC bank0 address.
>> > > >
>> > > > Please could we use "offset" rather than "off" for both of these? That
>> > > > way it's obvious these aren't properties for disabling some feature.
>> > > >
>> > > > How variable are these offsets in practice? Is the memory map not fixed?
>> > >
>> > > The offsets depends on the number of LLCC HW blocks. These number of
>> > > HW
>> > > blocks vary from
>> > > chipset to chipset and new registers could be added that changes the
>> > > offset.
>> >
>> > Surely if new registers are added, we need a new compatible string?
>> >
>> > Can't we encode the number of LLCC HW blocks, instead? Presumably that
>> > would give enough information to cover both llcc-bank-off and
>> > llcc-broadcast-off.
>> >
>> > [...]
>> 
>> Are you suggesting to move these offset handing out of DTS files and 
>> manage
>> in the driver?
> 
> Something like that, though it depends on how exactly the offsets can 
> be
> derived.
> 
> Using reg entries, as Matt suggested, sounds better though.
> 
>> > > > > +compatible devices:
>> > > > > +		qcom,sdm845-llcc
>> > > >
>> > > > Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's
>> > > > not clear what this means.
>> > > >
>> > > > > +
>> > > > > +Example:
>> > > > > +
>> > > > > +	qcom,system-cache@1300000 {
>> > > > > +		compatible = "qcom,llcc-core", "syscon", "simple-mfd";
>> > > >
>> > > > This looks very wrong. Why do you need syscon and simple-mfd?
>> > >
>> > > LLCC HW block has 3 functionalities:
>> > > System cache core, ECC & AMON drivers for debugging.
>> > > All three drivers use the same register space for configuration,
>> > > status etc.
>> > > In order to avoid remapping the same address region across multiple
>> > > drivers,
>> > > I have implemented this driver as a syncon and simple-mfd.
>> >
>> > Please don't do that; that's completely dependent on Linux
>> > implementation details.
>> 
>> Why do you think simple-mfd is not good here? The LLCC HW clock is 
>> outside
>> of CPUSS and has
>> multiple functional blocks.
> 
> For one thing, there's no need for this to be a syscon *and* a
> simple-mfd.
> 
> W.R.T. simple-mfd, I think it would bet better to decompose the device
> in a top-level driver, as I described in my prior reply, rather than
> describing a set of drivers (which are not themselves HW).
> 
>> > > > > +- cache-slices:
>> > > > > +	Usage: required
>> > > > > +	Value type: <prop-encoded-array>
>> > > > > +	Definition: The tuple has phandle to llcc device as the first
>> > > > > argument and the
>> > > > > +		    second argument is the usecase id of the client.
>> > > >
>> > > > What is a "usecase id" ?
>> > >
>> > > Usecase id for use case that wants to use system cache for eg:
>> > > video-encode
>> > > and video-decode
>> >
>> > Sure, but how is the value used? Is it the index of a slice? Or
>> > something more abstract?
>> 
>> This is used as an index to the SCT (System cache Table) configuration
>> data that controls the behavior of each cache slice.
> 
> Ok. Where does that SCT live? Is that in HW? Is it programmed by SW, or
> statically configured for a given platform?

SCT is in the HW. Kernel driver programs these settings for a given
platform. The table is also used to lookup size, cache slice id details
requested by client drivers.

> 
> Thanks,
> Mark.

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