linux-kernel - Re: [PATCH] regulator: fixed: Default enable high on DT regulators

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c2ace38e09344d325271498fe8133c888470b5f6.camel@nxp.com>
Date:   Wed, 3 Oct 2018 18:08:09 +0000
From:   Leonard Crestez <leonard.crestez@....com>
To:     "broonie@...nel.org" <broonie@...nel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>
CC:     dl-linux-imx <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andy Duan <fugang.duan@....com>,
        "festevam@...il.com" <festevam@...il.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "anders.roxell@...aro.org" <anders.roxell@...aro.org>,
        "john.stultz@...aro.org" <john.stultz@...aro.org>
Subject: Re: [PATCH] regulator: fixed: Default enable high on DT regulators

On Wed, 2018-10-03 at 13:10 +0100, Mark Brown wrote:
> On Tue, Oct 02, 2018 at 01:42:38PM +0000, Leonard Crestez wrote:
> 
> > This turns the phy off and on again instead of leaving it up from uboot
> > and it doesn't work for some reason. However looking at
> > reg_fixed_voltage_probe introducing an edge seems to be intentional for
> > regulators which are not marked with "enabled-at-boot". Right?
> 
> No, that's definitely not desired.  We don't want to change the state of
> the regulator at all if we can avoid it unless the user explicitly asked
> for it.

That also makes sense, for a top level perspective. But
reg_fixed_voltage_probe contains the following snippet:

	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;
	}

Unless config->enabled_at_boot the GPIO is initialized with the
opposite polarity of enabled. This means that fixed regulators which
are not marked with "enabled-at-boot" but happen to be "on" anyway are
always power cycled.

This is from before recent changes by Linus, code dates from 2012
commit 25a53dfbfbfd ("regulator: fixed: Use core GPIO enable support").
Even before that the logic was similar:

	drvdata->is_enabled = config->enabled_at_boot;
	ret = drvdata->is_enabled ?
        	config->enable_high : !config->enable_high;
	gpio_flag = ret ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;

Looking further back it seems that this behavior has always been
present in fixed-regulator code.

In theory it might be possible to request the GPIO while asking to keep
the value from the bootloader? Maybe I'm confused but I don't see an
easy way to do this through the GPIO api; functions for requesting in
output mode all seem to also ask for the initial value.

GPIOD_ASIS looks close but it doesn't even adjust the direction.

> > It's possible that you exposed an imx board-specific bug: maybe power
> > cycling the phy after uboot needs some missing fixup?
> 
> It'd probably also be good to sort this out though.

Yes, handled separately: https://lore.kernel.org/patchwork/patch/994871/