On Thu, 1 Jan 2009 18:21:23 +0800, "FENG Yu Ning"
<[email protected]> 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.
>
> 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.
>
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?
--
Thanks,
Joseph Smith
Set-Top-Linux
www.settoplinux.org
--
coreboot mailing list: [email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
