[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