| 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
| ||
|
Message-ID: <CAGXv+5G2kc5vkAuMC2JqLRSdjCeHMCpu8_E7DccJd2VoCqY1VQ@mail.gmail.com>
Date: Mon, 21 Oct 2024 11:45:36 +0800
From: Chen-Yu Tsai <wenst@...omium.org>
To: Albert Jakieła <jakiela@...gle.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH] arm64: dts: mt8186: Update regulators voltages
On Fri, Oct 18, 2024 at 5:44 PM Albert Jakieła <jakiela@...gle.com> wrote:
>
> Update minimum and maximum voltages and add
> missing regulators.
>
> Signed-off-by: Albert Jakieła <jakiela@...gle.com>
> ---
> .../boot/dts/mediatek/mt8186-corsola.dtsi | 107 +++++++++++-------
> 1 file changed, 63 insertions(+), 44 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
> index 682c6ad2574d..62158eac45d0 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola.dtsi
> @@ -1302,8 +1302,8 @@ mt6366_regulators: regulators {
>
> vcore {
> regulator-name = "pp0750_dvdd_core";
> - regulator-min-microvolt = <550000>;
> - regulator-max-microvolt = <800000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
No. The voltage range has been correctly limited to the consumer's range.
The voltage constraints are supposed to be the voltage range supported
by all its consumers, not what the regulator can actually provide. The
latter is already implied by the compatible string and the regulator
node name.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <200>;
> regulator-allowed-modes = <MT6397_BUCK_MODE_AUTO
> @@ -1313,8 +1313,8 @@ vcore {
>
> mt6366_vdram1_reg: vdram1 {
> regulator-name = "pp1125_emi_vdd2";
> - regulator-min-microvolt = <1125000>;
> - regulator-max-microvolt = <1125000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <2087500>;
Same thing about the voltage ranges.
> regulator-ramp-delay = <12500>;
> regulator-enable-ramp-delay = <0>;
> regulator-allowed-modes = <MT6397_BUCK_MODE_AUTO
> @@ -1322,6 +1322,16 @@ mt6366_vdram1_reg: vdram1 {
> regulator-always-on;
> };
>
> + mt6366_vpa_reg: vpa {
> + regulator-name = "ppvar_dvdd_vpa";
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <3650000>;
> + regulator-ramp-delay = <50000>;
> + regulator-enable-ramp-delay = <250>;
> + regulator-allowed-modes = <MT6397_BUCK_MODE_AUTO
> + MT6397_BUCK_MODE_FORCE_PWM>;
> + };
> +
This regulator is not used in the design and was purposefully omitted
when the board was upstreamed.
> mt6366_vgpu_reg: vgpu {
> /*
> * Called "ppvar_dvdd_gpu" in the schematic.
> @@ -1330,19 +1340,17 @@ mt6366_vgpu_reg: vgpu {
> */
> regulator-name = "ppvar_dvdd_vgpu";
> regulator-min-microvolt = <500000>;
> - regulator-max-microvolt = <950000>;
> + regulator-max-microvolt = <1293750>;
Why? The datasheet has a lower value and the OPPs only go up to 0.95V.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <200>;
> regulator-allowed-modes = <MT6397_BUCK_MODE_AUTO
> MT6397_BUCK_MODE_FORCE_PWM>;
> - regulator-coupled-with = <&mt6366_vsram_gpu_reg>;
> - regulator-coupled-max-spread = <10000>;
Definitely not. Please read
https://lore.kernel.org/all/20230301095523.428461-1-angelogioacchino.delregno@collabora.com/
and understand that things have been solved differently compared to the
ChromeOS downstream kernel.
> };
>
> mt6366_vproc11_reg: vproc11 {
> regulator-name = "ppvar_dvdd_proc_bc_mt6366";
> - regulator-min-microvolt = <600000>;
> - regulator-max-microvolt = <1200000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
Same thing about the voltage ranges.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <200>;
> regulator-allowed-modes = <MT6397_BUCK_MODE_AUTO
> @@ -1352,8 +1360,8 @@ mt6366_vproc11_reg: vproc11 {
>
> mt6366_vproc12_reg: vproc12 {
> regulator-name = "ppvar_dvdd_proc_lc";
> - regulator-min-microvolt = <600000>;
> - regulator-max-microvolt = <1200000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
Same thing about the voltage ranges.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <200>;
> regulator-allowed-modes = <MT6397_BUCK_MODE_AUTO
> @@ -1361,10 +1369,21 @@ mt6366_vproc12_reg: vproc12 {
> regulator-always-on;
> };
>
> + mt6366_vmodem_reg: vmodem {
> + regulator-name = "ppvar_vmodem";
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
> + regulator-ramp-delay = <6250>;
> + regulator-enable-ramp-delay = <900>;
> + regulator-allowed-modes = <MT6397_BUCK_MODE_AUTO
> + MT6397_BUCK_MODE_FORCE_PWM>;
> + regulator-always-on;
> + };
> +
This regulator is not used in the design and was purposefully omitted
when the board was upstreamed.
> mt6366_vs1_reg: vs1 {
> regulator-name = "pp2000_vs1";
> - regulator-min-microvolt = <2000000>;
> - regulator-max-microvolt = <2000000>;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <2587500>;
Same thing about the voltage ranges.
> regulator-ramp-delay = <12500>;
> regulator-enable-ramp-delay = <0>;
> regulator-always-on;
> @@ -1372,8 +1391,8 @@ mt6366_vs1_reg: vs1 {
>
> mt6366_vs2_reg: vs2 {
> regulator-name = "pp1350_vs2";
> - regulator-min-microvolt = <1350000>;
> - regulator-max-microvolt = <1350000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <2087500>;
Same thing about the voltage ranges. Furthermore, the power rail says
"pp1350", which is a big hint why this change is wrong.
> regulator-ramp-delay = <12500>;
> regulator-enable-ramp-delay = <0>;
> regulator-always-on;
> @@ -1397,7 +1416,7 @@ mt6366_vaud28_reg: vaud28 {
> mt6366_vaux18_reg: vaux18 {
> regulator-name = "pp1840_vaux18";
> regulator-min-microvolt = <1800000>;
> - regulator-max-microvolt = <1840000>;
> + regulator-max-microvolt = <1800000>;
The correct voltage is 1.84 V, however since earlier kernels did not
support the 0.01 steps, the minimum was kept at 1.8V. The 0.04V offset
is set either by bootloader or is the hardware default.
> regulator-enable-ramp-delay = <270>;
> };
>
> @@ -1410,8 +1429,8 @@ mt6366_vbif28_reg: vbif28 {
>
> mt6366_vcn18_reg: vcn18 {
> regulator-name = "pp1800_vcn18_x";
> - regulator-min-microvolt = <1800000>;
> - regulator-max-microvolt = <1800000>;
> + regulator-min-microvolt = <600000>;
> + regulator-max-microvolt = <2100000>;
Same thing about the voltage ranges. Furthermore, the power rail
says "pp1800", which is a big hint why this change is wrong.
> regulator-enable-ramp-delay = <270>;
> };
>
> @@ -1424,8 +1443,8 @@ mt6366_vcn28_reg: vcn28 {
>
> mt6366_vefuse_reg: vefuse {
> regulator-name = "pp1800_vefuse";
> - regulator-min-microvolt = <1800000>;
> - regulator-max-microvolt = <1800000>;
> + regulator-min-microvolt = <1700000>;
> + regulator-max-microvolt = <1900000>;
Same thing about the voltage ranges.
> regulator-enable-ramp-delay = <270>;
> };
>
> @@ -1438,15 +1457,15 @@ mt6366_vfe28_reg: vfe28 {
>
> mt6366_vemc_reg: vemc {
> regulator-name = "pp3000_vemc";
> - regulator-min-microvolt = <3000000>;
> - regulator-max-microvolt = <3000000>;
> + regulator-min-microvolt = <2900000>;
> + regulator-max-microvolt = <3300000>;
Same thing about the voltage ranges. Furthermore, the power rail
says "pp3000", which is a big hint why this change is wrong.
> regulator-enable-ramp-delay = <60>;
> };
>
> mt6366_vibr_reg: vibr {
> regulator-name = "pp2800_vibr_x";
> - regulator-min-microvolt = <2800000>;
> - regulator-max-microvolt = <2800000>;
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <3300000>;
Same thing about the voltage ranges. Furthermore, the power rail
says "pp2800", which is a big hint why this change is wrong.
> regulator-enable-ramp-delay = <60>;
> };
>
> @@ -1482,30 +1501,30 @@ mt6366_vmc_reg: vmc {
>
> mt6366_vmddr_reg: vmddr {
> regulator-name = "pm0750_emi_vmddr";
> - regulator-min-microvolt = <700000>;
> - regulator-max-microvolt = <750000>;
> + regulator-min-microvolt = <600000>;
> + regulator-max-microvolt = <2100000>;
Same thing about the voltage ranges.
> regulator-enable-ramp-delay = <325>;
> regulator-always-on;
> };
>
> mt6366_vmch_reg: vmch {
> regulator-name = "pp3000_vmch";
> - regulator-min-microvolt = <3000000>;
> - regulator-max-microvolt = <3000000>;
> + regulator-min-microvolt = <2900000>;
> + regulator-max-microvolt = <3300000>;
Same thing about the voltage ranges. Furthermore, the power rail
says "pp3000", which is a big hint why this change is wrong.
> regulator-enable-ramp-delay = <60>;
> };
>
> mt6366_vcn33_reg: vcn33 {
> regulator-name = "pp3300_vcn33_x";
> regulator-min-microvolt = <3300000>;
> - regulator-max-microvolt = <3300000>;
> + regulator-max-microvolt = <3500000>;
Same thing about the voltage ranges. Furthermore, the power rail
says "pp3300", which is a big hint why this change is wrong.
> regulator-enable-ramp-delay = <270>;
> };
>
> vdram2 {
> regulator-name = "pp0600_emi_vddq";
> regulator-min-microvolt = <600000>;
> - regulator-max-microvolt = <600000>;
> + regulator-max-microvolt = <1800000>;
Same thing about the voltage ranges. Furthermore, the power rail
says "pp0600", which is a big hint why this change is wrong.
> regulator-enable-ramp-delay = <3300>;
> regulator-always-on;
> };
> @@ -1518,6 +1537,7 @@ mt6366_vrf12_reg: vrf12 {
> };
>
> mt6366_vrf18_reg: vrf18 {
> + compatible = "regulator-fixed";
No. This is incorrect.
> regulator-name = "pp1800_vrf18_x";
> regulator-min-microvolt = <1800000>;
> regulator-max-microvolt = <1800000>;
> @@ -1526,8 +1546,8 @@ mt6366_vrf18_reg: vrf18 {
>
> vsim1 {
> regulator-name = "pp1860_vsim1_x";
> - regulator-min-microvolt = <1800000>;
> - regulator-max-microvolt = <1860000>;
> + regulator-min-microvolt = <1700000>;
> + regulator-max-microvolt = <3100000>;
The correct voltage is 1.86 V, however since earlier kernels did not
support the 0.01 steps, the minimum was kept at 1.8V. The 0.06V offset
is set either by bootloader or is the hardware default.
> regulator-enable-ramp-delay = <540>;
> };
>
> @@ -1540,18 +1560,17 @@ mt6366_vsim2_reg: vsim2 {
>
> mt6366_vsram_gpu_reg: vsram-gpu {
> regulator-name = "pp0900_dvdd_sram_gpu";
> - regulator-min-microvolt = <850000>;
> - regulator-max-microvolt = <1050000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
Same thing about the voltage ranges.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <240>;
> - regulator-coupled-with = <&mt6366_vgpu_reg>;
> - regulator-coupled-max-spread = <10000>;
> + regulator-always-on;
No and no. See above about why it is coupled, and there is no need to
keep the GPU always on.
> };
>
> mt6366_vsram_others_reg: vsram-others {
> regulator-name = "pp0900_dvdd_sram_core";
> - regulator-min-microvolt = <900000>;
> - regulator-max-microvolt = <900000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
Same thing about the voltage ranges. Furthermore, the power rail
says "pp0900", which is a big hint why this change is wrong.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <240>;
> regulator-always-on;
> @@ -1559,8 +1578,8 @@ mt6366_vsram_others_reg: vsram-others {
>
> mt6366_vsram_proc11_reg: vsram-proc11 {
> regulator-name = "pp0900_dvdd_sram_bc";
> - regulator-min-microvolt = <850000>;
> - regulator-max-microvolt = <1120000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
Same thing about the voltage ranges.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <240>;
> regulator-always-on;
> @@ -1568,8 +1587,8 @@ mt6366_vsram_proc11_reg: vsram-proc11 {
>
> mt6366_vsram_proc12_reg: vsram-proc12 {
> regulator-name = "pp0900_dvdd_sram_lc";
> - regulator-min-microvolt = <850000>;
> - regulator-max-microvolt = <1120000>;
> + regulator-min-microvolt = <500000>;
> + regulator-max-microvolt = <1293750>;
Same thing about the voltage ranges.
> regulator-ramp-delay = <6250>;
> regulator-enable-ramp-delay = <240>;
> regulator-always-on;
> @@ -1578,7 +1597,7 @@ mt6366_vsram_proc12_reg: vsram-proc12 {
> vusb {
> regulator-name = "pp3070_vusb";
> regulator-min-microvolt = <3000000>;
> - regulator-max-microvolt = <3070000>;
> + regulator-max-microvolt = <3100000>;
The correct voltage is 3.07 V, however since earlier kernels did not
support the 0.01V steps, the minimum was kept at 3.0V. The 0.07V offset
is set either by bootloader or is the hardware default.
> regulator-enable-ramp-delay = <270>;
> regulator-always-on;
> };
> @@ -1586,7 +1605,7 @@ vusb {
> vxo22 {
> regulator-name = "pp2240_vxo22";
> regulator-min-microvolt = <2200000>;
> - regulator-max-microvolt = <2240000>;
> + regulator-max-microvolt = <2200000>;
Same thing about the voltage ranges.
> regulator-enable-ramp-delay = <120>;
> /* Feeds DCXO internally */
> regulator-always-on;
So basically you are undoing all the cleanups that I did to get the
dtsi to fit upstream conventions and solutions that we have.
ChenYu
Powered by blists - more mailing lists