[PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan]

From: "Isaac J. Manjarres" <isaacm@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.

Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
(sai: use table instead of dt property and minor commit msg change)
Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@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" },
+	{ }
+};
+
 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;
 	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);
+
 	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;
+			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;
+			ret = regmap_write(drv_data->bcast_regmap,
+						LLCC_TRP_PCB_ACT, retain_pc);
+			if (ret)
+				return ret;
+		}
+
 		if (llcc_table[i].activate_on_init) {
 			desc.slice_id = llcc_table[i].slice_id;
 			ret = llcc_slice_activate(&desc);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Doug Anderson]

Hi,

On Mon, Aug 17, 2020 at 7:47 AM Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> From: "Isaac J. Manjarres" <isaacm@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:

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?

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.


> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> (sai: use table instead of dt property and minor commit msg change)
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@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" ?


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


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


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


-Doug

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan]

Hi,

On 2020-08-18 02:35, Doug Anderson wrote:
> Hi,
> 
> On Mon, Aug 17, 2020 at 7:47 AM Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>> 
>> From: "Isaac J. Manjarres" <isaacm@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 <isaacm@codeaurora.org>
>> (sai: use table instead of dt property and minor commit msg change)
>> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@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

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Doug Anderson]

Hi,

On Tue, Aug 18, 2020 at 2:40 AM Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi,
>
> On 2020-08-18 02:35, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Aug 17, 2020 at 7:47 AM Sai Prakash Ranjan
> > <saiprakash.ranjan@codeaurora.org> wrote:
> >>
> >> From: "Isaac J. Manjarres" <isaacm@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.

I guess to start, it wasn't obvious (to me) that there were two
choices and we were picking one.  Mentioning that the other
alternative was way-based allocation would help a lot.  Even if you
can't fully explain the differences between the two, adding something
to the commit message indicating that this is a policy decision (in
other words, both work but each have their tradeoffs) would help.
Something like this, if it's correct:

In general we try to enable capacity based allocation (instead of the
default way based allocation) since that gives us better performance
with the current software / hardware configuration.


> > 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 ;)

Thanks!


> >> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> >> (sai: use table instead of dt property and minor commit msg change)
> >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@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.

If you do it right, you'd only need to add lines to the SoCs that need
it.  Linux conventions in general are that it's OK (and encouraged) to
rely on the fact that if you don't mention a variable in static
initialization it's initted to 0/False/NULL.  So if the member
variable is "need_llcc_config" then you only need to add this in
places where you're setting it to true.  It only needs to be a boolean
so if later someone really is worried about all the bytes that flags
like this are taking up we can use a bitfield.  For now (presumably)
just adding a boolean would be more efficient since (presumably) the
extra code needed to access a bitfield would be more than the extra
data bytes used.  In theory this could also be initdata probably, too.


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

Are you trying to save space?  ...or complexity?  Sure a good compiler
will probably pool the constant string here so you won't need to
allocate it twice, but IMO having a second match table is more
complex.  You also need at least a pointer + bool per entry.  Each
probe will now need to parse through all these strings, too.  None of
this is a big deal, but I'd bet just adding a field to the existing
struct is lower overhead all around.


> Am I just overthinking this too much and should just go with the 2nd
> option as you mentioned?

Someone could feel free to vote against me, but I'd just add to the
existing config.


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

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan]

Hi Doug,

On 2020-08-18 20:42, Doug Anderson wrote:
> Hi,
> 
<snip>...

> 
> I guess to start, it wasn't obvious (to me) that there were two
> choices and we were picking one.  Mentioning that the other
> alternative was way-based allocation would help a lot.  Even if you
> can't fully explain the differences between the two, adding something
> to the commit message indicating that this is a policy decision (in
> other words, both work but each have their tradeoffs) would help.
> Something like this, if it's correct:
> 
> In general we try to enable capacity based allocation (instead of the
> default way based allocation) since that gives us better performance
> with the current software / hardware configuration.
> 

Thanks, I will add it for next version. Let me also go poke some arch 
teams
to understand if we actually do gain something with this selection, who 
knows
we might get some additional details as well.

>> >
>> > 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.
> 
> If you do it right, you'd only need to add lines to the SoCs that need
> it.  Linux conventions in general are that it's OK (and encouraged) to
> rely on the fact that if you don't mention a variable in static
> initialization it's initted to 0/False/NULL.  So if the member
> variable is "need_llcc_config" then you only need to add this in
> places where you're setting it to true.  It only needs to be a boolean
> so if later someone really is worried about all the bytes that flags
> like this are taking up we can use a bitfield.  For now (presumably)
> just adding a boolean would be more efficient since (presumably) the
> extra code needed to access a bitfield would be more than the extra
> data bytes used.  In theory this could also be initdata probably, too.
> 

Yes but in this case we would better be explicit about the capable SoCs
because for someone in QCOM it would be easier to confirm if the SoC is
actually capable but it will not be very obvious for outside folks that
the particular SoC actually supports it. I will use a boolean field 
properly
initialized to indicate if a particular SoC is capable or not.

> 
>> 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.
> 
> Are you trying to save space?  ...or complexity?  Sure a good compiler
> will probably pool the constant string here so you won't need to
> allocate it twice, but IMO having a second match table is more
> complex.  You also need at least a pointer + bool per entry.  Each
> probe will now need to parse through all these strings, too.  None of
> this is a big deal, but I'd bet just adding a field to the existing
> struct is lower overhead all around.
> 

Mostly space, but I agree now that the boolean field is indeed better 
than
a separate table.

> 
>> Am I just overthinking this too much and should just go with the 2nd
>> option as you mentioned?
> 
> Someone could feel free to vote against me, but I'd just add to the
> existing config.
> 

I vote for you :)

Thanks,
Sai

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

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan]

Hi,

On 2020-08-18 21:07, Sai Prakash Ranjan wrote:
> Hi Doug,
> 
>> 
>> I guess to start, it wasn't obvious (to me) that there were two
>> choices and we were picking one.  Mentioning that the other
>> alternative was way-based allocation would help a lot.  Even if you
>> can't fully explain the differences between the two, adding something
>> to the commit message indicating that this is a policy decision (in
>> other words, both work but each have their tradeoffs) would help.
>> Something like this, if it's correct:
>> 
>> In general we try to enable capacity based allocation (instead of the
>> default way based allocation) since that gives us better performance
>> with the current software / hardware configuration.
>> 
> 
> Thanks, I will add it for next version. Let me also go poke some arch 
> teams
> to understand if we actually do gain something with this selection, who 
> knows
> we might get some additional details as well.
> 

I got some information from arch team today, to quote them exactly:

1) What benefits capacity based allocation brings over the default way
based allocation?

"Capacity based allows finer grain partition. It is not about improved
performance but more flexibility in configuration."

2) Retain through power collapse, doesn’t it burn more power?

"This feature is similar to the standard feature of retention. Yes, when 
we
have cache in retention mode it burns more power but it keeps the values 
so
that when we wake up we can get more cache hits."


If its good enough, then I will add this info to the commit msg and post
next version.

Thanks,
Sai

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

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Doug Anderson]

Hi,

On Thu, Sep 3, 2020 at 2:58 AM Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi,
>
> On 2020-08-18 21:07, Sai Prakash Ranjan wrote:
> > Hi Doug,
> >
> >>
> >> I guess to start, it wasn't obvious (to me) that there were two
> >> choices and we were picking one.  Mentioning that the other
> >> alternative was way-based allocation would help a lot.  Even if you
> >> can't fully explain the differences between the two, adding something
> >> to the commit message indicating that this is a policy decision (in
> >> other words, both work but each have their tradeoffs) would help.
> >> Something like this, if it's correct:
> >>
> >> In general we try to enable capacity based allocation (instead of the
> >> default way based allocation) since that gives us better performance
> >> with the current software / hardware configuration.
> >>
> >
> > Thanks, I will add it for next version. Let me also go poke some arch
> > teams
> > to understand if we actually do gain something with this selection, who
> > knows
> > we might get some additional details as well.
> >
>
> I got some information from arch team today, to quote them exactly:
>
> 1) What benefits capacity based allocation brings over the default way
> based allocation?
>
> "Capacity based allows finer grain partition. It is not about improved
> performance but more flexibility in configuration."
>
> 2) Retain through power collapse, doesn’t it burn more power?
>
> "This feature is similar to the standard feature of retention. Yes, when
> we
> have cache in retention mode it burns more power but it keeps the values
> so
> that when we wake up we can get more cache hits."
>
>
> If its good enough, then I will add this info to the commit msg and post
> next version.

Sounds fine to me.  I was mostly looking for a high level idea of what
was happening here.  I am at least a little curious about the
retention bit.  Is that retention during S3, or during some sort of
Runtime PM?  Any idea how much power is burned?  Unless the power is
miniscule it seems hard to believe that it would be a net win to keep
a cache powered up during S3 unless you're planning on waking up a
lot.

-Doug

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan]

On 2020-09-03 19:16, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 3, 2020 at 2:58 AM Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>> 
>> Hi,
>> 
>> On 2020-08-18 21:07, Sai Prakash Ranjan wrote:
>> > Hi Doug,
>> >
>> >>
>> >> I guess to start, it wasn't obvious (to me) that there were two
>> >> choices and we were picking one.  Mentioning that the other
>> >> alternative was way-based allocation would help a lot.  Even if you
>> >> can't fully explain the differences between the two, adding something
>> >> to the commit message indicating that this is a policy decision (in
>> >> other words, both work but each have their tradeoffs) would help.
>> >> Something like this, if it's correct:
>> >>
>> >> In general we try to enable capacity based allocation (instead of the
>> >> default way based allocation) since that gives us better performance
>> >> with the current software / hardware configuration.
>> >>
>> >
>> > Thanks, I will add it for next version. Let me also go poke some arch
>> > teams
>> > to understand if we actually do gain something with this selection, who
>> > knows
>> > we might get some additional details as well.
>> >
>> 
>> I got some information from arch team today, to quote them exactly:
>> 
>> 1) What benefits capacity based allocation brings over the default way
>> based allocation?
>> 
>> "Capacity based allows finer grain partition. It is not about improved
>> performance but more flexibility in configuration."
>> 
>> 2) Retain through power collapse, doesn’t it burn more power?
>> 
>> "This feature is similar to the standard feature of retention. Yes, 
>> when
>> we
>> have cache in retention mode it burns more power but it keeps the 
>> values
>> so
>> that when we wake up we can get more cache hits."
>> 
>> 
>> If its good enough, then I will add this info to the commit msg and 
>> post
>> next version.
> 
> Sounds fine to me.  I was mostly looking for a high level idea of what
> was happening here.  I am at least a little curious about the
> retention bit.  Is that retention during S3, or during some sort of
> Runtime PM?  Any idea how much power is burned?  Unless the power is
> miniscule it seems hard to believe that it would be a net win to keep
> a cache powered up during S3 unless you're planning on waking up a
> lot.
> 

The retention setting is based on sub cache id(SCID), so I think its for
runtime pm, the power numbers weren't provided. But I believe these
decisions are made after solid testing and not some random 
approximations.

Thanks,
Sai

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

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Doug Anderson]

Hi,

On Thu, Sep 3, 2020 at 8:47 AM Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> On 2020-09-03 19:16, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Sep 3, 2020 at 2:58 AM Sai Prakash Ranjan
> > <saiprakash.ranjan@codeaurora.org> wrote:
> >>
> >> Hi,
> >>
> >> On 2020-08-18 21:07, Sai Prakash Ranjan wrote:
> >> > Hi Doug,
> >> >
> >> >>
> >> >> I guess to start, it wasn't obvious (to me) that there were two
> >> >> choices and we were picking one.  Mentioning that the other
> >> >> alternative was way-based allocation would help a lot.  Even if you
> >> >> can't fully explain the differences between the two, adding something
> >> >> to the commit message indicating that this is a policy decision (in
> >> >> other words, both work but each have their tradeoffs) would help.
> >> >> Something like this, if it's correct:
> >> >>
> >> >> In general we try to enable capacity based allocation (instead of the
> >> >> default way based allocation) since that gives us better performance
> >> >> with the current software / hardware configuration.
> >> >>
> >> >
> >> > Thanks, I will add it for next version. Let me also go poke some arch
> >> > teams
> >> > to understand if we actually do gain something with this selection, who
> >> > knows
> >> > we might get some additional details as well.
> >> >
> >>
> >> I got some information from arch team today, to quote them exactly:
> >>
> >> 1) What benefits capacity based allocation brings over the default way
> >> based allocation?
> >>
> >> "Capacity based allows finer grain partition. It is not about improved
> >> performance but more flexibility in configuration."
> >>
> >> 2) Retain through power collapse, doesn’t it burn more power?
> >>
> >> "This feature is similar to the standard feature of retention. Yes,
> >> when
> >> we
> >> have cache in retention mode it burns more power but it keeps the
> >> values
> >> so
> >> that when we wake up we can get more cache hits."
> >>
> >>
> >> If its good enough, then I will add this info to the commit msg and
> >> post
> >> next version.
> >
> > Sounds fine to me.  I was mostly looking for a high level idea of what
> > was happening here.  I am at least a little curious about the
> > retention bit.  Is that retention during S3, or during some sort of
> > Runtime PM?  Any idea how much power is burned?  Unless the power is
> > miniscule it seems hard to believe that it would be a net win to keep
> > a cache powered up during S3 unless you're planning on waking up a
> > lot.
> >
>
> The retention setting is based on sub cache id(SCID), so I think its for
> runtime pm, the power numbers weren't provided. But I believe these
> decisions are made after solid testing and not some random
> approximations.

Right, I believe it was tested, I just wonder if it was tested on a
phone vs. a laptop.  A phone is almost constantly waking up to deal
with stuff (which is why my phone battery barely lasts till the end of
the day).  Phones also usually have some type of self refresh on their
panels so they can be suspended even when they look awake which means
even more constant wakeups.  A laptop (especially without panel self
refresh) may have very different usage models.  I'm trying to confirm
that this setting is appropriate for both classes of devices or if it
has been only measured / optimized for the cell phone use case.

-Doug

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan]

Hi,

On 2020-09-03 21:24, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 3, 2020 at 8:47 AM Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>> 
>> On 2020-09-03 19:16, Doug Anderson wrote:
>> > Hi,
>> >
>> > On Thu, Sep 3, 2020 at 2:58 AM Sai Prakash Ranjan
>> > <saiprakash.ranjan@codeaurora.org> wrote:
>> >>
>> >> Hi,
>> >>
>> >> On 2020-08-18 21:07, Sai Prakash Ranjan wrote:
>> >> > Hi Doug,
>> >> >
>> >> >>
>> >> >> I guess to start, it wasn't obvious (to me) that there were two
>> >> >> choices and we were picking one.  Mentioning that the other
>> >> >> alternative was way-based allocation would help a lot.  Even if you
>> >> >> can't fully explain the differences between the two, adding something
>> >> >> to the commit message indicating that this is a policy decision (in
>> >> >> other words, both work but each have their tradeoffs) would help.
>> >> >> Something like this, if it's correct:
>> >> >>
>> >> >> In general we try to enable capacity based allocation (instead of the
>> >> >> default way based allocation) since that gives us better performance
>> >> >> with the current software / hardware configuration.
>> >> >>
>> >> >
>> >> > Thanks, I will add it for next version. Let me also go poke some arch
>> >> > teams
>> >> > to understand if we actually do gain something with this selection, who
>> >> > knows
>> >> > we might get some additional details as well.
>> >> >
>> >>
>> >> I got some information from arch team today, to quote them exactly:
>> >>
>> >> 1) What benefits capacity based allocation brings over the default way
>> >> based allocation?
>> >>
>> >> "Capacity based allows finer grain partition. It is not about improved
>> >> performance but more flexibility in configuration."
>> >>
>> >> 2) Retain through power collapse, doesn’t it burn more power?
>> >>
>> >> "This feature is similar to the standard feature of retention. Yes,
>> >> when
>> >> we
>> >> have cache in retention mode it burns more power but it keeps the
>> >> values
>> >> so
>> >> that when we wake up we can get more cache hits."
>> >>
>> >>
>> >> If its good enough, then I will add this info to the commit msg and
>> >> post
>> >> next version.
>> >
>> > Sounds fine to me.  I was mostly looking for a high level idea of what
>> > was happening here.  I am at least a little curious about the
>> > retention bit.  Is that retention during S3, or during some sort of
>> > Runtime PM?  Any idea how much power is burned?  Unless the power is
>> > miniscule it seems hard to believe that it would be a net win to keep
>> > a cache powered up during S3 unless you're planning on waking up a
>> > lot.
>> >
>> 
>> The retention setting is based on sub cache id(SCID), so I think its 
>> for
>> runtime pm, the power numbers weren't provided. But I believe these
>> decisions are made after solid testing and not some random
>> approximations.
> 
> Right, I believe it was tested, I just wonder if it was tested on a
> phone vs. a laptop.  A phone is almost constantly waking up to deal
> with stuff (which is why my phone battery barely lasts till the end of
> the day).  Phones also usually have some type of self refresh on their
> panels so they can be suspended even when they look awake which means
> even more constant wakeups.  A laptop (especially without panel self
> refresh) may have very different usage models.  I'm trying to confirm
> that this setting is appropriate for both classes of devices or if it
> has been only measured / optimized for the cell phone use case.
> 

Could be, but there are windows laptops based on QCOM SoCs where these
must have also been tested (note that this setting can also be in 
firmware
and no one would know), but I don't have numbers to quantify.

Thanks,
Sai

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

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Doug Anderson]

Hi,

On Thu, Sep 3, 2020 at 9:04 AM Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Hi,
>
> On 2020-09-03 21:24, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Sep 3, 2020 at 8:47 AM Sai Prakash Ranjan
> > <saiprakash.ranjan@codeaurora.org> wrote:
> >>
> >> On 2020-09-03 19:16, Doug Anderson wrote:
> >> > Hi,
> >> >
> >> > On Thu, Sep 3, 2020 at 2:58 AM Sai Prakash Ranjan
> >> > <saiprakash.ranjan@codeaurora.org> wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> On 2020-08-18 21:07, Sai Prakash Ranjan wrote:
> >> >> > Hi Doug,
> >> >> >
> >> >> >>
> >> >> >> I guess to start, it wasn't obvious (to me) that there were two
> >> >> >> choices and we were picking one.  Mentioning that the other
> >> >> >> alternative was way-based allocation would help a lot.  Even if you
> >> >> >> can't fully explain the differences between the two, adding something
> >> >> >> to the commit message indicating that this is a policy decision (in
> >> >> >> other words, both work but each have their tradeoffs) would help.
> >> >> >> Something like this, if it's correct:
> >> >> >>
> >> >> >> In general we try to enable capacity based allocation (instead of the
> >> >> >> default way based allocation) since that gives us better performance
> >> >> >> with the current software / hardware configuration.
> >> >> >>
> >> >> >
> >> >> > Thanks, I will add it for next version. Let me also go poke some arch
> >> >> > teams
> >> >> > to understand if we actually do gain something with this selection, who
> >> >> > knows
> >> >> > we might get some additional details as well.
> >> >> >
> >> >>
> >> >> I got some information from arch team today, to quote them exactly:
> >> >>
> >> >> 1) What benefits capacity based allocation brings over the default way
> >> >> based allocation?
> >> >>
> >> >> "Capacity based allows finer grain partition. It is not about improved
> >> >> performance but more flexibility in configuration."
> >> >>
> >> >> 2) Retain through power collapse, doesn’t it burn more power?
> >> >>
> >> >> "This feature is similar to the standard feature of retention. Yes,
> >> >> when
> >> >> we
> >> >> have cache in retention mode it burns more power but it keeps the
> >> >> values
> >> >> so
> >> >> that when we wake up we can get more cache hits."
> >> >>
> >> >>
> >> >> If its good enough, then I will add this info to the commit msg and
> >> >> post
> >> >> next version.
> >> >
> >> > Sounds fine to me.  I was mostly looking for a high level idea of what
> >> > was happening here.  I am at least a little curious about the
> >> > retention bit.  Is that retention during S3, or during some sort of
> >> > Runtime PM?  Any idea how much power is burned?  Unless the power is
> >> > miniscule it seems hard to believe that it would be a net win to keep
> >> > a cache powered up during S3 unless you're planning on waking up a
> >> > lot.
> >> >
> >>
> >> The retention setting is based on sub cache id(SCID), so I think its
> >> for
> >> runtime pm, the power numbers weren't provided. But I believe these
> >> decisions are made after solid testing and not some random
> >> approximations.
> >
> > Right, I believe it was tested, I just wonder if it was tested on a
> > phone vs. a laptop.  A phone is almost constantly waking up to deal
> > with stuff (which is why my phone battery barely lasts till the end of
> > the day).  Phones also usually have some type of self refresh on their
> > panels so they can be suspended even when they look awake which means
> > even more constant wakeups.  A laptop (especially without panel self
> > refresh) may have very different usage models.  I'm trying to confirm
> > that this setting is appropriate for both classes of devices or if it
> > has been only measured / optimized for the cell phone use case.
> >
>
> Could be, but there are windows laptops based on QCOM SoCs where these
> must have also been tested (note that this setting can also be in
> firmware
> and no one would know), but I don't have numbers to quantify.

OK, fair enough.  Thanks for the discussion.  I'm good with a somewhat
broad explanation in the commit message then and if we find that this
somehow affects power numbers in a bad way we can track down further.

-Doug

Re: [PATCHv2] soc: qcom: llcc: Support chipsets that can write to llcc registers

[Sai Prakash Ranjan]

On 2020-09-03 23:08, Doug Anderson wrote:
> Hi,
> 
> On Thu, Sep 3, 2020 at 9:04 AM Sai Prakash Ranjan
> <saiprakash.ranjan@codeaurora.org> wrote:
>> 
>> Hi,
>> 
>> On 2020-09-03 21:24, Doug Anderson wrote:
>> > Hi,
>> >
>> > On Thu, Sep 3, 2020 at 8:47 AM Sai Prakash Ranjan
>> > <saiprakash.ranjan@codeaurora.org> wrote:
>> >>
>> >> On 2020-09-03 19:16, Doug Anderson wrote:
>> >> > Hi,
>> >> >
>> >> > On Thu, Sep 3, 2020 at 2:58 AM Sai Prakash Ranjan
>> >> > <saiprakash.ranjan@codeaurora.org> wrote:
>> >> >>
>> >> >> Hi,
>> >> >>
>> >> >> On 2020-08-18 21:07, Sai Prakash Ranjan wrote:
>> >> >> > Hi Doug,
>> >> >> >
>> >> >> >>
>> >> >> >> I guess to start, it wasn't obvious (to me) that there were two
>> >> >> >> choices and we were picking one.  Mentioning that the other
>> >> >> >> alternative was way-based allocation would help a lot.  Even if you
>> >> >> >> can't fully explain the differences between the two, adding something
>> >> >> >> to the commit message indicating that this is a policy decision (in
>> >> >> >> other words, both work but each have their tradeoffs) would help.
>> >> >> >> Something like this, if it's correct:
>> >> >> >>
>> >> >> >> In general we try to enable capacity based allocation (instead of the
>> >> >> >> default way based allocation) since that gives us better performance
>> >> >> >> with the current software / hardware configuration.
>> >> >> >>
>> >> >> >
>> >> >> > Thanks, I will add it for next version. Let me also go poke some arch
>> >> >> > teams
>> >> >> > to understand if we actually do gain something with this selection, who
>> >> >> > knows
>> >> >> > we might get some additional details as well.
>> >> >> >
>> >> >>
>> >> >> I got some information from arch team today, to quote them exactly:
>> >> >>
>> >> >> 1) What benefits capacity based allocation brings over the default way
>> >> >> based allocation?
>> >> >>
>> >> >> "Capacity based allows finer grain partition. It is not about improved
>> >> >> performance but more flexibility in configuration."
>> >> >>
>> >> >> 2) Retain through power collapse, doesn’t it burn more power?
>> >> >>
>> >> >> "This feature is similar to the standard feature of retention. Yes,
>> >> >> when
>> >> >> we
>> >> >> have cache in retention mode it burns more power but it keeps the
>> >> >> values
>> >> >> so
>> >> >> that when we wake up we can get more cache hits."
>> >> >>
>> >> >>
>> >> >> If its good enough, then I will add this info to the commit msg and
>> >> >> post
>> >> >> next version.
>> >> >
>> >> > Sounds fine to me.  I was mostly looking for a high level idea of what
>> >> > was happening here.  I am at least a little curious about the
>> >> > retention bit.  Is that retention during S3, or during some sort of
>> >> > Runtime PM?  Any idea how much power is burned?  Unless the power is
>> >> > miniscule it seems hard to believe that it would be a net win to keep
>> >> > a cache powered up during S3 unless you're planning on waking up a
>> >> > lot.
>> >> >
>> >>
>> >> The retention setting is based on sub cache id(SCID), so I think its
>> >> for
>> >> runtime pm, the power numbers weren't provided. But I believe these
>> >> decisions are made after solid testing and not some random
>> >> approximations.
>> >
>> > Right, I believe it was tested, I just wonder if it was tested on a
>> > phone vs. a laptop.  A phone is almost constantly waking up to deal
>> > with stuff (which is why my phone battery barely lasts till the end of
>> > the day).  Phones also usually have some type of self refresh on their
>> > panels so they can be suspended even when they look awake which means
>> > even more constant wakeups.  A laptop (especially without panel self
>> > refresh) may have very different usage models.  I'm trying to confirm
>> > that this setting is appropriate for both classes of devices or if it
>> > has been only measured / optimized for the cell phone use case.
>> >
>> 
>> Could be, but there are windows laptops based on QCOM SoCs where these
>> must have also been tested (note that this setting can also be in
>> firmware
>> and no one would know), but I don't have numbers to quantify.
> 
> OK, fair enough.  Thanks for the discussion.  I'm good with a somewhat
> broad explanation in the commit message then and if we find that this
> somehow affects power numbers in a bad way we can track down further.
> 

Thanks, I agree that we should keep an eye in case of any fluctuations 
in power numbers.

Thanks,
Sai

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