Received-SPF: pass (google.com: domain of linux-kernel+bounces-208923-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <62c9b99d-f0a4-4207-84b9-2176823b9724@salutedevices.com>
Date: Tue, 11 Jun 2024 00:33:42 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 0/1] pwm-regulator with voltage table problem
To: Mark Brown <broonie@kernel.org>
CC: <lgirdwood@gmail.com>, <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <kernel@salutedevices.com>
References: <20240610120025.405062-1-gnstark@salutedevices.com>
 <Zmcdvm7XbtU1JlQr@finisterre.sirena.org.uk>
Content-Language: en-US
From: George Stark <gnstark@salutedevices.com>
In-Reply-To: <Zmcdvm7XbtU1JlQr@finisterre.sirena.org.uk>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

Hello Mark

Thanks for the review. Sorry about the separate cover-letter.

On 6/10/24 18:37, Mark Brown wrote:
> On Mon, Jun 10, 2024 at 03:00:24PM +0300, George Stark wrote:
> 
>> The situation: bootloader sets mean cpu power and mean cpu clock.
>> but that cpu power is not found in the voltage table (value is between table items)
>> due to different versions of bootloader and kernel and the regulator core sets
>> the minimal power but cpu clock stays the same. CPU hangs somewhere during boot.
> 
> Why not just add this OPP to the table the kernel knows about?  Clearly
> it's something the vendor set and presumably thinks the device can
> actually operate at.  As far as I can tell you're only having problems
> here because you've got a software defined regulator but haven't given
> the software information about this configuration so it's got no idea
> what's going on when bootstrapping.

Actually we did a similar thing - we modified voltage table adding 
duty-cycle that was set by bootloader with fractional voltage value that 
is not used in opp table - just to make pwm-regulator happy.
But this issue was very hard to find. Due to deviations of hardware 
component's characteristics some devices managed to keep working with 
minimal voltage till cpu opp driver got probed and appropriate voltage 
is restored. Other devices got stuck at different places with random errors.

> 
>> The core problem as I see it is if regulator is bound to CPU (or some other
>> complex consumer) it can't be changed except by the consumer at any stage. So
>> the regulator driver (core part) should wait for the own consumer to init
>> it properly but regulator can't be in unknown state after probing.
> 
> If the regulator is configured outside the constraints configured for it
> in the binding then the core will bring the regulator within those
> constraints, some systems with regulator configurations fixed in
> silicon rely on this for correct performance.  Regulators with
> unreadable hardware are very much an edge case when it comes to this,
> what works for one system will be broken for another one so we just have
> to pick a behaviour that will hopefully work more often than it breaks.
> We can't rely on consumers setting a voltage since consumers are only
> expected to set a voltage if they are actively managing it at runtime,
> other consumers should rely on system configuration.

Of course such a behavior should be configurable. At the other hand it 
may be too much changes for a corner case that's why I proposed only a
warning patch just to simplify detecting the problem.

Actually we already have a hint that says voltage is reset:
rdev_info(rdev, "Setting %d-%duV\n",
				  rdev->constraints->min_uV,
				  rdev->constraints->max_uV);
but there's no indication this is due to regulator device  error.

Should I consider adding my warning only for "system-critical-regulator" 
regulators (cpu power regulator is critical indeed)? Although this 
property is never used in mainline bindings.

-- 
Best regards
George