Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1422961 > unrolled thread

Re: [RFC PATCH] regulator: introduce boot protection flag

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] regulator: introduce boot protection flag Pingbo Wen <pingbo.wen@linaro.org> - 2016-06-15 14:10 +0200
    Re: [RFC PATCH] regulator: introduce boot protection flag Mark Brown <broonie@kernel.org> - 2016-06-15 15:40 +0200
      Re: [RFC PATCH] regulator: introduce boot protection flag Pingbo Wen <pingbo.wen@linaro.org> - 2016-06-17 05:40 +0200
        Re: [RFC PATCH] regulator: introduce boot protection flag Mark Brown <broonie@kernel.org> - 2016-06-17 13:50 +0200
          Re: [RFC PATCH] regulator: introduce boot protection flag Pingbo Wen <pingbo.wen@linaro.org> - 2016-06-23 14:10 +0200

#1422961 — Re: [RFC PATCH] regulator: introduce boot protection flag

Hi, Mark

On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
> On Mon, May 09, 2016 at 03:05:08PM +0800, WEN Pingbo wrote:
> 
>> And regulator core will postpone all operations until all consumers
>> have taked their place.
> 
> It doesn't, it postpones them until late_initacall().  This is both
> after the consumers have loaded if they are built in and before any
> consumers built as modules come up.

Yes, this patch only protects a regulator from regulator
registration(built in) to late_initcall(). But, IMO, if a regulator is
critical, it's weird to build as a module. Maybe I was thoughtless here.

If we take modules under consideration, and to make this patch more
universal, I think what we really need is adding a flag to protect a
regulator from registration to a specific consumer(not the first
consumer). The regulator driver gives the initial state, and the
specific consumer need to clear this flag while finishing regulator
setting(by calling a function like regulator_clear_protect()). And what
the regulator core need to do is staging all operations during
protection. And that will cover all consumers probing order, whenever
the regulator is registered.

Any idea?

> 
>> The boot_protection flag only work before late_initicall. And as other
>> constraints liked, you can specify this flag in a board file, or in
>> dts file.
> 
> Anything added to the DT ABI needs a binding.
> 

I will add bindings in next version.

>> +	/* constraints check has already done */
>> +	if (rdev->boot_mode)
>> +		rdev->desc->ops->set_mode(rdev, rdev->boot_mode);
> 
> This whole sequence of code ignores errors - that's not great.  We
> should at least log them.
> 

OK.

>> +	mutex_unlock(&rdev->mutex);
>> +
>> +	if (regulator)
>> +		regulator_set_voltage(regulator, regulator->min_uV,
>> +				regulator->max_uV);
> 
> That's...  exciting.  There's a couple of issues here.  One is that
> this is not operating on the rdev but rather on a consumer regulator
> device, the other is that we drop out of the lock before doing the
> update which tends to be a warning sign that something fun is going on
> and at least an internal function should be used.  These two most likely
> come down to the same issue.
> 

OK, some bugs here. I will use a unlock version.

Pingbo

[toc] | [next] | [standalone]

#1423022

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jun 15, 2016 at 08:05:13PM +0800, Pingbo Wen wrote:
> On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:

> > It doesn't, it postpones them until late_initacall().  This is both
> > after the consumers have loaded if they are built in and before any
> > consumers built as modules come up.

> Yes, this patch only protects a regulator from regulator
> registration(built in) to late_initcall(). But, IMO, if a regulator is
> critical, it's weird to build as a module. Maybe I was thoughtless here.

Well, this comes back to the issue with it being very use case specific
and not generally clear what critical regualtors are supposed to be.  It
really depends on what the system integrator wants, and there's things
like the distro use case where the normal thing to do is to build as
many drivers as possible as modules since you support such a huge range
of hardware.

> If we take modules under consideration, and to make this patch more
> universal, I think what we really need is adding a flag to protect a
> regulator from registration to a specific consumer(not the first
> consumer). The regulator driver gives the initial state, and the
> specific consumer need to clear this flag while finishing regulator
> setting(by calling a function like regulator_clear_protect()). And what
> the regulator core need to do is staging all operations during
> protection. And that will cover all consumers probing order, whenever
> the regulator is registered.

Having the consumer driver know that it's "critical" seems wrong since
different systems may have different ideas about that, it's probably
better to hook this in with the device model so that when the device
finishes probing that kicks things off.

> Any idea?

There were the other ideas I mentioned in my mail too.

[toc] | [prev] | [next] | [standalone]

#1424628

On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote:
> On Wed, Jun 15, 2016 at 08:05:13PM +0800, Pingbo Wen wrote:
>> On Thursday, June 09, 2016 01:16 AM, Mark Brown wrote:
>
>> If we take modules under consideration, and to make this patch more
>> universal, I think what we really need is adding a flag to protect a
>> regulator from registration to a specific consumer(not the first
>> consumer). The regulator driver gives the initial state, and the
>> specific consumer need to clear this flag while finishing regulator
>> setting(by calling a function like regulator_clear_protect()). And what
>> the regulator core need to do is staging all operations during
>> protection. And that will cover all consumers probing order, whenever
>> the regulator is registered.
> 
> Having the consumer driver know that it's "critical" seems wrong since
> different systems may have different ideas about that, it's probably
> better to hook this in with the device model so that when the device
> finishes probing that kicks things off.
> 

That will imply the protection would be end when the specific device has
probed, and consumers should take their place at the same time. But
there have some other devices, which will set the consumer in a IRQ
event, or after some other events, can't be covered.

We can set the protection flag easily, but it's hard to tell whether a
consumer is well initialized, the end of protection, since regulator
consumer is not initialized within one call.

Pingbo

[toc] | [prev] | [next] | [standalone]

#1424970

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 17, 2016 at 11:34:25AM +0800, Pingbo Wen wrote:
> On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote:

> > Having the consumer driver know that it's "critical" seems wrong since
> > different systems may have different ideas about that, it's probably
> > better to hook this in with the device model so that when the device
> > finishes probing that kicks things off.

> That will imply the protection would be end when the specific device has
> probed, and consumers should take their place at the same time. But
> there have some other devices, which will set the consumer in a IRQ
> event, or after some other events, can't be covered.

I don't understand what this means, sorry.

> We can set the protection flag easily, but it's hard to tell whether a
> consumer is well initialized, the end of protection, since regulator
> consumer is not initialized within one call.

If the driver is not initializing itself during probe the driver is
doing something wrong and needs to be fixed anyway.

[toc] | [prev] | [next] | [standalone]

#1429738

Hi, Mark

On Friday, June 17, 2016 07:42 PM, Mark Brown wrote:
> On Fri, Jun 17, 2016 at 11:34:25AM +0800, Pingbo Wen wrote:
>> On Wednesday, June 15, 2016 09:32 PM, Mark Brown wrote:
> 
>>> Having the consumer driver know that it's "critical" seems wrong since
>>> different systems may have different ideas about that, it's probably
>>> better to hook this in with the device model so that when the device
>>> finishes probing that kicks things off.
> 
>> That will imply the protection would be end when the specific device has
>> probed, and consumers should take their place at the same time. But
>> there have some other devices, which will set the consumer in a IRQ
>> event, or after some other events, can't be covered.
> 
> I don't understand what this means, sorry.
> 

I mean maybe there's some consumer driver only do partial initialization
during probing.

>> We can set the protection flag easily, but it's hard to tell whether a
>> consumer is well initialized, the end of protection, since regulator
>> consumer is not initialized within one call.
> 
> If the driver is not initializing itself during probe the driver is
> doing something wrong and needs to be fixed anyway.
> 

OK, if all driver have full initialized during probing, and we need
insert a hook after driver probing. I think we can add a function in
driver/base/dd.c:driver_probe_device() as this:

	ret = really_probe(dev, drv);
	...
	if (!ret)
		regulator_clean_up(dev);

And in regulator_clean_up(), we can iterate all regulator deivce, and
call a regulator_clear_boot_protection function:

	if (!rdev->constraints->boot_protection)
		return 0;

	if (strcmp(rdev->constraints->critical_consumer, dev_name(dev)))
		return 0;

	rdev->constraints->boot_protection = 0;
	...
	-real clean stuffs-

the critical_consumer can be specified in devicetree.

Add a callback in driver_probe_device() is not so good, but it's fine
for me.

Pingbo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web