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

[Carl-Daniel Hailfinger]

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.


> +#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 at gmx.net>

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/