[flashrom] [PATCH 1/8] ichspi: add ICH/PCH flash descriptor decoding via FDOC/FDOD

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Sat Sep 3 12:33:51 CEST 2011


On Fri, 02 Sep 2011 21:33:33 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 20.08.2011 12:39 schrieb Stefan Tauner:
> >
> > because this patch relies on (compiler) implementation-specific
> > layouting of bit-fields, it checks for basic compiler support in the
> > makefile and for correct layout before taking any action on runtime.
> >   
> 
> Checking for presence of bitfield support in the compiler is the way to
> madness. Next we have to test for C99 capability and other fun stuff.
> Just assume that bitfields are supported, otherwise compilation will
> blow up or the runtime checks will catch a miscompilation. That
> compile-time check (including the #if BITFIELDS) is nacked.

well, for now bit-fields are only needed in one place that is seldom
needed. why break compilation unnecessarily for all other cases too?
basically i agree, but since everyone thinks bit-fields break all the
time i thought it is a good idea to integrate it in the least
interfering way possible... :)

>[…]
> > +
> > +#define RET_OK		0
> > +#define RET_ERR		-1
> > +#define RET_WARN	-2
> > +#define RET_PARAM	-3
> > +#define RET_OOB		-4
> >   
> 
> Too generic names.

not at all, but they should be moved to wherever we want the generic
return macros to be eventually ;)
can we do that and/or introduce the universal values before/with merge
of this patch?
some additional FL_ prefix or so would be ok with me.
i could of course rename them now, and then again, but we should add
the universal return values soon-ish anyway.

> […]
> > diff --git a/ichspi.c b/ichspi.c
> > index 8b4210e..09af2b3 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -1190,6 +1197,7 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  	uint8_t old, new;
> >  	uint16_t spibar_offset, tmp2;
> >  	uint32_t tmp;
> > +	int ichspi_desc = 0;
> >  
> >  	switch (ich_generation) {
> >  	case 7:
> > @@ -1261,6 +1269,8 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
> >  			msg_pinfo("WARNING: SPI Configuration Lockdown activated.\n");
> >  			ichspi_lock = 1;
> >  		}
> > +		if (tmp2 & HSFS_FDV)
> > +			ichspi_desc = 1;
> >   
> 
> How does FDOVR (or whatever the correct acronym for Flash Descriptor
> Override is) relate to this?

Flash Descriptor Override Pin-Strap Status (FDOPSS).
it is later called flash descriptor *security* override pin, and this
hints at its true purpose: overriding the region access rights declared
in the descriptor.

quote ich8 datasheet description of the respective pin (GPIO33):
"If sampled low, the Flash Descriptor Security will be overridden. If
high, the security measures defined in the Flash Descriptor will be in effect."

even then the whole descriptor is of interest imo... and this will most
likely never occur in practice anyway.

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




More information about the flashrom mailing list