diff mbox series

[v2,13/13] qcom: llcc/edac: Support polling mode for ECC handling

Message ID 20221212123311.146261-14-manivannan.sadhasivam@linaro.org (mailing list archive)
State New, archived
Headers show
Series Qcom: LLCC/EDAC: Fix base address used for LLCC banks | expand

Commit Message

Manivannan Sadhasivam Dec. 12, 2022, 12:33 p.m. UTC 
Not all Qcom platforms support IRQ mode for ECC handling. For those
platforms, the current EDAC driver will not be probed due to missing ECC
IRQ in devicetree.

So add support for polling mode so that the EDAC driver can be used on all
Qcom platforms supporting LLCC.

The polling delay of 5000ms is chosed based on Qcom downstream/vendor
driver.

Reported-by: Luca Weiss <luca.weiss@fairphone.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/edac/qcom_edac.c     | 37 +++++++++++++++++++++++++-----------
 drivers/soc/qcom/llcc-qcom.c | 13 ++++++-------
 2 files changed, 32 insertions(+), 18 deletions(-)

Comments

Luca Weiss Dec. 12, 2022, 3:53 p.m. UTC | #1 
Hi Manivannan,

On Mon Dec 12, 2022 at 1:33 PM CET, Manivannan Sadhasivam wrote:
> Not all Qcom platforms support IRQ mode for ECC handling. For those
> platforms, the current EDAC driver will not be probed due to missing ECC
> IRQ in devicetree.
>
> So add support for polling mode so that the EDAC driver can be used on all
> Qcom platforms supporting LLCC.
>
> The polling delay of 5000ms is chosed based on Qcom downstream/vendor
> driver.

I think it does work for me on SM6350, I get this in dmesg:

[    0.054608] EDAC MC: Ver: 3.0.0
[    0.273913] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (POLLED)

What I've noticed though is that the 5000ms poll you defined in the
driver doesn't seem to be reflected at runtime? Or am I looking at
different things?

/ # cat /sys/devices/system/edac/qcom-llcc/poll_msec 
1000

Regards
Luca

>
> Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/edac/qcom_edac.c     | 37 +++++++++++++++++++++++++-----------
>  drivers/soc/qcom/llcc-qcom.c | 13 ++++++-------
>  2 files changed, 32 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> index 5be93577fc03..f7afb5375293 100644
> --- a/drivers/edac/qcom_edac.c
> +++ b/drivers/edac/qcom_edac.c
> @@ -76,6 +76,8 @@
>  #define DRP0_INTERRUPT_ENABLE           BIT(6)
>  #define SB_DB_DRP_INTERRUPT_ENABLE      0x3
>  
> +#define ECC_POLL_MSEC			5000
> +
>  enum {
>  	LLCC_DRAM_CE = 0,
>  	LLCC_DRAM_UE,
> @@ -283,8 +285,7 @@ dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 bank)
>  	return ret;
>  }
>  
> -static irqreturn_t
> -llcc_ecc_irq_handler(int irq, void *edev_ctl)
> +static irqreturn_t llcc_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->pvt_info;
> @@ -328,6 +329,11 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
>  	return irq_rc;
>  }
>  
> +static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
> +{
> +	llcc_ecc_irq_handler(0, edev_ctl);
> +}
> +
>  static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  {
>  	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
> @@ -356,22 +362,31 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
>  	edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
>  	edev_ctl->pvt_info = llcc_driv_data;
>  
> +	/* Check if LLCC driver has passed ECC IRQ */
> +	ecc_irq = llcc_driv_data->ecc_irq;
> +	if (ecc_irq > 0) {
> +		/* Use interrupt mode if IRQ is available */
> +		edac_op_state = EDAC_OPSTATE_INT;
> +	} else {
> +		/* Fall back to polling mode otherwise */
> +		edac_op_state = EDAC_OPSTATE_POLL;
> +		edev_ctl->poll_msec = ECC_POLL_MSEC;
> +		edev_ctl->edac_check = llcc_ecc_check;
> +	}
> +
>  	rc = edac_device_add_device(edev_ctl);
>  	if (rc)
>  		goto out_mem;
>  
>  	platform_set_drvdata(pdev, edev_ctl);
>  
> -	/* Request for ecc irq */
> -	ecc_irq = llcc_driv_data->ecc_irq;
> -	if (ecc_irq < 0) {
> -		rc = -ENODEV;
> -		goto out_dev;
> -	}
> -	rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
> +	/* Request ECC IRQ if available */
> +	if (ecc_irq > 0) {
> +		rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
>  			      IRQF_TRIGGER_HIGH, "llcc_ecc", edev_ctl);
> -	if (rc)
> -		goto out_dev;
> +		if (rc)
> +			goto out_dev;
> +	}
>  
>  	return rc;
>  
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index a29f22dad7fa..e044e6756415 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -1011,13 +1011,12 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  		goto err;
>  
>  	drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
> -	if (drv_data->ecc_irq >= 0) {
> -		llcc_edac = platform_device_register_data(&pdev->dev,
> -						"qcom_llcc_edac", -1, drv_data,
> -						sizeof(*drv_data));
> -		if (IS_ERR(llcc_edac))
> -			dev_err(dev, "Failed to register llcc edac driver\n");
> -	}
> +
> +	llcc_edac = platform_device_register_data(&pdev->dev,
> +					"qcom_llcc_edac", -1, drv_data,
> +					sizeof(*drv_data));
> +	if (IS_ERR(llcc_edac))
> +		dev_err(dev, "Failed to register llcc edac driver\n");
>  
>  	return 0;
>  err:
> -- 
> 2.25.1
Manivannan Sadhasivam Dec. 12, 2022, 4:16 p.m. UTC | #2 
On Mon, Dec 12, 2022 at 04:53:49PM +0100, Luca Weiss wrote:
> Hi Manivannan,
> 
> On Mon Dec 12, 2022 at 1:33 PM CET, Manivannan Sadhasivam wrote:
> > Not all Qcom platforms support IRQ mode for ECC handling. For those
> > platforms, the current EDAC driver will not be probed due to missing ECC
> > IRQ in devicetree.
> >
> > So add support for polling mode so that the EDAC driver can be used on all
> > Qcom platforms supporting LLCC.
> >
> > The polling delay of 5000ms is chosed based on Qcom downstream/vendor
> > driver.
> 
> I think it does work for me on SM6350, I get this in dmesg:
> 
> [    0.054608] EDAC MC: Ver: 3.0.0
> [    0.273913] EDAC DEVICE0: Giving out device to module qcom_llcc_edac controller llcc: DEV qcom_llcc_edac (POLLED)
> 
> What I've noticed though is that the 5000ms poll you defined in the
> driver doesn't seem to be reflected at runtime? Or am I looking at
> different things?
> 
> / # cat /sys/devices/system/edac/qcom-llcc/poll_msec 
> 1000
> 

Oops... Looks like the interval is hardcoded in edac_device driver:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/edac/edac_device.c#n449

This clearly needs to be fixed. Will do so in next version.

Thanks a lot for testing the series. I will also consider this as your t-b tag
for SM6350 and driver patches.

Thanks,
Mani

> Regards
> Luca
> 
> >
> > Reported-by: Luca Weiss <luca.weiss@fairphone.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/edac/qcom_edac.c     | 37 +++++++++++++++++++++++++-----------
> >  drivers/soc/qcom/llcc-qcom.c | 13 ++++++-------
> >  2 files changed, 32 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
> > index 5be93577fc03..f7afb5375293 100644
> > --- a/drivers/edac/qcom_edac.c
> > +++ b/drivers/edac/qcom_edac.c
> > @@ -76,6 +76,8 @@
> >  #define DRP0_INTERRUPT_ENABLE           BIT(6)
> >  #define SB_DB_DRP_INTERRUPT_ENABLE      0x3
> >  
> > +#define ECC_POLL_MSEC			5000
> > +
> >  enum {
> >  	LLCC_DRAM_CE = 0,
> >  	LLCC_DRAM_UE,
> > @@ -283,8 +285,7 @@ dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 bank)
> >  	return ret;
> >  }
> >  
> > -static irqreturn_t
> > -llcc_ecc_irq_handler(int irq, void *edev_ctl)
> > +static irqreturn_t llcc_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->pvt_info;
> > @@ -328,6 +329,11 @@ llcc_ecc_irq_handler(int irq, void *edev_ctl)
> >  	return irq_rc;
> >  }
> >  
> > +static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
> > +{
> > +	llcc_ecc_irq_handler(0, edev_ctl);
> > +}
> > +
> >  static int qcom_llcc_edac_probe(struct platform_device *pdev)
> >  {
> >  	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
> > @@ -356,22 +362,31 @@ static int qcom_llcc_edac_probe(struct platform_device *pdev)
> >  	edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
> >  	edev_ctl->pvt_info = llcc_driv_data;
> >  
> > +	/* Check if LLCC driver has passed ECC IRQ */
> > +	ecc_irq = llcc_driv_data->ecc_irq;
> > +	if (ecc_irq > 0) {
> > +		/* Use interrupt mode if IRQ is available */
> > +		edac_op_state = EDAC_OPSTATE_INT;
> > +	} else {
> > +		/* Fall back to polling mode otherwise */
> > +		edac_op_state = EDAC_OPSTATE_POLL;
> > +		edev_ctl->poll_msec = ECC_POLL_MSEC;
> > +		edev_ctl->edac_check = llcc_ecc_check;
> > +	}
> > +
> >  	rc = edac_device_add_device(edev_ctl);
> >  	if (rc)
> >  		goto out_mem;
> >  
> >  	platform_set_drvdata(pdev, edev_ctl);
> >  
> > -	/* Request for ecc irq */
> > -	ecc_irq = llcc_driv_data->ecc_irq;
> > -	if (ecc_irq < 0) {
> > -		rc = -ENODEV;
> > -		goto out_dev;
> > -	}
> > -	rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
> > +	/* Request ECC IRQ if available */
> > +	if (ecc_irq > 0) {
> > +		rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
> >  			      IRQF_TRIGGER_HIGH, "llcc_ecc", edev_ctl);
> > -	if (rc)
> > -		goto out_dev;
> > +		if (rc)
> > +			goto out_dev;
> > +	}
> >  
> >  	return rc;
> >  
> > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> > index a29f22dad7fa..e044e6756415 100644
> > --- a/drivers/soc/qcom/llcc-qcom.c
> > +++ b/drivers/soc/qcom/llcc-qcom.c
> > @@ -1011,13 +1011,12 @@ static int qcom_llcc_probe(struct platform_device *pdev)
> >  		goto err;
> >  
> >  	drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
> > -	if (drv_data->ecc_irq >= 0) {
> > -		llcc_edac = platform_device_register_data(&pdev->dev,
> > -						"qcom_llcc_edac", -1, drv_data,
> > -						sizeof(*drv_data));
> > -		if (IS_ERR(llcc_edac))
> > -			dev_err(dev, "Failed to register llcc edac driver\n");
> > -	}
> > +
> > +	llcc_edac = platform_device_register_data(&pdev->dev,
> > +					"qcom_llcc_edac", -1, drv_data,
> > +					sizeof(*drv_data));
> > +	if (IS_ERR(llcc_edac))
> > +		dev_err(dev, "Failed to register llcc edac driver\n");
> >  
> >  	return 0;
> >  err:
> > -- 
> > 2.25.1
>
diff mbox series

Patch

diff --git a/drivers/edac/qcom_edac.c b/drivers/edac/qcom_edac.c
index 5be93577fc03..f7afb5375293 100644
--- a/drivers/edac/qcom_edac.c
+++ b/drivers/edac/qcom_edac.c
@@ -76,6 +76,8 @@ 
 #define DRP0_INTERRUPT_ENABLE           BIT(6)
 #define SB_DB_DRP_INTERRUPT_ENABLE      0x3
 
+#define ECC_POLL_MSEC			5000
+
 enum {
 	LLCC_DRAM_CE = 0,
 	LLCC_DRAM_UE,
@@ -283,8 +285,7 @@  dump_syn_reg(struct edac_device_ctl_info *edev_ctl, int err_type, u32 bank)
 	return ret;
 }
 
-static irqreturn_t
-llcc_ecc_irq_handler(int irq, void *edev_ctl)
+static irqreturn_t llcc_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->pvt_info;
@@ -328,6 +329,11 @@  llcc_ecc_irq_handler(int irq, void *edev_ctl)
 	return irq_rc;
 }
 
+static void llcc_ecc_check(struct edac_device_ctl_info *edev_ctl)
+{
+	llcc_ecc_irq_handler(0, edev_ctl);
+}
+
 static int qcom_llcc_edac_probe(struct platform_device *pdev)
 {
 	struct llcc_drv_data *llcc_driv_data = pdev->dev.platform_data;
@@ -356,22 +362,31 @@  static int qcom_llcc_edac_probe(struct platform_device *pdev)
 	edev_ctl->panic_on_ue = LLCC_ERP_PANIC_ON_UE;
 	edev_ctl->pvt_info = llcc_driv_data;
 
+	/* Check if LLCC driver has passed ECC IRQ */
+	ecc_irq = llcc_driv_data->ecc_irq;
+	if (ecc_irq > 0) {
+		/* Use interrupt mode if IRQ is available */
+		edac_op_state = EDAC_OPSTATE_INT;
+	} else {
+		/* Fall back to polling mode otherwise */
+		edac_op_state = EDAC_OPSTATE_POLL;
+		edev_ctl->poll_msec = ECC_POLL_MSEC;
+		edev_ctl->edac_check = llcc_ecc_check;
+	}
+
 	rc = edac_device_add_device(edev_ctl);
 	if (rc)
 		goto out_mem;
 
 	platform_set_drvdata(pdev, edev_ctl);
 
-	/* Request for ecc irq */
-	ecc_irq = llcc_driv_data->ecc_irq;
-	if (ecc_irq < 0) {
-		rc = -ENODEV;
-		goto out_dev;
-	}
-	rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
+	/* Request ECC IRQ if available */
+	if (ecc_irq > 0) {
+		rc = devm_request_irq(dev, ecc_irq, llcc_ecc_irq_handler,
 			      IRQF_TRIGGER_HIGH, "llcc_ecc", edev_ctl);
-	if (rc)
-		goto out_dev;
+		if (rc)
+			goto out_dev;
+	}
 
 	return rc;
 
diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index a29f22dad7fa..e044e6756415 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -1011,13 +1011,12 @@  static int qcom_llcc_probe(struct platform_device *pdev)
 		goto err;
 
 	drv_data->ecc_irq = platform_get_irq_optional(pdev, 0);
-	if (drv_data->ecc_irq >= 0) {
-		llcc_edac = platform_device_register_data(&pdev->dev,
-						"qcom_llcc_edac", -1, drv_data,
-						sizeof(*drv_data));
-		if (IS_ERR(llcc_edac))
-			dev_err(dev, "Failed to register llcc edac driver\n");
-	}
+
+	llcc_edac = platform_device_register_data(&pdev->dev,
+					"qcom_llcc_edac", -1, drv_data,
+					sizeof(*drv_data));
+	if (IS_ERR(llcc_edac))
+		dev_err(dev, "Failed to register llcc edac driver\n");
 
 	return 0;
 err: