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

[Stephan Gerhold]

On Fri, May 26, 2023 at 11:11:44PM +0200, Konrad Dybcio wrote:
> 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
> 

Yeah I was also a bit torn between (pre-)defining labels for all
regulators vs the approach I chose in this patch. However, thinking
about it some more I realized that having the pre-defined labels
makes it far too easy to make another fairly hidden mistake:

Let's assume we would define labels for all regulators in
msm8916-pm8916.dtsi. That is, even for board-specific regulators
we add:

	/* ... */
	pm8916_l10: l10 {};
	/* ... */
	pm8916_l14: l14 {};
	pm8916:l15: l15 {};
	pm8916_l16: l16 {};
	/* ... */

Now someone new to device porting thinks "c'mon, gettin' camera workin'
can't be *that* hard". They look at downstream and quickly define a
device tree node with the necessary regulators:

	camera@1a {
		compatible = "sony,imx214";
		reg = <0x1a>;
		vddo-supply = <&pm8916_l10>;
		vdda-supply = <&pm8916_l16>;
		vddd-supply = <&pm8916_l2>;
		/* ... */
	};

They build this successfully and try to run it. Perhaps it even works
somewhat but it's quite dangerous: They did not define any voltage
constraints for l10 and l16! In this case, the regulator core does not
allow *changing* the regulator voltage, but it still allows
*enabling*/disabling the regulator with "who knows what RPM uses as
default voltage". They might even submit it upstream and reviewers
assume the voltages are already defined somewhere.

This cannot happen with my patch. The labels for the board-specific
regulators are not defined anywhere, so they would immediately get a
build error. They would probably look for examples how to define the
additional regulators. For pm8916_l16 they find examples like:

	&pm8916_rpm_regulators {
		pm8916_l16: l16 {
			regulator-min-microvolt = <1800000>;
			regulator-max-microvolt = <1800000>;
		};
	};

They copy this into their own board DT but (hopefully) wonder "Is this
actually the correct voltage for my case?". They look closer at the
downstream camera setup and see that vdda/l16 actually needs 2.7V.

For each additional regulator they need to actively add something and
think about the changes before it will build successfully.

I believe that in this case being unaware of additional required changes
is far more likely than making "fat finger" mistakes. Regulators is
simply something one needs to be careful about. Even if we add
fail-safes for regulator name typos, you could just as easily have typos
in the specified voltages. I always look twice before testing regulator
changes and would hope this applies to most people. :)

Thanks,
Stephan