| Message ID | 20190403122150.1231-1-urjaman@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [U-Boot] rk8xx: implement poweroff | expand |
Hi, On Wed, 3 Apr 2019 at 06:21, Urja Rannikko <urjaman@gmail.com> wrote: > > Based on snooping around the linux kernel rk8xx driver, and > tested to work on the ASUS C201. > > Signed-off-by: Urja Rannikko <urjaman@gmail.com> > --- > This is really handy to be able to poweroff (without pressing power button > for a long time) the C201 from u-boot, so i'm sending this as is. > The thing that is bothering me is the pmic_get --- i checked that > every rk8xx is named "pmic" in the device tree so it should work, but > it just feels really weird that this seems to be the best way to access > the driver... > > drivers/power/pmic/rk8xx.c | 34 ++++++++++++++++++++++++++++++++++ > include/power/rk8xx_pmic.h | 4 ++++ > 2 files changed, 38 insertions(+) This should really use a sysreset driver. You can make the PMIC have a child sysreset driver to implement this function. Regards, Simon
Hi, On Wed, Apr 24, 2019 at 3:54 AM Simon Glass <sjg@chromium.org> wrote: > > Hi, > > On Wed, 3 Apr 2019 at 06:21, Urja Rannikko <urjaman@gmail.com> wrote: > > > > Based on snooping around the linux kernel rk8xx driver, and > > tested to work on the ASUS C201. > > > > Signed-off-by: Urja Rannikko <urjaman@gmail.com> > > --- > > This is really handy to be able to poweroff (without pressing power button > > for a long time) the C201 from u-boot, so i'm sending this as is. > > The thing that is bothering me is the pmic_get --- i checked that > > every rk8xx is named "pmic" in the device tree so it should work, but > > it just feels really weird that this seems to be the best way to access > > the driver... > > > > drivers/power/pmic/rk8xx.c | 34 ++++++++++++++++++++++++++++++++++ > > include/power/rk8xx_pmic.h | 4 ++++ > > 2 files changed, 38 insertions(+) > > This should really use a sysreset driver. Thanks for the pointer since it wasnt very obvious because there isnt really any code that does this right now (nor does "sysreset" make one think of poweroff...). > You can make the PMIC have a child sysreset driver to implement this function. I looked for an example of this (even partial), and the only one i found was in drivers/power/pmic/stpmic1.c - so I assume like that but instead implement SYSRESET_POWER_OFF (could also implement SYSRESET_POWER, but that is kinda outside the scope of what i need right now...). And then add a generic sysreset-based poweroff command implementation. (I'm just saying my ideas out loud before implementing them so that you have a chance to say if they're all wrong, regardless i'll be back with patches when i have a moment to work on this...)
Hi Urja, On Fri, 26 Apr 2019 at 04:47, Urja Rannikko <urjaman@gmail.com> wrote: > > Hi, > > On Wed, Apr 24, 2019 at 3:54 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi, > > > > On Wed, 3 Apr 2019 at 06:21, Urja Rannikko <urjaman@gmail.com> wrote: > > > > > > Based on snooping around the linux kernel rk8xx driver, and > > > tested to work on the ASUS C201. > > > > > > Signed-off-by: Urja Rannikko <urjaman@gmail.com> > > > --- > > > This is really handy to be able to poweroff (without pressing power button > > > for a long time) the C201 from u-boot, so i'm sending this as is. > > > The thing that is bothering me is the pmic_get --- i checked that > > > every rk8xx is named "pmic" in the device tree so it should work, but > > > it just feels really weird that this seems to be the best way to access > > > the driver... > > > > > > drivers/power/pmic/rk8xx.c | 34 ++++++++++++++++++++++++++++++++++ > > > include/power/rk8xx_pmic.h | 4 ++++ > > > 2 files changed, 38 insertions(+) > > > > This should really use a sysreset driver. > Thanks for the pointer since it wasnt very obvious because there isnt > really any code that does this right now (nor does "sysreset" make one > think of poweroff...). Yes, and that was only recently added to the sysreset uclass. See for example http://patchwork.ozlabs.org/patch/1091216/ > > > You can make the PMIC have a child sysreset driver to implement this function. > I looked for an example of this (even partial), and the only one i > found was in drivers/power/pmic/stpmic1.c - so I assume like that but > instead implement SYSRESET_POWER_OFF (could also implement > SYSRESET_POWER, but that is kinda outside the scope of what i need > right now...). If in doubt sandbox is already a good thing to look for. E.g. drivers/power/pmic/sandbox.c > > And then add a generic sysreset-based poweroff command implementation. Sounds good - CONFIG_CMD_POWEROFF perhaps. Or maybe we should have a 'sysreset' command with sub-commands for warm, cold, power reset and power off? We could make the existing 'reset' command use sysreset if CONFIG_SYSRESET is enabled. > > (I'm just saying my ideas out loud before implementing them so that > you have a chance to say if they're all wrong, regardless i'll be back > with patches when i have a moment to work on this...) > Yes good idea. Whatever you do, sandbox is a great test environment for new commands. Regards, Simon
diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c index 25c339ab12..c42e180e21 100644 --- a/drivers/power/pmic/rk8xx.c +++ b/drivers/power/pmic/rk8xx.c @@ -87,6 +87,40 @@ static int rk8xx_probe(struct udevice *dev) return 0; } +#if CONFIG_IS_ENABLED(CMD_POWEROFF) +/* NOTE: Should only enable this function if the rockchip,system-power-manager + * property is in the device tree node, but it is there in every board that has + * an rk8xx in u-boot currently, so this is left as an excercise for later. + */ + +int do_poweroff(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + struct udevice *dev; + struct rk8xx_priv *priv; + u8 bits; + + /* "Hey, what would one name a pmic in the device tree..." */ + if (pmic_get("pmic", &dev) != 0) { + printf("pmic not found\n"); + return 1; + } + priv = dev_get_priv(dev); + + if (priv->variant == RK818_ID) + bits = DEV_OFF; + else + bits = DEV_OFF_RST; + + if (pmic_clrsetbits(dev, REG_DEVCTRL, 0, bits) != 0) { + printf("pmic_clrsetbits failed\n"); + return 1; + } + + printf("Poweroff apparently failed.\n"); + return 0; +} +#endif + static struct dm_pmic_ops rk8xx_ops = { .reg_count = rk8xx_reg_count, .read = rk8xx_read, diff --git a/include/power/rk8xx_pmic.h b/include/power/rk8xx_pmic.h index c06248f751..565b35985e 100644 --- a/include/power/rk8xx_pmic.h +++ b/include/power/rk8xx_pmic.h @@ -177,6 +177,10 @@ enum { #define RK8XX_ID_MSK 0xfff0 +/* DEVCTRL bits for poweroff */ +#define DEV_OFF_RST BIT(3) +#define DEV_OFF BIT(0) + struct rk8xx_reg_table { char *name; u8 reg_ctl;
Based on snooping around the linux kernel rk8xx driver, and tested to work on the ASUS C201. Signed-off-by: Urja Rannikko <urjaman@gmail.com> --- This is really handy to be able to poweroff (without pressing power button for a long time) the C201 from u-boot, so i'm sending this as is. The thing that is bothering me is the pmic_get --- i checked that every rk8xx is named "pmic" in the device tree so it should work, but it just feels really weird that this seems to be the best way to access the driver... drivers/power/pmic/rk8xx.c | 34 ++++++++++++++++++++++++++++++++++ include/power/rk8xx_pmic.h | 4 ++++ 2 files changed, 38 insertions(+)