> Date: Fri, 22 Nov 2024 13:53:58 +0300
> From: Paul Fertser <fercerpav@gmail.com>
> 
> On Fri, Nov 22, 2024 at 12:17:06AM +0100, Mark Kettenis wrote:
> > > +#define RK805_DEV_CTRL		0x4b
> > > +#define  RK805_DEV_CTRL_DEV_OFF	0x01
> > > +
> > > +#define RK806_SYS_CFG3		0x72
> > > +#define  RK806_SYS_CFG3_DEV_OFF	0x01
> > > +
> > > +#define RK808_DEVCTRL		0x4b
> > > +#define  RK808_DEVCTRL_DEV_OFF_RST	0x08
> > 
> > I wonder why for the RK808 the DEV_OFF_RST gets used whereas for the
> > others just DEV_OFF.  There is a DEV_OFF bit for the RK808 as well.  I
> > noticed that Linux does the same thing, but I do wonder what happens
> > if you use the DEV_OFF bit instead of the DEV_OFF_RST bit on the
> > RK808.
> 
> I wondered the same and my conclusion was that since DEV_OFF_RST not
> only powers the system off but also fully resets the PMIC that might
> provide better consistency (nice to have every startup from identical
> state) so should be preferred when available. Surprisingly only RK808
> seems to offer this (or probably the documentation for the others is
> inaccurate but I do not have hardware to test).
> 
> Per your request I tried DEV_OFF (0x01) bit on RK808 here and it
> "worked the same" (but I haven't really checked if any of the state
> was preserved). Since we know DEV_OFF_RST works for Linux users I do
> not see a good reason to deviate.

Yes, I agree.

> Here follows the updated diff which handles the non-deprecated node
> name.

Thanks.  I committed this version of the diff.

> diff --git sys/dev/fdt/rkpmic.c sys/dev/fdt/rkpmic.c
> index cec42e25a..67a613024 100644
> --- sys/dev/fdt/rkpmic.c
> +++ sys/dev/fdt/rkpmic.c
> @@ -33,6 +33,8 @@
>  
>  #include <dev/clock_subr.h>
>  
> +extern void (*powerdownfn)(void);
> +
>  #define RK80X_SECONDS		0x00
>  #define RK80X_MINUTES		0x01
>  #define RK80X_HOURS		0x02
> @@ -52,10 +54,20 @@
>  #define RK809_RTC_STATUS	0x0e
>  #define  RK80X_RTC_STATUS_POWER_UP	0x80
>  
> +#define RK805_DEV_CTRL		0x4b
> +#define  RK805_DEV_CTRL_DEV_OFF	0x01
> +
> +#define RK806_SYS_CFG3		0x72
> +#define  RK806_SYS_CFG3_DEV_OFF	0x01
> +
> +#define RK808_DEVCTRL		0x4b
> +#define  RK808_DEVCTRL_DEV_OFF_RST	0x08
> +
>  #define RK809_PMIC_SYS_CFG3	0xf4
>  #define  RK809_PMIC_SYS_CFG3_SLP_FUN_MASK	0x18
>  #define  RK809_PMIC_SYS_CFG3_SLP_FUN_NONE	0x00
>  #define  RK809_PMIC_SYS_CFG3_SLP_FUN_SLEEP	0x08
> +#define  RK809_PMIC_SYS_CFG3_DEV_OFF		0x01
>  #define RK809_PMIC_INT_STS0	0xf8
>  #define RK809_PMIC_INT_MSK0	0xf9
>  #define  RK809_PMIC_INT_MSK0_PWRON_FALL_INT_IM	0x01
> @@ -338,6 +350,8 @@ struct rkpmic_softc {
>  	struct spi_config sc_spi_conf;
>  
>  	int sc_rtc_ctrl_reg, sc_rtc_status_reg;
> +	uint8_t sc_dev_ctrl_reg, sc_dev_off_val;
> +
>  	struct todr_chip_handle sc_todr;
>  	const struct rkpmic_regdata *sc_regdata;
>  
> @@ -382,6 +396,9 @@ int	rkpmic_clock_write(struct rkpmic_softc *, struct clock_ymdhms *);
>  int	rkpmic_gettime(struct todr_chip_handle *, struct timeval *);
>  int	rkpmic_settime(struct todr_chip_handle *, struct timeval *);
>  
> +struct rkpmic_softc *rkpmic_sc;
> +void    rkpmic_powerdown(void);
> +
>  int
>  rkpmic_i2c_match(struct device *parent, void *match, void *aux)
>  {
> @@ -447,24 +464,34 @@ rkpmic_attach(struct device *parent, struct device *self, void *aux)
>  		chip = "RK805";
>  		sc->sc_rtc_ctrl_reg = RK805_RTC_CTRL;
>  		sc->sc_rtc_status_reg = RK805_RTC_STATUS;
> +		sc->sc_dev_ctrl_reg = RK805_DEV_CTRL;
> +		sc->sc_dev_off_val = RK805_DEV_CTRL_DEV_OFF;
>  		sc->sc_regdata = rk805_regdata;
>  	} else if (OF_is_compatible(sc->sc_node, "rockchip,rk806")) {
>  		chip = "RK806";
> +		sc->sc_dev_ctrl_reg = RK806_SYS_CFG3;
> +		sc->sc_dev_off_val = RK806_SYS_CFG3_DEV_OFF;
>  		sc->sc_regdata = rk806_regdata;
>  	} else if (OF_is_compatible(sc->sc_node, "rockchip,rk808")) {
>  		chip = "RK808";
>  		sc->sc_rtc_ctrl_reg = RK808_RTC_CTRL;
>  		sc->sc_rtc_status_reg = RK808_RTC_STATUS;
> +		sc->sc_dev_ctrl_reg = RK808_DEVCTRL;
> +		sc->sc_dev_off_val = RK808_DEVCTRL_DEV_OFF_RST;
>  		sc->sc_regdata = rk808_regdata;
>  	} else if (OF_is_compatible(sc->sc_node, "rockchip,rk809")) {
>  		chip = "RK809";
>  		sc->sc_rtc_ctrl_reg = RK809_RTC_CTRL;
>  		sc->sc_rtc_status_reg = RK809_RTC_STATUS;
> +		sc->sc_dev_ctrl_reg = RK809_PMIC_SYS_CFG3;
> +		sc->sc_dev_off_val = RK809_PMIC_SYS_CFG3_DEV_OFF;
>  		sc->sc_regdata = rk809_regdata;
>  	} else {
>  		chip = "RK817";
>  		sc->sc_rtc_ctrl_reg = RK809_RTC_CTRL;
>  		sc->sc_rtc_status_reg = RK809_RTC_STATUS;
> +		sc->sc_dev_ctrl_reg = RK809_PMIC_SYS_CFG3;
> +		sc->sc_dev_off_val = RK809_PMIC_SYS_CFG3_DEV_OFF;
>  		sc->sc_regdata = rk817_regdata;
>  	}
>  	printf(": %s\n", chip);
> @@ -511,6 +538,12 @@ rkpmic_attach(struct device *parent, struct device *self, void *aux)
>  			device_register_wakeup(&sc->sc_dev);
>  #endif
>  	}
> +
> +	if (OF_getpropbool(sc->sc_node, "system-power-controller") ||
> +	    OF_getpropbool(sc->sc_node, "rockchip,system-power-controller")) {
> +		rkpmic_sc = sc;
> +		powerdownfn = rkpmic_powerdown;
> +	}
>  }
>  
>  int
> @@ -557,6 +590,14 @@ rkpmic_intr(void *arg)
>  	return 1;
>  }
>  
> +void
> +rkpmic_powerdown(void)
> +{
> +	struct rkpmic_softc *sc = rkpmic_sc;
> +	rkpmic_reg_write(sc, sc->sc_dev_ctrl_reg,
> +	    rkpmic_reg_read(sc, sc->sc_dev_ctrl_reg) | sc->sc_dev_off_val);
> +}
> +
>  struct rkpmic_regulator {
>  	struct rkpmic_softc *rr_sc;
>  
>