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

[Mark Rutland]

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:
> > > Documentation for last level cache controller device tree bindings,
> > > client bindings usage examples.
> > > 
> > > Signed-off-by: Channagoud Kadabi <ckadabi@xxxxxxxxxxxxxx>
> > > ---
> > >  .../devicetree/bindings/arm/msm/qcom,llcc.txt      | 93
> > > ++++++++++++++++++++++
> > >  1 file changed, 93 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..d433b0c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
> > > @@ -0,0 +1,93 @@
> > > +* LLCC (Last Level Cache Controller)
> > > +
> > > +Properties:
> > > +- compatible:
> > > +	Usage: required
> > > +	Value type: <string>
> > > +	Definition: must be "qcom,llcc-core"
> > > +
> > > +- reg:
> > > +	Usage: required
> > > +	Value Type: <prop-encoded-array>
> > > +	Definition: must be addresses and sizes of the LLCC registers
> > > +
> > > +- 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.

[...]

> > > +
> > > +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.

Have one top level driver for the whole LLCC block, which maps the
registers, and provides an API for accessing them. When that probes, it
can cause the other drivers to be probed (e.g. with a platform device),
and those can access the LLCC registers via that API.

> > > +		reg = <0x1300000 0x50000>;
> > > +		reg-names = "llcc_base";
> > > +
> > > +		llcc: qcom,sdm845-llcc {
> > > +			compatible = "qcom,sdm845-llcc";
> > 
> > Why is this a sub-node?
> qcom,sdm845-llcc: This core driver as mentioned in the list above.
> > 
> > Why isn't the top-level node just "qcom,sdm845-llcc" ?
> > 
> > > +			#cache-cells = <1>;
> > > +			max-slices = <32>;
> > > +		};
> > > +
> > > +		qcom,llcc-ecc {
> > > +			compatible = "qcom,llcc-ecc";
> > > +		};
> 
> qcom,llcc-ecc: Driver #2 for ECC
> 
> > > +
> > > +		qcom,llcc-amon {
> > > +			compatible = "qcom,llcc-amon";
> > > +			qcom,fg-cnt = <0x7>;
> > > +		};
> > > +
> 
> qcom,llcc-amon: Driver #3 for AMON

Please describe the HW, not the drivers.

As above, I don't believe you need multiple nodes here. Linux can
instantiate the drivers as necessary.

[...]

> > > +- 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?

Thanks,
Mark.