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

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@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" ?


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