Date:   Wed, 9 Sep 2020 07:23:29 +0000
From:   Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Douglas Anderson <dianders@...omium.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        "Isaac J. Manjarres" <isaacm@...eaurora.org>
Subject: Re: [PATCHv3] soc: qcom: llcc: Support chipsets that can write to
 llcc registers

On 2020-09-09 12:38, Stephen Boyd wrote:
> Quoting Sai Prakash Ranjan (2020-09-09 00:04:00)
>> Hi,
>> 
>> On 2020-09-09 00:02, Stephen Boyd wrote:
>> > Quoting Sai Prakash Ranjan (2020-09-07 22:36:48)
>> >> From: "Isaac J. Manjarres" <isaacm@...eaurora.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.
>> >>
>> >> Reason for choosing capacity based allocation rather than the default
>> >> way based allocation is because capacity based allocation allows more
>> >> finer grain partition and provides more flexibility in configuration.
>> >> As for the retention through power collapse, it has an advantage where
>> >> the cache hits are more when we wake up from power collapse although
>> >> it does burn more power but the exact power numbers are not known at
>> >> the moment.
>> >>
>> >> Signed-off-by: Isaac J. Manjarres <isaacm@...eaurora.org>
>> >> (sai: use existing config instead of dt property and commit msg
>> >> change)
>> >
>> > Should be the following format:
>> >
>> > [saiprakash.ranjan@...eaurora.org: use existing...]
>> >
>> 
>> Hmm, is this documented somewhere because a quick grep shows
>> quite a few places where just the first name is added. Plus
>> there is already a signed-off-by line below this, so we know
>> that it is this 'sai' who made the extra changes.
> 
> See Documentation/process/submitting-patches.rst
> 

Thanks, it says **prepending the description with your mail and/or 
name**.
So its either mail and/or name, but I didn't give my full name
and I don't have a strong opinion, so I can change it to include
my full mail address.

>> 
>> >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>
>> >> ---
>> >> diff --git a/drivers/soc/qcom/llcc-qcom.c
>> >> b/drivers/soc/qcom/llcc-qcom.c
>> >> index 429b5a60a1ba..b908656ce519 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
>> >
>> > Use lowercase hex please. LLCC_COMMON_STATUS0 is using lowercase.
>> >
>> 
>> Ok
>> 
>> >> +
>> >>  #define BANK_OFFSET_STRIDE           0x80000
>> >>
>> >>  /**
>> >> @@ -89,6 +92,7 @@ struct llcc_slice_config {
>> >>  struct qcom_llcc_config {
>> >>         const struct llcc_slice_config *sct_data;
>> >>         int size;
>> >> +       bool need_llcc_cfg;
>> >>  };
>> >>
>> >>  static const struct llcc_slice_config sc7180_data[] =  {
>> >> @@ -122,11 +126,13 @@ static const struct llcc_slice_config
>> >> sdm845_data[] =  {
>> >>  static const struct qcom_llcc_config sc7180_cfg = {
>> >>         .sct_data       = sc7180_data,
>> >>         .size           = ARRAY_SIZE(sc7180_data),
>> >> +       .need_llcc_cfg  = true,
>> >>  };
>> >>
>> >>  static const struct qcom_llcc_config sdm845_cfg = {
>> >>         .sct_data       = sdm845_data,
>> >>         .size           = ARRAY_SIZE(sdm845_data),
>> >> +       .need_llcc_cfg  = false,
>> >
>> > false is the default so just leave it out?
>> >
>> 
>> Done on purpose as I wanted to be explicit here so that
>> anyone reading it knows that it doesn't support configuring
>> in kernel. Yes the default is false but it won't hurt to be
>> explicit here to avoid confusion.
> 
> I fear that being explicit will mean that all SoCs will be explicit and
> that's sort of useless. Does it need to be so explicit?
> 

 From what I know, the majority of the SoCs(upcoming) will have
it true so we don't have to fill them with false everywhere,
just have to do it for SDM845.

>> 
>> >>  };
>> >>
>> >>  static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>> >> @@ -327,6 +333,7 @@ static int qcom_llcc_cfg_program(struct
>> >> platform_device *pdev)
>> >>         u32 attr0_val;
>> >>         u32 max_cap_cacheline;
>> >>         u32 sz;
>> >> +       u32 disable_cap_alloc, retain_pc;
>> >>         int ret = 0;
>> >>         const struct llcc_slice_config *llcc_table;
>> >>         struct llcc_slice_desc desc;
>> >> @@ -369,6 +376,21 @@ static int qcom_llcc_cfg_program(struct
>> >> platform_device *pdev)
>> >>                                         attr0_val);
>> >>                 if (ret)
>> >>                         return ret;
>> >> +
>> >> +               if (drv_data->need_llcc_config) {
>> >> +                       disable_cap_alloc =
>> >> llcc_table[i].dis_cap_alloc << llcc_table[i].slice_id;
>> >
>> > Can we move u32 disable_cap_alloc, retain_pc here? That would keep it
>> > local to this if condition. Or make llc_table[i].slice_id into a local
>> > variable so the shift line isn't so long? Or make the body of this
>> > while
>> > loop a new function that takes an llcc_table[i] pointer so that lines
>> > are easier to read?
>> >
>> 
>> The whole function qcom_llcc_cfg_program() is just a loop so
>> adding another function moving that loop wouldn't look good.
> 
> Agreed it's one big loop, but having a function that deals with one
> "thing" in the loop is easy to reason about and read. Right now I have
> to read llc_table[i] a bunch of times. Yuck.
> 

Ok then I will move it to another function in a separate patch
before this change.

>> llc_table[i].slice_id is already used elsewhere in this function
>> and changing everywhere is not related to this patch. So I will
>> go with your suggestion to move disable_cap_alloc and retain_pc
>> to if block.
>> 
> 
> Ok.

Thanks,
Sai

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

Powered by blists - more mailing lists