Re: [PATCH] riscv: Add support for AMD/Xilinx MicroBlaze V

[Michal Simek <michal.simek@amd.com>]

Hi Conor,

On 11/3/23 17:25, Conor Dooley wrote:
> Yo,
> 
> Since you sent it to me, I may as well comment...
> 
> On Fri, Nov 03, 2023 at 05:03:25PM +0100, Michal Simek wrote:
>> MicroBlaze V is new AMD/Xilinx soft-core 32bit RISC-V processor IP.
>> It is hardware compatible with classic MicroBlaze processor.
>>
>> The patch contains initial wiring and configuration for initial HW design
>> with memory, cpu, interrupt controller, timers and uartlite console.
>>
>> Provided DT is just describing one configuration and should be taken only
>> as example.
> 
> 
>> @@ -0,0 +1,86 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * dts file for Xilinx MicroBlaze V
>> + *
>> + * (C) Copyright 2023, Advanced Micro Devices, Inc.
>> + *
>> + * Michal Simek <michal.simek@amd.com>
>> + */
>> +
>> +/dts-v1/;
>> +/ {
>> +	#address-cells = <1>;
>> +	#size-cells = <1>;
>> +	model = "Xilinx MicroBlaze V 32bit";
>> +	compatible = "xlnx,mbv32";
>> +
>> +	cpus: cpus {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		timebase-frequency = <102000000>;
>> +		cpu_0: cpu@0 {
>> +			device_type = "cpu";
>> +			compatible = "riscv";
> 
> You're missing a cpu specific compatible here.
> "riscv" in isolation is only for {emu,simu}lators.

Will add.

> 
>> +			reg = <0>;
>> +			status = "okay";
>> +			riscv,isa = "rv32imafdc";
>> +			clock-frequency = <100000000>;
>> +			i-cache-size = <32768>;
>> +			d-cache-size = <32768>;
> 
> Missing an interrupt-controller child node for the cpu-intc, no?

For u-boot interrupt controller is not needed but it is listed below that's why 
will try to wire it.

> 
>> +		};
>> +	};
>> +
>> +	aliases {
>> +		serial0 = &uart0;
>> +	};
>> +
>> +	chosen {
>> +		bootargs = "earlycon";
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +
>> +	memory@20000000 {
>> +		device_type = "memory";
>> +		reg = <0x20000000 0x20000000>;
>> +	};
>> +
>> +	axi: axi {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "simple-bus";
>> +		ranges;
>> +		bootph-all;
>> +
>> +		axi_intc: interrupt-controller@41200000 {
>> +			compatible = "xlnx.xps-intc";
> 
> This is some non-standard interrupt controller, rather than a plic,
> right?

non-standard riscv interrupt controller. But standard microblaze interrrupt 
controller. And of course it is not plic compatible. Based on my discussion with 
HW designers it shouldn't be a problem to use this one.

> Also, should you not also have a riscv,timer node?

It is design from Amd perspective as option to exchange microblaze with riscv 
that's why all IPs around it are the same. It means xps timer is used instead.

Thanks,
Michal