On Thu, Feb 01, 2018 at 12:39:09PM -0800, Channa wrote:

On 2018-02-01 02:44, Mark Rutland wrote:

quoted

On Thu, Jan 25, 2018 at 03:55:12PM -0800, Channagoud Kadabi wrote:

quoted

Documentation for last level cache controller device tree bindings,
client bindings usage examples.

Signed-off-by: Channagoud Kadabi <redacted>
---
 .../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.

[...]

quoted

quoted

+
+compatible devices:
+		qcom,sdm845-llcc

Huh? The "qcom,sdm845-llcc" bindings wasn't described above, and it's
not clear what this means.

quoted

+
+Example:
+
+	qcom,system-cache at 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.

quoted

quoted

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

quoted

Why isn't the top-level node just "qcom,sdm845-llcc" ?

quoted

+			#cache-cells = <1>;
+			max-slices = <32>;
+		};
+
+		qcom,llcc-ecc {
+			compatible = "qcom,llcc-ecc";
+		};

qcom,llcc-ecc: Driver #2 for ECC

quoted

quoted

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

[...]

quoted

quoted

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