[flashrom] [PATCH 01/11] improve macros for SSFS and SSFC bits

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Sun May 29 03:22:12 CEST 2011


On Sun, 29 May 2011 01:50:27 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 28.05.2011 05:38 schrieb Stefan Tauner:
> >  - introduce offset macros and use them to (re)define the existing mask macros
> >  - add comments
> >  - rename SSFS_CDS to SSFS_FDONE (abbr. used in datasheet not in SSFS but HSFS)
> >  - use those for refactoring and magic number elemination.
> >  - following patch uses them for pretty printing
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> > ---
> >  ichspi.c |   48 ++++++++++++++++++++++++++++++------------------
> >  1 files changed, 30 insertions(+), 18 deletions(-)
> >
> > diff --git a/ichspi.c b/ichspi.c
> > index 299cbf3..676b93a 100644
> > --- a/ichspi.c
> > +++ b/ichspi.c
> > @@ -47,24 +47,36 @@
> >  #define ICH9_REG_FDATA0		0x10	/* 64 Bytes */
> >  
> >  #define ICH9_REG_SSFS		0x90	/* 08 Bits */
> > -#define SSFS_SCIP		0x00000001
> > -#define SSFS_CDS		0x00000004
> > -#define SSFS_FCERR		0x00000008
> > -#define SSFS_AEL		0x00000010
> > +#define SSFS_SCIP_OFF		0	/* SPI Cycle In Progress */
> > +#define SSFS_SCIP		(0x1 << SSFS_SCIP_OFF)
> >   
> 
> This is probably a philosophical question, but should we use
> #define SSFS_SCIP (0x1 << 0)
> instead and skip definition of all *_OFF #defines? I don't have a good
> answer, so I'm hoping you can tell me which one is better.
stefan reinauer also thought this is a bit extensive (see his comments
on the earlier patch sets).
but offset and bit count/mask(length) are two independent informations
and we need both (the pretty printing macro uses them for example).
i don't see how we could get rid of either of them. if you see a way
i'd be happy to get rid of half of the lines :)

what we could and imho should do: move a lot of ichspi.c to a new
ichpci.h. flashrom is quite untidy imho. it does not always make sense
to pack stuff in different files especially if they are not needed
elsewhere (yet), but i have seen a few instances where it would have
made sense and the big blobs hinder me (while developing ./util/-apps
for example). in the case of the defines here... there is at least a
small motivation to move them to a header: ich_descriptor.c needs a few
(which are copied to the .h atm).

> > -	/* clear error status registers */
> > -	temp32 |= (SSFS_CDS + SSFS_FCERR);
> > +	/* Clear cycle done and cycle error status registers */
> > +	temp32 |= (SSFS_FDONE + SSFS_FCERR);
> >   
> 
> Odd. Did we really have + instead of | here? That looks wrong (yes,
> technically it is correct, but still, it suggests that the values are
> numeric instead of individual bits). Now that you're touching that code,
> could you fix it to use | instead? And please fix an earlier occurence
> of temp16 |= (SPIS_CDS + SPIS_FCERR) as well. Thanks!
heh i have never noticed that TBH. will fix.

> Turns out that this is probably better suited for patch 3/11.
why?

> 
> >  	REGWRITE32(ICH9_REG_SSFS, temp32);
> >  
> >  	/* Use 20 MHz */
> > @@ -720,7 +732,7 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
> >  	if (datalength != 0) {
> >  		uint32_t datatemp;
> >  		temp32 |= SSFC_DS;
> > -		datatemp = ((uint32_t) ((datalength - 1) & 0x3f)) << (8 + 8);
> > +		datatemp = (uint32_t) (((datalength - 1) << SSFC_DBC_OFF) & SSFC_DBC);
> >   
> 
> Are you really 100% sure that integer promotion rules allow that?
> datalength is uint8_t, and shifting it directly by 8 bits or more may
> result in undefined behaviour. Need to check that with a C guru.
no, i am not. according to a quick test datalength "is" already
32b before that line. datalength = datalength << 32; produces a
warning, 31 does not. also a minimum example does show this behavior so
maybe there are no integers <32b on my machine at all... no idea.
gcc version 4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5) on an amd64 machine.
i dont trust that result at all.

i do think the cast at the current place is redundant though and we
could move it inside the braces next to datalength where it might be at
least of some use. :)

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




More information about the flashrom mailing list