Hi Marco,

On 03.12.19 09:33, Marco Felsch wrote:

Hi Stefan,

On 19-12-03 09:11, Stefan Riedmüller wrote:

quoted

Hi Marco,

On 02.12.19 15:53, Marco Felsch wrote:

quoted

On 19-12-02 15:30, Stefan Riedmüller wrote:

quoted

Hi Marco,

On 02.12.19 15:14, Marco Felsch wrote:

quoted

Hi Stefan,

On 19-12-02 14:55, Stefan Riedmüller wrote:

quoted

Hi Marco,

On 02.12.19 13:42, Marco Felsch wrote:

quoted

Hi Stefan,

On 19-12-02 11:09, Stefan Riedmüller wrote:

quoted

Hi Marco,

your proposed setting is only valid for the LDO enabled case but not for the
case where the LDO's are in bypass mode. Is that intended? In bypass mode it
actually needs to be 0.925 V min for ARM and 1.15 V min for SOC.

The case is that the driver doesn't support the bypass mode currently so
yes it was intended.

Ok, I see.

quoted

quoted

Did you experience an issue with the current settings or is this just a
cosmetic change?

There is currently no issue because the internally LDO's don't try to
apply such a low voltage value. But I think it isn't a cosmetic change
because this value is wrong. We need to specify the valid voltage range.

Please correct me if I'm wrong, but isn't the regulator-min and
regulator-max values supposed to reflect the min and max values this
regulator can deliver?

Nope, the constraints are hard coded within the driver e.g. da9062: >
8<-----------------------------------------------------------------
/* Regulator operations */

/* Current limits array (in uA)
    * - DA9061_ID_[BUCK1|BUCK3]
    * - DA9062_ID_[BUCK1|BUCK2|BUCK4]
    * Entry indexes corresponds to register values.
    */
static const unsigned int da9062_buck_a_limits[] = {
	500000,  600000,  700000,  800000,  900000, 1000000,
	1100000, 1200000,
	1300000, 1400000, 1500000, 1600000, 1700000,
	1800000, 1900000, 2000000
};

/* Current limits array (in uA)
    * - DA9061_ID_BUCK2
    * - DA9062_ID_BUCK3
    * Entry indexes corresponds to register values.
    */
static const unsigned int
da9062_buck_b_limits[] = {
	1500000, 1600000, 1700000, 1800000,
	1900000, 2000000, 2100000, 2200000,
	2300000, 2400000, 2500000,
	2600000, 2700000, 2800000,
	2900000, 3000000
};

8<-----------------------------------------------------------------

These are the available current limits for the buck regulators. I don't see
where they correspond to the min/max settable output voltage. Maybe I missed
something?

Please check the following structs:

   - static const struct da9062_regulator_info local_da9061_regulator_info[]
   - static const struct da9062_regulator_info local_da9062_regulator_info[]

There you have the min_uV, uV_step, n_voltages so the core can validate
if the dt-value is within the range.

Thanks, that makes more sense.

quoted

quoted

The regulator bindings state:
- regulator-min-microvolt: smallest voltage consumers may set

- regulator-max-microvolt: largest voltage consumers may set

Yes and according the datasheet I mentionied the current values aren't
correct.

quoted

For me that is device depended and not design depended.

What is the scenario you're thinking about which would cause the SOC, as a
consumer, to request a lower voltage as it needs?

The thing is that the DT abstracts the HW and these values are not
correct. As mentioned in my commit message the values should meet
the datasheet restrictions and this isn't the case yet.

I don't agree. The datasheet you mention is the i.MX 6 datasheet and thus
the limitation should reside in the i.MX 6 regulators and not in the PMIC's.
This limitation is not just valid in combination with that PMIC but for all
i.MX 6.

The datasheet tells you which voltage should be applied to the imx6 and
so you have to set this here. What happens if the internally ldo
request a voltage value below 0.9V? Then the value will be applied
because we specified 0.73V and the system don't work anymore or did you
verified that case?

The LDO should not be able to request a too low voltage since the values 
from the i.MX 6 datasheet need to be applied to the LDO regulator and thus 
preventing the LDO from requesting this too low voltage. Why just fix this 
for this particular case with this one PMIC when it actually could be fixed 
for all i.MX 6 PMIC combinations? These limitations are fixed limitations 
from the i.MX 6 datasheet and do not result from the PMIC + i.MX 6 combination.

quoted

If I have this wrong and the maintainers agree with you could you please
make sure to account for the bypass mode as well since these values from the
datasheet are valid too?

As I said, the bypass mode isn't supported by the driver and all imx6
based devicetrees follow that case. So we don't have to take that into
account. Also we can't meet both contraints with one dt and futhermore
the bypass mode decrease your imx6 lifetime due the the increased ripple
on the arm-core supply. So I think no one wants this setup in the near
future.

Ok.

Regards,
Stefan

Regards,
   Marco

quoted

Regards,
Stefan

quoted

Regards,
    Marco

quoted

Regards,
Stefan

quoted

So you have to specify the min/max voltage for your design.

Regards,
     Marco

quoted

Maybe your change is better placed in the anatop regulators. Btw they also
have a 0.725 V minimum voltage:

   From arch/arm/boot/dts/imx6qdl.dtsi:

                                   reg_arm: regulator-vddcore {

                                           compatible = "fsl,anatop-regulator";
                                           regulator-name = "vddarm";

                                           regulator-min-microvolt = <725000>;

                                           regulator-max-microvolt = <1450000>;
                                           regulator-always-on;

                                           anatop-reg-offset = <0x140>;

                                           anatop-vol-bit-shift = <0>;

                                           anatop-vol-bit-width = <5>;

                                           anatop-delay-reg-offset = <0x170>;

                                           anatop-delay-bit-shift = <24>;

                                           anatop-delay-bit-width = <2>;

                                           anatop-min-bit-val = <1>;

                                           anatop-min-voltage = <725000>;

                                           anatop-max-voltage = <1450000>;

                                   };


Regards,
Stefan

quoted

Regards,
      Marco

quoted

Regards,
Stefan


On 29.11.19 17:48, Marco Felsch wrote:

quoted

The current set minimum voltage of 730000mV seems to be wrong. I don't
know the document which specifies that but the imx6qdl datasheets says
that the minimum voltage should be 1.05V for VDD_ARM (LDO enabled, lowest
opp) and 1.275V for VDD_SOC (LDO enabled, lowest opp).

Fixes: ddec5d1c0047 ("ARM: dts: imx6: Add initial support for phyCORE-i.MX 6 SOM")
Signed-off-by: Marco Felsch <redacted>
---
      arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi | 4 ++--
      1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
index 6486df3e2942..46d4953c5588 100644
--- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi

@@ -107,14 +107,14 @@
      		regulators {
      			vdd_arm: buck1 {
      				regulator-name = "vdd_arm";
-				regulator-min-microvolt = <730000>;
+				regulator-min-microvolt = <1050000>;
      				regulator-max-microvolt = <1380000>;
      				regulator-always-on;
      			};
      			vdd_soc: buck2 {
      				regulator-name = "vdd_soc";
-				regulator-min-microvolt = <730000>;
+				regulator-min-microvolt = <1275000>;
      				regulator-max-microvolt = <1380000>;
      				regulator-always-on;
      			};

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel