Date: Tue, 28 Apr 2020 06:24:10 +0200 From: Lukasz Majewski lukma@denx.de
Hi Lukasz,
Hi Mark,
The fix in commit b7adcdd073c0 has the side-effect that the regulator will be disabled when requesting the relevant gpio in regulator_common_ofdata_to_platdata() and enabled in regulator_pre_probe() when the regulator was already enabled. This leads to a short interruption in the 3.3V power to the PCIe slot on the firefly-rk3399 which makes an ADATA SX8000NP NVMe SSD unhappy.
Fix this by setting the GPIOD_IS_OUT_ACTIVE flag again when the 'regulator-boot-on' property is set, but check for this property explicitly instead of relying on the "boot_on" member of the uclass platdata.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
drivers/power/regulator/regulator-uclass.c | 3 --- drivers/power/regulator/regulator_common.c | 2 ++ 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/power/regulator/regulator-uclass.c b/drivers/power/regulator/regulator-uclass.c index c9d26344d7..90961de95c 100644 --- a/drivers/power/regulator/regulator-uclass.c +++ b/drivers/power/regulator/regulator-uclass.c @@ -464,9 +464,6 @@ static int regulator_pre_probe(struct udevice *dev) (uc_pdata->min_uA == uc_pdata->max_uA)) uc_pdata->flags |= REGULATOR_FLAG_AUTOSET_UA;
- if (uc_pdata->boot_on)
regulator_set_enable(dev, uc_pdata->boot_on);- return 0;
}
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 33b73b7c2f..bc13b88476 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -17,6 +17,8 @@ int regulator_common_ofdata_to_platdata(struct udevice *dev, if (!dev_read_bool(dev, "enable-active-high")) flags |= GPIOD_ACTIVE_LOW;
if (dev_read_bool(dev, "regulator-boot-on"))
flags |= GPIOD_IS_OUT_ACTIVE;/* Get optional enable GPIO desc */ gpio = &dev_pdata->gpio;
Sorry, but this is a simple revert of my commit and breaks use cases described in the commit message of this fix.
No, it isn't a simple revert, and I don't think it will break the i.MX ethernet driver.
I think the analysis in the commit message is a bit flawed. I don't think there was something actually wrong with commit e8e9715df2d4 because at the time regulator_pre_probe() was called before regulator_common_ofdata_to_platdata().
This was changed in 29f7d05a347ab ("dm: core: Move ofdata_to_platdata() call earlier"), which was the commit git bisect led me to. After that commit the boot_on member no longer being set.
My diff is indeed mostly a revert of yours, but it replaces the boot_on member check with an explicit check of the "boot-on" property. As a result the GPIOD_IS_OUT_ACTIVE flag is set correctly and the regulator should be turned on (or stay on in my case).
Do you see some kind of "glitch" on the gpio in regulator_common_of_platdata?
I don't have the tools to actually measure the pin but yes all the evidence points to a glitch. I also instrumented the actual gpio driver and it defenitely is setting the pin low (0) and setting it high (1) again later. So I think it is essential that GPIOD_IS_OUT_ACTIVE is set when the gpio is set in the gpio_request_by_name() call. Enabling it later later isn't good enough.
What I'm seeing on my hardware is that the NVMe device is still present on the PCIe bus, but reports a bogus size and a different firmware version string. I'm fairly certain that a glitch on the 3.3V pin is interrupting the load of the firmware that is stored on the card itself. When this happens, the only thing that fixes it is a complete power cycle of the board.
The regulator-boot-on property [1] shall prevent from the issue you described in the commit message of this revert.
Links: [1] - https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bind...
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de