On Friday 28 February 2014 10:57:21 Wolfgang Denk wrote:
Dear Charles,
In message 1393472979-7522-1-git-send-email-cdhmanning@gmail.com you
wrote:
Like many platforms, the Altera socfpga platform requires that the preloader be "signed" in a certain way or the built-in boot ROM will not boot the code.
...
diff --git a/include/crc32_alt.h b/include/crc32_alt.h new file mode 100644 index 0000000..813d55d --- /dev/null +++ b/include/crc32_alt.h
"alt" is a bad name as there is more than one alternative. Please use a descriptive name.
Ok I shall do that.
--- /dev/null +++ b/lib/crc32_alt.c @@ -0,0 +1,94 @@ +/*
- Copyright (C) 2014 Charles Manning cdhmanning@gmail.com
- SPDX-License-Identifier: GPL-2.0+
- Note that the CRC is **not** the zlib/Adler crc32 in crc32.c.
- It is the CRC-32 used in bzip2, ethernet and elsewhere.
- */
I understand this was copied from "lib/bzlib_crctable.c" ? BUt you claim copyright on this, without any attribution where you got it form. This is very, vary bad.
You understand incorrectly. I did not copy it from bzlib. I generated it. I had generated this table before I even knew it was part of ib/bzlib_crctable.c.
Of course it **must** have the same values in it because that is how the mathematics works out.
I hope you are as free with your retractions as you are with your accusations!
+static uint32_t crc_table[256] = {
- 0x00000000, 0x04c11db7, 0x09823b6e, 0x0d4326d9,
...
- 0xbcb4666d, 0xb8757bda, 0xb5365d03, 0xb1f740b4,
+};
Indeed this looks very much like a duplication of code we have elsewhere:
- in "lib/bzlib_crctable.c" (as BZ2_crc32Table[])
- in "drivers/mtd/ubi/crc32table.h" (as crc32table_be[])
+uint32_t crc32_alt(uint32_t crc, const void *_buf, int length) +{
- const uint8_t *buf = _buf;
- crc ^= ~0;
- while (length--) {
crc = (crc << 8) ^ crc_table[((crc >> 24) ^ *buf) & 0xff];buf++;- }
- crc ^= ~0;
- return crc;
+}
In addition to this, we also have
- crc32 in "lib/crc32.c"
- crc32() in "tools/mxsimage.c"
- make_crc_table() and pbl_crc32() in "tools/pblimage.c"
I really think we should factor out the common tables and code here. I will not accept yet another duplication of this - we already have way too many of these.
Based on your comments in another thread, I have suggested that I do one of two things:
1) Either have a new C file in lib that uses the bzlib table or 2) Extend the bzlib in a way that exposes a function called crc32_bzlib() or bzlib_crc32().
Whichever you like.
It seems to me that refactoring is best kept as a different patch.
May I humbly submit that it would be a good idea to bed this socfpga signer down. Then, as a separate commit and a separate patch, I will refactor with pbllimage and mxsimage.
Does that sound OK with you?
--- /dev/null +++ b/tools/socfpgaimage.c @@ -0,0 +1,278 @@
...
- Endian is LSB.
What does that mean? When talking about endianess, I know "Big-endian" and "Little-endian" - LSB is meaningless in this context (unless you write something like "LSB first" or "LSB last", but even this would not be really clear).
Sorry typo. I will fix.
- Note that the CRC used here is **not** the zlib/Adler crc32. It is
the + * CRC-32 used in bzip2, ethernet and elsewhere.
Does this have a name?
CRC-32 ... the real one. Adler was really a bit naughty in using the crc32 name for something that: a) is not the "most standard" crc b) is not even really a crc anyway -i t is more correctly a checksum.
- The image is padded out to 64k, because that is what is
- typically used to write the image to the boot medium.
- */
"typically used" - by what or whom? Is there any rechnical reason for such padding? If not, can we not rather omit this?
The files are often concatenated into blocks of 4 repeats and are also often written into NAND and aligning them to 64k makes some sense.
+/*
- Some byte marshalling functions...
- These load or get little endian values to/from the
- buffer.
- */
+static void load_le16(uint8_t *buf, uint16_t v) +{
- buf[0] = (v >> 0) & 0xff;
- buf[1] = (v >> 8) & 0xff;
+}
+static void load_le32(uint8_t *buf, uint32_t v) +{
- buf[0] = (v >> 0) & 0xff;
- buf[1] = (v >> 8) & 0xff;
- buf[2] = (v >> 16) & 0xff;
- buf[3] = (v >> 24) & 0xff;
+}
These are misnomers. You do not load something, but instead you store the value of 'v' into the buffer 'buf'.
And why do you invent new functions here instead of using existing code (like put_unaligned_le16(), put_unaligned_le32()) ?
I was not aware of the existence of these functions.
Thank you.
+static uint16_t get_le16(const uint8_t *buf) +{
- uint16_t retval;
- retval = (((uint16_t) buf[0]) << 0) |
(((uint16_t) buf[1]) << 8);- return retval;
+}
+static uint32_t get_le32(const uint8_t *buf) +{
- uint32_t retval;
- retval = (((uint32_t) buf[0]) << 0) |
(((uint32_t) buf[1]) << 8) |(((uint32_t) buf[2]) << 16) |(((uint32_t) buf[3]) << 24);- return retval;
+}
Why do you not use existing code (like get_unaligned_le16(), get_unaligned_le32()) ?
I was not aware of the existence of these functions.
Thank you.
+static int align4(int v) +{
- return ((v + 3) / 4) * 4;
+}
Don't we have macros to do this?
Possibly, I will look.
Thanks
Charles