[U-Boot] [PATCH] rpi: Fix device tree path on ARM64

[U-Boot] [PATCH] rpi: Fix device tree path on ARM64

[Tuomas Tynkkynen]

The directory structure of device tree files produced by the kernel's
'make dtbs_install' is different on ARM64, the RPi3 device tree file is
in a 'broadcom' subdirectory there. Make the set_fdtfile function account
for this so that the distro boot scripts can locate the DTB file.

Signed-off-by: Tuomas Tynkkynen <tuomas@tuxera.com>
---
 board/raspberrypi/rpi/rpi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
index 22e87a2..dbf69f8 100644
--- a/board/raspberrypi/rpi/rpi.c
+++ b/board/raspberrypi/rpi/rpi.c
@@ -243,12 +243,14 @@ int dram_init(void)
 
 static void set_fdtfile(void)
 {
-	const char *fdtfile;
+	char fdtfile[64] = "";
 
 	if (getenv("fdtfile"))
 		return;
 
-	fdtfile = model->fdtfile;
+	if (IS_ENABLED(CONFIG_ARM64))
+		strcat(fdtfile, "broadcom/");
+	strcat(fdtfile, model->fdtfile);
 	setenv("fdtfile", fdtfile);
 }
 
-- 
2.10.2

[U-Boot] [PATCH] rpi: Fix device tree path on ARM64

[Stephen Warren]

On 01/03/2017 03:39 AM, Tuomas Tynkkynen wrote:
> The directory structure of device tree files produced by the kernel's
> 'make dtbs_install' is different on ARM64, the RPi3 device tree file is
> in a 'broadcom' subdirectory there. Make the set_fdtfile function account
> for this so that the distro boot scripts can locate the DTB file.

I'm not 100% sure there's an expectation/guarantee that "make 
dtbs_install" will be used to create the DTB layout that the bootloader 
uses, although I suppose this is a reasonable thing to do.

> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c

>  static void set_fdtfile(void)

> -	fdtfile = model->fdtfile;
> +	if (IS_ENABLED(CONFIG_ARM64))
> +		strcat(fdtfile, "broadcom/");
> +	strcat(fdtfile, model->fdtfile);

Instead of writing code for this, and in particular code that doesn't 
check for buffer size/overflow/..., wouldn't it be better to simply edit 
the RPi 3 entry in rpi_models_new_scheme[] to contain "broadcom/" in the 
DTB filename string?

[U-Boot] [PATCH] rpi: Fix device tree path on ARM64

[Tuomas Tynkkynen]

On Thu, 5 Jan 2017 16:44:43 -0700
Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 01/03/2017 03:39 AM, Tuomas Tynkkynen wrote:
> > The directory structure of device tree files produced by the kernel's
> > 'make dtbs_install' is different on ARM64, the RPi3 device tree file is
> > in a 'broadcom' subdirectory there. Make the set_fdtfile function account
> > for this so that the distro boot scripts can locate the DTB file.
> 
> I'm not 100% sure there's an expectation/guarantee that "make 
> dtbs_install" will be used to create the DTB layout that the bootloader 
> uses, although I suppose this is a reasonable thing to do.
> 

It certainly would be nice from the distro's point of view to have the same
scripts work on both ARM and ARM64.

> > diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
> 
> >  static void set_fdtfile(void)
> 
> > -	fdtfile = model->fdtfile;
> > +	if (IS_ENABLED(CONFIG_ARM64))
> > +		strcat(fdtfile, "broadcom/");
> > +	strcat(fdtfile, model->fdtfile);
> 
> Instead of writing code for this, and in particular code that doesn't 
> check for buffer size/overflow/..., wouldn't it be better to simply edit 
> the RPi 3 entry in rpi_models_new_scheme[] to contain "broadcom/" in the 
> DTB filename string?
> 

I did it this way to keep the 32-bit builds looking in the old location,
as that's where it was planned to be added for 32-bit ARM:

https://patchwork.kernel.org/patch/9148261/

I did consider some preprocessor magic and compile-time string concatenation
but this seemed a cleaner way.

[U-Boot] [PATCH] rpi: Fix device tree path on ARM64

[Stephen Warren]

On 01/09/2017 06:34 AM, Tuomas Tynkkynen wrote:
> On Thu, 5 Jan 2017 16:44:43 -0700
> Stephen Warren <swarren@wwwdotorg.org> wrote:
>
>> On 01/03/2017 03:39 AM, Tuomas Tynkkynen wrote:
>>> The directory structure of device tree files produced by the kernel's
>>> 'make dtbs_install' is different on ARM64, the RPi3 device tree file is
>>> in a 'broadcom' subdirectory there. Make the set_fdtfile function account
>>> for this so that the distro boot scripts can locate the DTB file.
>>
>> I'm not 100% sure there's an expectation/guarantee that "make
>> dtbs_install" will be used to create the DTB layout that the bootloader
>> uses, although I suppose this is a reasonable thing to do.
>>
>
> It certainly would be nice from the distro's point of view to have the same
> scripts work on both ARM and ARM64.
>
>>> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
>>
>>>  static void set_fdtfile(void)
>>
>>> -	fdtfile = model->fdtfile;
>>> +	if (IS_ENABLED(CONFIG_ARM64))
>>> +		strcat(fdtfile, "broadcom/");
>>> +	strcat(fdtfile, model->fdtfile);
>>
>> Instead of writing code for this, and in particular code that doesn't
>> check for buffer size/overflow/..., wouldn't it be better to simply edit
>> the RPi 3 entry in rpi_models_new_scheme[] to contain "broadcom/" in the
>> DTB filename string?
>>
>
> I did it this way to keep the 32-bit builds looking in the old location,
> as that's where it was planned to be added for 32-bit ARM:
>
> https://patchwork.kernel.org/patch/9148261/
>
> I did consider some preprocessor magic and compile-time string concatenation
> but this seemed a cleaner way.

I'm not convinced it's cleaner. It feels best to represent data as data, 
rather than writing code to calculate it at run-time, especially once 
all the complexity of error-checking is added in. I'd suggest:

Adding the following before the models table:

#ifdef CONFIG_ARM64
#define DTB_DIR "broadcom/"
#else
#define DTB_DIR ""
#endif

Then changing the table entries from:

         [0x8] = {
                 "3 Model B",
                 "bcm2837-rpi-3-b.dtb",

... to:

         [0x8] = {
                 "3 Model B",
                 DTB_DIR "bcm2837-rpi-3-b.dtb",