Hi Marek, can we close the review on patch 1 and 3, please? I would like to merge into the spi-nor tree then prepare the PR to brian for v4.12.
Best regards, Cyrille Le 19/04/2017 à 22:12, Cyrille Pitchen a écrit : > 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.pitc...@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. >> > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >