LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alexandre Ghiti <alexandre.ghiti@canonical.com>
To: Ben Dooks <ben.dooks@codethink.co.uk>
Cc: Support Opensource <support.opensource@diasemi.com>,
	Lee Jones <lee.jones@linaro.org>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation
Date: Tue, 21 Sep 2021 13:33:34 +0200	[thread overview]
Message-ID: <CA+zEjCsPwGFng73OJShEc2g6wW1SOKwX7XYFqej_vkJKKUxL0A@mail.gmail.com> (raw)
In-Reply-To: <28072b8e-2c32-e67d-19d3-026913c0bc7f@codethink.co.uk>

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

  reply	other threads:[~2021-09-21 11:33 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21  5:33 Alexandre Ghiti
2021-09-21 10:16 ` Anup Patel
2021-09-21 11:20   ` Alexandre Ghiti
2021-09-21 10:25 ` Ben Dooks
2021-09-21 11:33   ` Alexandre Ghiti [this message]
2021-09-23 13:16     ` Alexandre Ghiti
2021-09-24 15:04 ` Adam Thomson
2021-09-24 16:17   ` Alexandre Ghiti
2021-09-29 13:33     ` Adam Thomson
2021-09-30  7:51       ` David Abdurachmanov
2021-09-30  9:28         ` Adam Thomson
2021-09-30 10:25         ` Alexandre Ghiti
2021-10-04 12:05           ` Alexandre Ghiti
2021-10-04 15:11             ` Adam Thomson
2021-10-05 13:43               ` Alexandre Ghiti
2021-10-06  9:30                 ` Adam Thomson
2021-10-06 11:35                   ` Alexandre Ghiti
2021-10-08  9:46                     ` Adam Thomson
2021-10-12 10:32                       ` Adam Thomson
2021-10-14 15:51                         ` Alexandre Ghiti
2021-10-15  8:47                           ` Adam Thomson
2021-09-30  9:37       ` Alexandre Ghiti
2021-09-30 10:47         ` Adam Thomson
2021-09-30  9:55       ` Alexandre Ghiti
2021-10-04 15:29         ` Adam Thomson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+zEjCsPwGFng73OJShEc2g6wW1SOKwX7XYFqej_vkJKKUxL0A@mail.gmail.com \
    --to=alexandre.ghiti@canonical.com \
    --cc=ben.dooks@codethink.co.uk \
    --cc=lee.jones@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=support.opensource@diasemi.com \
    --subject='Re: [PATCH] drivers: mfd: da9063: Add restart notifier implementation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).