Re: [PATCH 1/2] RISC-V: virt: riscvemu: use new-style DT overlay syntax

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 20.02.23 09:37, Marco Felsch wrote:
> Hi Ahmad,
> 
> On 23-02-17, Ahmad Fatoum wrote:
>> DTC nowdays also supports a much less verbose syntax for DT overlays
>> that is internally converted to the usual much more verbose fragment
>> syntax. Switch to it.
>>
>> No functional change intended.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  .../riscv/boards/riscvemu/overlay-of-sram.dts | 197 ++++++++----------
>>  1 file changed, 90 insertions(+), 107 deletions(-)
>>
>> diff --git a/arch/riscv/boards/riscvemu/overlay-of-sram.dts b/arch/riscv/boards/riscvemu/overlay-of-sram.dts
>> index 092fb02518b9..395fde84c1a9 100644
>> --- a/arch/riscv/boards/riscvemu/overlay-of-sram.dts
>> +++ b/arch/riscv/boards/riscvemu/overlay-of-sram.dts
>> @@ -3,127 +3,110 @@
>>  /dts-v1/;
>>  /plugin/;
>>  
>> -/ {
>> -	fragment@0 {
>> -		target-path = "/soc";
>> -		__overlay__ {
>> -			#address-cells = <2>;
>> -			#size-cells = <2>;
>> -			sram@1000 {
>> -				compatible = "mtd-ram";
>> -				reg = <0 0x1000 0 0x10000>;
>> -				#address-cells = <1>;
>> -				#size-cells = <1>;
>> +&{/soc} {
> 
> We could also move everything under the root node right? So the
> following is also possible:
> 
> &{/} {
> 	chosen {
> 		environment {
> 		};
> 	};
> 	soc {
> 	};
> };
> 
> If that is the case I would change it to the above syntax instead of
> having several ones. Apart from that the change looks good to me.

I'd rather be explicit. For example htif below is supposed to be under
SoC IMO, yet riscvemu places it under /. Being explicit at least gives
a warning at runtime. I could move some stuff under / { } and leave
override others by symbol, but what does this improve?

> 
> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de>
> 
>> +	#address-cells = <2>;
>> +	#size-cells = <2>;
>> +	sram@1000 {
>> +		compatible = "mtd-ram";
>> +		reg = <0 0x1000 0 0x10000>;
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +
>> +		partitions {
>> +			compatible = "fixed-partitions";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +
>> +			partition@0 {
>> +				label = "bootrom";
>> +				reg = <0x0 0x40>;
>> +			};
>>  
>> -				partitions {
>> -					compatible = "fixed-partitions";
>> -					#address-cells = <1>;
>> -					#size-cells = <1>;
>> -
>> -					partition@0 {
>> -						label = "bootrom";
>> -						reg = <0x0 0x40>;
>> -					};
>> -
>> -					partition@40 {
>> -						label = "fdt";
>> -						reg = <0x40 0x1fc0>;
>> -					};
>> -
>> -					environment_sram: partition@3000 {
>> -						label = "barebox-environment";
>> -						reg = <0x3000 0xb000>;
>> -					};
>> -
>> -					backend_state_sram: partition@e000 {
>> -						label = "barebox-state";
>> -						reg = <0xe000 0x1000>;
>> -					};
>> -				};
>> +			partition@40 {
>> +				label = "fdt";
>> +				reg = <0x40 0x1fc0>;
>> +			};
>> +
>> +			environment_sram: partition@3000 {
>> +				label = "barebox-environment";
>> +				reg = <0x3000 0xb000>;
>>  			};
>> -		};
>> -	};
>>  
>> -	fragment@2 {
>> -		target-path = "/chosen";
>> -		__overlay__ {
>> -			environment {
>> -				compatible = "barebox,environment";
>> -				device-path = "/soc/sram@1000/partitions/partition@3000";
>> +			backend_state_sram: partition@e000 {
>> +				label = "barebox-state";
>> +				reg = <0xe000 0x1000>;
>>  			};
>>  		};
>>  	};
>> +};
>>  
>> -	fragment@3 {
>> -		target-path = "/";
>> -		__overlay__ {
>> -			aliases {
>> -				state = "/state";
>> -			};
>> +&{/chosen} {
>> +	environment {
>> +		compatible = "barebox,environment";
>> +		device-path = "/soc/sram@1000/partitions/partition@3000";
>> +	};
>> +};
>> +
>> +&{/} {
>> +	aliases {
>> +		state = "/state";
>> +	};
>> +
>> +	state {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "barebox,state";
>> +		magic = <0x290cf8c6>;
>> +		backend-type = "raw";
>> +		backend = <&backend_state_sram>;
>> +		backend-stridesize = <64>;
>> +
>> +		bootstate {
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>>  
>> -			state {
>> +			system0 {
>>  				#address-cells = <1>;
>>  				#size-cells = <1>;
>> -				compatible = "barebox,state";
>> -				magic = <0x290cf8c6>;
>> -				backend-type = "raw";
>> -				backend = <&backend_state_sram>;
>> -				backend-stridesize = <64>;
>> -
>> -				bootstate {
>> -					#address-cells = <1>;
>> -					#size-cells = <1>;
>> -
>> -					system0 {
>> -						#address-cells = <1>;
>> -						#size-cells = <1>;
>> -
>> -						remaining_attempts@0 {
>> -							reg = <0x0 0x4>;
>> -							type = "uint32";
>> -							default = <3>;
>> -						};
>> -
>> -						priority@4 {
>> -							reg = <0x4 0x4>;
>> -							type = "uint32";
>> -							default = <20>;
>> -						};
>> -					};
>> -
>> -					system1 {
>> -						#address-cells = <1>;
>> -						#size-cells = <1>;
>> -
>> -						remaining_attempts@8 {
>> -							reg = <0x8 0x4>;
>> -							type = "uint32";
>> -							default = <3>;
>> -						};
>> -
>> -						priority@c {
>> -							reg = <0xc 0x4>;
>> -							type = "uint32";
>> -							default = <21>;
>> -						};
>> -					};
>> -
>> -					last_chosen@10 {
>> -						reg = <0x10 0x4>;
>> -						type = "uint32";
>> -					};
>> +
>> +				remaining_attempts@0 {
>> +					reg = <0x0 0x4>;
>> +					type = "uint32";
>> +					default = <3>;
>> +				};
>> +
>> +				priority@4 {
>> +					reg = <0x4 0x4>;
>> +					type = "uint32";
>> +					default = <20>;
>>  				};
>>  			};
>> -		};
>> -	};
>>  
>> -	fragment@4 {
>> -		target-path = "/htif";
>> -		#address-cells = <2>;
>> -		#size-cells = <2>;
>> +			system1 {
>> +				#address-cells = <1>;
>> +				#size-cells = <1>;
>>  
>> -		__overlay__ {
>> -			reg = <0 0x40008000 0 0x8>;
>> +				remaining_attempts@8 {
>> +					reg = <0x8 0x4>;
>> +					type = "uint32";
>> +					default = <3>;
>> +				};
>> +
>> +				priority@c {
>> +					reg = <0xc 0x4>;
>> +					type = "uint32";
>> +					default = <21>;
>> +				};
>> +			};
>> +
>> +			last_chosen@10 {
>> +				reg = <0x10 0x4>;
>> +				type = "uint32";
>> +			};
>>  		};
>>  	};
>>  };
>> +
>> +&{/htif} {
>> +	reg = <0 0x40008000 0 0x8>;
>> +};
>> -- 
>> 2.30.2
>>
>>
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |