[flashrom] [PATCH 2/4] add ICH/PCH flash descriptor decoding via FDOC/FDOD

[Stefan Tauner]

On Mon, 08 Aug 2011 03:41:28 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > +	union {			/* 0x00 */
> > +		uint32_t FLCOMP;
> > +		struct {
> > +			uint32_t comp1_density	:3;
> > +			uint32_t comp2_density	:3;
> > +			uint32_t		:11;
> > +			uint32_t freq_read	:3;
> > +			uint32_t fastread	:1;
> > +			uint32_t freq_fastread	:3;
> > +			uint32_t freq_write	:3;
> > +			uint32_t freq_read_id	:3;
> > +			uint32_t		:2;
> > +		};
> > +	};
> > +	union {			/* 0x04 */
> > +		uint32_t FLILL;
> > +		struct {
> > +			uint32_t invalid_instr0;
> >     
> 
> Really uint32_t? If FLILL is 32 bits, but all invalid_instr are 32 bits
> as well, the union members have inconsistent sizes.
> uint8_t invalid_instr0 maybe?
(quoted for reference only)
> 
> > +			uint32_t invalid_instr1;
> > +			uint32_t invalid_instr2;
> > +			uint32_t invalid_instr3;
> >     
> 
> Dito for the other invalid_instr above.
(quoted for reference only)
> 
> > +		};
> > +	};
> > +	union {			/* 0x08 */
> > +		uint32_t FLPB;
> > +		struct {
> > +			uint32_t FPBA	:13;
> > +			uint32_t	:19;
> > +		};
> > +	};
> >     
> 
> All code accessing FLPB/FPBA was broken due to the wrong FLILL union
> size, so if it worked anyway, you have another bug hidden somewhere.

actually no. in this patch accesses to FLPB and FPBA are fine because
they all dependent on the (common) starting point of the elements
*inside* the union only.

i set FPBA by
	desc->component.FLPB	= read_descriptor_reg(1, 2, spibar);
and then read out via the bit-fields.
it would be different if i would just memset the whole top-level struct.

in patch 3/4 i add a method that fills said structs using a uint32_t
array as input, but i do so by accessing the inner members directly
like above:
	desc->component.FLPB	= dump[(getFCBA(&desc->content) >> 2) + 2];

on my test machine the invalid instructions are all 0. the whole struct
is initialized to 0, so this was not detected in my tests.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner