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

[Paul Mackerras <paulus@ozlabs.org>]

On Thu, Jun 17, 2021 at 02:41:28PM +1000, Michael Ellerman wrote:
> Paul Mackerras <paulus@ozlabs.org> writes:
> >
> 
> Little bit of change log never hurts :)
> 
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> > ---
> >  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).

> > +				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.

> > +		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).

> > +			clock-frequency = <100000000>;
> > +			timebase-frequency = <100000000>;
> 
> Those seem quite high?

No, 100MHz is correct.

> > +			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.
> 
> > +			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.

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

Nope.

> > +			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.
> 
> > +			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.