| Message ID | 20210921053356.1705833-1-alexandre.ghiti@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: mfd: da9063: Add restart notifier implementation | expand |
On Tue, Sep 21, 2021 at 11:04 AM Alexandre Ghiti <alexandre.ghiti@canonical.com> 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 <alexandre.ghiti@canonical.com> > --- > 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 >
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 <alexandre.ghiti@canonical.com> 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.
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 <anup@brainfault.org> wrote: > > On Tue, Sep 21, 2021 at 11:04 AM Alexandre Ghiti > <alexandre.ghiti@canonical.com> 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 <alexandre.ghiti@canonical.com> > > --- > > 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 > >
On Tue, Sep 21, 2021 at 12:25 PM Ben Dooks <ben.dooks@codethink.co.uk> 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 <alexandre.ghiti@canonical.com> > > 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
On Tue, Sep 21, 2021 at 1:33 PM Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > On Tue, Sep 21, 2021 at 12:25 PM Ben Dooks <ben.dooks@codethink.co.uk> 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 <alexandre.ghiti@canonical.com> > > > > 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
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 <alexandre.ghiti@canonical.com> > --- > 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.
Hi Adam, On Fri, Sep 24, 2021 at 5:04 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> 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 <alexandre.ghiti@canonical.com> > > --- > > 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. >
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.
On Wed, Sep 29, 2021 at 4:36 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> 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 > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
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.
On Wed, Sep 29, 2021 at 3:33 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> 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.
Hi Adam, Thank you for your answer. On Wed, Sep 29, 2021 at 3:33 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> 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
Hi David, On Thu, Sep 30, 2021 at 9:51 AM David Abdurachmanov <david.abdurachmanov@gmail.com> wrote: > > On Wed, Sep 29, 2021 at 4:36 PM Adam Thomson > <Adam.Thomson.Opensource@diasemi.com> 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 > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
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.
On Thu, Sep 30, 2021 at 12:25 PM Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > Hi David, > > On Thu, Sep 30, 2021 at 9:51 AM David Abdurachmanov > <david.abdurachmanov@gmail.com> wrote: > > > > On Wed, Sep 29, 2021 at 4:36 PM Adam Thomson > > <Adam.Thomson.Opensource@diasemi.com> 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 > > > linux-riscv@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
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.
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.
On Mon, Oct 4, 2021 at 5:11 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> 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
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)?
On Wed, Oct 6, 2021 at 11:30 AM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> 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
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.
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.
Hi Adam, On Tue, Oct 12, 2021 at 12:33 PM Adam Thomson <Adam.Thomson.Opensource@diasemi.com> 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
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.
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);
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 <alexandre.ghiti@canonical.com> --- drivers/mfd/da9063-core.c | 25 +++++++++++++++++++++++++ include/linux/mfd/da9063/core.h | 3 +++ 2 files changed, 28 insertions(+)