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

Groups > linux.kernel > #1385772 > unrolled thread

Re: [PATCH RFC 4/8] mtd: spi-nor: fix support of Dual (x-y-2) and Quad (x-y-4) SPI protocols

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 RFC 4/8] mtd: spi-nor: fix support of Dual (x-y-2) and  Quad (x-y-4) SPI protocols "R, Vignesh" <vigneshr@ti.com> - 2016-04-24 07:10 +0200
    Re: [PATCH RFC 4/8] mtd: spi-nor: fix support of Dual (x-y-2) and  Quad (x-y-4) SPI protocols Cyrille Pitchen <cyrille.pitchen@atmel.com> - 2016-04-25 11:40 +0200

#1385772 — Re: [PATCH RFC 4/8] mtd: spi-nor: fix support of Dual (x-y-2) and Quad (x-y-4) SPI protocols

Hi Cyrille,

On 4/13/2016 10:53 PM, Cyrille Pitchen wrote:
[...]

> +
> +static int spi_nor_setup(struct spi_nor *nor, const struct flash_info *info,
> +			 const struct spi_nor_basic_flash_parameter *params,
> +			 const struct spi_nor_modes *modes)
> +{
> +	bool enable_quad_io, enable_4_4_4, enable_2_2_2;
> +	u32 rd_modes, wr_modes, cmd_modes, mask;
> +	const struct spi_nor_erase_type *erase_type;
> +	const struct spi_nor_read *read;
> +	int rd_midx, wr_midx, err = 0;
> +
> +	/* 2-2-2 or 4-4-4 modes must be supported by BOTH read and write */
> +	mask = (SNOR_MODE_2_2_2 | SNOR_MODE_4_4_4);
> +	cmd_modes = (modes->rd_modes & modes->wr_modes) & mask;
> +	rd_modes = (modes->rd_modes & ~mask) | cmd_modes;
> +	wr_modes = (modes->wr_modes & ~mask) | cmd_modes;
> +
> +	/* Setup read operation. */
> +	rd_midx = fls(params->rd_modes & rd_modes) - 1;
> +	if (spi_nor_midx2proto(rd_midx, &nor->read_proto)) {
> +		dev_err(nor->dev, "invalid (fast) read\n");
> +		return -EINVAL;
> +	}
> +	read = &params->reads[rd_midx];
> +	nor->read_opcode = read->opcode;
> +	nor->read_dummy = read->num_mode_clocks + read->num_wait_states;
> +
> +	/* Set page program op code and protocol. */
> +	wr_midx = fls(params->wr_modes & wr_modes) - 1;
> +	if (spi_nor_midx2proto(wr_midx, &nor->write_proto)) {
> +		dev_err(nor->dev, "invalid page program\n");
> +		return -EINVAL;
> +	}
> +	nor->program_opcode = params->page_programs[wr_midx];
> +
> +	/* Set sector erase op code and size. */
> +	erase_type = &params->erase_types[0];
> +#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
> +	for (i = 1; i < SNOR_MAX_ERASE_TYPES; ++i)

^^^ 'i' is undefined here.

> +		if (params->erase_types[i].size == 0x0c)
> +			erase_type = &params->erase_types[i];
> +#endif
> +	nor->erase_opcode = erase_type->opcode;
> +	nor->mtd.erasesize = (1 << erase_type->size);
> +
> +
> +	enable_quad_io = (SNOR_PROTO_DATA_FROM_PROTO(nor->read_proto) == 4 ||
> +			  SNOR_PROTO_DATA_FROM_PROTO(nor->write_proto) == 4);
> +	enable_4_4_4 = (nor->read_proto == SNOR_PROTO_4_4_4);
> +	enable_2_2_2 = (nor->read_proto == SNOR_PROTO_2_2_2);
> +
> +	/* Enable Quad I/O if needed. */
> +	if ((enable_quad_io || enable_4_4_4) &&
> +	    params->enable_quad_io &&
> +	    nor->reg_proto != SNOR_PROTO_4_4_4) {
> +		err = params->enable_quad_io(nor, true);
> +		if (err) {
> +			dev_err(nor->dev,
> +				"failed to enable the Quad I/O mode\n");
> +			return err;
> +		}
> +	}
> +
> +	/* Enter/Leave 2-2-2 or 4-4-4 if needed. */
> +	if (enable_2_2_2 && params->enable_2_2_2 &&
> +	    nor->reg_proto != SNOR_PROTO_2_2_2)
> +		err = params->enable_2_2_2(nor, true);
> +	else if (enable_4_4_4 && params->enable_4_4_4 &&
> +		 nor->reg_proto != SNOR_PROTO_4_4_4)
> +		err = params->enable_4_4_4(nor, true);
> +	else if (!enable_2_2_2 && params->enable_2_2_2 &&
> +		 nor->reg_proto == SNOR_PROTO_2_2_2)
> +		err = params->enable_2_2_2(nor, false);
> +	else if (!enable_4_4_4 && params->enable_4_4_4 &&
> +		 nor->reg_proto == SNOR_PROTO_4_4_4)
> +		err = params->enable_4_4_4(nor, false);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Fix erase protocol if needed, read and write protocols should
> +	 * already be valid.
> +	 */
> +	switch (nor->reg_proto) {
> +	case SNOR_PROTO_4_4_4:
> +		nor->erase_proto = SNOR_PROTO_4_4_4;
> +		break;
> +
> +	case SNOR_PROTO_2_2_2:
> +		nor->erase_proto = SNOR_PROTO_2_2_2;
> +		break;
> +
> +	default:
> +		nor->erase_proto = SNOR_PROTO_1_1_1;
> +		break;
> +	}
> +
> +	dev_dbg(nor->dev,
> +		"(Fast) Read:  opcode=%02Xh, protocol=%03x, mode=%u, wait=%u\n",
> +		nor->read_opcode, nor->read_proto,
> +		read->num_mode_clocks, read->num_wait_states);
> +	dev_dbg(nor->dev,
> +		"Page Program: opcode=%02Xh, protocol=%03x\n",
> +		nor->program_opcode, nor->write_proto);
> +	dev_dbg(nor->dev,
> +		"Sector Erase: opcode=%02Xh, protocol=%03x, sector size=%zu\n",
> +		nor->erase_opcode, nor->erase_proto, nor->mtd.erasesize);
> +
> +	return 0;
> +}
> +
> +int spi_nor_scan(struct spi_nor *nor, const char *name,
> +		 struct spi_nor_modes *modes)
> +{
> +	const struct spi_nor_basic_flash_parameter *params = NULL;
>  	const struct flash_info *info = NULL;
>  	struct device *dev = nor->dev;
>  	struct mtd_info *mtd = &nor->mtd;
> @@ -1342,11 +1483,17 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  	if (ret)
>  		return ret;
>  
> +	/* Reset SPI protocol for all commands */
> +	nor->erase_proto = SNOR_PROTO_1_1_1;
> +	nor->read_proto = SNOR_PROTO_1_1_1;
> +	nor->write_proto = SNOR_PROTO_1_1_1;
> +	nor->reg_proto = SNOR_PROTO_1_1_1;
> +
>  	if (name)
>  		info = spi_nor_match_id(name);
>  	/* Try to auto-detect if chip name wasn't specified or not found */
>  	if (!info)
> -		info = spi_nor_read_id(nor);
> +		info = spi_nor_read_id(nor, NULL, 0);
>  	if (IS_ERR_OR_NULL(info))
>  		return -ENOENT;
>  
> @@ -1357,7 +1504,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  	if (name && info->id_len) {
>  		const struct flash_info *jinfo;
>  
> -		jinfo = spi_nor_read_id(nor);
> +		jinfo = spi_nor_read_id(nor, info->params, modes->id_modes);
>  		if (IS_ERR(jinfo)) {
>  			return PTR_ERR(jinfo);
>  		} else if (jinfo->id_len != info->id_len ||
> @@ -1374,6 +1521,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  			info = jinfo;
>  		}
>  	}
> +	params = info->params;
>  
>  	mutex_init(&nor->lock);
>  
> @@ -1451,51 +1599,42 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  	if (np) {
>  		/* If we were instantiated by DT, use it */
>  		if (of_property_read_bool(np, "m25p,fast-read"))
> -			nor->flash_read = SPI_NOR_FAST;
> +			modes->rd_modes |= SNOR_MODE_1_1_1;
>  		else
> -			nor->flash_read = SPI_NOR_NORMAL;
> +			modes->rd_modes &= ~SNOR_MODE_1_1_1;
>  	} else {
>  		/* If we weren't instantiated by DT, default to fast-read */
> -		nor->flash_read = SPI_NOR_FAST;
> +		modes->rd_modes |= SNOR_MODE_1_1_1;
>  	}
>  
>  	/* Some devices cannot do fast-read, no matter what DT tells us */
>  	if (info->flags & SPI_NOR_NO_FR)
> -		nor->flash_read = SPI_NOR_NORMAL;
> +		modes->rd_modes &= ~SNOR_MODE_1_1_1;
>  
> -	/* Quad/Dual-read mode takes precedence over fast/normal */
> -	if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
> -		ret = set_quad_mode(nor, info);
> -		if (ret) {
> -			dev_err(dev, "quad mode not supported\n");

Now that the set_quad_mode() is removed, could you explain how spansion
flash enters SNOR_MODE_1_1_4 mode? Is it bootloader's responsibility to
flash's set quad enable bit?

Regards
Vignesh

[toc] | [next] | [standalone]

#1386173

Hi Vignesh,

Le 24/04/2016 07:06, R, Vignesh a écrit :
> Hi Cyrille,
> 
> On 4/13/2016 10:53 PM, Cyrille Pitchen wrote:
> [...]
>> @@ -1451,51 +1599,42 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  	if (np) {
>>  		/* If we were instantiated by DT, use it */
>>  		if (of_property_read_bool(np, "m25p,fast-read"))
>> -			nor->flash_read = SPI_NOR_FAST;
>> +			modes->rd_modes |= SNOR_MODE_1_1_1;
>>  		else
>> -			nor->flash_read = SPI_NOR_NORMAL;
>> +			modes->rd_modes &= ~SNOR_MODE_1_1_1;
>>  	} else {
>>  		/* If we weren't instantiated by DT, default to fast-read */
>> -		nor->flash_read = SPI_NOR_FAST;
>> +		modes->rd_modes |= SNOR_MODE_1_1_1;
>>  	}
>>  
>>  	/* Some devices cannot do fast-read, no matter what DT tells us */
>>  	if (info->flags & SPI_NOR_NO_FR)
>> -		nor->flash_read = SPI_NOR_NORMAL;
>> +		modes->rd_modes &= ~SNOR_MODE_1_1_1;
>>  
>> -	/* Quad/Dual-read mode takes precedence over fast/normal */
>> -	if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
>> -		ret = set_quad_mode(nor, info);
>> -		if (ret) {
>> -			dev_err(dev, "quad mode not supported\n");
> 
> Now that the set_quad_mode() is removed, could you explain how spansion
> flash enters SNOR_MODE_1_1_4 mode? Is it bootloader's responsibility to
> flash's set quad enable bit?
> 

The proposed mechanics relies on the struct spi_nor_basic_flash_parameter.
This structure provides the same kind of parameters that those we could find
in JEDEC SFDP (Serial Flash Discoverable Parameters):
- (Fast) Read op codes and protocols supported by the memory and the associated
  number of dummy cycles. More precisely the number of mode clock and wait
  state cycle.
- Page Program op code and protocols.
- Sector/Block Erase op codes and the associated erase size.
- The "Quad Enable Requirements (QER)" which tells us "whether the device 
  contains a Quad Enable (QE) bit used to enable 1-1-4 and 1-4-4 quad read or
  quad program operations" as found in the JESD216B specification and
  implemented here through the optional .enable_quad_io() hook.
- The "4-4-4 mode enable and disable sequences", implemented by the optional
  .enable_4_4_4() hook.
- The optional .enable_2_2_2() hook: the JESD216B specification misses to
  describe this procedure, only used by Micron AFAIK.

For this RFC series, I've only provided a patch (patch 6) for Micron as an
example of the new way to support QSPI memories.
I've already written another patch for the support of Macronix memories but I
didn't publish it yet.

I was waiting for comments on the series to know whether I'm on the good way
before spending much time on writing support to other manufacturers.

For Spansion, the .enable_4_4_4() and .enable_2_2_2() would not be implemented
as Spansion QSPI memories only supports 1-1-2, 1-2-2, 1-1-4 and 1-4-4
protocols. Hence only the .enable_quad_io() hook is needed. This hook should
be implemented using something very closed to the spansion_quad_enable()
function.

So at one side the struct spi_nor_basic_flash_parameter describes the SPI
memory hardware capabilities and SPI protocols it supports.
On the other side, the struct spi_nor_modes allows the caller of spi_nor_scan()
to describe the hardware capabilities of the SPI controller; which SPI
protocols the controller supports and which SPI protocols the user wants to
allow or disable.

As an example, if a QSPI memory has already entered its 4-4-4 mode, you can
still probe the memory from Linux by reading the JEDEC ID as long as you set
the SNOR_MODE_4_4_4 bit in modes.id_modes.
Then, if you don't set the SNOR_MODE_4_4_4 bit in modes.rd_modes but only the
SNOR_MODE_1_1_4 bit, spi_nor_scan() first makes the SPI exit its 4-4-4 mode
then select the SPI 1-1-4 (if supported by the memory) for Fast Read
operations. You can do so if for some reason you'd rather use the SPI 1-1-4
protocol instead of the 4-4-4.

So it's now up to the caller of spi_nor_scan() to tell the framework what
protocols it supports and what protocols it wants to use. The spi-nor framework
will select the best match between the memory and controller hardware
capabilities.

Anyway, thanks for the review! :)

> Regards
> Vignesh
> 

Best regards,

Cyrille

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web