Messages in this thread | | | Subject | Re: [PATCH v7 1/4] mtd: spi-nor: introduce SPI 1-2-2 and SPI 1-4-4 protocols | From | Cyrille Pitchen <> | Date | Wed, 19 Apr 2017 22:12:39 +0200 |
| |
Le 19/04/2017 à 01:02, Marek Vasut a écrit : > On 04/19/2017 12:51 AM, Cyrille Pitchen wrote: >> This patch changes the prototype of spi_nor_scan(): its 3rd parameter >> is replaced by a 'struct spi_nor_hwcaps' pointer, which tells the spi-nor >> framework about the actual hardware capabilities supported by the SPI >> controller and its driver. >> >> Besides, this patch also introduces a new 'struct spi_nor_flash_parameter' >> telling the spi-nor framework about the hardware capabilities supported by >> the SPI flash memory and the associated settings required to use those >> hardware caps. >> >> Then, to improve the readability of spi_nor_scan(), the discovery of the >> memory settings and the memory initialization are now split into two >> dedicated functions. >> >> 1 - spi_nor_init_params() >> >> The spi_nor_init_params() function is responsible for initializing the >> 'struct spi_nor_flash_parameter'. Currently this structure is filled with >> legacy values but further patches will allow to override some parameter >> values dynamically, for instance by reading the JESD216 Serial Flash >> Discoverable Parameter (SFDP) tables from the SPI memory. >> The spi_nor_init_params() function only deals with the hardware >> capabilities of the SPI flash memory: especially it doesn't care about >> the hardware capabilities supported by the SPI controller. >> >> 2 - spi_nor_setup() >> >> The second function is called once the 'struct spi_nor_flash_parameter' >> has been initialized by spi_nor_init_params(). >> With both 'struct spi_nor_flash_parameter' and 'struct spi_nor_hwcaps', >> the new argument of spi_nor_scan(), spi_nor_setup() computes the best >> match between hardware caps supported by both the (Q)SPI memory and >> controller hence selecting the relevant settings for (Fast) Read and Page >> Program operations. >> >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com> > > [...] > >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -119,13 +119,57 @@ >> /* Configuration Register bits. */ >> #define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */ >> >> -enum read_mode { >> - SPI_NOR_NORMAL = 0, >> - SPI_NOR_FAST, >> - SPI_NOR_DUAL, >> - SPI_NOR_QUAD, >> +/* Supported SPI protocols */ >> +#define SNOR_PROTO_INST_MASK GENMASK(23, 16) >> +#define SNOR_PROTO_INST_SHIFT 16 >> +#define SNOR_PROTO_INST(_nbits) \ >> + ((((u32)(_nbits)) << SNOR_PROTO_INST_SHIFT) & SNOR_PROTO_INST_MASK) > > Is the u32 cast needed ? > >> +#define SNOR_PROTO_ADDR_MASK GENMASK(15, 8) >> +#define SNOR_PROTO_ADDR_SHIFT 8 >> +#define SNOR_PROTO_ADDR(_nbits) \ >> + ((((u32)(_nbits)) << SNOR_PROTO_ADDR_SHIFT) & SNOR_PROTO_ADDR_MASK) >> + >> +#define SNOR_PROTO_DATA_MASK GENMASK(7, 0) >> +#define SNOR_PROTO_DATA_SHIFT 0 >> +#define SNOR_PROTO_DATA(_nbits) \ >> + ((((u32)(_nbits)) << SNOR_PROTO_DATA_SHIFT) & SNOR_PROTO_DATA_MASK) > > [...] > >> +static inline u8 spi_nor_get_protocol_inst_nbits(enum spi_nor_protocol proto) >> +{ >> + return ((u32)(proto & SNOR_PROTO_INST_MASK)) >> SNOR_PROTO_INST_SHIFT; > > DTTO, is the cast needed ? >
# Short answer:
not necessary in this very particular case but always a good practice.
# Comprehensive comparison with macro definitions:
This cast is as needed as adding parentheses around the parameters inside the definition of some function-like macro. Most of the time, you can perfectly live without them but in some specific usages unexpected patterns of behavior occur then you struggle debugging to fix the issue:
#define MULT(a, b) (a * b) /* instead of ((a) * (b)) */
int a;
a = MULT(2, 3); /* OK */ a = MULT(2 * 4, 8); /* OK */ a = MULT(2 + 4, 8); /* What result do you expect ? */
So it's always a good habit to cast into some unsigned integer type when working with bitmasks/bitfields as it's always a good habit to add parentheses around macro parameters: it's safer and it avoids falling into some nasty traps!
Please have a look at the definitions of GENMASK() and BIT() macros in include/linux/bitops.h: they are defined using unsigned integers and there are technical reasons behind that.
# Technical explanation:
First thing to care about: the enum
An enum is guaranteed to be represented by an integer, but the actual type (and its signedness) is implementation-dependent.
Second thing to know: the >> operator
The >> operator is perfectly defined when applied on an unsigned integer whereas its output depends on the implementation with a signed integer operand. In practice, in most cases, the >> on signed integer performs an arithmetic right shift and not a logical right shift as most people expect.
signed int v1 = 0x80000000;
(v1 >> 1) == 0xC0000000 (v1 >> 31) == 0xFFFFFFFF
unsigned int v2 = 0x80000000U;
(v2 >> 1) == 0x40000000U (v2 >> 31) == 0x00000001U
Then, if you define some bitmask/bitfield including the 31st bit:
#define FIELD_SHIFT 30 #define FIELD_MASK GENMASK(31, 30) #define FIELD_VAL0 (0x0U << FIELD_SHIFT) #define FIELD_VAL1 (0x1U << FIELD_SHIFT) #define FIELD_VAL2 (0x2U << FIELD_SHIFT) #define FIELD_VAL3 (0x3U << FIELD_SHIFT)
enum spi_nor_protocol { PROTO0 = [...],
PROTO42 = FIELD_VAL2 | [...], };
enum spi_nor_protocol proto = PROTO42 u8 val;
val = (proto & FIELD_MASK) >> FIELD_SHIFT; if (val == 0x2U) { /* * We may not reach this point as expected because val * may be equal to 0xFEU depending on the implementation. */ }
Best regards,
Cyrille
>> +} >> + >> +static inline u8 spi_nor_get_protocol_addr_nbits(enum spi_nor_protocol proto) >> +{ >> + return ((u32)(proto & SNOR_PROTO_ADDR_MASK)) >> SNOR_PROTO_ADDR_SHIFT; >> +} >> + >> +static inline u8 spi_nor_get_protocol_data_nbits(enum spi_nor_protocol proto) >> +{ >> + return ((u32)(proto & SNOR_PROTO_DATA_MASK)) >> SNOR_PROTO_DATA_SHIFT; >> +} >> + >> +static inline u8 spi_nor_get_protocol_width(enum spi_nor_protocol proto) >> +{ >> + return spi_nor_get_protocol_data_nbits(proto); >> +} > > [...] > > Looks good otherwise. >
| |