2021-09-21 05:35:40

by Alexandre Ghiti

[permalink] [raw]
Subject: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
notifier that will execute a small i2c sequence allowing to reset the
board.

The original implementation comes from Marcus Comstedt and Anders Montonen
(https://forums.sifive.com/t/reboot-command/4721/28).

Signed-off-by: Alexandre Ghiti <[email protected]>
---
drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
include/linux/mfd/da9063/core.h | 3 +++
2 files changed, 28 insertions(+)

diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
index df407c3afce3..c87b8d611f20 100644
--- a/drivers/mfd/da9063-core.c
+++ b/drivers/mfd/da9063-core.c
@@ -20,6 +20,7 @@
#include <linux/mutex.h>
#include <linux/mfd/core.h>
#include <linux/regmap.h>
+#include <linux/reboot.h>

#include <linux/mfd/da9063/core.h>
#include <linux/mfd/da9063/registers.h>
@@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
return ret;
}

+static int da9063_restart_notify(struct notifier_block *this,
+ unsigned long mode, void *cmd)
+{
+ struct da9063 *da9063 = container_of(this, struct da9063, restart_handler);
+
+ regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
+ regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
+ regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
+
+ return NOTIFY_DONE;
+}
+
int da9063_device_init(struct da9063 *da9063, unsigned int irq)
{
int ret;
@@ -197,6 +210,18 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
}
}

+ da9063->restart_handler.notifier_call = da9063_restart_notify;
+ da9063->restart_handler.priority = 128;
+ ret = register_restart_handler(&da9063->restart_handler);
+ if (ret) {
+ dev_err(da9063->dev, "Failed to register restart handler\n");
+ return ret;
+ }
+
+ devm_add_action(da9063->dev,
+ (void (*)(void *))unregister_restart_handler,
+ &da9063->restart_handler);
+
return ret;
}

diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index fa7a43f02f27..1b20c498e340 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -85,6 +85,9 @@ struct da9063 {
int chip_irq;
unsigned int irq_base;
struct regmap_irq_chip_data *regmap_irq;
+
+ /* Restart */
+ struct notifier_block restart_handler;
};

int da9063_device_init(struct da9063 *da9063, unsigned int irq);
--
2.30.2


2021-09-21 10:18:45

by Anup Patel

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Tue, Sep 21, 2021 at 11:04 AM Alexandre Ghiti
<[email protected]> wrote:
>
> The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
> notifier that will execute a small i2c sequence allowing to reset the
> board.
>
> The original implementation comes from Marcus Comstedt and Anders Montonen
> (https://forums.sifive.com/t/reboot-command/4721/28).
>
> Signed-off-by: Alexandre Ghiti <[email protected]>
> ---
> drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
> include/linux/mfd/da9063/core.h | 3 +++
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index df407c3afce3..c87b8d611f20 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -20,6 +20,7 @@
> #include <linux/mutex.h>
> #include <linux/mfd/core.h>
> #include <linux/regmap.h>
> +#include <linux/reboot.h>
>
> #include <linux/mfd/da9063/core.h>
> #include <linux/mfd/da9063/registers.h>
> @@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
> return ret;
> }
>
> +static int da9063_restart_notify(struct notifier_block *this,
> + unsigned long mode, void *cmd)
> +{
> + struct da9063 *da9063 = container_of(this, struct da9063, restart_handler);
> +
> + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> +
> + return NOTIFY_DONE;
> +}
> +
> int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> {
> int ret;
> @@ -197,6 +210,18 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> }
> }
>
> + da9063->restart_handler.notifier_call = da9063_restart_notify;
> + da9063->restart_handler.priority = 128;

How was this priority value chosen ?

I mean, we will be having SBI SRST as well so we should choose
a priority value lower than what we choose for SBI SRST handler.

Regards,
Anup

> + ret = register_restart_handler(&da9063->restart_handler);
> + if (ret) {
> + dev_err(da9063->dev, "Failed to register restart handler\n");
> + return ret;
> + }
> +
> + devm_add_action(da9063->dev,
> + (void (*)(void *))unregister_restart_handler,
> + &da9063->restart_handler);
> +
> return ret;
> }
>
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index fa7a43f02f27..1b20c498e340 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -85,6 +85,9 @@ struct da9063 {
> int chip_irq;
> unsigned int irq_base;
> struct regmap_irq_chip_data *regmap_irq;
> +
> + /* Restart */
> + struct notifier_block restart_handler;
> };
>
> int da9063_device_init(struct da9063 *da9063, unsigned int irq);
> --
> 2.30.2
>

2021-09-21 11:08:12

by Ben Dooks

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 21/09/2021 06:33, Alexandre Ghiti wrote:
> The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
> notifier that will execute a small i2c sequence allowing to reset the
> board.
>
> The original implementation comes from Marcus Comstedt and Anders Montonen
> (https://forums.sifive.com/t/reboot-command/4721/28).
>
> Signed-off-by: Alexandre Ghiti <[email protected]>

I've got a couple of comments here, mainly is this the right place
and has anyone other than you tested. I tried something similar on
my Unmatched and it just turned the board off.

> ---
> drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
> include/linux/mfd/da9063/core.h | 3 +++
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index df407c3afce3..c87b8d611f20 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -20,6 +20,7 @@
> #include <linux/mutex.h>
> #include <linux/mfd/core.h>
> #include <linux/regmap.h>
> +#include <linux/reboot.h>
>
> #include <linux/mfd/da9063/core.h>
> #include <linux/mfd/da9063/registers.h>
> @@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
> return ret;
> }
>
> +static int da9063_restart_notify(struct notifier_block *this,
> + unsigned long mode, void *cmd)
> +{
> + struct da9063 *da9063 = container_of(this, struct da9063, restart_handler);
> +
> + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> +
> + return NOTIFY_DONE;
> +}
> +

Firstly, do you also need to force the AUTOBOOT (bit 3, CONTROL_C)
to ensure the PMIC does restart.

> int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> {
> int ret;
> @@ -197,6 +210,18 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> }
> }
>
> + da9063->restart_handler.notifier_call = da9063_restart_notify;
> + da9063->restart_handler.priority = 128;
> + ret = register_restart_handler(&da9063->restart_handler);
> + if (ret) {
> + dev_err(da9063->dev, "Failed to register restart handler\n");
> + return ret;
> + }
> +
> + devm_add_action(da9063->dev,
> + (void (*)(void *))unregister_restart_handler,
> + &da9063->restart_handler);

there's devm_register_reboot_notifier()


> +
> return ret;
> }
>
> diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> index fa7a43f02f27..1b20c498e340 100644
> --- a/include/linux/mfd/da9063/core.h
> +++ b/include/linux/mfd/da9063/core.h
> @@ -85,6 +85,9 @@ struct da9063 {
> int chip_irq;
> unsigned int irq_base;
> struct regmap_irq_chip_data *regmap_irq;
> +
> + /* Restart */
> + struct notifier_block restart_handler;
> };
>
> int da9063_device_init(struct da9063 *da9063, unsigned int irq);

Note, the watchdog driver for the DA9063 also has a restart method
although it also does not set the AUTOBOOT bit either.

The best thing is to enable the watchdog driver, the SiFive release
does not have either the core DA9063 or the watchdog driver for it
enabled.

--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

2021-09-21 11:21:57

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

I chose a priority lower than the one you proposed in your SRST series
(https://lkml.org/lkml/2021/6/9/620).

Alex

On Tue, Sep 21, 2021 at 12:17 PM Anup Patel <[email protected]> wrote:
>
> On Tue, Sep 21, 2021 at 11:04 AM Alexandre Ghiti
> <[email protected]> wrote:
> >
> > The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
> > notifier that will execute a small i2c sequence allowing to reset the
> > board.
> >
> > The original implementation comes from Marcus Comstedt and Anders Montonen
> > (https://forums.sifive.com/t/reboot-command/4721/28).
> >
> > Signed-off-by: Alexandre Ghiti <[email protected]>
> > ---
> > drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
> > include/linux/mfd/da9063/core.h | 3 +++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> > index df407c3afce3..c87b8d611f20 100644
> > --- a/drivers/mfd/da9063-core.c
> > +++ b/drivers/mfd/da9063-core.c
> > @@ -20,6 +20,7 @@
> > #include <linux/mutex.h>
> > #include <linux/mfd/core.h>
> > #include <linux/regmap.h>
> > +#include <linux/reboot.h>
> >
> > #include <linux/mfd/da9063/core.h>
> > #include <linux/mfd/da9063/registers.h>
> > @@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
> > return ret;
> > }
> >
> > +static int da9063_restart_notify(struct notifier_block *this,
> > + unsigned long mode, void *cmd)
> > +{
> > + struct da9063 *da9063 = container_of(this, struct da9063, restart_handler);
> > +
> > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > +
> > + return NOTIFY_DONE;
> > +}
> > +
> > int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> > {
> > int ret;
> > @@ -197,6 +210,18 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> > }
> > }
> >
> > + da9063->restart_handler.notifier_call = da9063_restart_notify;
> > + da9063->restart_handler.priority = 128;
>
> How was this priority value chosen ?
>
> I mean, we will be having SBI SRST as well so we should choose
> a priority value lower than what we choose for SBI SRST handler.
>
> Regards,
> Anup
>
> > + ret = register_restart_handler(&da9063->restart_handler);
> > + if (ret) {
> > + dev_err(da9063->dev, "Failed to register restart handler\n");
> > + return ret;
> > + }
> > +
> > + devm_add_action(da9063->dev,
> > + (void (*)(void *))unregister_restart_handler,
> > + &da9063->restart_handler);
> > +
> > return ret;
> > }
> >
> > diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> > index fa7a43f02f27..1b20c498e340 100644
> > --- a/include/linux/mfd/da9063/core.h
> > +++ b/include/linux/mfd/da9063/core.h
> > @@ -85,6 +85,9 @@ struct da9063 {
> > int chip_irq;
> > unsigned int irq_base;
> > struct regmap_irq_chip_data *regmap_irq;
> > +
> > + /* Restart */
> > + struct notifier_block restart_handler;
> > };
> >
> > int da9063_device_init(struct da9063 *da9063, unsigned int irq);
> > --
> > 2.30.2
> >

2021-09-21 11:35:39

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Tue, Sep 21, 2021 at 12:25 PM Ben Dooks <[email protected]> wrote:
>
> On 21/09/2021 06:33, Alexandre Ghiti wrote:
> > The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
> > notifier that will execute a small i2c sequence allowing to reset the
> > board.
> >
> > The original implementation comes from Marcus Comstedt and Anders Montonen
> > (https://forums.sifive.com/t/reboot-command/4721/28).
> >
> > Signed-off-by: Alexandre Ghiti <[email protected]>
>
> I've got a couple of comments here, mainly is this the right place
> and has anyone other than you tested. I tried something similar on
> my Unmatched and it just turned the board off.

Someone else in the thread I mention in the commit log tried it, but
more feedback are welcome :)

For the Unmatched, this solution will be temporary, the best place
being openSBI which lacks i2c support for now.
But I think this can be used by other boards using this PMIC.

>
> > ---
> > drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
> > include/linux/mfd/da9063/core.h | 3 +++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> > index df407c3afce3..c87b8d611f20 100644
> > --- a/drivers/mfd/da9063-core.c
> > +++ b/drivers/mfd/da9063-core.c
> > @@ -20,6 +20,7 @@
> > #include <linux/mutex.h>
> > #include <linux/mfd/core.h>
> > #include <linux/regmap.h>
> > +#include <linux/reboot.h>
> >
> > #include <linux/mfd/da9063/core.h>
> > #include <linux/mfd/da9063/registers.h>
> > @@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
> > return ret;
> > }
> >
> > +static int da9063_restart_notify(struct notifier_block *this,
> > + unsigned long mode, void *cmd)
> > +{
> > + struct da9063 *da9063 = container_of(this, struct da9063, restart_handler);
> > +
> > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > +
> > + return NOTIFY_DONE;
> > +}
> > +
>
> Firstly, do you also need to force the AUTOBOOT (bit 3, CONTROL_C)
> to ensure the PMIC does restart.

I tried this too and actually, it seems that the value is read from
OTP at reboot and as it is not set here, it has no effect.

>
> > int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> > {
> > int ret;
> > @@ -197,6 +210,18 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> > }
> > }
> >
> > + da9063->restart_handler.notifier_call = da9063_restart_notify;
> > + da9063->restart_handler.priority = 128;
> > + ret = register_restart_handler(&da9063->restart_handler);
> > + if (ret) {
> > + dev_err(da9063->dev, "Failed to register restart handler\n");
> > + return ret;
> > + }
> > +
> > + devm_add_action(da9063->dev,
> > + (void (*)(void *))unregister_restart_handler,
> > + &da9063->restart_handler);
>
> there's devm_register_reboot_notifier()

Thanks for that!

>
>
> > +
> > return ret;
> > }
> >
> > diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> > index fa7a43f02f27..1b20c498e340 100644
> > --- a/include/linux/mfd/da9063/core.h
> > +++ b/include/linux/mfd/da9063/core.h
> > @@ -85,6 +85,9 @@ struct da9063 {
> > int chip_irq;
> > unsigned int irq_base;
> > struct regmap_irq_chip_data *regmap_irq;
> > +
> > + /* Restart */
> > + struct notifier_block restart_handler;
> > };
> >
> > int da9063_device_init(struct da9063 *da9063, unsigned int irq);
>
> Note, the watchdog driver for the DA9063 also has a restart method
> although it also does not set the AUTOBOOT bit either.
>
> The best thing is to enable the watchdog driver, the SiFive release
> does not have either the core DA9063 or the watchdog driver for it
> enabled.

It seems that for the restart implemented here to work, the AUTOBOOT
bit needs to be set in the OTP, which seems not to be the case with
the chip on this board: it's been discussed here
https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-support_tab_content
(see the accepted answer).

Thanks,

Alex

>
> --
> Ben Dooks http://www.codethink.co.uk/
> Senior Engineer Codethink - Providing Genius
>
> https://www.codethink.co.uk/privacy.html

2021-09-23 13:19:12

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Tue, Sep 21, 2021 at 1:33 PM Alexandre Ghiti
<[email protected]> wrote:
>
> On Tue, Sep 21, 2021 at 12:25 PM Ben Dooks <[email protected]> wrote:
> >
> > On 21/09/2021 06:33, Alexandre Ghiti wrote:
> > > The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
> > > notifier that will execute a small i2c sequence allowing to reset the
> > > board.
> > >
> > > The original implementation comes from Marcus Comstedt and Anders Montonen
> > > (https://forums.sifive.com/t/reboot-command/4721/28).
> > >
> > > Signed-off-by: Alexandre Ghiti <[email protected]>
> >
> > I've got a couple of comments here, mainly is this the right place
> > and has anyone other than you tested. I tried something similar on
> > my Unmatched and it just turned the board off.
>
> Someone else in the thread I mention in the commit log tried it, but
> more feedback are welcome :)
>
> For the Unmatched, this solution will be temporary, the best place
> being openSBI which lacks i2c support for now.
> But I think this can be used by other boards using this PMIC.
>
> >
> > > ---
> > > drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
> > > include/linux/mfd/da9063/core.h | 3 +++
> > > 2 files changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> > > index df407c3afce3..c87b8d611f20 100644
> > > --- a/drivers/mfd/da9063-core.c
> > > +++ b/drivers/mfd/da9063-core.c
> > > @@ -20,6 +20,7 @@
> > > #include <linux/mutex.h>
> > > #include <linux/mfd/core.h>
> > > #include <linux/regmap.h>
> > > +#include <linux/reboot.h>
> > >
> > > #include <linux/mfd/da9063/core.h>
> > > #include <linux/mfd/da9063/registers.h>
> > > @@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
> > > return ret;
> > > }
> > >
> > > +static int da9063_restart_notify(struct notifier_block *this,
> > > + unsigned long mode, void *cmd)
> > > +{
> > > + struct da9063 *da9063 = container_of(this, struct da9063, restart_handler);
> > > +
> > > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > > +
> > > + return NOTIFY_DONE;
> > > +}
> > > +
> >
> > Firstly, do you also need to force the AUTOBOOT (bit 3, CONTROL_C)
> > to ensure the PMIC does restart.
>
> I tried this too and actually, it seems that the value is read from
> OTP at reboot and as it is not set here, it has no effect.
>
> >
> > > int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> > > {
> > > int ret;
> > > @@ -197,6 +210,18 @@ int da9063_device_init(struct da9063 *da9063, unsigned int irq)
> > > }
> > > }
> > >
> > > + da9063->restart_handler.notifier_call = da9063_restart_notify;
> > > + da9063->restart_handler.priority = 128;
> > > + ret = register_restart_handler(&da9063->restart_handler);
> > > + if (ret) {
> > > + dev_err(da9063->dev, "Failed to register restart handler\n");
> > > + return ret;
> > > + }
> > > +
> > > + devm_add_action(da9063->dev,
> > > + (void (*)(void *))unregister_restart_handler,
> > > + &da9063->restart_handler);
> >
> > there's devm_register_reboot_notifier()
>
> Thanks for that!

Actually restart_notifier and reboot_notifier are not the same, see
https://elixir.bootlin.com/linux/latest/source/kernel/reboot.c#L72.
What we need here is restart_handler.

Thanks anyway :)

Alex

>
> >
> >
> > > +
> > > return ret;
> > > }
> > >
> > > diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
> > > index fa7a43f02f27..1b20c498e340 100644
> > > --- a/include/linux/mfd/da9063/core.h
> > > +++ b/include/linux/mfd/da9063/core.h
> > > @@ -85,6 +85,9 @@ struct da9063 {
> > > int chip_irq;
> > > unsigned int irq_base;
> > > struct regmap_irq_chip_data *regmap_irq;
> > > +
> > > + /* Restart */
> > > + struct notifier_block restart_handler;
> > > };
> > >
> > > int da9063_device_init(struct da9063 *da9063, unsigned int irq);
> >
> > Note, the watchdog driver for the DA9063 also has a restart method
> > although it also does not set the AUTOBOOT bit either.
> >
> > The best thing is to enable the watchdog driver, the SiFive release
> > does not have either the core DA9063 or the watchdog driver for it
> > enabled.
>
> It seems that for the restart implemented here to work, the AUTOBOOT
> bit needs to be set in the OTP, which seems not to be the case with
> the chip on this board: it's been discussed here
> https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-support_tab_content
> (see the accepted answer).
>
> Thanks,
>
> Alex
>
> >
> > --
> > Ben Dooks http://www.codethink.co.uk/
> > Senior Engineer Codethink - Providing Genius
> >
> > https://www.codethink.co.uk/privacy.html

2021-09-24 16:19:34

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

Hi Adam,

On Fri, Sep 24, 2021 at 5:04 PM Adam Thomson
<[email protected]> wrote:
>
> On 21 September 2021 06:34, Alexandre Ghiti wrote:
>
> > The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
> > notifier that will execute a small i2c sequence allowing to reset the
> > board.
> >
> > The original implementation comes from Marcus Comstedt and Anders
> > Montonen
> > (https://forums.sifive.com/t/reboot-command/4721/28).
> >
> > Signed-off-by: Alexandre Ghiti <[email protected]>
> > ---
> > drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
> > include/linux/mfd/da9063/core.h | 3 +++
> > 2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> > index df407c3afce3..c87b8d611f20 100644
> > --- a/drivers/mfd/da9063-core.c
> > +++ b/drivers/mfd/da9063-core.c
> > @@ -20,6 +20,7 @@
> > #include <linux/mutex.h>
> > #include <linux/mfd/core.h>
> > #include <linux/regmap.h>
> > +#include <linux/reboot.h>
> >
> > #include <linux/mfd/da9063/core.h>
> > #include <linux/mfd/da9063/registers.h>
> > @@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
> > return ret;
> > }
> >
> > +static int da9063_restart_notify(struct notifier_block *this,
> > + unsigned long mode, void *cmd)
> > +{
> > + struct da9063 *da9063 = container_of(this, struct da9063,
> > restart_handler);
> > +
> > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > +
> > + return NOTIFY_DONE;
> > +}
>
> I will talk with our HW team to clarify, but this sequence looks to be very
> specific to the needs of the platform in question which doesn't feel right to
> me. As was mentioned on another thread as well, the watchdog driver already has
> a restart function to reset the device (and thus the system), so I don't believe
> we should have multiple of these.

From the discussion that happened here
https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-support_tab_content,
it does not seem possible to use the watchdog on a chip whose OTP does
not set AUTOBOOT. But anyway, I'm looking forward to hearing from the
HW team :)

Thanks,

Alex

>
> For board specific sequences, there are machine quirks I believe which can be
> used to handle stuff like this, if this really isn't a generic solution to fit
> all cases.
>

2021-09-24 21:03:52

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 21 September 2021 06:34, Alexandre Ghiti wrote:

> The SiFive Unmatched board uses the da9063 PMIC for reset: add a restart
> notifier that will execute a small i2c sequence allowing to reset the
> board.
>
> The original implementation comes from Marcus Comstedt and Anders
> Montonen
> (https://forums.sifive.com/t/reboot-command/4721/28).
>
> Signed-off-by: Alexandre Ghiti <[email protected]>
> ---
> drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++
> include/linux/mfd/da9063/core.h | 3 +++
> 2 files changed, 28 insertions(+)
>
> diff --git a/drivers/mfd/da9063-core.c b/drivers/mfd/da9063-core.c
> index df407c3afce3..c87b8d611f20 100644
> --- a/drivers/mfd/da9063-core.c
> +++ b/drivers/mfd/da9063-core.c
> @@ -20,6 +20,7 @@
> #include <linux/mutex.h>
> #include <linux/mfd/core.h>
> #include <linux/regmap.h>
> +#include <linux/reboot.h>
>
> #include <linux/mfd/da9063/core.h>
> #include <linux/mfd/da9063/registers.h>
> @@ -158,6 +159,18 @@ static int da9063_clear_fault_log(struct da9063 *da9063)
> return ret;
> }
>
> +static int da9063_restart_notify(struct notifier_block *this,
> + unsigned long mode, void *cmd)
> +{
> + struct da9063 *da9063 = container_of(this, struct da9063,
> restart_handler);
> +
> + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> +
> + return NOTIFY_DONE;
> +}

I will talk with our HW team to clarify, but this sequence looks to be very
specific to the needs of the platform in question which doesn't feel right to
me. As was mentioned on another thread as well, the watchdog driver already has
a restart function to reset the device (and thus the system), so I don't believe
we should have multiple of these.

For board specific sequences, there are machine quirks I believe which can be
used to handle stuff like this, if this really isn't a generic solution to fit
all cases.

2021-09-29 13:38:45

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 24 September 2021 17:17, Alexandre Ghiti wrote:

> > > +static int da9063_restart_notify(struct notifier_block *this,
> > > + unsigned long mode, void *cmd)
> > > +{
> > > + struct da9063 *da9063 = container_of(this, struct da9063,
> > > restart_handler);
> > > +
> > > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > > +
> > > + return NOTIFY_DONE;
> > > +}
> >
> > I will talk with our HW team to clarify, but this sequence looks to be very
> > specific to the needs of the platform in question which doesn't feel right to
> > me. As was mentioned on another thread as well, the watchdog driver already
> has
> > a restart function to reset the device (and thus the system), so I don't believe
> > we should have multiple of these.
>
> From the discussion that happened here
> https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> support_tab_content,
> it does not seem possible to use the watchdog on a chip whose OTP does
> not set AUTOBOOT. But anyway, I'm looking forward to hearing from the
> HW team :)

So I've discussed this internally and so far it's not completely clear how the
sequence you provided actually performs the reset as you suggest. It certainly
doesn't look like it should, so maybe this relates to an external pin somehow
triggering the restart in this particular scenario? I'd be interested to
understand which event bits are set when the board does restart to understand
what did actually trigger the boot-up.

Regardless of this though, the consensus right now would be to use the RTC as a
wake event to restart the platform. An alarm can be set for a couple of seconds
into the future (or longer if required) and that would provide the event
required to come up from powerdown/shutdown, in the absence of AUTOBOOT being
set in OTP. I believe this would be the safest route to take in this case. You
can then just use the SHUTDOWN bit on CONTROL_F to take down the board.

To reiterate, I believe this should be made a board specific quirk, rather than
as part of the generic MFD core of DA9063, as the timings may vary for other
platforms.

2021-09-30 08:35:56

by David Abdurachmanov

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Wed, Sep 29, 2021 at 4:36 PM Adam Thomson
<[email protected]> wrote:
>
> On 24 September 2021 17:17, Alexandre Ghiti wrote:
>
> > > > +static int da9063_restart_notify(struct notifier_block *this,
> > > > + unsigned long mode, void *cmd)
> > > > +{
> > > > + struct da9063 *da9063 = container_of(this, struct da9063,
> > > > restart_handler);
> > > > +
> > > > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > > > +
> > > > + return NOTIFY_DONE;
> > > > +}
> > >
> > > I will talk with our HW team to clarify, but this sequence looks to be very
> > > specific to the needs of the platform in question which doesn't feel right to
> > > me. As was mentioned on another thread as well, the watchdog driver already
> > has
> > > a restart function to reset the device (and thus the system), so I don't believe
> > > we should have multiple of these.
> >
> > From the discussion that happened here
> > https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> > support_tab_content,
> > it does not seem possible to use the watchdog on a chip whose OTP does
> > not set AUTOBOOT. But anyway, I'm looking forward to hearing from the
> > HW team :)
>
> So I've discussed this internally and so far it's not completely clear how the
> sequence you provided actually performs the reset as you suggest. It certainly
> doesn't look like it should, so maybe this relates to an external pin somehow
> triggering the restart in this particular scenario? I'd be interested to
> understand which event bits are set when the board does restart to understand
> what did actually trigger the boot-up.
>
> Regardless of this though, the consensus right now would be to use the RTC as a
> wake event to restart the platform. An alarm can be set for a couple of seconds
> into the future (or longer if required) and that would provide the event
> required to come up from powerdown/shutdown, in the absence of AUTOBOOT being
> set in OTP. I believe this would be the safest route to take in this case. You
> can then just use the SHUTDOWN bit on CONTROL_F to take down the board.

Today I was looking into OpenBSD DA9063 drivers and they might be
doing what you described for the reset.

dev/fdt/dapmic.c

[..]
241 void
242 dapmic_reset(void)
243 {
244 struct dapmic_softc *sc = dapmic_cd.cd_devs[0];
245 uint8_t reg;
246
247 /* Enable tick alarm wakeup with a one second interval. */
248 reg = dapmic_reg_read(sc, ALARM_MO);
249 reg &= ~ALARM_MO_TICK_TYPE;
250 reg |= ALARM_MO_TICK_WAKE;
251 dapmic_reg_write(sc, ALARM_MO, reg);
252
253 /* Enable tick function. */
254 reg = dapmic_reg_read(sc, ALARM_Y);
255 reg |= ALARM_Y_TICK_ON;
256 dapmic_reg_write(sc, ALARM_Y, reg);
257
258 /* Clear events such that we wake up again. */
259 dapmic_reg_write(sc, EVENT_A, dapmic_reg_read(sc, EVENT_A));
260 dapmic_reg_write(sc, CONTROL_F, CONTROL_F_SHUTDOWN);
261 }
[..]

>
> To reiterate, I believe this should be made a board specific quirk, rather than
> as part of the generic MFD core of DA9063, as the timings may vary for other
> platforms.

Agree. Currently it seems Linux drivers expect DA9063 boards to have
AUTOBOOT ON set in OTP, which is not the case for SiFive Unmatched
(thus issues with reset and WDT).

david

> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv

2021-09-30 09:32:39

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 30 September 2021 08:51, David Abdurachmanov wrote:

> > Regardless of this though, the consensus right now would be to use the RTC as
> a
> > wake event to restart the platform. An alarm can be set for a couple of seconds
> > into the future (or longer if required) and that would provide the event
> > required to come up from powerdown/shutdown, in the absence of
> AUTOBOOT being
> > set in OTP. I believe this would be the safest route to take in this case. You
> > can then just use the SHUTDOWN bit on CONTROL_F to take down the board.
>
> Today I was looking into OpenBSD DA9063 drivers and they might be
> doing what you described for the reset.
>
> dev/fdt/dapmic.c
>
> [..]
> 241 void
> 242 dapmic_reset(void)
> 243 {
> 244 struct dapmic_softc *sc = dapmic_cd.cd_devs[0];
> 245 uint8_t reg;
> 246
> 247 /* Enable tick alarm wakeup with a one second interval. */
> 248 reg = dapmic_reg_read(sc, ALARM_MO);
> 249 reg &= ~ALARM_MO_TICK_TYPE;
> 250 reg |= ALARM_MO_TICK_WAKE;
> 251 dapmic_reg_write(sc, ALARM_MO, reg);
> 252
> 253 /* Enable tick function. */
> 254 reg = dapmic_reg_read(sc, ALARM_Y);
> 255 reg |= ALARM_Y_TICK_ON;
> 256 dapmic_reg_write(sc, ALARM_Y, reg);
> 257
> 258 /* Clear events such that we wake up again. */
> 259 dapmic_reg_write(sc, EVENT_A, dapmic_reg_read(sc, EVENT_A));
> 260 dapmic_reg_write(sc, CONTROL_F, CONTROL_F_SHUTDOWN);
> 261 }
> [..]

This is using the tick alarm which is repeating. That is one option or
alternatively there's a one-shot alarm option which can be used which might be
preferable as you won't see continues events from the RTC when the system boots
up again.

2021-09-30 09:40:53

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Wed, Sep 29, 2021 at 3:33 PM Adam Thomson
<[email protected]> wrote:
>
> On 24 September 2021 17:17, Alexandre Ghiti wrote:
>
> > > > +static int da9063_restart_notify(struct notifier_block *this,
> > > > + unsigned long mode, void *cmd)
> > > > +{
> > > > + struct da9063 *da9063 = container_of(this, struct da9063,
> > > > restart_handler);
> > > > +
> > > > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > > > +
> > > > + return NOTIFY_DONE;
> > > > +}
> > >
> > > I will talk with our HW team to clarify, but this sequence looks to be very
> > > specific to the needs of the platform in question which doesn't feel right to
> > > me. As was mentioned on another thread as well, the watchdog driver already
> > has
> > > a restart function to reset the device (and thus the system), so I don't believe
> > > we should have multiple of these.
> >
> > From the discussion that happened here
> > https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> > support_tab_content,
> > it does not seem possible to use the watchdog on a chip whose OTP does
> > not set AUTOBOOT. But anyway, I'm looking forward to hearing from the
> > HW team :)
>
> So I've discussed this internally and so far it's not completely clear how the
> sequence you provided actually performs the reset as you suggest. It certainly
> doesn't look like it should, so maybe this relates to an external pin somehow
> triggering the restart in this particular scenario? I'd be interested to
> understand which event bits are set when the board does restart to understand
> what did actually trigger the boot-up.
>
> Regardless of this though, the consensus right now would be to use the RTC as a
> wake event to restart the platform. An alarm can be set for a couple of seconds
> into the future (or longer if required) and that would provide the event
> required to come up from powerdown/shutdown, in the absence of AUTOBOOT being
> set in OTP. I believe this would be the safest route to take in this case. You
> can then just use the SHUTDOWN bit on CONTROL_F to take down the board.
>
> To reiterate, I believe this should be made a board specific quirk, rather than
> as part of the generic MFD core of DA9063, as the timings may vary for other
> platforms.

What timings are you referring to? Is the timing you're talking to the
time between the shutdown and the tick that wakes the device up?

Because I have another series ready which uses a new device tree
binding so that platforms that want the reset from the DA9063 can ask
for it via the device tree. And then I could add a property "duration"
that is platform dependent.

2021-09-30 10:28:48

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

Hi David,

On Thu, Sep 30, 2021 at 9:51 AM David Abdurachmanov
<[email protected]> wrote:
>
> On Wed, Sep 29, 2021 at 4:36 PM Adam Thomson
> <[email protected]> wrote:
> >
> > On 24 September 2021 17:17, Alexandre Ghiti wrote:
> >
> > > > > +static int da9063_restart_notify(struct notifier_block *this,
> > > > > + unsigned long mode, void *cmd)
> > > > > +{
> > > > > + struct da9063 *da9063 = container_of(this, struct da9063,
> > > > > restart_handler);
> > > > > +
> > > > > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > > > > +
> > > > > + return NOTIFY_DONE;
> > > > > +}
> > > >
> > > > I will talk with our HW team to clarify, but this sequence looks to be very
> > > > specific to the needs of the platform in question which doesn't feel right to
> > > > me. As was mentioned on another thread as well, the watchdog driver already
> > > has
> > > > a restart function to reset the device (and thus the system), so I don't believe
> > > > we should have multiple of these.
> > >
> > > From the discussion that happened here
> > > https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> > > support_tab_content,
> > > it does not seem possible to use the watchdog on a chip whose OTP does
> > > not set AUTOBOOT. But anyway, I'm looking forward to hearing from the
> > > HW team :)
> >
> > So I've discussed this internally and so far it's not completely clear how the
> > sequence you provided actually performs the reset as you suggest. It certainly
> > doesn't look like it should, so maybe this relates to an external pin somehow
> > triggering the restart in this particular scenario? I'd be interested to
> > understand which event bits are set when the board does restart to understand
> > what did actually trigger the boot-up.
> >
> > Regardless of this though, the consensus right now would be to use the RTC as a
> > wake event to restart the platform. An alarm can be set for a couple of seconds
> > into the future (or longer if required) and that would provide the event
> > required to come up from powerdown/shutdown, in the absence of AUTOBOOT being
> > set in OTP. I believe this would be the safest route to take in this case. You
> > can then just use the SHUTDOWN bit on CONTROL_F to take down the board.
>
> Today I was looking into OpenBSD DA9063 drivers and they might be
> doing what you described for the reset.
>
> dev/fdt/dapmic.c
>
> [..]
> 241 void
> 242 dapmic_reset(void)
> 243 {
> 244 struct dapmic_softc *sc = dapmic_cd.cd_devs[0];
> 245 uint8_t reg;
> 246
> 247 /* Enable tick alarm wakeup with a one second interval. */
> 248 reg = dapmic_reg_read(sc, ALARM_MO);
> 249 reg &= ~ALARM_MO_TICK_TYPE;
> 250 reg |= ALARM_MO_TICK_WAKE;
> 251 dapmic_reg_write(sc, ALARM_MO, reg);
> 252
> 253 /* Enable tick function. */
> 254 reg = dapmic_reg_read(sc, ALARM_Y);
> 255 reg |= ALARM_Y_TICK_ON;
> 256 dapmic_reg_write(sc, ALARM_Y, reg);
> 257
> 258 /* Clear events such that we wake up again. */
> 259 dapmic_reg_write(sc, EVENT_A, dapmic_reg_read(sc, EVENT_A));
> 260 dapmic_reg_write(sc, CONTROL_F, CONTROL_F_SHUTDOWN);
> 261 }
> [..]
>

Thanks for the pointer! I have just tested this sequence from the
u-boot shell, it resets the board correctly. But then if we try to
power down the board by a long press to the corresponding button on
the board within 16 seconds, it resets the board: so we cannot
shutdown the board in the next 16 seconds that follow this sequence.

Maybe that can be resolved by using the one-shot alarm as described by
Adam, I'll try to find that in the datasheet.

Thanks

Alex

> >
> > To reiterate, I believe this should be made a board specific quirk, rather than
> > as part of the generic MFD core of DA9063, as the timings may vary for other
> > platforms.
>
> Agree. Currently it seems Linux drivers expect DA9063 boards to have
> AUTOBOOT ON set in OTP, which is not the case for SiFive Unmatched
> (thus issues with reset and WDT).
>
> david
>
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

2021-09-30 12:19:09

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

Hi Adam,

Thank you for your answer.

On Wed, Sep 29, 2021 at 3:33 PM Adam Thomson
<[email protected]> wrote:
>
> On 24 September 2021 17:17, Alexandre Ghiti wrote:
>
> > > > +static int da9063_restart_notify(struct notifier_block *this,
> > > > + unsigned long mode, void *cmd)
> > > > +{
> > > > + struct da9063 *da9063 = container_of(this, struct da9063,
> > > > restart_handler);
> > > > +
> > > > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > > > +
> > > > + return NOTIFY_DONE;
> > > > +}
> > >
> > > I will talk with our HW team to clarify, but this sequence looks to be very
> > > specific to the needs of the platform in question which doesn't feel right to
> > > me. As was mentioned on another thread as well, the watchdog driver already
> > has
> > > a restart function to reset the device (and thus the system), so I don't believe
> > > we should have multiple of these.
> >
> > From the discussion that happened here
> > https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> > support_tab_content,
> > it does not seem possible to use the watchdog on a chip whose OTP does
> > not set AUTOBOOT. But anyway, I'm looking forward to hearing from the
> > HW team :)
>
> So I've discussed this internally and so far it's not completely clear how the
> sequence you provided actually performs the reset as you suggest. It certainly
> doesn't look like it should, so maybe this relates to an external pin somehow
> triggering the restart in this particular scenario? I'd be interested to
> understand which event bits are set when the board does restart to understand
> what did actually trigger the boot-up.

After clearing all those registers and a reset as done in this patch,
I get the following values:

FAULT_LOG: 0x00
EVENT_A: 0x10 => As per the datasheet, "Sequencer reached final
position caused event" ?
EVENT_B: 0x00
EVENT_C: 0x00
EVENT_D: 0x00

Do you need any other info?

>
> Regardless of this though, the consensus right now would be to use the RTC as a
> wake event to restart the platform. An alarm can be set for a couple of seconds
> into the future (or longer if required) and that would provide the event
> required to come up from powerdown/shutdown, in the absence of AUTOBOOT being
> set in OTP. I believe this would be the safest route to take in this case. You
> can then just use the SHUTDOWN bit on CONTROL_F to take down the board.
>
> To reiterate, I believe this should be made a board specific quirk, rather than
> as part of the generic MFD core of DA9063, as the timings may vary for other
> platforms.

Alex

2021-09-30 12:25:56

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 30 September 2021 10:38, Alexandre Ghiti wrote:

> > Regardless of this though, the consensus right now would be to use the RTC as
> a
> > wake event to restart the platform. An alarm can be set for a couple of seconds
> > into the future (or longer if required) and that would provide the event
> > required to come up from powerdown/shutdown, in the absence of
> AUTOBOOT being
> > set in OTP. I believe this would be the safest route to take in this case. You
> > can then just use the SHUTDOWN bit on CONTROL_F to take down the board.
> >
> > To reiterate, I believe this should be made a board specific quirk, rather than
> > as part of the generic MFD core of DA9063, as the timings may vary for other
> > platforms.
>
> What timings are you referring to? Is the timing you're talking to the
> time between the shutdown and the tick that wakes the device up?

That was one of the considerations.....

> Because I have another series ready which uses a new device tree
> binding so that platforms that want the reset from the DA9063 can ask
> for it via the device tree. And then I could add a property "duration"
> that is platform dependent.

... but having thought further on this. Say you use this approach within the
kernel then you're limiting that platform to immediate reboots are you not?
What happens if you say wanted to shutdown the platform, then reboot at some
more distant future point using the RTC? In this case that option is then off
the table as the kernel hard codes this and overrides any existing alarm.

I don't believe there's anything to stop us configuring an RTC alarm from
user-space, prior to the reboot/shutdown being triggered, and that can act as an
'immediate' restart or, if the user requires, a delayed wake could also used if
necessary.

Of course none of this resolves the watchdog case if that's used, but am not
sure if that's included/used in your setup.

2021-10-04 12:07:49

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Thu, Sep 30, 2021 at 12:25 PM Alexandre Ghiti
<[email protected]> wrote:
>
> Hi David,
>
> On Thu, Sep 30, 2021 at 9:51 AM David Abdurachmanov
> <[email protected]> wrote:
> >
> > On Wed, Sep 29, 2021 at 4:36 PM Adam Thomson
> > <[email protected]> wrote:
> > >
> > > On 24 September 2021 17:17, Alexandre Ghiti wrote:
> > >
> > > > > > +static int da9063_restart_notify(struct notifier_block *this,
> > > > > > + unsigned long mode, void *cmd)
> > > > > > +{
> > > > > > + struct da9063 *da9063 = container_of(this, struct da9063,
> > > > > > restart_handler);
> > > > > > +
> > > > > > + regmap_write(da9063->regmap, DA9063_REG_PAGE_CON, 0x00);
> > > > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_F, 0x04);
> > > > > > + regmap_write(da9063->regmap, DA9063_REG_CONTROL_A, 0x68);
> > > > > > +
> > > > > > + return NOTIFY_DONE;
> > > > > > +}
> > > > >
> > > > > I will talk with our HW team to clarify, but this sequence looks to be very
> > > > > specific to the needs of the platform in question which doesn't feel right to
> > > > > me. As was mentioned on another thread as well, the watchdog driver already
> > > > has
> > > > > a restart function to reset the device (and thus the system), so I don't believe
> > > > > we should have multiple of these.
> > > >
> > > > From the discussion that happened here
> > > > https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> > > > support_tab_content,
> > > > it does not seem possible to use the watchdog on a chip whose OTP does
> > > > not set AUTOBOOT. But anyway, I'm looking forward to hearing from the
> > > > HW team :)
> > >
> > > So I've discussed this internally and so far it's not completely clear how the
> > > sequence you provided actually performs the reset as you suggest. It certainly
> > > doesn't look like it should, so maybe this relates to an external pin somehow
> > > triggering the restart in this particular scenario? I'd be interested to
> > > understand which event bits are set when the board does restart to understand
> > > what did actually trigger the boot-up.
> > >
> > > Regardless of this though, the consensus right now would be to use the RTC as a
> > > wake event to restart the platform. An alarm can be set for a couple of seconds
> > > into the future (or longer if required) and that would provide the event
> > > required to come up from powerdown/shutdown, in the absence of AUTOBOOT being
> > > set in OTP. I believe this would be the safest route to take in this case. You
> > > can then just use the SHUTDOWN bit on CONTROL_F to take down the board.
> >
> > Today I was looking into OpenBSD DA9063 drivers and they might be
> > doing what you described for the reset.
> >
> > dev/fdt/dapmic.c
> >
> > [..]
> > 241 void
> > 242 dapmic_reset(void)
> > 243 {
> > 244 struct dapmic_softc *sc = dapmic_cd.cd_devs[0];
> > 245 uint8_t reg;
> > 246
> > 247 /* Enable tick alarm wakeup with a one second interval. */
> > 248 reg = dapmic_reg_read(sc, ALARM_MO);
> > 249 reg &= ~ALARM_MO_TICK_TYPE;
> > 250 reg |= ALARM_MO_TICK_WAKE;
> > 251 dapmic_reg_write(sc, ALARM_MO, reg);
> > 252
> > 253 /* Enable tick function. */
> > 254 reg = dapmic_reg_read(sc, ALARM_Y);
> > 255 reg |= ALARM_Y_TICK_ON;
> > 256 dapmic_reg_write(sc, ALARM_Y, reg);
> > 257
> > 258 /* Clear events such that we wake up again. */
> > 259 dapmic_reg_write(sc, EVENT_A, dapmic_reg_read(sc, EVENT_A));
> > 260 dapmic_reg_write(sc, CONTROL_F, CONTROL_F_SHUTDOWN);
> > 261 }
> > [..]
> >
>
> Thanks for the pointer! I have just tested this sequence from the
> u-boot shell, it resets the board correctly. But then if we try to
> power down the board by a long press to the corresponding button on
> the board within 16 seconds, it resets the board: so we cannot
> shutdown the board in the next 16 seconds that follow this sequence.
>
> Maybe that can be resolved by using the one-shot alarm as described by
> Adam, I'll try to find that in the datasheet.

After configuring the one-shot alarm, I still have those intempestive
reboots if I try to power down the board by a long press within 16
seconds. The only thing I found in the datasheet regarding this timing
is in case of power undervoltage, not sure how this is linked to what
I see.

@Adam Thomson Any ideas?

Thanks,

Alex

>
> Thanks
>
> Alex
>
> > >
> > > To reiterate, I believe this should be made a board specific quirk, rather than
> > > as part of the generic MFD core of DA9063, as the timings may vary for other
> > > platforms.
> >
> > Agree. Currently it seems Linux drivers expect DA9063 boards to have
> > AUTOBOOT ON set in OTP, which is not the case for SiFive Unmatched
> > (thus issues with reset and WDT).
> >
> > david
> >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv

2021-10-04 17:53:36

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 30 September 2021 10:55, Alexandre Ghiti wrote:

> > So I've discussed this internally and so far it's not completely clear how the
> > sequence you provided actually performs the reset as you suggest. It certainly
> > doesn't look like it should, so maybe this relates to an external pin somehow
> > triggering the restart in this particular scenario? I'd be interested to
> > understand which event bits are set when the board does restart to
> understand
> > what did actually trigger the boot-up.
>
> After clearing all those registers and a reset as done in this patch,
> I get the following values:
>
> FAULT_LOG: 0x00
> EVENT_A: 0x10 => As per the datasheet, "Sequencer reached final
> position caused event" ?
> EVENT_B: 0x00
> EVENT_C: 0x00
> EVENT_D: 0x00
>
> Do you need any other info?

Sadly that didn't show anything useful. The 'Sequencer' event will happen on
various power transitions so doesn't give us any clue as to the trigger here.
Think the next step would be to probe the various wake related pins of DA9063
to see their states during this process just in case there's anything useful
there, although I'd expect the event bits to mirror what was happening.

2021-10-04 22:26:06

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 04 October 2021 13:05, Alexandre Ghiti wrote:

> > > Today I was looking into OpenBSD DA9063 drivers and they might be
> > > doing what you described for the reset.
> > >
> > > dev/fdt/dapmic.c
> > >
> > > [..]
> > > 241 void
> > > 242 dapmic_reset(void)
> > > 243 {
> > > 244 struct dapmic_softc *sc = dapmic_cd.cd_devs[0];
> > > 245 uint8_t reg;
> > > 246
> > > 247 /* Enable tick alarm wakeup with a one second interval. */
> > > 248 reg = dapmic_reg_read(sc, ALARM_MO);
> > > 249 reg &= ~ALARM_MO_TICK_TYPE;
> > > 250 reg |= ALARM_MO_TICK_WAKE;
> > > 251 dapmic_reg_write(sc, ALARM_MO, reg);
> > > 252
> > > 253 /* Enable tick function. */
> > > 254 reg = dapmic_reg_read(sc, ALARM_Y);
> > > 255 reg |= ALARM_Y_TICK_ON;
> > > 256 dapmic_reg_write(sc, ALARM_Y, reg);
> > > 257
> > > 258 /* Clear events such that we wake up again. */
> > > 259 dapmic_reg_write(sc, EVENT_A, dapmic_reg_read(sc, EVENT_A));
> > > 260 dapmic_reg_write(sc, CONTROL_F, CONTROL_F_SHUTDOWN);
> > > 261 }
> > > [..]
> > >
> >
> > Thanks for the pointer! I have just tested this sequence from the
> > u-boot shell, it resets the board correctly. But then if we try to
> > power down the board by a long press to the corresponding button on
> > the board within 16 seconds, it resets the board: so we cannot
> > shutdown the board in the next 16 seconds that follow this sequence.
> >
> > Maybe that can be resolved by using the one-shot alarm as described by
> > Adam, I'll try to find that in the datasheet.
>
> After configuring the one-shot alarm, I still have those intempestive
> reboots if I try to power down the board by a long press within 16
> seconds. The only thing I found in the datasheet regarding this timing
> is in case of power undervoltage, not sure how this is linked to what
> I see.
>
> @Adam Thomson Any ideas?

Nothing immediately springs to mind. Can you confirm this is the nONKEY long
press that you're attempting here, which is resetting the board rather than
shutting down?

Also, would you able to again provide events and fault log when this unwanted
reset occurs, just in case there's anything there to give a clue. Can then
discuss internally to see if we can ascertain what might be happening here.

2021-10-05 13:47:37

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Mon, Oct 4, 2021 at 5:11 PM Adam Thomson
<[email protected]> wrote:
>
> On 04 October 2021 13:05, Alexandre Ghiti wrote:
>
> > > > Today I was looking into OpenBSD DA9063 drivers and they might be
> > > > doing what you described for the reset.
> > > >
> > > > dev/fdt/dapmic.c
> > > >
> > > > [..]
> > > > 241 void
> > > > 242 dapmic_reset(void)
> > > > 243 {
> > > > 244 struct dapmic_softc *sc = dapmic_cd.cd_devs[0];
> > > > 245 uint8_t reg;
> > > > 246
> > > > 247 /* Enable tick alarm wakeup with a one second interval. */
> > > > 248 reg = dapmic_reg_read(sc, ALARM_MO);
> > > > 249 reg &= ~ALARM_MO_TICK_TYPE;
> > > > 250 reg |= ALARM_MO_TICK_WAKE;
> > > > 251 dapmic_reg_write(sc, ALARM_MO, reg);
> > > > 252
> > > > 253 /* Enable tick function. */
> > > > 254 reg = dapmic_reg_read(sc, ALARM_Y);
> > > > 255 reg |= ALARM_Y_TICK_ON;
> > > > 256 dapmic_reg_write(sc, ALARM_Y, reg);
> > > > 257
> > > > 258 /* Clear events such that we wake up again. */
> > > > 259 dapmic_reg_write(sc, EVENT_A, dapmic_reg_read(sc, EVENT_A));
> > > > 260 dapmic_reg_write(sc, CONTROL_F, CONTROL_F_SHUTDOWN);
> > > > 261 }
> > > > [..]
> > > >
> > >
> > > Thanks for the pointer! I have just tested this sequence from the
> > > u-boot shell, it resets the board correctly. But then if we try to
> > > power down the board by a long press to the corresponding button on
> > > the board within 16 seconds, it resets the board: so we cannot
> > > shutdown the board in the next 16 seconds that follow this sequence.
> > >
> > > Maybe that can be resolved by using the one-shot alarm as described by
> > > Adam, I'll try to find that in the datasheet.
> >
> > After configuring the one-shot alarm, I still have those intempestive
> > reboots if I try to power down the board by a long press within 16
> > seconds. The only thing I found in the datasheet regarding this timing
> > is in case of power undervoltage, not sure how this is linked to what
> > I see.
> >
> > @Adam Thomson Any ideas?
>
> Nothing immediately springs to mind. Can you confirm this is the nONKEY long
> press that you're attempting here, which is resetting the board rather than
> shutting down?

Yes, this is the nONKEY long press that, if done within ~16sec after
the board is reset using the alarm, resets the board instead of
shutting it down.

>
> Also, would you able to again provide events and fault log when this unwanted
> reset occurs, just in case there's anything there to give a clue. Can then
> discuss internally to see if we can ascertain what might be happening here.

FAULT_LOG = 0x60
EVENT_A = 0x12
EVENT_B to EVENT_D = 0

But I'm unsure of those values since they are the same after the reset
triggered by the one-shot alarm *and* if I clear EVENT_A, the
intempestive reboot does not appear!

Thanks,

Alex

2021-10-06 09:31:36

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 05 October 2021 14:43, Alexandre Ghiti wrote:

> > > > Thanks for the pointer! I have just tested this sequence from the
> > > > u-boot shell, it resets the board correctly. But then if we try to
> > > > power down the board by a long press to the corresponding button on
> > > > the board within 16 seconds, it resets the board: so we cannot
> > > > shutdown the board in the next 16 seconds that follow this sequence.
> > > >
> > > > Maybe that can be resolved by using the one-shot alarm as described by
> > > > Adam, I'll try to find that in the datasheet.
> > >
> > > After configuring the one-shot alarm, I still have those intempestive
> > > reboots if I try to power down the board by a long press within 16
> > > seconds. The only thing I found in the datasheet regarding this timing
> > > is in case of power undervoltage, not sure how this is linked to what
> > > I see.
> > >
> > > @Adam Thomson Any ideas?
> >
> > Nothing immediately springs to mind. Can you confirm this is the nONKEY long
> > press that you're attempting here, which is resetting the board rather than
> > shutting down?
>
> Yes, this is the nONKEY long press that, if done within ~16sec after
> the board is reset using the alarm, resets the board instead of
> shutting it down.
>
> >
> > Also, would you able to again provide events and fault log when this unwanted
> > reset occurs, just in case there's anything there to give a clue. Can then
> > discuss internally to see if we can ascertain what might be happening here.
>
> FAULT_LOG = 0x60
> EVENT_A = 0x12
> EVENT_B to EVENT_D = 0
>
> But I'm unsure of those values since they are the same after the reset
> triggered by the one-shot alarm *and* if I clear EVENT_A, the
> intempestive reboot does not appear!

Thanks for the info. So we believe, based on the event registers values
provided, it is the RTC event as that's not cleared by a power-cycle (it's in
the always-on domain). The other test would be to mask this event immediately
after an RTC based reboot and see if the long key-press then shuts down the
device. I suspect it would in that case, as per you clearing the event.

I'm still curious as to the 16 seconds though. Would that be when the kernel
finally starts and masks/clears events (seems quite a long time)?

2021-10-06 11:38:26

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On Wed, Oct 6, 2021 at 11:30 AM Adam Thomson
<[email protected]> wrote:
>
> On 05 October 2021 14:43, Alexandre Ghiti wrote:
>
> > > > > Thanks for the pointer! I have just tested this sequence from the
> > > > > u-boot shell, it resets the board correctly. But then if we try to
> > > > > power down the board by a long press to the corresponding button on
> > > > > the board within 16 seconds, it resets the board: so we cannot
> > > > > shutdown the board in the next 16 seconds that follow this sequence.
> > > > >
> > > > > Maybe that can be resolved by using the one-shot alarm as described by
> > > > > Adam, I'll try to find that in the datasheet.
> > > >
> > > > After configuring the one-shot alarm, I still have those intempestive
> > > > reboots if I try to power down the board by a long press within 16
> > > > seconds. The only thing I found in the datasheet regarding this timing
> > > > is in case of power undervoltage, not sure how this is linked to what
> > > > I see.
> > > >
> > > > @Adam Thomson Any ideas?
> > >
> > > Nothing immediately springs to mind. Can you confirm this is the nONKEY long
> > > press that you're attempting here, which is resetting the board rather than
> > > shutting down?
> >
> > Yes, this is the nONKEY long press that, if done within ~16sec after
> > the board is reset using the alarm, resets the board instead of
> > shutting it down.
> >
> > >
> > > Also, would you able to again provide events and fault log when this unwanted
> > > reset occurs, just in case there's anything there to give a clue. Can then
> > > discuss internally to see if we can ascertain what might be happening here.
> >
> > FAULT_LOG = 0x60
> > EVENT_A = 0x12
> > EVENT_B to EVENT_D = 0
> >
> > But I'm unsure of those values since they are the same after the reset
> > triggered by the one-shot alarm *and* if I clear EVENT_A, the
> > intempestive reboot does not appear!
>
> Thanks for the info. So we believe, based on the event registers values
> provided, it is the RTC event as that's not cleared by a power-cycle (it's in
> the always-on domain). The other test would be to mask this event immediately
> after an RTC based reboot and see if the long key-press then shuts down the
> device. I suspect it would in that case, as per you clearing the event.

Indeed if I mask the RTC alarm in IRQ_MASK_A, the intempestive reboot
disappears. But that's not something we can do: the reset driver will
actually be implemented in openSBI at some point where the devices are
probed on-demand (the same applies to u-boot I think), so we cannot
clear or mask anything at boot.

>
> I'm still curious as to the 16 seconds though. Would that be when the kernel
> finally starts and masks/clears events (seems quite a long time)?

No, the kernel is not up yet. Maybe 16sec is not the right value, I
may have been influenced by the discussion here
https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-support_tab_content.

It seems there is some consensus about having this reset driver be a
SiFive Unmatched board specific thing: quid of the sequence I propose
in this patch then? It does not mess with the RTC registers, it
reboots reliably and there's no intempestive reboot: is it dangerous
to use? Or do you have another alternative?

Thanks,

Alex

2021-10-08 09:49:02

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 06 October 2021 12:36, Alexandre Ghiti wrote:

> > Thanks for the info. So we believe, based on the event registers values
> > provided, it is the RTC event as that's not cleared by a power-cycle (it's in
> > the always-on domain). The other test would be to mask this event
> immediately
> > after an RTC based reboot and see if the long key-press then shuts down the
> > device. I suspect it would in that case, as per you clearing the event.
>
> Indeed if I mask the RTC alarm in IRQ_MASK_A, the intempestive reboot
> disappears. But that's not something we can do: the reset driver will
> actually be implemented in openSBI at some point where the devices are
> probed on-demand (the same applies to u-boot I think), so we cannot
> clear or mask anything at boot.
>
> >
> > I'm still curious as to the 16 seconds though. Would that be when the kernel
> > finally starts and masks/clears events (seems quite a long time)?
>
> No, the kernel is not up yet. Maybe 16sec is not the right value, I
> may have been influenced by the discussion here
> https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> support_tab_content.
>
> It seems there is some consensus about having this reset driver be a
> SiFive Unmatched board specific thing: quid of the sequence I propose
> in this patch then? It does not mess with the RTC registers, it
> reboots reliably and there's no intempestive reboot: is it dangerous
> to use? Or do you have another alternative?

Yes, a board specific implementation would be the way to go. We're just checking
through the sequence again to be absolutely sure of any pitfalls that may
present themselves, and will get back to you when we have something more.

2021-10-12 10:36:03

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 08 October 2021 10:46, Adam Thomson wrote:

> > > Thanks for the info. So we believe, based on the event registers values
> > > provided, it is the RTC event as that's not cleared by a power-cycle (it's in
> > > the always-on domain). The other test would be to mask this event
> > immediately
> > > after an RTC based reboot and see if the long key-press then shuts down the
> > > device. I suspect it would in that case, as per you clearing the event.
> >
> > Indeed if I mask the RTC alarm in IRQ_MASK_A, the intempestive reboot
> > disappears. But that's not something we can do: the reset driver will
> > actually be implemented in openSBI at some point where the devices are
> > probed on-demand (the same applies to u-boot I think), so we cannot
> > clear or mask anything at boot.
> >
> > >
> > > I'm still curious as to the 16 seconds though. Would that be when the kernel
> > > finally starts and masks/clears events (seems quite a long time)?
> >
> > No, the kernel is not up yet. Maybe 16sec is not the right value, I
> > may have been influenced by the discussion here
> > https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> > support_tab_content.
> >
> > It seems there is some consensus about having this reset driver be a
> > SiFive Unmatched board specific thing: quid of the sequence I propose
> > in this patch then? It does not mess with the RTC registers, it
> > reboots reliably and there's no intempestive reboot: is it dangerous
> > to use? Or do you have another alternative?
>
> Yes, a board specific implementation would be the way to go. We're just checking
> through the sequence again to be absolutely sure of any pitfalls that may
> present themselves, and will get back to you when we have something more.

So having examined your sequence again it's now clearer as to what is happening.
With the sequence you provided this is only a partial reset whereby all of the
output rails are sequenced down then up again and restored to OTP voltages.
However the remainder of the chip settings aren't reset as this isn't a true
reset of the device going through full reload from OTP, so for example settings
of regulator mode GPIO states, or IRQ mask bits would persist on the restart,
which could have implications on system operation.

In addition the only bits of interest for you should be:

- CONTROL_F (0x13)
WAKE_UP (BIT 2) = 1
- CONTROL_A (0x0E)
SYSTEM_EN (BIT 0) = 0

Setting those two bits should be enough to trigger the partial reset sequence.
The other bits you had in your sequence don't seem to be necessary or relevant.

One final caveat to this approach is that there is a 16s internal timer (as you
noted before, VDD_START) which is started when the device moves to ACTIVE mode.
When that 16s timer expires the device will clear the WAKE_UP bit automatically.
This means there's the outside chance that you could try the reset command
sequence above around the same time, and that could mean you set the WAKE_UP
bit, but it's immediately cleared again by this timer expiry before the
SYSTEM_EN bit is set low. In that case there would be a need for an external
event (e.g. ONKEY) to kick the system to start again.

2021-10-14 20:55:10

by Alexandre Ghiti

[permalink] [raw]
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

Hi Adam,

On Tue, Oct 12, 2021 at 12:33 PM Adam Thomson
<[email protected]> wrote:
>
> On 08 October 2021 10:46, Adam Thomson wrote:
>
> > > > Thanks for the info. So we believe, based on the event registers values
> > > > provided, it is the RTC event as that's not cleared by a power-cycle (it's in
> > > > the always-on domain). The other test would be to mask this event
> > > immediately
> > > > after an RTC based reboot and see if the long key-press then shuts down the
> > > > device. I suspect it would in that case, as per you clearing the event.
> > >
> > > Indeed if I mask the RTC alarm in IRQ_MASK_A, the intempestive reboot
> > > disappears. But that's not something we can do: the reset driver will
> > > actually be implemented in openSBI at some point where the devices are
> > > probed on-demand (the same applies to u-boot I think), so we cannot
> > > clear or mask anything at boot.
> > >
> > > >
> > > > I'm still curious as to the 16 seconds though. Would that be when the kernel
> > > > finally starts and masks/clears events (seems quite a long time)?
> > >
> > > No, the kernel is not up yet. Maybe 16sec is not the right value, I
> > > may have been influenced by the discussion here
> > > https://www.dialog-semiconductor.com/products/pmics?post_id=10052#tab-
> > > support_tab_content.
> > >
> > > It seems there is some consensus about having this reset driver be a
> > > SiFive Unmatched board specific thing: quid of the sequence I propose
> > > in this patch then? It does not mess with the RTC registers, it
> > > reboots reliably and there's no intempestive reboot: is it dangerous
> > > to use? Or do you have another alternative?
> >
> > Yes, a board specific implementation would be the way to go. We're just checking
> > through the sequence again to be absolutely sure of any pitfalls that may
> > present themselves, and will get back to you when we have something more.
>
> So having examined your sequence again it's now clearer as to what is happening.
> With the sequence you provided this is only a partial reset whereby all of the
> output rails are sequenced down then up again and restored to OTP voltages.
> However the remainder of the chip settings aren't reset as this isn't a true
> reset of the device going through full reload from OTP, so for example settings
> of regulator mode GPIO states, or IRQ mask bits would persist on the restart,
> which could have implications on system operation.

Ok, it's not perfect but I think those are settings that will get
reinitialized by the corresponding drivers while booting, contrary to
the RTC registers which are clobbered by the other method.

>
> In addition the only bits of interest for you should be:
>
> - CONTROL_F (0x13)
> WAKE_UP (BIT 2) = 1
> - CONTROL_A (0x0E)
> SYSTEM_EN (BIT 0) = 0
>
> Setting those two bits should be enough to trigger the partial reset sequence.
> The other bits you had in your sequence don't seem to be necessary or relevant.
>
> One final caveat to this approach is that there is a 16s internal timer (as you
> noted before, VDD_START) which is started when the device moves to ACTIVE mode.
> When that 16s timer expires the device will clear the WAKE_UP bit automatically.
> This means there's the outside chance that you could try the reset command
> sequence above around the same time, and that could mean you set the WAKE_UP
> bit, but it's immediately cleared again by this timer expiry before the
> SYSTEM_EN bit is set low. In that case there would be a need for an external
> event (e.g. ONKEY) to kick the system to start again.

Ok, the risk exists but the window is quite small.

After all, the solution I first proposed is not perfect, but now we
know why it works and IMO it has less drawbacks than using the RTC
registers, so I think we should go for this solution. I'll see if I
can help Nikita implement this directly in openSBI.

@Adam Thomson I had migrated the DA9063 device tree bindings to yaml,
I'll push that soon. Thanks for all your help, much appreciated.

Thanks,

Alex

2021-10-15 16:44:38

by Adam Thomson

[permalink] [raw]
Subject: RE: [PATCH] drivers: mfd: da9063: Add restart notifier implementation

On 14 October 2021 16:51, Alexandre Ghiti wrote:

> > So having examined your sequence again it's now clearer as to what is
> happening.
> > With the sequence you provided this is only a partial reset whereby all of the
> > output rails are sequenced down then up again and restored to OTP voltages.
> > However the remainder of the chip settings aren't reset as this isn't a true
> > reset of the device going through full reload from OTP, so for example settings
> > of regulator mode GPIO states, or IRQ mask bits would persist on the restart,
> > which could have implications on system operation.
>
> Ok, it's not perfect but I think those are settings that will get
> reinitialized by the corresponding drivers while booting, contrary to
> the RTC registers which are clobbered by the other method.

Well I guess that depends on which DA9063 drivers are being loaded and what is
populated in DT. This is something you'll need to consider now and with future
updates around this board to make sure nothing untoward is happening as a side
effect.

>
> >
> > In addition the only bits of interest for you should be:
> >
> > - CONTROL_F (0x13)
> > WAKE_UP (BIT 2) = 1
> > - CONTROL_A (0x0E)
> > SYSTEM_EN (BIT 0) = 0
> >
> > Setting those two bits should be enough to trigger the partial reset sequence.
> > The other bits you had in your sequence don't seem to be necessary or
> relevant.
> >
> > One final caveat to this approach is that there is a 16s internal timer (as you
> > noted before, VDD_START) which is started when the device moves to ACTIVE
> mode.
> > When that 16s timer expires the device will clear the WAKE_UP bit
> automatically.
> > This means there's the outside chance that you could try the reset command
> > sequence above around the same time, and that could mean you set the
> WAKE_UP
> > bit, but it's immediately cleared again by this timer expiry before the
> > SYSTEM_EN bit is set low. In that case there would be a need for an external
> > event (e.g. ONKEY) to kick the system to start again.
>
> Ok, the risk exists but the window is quite small.
>
> After all, the solution I first proposed is not perfect, but now we
> know why it works and IMO it has less drawbacks than using the RTC
> registers, so I think we should go for this solution. I'll see if I
> can help Nikita implement this directly in openSBI.

Personally, if it was possible I think the RTC approach would be best as it's a
full reset and to me is far safer with regards to potential side effects, but as
that's not on the table then this seems the only other approach in your case.

> @Adam Thomson I had migrated the DA9063 device tree bindings to yaml,
> I'll push that soon. Thanks for all your help, much appreciated.

No problem and thanks for making that update. I'll take a look when the
changes are available.