Re: [PATCH] watchdog: qcom: Remove incorrect usage of QCOM_WDT_ENABLE_IRQ

Re: [PATCH] watchdog: qcom: Remove incorrect usage of QCOM_WDT_ENABLE_IRQ

[Guenter Roeck]

On 1/26/21 7:02 AM, Sai Prakash Ranjan wrote:
> As per register documentation, QCOM_WDT_ENABLE_IRQ which is BIT(1)
> of watchdog control register is wakeup interrupt enable bit and
> not related to bark interrupt at all, BIT(0) is used for that.
> So remove incorrect usage of this bit when supporting bark irq for
> pre-timeout notification. Currently with this bit set and bark
> interrupt specified, pre-timeout notification and/or watchdog
> reset/bite does not occur.
> 
> Fixes: 36375491a439 ("watchdog: qcom: support pre-timeout when the bark irq is available")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>

Assuming that pretimeout _does_ work with this patch applied,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Reading the conversations from when qcom pre-timeout support was
> added [1], Bjorn already had mentioned it was not right to touch this
> bit, not sure which SoC the pre-timeout was tested on at that time,
> but I have tested this on SDM845, SM8150, SC7180 and watchdog bark
> and bite does not occur with enabling this bit with the bark irq
> specified in DT.
> 
> [1] https://lore.kernel.org/linux-watchdog/20190906174009.GC11938@tuxbook-pro/
> 
> ---
>  drivers/watchdog/qcom-wdt.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 7cf0f2ec649b..e38a87ffe5f5 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -22,7 +22,6 @@ enum wdt_reg {
>  };
>  
>  #define QCOM_WDT_ENABLE		BIT(0)
> -#define QCOM_WDT_ENABLE_IRQ	BIT(1)
>  
>  static const u32 reg_offset_data_apcs_tmr[] = {
>  	[WDT_RST] = 0x38,
> @@ -63,16 +62,6 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>  	return container_of(wdd, struct qcom_wdt, wdd);
>  }
>  
> -static inline int qcom_get_enable(struct watchdog_device *wdd)
> -{
> -	int enable = QCOM_WDT_ENABLE;
> -
> -	if (wdd->pretimeout)
> -		enable |= QCOM_WDT_ENABLE_IRQ;
> -
> -	return enable;
> -}
> -
>  static irqreturn_t qcom_wdt_isr(int irq, void *arg)
>  {
>  	struct watchdog_device *wdd = arg;
> @@ -91,7 +80,7 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
>  	writel(1, wdt_addr(wdt, WDT_RST));
>  	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> -	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
> +	writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
>  	return 0;
>  }
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] watchdog: qcom: Remove incorrect usage of QCOM_WDT_ENABLE_IRQ

[Stephen Boyd]

Quoting Sai Prakash Ranjan (2021-01-26 07:02:41)
> As per register documentation, QCOM_WDT_ENABLE_IRQ which is BIT(1)
> of watchdog control register is wakeup interrupt enable bit and
> not related to bark interrupt at all, BIT(0) is used for that.
> So remove incorrect usage of this bit when supporting bark irq for
> pre-timeout notification. Currently with this bit set and bark
> interrupt specified, pre-timeout notification and/or watchdog
> reset/bite does not occur.

It looks like the QCOM_WDT_ENABLE_IRQ bit is to catch a problem where a
pending irq is unmasked but the watchdog irq isn't handled in time? So
some sort of irq storm?

> 
> Fixes: 36375491a439 ("watchdog: qcom: support pre-timeout when the bark irq is available")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] watchdog: qcom: Remove incorrect usage of QCOM_WDT_ENABLE_IRQ

[Jorge Ramirez-Ortiz, Foundries]

On 26/01/21, Sai Prakash Ranjan wrote:
> As per register documentation, QCOM_WDT_ENABLE_IRQ which is BIT(1)
> of watchdog control register is wakeup interrupt enable bit and
> not related to bark interrupt at all, BIT(0) is used for that.
> So remove incorrect usage of this bit when supporting bark irq for
> pre-timeout notification. Currently with this bit set and bark
> interrupt specified, pre-timeout notification and/or watchdog
> reset/bite does not occur.
> 
> Fixes: 36375491a439 ("watchdog: qcom: support pre-timeout when the bark irq is available")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
> 
> Reading the conversations from when qcom pre-timeout support was
> added [1], Bjorn already had mentioned it was not right to touch this
> bit, not sure which SoC the pre-timeout was tested on at that time,
> but I have tested this on SDM845, SM8150, SC7180 and watchdog bark
> and bite does not occur with enabling this bit with the bark irq
> specified in DT.

this was tested on QCS404. have you validated there? unfortunately I
no longer have access to that hardware or the documentation


> 
> [1] https://lore.kernel.org/linux-watchdog/20190906174009.GC11938@tuxbook-pro/
> 
> ---
>  drivers/watchdog/qcom-wdt.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 7cf0f2ec649b..e38a87ffe5f5 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -22,7 +22,6 @@ enum wdt_reg {
>  };
>  
>  #define QCOM_WDT_ENABLE		BIT(0)
> -#define QCOM_WDT_ENABLE_IRQ	BIT(1)
>  
>  static const u32 reg_offset_data_apcs_tmr[] = {
>  	[WDT_RST] = 0x38,
> @@ -63,16 +62,6 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>  	return container_of(wdd, struct qcom_wdt, wdd);
>  }
>  
> -static inline int qcom_get_enable(struct watchdog_device *wdd)
> -{
> -	int enable = QCOM_WDT_ENABLE;
> -
> -	if (wdd->pretimeout)
> -		enable |= QCOM_WDT_ENABLE_IRQ;
> -
> -	return enable;
> -}
> -
>  static irqreturn_t qcom_wdt_isr(int irq, void *arg)
>  {
>  	struct watchdog_device *wdd = arg;
> @@ -91,7 +80,7 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
>  	writel(1, wdt_addr(wdt, WDT_RST));
>  	writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>  	writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> -	writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
> +	writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
>  	return 0;
>  }
>  
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] watchdog: qcom: Remove incorrect usage of QCOM_WDT_ENABLE_IRQ

[Jorge Ramirez-Ortiz, Foundries]

On 28/01/21, Sai Prakash Ranjan wrote:
> On 2021-01-28 13:49, Jorge Ramirez-Ortiz, Foundries wrote:
> > On 26/01/21, Sai Prakash Ranjan wrote:
> > > As per register documentation, QCOM_WDT_ENABLE_IRQ which is BIT(1)
> > > of watchdog control register is wakeup interrupt enable bit and
> > > not related to bark interrupt at all, BIT(0) is used for that.
> > > So remove incorrect usage of this bit when supporting bark irq for
> > > pre-timeout notification. Currently with this bit set and bark
> > > interrupt specified, pre-timeout notification and/or watchdog
> > > reset/bite does not occur.
> > > 
> > > Fixes: 36375491a439 ("watchdog: qcom: support pre-timeout when the
> > > bark irq is available")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> > > ---
> > > 
> > > Reading the conversations from when qcom pre-timeout support was
> > > added [1], Bjorn already had mentioned it was not right to touch this
> > > bit, not sure which SoC the pre-timeout was tested on at that time,
> > > but I have tested this on SDM845, SM8150, SC7180 and watchdog bark
> > > and bite does not occur with enabling this bit with the bark irq
> > > specified in DT.
> > 
> > this was tested on QCS404. have you validated there? unfortunately I
> > no longer have access to that hardware or the documentation
> > 
> 
> I didn't validate on qcs404 yet since I didn't have access to it.
> But now that you mention it, let me arrange for a setup and test it
> there as well. Note: I did not see bark irq entry in upstream qcs404
> dtsi, so you must have had some local change when you tested?

TBH I dont quite remember. I suppose that if those with access to the
documents and hardware are OK with this change then it shouldnt cause
regressions (I just cant check from my end)


> 
> Thanks,
> Sai
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel