[PATCH v2 0/2] regulator_sync_state() support

[PATCH v2 0/2] regulator_sync_state() support

[Saravana Kannan]

Poorly worded subject, but patch 1/2 should give more details.

The simplified explanation of the problem is, for regulators left on by
the bootloader, we want to keep them on until all the consumers are
probed. This is because we need to protect consumer-A from turning off a
shared regulator used by consumer-B. Once consumer-B (and all the other
consumers come up), they can do it themselves and the regulator
framework no longer needs to keep the regulator on.

So, this is not just about module or device probe ordering between
suppliers and consumers. Even if we get the probe order prefectly right,
it still won't solve this problem.

We can eventually extend this to also cover voltage and other
properties, but in this patch series I want to get this right for
"enabled/disabled" first.

This patch series also has the additional benefit of turning off unused
regulators that are probed after the 30s timeout that's there today.

v1->v2:
Patch 1/2
- New patch that might get dropped based on review.

Patch 2/2
- This was the only patch in v1
- Made the late_initcall_sync timeout a commandline param
- If timeout is set, we also give up waiting for all consumers after
  the timeout expires.
- Made every regulator driver add sync_state() support

Saravana Kannan (2):
  driver core: Add dev_set_drv_sync_state()
  regulator: Add support for sync_state() callbacks

 drivers/regulator/core.c         | 112 +++++++++++++++++++++++++++++++
 include/linux/device.h           |  12 ++++
 include/linux/regulator/driver.h |   2 +
 3 files changed, 126 insertions(+)

-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH v2 1/2] driver core: Add dev_set_drv_sync_state()

[Saravana Kannan]

This can be used by frameworks to set the sync_state() helper functions
for drivers that don't already have them set.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---

I'm hoping I can drop this patch based on discussions in patch 1/2.

 include/linux/device.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index ac8e37cd716a..aac548beb154 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -806,6 +806,18 @@ static inline bool dev_has_sync_state(struct device *dev)
 	return false;
 }
 
+static inline int dev_set_drv_sync_state(struct device *dev,
+					 void (*fn)(struct device *dev))
+{
+	if (!dev || !dev->driver)
+		return 0;
+	if (dev->driver->sync_state && dev->driver->sync_state != fn)
+		return -EBUSY;
+	if (!dev->driver->sync_state)
+		dev->driver->sync_state = fn;
+	return 0;
+}
+
 /*
  * High level routines for use by the bus drivers
  */
-- 
2.27.0.rc0.183.gde8f92d652-goog

[PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Saravana Kannan]

When a regulator is left on by the bootloader or anything else before
the kernel starts (let's call this a "boot on" regulator), we need to
keep it on till all the consumers of the regulator have probed. This is
especially important for regulators that might be powering more than one
consumer. Otherwise, when the first consumer probes, enables and then
disables the "boot on" regulator, it'd turn off the power to rest of the
consumers of the "boot on" regulator.

Also, if a regulator driver is loaded as a module and the regulator
device has a few "boot on" regulators, we still want them to be turned
off after all the consumers have probed (if none of them have requested
for those regulators to stay on).

The sync_state() callback that's been added to drivers is meant for
situations like this. It gets called when all the consumers of a device
have probed successfully. To ease the transition to sync_state()
callbacks, it is never called before late_initcall_sync().

sync_state() callbacks become even more useful when combined with
fw_devlink.  If fw_devlink is off, sync_state() is called at
late_initcall_sync() or the regulator device probing successfully --
whichever is later. This is because, with fw_devlink off, there would be
no consumers to the regulator device when it probes.

If fw_devlink is not off (it's permissive by default now), then
sync_state() is called when all the consumers of the regulator device
have probed or at late_initcall_sync() -- whichever is later.

This commit adds a regulator_sync_state() helper function that takes
care of all the "boot on" regulator clean up for any regulator driver.
All one needs to do is add the following line to the driver struct.

.sync_state = regulator_sync_state,

Once that's done, then for any device that's probed by the driver, all
the "boot on" regulators supplied by the device will be kept on till all
the consumers of the device have probed. Once the consumers have probed,
the "boot on" regulators would be allowed to turn off if they are voted
on by any of the consumers.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/regulator/core.c         | 112 +++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |   2 +
 2 files changed, 114 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 941783a14b45..913d387541d6 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1768,6 +1768,88 @@ static struct regulator_dev *regulator_dev_lookup(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
+static void regulator_set_minimum_state(struct regulator_dev *rdev)
+{
+	struct device *dev = rdev->dev.parent;
+	struct regulation_constraints *c = rdev->constraints;
+
+	/*
+	 * The device doesn't have sync_state() support, or we already took
+	 * care of it.
+	 */
+	if (!dev_has_sync_state(dev) || rdev->sync_supply)
+		return;
+
+	/*
+	 * Wait for parent supply to be ready before trying to keep this
+	 * regulator on.
+	 */
+	if (rdev->supply_name && !rdev->supply)
+		return;
+
+	if (c && c->always_on)
+		return;
+
+	if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
+		return;
+
+	/*
+	 * If the regulator is disabled or has an error getting the status,
+	 * assume it's not on. We won't explicitly turn it off now, that can
+	 * happen later.
+	 */
+	if (_regulator_is_enabled(rdev) <= 0)
+		return;
+
+	/*
+	 * Get a regulator client handle to make requests on the regulator in
+	 * way that'll prevent it from going below the state in which the boot
+	 * loader left it in.
+	 */
+	rdev->sync_supply = create_regulator(rdev, NULL, "SYNC-SUPPLY");
+
+	/*
+	 * At this point, we shouldn't have trouble getting a regulator. If we
+	 * do, give up.
+	 */
+	if (rdev->sync_supply == NULL) {
+		dev_err(&rdev->dev, "Unable to make votes on the regulator\n");
+		rdev->sync_supply = ERR_PTR(-EINVAL);
+		return;
+	}
+	rdev->open_count++;
+
+	/* If we can't put a vote to keep the regulator enabled, we give up. */
+	if (regulator_enable(rdev->sync_supply)) {
+		dev_err(&rdev->dev, "Unable to keep the regulator on\n");
+		regulator_put(rdev->sync_supply);
+		rdev->sync_supply = ERR_PTR(-EINVAL);
+	}
+}
+
+static int regulator_rel_minimum_state(struct device *dev, void *data)
+{
+	struct regulator_dev *rdev = dev_to_rdev(dev);
+
+	if (dev->parent != data)
+		return 0;
+
+	if (IS_ERR_OR_NULL(rdev->sync_supply))
+		return 0;
+
+	regulator_disable(rdev->sync_supply);
+	regulator_put(rdev->sync_supply);
+	rdev->sync_supply = NULL;
+	return 0;
+}
+
+void regulator_sync_state(struct device *dev)
+{
+	class_for_each_device(&regulator_class, NULL, dev,
+			      regulator_rel_minimum_state);
+}
+EXPORT_SYMBOL_GPL(regulator_sync_state);
+
 static int regulator_resolve_supply(struct regulator_dev *rdev)
 {
 	struct regulator_dev *r;
@@ -1826,6 +1908,8 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		return ret;
 	}
 
+	regulator_set_minimum_state(rdev);
+
 	/*
 	 * In set_machine_constraints() we may have turned this regulator on
 	 * but we couldn't propagate to the supply if it hadn't been resolved
@@ -5188,6 +5272,15 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	    !rdev->desc->fixed_uV)
 		rdev->is_switch = true;
 
+	/*
+	 * I'm not too enthusiastic about this. I'd rather just go set
+	 * .sync_state() in all the regulator drivers. But let's get the rest
+	 * of the patch sorted out first.
+	 */
+	if (dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state))
+		dev_dbg(&rdev->dev, "parent sync_state() already set\n");
+	regulator_set_minimum_state(rdev);
+
 	dev_set_drvdata(&rdev->dev, rdev);
 	ret = device_register(&rdev->dev);
 	if (ret != 0) {
@@ -5729,6 +5822,15 @@ static int regulator_late_cleanup(struct device *dev, void *data)
 	return 0;
 }
 
+static int regulator_minimum_state_cleanup(struct device *dev, void *data)
+{
+	regulator_rel_minimum_state(dev, dev->parent);
+	return 0;
+}
+
+static unsigned int regulator_cleanup_timeout = 30000;
+core_param(regulator_cleanup_timeout, regulator_cleanup_timeout, uint, 0);
+
 static void regulator_init_complete_work_function(struct work_struct *work)
 {
 	/*
@@ -5741,6 +5843,16 @@ static void regulator_init_complete_work_function(struct work_struct *work)
 	class_for_each_device(&regulator_class, NULL, NULL,
 			      regulator_register_resolve_supply);
 
+	/*
+	 * If regulator_cleanup_timeout is set to a non-zero value, it probably
+	 * means some of the consumers will never probe or the regulators have
+	 * some restrictions on how long they can stay ON. So, don't wait
+	 * forever for consumer devices to probe.
+	 */
+	if (regulator_cleanup_timeout)
+		class_for_each_device(&regulator_class, NULL, NULL,
+				      regulator_minimum_state_cleanup);
+
 	/* If we have a full configuration then disable any regulators
 	 * we have permission to change the status for and which are
 	 * not in use or always_on.  This is effectively the default
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 7eb9fea8e482..2e2208e7b771 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -451,6 +451,7 @@ struct regulator_dev {
 	struct regulator *supply;	/* for tree */
 	const char *supply_name;
 	struct regmap *regmap;
+	struct regulator *sync_supply;
 
 	struct delayed_work disable_work;
 
@@ -475,6 +476,7 @@ devm_regulator_register(struct device *dev,
 			const struct regulator_desc *regulator_desc,
 			const struct regulator_config *config);
 void regulator_unregister(struct regulator_dev *rdev);
+void regulator_sync_state(struct device *dev);
 void devm_regulator_unregister(struct device *dev, struct regulator_dev *rdev);
 
 int regulator_notifier_call_chain(struct regulator_dev *rdev,
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Mark Brown]

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

On Thu, May 28, 2020 at 12:06:10PM -0700, Saravana Kannan wrote:
> When a regulator is left on by the bootloader or anything else before
> the kernel starts (let's call this a "boot on" regulator), we need to
> keep it on till all the consumers of the regulator have probed. This is

We we don't in general have a requirement that any of the consumers of a
regulator will ever probe.  As I thought I'd already made clear that
case really needs to be handled.

> especially important for regulators that might be powering more than one
> consumer. Otherwise, when the first consumer probes, enables and then
> disables the "boot on" regulator, it'd turn off the power to rest of the
> consumers of the "boot on" regulator.

Which is a problem because...?

> The sync_state() callback that's been added to drivers is meant for
> situations like this. It gets called when all the consumers of a device
> have probed successfully. To ease the transition to sync_state()
> callbacks, it is never called before late_initcall_sync().

This is not terribly useful for regulators where adding any of these
delays is going to create surprises.  Frankly I can't really see why
deferring things until late_initcall() would help anything.

> sync_state() callbacks become even more useful when combined with
> fw_devlink.  If fw_devlink is off, sync_state() is called at
> late_initcall_sync() or the regulator device probing successfully --
> whichever is later. This is because, with fw_devlink off, there would be
> no consumers to the regulator device when it probes.

This breaks the case where no driver ever instantiates for a device.

Oh, actually now I get to the very end of the patch I see there is
actually a timeout in here which wasn't mentioned in the changelog at
all.  I very nearly didn't read the actual code as according to the
changelog this issue hadn't been addressed.

> This commit adds a regulator_sync_state() helper function that takes
> care of all the "boot on" regulator clean up for any regulator driver.
> All one needs to do is add the following line to the driver struct.
> 
> .sync_state = regulator_sync_state,

Exactly the same issues as before apply here, why are devices getting
involved here?

> +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> +{

I find this name very confusing.  If anything it's doing the opposite of
setting a minimum state, it's trying to prevent that happening.

> +	/*
> +	 * Wait for parent supply to be ready before trying to keep this
> +	 * regulator on.
> +	 */
> +	if (rdev->supply_name && !rdev->supply)
> +		return;

I can't make sense of this.  This stuff only limits disabling, not
enabling, regulators, we're keeping things on here anyway and why would
we care about the supply for disabling?

> +static int regulator_rel_minimum_state(struct device *dev, void *data)
> +{
> +	struct regulator_dev *rdev = dev_to_rdev(dev);
> +
> +	if (dev->parent != data)
> +		return 0;

I've just realised that this is even more restrictive than the
descriptions have suggested - it's not just preventing any changes until
all potential consumers of a given regulator have instantiated, it's
preventing changes until all potential consumers of all resources
provided by a given device have instantiated.  Given that many systems
have a single PMIC which may also be providing other things like GPIOs
that would mean that any consumer that doesn't instantiate would prevent
any device getting turned off which seems even more concerning.

> +	if (IS_ERR_OR_NULL(rdev->sync_supply))
> +		return 0;
> +
> +	regulator_disable(rdev->sync_supply);

I think sync_supply needs a better name, it's a consumer rather than a
supply any sync doesn't really fit with what it's doing either - getting
rid of it might be syncing but it's not syncing anything.

The other thing here is that we're doing this silently which is going to
make problems harder to debug, if something gets turned off that wasn't
supposed to be turned off and the system collapses users are going to
have fun working out what happened - this is why the current code to
tidy up idle regulators prints something before it disables things.

> @@ -5188,6 +5272,15 @@ regulator_register(const struct regulator_desc *regulator_desc,
>  	    !rdev->desc->fixed_uV)
>  		rdev->is_switch = true;
>  
> +	/*
> +	 * I'm not too enthusiastic about this. I'd rather just go set
> +	 * .sync_state() in all the regulator drivers. But let's get the rest
> +	 * of the patch sorted out first.
> +	 */
> +	if (dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state))
> +		dev_dbg(&rdev->dev, "parent sync_state() already set\n");
> +	regulator_set_minimum_state(rdev);

If you don't want this to be configurable per driver then why do you
want to force it to be configured per driver?  I also think that is a
bad idea, it's just make work as far as I can see.

> +	/*
> +	 * If regulator_cleanup_timeout is set to a non-zero value, it probably
> +	 * means some of the consumers will never probe or the regulators have
> +	 * some restrictions on how long they can stay ON. So, don't wait
> +	 * forever for consumer devices to probe.
> +	 */
> +	if (regulator_cleanup_timeout)
> +		class_for_each_device(&regulator_class, NULL, NULL,
> +				      regulator_minimum_state_cleanup);
> +

This is not a timeout, it is a boolean flag - nothing ever actually sets
a delay or timeout based on this number.  I really think this should
just be a separate patch adding configurability for the existing delay
before finalization not something that randomly only affects this new
magic stuff as that makes things more complex and confusing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Saravana Kannan]

On Fri, May 29, 2020 at 6:00 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, May 28, 2020 at 12:06:10PM -0700, Saravana Kannan wrote:
> > When a regulator is left on by the bootloader or anything else before
> > the kernel starts (let's call this a "boot on" regulator), we need to
> > keep it on till all the consumers of the regulator have probed. This is
>
> We we don't in general have a requirement that any of the consumers of a
> regulator will ever probe.  As I thought I'd already made clear that
> case really needs to be handled.

Sorry, in the eagerness to address all your concerns, I had forgotten
to update the commit text in v2. I'll reword it to clarify this'll
happen only when there's no timeout set.

> > especially important for regulators that might be powering more than one
> > consumer. Otherwise, when the first consumer probes, enables and then
> > disables the "boot on" regulator, it'd turn off the power to rest of the
> > consumers of the "boot on" regulator.
>
> Which is a problem because...?

Those consumers that haven't probed can be powered on and active and
can crash the system if the power is pulled under their feet.

> > The sync_state() callback that's been added to drivers is meant for
> > situations like this. It gets called when all the consumers of a device
> > have probed successfully. To ease the transition to sync_state()
> > callbacks, it is never called before late_initcall_sync().
>
> This is not terribly useful for regulators where adding any of these
> delays is going to create surprises.  Frankly I can't really see why
> deferring things until late_initcall() would help anything.

Is this different from what's done already? Not having this delay is
easy -- but will have to be at a global level across
devices/resources.

> > sync_state() callbacks become even more useful when combined with
> > fw_devlink.  If fw_devlink is off, sync_state() is called at
> > late_initcall_sync() or the regulator device probing successfully --
> > whichever is later. This is because, with fw_devlink off, there would be
> > no consumers to the regulator device when it probes.
>
> This breaks the case where no driver ever instantiates for a device.
>
> Oh, actually now I get to the very end of the patch I see there is
> actually a timeout in here which wasn't mentioned in the changelog at
> all.  I very nearly didn't read the actual code as according to the
> changelog this issue hadn't been addressed.

Again, sorry I didn't update the commit text. I'll skip replying to
rest of your comments on the commit text, because all of them are due
to stale commit text.

> > This commit adds a regulator_sync_state() helper function that takes
> > care of all the "boot on" regulator clean up for any regulator driver.
> > All one needs to do is add the following line to the driver struct.
> >
> > .sync_state = regulator_sync_state,
>
> Exactly the same issues as before apply here, why are devices getting
> involved here?
>
> > +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> > +{
>
> I find this name very confusing.  If anything it's doing the opposite of
> setting a minimum state, it's trying to prevent that happening.

I agree. I renamed it to try and make it better, but I can see how
it's more confusing once you called it out. Suggestions?

regulator_set_boot_limits? regulator_set_boot_constraints?

> > +     /*
> > +      * Wait for parent supply to be ready before trying to keep this
> > +      * regulator on.
> > +      */
> > +     if (rdev->supply_name && !rdev->supply)
> > +             return;
>
> I can't make sense of this.  This stuff only limits disabling, not
> enabling, regulators, we're keeping things on here anyway and why would
> we care about the supply for disabling?

We want to wait till supply is set up before we try to put the
"enable" vote on a regulator. Otherwise, it won't be propagated
properly? I do know that regulator_resolve_supply() takes care of
propagating the use count, but we could delete that code if we just
till the supply is resolved before "enabling" the regulator. The usual
clients can't "get" the regulator anyway without the supply being
resolved. Long story short, doing it this way can allow us to delete
some functionally duplicative code.

> > +static int regulator_rel_minimum_state(struct device *dev, void *data)
> > +{
> > +     struct regulator_dev *rdev = dev_to_rdev(dev);
> > +
> > +     if (dev->parent != data)
> > +             return 0;
>
> I've just realised that this is even more restrictive than the
> descriptions have suggested - it's not just preventing any changes until
> all potential consumers of a given regulator have instantiated, it's
> preventing changes until all potential consumers of all resources
> provided by a given device have instantiated.  Given that many systems
> have a single PMIC which may also be providing other things like GPIOs
> that would mean that any consumer that doesn't instantiate would prevent
> any device getting turned off which seems even more concerning.

Your understanding is correct. I'll try to clarify the commit text as
best as I can. Your concerns are why this is not the default behavior.
For Android phones that are actually shipped, ensuring every enabled
consumer actually probes is trivial and desired.

> > +     if (IS_ERR_OR_NULL(rdev->sync_supply))
> > +             return 0;
> > +
> > +     regulator_disable(rdev->sync_supply);
>
> I think sync_supply needs a better name, it's a consumer rather than a
> supply any sync doesn't really fit with what it's doing either - getting
> rid of it might be syncing but it's not syncing anything.

Agree about the naming. Maybe boot_limits?

> The other thing here is that we're doing this silently which is going to
> make problems harder to debug, if something gets turned off that wasn't
> supposed to be turned off and the system collapses users are going to
> have fun working out what happened - this is why the current code to
> tidy up idle regulators prints something before it disables things.

Agreed. I'll add a log similar to the existing clean up path.

> > @@ -5188,6 +5272,15 @@ regulator_register(const struct regulator_desc *regulator_desc,
> >           !rdev->desc->fixed_uV)
> >               rdev->is_switch = true;
> >
> > +     /*
> > +      * I'm not too enthusiastic about this. I'd rather just go set
> > +      * .sync_state() in all the regulator drivers. But let's get the rest
> > +      * of the patch sorted out first.
> > +      */
> > +     if (dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state))
> > +             dev_dbg(&rdev->dev, "parent sync_state() already set\n");
> > +     regulator_set_minimum_state(rdev);
>
> If you don't want this to be configurable per driver then why do you
> want to force it to be configured per driver?  I also think that is a
> bad idea, it's just make work as far as I can see.

I think the last line is a typo?

Anyway, I just have the dev_set_drv_sync_state() call here instead of
spending time on a patch that sets .sync_state() for 100s of regulator
drivers before we can actually agree on the bigger picture/idea. Once
the patch becomes acceptable, I'd like to go set this for every
regulator driver and delete the call to dev_set_drv_sync_state().

As to why this is configurable per driver, we discussed this in the v1
thread. sync_state() isn't specific to the regulator framework (which
I'm sure you understand) -- so it needs to be flexible.
Devices/drivers like MFDs, PMICs with GPIOs, power domains with reset
control, etc might want to call into multiple frameworks when they get
their sync state. Some devices/drivers might want to do more stuff
specific to their device/system when they get the sync state callback
-- they can spin their own sync_state() callback, do their stuff and
then call regulator_sync_state() or whatever other framework they
want. That's why it's configurable per device/driver.

> > +     /*
> > +      * If regulator_cleanup_timeout is set to a non-zero value, it probably
> > +      * means some of the consumers will never probe or the regulators have
> > +      * some restrictions on how long they can stay ON. So, don't wait
> > +      * forever for consumer devices to probe.
> > +      */
> > +     if (regulator_cleanup_timeout)
> > +             class_for_each_device(&regulator_class, NULL, NULL,
> > +                                   regulator_minimum_state_cleanup);
> > +
>
> This is not a timeout, it is a boolean flag - nothing ever actually sets
> a delay or timeout based on this number.  I really think this should
> just be a separate patch adding configurability for the existing delay
> before finalization not something that randomly only affects this new
> magic stuff as that makes things more complex and confusing.

Sorry, this is a dumb mistake. I forgot to replace the 30000ms that's
a few lines below with regulator_cleanup_timeout.

I'm also reaching out to vendors/partners to try and gather some
examples. There are plenty of real world examples but the ones I know
are mostly in downstream kernels that are shipped in Android phones.
So trying to find ones in upstream or get vendors to speak up about
how often this is an issue downstream. This is also kind of a catch
22. Vendors hack up their regulator driver and the regulator framework
to get this "boot on" regulator handling working, but they can't
obviously upstream their drivers because the hacks aren't acceptable.
I don't want to debate that can of worms right now -- I'm trying to
find upstream examples first (which I hope isn't hard).

I'll send out v3 addressing your comments once I get a better handle
on the examples. Thanks for the reviews and being open to finding a
solution that works for everyone.

-Saravana

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Mark Brown]

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

On Fri, May 29, 2020 at 07:39:33PM -0700, Saravana Kannan wrote:
> On Fri, May 29, 2020 at 6:00 AM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, May 28, 2020 at 12:06:10PM -0700, Saravana Kannan wrote:

> > > especially important for regulators that might be powering more than one
> > > consumer. Otherwise, when the first consumer probes, enables and then
> > > disables the "boot on" regulator, it'd turn off the power to rest of the
> > > consumers of the "boot on" regulator.

> > Which is a problem because...?

> Those consumers that haven't probed can be powered on and active and
> can crash the system if the power is pulled under their feet.

This is I think the first time anyone has suggested that this is likely
to be an issue - the previous concerns have all been about screens
glitching.

> > > The sync_state() callback that's been added to drivers is meant for
> > > situations like this. It gets called when all the consumers of a device
> > > have probed successfully. To ease the transition to sync_state()
> > > callbacks, it is never called before late_initcall_sync().

> > This is not terribly useful for regulators where adding any of these
> > delays is going to create surprises.  Frankly I can't really see why
> > deferring things until late_initcall() would help anything.

> Is this different from what's done already?

At the minute we implement any requested changes immediately.

>                                              Not having this delay is
> easy -- but will have to be at a global level across
> devices/resources.

That seems fine given that there appears to be no reason to introduce a
delay.

> > > +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> > > +{

> > I find this name very confusing.  If anything it's doing the opposite of
> > setting a minimum state, it's trying to prevent that happening.

> I agree. I renamed it to try and make it better, but I can see how
> it's more confusing once you called it out. Suggestions?

> regulator_set_boot_limits? regulator_set_boot_constraints?

ignore_consumer_requests()?

> > > +     /*
> > > +      * Wait for parent supply to be ready before trying to keep this
> > > +      * regulator on.
> > > +      */
> > > +     if (rdev->supply_name && !rdev->supply)
> > > +             return;

> > I can't make sense of this.  This stuff only limits disabling, not
> > enabling, regulators, we're keeping things on here anyway and why would
> > we care about the supply for disabling?

> We want to wait till supply is set up before we try to put the
> "enable" vote on a regulator. Otherwise, it won't be propagated
> properly? I do know that regulator_resolve_supply() takes care of
> propagating the use count, but we could delete that code if we just
> till the supply is resolved before "enabling" the regulator. The usual
> clients can't "get" the regulator anyway without the supply being
> resolved. Long story short, doing it this way can allow us to delete
> some functionally duplicative code.

So this is to support some future change that you either haven't
written or haven't sent out yet.  Please don't do stuff like this, it
makes everything more confusing.  Send out isolated, coherent changes
that do a single thing per patch, if there's things you're thinking of
for future work then save them for when you actually do that future
work.  That makes everything clearer and easier to follow.

> > I've just realised that this is even more restrictive than the
> > descriptions have suggested - it's not just preventing any changes until
> > all potential consumers of a given regulator have instantiated, it's
> > preventing changes until all potential consumers of all resources
> > provided by a given device have instantiated.  Given that many systems
> > have a single PMIC which may also be providing other things like GPIOs
> > that would mean that any consumer that doesn't instantiate would prevent
> > any device getting turned off which seems even more concerning.

> Your understanding is correct. I'll try to clarify the commit text as
> best as I can. Your concerns are why this is not the default behavior.

My concern is that introducing extra delays is likely to make things
more fragile and complex.  As far as I can see this is just making
things even worse by adding spurious dependencies and delaying things
further.

> For Android phones that are actually shipped, ensuring every enabled
> consumer actually probes is trivial and desired.

Other users exist and need to be considered.  People need to be able to
run upstream kernels, they need to be able to run distros and other
systems that aren't vertically integrated like yours where people aren't
developing the DT and kernel together.

> > > +     if (IS_ERR_OR_NULL(rdev->sync_supply))
> > > +             return 0;
> > > +
> > > +     regulator_disable(rdev->sync_supply);

> > I think sync_supply needs a better name, it's a consumer rather than a
> > supply any sync doesn't really fit with what it's doing either - getting
> > rid of it might be syncing but it's not syncing anything.

> Agree about the naming. Maybe boot_limits?

Possibly.

> > > +     /*
> > > +      * I'm not too enthusiastic about this. I'd rather just go set
> > > +      * .sync_state() in all the regulator drivers. But let's get the rest
> > > +      * of the patch sorted out first.
> > > +      */
> > > +     if (dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state))
> > > +             dev_dbg(&rdev->dev, "parent sync_state() already set\n");
> > > +     regulator_set_minimum_state(rdev);

> > If you don't want this to be configurable per driver then why do you
> > want to force it to be configured per driver?  I also think that is a
> > bad idea, it's just make work as far as I can see.

> I think the last line is a typo?

No, it's not a typo - "make work" is busy work for the sake of it.  I'm
saying that this is just requiring a mechanical update in all drivers
both now and going forwards which is exactly the sort of thing we should
be factoring out of drivers.

> As to why this is configurable per driver, we discussed this in the v1
> thread. sync_state() isn't specific to the regulator framework (which
> I'm sure you understand) -- so it needs to be flexible.
> Devices/drivers like MFDs, PMICs with GPIOs, power domains with reset
> control, etc might want to call into multiple frameworks when they get
> their sync state. Some devices/drivers might want to do more stuff
> specific to their device/system when they get the sync state callback
> -- they can spin their own sync_state() callback, do their stuff and
> then call regulator_sync_state() or whatever other framework they
> want. That's why it's configurable per device/driver.

To repeat what I said at the time a device that's in multiple frameworks
like this is most likely a MFD with a bunch of broadly unrelated things
in it, where things are related we need to track those dependencies
anyway.  At best this is working around a problem that has been
introduced by the decision to do everything at far too broad a level,
it feels like it's defeating the point of trying to track dependencies
at all.  The whole performance seems completely redundant if we're
trying to aggregate things so aggressively, we end up needing pretty
much every device in the system to come up before we can do anything.
Together with the late_initcall() stuff this is sounding like what it's
really doing is going a very long way around introducing a new initcall
that gets triggered after all modules are loaded (which *was* one of the
earlier suggestions).

I really think this would be a lot simpler and easier for all concerned
if it operated per regulator like the changelog originally seemed to
suggest.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Saravana Kannan]

On Mon, Jun 1, 2020 at 10:23 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, May 29, 2020 at 07:39:33PM -0700, Saravana Kannan wrote:
> > On Fri, May 29, 2020 at 6:00 AM Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, May 28, 2020 at 12:06:10PM -0700, Saravana Kannan wrote:
>
> > > > especially important for regulators that might be powering more than one
> > > > consumer. Otherwise, when the first consumer probes, enables and then
> > > > disables the "boot on" regulator, it'd turn off the power to rest of the
> > > > consumers of the "boot on" regulator.
>
> > > Which is a problem because...?
>
> > Those consumers that haven't probed can be powered on and active and
> > can crash the system if the power is pulled under their feet.
>
> This is I think the first time anyone has suggested that this is likely
> to be an issue - the previous concerns have all been about screens
> glitching.

Looks like we got at least one concrete example now in [1].

>
> > > > The sync_state() callback that's been added to drivers is meant for
> > > > situations like this. It gets called when all the consumers of a device
> > > > have probed successfully. To ease the transition to sync_state()
> > > > callbacks, it is never called before late_initcall_sync().
>
> > > This is not terribly useful for regulators where adding any of these
> > > delays is going to create surprises.  Frankly I can't really see why
> > > deferring things until late_initcall() would help anything.
>
> > Is this different from what's done already?
>
> At the minute we implement any requested changes immediately.
>
> >                                              Not having this delay is
> > easy -- but will have to be at a global level across
> > devices/resources.
>
> That seems fine given that there appears to be no reason to introduce a
> delay.
>
> > > > +static void regulator_set_minimum_state(struct regulator_dev *rdev)
> > > > +{
>
> > > I find this name very confusing.  If anything it's doing the opposite of
> > > setting a minimum state, it's trying to prevent that happening.
>
> > I agree. I renamed it to try and make it better, but I can see how
> > it's more confusing once you called it out. Suggestions?
>
> > regulator_set_boot_limits? regulator_set_boot_constraints?
>
> ignore_consumer_requests()?

But it doesn't really ignore consumer requests though. In response to
all your comments above, I haven't done a good job of explaining the
issues and the solution. I'll try to redo that part again when I send
v3 and hopefully I can do better.

> > > > +     /*
> > > > +      * Wait for parent supply to be ready before trying to keep this
> > > > +      * regulator on.
> > > > +      */
> > > > +     if (rdev->supply_name && !rdev->supply)
> > > > +             return;
>
> > > I can't make sense of this.  This stuff only limits disabling, not
> > > enabling, regulators, we're keeping things on here anyway and why would
> > > we care about the supply for disabling?
>
> > We want to wait till supply is set up before we try to put the
> > "enable" vote on a regulator. Otherwise, it won't be propagated
> > properly? I do know that regulator_resolve_supply() takes care of
> > propagating the use count, but we could delete that code if we just
> > till the supply is resolved before "enabling" the regulator. The usual
> > clients can't "get" the regulator anyway without the supply being
> > resolved. Long story short, doing it this way can allow us to delete
> > some functionally duplicative code.
>
> So this is to support some future change that you either haven't
> written or haven't sent out yet.  Please don't do stuff like this, it
> makes everything more confusing.  Send out isolated, coherent changes
> that do a single thing per patch, if there's things you're thinking of
> for future work then save them for when you actually do that future
> work.  That makes everything clearer and easier to follow.

Ok. Thinking more about it, when I try to add voltage handling, I'll
need to call regulator_get_voltage_rdev(). So I might still need to
wait till supply is set up. However, if I don't really need it, I'll
make sure to drop any "future improvements" related changes.

> > > I've just realised that this is even more restrictive than the
> > > descriptions have suggested - it's not just preventing any changes until
> > > all potential consumers of a given regulator have instantiated, it's
> > > preventing changes until all potential consumers of all resources
> > > provided by a given device have instantiated.  Given that many systems
> > > have a single PMIC which may also be providing other things like GPIOs
> > > that would mean that any consumer that doesn't instantiate would prevent
> > > any device getting turned off which seems even more concerning.
>
> > Your understanding is correct. I'll try to clarify the commit text as
> > best as I can. Your concerns are why this is not the default behavior.
>
> My concern is that introducing extra delays is likely to make things
> more fragile and complex.  As far as I can see this is just making
> things even worse by adding spurious dependencies and delaying things
> further.

I wouldn't call it delaying any requests. This is just an additional
constraint like any other consumer. This definitely makes things more
stable in cases where all the devices probe and even in cases where
some of those devices might never probe (example I gave in [2]).

> > For Android phones that are actually shipped, ensuring every enabled
> > consumer actually probes is trivial and desired.
>
> Other users exist and need to be considered.  People need to be able to
> run upstream kernels, they need to be able to run distros and other
> systems that aren't vertically integrated like yours where people aren't
> developing the DT and kernel together.

Agreed. Which is why the timeout will be enabled by default and the
default behavior won't change. And I'm not making or asking for DT
changes. So it shouldn't break existing users.

> > > > +     if (IS_ERR_OR_NULL(rdev->sync_supply))
> > > > +             return 0;
> > > > +
> > > > +     regulator_disable(rdev->sync_supply);
>
> > > I think sync_supply needs a better name, it's a consumer rather than a
> > > supply any sync doesn't really fit with what it's doing either - getting
> > > rid of it might be syncing but it's not syncing anything.
>
> > Agree about the naming. Maybe boot_limits?
>
> Possibly.
>
> > > > +     /*
> > > > +      * I'm not too enthusiastic about this. I'd rather just go set
> > > > +      * .sync_state() in all the regulator drivers. But let's get the rest
> > > > +      * of the patch sorted out first.
> > > > +      */
> > > > +     if (dev_set_drv_sync_state(rdev->dev.parent, regulator_sync_state))
> > > > +             dev_dbg(&rdev->dev, "parent sync_state() already set\n");
> > > > +     regulator_set_minimum_state(rdev);
>
> > > If you don't want this to be configurable per driver then why do you
> > > want to force it to be configured per driver?  I also think that is a
> > > bad idea, it's just make work as far as I can see.
>
> > I think the last line is a typo?
>
> No, it's not a typo - "make work" is busy work for the sake of it.  I'm
> saying that this is just requiring a mechanical update in all drivers
> both now and going forwards which is exactly the sort of thing we should
> be factoring out of drivers.
>
> > As to why this is configurable per driver, we discussed this in the v1
> > thread. sync_state() isn't specific to the regulator framework (which
> > I'm sure you understand) -- so it needs to be flexible.
> > Devices/drivers like MFDs, PMICs with GPIOs, power domains with reset
> > control, etc might want to call into multiple frameworks when they get
> > their sync state. Some devices/drivers might want to do more stuff
> > specific to their device/system when they get the sync state callback
> > -- they can spin their own sync_state() callback, do their stuff and
> > then call regulator_sync_state() or whatever other framework they
> > want. That's why it's configurable per device/driver.
>
> To repeat what I said at the time a device that's in multiple frameworks
> like this is most likely a MFD with a bunch of broadly unrelated things
> in it, where things are related we need to track those dependencies
> anyway.

MFDs are just one example. In any case, we can stick with how I did it
in v2 where the regulator framework can set up the sync_state()
callback for the driver if it hasn't already set one up.

> At best this is working around a problem that has been
> introduced by the decision to do everything at far too broad a level,
> it feels like it's defeating the point of trying to track dependencies
> at all.  The whole performance seems completely redundant if we're
> trying to aggregate things so aggressively, we end up needing pretty
> much every device in the system to come up before we can do anything.

This is definitely not true on real Android phones. Keep in mind all
of this only applies to resources actually left on by the bootloader.
So, that drastically cuts down what all devices are affected by this.
So, if a PMIC has 5 regulators and only 1 is left on by the
bootloader, then this patch series would be a NOP for the 4
regulators.

> Together with the late_initcall() stuff this is sounding like what it's
> really doing is going a very long way around introducing a new initcall
> that gets triggered after all modules are loaded (which *was* one of the
> earlier suggestions).

That's definitely not the case. sync_state() callbacks will come way
before all the modules are loaded. Again, going by testing it on real
hardware.

-Saravana

[1] - https://lore.kernel.org/lkml/20200605063724.9030-1-m.szyprowski@samsung.com/#t
[2] - https://lore.kernel.org/lkml/CAGETcx8asyFRz5LmU4LSMJuPWvcWdvi1GHAhQ85AWdd6jcmdiA@mail.gmail.com/

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Mark Brown]

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

On Mon, Jun 08, 2020 at 08:16:44PM -0700, Saravana Kannan wrote:
> On Mon, Jun 1, 2020 at 10:23 AM Mark Brown <broonie@kernel.org> wrote:

> > This is I think the first time anyone has suggested that this is likely
> > to be an issue - the previous concerns have all been about screens
> > glitching.

> Looks like we got at least one concrete example now in [1].

That's the Exynos VDD_INT/CPU issue.  I'm not clear that this is
entirely covered TBH, AIUI it needs a coupler all the time so it's not
just a case of waiting for the consumers.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.

> But it doesn't really ignore consumer requests though. In response to
> all your comments above, I haven't done a good job of explaining the
> issues and the solution. I'll try to redo that part again when I send
> v3 and hopefully I can do better.

Yes, I think a lot of this is about clarity of explanation.

> > My concern is that introducing extra delays is likely to make things
> > more fragile and complex.  As far as I can see this is just making
> > things even worse by adding spurious dependencies and delaying things
> > further.

> I wouldn't call it delaying any requests. This is just an additional
> constraint like any other consumer. This definitely makes things more
> stable in cases where all the devices probe and even in cases where
> some of those devices might never probe (example I gave in [2]).

Since your current implementation restricts things until essentially the
entire system is running this is going to affect consumers that don't
share their regulator.  Part of the reason I am so against that idea is
that when it is very important that a driver be able to change the
voltage and have that actually take effect usually the hardware will be
built such that that regulator isn't shared so shareability issues don't
apply, we have regulator_get_exclusive() for such situations though
it's wound up not as widely used as it could be for a bunch of reasons.
Things like MMC where we have to conform to a hardware spec that
includes lowering as well as raising voltages will have issues with
this.

> > At best this is working around a problem that has been
> > introduced by the decision to do everything at far too broad a level,
> > it feels like it's defeating the point of trying to track dependencies
> > at all.  The whole performance seems completely redundant if we're
> > trying to aggregate things so aggressively, we end up needing pretty
> > much every device in the system to come up before we can do anything.

> This is definitely not true on real Android phones. Keep in mind all
> of this only applies to resources actually left on by the bootloader.
> So, that drastically cuts down what all devices are affected by this.
> So, if a PMIC has 5 regulators and only 1 is left on by the
> bootloader, then this patch series would be a NOP for the 4
> regulators.

This is your systems - it is not that unusual for systems to come up
with most if not all of the regulators up in order to keep things
simpler, it's generally easier during bringup and people don't always
see any particular reason to modify the bootloader to change things (and
may not be easily able to customize what the PMIC does itself even if
they wanted to do things at that level).  Such systems will be very much
impacted by this change.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Ulf Hansson]

On Tue, 9 Jun 2020 at 12:51, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jun 08, 2020 at 08:16:44PM -0700, Saravana Kannan wrote:
> > On Mon, Jun 1, 2020 at 10:23 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > This is I think the first time anyone has suggested that this is likely
> > > to be an issue - the previous concerns have all been about screens
> > > glitching.
>
> > Looks like we got at least one concrete example now in [1].
>
> That's the Exynos VDD_INT/CPU issue.  I'm not clear that this is
> entirely covered TBH, AIUI it needs a coupler all the time so it's not
> just a case of waiting for the consumers.
>
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.
>
> > But it doesn't really ignore consumer requests though. In response to
> > all your comments above, I haven't done a good job of explaining the
> > issues and the solution. I'll try to redo that part again when I send
> > v3 and hopefully I can do better.
>
> Yes, I think a lot of this is about clarity of explanation.
>
> > > My concern is that introducing extra delays is likely to make things
> > > more fragile and complex.  As far as I can see this is just making
> > > things even worse by adding spurious dependencies and delaying things
> > > further.
>
> > I wouldn't call it delaying any requests. This is just an additional
> > constraint like any other consumer. This definitely makes things more
> > stable in cases where all the devices probe and even in cases where
> > some of those devices might never probe (example I gave in [2]).
>
> Since your current implementation restricts things until essentially the
> entire system is running this is going to affect consumers that don't
> share their regulator.  Part of the reason I am so against that idea is
> that when it is very important that a driver be able to change the
> voltage and have that actually take effect usually the hardware will be
> built such that that regulator isn't shared so shareability issues don't
> apply, we have regulator_get_exclusive() for such situations though
> it's wound up not as widely used as it could be for a bunch of reasons.
> Things like MMC where we have to conform to a hardware spec that
> includes lowering as well as raising voltages will have issues with
> this.
>

eMMC is not only about voltage levels, but also about enable/disable
of the regulator(s).

More precisely, one needs to follow the steps specified in the eMMC
spec, when disabling the regulator(s).

In other words, the mmc host driver needs to be probed (consumer of
the regulator), before the regulator(s) can be disabled.

Kind regards
Uffe

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Mark Brown]

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

On Mon, Jun 15, 2020 at 12:27:23PM +0200, Ulf Hansson wrote:

> eMMC is not only about voltage levels, but also about enable/disable
> of the regulator(s).

> More precisely, one needs to follow the steps specified in the eMMC
> spec, when disabling the regulator(s).

> In other words, the mmc host driver needs to be probed (consumer of
> the regulator), before the regulator(s) can be disabled.

Right, though you can generally get away with it if you completely cut
the power.  The big thing is that as part of this we need to actually do
the things at the time the driver asks for them to be done rather than
some other time.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] regulator: Add support for sync_state() callbacks

[Ulf Hansson]

On Mon, 15 Jun 2020 at 13:49, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jun 15, 2020 at 12:27:23PM +0200, Ulf Hansson wrote:
>
> > eMMC is not only about voltage levels, but also about enable/disable
> > of the regulator(s).
>
> > More precisely, one needs to follow the steps specified in the eMMC
> > spec, when disabling the regulator(s).
>
> > In other words, the mmc host driver needs to be probed (consumer of
> > the regulator), before the regulator(s) can be disabled.
>
> Right, though you can generally get away with it if you completely cut
> the power.

Well, there are two regulators.

One is the VCC (vmmc) the other is VCCQ (vqmmc). If you can cut both,
yes, then you are right. However, quite often VCCQ (the rail for the
I/O voltage) is an always-on regulator.

Moreover, even if you can cut both regulators, I wouldn't do that,
unless really necessary. It's always better with a graceful power-off
sequence of a storage device, as otherwise flash corruption and other
bad things can more easily happen.

> The big thing is that as part of this we need to actually do
> the things at the time the driver asks for them to be done rather than
> some other time.

Right.

Kind regards
Uffe

Re: [PATCH v2 0/2] regulator_sync_state() support

[Mark Brown]

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

On Thu, May 28, 2020 at 12:06:08PM -0700, Saravana Kannan wrote:

> The simplified explanation of the problem is, for regulators left on by
> the bootloader, we want to keep them on until all the consumers are
> probed. This is because we need to protect consumer-A from turning off a
> shared regulator used by consumer-B. Once consumer-B (and all the other
> consumers come up), they can do it themselves and the regulator
> framework no longer needs to keep the regulator on.

> So, this is not just about module or device probe ordering between
> suppliers and consumers. Even if we get the probe order prefectly right,
> it still won't solve this problem.

This logic seems to be circular - can you be concrete please?

> We can eventually extend this to also cover voltage and other
> properties, but in this patch series I want to get this right for
> "enabled/disabled" first.

I'm quite worried about the extension to voltage changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]