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 - 16:59:43 EST
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.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.
>>
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>