Re: [PATCH 2/2] arm64: dts: qcom: sm8650: Add initial Valve Deckard devicetree

[Neil Armstrong <neil.armstrong@linaro.org>]

Hi,

On 9/22/26 13:39, Konrad Dybcio wrote:
> On 9/22/26 1:28 PM, Neil Armstrong wrote:
>> Add an initial DeviceTree for the Valve "Steam Frame"
>> codename Deckard. The Steam Frame is a virtual reality headset
>> based on the Qualcomm Snapdragon 8 Gen 3 SoC (SM8650).
>>
>> Providing initial support for:
>> - System reserved memory, regulators, gpio keys, PMIC functions..
>> - PMIC ADC Channels
>> - UFS Storage
>> - USB Gadget
>> - WCN7850 Wifi7 + Bluetooth
>> - I2C + basic peripherals
>> - SDCard slot
>>
>> The bootloader starts Linux in EL2, thus a bunch of EL1 & DSP related
>> nodes needs to be removed and the reserved-memory mapping must
>> be defined.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
> 
> [...]
> 
>> +/* Remove nodes */
>> +/delete-node/ &ipa;
>> +/delete-node/ &gpu_zap_shader;
>> +/delete-node/ &lpass_wsa2macro;
>> +/delete-node/ &lpass_rxmacro;
>> +/delete-node/ &lpass_txmacro;
>> +/delete-node/ &lpass_wsamacro;
>> +/delete-node/ &lpass_vamacro;
>> +/delete-node/ &lpass_tlmm;
>> +/delete-node/ &remoteproc_adsp;
>> +/delete-node/ &remoteproc_cdsp;
>> +/delete-node/ &remoteproc_mpss;
>> +/delete-node/ &reserved_memory;
>> +/delete-node/ &swr0;
>> +/delete-node/ &swr1;
>> +/delete-node/ &swr2;
>> +/delete-node/ &swr3;
> 
> Hm, maybe we should think about sm8650-audioreach (simialrly to
> how we have a split for the kodiak variants)

I will probably go in this direction when adding support for audio.

> 
> 
>> +
>> +/ {
>> +	model = "Valve Deckard";
>> +	compatible = "valve,deckard", "qcom,sm8650";
>> +
>> +	aliases {
>> +		serial0 = &uart15;
>> +		serial1 = &uart14;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +
>> +		framebuffer {
>> +			compatible = "simple-framebuffer";
>> +			width = <4320>;
>> +			height = <2160>;
>> +			stride = <(4320 * 4)>;
>> +			format = "a8r8g8b8";
>> +			memory-region = <&framebuffer_mem>;
>> +
>> +			/* Bootloader will enable the node */
>> +			status = "disabled";
> 
> Is there a reason not to enable it? Does the bootloader prefer to
> only enable it sometiems?

Exact

> 
> [...]
> 
>> +	reserved-memory {
>> +		ranges;
>> +
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
> 
> it's usually:
> 
> addr
> size
> ranges
> <newline>

Ack

> 
> [...]
> 
>> +
>> +		aop_mem: aop-region@81c00000 {
>> +			no-map;
>> +			reg = <0x0 0x81c00000 0x0 0x60000>;
> 
> 0.dtsi

Good catch

> 
>> +		};
>> +
>> +		ramoops: ramoops-region@84a00000 {
>> +			compatible = "ramoops";
>> +			reg = <0 0x84a00000 0 0x100000>;
>> +			record-size = <0x40000>;
>> +			console-size = <0x40000>;
>> +			pmsg-size = <0x1000>;
>> +			ecc-size = <16>;
>> +		};
>> +
>> +		aop_cmd_db_mem: aop-cmd-db-region@81c60000 {
>> +			compatible = "qcom,cmd-db";
>> +			reg = <0x0 0x81c60000 0x0 0x20000>;
>> +			no-map;
>> +		};
> 
> Likewise many of these reservations also exist there

Not sure slitting this would be worth it.

> 
> [...]
> 
>> +	/delete-node/ smp2p-adsp;
>> +	/delete-node/ smp2p-cdsp;
>> +	/delete-node/ smp2p-modem;
> 
> I think these shouldn't hurt even if your usecase is different

Right, but those describes a pure firmware interface.

>> +
>> +	wcn7850-pmu {
>> +		compatible = "qcom,wcn7850-pmu";
>> +
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&wlan_en>, <&bt_default>;
> 
> nit:
> 
> property-n
> property-names

Will fix

> 
> please
> 
> otherwise looks pretty good
> 
> Konrad

Thanks,
Neil