Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[Graeme Gregory]

On Tue, 29 Jun 2021, at 9:21 PM, H. Nikolaus Schaller wrote:
> Hi Mark,
> 
> > Am 29.06.2021 um 20:56 schrieb Mark Brown <broonie@xxxxxxxxxx>:
> > 
> > On Tue, Jun 29, 2021 at 08:34:55PM +0200, H. Nikolaus Schaller wrote:
> >>> Am 29.06.2021 um 17:59 schrieb Mark Brown <broonie@xxxxxxxxxx>:
> > 
> > 
> >> So it was working fine without having the supplying regulator resolved. AFAIK they
> >> just serve as fixed regulators in the device tree and have no physical equivalent.
> > 
> > No, not at all - it's representing whatever provides input power to the
> > regulator.  There may be no physical control of it at runtime on your
> > system but that may not be true on other systems.  It's quite common for
> > there to be a chain of regulators (eg, DCDCs supplying LDOs) and then
> > they all need to get get power managed appropriately and you don't end
> > up thinking a regulator is enabled when the input regulator is disabled.  
> 
> Yes, that is how it is chained in other cases.
> 
> > 
> >> My proposal just moves setting the supply_name behind devm_regulator_register() and
> >> by that restores the old behaviour.
> > 
> > This means that we won't actually map the supply and any system that
> > relies on software handling the supply regulator will be broken.
> 
> Well, it seems as if the supply regulators are only vsys_cobra and 
> vdds_1v8_main.
> At least in omap5-board-common.dtsi which is what I can test.
> 
> Both are fixed regulators where calling enable or not doesn't become
> physically visible. The hardware still supplies the 5V and 1.8V to the palmas
> chip.
> 
> Maybe the new rule by commit 98e48cd9283dreveals a design issue inside of
> the Palmas driver which assumes that there is no need to control its supply
> regulators. And does not handle probe deferral.
> 
> Then of course my patch is just a work-around for a bug but not a solution.
> 
> > 
> >> Well, unless...
> > 
> >> ... devm_regulator_register() does something differently if desc->supply_name
> >> is not set before and changed afterwards. It may miss that change.
> > 
> > We resolve supplies during regulator registration, this would
> > effectively just skip mapping of the supply.
> > 
> >> So I hope for guidance if my approach is good or needs a different solution.
> > 
> > What I would expect to happen here would be that once vsys_cobra is
> > registered the regulators supplied by it can register and then all their
> > consumers would in turn be able to register.  You should look into why
> > that supply regulator isn't appearing and resolve that, or if a consumer
> > isn't handling deferral then that would need to be addressed.
> 
> Ah, I think I get an idea what may be going wrong.
> 
> There seems to be a deadlock in probing:
> 
> 	e.g. ldo3_reg depends on vdds_1v8_main supply
> 	vdds_1v8_main depends on smps7_reg supply
> 	smps7_reg depends on vsys_cobra supply
> 	vsys_cobra depends on nothing
> 
> This would normally lead to a simple chain as you described above. But
> ldo3_reg and smps7_reg share the same driver and can probe successfully or
> fail only in common.
> 
> Now if ldo3_reg defers probe through the new rule, smps7_reg is never
> probed successfully because it is the same driver. Hence vdds_1v8_main can
> not become available. And the Palmas continues to run in boot initialization
> until some driver (MMC) wants to switch voltages or whatever.
> 
> Maybe Tony or Graeme has an idea how to do it right...
> 
Sorry after almost 10 years I have forgotten all about this driver.

Graeme