Re: [PATCH v2 4/6] arm64: dts: qcom: starqltechn: enable more features

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 10/24/23 17:43, Dzmitry Sankouski wrote:
> - enable usb 2.0
> - enable debug uart (uart9)
> - enable touchscreen
> - enable ipa so that we can bring up mobile data
> 
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> Cc: Andy Gross <agross@kernel.org>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-msm@vger.kernel.org
> 
> ---
> 
> Changes in v2:
> - hex to decimal in regulator values
> - fix dtb_check warnings
> 
>   .../dts/qcom/sdm845-samsung-starqltechn.dts   | 173 +++++++++++++++++-
>   1 file changed, 171 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts b/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts
> index 6fc30fd1262b..f2bc3c412a2f 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-samsung-starqltechn.dts
> @@ -11,11 +11,17 @@
>   #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>   #include "sdm845.dtsi"
>   
> +/delete-node/ &rmtfs_mem;
> +
>   / {
>   	chassis-type = "handset";
>   	model = "Samsung Galaxy S9 SM-G9600";
>   	compatible = "samsung,starqltechn", "qcom,sdm845";
>   
> +	aliases {
> +		serial0 = &uart9;
> +	};
> +
>   	chosen {
>   		#address-cells = <2>;
>   		#size-cells = <2>;
> @@ -68,6 +74,93 @@ memory@a1300000 {
>   			ftrace-size = <0x40000>;
>   			pmsg-size = <0x40000>;
>   		};
> +
> +		/* The rmtfs_mem needs to be guarded due to "XPU limitations"
> +		 * it is otherwise possible for an allocation adjacent to the
> +		 * rmtfs_mem region to trigger an XPU violation, causing a crash.
> +		 */
> +		rmtfs_lower_guard: memory@fde00000 {
> +			no-map;
> +			reg = <0 0xfde00000 0 0x1000>;
> +		};
> +
> +		rmtfs_mem: rmtfs-mem@fde01000 {
> +			compatible = "qcom,rmtfs-mem";
> +			reg = <0 0xfde01000 0 0x200000>;
> +			no-map;
> +
> +			qcom,client-id = <1>;
> +			qcom,vmid = <15>;
> +		};
> +
> +		rmtfs_upper_guard: rmtfs-upper-guard@fe001000 {
> +			no-map;
> +			reg = <0 0xfe001000 0 0x1000>;
> +		};
> +
> +		/*
> +		 * It seems like reserving the old rmtfs_mem region is also needed to prevent
> +		 * random crashes which are most likely modem related, more testing needed.
> +		 */
> +		removed_region: removed-region@88f00000 {
> +			no-map;
> +			reg = <0 0x88f00000 0 0x1c00000>;
> +		};
> +	};
> +
> +	i2c@21 {
> +		compatible = "i2c-gpio";
> +		sda-gpios = <&tlmm 127 0x0>;
GPIO_ACTIVE_HIGH
> +		scl-gpios = <&tlmm 128 0x0>;
GPIO_ACTIVE_HIGH

> +		i2c-gpio,delay-us = <0x2>;
> +		#address-cells = <0x1>;
1

> +		#size-cells = <0x0>;
0

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&i2c21_sda_state &i2c21_scl_state>;
pinctrl-n
pinctrl-names

is prefered

and address/size-cells usually come as the last pair of properties

> +
> +		regulator@60 {
> +			compatible = "samsung,s2dos05";
> +			reg = <0x60>;
> +
> +			regulators {
> +				s2dos05_ldo1: s2dos05-ldo1 {
> +					regulator-name = "s2dos05-ldo1";
> +					regulator-min-microvolt = <1500000>;
> +					regulator-max-microvolt = <2000000>;
> +					regulator-active-discharge = <0x1>;
1

> +				};
> +
> +				s2dos05_ldo2: s2dos05-ldo2 {
> +					regulator-name = "s2dos05-ldo2";
> +					regulator-min-microvolt = <1800000>;
> +					regulator-max-microvolt = <1800000>;
> +					regulator-active-discharge = <0x1>;
1

> +					regulator-boot-on;
> +				};
> +
> +				s2dos05_ldo3: s2dos05-ldo3 {
> +					regulator-name = "s2dos05-ldo3";
> +					regulator-min-microvolt = <3000000>;
> +					regulator-max-microvolt = <3000000>;
> +					regulator-active-discharge = <0x1>;
1

> +					regulator-boot-on;
> +				};
> +
> +				s2dos05_ldo4: s2dos05-ldo4 {
> +					regulator-name = "s2dos05-ldo4";
> +					regulator-min-microvolt = <2700000>;
> +					regulator-max-microvolt = <3775000>;
> +					regulator-active-discharge = <0x1>;
1

> +				};
> +
> +				s2dos05_buck1: s2dos05-buck1 {
> +					regulator-name = "s2dos05-buck1";
> +					regulator-min-microvolt = <850000>;
> +					regulator-max-microvolt = <2100000>;
> +					regulator-active-discharge = <0x1>;
1

> +				};
> +			};
> +		};
>   	};
>   };
>   
> @@ -135,8 +228,6 @@ vdda_pll_cc_ebi23:
>   		vdda_sp_sensor:
>   		vdda_ufs1_core:
>   		vdda_ufs2_core:
> -		vdda_usb1_ss_core:
> -		vdda_usb2_ss_core:
>   		vreg_l1a_0p875: ldo1 {
>   			regulator-min-microvolt = <880000>;
>   			regulator-max-microvolt = <880000>;
> @@ -157,6 +248,7 @@ vreg_l3a_1p0: ldo3 {
>   			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>   		};
>   
> +		vdda_usb1_ss_core:
>   		vdd_wcss_cx:
>   		vdd_wcss_mx:
>   		vdda_wcss_pll:
> @@ -365,6 +457,10 @@ &qupv3_id_1 {
>   	status = "okay";
>   };
>   
> +&gpi_dma1 {
> +	status = "okay";
> +};
> +
>   &uart9 {
>   	status = "okay";
>   };
> @@ -391,13 +487,50 @@ &sdhc_2 {
>   	status = "okay";
>   };
>   
> +&i2c11 {
> +	status = "okay";
> +	clock-frequency = <400000>;
status is preferred to come last

> +
> +	touchscreen@48 {
> +		compatible = "samsung,s6sy761";
> +		reg = <0x48>;
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <120 0x0>;
interrupts-extended with a fixed irq type (not _NONE)

> +		vdd-supply = <&s2dos05_ldo2>;
> +		avdd-supply = <&s2dos05_ldo3>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&touch_irq_state>;
ditto

> +	};
> +};
> +
> +/* Modem/wifi*/
missing space before */

> +&mss_pil {
> +	status = "okay";
> +	firmware-name = "qcom/sdm845/starqltechn/mba.mbn", "qcom/sdm845/starqltechn/modem.mbn";
Can you split these into two lines?

> +};
> +
> +&ipa {
> +	qcom,gsi-loader = "self";
> +	memory-region = <&ipa_fw_mem>;
> +	firmware-name = "qcom/sdm845/starqltechn/ipa_fws.mbn";
> +	status = "okay";
> +};
> +
>   &usb_1 {
>   	status = "okay";
> +	/*
> +	 * disable USB3 clock requirement as the device only supports
> +	 * USB2.
> +	 */
Can this comment fit into a /* single line */?
> +	qcom,select-utmi-as-pipe-clk;
status is supposed to be last

>   };
>   
>   &usb_1_dwc3 {
>   	/* Until we have Type C hooked up we'll force this as peripheral. */
>   	dr_mode = "peripheral";
> +
> +	maximum-speed = "high-speed";
>   };
>   
>   &usb_1_hsphy {
> @@ -449,4 +582,40 @@ sd_card_det_n_state: sd-card-det-n-state {
>   		function = "gpio";
>   		bias-pull-up;
>   	};
> +
> +	i2c21_sda_state: i2c-sda-state {
> +		pins = "gpio127";
> +		function = "gpio";
> +		drive-strength = <0x2>;
2

> +		bias-disable;
> +	};
> +
> +	i2c21_scl_state: i2c-scl-state {
> +		pins = "gpio128";
> +		function = "gpio";
> +		drive-strength = <0x2>;
2

> +		bias-disable;
> +	};
> +
> +	touch_irq_state: touch-irq-state {
> +		pins = "gpio120";
> +		function = "gpio";
> +		bias-disable;
> +		output-disable;
> +	};
> +};
> +
> +&qup_uart9_tx {
> +	drive-strength = <0x2>;
2

> +	bias-pull-up;
> +};
> +
> +&qup_uart9_rx {
> +	drive-strength = <0x2>;
2

> +	bias-pull-up;
> +};
> +
> +&qup_i2c11_default {
> +	drive-strength = <2>;
like here

Konrad