[PATCH 0/4] mfd: RK8xx tidyup

[PATCH 0/4] mfd: RK8xx tidyup

[Robin Murphy]

Hi all,

In trying to debug suspend issues on my RK3328 box, I was looking at
how the RK8xx driver handles the RK805 sleep pin, and frankly the whole
driver seemed untidy enough to warrant some cleanup and minor fixes
before going any further. I've based the series on top of Soeren's
"mfd: rk808: Always use poweroff when requested" patch[1].

Note that I've only had time to build-test these patches so far, but I
wanted to share them early for the sake of discussion in response to
the other thread[2].

Robin.

[1] https://patchwork.kernel.org/patch/11279249/
[2] https://patchwork.kernel.org/cover/11276945/

Robin Murphy (4):
  mfd: rk808: Set global instance unconditionally
  mfd: rk808: Always register syscore ops
  mfd: rk808: Reduce shutdown duplication
  mfd: rk808: Convert RK805 to syscore/PM ops

 drivers/mfd/rk808.c       | 122 ++++++++++++++++----------------------
 include/linux/mfd/rk808.h |   2 -
 2 files changed, 50 insertions(+), 74 deletions(-)

-- 
2.17.1

[PATCH 1/4] mfd: rk808: Set global instance unconditionally

[Robin Murphy]

The RK817 syscore ops rely on the global rk808_i2c_client being set,
but are essentially independent of whether this driver has authority
over system power control - indeed, setting the SLEEP pin functionality
is most likely wanted when firmware is in charge of power via PSCI.
There's also no harm in setting it unconditionally anyway, so do it.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/mfd/rk808.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index 616e44e7ef98..f2f2f98552a0 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -666,6 +666,8 @@ static int rk808_probe(struct i2c_client *client,
 		}
 	}
 
+	rk808_i2c_client = client;
+
 	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
 			      cells, nr_cells, NULL, 0,
 			      regmap_irq_get_domain(rk808->irq_data));
@@ -675,7 +677,6 @@ static int rk808_probe(struct i2c_client *client,
 	}
 
 	if (of_property_read_bool(np, "rockchip,system-power-controller")) {
-		rk808_i2c_client = client;
 		pm_power_off = rk808->pm_pwroff_fn;
 		pm_power_off_prepare = rk808->pm_pwroff_prep_fn;
 	}
-- 
2.17.1

[PATCH 2/4] mfd: rk808: Always register syscore ops

[Robin Murphy]

Registering the syscore shutdown notifier even when it's a
no-op for the given RK8xx variant should be harmless, and
saves a lot of bother in handling unregistering on probe
failure or module removal, which has been woefully lacking.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/mfd/rk808.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index f2f2f98552a0..387105830736 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -623,7 +623,6 @@ static int rk808_probe(struct i2c_client *client,
 		nr_pre_init_regs = ARRAY_SIZE(rk817_pre_init_reg);
 		cells = rk817s;
 		nr_cells = ARRAY_SIZE(rk817s);
-		register_syscore_ops(&rk808_syscore_ops);
 		break;
 	default:
 		dev_err(&client->dev, "Unsupported RK8XX ID %lu\n",
@@ -667,6 +666,7 @@ static int rk808_probe(struct i2c_client *client,
 	}
 
 	rk808_i2c_client = client;
+	register_syscore_ops(&rk808_syscore_ops);
 
 	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
 			      cells, nr_cells, NULL, 0,
@@ -684,6 +684,7 @@ static int rk808_probe(struct i2c_client *client,
 	return 0;
 
 err_irq:
+	unregister_syscore_ops(&rk808_syscore_ops);
 	regmap_del_irq_chip(client->irq, rk808->irq_data);
 	return ret;
 }
@@ -694,6 +695,8 @@ static int rk808_remove(struct i2c_client *client)
 
 	regmap_del_irq_chip(client->irq, rk808->irq_data);
 
+	unregister_syscore_ops(&rk808_syscore_ops);
+
 	/**
 	 * pm_power_off may points to a function from another module.
 	 * Check if the pointer is set by us and only then overwrite it.
-- 
2.17.1

[PATCH 3/4] mfd: rk808: Reduce shutdown duplication

[Robin Murphy]

Rather than having 3 almost-identical functions plus the machinery to
keep track of them, it's far simpler to just dynamically select the
appropriate register field per variant.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/mfd/rk808.c       | 56 +++++++++++++--------------------------
 include/linux/mfd/rk808.h |  1 -
 2 files changed, 19 insertions(+), 38 deletions(-)

diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index 387105830736..657b8baa3b8a 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -449,21 +449,6 @@ static const struct regmap_irq_chip rk818_irq_chip = {
 
 static struct i2c_client *rk808_i2c_client;
 
-static void rk805_device_shutdown(void)
-{
-	int ret;
-	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
-
-	if (!rk808)
-		return;
-
-	ret = regmap_update_bits(rk808->regmap,
-				 RK805_DEV_CTRL_REG,
-				 DEV_OFF, DEV_OFF);
-	if (ret)
-		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
-}
-
 static void rk805_device_shutdown_prepare(void)
 {
 	int ret;
@@ -482,29 +467,29 @@ static void rk805_device_shutdown_prepare(void)
 static void rk808_device_shutdown(void)
 {
 	int ret;
+	unsigned int reg, bit;
 	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
 
 	if (!rk808)
 		return;
 
-	ret = regmap_update_bits(rk808->regmap,
-				 RK808_DEVCTRL_REG,
-				 DEV_OFF_RST, DEV_OFF_RST);
-	if (ret)
-		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
-}
-
-static void rk818_device_shutdown(void)
-{
-	int ret;
-	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
-
-	if (!rk808)
+	switch (rk808->variant) {
+	case RK805_ID:
+		reg = RK805_DEV_CTRL_REG;
+		bit = DEV_OFF;
+		break;
+	case RK808_ID:
+		reg = RK808_DEVCTRL_REG,
+		bit = DEV_OFF_RST;
+		break;
+	case RK818_ID:
+		reg = RK818_DEVCTRL_REG;
+		bit = DEV_OFF;
+		break;
+	default:
 		return;
-
-	ret = regmap_update_bits(rk808->regmap,
-				 RK818_DEVCTRL_REG,
-				 DEV_OFF, DEV_OFF);
+	}
+	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
 	if (ret)
 		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
 }
@@ -594,7 +579,6 @@ static int rk808_probe(struct i2c_client *client,
 		nr_pre_init_regs = ARRAY_SIZE(rk805_pre_init_reg);
 		cells = rk805s;
 		nr_cells = ARRAY_SIZE(rk805s);
-		rk808->pm_pwroff_fn = rk805_device_shutdown;
 		rk808->pm_pwroff_prep_fn = rk805_device_shutdown_prepare;
 		break;
 	case RK808_ID:
@@ -604,7 +588,6 @@ static int rk808_probe(struct i2c_client *client,
 		nr_pre_init_regs = ARRAY_SIZE(rk808_pre_init_reg);
 		cells = rk808s;
 		nr_cells = ARRAY_SIZE(rk808s);
-		rk808->pm_pwroff_fn = rk808_device_shutdown;
 		break;
 	case RK818_ID:
 		rk808->regmap_cfg = &rk818_regmap_config;
@@ -613,7 +596,6 @@ static int rk808_probe(struct i2c_client *client,
 		nr_pre_init_regs = ARRAY_SIZE(rk818_pre_init_reg);
 		cells = rk818s;
 		nr_cells = ARRAY_SIZE(rk818s);
-		rk808->pm_pwroff_fn = rk818_device_shutdown;
 		break;
 	case RK809_ID:
 	case RK817_ID:
@@ -677,7 +659,7 @@ static int rk808_probe(struct i2c_client *client,
 	}
 
 	if (of_property_read_bool(np, "rockchip,system-power-controller")) {
-		pm_power_off = rk808->pm_pwroff_fn;
+		pm_power_off = rk808_device_shutdown;
 		pm_power_off_prepare = rk808->pm_pwroff_prep_fn;
 	}
 
@@ -701,7 +683,7 @@ static int rk808_remove(struct i2c_client *client)
 	 * pm_power_off may points to a function from another module.
 	 * Check if the pointer is set by us and only then overwrite it.
 	 */
-	if (rk808->pm_pwroff_fn && pm_power_off == rk808->pm_pwroff_fn)
+	if (pm_power_off == rk808_device_shutdown)
 		pm_power_off = NULL;
 
 	/**
diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
index a59bf323f713..b038653fa87e 100644
--- a/include/linux/mfd/rk808.h
+++ b/include/linux/mfd/rk808.h
@@ -620,7 +620,6 @@ struct rk808 {
 	long				variant;
 	const struct regmap_config	*regmap_cfg;
 	const struct regmap_irq_chip	*regmap_irq_chip;
-	void				(*pm_pwroff_fn)(void);
 	void				(*pm_pwroff_prep_fn)(void);
 };
 #endif /* __LINUX_REGULATOR_RK808_H */
-- 
2.17.1

[PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

[Robin Murphy]

RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
so it makes little sense for the driver to have to have two completely
different mechanisms to handle essentially the same thing. Bring RK805
in line with the RK809/RK817 flow to clean things up.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/mfd/rk808.c       | 58 +++++++++++++++++----------------------
 include/linux/mfd/rk808.h |  1 -
 2 files changed, 25 insertions(+), 34 deletions(-)

diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index 657b8baa3b8a..e88bdb889d3a 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -186,7 +186,6 @@ static const struct rk808_reg_data rk805_pre_init_reg[] = {
 	{RK805_BUCK4_CONFIG_REG, RK805_BUCK3_4_ILMAX_MASK,
 				 RK805_BUCK4_ILMAX_3500MA},
 	{RK805_BUCK4_CONFIG_REG, BUCK_ILMIN_MASK, BUCK_ILMIN_400MA},
-	{RK805_GPIO_IO_POL_REG, SLP_SD_MSK, SLEEP_FUN},
 	{RK805_THERMAL_REG, TEMP_HOTDIE_MSK, TEMP115C},
 };
 
@@ -449,21 +448,6 @@ static const struct regmap_irq_chip rk818_irq_chip = {
 
 static struct i2c_client *rk808_i2c_client;
 
-static void rk805_device_shutdown_prepare(void)
-{
-	int ret;
-	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
-
-	if (!rk808)
-		return;
-
-	ret = regmap_update_bits(rk808->regmap,
-				 RK805_GPIO_IO_POL_REG,
-				 SLP_SD_MSK, SHUTDOWN_FUN);
-	if (ret)
-		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
-}
-
 static void rk808_device_shutdown(void)
 {
 	int ret;
@@ -499,17 +483,29 @@ static void rk8xx_syscore_shutdown(void)
 	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
 	int ret;
 
-	if (system_state == SYSTEM_POWER_OFF &&
-	    (rk808->variant == RK809_ID || rk808->variant == RK817_ID)) {
+	if (system_state != SYSTEM_POWER_OFF)
+	       return;
+
+	switch (rk808->variant) {
+	case RK805_ID:
+		ret = regmap_update_bits(rk808->regmap,
+					 RK805_GPIO_IO_POL_REG,
+					 SLP_SD_MSK,
+					 SHUTDOWN_FUN);
+		break;
+	case RK809_ID:
+	case RK817_ID:
 		ret = regmap_update_bits(rk808->regmap,
 					 RK817_SYS_CFG(3),
 					 RK817_SLPPIN_FUNC_MSK,
 					 SLPPIN_DN_FUN);
-		if (ret) {
-			dev_warn(&rk808_i2c_client->dev,
-				 "Cannot switch to power down function\n");
-		}
+		break;
+	default:
+		return;
 	}
+	if (ret)
+		dev_warn(&rk808_i2c_client->dev,
+			 "Cannot switch to power down function\n");
 }
 
 static struct syscore_ops rk808_syscore_ops = {
@@ -579,7 +575,6 @@ static int rk808_probe(struct i2c_client *client,
 		nr_pre_init_regs = ARRAY_SIZE(rk805_pre_init_reg);
 		cells = rk805s;
 		nr_cells = ARRAY_SIZE(rk805s);
-		rk808->pm_pwroff_prep_fn = rk805_device_shutdown_prepare;
 		break;
 	case RK808_ID:
 		rk808->regmap_cfg = &rk808_regmap_config;
@@ -658,10 +653,8 @@ static int rk808_probe(struct i2c_client *client,
 		goto err_irq;
 	}
 
-	if (of_property_read_bool(np, "rockchip,system-power-controller")) {
+	if (of_property_read_bool(np, "rockchip,system-power-controller"))
 		pm_power_off = rk808_device_shutdown;
-		pm_power_off_prepare = rk808->pm_pwroff_prep_fn;
-	}
 
 	return 0;
 
@@ -686,13 +679,6 @@ static int rk808_remove(struct i2c_client *client)
 	if (pm_power_off == rk808_device_shutdown)
 		pm_power_off = NULL;
 
-	/**
-	 * As above, check if the pointer is set by us before overwrite.
-	 */
-	if (rk808->pm_pwroff_prep_fn &&
-	    pm_power_off_prepare == rk808->pm_pwroff_prep_fn)
-		pm_power_off_prepare = NULL;
-
 	return 0;
 }
 
@@ -702,6 +688,12 @@ static int __maybe_unused rk8xx_suspend(struct device *dev)
 	int ret = 0;
 
 	switch (rk808->variant) {
+	case RK805_ID:
+		ret = regmap_update_bits(rk808->regmap,
+					 RK805_GPIO_IO_POL_REG,
+					 SLP_SD_MSK,
+					 SLEEP_FUN);
+		break;
 	case RK809_ID:
 	case RK817_ID:
 		ret = regmap_update_bits(rk808->regmap,
diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
index b038653fa87e..e07f6e61cd38 100644
--- a/include/linux/mfd/rk808.h
+++ b/include/linux/mfd/rk808.h
@@ -620,6 +620,5 @@ struct rk808 {
 	long				variant;
 	const struct regmap_config	*regmap_cfg;
 	const struct regmap_irq_chip	*regmap_irq_chip;
-	void				(*pm_pwroff_prep_fn)(void);
 };
 #endif /* __LINUX_REGULATOR_RK808_H */
-- 
2.17.1

Re: [PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

[Anand Moon]

Hi Robin

On Tue, 10 Dec 2019 at 18:54, Robin Murphy <robin.murphy@arm.com> wrote:
>
> RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
> so it makes little sense for the driver to have to have two completely
> different mechanisms to handle essentially the same thing. Bring RK805
> in line with the RK809/RK817 flow to clean things up.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/mfd/rk808.c       | 58 +++++++++++++++++----------------------
>  include/linux/mfd/rk808.h |  1 -
>  2 files changed, 25 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
> index 657b8baa3b8a..e88bdb889d3a 100644
> --- a/drivers/mfd/rk808.c
> +++ b/drivers/mfd/rk808.c
> @@ -186,7 +186,6 @@ static const struct rk808_reg_data rk805_pre_init_reg[] = {
>         {RK805_BUCK4_CONFIG_REG, RK805_BUCK3_4_ILMAX_MASK,
>                                  RK805_BUCK4_ILMAX_3500MA},
>         {RK805_BUCK4_CONFIG_REG, BUCK_ILMIN_MASK, BUCK_ILMIN_400MA},
> -       {RK805_GPIO_IO_POL_REG, SLP_SD_MSK, SLEEP_FUN},
>         {RK805_THERMAL_REG, TEMP_HOTDIE_MSK, TEMP115C},
>  };
>
> @@ -449,21 +448,6 @@ static const struct regmap_irq_chip rk818_irq_chip = {
>
>  static struct i2c_client *rk808_i2c_client;
>
> -static void rk805_device_shutdown_prepare(void)
> -{
> -       int ret;
> -       struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
> -
> -       if (!rk808)
> -               return;
> -
> -       ret = regmap_update_bits(rk808->regmap,
> -                                RK805_GPIO_IO_POL_REG,
> -                                SLP_SD_MSK, SHUTDOWN_FUN);
> -       if (ret)
> -               dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
> -}
> -
>  static void rk808_device_shutdown(void)
>  {
>         int ret;
> @@ -499,17 +483,29 @@ static void rk8xx_syscore_shutdown(void)
>         struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
>         int ret;
>
> -       if (system_state == SYSTEM_POWER_OFF &&
> -           (rk808->variant == RK809_ID || rk808->variant == RK817_ID)) {
> +       if (system_state != SYSTEM_POWER_OFF)
> +              return;
> +
> +       switch (rk808->variant) {
> +       case RK805_ID:
> +               ret = regmap_update_bits(rk808->regmap,
> +                                        RK805_GPIO_IO_POL_REG,
> +                                        SLP_SD_MSK,
> +                                        SHUTDOWN_FUN);
> +               break;
> +       case RK809_ID:
> +       case RK817_ID:
>                 ret = regmap_update_bits(rk808->regmap,
>                                          RK817_SYS_CFG(3),
>                                          RK817_SLPPIN_FUNC_MSK,
>                                          SLPPIN_DN_FUN);
> -               if (ret) {
> -                       dev_warn(&rk808_i2c_client->dev,
> -                                "Cannot switch to power down function\n");
> -               }
> +               break;
> +       default:
> +               return;
>         }
> +       if (ret)
> +               dev_warn(&rk808_i2c_client->dev,
> +                        "Cannot switch to power down function\n");
>  }
>
>  static struct syscore_ops rk808_syscore_ops = {
> @@ -579,7 +575,6 @@ static int rk808_probe(struct i2c_client *client,
>                 nr_pre_init_regs = ARRAY_SIZE(rk805_pre_init_reg);
>                 cells = rk805s;
>                 nr_cells = ARRAY_SIZE(rk805s);
> -               rk808->pm_pwroff_prep_fn = rk805_device_shutdown_prepare;
>                 break;
>         case RK808_ID:
>                 rk808->regmap_cfg = &rk808_regmap_config;
> @@ -658,10 +653,8 @@ static int rk808_probe(struct i2c_client *client,
>                 goto err_irq;
>         }
>
> -       if (of_property_read_bool(np, "rockchip,system-power-controller")) {
> +       if (of_property_read_bool(np, "rockchip,system-power-controller"))
>                 pm_power_off = rk808_device_shutdown;
> -               pm_power_off_prepare = rk808->pm_pwroff_prep_fn;
> -       }
>
>         return 0;
>
> @@ -686,13 +679,6 @@ static int rk808_remove(struct i2c_client *client)
>         if (pm_power_off == rk808_device_shutdown)
>                 pm_power_off = NULL;
>
> -       /**
> -        * As above, check if the pointer is set by us before overwrite.
> -        */
> -       if (rk808->pm_pwroff_prep_fn &&
> -           pm_power_off_prepare == rk808->pm_pwroff_prep_fn)
> -               pm_power_off_prepare = NULL;
> -
>         return 0;
>  }
>
> @@ -702,6 +688,12 @@ static int __maybe_unused rk8xx_suspend(struct device *dev)
>         int ret = 0;
>
>         switch (rk808->variant) {
> +       case RK805_ID:
> +               ret = regmap_update_bits(rk808->regmap,
> +                                        RK805_GPIO_IO_POL_REG,
> +                                        SLP_SD_MSK,
> +                                        SLEEP_FUN);
> +               break;
>         case RK809_ID:
>         case RK817_ID:
>                 ret = regmap_update_bits(rk808->regmap,
> diff --git a/include/linux/mfd/rk808.h b/include/linux/mfd/rk808.h
> index b038653fa87e..e07f6e61cd38 100644
> --- a/include/linux/mfd/rk808.h
> +++ b/include/linux/mfd/rk808.h
> @@ -620,6 +620,5 @@ struct rk808 {
>         long                            variant;
>         const struct regmap_config      *regmap_cfg;
>         const struct regmap_irq_chip    *regmap_irq_chip;
> -       void                            (*pm_pwroff_prep_fn)(void);
>  };
>  #endif /* __LINUX_REGULATOR_RK808_H */
> --
> 2.17.1
>

I am sill getting the kernel warning on issue poweroff see below.
on my Rock960 Model A
I feel the reason for this is we now have two poweroff callback
1  pm_power_off = rk808_device_shutdown
2  rk8xx_syscore_shutdown

In my investigation earlier common function for shutdown solve
the issue of clean shutdown.

for *rockchip,system-power-controller* dts property
we can used flags if check if this property support clean shutdown
for that device.

[  565.009291] xhci-hcd xhci-hcd.0.auto: USB bus 5 deregistered
[  565.010179] reboot: Power down
[  565.010536] ------------[ cut here ]------------
[  565.010940] No atomic I2C transfer handler for 'i2c-0'
[  565.011437] WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:40
i2c_transfer+0xe4/0xf8
[  565.012126] Modules linked in: snd_soc_hdmi_codec dw_hdmi_i2s_audio
rockchipdrm nvme analogix_dp nvme_core brcmfmac hci_uart dw_mipi_dsi
dw_hdmi btbcm cec panfrost bluetooth drm_kms_helper brcmutil gpu_sched
cfg80211 crct10dif_ce snd_soc_rockchip_i2s snd_soc_simple_card drm
ecdh_generic snd_soc_rockchip_pcm snd_soc_simple_card_utils
phy_rockchip_pcie ecc rtc_rk808 rfkill rockchip_thermal
pcie_rockchip_host ip_tables x_tables ipv6 nf_defrag_ipv6
[  565.015578] CPU: 0 PID: 1 Comm: shutdown Not tainted
5.5.0-rc1-00292-gd46dd6369c55 #7
[  565.016260] Hardware name: 96boards Rock960 (DT)
[  565.016666] pstate: 60000085 (nZCv daIf -PAN -UAO)
[  565.017087] pc : i2c_transfer+0xe4/0xf8
[  565.017425] lr : i2c_transfer+0xe4/0xf8
[  565.017762] sp : ffff80001004baf0
[  565.018052] x29: ffff80001004baf0 x28: ffff00007d208000
[  565.018517] x27: 0000000000000000 x26: 0000000000000000
[  565.018982] x25: 0000000000000008 x24: 0000000000000000
[  565.019447] x23: ffff00007d208000 x22: ffff80001004bc64
[  565.019912] x21: ffff80001004bb48 x20: 0000000000000002
[  565.020377] x19: ffff000078502080 x18: 0000000000000010
[  565.020842] x17: 0000000000000001 x16: 0000000000000019
[  565.021307] x15: ffff00007d208470 x14: ffffffffffffffff
[  565.021772] x13: ffff80009004b857 x12: ffff80001004b860
[  565.022237] x11: ffff800011841000 x10: ffff800011a10658
[  565.022702] x9 : 0000000000000000 x8 : ffff800011a11000
[  565.023167] x7 : ffff800010697c78 x6 : 0000000000000262
[  565.023632] x5 : 0000000000000000 x4 : 0000000000000000
[  565.024096] x3 : 00000000ffffffff x2 : ffff800011841ab8
[  565.024561] x1 : 7b11701b0ae78800 x0 : 0000000000000000
[  565.025027] Call trace:
[  565.025246]  i2c_transfer+0xe4/0xf8
[  565.025556]  regmap_i2c_read+0x5c/0xa0
[  565.025886]  _regmap_raw_read+0xcc/0x138
[  565.026230]  _regmap_bus_read+0x3c/0x70
[  565.026568]  _regmap_read+0x60/0xe0
[  565.026875]  _regmap_update_bits+0xc8/0x108
[  565.027241]  regmap_update_bits_base+0x60/0x90
[  565.027633]  rk808_device_shutdown+0x6c/0x88
[  565.028010]  machine_power_off+0x24/0x30
[  565.028356]  kernel_power_off+0x64/0x70
[  565.028693]  __do_sys_reboot+0x15c/0x240
[  565.029038]  __arm64_sys_reboot+0x20/0x28
[  565.029390]  el0_svc_common.constprop.0+0x68/0x160
[  565.029811]  el0_svc_handler+0x20/0x80
[  565.030141]  el0_sync_handler+0x10c/0x180
[  565.030493]  el0_sync+0x140/0x180
[  565.030785] ---[ end trace 5167e842ce15f686 ]---

-Anand

Re: [PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

[Heiko Stübner]

Hi Anand,

Am Sonntag, 15. Dezember 2019, 19:51:50 CET schrieb Anand Moon:
> On Tue, 10 Dec 2019 at 18:54, Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
> > so it makes little sense for the driver to have to have two completely
> > different mechanisms to handle essentially the same thing. Bring RK805
> > in line with the RK809/RK817 flow to clean things up.
> >
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---

[...]

> I am sill getting the kernel warning on issue poweroff see below.
> on my Rock960 Model A
> I feel the reason for this is we now have two poweroff callback
> 1  pm_power_off = rk808_device_shutdown
> 2  rk8xx_syscore_shutdown

Nope, the issue is just the i2c subsystem complaining that the
Rocckhip i2c drives does not provide an atomic-transfer function, see
	"No atomic I2C transfer handler for 'i2c-0'"
in your warning.

Somewhere it was suggested that the current transfer function just
works as atomic as well.


> In my investigation earlier common function for shutdown solve
> the issue of clean shutdown.

This is simply a result of your syscore-shutdown function running way to
early, before the i2c subsystem switched to using atomic transfers.

This also indicates that this would really be way to early, as other parts
of the kernel could also still be running.

Heiko


> for *rockchip,system-power-controller* dts property
> we can used flags if check if this property support clean shutdown
> for that device.
> 
> [  565.009291] xhci-hcd xhci-hcd.0.auto: USB bus 5 deregistered
> [  565.010179] reboot: Power down
> [  565.010536] ------------[ cut here ]------------
> [  565.010940] No atomic I2C transfer handler for 'i2c-0'
> [  565.011437] WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:40
> i2c_transfer+0xe4/0xf8
> [  565.012126] Modules linked in: snd_soc_hdmi_codec dw_hdmi_i2s_audio
> rockchipdrm nvme analogix_dp nvme_core brcmfmac hci_uart dw_mipi_dsi
> dw_hdmi btbcm cec panfrost bluetooth drm_kms_helper brcmutil gpu_sched
> cfg80211 crct10dif_ce snd_soc_rockchip_i2s snd_soc_simple_card drm
> ecdh_generic snd_soc_rockchip_pcm snd_soc_simple_card_utils
> phy_rockchip_pcie ecc rtc_rk808 rfkill rockchip_thermal
> pcie_rockchip_host ip_tables x_tables ipv6 nf_defrag_ipv6
> [  565.015578] CPU: 0 PID: 1 Comm: shutdown Not tainted
> 5.5.0-rc1-00292-gd46dd6369c55 #7
> [  565.016260] Hardware name: 96boards Rock960 (DT)
> [  565.016666] pstate: 60000085 (nZCv daIf -PAN -UAO)
> [  565.017087] pc : i2c_transfer+0xe4/0xf8
> [  565.017425] lr : i2c_transfer+0xe4/0xf8
> [  565.017762] sp : ffff80001004baf0
> [  565.018052] x29: ffff80001004baf0 x28: ffff00007d208000
> [  565.018517] x27: 0000000000000000 x26: 0000000000000000
> [  565.018982] x25: 0000000000000008 x24: 0000000000000000
> [  565.019447] x23: ffff00007d208000 x22: ffff80001004bc64
> [  565.019912] x21: ffff80001004bb48 x20: 0000000000000002
> [  565.020377] x19: ffff000078502080 x18: 0000000000000010
> [  565.020842] x17: 0000000000000001 x16: 0000000000000019
> [  565.021307] x15: ffff00007d208470 x14: ffffffffffffffff
> [  565.021772] x13: ffff80009004b857 x12: ffff80001004b860
> [  565.022237] x11: ffff800011841000 x10: ffff800011a10658
> [  565.022702] x9 : 0000000000000000 x8 : ffff800011a11000
> [  565.023167] x7 : ffff800010697c78 x6 : 0000000000000262
> [  565.023632] x5 : 0000000000000000 x4 : 0000000000000000
> [  565.024096] x3 : 00000000ffffffff x2 : ffff800011841ab8
> [  565.024561] x1 : 7b11701b0ae78800 x0 : 0000000000000000
> [  565.025027] Call trace:
> [  565.025246]  i2c_transfer+0xe4/0xf8
> [  565.025556]  regmap_i2c_read+0x5c/0xa0
> [  565.025886]  _regmap_raw_read+0xcc/0x138
> [  565.026230]  _regmap_bus_read+0x3c/0x70
> [  565.026568]  _regmap_read+0x60/0xe0
> [  565.026875]  _regmap_update_bits+0xc8/0x108
> [  565.027241]  regmap_update_bits_base+0x60/0x90
> [  565.027633]  rk808_device_shutdown+0x6c/0x88
> [  565.028010]  machine_power_off+0x24/0x30
> [  565.028356]  kernel_power_off+0x64/0x70
> [  565.028693]  __do_sys_reboot+0x15c/0x240
> [  565.029038]  __arm64_sys_reboot+0x20/0x28
> [  565.029390]  el0_svc_common.constprop.0+0x68/0x160
> [  565.029811]  el0_svc_handler+0x20/0x80
> [  565.030141]  el0_sync_handler+0x10c/0x180
> [  565.030493]  el0_sync+0x140/0x180
> [  565.030785] ---[ end trace 5167e842ce15f686 ]---
> 
> -Anand
> 

Re: [PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

[Soeren Moch]

On 15.12.19 21:27, Heiko Stübner wrote:
> Hi Anand,
>
> Am Sonntag, 15. Dezember 2019, 19:51:50 CET schrieb Anand Moon:
>> On Tue, 10 Dec 2019 at 18:54, Robin Murphy <robin.murphy@arm.com> wrote:
>>> RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
>>> so it makes little sense for the driver to have to have two completely
>>> different mechanisms to handle essentially the same thing. Bring RK805
>>> in line with the RK809/RK817 flow to clean things up.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
> [...]
>
>> I am sill getting the kernel warning on issue poweroff see below.
>> on my Rock960 Model A
>> I feel the reason for this is we now have two poweroff callback
>> 1  pm_power_off = rk808_device_shutdown
>> 2  rk8xx_syscore_shutdown
> Nope, the issue is just the i2c subsystem complaining that the
> Rocckhip i2c drives does not provide an atomic-transfer function, see
> 	"No atomic I2C transfer handler for 'i2c-0'"
> in your warning.
>
> Somewhere it was suggested that the current transfer function just
> works as atomic as well.
I suggested to declare this function as "atomic" in a sense that it can
be used for power-off (this is what the i2c core expects from atomic
xfer functions, they are not used for anything else).
The current transfer function is not strictly atomic, since it expects
to get a completion interrupt. But nobody cares about the delivery of
such interrupt when the board is already switched off.

So the naming xfer_atomic is disadvantageous, if it had be named
xfer_poweroff instead there would be no doubt that the existing function
can be marked this way.
>> In my investigation earlier common function for shutdown solve
>> the issue of clean shutdown.
> This is simply a result of your syscore-shutdown function running way to
> early, before the i2c subsystem switched to using atomic transfers.
>
> This also indicates that this would really be way to early, as other parts
> of the kernel could also still be running.
Exactly. So I still think that my simple patch does the right thing, and
this warning can be safely ignored.

Soeren
>
> Heiko
>
>
>> for *rockchip,system-power-controller* dts property
>> we can used flags if check if this property support clean shutdown
>> for that device.
>>
>> [  565.009291] xhci-hcd xhci-hcd.0.auto: USB bus 5 deregistered
>> [  565.010179] reboot: Power down
>> [  565.010536] ------------[ cut here ]------------
>> [  565.010940] No atomic I2C transfer handler for 'i2c-0'
>> [  565.011437] WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:40
>> i2c_transfer+0xe4/0xf8
>> [  565.012126] Modules linked in: snd_soc_hdmi_codec dw_hdmi_i2s_audio
>> rockchipdrm nvme analogix_dp nvme_core brcmfmac hci_uart dw_mipi_dsi
>> dw_hdmi btbcm cec panfrost bluetooth drm_kms_helper brcmutil gpu_sched
>> cfg80211 crct10dif_ce snd_soc_rockchip_i2s snd_soc_simple_card drm
>> ecdh_generic snd_soc_rockchip_pcm snd_soc_simple_card_utils
>> phy_rockchip_pcie ecc rtc_rk808 rfkill rockchip_thermal
>> pcie_rockchip_host ip_tables x_tables ipv6 nf_defrag_ipv6
>> [  565.015578] CPU: 0 PID: 1 Comm: shutdown Not tainted
>> 5.5.0-rc1-00292-gd46dd6369c55 #7
>> [  565.016260] Hardware name: 96boards Rock960 (DT)
>> [  565.016666] pstate: 60000085 (nZCv daIf -PAN -UAO)
>> [  565.017087] pc : i2c_transfer+0xe4/0xf8
>> [  565.017425] lr : i2c_transfer+0xe4/0xf8
>> [  565.017762] sp : ffff80001004baf0
>> [  565.018052] x29: ffff80001004baf0 x28: ffff00007d208000
>> [  565.018517] x27: 0000000000000000 x26: 0000000000000000
>> [  565.018982] x25: 0000000000000008 x24: 0000000000000000
>> [  565.019447] x23: ffff00007d208000 x22: ffff80001004bc64
>> [  565.019912] x21: ffff80001004bb48 x20: 0000000000000002
>> [  565.020377] x19: ffff000078502080 x18: 0000000000000010
>> [  565.020842] x17: 0000000000000001 x16: 0000000000000019
>> [  565.021307] x15: ffff00007d208470 x14: ffffffffffffffff
>> [  565.021772] x13: ffff80009004b857 x12: ffff80001004b860
>> [  565.022237] x11: ffff800011841000 x10: ffff800011a10658
>> [  565.022702] x9 : 0000000000000000 x8 : ffff800011a11000
>> [  565.023167] x7 : ffff800010697c78 x6 : 0000000000000262
>> [  565.023632] x5 : 0000000000000000 x4 : 0000000000000000
>> [  565.024096] x3 : 00000000ffffffff x2 : ffff800011841ab8
>> [  565.024561] x1 : 7b11701b0ae78800 x0 : 0000000000000000
>> [  565.025027] Call trace:
>> [  565.025246]  i2c_transfer+0xe4/0xf8
>> [  565.025556]  regmap_i2c_read+0x5c/0xa0
>> [  565.025886]  _regmap_raw_read+0xcc/0x138
>> [  565.026230]  _regmap_bus_read+0x3c/0x70
>> [  565.026568]  _regmap_read+0x60/0xe0
>> [  565.026875]  _regmap_update_bits+0xc8/0x108
>> [  565.027241]  regmap_update_bits_base+0x60/0x90
>> [  565.027633]  rk808_device_shutdown+0x6c/0x88
>> [  565.028010]  machine_power_off+0x24/0x30
>> [  565.028356]  kernel_power_off+0x64/0x70
>> [  565.028693]  __do_sys_reboot+0x15c/0x240
>> [  565.029038]  __arm64_sys_reboot+0x20/0x28
>> [  565.029390]  el0_svc_common.constprop.0+0x68/0x160
>> [  565.029811]  el0_svc_handler+0x20/0x80
>> [  565.030141]  el0_sync_handler+0x10c/0x180
>> [  565.030493]  el0_sync+0x140/0x180
>> [  565.030785] ---[ end trace 5167e842ce15f686 ]---
>>
>> -Anand
>>
>
>
>

Re: [PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

[Anand Moon]

[-- Attachment #1: Type: text/plain, Size: 6608 bytes --]

Hi Heiko / Robin / Soeren,

On Mon, 16 Dec 2019 at 01:57, Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi Anand,
>
> Am Sonntag, 15. Dezember 2019, 19:51:50 CET schrieb Anand Moon:
> > On Tue, 10 Dec 2019 at 18:54, Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
> > > so it makes little sense for the driver to have to have two completely
> > > different mechanisms to handle essentially the same thing. Bring RK805
> > > in line with the RK809/RK817 flow to clean things up.
> > >
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
>
> [...]
>
> > I am sill getting the kernel warning on issue poweroff see below.
> > on my Rock960 Model A
> > I feel the reason for this is we now have two poweroff callback
> > 1  pm_power_off = rk808_device_shutdown
> > 2  rk8xx_syscore_shutdown
>
> Nope, the issue is just the i2c subsystem complaining that the
> Rocckhip i2c drives does not provide an atomic-transfer function, see
>         "No atomic I2C transfer handler for 'i2c-0'"
> in your warning.
>
> Somewhere it was suggested that the current transfer function just
> works as atomic as well.
>
>
> > In my investigation earlier common function for shutdown solve
> > the issue of clean shutdown.
>
> This is simply a result of your syscore-shutdown function running way to
> early, before the i2c subsystem switched to using atomic transfers.
>
> This also indicates that this would really be way to early, as other parts
> of the kernel could also still be running.
>

Yes, you are correct syscore-shutdown initiates
shutdown before all the device do clean shutdown.

So for best approach for clean atomic shutdown is to use
  /* driver model interfaces that don't relate to enumeration  */
        void (*shutdown)(struct i2c_client *client);
drop the registration of syscore and use core .i2c_shutdown.

I have prepare this patch on top of this series for RTF
This patch dose clean shutdown of all the devices before poweroff.
see the log below.

*Note*: This feature will likely break the clean reboot feature.
Rockchip device do not perform clean reboot as some of the IP
block are not released before clean reboot and it's remain stuck.
Like PCIe and MMC, We need to look into this as well.

Shutdown log of my RK3399 Rock960 Model A
[0] https://pastebin.com/peYxmzb7
------------------------------------------------------------------------
[  OK  ] Stopped LVM2 metadata daemon.
[  OK  ] Reached target Shutdown.
[  OK  ] Reached target Final Step.
[  OK  ] Closed LVM2 metadata daemon socket.
[  OK  ] Started Power-Off.
[  OK  ] Reached target Power-Off.
[  542.715237] systemd-shutdown[1]: Syncing filesystems and block devices.
[  543.158314] systemd-shutdown[1]: Sending SIGTERM to remaining processes...
[  543.168469] systemd-journald[280]: Received SIGTERM from PID 1
(systemd-shutdow).
[  543.202968] systemd-shutdown[1]: Sending SIGKILL to remaining processes...
[  543.212365] systemd-shutdown[1]: Unmounting file systems.
[  543.214708] [535]: Remounting '/' read-only in with options '(null)'.
[  543.229661] EXT4-fs (mmcblk1p1): re-mounted. Opts: (null)
[  543.239978] systemd-shutdown[1]: All filesystems unmounted.
[  543.240481] systemd-shutdown[1]: Deactivating swaps.
[  543.241052] systemd-shutdown[1]: All swaps deactivated.
[  543.241514] systemd-shutdown[1]: Detaching loop devices.
[  543.244806] systemd-shutdown[1]: All loop devices detached.
[  543.245307] systemd-shutdown[1]: Detaching DM devices.
[  543.245994] systemd-shutdown[1]: All DM devices detached.
[  543.246474] systemd-shutdown[1]: All filesystems, swaps, loop
devices and DM devices detached.
[  543.302732] systemd-shutdown[1]: Successfully changed into root pivot.
[  543.303356] systemd-shutdown[1]: Returning to initrd...
[  543.339679] shutdown[1]: Syncing filesystems and block devices.
[  543.341084] shutdown[1]: Sending SIGTERM to remaining processes...
[  543.348948] shutdown[1]: Sending SIGKILL to remaining processes...
[  543.356551] shutdown[1]: Unmounting file systems.
[  543.359097] sd-umoun[541]: Unmounting '/oldroot/sys/kernel/config'.
[  543.361716] sd-umoun[542]: Unmounting '/oldroot/sys/kernel/debug'.
[  543.364333] sd-umoun[543]: Unmounting '/oldroot/dev/mqueue'.
[  543.366765] sd-umoun[544]: Unmounting '/oldroot/dev/hugepages'.
[  543.369426] sd-umoun[545]: Unmounting '/oldroot/sys/fs/cgroup/memory'.
[  543.372338] sd-umoun[546]: Unmounting '/oldroot/sys/fs/cgroup/perf_event'.
[  543.375030] sd-umoun[547]: Unmounting '/oldroot/sys/fs/cgroup/cpu,cpuacct'.
[  543.377744] sd-umoun[548]: Unmounting '/oldroot/sys/fs/cgroup/pids'.
[  543.380620] sd-umoun[549]: Unmounting '/oldroot/sys/fs/cgroup/blkio'.
[  543.383256] sd-umoun[550]: Unmounting '/oldroot/sys/fs/cgroup/hugetlb'.
[  543.386015] sd-umoun[551]: Unmounting '/oldroot/sys/fs/cgroup/devices'.
[  543.389114] sd-umoun[552]: Unmounting '/oldroot/sys/fs/cgroup/cpuset'.
[  543.391817] sd-umoun[553]: Unmounting '/oldroot/sys/fs/pstore'.
[  543.394401] sd-umoun[554]: Unmounting '/oldroot/sys/fs/cgroup/systemd'.
[  543.397245] sd-umoun[555]: Unmounting '/oldroot/sys/fs/cgroup/unified'.
[  543.400083] sd-umoun[556]: Unmounting '/oldroot/sys/fs/cgroup'.
[  543.402654] sd-umoun[557]: Unmounting '/oldroot/dev/pts'.
[  543.405351] sd-umoun[558]: Unmounting '/oldroot/dev/shm'.
[  543.407876] sd-umoun[559]: Unmounting '/oldroot/sys/kernel/security'.
[  543.410313] sd-umoun[560]: Unmounting '/oldroot'.
[  543.410886] sd-umoun[560]: Failed to unmount /oldroot: Device or
resource busy
[  543.413355] sd-umoun[561]: Unmounting '/oldroot/run'.
[  543.415750] sd-umoun[562]: Unmounting '/oldroot/dev'.
[  543.418013] sd-umoun[563]: Unmounting '/oldroot/sys'.
[  543.420892] sd-umoun[564]: Unmounting '/oldroot/proc'.
[  543.423833] sd-umoun[565]: Unmounting '/oldroot'.
[  543.486268] shutdown[1]: All filesystems unmounted.
[  543.486710] shutdown[1]: Deactivating swaps.
[  543.487153] shutdown[1]: All swaps deactivated.
[  543.487556] shutdown[1]: Detaching loop devices.
[  543.490300] shutdown[1]: All loop devices detached.
[  543.490735] shutdown[1]: Detaching DM devices.
[  543.491382] shutdown[1]: All DM devices detached.
[  543.491801] shutdown[1]: All filesystems, swaps, loop devices and
DM devices detached.
[  543.494678] shutdown[1]: Syncing filesystems and block devices.
[  543.495770] shutdown[1]: Powering off.
[  543.496112] kvm: exiting hardware virtualization

-Anand

[-- Attachment #2: i2c_shutdown.patch --]
[-- Type: application/octet-stream, Size: 3272 bytes --]

diff --git a/drivers/mfd/rk808.c b/drivers/mfd/rk808.c
index e88bdb889d3a..823803df2d7b 100644
--- a/drivers/mfd/rk808.c
+++ b/drivers/mfd/rk808.c
@@ -448,36 +448,7 @@ static const struct regmap_irq_chip rk818_irq_chip = {
 
 static struct i2c_client *rk808_i2c_client;
 
-static void rk808_device_shutdown(void)
-{
-	int ret;
-	unsigned int reg, bit;
-	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
-
-	if (!rk808)
-		return;
-
-	switch (rk808->variant) {
-	case RK805_ID:
-		reg = RK805_DEV_CTRL_REG;
-		bit = DEV_OFF;
-		break;
-	case RK808_ID:
-		reg = RK808_DEVCTRL_REG,
-		bit = DEV_OFF_RST;
-		break;
-	case RK818_ID:
-		reg = RK818_DEVCTRL_REG;
-		bit = DEV_OFF;
-		break;
-	default:
-		return;
-	}
-	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
-	if (ret)
-		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
-}
-
+#if 0
 static void rk8xx_syscore_shutdown(void)
 {
 	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
@@ -511,6 +482,7 @@ static void rk8xx_syscore_shutdown(void)
 static struct syscore_ops rk808_syscore_ops = {
 	.shutdown = rk8xx_syscore_shutdown,
 };
+#endif
 
 static const struct of_device_id rk808_of_match[] = {
 	{ .compatible = "rockchip,rk805" },
@@ -643,8 +615,9 @@ static int rk808_probe(struct i2c_client *client,
 	}
 
 	rk808_i2c_client = client;
+#if 0
 	register_syscore_ops(&rk808_syscore_ops);
-
+#endif
 	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
 			      cells, nr_cells, NULL, 0,
 			      regmap_irq_get_domain(rk808->irq_data));
@@ -653,13 +626,16 @@ static int rk808_probe(struct i2c_client *client,
 		goto err_irq;
 	}
 
+#if 0
 	if (of_property_read_bool(np, "rockchip,system-power-controller"))
 		pm_power_off = rk808_device_shutdown;
-
+#endif
 	return 0;
 
 err_irq:
+#if 0
 	unregister_syscore_ops(&rk808_syscore_ops);
+#endif
 	regmap_del_irq_chip(client->irq, rk808->irq_data);
 	return ret;
 }
@@ -670,18 +646,48 @@ static int rk808_remove(struct i2c_client *client)
 
 	regmap_del_irq_chip(client->irq, rk808->irq_data);
 
+#if 0
 	unregister_syscore_ops(&rk808_syscore_ops);
-
 	/**
 	 * pm_power_off may points to a function from another module.
 	 * Check if the pointer is set by us and only then overwrite it.
 	 */
 	if (pm_power_off == rk808_device_shutdown)
 		pm_power_off = NULL;
-
+#endif
 	return 0;
 }
 
+static void rk808_shutdown(struct i2c_client *client)
+{
+	int ret;
+	unsigned int reg, bit;
+	struct rk808 *rk808 = i2c_get_clientdata(client);
+
+	if (!rk808)
+		return;
+
+	switch (rk808->variant) {
+	case RK805_ID:
+		reg = RK805_DEV_CTRL_REG;
+		bit = DEV_OFF;
+		break;
+	case RK808_ID:
+		reg = RK808_DEVCTRL_REG,
+		bit = DEV_OFF_RST;
+		break;
+	case RK818_ID:
+		reg = RK818_DEVCTRL_REG;
+		bit = DEV_OFF;
+		break;
+	default:
+		return;
+	}
+	ret = regmap_update_bits(rk808->regmap, reg, bit, bit);
+	if (ret)
+		dev_err(&rk808_i2c_client->dev, "Failed to shutdown device!\n");
+}
+
 static int __maybe_unused rk8xx_suspend(struct device *dev)
 {
 	struct rk808 *rk808 = i2c_get_clientdata(rk808_i2c_client);
@@ -737,6 +743,7 @@ static struct i2c_driver rk808_i2c_driver = {
 	},
 	.probe    = rk808_probe,
 	.remove   = rk808_remove,
+	.shutdown  = rk808_shutdown,
 };
 
 module_i2c_driver(rk808_i2c_driver);

Re: [PATCH 0/4] mfd: RK8xx tidyup

[Lee Jones]

On Tue, 10 Dec 2019, Robin Murphy wrote:

> Hi all,
> 
> In trying to debug suspend issues on my RK3328 box, I was looking at
> how the RK8xx driver handles the RK805 sleep pin, and frankly the whole
> driver seemed untidy enough to warrant some cleanup and minor fixes
> before going any further. I've based the series on top of Soeren's
> "mfd: rk808: Always use poweroff when requested" patch[1].
> 
> Note that I've only had time to build-test these patches so far, but I
> wanted to share them early for the sake of discussion in response to
> the other thread[2].
> 
> Robin.
> 
> [1] https://patchwork.kernel.org/patch/11279249/
> [2] https://patchwork.kernel.org/cover/11276945/
> 
> Robin Murphy (4):
>   mfd: rk808: Set global instance unconditionally
>   mfd: rk808: Always register syscore ops
>   mfd: rk808: Reduce shutdown duplication
>   mfd: rk808: Convert RK805 to syscore/PM ops
> 
>  drivers/mfd/rk808.c       | 122 ++++++++++++++++----------------------
>  include/linux/mfd/rk808.h |   2 -
>  2 files changed, 50 insertions(+), 74 deletions(-)

Not sure what's happening with these (competing?) patch-sets.  I'm not
planning on getting involved until you guys have arrived at and agreed
upon a single solution.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

[Robin Murphy]

On 2019-12-16 9:50 am, Anand Moon wrote:
> Hi Heiko / Robin / Soeren,
> 
> On Mon, 16 Dec 2019 at 01:57, Heiko Stübner <heiko@sntech.de> wrote:
>>
>> Hi Anand,
>>
>> Am Sonntag, 15. Dezember 2019, 19:51:50 CET schrieb Anand Moon:
>>> On Tue, 10 Dec 2019 at 18:54, Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
>>>> so it makes little sense for the driver to have to have two completely
>>>> different mechanisms to handle essentially the same thing. Bring RK805
>>>> in line with the RK809/RK817 flow to clean things up.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>
>> [...]
>>
>>> I am sill getting the kernel warning on issue poweroff see below.
>>> on my Rock960 Model A
>>> I feel the reason for this is we now have two poweroff callback
>>> 1  pm_power_off = rk808_device_shutdown
>>> 2  rk8xx_syscore_shutdown
>>
>> Nope, the issue is just the i2c subsystem complaining that the
>> Rocckhip i2c drives does not provide an atomic-transfer function, see
>>          "No atomic I2C transfer handler for 'i2c-0'"
>> in your warning.
>>
>> Somewhere it was suggested that the current transfer function just
>> works as atomic as well.
>>
>>
>>> In my investigation earlier common function for shutdown solve
>>> the issue of clean shutdown.
>>
>> This is simply a result of your syscore-shutdown function running way to
>> early, before the i2c subsystem switched to using atomic transfers.
>>
>> This also indicates that this would really be way to early, as other parts
>> of the kernel could also still be running.
>>
> 
> Yes, you are correct syscore-shutdown initiates
> shutdown before all the device do clean shutdown.
> 
> So for best approach for clean atomic shutdown is to use
>    /* driver model interfaces that don't relate to enumeration  */
>          void (*shutdown)(struct i2c_client *client);
> drop the registration of syscore and use core .i2c_shutdown.

Huh? If you understand that the syscore shutdown hook is too early, why 
would it seem a good idea to pull the plug even earlier from driver 
shutdown? Not to mention that your patch as proposed breaks all the 
GPIO-based shutdown flows.

If you really care about avoiding the spurious warning, implement the 
expected polled-mode transfer function in the I2C driver. Trying to hack 
around it by issuing I2C-based shutdown from anywhere other than 
pm_power_off is a waste of everyone's time.

> I have prepare this patch on top of this series for RTF
> This patch dose clean shutdown of all the devices before poweroff.
> see the log below.
> 
> *Note*: This feature will likely break the clean reboot feature.
> Rockchip device do not perform clean reboot as some of the IP
> block are not released before clean reboot and it's remain stuck.
> Like PCIe and MMC, We need to look into this as well.

As mentioned before, that likely has nothing to do with the PMIC, and 
really sounds like the issue with Trusted Firmware not reenabling all 
the SoC power domains before reset - a fix for that has already been 
identified, see here: 
https://forum.armbian.com/topic/7552-roc-rk3399-pc-renegade-elite/?do=findComment&comment=90289

Robin.

> Shutdown log of my RK3399 Rock960 Model A
> [0] https://pastebin.com/peYxmzb7
> ------------------------------------------------------------------------
> [  OK  ] Stopped LVM2 metadata daemon.
> [  OK  ] Reached target Shutdown.
> [  OK  ] Reached target Final Step.
> [  OK  ] Closed LVM2 metadata daemon socket.
> [  OK  ] Started Power-Off.
> [  OK  ] Reached target Power-Off.
> [  542.715237] systemd-shutdown[1]: Syncing filesystems and block devices.
> [  543.158314] systemd-shutdown[1]: Sending SIGTERM to remaining processes...
> [  543.168469] systemd-journald[280]: Received SIGTERM from PID 1
> (systemd-shutdow).
> [  543.202968] systemd-shutdown[1]: Sending SIGKILL to remaining processes...
> [  543.212365] systemd-shutdown[1]: Unmounting file systems.
> [  543.214708] [535]: Remounting '/' read-only in with options '(null)'.
> [  543.229661] EXT4-fs (mmcblk1p1): re-mounted. Opts: (null)
> [  543.239978] systemd-shutdown[1]: All filesystems unmounted.
> [  543.240481] systemd-shutdown[1]: Deactivating swaps.
> [  543.241052] systemd-shutdown[1]: All swaps deactivated.
> [  543.241514] systemd-shutdown[1]: Detaching loop devices.
> [  543.244806] systemd-shutdown[1]: All loop devices detached.
> [  543.245307] systemd-shutdown[1]: Detaching DM devices.
> [  543.245994] systemd-shutdown[1]: All DM devices detached.
> [  543.246474] systemd-shutdown[1]: All filesystems, swaps, loop
> devices and DM devices detached.
> [  543.302732] systemd-shutdown[1]: Successfully changed into root pivot.
> [  543.303356] systemd-shutdown[1]: Returning to initrd...
> [  543.339679] shutdown[1]: Syncing filesystems and block devices.
> [  543.341084] shutdown[1]: Sending SIGTERM to remaining processes...
> [  543.348948] shutdown[1]: Sending SIGKILL to remaining processes...
> [  543.356551] shutdown[1]: Unmounting file systems.
> [  543.359097] sd-umoun[541]: Unmounting '/oldroot/sys/kernel/config'.
> [  543.361716] sd-umoun[542]: Unmounting '/oldroot/sys/kernel/debug'.
> [  543.364333] sd-umoun[543]: Unmounting '/oldroot/dev/mqueue'.
> [  543.366765] sd-umoun[544]: Unmounting '/oldroot/dev/hugepages'.
> [  543.369426] sd-umoun[545]: Unmounting '/oldroot/sys/fs/cgroup/memory'.
> [  543.372338] sd-umoun[546]: Unmounting '/oldroot/sys/fs/cgroup/perf_event'.
> [  543.375030] sd-umoun[547]: Unmounting '/oldroot/sys/fs/cgroup/cpu,cpuacct'.
> [  543.377744] sd-umoun[548]: Unmounting '/oldroot/sys/fs/cgroup/pids'.
> [  543.380620] sd-umoun[549]: Unmounting '/oldroot/sys/fs/cgroup/blkio'.
> [  543.383256] sd-umoun[550]: Unmounting '/oldroot/sys/fs/cgroup/hugetlb'.
> [  543.386015] sd-umoun[551]: Unmounting '/oldroot/sys/fs/cgroup/devices'.
> [  543.389114] sd-umoun[552]: Unmounting '/oldroot/sys/fs/cgroup/cpuset'.
> [  543.391817] sd-umoun[553]: Unmounting '/oldroot/sys/fs/pstore'.
> [  543.394401] sd-umoun[554]: Unmounting '/oldroot/sys/fs/cgroup/systemd'.
> [  543.397245] sd-umoun[555]: Unmounting '/oldroot/sys/fs/cgroup/unified'.
> [  543.400083] sd-umoun[556]: Unmounting '/oldroot/sys/fs/cgroup'.
> [  543.402654] sd-umoun[557]: Unmounting '/oldroot/dev/pts'.
> [  543.405351] sd-umoun[558]: Unmounting '/oldroot/dev/shm'.
> [  543.407876] sd-umoun[559]: Unmounting '/oldroot/sys/kernel/security'.
> [  543.410313] sd-umoun[560]: Unmounting '/oldroot'.
> [  543.410886] sd-umoun[560]: Failed to unmount /oldroot: Device or
> resource busy
> [  543.413355] sd-umoun[561]: Unmounting '/oldroot/run'.
> [  543.415750] sd-umoun[562]: Unmounting '/oldroot/dev'.
> [  543.418013] sd-umoun[563]: Unmounting '/oldroot/sys'.
> [  543.420892] sd-umoun[564]: Unmounting '/oldroot/proc'.
> [  543.423833] sd-umoun[565]: Unmounting '/oldroot'.
> [  543.486268] shutdown[1]: All filesystems unmounted.
> [  543.486710] shutdown[1]: Deactivating swaps.
> [  543.487153] shutdown[1]: All swaps deactivated.
> [  543.487556] shutdown[1]: Detaching loop devices.
> [  543.490300] shutdown[1]: All loop devices detached.
> [  543.490735] shutdown[1]: Detaching DM devices.
> [  543.491382] shutdown[1]: All DM devices detached.
> [  543.491801] shutdown[1]: All filesystems, swaps, loop devices and
> DM devices detached.
> [  543.494678] shutdown[1]: Syncing filesystems and block devices.
> [  543.495770] shutdown[1]: Powering off.
> [  543.496112] kvm: exiting hardware virtualization
> 
> -Anand
> 

Re: [PATCH 4/4] mfd: rk808: Convert RK805 to syscore/PM ops

[Anand Moon]

Hi Robin,

On Mon, 16 Dec 2019 at 18:08, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2019-12-16 9:50 am, Anand Moon wrote:
> > Hi Heiko / Robin / Soeren,
> >
> > On Mon, 16 Dec 2019 at 01:57, Heiko Stübner <heiko@sntech.de> wrote:
> >>
> >> Hi Anand,
> >>
> >> Am Sonntag, 15. Dezember 2019, 19:51:50 CET schrieb Anand Moon:
> >>> On Tue, 10 Dec 2019 at 18:54, Robin Murphy <robin.murphy@arm.com> wrote:
> >>>>
> >>>> RK805 has the same kind of dual-role sleep/shutdown pin as RK809/RK817,
> >>>> so it makes little sense for the driver to have to have two completely
> >>>> different mechanisms to handle essentially the same thing. Bring RK805
> >>>> in line with the RK809/RK817 flow to clean things up.
> >>>>
> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> >>>> ---
> >>
> >> [...]
> >>
> >>> I am sill getting the kernel warning on issue poweroff see below.
> >>> on my Rock960 Model A
> >>> I feel the reason for this is we now have two poweroff callback
> >>> 1  pm_power_off = rk808_device_shutdown
> >>> 2  rk8xx_syscore_shutdown
> >>
> >> Nope, the issue is just the i2c subsystem complaining that the
> >> Rocckhip i2c drives does not provide an atomic-transfer function, see
> >>          "No atomic I2C transfer handler for 'i2c-0'"
> >> in your warning.
> >>
> >> Somewhere it was suggested that the current transfer function just
> >> works as atomic as well.
> >>
> >>
> >>> In my investigation earlier common function for shutdown solve
> >>> the issue of clean shutdown.
> >>
> >> This is simply a result of your syscore-shutdown function running way to
> >> early, before the i2c subsystem switched to using atomic transfers.
> >>
> >> This also indicates that this would really be way to early, as other parts
> >> of the kernel could also still be running.
> >>
> >
> > Yes, you are correct syscore-shutdown initiates
> > shutdown before all the device do clean shutdown.
> >
> > So for best approach for clean atomic shutdown is to use
> >    /* driver model interfaces that don't relate to enumeration  */
> >          void (*shutdown)(struct i2c_client *client);
> > drop the registration of syscore and use core .i2c_shutdown.
>
> Huh? If you understand that the syscore shutdown hook is too early, why
> would it seem a good idea to pull the plug even earlier from driver
> shutdown? Not to mention that your patch as proposed breaks all the
> GPIO-based shutdown flows.
>
Ok opps, I might have missed some thing.
I just look into logs to between syscore shutdown and I2C shutdown
get more insight, so I feel I2C shutdown dose clean shutdown.

> If you really care about avoiding the spurious warning, implement the
> expected polled-mode transfer function in the I2C driver. Trying to hack
> around it by issuing I2C-based shutdown from anywhere other than
> pm_power_off is a waste of everyone's time.

I have tired this I2C shutdown approach earlier, but as their were
issue with clean restart, so I dropped this line.
Then I tired to add restart notifier to handle restart that also
did not work my boards, so I am exploring more.

>
> > I have prepare this patch on top of this series for RTF
> > This patch dose clean shutdown of all the devices before poweroff.
> > see the log below.
> >
> > *Note*: This feature will likely break the clean reboot feature.
> > Rockchip device do not perform clean reboot as some of the IP
> > block are not released before clean reboot and it's remain stuck.
> > Like PCIe and MMC, We need to look into this as well.
>
> As mentioned before, that likely has nothing to do with the PMIC, and
> really sounds like the issue with Trusted Firmware not reenabling all
> the SoC power domains before reset - a fix for that has already been
> identified, see here:
> https://forum.armbian.com/topic/7552-roc-rk3399-pc-renegade-elite/?do=findComment&comment=90289
>
> Robin.
>

If we go with I2C shutdown feature, some how some device will not
release resources and it remains stuck before the initialization next u-boot.
See the log below. https://pastebin.com/xxwvERaz

ATF changes will some into affect after the restart of the devices.
FYI I am testing with all the latest AFT patches from Armbian and latest u-boot.

-Anand

Re: [PATCH 0/4] mfd: RK8xx tidyup

[Soeren Moch]

On 16.12.19 12:12, Lee Jones wrote:
> On Tue, 10 Dec 2019, Robin Murphy wrote:
>
>> Hi all,
>>
>> In trying to debug suspend issues on my RK3328 box, I was looking at
>> how the RK8xx driver handles the RK805 sleep pin, and frankly the whole
>> driver seemed untidy enough to warrant some cleanup and minor fixes
>> before going any further. I've based the series on top of Soeren's
>> "mfd: rk808: Always use poweroff when requested" patch[1].
>>
>> Note that I've only had time to build-test these patches so far, but I
>> wanted to share them early for the sake of discussion in response to
>> the other thread[2].
>>
>> Robin.
>>
>> [1] https://patchwork.kernel.org/patch/11279249/
>> [2] https://patchwork.kernel.org/cover/11276945/
>>
>> Robin Murphy (4):
>>   mfd: rk808: Set global instance unconditionally
>>   mfd: rk808: Always register syscore ops
>>   mfd: rk808: Reduce shutdown duplication
>>   mfd: rk808: Convert RK805 to syscore/PM ops
>>
>>  drivers/mfd/rk808.c       | 122 ++++++++++++++++----------------------
>>  include/linux/mfd/rk808.h |   2 -
>>  2 files changed, 50 insertions(+), 74 deletions(-)
> Not sure what's happening with these (competing?) patch-sets.  I'm not
> planning on getting involved until you guys have arrived at and agreed
> upon a single solution.
>
My understanding is like this:

The patch [1] fixes power-off to use the method requested in the
devicetree. This patch series on top of that cleans up the rk808
power-off code, which perfectly makes sense for me. I think these 2
patch series are not controversial as such.

And then we have the series [2], which is a mix of
- some clean-up
- converting the existing code to use syscon_shutdown for actual power-off

Robin, Heiko, and myself agree that the shutdown method introduced in
[2] is fundamentally wrong. All syscon_shutdown methods of all
registered syscons have to run before the actual shutdown, what is
triggered in pm_power_off. This is how it works now, and what is the
right way to do it. Patch series [2] breaks this promise since it does
the actual shutdown in the middle of the chain of syscon_shutdown handlers.

In the discussion about series [2] Anand repeatedly talks about restart
problems. But this rk808 mfd driver is not involved in restart, also
patch series [2] 
does not change that. So I cannot see what should be the point here.

There was an earlier attempt [3] to enhance this rk808 mfd driver for
restart. I'm not sure, however, what happened to this. For me it could
make sense to reimplement such restart functionality on top of this
"mfd: RK8xx tidyup" series.

Regards,
Soeren

[3] https://lore.kernel.org/patchwork/patch/934323/

Re: [PATCH 0/4] mfd: RK8xx tidyup

[Soeren Moch]

On 16.12.19 12:12, Lee Jones wrote:
> On Tue, 10 Dec 2019, Robin Murphy wrote:
>
>> Hi all,
>>
>> In trying to debug suspend issues on my RK3328 box, I was looking at
>> how the RK8xx driver handles the RK805 sleep pin, and frankly the whole
>> driver seemed untidy enough to warrant some cleanup and minor fixes
>> before going any further. I've based the series on top of Soeren's
>> "mfd: rk808: Always use poweroff when requested" patch[1].
>>
>> Note that I've only had time to build-test these patches so far, but I
>> wanted to share them early for the sake of discussion in response to
>> the other thread[2].
>>
>> Robin.
>>
>> [1] https://patchwork.kernel.org/patch/11279249/
>> [2] https://patchwork.kernel.org/cover/11276945/
>>
>> Robin Murphy (4):
>>   mfd: rk808: Set global instance unconditionally
>>   mfd: rk808: Always register syscore ops
>>   mfd: rk808: Reduce shutdown duplication
>>   mfd: rk808: Convert RK805 to syscore/PM ops
>>
>>  drivers/mfd/rk808.c       | 122 ++++++++++++++++----------------------
>>  include/linux/mfd/rk808.h |   2 -
>>  2 files changed, 50 insertions(+), 74 deletions(-)
> Not sure what's happening with these (competing?) patch-sets.  I'm not
> planning on getting involved until you guys have arrived at and agreed
> upon a single solution.
>
My understanding is like this:

The patch [1] fixes power-off to use the method requested in the
devicetree. This patch series on top of that cleans up the rk808
power-off code, which perfectly makes sense for me. I think these 2
patch series are not controversial as such.

And then we have the series [2], which is a mix of
- some clean-up
- converting the existing code to use syscon_shutdown for actual power-off

Robin, Heiko, and myself agree that the shutdown method introduced in
[2] is fundamentally wrong. All syscon_shutdown methods of all
registered syscons have to run before the actual shutdown, what is
triggered in pm_power_off. This is how it works now, and what is the
right way to do it. Patch series [2] breaks this promise since it does
the actual shutdown in the middle of the chain of syscon_shutdown handlers.

In the discussion about series [2] Anand repeatedly talks about restart
problems. But this rk808 mfd driver is not involved in restart, also
patch series [2] 
does not change that. So I cannot see what should be the point here.

There was an earlier attempt [3] to enhance this rk808 mfd driver for
restart. I'm not sure, however, what happened to this. For me it could
make sense to reimplement such restart functionality on top of this
"mfd: RK8xx tidyup" series.

Regards,
Soeren

[3] https://lore.kernel.org/patchwork/patch/934323/



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH 0/4] mfd: RK8xx tidyup

[Anand Moon]

Hi All,

On Tue, 17 Dec 2019 at 05:00, Soeren Moch <smoch@web.de> wrote:
>
> On 16.12.19 12:12, Lee Jones wrote:
> > On Tue, 10 Dec 2019, Robin Murphy wrote:
> >
> >> Hi all,
> >>
> >> In trying to debug suspend issues on my RK3328 box, I was looking at
> >> how the RK8xx driver handles the RK805 sleep pin, and frankly the whole
> >> driver seemed untidy enough to warrant some cleanup and minor fixes
> >> before going any further. I've based the series on top of Soeren's
> >> "mfd: rk808: Always use poweroff when requested" patch[1].
> >>
> >> Note that I've only had time to build-test these patches so far, but I
> >> wanted to share them early for the sake of discussion in response to
> >> the other thread[2].
> >>
> >> Robin.
> >>
> >> [1] https://patchwork.kernel.org/patch/11279249/
> >> [2] https://patchwork.kernel.org/cover/11276945/
> >>
> >> Robin Murphy (4):
> >>   mfd: rk808: Set global instance unconditionally
> >>   mfd: rk808: Always register syscore ops
> >>   mfd: rk808: Reduce shutdown duplication
> >>   mfd: rk808: Convert RK805 to syscore/PM ops
> >>
> >>  drivers/mfd/rk808.c       | 122 ++++++++++++++++----------------------
> >>  include/linux/mfd/rk808.h |   2 -
> >>  2 files changed, 50 insertions(+), 74 deletions(-)
> > Not sure what's happening with these (competing?) patch-sets.  I'm not
> > planning on getting involved until you guys have arrived at and agreed
> > upon a single solution.
> >
> My understanding is like this:
>
> The patch [1] fixes power-off to use the method requested in the
> devicetree. This patch series on top of that cleans up the rk808
> power-off code, which perfectly makes sense for me. I think these 2
> patch series are not controversial as such.
>
> And then we have the series [2], which is a mix of
> - some clean-up
> - converting the existing code to use syscon_shutdown for actual power-off
>
> Robin, Heiko, and myself agree that the shutdown method introduced in
> [2] is fundamentally wrong. All syscon_shutdown methods of all
> registered syscons have to run before the actual shutdown, what is
> triggered in pm_power_off. This is how it works now, and what is the
> right way to do it. Patch series [2] breaks this promise since it does
> the actual shutdown in the middle of the chain of syscon_shutdown handlers.
>
> In the discussion about series [2] Anand repeatedly talks about restart
> problems. But this rk808 mfd driver is not involved in restart, also
> patch series [2]
> does not change that. So I cannot see what should be the point here.
>
Sorry I dont have any expert knowledge on this, so continue on best approach..

> There was an earlier attempt [3] to enhance this rk808 mfd driver for
> restart. I'm not sure, however, what happened to this. For me it could
> make sense to reimplement such restart functionality on top of this
> "mfd: RK8xx tidyup" series.
>
> Regards,
> Soeren
>
> [3] https://lore.kernel.org/patchwork/patch/934323/
>
>

-Anand

Re: [PATCH 0/4] mfd: RK8xx tidyup

[Soeren Moch]

On 10.12.19 14:24, Robin Murphy wrote:
> Hi all,
>
> In trying to debug suspend issues on my RK3328 box, I was looking at
> how the RK8xx driver handles the RK805 sleep pin, and frankly the whole
> driver seemed untidy enough to warrant some cleanup and minor fixes
> before going any further. I've based the series on top of Soeren's
> "mfd: rk808: Always use poweroff when requested" patch[1].
>
> Note that I've only had time to build-test these patches so far, but I
> wanted to share them early for the sake of discussion in response to
> the other thread[2].
>
> Robin.
>
> [1] https://patchwork.kernel.org/patch/11279249/
> [2] https://patchwork.kernel.org/cover/11276945/
>
> Robin Murphy (4):
>   mfd: rk808: Set global instance unconditionally
>   mfd: rk808: Always register syscore ops
>   mfd: rk808: Reduce shutdown duplication
>   mfd: rk808: Convert RK805 to syscore/PM ops
>
>  drivers/mfd/rk808.c       | 122 ++++++++++++++++----------------------
>  include/linux/mfd/rk808.h |   2 -
>  2 files changed, 50 insertions(+), 74 deletions(-)
>
The whole series, on top of [1]:
Tested on RockPro64 board with RK808 PMIC.

Tested-by: Soeren Moch <smoch@web.de>

Regards,
Soeren