mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH v3] FIT: Parse `load` and `entry` addresses.
@ 2020-07-14  8:11 Christian Mauderer
  2020-07-14  8:24 ` Ahmad Fatoum
  0 siblings, 1 reply; 4+ messages in thread
From: Christian Mauderer @ 2020-07-14  8:11 UTC (permalink / raw)
  To: barebox; +Cc: a.fatoum

According to the U-Boot documentation for the FIT file format, the load
and entry have to be allways defined for a "kernel" or "standalone".
But Barebox ignored the parameters. That changes with this patch.

For backward compatibility the default address is still used for images
without `load` or `entry`.

Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
---
 arch/arm/mach-layerscape/ppa.c |  3 +-
 common/bootm.c                 |  7 +++--
 common/image-fit.c             | 51 ++++++++++++++++++++++++++++++++--
 include/image-fit.h            |  3 +-
 4 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-layerscape/ppa.c b/arch/arm/mach-layerscape/ppa.c
index 477e89478..3eb7ab641 100644
--- a/arch/arm/mach-layerscape/ppa.c
+++ b/arch/arm/mach-layerscape/ppa.c
@@ -82,7 +82,8 @@ static int ppa_init(void *ppa, size_t ppa_size, void *sec_firmware_addr)
 	}
 
 
-	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size);
+	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size,
+			     NULL, NULL);
 	if (ret) {
 		pr_err("Cannot open firmware image in ppa FIT image: %s\n",
 		       strerror(-ret));
diff --git a/common/bootm.c b/common/bootm.c
index 366f31455..94600cae3 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -222,7 +222,7 @@ int bootm_load_initrd(struct image_data *data, unsigned long load_address)
 		unsigned long initrd_size;
 
 		ret = fit_open_image(data->os_fit, data->fit_config, "ramdisk",
-				     &initrd, &initrd_size);
+				     &initrd, &initrd_size, NULL, NULL);
 
 		data->initrd_res = request_sdram_region("initrd",
 				load_address,
@@ -348,7 +348,7 @@ void *bootm_get_devicetree(struct image_data *data)
 		unsigned long of_size;
 
 		ret = fit_open_image(data->os_fit, data->fit_config, "fdt",
-				     &of_tree, &of_size);
+				     &of_tree, &of_size, NULL, NULL);
 		if (ret)
 			return ERR_PTR(ret);
 
@@ -622,7 +622,8 @@ int bootm_boot(struct bootm_data *bootm_data)
 		}
 
 		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
-				     &data->fit_kernel, &data->fit_kernel_size);
+				     &data->fit_kernel, &data->fit_kernel_size,
+				     &data->os_address, &data->os_entry);
 		if (ret)
 			goto err_out;
 	}
diff --git a/common/image-fit.c b/common/image-fit.c
index 2681d62a9..7f0d3f98f 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -517,12 +517,30 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
 	return 1;
 }
 
+static int fit_get_address(struct device_node *image, const char *property,
+			   unsigned long *addr)
+{
+	const __be32 *cell;
+	int len = 0;
+
+	cell = of_get_property(image, property, &len);
+	if (!cell)
+		return -EINVAL;
+	if (len > sizeof(*addr))
+		return -ENOTSUPP;
+	
+	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
+	return 0;
+}
+
 /**
  * fit_open_image - Open an image in a FIT image
  * @handle: The FIT image handle
  * @name: The name of the image to open
  * @outdata: The returned image
  * @outsize: Size of the returned image
+ * @load: The load address given by the image
+ * @entry: The entry address given by the image
  *
  * Open an image in a FIT image. The returned image is freed during fit_close().
  * @configuration holds the cookie returned from fit_open_configuration() if
@@ -532,11 +550,16 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
  * then only the hash is checked (because opening the configuration already
  * checks the RSA signature of all involved nodes).
  *
+ * The load address and entry point of the image description in the FIT will be
+ * parsed if they exist and if the @load and @entry parameters are not NULL.
+ * Otherwise @load and @entry won't be changed.
+ *
  * Return: 0 for success, negative error code otherwise
  */
 int fit_open_image(struct fit_handle *handle, void *configuration,
 		   const char *name, const void **outdata,
-		   unsigned long *outsize)
+		   unsigned long *outsize, unsigned long *load,
+		   unsigned long *entry)
 {
 	struct device_node *image;
 	const char *unit, *type = NULL, *desc= "(no description)";
@@ -559,7 +582,31 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 		return -ENOENT;
 
 	of_property_read_string(image, "description", &desc);
-	pr_info("image '%s': '%s'\n", unit, desc);
+
+	if (load) {
+		ret = fit_get_address(image, "load", load);
+		if (ret < 0)
+			pr_info("Couldn't get load address in %s. Use default.\n",
+				image->full_name);
+	}
+
+	if (load && entry) {
+		ret = fit_get_address(image, "entry", entry);
+		if (ret < 0)
+			pr_info("Couldn't get entry point in %s. Use default.\n",
+				image->full_name);
+		else
+			/* Barebox uses an entry relative to load but the FIT
+			 * images assume an absolute entry. */
+			*entry -= *load;
+	}
+
+	pr_info("image '%s': '%s'", unit, desc);
+	if (load)
+		pr_cont("; load: 0x%lx", *load);
+	if (entry)
+		pr_cont("; entry (relative to load): 0x%lx", *entry);
+	pr_cont("\n");
 
 	of_property_read_string(image, "type", &type);
 	if (!type) {
diff --git a/include/image-fit.h b/include/image-fit.h
index 27c9e8351..038732d0d 100644
--- a/include/image-fit.h
+++ b/include/image-fit.h
@@ -31,7 +31,8 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
 		  const char *name);
 int fit_open_image(struct fit_handle *handle, void *configuration,
 		   const char *name, const void **outdata,
-		   unsigned long *outsize);
+		   unsigned long *outsize, unsigned long *load,
+		   unsigned long *entry);
 
 void fit_close(struct fit_handle *handle);
 
-- 
2.26.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] FIT: Parse `load` and `entry` addresses.
  2020-07-14  8:11 [PATCH v3] FIT: Parse `load` and `entry` addresses Christian Mauderer
@ 2020-07-14  8:24 ` Ahmad Fatoum
  2020-07-14  8:29   ` Christian Mauderer
  0 siblings, 1 reply; 4+ messages in thread
From: Ahmad Fatoum @ 2020-07-14  8:24 UTC (permalink / raw)
  To: Christian Mauderer, barebox

Hi,

A final nitpick, see below:

On 7/14/20 10:11 AM, Christian Mauderer wrote:
> According to the U-Boot documentation for the FIT file format, the load
> and entry have to be allways defined for a "kernel" or "standalone".
> But Barebox ignored the parameters. That changes with this patch.
> 
> For backward compatibility the default address is still used for images
> without `load` or `entry`.
> 
> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
> ---
>  arch/arm/mach-layerscape/ppa.c |  3 +-
>  common/bootm.c                 |  7 +++--
>  common/image-fit.c             | 51 ++++++++++++++++++++++++++++++++--
>  include/image-fit.h            |  3 +-
>  4 files changed, 57 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-layerscape/ppa.c b/arch/arm/mach-layerscape/ppa.c
> index 477e89478..3eb7ab641 100644
> --- a/arch/arm/mach-layerscape/ppa.c
> +++ b/arch/arm/mach-layerscape/ppa.c
> @@ -82,7 +82,8 @@ static int ppa_init(void *ppa, size_t ppa_size, void *sec_firmware_addr)
>  	}
>  
>  
> -	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size);
> +	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size,
> +			     NULL, NULL);
>  	if (ret) {
>  		pr_err("Cannot open firmware image in ppa FIT image: %s\n",
>  		       strerror(-ret));
> diff --git a/common/bootm.c b/common/bootm.c
> index 366f31455..94600cae3 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -222,7 +222,7 @@ int bootm_load_initrd(struct image_data *data, unsigned long load_address)
>  		unsigned long initrd_size;
>  
>  		ret = fit_open_image(data->os_fit, data->fit_config, "ramdisk",
> -				     &initrd, &initrd_size);
> +				     &initrd, &initrd_size, NULL, NULL);
>  
>  		data->initrd_res = request_sdram_region("initrd",
>  				load_address,
> @@ -348,7 +348,7 @@ void *bootm_get_devicetree(struct image_data *data)
>  		unsigned long of_size;
>  
>  		ret = fit_open_image(data->os_fit, data->fit_config, "fdt",
> -				     &of_tree, &of_size);
> +				     &of_tree, &of_size, NULL, NULL);
>  		if (ret)
>  			return ERR_PTR(ret);
>  
> @@ -622,7 +622,8 @@ int bootm_boot(struct bootm_data *bootm_data)
>  		}
>  
>  		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
> -				     &data->fit_kernel, &data->fit_kernel_size);
> +				     &data->fit_kernel, &data->fit_kernel_size,
> +				     &data->os_address, &data->os_entry);
>  		if (ret)
>  			goto err_out;
>  	}
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 2681d62a9..7f0d3f98f 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -517,12 +517,30 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>  	return 1;
>  }
>  
> +static int fit_get_address(struct device_node *image, const char *property,
> +			   unsigned long *addr)
> +{
> +	const __be32 *cell;
> +	int len = 0;
> +
> +	cell = of_get_property(image, property, &len);
> +	if (!cell)
> +		return -EINVAL;
> +	if (len > sizeof(*addr))
> +		return -ENOTSUPP;
> +	
> +	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
> +	return 0;
> +}
> +
>  /**
>   * fit_open_image - Open an image in a FIT image
>   * @handle: The FIT image handle
>   * @name: The name of the image to open
>   * @outdata: The returned image
>   * @outsize: Size of the returned image
> + * @load: The load address given by the image
> + * @entry: The entry address given by the image
>   *
>   * Open an image in a FIT image. The returned image is freed during fit_close().
>   * @configuration holds the cookie returned from fit_open_configuration() if
> @@ -532,11 +550,16 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>   * then only the hash is checked (because opening the configuration already
>   * checks the RSA signature of all involved nodes).
>   *
> + * The load address and entry point of the image description in the FIT will be
> + * parsed if they exist and if the @load and @entry parameters are not NULL.
> + * Otherwise @load and @entry won't be changed.
> + *
>   * Return: 0 for success, negative error code otherwise
>   */
>  int fit_open_image(struct fit_handle *handle, void *configuration,
>  		   const char *name, const void **outdata,
> -		   unsigned long *outsize)
> +		   unsigned long *outsize, unsigned long *load,
> +		   unsigned long *entry)
>  {
>  	struct device_node *image;
>  	const char *unit, *type = NULL, *desc= "(no description)";
> @@ -559,7 +582,31 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  		return -ENOENT;
>  
>  	of_property_read_string(image, "description", &desc);
> -	pr_info("image '%s': '%s'\n", unit, desc);

Leave it, but without newline

> +
> +	if (load) {
> +		ret = fit_get_address(image, "load", load);
> +		if (ret < 0)
> +			pr_info("Couldn't get load address in %s. Use default.\n",
> +				image->full_name);
		else
			pr_cont("; load: 0x%lx", *load);
> +	}
> +
> +	if (load && entry) {
> +		ret = fit_get_address(image, "entry", entry);
> +		if (ret < 0)
{
> +			pr_info("Couldn't get entry point in %s. Use default.\n",
> +				image->full_name);> +		else
} else {
> +			/* Barebox uses an entry relative to load but the FIT
> +			 * images assume an absolute entry. */
> +			*entry -= *load;
			pr_cont("; entry (relative to load): 0x%lx", *entry);
}
> +	}
> +
> +	pr_info("image '%s': '%s'", unit, desc);
> +	if (load)
> +		pr_cont("; load: 0x%lx", *load);
> +	if (entry)
> +		pr_cont("; entry (relative to load): 0x%lx", *entry);

if (!load && entry), you don't populate *entry, yet to print it here.
This address was not made relative, despite that the output claims it is.

With the suggestions above:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Thanks
Ahmad

> +	pr_cont("\n");
>  
>  	of_property_read_string(image, "type", &type);
>  	if (!type) {
> diff --git a/include/image-fit.h b/include/image-fit.h
> index 27c9e8351..038732d0d 100644
> --- a/include/image-fit.h
> +++ b/include/image-fit.h
> @@ -31,7 +31,8 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>  		  const char *name);
>  int fit_open_image(struct fit_handle *handle, void *configuration,
>  		   const char *name, const void **outdata,
> -		   unsigned long *outsize);
> +		   unsigned long *outsize, unsigned long *load,
> +		   unsigned long *entry);
>  
>  void fit_close(struct fit_handle *handle);
>  
> 

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] FIT: Parse `load` and `entry` addresses.
  2020-07-14  8:24 ` Ahmad Fatoum
@ 2020-07-14  8:29   ` Christian Mauderer
  2020-07-14  8:39     ` Ahmad Fatoum
  0 siblings, 1 reply; 4+ messages in thread
From: Christian Mauderer @ 2020-07-14  8:29 UTC (permalink / raw)
  To: Ahmad Fatoum, barebox

On 14/07/2020 10:24, Ahmad Fatoum wrote:
> Hi,
> 
> A final nitpick, see below:
> 
> On 7/14/20 10:11 AM, Christian Mauderer wrote:
>> According to the U-Boot documentation for the FIT file format, the load
>> and entry have to be allways defined for a "kernel" or "standalone".
>> But Barebox ignored the parameters. That changes with this patch.
>>
>> For backward compatibility the default address is still used for images
>> without `load` or `entry`.
>>
>> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
>> ---
>>  arch/arm/mach-layerscape/ppa.c |  3 +-
>>  common/bootm.c                 |  7 +++--
>>  common/image-fit.c             | 51 ++++++++++++++++++++++++++++++++--
>>  include/image-fit.h            |  3 +-
>>  4 files changed, 57 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/mach-layerscape/ppa.c b/arch/arm/mach-layerscape/ppa.c
>> index 477e89478..3eb7ab641 100644
>> --- a/arch/arm/mach-layerscape/ppa.c
>> +++ b/arch/arm/mach-layerscape/ppa.c
>> @@ -82,7 +82,8 @@ static int ppa_init(void *ppa, size_t ppa_size, void *sec_firmware_addr)
>>  	}
>>  
>>  
>> -	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size);
>> +	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size,
>> +			     NULL, NULL);
>>  	if (ret) {
>>  		pr_err("Cannot open firmware image in ppa FIT image: %s\n",
>>  		       strerror(-ret));
>> diff --git a/common/bootm.c b/common/bootm.c
>> index 366f31455..94600cae3 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -222,7 +222,7 @@ int bootm_load_initrd(struct image_data *data, unsigned long load_address)
>>  		unsigned long initrd_size;
>>  
>>  		ret = fit_open_image(data->os_fit, data->fit_config, "ramdisk",
>> -				     &initrd, &initrd_size);
>> +				     &initrd, &initrd_size, NULL, NULL);
>>  
>>  		data->initrd_res = request_sdram_region("initrd",
>>  				load_address,
>> @@ -348,7 +348,7 @@ void *bootm_get_devicetree(struct image_data *data)
>>  		unsigned long of_size;
>>  
>>  		ret = fit_open_image(data->os_fit, data->fit_config, "fdt",
>> -				     &of_tree, &of_size);
>> +				     &of_tree, &of_size, NULL, NULL);
>>  		if (ret)
>>  			return ERR_PTR(ret);
>>  
>> @@ -622,7 +622,8 @@ int bootm_boot(struct bootm_data *bootm_data)
>>  		}
>>  
>>  		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
>> -				     &data->fit_kernel, &data->fit_kernel_size);
>> +				     &data->fit_kernel, &data->fit_kernel_size,
>> +				     &data->os_address, &data->os_entry);
>>  		if (ret)
>>  			goto err_out;
>>  	}
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index 2681d62a9..7f0d3f98f 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -517,12 +517,30 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>  	return 1;
>>  }
>>  
>> +static int fit_get_address(struct device_node *image, const char *property,
>> +			   unsigned long *addr)
>> +{
>> +	const __be32 *cell;
>> +	int len = 0;
>> +
>> +	cell = of_get_property(image, property, &len);
>> +	if (!cell)
>> +		return -EINVAL;
>> +	if (len > sizeof(*addr))
>> +		return -ENOTSUPP;
>> +	
>> +	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
>> +	return 0;
>> +}
>> +
>>  /**
>>   * fit_open_image - Open an image in a FIT image
>>   * @handle: The FIT image handle
>>   * @name: The name of the image to open
>>   * @outdata: The returned image
>>   * @outsize: Size of the returned image
>> + * @load: The load address given by the image
>> + * @entry: The entry address given by the image
>>   *
>>   * Open an image in a FIT image. The returned image is freed during fit_close().
>>   * @configuration holds the cookie returned from fit_open_configuration() if
>> @@ -532,11 +550,16 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>   * then only the hash is checked (because opening the configuration already
>>   * checks the RSA signature of all involved nodes).
>>   *
>> + * The load address and entry point of the image description in the FIT will be
>> + * parsed if they exist and if the @load and @entry parameters are not NULL.
>> + * Otherwise @load and @entry won't be changed.
>> + *
>>   * Return: 0 for success, negative error code otherwise
>>   */
>>  int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		   const char *name, const void **outdata,
>> -		   unsigned long *outsize)
>> +		   unsigned long *outsize, unsigned long *load,
>> +		   unsigned long *entry)
>>  {
>>  	struct device_node *image;
>>  	const char *unit, *type = NULL, *desc= "(no description)";
>> @@ -559,7 +582,31 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		return -ENOENT;
>>  
>>  	of_property_read_string(image, "description", &desc);
>> -	pr_info("image '%s': '%s'\n", unit, desc);
> 
> Leave it, but without newline
> 

OK.

>> +
>> +	if (load) {
>> +		ret = fit_get_address(image, "load", load);
>> +		if (ret < 0)
>> +			pr_info("Couldn't get load address in %s. Use default.\n",
>> +				image->full_name);
> 		else
> 			pr_cont("; load: 0x%lx", *load);

OK

>> +	}
>> +
>> +	if (load && entry) {
>> +		ret = fit_get_address(image, "entry", entry);
>> +		if (ret < 0)
> {

Why a bracket here but not in the load case? It's clear for the else
case but not for the if.

>> +			pr_info("Couldn't get entry point in %s. Use default.\n",
>> +				image->full_name);> +		else
> } else {
>> +			/* Barebox uses an entry relative to load but the FIT
>> +			 * images assume an absolute entry. */
>> +			*entry -= *load;
> 			pr_cont("; entry (relative to load): 0x%lx", *entry);
> }
>> +	}
>> +
>> +	pr_info("image '%s': '%s'", unit, desc);
>> +	if (load)
>> +		pr_cont("; load: 0x%lx", *load);
>> +	if (entry)
>> +		pr_cont("; entry (relative to load): 0x%lx", *entry);
> 
> if (!load && entry), you don't populate *entry, yet to print it here.
> This address was not made relative, despite that the output claims it is.

The default case for entry points in barebox is to handle it as a
relative address. So I assume if I don't change it, it will be relative.
But if I move the print further up like you suggested, it won't be
relevant anyway.

> 
> With the suggestions above:
> 
> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Thanks
> Ahmad
> 
>> +	pr_cont("\n");
>>  
>>  	of_property_read_string(image, "type", &type);
>>  	if (!type) {
>> diff --git a/include/image-fit.h b/include/image-fit.h
>> index 27c9e8351..038732d0d 100644
>> --- a/include/image-fit.h
>> +++ b/include/image-fit.h
>> @@ -31,7 +31,8 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>  		  const char *name);
>>  int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		   const char *name, const void **outdata,
>> -		   unsigned long *outsize);
>> +		   unsigned long *outsize, unsigned long *load,
>> +		   unsigned long *entry);
>>  
>>  void fit_close(struct fit_handle *handle);
>>  
>>
> 

-- 
--------------------------------------------
embedded brains GmbH
Herr Christian Mauderer
Dornierstr. 4
D-82178 Puchheim
Germany
email: christian.mauderer@embedded-brains.de
Phone: +49-89-18 94 741 - 18
Fax:   +49-89-18 94 741 - 08
PGP: Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] FIT: Parse `load` and `entry` addresses.
  2020-07-14  8:29   ` Christian Mauderer
@ 2020-07-14  8:39     ` Ahmad Fatoum
  0 siblings, 0 replies; 4+ messages in thread
From: Ahmad Fatoum @ 2020-07-14  8:39 UTC (permalink / raw)
  To: Christian Mauderer, barebox

Hello,

On 7/14/20 10:29 AM, Christian Mauderer wrote:
> On 14/07/2020 10:24, Ahmad Fatoum wrote:
>> Hi,
>>
>> A final nitpick, see below:
>>
>> On 7/14/20 10:11 AM, Christian Mauderer wrote:
>>> According to the U-Boot documentation for the FIT file format, the load
>>> and entry have to be allways defined for a "kernel" or "standalone".
>>> But Barebox ignored the parameters. That changes with this patch.
>>>
>>> For backward compatibility the default address is still used for images
>>> without `load` or `entry`.
>>>
>>> Signed-off-by: Christian Mauderer <christian.mauderer@embedded-brains.de>
>>> ---
>>>  arch/arm/mach-layerscape/ppa.c |  3 +-
>>>  common/bootm.c                 |  7 +++--
>>>  common/image-fit.c             | 51 ++++++++++++++++++++++++++++++++--
>>>  include/image-fit.h            |  3 +-
>>>  4 files changed, 57 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-layerscape/ppa.c b/arch/arm/mach-layerscape/ppa.c
>>> index 477e89478..3eb7ab641 100644
>>> --- a/arch/arm/mach-layerscape/ppa.c
>>> +++ b/arch/arm/mach-layerscape/ppa.c
>>> @@ -82,7 +82,8 @@ static int ppa_init(void *ppa, size_t ppa_size, void *sec_firmware_addr)
>>>  	}
>>>  
>>>  
>>> -	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size);
>>> +	ret = fit_open_image(fit, conf, "firmware", &buf, &firmware_size,
>>> +			     NULL, NULL);
>>>  	if (ret) {
>>>  		pr_err("Cannot open firmware image in ppa FIT image: %s\n",
>>>  		       strerror(-ret));
>>> diff --git a/common/bootm.c b/common/bootm.c
>>> index 366f31455..94600cae3 100644
>>> --- a/common/bootm.c
>>> +++ b/common/bootm.c
>>> @@ -222,7 +222,7 @@ int bootm_load_initrd(struct image_data *data, unsigned long load_address)
>>>  		unsigned long initrd_size;
>>>  
>>>  		ret = fit_open_image(data->os_fit, data->fit_config, "ramdisk",
>>> -				     &initrd, &initrd_size);
>>> +				     &initrd, &initrd_size, NULL, NULL);
>>>  
>>>  		data->initrd_res = request_sdram_region("initrd",
>>>  				load_address,
>>> @@ -348,7 +348,7 @@ void *bootm_get_devicetree(struct image_data *data)
>>>  		unsigned long of_size;
>>>  
>>>  		ret = fit_open_image(data->os_fit, data->fit_config, "fdt",
>>> -				     &of_tree, &of_size);
>>> +				     &of_tree, &of_size, NULL, NULL);
>>>  		if (ret)
>>>  			return ERR_PTR(ret);
>>>  
>>> @@ -622,7 +622,8 @@ int bootm_boot(struct bootm_data *bootm_data)
>>>  		}
>>>  
>>>  		ret = fit_open_image(data->os_fit, data->fit_config, "kernel",
>>> -				     &data->fit_kernel, &data->fit_kernel_size);
>>> +				     &data->fit_kernel, &data->fit_kernel_size,
>>> +				     &data->os_address, &data->os_entry);
>>>  		if (ret)
>>>  			goto err_out;
>>>  	}
>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>> index 2681d62a9..7f0d3f98f 100644
>>> --- a/common/image-fit.c
>>> +++ b/common/image-fit.c
>>> @@ -517,12 +517,30 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>>  	return 1;
>>>  }
>>>  
>>> +static int fit_get_address(struct device_node *image, const char *property,
>>> +			   unsigned long *addr)
>>> +{
>>> +	const __be32 *cell;
>>> +	int len = 0;
>>> +
>>> +	cell = of_get_property(image, property, &len);
>>> +	if (!cell)
>>> +		return -EINVAL;
>>> +	if (len > sizeof(*addr))
>>> +		return -ENOTSUPP;
>>> +	
>>> +	*addr = (unsigned long)of_read_number(cell, len / sizeof(*cell));
>>> +	return 0;
>>> +}
>>> +
>>>  /**
>>>   * fit_open_image - Open an image in a FIT image
>>>   * @handle: The FIT image handle
>>>   * @name: The name of the image to open
>>>   * @outdata: The returned image
>>>   * @outsize: Size of the returned image
>>> + * @load: The load address given by the image
>>> + * @entry: The entry address given by the image
>>>   *
>>>   * Open an image in a FIT image. The returned image is freed during fit_close().
>>>   * @configuration holds the cookie returned from fit_open_configuration() if
>>> @@ -532,11 +550,16 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>>   * then only the hash is checked (because opening the configuration already
>>>   * checks the RSA signature of all involved nodes).
>>>   *
>>> + * The load address and entry point of the image description in the FIT will be
>>> + * parsed if they exist and if the @load and @entry parameters are not NULL.
>>> + * Otherwise @load and @entry won't be changed.
>>> + *
>>>   * Return: 0 for success, negative error code otherwise
>>>   */
>>>  int fit_open_image(struct fit_handle *handle, void *configuration,
>>>  		   const char *name, const void **outdata,
>>> -		   unsigned long *outsize)
>>> +		   unsigned long *outsize, unsigned long *load,
>>> +		   unsigned long *entry)
>>>  {
>>>  	struct device_node *image;
>>>  	const char *unit, *type = NULL, *desc= "(no description)";
>>> @@ -559,7 +582,31 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>>  		return -ENOENT;
>>>  
>>>  	of_property_read_string(image, "description", &desc);
>>> -	pr_info("image '%s': '%s'\n", unit, desc);
>>
>> Leave it, but without newline
>>
> 
> OK.
> 
>>> +
>>> +	if (load) {
>>> +		ret = fit_get_address(image, "load", load);
>>> +		if (ret < 0)
>>> +			pr_info("Couldn't get load address in %s. Use default.\n",
>>> +				image->full_name);
>> 		else
>> 			pr_cont("; load: 0x%lx", *load);
> 
> OK
> 
>>> +	}
>>> +
>>> +	if (load && entry) {
>>> +		ret = fit_get_address(image, "entry", entry);
>>> +		if (ret < 0)
>> {
> 
> Why a bracket here but not in the load case? It's clear for the else
> case but not for the if.

You have to settle on something for uniformity. We try to stick to the
kernel coding style, see second to last code listing at
https://www.kernel.org/doc/html/v5.8-rc4/process/coding-style.html#placing-braces-and-spaces

> 
>>> +			pr_info("Couldn't get entry point in %s. Use default.\n",
>>> +				image->full_name);> +		else
>> } else {
>>> +			/* Barebox uses an entry relative to load but the FIT
>>> +			 * images assume an absolute entry. */
>>> +			*entry -= *load;
>> 			pr_cont("; entry (relative to load): 0x%lx", *entry);
>> }
>>> +	}
>>> +
>>> +	pr_info("image '%s': '%s'", unit, desc);
>>> +	if (load)
>>> +		pr_cont("; load: 0x%lx", *load);
>>> +	if (entry)
>>> +		pr_cont("; entry (relative to load): 0x%lx", *entry);
>>
>> if (!load && entry), you don't populate *entry, yet to print it here.
>> This address was not made relative, despite that the output claims it is.
> 
> The default case for entry points in barebox is to handle it as a
> relative address. So I assume if I don't change it, it will be relative.
> But if I move the print further up like you suggested, it won't be
> relevant anyway.

You're right. I didn't think this through.

> 
>>
>> With the suggestions above:
>>
>> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>
>> Thanks
>> Ahmad
>>
>>> +	pr_cont("\n");
>>>  
>>>  	of_property_read_string(image, "type", &type);
>>>  	if (!type) {
>>> diff --git a/include/image-fit.h b/include/image-fit.h
>>> index 27c9e8351..038732d0d 100644
>>> --- a/include/image-fit.h
>>> +++ b/include/image-fit.h
>>> @@ -31,7 +31,8 @@ int fit_has_image(struct fit_handle *handle, void *configuration,
>>>  		  const char *name);
>>>  int fit_open_image(struct fit_handle *handle, void *configuration,
>>>  		   const char *name, const void **outdata,
>>> -		   unsigned long *outsize);
>>> +		   unsigned long *outsize, unsigned long *load,
>>> +		   unsigned long *entry);
>>>  
>>>  void fit_close(struct fit_handle *handle);
>>>  
>>>
>>
> 

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-07-14  8:39 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-14  8:11 [PATCH v3] FIT: Parse `load` and `entry` addresses Christian Mauderer
2020-07-14  8:24 ` Ahmad Fatoum
2020-07-14  8:29   ` Christian Mauderer
2020-07-14  8:39     ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox