[RFC]flashrom: improve basic design

[FENG Yu Ning]

There is a defect in basic design of flashrom. It leads to problems
some developers are facing or going to face. A partial solution is
proposed.

1. The Defect

struct flashchip {

/* ... */

int (*probe) (struct flashchip *flash);
    int (*erase) (struct flashchip *flash);
    int (*write) (struct flashchip *flash, uint8_t *buf);
    int (*read) (struct flashchip *flash, uint8_t *buf);

/* ... */

};

The struct specifies drivers for operations. It is a good design if
the drivers deal with flash chips directly and provide interfaces for
upper layer program to use. However, those drivers deal with every
component in the communication chain. They do not fit into a structure
storing information closely related to flash chips.

2. Problems

Supporting non-standard flash chips needs to write a new driver. It
requires the developer to be familiar with the internals of flashrom.

Probe functions and IDs are seperated. They are associated by the
flashchip structure. In nature, probe functions and IDs are questions
and answers, and should be associated more closely. Seperated ID
fields in struct flashchip are not very meaningful.

flashrom will probably have plugins in the future. Though chips are
operated in the same way from the view of their host devices, there is
not much useful information for plugins in struct flashchip.

3. Solution

We need an abstract specification of operations in struct flashchip,
not actual code. If there have to be some information not related to
flash chips, the less there is, the better.

Abstract operation specification for SPI flash chips:

/* BEGINNING OF SPECIFICATION */

struct cycles {
        u8 type;	/* CYCLETYPE_PREOPCODE, CYCLETYPE_OPCODE, */
       		/* CYCLETYPE_ADDRESS, CYCLETYPE_DUMMY,	  */
    		/* CYCLETYPE_DATA_IN, CYCLETYPE_DATA_OUT  */
        u8 length;	/* in bytes */
    u8 value_type;	/* VALUETYPE_VARIABLE, VALUETYPE_FIXED	  */
    u32 value;
};

struct cycles byte_read[] =
        {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x3},
       	 {CYCLETYPE_ADDRESS, 3, VALUETYPE_VARIABLE},
     {CYCLETYPE_DATA_IN, 1, VALUETYPE_VARIABLE}
    };

struct cycles byte_program[] =
        {{CYCLETYPE_PREOPCODE, 1, VALUETYPE_FIXED, 0x6},
     {CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x2},
       	 {CYCLETYPE_ADDRESS, 3, VALUETYPE_VARIABLE},
     {CYCLETYPE_DATA_OUT, 1, VALUETYPE_VARIABLE}
    };

struct cycles SST25VF040B_probe_rdid[] =
        {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x90},
     {CYCLETYPE_ADDRESS, 3, VALUETYPE_FIXED, 0},
     {CYCLETYPE_DATA_IN, 2, VALUETYPE_FIXED, 0xbf8d}
    };

struct cycles SST25VF040B_probe_jedec[] =
        {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x9f},
     {CYCLETYPE_DATA_IN, 3, VALUETYPE_FIXED, 0xbf258d}
    };

/* END OF SPECIFICATION */

Specifications for LPC and FWH flash chips will be different, but the
point is abstraction.

Comments are appreciated.

New year's greetings,
yu ning

[Joseph Smith]

On Thu, 1 Jan 2009 18:21:23 +0800, "FENG Yu Ning"
 wrote:
> There is a defect in basic design of flashrom. It leads to problems
> some developers are facing or going to face. A partial solution is
> proposed.
> The Defect
>
> struct flashchip {
>     /* ... */
>
> int (*probe) (struct flashchip *flash);
>    int (*erase) (struct flashchip *flash);
>    int (*write) (struct flashchip *flash, uint8_t *buf);
>    int (*read) (struct flashchip *flash, uint8_t *buf);
>     /* ... */
>
> };
>
> The struct specifies drivers for operations. It is a good design if
> the drivers deal with flash chips directly and provide interfaces for
> upper layer program to use. However, those drivers deal with every
> component in the communication chain. They do not fit into a structure
> storing information closely related to flash chips.
> Problems
>
> Supporting non-standard flash chips needs to write a new driver. It
> requires the developer to be familiar with the internals of flashrom.
>
> Probe functions and IDs are seperated. They are associated by the
> flashchip structure. In nature, probe functions and IDs are questions
> and answers, and should be associated more closely. Seperated ID
> fields in struct flashchip are not very meaningful.
>
> flashrom will probably have plugins in the future. Though chips are
> operated in the same way from the view of their host devices, there is
> not much useful information for plugins in struct flashchip.
> Solution
>
> We need an abstract specification of operations in struct flashchip,
> not actual code. If there have to be some information not related to
> flash chips, the less there is, the better.
>
> Abstract operation specification for SPI flash chips:
>
> /* BEGINNING OF SPECIFICATION */
>
> struct cycles {
>         u8 type;	/* CYCLETYPE_PREOPCODE, CYCLETYPE_OPCODE, */
>       		/* CYCLETYPE_ADDRESS, CYCLETYPE_DUMMY,	  */
>    		/* CYCLETYPE_DATA_IN, CYCLETYPE_DATA_OUT  */
>         u8 length;	/* in bytes */
>    u8 value_type;	/* VALUETYPE_VARIABLE, VALUETYPE_FIXED	  */
>    u32 value;
> };
>
> struct cycles byte_read[] =
>         {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x3},
>        	 {CYCLETYPE_ADDRESS, 3, VALUETYPE_VARIABLE},
>     {CYCLETYPE_DATA_IN, 1, VALUETYPE_VARIABLE}
>    };
>
> struct cycles byte_program[] =
>         {{CYCLETYPE_PREOPCODE, 1, VALUETYPE_FIXED, 0x6},
>     {CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x2},
>        	 {CYCLETYPE_ADDRESS, 3, VALUETYPE_VARIABLE},
>     {CYCLETYPE_DATA_OUT, 1, VALUETYPE_VARIABLE}
>    };
>
> struct cycles SST25VF040B_probe_rdid[] =
>         {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x90},
>     {CYCLETYPE_ADDRESS, 3, VALUETYPE_FIXED, 0},
>     {CYCLETYPE_DATA_IN, 2, VALUETYPE_FIXED, 0xbf8d}
>    };
>
> struct cycles SST25VF040B_probe_jedec[] =
>         {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x9f},
>     {CYCLETYPE_DATA_IN, 3, VALUETYPE_FIXED, 0xbf258d}
>    };
>
> /* END OF SPECIFICATION */
>
> Specifications for LPC and FWH flash chips will be different, but the
> point is abstraction.
>
> Comments are appreciated.

I agree here, especially if we are going to support external programmers
(plug-ins). This would be a big flashrom change though, and a considerable
amount of work. Have you put together a patch for review yet?

[FENG Yu Ning]

On Fri, Jan 2, 2009 at 12:20 PM, Joseph Smith  wrote:
> This would be a big flashrom change though, and a considerable
> amount of work.

I realize that. I think of it as a branch.
> Have you put together a patch for review yet?

Not coding yet. I am not sure how good/bad the idea is. I'd like to
gather some feedback before doing more work.

yu ning

[Carl-Daniel Hailfinger]

Hi Yu Ning,

thanks for yor design proposal.

I really like some stuff in there, especially the suggestion to couple
probe functions and associated IDs. I have a patch for this somewhere, I
need to dig it up and resend.

Unfortunately, other parts of the proposed abstraction level will make
it harder to understand chip drivers, especially for people who just
want to contribute support for one of their own chips.

There are a few ways out, but none of them makes me completely happy.
1. Put all flash chip data in a text/xml/whatever file and feed flash
chip drivers with that data. This has the advantage of separating
flashrom code from actual chip definitions and makes it possible to add
support for new chips without recompiling just by exchanging a text
file. While XML may sound cool, it is definitely a big chunk of code and
we'll lose if we ever try to put flashrom in the ROM. This is equivalent
to a complete rewrite of flashrom from scratch.
2. Convert chip drivers to use accessor functions for reads and writes.
The SPI code already does that and needs no changes to support external
programmers.
3. Some new way to solve all this.

On 01.01.2009 11:21, FENG Yu Ning wrote:
> There is a defect in basic design of flashrom. It leads to problems
> some developers are facing or going to face. A partial solution is
> proposed.
> The Defect
>
> struct flashchip {
>     /* ... */
>
> int (*probe) (struct flashchip *flash);
>    int (*erase) (struct flashchip *flash);
>    int (*write) (struct flashchip *flash, uint8_t *buf);
>    int (*read) (struct flashchip *flash, uint8_t *buf);
>     /* ... */
>
> };
>
> The struct specifies drivers for operations. It is a good design if
> the drivers deal with flash chips directly and provide interfaces for
> upper layer program to use. However, those drivers deal with every
> component in the communication chain. They do not fit into a structure
> storing information closely related to flash chips.
> Problems
>
> Supporting non-standard flash chips needs to write a new driver. It
> requires the developer to be familiar with the internals of flashrom.
>
> Probe functions and IDs are seperated. They are associated by the
> flashchip structure. In nature, probe functions and IDs are questions
> and answers, and should be associated more closely. Seperated ID
> fields in struct flashchip are not very meaningful.
>
> flashrom will probably have plugins in the future. Though chips are
> operated in the same way from the view of their host devices, there is
> not much useful information for plugins in struct flashchip.
> Solution
>
> We need an abstract specification of operations in struct flashchip,
> not actual code. If there have to be some information not related to
> flash chips, the less there is, the better.
>
> Abstract operation specification for SPI flash chips:
>
> /* BEGINNING OF SPECIFICATION */
>
> struct cycles {
>         u8 type;	/* CYCLETYPE_PREOPCODE, CYCLETYPE_OPCODE, */
>       		/* CYCLETYPE_ADDRESS, CYCLETYPE_DUMMY,	  */
>    		/* CYCLETYPE_DATA_IN, CYCLETYPE_DATA_OUT  */
>         u8 length;	/* in bytes */
>    u8 value_type;	/* VALUETYPE_VARIABLE, VALUETYPE_FIXED	  */
>    u32 value;
> };
>
> struct cycles byte_read[] =
>         {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x3},
>        	 {CYCLETYPE_ADDRESS, 3, VALUETYPE_VARIABLE},
>     {CYCLETYPE_DATA_IN, 1, VALUETYPE_VARIABLE}
>    };
>
> struct cycles byte_program[] =
>         {{CYCLETYPE_PREOPCODE, 1, VALUETYPE_FIXED, 0x6},
>     {CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x2},
>        	 {CYCLETYPE_ADDRESS, 3, VALUETYPE_VARIABLE},
>     {CYCLETYPE_DATA_OUT, 1, VALUETYPE_VARIABLE}
>    };
>
> struct cycles SST25VF040B_probe_rdid[] =
>         {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x90},
>     {CYCLETYPE_ADDRESS, 3, VALUETYPE_FIXED, 0},
>     {CYCLETYPE_DATA_IN, 2, VALUETYPE_FIXED, 0xbf8d}
>    };
>
> struct cycles SST25VF040B_probe_jedec[] =
>         {{CYCLETYPE_OPCODE, 1, VALUETYPE_FIXED, 0x9f},
>     {CYCLETYPE_DATA_IN, 3, VALUETYPE_FIXED, 0xbf258d}
>    };
>
> /* END OF SPECIFICATION */

I believe that it is fundamentally wrong to differentiate between
preopcodes and opcodes. That's just a way to confuse people, invented by
chip and controller vendors in a misdirected attempt to make flash
access easier or safer. In reality, it hurts us a lot (no usability
benefits) and the security benefits are debatable as well, depending on
which host you look at.
On the wire, there is no difference between preopcodes and opcodes. They
are just bytes in a command stream. Sure, the command stream has chunks,
but the chip does not care whether you treat an opcode as preopcode or
normal opcode. It just expects certain bytes after each other in the
command stream. A write operation should send a command stream chunk to
the controller driver without having to care about preopcode/opcode
separation.

I'll probably have to send a patch to kill all this preopcode/opcode stuff.

Regards,
Carl-Daniel

[FENG Yu Ning]

On Jan 5, 2009, Carl-Daniel Hailfinger wrote:
> thanks for yor design proposal.

Thanks for reading and commenting.

Having thought more, I realized the merit of the original design, and
I also realized what drives me eager to change it.

In the original design, every flash chip model could be specified its
own operation drivers. It is a good design providing mechanism.

However, too much code reuse keeps us from obtaining benefit from the
design. Most SPI flash chips have the same wrapper functions
probe_spi_rdid, spi_chip_write and spi_chip_read as their operation
drivers. Those drivers have to satisfy many flash chip. It is (too)
difficult.

We should limit how much we reuse operation drivers. My opinion is,
for read, erase and write, only the flash chip models in the same
series share drivers, and every model has its own probe driver. Thus,
we will never need IDs or page_size in struct flashchip, and will not
be bothered by related problems, such as grouping probe functions and
IDs, paged write, etc..

There would be code redundancy. We have to choose one to suffer from.
> Unfortunately, other parts of the proposed abstraction level will make
> it harder to understand chip drivers, especially for people who just
> want to contribute support for one of their own chips.

Yes.

Deeper thoughts make me realize that I am defining a
language/code (the specification), and trying to implement a built-in
compiler/virtual_machine (the chipset driver has to be intelligent).
It would be too complex.
> I believe that it is fundamentally wrong to differentiate between
> preopcodes and opcodes.

Agreed.
> I'll probably have to send a patch to kill all this preopcode/opcode stuff.

hmm.. for ICH7 like chipsets, we just cannot send a WREN like other
commands if SPI opmenu is locked.

BTW, do you have some comment on this issue?
http://www.coreboot.org/pipermail/coreboot/2008-December/043704.html

As you have said, it hurts.

yu ning