[U-Boot,v2,3/3] rk8xx: add a sysreset driver for poweroff

Based on snooping around the linux kernel rk8xx driver.
Tested on an ASUS C201.

Signed-off-by: Urja Rannikko <urjaman@gmail.com>
---
 drivers/power/pmic/Kconfig |  1 +
 drivers/power/pmic/rk8xx.c | 62 +++++++++++++++++++++++++++++++++++---
 include/power/rk8xx_pmic.h |  4 +++
 3 files changed, 63 insertions(+), 4 deletions(-)

Hi Urja, Simon,

This patch is not able to pass the sandbox_spl test, it reports:
[1]    26463 segmentation fault (core dumped)  ./u-boot

The driver looks good to me, no idea what cause the issue.

Thanks,
- Kever

Urja Rannikko <urjaman@gmail.com> 于2019年5月17日周五 上午5:49写道：

> Based on snooping around the linux kernel rk8xx driver.
> Tested on an ASUS C201.
>
> Signed-off-by: Urja Rannikko <urjaman@gmail.com>
> ---
>  drivers/power/pmic/Kconfig |  1 +
>  drivers/power/pmic/rk8xx.c | 62 +++++++++++++++++++++++++++++++++++---
>  include/power/rk8xx_pmic.h |  4 +++
>  3 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
> index 52edb29b48..3f6e30d503 100644
> --- a/drivers/power/pmic/Kconfig
> +++ b/drivers/power/pmic/Kconfig
> @@ -124,6 +124,7 @@ config PMIC_PM8916
>  config PMIC_RK8XX
>         bool "Enable support for Rockchip PMIC RK8XX"
>         depends on DM_PMIC
> +       select SYSRESET_CMD_POWEROFF if CMD_POWEROFF
>         ---help---
>         The Rockchip RK808 PMIC provides four buck DC-DC convertors, 8
> LDOs,
>         an RTC and two low Rds (resistance (drain to source)) switches. It
> is
> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c
> index 25c339ab12..52e41051ae 100644
> --- a/drivers/power/pmic/rk8xx.c
> +++ b/drivers/power/pmic/rk8xx.c
> @@ -6,6 +6,9 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <sysreset.h>
> +#include <dm/device.h>
> +#include <dm/lists.h>
>  #include <errno.h>
>  #include <power/rk8xx_pmic.h>
>  #include <power/pmic.h>
> @@ -49,9 +52,9 @@ static int rk8xx_read(struct udevice *dev, uint reg,
> uint8_t *buff, int len)
>         return 0;
>  }
>
> -#if CONFIG_IS_ENABLED(PMIC_CHILDREN)
>  static int rk8xx_bind(struct udevice *dev)
>  {
> +#if CONFIG_IS_ENABLED(PMIC_CHILDREN)
>         ofnode regulators_node;
>         int children;
>
> @@ -68,10 +71,15 @@ static int rk8xx_bind(struct udevice *dev)
>         if (!children)
>                 debug("%s: %s - no child found\n", __func__, dev->name);
>
> +#endif
> +
> +       if (CONFIG_IS_ENABLED(SYSRESET))
> +               return device_bind_driver(dev, "rk8xx-sysreset",
> +                                         "rk8xx-sysreset", NULL);
> +
>         /* Always return success for this device */
>         return 0;
>  }
> -#endif
>
>  static int rk8xx_probe(struct udevice *dev)
>  {
> @@ -103,10 +111,56 @@ U_BOOT_DRIVER(pmic_rk8xx) = {
>         .name = "rk8xx pmic",
>         .id = UCLASS_PMIC,
>         .of_match = rk8xx_ids,
> -#if CONFIG_IS_ENABLED(PMIC_CHILDREN)
>         .bind = rk8xx_bind,
> -#endif
>         .priv_auto_alloc_size   = sizeof(struct rk8xx_priv),
>         .probe = rk8xx_probe,
>         .ops = &rk8xx_ops,
>  };
> +
> +#if IS_ENABLED(CONFIG_SYSRESET)
> +/* 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.
> + */
> +static int rk8xx_sysreset_request(struct udevice *dev, enum sysreset_t
> type)
> +{
> +       struct udevice *pmic_dev;
> +       struct rk8xx_priv *priv;
> +       int ret;
> +       u8 bits;
> +
> +       if (type != SYSRESET_POWER_OFF)
> +               return -EPROTONOSUPPORT;
> +
> +       ret = uclass_get_device_by_driver(UCLASS_PMIC,
> +                                         DM_GET_DRIVER(pmic_rk8xx),
> +                                         &pmic_dev);
> +
> +       if (ret)
> +               return -EOPNOTSUPP;
> +
> +       priv = dev_get_priv(pmic_dev);
> +
> +       if (priv->variant == RK818_ID)
> +               bits = DEV_OFF;
> +       else
> +               bits = DEV_OFF_RST;
> +
> +       ret = pmic_clrsetbits(pmic_dev, REG_DEVCTRL, 0, bits);
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       return -EINPROGRESS;
> +}
> +
> +static struct sysreset_ops rk8xx_sysreset_ops = {
> +       .request = rk8xx_sysreset_request,
> +};
> +
> +U_BOOT_DRIVER(rk8xx_sysreset) = {
> +       .name = "rk8xx-sysreset",
> +       .id = UCLASS_SYSRESET,
> +       .ops = &rk8xx_sysreset_ops,
> +};
> +#endif
> 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;
> --
> 2.21.0
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

Hi Kever,

On Tue, 13 Aug 2019 at 20:46, Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Hi Urja, Simon,
>
> This patch is not able to pass the sandbox_spl test, it reports:
> [1]    26463 segmentation fault (core dumped)  ./u-boot
>
> The driver looks good to me, no idea what cause the issue.
>
> Thanks,
> - Kever
>
> Urja Rannikko <urjaman@gmail.com> 于2019年5月17日周五 上午5:49写道：
>>
>> Based on snooping around the linux kernel rk8xx driver.
>> Tested on an ASUS C201.
>>
>> Signed-off-by: Urja Rannikko <urjaman@gmail.com>
>> ---
>>  drivers/power/pmic/Kconfig |  1 +
>>  drivers/power/pmic/rk8xx.c | 62 +++++++++++++++++++++++++++++++++++---
>>  include/power/rk8xx_pmic.h |  4 +++
>>  3 files changed, 63 insertions(+), 4 deletions(-)
>>

This driver is enabled for sandbox, although I doubt it is in the
device tree, so I'm not sure why it would be called. But if it is, and
it directly accesses memory, then it might be the reason.

You should run the test under gdb to see where it is crashing.

Regards,
Simon

Simon, Stephen,

On 2019/8/15 上午3:35, Simon Glass wrote:
> Hi Kever,
>
> On Tue, 13 Aug 2019 at 20:46, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hi Urja, Simon,
>>
>> This patch is not able to pass the sandbox_spl test, it reports:
>> [1]    26463 segmentation fault (core dumped)  ./u-boot
>>
>> The driver looks good to me, no idea what cause the issue.
>>
>> Thanks,
>> - Kever
>>
>> Urja Rannikko <urjaman@gmail.com> 于2019年5月17日周五 上午5:49写道：
>>> Based on snooping around the linux kernel rk8xx driver.
>>> Tested on an ASUS C201.
>>>
>>> Signed-off-by: Urja Rannikko <urjaman@gmail.com>
>>> ---
>>>   drivers/power/pmic/Kconfig |  1 +
>>>   drivers/power/pmic/rk8xx.c | 62 +++++++++++++++++++++++++++++++++++---
>>>   include/power/rk8xx_pmic.h |  4 +++
>>>   3 files changed, 63 insertions(+), 4 deletions(-)
>>>
> This driver is enabled for sandbox, although I doubt it is in the
> device tree, so I'm not sure why it would be called. But if it is, and
> it directly accesses memory, then it might be the reason.
>
> You should run the test under gdb to see where it is crashing.

gdb output is:
Program received signal SIGSEGV, Segmentation fault.
0x00005555556182c6 in strcmp (cs=cs@entry=0x55555566023b "root_driver", 
ct=0x1 <error: Cannot access memory at address 0x1>)
     at lib/string.c:190
190            if ((__res = *cs - *ct++) != 0 || !*cs++)


This does not help much for crashing reason, and I have narrow down the 
cause, I believe the
crash related to "DM_GET_DRIVER(pmic_rk8xx)",
- if I replace the 'pmic_rk8xx' in DM_GET_DRIVER() to any of other 
available driver, u-boot does not crash;
- if I move the new 'rk8xx_sysreset' driver to other files, eg. 
pmic/sandbox.c, u-boot does not crash;
Any more suggestion, or could you help to cherry pick this patch, and 
you should reproduce
this issue:
     make sandbox_spl_defconfig all
     ./u-boot


Thanks,
- Kever
>
> Regards,
> Simon
>