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

[Channa]

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,  <risha...@codeaurora.org> wrote:

On 2018-04-17 10:43, risha...@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 <ckad...@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <risha...@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 000..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: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+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

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

[Channa]

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,   wrote:

On 2018-04-17 10:43, risha...@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 
Signed-off-by: Rishabh Bhatnagar 
---
 .../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 000..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: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+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

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

[Channa]

On 2018-04-18 07:52, Rob Herring wrote:

On Tue, Apr 17, 2018 at 5:12 PM,   wrote:

On 2018-04-17 10:43, risha...@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 
Signed-off-by: Rishabh Bhatnagar 
---
 .../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 000..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: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+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


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

a Linux Foundation Collaborative Project

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

[Channa]

On 2018-04-18 07:52, Rob Herring wrote:

On Tue, Apr 17, 2018 at 5:12 PM,   wrote:

On 2018-04-17 10:43, risha...@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 
Signed-off-by: Rishabh Bhatnagar 
---
 .../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 000..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: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+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


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

a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] drivers: soc: Add LLCC driver

[Channa]

On 2018-04-02 02:11, Stanimir Varbanov wrote:

Hi,

Please adjust your mail client to drop on new line on 80 column!

On 03/29/2018 08:55 PM, Channa wrote:

On 2018-03-29 01:08, Stanimir Varbanov wrote:

Hi,

On 03/27/2018 09:52 PM, Rishabh Bhatnagar wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice getting its own priority, size, ID and other config 
parameters.

LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of 
the

 slice they get and activate or deactivate the slice as needed. LLCC
driver
provides API interfaces for the clients to perform these operations.

Signed-off-by: Channagoud Kadabi <ckad...@codeaurora.org>
Signed-off-by: Rishabh Bhatnagar <risha...@codeaurora.org>
---
 drivers/soc/qcom/Kconfig   |  16 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c | 120 ++
 drivers/soc/qcom/llcc-slice.c  | 454
+


I'd name it just llcc.c, slice suffix didn't add any value.


 include/linux/soc/qcom/llcc-qcom.h | 178 +++
 5 files changed, 770 insertions(+)
 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e050eb8..28237fc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -21,6 +21,22 @@ config QCOM_GSBI
   functions for connecting the underlying serial UART, SPI,
and I2C
   devices to the output pins.

+config QCOM_LLCC
+    tristate "Qualcomm Technologies, Inc. LLCC driver"
+    depends on ARCH_QCOM
+    help
+  Qualcomm Technologies, Inc. platform specific LLCC driver for
Last
+  Level Cache. This provides interfaces to client's that use 
the

LLCC.
+  Say yes here to enable LLCC slice driver.
+
+config QCOM_SDM845_LLCC
+    tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
+    depends on QCOM_LLCC
+    help
+  Say yes here to enable the LLCC driver for SDM845. This is
provides
+  data required to configure LLCC so that clients can start
using the
+  LLCC slices.
+
 config QCOM_MDT_LOADER
 tristate
 select QCOM_SCM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index dcebf28..e16d6a2 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,3 +12,5 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)    += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)    += smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
+obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
diff --git a/drivers/soc/qcom/llcc-sdm845.c
b/drivers/soc/qcom/llcc-sdm845.c
new file mode 100644
index 000..cd431d9
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,120 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ * This program is free software; you can redistribute it and/or 
modify
+ * it under the terms of the GNU General Public License version 2 
and

+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+/*
+ * SCT entry contains of the following parameters
+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case
+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for
replacement
+ * fixed_size: Determine of the slice has a fixed capacity
+ * bonus_ways: Bonus ways to be used by any slice, bonus way is 
used

only if
+ * it't not a reserved way.
+ * res_ways: Reserved ways for the cache slice, the reserved ways
cannot be used
+ *   by any other client than the one its assigned to.
+ * cache_mode: Each slice operates as a cache, this controls the
mode of the
+ * slice normal or TCM
+ * probe_target_ways: Determines what ways to probe for access hit.
When
+ *    configured to 1 only bonus and reseved ways
are probed.
+ *    when configured to 0 all ways in llcc are 
probed.

+ * dis_cap_alloc: Disable capacity based allocation for a client
+ * retain_on_pc: If this bit is set and client has maitained active
vote
+ *   then the ways assigned to this client are not
flushed on power
+ *   collapse.
+ * 

Re: [PATCH v3 2/2] drivers: soc: Add LLCC driver

[Channa]

On 2018-04-02 02:11, Stanimir Varbanov wrote:

Hi,

Please adjust your mail client to drop on new line on 80 column!

On 03/29/2018 08:55 PM, Channa wrote:

On 2018-03-29 01:08, Stanimir Varbanov wrote:

Hi,

On 03/27/2018 09:52 PM, Rishabh Bhatnagar wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice getting its own priority, size, ID and other config 
parameters.

LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of 
the

 slice they get and activate or deactivate the slice as needed. LLCC
driver
provides API interfaces for the clients to perform these operations.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/soc/qcom/Kconfig   |  16 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c | 120 ++
 drivers/soc/qcom/llcc-slice.c  | 454
+


I'd name it just llcc.c, slice suffix didn't add any value.


 include/linux/soc/qcom/llcc-qcom.h | 178 +++
 5 files changed, 770 insertions(+)
 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e050eb8..28237fc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -21,6 +21,22 @@ config QCOM_GSBI
   functions for connecting the underlying serial UART, SPI,
and I2C
   devices to the output pins.

+config QCOM_LLCC
+    tristate "Qualcomm Technologies, Inc. LLCC driver"
+    depends on ARCH_QCOM
+    help
+  Qualcomm Technologies, Inc. platform specific LLCC driver for
Last
+  Level Cache. This provides interfaces to client's that use 
the

LLCC.
+  Say yes here to enable LLCC slice driver.
+
+config QCOM_SDM845_LLCC
+    tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
+    depends on QCOM_LLCC
+    help
+  Say yes here to enable the LLCC driver for SDM845. This is
provides
+  data required to configure LLCC so that clients can start
using the
+  LLCC slices.
+
 config QCOM_MDT_LOADER
 tristate
 select QCOM_SCM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index dcebf28..e16d6a2 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,3 +12,5 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)    += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)    += smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
+obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
diff --git a/drivers/soc/qcom/llcc-sdm845.c
b/drivers/soc/qcom/llcc-sdm845.c
new file mode 100644
index 000..cd431d9
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,120 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ * This program is free software; you can redistribute it and/or 
modify
+ * it under the terms of the GNU General Public License version 2 
and

+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+/*
+ * SCT entry contains of the following parameters
+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case
+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for
replacement
+ * fixed_size: Determine of the slice has a fixed capacity
+ * bonus_ways: Bonus ways to be used by any slice, bonus way is 
used

only if
+ * it't not a reserved way.
+ * res_ways: Reserved ways for the cache slice, the reserved ways
cannot be used
+ *   by any other client than the one its assigned to.
+ * cache_mode: Each slice operates as a cache, this controls the
mode of the
+ * slice normal or TCM
+ * probe_target_ways: Determines what ways to probe for access hit.
When
+ *    configured to 1 only bonus and reseved ways
are probed.
+ *    when configured to 0 all ways in llcc are 
probed.

+ * dis_cap_alloc: Disable capacity based allocation for a client
+ * retain_on_pc: If this bit is set and client has maitained active
vote
+ *   then the ways assigned to this client are not
flushed on power
+ *   collapse.
+ * activate_on_init: Activate the slice immidiately after

Re: [PATCH v3 2/2] drivers: soc: Add LLCC driver

[Channa]

On 2018-03-29 01:08, Stanimir Varbanov wrote:

Hi,

On 03/27/2018 09:52 PM, Rishabh Bhatnagar wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice getting its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
 slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API interfaces for the clients to perform these operations.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/soc/qcom/Kconfig   |  16 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c | 120 ++
 drivers/soc/qcom/llcc-slice.c  | 454 
+


I'd name it just llcc.c, slice suffix didn't add any value.


 include/linux/soc/qcom/llcc-qcom.h | 178 +++
 5 files changed, 770 insertions(+)
 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e050eb8..28237fc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -21,6 +21,22 @@ config QCOM_GSBI
   functions for connecting the underlying serial UART, SPI, 
and I2C

   devices to the output pins.

+config QCOM_LLCC
+   tristate "Qualcomm Technologies, Inc. LLCC driver"
+   depends on ARCH_QCOM
+   help
+ Qualcomm Technologies, Inc. platform specific LLCC driver for Last
+	  Level Cache. This provides interfaces to client's that use the 
LLCC.

+ Say yes here to enable LLCC slice driver.
+
+config QCOM_SDM845_LLCC
+   tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
+   depends on QCOM_LLCC
+   help
+	  Say yes here to enable the LLCC driver for SDM845. This is 
provides
+	  data required to configure LLCC so that clients can start using 
the

+ LLCC slices.
+
 config QCOM_MDT_LOADER
tristate
select QCOM_SCM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index dcebf28..e16d6a2 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,3 +12,5 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)   += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
+obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
diff --git a/drivers/soc/qcom/llcc-sdm845.c 
b/drivers/soc/qcom/llcc-sdm845.c

new file mode 100644
index 000..cd431d9
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,120 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ * This program is free software; you can redistribute it and/or 
modify

+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+/*
+ * SCT entry contains of the following parameters
+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case
+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for 
replacement

+ * fixed_size: Determine of the slice has a fixed capacity
+ * bonus_ways: Bonus ways to be used by any slice, bonus way is used 
only if

+ * it't not a reserved way.
+ * res_ways: Reserved ways for the cache slice, the reserved ways 
cannot be used

+ *   by any other client than the one its assigned to.
+ * cache_mode: Each slice operates as a cache, this controls the mode 
of the

+ * slice normal or TCM
+ * probe_target_ways: Determines what ways to probe for access hit. 
When
+ *configured to 1 only bonus and reseved ways are 
probed.
+ *when configured to 0 all ways in llcc are 
probed.

+ * dis_cap_alloc: Disable capacity based allocation for a client
+ * retain_on_pc: If this bit is set and client has maitained active 
vote
+ *   then the ways assigned to this client are not 
flushed on power

+ *   collapse.
+ * activate_on_init: Activate the slice immidiately after the SCT is 
programmed

+ */
+#define SCT_ENTRY(n, uid, sid, mc, p, fs, bway, rway, cmod, ptw, 

Re: [PATCH v3 2/2] drivers: soc: Add LLCC driver

[Channa]

On 2018-03-29 01:08, Stanimir Varbanov wrote:

Hi,

On 03/27/2018 09:52 PM, Rishabh Bhatnagar wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice getting its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
 slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API interfaces for the clients to perform these operations.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/soc/qcom/Kconfig   |  16 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c | 120 ++
 drivers/soc/qcom/llcc-slice.c  | 454 
+


I'd name it just llcc.c, slice suffix didn't add any value.


 include/linux/soc/qcom/llcc-qcom.h | 178 +++
 5 files changed, 770 insertions(+)
 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e050eb8..28237fc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -21,6 +21,22 @@ config QCOM_GSBI
   functions for connecting the underlying serial UART, SPI, 
and I2C

   devices to the output pins.

+config QCOM_LLCC
+   tristate "Qualcomm Technologies, Inc. LLCC driver"
+   depends on ARCH_QCOM
+   help
+ Qualcomm Technologies, Inc. platform specific LLCC driver for Last
+	  Level Cache. This provides interfaces to client's that use the 
LLCC.

+ Say yes here to enable LLCC slice driver.
+
+config QCOM_SDM845_LLCC
+   tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
+   depends on QCOM_LLCC
+   help
+	  Say yes here to enable the LLCC driver for SDM845. This is 
provides
+	  data required to configure LLCC so that clients can start using 
the

+ LLCC slices.
+
 config QCOM_MDT_LOADER
tristate
select QCOM_SCM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index dcebf28..e16d6a2 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,3 +12,5 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)   += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
+obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
diff --git a/drivers/soc/qcom/llcc-sdm845.c 
b/drivers/soc/qcom/llcc-sdm845.c

new file mode 100644
index 000..cd431d9
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,120 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ * This program is free software; you can redistribute it and/or 
modify

+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+/*
+ * SCT entry contains of the following parameters
+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case
+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for 
replacement

+ * fixed_size: Determine of the slice has a fixed capacity
+ * bonus_ways: Bonus ways to be used by any slice, bonus way is used 
only if

+ * it't not a reserved way.
+ * res_ways: Reserved ways for the cache slice, the reserved ways 
cannot be used

+ *   by any other client than the one its assigned to.
+ * cache_mode: Each slice operates as a cache, this controls the mode 
of the

+ * slice normal or TCM
+ * probe_target_ways: Determines what ways to probe for access hit. 
When
+ *configured to 1 only bonus and reseved ways are 
probed.
+ *when configured to 0 all ways in llcc are 
probed.

+ * dis_cap_alloc: Disable capacity based allocation for a client
+ * retain_on_pc: If this bit is set and client has maitained active 
vote
+ *   then the ways assigned to this client are not 
flushed on power

+ *   collapse.
+ * activate_on_init: Activate the slice immidiately after the SCT is 
programmed

+ */
+#define SCT_ENTRY(n, uid, sid, mc, p, fs, bway, rway, cmod, ptw, dca, 
rp, a) \

+   {

Re: [PATCH 2/2] drivers: soc: Add LLCC driver

[Channa]

On 2018-03-19 07:55, Jordan Crouse wrote:

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

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice gets its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
slice for their usecase and activate or deactivate the slice as 
needed.

LLCC driver provides API interfaces for the clients to perform these
operations.





+/**
+ * llcc_slice_deactivate - Deactivate the llcc slice
+ * @desc: Pointer to llcc slice descriptor
+ *
+ * A value zero will be returned on success and a negative errno will
+ * be returned in error cases
+ */
+int llcc_slice_deactivate(struct llcc_slice_desc *desc)
+{
+   u32 act_ctrl_val;
+   int rc = -EINVAL;
+   struct llcc_drv_data *drv;
+
+   if (desc == NULL) {
+   pr_err("Input descriptor supplied is invalid\n");


Sorry that this is out of the blue, but I was reviewing a client driver 
that
uses this API. This should not print an error - we should be allowed to 
safely

pass a null pointer from an aborted sequence in the driver without the
conditional checks and it shouldn't generate a bit of log spam as it 
goes about

it's business.


Thanks Jordan. I will incorporate this change in my next patchset.



Jordan


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

a Linux Foundation Collaborative Project

Re: [PATCH 2/2] drivers: soc: Add LLCC driver

[Channa]

On 2018-03-19 07:55, Jordan Crouse wrote:

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

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice gets its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
slice for their usecase and activate or deactivate the slice as 
needed.

LLCC driver provides API interfaces for the clients to perform these
operations.





+/**
+ * llcc_slice_deactivate - Deactivate the llcc slice
+ * @desc: Pointer to llcc slice descriptor
+ *
+ * A value zero will be returned on success and a negative errno will
+ * be returned in error cases
+ */
+int llcc_slice_deactivate(struct llcc_slice_desc *desc)
+{
+   u32 act_ctrl_val;
+   int rc = -EINVAL;
+   struct llcc_drv_data *drv;
+
+   if (desc == NULL) {
+   pr_err("Input descriptor supplied is invalid\n");


Sorry that this is out of the blue, but I was reviewing a client driver 
that
uses this API. This should not print an error - we should be allowed to 
safely

pass a null pointer from an aborted sequence in the driver without the
conditional checks and it shouldn't generate a bit of log spam as it 
goes about

it's business.


Thanks Jordan. I will incorporate this change in my next patchset.



Jordan


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

a Linux Foundation Collaborative Project

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

[Channa]

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: 
> > > > +   Definition: Offsets of llcc banks from llcc base address starting
> > > > from
> > > > +   LLCC bank0.
> > > > +
> > > > +- llcc-broadcast-off:
> > > > +   Usage: required
> > > > +   Value Type: 
> > > > +   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@130 {
> > > > +   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: 
> > > > +   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

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

[Channa]

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: 
> > > > +   Definition: Offsets of llcc banks from llcc base address starting
> > > > from
> > > > +   LLCC bank0.
> > > > +
> > > > +- llcc-broadcast-off:
> > > > +   Usage: required
> > > > +   Value Type: 
> > > > +   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@130 {
> > > > +   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: 
> > > > +   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

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

[Channa]

On 2018-02-08 08:52, Matt Sealey wrote:

Hiya,

On 25 January 2018 at 17:55, Channagoud Kadabi  
wrote:

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


[snippety snip]


+- llcc-bank-off:
+   Usage: required
+   Value Type: 
+   Definition: Offsets of llcc banks from llcc base address 
starting from

+   LLCC bank0.
+
+- llcc-broadcast-off:
+   Usage: required
+   Value Type: 
+   Definition: Offset of broadcast register from LLCC bank0 
address.


What's with the redundant namespacing?

Have we not, as a community, realised that we do not need to namespace
properties which are only present under
a single binding or node, or even those that aren't? This mess started
with the regulator bindings and it's never
stopped. What are we at now, 25 years of device trees, 10 years of FDT 
on Arm?


Notwithstanding the complete waste of rodata in the kernel image for
matching (& increased time to compare), why
wouldn't you consider why "bank-offset" for your node be any different
than a common property for any other node?


I will clean up the name space and use bank-offset for the property 
name.




And if you need to describe register offsets... why aren't you able to
use the reg property?


Reg property did not suit well for my need, so I choose to maintain 
offsets instead.


The registers in the HW block are organized as
(offset1)(offset2) (offset3) (offset4)
Base(Block0) -- Block1 -- Block 2 -- Block 3 -- Broadcast_Block

Each block has identical register mapping. You can think of it as 4 
instances of identical HW.
Broadcast block is to simplify writes, you don't need to write to 
individual blocks instead write to broadcast block.


I use simple-mfd/syscon as the hardware has multiple functions.
Doing regmap on the the Base (Block0) and maintaining offset makes the 
driver cleaner rather than using the reg property for

each instance.



Ta,
Matt


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

a Linux Foundation Collaborative Project

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

[Channa]

On 2018-02-08 08:52, Matt Sealey wrote:

Hiya,

On 25 January 2018 at 17:55, Channagoud Kadabi  
wrote:

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


[snippety snip]


+- llcc-bank-off:
+   Usage: required
+   Value Type: 
+   Definition: Offsets of llcc banks from llcc base address 
starting from

+   LLCC bank0.
+
+- llcc-broadcast-off:
+   Usage: required
+   Value Type: 
+   Definition: Offset of broadcast register from LLCC bank0 
address.


What's with the redundant namespacing?

Have we not, as a community, realised that we do not need to namespace
properties which are only present under
a single binding or node, or even those that aren't? This mess started
with the regulator bindings and it's never
stopped. What are we at now, 25 years of device trees, 10 years of FDT 
on Arm?


Notwithstanding the complete waste of rodata in the kernel image for
matching (& increased time to compare), why
wouldn't you consider why "bank-offset" for your node be any different
than a common property for any other node?


I will clean up the name space and use bank-offset for the property 
name.




And if you need to describe register offsets... why aren't you able to
use the reg property?


Reg property did not suit well for my need, so I choose to maintain 
offsets instead.


The registers in the HW block are organized as
(offset1)(offset2) (offset3) (offset4)
Base(Block0) -- Block1 -- Block 2 -- Block 3 -- Broadcast_Block

Each block has identical register mapping. You can think of it as 4 
instances of identical HW.
Broadcast block is to simplify writes, you don't need to write to 
individual blocks instead write to broadcast block.


I use simple-mfd/syscon as the hardware has multiple functions.
Doing regmap on the the Base (Block0) and maintaining offset makes the 
driver cleaner rather than using the reg property for

each instance.



Ta,
Matt


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

a Linux Foundation Collaborative Project

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

[Channa]

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:
> > Documentation for last level cache controller device tree bindings,
> > client bindings usage examples.
> >
> > Signed-off-by: Channagoud Kadabi <ckad...@codeaurora.org>
> > ---
> >  .../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 000..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: 
> > + Definition: must be "qcom,llcc-core"
> > +
> > +- reg:
> > + Usage: required
> > + Value Type: 
> > + Definition: must be addresses and sizes of the LLCC registers
> > +
> > +- llcc-bank-off:
> > + Usage: required
> > + Value Type: 
> > + Definition: Offsets of llcc banks from llcc base address starting
> > from
> > + LLCC bank0.
> > +
> > +- llcc-broadcast-off:
> > + Usage: required
> > + Value Type: 
> > + 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?





> > +
> > +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@130 {
> > + 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.



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 = <0x130 0x5>;
> > + 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. 

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

[Channa]

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:
> > Documentation for last level cache controller device tree bindings,
> > client bindings usage examples.
> >
> > Signed-off-by: Channagoud Kadabi 
> > ---
> >  .../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 000..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: 
> > + Definition: must be "qcom,llcc-core"
> > +
> > +- reg:
> > + Usage: required
> > + Value Type: 
> > + Definition: must be addresses and sizes of the LLCC registers
> > +
> > +- llcc-bank-off:
> > + Usage: required
> > + Value Type: 
> > + Definition: Offsets of llcc banks from llcc base address starting
> > from
> > + LLCC bank0.
> > +
> > +- llcc-broadcast-off:
> > + Usage: required
> > + Value Type: 
> > + 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?





> > +
> > +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@130 {
> > + 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.



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 = <0x130 0x5>;
> > + 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 

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

[Channa]

On 2018-02-01 02:48, 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 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 93 
++

 1 file changed, 93 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


+Example:
+
+   qcom,system-cache@130 {
+   compatible = "qcom,llcc-core", "syscon", "simple-mfd";
+   reg = <0x130 0x5>;
+   reg-names = "llcc_base";
+
+   llcc: qcom,sdm845-llcc {
+   compatible = "qcom,sdm845-llcc";
+   #cache-cells = <1>;
+   max-slices = <32>;
+   };
+
+   qcom,llcc-ecc {
+   compatible = "qcom,llcc-ecc";
+   };
+
+   qcom,llcc-amon {
+   compatible = "qcom,llcc-amon";
+   qcom,fg-cnt = <0x7>;
+   };
+
+   };


The "qcom,llcc-ecc" and "qcom,llcc-amon" bindings doesn't seem to be
used by the driver in patch 2, and it's not clear how they are intended
to be used, so I think they should go from the binding for now.


Sure I can remove them for now and add them when the I push other 
drivers

for review.



I don't think you need syscon and simple-mfd, and I think you can


I used syscon and simple-mfd because three drivers touch the same 
address space.


Driver 1:
system cache core: The purpose of the driver is to partition the system 
cache
and program the settings such as priority, lines to probe while doing a 
look up

in the system cache, low power related settings etc.
The driver also provides API for clients to query the cache slice 
details,

activate and deactivate them.

Driver 2:
ECC to detect single and double bit errors in LLCC memory.
Implemented using EDAC framework.

Driver 3:
AMON: Activity Monitor to detect live lock ups in the HW block.
Implemented as SOC driver similar to driver #1.

Since the hardware block has multiple functional units the driver is 
implemented

to use syscon/regmap interface.


simplify the binding to a single node like:

qcom,system-cache@130 {
compatible = "qcom,sdm845-llcc";
reg = <0x130 0x5>;
#cache-slice-cells = <1>;
max-slices = <32>;
}

If ECC and AMON are option features, we can always add boolean
properties for those later, e.g. "has-ecc".

Thanks,
Mark.


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

a Linux Foundation Collaborative Project

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

[Channa]

On 2018-02-01 02:48, 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 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 93 
++

 1 file changed, 93 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


+Example:
+
+   qcom,system-cache@130 {
+   compatible = "qcom,llcc-core", "syscon", "simple-mfd";
+   reg = <0x130 0x5>;
+   reg-names = "llcc_base";
+
+   llcc: qcom,sdm845-llcc {
+   compatible = "qcom,sdm845-llcc";
+   #cache-cells = <1>;
+   max-slices = <32>;
+   };
+
+   qcom,llcc-ecc {
+   compatible = "qcom,llcc-ecc";
+   };
+
+   qcom,llcc-amon {
+   compatible = "qcom,llcc-amon";
+   qcom,fg-cnt = <0x7>;
+   };
+
+   };


The "qcom,llcc-ecc" and "qcom,llcc-amon" bindings doesn't seem to be
used by the driver in patch 2, and it's not clear how they are intended
to be used, so I think they should go from the binding for now.


Sure I can remove them for now and add them when the I push other 
drivers

for review.



I don't think you need syscon and simple-mfd, and I think you can


I used syscon and simple-mfd because three drivers touch the same 
address space.


Driver 1:
system cache core: The purpose of the driver is to partition the system 
cache
and program the settings such as priority, lines to probe while doing a 
look up

in the system cache, low power related settings etc.
The driver also provides API for clients to query the cache slice 
details,

activate and deactivate them.

Driver 2:
ECC to detect single and double bit errors in LLCC memory.
Implemented using EDAC framework.

Driver 3:
AMON: Activity Monitor to detect live lock ups in the HW block.
Implemented as SOC driver similar to driver #1.

Since the hardware block has multiple functional units the driver is 
implemented

to use syscon/regmap interface.


simplify the binding to a single node like:

qcom,system-cache@130 {
compatible = "qcom,sdm845-llcc";
reg = <0x130 0x5>;
#cache-slice-cells = <1>;
max-slices = <32>;
}

If ECC and AMON are option features, we can always add boolean
properties for those later, e.g. "has-ecc".

Thanks,
Mark.


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

a Linux Foundation Collaborative Project

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

[Channa]

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 
---
 .../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 000..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: 
+   Definition: must be "qcom,llcc-core"
+
+- reg:
+   Usage: required
+   Value Type: 
+   Definition: must be addresses and sizes of the LLCC registers
+
+- llcc-bank-off:
+   Usage: required
+   Value Type: 
+	Definition: Offsets of llcc banks from llcc base address starting 
from

+   LLCC bank0.
+
+- llcc-broadcast-off:
+   Usage: required
+   Value Type: 
+   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.





+
+- #cache-cells:
+   Usage: required
+   Value Type: 
+   Definition: Number of cache cells, must be 1


What's this for, and how is it used?


This is to obtain the phandle arguments from client devices. Related to 
cache-slices property.





+
+- max-slices:
+   usage: required
+   Value Type: 
+   Definition: Number of cache slices supported by hardware
+
+- status:
+   Usage: optional
+   Value type: 
+   Definition: Property to enable or disable the driver


This is a standard property, so I don't think it needs to be described
here.


Sure, will remove it.




+
+== llcc amon device ==
+
+Properties:
+-qcom,fg-cnt : The value of fine grained counter of activity monitor
+   block.


Could you elaborate on this?


This is counter value programmed in the HW to detect live locks.
This parameter is tunable to avoid false positives.




+
+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@130 {
+   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.




+   reg = <0x130 0x5>;
+   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


+   };
+
+== Client ==
+
+Properties:
+- cache-slice-names:
+   Usage: required
+   Value type: 
+	Definition: A set of names that identify the usecase names of a 
client that uses

+   cache slice. These strings are used to look up the cache 
slice
+   entries by name.
+
+- cache-slices:
+   Usage: required
+   Value type: 
+	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




Is this meant to align with #cache-cells? It would be best to keep a
common prefix (i.e. call that #cache-slice-cells).


Yes. Will update the name.



Thanks,
Mark.


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

a Linux Foundation 

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

[Channa]

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 
---
 .../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 000..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: 
+   Definition: must be "qcom,llcc-core"
+
+- reg:
+   Usage: required
+   Value Type: 
+   Definition: must be addresses and sizes of the LLCC registers
+
+- llcc-bank-off:
+   Usage: required
+   Value Type: 
+	Definition: Offsets of llcc banks from llcc base address starting 
from

+   LLCC bank0.
+
+- llcc-broadcast-off:
+   Usage: required
+   Value Type: 
+   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.





+
+- #cache-cells:
+   Usage: required
+   Value Type: 
+   Definition: Number of cache cells, must be 1


What's this for, and how is it used?


This is to obtain the phandle arguments from client devices. Related to 
cache-slices property.





+
+- max-slices:
+   usage: required
+   Value Type: 
+   Definition: Number of cache slices supported by hardware
+
+- status:
+   Usage: optional
+   Value type: 
+   Definition: Property to enable or disable the driver


This is a standard property, so I don't think it needs to be described
here.


Sure, will remove it.




+
+== llcc amon device ==
+
+Properties:
+-qcom,fg-cnt : The value of fine grained counter of activity monitor
+   block.


Could you elaborate on this?


This is counter value programmed in the HW to detect live locks.
This parameter is tunable to avoid false positives.




+
+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@130 {
+   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.




+   reg = <0x130 0x5>;
+   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


+   };
+
+== Client ==
+
+Properties:
+- cache-slice-names:
+   Usage: required
+   Value type: 
+	Definition: A set of names that identify the usecase names of a 
client that uses

+   cache slice. These strings are used to look up the cache 
slice
+   entries by name.
+
+- cache-slices:
+   Usage: required
+   Value type: 
+	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




Is this meant to align with #cache-cells? It would be best to keep a
common prefix (i.e. call that #cache-slice-cells).


Yes. Will update the name.



Thanks,
Mark.


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

a Linux Foundation Collaborative Project

Re: [PATCH 3/3] arm64: Add work around for Arm Cortex-A55 Erratum 1024718

[Channa]

On 2018-01-17 02:28, Suzuki K Poulose wrote:

On 17/01/18 03:34, ckad...@codeaurora.org wrote:

On 2018-01-16 02:23, Suzuki K Poulose wrote:

Some variants of the Arm Cortex-55 cores (r0p0, r0p1, r1p0) suffer
from an erratum 1024718, which causes incorrect updates when DBM/AP
bits in a page table entry is modified without a break-before-make
sequence. The work around is to disable the hardware DBM feature
on the affected cores. The hardware Access Flag management features
is not affected.

The hardware DBM feature is a non-conflicting capability, i.e, the
kernel could handle cores using the feature and those without having
the features running at the same time. So this work around is 
detected
at early boot time, rather than delaying it until the CPUs are 
brought
up into the kernel with MMU turned on. This also avoids other 
complexities
with late CPUs turning online, with or without the hardware DBM 
features.


Cc: Catalin Marinas 
Cc: Mark Rutland 
Cc: Will Deacon 
Signed-off-by: Suzuki K Poulose 
---
 Documentation/arm64/silicon-errata.txt |  1 +
 arch/arm64/Kconfig | 14 ++
 arch/arm64/mm/proc.S   |  5 +
 3 files changed, 20 insertions(+)

diff --git a/Documentation/arm64/silicon-errata.txt
b/Documentation/arm64/silicon-errata.txt
index b9d93e981a05..5203e71c113d 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -55,6 +55,7 @@ stable kernels.
 | ARM    | Cortex-A57  | #834220 |
ARM64_ERRATUM_834220    |
 | ARM    | Cortex-A72  | #853709 | N/A
 |
 | ARM    | Cortex-A73  | #858921 |
ARM64_ERRATUM_858921    |
+| ARM    | Cortex-A55  | #1024718    |
ARM64_ERRATUM_1024718   |
 | ARM    | MMU-500 | #841119,#826419 | N/A
 |
 |    | | |
 |
 | Cavium | ThunderX ITS    | #22375, #24313  |
CAVIUM_ERRATUM_22375    |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 664fadc2aa2e..19b8407a0325 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -461,6 +461,20 @@ config ARM64_ERRATUM_843419

   If unsure, say Y.

+config ARM64_ERRATUM_1024718
+    bool "Cortex-A55: 1024718: Update of DBM/AP bits without break
before make might result in incorrect update"
+    default y
+    help
+  This option adds work around for Arm Cortex-A55 Erratum 
1024718.

+
+  Affected Cortex-A55 cores (r0p0, r0p1, r1p0) could cause 
incorrect
+  update of the hardware dirty bit when the DBM/AP bits are 
updated
+  without a break-before-make. The work around is to disable the 
usage
+  of hardware DBM locally on the affected cores. CPUs not 
affected by

+  erratum will continue to use the feature.
+
+  If unsure, say Y.
+
 config CAVIUM_ERRATUM_22375
 bool "Cavium erratum 22375, 24313"
 default y
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 5a59eea49395..ba2c22180f4e 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -252,6 +252,11 @@ ENTRY(__cpu_setup)
 cbz    x9, 2f
 cmp    x9, #2
 b.lt    1f
+#ifdef CONFIG_ARM64_ERRATUM_1024718
+    /* Disable hardware DBM on Cortex-A55 r0p0, r0p1 & r1p0 */
+    cpu_midr_match MIDR_CORTEX_A55, MIDR_CPU_VAR_REV(0, 0),


What is there is a custom core with different MIDRs, can we specify 
multiple MIDR values?


At the moment no. May be we could pass a table of such values to the 
macro ?


Would it be good to clear the bit as part of 
arch/arm64/kernel/cpu_errata.c so we can specify multiple MIDR values 
if required.


The problem is, we already have some part of the kernel mappings with
PTE_DBM set
(PTE_WRITE = PTE_DBM with CONFIG_HW_AFDBM) and could potentially hit 
the errata,

before we disable it on the CPU. Also, if the CPU is brought up late
by userspace,
that adds more entities. I had another approach, where we delay 
enabling the
TCR_HD until all cores are up. But then it has other complexities with 
the CPU

feature framework.
e.g, we can't use the feature unless we turn the HADBS feature bit to
HIGHER_SAFE
so that we can turn it on if at least one CPU has it. But then, we 
don't know
what the future values of the feature could imply, leaving that choice 
unsafe.
Also, a late CPU will be prevented from booting if it doesn't have DBM 
unless

we hack the framework.


I was thinking if we can enable the DBM feature based on a cpu feature 
register.
Not sure if all future CPUs would have a bit for identifying whether DBM 
is supported

or not.



So an early check seemed the easier solution at the moment. I will take 
a look
at changing the framework a little bit and see where it takes us. 
Otherwise,

we could switch back to a table of affected MIDRs.


Agree, its better to change the implementation to take a 

Re: [PATCH 3/3] arm64: Add work around for Arm Cortex-A55 Erratum 1024718

[Channa]

On 2018-01-17 02:28, Suzuki K Poulose wrote:

On 17/01/18 03:34, ckad...@codeaurora.org wrote:

On 2018-01-16 02:23, Suzuki K Poulose wrote:

Some variants of the Arm Cortex-55 cores (r0p0, r0p1, r1p0) suffer
from an erratum 1024718, which causes incorrect updates when DBM/AP
bits in a page table entry is modified without a break-before-make
sequence. The work around is to disable the hardware DBM feature
on the affected cores. The hardware Access Flag management features
is not affected.

The hardware DBM feature is a non-conflicting capability, i.e, the
kernel could handle cores using the feature and those without having
the features running at the same time. So this work around is 
detected
at early boot time, rather than delaying it until the CPUs are 
brought
up into the kernel with MMU turned on. This also avoids other 
complexities
with late CPUs turning online, with or without the hardware DBM 
features.


Cc: Catalin Marinas 
Cc: Mark Rutland 
Cc: Will Deacon 
Signed-off-by: Suzuki K Poulose 
---
 Documentation/arm64/silicon-errata.txt |  1 +
 arch/arm64/Kconfig | 14 ++
 arch/arm64/mm/proc.S   |  5 +
 3 files changed, 20 insertions(+)

diff --git a/Documentation/arm64/silicon-errata.txt
b/Documentation/arm64/silicon-errata.txt
index b9d93e981a05..5203e71c113d 100644
--- a/Documentation/arm64/silicon-errata.txt
+++ b/Documentation/arm64/silicon-errata.txt
@@ -55,6 +55,7 @@ stable kernels.
 | ARM    | Cortex-A57  | #834220 |
ARM64_ERRATUM_834220    |
 | ARM    | Cortex-A72  | #853709 | N/A
 |
 | ARM    | Cortex-A73  | #858921 |
ARM64_ERRATUM_858921    |
+| ARM    | Cortex-A55  | #1024718    |
ARM64_ERRATUM_1024718   |
 | ARM    | MMU-500 | #841119,#826419 | N/A
 |
 |    | | |
 |
 | Cavium | ThunderX ITS    | #22375, #24313  |
CAVIUM_ERRATUM_22375    |
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 664fadc2aa2e..19b8407a0325 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -461,6 +461,20 @@ config ARM64_ERRATUM_843419

   If unsure, say Y.

+config ARM64_ERRATUM_1024718
+    bool "Cortex-A55: 1024718: Update of DBM/AP bits without break
before make might result in incorrect update"
+    default y
+    help
+  This option adds work around for Arm Cortex-A55 Erratum 
1024718.

+
+  Affected Cortex-A55 cores (r0p0, r0p1, r1p0) could cause 
incorrect
+  update of the hardware dirty bit when the DBM/AP bits are 
updated
+  without a break-before-make. The work around is to disable the 
usage
+  of hardware DBM locally on the affected cores. CPUs not 
affected by

+  erratum will continue to use the feature.
+
+  If unsure, say Y.
+
 config CAVIUM_ERRATUM_22375
 bool "Cavium erratum 22375, 24313"
 default y
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 5a59eea49395..ba2c22180f4e 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -252,6 +252,11 @@ ENTRY(__cpu_setup)
 cbz    x9, 2f
 cmp    x9, #2
 b.lt    1f
+#ifdef CONFIG_ARM64_ERRATUM_1024718
+    /* Disable hardware DBM on Cortex-A55 r0p0, r0p1 & r1p0 */
+    cpu_midr_match MIDR_CORTEX_A55, MIDR_CPU_VAR_REV(0, 0),


What is there is a custom core with different MIDRs, can we specify 
multiple MIDR values?


At the moment no. May be we could pass a table of such values to the 
macro ?


Would it be good to clear the bit as part of 
arch/arm64/kernel/cpu_errata.c so we can specify multiple MIDR values 
if required.


The problem is, we already have some part of the kernel mappings with
PTE_DBM set
(PTE_WRITE = PTE_DBM with CONFIG_HW_AFDBM) and could potentially hit 
the errata,

before we disable it on the CPU. Also, if the CPU is brought up late
by userspace,
that adds more entities. I had another approach, where we delay 
enabling the
TCR_HD until all cores are up. But then it has other complexities with 
the CPU

feature framework.
e.g, we can't use the feature unless we turn the HADBS feature bit to
HIGHER_SAFE
so that we can turn it on if at least one CPU has it. But then, we 
don't know
what the future values of the feature could imply, leaving that choice 
unsafe.
Also, a late CPU will be prevented from booting if it doesn't have DBM 
unless

we hack the framework.


I was thinking if we can enable the DBM feature based on a cpu feature 
register.
Not sure if all future CPUs would have a bit for identifying whether DBM 
is supported

or not.



So an early check seemed the easier solution at the moment. I will take 
a look
at changing the framework a little bit and see where it takes us. 
Otherwise,

we could switch back to a table of affected MIDRs.


Agree, its better to change the implementation to take a table of MIDRs.



Suzuki


--
--
The Qualcomm Innovation Center, Inc. is a member of the Code