[PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Bjorn Andersson]

PMS405 S5 was upstreamed without a voltage and PMS405 L3 is outside the
acceptable range, causing PCIe to fail. Fix these.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/boot/dts/qcom/qcs404-evb.dtsi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
index 579ddaf4f5fa..072061aa1b79 100644
--- a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
@@ -72,8 +72,8 @@
 		};
 
 		vreg_s5_1p35: s5 {
-			regulator-min-microvolt = <>;
-			regulator-max-microvolt = <>;
+			regulator-min-microvolt = <1352000>;
+			regulator-max-microvolt = <1352000>;
 		};
 
 		vreg_l1_1p3: l1 {
@@ -87,7 +87,7 @@
 		};
 
 		vreg_l3_1p05: l3 {
-			regulator-min-microvolt = <976000>;
+			regulator-min-microvolt = <1050000>;
 			regulator-max-microvolt = <1160000>;
 		};
 
-- 
2.18.0

Re: [PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Jorge Ramirez]

On 1/26/19 00:29, Bjorn Andersson wrote:
> PMS405 S5 was upstreamed without a voltage and PMS405 L3 is outside the
> acceptable range, causing PCIe to fail. Fix these.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qcs404-evb.dtsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
> index 579ddaf4f5fa..072061aa1b79 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
> @@ -72,8 +72,8 @@
>  		};
>  
>  		vreg_s5_1p35: s5 {
> -			regulator-min-microvolt = <>;
> -			regulator-max-microvolt = <>;
> +			regulator-min-microvolt = <1352000>;
> +			regulator-max-microvolt = <1352000>;
>  		};
>  
>  		vreg_l1_1p3: l1 {
> @@ -87,7 +87,7 @@
>  		};
>  
>  		vreg_l3_1p05: l3 {
> -			regulator-min-microvolt = <976000>;
> +			regulator-min-microvolt = <1050000>;


the linear range for this regulator is
 - REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),

meaning that 1050000 is actually not a valid selectable value (ie, after
applying the above constrains 1056000 would be selected instead)

In order for a driver to be able to successfully request min = 1050000,
regulator-min-microvolt should be set to 1048000 (and 1056000 would be
applied)

the question is, should this property contain only hardware achievable
values? or should drivers only request hardware achievable values? the
way the constrains are implemented it has to be one of the two (I think
the former would be more intuitive - ie if the dts
regulator-min-microvolt is a valid value)

>  			regulator-max-microvolt = <1160000>;
>  		};
>  
> 

Re: [PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Niklas Cassel]

Adding Mark Brown on CC.

On Tue, Jan 29, 2019 at 10:58:47PM +0100, Jorge Ramirez wrote:
> On 1/26/19 00:29, Bjorn Andersson wrote:
> > PMS405 S5 was upstreamed without a voltage and PMS405 L3 is outside the
> > acceptable range, causing PCIe to fail. Fix these.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/qcs404-evb.dtsi | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
> > index 579ddaf4f5fa..072061aa1b79 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/qcs404-evb.dtsi
> > @@ -72,8 +72,8 @@
> >  		};
> >  
> >  		vreg_s5_1p35: s5 {
> > -			regulator-min-microvolt = <>;
> > -			regulator-max-microvolt = <>;
> > +			regulator-min-microvolt = <1352000>;
> > +			regulator-max-microvolt = <1352000>;
> >  		};
> >  
> >  		vreg_l1_1p3: l1 {
> > @@ -87,7 +87,7 @@
> >  		};
> >  
> >  		vreg_l3_1p05: l3 {
> > -			regulator-min-microvolt = <976000>;
> > +			regulator-min-microvolt = <1050000>;
> 
> 
> the linear range for this regulator is
>  - REGULATOR_LINEAR_RANGE(312000, 0, 127, 8000),
> 
> meaning that 1050000 is actually not a valid selectable value (ie, after
> applying the above constrains 1056000 would be selected instead)
> 
> In order for a driver to be able to successfully request min = 1050000,
> regulator-min-microvolt should be set to 1048000 (and 1056000 would be
> applied)
> 
> the question is, should this property contain only hardware achievable
> values? or should drivers only request hardware achievable values? the
> way the constrains are implemented it has to be one of the two (I think
> the former would be more intuitive - ie if the dts
> regulator-min-microvolt is a valid value)
> 
> >  			regulator-max-microvolt = <1160000>;
> >  		};
> >  
> > 
> 

Re: [PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]

On Tue, Jan 29, 2019 at 11:46:52PM +0100, Niklas Cassel wrote:

> Adding Mark Brown on CC.

It really helps if you ask a specific question when doing something like
this rather than just have a big quoted mail - it makes it much easier
to find what's relevant rather than trying to find things, especially
when they're buried behind several layers of quoting.

> On Tue, Jan 29, 2019 at 10:58:47PM +0100, Jorge Ramirez wrote:
> > On 1/26/19 00:29, Bjorn Andersson wrote:

> > the question is, should this property contain only hardware achievable
> > values? or should drivers only request hardware achievable values? the
> > way the constrains are implemented it has to be one of the two (I think
> > the former would be more intuitive - ie if the dts
> > regulator-min-microvolt is a valid value)

Drivers should not be coded with a specific regulator or board in mind
and should just request whatever they need.  This will then be matched
with whatever the board is actually able to deliver.  Similarly there is
no requirement that machine constraints be written with specific
reference to what the physical regulator on the board is able to do, for
example the constraints will come from electrical engineering
restrictions like the specifications of the parts connected to the
regulator rather than from what the regulator can actually do so people
should feel free to just write down the actual physical constraint and
let the regulator API ensure that the constraint is met.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Bjorn Andersson]

On Mon 04 Feb 02:43 PST 2019, Mark Brown wrote:

> On Tue, Jan 29, 2019 at 11:46:52PM +0100, Niklas Cassel wrote:
> 
> > Adding Mark Brown on CC.
> 
> It really helps if you ask a specific question when doing something like
> this rather than just have a big quoted mail - it makes it much easier
> to find what's relevant rather than trying to find things, especially
> when they're buried behind several layers of quoting.
> 
> > On Tue, Jan 29, 2019 at 10:58:47PM +0100, Jorge Ramirez wrote:
> > > On 1/26/19 00:29, Bjorn Andersson wrote:
> 
> > > the question is, should this property contain only hardware achievable
> > > values? or should drivers only request hardware achievable values? the
> > > way the constrains are implemented it has to be one of the two (I think
> > > the former would be more intuitive - ie if the dts
> > > regulator-min-microvolt is a valid value)
> 
> Drivers should not be coded with a specific regulator or board in mind
> and should just request whatever they need.  This will then be matched
> with whatever the board is actually able to deliver.  Similarly there is
> no requirement that machine constraints be written with specific
> reference to what the physical regulator on the board is able to do, for
> example the constraints will come from electrical engineering
> restrictions like the specifications of the parts connected to the
> regulator rather than from what the regulator can actually do so people
> should feel free to just write down the actual physical constraint and
> let the regulator API ensure that the constraint is met.

We have a regulator that is described as 1.05V in the schematics for the
board we're working on and we have the USB block wanting 1.05V on one of
its pins.  But the particular regulator works in steps of 8mV, and the
adjacent steps are 1.048V and 1.056V.

A) If we describe the min-microvolt = max-microvolt = 1.05V then the
regulator frameworks adjusts the min value to 1.056V, per the steps, and
fail as min > max.

B) The USB driver that we inherited was written to request min/max of
1.05V/1.05V, so pointing this to a regulator with min/max of e.g.
1.05V/1.06V we're outside the adjusted range of 1.056V/1.06V.


So the question is, should the board dts be written with
min/max-microvolt adjusted to match the hardware steps? Or could the
regulator framework be made to round down to the previous valid step
instead of up?

Regards,
Bjorn

Re: [PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]

On Mon, Feb 04, 2019 at 08:03:37AM -0800, Bjorn Andersson wrote:

> We have a regulator that is described as 1.05V in the schematics for the
> board we're working on and we have the USB block wanting 1.05V on one of
> its pins.  But the particular regulator works in steps of 8mV, and the
> adjacent steps are 1.048V and 1.056V.

> A) If we describe the min-microvolt = max-microvolt = 1.05V then the
> regulator frameworks adjusts the min value to 1.056V, per the steps, and
> fail as min > max.

Right, because that constraint isn't satisifiable on the board, an exact
value has been asked for which can't be delivered.  We *could* try to
allow some fudge factor for tolerance but that feels risky, it'd be
better if the constraints were written to be satisfiable.

> B) The USB driver that we inherited was written to request min/max of
> 1.05V/1.05V, so pointing this to a regulator with min/max of e.g.
> 1.05V/1.06V we're outside the adjusted range of 1.056V/1.06V.

This is just very bad practice on the part of the USB driver, if it is
not varying the voltage it should not be setting the voltage and let the
machine figure things out.  It is completely pointless for the driver to
be setting an exact value that it never varies, this is the sort of
thing machine constraints are for as it leads to trouble like this.
This is one of the many unfortunate practices in the Qualcomm BSP code
sadly.

> So the question is, should the board dts be written with
> min/max-microvolt adjusted to match the hardware steps? Or could the

Well, it is definitely unwise of the board to request a specific
voltage if the board can't physically set it, that's asking for trouble,
so I'd have expected the board should pick the value it wants.

> regulator framework be made to round down to the previous valid step
> instead of up?

We definitely don't want to round voltages down, it is vastly more
common for devices to experience problems like brownouts if they go
under voltage so it'd be more likely to cause harm than good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Bjorn Andersson]

On Mon 04 Feb 10:23 PST 2019, Mark Brown wrote:

> On Mon, Feb 04, 2019 at 08:03:37AM -0800, Bjorn Andersson wrote:
> 
> > We have a regulator that is described as 1.05V in the schematics for the
> > board we're working on and we have the USB block wanting 1.05V on one of
> > its pins.  But the particular regulator works in steps of 8mV, and the
> > adjacent steps are 1.048V and 1.056V.
> 
> > A) If we describe the min-microvolt = max-microvolt = 1.05V then the
> > regulator frameworks adjusts the min value to 1.056V, per the steps, and
> > fail as min > max.
> 
> Right, because that constraint isn't satisifiable on the board, an exact
> value has been asked for which can't be delivered.  We *could* try to
> allow some fudge factor for tolerance but that feels risky, it'd be
> better if the constraints were written to be satisfiable.
> 

Okay, I don't see a big problem with expecting the DT to only contain
values that the hardware can actually do. I do however see that
machine_constraints_voltage() has been changes to expect the driver to
return -ENOTRECOVERABLE when we don't have a way to query the voltage
during probe.

Updating the qcom_smd-regulator driver to do this causes this regulator
to not be probed as we now have:
[    0.199911] l3: Setting 1050000-1050000uV^M
[    0.199927] l3: unsupportable voltage constraints 1056000-1048000uV^M

So there's a risk of this breaking compatibility with older dtb files on
other boards. We will have to do some more verification on this.

But we can start by making sure we're specifying valid constraints for
the hardware.

> > B) The USB driver that we inherited was written to request min/max of
> > 1.05V/1.05V, so pointing this to a regulator with min/max of e.g.
> > 1.05V/1.06V we're outside the adjusted range of 1.056V/1.06V.
> 
> This is just very bad practice on the part of the USB driver, if it is
> not varying the voltage it should not be setting the voltage and let the
> machine figure things out.  It is completely pointless for the driver to
> be setting an exact value that it never varies, this is the sort of
> thing machine constraints are for as it leads to trouble like this.
> This is one of the many unfortunate practices in the Qualcomm BSP code
> sadly.
> 

We're taking care of this, but I spotted the min = max =
non-supported-value issue on SDM845 recently as well and wanted to know
which path to take.

> > So the question is, should the board dts be written with
> > min/max-microvolt adjusted to match the hardware steps? Or could the
> 
> Well, it is definitely unwise of the board to request a specific
> voltage if the board can't physically set it, that's asking for trouble,
> so I'd have expected the board should pick the value it wants.
> 

Agreed, and if we can make the initial get_voltage fail with
-ENOTRECOVERABLE then we get a proper failure on an invalid DT, so I'll
review the other platforms using this driver to see if we can introduce
this.

> > regulator framework be made to round down to the previous valid step
> > instead of up?
> 
> We definitely don't want to round voltages down, it is vastly more
> common for devices to experience problems like brownouts if they go
> under voltage so it'd be more likely to cause harm than good.

We're 2mV off in this case, but it could have been way off. So I'm good
with this position.

Thanks Mark

Regards,
Bjorn

Re: [PATCH] arm64: dts: qcs404: evb: Fix voltages for s5 and l3

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

On Mon, Feb 04, 2019 at 11:25:10AM -0800, Bjorn Andersson wrote:
> On Mon 04 Feb 10:23 PST 2019, Mark Brown wrote:
> > On Mon, Feb 04, 2019 at 08:03:37AM -0800, Bjorn Andersson wrote:

> Okay, I don't see a big problem with expecting the DT to only contain
> values that the hardware can actually do. I do however see that
> machine_constraints_voltage() has been changes to expect the driver to
> return -ENOTRECOVERABLE when we don't have a way to query the voltage
> during probe.

> So there's a risk of this breaking compatibility with older dtb files on
> other boards. We will have to do some more verification on this.

That's basically only going to affect Qualcomm hardware FWIW, I had
thought that such devices didn't manage to probe with upstream before
that change either but I could be misremembering.

> > > regulator framework be made to round down to the previous valid step
> > > instead of up?

> > We definitely don't want to round voltages down, it is vastly more
> > common for devices to experience problems like brownouts if they go
> > under voltage so it'd be more likely to cause harm than good.

> We're 2mV off in this case, but it could have been way off. So I'm good
> with this position.

Never mind that that's 2mV assuming the regulator has perfect accuracy
and there's headroom.  For something like this it's likely not a problem
but some high current things (especially CPUs or anything like that) are
operating on thin margins.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]