From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: rkpmic(4) support system power down
To: Paul Fertser <fercerpav@gmail.com>
Cc: tech@openbsd.org
Date: Fri, 22 Nov 2024 00:17:06 +0100

> Date: Thu, 21 Nov 2024 15:03:32 +0300
> From: Paul Fertser <fercerpav@gmail.com>
> 
> Hello,
> 
> When the board DT suggests with the documented property
> "rockchip,system-power-controller" we can power it down by asking the
> PMIC directly just seize the opportunity to not count on (potentially
> outdated) TF-A to do the right thing.
> 
> Tested on Pinebook Pro which has RK808 PMIC. The others were added
> based on reading the corresponding datasheets so compile-tested only.

This looks good.  However...

> ---
> 
> diff --git a/sys/dev/fdt/rkpmic.c b/sys/dev/fdt/rkpmic.c
> index cec42e25a..b65bbb4d3 100644
> --- a/sys/dev/fdt/rkpmic.c
> +++ b/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

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.

> +
>  #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,11 @@ rkpmic_attach(struct device *parent, struct device *self, void *aux)
>  			device_register_wakeup(&sc->sc_dev);
>  #endif
>  	}
> +
> +	if (OF_getpropbool(sc->sc_node, "rockchip,system-power-controller")) {
> +		rkpmic_sc = sc;
> +		powerdownfn = rkpmic_powerdown;
> +	}
>  }
>  
>  int
> @@ -557,6 +589,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;
>  
> 
>