Re: [coreboot] [RFC]flashrom: improve basic design

[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