From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935331AbcAUK3Y (ORCPT ); Thu, 21 Jan 2016 05:29:24 -0500 Received: from mailgw02.mediatek.com ([210.61.82.184]:56551 "EHLO mailgw02.hq.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S935239AbcAUK2b (ORCPT ); Thu, 21 Jan 2016 05:28:31 -0500 Message-ID: <1453372104.8457.12.camel@mtksdaap41> Subject: Re: [PATCH] soc: mediatek: PMIC wrap: clear the STAUPD_TRIG bit of WDT_SRC_EN From: Henry Chen To: Matthias Brugger CC: Daniel Kurtz , Sascha Hauer , "linux-arm-kernel@lists.infradead.org" , "moderated list:ARM/Mediatek SoC support" , "linux-kernel@vger.kernel.org" Date: Thu, 21 Jan 2016 18:28:24 +0800 In-Reply-To: <56A0A249.30003@gmail.com> References: <1453265564-16379-1-git-send-email-henryc.chen@mediatek.com> <1453288312.8457.9.camel@mtksdaap41> <56A0A249.30003@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2016-01-21 at 10:18 +0100, Matthias Brugger wrote: > > On 20/01/16 12:11, Henry Chen wrote: > > On Wed, 2016-01-20 at 02:32 -0800, Daniel Kurtz wrote: > >> On Tue, Jan 19, 2016 at 8:52 PM, Henry Chen wrote: > >>> Since STAUPD interrupts aren't handled on mt8173, disable watchdog timeout > >>> monitor of STAUPD to avoid WDT_INT triggered by STAUPD. > >>> > >>> Signed-off-by: Henry Chen > >> > >>> --- > >>> drivers/soc/mediatek/mtk-pmic-wrap.c | 19 +++++++++++++++++-- > >>> 1 file changed, 17 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> index af919b1..998f561 100644 > >>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> @@ -60,6 +60,15 @@ > >>> #define PWRAP_MAN_CMD_OP_OUTD (0x9 << 8) > >>> #define PWRAP_MAN_CMD_OP_OUTQ (0xa << 8) > >>> > >>> +/* macro for Watch Dog Timer Source */ > >>> +#define PWRAP_WDT_SRC_EN_STAUPD_TRIG (1 << 25) > >>> +#define PWRAP_WDT_SRC_EN_HARB_STAUPD_DLE (1 << 20) > >>> +#define PWRAP_WDT_SRC_EN_HARB_STAUPD_ALE (1 << 6) > >>> +#define PWRAP_WDT_SRC_MASK_ALL 0xffffffff > >>> +#define PWRAP_WDT_SRC_MASK_NO_STAUPD ~(PWRAP_WDT_SRC_EN_STAUPD_TRIG | \ > >>> + PWRAP_WDT_SRC_EN_HARB_STAUPD_DLE | \ > >>> + PWRAP_WDT_SRC_EN_HARB_STAUPD_ALE) > >>> + > >>> /* macro for slave device wrapper registers */ > >>> #define PWRAP_DEW_BASE 0xbc00 > >>> #define PWRAP_DEW_EVENT_OUT_EN (PWRAP_DEW_BASE + 0x0) > >>> @@ -830,7 +839,7 @@ MODULE_DEVICE_TABLE(of, of_pwrap_match_tbl); > >>> > >>> static int pwrap_probe(struct platform_device *pdev) > >>> { > >>> - int ret, irq; > >>> + int ret, irq, wdt_src; > >>> struct pmic_wrapper *wrp; > >>> struct device_node *np = pdev->dev.of_node; > >>> const struct of_device_id *of_id = > >>> @@ -920,7 +929,13 @@ static int pwrap_probe(struct platform_device *pdev) > >>> > >>> /* Initialize watchdog, may not be done by the bootloader */ > >>> pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT); > >>> - pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN); > >>> + /* > >>> + * Since STAUPD was not used on mt8173 platform, > >>> + * so STAUPD of WDT_SRC which should be turned off > >>> + */ > >> > >> It is a little awkward that "!pwrap_is_mt8135(wrp)" means mt8173. Is > >> this always true? > >> Why aren't we clearing STAUPD for mt8135 too? > >> I assume it is because mt8135 does not define this bit? > >> What about other devices that are not mt8135 or mt8173? > > > > MT8135 used STAUPD because it have pwrap event pin and it was different > > with mt8173. pwrap event have been removed on mt8173, so it no need to > > set STAUPD. > > > > I think we already had this discussion once. It seems as if there are > different versions of pmic-wrapper. An older version used by mt8135 and > e.g. mt6589 and a newer one used by mt8173. > > So I agree with Daniel that we should check pwrap_is_mt8173 instead. > Apart from that the patch looks good. > Ok, I will send a patch to change that, thanks. Henry > Regards, > Matthias > > > Henry > > > >> Perhaps change the comment to: > >> "This driver does not use STAUPD, so clear this WDT SRC for all > >> devices for which it exists to avoid unhandled interrupts" > >> > > > >> Other than that, this patch is: > >> Reviewed-by: Daniel Kurtz > >> > >>> + wdt_src = pwrap_is_mt8135(wrp) ? > >>> + PWRAP_WDT_SRC_MASK_ALL : PWRAP_WDT_SRC_MASK_NO_STAUPD; > >>> + pwrap_writel(wrp, wdt_src, PWRAP_WDT_SRC_EN); > >>> pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN); > >>> pwrap_writel(wrp, ~((1 << 31) | (1 << 1)), PWRAP_INT_EN); > >>> > >>> -- > >>> 1.8.1.1.dirty > >>> > > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Henry Chen Subject: Re: [PATCH] soc: mediatek: PMIC wrap: clear the STAUPD_TRIG bit of WDT_SRC_EN Date: Thu, 21 Jan 2016 18:28:24 +0800 Message-ID: <1453372104.8457.12.camel@mtksdaap41> References: <1453265564-16379-1-git-send-email-henryc.chen@mediatek.com> <1453288312.8457.9.camel@mtksdaap41> <56A0A249.30003@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56A0A249.30003-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+glpam-linux-mediatek=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Matthias Brugger Cc: Sascha Hauer , "moderated list:ARM/Mediatek SoC support" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-mediatek@lists.infradead.org On Thu, 2016-01-21 at 10:18 +0100, Matthias Brugger wrote: > > On 20/01/16 12:11, Henry Chen wrote: > > On Wed, 2016-01-20 at 02:32 -0800, Daniel Kurtz wrote: > >> On Tue, Jan 19, 2016 at 8:52 PM, Henry Chen wrote: > >>> Since STAUPD interrupts aren't handled on mt8173, disable watchdog timeout > >>> monitor of STAUPD to avoid WDT_INT triggered by STAUPD. > >>> > >>> Signed-off-by: Henry Chen > >> > >>> --- > >>> drivers/soc/mediatek/mtk-pmic-wrap.c | 19 +++++++++++++++++-- > >>> 1 file changed, 17 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> index af919b1..998f561 100644 > >>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> @@ -60,6 +60,15 @@ > >>> #define PWRAP_MAN_CMD_OP_OUTD (0x9 << 8) > >>> #define PWRAP_MAN_CMD_OP_OUTQ (0xa << 8) > >>> > >>> +/* macro for Watch Dog Timer Source */ > >>> +#define PWRAP_WDT_SRC_EN_STAUPD_TRIG (1 << 25) > >>> +#define PWRAP_WDT_SRC_EN_HARB_STAUPD_DLE (1 << 20) > >>> +#define PWRAP_WDT_SRC_EN_HARB_STAUPD_ALE (1 << 6) > >>> +#define PWRAP_WDT_SRC_MASK_ALL 0xffffffff > >>> +#define PWRAP_WDT_SRC_MASK_NO_STAUPD ~(PWRAP_WDT_SRC_EN_STAUPD_TRIG | \ > >>> + PWRAP_WDT_SRC_EN_HARB_STAUPD_DLE | \ > >>> + PWRAP_WDT_SRC_EN_HARB_STAUPD_ALE) > >>> + > >>> /* macro for slave device wrapper registers */ > >>> #define PWRAP_DEW_BASE 0xbc00 > >>> #define PWRAP_DEW_EVENT_OUT_EN (PWRAP_DEW_BASE + 0x0) > >>> @@ -830,7 +839,7 @@ MODULE_DEVICE_TABLE(of, of_pwrap_match_tbl); > >>> > >>> static int pwrap_probe(struct platform_device *pdev) > >>> { > >>> - int ret, irq; > >>> + int ret, irq, wdt_src; > >>> struct pmic_wrapper *wrp; > >>> struct device_node *np = pdev->dev.of_node; > >>> const struct of_device_id *of_id = > >>> @@ -920,7 +929,13 @@ static int pwrap_probe(struct platform_device *pdev) > >>> > >>> /* Initialize watchdog, may not be done by the bootloader */ > >>> pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT); > >>> - pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN); > >>> + /* > >>> + * Since STAUPD was not used on mt8173 platform, > >>> + * so STAUPD of WDT_SRC which should be turned off > >>> + */ > >> > >> It is a little awkward that "!pwrap_is_mt8135(wrp)" means mt8173. Is > >> this always true? > >> Why aren't we clearing STAUPD for mt8135 too? > >> I assume it is because mt8135 does not define this bit? > >> What about other devices that are not mt8135 or mt8173? > > > > MT8135 used STAUPD because it have pwrap event pin and it was different > > with mt8173. pwrap event have been removed on mt8173, so it no need to > > set STAUPD. > > > > I think we already had this discussion once. It seems as if there are > different versions of pmic-wrapper. An older version used by mt8135 and > e.g. mt6589 and a newer one used by mt8173. > > So I agree with Daniel that we should check pwrap_is_mt8173 instead. > Apart from that the patch looks good. > Ok, I will send a patch to change that, thanks. Henry > Regards, > Matthias > > > Henry > > > >> Perhaps change the comment to: > >> "This driver does not use STAUPD, so clear this WDT SRC for all > >> devices for which it exists to avoid unhandled interrupts" > >> > > > >> Other than that, this patch is: > >> Reviewed-by: Daniel Kurtz > >> > >>> + wdt_src = pwrap_is_mt8135(wrp) ? > >>> + PWRAP_WDT_SRC_MASK_ALL : PWRAP_WDT_SRC_MASK_NO_STAUPD; > >>> + pwrap_writel(wrp, wdt_src, PWRAP_WDT_SRC_EN); > >>> pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN); > >>> pwrap_writel(wrp, ~((1 << 31) | (1 << 1)), PWRAP_INT_EN); > >>> > >>> -- > >>> 1.8.1.1.dirty > >>> > > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: henryc.chen@mediatek.com (Henry Chen) Date: Thu, 21 Jan 2016 18:28:24 +0800 Subject: [PATCH] soc: mediatek: PMIC wrap: clear the STAUPD_TRIG bit of WDT_SRC_EN In-Reply-To: <56A0A249.30003@gmail.com> References: <1453265564-16379-1-git-send-email-henryc.chen@mediatek.com> <1453288312.8457.9.camel@mtksdaap41> <56A0A249.30003@gmail.com> Message-ID: <1453372104.8457.12.camel@mtksdaap41> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 2016-01-21 at 10:18 +0100, Matthias Brugger wrote: > > On 20/01/16 12:11, Henry Chen wrote: > > On Wed, 2016-01-20 at 02:32 -0800, Daniel Kurtz wrote: > >> On Tue, Jan 19, 2016 at 8:52 PM, Henry Chen wrote: > >>> Since STAUPD interrupts aren't handled on mt8173, disable watchdog timeout > >>> monitor of STAUPD to avoid WDT_INT triggered by STAUPD. > >>> > >>> Signed-off-by: Henry Chen > >> > >>> --- > >>> drivers/soc/mediatek/mtk-pmic-wrap.c | 19 +++++++++++++++++-- > >>> 1 file changed, 17 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> index af919b1..998f561 100644 > >>> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > >>> @@ -60,6 +60,15 @@ > >>> #define PWRAP_MAN_CMD_OP_OUTD (0x9 << 8) > >>> #define PWRAP_MAN_CMD_OP_OUTQ (0xa << 8) > >>> > >>> +/* macro for Watch Dog Timer Source */ > >>> +#define PWRAP_WDT_SRC_EN_STAUPD_TRIG (1 << 25) > >>> +#define PWRAP_WDT_SRC_EN_HARB_STAUPD_DLE (1 << 20) > >>> +#define PWRAP_WDT_SRC_EN_HARB_STAUPD_ALE (1 << 6) > >>> +#define PWRAP_WDT_SRC_MASK_ALL 0xffffffff > >>> +#define PWRAP_WDT_SRC_MASK_NO_STAUPD ~(PWRAP_WDT_SRC_EN_STAUPD_TRIG | \ > >>> + PWRAP_WDT_SRC_EN_HARB_STAUPD_DLE | \ > >>> + PWRAP_WDT_SRC_EN_HARB_STAUPD_ALE) > >>> + > >>> /* macro for slave device wrapper registers */ > >>> #define PWRAP_DEW_BASE 0xbc00 > >>> #define PWRAP_DEW_EVENT_OUT_EN (PWRAP_DEW_BASE + 0x0) > >>> @@ -830,7 +839,7 @@ MODULE_DEVICE_TABLE(of, of_pwrap_match_tbl); > >>> > >>> static int pwrap_probe(struct platform_device *pdev) > >>> { > >>> - int ret, irq; > >>> + int ret, irq, wdt_src; > >>> struct pmic_wrapper *wrp; > >>> struct device_node *np = pdev->dev.of_node; > >>> const struct of_device_id *of_id = > >>> @@ -920,7 +929,13 @@ static int pwrap_probe(struct platform_device *pdev) > >>> > >>> /* Initialize watchdog, may not be done by the bootloader */ > >>> pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT); > >>> - pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN); > >>> + /* > >>> + * Since STAUPD was not used on mt8173 platform, > >>> + * so STAUPD of WDT_SRC which should be turned off > >>> + */ > >> > >> It is a little awkward that "!pwrap_is_mt8135(wrp)" means mt8173. Is > >> this always true? > >> Why aren't we clearing STAUPD for mt8135 too? > >> I assume it is because mt8135 does not define this bit? > >> What about other devices that are not mt8135 or mt8173? > > > > MT8135 used STAUPD because it have pwrap event pin and it was different > > with mt8173. pwrap event have been removed on mt8173, so it no need to > > set STAUPD. > > > > I think we already had this discussion once. It seems as if there are > different versions of pmic-wrapper. An older version used by mt8135 and > e.g. mt6589 and a newer one used by mt8173. > > So I agree with Daniel that we should check pwrap_is_mt8173 instead. > Apart from that the patch looks good. > Ok, I will send a patch to change that, thanks. Henry > Regards, > Matthias > > > Henry > > > >> Perhaps change the comment to: > >> "This driver does not use STAUPD, so clear this WDT SRC for all > >> devices for which it exists to avoid unhandled interrupts" > >> > > > >> Other than that, this patch is: > >> Reviewed-by: Daniel Kurtz > >> > >>> + wdt_src = pwrap_is_mt8135(wrp) ? > >>> + PWRAP_WDT_SRC_MASK_ALL : PWRAP_WDT_SRC_MASK_NO_STAUPD; > >>> + pwrap_writel(wrp, wdt_src, PWRAP_WDT_SRC_EN); > >>> pwrap_writel(wrp, 0x1, PWRAP_TIMER_EN); > >>> pwrap_writel(wrp, ~((1 << 31) | (1 << 1)), PWRAP_INT_EN); > >>> > >>> -- > >>> 1.8.1.1.dirty > >>> > > > >