On Friday 28 February 2014 10:57:21 Wolfgang Denk wrote: > Dear Charles, > > In message <[email protected]> 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 <[email protected]> > > + * > > + * 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
