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

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

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