Re: [PATCH] soc: mediatek: PMIC wrap: clear the STAUPD_TRIG bit of WDT_SRC_EN

[Henry Chen <henryc.chen@mediatek.com>]

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 <henryc.chen@mediatek.com> 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 <henryc.chen@mediatek.com>
> >>
> >>> ---
> >>>   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 <djkurtz@chromium.org>
> >>
> >>> +       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
> >>>
> >
> >