Re: [PATCH v7 1/4] mtd: spi-nor: introduce SPI 1-2-2 and SPI 1-4-4 protocols
From: Cyrille Pitchen
Date: Wed Apr 19 2017 - 17:29:37 EST
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@xxxxxxxxx>
>
> [...]
>
>> --- 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.
>