[V5,1/1] driver:mtd:spi-nor: Add quad I/O support for Micron spi nor

This patch adds code which enables Quad I/O mode on Micron SPI NOR flashes.

For Micron SPI NOR flash,enabling or disabling quad I/O protocol is controlled
by EVCR (Enhanced Volatile Configuration Register), Quad I/O protocol bit 7.
When EVCR bit 7 is reset to 0,the SPI NOR flash will operate in quad I/O mode.

Signed-off-by: bean huo <beanhuo@micron.com>
Acked-by: Marek Vasut <marex@denx.de>
---
 v1-v2:
	Modified to that capture wait_till_ready()
	return value,if error,directly return its
	the value.
 v2-v3:
	Directly use the reurning error value of
	read_reg and write_reg,instead of -EINVAL.
 v3-v4:
	Modify commit logs that wraped into 80 columns 
 v4-v5:
	Rebuild new patch based on latest linux-mtd

 drivers/mtd/spi-nor/spi-nor.c |   46 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h   |    6 ++++++
 2 files changed, 52 insertions(+)

On 11/05/2014 09:09 PM, bpqw wrote:
> This patch adds code which enables Quad I/O mode on Micron SPI NOR flashes.
> 
> For Micron SPI NOR flash,enabling or disabling quad I/O protocol is
> controlled
> by EVCR (Enhanced Volatile Configuration Register), Quad I/O protocol bit 7.
> When EVCR bit 7 is reset to 0,the SPI NOR flash will operate in quad I/O
> mode.

Hi, I'm having trouble with this patch using a Cadence QSPI controller and
Micron n25q00 part.

I can use quad commands in Extended SPI mode, but I can't make this EVCR Quad
mode work.

The Cadence QSPI Controller has fields to configure the quad transfer, and can
support quad opcode, quad address, and quad data, or some combination.  There
is a chart in the docs which shows the combinations for various read commands.

Problem is, I've tried all of the combinations and all I get is FF with this
EVCR patch.

If I don't set the quad mode in the EVCR, then I can use quad read commands no
problem.

Bottom line, with the Cadence QSPI controller, if I use quad commands in
Extended SPI mode, then all good.  If I use this EVCR quad mode, then all bad.

Anybody else have a Cadence QSPI controller and using EVCR quad mode
successfully?

Thanks,
Graham Moore

On 12 November 2014 01:11, Graham Moore <grmoore@opensource.altera.com> wrote:
> On 11/05/2014 09:09 PM, bpqw wrote:
>> This patch adds code which enables Quad I/O mode on Micron SPI NOR flashes.
>>
>> For Micron SPI NOR flash,enabling or disabling quad I/O protocol is
>> controlled
>> by EVCR (Enhanced Volatile Configuration Register), Quad I/O protocol bit 7.
>> When EVCR bit 7 is reset to 0,the SPI NOR flash will operate in quad I/O
>> mode.
>
> Hi, I'm having trouble with this patch using a Cadence QSPI controller and
> Micron n25q00 part.
>
> I can use quad commands in Extended SPI mode, but I can't make this EVCR Quad
> mode work.
>
> The Cadence QSPI Controller has fields to configure the quad transfer, and can
> support quad opcode, quad address, and quad data, or some combination.  There
> is a chart in the docs which shows the combinations for various read commands.
>
> Problem is, I've tried all of the combinations and all I get is FF with this
> EVCR patch.
>
> If I don't set the quad mode in the EVCR, then I can use quad read commands no
> problem.
>
> Bottom line, with the Cadence QSPI controller, if I use quad commands in
> Extended SPI mode, then all good.  If I use this EVCR quad mode, then all bad.
>
> Anybody else have a Cadence QSPI controller and using EVCR quad mode
> successfully?

I have almost verified all the micros parts for operating quad mode
and the quad enable bit is
volatile by default and no need to set it on software.

Why this code is meant for - does micron has changed this bit
operation on newly added parts?

thanks!

>Hi, I'm having trouble with this patch using a Cadence QSPI controller and Micron n25q00 part.

>I can use quad commands in Extended SPI mode, but I can't make this EVCR Quad mode work.


Yes,but if you use quad commands in Extended spi mode,only for Quad commands,the command line is DQ0,
Address/data line is DQ0,DQ1,DQ2 and DQ3(1-x-4). 
But if in Quad I/O mode,for all the commands,the command/address/data line will be 4,they are DQ0,DQ1,DQ2 and DQ3(4-x-4).

>The Cadence QSPI Controller has fields to configure the quad transfer, and can support quad opcode, 
>quad address, and quad data, or some combination.  There is a chart in the docs which shows the combinations for various read commands.

>Problem is, I've tried all of the combinations and all I get is FF with this EVCR patch.

This maybe your spi controller is still extended mode, 
Once EVCR bit 7 is set to 0, the spi nor device will operate in quad I/O.Command-address-data line is 4-x-4.
So after send WRITE EVCR command , spi controller also  must transfer to quad I/O Mode,and set its Command-address-data line also
Should be 4-x-4 .

>If I don't set the quad mode in the EVCR, then I can use quad read commands no problem.
Yes,you don't set the quad mode in the EVCR,you can use quad read commands,but this patch is for enable Micron SPI nor Quad I/O mode,
If you want to enable it ,you must set EVCR.

>Bottom line, with the Cadence QSPI controller, if I use quad commands in Extended SPI mode, then all good.  If I use this EVCR quad mode, then all bad.

>Anybody else have a Cadence QSPI controller and using EVCR quad mode successfully?

>Thanks,
>Graham Moore

Hi,Brian

Whether this patch can be merged?thanks.

>I have almost verified all the micros parts for operating quad mode and the quad enable bit is 
>volatile by default and no need to set it on software.

>Why this code is meant for - does micron has changed this bit operation on newly added parts?

>thanks!
>--
>Jagan.

For Micron Spi norflash,if you want to make it work Quad I/O mode,you can do it by set
Two registers,Nonvolatile Configuration resister(NVCR) and Enhanced volatile confuration register(EVCR),
but according to spi-nor.c,and micron spi nor,we recommend that if want to enable Micron spi nor Quad I/O
mode,the best way is to set EVCR.
Of course,you can use Quad/Dual operation command to read/write Micron spi nor in the spi nor Extended I/O mode.
But their command-address-data is different.

The purpose of this patch is only to enable Micron spi nor Quad I/O mode,if want to make Micron spi nor work 
Under Quad I/O mode.

Hi,Brian

How about this patch?Please give me some tips,thanks.

On Wed, 12 Nov 2014, bpqw wrote:

> This maybe your spi controller is still extended mode, 
> Once EVCR bit 7 is set to 0, the spi nor device will operate in quad I/O.Command-address-data line is 4-x-4.
> So after send WRITE EVCR command , spi controller also  must transfer to quad I/O Mode,and set its Command-address-data line also
> Should be 4-x-4 .

Thanks, this helped.  I added some code to snoop the command stream for 
WRITE EVCR with quad mode, and then set up the quad mode in the 
controller.  Seems kinda ugly, but working now.

-Graham

>> This maybe your spi controller is still extended mode, Once EVCR bit 7 
>> is set to 0, the spi nor device will operate in quad I/O.Command-address-data line is 4-x-4.
>> So after send WRITE EVCR command , spi controller also  must transfer 
>> to quad I/O Mode,and set its Command-address-data line also Should be 4-x-4 .

>Thanks, this helped.  I added some code to snoop the command stream for WRITE EVCR with quad mode, 
>and then set up the quad mode in the controller.  Seems kinda ugly, but working now.

>-Graham

Yes ,if enable spi nor Quad I/O, firstly, must check spi controller if support Quad I/O protocol,
and after enable spi nor Quad I/O mode ,spi controller also must be transferred to Quad I/O protocol.
Their two side must be matched together.
Maybe spi controller can do this changes.

Hi,Brian

Can you give me some tips about this patch? Thanks!

-Bean Huo

On Thu, Nov 06, 2014 at 03:09:06AM +0000, bpqw wrote:
> This patch adds code which enables Quad I/O mode on Micron SPI NOR flashes.
> 
> For Micron SPI NOR flash,enabling or disabling quad I/O protocol is controlled
> by EVCR (Enhanced Volatile Configuration Register), Quad I/O protocol bit 7.
> When EVCR bit 7 is reset to 0,the SPI NOR flash will operate in quad I/O mode.
> 
> Signed-off-by: bean huo <beanhuo@micron.com>
> Acked-by: Marek Vasut <marex@denx.de>
> ---
>  v1-v2:
> 	Modified to that capture wait_till_ready()
> 	return value,if error,directly return its
> 	the value.
>  v2-v3:
> 	Directly use the reurning error value of
> 	read_reg and write_reg,instead of -EINVAL.
>  v3-v4:
> 	Modify commit logs that wraped into 80 columns 
>  v4-v5:
> 	Rebuild new patch based on latest linux-mtd

You probably aren't based on l2-mtd.git. Your patch still doesn't build.
I can fix it up if it's easy, but FYI. Still reviewing...

drivers/mtd/spi-nor/spi-nor.c: In function ‘micron_quad_enable’:
drivers/mtd/spi-nor/spi-nor.c:874:2: warning: passing argument 3 of ‘nor->read_reg’ from incompatible pointer type [enabled by default]
  ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1);
  ^
drivers/mtd/spi-nor/spi-nor.c:874:2: note: expected ‘u8 *’ but argument is of type ‘int *’
drivers/mtd/spi-nor/spi-nor.c:891:2: error: implicit declaration of function ‘wait_till_ready’ [-Werror=implicit-function-declaration]
  ret = wait_till_ready(nor);
  ^
drivers/mtd/spi-nor/spi-nor.c:896:2: warning: passing argument 3 of ‘nor->read_reg’ from incompatible pointer type [enabled by default]
  ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1);
  ^
drivers/mtd/spi-nor/spi-nor.c:896:2: note: expected ‘u8 *’ but argument is of type ‘int *’

Brian

>You probably aren't based on l2-mtd.git. Your patch still doesn't build.
>I can fix it up if it's easy, but FYI. Still reviewing...

>Brian

Hi, Brian

Thanks for your hard work. Finally received your response, I am very happy.
How about this patch? Whether or not rebuild it based on lastest l2-mtd?
Look forward to your letter.thanks.

First of all, can you fix your mail so that you have a proper 'From'?
That should be your real name (not bpqw), so that it gives a proper
patch author. If you can't get your mail header to have the right
'From:' line, then it also works to begin your mail with:

From: Your Name <your@email.com>

On Thu, Nov 06, 2014 at 03:09:06AM +0000, bpqw wrote:
> This patch adds code which enables Quad I/O mode on Micron SPI NOR flashes.
> 
> For Micron SPI NOR flash,enabling or disabling quad I/O protocol is controlled
> by EVCR (Enhanced Volatile Configuration Register), Quad I/O protocol bit 7.
> When EVCR bit 7 is reset to 0,the SPI NOR flash will operate in quad I/O mode.

What's the difference between using EVCR and the ENTER QUAD I/O MODE
(35h) command I see in some of your datasheets? Are both supported on
all Micron quad I/O SPI NOR flash?

Also, which SPI NOR is this enabled for? I don't see any Micron entries
in spi_nor_ids[] which contain the SPI_NOR_QUAD_READ flag.

> Signed-off-by: bean huo <beanhuo@micron.com>
> Acked-by: Marek Vasut <marex@denx.de>
> ---
>  v1-v2:
> 	Modified to that capture wait_till_ready()
> 	return value,if error,directly return its
> 	the value.
>  v2-v3:
> 	Directly use the reurning error value of
> 	read_reg and write_reg,instead of -EINVAL.
>  v3-v4:
> 	Modify commit logs that wraped into 80 columns 
>  v4-v5:
> 	Rebuild new patch based on latest linux-mtd

Please rebase on l2-mtd.git. Sorry if that wasn't clear earlier.

>  drivers/mtd/spi-nor/spi-nor.c |   46 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |    6 ++++++
>  2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c51ee52..2a31742 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -874,6 +874,45 @@ static int spansion_quad_enable(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +static int micron_quad_enable(struct spi_nor *nor)
> +{
> +	int ret, val;
> +
> +	ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1);
> +	if (ret < 0) {
> +		dev_err(nor->dev, "error %d reading EVCR\n", ret);
> +		return ret;
> +	}
> +
> +	write_enable(nor);
> +
> +	/* set EVCR ,enable quad I/O */
> +	nor->cmd_buf[0] = val & ~EVCR_QUAD_EN_MICRON;
> +	ret = nor->write_reg(nor, SPINOR_OP_WD_EVCR, nor->cmd_buf, 1, 0);
> +	if (ret < 0) {
> +		dev_err(nor->dev,
> +			"error while writing EVCR register\n");

Join the above two lines?

> +		return ret;
> +	}
> +
> +	ret = wait_till_ready(nor);

It's spi_nor_wait_till_ready(), now.

> +	if (ret)
> +		return ret;
> +
> +	/* read EVCR and check it */
> +	ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1);
> +	if (ret < 0) {
> +		dev_err(nor->dev, "error %d reading EVCR\n", ret);
> +		return ret;
> +	}
> +	if (val & EVCR_QUAD_EN_MICRON) {
> +		dev_err(nor->dev, "Micron EVCR Quad bit not clear\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int set_quad_mode(struct spi_nor *nor, u32 jedec_id)
>  {
>  	int status;
> @@ -886,6 +925,13 @@ static int set_quad_mode(struct spi_nor *nor, u32 jedec_id)
>  			return -EINVAL;
>  		}
>  		return status;
> +	case CFI_MFR_ST:
> +		status = micron_quad_enable(nor);
> +		if (status) {
> +			dev_err(nor->dev, "Micron quad-read not enabled\n");
> +			return -EINVAL;
> +		}
> +		return status;
>  	default:
>  		status = spansion_quad_enable(nor);
>  		if (status) {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 046a0a2..42e7e37 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -56,6 +56,10 @@
>  /* Used for Spansion flashes only. */
>  #define SPINOR_OP_BRWR		0x17	/* Bank register write */
>  
> +/* Used for Micron flashes only. */
> +#define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
> +#define SPINOR_OP_WD_EVCR      0x61    /* Write EVCR register */
> +
>  /* Status Register bits. */
>  #define SR_WIP			1	/* Write in progress */
>  #define SR_WEL			2	/* Write enable latch */
> @@ -67,6 +71,8 @@
>  
>  #define SR_QUAD_EN_MX		0x40	/* Macronix Quad I/O */
>  
> +#define EVCR_QUAD_EN_MICRON    0x80    /* Micron Quad I/O */

Like with other register bitfields (SR, FSR), please place a comment
above to describe the register, like:

/* Enhanced Volatile Configuration Register bits */

> +
>  /* Flag Status Register bits */
>  #define FSR_READY		0x80
>  

Brian

>First of all, can you fix your mail so that you have a proper 'From'?
>That should be your real name (not bpqw), so that it gives a proper patch author. 
>If you can't get your mail header to have the right 'From:' line, then it also works to begin your mail with:

Sorry for this confusion. This bpqw email is our software public mailbox dedicated to submit linux patch.
Because our personal email title include Chinese name, this will result to messy code in from item.
I have ever submit one patch many times by my personal mail, but I always didn't accept maintainer's response.
So I think, maybe my patch with Chinese name has been moved into maintainer's junk folder.

I don't know my mail from-field with Chinese name can or not be accepted, if can, 
I will submit next version patch by my personal mail.
 
>> This patch adds code which enables Quad I/O mode on Micron SPI NOR flashes.
>> 
>> For Micron SPI NOR flash,enabling or disabling quad I/O protocol is 
>> controlled by EVCR (Enhanced Volatile Configuration Register), Quad I/O protocol bit 7.
>> When EVCR bit 7 is reset to 0,the SPI NOR flash will operate in quad I/O mode.

>What's the difference between using EVCR and the ENTER QUAD I/O MODE
>(35h) command I see in some of your datasheets? Are both supported on all Micron quad I/O SPI NOR flash?

There is no difference between using EVCR and the ENTER QUAD I/O MODE command.
But, for some Micron spi nor, there no ENTER Quad I/O command(35h),such as n25q064.
for all current Micron spi nor, if it support quad I/O mode, Using EVCR definitely be supported.
So, we recommend that enable QUAD I/O mode by writing ECVR.
  

>Also, which SPI NOR is this enabled for? I don't see any Micron entries in spi_nor_ids[] which contain the SPI_NOR_QUAD_READ flag.

Yes, we now don't see any Micron entries in spi_nor_ids[] which contain the SPI_NOR_QUAD_READ flag.
But Micron spi nor in spi_nor_ids[] all support Quad I/O mode. maybe customer want to use default mode(extended I/O mode),
When submitted relevant patch, they didn't SPI_NOR_QUAD_READ flag in the spi_nor_ids[].
This patch is just for wanting to enable Micron Quad I/O mode.


>> Signed-off-by: bean huo <beanhuo@micron.com>
>> Acked-by: Marek Vasut <marex@denx.de>
>> ---
>>  v1-v2:
>> 	Modified to that capture wait_till_ready()
>> 	return value,if error,directly return its
>> 	the value.
>>  v2-v3:
>> 	Directly use the reurning error value of
>> 	read_reg and write_reg,instead of -EINVAL.
>>  v3-v4:
>> 	Modify commit logs that wraped into 80 columns
>>  v4-v5:
>> 	Rebuild new patch based on latest linux-mtd

>Please rebase on l2-mtd.git. Sorry if that wasn't clear earlier.

OK,I will rebase it and sumbit a new version. Thanks for your suggestion.

>> +		dev_err(nor->dev,
>> +			"error while writing EVCR register\n");

>Join the above two lines?

Will be fixed it in the next version.

>> +		return ret;
>> +	}
>> +
>> +	ret = wait_till_ready(nor);

>It's spi_nor_wait_till_ready(), now.

OK, will be fixed it.

>>  
>>  #define SR_QUAD_EN_MX		0x40	/* Macronix Quad I/O */
>>  
>> +#define EVCR_QUAD_EN_MICRON    0x80    /* Micron Quad I/O */

>Like with other register bitfields (SR, FSR), please place a comment above to describe the register, like:


OK, will be fixed it.

>Brian

All in all ,thanks for your response and valuable suggestions.
I will rebuild a new version, and submit it .

---Bean Huo---

On Thu, Nov 27, 2014 at 05:55:43AM +0000, bpqw wrote:
 
> >What's the difference between using EVCR and the ENTER QUAD I/O MODE
> >(35h) command I see in some of your datasheets? Are both supported on all Micron quad I/O SPI NOR flash?
> 
> There is no difference between using EVCR and the ENTER QUAD I/O MODE command.
> But, for some Micron spi nor, there no ENTER Quad I/O command(35h),such as n25q064.
> for all current Micron spi nor, if it support quad I/O mode, Using EVCR definitely be supported.
> So, we recommend that enable QUAD I/O mode by writing ECVR.

Good to know. Thanks for the info. Maybe you can note this somewhere in
your next version, like a comment in the commit description.

> >Also, which SPI NOR is this enabled for? I don't see any Micron entries in spi_nor_ids[] which contain the SPI_NOR_QUAD_READ flag.
> 
> Yes, we now don't see any Micron entries in spi_nor_ids[] which
> contain the SPI_NOR_QUAD_READ flag. But Micron spi nor in
> spi_nor_ids[] all support Quad I/O mode.

Then add them! At least, for flash that support them (for all packages
that might share the same ID).

> maybe customer want to use default mode(extended I/O mode),
> When submitted relevant patch, they didn't SPI_NOR_QUAD_READ flag in the spi_nor_ids[].

The flag may have been omitted because it didn't exist at the time. Or
it didn't get tested.

Regardless, just because you enable this flag in spi_nor_ids[] doesn't
mean that it will enable Quad I/O for users that can't support it; board
files / DT descriptions still have to opt in by advertising hardware
support for Quad I/O.

See, for example, spi-tx-bus-width and spi-rx-bus-width properties in
Documentation/devicetree/bindings/spi/spi-bus.txt.

> This patch is just for wanting to enable Micron Quad I/O mode.

That's fine. But I'd welcome any follow-up patch to add the QUAD flag to
the right Micron table entries. Otherwise, this patch doesn't actually
help anyone.

BTW, given that you aren't changing any entries to spi_nor_ids[] yet,
have you actually tested this patch? Or are you only working off the
specifications / datasheets?

Thanks,
Brian

>>>Also, which SPI NOR is this enabled for? I don't see any Micron entries in spi_nor_ids[] which contain the SPI_NOR_QUAD_READ flag.
>> 
>> Yes, we now don't see any Micron entries in spi_nor_ids[] which 
>> contain the SPI_NOR_QUAD_READ flag. But Micron spi nor in 
>> spi_nor_ids[] all support Quad I/O mode.

>Then add them! At least, for flash that support them (for all packages that might share the same ID).

Ok,I will add them for our Micron spi nor.

>> This patch is just for wanting to enable Micron Quad I/O mode.

>That's fine. But I'd welcome any follow-up patch to add the QUAD flag to the right Micron table entries. Otherwise, this patch doesn't actually help anyone.

>BTW, given that you aren't changing any entries to spi_nor_ids[] yet, have you actually tested this patch? Or are you only working off the specifications / datasheets?

Yes, I have tested my patch based on latest linux kernel ,the spi nor  are just our Micron spi nor,
such as 45nm MT25QL256Mb and Mt25TL245Mb.Before submitting a patch, our team will review and test it.
only past testing and confirm OK,this patch can be submitted. 

>Thanks,
>Brian

Hi, Brian

Thanks again for your warming response. I rebase a new version patch based on latest l2-mtd.
Besides, we will submit patch by our personal email.