From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Bjorn Andersson <andersson@kernel.org>
Cc: bp@alien8.de, mchehab@kernel.org, james.morse@arm.com,
rric@kernel.org, linux-arm-msm@vger.kernel.org,
linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
quic_saipraka@quicinc.com, stable@vger.kernel.org
Subject: Re: [PATCH v6 2/2] EDAC/qcom: Get rid of hardcoded register offsets
Date: Mon, 10 Apr 2023 21:21:11 +0530 [thread overview]
Message-ID: <20230410155111.GB4630@thinkpad> (raw)
In-Reply-To: <20230315222242.hbe3z2d3c2i6psuw@ripper>
On Wed, Mar 15, 2023 at 03:22:42PM -0700, Bjorn Andersson wrote:
> On Tue, Mar 14, 2023 at 12:10:32PM +0530, Manivannan Sadhasivam wrote:
> > The LLCC EDAC register offsets varies between each SoC. Hardcoding the
> > register offsets won't work and will often result in crash due to
> > accessing the wrong locations.
> >
> > Hence, get the register offsets from the LLCC driver matching the
> > individual SoCs.
> >
> > Cc: <stable@vger.kernel.org> # 6.0: 5365cea199c7 ("soc: qcom: llcc: Rename reg_offset structs to reflect LLCC version")
> > Cc: <stable@vger.kernel.org> # 6.0: c13d7d261e36 ("soc: qcom: llcc: Pass LLCC version based register offsets to EDAC driver")
> > Cc: <stable@vger.kernel.org> # 6.0
>
> Why is there three Cc: stable here, is this a new format for Fixes:?
>
This is to specify the dependencies to the stable maintainers during backport.
- Mani
> Regards,
> Bjorn
>
> > Fixes: a6e9d7ef252c ("soc: qcom: llcc: Add configuration data for SM8450 SoC")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > drivers/edac/qcom_edac.c | 116 ++++++++++++++---------------
> > include/linux/soc/qcom/llcc-qcom.h | 6 --
> > 2 files changed, 58 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> > index 9cac49596a6f..c0fd0640d362 100644
> > --- a/drivers/edac/qcom_edac.c
> > +++ b/drivers/edac/qcom_edac.c
> > @@ -21,30 +21,9 @@
> > #define TRP_SYN_REG_CNT 6
> > #define DRP_SYN_REG_CNT 8
> >
> > -#define LLCC_COMMON_STATUS0 0x0003000c
> > #define LLCC_LB_CNT_MASK GENMASK(31, 28)
> > #define LLCC_LB_CNT_SHIFT 28
> >
> > -/* Single & double bit syndrome register offsets */
> > -#define TRP_ECC_SB_ERR_SYN0 0x0002304c
> > -#define TRP_ECC_DB_ERR_SYN0 0x00020370
> > -#define DRP_ECC_SB_ERR_SYN0 0x0004204c
> > -#define DRP_ECC_DB_ERR_SYN0 0x00042070
> > -
> > -/* Error register offsets */
> > -#define TRP_ECC_ERROR_STATUS1 0x00020348
> > -#define TRP_ECC_ERROR_STATUS0 0x00020344
> > -#define DRP_ECC_ERROR_STATUS1 0x00042048
> > -#define DRP_ECC_ERROR_STATUS0 0x00042044
> > -
> > -/* TRP, DRP interrupt register offsets */
> > -#define DRP_INTERRUPT_STATUS 0x00041000
> > -#define TRP_INTERRUPT_0_STATUS 0x00020480
> > -#define DRP_INTERRUPT_CLEAR 0x00041008
> > -#define DRP_ECC_ERROR_CNTR_CLEAR 0x00040004
> > -#define TRP_INTERRUPT_0_CLEAR 0x00020484
> > -#define TRP_ECC_ERROR_CNTR_CLEAR 0x00020440
> > -
> > /* Mask and shift macros */
> > #define ECC_DB_ERR_COUNT_MASK GENMASK(4, 0)
> > #define ECC_DB_ERR_WAYS_MASK GENMASK(31, 16)
> > @@ -60,15 +39,6 @@
> > #define DRP_TRP_INT_CLEAR GENMASK(1, 0)
> > #define DRP_TRP_CNT_CLEAR GENMASK(1, 0)
> >
> > -/* Config registers offsets*/
> > -#define DRP_ECC_ERROR_CFG 0x00040000
> > -
> > -/* Tag RAM, Data RAM interrupt register offsets */
> > -#define CMN_INTERRUPT_0_ENABLE 0x0003001c
> > -#define CMN_INTERRUPT_2_ENABLE 0x0003003c
> > -#define TRP_INTERRUPT_0_ENABLE 0x00020488
> > -#define DRP_INTERRUPT_ENABLE 0x0004100c
> > -
> > #define SB_ERROR_THRESHOLD 0x1
> > #define SB_ERROR_THRESHOLD_SHIFT 24
> > #define SB_DB_TRP_INTERRUPT_ENABLE 0x3
> > @@ -86,9 +56,6 @@ enum {
> > static const struct llcc_edac_reg_data edac_reg_data[] = {
> > [LLCC_DRAM_CE] = {
> > .name = "DRAM Single-bit",
> > - .synd_reg = DRP_ECC_SB_ERR_SYN0,
> > - .count_status_reg = DRP_ECC_ERROR_STATUS1,
> > - .ways_status_reg = DRP_ECC_ERROR_STATUS0,
> > .reg_cnt = DRP_SYN_REG_CNT,
> > .count_mask = ECC_SB_ERR_COUNT_MASK,
> > .ways_mask = ECC_SB_ERR_WAYS_MASK,
> > @@ -96,9 +63,6 @@ static const struct llcc_edac_reg_data edac_reg_data[] = {
> > },
> > [LLCC_DRAM_UE] = {
> > .name = "DRAM Double-bit",
> > - .synd_reg = DRP_ECC_DB_ERR_SYN0,
> > - .count_status_reg = DRP_ECC_ERROR_STATUS1,
> > - .ways_status_reg = DRP_ECC_ERROR_STATUS0,
> > .reg_cnt = DRP_SYN_REG_CNT,
> > .count_mask = ECC_DB_ERR_COUNT_MASK,
> > .ways_mask = ECC_DB_ERR_WAYS_MASK,
> > @@ -106,9 +70,6 @@ static const struct llcc_edac_reg_data edac_reg_data[] = {
> > },
> > [LLCC_TRAM_CE] = {
> > .name = "TRAM Single-bit",
> > - .synd_reg = TRP_ECC_SB_ERR_SYN0,
> > - .count_status_reg = TRP_ECC_ERROR_STATUS1,
> > - .ways_status_reg = TRP_ECC_ERROR_STATUS0,
> > .reg_cnt = TRP_SYN_REG_CNT,
> > .count_mask = ECC_SB_ERR_COUNT_MASK,
> > .ways_mask = ECC_SB_ERR_WAYS_MASK,
> > @@ -116,9 +77,6 @@ static const struct llcc_edac_reg_data edac_reg_data[] = {
> > },
> > [LLCC_TRAM_UE] = {
> > .name = "TRAM Double-bit",
> > - .synd_reg = TRP_ECC_DB_ERR_SYN0,
> > - .count_status_reg = TRP_ECC_ERROR_STATUS1,
> > - .ways_status_reg = TRP_ECC_ERROR_STATUS0,
> > .reg_cnt = TRP_SYN_REG_CNT,
> > .count_mask = ECC_DB_ERR_COUNT_MASK,
> > .ways_mask = ECC_DB_ERR_WAYS_MASK,
> > @@ -126,7 +84,7 @@ static const struct llcc_edac_reg_data edac_reg_data[] = {
> > },
> > };
> >
> > -static int qcom_llcc_core_setup(struct regmap *llcc_bcast_regmap)
> > +static int qcom_llcc_core_setup(struct llcc_drv_data *drv, struct regmap *llcc_bcast_regmap)
> > {
> > u32 sb_err_threshold;
> > int ret;
> > @@ -135,31 +93,31 @@ static int qcom_llcc_core_setup(struct regmap *llcc_bcast_regmap)
> > * Configure interrupt enable registers such that Tag, Data RAM related
> > * interrupts are propagated to interrupt controller for servicing
> > */
> > - ret = regmap_update_bits(llcc_bcast_regmap, CMN_INTERRUPT_2_ENABLE,
> > + ret = regmap_update_bits(llcc_bcast_regmap, drv->edac_reg_offset->cmn_interrupt_2_enable,
> > TRP0_INTERRUPT_ENABLE,
> > TRP0_INTERRUPT_ENABLE);
> > if (ret)
> > return ret;
> >
> > - ret = regmap_update_bits(llcc_bcast_regmap, TRP_INTERRUPT_0_ENABLE,
> > + ret = regmap_update_bits(llcc_bcast_regmap, drv->edac_reg_offset->trp_interrupt_0_enable,
> > SB_DB_TRP_INTERRUPT_ENABLE,
> > SB_DB_TRP_INTERRUPT_ENABLE);
> > if (ret)
> > return ret;
> >
> > sb_err_threshold = (SB_ERROR_THRESHOLD << SB_ERROR_THRESHOLD_SHIFT);
> > - ret = regmap_write(llcc_bcast_regmap, DRP_ECC_ERROR_CFG,
> > + ret = regmap_write(llcc_bcast_regmap, drv->edac_reg_offset->drp_ecc_error_cfg,
> > sb_err_threshold);
> > if (ret)
> > return ret;
> >
> > - ret = regmap_update_bits(llcc_bcast_regmap, CMN_INTERRUPT_2_ENABLE,
> > + ret = regmap_update_bits(llcc_bcast_regmap, drv->edac_reg_offset->cmn_interrupt_2_enable,
> > DRP0_INTERRUPT_ENABLE,
> > DRP0_INTERRUPT_ENABLE);
> > if (ret)
> > return ret;
> >
> > - ret = regmap_write(llcc_bcast_regmap, DRP_INTERRUPT_ENABLE,
> > + ret = regmap_write(llcc_bcast_regmap, drv->edac_reg_offset->drp_interrupt_enable,
> > SB_DB_DRP_INTERRUPT_ENABLE);
> > return ret;
> > }
> > @@ -173,24 +131,28 @@ qcom_llcc_clear_error_status(int err_type, struct llcc_drv_data *drv)
> > switch (err_type) {
> > case LLCC_DRAM_CE:
> > case LLCC_DRAM_UE:
> > - ret = regmap_write(drv->bcast_regmap, DRP_INTERRUPT_CLEAR,
> > + ret = regmap_write(drv->bcast_regmap,
> > + drv->edac_reg_offset->drp_interrupt_clear,
> > DRP_TRP_INT_CLEAR);
> > if (ret)
> > return ret;
> >
> > - ret = regmap_write(drv->bcast_regmap, DRP_ECC_ERROR_CNTR_CLEAR,
> > + ret = regmap_write(drv->bcast_regmap,
> > + drv->edac_reg_offset->drp_ecc_error_cntr_clear,
> > DRP_TRP_CNT_CLEAR);
> > if (ret)
> > return ret;
> > break;
> > case LLCC_TRAM_CE:
> > case LLCC_TRAM_UE:
> > - ret = regmap_write(drv->bcast_regmap, TRP_INTERRUPT_0_CLEAR,
> > + ret = regmap_write(drv->bcast_regmap,
> > + drv->edac_reg_offset->trp_interrupt_0_clear,
> > DRP_TRP_INT_CLEAR);
> > if (ret)
> > return ret;
> >
> > - ret = regmap_write(drv->bcast_regmap, TRP_ECC_ERROR_CNTR_CLEAR,
> > + ret = regmap_write(drv->bcast_regmap,
> > + drv->edac_reg_offset->trp_ecc_error_cntr_clear,
> > DRP_TRP_CNT_CLEAR);
> > if (ret)
> > return ret;
> > @@ -202,16 +164,54 @@ qcom_llcc_clear_error_status(int err_type, struct llcc_drv_data *drv)
> > return ret;
> > }
> >
> > +struct qcom_llcc_syn_regs {
> > + u32 synd_reg;
> > + u32 count_status_reg;
> > + u32 ways_status_reg;
> > +};
> > +
> > +static void get_reg_offsets(struct llcc_drv_data *drv, int err_type,
> > + struct qcom_llcc_syn_regs *syn_regs)
> > +{
> > + const struct llcc_edac_reg_offset *edac_reg_offset = drv->edac_reg_offset;
> > +
> > + switch (err_type) {
> > + case LLCC_DRAM_CE:
> > + syn_regs->synd_reg = edac_reg_offset->drp_ecc_sb_err_syn0;
> > + syn_regs->count_status_reg = edac_reg_offset->drp_ecc_error_status1;
> > + syn_regs->ways_status_reg = edac_reg_offset->drp_ecc_error_status0;
> > + break;
> > + case LLCC_DRAM_UE:
> > + syn_regs->synd_reg = edac_reg_offset->drp_ecc_db_err_syn0;
> > + syn_regs->count_status_reg = edac_reg_offset->drp_ecc_error_status1;
> > + syn_regs->ways_status_reg = edac_reg_offset->drp_ecc_error_status0;
> > + break;
> > + case LLCC_TRAM_CE:
> > + syn_regs->synd_reg = edac_reg_offset->trp_ecc_sb_err_syn0;
> > + syn_regs->count_status_reg = edac_reg_offset->trp_ecc_error_status1;
> > + syn_regs->ways_status_reg = edac_reg_offset->trp_ecc_error_status0;
> > + break;
> > + case LLCC_TRAM_UE:
> > + syn_regs->synd_reg = edac_reg_offset->trp_ecc_db_err_syn0;
> > + syn_regs->count_status_reg = edac_reg_offset->trp_ecc_error_status1;
> > + syn_regs->ways_status_reg = edac_reg_offset->trp_ecc_error_status0;
> > + break;
> > + }
> > +}
> > +
> > /* Dump Syndrome registers data for Tag RAM, Data RAM bit errors*/
> > static int
> > dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> > {
> > struct llcc_edac_reg_data reg_data = edac_reg_data[err_type];
> > + struct qcom_llcc_syn_regs regs = { };
> > int err_cnt, err_ways, ret, i;
> > u32 synd_reg, synd_val;
> >
> > + get_reg_offsets(drv, err_type, ®s);
> > +
> > for (i = 0; i < reg_data.reg_cnt; i++) {
> > - synd_reg = reg_data.synd_reg + (i * 4);
> > + synd_reg = regs.synd_reg + (i * 4);
> > ret = regmap_read(drv->regmap, drv->offsets[bank] + synd_reg,
> > &synd_val);
> > if (ret)
> > @@ -222,7 +222,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> > }
> >
> > ret = regmap_read(drv->regmap,
> > - drv->offsets[bank] + reg_data.count_status_reg,
> > + drv->offsets[bank] + regs.count_status_reg,
> > &err_cnt);
> > if (ret)
> > goto clear;
> > @@ -233,7 +233,7 @@ dump_syn_reg_values(struct llcc_drv_data *drv, u32 bank, int err_type)
> > reg_data.name, err_cnt);
> >
> > ret = regmap_read(drv->regmap,
> > - drv->offsets[bank] + reg_data.ways_status_reg,
> > + drv->offsets[bank] + regs.ways_status_reg,
> > &err_ways);
> > if (ret)
> > goto clear;
> > @@ -296,7 +296,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,
> > + drv->offsets[i] + drv->edac_reg_offset->drp_interrupt_status,
> > &drp_error);
> >
> > if (!ret && (drp_error & SB_ECC_ERROR)) {
> > @@ -312,7 +312,7 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
> > irq_rc = IRQ_HANDLED;
> >
> > ret = regmap_read(drv->regmap,
> > - drv->offsets[i] + TRP_INTERRUPT_0_STATUS,
> > + drv->offsets[i] + drv->edac_reg_offset->trp_interrupt_0_status,
> > &trp_error);
> >
> > if (!ret && (trp_error & SB_ECC_ERROR)) {
> > @@ -339,7 +339,7 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
> > int ecc_irq;
> > int rc;
> >
> > - rc = qcom_llcc_core_setup(llcc_driv_data->bcast_regmap);
> > + rc = qcom_llcc_core_setup(llcc_driv_data, llcc_driv_data->bcast_regmap);
> > if (rc)
> > return rc;
> >
> > diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
> > index ad1fd718169d..3a2ea97e3d36 100644
> > --- a/include/linux/soc/qcom/llcc-qcom.h
> > +++ b/include/linux/soc/qcom/llcc-qcom.h
> > @@ -69,9 +69,6 @@ struct llcc_slice_desc {
> > /**
> > * struct llcc_edac_reg_data - llcc edac registers data for each error type
> > * @name: Name of the error
> > - * @synd_reg: Syndrome register address
> > - * @count_status_reg: Status register address to read the error count
> > - * @ways_status_reg: Status register address to read the error ways
> > * @reg_cnt: Number of registers
> > * @count_mask: Mask value to get the error count
> > * @ways_mask: Mask value to get the error ways
> > @@ -80,9 +77,6 @@ struct llcc_slice_desc {
> > */
> > struct llcc_edac_reg_data {
> > char *name;
> > - u64 synd_reg;
> > - u64 count_status_reg;
> > - u64 ways_status_reg;
> > u32 reg_cnt;
> > u32 count_mask;
> > u32 ways_mask;
> > --
> > 2.25.1
> >
--
மணிவண்ணன் சதாசிவம்
next prev parent reply other threads:[~2023-04-10 15:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-14 6:40 [PATCH v6 0/2] Fix crash when using Qcom LLCC/EDAC drivers Manivannan Sadhasivam
2023-03-14 6:40 ` [PATCH v6 1/2] EDAC/qcom: Remove extra error no assignment in qcom_llcc_core_setup() Manivannan Sadhasivam
2023-03-14 6:40 ` [PATCH v6 2/2] EDAC/qcom: Get rid of hardcoded register offsets Manivannan Sadhasivam
2023-03-15 22:22 ` Bjorn Andersson
2023-04-10 15:51 ` Manivannan Sadhasivam [this message]
2023-04-10 18:32 ` Greg KH
2023-05-11 16:12 ` Dmitry Baryshkov
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=20230410155111.GB4630@thinkpad \
--to=manivannan.sadhasivam@linaro.org \
--cc=andersson@kernel.org \
--cc=bp@alien8.de \
--cc=james.morse@arm.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-edac@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=quic_saipraka@quicinc.com \
--cc=rric@kernel.org \
--cc=stable@vger.kernel.org \
/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).