[PATCH] regulator: fix: enable gpio when requested

[Mark Kettenis]

> Date: Tue, 28 Apr 2020 20:26:09 +0200
> From: Lukasz Majewski <lukma at denx.de>

Ping?

> Hi Mark,
> 
> > > Date: Tue, 28 Apr 2020 06:24:10 +0200
> > > From: Lukasz Majewski <lukma at 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 at 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 need to check this - as this issue was critical for i.MX53 (it caused
> the board not being able to update U-Boot after this change).
> 
> > 
> > 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).
> 
> I must have overlooked the difference. In my case the regulator had to
> be powered up early (in the preprobe) so ETH initialization would be
> performed with enabled power.
> 
> > 
> > > 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/bindings/regulator/regulator.yaml#L40
> > > 
> > > 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 at denx.de  
> 
> 
> 
> 
> 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 at denx.de
> 
> [2:application/pgp-signature Show Save:noname (488B)]
>