Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1618420 > unrolled thread

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

Started byMarek Vasut <marek.vasut@gmail.com>
First post2017-04-07 02:20 +0200
Last post2017-04-09 23:50 +0200
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4  protocols Marek Vasut <marek.vasut@gmail.com> - 2017-04-07 02:20 +0200
    Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4  protocols Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> - 2017-04-09 22:00 +0200
      Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4  protocols Marek Vasut <marek.vasut@gmail.com> - 2017-04-09 23:00 +0200
        Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4  protocols Marek Vasut <marek.vasut@gmail.com> - 2017-04-09 23:50 +0200
        Re: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4  protocols Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> - 2017-04-09 23:50 +0200

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

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-04-07 02:20 +0200
SubjectRe: [PATCH v5 2/6] mtd: m25p80: add support of SPI 1-2-2 and 1-4-4 protocols
Message-ID<ttpN7-84a-11@gated-at.bofh.it>
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@atmel.com>
> ---
>  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 ?

> +	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);
> +}
> +
>  static int m25p80_read_reg(struct spi_nor *nor, u8 code, u8 *val, int len)
>  {
>  	struct m25p *flash = nor->priv;
> @@ -78,11 +91,16 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  {
>  	struct m25p *flash = nor->priv;
>  	struct spi_device *spi = flash->spi;
> -	struct spi_transfer t[2] = {};
> +	unsigned int inst_nbits, addr_nbits, data_nbits, data_idx;
> +	struct spi_transfer t[3] = {};
>  	struct spi_message m;
>  	int cmd_sz = m25p_cmdsz(nor);
>  	ssize_t ret;
>  
> +	/* get transfer protocols. */
> +	m25p80_proto2nbits(nor->write_proto, &inst_nbits,
> +			   &addr_nbits, &data_nbits);
> +
>  	spi_message_init(&m);
>  
>  	if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
> @@ -92,12 +110,27 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  	m25p_addr2cmd(nor, to, flash->command);
>  
>  	t[0].tx_buf = flash->command;
> +	t[0].tx_nbits = inst_nbits;
>  	t[0].len = cmd_sz;
>  	spi_message_add_tail(&t[0], &m);
>  
> -	t[1].tx_buf = buf;
> -	t[1].len = len;
> -	spi_message_add_tail(&t[1], &m);
> +	/* split the op code and address bytes into two transfers if needed. */
> +	data_idx = 1;
> +	if (addr_nbits != inst_nbits) {
> +		t[0].len = 1;
> +
> +		t[1].tx_buf = &flash->command[1];
> +		t[1].tx_nbits = addr_nbits;
> +		t[1].len = cmd_sz - 1;
> +		spi_message_add_tail(&t[1], &m);
> +
> +		data_idx = 2;
> +	}
> +
> +	t[data_idx].tx_buf = buf;
> +	t[data_idx].tx_nbits = data_nbits;
> +	t[data_idx].len = len;
> +	spi_message_add_tail(&t[data_idx], &m);
>  
>  	ret = spi_sync(spi, &m);
>  	if (ret)
> @@ -109,18 +142,6 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>  	return ret;
>  }
>  
> -static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
> -{
> -	switch (nor->read_proto) {
> -	case SNOR_PROTO_1_1_2:
> -		return 2;
> -	case SNOR_PROTO_1_1_4:
> -		return 4;
> -	default:
> -		return 0;
> -	}
> -}
> -
>  /*
>   * Read an address range from the nor chip.  The address range
>   * may be any size provided it is within the physical boundaries.
> @@ -130,13 +151,19 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>  {
>  	struct m25p *flash = nor->priv;
>  	struct spi_device *spi = flash->spi;
> -	struct spi_transfer t[2];
> +	unsigned int inst_nbits, addr_nbits, data_nbits, data_idx;
> +	struct spi_transfer t[3];
>  	struct spi_message m;
>  	unsigned int dummy = nor->read_dummy;
>  	ssize_t ret;
> +	int cmd_sz;
> +
> +	/* get transfer protocols. */
> +	m25p80_proto2nbits(nor->read_proto, &inst_nbits,
> +			   &addr_nbits, &data_nbits);
>  
>  	/* convert the dummy cycles to the number of bytes */
> -	dummy /= 8;
> +	dummy = (dummy * addr_nbits) / 8;
>  
>  	if (spi_flash_read_supported(spi)) {
>  		struct spi_flash_read_message msg;
> @@ -149,10 +176,9 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>  		msg.read_opcode = nor->read_opcode;
>  		msg.addr_width = nor->addr_width;
>  		msg.dummy_bytes = dummy;
> -		/* TODO: Support other combinations */
> -		msg.opcode_nbits = SPI_NBITS_SINGLE;
> -		msg.addr_nbits = SPI_NBITS_SINGLE;
> -		msg.data_nbits = m25p80_rx_nbits(nor);
> +		msg.opcode_nbits = inst_nbits;
> +		msg.addr_nbits = addr_nbits;
> +		msg.data_nbits = data_nbits;
>  
>  		ret = spi_flash_read(spi, &msg);
>  		if (ret < 0)
> @@ -167,20 +193,45 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>  	m25p_addr2cmd(nor, from, flash->command);
>  
>  	t[0].tx_buf = flash->command;
> +	t[0].tx_nbits = inst_nbits;
>  	t[0].len = m25p_cmdsz(nor) + dummy;
>  	spi_message_add_tail(&t[0], &m);
>  
> -	t[1].rx_buf = buf;
> -	t[1].rx_nbits = m25p80_rx_nbits(nor);
> -	t[1].len = min3(len, spi_max_transfer_size(spi),
> -			spi_max_message_size(spi) - t[0].len);
> -	spi_message_add_tail(&t[1], &m);
> +	/*
> +	 * Set all dummy/mode cycle bits to avoid sending some manufacturer
> +	 * specific pattern, which might make the memory enter its Continuous
> +	 * Read mode by mistake.
> +	 * Based on the different mode cycle bit patterns listed and described
> +	 * in the JESD216B speficication, the 0xff value works for all memories
                           ^
                           specification, typo

> +	 * and all manufacturers.
> +	 */
> +	cmd_sz = t[0].len;
> +	memset(flash->command + cmd_sz - dummy, 0xff, dummy);
> +
> +	/* split the op code and address bytes into two transfers if needed. */
> +	data_idx = 1;
> +	if (addr_nbits != inst_nbits) {
> +		t[0].len = 1;
> +
> +		t[1].tx_buf = &flash->command[1];
> +		t[1].tx_nbits = addr_nbits;
> +		t[1].len = cmd_sz - 1;
> +		spi_message_add_tail(&t[1], &m);
> +
> +		data_idx = 2;
> +	}
> +
> +	t[data_idx].rx_buf = buf;
> +	t[data_idx].rx_nbits = data_nbits;
> +	t[data_idx].len = min3(len, spi_max_transfer_size(spi),
> +			       spi_max_message_size(spi) - cmd_sz);
> +	spi_message_add_tail(&t[data_idx], &m);
>  
>  	ret = spi_sync(spi, &m);
>  	if (ret)
>  		return ret;
>  
> -	ret = m.actual_length - m25p_cmdsz(nor) - dummy;
> +	ret = m.actual_length - cmd_sz;
>  	if (ret < 0)
>  		return -EIO;
>  	return ret;
> @@ -223,11 +274,20 @@ static int m25p_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, flash);
>  	flash->spi = spi;
>  
> -	if (spi->mode & SPI_RX_QUAD)
> +	if (spi->mode & SPI_RX_QUAD) {
>  		hwcaps.mask |= SNOR_HWCAPS_READ_1_1_4;
> -	else if (spi->mode & SPI_RX_DUAL)
> +
> +		if (spi->mode & SPI_TX_QUAD)
> +			hwcaps.mask |= (SNOR_HWCAPS_READ_1_4_4 |
> +					SNOR_HWCAPS_PP_1_1_4 |
> +					SNOR_HWCAPS_PP_1_4_4);
> +	} else if (spi->mode & SPI_RX_DUAL) {
>  		hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
>  
> +		if (spi->mode & SPI_TX_DUAL)
> +			hwcaps.mask |= SNOR_HWCAPS_READ_1_2_2;
> +	}
> +
>  	if (data && data->name)
>  		nor->mtd.name = data->name;
>  
> 


-- 
Best regards,
Marek Vasut

[toc] | [next] | [standalone]

#1619542

FromCyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Date2017-04-09 22:00 +0200
Message-ID<tura9-7lQ-13@gated-at.bofh.it>
In reply to#1618420
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@atmel.com>
>> ---
>>  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.

>> +	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);
>> +}
>> +
>>  static int m25p80_read_reg(struct spi_nor *nor, u8 code, u8 *val, int len)
>>  {
>>  	struct m25p *flash = nor->priv;
>> @@ -78,11 +91,16 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>>  {
>>  	struct m25p *flash = nor->priv;
>>  	struct spi_device *spi = flash->spi;
>> -	struct spi_transfer t[2] = {};
>> +	unsigned int inst_nbits, addr_nbits, data_nbits, data_idx;
>> +	struct spi_transfer t[3] = {};
>>  	struct spi_message m;
>>  	int cmd_sz = m25p_cmdsz(nor);
>>  	ssize_t ret;
>>  
>> +	/* get transfer protocols. */
>> +	m25p80_proto2nbits(nor->write_proto, &inst_nbits,
>> +			   &addr_nbits, &data_nbits);
>> +
>>  	spi_message_init(&m);
>>  
>>  	if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
>> @@ -92,12 +110,27 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>>  	m25p_addr2cmd(nor, to, flash->command);
>>  
>>  	t[0].tx_buf = flash->command;
>> +	t[0].tx_nbits = inst_nbits;
>>  	t[0].len = cmd_sz;
>>  	spi_message_add_tail(&t[0], &m);
>>  
>> -	t[1].tx_buf = buf;
>> -	t[1].len = len;
>> -	spi_message_add_tail(&t[1], &m);
>> +	/* split the op code and address bytes into two transfers if needed. */
>> +	data_idx = 1;
>> +	if (addr_nbits != inst_nbits) {
>> +		t[0].len = 1;
>> +
>> +		t[1].tx_buf = &flash->command[1];
>> +		t[1].tx_nbits = addr_nbits;
>> +		t[1].len = cmd_sz - 1;
>> +		spi_message_add_tail(&t[1], &m);
>> +
>> +		data_idx = 2;
>> +	}
>> +
>> +	t[data_idx].tx_buf = buf;
>> +	t[data_idx].tx_nbits = data_nbits;
>> +	t[data_idx].len = len;
>> +	spi_message_add_tail(&t[data_idx], &m);
>>  
>>  	ret = spi_sync(spi, &m);
>>  	if (ret)
>> @@ -109,18 +142,6 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>>  	return ret;
>>  }
>>  
>> -static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
>> -{
>> -	switch (nor->read_proto) {
>> -	case SNOR_PROTO_1_1_2:
>> -		return 2;
>> -	case SNOR_PROTO_1_1_4:
>> -		return 4;
>> -	default:
>> -		return 0;
>> -	}
>> -}
>> -
>>  /*
>>   * Read an address range from the nor chip.  The address range
>>   * may be any size provided it is within the physical boundaries.
>> @@ -130,13 +151,19 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>>  {
>>  	struct m25p *flash = nor->priv;
>>  	struct spi_device *spi = flash->spi;
>> -	struct spi_transfer t[2];
>> +	unsigned int inst_nbits, addr_nbits, data_nbits, data_idx;
>> +	struct spi_transfer t[3];
>>  	struct spi_message m;
>>  	unsigned int dummy = nor->read_dummy;
>>  	ssize_t ret;
>> +	int cmd_sz;
>> +
>> +	/* get transfer protocols. */
>> +	m25p80_proto2nbits(nor->read_proto, &inst_nbits,
>> +			   &addr_nbits, &data_nbits);
>>  
>>  	/* convert the dummy cycles to the number of bytes */
>> -	dummy /= 8;
>> +	dummy = (dummy * addr_nbits) / 8;
>>  
>>  	if (spi_flash_read_supported(spi)) {
>>  		struct spi_flash_read_message msg;
>> @@ -149,10 +176,9 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>>  		msg.read_opcode = nor->read_opcode;
>>  		msg.addr_width = nor->addr_width;
>>  		msg.dummy_bytes = dummy;
>> -		/* TODO: Support other combinations */
>> -		msg.opcode_nbits = SPI_NBITS_SINGLE;
>> -		msg.addr_nbits = SPI_NBITS_SINGLE;
>> -		msg.data_nbits = m25p80_rx_nbits(nor);
>> +		msg.opcode_nbits = inst_nbits;
>> +		msg.addr_nbits = addr_nbits;
>> +		msg.data_nbits = data_nbits;
>>  
>>  		ret = spi_flash_read(spi, &msg);
>>  		if (ret < 0)
>> @@ -167,20 +193,45 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>>  	m25p_addr2cmd(nor, from, flash->command);
>>  
>>  	t[0].tx_buf = flash->command;
>> +	t[0].tx_nbits = inst_nbits;
>>  	t[0].len = m25p_cmdsz(nor) + dummy;
>>  	spi_message_add_tail(&t[0], &m);
>>  
>> -	t[1].rx_buf = buf;
>> -	t[1].rx_nbits = m25p80_rx_nbits(nor);
>> -	t[1].len = min3(len, spi_max_transfer_size(spi),
>> -			spi_max_message_size(spi) - t[0].len);
>> -	spi_message_add_tail(&t[1], &m);
>> +	/*
>> +	 * Set all dummy/mode cycle bits to avoid sending some manufacturer
>> +	 * specific pattern, which might make the memory enter its Continuous
>> +	 * Read mode by mistake.
>> +	 * Based on the different mode cycle bit patterns listed and described
>> +	 * in the JESD216B speficication, the 0xff value works for all memories
>                            ^
>                            specification, typo
> 

good catch :)

Best regards,

Cyrille

>> +	 * and all manufacturers.
>> +	 */
>> +	cmd_sz = t[0].len;
>> +	memset(flash->command + cmd_sz - dummy, 0xff, dummy);
>> +
>> +	/* split the op code and address bytes into two transfers if needed. */
>> +	data_idx = 1;
>> +	if (addr_nbits != inst_nbits) {
>> +		t[0].len = 1;
>> +
>> +		t[1].tx_buf = &flash->command[1];
>> +		t[1].tx_nbits = addr_nbits;
>> +		t[1].len = cmd_sz - 1;
>> +		spi_message_add_tail(&t[1], &m);
>> +
>> +		data_idx = 2;
>> +	}
>> +
>> +	t[data_idx].rx_buf = buf;
>> +	t[data_idx].rx_nbits = data_nbits;
>> +	t[data_idx].len = min3(len, spi_max_transfer_size(spi),
>> +			       spi_max_message_size(spi) - cmd_sz);
>> +	spi_message_add_tail(&t[data_idx], &m);
>>  
>>  	ret = spi_sync(spi, &m);
>>  	if (ret)
>>  		return ret;
>>  
>> -	ret = m.actual_length - m25p_cmdsz(nor) - dummy;
>> +	ret = m.actual_length - cmd_sz;
>>  	if (ret < 0)
>>  		return -EIO;
>>  	return ret;
>> @@ -223,11 +274,20 @@ static int m25p_probe(struct spi_device *spi)
>>  	spi_set_drvdata(spi, flash);
>>  	flash->spi = spi;
>>  
>> -	if (spi->mode & SPI_RX_QUAD)
>> +	if (spi->mode & SPI_RX_QUAD) {
>>  		hwcaps.mask |= SNOR_HWCAPS_READ_1_1_4;
>> -	else if (spi->mode & SPI_RX_DUAL)
>> +
>> +		if (spi->mode & SPI_TX_QUAD)
>> +			hwcaps.mask |= (SNOR_HWCAPS_READ_1_4_4 |
>> +					SNOR_HWCAPS_PP_1_1_4 |
>> +					SNOR_HWCAPS_PP_1_4_4);
>> +	} else if (spi->mode & SPI_RX_DUAL) {
>>  		hwcaps.mask |= SNOR_HWCAPS_READ_1_1_2;
>>  
>> +		if (spi->mode & SPI_TX_DUAL)
>> +			hwcaps.mask |= SNOR_HWCAPS_READ_1_2_2;
>> +	}
>> +
>>  	if (data && data->name)
>>  		nor->mtd.name = data->name;
>>  
>>
> 
>

[toc] | [prev] | [next] | [standalone]

#1619548

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-04-09 23:00 +0200
Message-ID<tus6d-7VF-9@gated-at.bofh.it>
In reply to#1619542
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@atmel.com>
>>> ---
>>>  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)) */

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

[toc] | [prev] | [next] | [standalone]

#1619557

FromMarek Vasut <marek.vasut@gmail.com>
Date2017-04-09 23:50 +0200
Message-ID<tusSC-8s2-7@gated-at.bofh.it>
In reply to#1619548
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@atmel.com>
>>>>> ---
>>>>>  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

[toc] | [prev] | [next] | [standalone]

#1619558

FromCyrille Pitchen <cyrille.pitchen@wedev4u.fr>
Date2017-04-09 23:50 +0200
Message-ID<tusSC-8s2-9@gated-at.bofh.it>
In reply to#1619548
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@atmel.com>
>>>> ---
>>>>  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.

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, ...


> 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);
>>>> +}
>>>> +
> [...]
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web