| Message ID | 1435663042-25425-1-git-send-email-mike.looijmans@topic.nl |
|---|---|
| State | Not Applicable |
| Headers | show |
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/
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/
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
>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
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
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
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");
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(+)