[PATCH] ihex: Add support for CS:IP/EIP records

[PATCH] ihex: Add support for CS:IP/EIP records

[Mark Brown]

ihex firmwares can include a jump address for starting execution. Add
a -j option which will cause this to be written into the generated
file as a record with address zero and data consisting of the address
to jump to, allowing drivers to make use of this information.

This format is chosen because it most closely follows the original ihex
format, though it may make more sense to write a record with length zero
and the address stored as the address.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 firmware/ihex2fw.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/firmware/ihex2fw.c b/firmware/ihex2fw.c
index 5a03ba8..2ea707b 100644
--- a/firmware/ihex2fw.c
+++ b/firmware/ihex2fw.c
@@ -55,6 +55,7 @@ static int output_records(int outfd);
 
 static int sort_records = 0;
 static int wide_records = 0;
+static int include_jump = 0;
 
 static int usage(void)
 {
@@ -63,6 +64,7 @@ static int usage(void)
 	fprintf(stderr, "usage: ihex2fw [<options>] <src.HEX> <dst.fw>\n");
 	fprintf(stderr, "       -w: wide records (16-bit length)\n");
 	fprintf(stderr, "       -s: sort records by address\n");
+	fprintf(stderr, "       -j: include records for CS:IP/EIP address\n");
 	return 1;
 }
 
@@ -73,7 +75,7 @@ int main(int argc, char **argv)
 	uint8_t *data;
 	int opt;
 
-	while ((opt = getopt(argc, argv, "ws")) != -1) {
+	while ((opt = getopt(argc, argv, "wsj")) != -1) {
 		switch (opt) {
 		case 'w':
 			wide_records = 1;
@@ -81,7 +83,9 @@ int main(int argc, char **argv)
 		case 's':
 			sort_records = 1;
 			break;
-		default:
+		case 'j':
+			include_jump = 1;
+			break;
 			return usage();
 		}
 	}
@@ -128,6 +132,7 @@ static int process_ihex(uint8_t *data, ssize_t size)
 {
 	struct ihex_binrec *record;
 	uint32_t offset = 0;
+	uint32_t *data32;
 	uint8_t type, crc = 0, crcbyte = 0;
 	int i, j;
 	int line = 1;
@@ -223,8 +228,13 @@ next_record:
 			return -EINVAL;
 		}
 
+		data32 = &record->data[0];
+		*data32 = htonl(*data32);
+
 		/* These records contain the CS/IP or EIP where execution
-		 * starts. Don't really know what to do with them. */
+		 * starts. If requested output this as a record. */
+		if (include_jump)
+			file_record(record);
 		goto next_record;
 
 	default:
-- 
1.7.1

Re: [PATCH] ihex: Add support for CS:IP/EIP records

[David Woodhouse]

On Thu, 2010-09-09 at 14:40 +0100, Mark Brown wrote:
> ihex firmwares can include a jump address for starting execution. Add
> a -j option which will cause this to be written into the generated
> file as a record with address zero and data consisting of the address
> to jump to, allowing drivers to make use of this information.

Some questions...

- Which firmware blobs that are *already* in the kernel would need this?
  Remember, we're not adding new crap to the kernel source tree; new
  firmware images get added to linux-firmware.git instead.

- Why make the include_jump code optional? When would we ever *not* want
  to include such a record -- when does the input ihex file contain such
  a record that the kernel driver *must* not see because it can't cope
  with it?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] ihex: Add support for CS:IP/EIP records

[Mark Brown]

On Sat, Sep 11, 2010 at 09:29:04AM +0100, David Woodhouse wrote:

> - Which firmware blobs that are *already* in the kernel would need this?
>   Remember, we're not adding new crap to the kernel source tree; new
>   firmware images get added to linux-firmware.git instead.

None to my knowledge, however I don't understand why this would be
relevant to adding new features to ihex2fw?

> - Why make the include_jump code optional? When would we ever *not* want
>   to include such a record -- when does the input ihex file contain such
>   a record that the kernel driver *must* not see because it can't cope
>   with it?

Given that our binary ihex format discards all record type information I
would expect all existing users would be confused if they started seeing
these records so it seems safer to only generate them when required.  It
seems likely that some ihex images will contain jump addresses generated
by their toolchain which do not need to be parsed since the address is
always the same (eg, the address the device uses when brought out of
reset), though none of those in the kernel tree currently seem to.

Re: [PATCH] ihex: Add support for CS:IP/EIP records

[Andrew Morton]

On Thu,  9 Sep 2010 14:40:26 +0100
Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> +		data32 = &record->data[0];
> +		*data32 = htonl(*data32);

firmware/ihex2fw.c: In function 'process_ihex':
firmware/ihex2fw.c:231: warning: assignment from incompatible pointer type

And afacit data[] isn't 32-bit aligned and I don't know that all
architectures support htonl() against a misaligned pointer.

I'll drop the patch.

Re: [PATCH] ihex: Add support for CS:IP/EIP records

[Mark Brown]

On Wed, Sep 15, 2010 at 02:27:44PM -0700, Andrew Morton wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:

> > +		data32 = &record->data[0];
> > +		*data32 = htonl(*data32);

> firmware/ihex2fw.c: In function 'process_ihex':
> firmware/ihex2fw.c:231: warning: assignment from incompatible pointer type

Which architecture are you using - my AMD64 system isn't noticing these
things for me?

> And afacit data[] isn't 32-bit aligned and I don't know that all
> architectures support htonl() against a misaligned pointer.

We're pretty much safe in userspace, if the hardware doesn't support it
then as far as I'm aware pretty much all OSs do a software fixup.  This
isn't great for performance but works.

> I'll drop the patch.

OK, I'll spin it later today.