diff mbox series

[v2,4/6] arm64: dts: qcom: sc8280xp: Add reference device

Message ID 20220622041224.627803-5-bjorn.andersson@linaro.org (mailing list archive)
State Superseded
Headers show
Series arm64: dts: qcom: Introduce SC8280XP | expand

Commit Message

Bjorn Andersson June 22, 2022, 4:12 a.m. UTC
Add basic support for the SC8280XP reference device, which allows it to
boot to a shell (using EFIFB) with functional storage (UFS), USB,
keyboard, touchpad, touchscreen, backlight and remoteprocs.

The PMICs are, per socinfo, reused from other platforms. But given that
the address of the PMICs doesn't match other cases and that it's
desirable to label things according to the schematics a new dtsi file is
created to represent the reference combination of PMICs.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Reordered "status" last
- Fixed invalid PMIC gpio 0
- Replaced "hid" name with touchscreen, touchpad and keyboard
- Added &xo_board_clk frequency

 arch/arm64/boot/dts/qcom/Makefile            |   1 +
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts    | 432 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 108 +++++
 3 files changed, 541 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
 create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi

Comments

Johan Hovold June 22, 2022, 9:06 a.m. UTC | #1
On Tue, Jun 21, 2022 at 09:12:22PM -0700, Bjorn Andersson wrote:
> Add basic support for the SC8280XP reference device, which allows it to
> boot to a shell (using EFIFB) with functional storage (UFS), USB,
> keyboard, touchpad, touchscreen, backlight and remoteprocs.
> 
> The PMICs are, per socinfo, reused from other platforms. But given that
> the address of the PMICs doesn't match other cases and that it's
> desirable to label things according to the schematics a new dtsi file is
> created to represent the reference combination of PMICs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Reordered "status" last
> - Fixed invalid PMIC gpio 0
> - Replaced "hid" name with touchscreen, touchpad and keyboard

You also added the HID vdd-supply properties from the X13s devicetree.

I assume the CRD also uses vreg_misc_3p3 for these (always-on for now
anyway).

> - Added &xo_board_clk frequency

> +	vreg_misc_3p3: misc-3p3-regulator {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_MISC_3P3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&pmc8280_1_gpios 1 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&misc_3p3_reg_en>;
> +
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};

> +&qup0_i2c4 {
> +	clock-frequency = <400000>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup0_i2c4_default>, <&ts0_default>;
> +
> +	status = "okay";
> +
> +	touchscreen@10 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x10>;
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 175 IRQ_TYPE_LEVEL_LOW>;
> +		vdd-supply = <&vreg_misc_3p3>;
> +	};
> +};

> +&qup2_i2c5 {
> +	clock-frequency = <400000>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
> +
> +	status = "okay";
> +
> +	touchpad@15 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x15>;
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 182 IRQ_TYPE_LEVEL_LOW>;
> +		vdd-supply = <&vreg_misc_3p3>;
> +	};
> +
> +	keyboard@68 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x68>;
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 104 IRQ_TYPE_LEVEL_LOW>;
> +		vdd-supply = <&vreg_misc_3p3>;
> +	};
> +};

[...]

> +&usb_1_qmpphy {
> +	vdda-phy-supply = <&vreg_l4b>;
> +	vdda-pll-supply = <&vreg_l3b>;
> +
> +	status = "okay";
> +};
> +
> +/* PINCTRL - additions to nodes defined in sc8280xp.dtsi */
> +
> +&pmc8280_1_gpios {
> +	edp_bl_en: edp-bl-en-state {
> +		pins = "gpio8";
> +		function = "normal";
> +	};
> +
> +	edp_bl_reg_en: edp-bl-reg-en-state {
> +		pins = "gpio9";
> +		function = "normal";
> +	};
> +
> +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> +		pins = "gpio1";
> +		function = "normal";
> +	};
> +};
> +
> +&pmc8280c_gpios {
> +	edp_bl_pwm: edp-bl-pwm-state {
> +		pins = "gpio8";
> +		function = "func1";
> +	};
> +};
> +
> +&xo_board_clk {
> +	clock-frequency = <38400000>;
> +};

Nit: This one should go after &usb_1_qmpphy above if we want to keep the
pinctrl nodes last.

> +
> +&tlmm {

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>

Johan
Konrad Dybcio June 22, 2022, 12:33 p.m. UTC | #2
On 22.06.2022 06:12, Bjorn Andersson wrote:
> Add basic support for the SC8280XP reference device, which allows it to
> boot to a shell (using EFIFB) with functional storage (UFS), USB,
> keyboard, touchpad, touchscreen, backlight and remoteprocs.
> 
> The PMICs are, per socinfo, reused from other platforms. But given that
> the address of the PMICs doesn't match other cases and that it's
> desirable to label things according to the schematics a new dtsi file is
> created to represent the reference combination of PMICs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> 
> Changes since v1:
> - Reordered "status" last
> - Fixed invalid PMIC gpio 0
> - Replaced "hid" name with touchscreen, touchpad and keyboard
> - Added &xo_board_clk frequency
> 
>  arch/arm64/boot/dts/qcom/Makefile            |   1 +
>  arch/arm64/boot/dts/qcom/sc8280xp-crd.dts    | 432 +++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 108 +++++
>  3 files changed, 541 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
>  create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 2f8aec2cc6db..ceeae094a59f 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -89,6 +89,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-herobrine-villager-r0.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-idp.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-idp2.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-crd-r3.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= sc8280xp-crd.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm630-sony-xperia-ganges-kirin.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm630-sony-xperia-nile-discovery.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= sdm630-sony-xperia-nile-pioneer.dtb
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> new file mode 100644
> index 000000000000..38a64e886466
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/gpio-keys.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +
> +#include "sc8280xp.dtsi"
> +#include "sc8280xp-pmics.dtsi"
> +
> +/ {
> +	model = "Qualcomm SC8280XP CRD";
> +	compatible = "qcom,sc8280xp-crd", "qcom,sc8280xp";
> +
> +	aliases {
> +		serial0 = &qup2_uart17;
> +	};
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pmc8280c_lpg 3 1000000>;
> +		enable-gpios = <&pmc8280_1_gpios 8 GPIO_ACTIVE_HIGH>;
> +		power-supply = <&vreg_edp_bl>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&edp_bl_en>, <&edp_bl_pwm>;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	vreg_edp_bl: edp-bl-regulator {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_EDP_BL";
> +		regulator-min-microvolt = <3600000>;
> +		regulator-max-microvolt = <3600000>;
> +
> +		gpio = <&pmc8280_1_gpios 9 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&edp_bl_reg_en>;
> +
> +		regulator-boot-on;
> +	};
> +
> +	vreg_misc_3p3: misc-3p3-regulator {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_MISC_3P3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&pmc8280_1_gpios 1 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&misc_3p3_reg_en>;
> +
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};
> +
> +	reserved-memory {
> +	};
Seems redundant.


> +};
> +
> +&apps_rsc {
> +	pmc8280-1-rpmh-regulators {
> +		compatible = "qcom,pm8350-rpmh-regulators";
> +		qcom,pmic-id = "b";
> +
> +		vdd-l3-l5-supply = <&vreg_s11b>;
> +
> +		vreg_s11b: smps11 {
> +			regulator-name = "vreg_s11b";
> +			regulator-min-microvolt = <1272000>;
> +			regulator-max-microvolt = <1272000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
> +
> +		vreg_l3b: ldo3 {
> +			regulator-name = "vreg_l3b";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +			regulator-boot-on;
> +			regulator-always-on;
> +		};
> +
> +		vreg_l4b: ldo4 {
> +			regulator-name = "vreg_l4b";
> +			regulator-min-microvolt = <912000>;
> +			regulator-max-microvolt = <912000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +
> +		vreg_l6b: ldo6 {
> +			regulator-name = "vreg_l6b";
> +			regulator-min-microvolt = <880000>;
> +			regulator-max-microvolt = <880000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +			regulator-boot-on;
> +		};
> +	};
> +
> +	pmc8280c-rpmh-regulators {
> +		compatible = "qcom,pm8350c-rpmh-regulators";
> +		qcom,pmic-id = "c";
> +
> +		vreg_l1c: ldo1 {
> +			regulator-name = "vreg_l1c";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +
> +		vreg_l7c: ldo7 {
> +			regulator-name = "vreg_l7c";
> +			regulator-min-microvolt = <2504000>;
> +			regulator-max-microvolt = <2504000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +
> +		vreg_l13c: ldo13 {
> +			regulator-name = "vreg_l13c";
> +			regulator-min-microvolt = <3072000>;
> +			regulator-max-microvolt = <3072000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +	};
> +
> +	pmc8280-2-rpmh-regulators {
> +		compatible = "qcom,pm8350-rpmh-regulators";
> +		qcom,pmic-id = "d";
> +
> +		vdd-l1-l4-supply = <&vreg_s11b>;
> +
> +		vreg_l3d: ldo3 {
> +			regulator-name = "vreg_l3d";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +
> +		vreg_l4d: ldo4 {
> +			regulator-name = "vreg_l4d";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +
> +		vreg_l6d: ldo6 {
> +			regulator-name = "vreg_l6d";
> +			regulator-min-microvolt = <880000>;
> +			regulator-max-microvolt = <880000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +
> +		vreg_l7d: ldo7 {
> +			regulator-name = "vreg_l7d";
> +			regulator-min-microvolt = <3072000>;
> +			regulator-max-microvolt = <3072000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +
> +		vreg_l9d: ldo9 {
> +			regulator-name = "vreg_l9d";
> +			regulator-min-microvolt = <912000>;
> +			regulator-max-microvolt = <912000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +			regulator-allow-set-load;
> +		};
> +	};
> +};
> +
> +&pmc8280c_lpg {
> +	status = "okay";
> +};
> +
> +&pmk8280_pon_pwrkey {
> +	status = "okay";
> +};
> +
> +&qup0 {
> +	status = "okay";
> +};
> +
> +&qup0_i2c4 {
> +	clock-frequency = <400000>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup0_i2c4_default>, <&ts0_default>;
> +
> +	status = "okay";
> +
> +	touchscreen@10 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x10>;
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 175 IRQ_TYPE_LEVEL_LOW>;
> +		vdd-supply = <&vreg_misc_3p3>;
> +	};
> +};
> +
> +&qup1 {
> +	status = "okay";
> +};
> +
> +&qup2 {
> +	status = "okay";
> +};
> +
> +&qup2_i2c5 {
> +	clock-frequency = <400000>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
> +
> +	status = "okay";
> +
I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?


> +	touchpad@15 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x15>;
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 182 IRQ_TYPE_LEVEL_LOW>;
> +		vdd-supply = <&vreg_misc_3p3>;
> +	};
> +
> +	keyboard@68 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x68>;
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 104 IRQ_TYPE_LEVEL_LOW>;
> +		vdd-supply = <&vreg_misc_3p3>;
> +	};
> +};
> +
> +&qup2_uart17 {
> +	compatible = "qcom,geni-debug-uart";
> +
> +	status = "okay";
> +};
> +
> +&remoteproc_adsp {
> +	firmware-name = "qcom/sc8280xp/qcadsp8280.mbn";
> +
> +	status = "okay";
> +};
> +
> +&remoteproc_nsp0 {
> +	firmware-name = "qcom/sc8280xp/qccdsp8280.mbn";
> +
> +	status = "okay";
> +};
> +
> +&ufs_mem_hc {
> +	reset-gpios = <&tlmm 228 GPIO_ACTIVE_LOW>;
> +
> +	vcc-supply = <&vreg_l7c>;
> +	vcc-max-microamp = <800000>;
> +	vccq-supply = <&vreg_l3d>;
> +	vccq-max-microamp = <900000>;
> +
> +	status = "okay";
> +};
> +
> +&ufs_mem_phy {
> +	vdda-phy-supply = <&vreg_l6b>;
> +	vdda-pll-supply = <&vreg_l3b>;
> +
> +	status = "okay";
> +};
> +
> +&usb_0 {
> +	status = "okay";
> +};
> +
> +&usb_0_dwc3 {
> +	/* TODO: Define USB-C connector properly */
> +	dr_mode = "host";
> +};
> +
> +&usb_0_hsphy {
> +	vdda-pll-supply = <&vreg_l9d>;
> +	vdda18-supply = <&vreg_l1c>;
> +	vdda33-supply = <&vreg_l7d>;
> +
> +	status = "okay";
> +};
> +
> +&usb_0_qmpphy {
> +	vdda-phy-supply = <&vreg_l9d>;
> +	vdda-pll-supply = <&vreg_l4d>;
> +
> +	status = "okay";
> +};
> +
> +&usb_1 {
> +	status = "okay";
> +};
> +
> +&usb_1_dwc3 {
> +	/* TODO: Define USB-C connector properly */
> +	dr_mode = "host";
> +};
> +
> +&usb_1_hsphy {
> +	vdda-pll-supply = <&vreg_l4b>;
> +	vdda18-supply = <&vreg_l1c>;
> +	vdda33-supply = <&vreg_l13c>;
> +
> +	status = "okay";
> +};
> +
> +&usb_1_qmpphy {
> +	vdda-phy-supply = <&vreg_l4b>;
> +	vdda-pll-supply = <&vreg_l3b>;
> +
> +	status = "okay";
> +};
> +
> +/* PINCTRL - additions to nodes defined in sc8280xp.dtsi */
This comment seems redundant.

> +
> +&pmc8280_1_gpios {
> +	edp_bl_en: edp-bl-en-state {
> +		pins = "gpio8";
> +		function = "normal";
> +	};
> +
> +	edp_bl_reg_en: edp-bl-reg-en-state {
> +		pins = "gpio9";
> +		function = "normal";
> +	};
> +
> +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> +		pins = "gpio1";
> +		function = "normal";
> +	};
> +};
> +
> +&pmc8280c_gpios {
> +	edp_bl_pwm: edp-bl-pwm-state {
> +		pins = "gpio8";
> +		function = "func1";
> +	};
> +};
> +
> +&xo_board_clk {
> +	clock-frequency = <38400000>;
> +};
> +
> +&tlmm {
> +	gpio-reserved-ranges = <74 6>, <83 4>, <125 2>, <128 2>, <154 7>;
> +
> +	kybd_default: kybd-default-state {
> +		disable {
> +			pins = "gpio102";
> +			function = "gpio";
> +			output-low;
> +		};
> +
> +		int-n {
> +			pins = "gpio104";
> +			function = "gpio";
> +			bias-disable;
> +		};
> +
> +		reset {
> +			pins = "gpio105";
> +			function = "gpio";
> +			bias-disable;
> +		};
> +	};
> +
> +	qup0_i2c4_default: qup0-i2c4-default-state {
> +		pins = "gpio171", "gpio172";
> +		function = "qup4";
> +
> +		bias-disable;
> +		drive-strength = <16>;
> +	};
> +
> +	qup2_i2c5_default: qup2-i2c5-default-state {
> +		pins = "gpio81", "gpio82";
> +		function = "qup21";
> +
> +		bias-disable;
> +		drive-strength = <16>;
> +	};
> +
> +	tpad_default: tpad-default-state {
> +		int-n {
If you aren't gonna add more pins to this touchpad block, I think you could drop this extra level.


> +			pins = "gpio182";
> +			function = "gpio";
> +			bias-disable;
> +		};
> +	};
> +
> +	ts0_default: ts0-default-state {
> +		int-n {
> +			pins = "gpio175";
> +			function = "gpio";
> +			bias-pull-up;
> +		};
> +
> +		reset-n {
> +			pins = "gpio99";
> +			function = "gpio";
> +			output-high;
> +			drive-strength = <16>;
> +		};
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> new file mode 100644
> index 000000000000..36ed7d808ab8
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
Is it the only configuration supported by Qualcomm, or only a reference one?

Konrad


> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/spmi/spmi.h>
> +
> +&spmi_bus {
> +	pmk8280: pmic@0 {
> +		compatible = "qcom,pmk8350", "qcom,spmi-pmic";
> +		reg = <0x0 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pmk8280_pon: pon@1300 {
> +			compatible = "qcom,pm8998-pon";
> +			reg = <0x1300>;
> +
> +			pmk8280_pon_pwrkey: pwrkey {
> +				compatible = "qcom,pmk8350-pwrkey";
> +				interrupts = <0x0 0x13 0x7 IRQ_TYPE_EDGE_BOTH>;
> +				linux,code = <KEY_POWER>;
> +				status = "disabled";
> +			};
> +		};
> +	};
> +
> +	pmc8280_1: pmic@1 {
> +		compatible = "qcom,pm8350", "qcom,spmi-pmic";
> +		reg = <0x1 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pmc8280_1_gpios: gpio@8800 {
> +			compatible = "qcom,pm8350-gpio", "qcom,spmi-gpio";
> +			reg = <0x8800>;
> +			gpio-controller;
> +			gpio-ranges = <&pmc8280_1_gpios 0 0 10>;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> +
> +	pmc8280c: pmic@2 {
> +		compatible = "qcom,pm8350c", "qcom,spmi-pmic";
> +		reg = <0x2 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pmc8280c_gpios: gpio@8800 {
> +			compatible = "qcom,pm8350c-gpio", "qcom,spmi-gpio";
> +			reg = <0x8800>;
> +			gpio-controller;
> +			gpio-ranges = <&pmc8280c_gpios 0 0 9>;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +
> +		pmc8280c_lpg: lpg@e800 {
> +			compatible = "qcom,pm8350c-pwm";
> +			reg = <0xe800>;
> +
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			#pwm-cells = <2>;
> +
> +			status = "disabled";
> +		};
> +	};
> +
> +	pmc8280_2: pmic@3 {
> +		compatible = "qcom,pm8350", "qcom,spmi-pmic";
> +		reg = <0x3 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pmc8280_2_gpios: gpio@8800 {
> +			compatible = "qcom,pm8350-gpio", "qcom,spmi-gpio";
> +			reg = <0x8800>;
> +			gpio-controller;
> +			gpio-ranges = <&pmc8280_2_gpios 0 0 10>;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> +
> +	pmr735a: pmic@4 {
> +		compatible = "qcom,pmr735a", "qcom,spmi-pmic";
> +		reg = <0x4 SPMI_USID>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		pmr735a_gpios: gpio@8800 {
> +			compatible = "qcom,pmr735a-gpio", "qcom,spmi-gpio";
> +			reg = <0x8800>;
> +			gpio-controller;
> +			gpio-ranges = <&pmr735a_gpios 0 0 4>;
> +			#gpio-cells = <2>;
> +			interrupt-controller;
> +			#interrupt-cells = <2>;
> +		};
> +	};
> +};
>
Johan Hovold June 22, 2022, 1:09 p.m. UTC | #3
On Tue, Jun 21, 2022 at 09:12:22PM -0700, Bjorn Andersson wrote:
> Add basic support for the SC8280XP reference device, which allows it to
> boot to a shell (using EFIFB) with functional storage (UFS), USB,
> keyboard, touchpad, touchscreen, backlight and remoteprocs.
> 
> The PMICs are, per socinfo, reused from other platforms. But given that
> the address of the PMICs doesn't match other cases and that it's
> desirable to label things according to the schematics a new dtsi file is
> created to represent the reference combination of PMICs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Reordered "status" last
> - Fixed invalid PMIC gpio 0
> - Replaced "hid" name with touchscreen, touchpad and keyboard
> - Added &xo_board_clk frequency

> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>

> +#include <dt-bindings/input/gpio-keys.h>

This one is unused and should be dropped.

> +#include <dt-bindings/input/input.h>

And this one belongs in sc8280xp-pmics.dtsi where it's used.

> +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> +
> +#include "sc8280xp.dtsi"
> +#include "sc8280xp-pmics.dtsi"
> +
> +/ {
> +	model = "Qualcomm SC8280XP CRD";
> +	compatible = "qcom,sc8280xp-crd", "qcom,sc8280xp";
> +
> +	aliases {
> +		serial0 = &qup2_uart17;
> +	};
> +
> +	backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pmc8280c_lpg 3 1000000>;
> +		enable-gpios = <&pmc8280_1_gpios 8 GPIO_ACTIVE_HIGH>;
> +		power-supply = <&vreg_edp_bl>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&edp_bl_en>, <&edp_bl_pwm>;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	vreg_edp_bl: edp-bl-regulator {

The fixed regulator nodes should be renamed "regulator-edp-bl"...

> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_EDP_BL";
> +		regulator-min-microvolt = <3600000>;
> +		regulator-max-microvolt = <3600000>;
> +
> +		gpio = <&pmc8280_1_gpios 9 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&edp_bl_reg_en>;
> +
> +		regulator-boot-on;
> +	};
> +
> +	vreg_misc_3p3: misc-3p3-regulator {

...and "regulator-misc-3p3" (e.g. so we have a common prefix to sort
by).

> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_MISC_3P3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&pmc8280_1_gpios 1 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&misc_3p3_reg_en>;
> +
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};
> +
> +	reserved-memory {
> +	};
> +};

Johan
Johan Hovold June 22, 2022, 1:43 p.m. UTC | #4
On Wed, Jun 22, 2022 at 02:33:02PM +0200, Konrad Dybcio wrote:
> On 22.06.2022 06:12, Bjorn Andersson wrote:

> > +&qup2_i2c5 {
> > +	clock-frequency = <400000>;
> > +
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
> > +
> > +	status = "okay";
> > +
> I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?
> 

No, quite the opposite, status go at the end.

(And please break your lines at 72 cols or so).

> > +
> > +/* PINCTRL - additions to nodes defined in sc8280xp.dtsi */
> This comment seems redundant.

Nope, it's a marker that explains why the pinctrl nodes are seemingly
out of sort order. We've decided to group them all at the end.

But sure "- additions to nodes defined in sc8280xp.dtsi" could be moved
since we also have PMIC pinctrl nodes here (as I just did for the x13s
dts).

> 
> > +
> > +&pmc8280_1_gpios {
> > +	edp_bl_en: edp-bl-en-state {
> > +		pins = "gpio8";
> > +		function = "normal";
> > +	};
> > +
> > +	edp_bl_reg_en: edp-bl-reg-en-state {
> > +		pins = "gpio9";
> > +		function = "normal";
> > +	};
> > +
> > +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> > +		pins = "gpio1";
> > +		function = "normal";
> > +	};
> > +};

> > +	tpad_default: tpad-default-state {
> > +		int-n {
> If you aren't gonna add more pins to this touchpad block, I think you could drop this extra level.

You'd just lose information (what the pin is used for) with no real
gain.

> > +			pins = "gpio182";
> > +			function = "gpio";
> > +			bias-disable;
> > +		};
> > +	};

Johan
Konrad Dybcio June 22, 2022, 2:36 p.m. UTC | #5
On 22.06.2022 15:43, Johan Hovold wrote:
> On Wed, Jun 22, 2022 at 02:33:02PM +0200, Konrad Dybcio wrote:
>> On 22.06.2022 06:12, Bjorn Andersson wrote:
> 
>>> +&qup2_i2c5 {
>>> +	clock-frequency = <400000>;
>>> +
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
>>> +
>>> +	status = "okay";
>>> +
>> I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?
>>
> 
> No, quite the opposite, status go at the end.
Then all other device DTs should be updated, as in dts/qcom/
everybody keeps it first in non-SoC/PMIC files.

Konrad
> 
> (And please break your lines at 72 cols or so).
> 
>>> +
>>> +/* PINCTRL - additions to nodes defined in sc8280xp.dtsi */
>> This comment seems redundant.
> 
> Nope, it's a marker that explains why the pinctrl nodes are seemingly
> out of sort order. We've decided to group them all at the end.
> 
> But sure "- additions to nodes defined in sc8280xp.dtsi" could be moved
> since we also have PMIC pinctrl nodes here (as I just did for the x13s
> dts).
> 
>>
>>> +
>>> +&pmc8280_1_gpios {
>>> +	edp_bl_en: edp-bl-en-state {
>>> +		pins = "gpio8";
>>> +		function = "normal";
>>> +	};
>>> +
>>> +	edp_bl_reg_en: edp-bl-reg-en-state {
>>> +		pins = "gpio9";
>>> +		function = "normal";
>>> +	};
>>> +
>>> +	misc_3p3_reg_en: misc-3p3-reg-en-state {
>>> +		pins = "gpio1";
>>> +		function = "normal";
>>> +	};
>>> +};
> 
>>> +	tpad_default: tpad-default-state {
>>> +		int-n {
>> If you aren't gonna add more pins to this touchpad block, I think you could drop this extra level.
> 
> You'd just lose information (what the pin is used for) with no real
> gain.
> 
>>> +			pins = "gpio182";
>>> +			function = "gpio";
>>> +			bias-disable;
>>> +		};
>>> +	};
> 
> Johan
Johan Hovold June 22, 2022, 2:44 p.m. UTC | #6
On Wed, Jun 22, 2022 at 04:36:35PM +0200, Konrad Dybcio wrote:
> 
> 
> On 22.06.2022 15:43, Johan Hovold wrote:
> > On Wed, Jun 22, 2022 at 02:33:02PM +0200, Konrad Dybcio wrote:
> >> On 22.06.2022 06:12, Bjorn Andersson wrote:
> > 
> >>> +&qup2_i2c5 {
> >>> +	clock-frequency = <400000>;
> >>> +
> >>> +	pinctrl-names = "default";
> >>> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
> >>> +
> >>> +	status = "okay";
> >>> +
> >> I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?
> >>
> > 
> > No, quite the opposite, status go at the end.
> Then all other device DTs should be updated, as in dts/qcom/
> everybody keeps it first in non-SoC/PMIC files.

Seems like a lot of churn so maybe not worth it.

Johan
Krzysztof Kozlowski June 22, 2022, 2:48 p.m. UTC | #7
On 22/06/2022 16:36, Konrad Dybcio wrote:
> 
> 
> On 22.06.2022 15:43, Johan Hovold wrote:
>> On Wed, Jun 22, 2022 at 02:33:02PM +0200, Konrad Dybcio wrote:
>>> On 22.06.2022 06:12, Bjorn Andersson wrote:
>>
>>>> +&qup2_i2c5 {
>>>> +	clock-frequency = <400000>;
>>>> +
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
>>>> +
>>>> +	status = "okay";
>>>> +
>>> I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?
>>>
>>
>> No, quite the opposite, status go at the end.
> Then all other device DTs should be updated, as in dts/qcom/
> everybody keeps it first in non-SoC/PMIC files.

The word "should" is a bit too much here, but I agree, we can update all
of them to match one, chosen approach.

However the location for "status" property is more important for the
definition of nodes in DTSI, because it's the least important piece
there and also kind of expected - here go properties + I disable it. For
me this is more important.

For node redefinition in DTS, I see benefits in two approaches:
1. Let me first enable the node and then configure it.
2. Let me configure the node and enable it.

Best regards,
Krzysztof
Konrad Dybcio June 22, 2022, 3:10 p.m. UTC | #8
On 22.06.2022 16:48, Krzysztof Kozlowski wrote:
> On 22/06/2022 16:36, Konrad Dybcio wrote:
>>
>>
>> On 22.06.2022 15:43, Johan Hovold wrote:
>>> On Wed, Jun 22, 2022 at 02:33:02PM +0200, Konrad Dybcio wrote:
>>>> On 22.06.2022 06:12, Bjorn Andersson wrote:
>>>
>>>>> +&qup2_i2c5 {
>>>>> +	clock-frequency = <400000>;
>>>>> +
>>>>> +	pinctrl-names = "default";
>>>>> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
>>>>> +
>>>>> +	status = "okay";
>>>>> +
>>>> I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?
>>>>
>>>
>>> No, quite the opposite, status go at the end.
>> Then all other device DTs should be updated, as in dts/qcom/
>> everybody keeps it first in non-SoC/PMIC files.
> 
> The word "should" is a bit too much here, but I agree, we can update all
> of them to match one, chosen approach.
> 
> However the location for "status" property is more important for the
> definition of nodes in DTSI, because it's the least important piece
> there and also kind of expected - here go properties + I disable it. For
> me this is more important.
> 
> For node redefinition in DTS, I see benefits in two approaches:
> 1. Let me first enable the node and then configure it.
> 2. Let me configure the node and enable it.
I looked around non-qcom device trees and it looks like the common
consensus is 2. Although I personally visually prefer 1. and it's
been used in all qcom arm64 DTs to date, I don't think there are any
blockers for us to switch to 1. going forward to keep it consistent.

That's if we want to clean up the existing ones, as changing the rules
and not applying that to the older files will make for a huge mess as
time goes on and will unnecessarily prolong the review process (as
existing DTs are commonly a source of reference and people make
certain choices based on those).

I don't think the DTS specification or the Linux docs explicitly which
one to choose though.

Konrad
> 
> Best regards,
> Krzysztof
Johan Hovold June 22, 2022, 3:26 p.m. UTC | #9
On Wed, Jun 22, 2022 at 05:10:50PM +0200, Konrad Dybcio wrote:
> 
> 
> On 22.06.2022 16:48, Krzysztof Kozlowski wrote:
> > On 22/06/2022 16:36, Konrad Dybcio wrote:
> >>
> >>
> >> On 22.06.2022 15:43, Johan Hovold wrote:
> >>> On Wed, Jun 22, 2022 at 02:33:02PM +0200, Konrad Dybcio wrote:
> >>>> On 22.06.2022 06:12, Bjorn Andersson wrote:
> >>>
> >>>>> +&qup2_i2c5 {
> >>>>> +	clock-frequency = <400000>;
> >>>>> +
> >>>>> +	pinctrl-names = "default";
> >>>>> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
> >>>>> +
> >>>>> +	status = "okay";
> >>>>> +
> >>>> I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?
> >>>>
> >>>
> >>> No, quite the opposite, status go at the end.
> >> Then all other device DTs should be updated, as in dts/qcom/
> >> everybody keeps it first in non-SoC/PMIC files.
> > 
> > The word "should" is a bit too much here, but I agree, we can update all
> > of them to match one, chosen approach.
> > 
> > However the location for "status" property is more important for the
> > definition of nodes in DTSI, because it's the least important piece
> > there and also kind of expected - here go properties + I disable it. For
> > me this is more important.

Right, and this is the argument for keeping status last, something which
is well defined.

If you look at some of the qcom dtsi it's hard to determine whether a
node is disabled or not because the status property does not actually go
"first" but is rather typically mixed up somewhere in the middle (or
upper part) of nodes.

> > For node redefinition in DTS, I see benefits in two approaches:
> > 1. Let me first enable the node and then configure it.
> > 2. Let me configure the node and enable it.

So for consistency, just put status last everywhere (dtsi and dts) and
be done with it.

> I looked around non-qcom device trees and it looks like the common
> consensus is 2. Although I personally visually prefer 1. and it's
> been used in all qcom arm64 DTs to date, I don't think there are any
> blockers for us to switch to 1. going forward to keep it consistent.

You mean inconsistent with the majority of dts? ;)

> That's if we want to clean up the existing ones, as changing the rules
> and not applying that to the older files will make for a huge mess as
> time goes on and will unnecessarily prolong the review process (as
> existing DTs are commonly a source of reference and people make
> certain choices based on those).

That's a fair point. Consistency is good, and dt snipped tends to be
copied, but it's not the end of the world to not update old dts either.

> I don't think the DTS specification or the Linux docs explicitly which
> one to choose though.

No, but a praxis has been developed over time (e.g. compatible first,
reg second, status last).

Johan
Konrad Dybcio June 22, 2022, 3:30 p.m. UTC | #10
On 22.06.2022 17:26, Johan Hovold wrote:
> On Wed, Jun 22, 2022 at 05:10:50PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 22.06.2022 16:48, Krzysztof Kozlowski wrote:
>>> On 22/06/2022 16:36, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 22.06.2022 15:43, Johan Hovold wrote:
>>>>> On Wed, Jun 22, 2022 at 02:33:02PM +0200, Konrad Dybcio wrote:
>>>>>> On 22.06.2022 06:12, Bjorn Andersson wrote:
>>>>>
>>>>>>> +&qup2_i2c5 {
>>>>>>> +	clock-frequency = <400000>;
>>>>>>> +
>>>>>>> +	pinctrl-names = "default";
>>>>>>> +	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
>>>>>>> +
>>>>>>> +	status = "okay";
>>>>>>> +
>>>>>> I think all device DTs generally have 'status = "okay"' at the beginning. Should we change that?
>>>>>>
>>>>>
>>>>> No, quite the opposite, status go at the end.
>>>> Then all other device DTs should be updated, as in dts/qcom/
>>>> everybody keeps it first in non-SoC/PMIC files.
>>>
>>> The word "should" is a bit too much here, but I agree, we can update all
>>> of them to match one, chosen approach.
>>>
>>> However the location for "status" property is more important for the
>>> definition of nodes in DTSI, because it's the least important piece
>>> there and also kind of expected - here go properties + I disable it. For
>>> me this is more important.
> 
> Right, and this is the argument for keeping status last, something which
> is well defined.
> 
> If you look at some of the qcom dtsi it's hard to determine whether a
> node is disabled or not because the status property does not actually go
> "first" but is rather typically mixed up somewhere in the middle (or
> upper part) of nodes.
> 
>>> For node redefinition in DTS, I see benefits in two approaches:
>>> 1. Let me first enable the node and then configure it.
>>> 2. Let me configure the node and enable it.
> 
> So for consistency, just put status last everywhere (dtsi and dts) and
> be done with it.
That works.


> 
>> I looked around non-qcom device trees and it looks like the common
>> consensus is 2. Although I personally visually prefer 1. and it's
>> been used in all qcom arm64 DTs to date, I don't think there are any
>> blockers for us to switch to 1. going forward to keep it consistent.
> 
> You mean inconsistent with the majority of dts? ;)
Not like anything involving Qualcomm was ever consistent or compliant with the majority :D

Konrad
> 
>> That's if we want to clean up the existing ones, as changing the rules
>> and not applying that to the older files will make for a huge mess as
>> time goes on and will unnecessarily prolong the review process (as
>> existing DTs are commonly a source of reference and people make
>> certain choices based on those).
> 
> That's a fair point. Consistency is good, and dt snipped tends to be
> copied, but it's not the end of the world to not update old dts either.
> 
>> I don't think the DTS specification or the Linux docs explicitly which
>> one to choose though.
> 
> No, but a praxis has been developed over time (e.g. compatible first,
> reg second, status last).
> 
> Johan
Johan Hovold June 22, 2022, 3:37 p.m. UTC | #11
On Wed, Jun 22, 2022 at 05:30:24PM +0200, Konrad Dybcio wrote:
> On 22.06.2022 17:26, Johan Hovold wrote:
> > On Wed, Jun 22, 2022 at 05:10:50PM +0200, Konrad Dybcio wrote:
> >> On 22.06.2022 16:48, Krzysztof Kozlowski wrote:
> >>> On 22/06/2022 16:36, Konrad Dybcio wrote:
> >>>> On 22.06.2022 15:43, Johan Hovold wrote:

> >>>>> No, quite the opposite, status go at the end.
> >>>> Then all other device DTs should be updated, as in dts/qcom/
> >>>> everybody keeps it first in non-SoC/PMIC files.
> >>>
> >>> The word "should" is a bit too much here, but I agree, we can update all
> >>> of them to match one, chosen approach.
> >>>
> >>> However the location for "status" property is more important for the
> >>> definition of nodes in DTSI, because it's the least important piece
> >>> there and also kind of expected - here go properties + I disable it. For
> >>> me this is more important.
> > 
> > Right, and this is the argument for keeping status last, something which
> > is well defined.
> > 
> > If you look at some of the qcom dtsi it's hard to determine whether a
> > node is disabled or not because the status property does not actually go
> > "first" but is rather typically mixed up somewhere in the middle (or
> > upper part) of nodes.
> > 
> >>> For node redefinition in DTS, I see benefits in two approaches:
> >>> 1. Let me first enable the node and then configure it.
> >>> 2. Let me configure the node and enable it.
> > 
> > So for consistency, just put status last everywhere (dtsi and dts) and
> > be done with it.
> That works.

Actually, it looks like a lot of the qcom dtsi already do this too (i.e.
put status last). The dts may be more inconsistent on this matter
judging from a quick look.

> >> I looked around non-qcom device trees and it looks like the common
> >> consensus is 2. Although I personally visually prefer 1. and it's
> >> been used in all qcom arm64 DTs to date, I don't think there are any
> >> blockers for us to switch to 1. going forward to keep it consistent.
> > 
> > You mean inconsistent with the majority of dts? ;)
> Not like anything involving Qualcomm was ever consistent or compliant
> with the majority :D

Heh. :)

Johan
Konrad Dybcio June 22, 2022, 3:39 p.m. UTC | #12
On 22.06.2022 17:37, Johan Hovold wrote:
> On Wed, Jun 22, 2022 at 05:30:24PM +0200, Konrad Dybcio wrote:
>> On 22.06.2022 17:26, Johan Hovold wrote:
>>> On Wed, Jun 22, 2022 at 05:10:50PM +0200, Konrad Dybcio wrote:
>>>> On 22.06.2022 16:48, Krzysztof Kozlowski wrote:
>>>>> On 22/06/2022 16:36, Konrad Dybcio wrote:
>>>>>> On 22.06.2022 15:43, Johan Hovold wrote:
> 
>>>>>>> No, quite the opposite, status go at the end.
>>>>>> Then all other device DTs should be updated, as in dts/qcom/
>>>>>> everybody keeps it first in non-SoC/PMIC files.
>>>>>
>>>>> The word "should" is a bit too much here, but I agree, we can update all
>>>>> of them to match one, chosen approach.
>>>>>
>>>>> However the location for "status" property is more important for the
>>>>> definition of nodes in DTSI, because it's the least important piece
>>>>> there and also kind of expected - here go properties + I disable it. For
>>>>> me this is more important.
>>>
>>> Right, and this is the argument for keeping status last, something which
>>> is well defined.
>>>
>>> If you look at some of the qcom dtsi it's hard to determine whether a
>>> node is disabled or not because the status property does not actually go
>>> "first" but is rather typically mixed up somewhere in the middle (or
>>> upper part) of nodes.
>>>
>>>>> For node redefinition in DTS, I see benefits in two approaches:
>>>>> 1. Let me first enable the node and then configure it.
>>>>> 2. Let me configure the node and enable it.
>>>
>>> So for consistency, just put status last everywhere (dtsi and dts) and
>>> be done with it.
>> That works.
> 
> Actually, it looks like a lot of the qcom dtsi already do this too (i.e.
> put status last). The dts may be more inconsistent on this matter
> judging from a quick look.
Yes, as I mentioned this concerns the device-specific trees, as
the includable ones are (or well, should have been made) fine.

Konrad
> 
>>>> I looked around non-qcom device trees and it looks like the common
>>>> consensus is 2. Although I personally visually prefer 1. and it's
>>>> been used in all qcom arm64 DTs to date, I don't think there are any
>>>> blockers for us to switch to 1. going forward to keep it consistent.
>>>
>>> You mean inconsistent with the majority of dts? ;)
>> Not like anything involving Qualcomm was ever consistent or compliant
>> with the majority :D
> 
> Heh. :)
> 
> Johan
Bjorn Andersson June 22, 2022, 9 p.m. UTC | #13
On Wed 22 Jun 07:33 CDT 2022, Konrad Dybcio wrote:
> On 22.06.2022 06:12, Bjorn Andersson wrote:
[..]
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> > new file mode 100644
> > index 000000000000..36ed7d808ab8
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
> Is it the only configuration supported by Qualcomm, or only a reference one?
> 

It's the reference one, but reused on the devices I've seen so far.

Regards,
Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index 2f8aec2cc6db..ceeae094a59f 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -89,6 +89,7 @@  dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-herobrine-villager-r0.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-idp.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-idp2.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sc7280-crd-r3.dtb
+dtb-$(CONFIG_ARCH_QCOM)	+= sc8280xp-crd.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sdm630-sony-xperia-ganges-kirin.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sdm630-sony-xperia-nile-discovery.dtb
 dtb-$(CONFIG_ARCH_QCOM)	+= sdm630-sony-xperia-nile-pioneer.dtb
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
new file mode 100644
index 000000000000..38a64e886466
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -0,0 +1,432 @@ 
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/gpio-keys.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+
+#include "sc8280xp.dtsi"
+#include "sc8280xp-pmics.dtsi"
+
+/ {
+	model = "Qualcomm SC8280XP CRD";
+	compatible = "qcom,sc8280xp-crd", "qcom,sc8280xp";
+
+	aliases {
+		serial0 = &qup2_uart17;
+	};
+
+	backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pmc8280c_lpg 3 1000000>;
+		enable-gpios = <&pmc8280_1_gpios 8 GPIO_ACTIVE_HIGH>;
+		power-supply = <&vreg_edp_bl>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&edp_bl_en>, <&edp_bl_pwm>;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	vreg_edp_bl: edp-bl-regulator {
+		compatible = "regulator-fixed";
+
+		regulator-name = "VREG_EDP_BL";
+		regulator-min-microvolt = <3600000>;
+		regulator-max-microvolt = <3600000>;
+
+		gpio = <&pmc8280_1_gpios 9 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&edp_bl_reg_en>;
+
+		regulator-boot-on;
+	};
+
+	vreg_misc_3p3: misc-3p3-regulator {
+		compatible = "regulator-fixed";
+
+		regulator-name = "VREG_MISC_3P3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+
+		gpio = <&pmc8280_1_gpios 1 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&misc_3p3_reg_en>;
+
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
+	reserved-memory {
+	};
+};
+
+&apps_rsc {
+	pmc8280-1-rpmh-regulators {
+		compatible = "qcom,pm8350-rpmh-regulators";
+		qcom,pmic-id = "b";
+
+		vdd-l3-l5-supply = <&vreg_s11b>;
+
+		vreg_s11b: smps11 {
+			regulator-name = "vreg_s11b";
+			regulator-min-microvolt = <1272000>;
+			regulator-max-microvolt = <1272000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+		};
+
+		vreg_l3b: ldo3 {
+			regulator-name = "vreg_l3b";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+			regulator-boot-on;
+			regulator-always-on;
+		};
+
+		vreg_l4b: ldo4 {
+			regulator-name = "vreg_l4b";
+			regulator-min-microvolt = <912000>;
+			regulator-max-microvolt = <912000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+
+		vreg_l6b: ldo6 {
+			regulator-name = "vreg_l6b";
+			regulator-min-microvolt = <880000>;
+			regulator-max-microvolt = <880000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+			regulator-boot-on;
+		};
+	};
+
+	pmc8280c-rpmh-regulators {
+		compatible = "qcom,pm8350c-rpmh-regulators";
+		qcom,pmic-id = "c";
+
+		vreg_l1c: ldo1 {
+			regulator-name = "vreg_l1c";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+
+		vreg_l7c: ldo7 {
+			regulator-name = "vreg_l7c";
+			regulator-min-microvolt = <2504000>;
+			regulator-max-microvolt = <2504000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+
+		vreg_l13c: ldo13 {
+			regulator-name = "vreg_l13c";
+			regulator-min-microvolt = <3072000>;
+			regulator-max-microvolt = <3072000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+	};
+
+	pmc8280-2-rpmh-regulators {
+		compatible = "qcom,pm8350-rpmh-regulators";
+		qcom,pmic-id = "d";
+
+		vdd-l1-l4-supply = <&vreg_s11b>;
+
+		vreg_l3d: ldo3 {
+			regulator-name = "vreg_l3d";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+
+		vreg_l4d: ldo4 {
+			regulator-name = "vreg_l4d";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+
+		vreg_l6d: ldo6 {
+			regulator-name = "vreg_l6d";
+			regulator-min-microvolt = <880000>;
+			regulator-max-microvolt = <880000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+
+		vreg_l7d: ldo7 {
+			regulator-name = "vreg_l7d";
+			regulator-min-microvolt = <3072000>;
+			regulator-max-microvolt = <3072000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+
+		vreg_l9d: ldo9 {
+			regulator-name = "vreg_l9d";
+			regulator-min-microvolt = <912000>;
+			regulator-max-microvolt = <912000>;
+			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+			regulator-allow-set-load;
+		};
+	};
+};
+
+&pmc8280c_lpg {
+	status = "okay";
+};
+
+&pmk8280_pon_pwrkey {
+	status = "okay";
+};
+
+&qup0 {
+	status = "okay";
+};
+
+&qup0_i2c4 {
+	clock-frequency = <400000>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&qup0_i2c4_default>, <&ts0_default>;
+
+	status = "okay";
+
+	touchscreen@10 {
+		compatible = "hid-over-i2c";
+		reg = <0x10>;
+		hid-descr-addr = <0x1>;
+		interrupts-extended = <&tlmm 175 IRQ_TYPE_LEVEL_LOW>;
+		vdd-supply = <&vreg_misc_3p3>;
+	};
+};
+
+&qup1 {
+	status = "okay";
+};
+
+&qup2 {
+	status = "okay";
+};
+
+&qup2_i2c5 {
+	clock-frequency = <400000>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&qup2_i2c5_default>, <&kybd_default>, <&tpad_default>;
+
+	status = "okay";
+
+	touchpad@15 {
+		compatible = "hid-over-i2c";
+		reg = <0x15>;
+		hid-descr-addr = <0x1>;
+		interrupts-extended = <&tlmm 182 IRQ_TYPE_LEVEL_LOW>;
+		vdd-supply = <&vreg_misc_3p3>;
+	};
+
+	keyboard@68 {
+		compatible = "hid-over-i2c";
+		reg = <0x68>;
+		hid-descr-addr = <0x1>;
+		interrupts-extended = <&tlmm 104 IRQ_TYPE_LEVEL_LOW>;
+		vdd-supply = <&vreg_misc_3p3>;
+	};
+};
+
+&qup2_uart17 {
+	compatible = "qcom,geni-debug-uart";
+
+	status = "okay";
+};
+
+&remoteproc_adsp {
+	firmware-name = "qcom/sc8280xp/qcadsp8280.mbn";
+
+	status = "okay";
+};
+
+&remoteproc_nsp0 {
+	firmware-name = "qcom/sc8280xp/qccdsp8280.mbn";
+
+	status = "okay";
+};
+
+&ufs_mem_hc {
+	reset-gpios = <&tlmm 228 GPIO_ACTIVE_LOW>;
+
+	vcc-supply = <&vreg_l7c>;
+	vcc-max-microamp = <800000>;
+	vccq-supply = <&vreg_l3d>;
+	vccq-max-microamp = <900000>;
+
+	status = "okay";
+};
+
+&ufs_mem_phy {
+	vdda-phy-supply = <&vreg_l6b>;
+	vdda-pll-supply = <&vreg_l3b>;
+
+	status = "okay";
+};
+
+&usb_0 {
+	status = "okay";
+};
+
+&usb_0_dwc3 {
+	/* TODO: Define USB-C connector properly */
+	dr_mode = "host";
+};
+
+&usb_0_hsphy {
+	vdda-pll-supply = <&vreg_l9d>;
+	vdda18-supply = <&vreg_l1c>;
+	vdda33-supply = <&vreg_l7d>;
+
+	status = "okay";
+};
+
+&usb_0_qmpphy {
+	vdda-phy-supply = <&vreg_l9d>;
+	vdda-pll-supply = <&vreg_l4d>;
+
+	status = "okay";
+};
+
+&usb_1 {
+	status = "okay";
+};
+
+&usb_1_dwc3 {
+	/* TODO: Define USB-C connector properly */
+	dr_mode = "host";
+};
+
+&usb_1_hsphy {
+	vdda-pll-supply = <&vreg_l4b>;
+	vdda18-supply = <&vreg_l1c>;
+	vdda33-supply = <&vreg_l13c>;
+
+	status = "okay";
+};
+
+&usb_1_qmpphy {
+	vdda-phy-supply = <&vreg_l4b>;
+	vdda-pll-supply = <&vreg_l3b>;
+
+	status = "okay";
+};
+
+/* PINCTRL - additions to nodes defined in sc8280xp.dtsi */
+
+&pmc8280_1_gpios {
+	edp_bl_en: edp-bl-en-state {
+		pins = "gpio8";
+		function = "normal";
+	};
+
+	edp_bl_reg_en: edp-bl-reg-en-state {
+		pins = "gpio9";
+		function = "normal";
+	};
+
+	misc_3p3_reg_en: misc-3p3-reg-en-state {
+		pins = "gpio1";
+		function = "normal";
+	};
+};
+
+&pmc8280c_gpios {
+	edp_bl_pwm: edp-bl-pwm-state {
+		pins = "gpio8";
+		function = "func1";
+	};
+};
+
+&xo_board_clk {
+	clock-frequency = <38400000>;
+};
+
+&tlmm {
+	gpio-reserved-ranges = <74 6>, <83 4>, <125 2>, <128 2>, <154 7>;
+
+	kybd_default: kybd-default-state {
+		disable {
+			pins = "gpio102";
+			function = "gpio";
+			output-low;
+		};
+
+		int-n {
+			pins = "gpio104";
+			function = "gpio";
+			bias-disable;
+		};
+
+		reset {
+			pins = "gpio105";
+			function = "gpio";
+			bias-disable;
+		};
+	};
+
+	qup0_i2c4_default: qup0-i2c4-default-state {
+		pins = "gpio171", "gpio172";
+		function = "qup4";
+
+		bias-disable;
+		drive-strength = <16>;
+	};
+
+	qup2_i2c5_default: qup2-i2c5-default-state {
+		pins = "gpio81", "gpio82";
+		function = "qup21";
+
+		bias-disable;
+		drive-strength = <16>;
+	};
+
+	tpad_default: tpad-default-state {
+		int-n {
+			pins = "gpio182";
+			function = "gpio";
+			bias-disable;
+		};
+	};
+
+	ts0_default: ts0-default-state {
+		int-n {
+			pins = "gpio175";
+			function = "gpio";
+			bias-pull-up;
+		};
+
+		reset-n {
+			pins = "gpio99";
+			function = "gpio";
+			output-high;
+			drive-strength = <16>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
new file mode 100644
index 000000000000..36ed7d808ab8
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
@@ -0,0 +1,108 @@ 
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/spmi/spmi.h>
+
+&spmi_bus {
+	pmk8280: pmic@0 {
+		compatible = "qcom,pmk8350", "qcom,spmi-pmic";
+		reg = <0x0 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pmk8280_pon: pon@1300 {
+			compatible = "qcom,pm8998-pon";
+			reg = <0x1300>;
+
+			pmk8280_pon_pwrkey: pwrkey {
+				compatible = "qcom,pmk8350-pwrkey";
+				interrupts = <0x0 0x13 0x7 IRQ_TYPE_EDGE_BOTH>;
+				linux,code = <KEY_POWER>;
+				status = "disabled";
+			};
+		};
+	};
+
+	pmc8280_1: pmic@1 {
+		compatible = "qcom,pm8350", "qcom,spmi-pmic";
+		reg = <0x1 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pmc8280_1_gpios: gpio@8800 {
+			compatible = "qcom,pm8350-gpio", "qcom,spmi-gpio";
+			reg = <0x8800>;
+			gpio-controller;
+			gpio-ranges = <&pmc8280_1_gpios 0 0 10>;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+	};
+
+	pmc8280c: pmic@2 {
+		compatible = "qcom,pm8350c", "qcom,spmi-pmic";
+		reg = <0x2 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pmc8280c_gpios: gpio@8800 {
+			compatible = "qcom,pm8350c-gpio", "qcom,spmi-gpio";
+			reg = <0x8800>;
+			gpio-controller;
+			gpio-ranges = <&pmc8280c_gpios 0 0 9>;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		pmc8280c_lpg: lpg@e800 {
+			compatible = "qcom,pm8350c-pwm";
+			reg = <0xe800>;
+
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			#pwm-cells = <2>;
+
+			status = "disabled";
+		};
+	};
+
+	pmc8280_2: pmic@3 {
+		compatible = "qcom,pm8350", "qcom,spmi-pmic";
+		reg = <0x3 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pmc8280_2_gpios: gpio@8800 {
+			compatible = "qcom,pm8350-gpio", "qcom,spmi-gpio";
+			reg = <0x8800>;
+			gpio-controller;
+			gpio-ranges = <&pmc8280_2_gpios 0 0 10>;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+	};
+
+	pmr735a: pmic@4 {
+		compatible = "qcom,pmr735a", "qcom,spmi-pmic";
+		reg = <0x4 SPMI_USID>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		pmr735a_gpios: gpio@8800 {
+			compatible = "qcom,pmr735a-gpio", "qcom,spmi-gpio";
+			reg = <0x8800>;
+			gpio-controller;
+			gpio-ranges = <&pmr735a_gpios 0 0 4>;
+			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+	};
+};