[coreboot] [RFC]flashrom: improve basic design

[Joseph Smith]

On Thu, 1 Jan 2009 18:21:23 +0800, "FENG Yu Ning"
<fengyuning1984 at gmail.com> 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