Re: [PATCH 3/4] edac: qcom: add LCP/SRP DDR DRAM ECC support for Shikra

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 14 Oct 2026 21:26:18 +0530
Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com> wrote:

> Add qcom_llcc_lcp_setup() to enable LCP/SRP interrupt routing when
> firmware has not pre-configured it, and llcc_lcp_ecc_irq_handler() to
> report per-bank correctable and uncorrectable DDR DRAM ECC errors.
> 
> LCP/SRP setup is skipped when ecc_irq_configured is set (e.g. Shikra,
> where DSF firmware handles IRQ routing). A missing lcp_irq is
> non-fatal; TRP/DRP ECC reporting continues unaffected.
> 
> Signed-off-by: Faiyaz Mohammed <faiyaz.mohammed@oss.qualcomm.com>
Trivial stuff only inline.

> ---
>  drivers/edac/qcom_edac.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 147 insertions(+)
> 
> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index f3da9385ca0d..b4f7b2493650 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c

>  /* Clear the error interrupt and counter registers */
>  static int
>  qcom_llcc_clear_error_status(int err_type, struct llcc_drv_data *drv)
> @@ -334,6 +366,92 @@ static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
>  	llcc_ecc_irq_handler(0, edev_ctl);
>  }
>  
> +/*
> + * Handler for the optional LCP/SRP DDR DRAM ECC interrupt
> + * (ddrss_apps_interrupt[1]).  The LCP/SRP registers live in each LLCC
> + * instance's address space, so iterate over all banks and read status
> + * per-instance.  Broadcast-write the clear after the full loop, mirroring
> + * the TRP/DRP handler pattern.
> + */
> +static irqreturn_t llcc_lcp_ecc_irq_handler(int irq, void *edev_ctl)
> +{
> +	struct edac_device_ctl_info *edac_dev_ctl = edev_ctl;
> +	struct llcc_drv_data *drv = edac_dev_ctl->dev->platform_data;
> +	const struct llcc_edac_reg_offset *reg = drv->edac_reg_offset;
> +	irqreturn_t irq_rc = IRQ_NONE;
> +	u32 intr_status, err_status, clr_reg;
> +	int i, ret;
> +
> +	/* Iterate over banks: LCP/SRP status registers are per-LLCC instance */
> +	for (i = 0; i < drv->num_banks; i++) {
	for (int i = 0;

Fine in modern kernel code and makes scope more explicit.

> +		ret = regmap_read(drv->regmaps[i], reg->lcp_srp_interrupt_status,

I'd use a local variable to hold the regmap pointer.

> +				  &intr_status);
> +		if (ret)

A read fail, without any logging seems unwise as you may have just jumped the
error. 

> +			continue;
> +
> +		intr_status &= LCP_SB_ECC_ERROR | LCP_DB_ECC_ERROR;
> +		if (!intr_status)
> +			continue;
> +
> +		irq_rc = IRQ_HANDLED;
> +
> +		ret = regmap_read(drv->regmaps[i], reg->lcp_srp_ecc_error_status1,
> +				  &err_status);
> +		if (ret) {
> +			edac_printk(KERN_ERR, EDAC_LLCC,
> +				    "LCP/SRP error status read failed for bank%u: %d\n",
> +				    i, ret);
> +			continue;
> +		}
> +
> +		if (intr_status & LCP_DB_ECC_ERROR) {
> +			u32 db_cnt = FIELD_GET(LCP_DB_ERR_COUNT_MASK, err_status);
> +
> +			edac_printk(KERN_CRIT, EDAC_LLCC,
> +				    "LCP/SRP DDR DRAM Double-bit ECC error in bank%u: count=%u status=0x%08x\n",
> +				    i, db_cnt, err_status);
> +			edac_device_handle_ue(edac_dev_ctl, 0, i,
> +					      "LCP/SRP DDR DRAM uncorrectable error");
> +		}
> +
> +		if (intr_status & LCP_SB_ECC_ERROR) {
> +			u32 sb_cnt = FIELD_GET(LCP_SB_ERR_COUNT_MASK, err_status);
> +
> +			edac_printk(KERN_WARNING, EDAC_LLCC,
> +				    "LCP/SRP DDR DRAM Single-bit ECC error in bank%u: count=%u status=0x%08x\n",
> +				    i, sb_cnt, err_status);
> +			edac_device_handle_ce(edac_dev_ctl, 0, i,
> +					      "LCP/SRP DDR DRAM correctable error");
> +		}
> +	}
> +
> +	if (irq_rc != IRQ_HANDLED)
> +		return irq_rc;
> +
> +	clr_reg = reg->lcp_srp_interrupt_clear ?
> +			reg->lcp_srp_interrupt_clear :
> +			reg->lcp_srp_interrupt_status;
> +
> +	if (clr_reg) {
> +		ret = regmap_write(drv->bcast_regmap, clr_reg,
> +				   LCP_SB_ECC_ERROR | LCP_DB_ECC_ERROR);
> +		if (ret)
> +			edac_printk(KERN_ERR, EDAC_LLCC,
> +				    "LCP/SRP interrupt clear failed: %d\n", ret);
> +	}
> +
> +	if (reg->lcp_srp_ecc_error_cntr_clear) {
> +		ret = regmap_write(drv->bcast_regmap,
> +				   reg->lcp_srp_ecc_error_cntr_clear,
> +				   DRP_TRP_CNT_CLEAR);
> +		if (ret)
> +			edac_printk(KERN_ERR, EDAC_LLCC,
> +				    "LCP/SRP counter clear failed: %d\n", ret);
> +	}
> +
> +	return irq_rc;
> +}
> +
>  static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  {
>  	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
> @@ -372,6 +490,8 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  			edac_op_state = EDAC_OPSTATE_INT;
>  			goto irq_done;
>  		}
> +		dev_warn(dev, "failed to request LLCC ECC irq %d: %d, falling back to polling\n",
> +			 ecc_irq, rc);
>  	}
>  
>  	/* Fall back to polling mode otherwise */
> @@ -386,6 +506,33 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  		return rc;
>  	}
>  
> +	/*
> +	 * Set up LCP/SRP DDR DRAM ECC after the EDAC device is published.
> +	 * Skip hardware routing setup if firmware already configured IRQ
> +	 * propagation (irq_configured); only request the IRQ handler.
> +	 * Guard on a non-zero lcp_srp_interrupt_enable offset to confirm
> +	 * this LLCC variant actually has an LCP/SRP block.
> +	 */
> +	if (llcc_driv_data->lcp_irq > 0 &&
> +	    llcc_driv_data->edac_reg_offset->lcp_srp_interrupt_enable) {
> +		if (!llcc_driv_data->ecc_irq_configured) {
> +			rc = qcom_llcc_lcp_setup(llcc_driv_data,
> +						 llcc_driv_data->bcast_regmap);
> +			if (rc) {
> +				dev_warn(dev, "LCP/SRP interrupt setup failed: %d\n", rc);

> +				goto lcp_done;
> +			}
> +		}
> +		rc = devm_request_irq(dev, llcc_driv_data->lcp_irq,
> +				      llcc_lcp_ecc_irq_handler,
> +				      0, "llcc_lcp_ecc", edev_ctl);
> +		if (rc)
> +			dev_warn(dev,
> +				 "failed to request LCP ECC irq %d: %d, DDR DRAM ECC reporting disabled\n",
> +				 llcc_driv_data->lcp_irq, rc);
> +	}
> +lcp_done:
> +	rc = 0;
>  	platform_set_drvdata(pdev, edev_ctl);
>  
>  	return rc;
>