[coreboot] ft4232-based spi programmer
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Tue Jun 16 23:12:25 CEST 2009
On 16.06.2009 19:44, Uwe Hermann wrote:
> 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 <pgf at laptop.org>
>>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>>
>
> Acked-by: Uwe Hermann <uwe at hermann-uwe.de>
>
> 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.
>
Thanks!
>> 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
>
Ah yes, that's an artifact of how the detection works. Basically, the
main makefile includes a helper makefile (.features) which is generated
by the features target. However, since that means .features is changed
after make features, make restarts to read in the new file.
I have another version of the makefile patch, but it is really ugly. It
does avoid the restart, though.
>> @@ -22,6 +22,7 @@
>> CC ?= gcc
>> STRIP = strip
>> INSTALL = install
>> +DIFF = diff
>>
>
> Please use space for indentation here as the other lines do.
>
Done.
>> 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 <pgf at laptop.org>
>> + * Copyright (C) 2009 Carl-Daniel Hailfinger
>>
>
>
>
>> + * based initially on example program bitbang_ft2232.c from
>> + * the libftdi-0.16 distribution, from:
>> + * Intra2net AG <opensource at intra2net.com>
>>
>
> Please drop this, I can't spot any non-trivial amount of code which
> comes/remains from that file.
>
Done.
>> + // 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.
>
Yes, will be handled in a followup patch.
>> + 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.
>
Capital letter at the beginning: OK.
Full stop at the end: Sorry, these are not sentences. The Linux kernel
has no full stops at the end of its messages. Most other open source
projects don't have full stops at the end of their messages. Actually,
I'm thinging about removing all those full stops. It would certainly
help reduce size of our binaries and of all logs by a few bytes.
> For stderr output we might also add "ERROR:" in front, at least for
> critical errors, dunno.
>
Hm maybe. flashrom output is not consistent in this regard.
>> + 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...
>
No idea. Paul?
>> +
>> + 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.
>
printf_debug is often unusable for everyone besides the original
programmer. Maybe we could drop the message.
>> @@ -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.
>
Done.
Thanks for the review, committed in r598.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list