On Sun, 29 May 2011 01:50:27 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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. :)