Hi Bernhard,

On Mon, 30 May 2016 23:23:31 +0200
Bernhard Nortmann <[email protected]> wrote:

> Am 30.05.2016 um 17:12 schrieb Boris Brezillon:
> > Generating raw NAND images is particularly useful for boot0 images
> > creation since the mainline driver is not supporting the funky layout
> > used by Allwinner's ROM code to load the boot0 binary from NAND.
> >
> > This tools also allows one to generate raw images for 'normal' partitions
> > so that they can be flashed before soldering on the NAND on the board
> > (using a regular NAND programmer).
> >
> > The tool takes care of generating ECC bytes and randomizing data as
> > expected by the NAND controller, and re-arranging the ECC/data sections
> > correctly.
> >
> > Signed-off-by: Boris Brezillon <[email protected]>
> > ---
> > Hi Siarhei,
> >
> > You seem to be the one maintaining the sunxi-tools repo, and I'm not sure
> > what's the process to submit patches (PR from github, or submitting
> > patches to the linux-sunxi ML).
> >
> > The tool I'm adding here is really useful when it comes to creating NAND
> > images, and I'd like to see it included in the sunxi-tools.
> >
> > Let me know if you have any questions.
> >
> > Thanks,
> >
> > Boris
> >
> >   Makefile             |    2 +-
> >   README.md            |    3 +
> >   nand-image-builder.c | 1122 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 1126 insertions(+), 1 deletion(-)
> >   create mode 100644 nand-image-builder.c
> >
> > diff --git a/Makefile b/Makefile
> > index 623dda1..434f084 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -33,7 +33,7 @@ DEFINES += -D_NETBSD_SOURCE
> >   endif
> >   
> >   # Tools useful on host and target
> > -TOOLS = sunxi-fexc sunxi-bootinfo sunxi-fel sunxi-nand-part
> > +TOOLS = sunxi-fexc sunxi-bootinfo sunxi-fel sunxi-nand-part 
> > sunxi-nand-image-builder  
> 
> Given the rather specific nature of this utility, I'm inclined to prefer 
> that
> it be added to the MISC_TOOLS target instead.

Sure.

> 
[...]

> > +
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <stdio.h>
> > +#include <linux/kernel.h>  
> What's this include for? It breaks compilation for OSX 
> (https://travis-ci.org/n1tehawk/sunxi-tools/jobs/133998103)
> 
> > +#include <linux/errno.h>  
> Same as above. Should be replaced by a generic "#include <errno.h>".

Yep, will remove or modify these inclusions.

> 
> > +#include <asm/byteorder.h>
> > +#include <endian.h>  
> Again not available on OSX 
> (https://travis-ci.org/n1tehawk/sunxi-tools/jobs/134007716
> and https://travis-ci.org/n1tehawk/sunxi-tools/jobs/134013177)
> 
> You might want to have a look at the portable_endian.h available in our 
> include/
> subdir. Substituting '#include "portable_endian.h"' here seems to work fine.

Okay, I'll switch to protable_endian.h then.

[...]

> > +
> > +struct image_info {
> > +   int ecc_strength;
> > +   int ecc_step_size;
> > +   int page_size;
> > +   int oob_size;
> > +   int usable_page_size;
> > +   int eraseblock_size;
> > +   int scramble;
> > +   int boot0;
> > +   loff_t offset;  
> loff_t is gcc-specific(?), and once again breaks on OSX:
> https://travis-ci.org/n1tehawk/sunxi-tools/jobs/134014262
> 
> Is there a reason why the standard "off_t" is insufficient here?

Nope, I'll switch to off_t.

> 
> > +   const char *source;
> > +   const char *dest;
> > +};
> > +
[...]

> > +static inline int parity(unsigned int x)
> > +{
> > +   /*
> > +    * public domain code snippet, lifted from
> > +    * http://www-graphics.stanford.edu/~seander/bithacks.html
> > +    */
> > +   x ^= x >> 1;
> > +   x ^= x >> 2;
> > +   x = (x & 0x11111111U) * 0x11111111U;
> > +   return (x >> 28) & 1;
> > +}  
> Function parity() is unused, which makes clang unhappy:
> https://travis-ci.org/n1tehawk/sunxi-tools/jobs/133998102
> 
> If the code is supposed to remain, e.g. for clarity's sake / reference,
> I suggest enclosing it with "#if 0" [...] "#endif".

Nope, they should be removed, it's just that gcc was not complaining
about unused functions because of the inline specifier.

I'll remove all the functions you pointed as unused.

[...]

> 
> I have verified that with
> https://github.com/n1tehawk/sunxi-tools/commit/4f71a411e0b28b8c737c0e2948b0676ea4c78e8a
> applied, our build tests pass 
> (https://travis-ci.org/n1tehawk/sunxi-tools/builds/134018068).

Cool, I'll fix the implementation accordingly, send a PR and notify the
presence of new version on the ML (unless you want me to post a v2 of
this patch on the ML).

Thanks for the review and fixes.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/d/optout.

Reply via email to