Hi,
On 2020-08-18 02:35, Doug Anderson wrote:
Hi,
On Mon, Aug 17, 2020 at 7:47 AM Sai Prakash Ranjan
<saiprakash.ran...@codeaurora.org> wrote:
From: "Isaac J. Manjarres" <isa...@codeaurora.org>
Older chipsets may not be allowed to configure certain LLCC registers
as that is handled by the secure side software. However, this is not
the case for newer chipsets and they must configure these registers
according to the contents of the SCT table, while keeping in mind that
older targets may not have these capabilities. So add support to allow
such configuration of registers to enable capacity based allocation
and power collapse retention for capable chipsets.
I have very little idea about what the above means. That being said,
what's broken that this patch fixes? Please include this in the CL
description. It should answer, in the very least, the following two
questions:
As the commit message says about secure software configuring these LLCC
registers,
usually 2 things can happen in that case.
1) Accessing those registers in non secure world like Kernel would
result in a
fault which is trapped by secure side.
2) Access to those registers may be just ignored and there will be no
faults.
So for older chipsets, this is a fix to not allow them to access those
registers.
For newer chipsets, we follow the recommended settings from HW/SW arch
teams.
But... upstream llcc driver only supports SDM845 currently which is not
required
to configure those registers and as per my testing, no crash is observed
on SDM845.
So we won't need fixes tag.
a) Were existing attempts to do capacity based allocation failing, or
is capacity based allocation a new whizbang feature that a future
patch will add and you need this one to land first?
Capacity-based allocation and Way-based allocation are cache
partitioning
schemes/algorithms usually used in shared LLCs. Now which one to use or
why
one is preferred over the other are decided by HW/SW architecture teams
and are
recommended by them. So if the question is what is capacity based
allocation and
how it works, then I am afraid that I will not be able to explain that
algorithm
just like that.
b) Why was it bad not to enable power collapse retention? Was this
causing things to get corrupted after resume? Was this causing us to
fail to suspend? Were we burning too little power in S3 and the
battery vendors are looking for an excuse to sell bigger batteries?
I'm not very smart and am also lacking documentation for what the heck
all this is, so I'm looking for the "why" of your patch.
That's a fair point. I will try to dig through to find some context for
"question b"
and check if there were any battery vendors involved in this decision ;)
Signed-off-by: Isaac J. Manjarres <isa...@codeaurora.org>
(sai: use table instead of dt property and minor commit msg change)
Signed-off-by: Sai Prakash Ranjan <saiprakash.ran...@codeaurora.org>
---
Changes in v2:
* Fix build errors reported by kernel test robot.
---
drivers/soc/qcom/llcc-qcom.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/soc/qcom/llcc-qcom.c
b/drivers/soc/qcom/llcc-qcom.c
index 429b5a60a1ba..865f607cf502 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -45,6 +45,9 @@
#define LLCC_TRP_ATTR0_CFGn(n) (0x21000 + SZ_8 * n)
#define LLCC_TRP_ATTR1_CFGn(n) (0x21004 + SZ_8 * n)
+#define LLCC_TRP_SCID_DIS_CAP_ALLOC 0x21F00
+#define LLCC_TRP_PCB_ACT 0x21F04
+
#define BANK_OFFSET_STRIDE 0x80000
/**
@@ -318,6 +321,11 @@ size_t llcc_get_slice_size(struct llcc_slice_desc
*desc)
}
EXPORT_SYMBOL_GPL(llcc_get_slice_size);
+static const struct of_device_id __maybe_unused
qcom_llcc_configure_of_match[] = {
+ { .compatible = "qcom,sc7180-llcc" },
+ { }
+};
Why are you introducing a whole second table? Shouldn't you just add
a field to "struct qcom_llcc_config" ?
This was my 2nd option, first one was to have this based on the version
of LLCC
which are exposed by hw info registers. But that didn't turn out good
since I
couldn't find any relation of this property with LLCC version.
Second option was as you mentioned to have a field to qcom_llcc_config.
Now this is good,
but then I thought that if we add LLCC support for 20(random number)
SoCs of which
10 is capable of supporting cap_based_alloc and rest 10 are not, then we
will still be adding
20 more lines to each SoC's llcc_config if we follow this 2nd option.
So why not opt for a 3rd option with the table where you just need to
specify only the capable
targets which is just 10 in our sample case above.
Am I just overthinking this too much and should just go with the 2nd
option as you mentioned?
+
static int qcom_llcc_cfg_program(struct platform_device *pdev)
{
int i;
@@ -327,13 +335,17 @@ static int qcom_llcc_cfg_program(struct
platform_device *pdev)
u32 attr0_val;
u32 max_cap_cacheline;
u32 sz;
+ u32 disable_cap_alloc = 0, retain_pc = 0;
Don't init to 0. See below.
int ret = 0;
const struct llcc_slice_config *llcc_table;
struct llcc_slice_desc desc;
+ const struct of_device_id *llcc_configure;
sz = drv_data->cfg_size;
llcc_table = drv_data->cfg;
+ llcc_configure = of_match_node(qcom_llcc_configure_of_match,
pdev->dev.of_node);
+
As per above, just use the existing config.
See above explanation.
for (i = 0; i < sz; i++) {
attr1_cfg =
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
attr0_cfg =
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);
@@ -369,6 +381,21 @@ static int qcom_llcc_cfg_program(struct
platform_device *pdev)
attr0_val);
if (ret)
return ret;
+
+ if (llcc_configure) {
+ disable_cap_alloc |=
llcc_table[i].dis_cap_alloc << llcc_table[i].slice_id;
Don't "|=". You're the only place touching this variable. Just set
it.
Ack, will change.
+ ret = regmap_write(drv_data->bcast_regmap,
+
LLCC_TRP_SCID_DIS_CAP_ALLOC, disable_cap_alloc);
+ if (ret)
+ return ret;
+
+ retain_pc |= llcc_table[i].retain_on_pc <<
llcc_table[i].slice_id;
Don't "|=". You're the only place touching this variable. Just set
it.
Ack, will change.
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
