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