Re: [PATCH v2 2/2] mfd: rk808: Add restart functionality

[Daniel Schultz <d.schultz@phytec.de>]

Hi,

I can add a DTS property to enable the PMIC restart functions? So this 
feature can be used if its desired.

My problem with the CRU reset is a strange behaviour in the ROM code. 
During a reset from MMC1 (eMMC), the ROM tries to load the SPL from MMC0 
(SD).

Daniel


On 03/13/2018 04:32 AM, Joseph Chen wrote:
> Hi, Daniel：
>
>         On Rockchip platforms, we always use CRU to restart system. 
> From your patch, I think you would like to support restart by CRU or 
> PMIC, right ?
>
>         I think restart by PMIC is not accepted, because PMIC restart 
> means all regualtors are reset (voltage drops to 0mv and then power on 
> again), including vdd_arm, vdd_logic, vcc_ddr, etc. This is like a 
> cold boot for machine.
>
>        We use CRU reset, because we want to keep status of last log, 
> GRF, CRU or other registers, but PMIC restart can make them lost.
>
>
> 在 2018/3/12 22:10, Lee Jones 写道:
>> Rockchip guys,
>>
>> I'd really appreciate your input on these two patches please.
>>
>> Please provide Reviewed-by/Tested-by tags.
>>
>> On Wed, 07 Mar 2018, Daniel Schultz wrote:
>>> When using Rockchip SoCs with rk805/808/818 PMICs, restarts are 
>>> realized by
>>> setting the reset registers in the "Clock and Reset Unit".
>>>
>>> Since all three shutdown functions have almost the same code, all logic
>>> from the shutdown functions can be refactored to a new function
>>> "rk808_update_bits", which can update a register by a given address and
>>> bitmask.
>>>
>>> After that, notifier blocks and the restart functions were added to the
>>> driver. Like the shutdown function, the restart is bound to the
>>> "rockchip,system-power-controller" device tree property and can easily
>>> revoked to the software restart by removing the property.
>>>
>>> Signed-off-by: Daniel Schultz <d.schultz@phytec.de>
>>> ---
>>> Changes:
>>>     v2: Re-submit with recipients from Rockchip.
>>>
>>>   drivers/mfd/rk808.c       | 97 
>>> ++++++++++++++++++++++++++++++-----------------
>>>   include/linux/mfd/rk808.h |  1 +
>>>   2 files changed, 63 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
>>> index d138721..2c68f8c 100644
>>> --- a/drivers/mfd/rk808.c
>>> +++ b/drivers/mfd/rk808.c
>>> @@ -27,6 +27,7 @@
>>>   #include <linux/module.h>
>>>   #include <linux/of_device.h>
>>>   #include <linux/regmap.h>
>>> +#include <linux/reboot.h>
>>>     struct rk808_reg_data {
>>>       int addr;
>>> @@ -369,59 +370,73 @@ static const struct regmap_irq_chip 
>>> rk818_irq_chip = {
>>>     static struct i2c_client *rk808_i2c_client;
>>>   -static void rk805_device_shutdown(void)
>>> +static void rk808_update_bits(unsigned int reg, unsigned int bit_mask)
>>>   {
>>>       int ret;
>>>       struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
>>>         if (!rk808) {
>>>           dev_warn(&rk808_i2c_client->dev,
>>> -             "have no rk805, so do nothing here\n");
>>> +             "have no %s, so do nothing here\n",
>>> +             rk808->regmap_irq_chip->name);
>>>           return;
>>>       }
>>>         ret = regmap_update_bits(rk808->regmap,
>>> -                 RK805_DEV_CTRL_REG,
>>> -                 DEV_OFF, DEV_OFF);
>>> +                 reg,
>>> +                 bit_mask, bit_mask);
>>>       if (ret)
>>> -        dev_err(&rk808_i2c_client->dev, "power off error!\n");
>>> +        dev_err(&rk808_i2c_client->dev, "can't write to DEVCTRL: 
>>> %x!\n",
>>> +            ret);
>>>   }
>>>   -static void rk808_device_shutdown(void)
>>> +static void rk805_device_shutdown(void)
>>>   {
>>> -    int ret;
>>> -    struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
>>> -
>>> -    if (!rk808) {
>>> -        dev_warn(&rk808_i2c_client->dev,
>>> -             "have no rk808, so do nothing here\n");
>>> -        return;
>>> -    }
>>> +    rk808_update_bits(RK805_DEV_CTRL_REG, DEV_OFF);
>>> +}
>>> +static int rk805_restart_notify(struct notifier_block *this,
>>> +                   unsigned long mode, void *cmd)
>>> +{
>>> +    rk808_update_bits(RK805_DEV_CTRL_REG, DEV_OFF_RST);
>>> +    return NOTIFY_DONE;
>>> +}
>>>   -    ret = regmap_update_bits(rk808->regmap,
>>> -                 RK808_DEVCTRL_REG,
>>> -                 DEV_OFF_RST, DEV_OFF_RST);
>>> -    if (ret)
>>> -        dev_err(&rk808_i2c_client->dev, "power off error!\n");
>>> +static void rk808_device_shutdown(void)
>>> +{
>>> +    rk808_update_bits(RK808_DEVCTRL_REG, DEV_OFF_RST);
>>> +}
>>> +static int rk808_restart_notify(struct notifier_block *this,
>>> +                   unsigned long mode, void *cmd)
>>> +{
>>> +    rk808_update_bits(RK808_DEVCTRL_REG, DEV_OFF);
>>> +    return NOTIFY_DONE;
>>>   }
>>>     static void rk818_device_shutdown(void)
>>>   {
>>> -    int ret;
>>> -    struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
>>> +    rk808_update_bits(RK818_DEVCTRL_REG, DEV_OFF_RST);
>>> +}
>>> +static int rk818_restart_notify(struct notifier_block *this,
>>> +                   unsigned long mode, void *cmd)
>>> +{
>>> +    rk808_update_bits(RK818_DEVCTRL_REG, DEV_OFF);
>>> +    return NOTIFY_DONE;
>>> +}
>>>   -    if (!rk808) {
>>> -        dev_warn(&rk808_i2c_client->dev,
>>> -             "have no rk818, so do nothing here\n");
>>> -        return;
>>> -    }
>>> +static struct notifier_block rk805_restart_handler = {
>>> +    .notifier_call = rk805_restart_notify,
>>> +    .priority = 196,
>>> +};
>>>   -    ret = regmap_update_bits(rk808->regmap,
>>> -                 RK818_DEVCTRL_REG,
>>> -                 DEV_OFF_RST, DEV_OFF_RST);
>>> -    if (ret)
>>> -        dev_err(&rk808_i2c_client->dev, "power off error!\n");
>>> -}
>>> +static struct notifier_block rk808_restart_handler = {
>>> +    .notifier_call = rk808_restart_notify,
>>> +    .priority = 196,
>>> +};
>>> +
>>> +static struct notifier_block rk818_restart_handler = {
>>> +    .notifier_call = rk818_restart_notify,
>>> +    .priority = 196,
>>> +};
>>>     static const struct of_device_id rk808_of_match[] = {
>>>       { .compatible = "rockchip,rk805" },
>>> @@ -476,6 +491,7 @@ static int rk808_probe(struct i2c_client *client,
>>>           cells = rk805s;
>>>           nr_cells = ARRAY_SIZE(rk805s);
>>>           pm_pwroff_fn = rk805_device_shutdown;
>>> +        rk808->nb = &rk805_restart_handler;
>>>           break;
>>>       case RK808_ID:
>>>           rk808->regmap_cfg = &rk808_regmap_config;
>>> @@ -485,6 +501,7 @@ static int rk808_probe(struct i2c_client *client,
>>>           cells = rk808s;
>>>           nr_cells = ARRAY_SIZE(rk808s);
>>>           pm_pwroff_fn = rk808_device_shutdown;
>>> +        rk808->nb = &rk808_restart_handler;
>>>           break;
>>>       case RK818_ID:
>>>           rk808->regmap_cfg = &rk818_regmap_config;
>>> @@ -494,6 +511,7 @@ static int rk808_probe(struct i2c_client *client,
>>>           cells = rk818s;
>>>           nr_cells = ARRAY_SIZE(rk818s);
>>>           pm_pwroff_fn = rk818_device_shutdown;
>>> +        rk808->nb = &rk818_restart_handler;
>>>           break;
>>>       default:
>>>           dev_err(&client->dev, "Unsupported RK8XX ID %lu\n",
>>> @@ -546,11 +564,18 @@ static int rk808_probe(struct i2c_client *client,
>>>         pm_off = of_property_read_bool(np,
>>>                   "rockchip,system-power-controller");
>>> -    if (pm_off && !pm_power_off) {
>>> +    if (pm_off) {
>>>           rk808_i2c_client = client;
>>> -        pm_power_off = pm_pwroff_fn;
>>> +        if (!pm_power_off)
>>> +            pm_power_off = pm_pwroff_fn;
>>> +        if (rk808->nb) {
>>> +            ret = register_restart_handler(rk808->nb);
>>> +            if (ret)
>>> +                dev_err(&client->dev,
>>> +                    "cannot register restart handler, %d\n",
>>> +                    ret);
>>> +        }
>>>       }
>>> -
>>>       return 0;
>>>     err_irq:
>>> @@ -564,6 +589,8 @@ static int rk808_remove(struct i2c_client *client)
>>>         regmap_del_irq_chip(client->irq, rk808->irq_data);
>>>       pm_power_off = NULL;
>>> +    if (rk808->nb)
>>> +        unregister_restart_handler(rk808->nb);
>>>         return 0;
>>>   }
>>> diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
>>> index 338e0f6..9c26ca2 100644
>>> --- a/include/linux/mfd/rk808.h
>>> +++ b/include/linux/mfd/rk808.h
>>> @@ -453,5 +453,6 @@ struct rk808 {
>>>       long                variant;
>>>       const struct regmap_config    *regmap_cfg;
>>>       const struct regmap_irq_chip    *regmap_irq_chip;
>>> +    struct notifier_block        *nb;
>>>   };
>>>   #endif /* __LINUX_REGULATOR_RK808_H */
>