Re: [flashrom] [PATCH 01/12] improve macros for SSFS and SSFC bits

[Carl-Daniel Hailfinger]

Am 08.06.2011 04:55 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 <[e-mail]>
> ---
>  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)
> +#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)

This is a code change. Can you mention it in the changelog, please? Some
generic statement like "fix masks for register definitions SSFC_COP,
..." would suffice and help others to understand what you were doing.
Please recheck that the changes were intentional, though.
> +#define SSFC_DBC_OFF		(8 + 8)		/* 8-13: Data Byte Count */
> +#define SSFC_DBC		(0x3f << SSFC_DBC_OFF)

Code change, see above.
> +#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)

Code change, see above.
> +#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. */
>
> @@ -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);

Please change this to

datatemp = ((((uint32_t)datalength - 1) << SSFC_DBC_OFF) & SSFC_DBC);
> temp32 |= datatemp;
>   }

I'm still not entirely happy with "duplicating" all those definitions,
once with _OFF and once without. That said, I see where you're going
with that, so this patch is (with the casting change)
Acked-by: Carl-Daniel Hailfinger <[e-mail]>

No need to resend, just change the commit message and the cast before
you commit.

Regards,
Carl-Daniel

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