On Sun, 29 May 2011 01:50:27 +0200 Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> > > --- > > 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 _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
