Question about

Question about

[Vladimir Zapolskiy]

Hello,

I need to set a GPIO (active high) output high on boot, which enables a
power rail supplying some external devices.

I have a question regarding "regulator-boot-on" and "enable-active-high"
fixed regulator device tree properties (actually AFAIU it applies to
gpio regulator as well, by the way, which one is proper to use in my
situation?)

Here is what we have from the code:

  [...]
  constraints->boot_on = of_property_read_bool(np, "regulator-boot-on");
  [...]
  if (init_data->constraints.boot_on)
          config->enabled_at_boot = true;
  [...]
  config->enable_high = of_property_read_bool(np, "enable-active-high");
  [...]
  cfg.ena_gpio_invert = !config->enable_high;
  if (config->enabled_at_boot) {
          if (config->enable_high)
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
          else
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
  } else {
          if (config->enable_high)
                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
          else
                 cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
  }
  [...]
  ret = gpio_request_one(config->ena_gpio,
                         GPIOF_DIR_OUT | config->ena_gpio_flags,
                         rdev_get_name(rdev));
  [...]
  /* Enable GPIO at initial use */
  if (pin->enable_count == 0)
          gpiod_set_value_cansleep(pin->gpiod,
                                  !pin->ena_gpio_invert);
  [...]


If we simplify the matter by assuming GPIOF_OUT_INIT_LOW is inverted
GPIOF_OUT_INIT_HIGH and vice versa, then it is easy to compute by
running over the variants that GPIO output value is set HIGH (regardless
of GPIO active low status) if and only if "regulator-boot-on" is
provided and "enable-active-high" has no effect at all.

This fact confuses me, because from the general regulator and fixed
regulator device tree bindings documentation I get:

  [...]
  - regulator-boot-on: bootloader/firmware enabled regulator
  [...]
  - enable-active-high: Polarity of GPIO is Active high
  If this property is missing, the default assumed is Active low.
  [...]

According to the documentation I'd assume that "regulator-boot-on" does
not touch gpio output value setting (so, if it is controlled by
bootloader or firmware, then it might be out of Linux kernel control).
Also my impression of "enable-active-high" property is that is should
have some effect on the GPIO output value (but it is not, see above),
and actually I don't quite understand why this property exists - there
is a high chance that "enable-active-high" and the real GPIO polarity do
not coincide, it should be more reliable to get GPIO flags of a
particular GPIO right in the regulator driver/framework.

Let's consider two possible configurations:

| regulator-boot-on | enable-active-high | GPIO polarity | GPIO output |
+-------------------+--------------------+---------------+-------------+
|        no         |         yes        |  active high  |    low      |
|        no         |          no        |  active low   |   high      |

I'd rather think that both resulting GPIO outputs are incorrect or
better to say do not correspond to my perception of "regulator-boot-on"
and "enable-active-high" DTS properties described in the documentation,
however above "enable-active-high" and actual GPIO polarity are the same
(when they are not, it is another open topic for discussion).

Do I miss something or have a mistake? Is there a problem in the
implemented logic?

Should documentation be updated to reflect "regulator-boot-on" role that
a regulator is re-enabled by the kernel?

Should "enable-active-high" be replaced by getting GPIO flags directly?

Thank you in advance.

--
With best wishes,
Vladimir

Question about fixed regulator DT properties

[Vladimir Zapolskiy]

Hello Mark,

On 18.11.2014 17:00, Vladimir Zapolskiy wrote:
> Hello,
> 
> I need to set a GPIO (active high) output high on boot, which enables a
> power rail supplying some external devices.
> 
> I have a question regarding "regulator-boot-on" and "enable-active-high"
> fixed regulator device tree properties (actually AFAIU it applies to
> gpio regulator as well, by the way, which one is proper to use in my
> situation?)
> 
> Here is what we have from the code:
> 
>   [...]
>   constraints->boot_on = of_property_read_bool(np, "regulator-boot-on");
>   [...]
>   if (init_data->constraints.boot_on)
>           config->enabled_at_boot = true;
>   [...]
>   config->enable_high = of_property_read_bool(np, "enable-active-high");
>   [...]
>   cfg.ena_gpio_invert = !config->enable_high;
>   if (config->enabled_at_boot) {
>           if (config->enable_high)
>                   cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
>           else
>                   cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
>   } else {
>           if (config->enable_high)
>                   cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
>           else
>                  cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
>   }
>   [...]
>   ret = gpio_request_one(config->ena_gpio,
>                          GPIOF_DIR_OUT | config->ena_gpio_flags,
>                          rdev_get_name(rdev));
>   [...]
>   /* Enable GPIO at initial use */
>   if (pin->enable_count == 0)
>           gpiod_set_value_cansleep(pin->gpiod,
>                                   !pin->ena_gpio_invert);
>   [...]
> 
> 
> If we simplify the matter by assuming GPIOF_OUT_INIT_LOW is inverted
> GPIOF_OUT_INIT_HIGH and vice versa, then it is easy to compute by
> running over the variants that GPIO output value is set HIGH (regardless
> of GPIO active low status) if and only if "regulator-boot-on" is
> provided and "enable-active-high" has no effect at all.
> 
> This fact confuses me, because from the general regulator and fixed
> regulator device tree bindings documentation I get:
> 
>   [...]
>   - regulator-boot-on: bootloader/firmware enabled regulator
>   [...]
>   - enable-active-high: Polarity of GPIO is Active high
>   If this property is missing, the default assumed is Active low.
>   [...]
> 
> According to the documentation I'd assume that "regulator-boot-on" does
> not touch gpio output value setting (so, if it is controlled by
> bootloader or firmware, then it might be out of Linux kernel control).
> Also my impression of "enable-active-high" property is that is should
> have some effect on the GPIO output value (but it is not, see above),
> and actually I don't quite understand why this property exists - there
> is a high chance that "enable-active-high" and the real GPIO polarity do
> not coincide, it should be more reliable to get GPIO flags of a
> particular GPIO right in the regulator driver/framework.
> 
> Let's consider two possible configurations:
> 
> | regulator-boot-on | enable-active-high | GPIO polarity | GPIO output |
> +-------------------+--------------------+---------------+-------------+
> |        no         |         yes        |  active high  |    low      |
> |        no         |          no        |  active low   |   high      |
> 
> I'd rather think that both resulting GPIO outputs are incorrect or
> better to say do not correspond to my perception of "regulator-boot-on"
> and "enable-active-high" DTS properties described in the documentation,
> however above "enable-active-high" and actual GPIO polarity are the same
> (when they are not, it is another open topic for discussion).
> 
> Do I miss something or have a mistake? Is there a problem in the
> implemented logic?
> 
> Should documentation be updated to reflect "regulator-boot-on" role that
> a regulator is re-enabled by the kernel?
> 
> Should "enable-active-high" be replaced by getting GPIO flags directly?
> 
> Thank you in advance.
> 

sorry for non-informative original subject, I would appreciate to get
any comments from you on the topic, if there is a problem, it may caused
by your commit 25a53dfbfbf.

If there is an actual problem, please let me know, I'm always willing to
improve the kernel.

--
With best wishes,
Vladimir

Re: Question about fixed regulator DT properties

[Mark Brown]

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

On Wed, Nov 19, 2014 at 04:38:01PM +0200, Vladimir Zapolskiy wrote:
> Hello Mark,
> 
> On 18.11.2014 17:00, Vladimir Zapolskiy wrote:

Your mail is really quite long and all in quotes which makes it hard to
follow, brevity is really helpful to readers.

> > | regulator-boot-on | enable-active-high | GPIO polarity | GPIO output |
> > +-------------------+--------------------+---------------+-------------+
> > |        no         |         yes        |  active high  |    low      |
> > |        no         |          no        |  active low   |   high      |

> > I'd rather think that both resulting GPIO outputs are incorrect or
> > better to say do not correspond to my perception of "regulator-boot-on"
> > and "enable-active-high" DTS properties described in the documentation,
> > however above "enable-active-high" and actual GPIO polarity are the same
> > (when they are not, it is another open topic for discussion).

What you're saying seems sensible.

> > Should documentation be updated to reflect "regulator-boot-on" role that
> > a regulator is re-enabled by the kernel?

I'm confused about this.  That's the sole purpose of the flag and as far
as I can tell it's what the documentation says.

> > Should "enable-active-high" be replaced by getting GPIO flags directly?

Probably makes sense, it predates those flags by quite some time.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Question about fixed regulator DT properties

[Vladimir Zapolskiy]

Hello Mark,

On 25.11.2014 14:17, Mark Brown wrote:
> On Wed, Nov 19, 2014 at 04:38:01PM +0200, Vladimir Zapolskiy wrote:
>> Hello Mark,
>>
>> On 18.11.2014 17:00, Vladimir Zapolskiy wrote:
> 
> Your mail is really quite long and all in quotes which makes it hard to
> follow, brevity is really helpful to readers.

my sole purpose was to describe the problems I encounter in details,
sorry for excessive verbosity.

Just to summarize my findings:
a) "enable-active-high" property has no effect on GPIO output,
b) "regulator-boot-on" does not mean that the regulator is controlled by
bootloader or firmware exclusively.

>>> | regulator-boot-on | enable-active-high | GPIO polarity | GPIO output |
>>> +-------------------+--------------------+---------------+-------------+
>>> |        no         |         yes        |  active high  |    low      |
>>> |        no         |          no        |  active low   |   high      |
> 
>>> I'd rather think that both resulting GPIO outputs are incorrect or
>>> better to say do not correspond to my perception of "regulator-boot-on"
>>> and "enable-active-high" DTS properties described in the documentation,
>>> however above "enable-active-high" and actual GPIO polarity are the same
>>> (when they are not, it is another open topic for discussion).
> 
> What you're saying seems sensible.

Good, I read it as a confirmation that the problem exists.

>>> Should documentation be updated to reflect "regulator-boot-on" role that
>>> a regulator is re-enabled by the kernel?
> 
> I'm confused about this.  That's the sole purpose of the flag and as far
> as I can tell it's what the documentation says.

Documentation/devicetree/bindings/regulator/regulator.txt says:

  - regulator-boot-on: bootloader/firmware enabled regulator

I would suggest to add Linux kernel to that list of regulator
controllers, if it is the intention. In its current state the
documentation makes an impression that "regulator-boot-on" property
instructs the kernel to ignore regulator setup, since it is already
controlled by bootloader or firmware.

>>> Should "enable-active-high" be replaced by getting GPIO flags directly?
> 
> Probably makes sense, it predates those flags by quite some time.
> 

If you have no objection I'll take a look how to fix it by removing
"enable-active-high" flag completely from the driver's logic,
fortunately the flag has no tangible effect at the moment as it is shown
by my analysis.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question about fixed regulator DT properties

[Mark Brown]

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

On Wed, Nov 26, 2014 at 07:27:06PM +0200, Vladimir Zapolskiy wrote:
> On 25.11.2014 14:17, Mark Brown wrote:

> b) "regulator-boot-on" does not mean that the regulator is controlled by
> bootloader or firmware exclusively.

That's correct...

> >>> Should documentation be updated to reflect "regulator-boot-on" role that
> >>> a regulator is re-enabled by the kernel?

> > I'm confused about this.  That's the sole purpose of the flag and as far
> > as I can tell it's what the documentation says.

> Documentation/devicetree/bindings/regulator/regulator.txt says:

>   - regulator-boot-on: bootloader/firmware enabled regulator

> I would suggest to add Linux kernel to that list of regulator
> controllers, if it is the intention. In its current state the
> documentation makes an impression that "regulator-boot-on" property
> instructs the kernel to ignore regulator setup, since it is already
> controlled by bootloader or firmware.

No, not at all.  It's referring to the state when Linux starts.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Question about fixed regulator DT properties

[Vladimir Zapolskiy]

Hi, MarkOn 26.11.2014 19:53, Mark Brown wrote:
> On Wed, Nov 26, 2014 at 07:27:06PM +0200, Vladimir Zapolskiy wrote:
>> On 25.11.2014 14:17, Mark Brown wrote:
> 
>> b) "regulator-boot-on" does not mean that the regulator is controlled by
>> bootloader or firmware exclusively.
> 
> That's correct...
> 
>>>>> Should documentation be updated to reflect "regulator-boot-on" role that
>>>>> a regulator is re-enabled by the kernel?
> 
>>> I'm confused about this.  That's the sole purpose of the flag and as far
>>> as I can tell it's what the documentation says.
> 
>> Documentation/devicetree/bindings/regulator/regulator.txt says:
> 
>>   - regulator-boot-on: bootloader/firmware enabled regulator
> 
>> I would suggest to add Linux kernel to that list of regulator
>> controllers, if it is the intention. In its current state the
>> documentation makes an impression that "regulator-boot-on" property
>> instructs the kernel to ignore regulator setup, since it is already
>> controlled by bootloader or firmware.
> 
> No, not at all.  It's referring to the state when Linux starts.
> 

thank you for clarification, to grasp the underlying policy let me ask
for some more information.

If I want to enable a fixed regulator (not controlled by
bootloader/firmware) by Linux on boot or when fixed.ko module is bound,
shall I specify the same "regulator-boot-on" property? At least this is
the practical way to enable a fixed and/or gpio regulator right now, but
is it correct?

Or should the regulator always be enabled externally (assuming
"regulator-always-on" is omitted) after registration independently on
"regulator-boot-on" property?

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question about fixed regulator DT properties

[Mark Brown]

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

On Wed, Nov 26, 2014 at 09:13:50PM +0200, Vladimir Zapolskiy wrote:

> If I want to enable a fixed regulator (not controlled by
> bootloader/firmware) by Linux on boot or when fixed.ko module is bound,
> shall I specify the same "regulator-boot-on" property? At least this is
> the practical way to enable a fixed and/or gpio regulator right now, but
> is it correct?

It depends what you're trying to accomplish by doing this.

> Or should the regulator always be enabled externally (assuming
> "regulator-always-on" is omitted) after registration independently on
> "regulator-boot-on" property?

Best practice is that there should be a consumer which keeps the
regulator enabled whenever it is required.  There should normally be
little use for boot-on, it's mostly there to ease handover from the
bootloader in cases where we can't read the hardware state - if you're
not sure if you should use it the chances are you shouldn't.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Question about fixed regulator DT properties

[Vladimir Zapolskiy]

On 26.11.2014 21:20, Mark Brown wrote:
> On Wed, Nov 26, 2014 at 09:13:50PM +0200, Vladimir Zapolskiy wrote:
> 
>> If I want to enable a fixed regulator (not controlled by
>> bootloader/firmware) by Linux on boot or when fixed.ko module is bound,
>> shall I specify the same "regulator-boot-on" property? At least this is
>> the practical way to enable a fixed and/or gpio regulator right now, but
>> is it correct?
> 
> It depends what you're trying to accomplish by doing this.

If "regulator-boot-on" is specified and the regulator is enabled by
bootloader/firmware, then the kernel re-enables it.

If "regulator-boot-on" is specified and the regulator is untouched by
bootloader/firmware, then the kernel simply enables it.

As far as I understand the latter side-effect is exploited on quite many
ARM boards, when there is no defined regulator consumer, but I agree
that it looks hackish. My assumption is that probably fixed regulator
logic around "regulator-boot-on" property should be changed, so that the
kernel will not attempt to physically re-enable/enable the
"regulator-boot-on" regulator at all, then misusage of the property
should gone forced by necessity of finding a proper regulator consumer.

>> Or should the regulator always be enabled externally (assuming
>> "regulator-always-on" is omitted) after registration independently on
>> "regulator-boot-on" property?
> 
> Best practice is that there should be a consumer which keeps the
> regulator enabled whenever it is required.  There should normally be
> little use for boot-on, it's mostly there to ease handover from the
> bootloader in cases where we can't read the hardware state - if you're
> not sure if you should use it the chances are you shouldn't.
> 

Right, thank you for explanation.

--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Question about fixed regulator DT properties

[Mark Brown]

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

On Wed, Nov 26, 2014 at 09:57:33PM +0200, Vladimir Zapolskiy wrote:

> If "regulator-boot-on" is specified and the regulator is untouched by
> bootloader/firmware, then the kernel simply enables it.

> As far as I understand the latter side-effect is exploited on quite many
> ARM boards, when there is no defined regulator consumer, but I agree
> that it looks hackish. My assumption is that probably fixed regulator

It's not just hackish, if the regulator actually needs to be on but has
no users it's actively broken since the core might decide to power it
off at any time for any reason.  Such regulators *must* be flagged as
always-on.

> logic around "regulator-boot-on" property should be changed, so that the
> kernel will not attempt to physically re-enable/enable the
> "regulator-boot-on" regulator at all, then misusage of the property
> should gone forced by necessity of finding a proper regulator consumer.

The only boards which might be able to get away with this are
non-DT/ACPI ones which aren't getting merged anyway and it is in general
going to be better to expand the set of cases where we do the disables
(since it saves power).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]