On Mar 12, 2015, at 12:05 PM, Mark Rutland [off-list ref] wrote:

Hi Kumar,

quoted

+/ {
+	model = "Qualcomm Technologies, Inc. MSM 8916 MTP";
+	compatible = "qcom,msm8916-mtp", "qcom,msm8916-mtp-smb1360",
+			"qcom,msm8916", "qcom,mtp";
+};

No /chosen/stdout-path?

Nope ;).

Does your UART driver support earlycon?

It does.

[...]

quoted

+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		CPU0: cpu at 0 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x0>;
+		};
+
+		CPU1: cpu at 1 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x1>;
+		};
+
+		CPU2: cpu at 2 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x2>;
+		};
+
+		CPU3: cpu at 3 {
+			device_type = "cpu";
+			compatible = "arm,cortex-a53", "arm,armv8";
+			reg = <0x3>;
+		};
+	};

The secondary CPUs need an enable-method. Are you using PSCI or
spin-table?

This is on purpose.  We aren?t using either PSCI or spin-table.  Right now the dts is for booting on a single core.  I can drop CPU1..CPU3 if that helps.

Which exception level do the CPUs enter the kernel?

quoted

+	timer {
+		compatible = "arm,armv7-timer";

This should be "arm,armv8-timer?.

will change

quoted

+		interrupts = <GIC_PPI 2 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 3 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
+		clock-frequency = <19200000>;
+	};

NAK. CNTFRQ should be programmed on all CPUs prior to entering the
kernel, per the boot protocol. You should not need clock-frequency here.

Will drop clock-frequency.

[...]

quoted

+		intc: interrupt-controller at b000000 {
+			compatible = "qcom,msm-qgic2";

This string isn't documented (but seems to be supported by the GIC
driver).

There?s a patch posted to add ?qcom,msm-qgic2? to the binding doc.

How does this differ from other GIC implementations?

Not sure the exact details, just that its qcom?s on implementation of the GIC spec.

quoted

+			interrupt-controller;
+			#interrupt-cells = <3>;
+			reg = <0x0b000000 0x1000>, <0x0b002000 0x1000>;
+		};

No GICH, GICV, maintenance interrupt?

Nope.

Minor nit, but I'd prefer if the reg entries were on individual lines as
happens in other dts.

Thanks,
Mark.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project