Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4 protocols

[Marek Vasut]

On 04/09/2017 11:30 PM, Cyrille Pitchen wrote:
> Le 09/04/2017 à 22:46, Marek Vasut a écrit :
>> On 04/09/2017 09:37 PM, Cyrille Pitchen wrote:
>>> Hi Marek,
>>>
>>> Le 07/04/2017 à 01:37, Marek Vasut a écrit :
>>>> On 03/23/2017 12:33 AM, Cyrille Pitchen wrote:
>>>>> Before this patch, m25p80_read() supported few SPI protocols:
>>>>> - regular SPI 1-1-1
>>>>> - SPI Dual Output 1-1-2
>>>>> - SPI Quad Output 1-1-4
>>>>> On the other hand, m25p80_write() only supported SPI 1-1-1.
>>>>>
>>>>> This patch updates both m25p80_read() and m25p80_write() functions to let
>>>>> them support SPI 1-2-2 and SPI 1-4-4 protocols for Fast Read and Page
>>>>> Program SPI commands.
>>>>>
>>>>> It adopts a conservative approach to avoid regressions. Hence the new
>>>>                                              ^ FYI, regression != bug
>>>>
>>>>> implementations try to be as close as possible to the old implementations,
>>>>> so the main differences are:
>>>>> - the tx_nbits values now being set properly for the spi_transfer
>>>>>   structures carrying the (op code + address/dummy) bytes
>>>>> - and the spi_transfer structure being split into 2 spi_transfer
>>>>>   structures when the numbers of I/O lines are different for op code and
>>>>>   for address/dummy byte transfers on the SPI bus.
>>>>>
>>>>> Besides, the current spi-nor framework supports neither the SPI 2-2-2 nor
>>>>> the SPI 4-4-4 protocols. So, for now, we don't need to update the
>>>>> m25p80_{read|write}_reg() functions as SPI 1-1-1 is the only one possible
>>>>> protocol.
>>>>>
>>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@xxxxxxxxx>
>>>>> ---
>>>>>  drivers/mtd/devices/m25p80.c | 120 ++++++++++++++++++++++++++++++++-----------
>>>>>  1 file changed, 90 insertions(+), 30 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>>>> index 68986a26c8fe..64d562efc25d 100644
>>>>> --- a/drivers/mtd/devices/m25p80.c
>>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>>> @@ -34,6 +34,19 @@ struct m25p {
>>>>>  	u8			command[MAX_CMD_SIZE];
>>>>>  };
>>>>>  
>>>>> +static inline void m25p80_proto2nbits(enum spi_nor_protocol proto,
>>>>> +				      unsigned int *inst_nbits,
>>>>> +				      unsigned int *addr_nbits,
>>>>> +				      unsigned int *data_nbits)
>>>>> +{
>>>>
>>>> Why don't we just have some generic macros to extract the number of bits
>>>> from $proto ?
>>>>
>>>
>>> from Documentation/process/coding-style.rst:
>>> "Generally, inline functions are preferable to macros resembling functions."
>>>
>>> inline functions provide better type checking of their arguments and/or
>>> returned value than macros.
>>>
>>> Type checking is also the reason I have chosen to create the 'enum
>>> spi_nor_protocol' rather than using constant macros.
>>
>> That part I get (no, not really [1], inline is compiler _hint_ and for
>> static function, the compiler is smart enough to figure out it should
>> inline it, so drop it. Also cf. __always_inline).
>>
>> What I don't quite get is why don't we just encode the proto as ie.
>>
>> #define PROTO_1_1_4 0x00010204 /* (== BIT(16) | BIT(8) | BIT(2)) */
>>
> 
> This is what I did in former versions of the patch: the scheme you
> propose requires more bits to encode the number of I/O lines for
> instruction, address and data: there would be less bits available for
> future extensions.

Are we ever gonna reach 128bit SPI ? I don't think so. Yes, it requires
more bits, but it also makes it easier to extract information from it
without some elaborate loops.

> Also using the notion of protocol class (1-1-N, 1-N-N, N-N-N) in the
> encoding scheme prevents from setting impossible combinations like
> 4-1-4, 1-2-4, ...

I'd suspect that the review process would catch this.

>> in which case this whole function would turn into constant-time
>>
>> return (proto >> (n * 8)) & 0xff;
>>
>> where n is 0 for data, 1 for address , 2 for command .
>>
>> [1] https://lwn.net/Articles/166172/
>>
>>>>> +	if (inst_nbits)
>>>>> +		*inst_nbits = spi_nor_get_protocol_inst_width(proto);
>>>>> +	if (addr_nbits)
>>>>> +		*addr_nbits = spi_nor_get_protocol_addr_width(proto);
>>>>> +	if (data_nbits)
>>>>> +		*data_nbits = spi_nor_get_protocol_data_width(proto);
>>>>> +}
>>>>> +
>> [...]
>>
> 


-- 
Best regards,
Marek Vasut