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

[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?