[PATCH 1/2] soc: qcom: llcc: Rename llcc-sdm845 to llcc-plat

[PATCH 1/2] soc: qcom: llcc: Rename llcc-sdm845 to llcc-plat

[Vivek Gautam]

To avoid adding files for each future supported SoCs rename
the file to a generic name - llcc-plat, so that llcc configuration
tables for other SoCs can be added in the same driver.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/soc/qcom/Kconfig                        | 10 +++++-----
 drivers/soc/qcom/Makefile                       |  2 +-
 drivers/soc/qcom/{llcc-sdm845.c => llcc-plat.c} |  0
 3 files changed, 6 insertions(+), 6 deletions(-)
 rename drivers/soc/qcom/{llcc-sdm845.c => llcc-plat.c} (100%)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a6d1bfb17279..8110d415b18e 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -62,13 +62,13 @@ config QCOM_LLCC
 	  to clients that use the LLCC. Say yes here to enable LLCC slice
 	  driver.
 
-config QCOM_SDM845_LLCC
-	tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
+config QCOM_PLAT_LLCC
+	tristate "Qualcomm Technologies, Inc. platform LLCC driver"
 	depends on QCOM_LLCC
 	help
-	  Say yes here to enable the LLCC driver for SDM845. This provides
-	  data required to configure LLCC so that clients can start using the
-	  LLCC slices.
+	  Say yes here to enable the LLCC driver for Qcom platforms, such as
+	  SDM845. This provides data required to configure LLCC so that
+	  clients can start using the LLCC slices.
 
 config QCOM_MDT_LOADER
 	tristate
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index eeb088beb15f..3bf26667d7ee 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -21,6 +21,6 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
 obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
-obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
+obj-$(CONFIG_QCOM_PLAT_LLCC) += llcc-plat.o
 obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
 obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
diff --git a/drivers/soc/qcom/llcc-sdm845.c b/drivers/soc/qcom/llcc-plat.c
similarity index 100%
rename from drivers/soc/qcom/llcc-sdm845.c
rename to drivers/soc/qcom/llcc-plat.c
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

[PATCH 2/2] soc: qcom: llcc-plat: Make the driver more generic

[Vivek Gautam]

- Remove 'sdm845' from names, and use 'plat' instead.
- Move SCT_ENTRY macro to header file.
- Create a new config structure to asssign to of-match-data.

Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
---
 drivers/soc/qcom/llcc-plat.c       | 77 ++++++++++++--------------------------
 include/linux/soc/qcom/llcc-qcom.h | 45 ++++++++++++++++++++++
 2 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/drivers/soc/qcom/llcc-plat.c b/drivers/soc/qcom/llcc-plat.c
index 86600d97c36d..31cff0f75b53 100644
--- a/drivers/soc/qcom/llcc-plat.c
+++ b/drivers/soc/qcom/llcc-plat.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved.
  *
  */
 
@@ -10,47 +10,7 @@
 #include <linux/of_device.h>
 #include <linux/soc/qcom/llcc-qcom.h>
 
-/*
- * SCT(System Cache Table) entry contains of the following members:
- * usecase_id: 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: Boolean indicating if the slice has a fixed capacity
- * bonus_ways: Bonus ways are additional ways to be used for any slice,
- *		if client ends up using more than reserved cache ways. Bonus
- *		ways are allocated only if they are not reserved for some
- *		other client.
- * 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(Tightly Coupled Memory)
- * probe_target_ways: Determines what ways to probe for access hit. When
- *                    configured to 1 only bonus and reserved 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 maintained active vote
- *               then the ways assigned to this client are not flushed on power
- *               collapse.
- * activate_on_init: Activate the slice immediately after the SCT is programmed
- */
-#define SCT_ENTRY(uid, sid, mc, p, fs, bway, rway, cmod, ptw, dca, rp, a) \
-	{					\
-		.usecase_id = uid,		\
-		.slice_id = sid,		\
-		.max_cap = mc,			\
-		.priority = p,			\
-		.fixed_size = fs,		\
-		.bonus_ways = bway,		\
-		.res_ways = rway,		\
-		.cache_mode = cmod,		\
-		.probe_target_ways = ptw,	\
-		.dis_cap_alloc = dca,		\
-		.retain_on_pc = rp,		\
-		.activate_on_init = a,		\
-	}
-
-static struct llcc_slice_config sdm845_data[] =  {
+static const struct llcc_slice_config sdm845_data[] =  {
 	SCT_ENTRY(LLCC_CPUSS,    1,  2816, 1, 0, 0xffc, 0x2,   0, 0, 1, 1, 1),
 	SCT_ENTRY(LLCC_VIDSC0,   2,  512,  2, 1, 0x0,   0x0f0, 0, 0, 1, 1, 0),
 	SCT_ENTRY(LLCC_VIDSC1,   3,  512,  2, 1, 0x0,   0x0f0, 0, 0, 1, 1, 0),
@@ -71,30 +31,39 @@ static struct llcc_slice_config sdm845_data[] =  {
 	SCT_ENTRY(LLCC_AUDHW,    22, 1024, 1, 1, 0xffc, 0x2,   0, 0, 1, 1, 0),
 };
 
-static int sdm845_qcom_llcc_remove(struct platform_device *pdev)
+static const struct qcom_llcc_config sdm845_cfg = {
+	.sct_data	= sdm845_data,
+	.size		= ARRAY_SIZE(sdm845_data),
+};
+
+static int qcom_plat_llcc_remove(struct platform_device *pdev)
 {
 	return qcom_llcc_remove(pdev);
 }
 
-static int sdm845_qcom_llcc_probe(struct platform_device *pdev)
+static int qcom_plat_llcc_probe(struct platform_device *pdev)
 {
-	return qcom_llcc_probe(pdev, sdm845_data, ARRAY_SIZE(sdm845_data));
+	const struct qcom_llcc_config *cfg;
+
+	cfg = of_device_get_match_data(&pdev->dev);
+
+	return qcom_llcc_probe(pdev, cfg->sct_data, cfg->size);
 }
 
-static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
-	{ .compatible = "qcom,sdm845-llcc", },
+static const struct of_device_id qcom_plat_llcc_of_match[] = {
+	{ .compatible = "qcom,sdm845-llcc", .data = &sdm845_cfg },
 	{ }
 };
 
-static struct platform_driver sdm845_qcom_llcc_driver = {
+static struct platform_driver qcom_plat_llcc_driver = {
 	.driver = {
-		.name = "sdm845-llcc",
-		.of_match_table = sdm845_qcom_llcc_of_match,
+		.name = "qcom-plat-llcc",
+		.of_match_table = qcom_plat_llcc_of_match,
 	},
-	.probe = sdm845_qcom_llcc_probe,
-	.remove = sdm845_qcom_llcc_remove,
+	.probe = qcom_plat_llcc_probe,
+	.remove = qcom_plat_llcc_remove,
 };
-module_platform_driver(sdm845_qcom_llcc_driver);
+module_platform_driver(qcom_plat_llcc_driver);
 
-MODULE_DESCRIPTION("QCOM sdm845 LLCC driver");
+MODULE_DESCRIPTION("QCOM platform LLCC driver");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
index eb71a50b8afc..8776bb5d3891 100644
--- a/include/linux/soc/qcom/llcc-qcom.h
+++ b/include/linux/soc/qcom/llcc-qcom.h
@@ -27,6 +27,46 @@
 #define LLCC_MDMPNG      21
 #define LLCC_AUDHW       22
 
+/*
+ * SCT(System Cache Table) entry contains of the following members:
+ * usecase_id: 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: Boolean indicating if the slice has a fixed capacity
+ * bonus_ways: Bonus ways are additional ways to be used for any slice,
+ *		if client ends up using more than reserved cache ways. Bonus
+ *		ways are allocated only if they are not reserved for some
+ *		other client.
+ * 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(Tightly Coupled Memory)
+ * probe_target_ways: Determines what ways to probe for access hit. When
+ *                    configured to 1 only bonus and reserved 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 maintained active vote
+ *               then the ways assigned to this client are not flushed on power
+ *               collapse.
+ * activate_on_init: Activate the slice immediately after the SCT is programmed
+ */
+#define SCT_ENTRY(uid, sid, mc, p, fs, bway, rway, cmod, ptw, dca, rp, a) \
+	{					\
+		.usecase_id = uid,		\
+		.slice_id = sid,		\
+		.max_cap = mc,			\
+		.priority = p,			\
+		.fixed_size = fs,		\
+		.bonus_ways = bway,		\
+		.res_ways = rway,		\
+		.cache_mode = cmod,		\
+		.probe_target_ways = ptw,	\
+		.dis_cap_alloc = dca,		\
+		.retain_on_pc = rp,		\
+		.activate_on_init = a,		\
+	}
+
 /**
  * llcc_slice_desc - Cache slice descriptor
  * @slice_id: llcc slice id
@@ -67,6 +107,11 @@ struct llcc_slice_config {
 	bool activate_on_init;
 };
 
+struct qcom_llcc_config {
+	const struct llcc_slice_config *sct_data;
+	int size;
+};
+
 /**
  * llcc_drv_data - Data associated with the llcc driver
  * @regmap: regmap associated with the llcc device
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 1/2] soc: qcom: llcc: Rename llcc-sdm845 to llcc-plat

[Bjorn Andersson]

On Thu 11 Jul 04:03 PDT 2019, Vivek Gautam wrote:

> To avoid adding files for each future supported SoCs rename
> the file to a generic name - llcc-plat, so that llcc configuration
> tables for other SoCs can be added in the same driver.
> 

We've had a generic LLCC Kconfig option and then a specific SDM845 one,
with this change we have two different generic options and both would
either always be enabled or disabled.

So I think you should drop QCOM_SDM845_LLCC and build both llcc-slice
and llcc-plat into the same qcom_llcc.ko instead.

Regards,
Bjorn

> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig                        | 10 +++++-----
>  drivers/soc/qcom/Makefile                       |  2 +-
>  drivers/soc/qcom/{llcc-sdm845.c => llcc-plat.c} |  0
>  3 files changed, 6 insertions(+), 6 deletions(-)
>  rename drivers/soc/qcom/{llcc-sdm845.c => llcc-plat.c} (100%)
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a6d1bfb17279..8110d415b18e 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -62,13 +62,13 @@ config QCOM_LLCC
>  	  to clients that use the LLCC. Say yes here to enable LLCC slice
>  	  driver.
>  
> -config QCOM_SDM845_LLCC
> -	tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
> +config QCOM_PLAT_LLCC
> +	tristate "Qualcomm Technologies, Inc. platform LLCC driver"
>  	depends on QCOM_LLCC
>  	help
> -	  Say yes here to enable the LLCC driver for SDM845. This provides
> -	  data required to configure LLCC so that clients can start using the
> -	  LLCC slices.
> +	  Say yes here to enable the LLCC driver for Qcom platforms, such as
> +	  SDM845. This provides data required to configure LLCC so that
> +	  clients can start using the LLCC slices.
>  
>  config QCOM_MDT_LOADER
>  	tristate
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index eeb088beb15f..3bf26667d7ee 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -21,6 +21,6 @@ obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
>  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>  obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
> -obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
> +obj-$(CONFIG_QCOM_PLAT_LLCC) += llcc-plat.o
>  obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
>  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> diff --git a/drivers/soc/qcom/llcc-sdm845.c b/drivers/soc/qcom/llcc-plat.c
> similarity index 100%
> rename from drivers/soc/qcom/llcc-sdm845.c
> rename to drivers/soc/qcom/llcc-plat.c
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH 2/2] soc: qcom: llcc-plat: Make the driver more generic

[Bjorn Andersson]

On Thu 11 Jul 04:03 PDT 2019, Vivek Gautam wrote:

> - Remove 'sdm845' from names, and use 'plat' instead.
> - Move SCT_ENTRY macro to header file.
> - Create a new config structure to asssign to of-match-data.
> 

I interpret the intention of these two patches as that you want to add
some new platform without having to create one llcc-xyz.c per platform.

If that's the case then the only user of this macro would be in plat.c,
so I don't see a reason for moving it to the header file.

> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> ---
>  drivers/soc/qcom/llcc-plat.c       | 77 ++++++++++++--------------------------
>  include/linux/soc/qcom/llcc-qcom.h | 45 ++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-plat.c b/drivers/soc/qcom/llcc-plat.c
> index 86600d97c36d..31cff0f75b53 100644
> --- a/drivers/soc/qcom/llcc-plat.c
> +++ b/drivers/soc/qcom/llcc-plat.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved.
>   *
>   */
>  
> @@ -10,47 +10,7 @@
>  #include <linux/of_device.h>
>  #include <linux/soc/qcom/llcc-qcom.h>
>  
> -/*
> - * SCT(System Cache Table) entry contains of the following members:

Should have caught this during previous review, but this comment simply
duplicates the kerneldoc for struct llcc_slice_config.

> - * usecase_id: 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: Boolean indicating if the slice has a fixed capacity
> - * bonus_ways: Bonus ways are additional ways to be used for any slice,
> - *		if client ends up using more than reserved cache ways. Bonus
> - *		ways are allocated only if they are not reserved for some
> - *		other client.
> - * 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(Tightly Coupled Memory)
> - * probe_target_ways: Determines what ways to probe for access hit. When
> - *                    configured to 1 only bonus and reserved 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 maintained active vote
> - *               then the ways assigned to this client are not flushed on power
> - *               collapse.
> - * activate_on_init: Activate the slice immediately after the SCT is programmed
> - */
> -#define SCT_ENTRY(uid, sid, mc, p, fs, bway, rway, cmod, ptw, dca, rp, a) \

This simply maps macro arguments 1:1 to struct members, there's no need
for a macro for this.

> -	{					\
> -		.usecase_id = uid,		\
> -		.slice_id = sid,		\
> -		.max_cap = mc,			\
> -		.priority = p,			\
> -		.fixed_size = fs,		\
> -		.bonus_ways = bway,		\
> -		.res_ways = rway,		\
> -		.cache_mode = cmod,		\
> -		.probe_target_ways = ptw,	\
> -		.dis_cap_alloc = dca,		\
> -		.retain_on_pc = rp,		\
> -		.activate_on_init = a,		\
> -	}
> -
> -static struct llcc_slice_config sdm845_data[] =  {
> +static const struct llcc_slice_config sdm845_data[] =  {
>  	SCT_ENTRY(LLCC_CPUSS,    1,  2816, 1, 0, 0xffc, 0x2,   0, 0, 1, 1, 1),
>  	SCT_ENTRY(LLCC_VIDSC0,   2,  512,  2, 1, 0x0,   0x0f0, 0, 0, 1, 1, 0),
>  	SCT_ENTRY(LLCC_VIDSC1,   3,  512,  2, 1, 0x0,   0x0f0, 0, 0, 1, 1, 0),
> @@ -71,30 +31,39 @@ static struct llcc_slice_config sdm845_data[] =  {
>  	SCT_ENTRY(LLCC_AUDHW,    22, 1024, 1, 1, 0xffc, 0x2,   0, 0, 1, 1, 0),
>  };
>  
> -static int sdm845_qcom_llcc_remove(struct platform_device *pdev)
> +static const struct qcom_llcc_config sdm845_cfg = {
> +	.sct_data	= sdm845_data,
> +	.size		= ARRAY_SIZE(sdm845_data),
> +};
> +
> +static int qcom_plat_llcc_remove(struct platform_device *pdev)
>  {
>  	return qcom_llcc_remove(pdev);
>  }
>  
> -static int sdm845_qcom_llcc_probe(struct platform_device *pdev)
> +static int qcom_plat_llcc_probe(struct platform_device *pdev)
>  {
> -	return qcom_llcc_probe(pdev, sdm845_data, ARRAY_SIZE(sdm845_data));
> +	const struct qcom_llcc_config *cfg;
> +
> +	cfg = of_device_get_match_data(&pdev->dev);
> +
> +	return qcom_llcc_probe(pdev, cfg->sct_data, cfg->size);
>  }
>  
> -static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
> -	{ .compatible = "qcom,sdm845-llcc", },
> +static const struct of_device_id qcom_plat_llcc_of_match[] = {
> +	{ .compatible = "qcom,sdm845-llcc", .data = &sdm845_cfg },
>  	{ }
>  };
>  
> -static struct platform_driver sdm845_qcom_llcc_driver = {
> +static struct platform_driver qcom_plat_llcc_driver = {
>  	.driver = {
> -		.name = "sdm845-llcc",
> -		.of_match_table = sdm845_qcom_llcc_of_match,
> +		.name = "qcom-plat-llcc",

With this being the "one and only llcc driver", why not making it
"qcom_llcc"?

> +		.of_match_table = qcom_plat_llcc_of_match,
>  	},
> -	.probe = sdm845_qcom_llcc_probe,
> -	.remove = sdm845_qcom_llcc_remove,
> +	.probe = qcom_plat_llcc_probe,
> +	.remove = qcom_plat_llcc_remove,
>  };
> -module_platform_driver(sdm845_qcom_llcc_driver);
> +module_platform_driver(qcom_plat_llcc_driver);
>  
> -MODULE_DESCRIPTION("QCOM sdm845 LLCC driver");
> +MODULE_DESCRIPTION("QCOM platform LLCC driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h

This file should be describing the public interface to the llcc, the
private pieces is better kept in drivers/soc/qcom/llcc.h

But this patch makes me wonder if there's a need to split llcc-slice and
llcc-plat (and have a header file to describe API between them) instead
of just having one file.

Regards,
Bjorn

> index eb71a50b8afc..8776bb5d3891 100644
> --- a/include/linux/soc/qcom/llcc-qcom.h
> +++ b/include/linux/soc/qcom/llcc-qcom.h
> @@ -27,6 +27,46 @@
>  #define LLCC_MDMPNG      21
>  #define LLCC_AUDHW       22
>  
> +/*
> + * SCT(System Cache Table) entry contains of the following members:
> + * usecase_id: 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: Boolean indicating if the slice has a fixed capacity
> + * bonus_ways: Bonus ways are additional ways to be used for any slice,
> + *		if client ends up using more than reserved cache ways. Bonus
> + *		ways are allocated only if they are not reserved for some
> + *		other client.
> + * 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(Tightly Coupled Memory)
> + * probe_target_ways: Determines what ways to probe for access hit. When
> + *                    configured to 1 only bonus and reserved 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 maintained active vote
> + *               then the ways assigned to this client are not flushed on power
> + *               collapse.
> + * activate_on_init: Activate the slice immediately after the SCT is programmed
> + */
> +#define SCT_ENTRY(uid, sid, mc, p, fs, bway, rway, cmod, ptw, dca, rp, a) \
> +	{					\
> +		.usecase_id = uid,		\
> +		.slice_id = sid,		\
> +		.max_cap = mc,			\
> +		.priority = p,			\
> +		.fixed_size = fs,		\
> +		.bonus_ways = bway,		\
> +		.res_ways = rway,		\
> +		.cache_mode = cmod,		\
> +		.probe_target_ways = ptw,	\
> +		.dis_cap_alloc = dca,		\
> +		.retain_on_pc = rp,		\
> +		.activate_on_init = a,		\
> +	}
> +
>  /**
>   * llcc_slice_desc - Cache slice descriptor
>   * @slice_id: llcc slice id
> @@ -67,6 +107,11 @@ struct llcc_slice_config {
>  	bool activate_on_init;
>  };
>  
> +struct qcom_llcc_config {
> +	const struct llcc_slice_config *sct_data;
> +	int size;
> +};
> +
>  /**
>   * llcc_drv_data - Data associated with the llcc driver
>   * @regmap: regmap associated with the llcc device
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

Re: [PATCH 1/2] soc: qcom: llcc: Rename llcc-sdm845 to llcc-plat

[Vivek Gautam]

On Thu, Jul 11, 2019 at 9:19 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 11 Jul 04:03 PDT 2019, Vivek Gautam wrote:
>
> > To avoid adding files for each future supported SoCs rename
> > the file to a generic name - llcc-plat, so that llcc configuration
> > tables for other SoCs can be added in the same driver.
> >
>
> We've had a generic LLCC Kconfig option and then a specific SDM845 one,
> with this change we have two different generic options and both would
> either always be enabled or disabled.
>
> So I think you should drop QCOM_SDM845_LLCC and build both llcc-slice
> and llcc-plat into the same qcom_llcc.ko instead.

Yea. I can chuck off the llcc-slice module. But for readability would
it still be
better to maintain separate files. I will drop the SDM845 config, and keep only
QCOM_LLC.

Best regards
Vivek

>
> Regards,
> Bjorn
>
> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> > ---
> >  drivers/soc/qcom/Kconfig                        | 10 +++++-----
> >  drivers/soc/qcom/Makefile                       |  2 +-
> >  drivers/soc/qcom/{llcc-sdm845.c => llcc-plat.c} |  0
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> >  rename drivers/soc/qcom/{llcc-sdm845.c => llcc-plat.c} (100%)
> >
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index a6d1bfb17279..8110d415b18e 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -62,13 +62,13 @@ config QCOM_LLCC
> >         to clients that use the LLCC. Say yes here to enable LLCC slice
> >         driver.
> >
> > -config QCOM_SDM845_LLCC
> > -     tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
> > +config QCOM_PLAT_LLCC
> > +     tristate "Qualcomm Technologies, Inc. platform LLCC driver"
> >       depends on QCOM_LLCC
> >       help
> > -       Say yes here to enable the LLCC driver for SDM845. This provides
> > -       data required to configure LLCC so that clients can start using the
> > -       LLCC slices.
> > +       Say yes here to enable the LLCC driver for Qcom platforms, such as
> > +       SDM845. This provides data required to configure LLCC so that
> > +       clients can start using the LLCC slices.
> >
> >  config QCOM_MDT_LOADER
> >       tristate
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index eeb088beb15f..3bf26667d7ee 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -21,6 +21,6 @@ obj-$(CONFIG_QCOM_SMSM)     += smsm.o
> >  obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> >  obj-$(CONFIG_QCOM_APR) += apr.o
> >  obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
> > -obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
> > +obj-$(CONFIG_QCOM_PLAT_LLCC) += llcc-plat.o
> >  obj-$(CONFIG_QCOM_RPMHPD) += rpmhpd.o
> >  obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> > diff --git a/drivers/soc/qcom/llcc-sdm845.c b/drivers/soc/qcom/llcc-plat.c
> > similarity index 100%
> > rename from drivers/soc/qcom/llcc-sdm845.c
> > rename to drivers/soc/qcom/llcc-plat.c
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> > of Code Aurora Forum, hosted by The Linux Foundation
> >



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH 2/2] soc: qcom: llcc-plat: Make the driver more generic

[Vivek Gautam]

Hi Bjorn,


Thanks for the review.

On Thu, Jul 11, 2019 at 9:29 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 11 Jul 04:03 PDT 2019, Vivek Gautam wrote:
>
> > - Remove 'sdm845' from names, and use 'plat' instead.
> > - Move SCT_ENTRY macro to header file.
> > - Create a new config structure to asssign to of-match-data.
> >
>
> I interpret the intention of these two patches as that you want to add
> some new platform without having to create one llcc-xyz.c per platform.

That's right. The intention is to avoid creating a new platform specific file.

>
> If that's the case then the only user of this macro would be in plat.c,
> so I don't see a reason for moving it to the header file.

Alright. Better to keep it in the driver file itself.

>
> > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> > ---
> >  drivers/soc/qcom/llcc-plat.c       | 77 ++++++++++++--------------------------
> >  include/linux/soc/qcom/llcc-qcom.h | 45 ++++++++++++++++++++++
> >  2 files changed, 68 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/llcc-plat.c b/drivers/soc/qcom/llcc-plat.c
> > index 86600d97c36d..31cff0f75b53 100644
> > --- a/drivers/soc/qcom/llcc-plat.c
> > +++ b/drivers/soc/qcom/llcc-plat.c
> > @@ -1,6 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> > - * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved.
> >   *
> >   */
> >
> > @@ -10,47 +10,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/soc/qcom/llcc-qcom.h>
> >
> > -/*
> > - * SCT(System Cache Table) entry contains of the following members:
>
> Should have caught this during previous review, but this comment simply
> duplicates the kerneldoc for struct llcc_slice_config.

Ok, i noticed it now. Will clean it up. I can remove this comment, and update
the one for struct llcc_slice_config.

>
> > - * usecase_id: 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: Boolean indicating if the slice has a fixed capacity
> > - * bonus_ways: Bonus ways are additional ways to be used for any slice,
> > - *           if client ends up using more than reserved cache ways. Bonus
> > - *           ways are allocated only if they are not reserved for some
> > - *           other client.
> > - * 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(Tightly Coupled Memory)
> > - * probe_target_ways: Determines what ways to probe for access hit. When
> > - *                    configured to 1 only bonus and reserved 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 maintained active vote
> > - *               then the ways assigned to this client are not flushed on power
> > - *               collapse.
> > - * activate_on_init: Activate the slice immediately after the SCT is programmed
> > - */
> > -#define SCT_ENTRY(uid, sid, mc, p, fs, bway, rway, cmod, ptw, dca, rp, a) \
>
> This simply maps macro arguments 1:1 to struct members, there's no need
> for a macro for this.

Sure, will remove the macro.

>
> > -     {                                       \
> > -             .usecase_id = uid,              \
> > -             .slice_id = sid,                \
> > -             .max_cap = mc,                  \
> > -             .priority = p,                  \
> > -             .fixed_size = fs,               \
> > -             .bonus_ways = bway,             \
> > -             .res_ways = rway,               \
> > -             .cache_mode = cmod,             \
> > -             .probe_target_ways = ptw,       \
> > -             .dis_cap_alloc = dca,           \
> > -             .retain_on_pc = rp,             \
> > -             .activate_on_init = a,          \
> > -     }
> > -
> > -static struct llcc_slice_config sdm845_data[] =  {
> > +static const struct llcc_slice_config sdm845_data[] =  {
> >       SCT_ENTRY(LLCC_CPUSS,    1,  2816, 1, 0, 0xffc, 0x2,   0, 0, 1, 1, 1),
> >       SCT_ENTRY(LLCC_VIDSC0,   2,  512,  2, 1, 0x0,   0x0f0, 0, 0, 1, 1, 0),
> >       SCT_ENTRY(LLCC_VIDSC1,   3,  512,  2, 1, 0x0,   0x0f0, 0, 0, 1, 1, 0),
> > @@ -71,30 +31,39 @@ static struct llcc_slice_config sdm845_data[] =  {
> >       SCT_ENTRY(LLCC_AUDHW,    22, 1024, 1, 1, 0xffc, 0x2,   0, 0, 1, 1, 0),
> >  };
> >
> > -static int sdm845_qcom_llcc_remove(struct platform_device *pdev)
> > +static const struct qcom_llcc_config sdm845_cfg = {
> > +     .sct_data       = sdm845_data,
> > +     .size           = ARRAY_SIZE(sdm845_data),
> > +};
> > +
> > +static int qcom_plat_llcc_remove(struct platform_device *pdev)
> >  {
> >       return qcom_llcc_remove(pdev);
> >  }
> >
> > -static int sdm845_qcom_llcc_probe(struct platform_device *pdev)
> > +static int qcom_plat_llcc_probe(struct platform_device *pdev)
> >  {
> > -     return qcom_llcc_probe(pdev, sdm845_data, ARRAY_SIZE(sdm845_data));
> > +     const struct qcom_llcc_config *cfg;
> > +
> > +     cfg = of_device_get_match_data(&pdev->dev);
> > +
> > +     return qcom_llcc_probe(pdev, cfg->sct_data, cfg->size);
> >  }
> >
> > -static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
> > -     { .compatible = "qcom,sdm845-llcc", },
> > +static const struct of_device_id qcom_plat_llcc_of_match[] = {
> > +     { .compatible = "qcom,sdm845-llcc", .data = &sdm845_cfg },
> >       { }
> >  };
> >
> > -static struct platform_driver sdm845_qcom_llcc_driver = {
> > +static struct platform_driver qcom_plat_llcc_driver = {
> >       .driver = {
> > -             .name = "sdm845-llcc",
> > -             .of_match_table = sdm845_qcom_llcc_of_match,
> > +             .name = "qcom-plat-llcc",
>
> With this being the "one and only llcc driver", why not making it
> "qcom_llcc"?

Sure, will make it just "qcom_llcc" driver.

>
> > +             .of_match_table = qcom_plat_llcc_of_match,
> >       },
> > -     .probe = sdm845_qcom_llcc_probe,
> > -     .remove = sdm845_qcom_llcc_remove,
> > +     .probe = qcom_plat_llcc_probe,
> > +     .remove = qcom_plat_llcc_remove,
> >  };
> > -module_platform_driver(sdm845_qcom_llcc_driver);
> > +module_platform_driver(qcom_plat_llcc_driver);
> >
> > -MODULE_DESCRIPTION("QCOM sdm845 LLCC driver");
> > +MODULE_DESCRIPTION("QCOM platform LLCC driver");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
>
> This file should be describing the public interface to the llcc, the
> private pieces is better kept in drivers/soc/qcom/llcc.h

Yes. I will split the things in two separate files as suggested.

>
> But this patch makes me wonder if there's a need to split llcc-slice and
> llcc-plat (and have a header file to describe API between them) instead
> of just having one file.

Nice opportunity to merge the two files? :)

Best regards
Vivek
>
> Regards,
> Bjorn
>
> > index eb71a50b8afc..8776bb5d3891 100644
> > --- a/include/linux/soc/qcom/llcc-qcom.h
> > +++ b/include/linux/soc/qcom/llcc-qcom.h
> > @@ -27,6 +27,46 @@
> >  #define LLCC_MDMPNG      21
> >  #define LLCC_AUDHW       22
> >
> > +/*
> > + * SCT(System Cache Table) entry contains of the following members:
> > + * usecase_id: 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: Boolean indicating if the slice has a fixed capacity
> > + * bonus_ways: Bonus ways are additional ways to be used for any slice,
> > + *           if client ends up using more than reserved cache ways. Bonus
> > + *           ways are allocated only if they are not reserved for some
> > + *           other client.
> > + * 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(Tightly Coupled Memory)
> > + * probe_target_ways: Determines what ways to probe for access hit. When
> > + *                    configured to 1 only bonus and reserved 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 maintained active vote
> > + *               then the ways assigned to this client are not flushed on power
> > + *               collapse.
> > + * activate_on_init: Activate the slice immediately after the SCT is programmed
> > + */
> > +#define SCT_ENTRY(uid, sid, mc, p, fs, bway, rway, cmod, ptw, dca, rp, a) \
> > +     {                                       \
> > +             .usecase_id = uid,              \
> > +             .slice_id = sid,                \
> > +             .max_cap = mc,                  \
> > +             .priority = p,                  \
> > +             .fixed_size = fs,               \
> > +             .bonus_ways = bway,             \
> > +             .res_ways = rway,               \
> > +             .cache_mode = cmod,             \
> > +             .probe_target_ways = ptw,       \
> > +             .dis_cap_alloc = dca,           \
> > +             .retain_on_pc = rp,             \
> > +             .activate_on_init = a,          \
> > +     }
> > +
> >  /**
> >   * llcc_slice_desc - Cache slice descriptor
> >   * @slice_id: llcc slice id
> > @@ -67,6 +107,11 @@ struct llcc_slice_config {
> >       bool activate_on_init;
> >  };
> >
> > +struct qcom_llcc_config {
> > +     const struct llcc_slice_config *sct_data;
> > +     int size;
> > +};
> > +
> >  /**
> >   * llcc_drv_data - Data associated with the llcc driver
> >   * @regmap: regmap associated with the llcc device
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> > of Code Aurora Forum, hosted by The Linux Foundation
> >



--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation