| Message ID | 010101746c377537-ce93e925-598b-4dce-bb16-4cda020f4d6f-000000@us-west-2.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv3] soc: qcom: llcc: Support chipsets that can write to llcc registers | expand |
Hi, On Mon, Sep 7, 2020 at 10:36 PM Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote: > > --- a/include/linux/soc/qcom/llcc-qcom.h > +++ b/include/linux/soc/qcom/llcc-qcom.h > @@ -73,6 +73,7 @@ struct llcc_edac_reg_data { > * @bitmap: Bit map to track the active slice ids > * @offsets: Pointer to the bank offsets array > * @ecc_irq: interrupt for llcc cache error detection and reporting > + * @need_llcc_config: check if llcc configuration is required > */ > struct llcc_drv_data { > struct regmap *regmap; > @@ -85,6 +86,7 @@ struct llcc_drv_data { > unsigned long *bitmap; > u32 *offsets; > int ecc_irq; > + bool need_llcc_config; Do you really need to add this into "struct llcc_drv_data"? You use it once at probe time and you could just pass it in to qcom_llcc_cfg_program(), or just pass the "struct qcom_llcc_config" to qcom_llcc_cfg_program()? It's not a huge deal, but it would make your patch simpler and keep an extra element out of the include file. In any case: Reviewed-by: Douglas Anderson <dianders@chromium.org>
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@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. >> >> 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@codeaurora.org> >> (sai: use existing config instead of dt property and commit msg >> change) > > Should be the following format: > > [saiprakash.ranjan@codeaurora.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. >> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.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. >> }; >> >> 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. 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. Thanks for the review. Thanks, Sai
Hi, On 2020-09-08 20:30, Doug Anderson wrote: > Hi, > > On Mon, Sep 7, 2020 at 10:36 PM Sai Prakash Ranjan > <saiprakash.ranjan@codeaurora.org> wrote: >> >> --- a/include/linux/soc/qcom/llcc-qcom.h >> +++ b/include/linux/soc/qcom/llcc-qcom.h >> @@ -73,6 +73,7 @@ struct llcc_edac_reg_data { >> * @bitmap: Bit map to track the active slice ids >> * @offsets: Pointer to the bank offsets array >> * @ecc_irq: interrupt for llcc cache error detection and reporting >> + * @need_llcc_config: check if llcc configuration is required >> */ >> struct llcc_drv_data { >> struct regmap *regmap; >> @@ -85,6 +86,7 @@ struct llcc_drv_data { >> unsigned long *bitmap; >> u32 *offsets; >> int ecc_irq; >> + bool need_llcc_config; > > Do you really need to add this into "struct llcc_drv_data"? You use > it once at probe time and you could just pass it in to > qcom_llcc_cfg_program(), or just pass the "struct qcom_llcc_config" to > qcom_llcc_cfg_program()? It's not a huge deal, but it would make your > patch simpler and keep an extra element out of the include file. > I just kept it following how other properties were passed to qcom_llcc_cfg_program(), but yes its better to just pass qcom_llcc_config to qcom_llcc_cfg_program() so that any future additions also can use it, will change it in the next version. > In any case: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thanks, Sai
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@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. >> >> >> >> 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@codeaurora.org> >> >> (sai: use existing config instead of dt property and commit msg >> >> change) >> > >> > Should be the following format: >> > >> > [saiprakash.ranjan@codeaurora.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@codeaurora.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
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 + #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, }; 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; + 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); @@ -474,6 +496,7 @@ static int qcom_llcc_probe(struct platform_device *pdev) drv_data->cfg = llcc_cfg; drv_data->cfg_size = sz; + drv_data->need_llcc_config = cfg->need_llcc_cfg; mutex_init(&drv_data->lock); platform_set_drvdata(pdev, drv_data); diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h index 90b864655822..52c780085f61 100644 --- a/include/linux/soc/qcom/llcc-qcom.h +++ b/include/linux/soc/qcom/llcc-qcom.h @@ -73,6 +73,7 @@ struct llcc_edac_reg_data { * @bitmap: Bit map to track the active slice ids * @offsets: Pointer to the bank offsets array * @ecc_irq: interrupt for llcc cache error detection and reporting + * @need_llcc_config: check if llcc configuration is required */ struct llcc_drv_data { struct regmap *regmap; @@ -85,6 +86,7 @@ struct llcc_drv_data { unsigned long *bitmap; u32 *offsets; int ecc_irq; + bool need_llcc_config; }; #if IS_ENABLED(CONFIG_QCOM_LLCC)