Re: [PATCH 02/11] powerpc: Add Microwatt device tree

On Thu, Jun 17, 2021 at 02:41:28PM +1000, Michael Ellerman wrote:

Paul Mackerras [off-list ref] writes:

quoted

Little bit of change log never hurts :)

quoted

Signed-off-by: Paul Mackerras <redacted>
---
 arch/powerpc/boot/dts/microwatt.dts | 105 ++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
 create mode 100644 arch/powerpc/boot/dts/microwatt.dts

diff --git a/arch/powerpc/boot/dts/microwatt.dts b/arch/powerpc/boot/dts/microwatt.dts
new file mode 100644
index 000000000000..9b2e64da9432
--- /dev/null
+++ b/arch/powerpc/boot/dts/microwatt.dts

@@ -0,0 +1,105 @@
+/dts-v1/;
+
+/ {
+	#size-cells = <0x02>;
+	#address-cells = <0x02>;
+	model-name = "microwatt";
+	compatible = "microwatt-soc";
+
+	reserved-memory {
+		#size-cells = <0x02>;
+		#address-cells = <0x02>;
+		ranges;
+	};
+
+	memory@0 {
+		device_type = "memory";
+		reg = <0x00000000 0x00000000 0x00000000 0x10000000>;
+	};
+
+	cpus {
+		#size-cells = <0x00>;
+		#address-cells = <0x01>;
+
+		ibm,powerpc-cpu-features {
+			display-name = "Microwatt";
+			isa = <3000>;
+			device_type = "cpu-features";
+			compatible = "ibm,powerpc-cpu-features";
+
+			mmu-radix {
+				isa = <3000>;
+				usable-privilege = <2>;

skiboot says 6?

That's for a machine with hypervisor mode - if I make it 6 here, then
the kernel prints a message about "HV feature passed to guest" and
then another about "missing dependency" and ends up not enabling the
feature.

Note that microwatt usually has MSR[HV] = 0 (you can set it to 1 but
it doesn't do anything).  Arguably it should force it to 1 always, but
if I do that, then the kernel starts trying to execute hrfid
instructions, which microwatt doesn't have (for example in
masked_Hinterrupt).

quoted

+				os-support = <0x00>;
+			};
+
+			little-endian {
+				isa = <0>;

I guess you just copied that from skiboot.

The binding says it's required, but AFAICS the kernel doesn't use it.

And isa = 0 mean ISA_BASE, according to the skiboot source.

I changed it to 2050 since true little-endian mode was introduced for
POWER6.

quoted

+		PowerPC,Microwatt@0 {
+			i-cache-sets = <2>;
+			ibm,dec-bits = <64>;
+			reservation-granule-size = <64>;

Never seen that one before.

It's in PAPR+ (D.6.1.4, CPU Node Properties).

quoted

+			clock-frequency = <100000000>;
+			timebase-frequency = <100000000>;

Those seem quite high?

No, 100MHz is correct.

quoted

+			i-tlb-sets = <1>;
+			ibm,ppc-interrupt-server#s = <0>;
+			i-cache-block-size = <64>;
+			d-cache-block-size = <64>;

The kernel reads those, but also hard codes 128 in places.

Interesting, because it all seems to work.  I assume the critical
thing is doing the right dcbz's.

See L1_CACHE_BYTES.

quoted

+			ibm,pa-features = [40 00 c2 27 00 00 00 80 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80 00 80 00 80 00 00 00 80 00 80 00 00 00 80 00 80 00 80 00 80 00 80 00 80 00 80 00 80 00 80 00 80 00 80 00 80 00 80 00];

Do you need that?

You shouldn't, if we've done things right with the cpu-features support.

Turns out I don't need it.

quoted

+			d-cache-sets = <2>;
+			ibm,pir = <0x3c>;

Needed?

Nope.

quoted

+			i-tlb-size = <64>;
+			cpu-version = <0x990000>;
+			status = "okay";
+			i-cache-size = <0x1000>;
+			ibm,processor-radix-AP-encodings = <0x0c 0xa0000010 0x20000015 0x4000001e>;
+			tlb-size = <0>;
+			tlb-sets = <0>;

Does the kernel use those? I can't find it.

quoted

+			device_type = "cpu";
+			d-tlb-size = <128>;
+			d-tlb-sets = <2>;
+			reg = <0>;
+			general-purpose;
+			64-bit;
+			d-cache-size = <0x1000>;
+			ibm,chip-id = <0x00>;
+		};
+	};
+
+	chosen {
+		bootargs = "";
+		ibm,architecture-vec-5 = [19 00 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 40 00 40];

Do you need that?

I assume you run with MSR[HV] = 1 (you don't say anywhere), in which
case we never look at that property.

I do need that given we're running with MSR[HV] = 0; without that the
kernel assumes HPT mode.

Paul.