[PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[Mike Looijmans]

Micron QUAD mode expects command, address and data on 4 lanes instead of just
one for command (extended SPI mode). This requires the controller to be in a
special mode, so check first if the controller could be in that mode. If a
controller does not have the SPI_TX_QUAD mode set, this setting has no chance
of being valid at all, so don't try to enable it then, and just keep using
the extended SPI mode.

Tested on a Zynq 7000 with a n25q256a flash chip, this failed to function
because of the introduction of:
"driver:mtd:spi-nor: Add quad I/O support for Micron spi nor"
This commit sets QUAD mode for most Micron chips without asking the controller
whether it's possible to do so, and without telling the controller that a
different mode is required, so it couldn't work.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/mtd/spi-nor/spi-nor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e8f6131..10ba94f 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1398,6 +1398,8 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
 		}
 		return status;
 	case CFI_MFR_ST:
+		if (!(nor->spi->mode & SPI_TX_QUAD))
+			return 0;
 		status = micron_quad_enable(nor);
 		if (status) {
 			dev_err(nor->dev, "Micron quad-read not enabled\n");
-- 
1.9.1

Re: [PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[Harini Katakam]

On Tue, Jun 30, 2015 at 4:47 PM, Mike Looijmans <mike.looijmans@topic.nl> wrote:
> Micron QUAD mode expects command, address and data on 4 lanes instead of just
> one for command (extended SPI mode). This requires the controller to be in a
> special mode, so check first if the controller could be in that mode. If a
> controller does not have the SPI_TX_QUAD mode set, this setting has no chance
> of being valid at all, so don't try to enable it then, and just keep using
> the extended SPI mode.
>
> Tested on a Zynq 7000 with a n25q256a flash chip, this failed to function
> because of the introduction of:
> "driver:mtd:spi-nor: Add quad I/O support for Micron spi nor"
> This commit sets QUAD mode for most Micron chips without asking the controller
> whether it's possible to do so, and without telling the controller that a
> different mode is required, so it couldn't work.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Reviewed-by: Harini Katakam <harinik@xilinx.com>

Regards,
Harini

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e8f6131..10ba94f 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1398,6 +1398,8 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
>                 }
>                 return status;
>         case CFI_MFR_ST:
> +               if (!(nor->spi->mode & SPI_TX_QUAD))
> +                       return 0;
>                 status = micron_quad_enable(nor);
>                 if (status) {
>                         dev_err(nor->dev, "Micron quad-read not enabled\n");
> --
> 1.9.1
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[Shubhrajyoti Datta]

On Tue, Jun 30, 2015 at 4:47 PM, Mike Looijmans <mike.looijmans@topic.nl> wrote:
> Micron QUAD mode expects command, address and data on 4 lanes instead of just
> one for command (extended SPI mode). This requires the controller to be in a
> special mode, so check first if the controller could be in that mode. If a
> controller does not have the SPI_TX_QUAD mode set, this setting has no chance
> of being valid at all, so don't try to enable it then, and just keep using
> the extended SPI mode.
>
> Tested on a Zynq 7000 with a n25q256a flash chip, this failed to function
> because of the introduction of:
> "driver:mtd:spi-nor: Add quad I/O support for Micron spi nor"
> This commit sets QUAD mode for most Micron chips without asking the controller
> whether it's possible to do so, and without telling the controller that a
> different mode is required, so it couldn't work.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e8f6131..10ba94f 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1398,6 +1398,8 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
>                 }
>                 return status;
>         case CFI_MFR_ST:
> +               if (!(nor->spi->mode & SPI_TX_QUAD))
> +                       return 0;

Should we return 0 ?

>                 status = micron_quad_enable(nor);
>                 if (status) {
>                         dev_err(nor->dev, "Micron quad-read not enabled\n");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[Graham Moore]

On 06/30/2015 06:17 AM, Mike Looijmans wrote:
> Micron QUAD mode expects command, address and data on 4 lanes instead of just
> one for command (extended SPI mode). This requires the controller to be in a
> special mode, so check first if the controller could be in that mode. If a
> controller does not have the SPI_TX_QUAD mode set, this setting has no chance
> of being valid at all, so don't try to enable it then, and just keep using
> the extended SPI mode.
>
> Tested on a Zynq 7000 with a n25q256a flash chip, this failed to function
> because of the introduction of:
> "driver:mtd:spi-nor: Add quad I/O support for Micron spi nor"
> This commit sets QUAD mode for most Micron chips without asking the controller
> whether it's possible to do so, and without telling the controller that a
> different mode is required, so it couldn't work.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>   drivers/mtd/spi-nor/spi-nor.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e8f6131..10ba94f 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1398,6 +1398,8 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
>   		}
>   		return status;
>   	case CFI_MFR_ST:
> +		if (!(nor->spi->mode & SPI_TX_QUAD))
> +			return 0;

This is a great idea, but what codebase are you working from?  My l2-mtd 
tree doesn't have a 'spi' member of spi_nor, and this case statement is 
around line 977, not 1398.

>   		status = micron_quad_enable(nor);
>   		if (status) {
>   			dev_err(nor->dev, "Micron quad-read not enabled\n");
>

Regards,
Graham

RE: [PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[Bean Huo 霍斌斌 (beanhuo)]

>drivers/mtd/spi-nor/spi-nor.c | 2 ++
> 1 file changed, 2 insertions(+)

>diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index e8f6131..10ba94f 100644
>--- a/drivers/mtd/spi-nor/spi-nor.c
>+++ b/drivers/mtd/spi-nor/spi-nor.c
>@@ -1398,6 +1398,8 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
 >		}
 >		return status;
 >	case CFI_MFR_ST:
>+		if (!(nor->spi->mode & SPI_TX_QUAD))
>+			return 0;
I think, this action should be done before callback set_quad_mode,and add some codes in 
Spi controller driver.what is more,how do you get spi member in spi_nor?seems no this member.

I have one concern that as long as spi nor be configed into quad mode,spi controller must be also 
immediately Transfer into quad mode, otherwise read status will be fail.
But setting spi controller layer(driver/spi/) and configure spi nor layer(driver/mtd/spi-nor)
 are not the same mtd layer,I found that it's hard to do.
But for new structure spi controller(such as driver/mtd/spi-nor/fsl-quadspi.c) is very reasonable.and 
it can be easy to set spi controller and spi nor into quad mode at the same time.

 >		status = micron_quad_enable(nor);
 >		if (status) {
 >			dev_err(nor->dev, "Micron quad-read not enabled\n");
>--
>1.9.1

Re: [PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[Mike Looijmans]

On 30-06-15 17:42, Graham Moore wrote:
> On 06/30/2015 06:17 AM, Mike Looijmans wrote:
>> Micron QUAD mode expects command, address and data on 4 lanes instead of just
>> one for command (extended SPI mode). This requires the controller to be in a
>> special mode, so check first if the controller could be in that mode. If a
>> controller does not have the SPI_TX_QUAD mode set, this setting has no chance
>> of being valid at all, so don't try to enable it then, and just keep using
>> the extended SPI mode.
>>
>> Tested on a Zynq 7000 with a n25q256a flash chip, this failed to function
>> because of the introduction of:
>> "driver:mtd:spi-nor: Add quad I/O support for Micron spi nor"
>> This commit sets QUAD mode for most Micron chips without asking the controller
>> whether it's possible to do so, and without telling the controller that a
>> different mode is required, so it couldn't work.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   drivers/mtd/spi-nor/spi-nor.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index e8f6131..10ba94f 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1398,6 +1398,8 @@ static int set_quad_mode(struct spi_nor *nor, struct
>> flash_info *info)
>>           }
>>           return status;
>>       case CFI_MFR_ST:
>> +        if (!(nor->spi->mode & SPI_TX_QUAD))
>> +            return 0;
>
> This is a great idea, but what codebase are you working from?  My l2-mtd tree
> doesn't have a 'spi' member of spi_nor, and this case statement is around line
> 977, not 1398.

I was working from the Xilinx kernel tree, and there's a lot of changes there. 
The one that added the "struct spi_device* spi" to the nor struct is:
63697f5cf9a6201556947ef0ea29442843e5ba61 mtd: spi-nor: Changes for stacked and 
parallel

>>           status = micron_quad_enable(nor);
>>           if (status) {
>>               dev_err(nor->dev, "Micron quad-read not enabled\n");
>>




Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH] mtd: spi-nor: Only set Micron quad-read mode when controller in 4-lane TX mode

[Mike Looijmans]

On 01-07-15 04:19, Bean Huo 霍斌斌 (beanhuo) wrote:
>> drivers/mtd/spi-nor/spi-nor.c | 2 ++
>> 1 file changed, 2 insertions(+)
> 
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index e8f6131..10ba94f 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1398,6 +1398,8 @@ static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
>   >		}
>   >		return status;
>   >	case CFI_MFR_ST:
>> +		if (!(nor->spi->mode & SPI_TX_QUAD))
>> +			return 0;
> I think, this action should be done before callback set_quad_mode,and add some codes in
> Spi controller driver.what is more,how do you get spi member in spi_nor?seems no this member.

Too many changes between the Xilinx tree and mainline. My patch isn't for
mainline, I'll make a new one.

> I have one concern that as long as spi nor be configed into quad mode,spi controller must be also
> immediately Transfer into quad mode, otherwise read status will be fail.

The original commit that added the micron Quad mode did not do this. I wonder
how this was tested, because it could never have worked on any setup I'm aware
of. After setting the "quad mode" bit in volatile config, the controller needs
to start sending commands on all 4 lanes.
Even if it succeeds in doing that, a soft reset (e.g. unloading/loading the
driver, or kexec to another kernel) will bring the system in a state where the
controller will use 1 lane for commands while the chip expects 4, thus
breaking all communication.

Having slept and pondered about this, I suggest plainly removing the Micron
Quad mode code, because it is just broken and can never work. There's no
actual performance advantage here, apart from just saving a few clocks on
commands, which would be less than 0.1% on a 1k data read.

> But setting spi controller layer(driver/spi/) and configure spi nor layer(driver/mtd/spi-nor)
>   are not the same mtd layer,I found that it's hard to do.
> But for new structure spi controller(such as driver/mtd/spi-nor/fsl-quadspi.c) is very reasonable.and
> it can be easy to set spi controller and spi nor into quad mode at the same time.

I'm sorry, but I didn't understand what you meant  here.



Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail