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;
Message-ID: <58b92e75-f373-dae7-7031-8abd465bb874@samsung.com>
Date:   Thu, 25 Aug 2022 13:32:50 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0)
        Gecko/20100101 Thunderbird/91.12.0
Subject: Re: [PATCH v4] regulator: core: Resolve supply name earlier to
 prevent double-init
Content-Language: en-US
To:     =?UTF-8?Q?Christian_Kohlschu=cc=88tter?= 
        <christian@kohlschutter.com>, broonie@kernel.org
Cc:     heiko@sntech.de, lgirdwood@gmail.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mmc@vger.kernel.org, linux-rockchip@lists.infradead.org,
        m.reichl@fivetechno.de, robin.murphy@arm.com,
        vincent.legoll@gmail.com, wens@kernel.org
From:   Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <20220818124646.6005-1-christian@kohlschutter.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <YvorNPDQQr2SH/NF@sirena.org.uk>
        <20220818124646.6005-1-christian@kohlschutter.com>
        <CGME20220825113251eucas1p247c3d57de823da148ca4790975a4aba8@eucas1p2.samsung.com>
Precedence: bulk

Hi Christian,

On 18.08.2022 14:46, Christian Kohlschütter wrote:
> From: Christian Kohlschütter <christian@kohlschutter.com>
>
> Previously, an unresolved regulator supply reference upon calling
> regulator_register on an always-on or boot-on regulator caused
> set_machine_constraints to be called twice.
>
> This in turn may initialize the regulator twice, leading to voltage
> glitches that are timing-dependent. A simple, unrelated configuration
> change may be enough to hide this problem, only to be surfaced by
> chance.
>
> One such example is the SD-Card voltage regulator in a NanoPI R4S that
> would not initialize reliably unless the registration flow was just
> complex enough to allow the regulator to properly reset between calls.
>
> Fix this by re-arranging regulator_register, trying resolve the
> regulator's supply early enough that set_machine_constraints does not
> need to be called twice.
>
> Signed-off-by: Christian Kohlschütter <christian@kohlschutter.com>

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.

In the kernel log I also noticed lots of the following warnings, which 
were not present before this patch:

[    2.977695] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    2.987715] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    2.997524] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.007560] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.017240] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.026765] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.036501] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.045809] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.055497] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.064765] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.074549] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.085336] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.094815] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.104282] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.113755] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.124446] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.135298] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.144788] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.154562] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!
[    3.164340] debugfs: Directory '(null)-SUPPLY' with parent 
'reg-dummy-regulator-dummy' already present!

They are printed when the main PMIC is being probed and registered. This 
shows that the supplies for some regulators are not properly found, 
probably due to the circular dependencies there ('vdd_1v2' is provided 
by the same PMIC, which use it as a supply for other regulators).

Let me know if I can help debugging this issue further, but for now I 
would suggest reverting this change.

> ---
>   drivers/regulator/core.c | 52 +++++++++++++++++++++++++---------------
>   1 file changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index d8373cb04f9..a5033c6ba01 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5496,7 +5496,39 @@ regulator_register(const struct regulator_desc *regulator_desc,
>   	BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
>   	INIT_DELAYED_WORK(&rdev->disable_work, regulator_disable_work);
>   
> -	/* preform any regulator specific init */
> +	/* set regulator constraints */
> +	if (init_data)
> +		rdev->constraints = kmemdup(&init_data->constraints,
> +					    sizeof(*rdev->constraints),
> +					    GFP_KERNEL);
> +	else
> +		rdev->constraints = kzalloc(sizeof(*rdev->constraints),
> +					    GFP_KERNEL);
> +	if (!rdev->constraints) {
> +		ret = -ENOMEM;
> +		goto clean;
> +	}
> +
> +	if (init_data && init_data->supply_regulator)
> +		rdev->supply_name = init_data->supply_regulator;
> +	else if (regulator_desc->supply_name)
> +		rdev->supply_name = regulator_desc->supply_name;
> +
> +	if ((rdev->supply_name && !rdev->supply) &&
> +			(rdev->constraints->always_on ||
> +			 rdev->constraints->boot_on)) {
> +		/* Try to resolve the name of the supplying regulator here first
> +		 * so we prevent double-initializing the regulator, which may
> +		 * cause timing-specific voltage brownouts/glitches that are
> +		 * hard to debug.
> +		 */
> +		ret = regulator_resolve_supply(rdev);
> +		if (ret)
> +			rdev_dbg(rdev, "unable to resolve supply early: %pe\n",
> +					 ERR_PTR(ret));
> +	}
> +
> +	/* perform any regulator specific init */
>   	if (init_data && init_data->regulator_init) {
>   		ret = init_data->regulator_init(rdev->reg_data);
>   		if (ret < 0)
> @@ -5522,24 +5554,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
>   		    (unsigned long) atomic_inc_return(&regulator_no));
>   	dev_set_drvdata(&rdev->dev, rdev);
>   
> -	/* set regulator constraints */
> -	if (init_data)
> -		rdev->constraints = kmemdup(&init_data->constraints,
> -					    sizeof(*rdev->constraints),
> -					    GFP_KERNEL);
> -	else
> -		rdev->constraints = kzalloc(sizeof(*rdev->constraints),
> -					    GFP_KERNEL);
> -	if (!rdev->constraints) {
> -		ret = -ENOMEM;
> -		goto wash;
> -	}
> -
> -	if (init_data && init_data->supply_regulator)
> -		rdev->supply_name = init_data->supply_regulator;
> -	else if (regulator_desc->supply_name)
> -		rdev->supply_name = regulator_desc->supply_name;
> -
>   	ret = set_machine_constraints(rdev);
>   	if (ret == -EPROBE_DEFER) {
>   		/* Regulator might be in bypass mode and so needs its supply

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland