regulator: BD71837: possible regression

regulator: BD71837: possible regression

[Vaittinen, Matti]

Hello Angus,

I'll add the linux list and Mark to CC as this sounds like a regression
which may impact to other regulator drivers too. Mark, please let me
know if you don't feel adding you to discussions like this are
appropriate so I don't do it in the future.

On Mon, 2019-05-13 at 17:21 -0700, Angus Ainslie wrote:
> Hi Matti,
> 
> I've been doing some integration with our feature branches on linux-
> next 
> (next-20190510) and I hit a snag as soon as I enable the BD71837.
> 
> There's a couple of weird things I'm seeing
> 
> [    0.907025] bd718xx-pmic bd718xx-pmic.2.auto: no default pinctrl 
> state
> [    0.907651] bd718xx-pmic bd718xx-pmic.2.auto: Unlocked lock
> register 
> 0x2f
> [    0.932483] rohm-bd718x7 0-004b: Looking up buck6-supply from
> device 
> tree
> [    0.932499] rohm-bd718x7 0-004b: Looking up buck6-supply property
> in 
> node /soc@0/bus@30800000/i2c@30a20000/pmic@4b fa
> iled
> [    0.937861] rohm-bd718x7 0-004b: Looking up buck7-supply from
> device 
> tree
> [    0.937877] rohm-bd718x7 0-004b: Looking up buck7-supply property
> in 
> node /soc@0/bus@30800000/i2c@30a20000/pmic@4b fa
> iled

Does this mean we have a regression in linux-next? I guess you have not
seen these issues in older kernels?

> What should buck6/7 supply be set to ?

The buck6 and buck7 are not supplied by any other regulators. What is
special about buck6 and buck7 is that they are supplying LDOs 5 and 6.
So I assume the problem emerges when LDOs are trying to find their
suppliers. (I am not sure if supplier is correct word - but what I mean
is that bucks 6 and 7 are kind of parents for LDOs). This is reflected
in regulator_desc for LDOs. (The supply_name field is set).

I am not sure but perhaps the regulator core is changed so that this
parent/child relation must be modelled using <foo>-supply properties in
device-tree. Are you able to bisect the change which breaks this? There
may be other regulator drivers doing the same as bd718x7 is (which
means trusiting to setting the supply_name in desc to be enough - and
without deeper understanding I'd say it should be enough).

If this change is intentional and buck6-supply and buck7-supply are bow
required also in DT, then we should reflect this fact also in bindings
doc for BD71837 and BD71847.

> The rtc also seems to have lost it's mind
> 
> [  497.363621] rtc rtc0: Timeout waiting for LPSRT Counter to change
> 
> Am I missing linking the bd71837 clock to the rtc somehow ?

I don't think there is any SW actions required. The 30.72K clock can be
controlled using clk-bd718x7 driver - but I don't think there has been
changes there and this drivers should not touch the clk gate unless
asked by some user. Still, if the bucks6/7 get disabled for some reason
(or if LDOs are enabled before these bucks are enabled) - then, without
the correct parent child relation the LDOs can't get enabled - and
voltage monitoring may kick-in. Yet, assuming the SoC is powered by
bd71837 this should probably lead to reset-loop and not just to clock
errors...

I would definitely start looking at the changes in how suppliers are
looked up and maybe also tried setting the buck6-supply and buck7-
supply properties with phandles to buck6 and buck7 nodes. I can also
try checking the core if you don't have the possibility to do bisecting
- but I am not able to do it right now. It may be I can check this only
at next week.

> And finally the reset is hanging again even with 
> "rohm,reset-snvs-powered".

This could well be due to the missing parent/child relation between
bucks and LDOs.

> 
> Pointers would be appreciated.
> 
> Thanks
> Angus

Br,
	Matti Vaittinen

Re: regulator: BD71837: possible regression

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On Tue, May 14, 2019 at 06:14:41AM +0000, Vaittinen, Matti wrote:

> I am not sure but perhaps the regulator core is changed so that this
> parent/child relation must be modelled using <foo>-supply properties in
> device-tree. Are you able to bisect the change which breaks this? There
> may be other regulator drivers doing the same as bd718x7 is (which
> means trusiting to setting the supply_name in desc to be enough - and
> without deeper understanding I'd say it should be enough).

The framework will look for the parent regulator and warn if it can't
find it but it should still instantiate it if the mapping is a hard
failure (as opposed to a probe deferral).

> If this change is intentional and buck6-supply and buck7-supply are bow
> required also in DT, then we should reflect this fact also in bindings
> doc for BD71837 and BD71847.

It is always and has always been best practice to wire up the regulators
as completely as possible; this is less error prone and gives you more
ability to take advantage of framework improvements.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]