From: sashiko-bot@kernel.org
To: "Konrad Dybcio" <konradybcio@kernel.org>
Cc: devicetree@vger.kernel.org, robh@kernel.org, conor+dt@kernel.org
Subject: Re: [PATCH 2/3] arm64: dts: qcom: Consolidate Glymur CoB power grid
Date: Wed, 28 Oct 2026 20:17:42 +0000 [thread overview]
Message-ID: <sashiko-outbox-188374@kernel.org> (raw)
In-Reply-To: <20261028-topic-sip_cob-v1-2-80ab6704aca3@oss.qualcomm.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: qcom: invalid step voltages on vreg_l7b0 and vreg_s9f0
- [Medium] arm64: dts: qcom: yoga-slim7x retains swapped USB QMP PHY supplies
- [Medium] arm64: dts: qcom: mahua-tcsr QREF fixups missing from mahua.dtsi
--
commit e05c2da4bef582e863ac9bb6fa079eb8effdd101
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
arm64: dts: qcom: Consolidate Glymur CoB power grid
This commit factors out the common PMIC includes, RPMH regulator definitions,
and on-SoC peripheral supply properties for Glymur CoB devices into a shared
glymur-cob.dtsi file and updates the board and Mahua device trees to use it.
> diff --git a/arch/arm64/boot/dts/qcom/glymur-cob.dtsi b/arch/arm64/boot/dts/qcom/glymur-cob.dtsi
> new file mode 100644
> index 0000000000000..8b4ba1d384a0d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/glymur-cob.dtsi
> @@ -0,0 +1,310 @@
[ ... ]
> +&apps_rsc {
> + rpmh_vregs_b_e0: regulators-0 {
> + compatible = "qcom,pmh0101-rpmh-regulators";
> + qcom,pmic-id = "B_E0";
> +
> + vreg_l1b0: ldo1 {
> + regulator-name = "vreg_l1b0";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
> +
> + vreg_l7b0: ldo7 {
> + regulator-name = "vreg_l7b0";
> + regulator-min-microvolt = <3100000>;
> + regulator-max-microvolt = <3100000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
[ ... ]
> + rpmh_vregs_f_e0: regulators-2 {
> + compatible = "qcom,pmh0110-rpmh-regulators";
> + qcom,pmic-id = "F_E0";
> +
> + vreg_l2f0: ldo2 {
> + regulator-name = "vreg_l2f0";
> + regulator-min-microvolt = <944000>;
> + regulator-max-microvolt = <944000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
> +
> + vreg_l3f0: ldo3 {
> + regulator-name = "vreg_l3f0";
> + regulator-min-microvolt = <912000>;
> + regulator-max-microvolt = <912000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
> +
> + /* Voltage input for PMCX0104 */
> + vreg_s9f0: smps9 {
> + regulator-name = "vreg_s9f0";
> + regulator-min-microvolt = <1900000>;
> + regulator-max-microvolt = <1900000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
> + };
[Severity: High]
Will setting regulator-min-microvolt == regulator-max-microvolt to 3100000 on
vreg_l7b0 (ldo7 on pmh0101) and 1900000 on vreg_s9f0 (smps9 on pmh0110) cause
rpmh_regulator_probe() to fail with -EINVAL for both the B_E0 and F_E0 PMIC
regulator nodes?
In drivers/regulator/qcom-rpmh-regulator.c, pmh0101_vreg_data maps ldo7 to
pmic5_pldo530_mvp300:
static const struct rpmh_vreg_hw_data pmic5_pldo530_mvp300 = {
.regulator_type = VRM,
.ops = &rpmh_regulator_vrm_drms_ops,
.voltage_ranges = (struct linear_range[]) {
REGULATOR_LINEAR_RANGE(1504000, 0, 255, 8000),
},
Because (3100000 - 1504000) / 8000 is 199.5, 3100000 uV lies halfway between
the valid hardware steps 3096000 uV and 3104000 uV.
Similarly, pmh0110_vreg_data maps smps9 to pmic5_ftsmps530:
static const struct rpmh_vreg_hw_data pmic5_ftsmps530 = {
.regulator_type = VRM,
.ops = &rpmh_regulator_vrm_ops,
.voltage_ranges = (struct linear_range[]) {
REGULATOR_LINEAR_RANGE(252000, 0, 305, 4000),
REGULATOR_LINEAR_RANGE(1480000, 306, 464, 8000),
},
Because (1900000 - 1480000) / 8000 is 52.5, 1900000 uV lies halfway between
the valid hardware steps 1896000 uV and 1904000 uV (previously vreg_s9f0 used
a range of 1900000 to 2000000 uV, which contained valid steps).
When rpmh_regulator_probe() -> rpmh_regulator_init_vreg() ->
devm_regulator_register() -> set_machine_constraints() ->
machine_constraints_voltage() checks a fixed constraint where cmin == cmax is
not an exact hardware step, clamping produces max_uV < min_uV (3096000 <
3104000 for vreg_l7b0, and 1896000 < 1904000 for vreg_s9f0) and returns
-EINVAL, aborting registration of the entire PMIC regulator block in
rpmh_regulator_probe():
ret = rpmh_regulator_init_vreg(vreg, dev, node, pmic_id,
vreg_data);
if (ret < 0)
return ret;
> diff --git a/arch/arm64/boot/dts/qcom/glymur-lenovo-yoga-slim7x.dts b/arch/arm64/boot/dts/qcom/glymur-lenovo-yoga-slim7x.dts
> index 56c22daab0569..a8580499e7093 100644
> --- a/arch/arm64/boot/dts/qcom/glymur-lenovo-yoga-slim7x.dts
> +++ b/arch/arm64/boot/dts/qcom/glymur-lenovo-yoga-slim7x.dts
[ ... ]
> @@ -1142,9 +928,6 @@ &usb_0_dwc3_hs {
> };
>
> &usb_0_hsphy {
> - vdd-supply = <&vreg_l3f0>;
> - vdda12-supply = <&vreg_l4h0>;
> -
> phys = <&smb2370_j_e2_eusb2_repeater>;
>
> status = "okay";
> @@ -1171,9 +954,6 @@ &usb_1_dwc3_hs {
> };
>
> &usb_1_hsphy {
> - vdd-supply = <&vreg_l3f0>;
> - vdda12-supply = <&vreg_l4h0>;
> -
> phys = <&smb2370_k_e2_eusb2_repeater>;
>
> status = "okay";
> @@ -1200,9 +980,6 @@ &usb_2_dwc3_hs {
> };
>
> &usb_2_hsphy {
> - vdd-supply = <&vreg_l4c1>;
> - vdda12-supply = <&vreg_l4f1>;
> -
> phys = <&smb2370_l_e2_eusb2_repeater>;
>
> status = "okay";
[Severity: Medium]
Should the local supply properties on &usb_0_qmpphy, &usb_1_qmpphy, and
&usb_2_qmpphy in arch/arm64/boot/dts/qcom/glymur-lenovo-yoga-slim7x.dts also
be removed so they inherit the consolidated definitions from glymur-cob.dtsi?
Right after each of the hsphy nodes above, glymur-lenovo-yoga-slim7x.dts still
has:
&usb_0_qmpphy {
vdda-phy-supply = <&vreg_l4h0>;
vdda-pll-supply = <&vreg_l3f0>;
refgen-supply = <&vreg_l2f0>;
status = "okay";
};
...
&usb_1_qmpphy {
vdda-phy-supply = <&vreg_l4h0>;
vdda-pll-supply = <&vreg_l1h0>;
refgen-supply = <&vreg_l2f0>;
status = "okay";
};
...
&usb_2_qmpphy {
vdda-phy-supply = <&vreg_l4f1>;
vdda-pll-supply = <&vreg_l4c1>;
refgen-supply = <&vreg_l1c1>;
status = "okay";
};
Those local properties override glymur-cob.dtsi with the 1.2 V rails
(vreg_l4h0, vreg_l4f1) and 0.9 V rails (vreg_l3f0, vreg_l1h0, vreg_l4c1)
swapped between vdda-phy-supply and vdda-pll-supply, and with vreg_l1c1 on
&usb_2_qmpphy refgen-supply.
> diff --git a/arch/arm64/boot/dts/qcom/mahua-crd.dts b/arch/arm64/boot/dts/qcom/mahua-crd.dts
> index 51708954f17a7..8a36d0caaf9e2 100644
> --- a/arch/arm64/boot/dts/qcom/mahua-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/mahua-crd.dts
> @@ -26,17 +26,16 @@ &pcie3b_phy {
> };
>
> &tcsr {
> - vdda-qrefrpt0-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrpt1-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrpt2-0p9-supply = <&vreg_l2f1>;
> + /delete-property/ vdda-qrefrx4-0p9-supply;
> + /delete-property/ vdda-qrefrx5-0p9-supply;
> + /delete-property/ vdda-qreftx0-0p9-supply;
> + /delete-property/ vdda-qreftx0-1p2-supply;
> + /delete-property/ vdda-refgen4-0p9-supply;
> + /delete-property/ vdda-refgen4-1p2-supply;
> +
> vdda-qrefrpt3-0p9-supply = <&vreg_l1f1>;
> - vdda-qrefrpt4-0p9-supply = <&vreg_l2h0>;
> vdda-qrefrpt5-0p9-supply = <&vreg_l2h0>;
> - vdda-qrefrx0-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrx1-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrx2-0p9-supply = <&vreg_l2f1>;
> vdda-qrefrx3-0p9-supply = <&vreg_l2h0>;
> - vdda-qreftx1-0p9-supply = <&vreg_l1f1>;
> vdda-refgen3-0p9-supply = <&vreg_l1f1>;
> vdda-refgen3-1p2-supply = <&vreg_l4f1>;
> };
> diff --git a/arch/arm64/boot/dts/qcom/mahua-lenovo-thinkpad-t14s-gen7-lcd.dts b/arch/arm64/boot/dts/qcom/mahua-lenovo-thinkpad-t14s-gen7-lcd.dts
> index 860e9ef01aa6f..a77eb75658047 100644
> --- a/arch/arm64/boot/dts/qcom/mahua-lenovo-thinkpad-t14s-gen7-lcd.dts
> +++ b/arch/arm64/boot/dts/qcom/mahua-lenovo-thinkpad-t14s-gen7-lcd.dts
[ ... ]
> @@ -948,36 +751,9 @@ right_spkr: speaker@0,1 {
> };
> };
>
> -&smb2370_j_e2_eusb2_repeater {
> - vdd18-supply = <&vreg_l15b0>;
> - vdd3-supply = <&vreg_l7b0>;
> -};
> -
> -&smb2370_k_e2_eusb2_repeater {
> - vdd18-supply = <&vreg_l15b0>;
> - vdd3-supply = <&vreg_l7b0>;
> -};
> -
> &tcsr {
> - vdda-qrefrpt0-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrpt1-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrpt2-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrpt3-0p9-supply = <&vreg_l2h0>;
> - vdda-qrefrpt4-0p9-supply = <&vreg_l2h0>;
> vdda-qrefrpt5-0p9-supply = <&vreg_l2h0>;
> - vdda-qrefrx0-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrx1-0p9-supply = <&vreg_l2f1>;
> - vdda-qrefrx2-0p9-supply = <&vreg_l2f1>;
> vdda-qrefrx3-0p9-supply = <&vreg_l2h0>;
> - vdda-qrefrx4-0p9-supply = <&vreg_l2h0>;
> - vdda-qrefrx5-0p9-supply = <&vreg_l3f0>;
> - vdda-qreftx0-0p9-supply = <&vreg_l3f0>;
> - vdda-qreftx0-1p2-supply = <&vreg_l4h0>;
> - vdda-qreftx1-0p9-supply = <&vreg_l1f1>;
> - vdda-refgen3-0p9-supply = <&vreg_l2f0>;
> - vdda-refgen3-1p2-supply = <&vreg_l4h0>;
> - vdda-refgen4-0p9-supply = <&vreg_l1f1>;
> - vdda-refgen4-1p2-supply = <&vreg_l4f1>;
> };
[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/mahua.dtsi b/arch/arm64/boot/dts/qcom/mahua.dtsi
> index a59c5ed7b48e7..16f62040aa84b 100644
> --- a/arch/arm64/boot/dts/qcom/mahua.dtsi
> +++ b/arch/arm64/boot/dts/qcom/mahua.dtsi
> @@ -5,6 +5,7 @@
>
> /* Mahua is heavily based on Glymur, with some meaningful differences */
> #include "glymur.dtsi"
> +#include "glymur-cob.dtsi"
[Severity: Medium]
Since mahua.dtsi now includes glymur-cob.dtsi and sets &tcsr compatible to
"qcom,mahua-tcsr", should the Mahua-specific &tcsr property deletions and
supply overrides in mahua-crd.dts live in mahua.dtsi (or a Mahua CoB dtsi)
instead?
With those &tcsr changes placed only in mahua-crd.dts,
mahua-lenovo-thinkpad-t14s-gen7-lcd.dts only overrides vdda-qrefrpt5-0p9-supply
and vdda-qrefrx3-0p9-supply, leaving it with Glymur's refgen3 and qrefrpt3
supplies (vreg_l2f0, vreg_l4h0, vreg_l2h0 instead of vreg_l1f1 and vreg_l4f1)
as well as the unused Glymur-only qrefrx4, qrefrx5, qreftx0, and refgen4
properties from glymur-cob.dtsi.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-topic-sip_cob-v1-0-80ab6704aca3@oss.qualcomm.com?part=2
next prev parent reply other threads:[~2026-10-28 20:17 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-28 20:04 [PATCH 0/3] Commonize Glymur regulator settings for fixed rails Konrad Dybcio
2026-10-28 20:04 ` [PATCH 1/3] arm64: dts: qcom: Unify Glymur/Mahua regulator handles Konrad Dybcio
2026-10-29 8:36 ` Abel Vesa
2026-10-28 20:04 ` [PATCH 2/3] arm64: dts: qcom: Consolidate Glymur CoB power grid Konrad Dybcio
2026-10-28 20:17 ` sashiko-bot [this message]
2026-10-29 8:51 ` Abel Vesa
2026-10-28 20:04 ` [PATCH 3/3] arm64: dts: qcom: glymur-crd: Drop L4F0 Konrad Dybcio
2026-10-29 8:37 ` Abel Vesa
2026-10-29 9:39 ` [PATCH v1 0/3] Commonize Glymur regulator settings for fixed rails Sivansh Gupta
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-188374@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=konradybcio@kernel.org \
--cc=robh@kernel.org \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox