> From: Jeremie Courreges-Anglas <j...@wxcvbn.org> > Date: Sat, 01 Oct 2022 18:36:35 +0200 > > On Thu, Aug 18 2022, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote: > > Some time ago I wanted to get a clean poweroff from the power button on > > my Unmatched, so that I don't get fsck at reboot the morning after > > someone sleeps in the room where the machine lives. kettenis kindly > > provided sfgpio(4) to get interrupts working on dapmic(4) instead of my > > initial hack that used polling. > > > > One issue I struggled with for a bit is that masking irqs also masks the > > wake-up events, particularly the events we use for dapmic_reset(). > > With the diff below most interrupt events are masked off during runtime, > > until we're shutting down/rebooting. Maybe this is too paranoid and > > I should let more events go through the intr handler, eg for wake-up > > events that I don't envision yet? (I would love to get wake on lan > > support in cad(4) but my attempts led nowhere so far.) > > > > Also interesting, the fault register needs to be cleared at boot, else > > the interrupt will keep triggering after eg a hard button-driven poweroff. > > We could log the faults found in FAULT_LOG at boot time to know why the > > machine has stopped. But I'm more concerned about what to do if we get > > a fault at runtime (see the XXX). When I tried to disestablish the > > interrupt handler with fdt_intr_disestablish(sc->sc_ih) from > > dapmic_reset(), I got a fault. Maybe something worth investigating. > > The FAULT_LOG register handling seems appropriate after reading entry > 6.2 in: > > > https://www.renesas.com/eu/en/document/apn/shared-irq-line-considerations-pm-059 > > I wish I had read this document earlier. :) > > > The code below is based off da9063_datasheet_2v2.pdf. I don't know of > > other machines we run on that use this controller, the only entry in > > dmesglog is matthieu's Unmatched machine. > > > > Tests, input and oks welcome. > > In the case where my hifive unmatched lives, I have wired both the > shutdown and the reset buttons. Yesterday I tried to see whether the > reset button could be handled as a clean reboot, but looking at the > hifive unmatched board schematics it seems that the PMIC has no control > over it (even if tweaking nRES_MODE). So the diff stands for scrutiny. > I would like input and maybe oks to get this in. ;)
Finally managed to get this tested. It works for my chassis as well! A few nits below. With those fixed, ok kettenis@ > Index: dev/fdt/dapmic.c > =================================================================== > RCS file: /cvs/src/sys/dev/fdt/dapmic.c,v > retrieving revision 1.2 > diff -u -p -r1.2 dapmic.c > --- dev/fdt/dapmic.c 6 Apr 2022 18:59:28 -0000 1.2 > +++ dev/fdt/dapmic.c 17 Aug 2022 21:59:57 -0000 > @@ -19,6 +19,9 @@ > #include <sys/systm.h> > #include <sys/device.h> > #include <sys/malloc.h> > +#include <sys/task.h> > +#include <sys/proc.h> > +#include <sys/signalvar.h> > > #include <dev/ofw/openfirm.h> > #include <dev/ofw/ofw_regulator.h> > @@ -28,11 +31,31 @@ > > #include <dev/clock_subr.h> > > +#include <machine/fdt.h> > + > extern void (*cpuresetfn)(void); > extern void (*powerdownfn)(void); > > /* Registers */ > +#define FAULT_LOG 0x05 > #define EVENT_A 0x06 > +#define EVENT_A_EVENTS_D (1 << 7) > +#define EVENT_A_EVENTS_C (1 << 6) > +#define EVENT_A_EVENTS_B (1 << 5) > +#define EVENT_A_E_nONKEY (1 << 0) > +#define EVENT_B 0x07 > +#define EVENT_C 0x08 > +#define EVENT_D 0x09 > +#define IRQ_MASK_A 0x0a > +#define IRQ_MASK_A_M_RESERVED ((1 << 7) | (1 << 6) | (1 << 5)) > +#define IRQ_MASK_A_M_SEQ_RDY (1 << 4) > +#define IRQ_MASK_A_M_ADC_RDY (1 << 3) > +#define IRQ_MASK_A_M_TICK (1 << 2) > +#define IRQ_MASK_A_M_ALARM (1 << 1) > +#define IRQ_MASK_A_M_nONKEY (1 << 0) > +#define IRQ_MASK_B 0x0b > +#define IRQ_MASK_C 0x0c > +#define IRQ_MASK_D 0x0d > #define CONTROL_F 0x13 > #define CONTROL_F_WAKE_UP (1 << 2) > #define CONTROL_F_SHUTDOWN (1 << 1) > @@ -55,11 +78,20 @@ extern void (*powerdownfn)(void); > #define ALARM_Y 0x4b > #define ALARM_Y_TICK_ON (1 << 7) > > +#ifdef DAPMIC_DEBUG > +# define DPRINTF(args) do { printf args; } while (0) > +#else > +# define DPRINTF(args) do {} while (0) > +#endif > + > struct dapmic_softc { > struct device sc_dev; > i2c_tag_t sc_tag; > i2c_addr_t sc_addr; > > + int (*sc_ih)(void *); > + struct task sc_task; > + > struct todr_chip_handle sc_todr; > }; > > @@ -80,8 +112,11 @@ int dapmic_clock_read(struct dapmic_soft > int dapmic_clock_write(struct dapmic_softc *, struct clock_ymdhms *); > int dapmic_gettime(struct todr_chip_handle *, struct timeval *); > int dapmic_settime(struct todr_chip_handle *, struct timeval *); > +void dapmic_reset_irq_mask(struct dapmic_softc *); > void dapmic_reset(void); > void dapmic_powerdown(void); > +int dapmic_intr(void *); > +void dapmic_shutdown_task(void *); > > int > dapmic_match(struct device *parent, void *match, void *aux) > @@ -96,6 +131,7 @@ dapmic_attach(struct device *parent, str > { > struct dapmic_softc *sc = (struct dapmic_softc *)self; > struct i2c_attach_args *ia = aux; > + int node = *(int *)ia->ia_cookie; > > sc->sc_tag = ia->ia_tag; > sc->sc_addr = ia->ia_addr; > @@ -105,12 +141,35 @@ dapmic_attach(struct device *parent, str > sc->sc_todr.todr_settime = dapmic_settime; > todr_attach(&sc->sc_todr); > > - printf("\n"); > - > if (cpuresetfn == NULL) > cpuresetfn = dapmic_reset; > if (powerdownfn == NULL) > powerdownfn = dapmic_powerdown; > + > + task_set(&sc->sc_task, dapmic_shutdown_task, sc); > + > + /* Mask away events we don't care about */ > + dapmic_reg_write(sc, IRQ_MASK_A, > + 0xff & ~(IRQ_MASK_A_M_RESERVED | IRQ_MASK_A_M_nONKEY)); > + dapmic_reg_write(sc, IRQ_MASK_B, 0xff); > + dapmic_reg_write(sc, IRQ_MASK_C, 0xff); > + dapmic_reg_write(sc, IRQ_MASK_D, 0xff); > + > + /* Clear past faults and events */ > + dapmic_reg_write(sc, FAULT_LOG, dapmic_reg_read(sc, FAULT_LOG)); > + dapmic_reg_write(sc, EVENT_A, dapmic_reg_read(sc, EVENT_A)); > + dapmic_reg_write(sc, EVENT_B, dapmic_reg_read(sc, EVENT_B)); > + dapmic_reg_write(sc, EVENT_C, dapmic_reg_read(sc, EVENT_C)); > + dapmic_reg_write(sc, EVENT_D, dapmic_reg_read(sc, EVENT_D)); > + > + if (node != 0) { > + sc->sc_ih = fdt_intr_establish_idx(node, 0, IPL_CLOCK, > + dapmic_intr, sc, sc->sc_dev.dv_xname); > + if (sc->sc_ih == NULL) > + printf(", can't establish interrupt"); > + } > + > + printf("\n"); > } > > uint8_t > @@ -239,11 +298,23 @@ dapmic_settime(struct todr_chip_handle * > } > > void > +dapmic_reset_irq_mask(struct dapmic_softc *sc) > +{ > + dapmic_reg_write(sc, IRQ_MASK_A, 0); > + dapmic_reg_write(sc, IRQ_MASK_B, 0); > + dapmic_reg_write(sc, IRQ_MASK_C, 0); > + dapmic_reg_write(sc, IRQ_MASK_D, 0); > +} > + > +void > dapmic_reset(void) > { > struct dapmic_softc *sc = dapmic_cd.cd_devs[0]; > uint8_t reg; > > + /* Re-enable irqs and the associated wake-up events */ Can you add a '.' at the end of the sentence to be consistent with the other comments in this driver? > + dapmic_reset_irq_mask(sc); > + > /* Enable tick alarm wakeup with a one second interval. */ > reg = dapmic_reg_read(sc, ALARM_MO); > reg &= ~ALARM_MO_TICK_TYPE; > @@ -266,10 +337,90 @@ dapmic_powerdown(void) > struct dapmic_softc *sc = dapmic_cd.cd_devs[0]; > uint8_t reg; > > + /* Re-enable irqs and the associated wake-up events */ Here as well? > + dapmic_reset_irq_mask(sc); > + > /* Disable tick function such that it doesn't wake us up. */ > reg = dapmic_reg_read(sc, ALARM_Y); > reg &= ~ALARM_Y_TICK_ON; > dapmic_reg_write(sc, ALARM_Y, reg); > > dapmic_reg_write(sc, CONTROL_F, CONTROL_F_SHUTDOWN); > +} > + > +void > +dapmic_shutdown_task(void *arg) > +{ > + extern int allowpowerdown; > + > + if (allowpowerdown == 1) { > + allowpowerdown = 0; > + prsignal(initprocess, SIGUSR2); > + } > +} > + > +int > +dapmic_intr(void *arg) > +{ > + struct dapmic_softc *sc = arg; > + uint8_t event_a, event_b, event_c, event_d, fault; > + > + event_b = event_c = event_d = 0; > + > + event_a = dapmic_reg_read(sc, EVENT_A); > + DPRINTF(("%s: %s: event_a %#02.2hhx", sc->sc_dev.dv_xname, __func__, > + event_a)); > + > + /* Acknowledge all events */ And here. > + if (event_a & EVENT_A_EVENTS_B) { > + event_b = dapmic_reg_read(sc, EVENT_B); > + DPRINTF((", event_b %#02.2hhx", event_b)); > + if (event_b != 0) > + dapmic_reg_write(sc, EVENT_B, event_b); > + } > + if (event_a & EVENT_A_EVENTS_C) { > + event_c = dapmic_reg_read(sc, EVENT_C); > + DPRINTF((", event_c %#02.2hhx", event_c)); > + if (event_c != 0) > + dapmic_reg_write(sc, EVENT_C, event_c); > + } > + if (event_a & EVENT_A_EVENTS_D) { > + event_d = dapmic_reg_read(sc, EVENT_D); > + DPRINTF((", event_d %#02.2hhx", event_d)); > + if (event_d != 0) > + dapmic_reg_write(sc, EVENT_D, event_d); > + } > + event_a &= ~(EVENT_A_EVENTS_B|EVENT_A_EVENTS_C|EVENT_A_EVENTS_D); > + if (event_a != 0) > + dapmic_reg_write(sc, EVENT_A, event_a); > + > + DPRINTF(("\n")); > + > + fault = dapmic_reg_read(sc, FAULT_LOG); > + if (fault != 0) { > + static int warned; > + if (!warned) { > + warned = 1; > + printf("%s: FAULT_LOG %#02.2hhx\n", sc->sc_dev.dv_xname, > + fault); > + } > + /* > + * Don't blindly acknowledge the fault log bits, else we may > + * prevent legit behavior like a forced poweroff with a long > + * power button press. > + * XXX If nothing useful can be done here, disestablish the > + * handler instead? > + */ > +#if 0 > + dapmic_reg_write(sc, FAULT_LOG, fault); > +#endif Can we remove the "XXX" bit of the comment and the #if 0 bit of code? > + } > + > + if (event_a & EVENT_A_E_nONKEY) > + task_add(systq, &sc->sc_task); > + > + if (event_a | event_b | event_c | event_d) > + return 1; > + > + return 0; > } > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE > >