linux-kernel - Re: [PATCH v2 6/7] arm64: dts: exynos: Add dts file for Exynos5433-based TM2 board

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <57C96297.1080704@samsung.com>
Date:   Fri, 02 Sep 2016 20:29:27 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Javier Martinez Canillas <javier@....samsung.com>,
        k.kozlowski@...sung.com, kgene@...nel.org, robh+dt@...nel.org,
        mark.rutland@....com, catalin.marinas@....com, will.deacon@....com,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     krzk@...nel.org, jh80.chung@...sung.com, sw0312.kim@...sung.com,
        jy0922.shim@...sung.com, inki.dae@...sung.com,
        jonghwa3.lee@...sung.com, beomho.seo@...sung.com,
        jaewon02.kim@...sung.com, human.hwang@...sung.com,
        ideal.song@...sung.com, ingi2.kim@...sung.com,
        m.szyprowski@...sung.com, a.hajda@...sung.com,
        s.nawrocki@...sung.com, chanwoo@...nel.org
Subject: Re: [PATCH v2 6/7] arm64: dts: exynos: Add dts file for
 Exynos5433-based TM2 board

Hi Javier,

On 2016년 08월 27일 03:30, Javier Martinez Canillas wrote:
> Hello Chanwoo,
> 
> The patch looks mostly good to me, I've just some comments:
> 
> [snip]
> 
>> +
>> +&decon {
>> +	status = "okay";
>> +	iommu-reserved-mapping = <0x20000000 0x20000000 0xc0000000>;
>> +
> 
> This property never made to mainline due not having an agreement on
> how this should be fixed properly IIUC [0]. So you should remove it.

OK. I'll remove it.

> 
> [snip]
> 
>> +
>> +	s2mps13-pmic@66 {
>> +		compatible = "samsung,s2mps13-pmic";
>> +		interrupt-parent = <&gpa0>;
>> +		interrupts = <7 IRQ_TYPE_NONE>;
>> +		reg = <0x66>;
>> +		samsung,s2mps11-wrstbi-ground;
>> +
>> +		s2mps13_osc: clocks {
>> +			compatible = "samsung,s2mps13-clk";
>> +			#clock-cells = <1>;
>> +			clock-output-names = "s2mps13_ap", "s2mps13_cp",
>> +				"s2mps13_bt";
>> +		};
>> +
> 
> 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]
> 
>> +			};
>> +
>> +			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.

> 
> [snip]
> 
>> +
>> +			ldo6_reg: LDO6 {
>> +				regulator-name = "VDD10_MIPI2L_1.0V_AP";
>> +				regulator-min-microvolt = <1000000>;
>> +				regulator-max-microvolt = <1000000>;
>> +				regulator-always-on;
> 
> Same question, this is used by both the dsi and usbdrd30 nodes so maybe
> it shouldn't be marked as always-on as well.

OK. I'll remove it.

> 
>> +				regulator-state-mem {
>> +					regulator-off-in-suspend;
>> +				};
>> +			};
>> +
>> +			ldo7_reg: LDO7 {
>> +				regulator-name = "VDD18_MIPI2L_1.8V_AP";
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-always-on;
> 
> This is used by the dsi node as well.

OK. I'll remove it.

> 
> [snip]
> 
>> +
>> +			ldo10_reg: LDO10 {
>> +				regulator-name = "VDD33_USB30_3.0V_AP";
>> +				regulator-min-microvolt = <3000000>;
>> +				regulator-max-microvolt = <3000000>;
>> +				regulator-always-on;
> 
> Use by the usbdrd30 node.

OK. I'll remove it.

> 
> [snip]
> 
>> +
>> +			ldo18_reg: LDO18 {
>> +				regulator-name = "V_CODEC_1.8V_AP";
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <1800000>;
>> +				regulator-always-on;
> 
> Use by the wm5110-codec node.

OK. I'll remove it.

> 
> [snip]
> 
>> +
>> +			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.

> 
>> +				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.

> 
>> +				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.

> 
> Reviewed-by: Javier Martinez Canillas <javier@....samsung.com>

Thanks for your review.

> 
> [0]: http://www.spinics.net/lists/arm-kernel/msg419747.html
> 
> Best regards,
> 
-- 
Best Regards,
Chanwoo Choi