Add Tegra2 SPI controller.

Add Tegra2 SPI controller.

Co-worked with David Hendrix (dhendrix@) to finalize this difficult tasks. The credits of porting to ARM platform go to Stefan (reinauer@).

Original code comes from u-boot, and is adapted to flashrom call model. The main change is fixing the endian issue (refer to next4Bytes(), for multiple bytes write/read in one GO). Another is we move UART disable in spi_init() so that there is no delay between every CS activate. We get quite good results (see TEST field below).


Change-Id: Ica523803491c270059584695b3cff0cb68234425

R=dhendrix@chromium.org
BUG=chrome-os-partner:2825
TEST=Tested the following cases on Tegra2 Seaboard. Compiled successfully and tested on x86.

% flashrom -V | grep Found
W25Q16 ...
% flashrom --wp-status
% flashrom --wp-range 0x100000 0x100000
% flashrom --wp-range 0 0
% flashrom --wp-enable
% flashrom --wp-disable
% flashrom -r /tmp/flash
  (it takes  5.6 secs, w/o fmap search)
% flashrom -w /tmp/diff_content
  (it takes 32.6 secs, w/o fmap search)
% flashrom -E
  (it takes 20.8 secs, w/o fmap search)

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=bc351d0

[dhendrix, 2011-03-25 02:08:06]

I am still reviewing the stuff in tegra2_spi.c, but I have a few comments to
help you clean-up.

Also, please rebase this CL so that we isolate the tegra2 changes from other
stuff.

[Olof Johansson, 2011-03-25 04:08:38]

I don't understand why you've chosen to re-implement the driver in userspace
when there's already a kernel driver that exposes a standard interface to
userspace. Is it somehow lacking for this kind of use? Should it be improved?

[yjlou, 2011-03-25 04:21:45]

Good question. Many applications use flashrom on x86 platform, for example,
firmware updater, HWQual and factory install shim. We hope those program can be
ported to ARM with less modification as possible. 

Adapting flashrom is one of solutions, and also the easiest solution to provide
flash chip ID query, write protection, and partial read/write/erase features.

On 2011/03/25 04:08:38, Olof Johansson wrote:
> I don't understand why you've chosen to re-implement the driver in userspace
> when there's already a kernel driver that exposes a standard interface to
> userspace. Is it somehow lacking for this kind of use? Should it be improved?

[dhendrix, 2011-03-25 04:34:43]

mostly cosmetic nit-picks. overall, good stuff!

http://codereview.chromium.org/6731011/diff/3001/physmap.c
File physmap.c (right):

http://codereview.chromium.org/6731011/diff/3001/physmap.c#newcode523
physmap.c:523: /* MSRs do not exist on non-x86 architectures. */
You can remove this file from the patch set. We'll correct the comment in
upstream flashrom instead. It will appear in our repo next time we do a sync-up.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c
File tegra2_spi.c (right):

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode32
tegra2_spi.c:32: int need_spi_clock_disable;
you can remove the need_spi_clock_disable variable

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode87
tegra2_spi.c:87: 
2 extra newlines

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode138
tegra2_spi.c:138: 
extra newline

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode146
tegra2_spi.c:146: 
extra newline

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode153
tegra2_spi.c:153: 
extra newline

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode164
tegra2_spi.c:164: 
extra newline

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode237
tegra2_spi.c:237: * Also upates the byte counts reminded and to be consumed in
this round.
suggest alternative wording: "Also updates the byte count remaining to be used
this round."

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode265
tegra2_spi.c:265: int next4Bytes(uint32_t *writecnt, uint32_t *readcnt, int
*bits,
I recommend renaming bits to something like num_bits so that it is not confused
for an input or output buffer.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode275
tegra2_spi.c:275: *to_read = min(*readcnt, 4 - *to_write);
min(*readcnt, 4) since *to_write is 0.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode288
tegra2_spi.c:288: return 0;  /* handled write and read requests. */
Maybe just return *bits? The loop in tegra2_spi_send_command() only checks if
*bits == 0.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode292
tegra2_spi.c:292: * Tegra2 FIFO design is ... interesting. For example, you want
to Tx 2 bytes:
LOL! Excellent comment :-)

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode311
tegra2_spi.c:311: * left-shifts 1 bit for every bit comes in. Hence, after
reading 3 byes,
s/byes/bytes

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode325
tegra2_spi.c:325: uint32_t status;
The "status" variable isn't really used anymore -- it's only assigned. I think
you can get rid of it.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode355
tegra2_spi.c:355: (SPI_STAT_BSY | SPI_STAT_RDY)) != SPI_STAT_RDY;
heh, this is getting a little bit too complicated to fit cleanly into a control
statement imho :-)

I suggest:
for (tm = 0; tm < SPI_TIMEOUT; tm++) {
        if (mmio_readl(spi_sts) &
           (SPI_STAT_BSY | SPI_STAT_RDY) == SPI_STAT_RDY)
                break;
        programmer_delay(10);
}

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode360
tegra2_spi.c:360: if (tm >= SPI_TIMEOUT) {
Is this necessary as to avoid using the UART? Maybe add a comment to that
effect.

Actually, this might look better if you simply break when the timeout is reached
and then decide to print the message later. You can also get rid of the
"delayed_msg variable that way. For example:
if (tm >= SPI_TIMEOUT) {
        retval = -1;
        break;
}
...
mmio_writel(mmio_readl(spi_sts), spi_sts)
spi_cs_deactivate();
/* print this after spi_cs_deactivate since console output may utilize UART */
if (timeout >= SPI_TIMEOUT)
        msg_perr("%s(): %d BSY & RDY timout ...");

return retval;

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode421
tegra2_spi.c:421: clkrst_enb = (uint32_t *)(clkrst_base +
CLK_RST_ENB_H_0_OFFSET);
You can remove this part. It is no longer needed since spi_init() sets up all
the clocks.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode435
tegra2_spi.c:435: if (need_spi_clock_disable) {
You can remove this part.

Perhaps we should make more effort to get the reverse-MMIO writes patch
integrated so that we can use it in spi_init() to un-do any clock enable
routines. (That's for another CL...)

[dhendrix, 2011-03-25 04:41:53]

On 2011/03/25 04:08:38, Olof Johansson wrote:
> I don't understand why you've chosen to re-implement the driver in userspace
> when there's already a kernel driver that exposes a standard interface to
> userspace. Is it somehow lacking for this kind of use? Should it be improved?

In short, it didn't work. spidev did not expose the necessary interfaces and
tegra_spi did not recognize this SoC.

If possible, we should make necessary improvements/additions to the kernel code
since it probably does a better job at managing the clock and reset registers.

[Louis, 2011-03-25 10:55:48]

David, thanks for the detailed review. I've changed my code according to your
comments, and also replied if I didn't change code. Please take a look of them.

Note that I also refined the issue description about how I tested. Please let me
know if you think more tests are needed.

http://codereview.chromium.org/6731011/diff/3001/physmap.c
File physmap.c (right):

http://codereview.chromium.org/6731011/diff/3001/physmap.c#newcode523
physmap.c:523: /* MSRs do not exist on non-x86 architectures. */
On 2011/03/25 04:34:43, dhendrix wrote:
> You can remove this file from the patch set. We'll correct the comment in
> upstream flashrom instead. It will appear in our repo next time we do a
sync-up.

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c
File tegra2_spi.c (right):

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode32
tegra2_spi.c:32: int need_spi_clock_disable;
On 2011/03/25 04:34:43, dhendrix wrote:
> you can remove the need_spi_clock_disable variable

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode87
tegra2_spi.c:87: 
On 2011/03/25 04:34:43, dhendrix wrote:
> 2 extra newlines

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode138
tegra2_spi.c:138: 
On 2011/03/25 04:34:43, dhendrix wrote:
> extra newline

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode146
tegra2_spi.c:146: 
On 2011/03/25 04:34:43, dhendrix wrote:
> extra newline

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode153
tegra2_spi.c:153: 
On 2011/03/25 04:34:43, dhendrix wrote:
> extra newline

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode164
tegra2_spi.c:164: 
On 2011/03/25 04:34:43, dhendrix wrote:
> extra newline

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode237
tegra2_spi.c:237: * Also upates the byte counts reminded and to be consumed in
this round.
On 2011/03/25 04:34:43, dhendrix wrote:
> suggest alternative wording: "Also updates the byte count remaining to be used
> this round."

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode265
tegra2_spi.c:265: int next4Bytes(uint32_t *writecnt, uint32_t *readcnt, int
*bits,
On 2011/03/25 04:34:43, dhendrix wrote:
> I recommend renaming bits to something like num_bits so that it is not
confused
> for an input or output buffer.

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode275
tegra2_spi.c:275: *to_read = min(*readcnt, 4 - *to_write);
This case has been optimized and refined. :-)

On 2011/03/25 04:34:43, dhendrix wrote:
> min(*readcnt, 4) since *to_write is 0.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode288
tegra2_spi.c:288: return 0;  /* handled write and read requests. */
Intentionally separate them because the meanings are slightly different:

  num_bits is stored the cycle count for this transaction.
  function return value is to indicate if a further call is needed.

But if you assist, I welcome your argument. :-)

On 2011/03/25 04:34:43, dhendrix wrote:
> Maybe just return *bits? The loop in tegra2_spi_send_command() only checks if
> *bits == 0.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode292
tegra2_spi.c:292: * Tegra2 FIFO design is ... interesting. For example, you want
to Tx 2 bytes:
On 2011/03/25 04:34:43, dhendrix wrote:
> LOL! Excellent comment :-)

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode311
tegra2_spi.c:311: * left-shifts 1 bit for every bit comes in. Hence, after
reading 3 byes,
On 2011/03/25 04:34:43, dhendrix wrote:
> s/byes/bytes

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode325
tegra2_spi.c:325: uint32_t status;
It is used in line 365 for snprintf delayed_msg.
On 2011/03/25 04:34:43, dhendrix wrote:
> The "status" variable isn't really used anymore -- it's only assigned. I think
> you can get rid of it.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode355
tegra2_spi.c:355: (SPI_STAT_BSY | SPI_STAT_RDY)) != SPI_STAT_RDY;
Good suggestion. Thanks.

On 2011/03/25 04:34:43, dhendrix wrote:
> heh, this is getting a little bit too complicated to fit cleanly into a
control
> statement imho :-)
> 
> I suggest:
> for (tm = 0; tm < SPI_TIMEOUT; tm++) {
>         if (mmio_readl(spi_sts) &
>            (SPI_STAT_BSY | SPI_STAT_RDY) == SPI_STAT_RDY)
>                 break;
>         programmer_delay(10);
> }

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode360
tegra2_spi.c:360: if (tm >= SPI_TIMEOUT) {
Ah... I just think more for future if we need to add another delayed_msg. Let me
know if you feel uncomfortable. :-)

Also added reason in comment.

On 2011/03/25 04:34:43, dhendrix wrote:
> Is this necessary as to avoid using the UART? Maybe add a comment to that
> effect.
> 
> Actually, this might look better if you simply break when the timeout is
reached
> and then decide to print the message later. You can also get rid of the
> "delayed_msg variable that way. For example:
> if (tm >= SPI_TIMEOUT) {
>         retval = -1;
>         break;
> }
> ...
> mmio_writel(mmio_readl(spi_sts), spi_sts)
> spi_cs_deactivate();
> /* print this after spi_cs_deactivate since console output may utilize UART */
> if (timeout >= SPI_TIMEOUT)
>         msg_perr("%s(): %d BSY & RDY timout ...");
> 
> return retval;

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode421
tegra2_spi.c:421: clkrst_enb = (uint32_t *)(clkrst_base +
CLK_RST_ENB_H_0_OFFSET);
On 2011/03/25 04:34:43, dhendrix wrote:
> You can remove this part. It is no longer needed since spi_init() sets up all
> the clocks.

Done.

http://codereview.chromium.org/6731011/diff/3001/tegra2_spi.c#newcode435
tegra2_spi.c:435: if (need_spi_clock_disable) {
On 2011/03/25 04:34:43, dhendrix wrote:
> You can remove this part.
> 
> Perhaps we should make more effort to get the reverse-MMIO writes patch
> integrated so that we can use it in spi_init() to un-do any clock enable
> routines. (That's for another CL...)

Done.

[Stefan Reinauer, 2011-03-25 16:56:07]

On 2011/03/25 04:08:38, Olof Johansson wrote:
> I don't understand why you've chosen to re-implement the driver in userspace
> when there's already a kernel driver that exposes a standard interface to
> userspace. Is it somehow lacking for this kind of use? Should it be improved?

That's probably my fault. In the initial version I took the SPI code from uboot,
not knowing whether there was a Linux SPI driver that could be used. Generally
flashrom likes to include the hardware drivers itself, so it can be fixed /
updated without touching other projects. On the other hand, this attitude has
its roots on x86, where there are/were no usable SPI drivers at the time SPI
support was developed.  

In fact, there is a very recent extension to flashrom that allows using Linux
SPI drivers for the deed: 

http://www.flashrom.org/pipermail/flashrom/2011-February/005862.html

Maybe we should merge this into our tree?

Is the Linux SPI driver used for anything else on our Tegra2 platforms?

[Stefan Reinauer, 2011-03-25 16:57:47]

Old comments. forgot to publish. Mostly obsolete now I guess

http://codereview.chromium.org/6731011/diff/3001/Makefile
File Makefile (right):

http://codereview.chromium.org/6731011/diff/3001/Makefile#newcode120
Makefile:120: CONFIG_SERPROG ?= no
I think this should go into the ebuild instead. On the other hand, disabling all
those options could kind of make sense for the x86 versions of flashrom on
chromeos machines, too.

http://codereview.chromium.org/6731011/diff/3001/cli_mfg.c
File cli_mfg.c (left):

http://codereview.chromium.org/6731011/diff/3001/cli_mfg.c#oldcode453
cli_mfg.c:453: programmer_table[programmer].name);
This was separated out into an extra CL

http://codereview.chromium.org/6731011/diff/3001/internal.c
File internal.c (right):

http://codereview.chromium.org/6731011/diff/3001/internal.c#newcode253
internal.c:253: #endif
everything up to here in internal.c should be dropped as it has been factored
out into an extra CL

http://codereview.chromium.org/6731011/diff/3001/mec1308.c
File mec1308.c (right):

http://codereview.chromium.org/6731011/diff/3001/mec1308.c#newcode35
mec1308.c:35: #if defined(__i386__) || defined(__x86_64__)
I think this decision should be made elsewhere. And/or it should probably be
HAVE_IO_INSTRUCTIONS or something

[Stefan Reinauer, 2011-03-25 17:01:17]

some dead code that remained from the early attempts.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h
File tegra2_spi.h (right):

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h#newcode56
tegra2_spi.h:56: 
The whole "go away code" section is not used anymore and should be dropped.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h#newcode69
tegra2_spi.h:69: } spi_tegra_t;
This struct is no longer used. It should be dropped.

[sjg, 2011-03-25 17:20:44]

Hi,

We should get a clear statement of why this user space program is
re-implementing a driver. It is hard to match the performance of a kernel
driver, it is re-implementing existing code, it potentially creates conflicts
with other user space programs. There are probably other bad things.

What am I missing here?

Thanks,
Simon

[Stefan Reinauer, 2011-03-25 17:22:03]

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c
File tegra2_spi.c (right):

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode411
tegra2_spi.c:411: spi_init();
It seems spi_init() could just be rolled into this function. Having
tegra2_spi_init() and a separate spi_init() does not add to making the code
easier to understand.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode420
tegra2_spi.c:420: spi_shutdown();
Same applies for tegra2_spi_shutdown() and spi_shutdown().

[Stefan Reinauer, 2011-03-25 17:24:38]

On 2011/03/25 17:20:44, sjg wrote:
> Hi,
> 
> We should get a clear statement of why this user space program is
> re-implementing a driver. It is hard to match the performance of a kernel
> driver, it is re-implementing existing code, it potentially creates conflicts
> with other user space programs. There are probably other bad things.

The goal was to be as close to the uboot driver as possible, based on the
assumption that the kernel SPI driver could not be used.

This changed, so we should revisit the decision. From a flashrom perspective it
would be nice to be able to flash the firmware without potentially having to
recompile the kernel. In our case this does not matter of course.

[sjg, 2011-03-25 17:29:07]

On 2011/03/25 17:24:38, Stefan Reinauer wrote:
> On 2011/03/25 17:20:44, sjg wrote:
> > Hi,
> > 
> > We should get a clear statement of why this user space program is
> > re-implementing a driver. It is hard to match the performance of a kernel
> > driver, it is re-implementing existing code, it potentially creates
conflicts
> > with other user space programs. There are probably other bad things.
> 
> The goal was to be as close to the uboot driver as possible, based on the
> assumption that the kernel SPI driver could not be used.
> 
> This changed, so we should revisit the decision. From a flashrom perspective
it
> would be nice to be able to flash the firmware without potentially having to
> recompile the kernel. In our case this does not matter of course.

OK thanks, not sure why being close to U-Boot is a good thing. Why would you
need to recompile the kernel? Are you talking about during development (suggest
use modules / NFS root / debugger), or something else?

So does this mean that this CL will be re-done?

[Stefan Reinauer, 2011-03-25 17:34:06]

On 2011/03/25 17:20:44, sjg wrote:
> We should get a clear statement of why this user space program is
> re-implementing a driver. It is hard to match the performance of a kernel
> driver, it is re-implementing existing code, it potentially creates conflicts
> with other user space programs. There are probably other bad things.

Has anyone benchmarked the performance of flashrom + linux-spi vs flashrom +
built-in driver? It's not completely obvious why the kernel driver would be
faster.
I agree, touching hardware outside of the kernel is at the best slightly
ill-designed and potentially dangerous. However, it is a sane thing to do on our
x86 platforms, where kernel support for on-board/on-chip components is not as
wide spread as on ARM systems.

[sjg, 2011-03-25 18:00:55]

On 2011/03/25 17:34:06, Stefan Reinauer wrote:
> On 2011/03/25 17:20:44, sjg wrote:
> > We should get a clear statement of why this user space program is
> > re-implementing a driver. It is hard to match the performance of a kernel
> > driver, it is re-implementing existing code, it potentially creates
conflicts
> > with other user space programs. There are probably other bad things.
> 
> Has anyone benchmarked the performance of flashrom + linux-spi vs flashrom +
> built-in driver? It's not completely obvious why the kernel driver would be
> faster.
> I agree, touching hardware outside of the kernel is at the best slightly
> ill-designed and potentially dangerous. However, it is a sane thing to do on
our
> x86 platforms, where kernel support for on-board/on-chip components is not as
> wide spread as on ARM systems.

After discussions we are running with this for now, with a plan to revisit when
there is time.

Thanks for the info.

-Simon

[robotboy, 2011-03-25 22:15:56]

I've managed to get this to write U-Boot to SPI on Kaen, but only with an
additional patch from Dave that disables fmap support.

[Stefan Reinauer, 2011-03-25 22:17:08]

On 2011/03/25 22:15:56, robotboy wrote:
> I've managed to get this to write U-Boot to SPI on Kaen, but only with an
> additional patch from Dave that disables fmap support.

Can we figure out a way to fix fmap on ARM rather than disabling it?

[dhendrix, 2011-03-25 22:32:43]

On 2011/03/25 22:17:08, Stefan Reinauer wrote:
> On 2011/03/25 22:15:56, robotboy wrote:
> > I've managed to get this to write U-Boot to SPI on Kaen, but only with an
> > additional patch from Dave that disables fmap support.
> 
> Can we figure out a way to fix fmap on ARM rather than disabling it?

The problem that Anton had was that the existing ROM image was filled with
somewhat-legit looking garbage which possibly contained multiple copies of a
flash map for an x86 platform. So it was barfing because the existing ROM image
had >64 FMAP regions somehow. I think this is just a corner case...

We thought to use the -E option to erase the ROM, but that code path still
includes fmap processing. I think we should just make -E skip fmap. Comments?
Concerns? Voice them here: http://codereview.chromium.org/6749019

[dhendrix, 2011-03-26 06:47:32]

LGTM, by the way :-)

Bonus: We've already had a few successful trials.

n 2011/03/25 22:32:43, dhendrix wrote:
> On 2011/03/25 22:17:08, Stefan Reinauer wrote:
> > On 2011/03/25 22:15:56, robotboy wrote:
> > > I've managed to get this to write U-Boot to SPI on Kaen, but only with an
> > > additional patch from Dave that disables fmap support.
> > 
> > Can we figure out a way to fix fmap on ARM rather than disabling it?
> 
> The problem that Anton had was that the existing ROM image was filled with
> somewhat-legit looking garbage which possibly contained multiple copies of a
> flash map for an x86 platform. So it was barfing because the existing ROM
image
> had >64 FMAP regions somehow. I think this is just a corner case...
> 
> We thought to use the -E option to erase the ROM, but that code path still
> includes fmap processing. I think we should just make -E skip fmap. Comments?
> Concerns? Voice them here: http://codereview.chromium.org/6749019

[Micah C, 2011-03-28 02:08:22]

A few drive-by comments about readability and questions about constant delays.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c
File processor_enable.c (right):

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode89
processor_enable.c:89: static int is_tegra2(void)
This function is complex enough to justify a comment explaining the goal. 
Something like this:

// Return true if executing on an Nvidia Tegra20 CPU, indicated when
/proc/cpuinfo contains a line like "CPU part : 0xc09"

In the future this will probably need to be extended to support a list of CPU
part identifiers, since future CPUs in the family will likely include a common
control interface.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode104
processor_enable.c:104: if (strncmp(ptr, "CPU part", sizeof("CPU part") - 1) ==
0)
The appearance of the constant string "CPU part" 3 times in two lines suggests
that it might benefit from being a named constant.

How about declaring:
const char *field = "CPU part";
const char *value = "0xc09";

and then using the named constants for the comparison and the sizeof operators? 
 Some future implementation might replace the "value" with another loop over a
list of values, and using a named variable will make that be a smaller change.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode111
processor_enable.c:111: fclose(cpuinfo);
This seems like a surprising place to close the file descriptor, but I see that
you're doing this to make sure that it's not leaked when returning from within
this loop.

How about changing the looping structure so that the loop always ends, and there
is just 1 return path from the function?  Maybe use a local variable to
represent the return value, and then forcing it to 1 or 0 based on the loop exit
condition?

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode134
processor_enable.c:134: msg_pinfo("Detected NVIDIA Tegra 2.\n");
Is this a temporary debugging message, or something that's valuable for the
final implementation?  Are there similar logging messages for the other chipsets
that flashrom supports?

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c
File tegra2_spi.c (right):

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode164
tegra2_spi.c:164: programmer_delay(100000);
This is a big delay.  Is there any other signal that can be used to indicate the
transfer has been finished which might sometimes come sooner than 100ms?

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode182
tegra2_spi.c:182: programmer_delay(2000);
Is there some status register that can detect UART Tx FIFO empty which is more
reliable than a CPU delay?

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode190
tegra2_spi.c:190: * Looks like we may also need to dynamically change the
pinmux,
It might be a good idea to create functions like "save_pinmux_state()" and
"restore_pinmux_state()" to use from spi_init() and spi_shutdown().  If there
are many different boards in the world with Tegra2 CPUs, they might have
different pinmux settings than Seaboard, so hardcoding the constant settings in
spi_shutdown may be brittle.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode345
tegra2_spi.c:345: programmer_delay(10);
Is this "10" the same 10us that's multiplied into the SPI_TIMEOUT definition? 
Perhaps it deserves its own constant if it's something that's specified for the
controller interface.

[dhendrix, 2011-03-28 05:06:05]

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c
File processor_enable.c (right):

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode134
processor_enable.c:134: msg_pinfo("Detected NVIDIA Tegra 2.\n");
On 2011/03/28 02:08:22, Micah C wrote:
> Is this a temporary debugging message, or something that's valuable for the
> final implementation?  Are there similar logging messages for the other
chipsets
> that flashrom supports?

Nice catch. We should suppress this for normal usage and use "msg_pdbg()"
instead.

There is a similar message for other chipsets, but I believe that is #ifdef'd
out since it's within a bunch of code that is more specific to x86...

[Louis, 2011-03-29 07:55:02]

Replied Stefan's old comments. All are submitted by other CLs.

http://codereview.chromium.org/6731011/diff/3001/Makefile
File Makefile (right):

http://codereview.chromium.org/6731011/diff/3001/Makefile#newcode120
Makefile:120: CONFIG_SERPROG ?= no
Oooooops. David has committed this in CL 6732047.

On 2011/03/25 16:57:47, Stefan Reinauer wrote:
> I think this should go into the ebuild instead. On the other hand, disabling
all
> those options could kind of make sense for the x86 versions of flashrom on
> chromeos machines, too.

http://codereview.chromium.org/6731011/diff/3001/cli_mfg.c
File cli_mfg.c (left):

http://codereview.chromium.org/6731011/diff/3001/cli_mfg.c#oldcode453
cli_mfg.c:453: programmer_table[programmer].name);
On 2011/03/25 16:57:47, Stefan Reinauer wrote:
> This was separated out into an extra CL

Done.

http://codereview.chromium.org/6731011/diff/3001/internal.c
File internal.c (right):

http://codereview.chromium.org/6731011/diff/3001/internal.c#newcode253
internal.c:253: #endif
On 2011/03/25 16:57:47, Stefan Reinauer wrote:
> everything up to here in internal.c should be dropped as it has been factored
> out into an extra CL

Done.

http://codereview.chromium.org/6731011/diff/3001/mec1308.c
File mec1308.c (right):

http://codereview.chromium.org/6731011/diff/3001/mec1308.c#newcode35
mec1308.c:35: #if defined(__i386__) || defined(__x86_64__)
hm... the code has be committed in David's CL. 

On 2011/03/25 16:57:47, Stefan Reinauer wrote:
> I think this decision should be made elsewhere. And/or it should probably be
> HAVE_IO_INSTRUCTIONS or something

[Louis, 2011-03-29 08:22:54]

For the reason using flashrom, the most important thing is we want most of
clients working well on x86 can port to ARM smoothly, for example, the factory
install shim, firmware updater, and HWQual.

The rought idea is that clients are platform independent while flashrom handles
platform-specific work as much as possible. We also hope a utility can provide
all flash functions we need in ChromeOS, such as, partial read/write/erase,
flash ID detection and write protection/status report. We also want to keep same
calling conventions from x86 to ARM. Those are why we select flashrom for ARM.

On 2011/03/25 18:00:55, sjg wrote:
> On 2011/03/25 17:34:06, Stefan Reinauer wrote:
> > On 2011/03/25 17:20:44, sjg wrote:
> > > We should get a clear statement of why this user space program is
> > > re-implementing a driver. It is hard to match the performance of a kernel
> > > driver, it is re-implementing existing code, it potentially creates
> conflicts
> > > with other user space programs. There are probably other bad things.
> > 
> > Has anyone benchmarked the performance of flashrom + linux-spi vs flashrom +
> > built-in driver? It's not completely obvious why the kernel driver would be
> > faster.
> > I agree, touching hardware outside of the kernel is at the best slightly
> > ill-designed and potentially dangerous. However, it is a sane thing to do on
> our
> > x86 platforms, where kernel support for on-board/on-chip components is not
as
> > wide spread as on ARM systems.
> 
> After discussions we are running with this for now, with a plan to revisit
when
> there is time.
> 
> Thanks for the info.
> 
> -Simon

[robotboy1, 2011-03-29 15:40:28]

I think the question was more about why flashrom implemented a driver in
user space instead of using the existing SPI driver in the kernel or even
the mtd layer  on top of that SPI driver.

The answer is that the SPI driver in the kernel didn't work when the
flashrom work on ARM started.  And it was concluded that the fastest path
was to port the U-Boot code over.  Once the SPI driver does work then
flashrom can be ported to use it and the mtd layer instead of it's user
space SPI driver.  The interface to all of our tools would remain the same
(it would be flashrom).  But flashrom itself would be much smaller/simpler.

-Anton

On Tue, Mar 29, 2011 at 1:22 AM, <yjlou@chromium.org> wrote:

> For the reason using flashrom, the most important thing is we want most of
> clients working well on x86 can port to ARM smoothly, for example, the
> factory
> install shim, firmware updater, and HWQual.
>
> The rought idea is that clients are platform independent while flashrom
> handles
> platform-specific work as much as possible. We also hope a utility can
> provide
> all flash functions we need in ChromeOS, such as, partial read/write/erase,
> flash ID detection and write protection/status report. We also want to keep
> same
> calling conventions from x86 to ARM. Those are why we select flashrom for
> ARM.
>
>
> On 2011/03/25 18:00:55, sjg wrote:
>
>> On 2011/03/25 17:34:06, Stefan Reinauer wrote:
>> > On 2011/03/25 17:20:44, sjg wrote:
>> > > We should get a clear statement of why this user space program is
>> > > re-implementing a driver. It is hard to match the performance of a
>> kernel
>> > > driver, it is re-implementing existing code, it potentially creates
>> conflicts
>> > > with other user space programs. There are probably other bad things.
>> >
>> > Has anyone benchmarked the performance of flashrom + linux-spi vs
>> flashrom +
>> > built-in driver? It's not completely obvious why the kernel driver would
>> be
>> > faster.
>> > I agree, touching hardware outside of the kernel is at the best slightly
>> > ill-designed and potentially dangerous. However, it is a sane thing to
>> do on
>> our
>> > x86 platforms, where kernel support for on-board/on-chip components is
>> not
>>
> as
>
>> > wide spread as on ARM systems.
>>
>
>  After discussions we are running with this for now, with a plan to revisit
>>
> when
>
>> there is time.
>>
>
>  Thanks for the info.
>>
>
>  -Simon
>>
>
>
>
> http://codereview.chromium.org/6731011/
>

[dhendrix, 2011-03-29 18:24:02]

I think we've beaten this horse dead enough and should submit the code as-is and
follow-up with a CL to implement Micah's suggestions for code clean-up. The
reason is because we have already used this code successfully, and so we already
have a lot of testing done for it. Making more changes invalidates the testing
we've already done.

Let's push this code to git as the first "stable" release with Tegra2 support
and make improvements from there.

[sjg, 2011-03-29 18:45:44]

On 2011/03/29 18:24:02, dhendrix wrote:
> I think we've beaten this horse dead enough and should submit the code as-is
and
> follow-up with a CL to implement Micah's suggestions for code clean-up. The
> reason is because we have already used this code successfully, and so we
already
> have a lot of testing done for it. Making more changes invalidates the testing
> we've already done.
> 
> Let's push this code to git as the first "stable" release with Tegra2 support
> and make improvements from there.

Yes agreed. So where is the test program? I would like to take a look.

[dhendrix, 2011-03-29 19:45:09]

On 2011/03/29 18:45:44, sjg wrote:
> On 2011/03/29 18:24:02, dhendrix wrote:
> > I think we've beaten this horse dead enough and should submit the code as-is
> and
> > follow-up with a CL to implement Micah's suggestions for code clean-up. The
> > reason is because we have already used this code successfully, and so we
> already
> > have a lot of testing done for it. Making more changes invalidates the
testing
> > we've already done.
> > 
> > Let's push this code to git as the first "stable" release with Tegra2
support
> > and make improvements from there.
> 
> Yes agreed. So where is the test program? I would like to take a look.

First and foremost is that Anton has already used flashrom with this patch set
applied to successfully flash the machines that arrived last week. So we already
have a version being used out in the wild that is not tracked in git, which is
obviously a bad thing.

We also have a unit test harness + micro-tests written in shell that are in the
util/ directory of Flashrom (util/README has basic instructions). Here is how to
get it up and running on Kaen:
1) Copy the test scripts (or just util/*.sh if you're feeling lazy) somewhere on
your target machine.

2) Copy the flashrom binary to test to your test machine. Keep in mind that the
test harness expects to find a "stable" version of Flashrom to fall back on
somewhere in your $PATH. But since this doesn't exist on ARM yet, you'll only
have the one test binary to work with.

3) Run the tests. Something like this ought to do the trick:
FLASHROM=/path/to/test/binary sh do_tests.sh chip_size.sh wp-toggle.sh
wp-range.sh partial_writes_x86_bios.sh

(note: partial_writes_x86_bios.sh is named inappropriately... "x86 bios" is the
name given to the ROM attached to the host chipset, which is the tegra2 in this
case).

[Louis, 2011-03-30 10:15:39]

Thanks everyone's reviews. I should have gone through all your comments and
updated the code in patch set 11.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c
File processor_enable.c (right):

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode89
processor_enable.c:89: static int is_tegra2(void)
Done. Good point.
On 2011/03/28 02:08:22, Micah C wrote:
> This function is complex enough to justify a comment explaining the goal. 
> Something like this:
> 
> // Return true if executing on an Nvidia Tegra20 CPU, indicated when
> /proc/cpuinfo contains a line like "CPU part : 0xc09"
> 
> In the future this will probably need to be extended to support a list of CPU
> part identifiers, since future CPUs in the family will likely include a common
> control interface.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode104
processor_enable.c:104: if (strncmp(ptr, "CPU part", sizeof("CPU part") - 1) ==
0)
On 2011/03/28 02:08:22, Micah C wrote:
> The appearance of the constant string "CPU part" 3 times in two lines suggests
> that it might benefit from being a named constant.
> 
> How about declaring:
> const char *field = "CPU part";
> const char *value = "0xc09";
> 
> and then using the named constants for the comparison and the sizeof
operators? 
>  Some future implementation might replace the "value" with another loop over a
> list of values, and using a named variable will make that be a smaller change.

Good idea. Done.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode111
processor_enable.c:111: fclose(cpuinfo);
On 2011/03/28 02:08:22, Micah C wrote:
> This seems like a surprising place to close the file descriptor, but I see
that
> you're doing this to make sure that it's not leaked when returning from within
> this loop.
> 
> How about changing the looping structure so that the loop always ends, and
there
> is just 1 return path from the function?  Maybe use a local variable to
> represent the return value, and then forcing it to 1 or 0 based on the loop
exit
> condition?

Done.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode134
processor_enable.c:134: msg_pinfo("Detected NVIDIA Tegra 2.\n");
On 2011/03/28 02:08:22, Micah C wrote:
> Is this a temporary debugging message, or something that's valuable for the
> final implementation?  Are there similar logging messages for the other
chipsets
> that flashrom supports?

Done.

http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode134
processor_enable.c:134: msg_pinfo("Detected NVIDIA Tegra 2.\n");
On 2011/03/28 05:06:05, dhendrix wrote:
> On 2011/03/28 02:08:22, Micah C wrote:
> > Is this a temporary debugging message, or something that's valuable for the
> > final implementation?  Are there similar logging messages for the other
> chipsets
> > that flashrom supports?
> 
> Nice catch. We should suppress this for normal usage and use "msg_pdbg()"
> instead.
> 
> There is a similar message for other chipsets, but I believe that is #ifdef'd
> out since it's within a bunch of code that is more specific to x86...

Done.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c
File tegra2_spi.c (right):

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode164
tegra2_spi.c:164: programmer_delay(100000);
I don't know a signal for this. Actually, this is only called once, and should
not a big deal.

On 2011/03/28 02:08:22, Micah C wrote:
> This is a big delay.  Is there any other signal that can be used to indicate
the
> transfer has been finished which might sometimes come sooner than 100ms?

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode182
tegra2_spi.c:182: programmer_delay(2000);
This is not necessary because nothing will be send out when UART is disbled.
Removed.
On 2011/03/28 02:08:22, Micah C wrote:
> Is there some status register that can detect UART Tx FIFO empty which is more
> reliable than a CPU delay?

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode190
tegra2_spi.c:190: * Looks like we may also need to dynamically change the
pinmux,
On 2011/03/28 02:08:22, Micah C wrote:
> It might be a good idea to create functions like "save_pinmux_state()" and
> "restore_pinmux_state()" to use from spi_init() and spi_shutdown().  If there
> are many different boards in the world with Tegra2 CPUs, they might have
> different pinmux settings than Seaboard, so hardcoding the constant settings
in
> spi_shutdown may be brittle.

Done. Call register_shutdown() to restore all changed register in the end.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode345
tegra2_spi.c:345: programmer_delay(10);
Good catch. I refined this to 2us (roughly larger than 1.333 us = 1 / 6MHz * 8
bits)
On 2011/03/28 02:08:22, Micah C wrote:
> Is this "10" the same 10us that's multiplied into the SPI_TIMEOUT definition? 
> Perhaps it deserves its own constant if it's something that's specified for
the
> controller interface.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode411
tegra2_spi.c:411: spi_init();
On 2011/03/25 17:22:03, Stefan Reinauer wrote:
> It seems spi_init() could just be rolled into this function. Having
> tegra2_spi_init() and a separate spi_init() does not add to making the code
> easier to understand.

Done.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode420
tegra2_spi.c:420: spi_shutdown();
On 2011/03/25 17:22:03, Stefan Reinauer wrote:
> Same applies for tegra2_spi_shutdown() and spi_shutdown().

Done.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h
File tegra2_spi.h (right):

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h#newcode56
tegra2_spi.h:56: 
On 2011/03/25 17:01:17, Stefan Reinauer wrote:
> The whole "go away code" section is not used anymore and should be dropped.

Done.

http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h#newcode69
tegra2_spi.h:69: } spi_tegra_t;
On 2011/03/25 17:01:17, Stefan Reinauer wrote:
> This struct is no longer used. It should be dropped.

Done.

[Micah C, 2011-03-30 23:26:53]

LGTM

On 2011/03/30 10:15:39, Louis wrote:
> Thanks everyone's reviews. I should have gone through all your comments and
> updated the code in patch set 11.
> 
> http://codereview.chromium.org/6731011/diff/7004/processor_enable.c
> File processor_enable.c (right):
> 
> http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode89
> processor_enable.c:89: static int is_tegra2(void)
> Done. Good point.
> On 2011/03/28 02:08:22, Micah C wrote:
> > This function is complex enough to justify a comment explaining the goal. 
> > Something like this:
> > 
> > // Return true if executing on an Nvidia Tegra20 CPU, indicated when
> > /proc/cpuinfo contains a line like "CPU part : 0xc09"
> > 
> > In the future this will probably need to be extended to support a list of
CPU
> > part identifiers, since future CPUs in the family will likely include a
common
> > control interface.
> 
> http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode104
> processor_enable.c:104: if (strncmp(ptr, "CPU part", sizeof("CPU part") - 1)
==
> 0)
> On 2011/03/28 02:08:22, Micah C wrote:
> > The appearance of the constant string "CPU part" 3 times in two lines
suggests
> > that it might benefit from being a named constant.
> > 
> > How about declaring:
> > const char *field = "CPU part";
> > const char *value = "0xc09";
> > 
> > and then using the named constants for the comparison and the sizeof
> operators? 
> >  Some future implementation might replace the "value" with another loop over
a
> > list of values, and using a named variable will make that be a smaller
change.
> 
> Good idea. Done.
> 
> http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode111
> processor_enable.c:111: fclose(cpuinfo);
> On 2011/03/28 02:08:22, Micah C wrote:
> > This seems like a surprising place to close the file descriptor, but I see
> that
> > you're doing this to make sure that it's not leaked when returning from
within
> > this loop.
> > 
> > How about changing the looping structure so that the loop always ends, and
> there
> > is just 1 return path from the function?  Maybe use a local variable to
> > represent the return value, and then forcing it to 1 or 0 based on the loop
> exit
> > condition?
> 
> Done.
> 
> http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode134
> processor_enable.c:134: msg_pinfo("Detected NVIDIA Tegra 2.\n");
> On 2011/03/28 02:08:22, Micah C wrote:
> > Is this a temporary debugging message, or something that's valuable for the
> > final implementation?  Are there similar logging messages for the other
> chipsets
> > that flashrom supports?
> 
> Done.
> 
> http://codereview.chromium.org/6731011/diff/7004/processor_enable.c#newcode134
> processor_enable.c:134: msg_pinfo("Detected NVIDIA Tegra 2.\n");
> On 2011/03/28 05:06:05, dhendrix wrote:
> > On 2011/03/28 02:08:22, Micah C wrote:
> > > Is this a temporary debugging message, or something that's valuable for
the
> > > final implementation?  Are there similar logging messages for the other
> > chipsets
> > > that flashrom supports?
> > 
> > Nice catch. We should suppress this for normal usage and use "msg_pdbg()"
> > instead.
> > 
> > There is a similar message for other chipsets, but I believe that is
#ifdef'd
> > out since it's within a bunch of code that is more specific to x86...
> 
> Done.
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c
> File tegra2_spi.c (right):
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode164
> tegra2_spi.c:164: programmer_delay(100000);
> I don't know a signal for this. Actually, this is only called once, and should
> not a big deal.
> 
> On 2011/03/28 02:08:22, Micah C wrote:
> > This is a big delay.  Is there any other signal that can be used to indicate
> the
> > transfer has been finished which might sometimes come sooner than 100ms?
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode182
> tegra2_spi.c:182: programmer_delay(2000);
> This is not necessary because nothing will be send out when UART is disbled.
> Removed.
> On 2011/03/28 02:08:22, Micah C wrote:
> > Is there some status register that can detect UART Tx FIFO empty which is
more
> > reliable than a CPU delay?
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode190
> tegra2_spi.c:190: * Looks like we may also need to dynamically change the
> pinmux,
> On 2011/03/28 02:08:22, Micah C wrote:
> > It might be a good idea to create functions like "save_pinmux_state()" and
> > "restore_pinmux_state()" to use from spi_init() and spi_shutdown().  If
there
> > are many different boards in the world with Tegra2 CPUs, they might have
> > different pinmux settings than Seaboard, so hardcoding the constant settings
> in
> > spi_shutdown may be brittle.
> 
> Done. Call register_shutdown() to restore all changed register in the end.
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode345
> tegra2_spi.c:345: programmer_delay(10);
> Good catch. I refined this to 2us (roughly larger than 1.333 us = 1 / 6MHz * 8
> bits)
> On 2011/03/28 02:08:22, Micah C wrote:
> > Is this "10" the same 10us that's multiplied into the SPI_TIMEOUT
definition? 
> > Perhaps it deserves its own constant if it's something that's specified for
> the
> > controller interface.
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode411
> tegra2_spi.c:411: spi_init();
> On 2011/03/25 17:22:03, Stefan Reinauer wrote:
> > It seems spi_init() could just be rolled into this function. Having
> > tegra2_spi_init() and a separate spi_init() does not add to making the code
> > easier to understand.
> 
> Done.
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.c#newcode420
> tegra2_spi.c:420: spi_shutdown();
> On 2011/03/25 17:22:03, Stefan Reinauer wrote:
> > Same applies for tegra2_spi_shutdown() and spi_shutdown().
> 
> Done.
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h
> File tegra2_spi.h (right):
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h#newcode56
> tegra2_spi.h:56: 
> On 2011/03/25 17:01:17, Stefan Reinauer wrote:
> > The whole "go away code" section is not used anymore and should be dropped.
> 
> Done.
> 
> http://codereview.chromium.org/6731011/diff/7004/tegra2_spi.h#newcode69
> tegra2_spi.h:69: } spi_tegra_t;
> On 2011/03/25 17:01:17, Stefan Reinauer wrote:
> > This struct is no longer used. It should be dropped.
> 
> Done.

[Stefan Reinauer, 2011-03-31 00:13:37]

LGTM