> Date: Tue, 28 Apr 2020 20:26:09 +0200 > From: Lukasz Majewski <[email protected]>
Ping? > Hi Mark, > > > > Date: Tue, 28 Apr 2020 06:24:10 +0200 > > > From: Lukasz Majewski <[email protected]> > > > > 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 <[email protected]> > > > > --- > > > > 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: > > > [email protected] > > > > > 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: [email protected] > > [2:application/pgp-signature Show Save:noname (488B)] >
