On 10/25/23 16:24, Sibi Sankar wrote:
From: Rajendra Nayak <quic_rjendra@xxxxxxxxxxx> Add base dtsi and QCP board (Qualcomm Compute Platform) dts file for SC8380XP SoC, describing the CPUs, GCC and RPMHCC clock controllers, geni UART, interrupt controller, TLMM, reserved memory, interconnects, SMMU and LLCC nodes. Co-developed-by: Abel Vesa <abel.vesa@xxxxxxxxxx> Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx> Signed-off-by: Rajendra Nayak <quic_rjendra@xxxxxxxxxxx> Co-developed-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx> Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx> ---
[...]
+&tlmm { + gpio-reserved-ranges = <33 3>, <44 4>, <238 1>;
It would be really cool if you added an explanation on why these GPIOs need to be reserved, especially since you can see what's connected on there on schematics. So, like: gpio-reserved-ranges = <33 3>, /* something */ <44 4>, /* something else (fp scanner?) <238 1>; /* UFS reset? */ [...]
+ compatible = "qcom,oryon";
Again, this compatible won't fly unless all of these cores are totally identical and Oryon is only a name for this generation on this SoC (which I believe not to be the case).
+ reg = <0x0 0x0>; + enable-method = "psci"; + next-level-cache = <&L1_0>; + + L1_0: l1-cache { + compatible = "cache";
I'm not sure if L1 is supposed to be described in the DT, Krzysztof should know.
+ next-level-cache = <&L2_0>; + + L2_0: l2-cache-0 { + compatible = "cache";
cache-level? cache-unified? [...]
+ memory@80000000 { + device_type = "memory"; + /* We expect the bootloader to fill in the size */ + reg = <0 0x80000000 0 0x80000000>;
That contradicts the comment you made above. Plus, 2 GiB seems a bit low for this SoC :D [...]
+ gunyah_hyp_mem: gunyah-hyp-region@80000000 {
you can probably strip the "-region" part, as this is implied by being a child of /reserved-memory
+ pld_pep_mem: pld-pep-region@81f30000 {
What's PLD? What's this region used for? PEP is a Windows invention. [...]
+ av1_encoder_mem: av1-encoder-region@8e900000 {
Is AV1enc hardware separate from iris? [...]
+ gcc: clock-controller@100000 { + compatible = "qcom,sc8380xp-gcc"; + reg = <0 0x100000 0 0x200000>;
The address part of reg should be padded to 8 hex digits.
+ + interconnects = <&clk_virt MASTER_QUP_CORE_2 0 &clk_virt SLAVE_QUP_CORE_2 0>,
QCOM_ICC_TAG_ALWAYS would be nicer than 0 (see sa8775p) [...]
+ + interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>,
One space after and before '=' Konrad