Re: [PATCH v2 2/3] arm64: dts: qcom: add HP OmniBook Ultra 14-kg0xxx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: missing pmh0101 and pmh0110-glymur includes
- [Medium] arm64: dts: qcom: missing serial0 alias for enabled uart21
- [Medium] arm64: dts: qcom: missing regulator-always-on on vreg_l18b_e0_1p2 for pcie3b
--

commit 16bebd9c6c0b1cc3644884c02e54b4e1e8aa6439
Author: Jason Pettit <jason.pettit@oss.qualcomm.com>

arm64: dts: qcom: add HP OmniBook Ultra 14-kg0xxx

This commit adds a device tree for the HP OmniBook Ultra 14-kg0xxx laptop
based on the Qualcomm Snapdragon X2 Elite (Glymur) platform.

> diff --git a/arch/arm64/boot/dts/qcom/glymur-hp-omnibook-ultra-kg0xxx.dts b/arch/arm64/boot/dts/qcom/glymur-hp-omnibook-ultra-kg0xxx.dts
> new file mode 100644
> index 0000000000000..fdf8df6374fa2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/glymur-hp-omnibook-ultra-kg0xxx.dts
> @@ -0,0 +1,986 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +/dts-v1/;
> +
> +#include "glymur.dtsi"
> +
> +#include "pmcx0102.dtsi"
> +#include "pmk8850.dtsi"
> +#include "smb2370.dtsi"

[Severity: Medium]
Should this device tree also include pmh0101.dtsi and pmh0110-glymur.dtsi (and
possibly pmh0104-glymur.dtsi) like the other Glymur board files?

Further down under &apps_rsc, this board configures RPMh regulators on PMH0101
(B_E0) in regulators-0 and on PMH0110 (F_E0, H_E0, and F_E1) in regulators-1,
regulators-2, and regulators-3:

	regulators-0 {
		compatible = "qcom,pmh0101-rpmh-regulators";
		qcom,pmic-id = "B_E0";
		...
	};

	regulators-1 {
		compatible = "qcom,pmh0110-rpmh-regulators";
		qcom,pmic-id = "F_E0";
		...
	};

Without pmh0101.dtsi and pmh0110-glymur.dtsi included, the SPMI PMIC devices on
spmi_bus0 and spmi_bus1, their SPMI GPIO controllers, their qcom,spmi-temp-alarm
sensors, and their thermal zones (pmh0101-thermal, pmh0110-f0-thermal,
pmh0110-f1-thermal, and pmh0110-h0-thermal) are omitted, leaving those PMICs
without thermal monitoring or shutdown trip points.

[ ... ]

> +/ {
> +	compatible = "hp,omnibook-ultra-kg0xxx", "qcom,glymur";
> +	model = "HP OmniBook Ultra Laptop 14-kg0xxx";
> +
> +	aliases {
> +		i2c0 = &i2c0;
> +		i2c4 = &i2c4;
> +		i2c8 = &i2c8;
> +
> +		serial1 = &uart14;
> +	};

[Severity: Medium]
Should aliases also define serial0 = &uart21 (or should &uart21 be disabled if
it is not routed on this board)?

In arch/arm64/boot/dts/qcom/glymur.dtsi, uart21 (serial@894000, compatible
qcom,geni-debug-uart) is enabled by default and claims gpio86 and gpio87 via
qup_uart21_default.

When qcom_geni_serial_probe() in drivers/tty/serial/qcom_geni_serial.c probes a
qcom,geni-debug-uart device, it looks up the serial alias ID and passes it to
get_port_from_line():

	if (data->console) {
		drv = &qcom_geni_console_driver;
		line = of_alias_get_id(pdev->dev.of_node, "serial");
	}

	port = get_port_from_line(line, data->console, &pdev->dev);

In get_port_from_line(), console ports require a valid alias in 0 ..
GENI_UART_CONS_PORTS - 1:

	if (console) {
		if (line < 0 || line >= nr_ports)
			return ERR_PTR(-ENXIO);

		port = &qcom_geni_console_port;
	}

Because no serial0 alias is present, of_alias_get_id() returns -ENODEV and
uart21 fails probe with -ENXIO on every boot.

[ ... ]

> +&apps_rsc {
> +	regulators-0 {
> +		compatible = "qcom,pmh0101-rpmh-regulators";
> +		qcom,pmic-id = "B_E0";
> +
> +		vreg_l1b_e0_1p8: ldo1 {
> +			regulator-name = "vreg_l1b_e0_1p8";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +
> +		vreg_l7b_e0_3p1: ldo7 {
> +			regulator-name = "vreg_l7b_e0_3p1";
> +			regulator-min-microvolt = <3072000>;
> +			regulator-max-microvolt = <3072000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +
> +		vreg_l15b_e0_1p8: ldo15 {
> +			regulator-name = "vreg_l15b_e0_1p8";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +
> +		vreg_l18b_e0_1p2: ldo18 {
> +			regulator-name = "vreg_l18b_e0_1p2";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +	};

[Severity: Medium]
Does vreg_l18b_e0_1p2 need regulator-always-on here?

This board enables &pcie3b for the NVMe SSD using gpio155 for reset-gpios and
gpio157 for wake-gpios:

&pcie3b_port0 {
	reset-gpios = <&tlmm 155 GPIO_ACTIVE_LOW>;
	wake-gpios = <&tlmm 157 GPIO_ACTIVE_LOW>;
};

In arch/arm64/boot/dts/qcom/glymur-asus-zenbook-a14-ux3407na.dts, ldo18 on B_E0
is marked regulator-always-on because it provides the reference voltage for the
&pcie3b wake and reset signals:

		vreg_l18b0: ldo18 {
			regulator-name = "vreg_l18b0";
			regulator-min-microvolt = <1200000>;
			regulator-max-microvolt = <1200000>;
			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;

			/*
			 * This is also reference voltage for &pcie3b wake and
			 * reset signals.
			 */
			regulator-always-on;
		};

In glymur-hp-omnibook-ultra-kg0xxx.dts, vreg_l18b_e0_1p2 is only referenced as
vdd-io-supply by the SoundWire speaker nodes under &swr0 and &swr3, which do
not probe until &remoteproc_adsp boots. If the ADSP firmware or audio drivers
are not loaded before unused regulators are disabled, turning off
vreg_l18b_e0_1p2 can drop the reference supply for the pcie3b reset and wake
signals and cause the NVMe SSD to drop off the bus.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-hp-omnibook-v2-v2-0-1b99767deda0@oss.qualcomm.com?part=2