Re: [coreboot] ft4232-based spi programmer

[Uwe Hermann]

On Tue, Jun 16, 2009 at 01:42:33PM +0200, Carl-Daniel Hailfinger wrote:
> On 12.06.2009 12:22, Carl-Daniel Hailfinger wrote:
> > This patch adds support for a new SPI programmer, based on the
> > FT2232H/4232H chip from FTDI.
> >
> > FTDI support is autodetected during compilation.
> >
> > Paul writes:
> > There are certainly possible improvements: The code has hard-coded
> > values for which interface of the ftdi chip to use (interface B was
> > chosen because libftdi seems to have trouble with A right now), what
> > clock rate use for the SPI interface (I've been running at 30Mhz, but
> > the patch sets it to 10Mhz), and possibly others. I think this means
> > that per-programmer options might be a good idea at some point.
> >
> > Carl-Daniel writes:
> > There is one additional FIXME comment in the code, but AFAICS that
> > problem is not solvable with current libftdi.
> >
> > Signed-off-by: Paul Fox 
> > Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Uwe Hermann

Looks ok to me, builds fine with and without libftdi installed. There
are some minor comments below, but those issues can be tackled in
another patch.
> Index: flashrom-ftdi2232_spi/Makefile
>
> --- flashrom-ftdi2232_spi/Makefile	(Revision 597)
> +++ flashrom-ftdi2232_spi/Makefile	(Arbeitskopie)

Something here should be fixed probably, some of the tests are performed
multiple times it seems and some printf's are confusing:

$ make
Checking for a C compiler... found.
Checking for FTDI support... not found.
Checking for a C compiler... found.
Checking for FTDI support... not found.
Checking for pciutils and zlib... found.
[...]
$ make  (again, after a successful build)
Checking for a C compiler... found.
Checking for FTDI support... not found.
Checking for pciutils and zlib... found.
$ make distclean
Checking for a C compiler... found.
Checking for FTDI support... not found.
rm -f flashrom *.o
rm -f .dependencies .features

With libftdi installed:

$ make
Checking for a C compiler... found.
Checking for FTDI support... found.
Checking for a C compiler... found.
Checking for FTDI support... found.
Checking for pciutils and zlib... found.
cc -Os -Wall -Werror  -DFT2232_SPI_SUPPORT=1 -c chipset_enable.c -o chipset_enable.o
[...]
$ make
Checking for a C compiler... found.
Checking for FTDI support... found.
Checking for pciutils and zlib... found.
$ make distclean
Checking for a C compiler... found.
Checking for FTDI support... found.
rm -f flashrom *.o
rm -f .dependencies .features
> @@ -22,6 +22,7 @@
>  CC      ?= gcc
>  STRIP   = strip
>  INSTALL = install
> +DIFF	= diff

Please use space for indentation here as the other lines do.
> Index: flashrom-ftdi2232_spi/ft2232_spi.c
>
> --- flashrom-ftdi2232_spi/ft2232_spi.c	(Revision 0)
> +++ flashrom-ftdi2232_spi/ft2232_spi.c	(Revision 0)
> @@ -0,0 +1,288 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2009 Paul Fox
> Copyright (C) 2009 Carl-Daniel Hailfinger
> based initially on example program bitbang_ft2232.c from
> the libftdi-0.16 distribution, from:
> Intra2net AG

Please drop this, I can't spot any non-trivial amount of code which
comes/remains from that file.
> // f = ftdi_usb_open(ftdic, 0x0403, 0x6010); // FT2232
> f = ftdi_usb_open(ftdic, 0x0403, 0x6011); // FT4232

Yep, this should be coupled with a command line option later, where the
user specifies the USB IDs or the like.
> if (f < 0 && f != -5) {
> fprintf(stderr, "unable to open ftdi device: %d (%s)\n", f,
> 		ftdi_get_error_string(ftdic));

Please start all messages to stdout or stderr with capital letter and
end them with full stop for consistency, all other flashrom output does
the same.

For stderr output we might also add "ERROR:" in front, at least for
critical errors, dunno.
> exit(-1);
> }
> if (ftdi_set_interface(ftdic, INTERFACE_B) < 0) {
> fprintf(stderr, "unable to select FT2232 channel B: %s\n",
> 		ftdic->error_str);
> }
> if (ftdi_usb_reset(ftdic) < 0) {
> fprintf(stderr, "unable to reset ftdi device\n");
> }

There's no return or exit here, are these non-critical? Same for other
such checks...
> if (ftdi_set_latency_timer(ftdic, 2) < 0) {
> fprintf(stderr, "unable to set latency timer\n");
> }
> if (ftdi_write_data_set_chunksize(ftdic, 512)) {
> fprintf(stderr, "unable to set chunk size\n");
> }
> if (ftdi_set_bitmode(ftdic, 0x00, 2) < 0) {
> fprintf(stderr, "unable to set bitmode\n");
> }
> printf_debug("\nft2232 chosen\n");

This is a bit unclear, please make the message more usable.
> @@ -589,7 +605,7 @@
>         "   -i | --image <name>:              only flash image name from flash layout\n"
>         "   -L | --list-supported:            print supported devices\n"
>         "   -p | --programmer <name>:         specify the programmer device\n"
>     "                                     (internal, dummy, nic3com, satasii, it87spi)\n"
>     "                                     (internal, dummy, nic3com, satasii, it87spi, ft2232spi)\n"

TODO: Document new programmer in manpage.

Uwe.