Hello Chanwoo,

On 09/02/2016 01:29 PM, Chanwoo Choi wrote:

Hi Javier,

On 2016? 08? 27? 03:30, Javier Martinez Canillas wrote:

quoted

Hello Chanwoo,

[snip]

quoted

quoted

+

I see that most of the following regulators are marked as always-on
but I wonder if this is really needed. For example some of them are
looked up by consumer devices.

[snip]

quoted

+			};
+
+			ldo3_reg: LDO3 {
+				regulator-name = "VDD1_E_1.8V_AP";
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-always-on;
+			};

This is used by both the ADC and the TMU so I guess it should be safe
to not mark it as always-on (unless is used by other critical IP block
not described in the DT).

This regulator should be always ON state.
This regulator provides the voltage to ALIVE domain of Exynos5433.

Ok, that's what I wanted to know since I don't have a schematics of this board.

Maybe add a comment explaining why should be always-on for all regulators that
need it?

[snip]

quoted

quoted

+
+			buck2_reg: BUCK2 {
+				regulator-name = "VDD_EGL_1.0V_AP";

I wonder if this shouldn't be "VDD_ATL_1.0V_AP" or something since
the big cluster isn't called Eagle like in arm32 Exynos but Atlas?

I used the regulator's name according to TM2's schematic.
As I knew, Eagle means the big cores.

I see, as I mentioned I don't have a TM2 schematic, but the Exynos5433 SoC
manual doesn't refer the big and LITTLE cores as Eagle and King Fisher but
as Atlas and Apollo.

In any case, I think that makes sense to match what is in the schematic so
I agree with you to use whatever is there regardless if matches the manual
or not.

quoted

quoted

+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <1300000>;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+
+			buck3_reg: BUCK3 {
+				regulator-name = "VDD_KFC_1.0V_AP";

Same, maybe using "VDD_APL_1.0V_AP" since the big cluster is Apollo?

ditto.
The KFC (King Fisher) means the little cores.

quoted

quoted

+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <1200000>;
+				regulator-always-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
+			};
+

Used by the big and LITTLE clusters respectively, although for these two
I'm not that sure if it would be safe to remove the always-on property.

quoted

Reviewed-by: Javier Martinez Canillas <redacted>

Thanks for your review.

quoted

[0]: http://www.spinics.net/lists/arm-kernel/msg419747.html

Best regards,

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America