[PATCH 1/2] mfd: da9062: enable being a system-power-controller

[PATCH 1/2] mfd: da9062: enable being a system-power-controller

[Helmut Grohne]

The DA9062 can be the device used to power the CPU. In that case, it can
be used to power off the system. In the CONTROL_A register, the M_*_EN
bits must be zero for the corresponding *_EN bits to have an effect. We
zero them all to turn off the system.

Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
---
 drivers/mfd/da9062-core.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

This patch includes the functionality into the da9062-core driver. If
that is not the preferred way to integrate it, it can be added as a
mfd_cell instead. In that case, I can move the functionality to a new
drivers/power/reset/da9062-poweroff.c. As far as I can see, doing so
implies that we can no longer use the standard system-power-controller
property though and must use a new compatible property
dlg,da9062-poweroff. Please let me know your preference.

diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
index e69626867c26..a2b5dfee677f 100644
--- a/drivers/mfd/da9062-core.c
+++ b/drivers/mfd/da9062-core.c
@@ -572,6 +572,23 @@ static const struct of_device_id da9062_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, da9062_dt_ids);
 
+/* Hold client since pm_power_off is global. */
+static struct i2c_client *da9062_i2c_client;
+
+static void da9062_power_off(void)
+{
+	struct da9062 *chip = i2c_get_clientdata(da9062_i2c_client);
+	const unsigned int mask = DA9062AA_SYSTEM_EN_MASK |
+		DA9062AA_POWER_EN_MASK | DA9062AA_POWER1_EN_MASK |
+		DA9062AA_M_SYSTEM_EN_MASK | DA9062AA_M_POWER_EN_MASK |
+		DA9062AA_M_POWER1_EN_MASK;
+	int ret = regmap_update_bits(chip->regmap, DA9062AA_CONTROL_A, mask, 0);
+
+	if (ret < 0)
+		dev_err(&da9062_i2c_client->dev,
+			"DA9062AA_CONTROL_A update failed, %d\n", ret);
+}
+
 static int da9062_i2c_probe(struct i2c_client *i2c,
 	const struct i2c_device_id *id)
 {
@@ -661,6 +678,15 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
+		if (!pm_power_off) {
+			da9062_i2c_client = i2c;
+			pm_power_off = da9062_power_off;
+		} else {
+			dev_warn(&i2c->dev, "Poweroff callback already assigned\n");
+		}
+	}
+
 	return ret;
 }
 
@@ -668,6 +694,11 @@ static int da9062_i2c_remove(struct i2c_client *i2c)
 {
 	struct da9062 *chip = i2c_get_clientdata(i2c);
 
+	if (pm_power_off == da9062_power_off)
+		pm_power_off = NULL;
+	if (da9062_i2c_client)
+		da9062_i2c_client = NULL;
+
 	mfd_remove_devices(chip->dev);
 	regmap_del_irq_chip(i2c->irq, chip->regmap_irq);
 
-- 
2.20.1

RE: [PATCH 1/2] mfd: da9062: enable being a system-power-controller

[Adam Thomson]

On 07 January 2020 12:06, Helmut Grohne wrote:

> The DA9062 can be the device used to power the CPU. In that case, it can
> be used to power off the system. In the CONTROL_A register, the M_*_EN
> bits must be zero for the corresponding *_EN bits to have an effect. We
> zero them all to turn off the system.
> 
> Signed-off-by: Helmut Grohne <helmut.grohne@intenta.de>
> ---
>  drivers/mfd/da9062-core.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> This patch includes the functionality into the da9062-core driver. If
> that is not the preferred way to integrate it, it can be added as a
> mfd_cell instead. In that case, I can move the functionality to a new
> drivers/power/reset/da9062-poweroff.c. As far as I can see, doing so
> implies that we can no longer use the standard system-power-controller
> property though and must use a new compatible property
> dlg,da9062-poweroff. Please let me know your preference.

I have concerns about using regmap/I2C within the pm_power_off() callback
function although I am aware there are other examples of this in the kernel. At
the point that is called I believe IRQs are disabled so it would require a
platform to have an atomic version of the I2C bus's xfer function. Don't know
if there's a check to see if the bus supports this, but if not then maybe it's
something worth adding? That way we can then only support the pm_power_off()
approach on systems which can actually do it.

> 
> diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> index e69626867c26..a2b5dfee677f 100644
> --- a/drivers/mfd/da9062-core.c
> +++ b/drivers/mfd/da9062-core.c
> @@ -572,6 +572,23 @@ static const struct of_device_id da9062_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, da9062_dt_ids);
> 
> +/* Hold client since pm_power_off is global. */
> +static struct i2c_client *da9062_i2c_client;
> +
> +static void da9062_power_off(void)
> +{
> +	struct da9062 *chip = i2c_get_clientdata(da9062_i2c_client);
> +	const unsigned int mask = DA9062AA_SYSTEM_EN_MASK |
> +		DA9062AA_POWER_EN_MASK | DA9062AA_POWER1_EN_MASK
> |
> +		DA9062AA_M_SYSTEM_EN_MASK |
> DA9062AA_M_POWER_EN_MASK |
> +		DA9062AA_M_POWER1_EN_MASK;
> +	int ret = regmap_update_bits(chip->regmap, DA9062AA_CONTROL_A,
> mask, 0);
> +
> +	if (ret < 0)
> +		dev_err(&da9062_i2c_client->dev,
> +			"DA9062AA_CONTROL_A update failed, %d\n", ret);
> +}
> +
>  static int da9062_i2c_probe(struct i2c_client *i2c,
>  	const struct i2c_device_id *id)
>  {
> @@ -661,6 +678,15 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
>  		return ret;
>  	}
> 
> +	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
> +		if (!pm_power_off) {
> +			da9062_i2c_client = i2c;
> +			pm_power_off = da9062_power_off;
> +		} else {
> +			dev_warn(&i2c->dev, "Poweroff callback already
> assigned\n");
> +		}
> +	}
> +
>  	return ret;
>  }
> 
> @@ -668,6 +694,11 @@ static int da9062_i2c_remove(struct i2c_client *i2c)
>  {
>  	struct da9062 *chip = i2c_get_clientdata(i2c);
> 
> +	if (pm_power_off == da9062_power_off)
> +		pm_power_off = NULL;
> +	if (da9062_i2c_client)
> +		da9062_i2c_client = NULL;
> +
>  	mfd_remove_devices(chip->dev);
>  	regmap_del_irq_chip(i2c->irq, chip->regmap_irq);
> 
> --
> 2.20.1

Re: [PATCH 1/2] mfd: da9062: enable being a system-power-controller

[Helmut Grohne]

Hi,

Thank you for reviewing the code.

On Thu, Jan 23, 2020 at 04:51:37PM +0100, Adam Thomson wrote:
> I have concerns about using regmap/I2C within the pm_power_off() callback
> function although I am aware there are other examples of this in the kernel. At
> the point that is called I believe IRQs are disabled so it would require a
> platform to have an atomic version of the I2C bus's xfer function. Don't know
> if there's a check to see if the bus supports this, but if not then maybe it's
> something worth adding? That way we can then only support the pm_power_off()
> approach on systems which can actually do it.

On arm, machine_power_off calls the pm_power_off callback after issuing
local_irq_disable() and smp_send_stop(). So I think your intuition is
correct that we are running with only one CPU left with IRQs disabled.

I have tested this code on a board with an i2c-cadence bus. This driver
seems to use IRQs for completion tracking with no fallback to polling.
I'm now puzzled as to why this works at all. Given that I'm using
regmap_update_bits on a volatile register, it would have to complete the
read before performing the relevant write. Nevertheless, it reliably
turns off here. So I'm starting to wonder whether there is a flaw in the
analysis.

I also looked into whether linux/i2c.h would tell us about the
availability of an atomic xfer function. Indeed, the i2c_algorithm
structure has a master_xfer_atomic specifically for this purpose. The
i2c core will automatically use this function when irqs are disabled.
Unfortunately, very few buses implement this function. In particular,
i2c-cadence lacks it.

So I could check for i2c_dev->adapter->algo->master_xfer_atomic != NULL
indeed. Possibly this could be wrapped in a central inline function.

I concur that quite a few other drivers use a regmap/i2c from their
pm_power_off hook. Examples include:
 * arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c (i2c without regmap)
 * drivers/mfd/axp20x.c (regmap without i2c)
 * drivers/mfd/dm355evm_msp.c (i2c without regmap)
 * drivers/mfd/max77620.c (regmap and i2c)
 * drivers/mfd/max8907.c (regmap and i2c)
 * drivers/mfd/palmas.c (regmap and i2c)
 * drivers/mfd/retu-mfd.c (regmap and i2c)
 * drivers/mfd/rn5t618.c (regmap and i2c)
 * drivers/mfd/tps6586x.c (regmap and i2c)
 * drivers/mfd/tps65910.c (regmap and i2c)
 * drivers/mfd/tps80031.c (regmap and i2c)
 * drivers/mfd/twl4030-power.c (i2c without regmap)
 * drivers/regulator/act8865-regulator.c (regmap and i2c)

For this reason, I think the practice of using regmap/i2c within
pm_power_off is well-established and should not be questioned for an
individual device. In addition, the relevant functionality must be
explicitly requested by modifying a board-specific device-tree. It can
be assumed that an integrator would test whether the mfd actually works
as a power controller when adding the relevant property. Given that we
turned off other CPUs and IRQs, the behaviour should be fairly reliable.

I think that requiring atomic transfers for pm_power_off would be
relatively easy to implement (for all mfds). However, I also think that
it would break a fair number of boards, because so few buses implement
atomic transfers. As such, I don't think we can actually require it
before requiring all buses to implement atomic transfers. At that point,
the check becomes useless, because the i2c core will automatically use
atomic transfers during pm_power_off.

Given these reasons (consistency with other drivers, testing and "don't
break"), I think that including the functionality without an additional
check is a reasonable thing to do.

Helmut

RE: [PATCH 1/2] mfd: da9062: enable being a system-power-controller

[Adam Thomson]

On 24 January 2020 08:53, Helmut Grohne wrote:

> Hi,
> 
> Thank you for reviewing the code.
> 
> On Thu, Jan 23, 2020 at 04:51:37PM +0100, Adam Thomson wrote:
> > I have concerns about using regmap/I2C within the pm_power_off() callback
> > function although I am aware there are other examples of this in the kernel. At
> > the point that is called I believe IRQs are disabled so it would require a
> > platform to have an atomic version of the I2C bus's xfer function. Don't know
> > if there's a check to see if the bus supports this, but if not then maybe it's
> > something worth adding? That way we can then only support the
> pm_power_off()
> > approach on systems which can actually do it.
> 
> On arm, machine_power_off calls the pm_power_off callback after issuing
> local_irq_disable() and smp_send_stop(). So I think your intuition is
> correct that we are running with only one CPU left with IRQs disabled.
> 
> I have tested this code on a board with an i2c-cadence bus. This driver
> seems to use IRQs for completion tracking with no fallback to polling.
> I'm now puzzled as to why this works at all. Given that I'm using
> regmap_update_bits on a volatile register, it would have to complete the
> read before performing the relevant write. Nevertheless, it reliably
> turns off here. So I'm starting to wonder whether there is a flaw in the
> analysis.
> 
> I also looked into whether linux/i2c.h would tell us about the
> availability of an atomic xfer function. Indeed, the i2c_algorithm
> structure has a master_xfer_atomic specifically for this purpose. The
> i2c core will automatically use this function when irqs are disabled.
> Unfortunately, very few buses implement this function. In particular,
> i2c-cadence lacks it.
> 
> So I could check for i2c_dev->adapter->algo->master_xfer_atomic != NULL
> indeed. Possibly this could be wrapped in a central inline function.

Yes, I'd be tempted to make this a nice wrapper function to hide the
particulars, were someone to implement this.

> 
> I concur that quite a few other drivers use a regmap/i2c from their
> pm_power_off hook. Examples include:
>  * arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c (i2c without regmap)
>  * drivers/mfd/axp20x.c (regmap without i2c)
>  * drivers/mfd/dm355evm_msp.c (i2c without regmap)
>  * drivers/mfd/max77620.c (regmap and i2c)
>  * drivers/mfd/max8907.c (regmap and i2c)
>  * drivers/mfd/palmas.c (regmap and i2c)
>  * drivers/mfd/retu-mfd.c (regmap and i2c)
>  * drivers/mfd/rn5t618.c (regmap and i2c)
>  * drivers/mfd/tps6586x.c (regmap and i2c)
>  * drivers/mfd/tps65910.c (regmap and i2c)
>  * drivers/mfd/tps80031.c (regmap and i2c)
>  * drivers/mfd/twl4030-power.c (i2c without regmap)
>  * drivers/regulator/act8865-regulator.c (regmap and i2c)
> 
> For this reason, I think the practice of using regmap/i2c within
> pm_power_off is well-established and should not be questioned for an
> individual device. In addition, the relevant functionality must be
> explicitly requested by modifying a board-specific device-tree. It can
> be assumed that an integrator would test whether the mfd actually works
> as a power controller when adding the relevant property. Given that we
> turned off other CPUs and IRQs, the behaviour should be fairly reliable.

I never like assumptions and they tend to catch people out. A lot of the time
driver developers will use another as a template/example and so the same
possible mistakes can be duplicated. I don't know for certain these are mistakes
but the code seems to indicate that could be the case, and there's a good
reason that atomic I2C transfer code has been added into the kernel. I also
prefer code that helps people identify where a problem might lie so having a
check for I2C atomic support would be useful to indicate if there is a problem.

Lee, do you have any further insight into any of this? Am I barking up the
wrong tree here?

> 
> I think that requiring atomic transfers for pm_power_off would be
> relatively easy to implement (for all mfds). However, I also think that
> it would break a fair number of boards, because so few buses implement
> atomic transfers. As such, I don't think we can actually require it
> before requiring all buses to implement atomic transfers. At that point,
> the check becomes useless, because the i2c core will automatically use
> atomic transfers during pm_power_off.
> 
> Given these reasons (consistency with other drivers, testing and "don't
> break"), I think that including the functionality without an additional
> check is a reasonable thing to do.
> 
> Helmut

Re: [PATCH 1/2] mfd: da9062: enable being a system-power-controller

[Lee Jones]

On Fri, 24 Jan 2020, Adam Thomson wrote:
> On 24 January 2020 08:53, Helmut Grohne wrote:
> > On Thu, Jan 23, 2020 at 04:51:37PM +0100, Adam Thomson wrote:
> > > I have concerns about using regmap/I2C within the pm_power_off() callback
> > > function although I am aware there are other examples of this in the kernel. At
> > > the point that is called I believe IRQs are disabled so it would require a
> > > platform to have an atomic version of the I2C bus's xfer function. Don't know
> > > if there's a check to see if the bus supports this, but if not then maybe it's
> > > something worth adding? That way we can then only support the
> > pm_power_off()
> > > approach on systems which can actually do it.
> > 
> > On arm, machine_power_off calls the pm_power_off callback after issuing
> > local_irq_disable() and smp_send_stop(). So I think your intuition is
> > correct that we are running with only one CPU left with IRQs disabled.
> > 
> > I have tested this code on a board with an i2c-cadence bus. This driver
> > seems to use IRQs for completion tracking with no fallback to polling.
> > I'm now puzzled as to why this works at all. Given that I'm using
> > regmap_update_bits on a volatile register, it would have to complete the
> > read before performing the relevant write. Nevertheless, it reliably
> > turns off here. So I'm starting to wonder whether there is a flaw in the
> > analysis.
> > 
> > I also looked into whether linux/i2c.h would tell us about the
> > availability of an atomic xfer function. Indeed, the i2c_algorithm
> > structure has a master_xfer_atomic specifically for this purpose. The
> > i2c core will automatically use this function when irqs are disabled.
> > Unfortunately, very few buses implement this function. In particular,
> > i2c-cadence lacks it.
> > 
> > So I could check for i2c_dev->adapter->algo->master_xfer_atomic != NULL
> > indeed. Possibly this could be wrapped in a central inline function.
> 
> Yes, I'd be tempted to make this a nice wrapper function to hide the
> particulars, were someone to implement this.
> 
> > 
> > I concur that quite a few other drivers use a regmap/i2c from their
> > pm_power_off hook. Examples include:
> >  * arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c (i2c without regmap)
> >  * drivers/mfd/axp20x.c (regmap without i2c)
> >  * drivers/mfd/dm355evm_msp.c (i2c without regmap)
> >  * drivers/mfd/max77620.c (regmap and i2c)
> >  * drivers/mfd/max8907.c (regmap and i2c)
> >  * drivers/mfd/palmas.c (regmap and i2c)
> >  * drivers/mfd/retu-mfd.c (regmap and i2c)
> >  * drivers/mfd/rn5t618.c (regmap and i2c)
> >  * drivers/mfd/tps6586x.c (regmap and i2c)
> >  * drivers/mfd/tps65910.c (regmap and i2c)
> >  * drivers/mfd/tps80031.c (regmap and i2c)
> >  * drivers/mfd/twl4030-power.c (i2c without regmap)
> >  * drivers/regulator/act8865-regulator.c (regmap and i2c)
> > 
> > For this reason, I think the practice of using regmap/i2c within
> > pm_power_off is well-established and should not be questioned for an
> > individual device. In addition, the relevant functionality must be
> > explicitly requested by modifying a board-specific device-tree. It can
> > be assumed that an integrator would test whether the mfd actually works
> > as a power controller when adding the relevant property. Given that we
> > turned off other CPUs and IRQs, the behaviour should be fairly reliable.
> 
> I never like assumptions and they tend to catch people out. A lot of the time
> driver developers will use another as a template/example and so the same
> possible mistakes can be duplicated. I don't know for certain these are mistakes
> but the code seems to indicate that could be the case, and there's a good
> reason that atomic I2C transfer code has been added into the kernel. I also
> prefer code that helps people identify where a problem might lie so having a
> check for I2C atomic support would be useful to indicate if there is a problem.
> 
> Lee, do you have any further insight into any of this? Am I barking up the
> wrong tree here?

Not off-hand, sorry.  I would have to do a deep-dive to figure it
out for myself.

Maybe this is where Mark and/or Wolfram (Cc'ed) have some knowledge.

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