Re: [PATCH 7/8] arm64: dts: qcom: msm8916: Define regulator constraints next to usage

[Konrad Dybcio]

On 26.05.2023 08:47, Stephan Gerhold wrote:
> On Fri, May 26, 2023 at 01:35:06AM +0200, Konrad Dybcio wrote:
>> On 17.05.2023 20:48, Stephan Gerhold wrote:
>>> Right now each MSM8916 device has a huge block of regulator constraints
>>> with allowed voltages for each regulator. For lack of better
>>> documentation these voltages are often copied as-is from the vendor
>>> device tree, without much extra thought.
>>>
>>> Unfortunately, the voltages in the vendor device trees are often
>>> misleading or even wrong, e.g. because:
>>>
>>>  - There is a large voltage range allowed and the actual voltage is
>>>    only set somewhere hidden in some messy vendor driver. This is often
>>>    the case for pm8916_{l14,l15,l16} because they have a broad range of
>>>    1.8-3.3V by default.
>>>
>>>  - The voltage is actually wrong but thanks to the voltage constraints
>>>    in the RPM firmware it still ends up applying the correct voltage.
>>>
>>> To have proper regulator constraints it is important to review them in
>>> context of the usage. The current setup in the MSM8916 device trees
>>> makes this quite hard because each device duplicates the standard
>>> voltages for components of the SoC and mixes those with minor
>>> device-specific additions and dummy voltages for completely unused
>>> regulators.
>>>
>>> The actual usage of the regulators for the SoC components is in
>>> msm8916-pm8916.dtsi, so it can and should also define the related
>>> voltage constraints. These are not board-specific but defined in the
>>> APQ8016E/PM8916 Device Specification. The board DT can then focus on
>>> describing the actual board-specific regulators, which makes it much
>>> easier to review and spot potential mistakes there.
>>>
>>> Note that this commit does not make any functional change. All used
>>> regulators still have the same regulator constraints as before. Unused
>>> regulators do not have regulator constraints anymore because most of
>>> these were too broad or even entirely wrong. They should be added back
>>> with proper voltage constraints when there is an actual usage.
>>>
>>> Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
>>> ---
>> I'm a bit torn between saying "this is very nice already" and "we should
>> probably override each regulator individually" like so:
>>
>> &pm8916_l17 {
>> 	[...]
>> }
>>
>> to minimize mistakes..
>>
>> Not sure what to make of it, I see Bjorn already applied this, so I guess
>> I'm just leaving some potential ideas for the future here.
>>
> 
> Sorry, could you elaborate a bit on what changes you would make exactly?
Assigning the voltage ranges through direct reference to each individual
regulator, instead of overwriting them through referencing the
pm8916_rpm_regulators label and (essentially) redefining them.

> 
> The way it works in this patch is that regulators that are used by the
> SoC are defined in msm8916-pm8916.dtsi. All other (board-specific)
> regulators must be defined together with proper voltages in the board DT.
> 
> What kind of mistake are you thinking of?
Fat fingers, mostly

So suppose your device needs a different voltage on L18, so you do

&pm8916_rpm_regulators {
	l19 { //fat fingers burn devices
		regulator-min-microvolt = <12341234>;
		regulator-max-microvolt = <43143144>;
	};
};

DTC will happily eat that


since we use labels, one would have to fatfinger twice, like so:
&pm8916_rpm_regulators {
	pm8916_l19: l19 { //this was still supposed to be l18
...


as these two combinations will trigger a build error


&pm8916_rpm_regulators {
	pm8916_l19: l18 { //duplicate label vs actual l19

---

&pm8916_rpm_regulators {
	pm8916_l18: l19 { //duplicate label vs actual l18


Konrad

> 
> Thanks,
> Stephan