[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
doesn't enable STAUPD_PRD together, interrupt will be triggered because
STAUPD timeout. To avoid unexpected interrupt, enable periodic status
update which will be updated to PMIC every selected time period.

Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index a8cde17..6e5c20f 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	/*
+	 * Enable periodic status update which will be updated to PMIC
+	 * every selected time period.
+	 */
+	pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
 	/* Initialize watchdog, may not be done by the bootloader */
 	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
 	pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
-- 
1.9.1

[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
doesn't enable STAUPD_PRD together, interrupt will be triggered because
STAUPD timeout. To avoid unexpected interrupt, enable periodic status
update which will be updated to PMIC every selected time period.

Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index a8cde17..6e5c20f 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	/*
+	 * Enable periodic status update which will be updated to PMIC
+	 * every selected time period.
+	 */
+	pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
 	/* Initialize watchdog, may not be done by the bootloader */
 	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
 	pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
-- 
1.9.1

[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
doesn't enable STAUPD_PRD together, interrupt will be triggered because
STAUPD timeout. To avoid unexpected interrupt, enable periodic status
update which will be updated to PMIC every selected time period.

Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
---
 drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
index a8cde17..6e5c20f 100644
--- a/drivers/soc/mediatek/mtk-pmic-wrap.c
+++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
@@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	/*
+	 * Enable periodic status update which will be updated to PMIC
+	 * every selected time period.
+	 */
+	pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
 	/* Initialize watchdog, may not be done by the bootloader */
 	pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
 	pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
-- 
1.9.1

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Daniel Kurtz]

On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
>
> The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> doesn't enable STAUPD_PRD together, interrupt will be triggered because
> STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> update which will be updated to PMIC every selected time period.

Sorry, I don't really understand this.

What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?

How does setting STAUPD_PRD disable this "unexpected interrupt"?

>From the MT8173 Datasheet, I can see that the value written to
STAUPD_PRD is the "periodic status update timing (period)".

> Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index a8cde17..6e5c20f 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> +       /*
> +        * Enable periodic status update which will be updated to PMIC
> +        * every selected time period.
> +        */
> +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);

nit: Perhaps use a define for 5, and specify the real period value.
Something like this:

#define PWRAP_STAUPD_98_5US  5


>         /* Initialize watchdog, may not be done by the bootloader */
>         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Daniel Kurtz]

On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>
> The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> doesn't enable STAUPD_PRD together, interrupt will be triggered because
> STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> update which will be updated to PMIC every selected time period.

Sorry, I don't really understand this.

What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?

How does setting STAUPD_PRD disable this "unexpected interrupt"?

>From the MT8173 Datasheet, I can see that the value written to
STAUPD_PRD is the "periodic status update timing (period)".

> Signed-off-by: Henry Chen <henryc.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index a8cde17..6e5c20f 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> +       /*
> +        * Enable periodic status update which will be updated to PMIC
> +        * every selected time period.
> +        */
> +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);

nit: Perhaps use a define for 5, and specify the real period value.
Something like this:

#define PWRAP_STAUPD_98_5US  5


>         /* Initialize watchdog, may not be done by the bootloader */
>         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Daniel Kurtz]

On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
>
> The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> doesn't enable STAUPD_PRD together, interrupt will be triggered because
> STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> update which will be updated to PMIC every selected time period.

Sorry, I don't really understand this.

What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?

How does setting STAUPD_PRD disable this "unexpected interrupt"?

>From the MT8173 Datasheet, I can see that the value written to
STAUPD_PRD is the "periodic status update timing (period)".

> Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index a8cde17..6e5c20f 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> +       /*
> +        * Enable periodic status update which will be updated to PMIC
> +        * every selected time period.
> +        */
> +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);

nit: Perhaps use a define for 5, and specify the real period value.
Something like this:

#define PWRAP_STAUPD_98_5US  5


>         /* Initialize watchdog, may not be done by the bootloader */
>         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
> >
> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> > update which will be updated to PMIC every selected time period.
> 
> Sorry, I don't really understand this.
> 
> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
> 
> How does setting STAUPD_PRD disable this "unexpected interrupt"?
> 
Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
enabled:

bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor

Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
watchdog timeout.


> From the MT8173 Datasheet, I can see that the value written to
> STAUPD_PRD is the "periodic status update timing (period)".
> 
> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > index a8cde17..6e5c20f 100644
> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
> >                 return -ENODEV;
> >         }
> >
> > +       /*
> > +        * Enable periodic status update which will be updated to PMIC
> > +        * every selected time period.
> > +        */
> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
> 
> nit: Perhaps use a define for 5, and specify the real period value.
> Something like this:
> 
> #define PWRAP_STAUPD_98_5US  5
> 
ok.

> 
> >         /* Initialize watchdog, may not be done by the bootloader */
> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >
> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> > update which will be updated to PMIC every selected time period.
> 
> Sorry, I don't really understand this.
> 
> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
> 
> How does setting STAUPD_PRD disable this "unexpected interrupt"?
> 
Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
enabled:

bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor

Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
watchdog timeout.


> From the MT8173 Datasheet, I can see that the value written to
> STAUPD_PRD is the "periodic status update timing (period)".
> 
> > Signed-off-by: Henry Chen <henryc.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > index a8cde17..6e5c20f 100644
> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
> >                 return -ENODEV;
> >         }
> >
> > +       /*
> > +        * Enable periodic status update which will be updated to PMIC
> > +        * every selected time period.
> > +        */
> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
> 
> nit: Perhaps use a define for 5, and specify the real period value.
> Something like this:
> 
> #define PWRAP_STAUPD_98_5US  5
> 
ok.

> 
> >         /* Initialize watchdog, may not be done by the bootloader */
> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
> >
> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> > update which will be updated to PMIC every selected time period.
> 
> Sorry, I don't really understand this.
> 
> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
> 
> How does setting STAUPD_PRD disable this "unexpected interrupt"?
> 
Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
enabled:

bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor

Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
watchdog timeout.


> From the MT8173 Datasheet, I can see that the value written to
> STAUPD_PRD is the "periodic status update timing (period)".
> 
> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > index a8cde17..6e5c20f 100644
> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
> >                 return -ENODEV;
> >         }
> >
> > +       /*
> > +        * Enable periodic status update which will be updated to PMIC
> > +        * every selected time period.
> > +        */
> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
> 
> nit: Perhaps use a define for 5, and specify the real period value.
> Something like this:
> 
> #define PWRAP_STAUPD_98_5US  5
> 
ok.

> 
> >         /* Initialize watchdog, may not be done by the bootloader */
> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Daniel Kurtz]

On Mon, Jan 4, 2016 at 11:05 AM, Henry Chen <henryc.chen@mediatek.com> wrote:
> On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
>> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
>> >
>> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
>> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
>> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
>> > update which will be updated to PMIC every selected time period.
>>
>> Sorry, I don't really understand this.
>>
>> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
>>
>> How does setting STAUPD_PRD disable this "unexpected interrupt"?
>>
> Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
> enabled:
>
> bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor
>
> Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
> watchdog timeout.

Sorry, I still don't understand.

IIUC, setting STAUPD_PRD sets the period at which status updates are
reported (announced via the shared STAUPD/WDT interrupt).

So, setting STAUPD_PRD=5 should set the reporting period to 98.5 us.
But, how does changing this period fix the "unexpected interrupt"?
I can understand how it might change the timing of the interrupt, but
why does it make the interrupt no longer occur?
We are still triggering the interrupt when we write bit[25]
(STAUPD_TRIG) of WDT_SRC_EN, two lines later.

Isn't this still requesting a STAUPD interrupt 98.5 us later?  (which,
since STAUPD interrupts aren't handled, is an "unexpected interrupt")
Wouldn't a better fix be to just clear the STAUPD_TRIG bit of
WDT_SRC_EN, and just not trigger STAUPD in the first place if we can't
handle them?

-Dan

>> From the MT8173 Datasheet, I can see that the value written to
>> STAUPD_PRD is the "periodic status update timing (period)".
>>
>> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
>> > ---
>> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > index a8cde17..6e5c20f 100644
>> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
>> >                 return -ENODEV;
>> >         }
>> >
>> > +       /*
>> > +        * Enable periodic status update which will be updated to PMIC
>> > +        * every selected time period.
>> > +        */
>> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
>>
>> nit: Perhaps use a define for 5, and specify the real period value.
>> Something like this:
>>
>> #define PWRAP_STAUPD_98_5US  5
>>
> ok.
>
>>
>> >         /* Initialize watchdog, may not be done by the bootloader */
>> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
>> > --
>> > 1.9.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>
>

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Daniel Kurtz]

On Mon, Jan 4, 2016 at 11:05 AM, Henry Chen <henryc.chen@mediatek.com> wrote:
> On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
>> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
>> >
>> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
>> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
>> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
>> > update which will be updated to PMIC every selected time period.
>>
>> Sorry, I don't really understand this.
>>
>> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
>>
>> How does setting STAUPD_PRD disable this "unexpected interrupt"?
>>
> Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
> enabled:
>
> bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor
>
> Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
> watchdog timeout.

Sorry, I still don't understand.

IIUC, setting STAUPD_PRD sets the period at which status updates are
reported (announced via the shared STAUPD/WDT interrupt).

So, setting STAUPD_PRD=5 should set the reporting period to 98.5 us.
But, how does changing this period fix the "unexpected interrupt"?
I can understand how it might change the timing of the interrupt, but
why does it make the interrupt no longer occur?
We are still triggering the interrupt when we write bit[25]
(STAUPD_TRIG) of WDT_SRC_EN, two lines later.

Isn't this still requesting a STAUPD interrupt 98.5 us later?  (which,
since STAUPD interrupts aren't handled, is an "unexpected interrupt")
Wouldn't a better fix be to just clear the STAUPD_TRIG bit of
WDT_SRC_EN, and just not trigger STAUPD in the first place if we can't
handle them?

-Dan

>> From the MT8173 Datasheet, I can see that the value written to
>> STAUPD_PRD is the "periodic status update timing (period)".
>>
>> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
>> > ---
>> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > index a8cde17..6e5c20f 100644
>> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
>> >                 return -ENODEV;
>> >         }
>> >
>> > +       /*
>> > +        * Enable periodic status update which will be updated to PMIC
>> > +        * every selected time period.
>> > +        */
>> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
>>
>> nit: Perhaps use a define for 5, and specify the real period value.
>> Something like this:
>>
>> #define PWRAP_STAUPD_98_5US  5
>>
> ok.
>
>>
>> >         /* Initialize watchdog, may not be done by the bootloader */
>> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
>> > --
>> > 1.9.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>
>

[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Daniel Kurtz]

On Mon, Jan 4, 2016 at 11:05 AM, Henry Chen <henryc.chen@mediatek.com> wrote:
> On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
>> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
>> >
>> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
>> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
>> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
>> > update which will be updated to PMIC every selected time period.
>>
>> Sorry, I don't really understand this.
>>
>> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
>>
>> How does setting STAUPD_PRD disable this "unexpected interrupt"?
>>
> Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
> enabled:
>
> bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor
>
> Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
> watchdog timeout.

Sorry, I still don't understand.

IIUC, setting STAUPD_PRD sets the period at which status updates are
reported (announced via the shared STAUPD/WDT interrupt).

So, setting STAUPD_PRD=5 should set the reporting period to 98.5 us.
But, how does changing this period fix the "unexpected interrupt"?
I can understand how it might change the timing of the interrupt, but
why does it make the interrupt no longer occur?
We are still triggering the interrupt when we write bit[25]
(STAUPD_TRIG) of WDT_SRC_EN, two lines later.

Isn't this still requesting a STAUPD interrupt 98.5 us later?  (which,
since STAUPD interrupts aren't handled, is an "unexpected interrupt")
Wouldn't a better fix be to just clear the STAUPD_TRIG bit of
WDT_SRC_EN, and just not trigger STAUPD in the first place if we can't
handle them?

-Dan

>> From the MT8173 Datasheet, I can see that the value written to
>> STAUPD_PRD is the "periodic status update timing (period)".
>>
>> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
>> > ---
>> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > index a8cde17..6e5c20f 100644
>> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
>> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
>> >                 return -ENODEV;
>> >         }
>> >
>> > +       /*
>> > +        * Enable periodic status update which will be updated to PMIC
>> > +        * every selected time period.
>> > +        */
>> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
>>
>> nit: Perhaps use a define for 5, and specify the real period value.
>> Something like this:
>>
>> #define PWRAP_STAUPD_98_5US  5
>>
> ok.
>
>>
>> >         /* Initialize watchdog, may not be done by the bootloader */
>> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
>> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
>> > --
>> > 1.9.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo at vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>
>

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

On Wed, 2016-01-06 at 10:37 +0800, Daniel Kurtz wrote:
> On Mon, Jan 4, 2016 at 11:05 AM, Henry Chen <henryc.chen@mediatek.com> wrote:
> > On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
> >> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
> >> >
> >> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> >> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
> >> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> >> > update which will be updated to PMIC every selected time period.
> >>
> >> Sorry, I don't really understand this.
> >>
> >> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
> >>
> >> How does setting STAUPD_PRD disable this "unexpected interrupt"?
> >>
> > Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
> > enabled:
> >
> > bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor
> >
> > Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
> > watchdog timeout.
> 
> Sorry, I still don't understand.
Sorry, I will try to explain the behavior more clearly.
> 
> IIUC, setting STAUPD_PRD sets the period at which status updates are
> reported (announced via the shared STAUPD/WDT interrupt).
> 
> So, setting STAUPD_PRD=5 should set the reporting period to 98.5 us.
> But, how does changing this period fix the "unexpected interrupt"?

Setting STAUPD_PRD which means pmic wrap will get the status from PMIC
every 98.5us. Each time watchdog saw the STAUPD update, the interrupt
won't be trigger.

> I can understand how it might change the timing of the interrupt, but
> why does it make the interrupt no longer occur?

STAUPD_PRD was not the timer to trigger interrupt, I think what you said
was WDT_UNIT.

> We are still triggering the interrupt when we write bit[25]
> (STAUPD_TRIG) of WDT_SRC_EN, two lines later.

if STAUPD_TRIG was set, STAUPD_PRD=5 => STAUPD will trigger signal to
get the status from PMIC every 98.5us => the interrupt will not trigger,
because STAUPD_PRD working.
 
if STAUPD_TRIG was set, STAUPD_PRD=0 => STAUPD disable.=> the interrupt
will trigger by watchdog because STAUPD won't trigger the signal.

> Isn't this still requesting a STAUPD interrupt 98.5 us later?  (which,
> since STAUPD interrupts aren't handled, is an "unexpected interrupt")
> Wouldn't a better fix be to just clear the STAUPD_TRIG bit of
> WDT_SRC_EN, and just not trigger STAUPD in the first place if we can't
> handle them?
Yes. Maybe to clear the STAUPD_TRIG bit of WDT_SRC_EN was better to fix
the problem.

Henry
> 
> -Dan
> 
> >> From the MT8173 Datasheet, I can see that the value written to
> >> STAUPD_PRD is the "periodic status update timing (period)".
> >>
> >> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> >> > ---
> >> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
> >> >  1 file changed, 5 insertions(+)
> >> >
> >> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > index a8cde17..6e5c20f 100644
> >> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
> >> >                 return -ENODEV;
> >> >         }
> >> >
> >> > +       /*
> >> > +        * Enable periodic status update which will be updated to PMIC
> >> > +        * every selected time period.
> >> > +        */
> >> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
> >>
> >> nit: Perhaps use a define for 5, and specify the real period value.
> >> Something like this:
> >>
> >> #define PWRAP_STAUPD_98_5US  5
> >>
> > ok.
> >
> >>
> >> >         /* Initialize watchdog, may not be done by the bootloader */
> >> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> >> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> >> > --
> >> > 1.9.1
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> >

Re: [PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

On Wed, 2016-01-06 at 10:37 +0800, Daniel Kurtz wrote:
> On Mon, Jan 4, 2016 at 11:05 AM, Henry Chen <henryc.chen@mediatek.com> wrote:
> > On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
> >> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
> >> >
> >> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> >> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
> >> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> >> > update which will be updated to PMIC every selected time period.
> >>
> >> Sorry, I don't really understand this.
> >>
> >> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
> >>
> >> How does setting STAUPD_PRD disable this "unexpected interrupt"?
> >>
> > Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
> > enabled:
> >
> > bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor
> >
> > Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
> > watchdog timeout.
> 
> Sorry, I still don't understand.
Sorry, I will try to explain the behavior more clearly.
> 
> IIUC, setting STAUPD_PRD sets the period at which status updates are
> reported (announced via the shared STAUPD/WDT interrupt).
> 
> So, setting STAUPD_PRD=5 should set the reporting period to 98.5 us.
> But, how does changing this period fix the "unexpected interrupt"?

Setting STAUPD_PRD which means pmic wrap will get the status from PMIC
every 98.5us. Each time watchdog saw the STAUPD update, the interrupt
won't be trigger.

> I can understand how it might change the timing of the interrupt, but
> why does it make the interrupt no longer occur?

STAUPD_PRD was not the timer to trigger interrupt, I think what you said
was WDT_UNIT.

> We are still triggering the interrupt when we write bit[25]
> (STAUPD_TRIG) of WDT_SRC_EN, two lines later.

if STAUPD_TRIG was set, STAUPD_PRD=5 => STAUPD will trigger signal to
get the status from PMIC every 98.5us => the interrupt will not trigger,
because STAUPD_PRD working.
 
if STAUPD_TRIG was set, STAUPD_PRD=0 => STAUPD disable.=> the interrupt
will trigger by watchdog because STAUPD won't trigger the signal.

> Isn't this still requesting a STAUPD interrupt 98.5 us later?  (which,
> since STAUPD interrupts aren't handled, is an "unexpected interrupt")
> Wouldn't a better fix be to just clear the STAUPD_TRIG bit of
> WDT_SRC_EN, and just not trigger STAUPD in the first place if we can't
> handle them?
Yes. Maybe to clear the STAUPD_TRIG bit of WDT_SRC_EN was better to fix
the problem.

Henry
> 
> -Dan
> 
> >> From the MT8173 Datasheet, I can see that the value written to
> >> STAUPD_PRD is the "periodic status update timing (period)".
> >>
> >> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> >> > ---
> >> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
> >> >  1 file changed, 5 insertions(+)
> >> >
> >> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > index a8cde17..6e5c20f 100644
> >> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
> >> >                 return -ENODEV;
> >> >         }
> >> >
> >> > +       /*
> >> > +        * Enable periodic status update which will be updated to PMIC
> >> > +        * every selected time period.
> >> > +        */
> >> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
> >>
> >> nit: Perhaps use a define for 5, and specify the real period value.
> >> Something like this:
> >>
> >> #define PWRAP_STAUPD_98_5US  5
> >>
> > ok.
> >
> >>
> >> >         /* Initialize watchdog, may not be done by the bootloader */
> >> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> >> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> >> > --
> >> > 1.9.1
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> >

[PATCH] soc: mediatek: PMIC wrap: Enable STAUPD_PRD before WDT_SRC_EN enabled.

[Henry Chen]

On Wed, 2016-01-06 at 10:37 +0800, Daniel Kurtz wrote:
> On Mon, Jan 4, 2016 at 11:05 AM, Henry Chen <henryc.chen@mediatek.com> wrote:
> > On Thu, 2015-12-31 at 22:19 +0800, Daniel Kurtz wrote:
> >> On Wed, Dec 30, 2015 at 8:36 PM, Henry Chen <henryc.chen@mediatek.com> wrote:
> >> >
> >> > The STAUPD_TRIG will be enable when WDT_INT enable on probe function, if
> >> > doesn't enable STAUPD_PRD together, interrupt will be triggered because
> >> > STAUPD timeout. To avoid unexpected interrupt, enable periodic status
> >> > update which will be updated to PMIC every selected time period.
> >>
> >> Sorry, I don't really understand this.
> >>
> >> What exactly is triggering the unexpected watchdog interrupt (WDT_INT)?
> >>
> >> How does setting STAUPD_PRD disable this "unexpected interrupt"?
> >>
> > Yes, WDT_INT was triggered because the bit[25] of WDT_SRC_EN was
> > enabled:
> >
> > bit[25] STAUPD_TRIG: STAUPD trigger signal timeout monitor
> >
> > Setting STAUPD_PRD will update the status of PMIC periodic to avoid this
> > watchdog timeout.
> 
> Sorry, I still don't understand.
Sorry, I will try to explain the behavior more clearly.
> 
> IIUC, setting STAUPD_PRD sets the period at which status updates are
> reported (announced via the shared STAUPD/WDT interrupt).
> 
> So, setting STAUPD_PRD=5 should set the reporting period to 98.5 us.
> But, how does changing this period fix the "unexpected interrupt"?

Setting STAUPD_PRD which means pmic wrap will get the status from PMIC
every 98.5us. Each time watchdog saw the STAUPD update, the interrupt
won't be trigger.

> I can understand how it might change the timing of the interrupt, but
> why does it make the interrupt no longer occur?

STAUPD_PRD was not the timer to trigger interrupt, I think what you said
was WDT_UNIT.

> We are still triggering the interrupt when we write bit[25]
> (STAUPD_TRIG) of WDT_SRC_EN, two lines later.

if STAUPD_TRIG was set, STAUPD_PRD=5 => STAUPD will trigger signal to
get the status from PMIC every 98.5us => the interrupt will not trigger,
because STAUPD_PRD working.
 
if STAUPD_TRIG was set, STAUPD_PRD=0 => STAUPD disable.=> the interrupt
will trigger by watchdog because STAUPD won't trigger the signal.

> Isn't this still requesting a STAUPD interrupt 98.5 us later?  (which,
> since STAUPD interrupts aren't handled, is an "unexpected interrupt")
> Wouldn't a better fix be to just clear the STAUPD_TRIG bit of
> WDT_SRC_EN, and just not trigger STAUPD in the first place if we can't
> handle them?
Yes. Maybe to clear the STAUPD_TRIG bit of WDT_SRC_EN was better to fix
the problem.

Henry
> 
> -Dan
> 
> >> From the MT8173 Datasheet, I can see that the value written to
> >> STAUPD_PRD is the "periodic status update timing (period)".
> >>
> >> > Signed-off-by: Henry Chen <henryc.chen@mediatek.com>
> >> > ---
> >> >  drivers/soc/mediatek/mtk-pmic-wrap.c | 5 +++++
> >> >  1 file changed, 5 insertions(+)
> >> >
> >> > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > index a8cde17..6e5c20f 100644
> >> > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> >> > @@ -904,6 +904,11 @@ static int pwrap_probe(struct platform_device *pdev)
> >> >                 return -ENODEV;
> >> >         }
> >> >
> >> > +       /*
> >> > +        * Enable periodic status update which will be updated to PMIC
> >> > +        * every selected time period.
> >> > +        */
> >> > +       pwrap_writel(wrp, 0x5, PWRAP_STAUPD_PRD);
> >>
> >> nit: Perhaps use a define for 5, and specify the real period value.
> >> Something like this:
> >>
> >> #define PWRAP_STAUPD_98_5US  5
> >>
> > ok.
> >
> >>
> >> >         /* Initialize watchdog, may not be done by the bootloader */
> >> >         pwrap_writel(wrp, 0xf, PWRAP_WDT_UNIT);
> >> >         pwrap_writel(wrp, 0xffffffff, PWRAP_WDT_SRC_EN);
> >> > --
> >> > 1.9.1
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> > the body of a message to majordomo at vger.kernel.org
> >> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> >