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.
+#define SSFS_FDONE_OFF 2 /* Cycle Done Status */ +#define SSFS_FDONE (0x1 << SSFS_FDONE_OFF) +#define SSFS_FCERR_OFF 3 /* Flash Cycle Error */ +#define SSFS_FCERR (0x1 << SSFS_FCERR_OFF) +#define SSFS_AEL_OFF 4 /* Access Error Log */ +#define SSFS_AEL (0x1 << SSFS_AEL_OFF) /* The following bits are reserved in SSFS: 1,5-7. */ #define SSFS_RESERVED_MASK 0x000000e2
#define ICH9_REG_SSFC 0x91 /* 24 Bits */ -#define SSFC_SCGO 0x00000200 -#define SSFC_ACS 0x00000400 -#define SSFC_SPOP 0x00000800 -#define SSFC_COP 0x00001000 -#define SSFC_DBC 0x00010000 -#define SSFC_DS 0x00400000 -#define SSFC_SME 0x00800000 -#define SSFC_SCF 0x01000000 -#define SSFC_SCF_20MHZ 0x00000000 -#define SSFC_SCF_33MHZ 0x01000000 +#define SSFC_SCGO_OFF (1 + 8) /* 1: SPI Cycle Go */ +#define SSFC_SCGO (0x1 << SSFC_SCGO_OFF) +#define SSFC_ACS_OFF (2 + 8) /* 2: Atomic Cycle Sequence */ +#define SSFC_ACS (0x1 << SSFC_ACS_OFF) +#define SSFC_SPOP_OFF (3 + 8) /* 3: Sequence Prefix Opcode Pointer */ +#define SSFC_SPOP (0x1 << SSFC_SPOP_OFF) +#define SSFC_COP_OFF (4 + 8) /* 4-6: Cycle Opcode Pointer */ +#define SSFC_COP (0x7 << SSFC_COP_OFF) +#define SSFC_DBC_OFF (8 + 8) /* 8-13: Data Byte Count */ +#define SSFC_DBC (0x3f << SSFC_DBC_OFF) +#define SSFC_DS_OFF (14 + 8) /* 14: Data Cycle */ +#define SSFC_DS (0x1 << SSFC_DS_OFF) +#define SSFC_SME_OFF (15 + 8) /* 15: SPI SMI# Enable */ +#define SSFC_SME (0x1 << SSFC_SME_OFF) +#define SSFC_SCF_OFF (16 + 8) /* 16-18: SPI Cycle Frequency */ +#define SSFC_SCF (0x7 << SSFC_SCF_OFF) +#define SSFC_SCF_20MHZ 0x00000000 +#define SSFC_SCF_33MHZ 0x01000000 /* We combine SSFS and SSFC to one 32-bit word,
- therefore SSFC bits are off by 8.
- The following bits are reserved in SSFC: 23-19,7,0. */
@@ -709,8 +721,8 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, temp32 = REGREAD32(ICH9_REG_SSFS); /* Keep reserved bits only */ temp32 &= SSFS_RESERVED_MASK | SSFC_RESERVED_MASK;
- /* 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!
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.
temp32 |= datatemp;}
Rest looks good.
Once the _OFF stuff is decided and once we are 100% sure that the integer promotion rules are applied correctly, this is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
Am 29.05.2011 01:50 schrieb Carl-Daniel Hailfinger:
Am 28.05.2011 05:38 schrieb Stefan Tauner:
@@ -709,8 +721,8 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset, temp32 = REGREAD32(ICH9_REG_SSFS); /* Keep reserved bits only */ temp32 &= SSFS_RESERVED_MASK | SSFC_RESERVED_MASK;
- /* 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!
Turns out that this is probably better suited for patch 3/11.
Regards, Carl-Daniel
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. :)
-
Carl-Daniel Hailfinger -
Stefan Tauner