Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\))
Subject: Re: [PATCH v4] regulator: core: Resolve supply name earlier to
 prevent double-init
From:   =?utf-8?Q?Christian_Kohlsch=C3=BCtter?= 
        <christian@kohlschutter.com>
In-Reply-To: <a78822d5-f0a1-6c42-8269-e8168ea5cd5b@samsung.com>
Date:   Thu, 25 Aug 2022 17:18:20 +0200
Cc:     =?utf-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>,
        Liam Girdwood <lgirdwood@gmail.com>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Linux MMC List <linux-mmc@vger.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        Markus Reichl <m.reichl@fivetechno.de>,
        Robin Murphy <robin.murphy@arm.com>,
        Vincent Legoll <vincent.legoll@gmail.com>, wens@kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <932DEB17-70FB-4416-80B3-C48A7C31848F@kohlschutter.com>
References: <YvorNPDQQr2SH/NF@sirena.org.uk>
 <20220818124646.6005-1-christian@kohlschutter.com>
 <CGME20220825113251eucas1p247c3d57de823da148ca4790975a4aba8@eucas1p2.samsung.com>
 <58b92e75-f373-dae7-7031-8abd465bb874@samsung.com>
 <YwdpOX0xCzYwhjmx@sirena.org.uk>
 <a78822d5-f0a1-6c42-8269-e8168ea5cd5b@samsung.com>
To:     Marek Szyprowski <m.szyprowski@samsung.com>,
        Mark Brown <broonie@kernel.org>
Precedence: bulk

> On 25. Aug 2022, at 16:23, Marek Szyprowski <m.szyprowski@samsung.com> =
wrote:
>=20
> Hi Mark,
>=20
> On 25.08.2022 14:21, Mark Brown wrote:
>> On Thu, Aug 25, 2022 at 01:32:50PM +0200, Marek Szyprowski wrote:
>>=20
>>> This patch landed recently in linux next as commit 8a866d527ac0
>>> ("regulator: core: Resolve supply name earlier to prevent =
double-init").
>>> Unfortunately it breaks booting of Samsung Exynos 5800 based =
Peach-Pi
>>> (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit
>>> (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last
>>> message in the kernel log is a message about disabling 'vdd_1v2'
>>> regulator. This regulator is not used directly, however it is a =
supply
>>> for other critical regulators.
>> This suggests that supplies are ending up not getting bound.  Could =
you
>> perhaps add logging to check that we're attempting to resolve the =
supply
>> (in the
>>=20
>>=20
>> +       if ((rdev->supply_name && !rdev->supply) &&
>> +                       (rdev->constraints->always_on ||
>> +                        rdev->constraints->boot_on)) {
>>=20
>> block)?
>=20
>=20
> I've spent a little time debugging this issue and here are my =
findings.=20
> The problem is during the 'vdd_mif' regulator registration. It has one=20=

> supply called 'inb1' and provided by 'vdd_1v2' regulator. Both =
'vdd_mif'=20
> and 'vdd_1v2' regulators are provided by the same PMIC.
>=20
> The problem is that 'inb1' supply is being routed to dummy regulator=20=

> after this change. The regulator_resolve_supply(), which is just after=20=

> the above mentioned check, returns 0 and bounds 'vdd_mif' supply to=20
> dummy-regulator. This happens because regulator_dev_lookup() called in=20=

> regulator_resolve_supply() returns -19, what in turn lets the code to=20=

> use dummy-regulator. I didn't check why it doesn't return =
-EPROBE_DEFER=20
> in that case yet.
>=20
>> I'd also note that it's useful to paste the actual error
>> messages you're seeing rather than just a description of them.
>=20
> There is really nothing more that I can paste here:
>=20
> [   32.306264] systemd-logind[1375]: New seat seat0.
> [   32.331790] systemd-logind[1375]: Watching system buttons on=20
> /dev/input/event1 (gpio-keys)
> [   32.550686] systemd-logind[1375]: Watching system buttons on=20
> /dev/input/event0 (cros_ec)
> [   32.570493] systemd-logind[1375]: Failed to start user service,=20
> ignoring: Unknown unit: user@0.service
> [   32.750913] systemd-logind[1375]: New session c1 of user root.
> [   35.070357] vdd_1v2:
>=20
> --- EOF ---
>=20

I can reproduce these findings (also see the difference in "cat =
/sys/kernel/debug/regulator/regulator_summary")

The check "if (have_full_constraints())" in "regulator_resolve_supply" =
is called even though regulator_dev_lookup returned -ENODEV (-19).
Since my patch now calls "regulator_resolve_supply" twice, the first =
round should really treat ENODEV as "defer".

I propose adding a boolean defer argument to regulator_resolve_supply =
(with defer=3Dtrue in the first, opportunistic run, and false in any =
other situation). I'll have a patch ready later tonight.

Thanks!
Christian