From: Bjorn Andersson <andersson@kernel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
bp@alien8.de, tony.luck@intel.com, quic_saipraka@quicinc.com,
konrad.dybcio@linaro.org, linux-arm-msm@vger.kernel.org,
linux-kernel@vger.kernel.org, james.morse@arm.com,
mchehab@kernel.org, rric@kernel.org, linux-edac@vger.kernel.org,
quic_ppareek@quicinc.com, stable@vger.kernel.org
Subject: Re: [PATCH 12/12] qcom: llcc/edac: Fix the base address used for accessing LLCC banks
Date: Wed, 7 Dec 2022 10:17:54 -0600 [thread overview]
Message-ID: <20221207161754.ipiordkogu2fk2dd@builder.lan> (raw)
In-Reply-To: <20221207135922.314827-14-manivannan.sadhasivam@linaro.org>
On Wed, Dec 07, 2022 at 07:29:22PM +0530, Manivannan Sadhasivam wrote:
> The Qualcomm LLCC/EDAC drivers were using a fixed register stride for
> accessing the (Control and Status Registers) CSRs of each LLCC bank.
> This stride only works for some SoCs like SDM845 for which driver
> support was initially added.
>
> But the later SoCs use different register stride that vary between the
> banks with holes in-between. So it is not possible to use a single register
> stride for accessing the CSRs of each bank. By doing so could result in a
> crash.
>
> For fixing this issue, let's obtain the base address of each LLCC bank from
> devicetree and get rid of the fixed stride.
>
> Cc: <stable@vger.kernel.org> # 4.20
> Fixes: a3134fb09e0b ("drivers: soc: Add LLCC driver")
> Fixes: 27450653f1db ("drivers: edac: Add EDAC driver support for QCOM SoCs")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> drivers/edac/qcom_edac.c | 14 +++----
> drivers/soc/qcom/llcc-qcom.c | 64 ++++++++++++++++++------------
> include/linux/soc/qcom/llcc-qcom.h | 4 +-
> 3 files changed, 44 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index 97a27e42dd61..70bd39a91b89 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c
> @@ -213,7 +213,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
>
> for (i = 0; i < reg_data.reg_cnt; i++) {
> synd_reg = reg_data.synd_reg + (i * 4);
> - ret = regmap_read(drv->regmap, drv->offsets[bank] + synd_reg,
> + ret = regmap_read(drv->regmap[bank], synd_reg,
> &synd_val);
> if (ret)
> goto clear;
> @@ -222,8 +222,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> reg_data.name, i, synd_val);
> }
>
> - ret = regmap_read(drv->regmap,
> - drv->offsets[bank] + reg_data.count_status_reg,
> + ret = regmap_read(drv->regmap[bank], reg_data.count_status_reg,
> &err_cnt);
> if (ret)
> goto clear;
> @@ -233,8 +232,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> edac_printk(KERN_CRIT, EDAC_LLCC, "%s: Error count: 0x%4x\n",
> reg_data.name, err_cnt);
>
> - ret = regmap_read(drv->regmap,
> - drv->offsets[bank] + reg_data.ways_status_reg,
> + ret = regmap_read(drv->regmap[bank], reg_data.ways_status_reg,
> &err_ways);
> if (ret)
> goto clear;
> @@ -296,8 +294,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
>
> /* Iterate over the banks and look for Tag RAM or Data RAM errors */
> for (i = 0; i < drv->num_banks; i++) {
> - ret = regmap_read(drv->regmap,
> - drv->offsets[i] + DRP_INTERRUPT_STATUS,
> + ret = regmap_read(drv->regmap[i], DRP_INTERRUPT_STATUS,
> &drp_error);
>
> if (!ret && (drp_error & SB_ECC_ERROR)) {
> @@ -312,8 +309,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
> if (!ret)
> irq_rc = IRQ_HANDLED;
>
> - ret = regmap_read(drv->regmap,
> - drv->offsets[i] + TRP_INTERRUPT_0_STATUS,
> + ret = regmap_read(drv->regmap[i], TRP_INTERRUPT_0_STATUS,
> &trp_error);
>
> if (!ret && (trp_error & SB_ECC_ERROR)) {
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 23ce2f78c4ed..7264ac9993e0 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -62,8 +62,6 @@
> #define LLCC_TRP_WRSC_CACHEABLE_EN 0x21f2c
> #define LLCC_TRP_ALGO_CFG8 0x21f30
>
> -#define BANK_OFFSET_STRIDE 0x80000
> -
> #define LLCC_VERSION_2_0_0_0 0x02000000
> #define LLCC_VERSION_2_1_0_0 0x02010000
> #define LLCC_VERSION_4_1_0_0 0x04010000
> @@ -927,6 +925,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> const struct llcc_slice_config *llcc_cfg;
> u32 sz;
> u32 version;
> + struct regmap *regmap;
>
> drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
> if (!drv_data) {
> @@ -934,12 +933,46 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> goto err;
> }
>
> - drv_data->regmap = qcom_llcc_init_mmio(pdev, "llcc_base");
> - if (IS_ERR(drv_data->regmap)) {
> - ret = PTR_ERR(drv_data->regmap);
> + /* Initialize the first LLCC bank regmap */
> + regmap = qcom_llcc_init_mmio(pdev, "llcc0_base");
> + if (IS_ERR(regmap)) {
> + ret = PTR_ERR(regmap);
> + goto err;
> + }
> +
> + cfg = of_device_get_match_data(&pdev->dev);
> +
> + ret = regmap_read(regmap, cfg->reg_offset[LLCC_COMMON_STATUS0],
> + &num_banks);
Don't be afraid of leaving this line unwrapped.
There's a few lines changed above that would be easier to read if
allowed beyond 80 chars as well.
> + if (ret)
> + goto err;
> +
> + num_banks &= LLCC_LB_CNT_MASK;
> + num_banks >>= LLCC_LB_CNT_SHIFT;
> + drv_data->num_banks = num_banks;
> +
> + drv_data->regmap = devm_kcalloc(dev, num_banks, sizeof(*drv_data->regmap), GFP_KERNEL);
> + if (!drv_data->regmap) {
> + ret = -ENOMEM;
> goto err;
> }
>
> + drv_data->regmap[0] = regmap;
> +
> + /* Initialize rest of LLCC bank regmaps */
> + for (i = 1; i < num_banks; i++) {
> + char *base = kasprintf(GFP_KERNEL, "llcc%d_base", i);
As mentioned in the binding, I think you should obtain the regions by
index instead of name.
> +
> + drv_data->regmap[i] = qcom_llcc_init_mmio(pdev, base);
> + if (IS_ERR(drv_data->regmap[i])) {
> + ret = PTR_ERR(drv_data->regmap[i]);
> + kfree(base);
> + goto err;
> + }
> +
> + kfree(base);
> + }
> +
> drv_data->bcast_regmap =
> qcom_llcc_init_mmio(pdev, "llcc_broadcast_base");
> if (IS_ERR(drv_data->bcast_regmap)) {
> @@ -947,8 +980,6 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> goto err;
> }
>
> - cfg = of_device_get_match_data(&pdev->dev);
> -
> /* Extract version of the IP */
> ret = regmap_read(drv_data->bcast_regmap, cfg->reg_offset[LLCC_COMMON_HW_INFO],
> &version);
> @@ -957,15 +988,6 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>
> drv_data->version = version;
>
> - ret = regmap_read(drv_data->regmap, cfg->reg_offset[LLCC_COMMON_STATUS0],
> - &num_banks);
> - if (ret)
> - goto err;
> -
> - num_banks &= LLCC_LB_CNT_MASK;
> - num_banks >>= LLCC_LB_CNT_SHIFT;
> - drv_data->num_banks = num_banks;
> -
> llcc_cfg = cfg->sct_data;
> sz = cfg->size;
>
> @@ -973,16 +995,6 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> if (llcc_cfg[i].slice_id > drv_data->max_slices)
> drv_data->max_slices = llcc_cfg[i].slice_id;
>
> - drv_data->offsets = devm_kcalloc(dev, num_banks, sizeof(u32),
> - GFP_KERNEL);
> - if (!drv_data->offsets) {
> - ret = -ENOMEM;
> - goto err;
> - }
> -
> - for (i = 0; i < num_banks; i++)
> - drv_data->offsets[i] = i * BANK_OFFSET_STRIDE;
> -
> drv_data->bitmap = devm_bitmap_zalloc(dev, drv_data->max_slices,
> GFP_KERNEL);
> if (!drv_data->bitmap) {
> diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
> index ad1fd718169d..4b8bf585f9ba 100644
> --- a/include/linux/soc/qcom/llcc-qcom.h
> +++ b/include/linux/soc/qcom/llcc-qcom.h
> @@ -129,12 +129,11 @@ struct llcc_edac_reg_offset {
Please also update @regmap description.
> * @max_slices: max slices as read from device tree
> * @num_banks: Number of llcc banks
> * @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
> * @version: Indicates the LLCC version
> */
> struct llcc_drv_data {
> - struct regmap *regmap;
> + struct regmap **regmap;
How about making this plural, to reflect that it's now a set of regmaps?
Regards,
Bjorn
> struct regmap *bcast_regmap;
> const struct llcc_slice_config *cfg;
> const struct llcc_edac_reg_offset *edac_reg_offset;
> @@ -143,7 +142,6 @@ struct llcc_drv_data {
> u32 max_slices;
> u32 num_banks;
> unsigned long *bitmap;
> - u32 *offsets;
> int ecc_irq;
> u32 version;
> };
> --
> 2.25.1
>
next prev parent reply other threads:[~2022-12-07 16:18 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-07 13:59 [PATCH 00/12] Qcom: LLCC/EDAC: Fix base address used for LLCC banks Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 01/12] dt-bindings: arm: msm: Update the maintainers for LLCC Manivannan Sadhasivam
2022-12-08 3:15 ` Sai Prakash Ranjan
2022-12-12 5:54 ` Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 02/12] dt-bindings: arm: msm: Fix register regions used for LLCC banks Manivannan Sadhasivam
2022-12-07 16:08 ` Bjorn Andersson
2022-12-07 16:53 ` Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 03/12] arm64: dts: qcom: sdm845: Fix the base addresses of " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 04/12] arm64: dts: qcom: sc7180: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 05/12] arm64: dts: qcom: sc7280: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 06/12] arm64: dts: qcom: sc8280xp: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 07/12] arm64: dts: qcom: sm8150: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 08/12] arm64: dts: qcom: sm8250: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 09/12] arm64: dts: qcom: sm8350: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 10/12] arm64: dts: qcom: sm8450: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 11/12] arm64: dts: qcom: sm6350: " Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 12/12] llcc/edac: Fix the base address used for accessing " Manivannan Sadhasivam
2022-12-07 14:06 ` Manivannan Sadhasivam
2022-12-07 13:59 ` [PATCH 12/12] qcom: " Manivannan Sadhasivam
2022-12-07 16:17 ` Bjorn Andersson [this message]
2022-12-08 9:16 ` [PATCH 00/12] Qcom: LLCC/EDAC: Fix base address used for " Luca Weiss
2022-12-12 8:31 ` Manivannan Sadhasivam
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221207161754.ipiordkogu2fk2dd@builder.lan \
--to=andersson@kernel.org \
--cc=bp@alien8.de \
--cc=james.morse@arm.com \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=mchehab@kernel.org \
--cc=quic_ppareek@quicinc.com \
--cc=quic_saipraka@quicinc.com \
--cc=robh+dt@kernel.org \
--cc=rric@kernel.org \
--cc=stable@vger.kernel.org \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).