Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6d40876c-2751-01bb-94ab-7c9ab90e636f@gmail.com>
Date:   Fri, 24 Jun 2022 11:44:07 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH 1/2] of: create of_root if no dtb provided
Content-Language: en-US
To:     =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>
Cc:     Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, Lizhi Hou <lizhi.hou@xilinx.com>,
        Allan Nielsen <allan.nielsen@microchip.com>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Steen Hegelund <steen.hegelund@microchip.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
References: <20220624034327.2542112-1-frowand.list@gmail.com>
 <20220624034327.2542112-2-frowand.list@gmail.com>
 <20220624141320.3c473605@fixe.home>
From:   Frank Rowand <frowand.list@gmail.com>
In-Reply-To: <20220624141320.3c473605@fixe.home>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 6/24/22 08:13, Clément Léger wrote:
> Le Thu, 23 Jun 2022 22:43:26 -0500,
> frowand.list@gmail.com a écrit :
> 
>>  
>> +/*
>> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
>> + * scripts/Makefile.lib
>> + */
>> +extern void *__dtb_empty_root_begin;
>> +
>>  /*
>>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>>   * @limit: maximum entries
>> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
>>   */
>>  void __init unflatten_device_tree(void)
>>  {
> 
> Hi Frank,
> 
> This function is only defined when CONFIG_OF_EARLY_FLATTREE is enabled.

More precisely, only if CONFIG_OF_FLATTREE is enabled.  But that would
most likely be seleved by CONFIG_OF_EARLY_FLATTREE, so in practice the
issue you raise is valid.

> Which means that on platforms that do not select this, the default
> empty device-tree creation will not be done.

Yes, so platforms that need this functionality need to select this
option.

> 
> This configuration option is selected by the platform and not by the
> user. On x86, only one config enables this (X86_INTEL_CE) which means
> this won't work on all the other platforms even if CONFIG_OF is
> selected. I would need this to work by only selected CONFIG_OF.

Maybe this means that CONFIG_OF should be changed to select
CONFIG_OF_FLATTREE.  Any opinions on this Rob?

> That's why I decided to add the of_root creation in of_core_init()
> using a function (of_fdt_unflatten()) that is provided if CONFIG_OF is
> defined.

I mentioned this in response to the previous patch series, but will
repeat here for those who might not have read that email thread.

I do not want the root live tree to be created buy different code in
different places; I want one central place where this occurs.  When
the tree can be created in multiple places by different code blocks,
it becomes more difficult to understand the code and more likely that
one of the tree creation code blocks is not updated when another is.

> 
>> -	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
>> +	if (!initial_boot_params) {
>> +		initial_boot_params = (void *) __dtb_empty_root_begin;
>> +		unflatten_and_copy_device_tree();
>> +	} else {
>> +		__unflatten_device_tree(initial_boot_params, NULL, &of_root,
>>  				early_init_dt_alloc_memory_arch, false);
>> +	}
>>  
>>  	/* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
>>  	of_alias_scan(early_init_dt_alloc_memory_arch);
>> @@ -1373,6 +1384,12 @@ void __init unflatten_and_copy_device_tree(void)
>>  	unflatten_device_tree();
>>  }
>>  
>> +void __init setup_of(void)
>> +{
>> +	if (!of_root)
>> +		unflatten_device_tree();
>> +}
>> +
>>  #ifdef CONFIG_SYSFS
>>  static ssize_t of_fdt_raw_read(struct file *filp, struct kobject *kobj,
>>  			       struct bin_attribute *bin_attr,
>> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
>> index d69ad5bb1eb1..4566876db351 100644
>> --- a/include/linux/of_fdt.h
>> +++ b/include/linux/of_fdt.h
>> @@ -81,6 +81,7 @@ extern const void *of_flat_dt_match_machine(const void *default_match,
>>  /* Other Prototypes */
>>  extern void unflatten_device_tree(void);
>>  extern void unflatten_and_copy_device_tree(void);
>> +extern void setup_of(void);
>>  extern void early_init_devtree(void *);
>>  extern void early_get_first_memblock_info(void *, phys_addr_t *);
>>  #else /* CONFIG_OF_EARLY_FLATTREE */
>> @@ -91,6 +92,7 @@ static inline void early_init_fdt_reserve_self(void) {}
>>  static inline const char *of_flat_dt_get_machine_name(void) { return NULL; }
>>  static inline void unflatten_device_tree(void) {}
>>  static inline void unflatten_and_copy_device_tree(void) {}
>> +static inline void of_setup(void) {}
> 

> Shouldn't this be setup_of(void) ?

Yes, thanks!  Will fix.

One other thing I need to do is test this patch on a user mode linux
kernel.

-Frank