[PATCH v3 00/36] mtd: st_spi_fsm: Add new driver

[PATCH v3 00/36] mtd: st_spi_fsm: Add new driver

[Lee Jones]

Version 3:
  Okay, this thing should be fully functional now. Identify a chip
  based on it's JEDEC ID, Read, Write, Erase (all or by sector).
  Support for various chip quirks added too.

Version 2:
  The first bunch of these patches have been on the MLs before, but
  didn't receive a great deal of attention for the most part. We are
  a little more featureful this time however. We can now successfully
  setup and configure the N25Q256. We still can't read/write/erase
  it though. I'll start work on that next week and will provide it in
  the next instalment.

Version 1:
  First stab at getting this thing Mainlined. It doesn't do a great deal
  yet, but we are able to initialise the device and dynamically set it up
  correctly based on an extracted JEDEC ID.
 
 Documentation/devicetree/bindings/mtd/st-fsm.txt |   26 ++
 arch/arm/boot/dts/stih416-b2105.dts              |   14 +
 arch/arm/boot/dts/stih416-pinctrl.dtsi           |   12 +
 drivers/mtd/devices/Kconfig                      |    7 +
 drivers/mtd/devices/Makefile                     |    1 +
 drivers/mtd/devices/serial_flash_cmds.h          |   91 +++++++
 drivers/mtd/devices/st_spi_fsm.c                 | 1715 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h                 |  456 +++++++++++++++++++++++++++++++
 8 files changed, 2322 insertions(+)

[PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework

[Lee Jones]

This is a new driver. It's used to communicate with a special type of
optimised Serial Flash Controller called the FSM. The FSM uses a subset
of the SPI protocol to communicate with supported NOR-Flash devices.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/Kconfig      |   7 +++
 drivers/mtd/devices/Makefile     |   1 +
 drivers/mtd/devices/st_spi_fsm.c | 111 +++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |  27 ++++++++++
 4 files changed, 146 insertions(+)
 create mode 100644 drivers/mtd/devices/st_spi_fsm.c
 create mode 100644 drivers/mtd/devices/st_spi_fsm.h

diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index 74ab4b7..d977281 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -217,6 +217,13 @@ config MTD_DOCG3
 	  M-Systems and now Sandisk. The support is very experimental,
 	  and doesn't give access to any write operations.
 
+config MTD_ST_SPI_FSM
+	tristate "ST Microelectronics SPI FSM Serial Flash Controller"
+	help
+	  This provides an MTD device driver for the ST Microelectronics
+	  SPI FSM Serial Flash Controller and support for a subset of
+	  connected Serial Flash devices.
+
 if MTD_DOCG3
 config BCH_CONST_M
 	default 14
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index d83bd73..c68868f 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_OMAP_BCH)	+= elm.o
 obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
 obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
+obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
 
 
 CFLAGS_docg3.o			+= -I$(src)
diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
new file mode 100644
index 0000000..1e3abde
--- /dev/null
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -0,0 +1,111 @@
+/*
+ * st_spi_fsm.c	Support for ST Serial Flash Controller
+ *
+ * Author: Angus Clark <angus.clark@st.com>
+ *
+ * Copyright (C) 2010-2013 STicroelectronics Limited
+ *
+ * JEDEC probe based on drivers/mtd/devices/m25p80.c
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mtd/mtd.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include <asm/io.h>
+
+#include "st_spi_fsm.h"
+
+static int stfsm_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct resource *res;
+	struct stfsm *fsm;
+
+	if (!np) {
+		dev_err(&pdev->dev, "No DT found\n");
+		return -EINVAL;
+	}
+
+	fsm = devm_kzalloc(&pdev->dev, sizeof(*fsm), GFP_KERNEL);
+	if (!fsm)
+		return -ENOMEM;
+
+	fsm->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Resource not found\n");
+		return -ENODEV;
+	}
+
+	fsm->region = devm_request_mem_region(&pdev->dev, res->start,
+					      resource_size(res), pdev->name);
+	if (!fsm->region) {
+		dev_err(&pdev->dev,
+			"Failed to reserve memory region [0x%08x-0x%08x]\n",
+			res->start, res->end);
+		return -EBUSY;
+	}
+
+	fsm->base = devm_ioremap_nocache(&pdev->dev,
+					 res->start, resource_size(res));
+	if (!fsm->base) {
+		dev_err(&pdev->dev, "Failed to ioremap [0x%08x]\n", res->start);
+		return -EINVAL;
+	}
+
+	mutex_init(&fsm->lock);
+
+	platform_set_drvdata(pdev, fsm);
+
+	fsm->mtd.dev.parent	= &pdev->dev;
+	fsm->mtd.type		= MTD_NORFLASH;
+	fsm->mtd.writesize	= 4;
+	fsm->mtd.writebufsize	= fsm->mtd.writesize;
+	fsm->mtd.flags		= MTD_CAP_NORFLASH;
+
+	return mtd_device_parse_register(&fsm->mtd, NULL, NULL, NULL, 0);
+}
+
+static int stfsm_remove(struct platform_device *pdev)
+{
+	struct stfsm *fsm = platform_get_drvdata(pdev);
+	int err;
+
+	err = mtd_device_unregister(&fsm->mtd);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static struct of_device_id stfsm_match[] = {
+	{ .compatible = "st,spi-fsm", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, spi_fsm_match);
+
+static struct platform_driver stfsm_driver = {
+	.probe		= stfsm_probe,
+	.remove		= stfsm_remove,
+	.driver		= {
+		.name	= "st-spi-fsm",
+		.owner	= THIS_MODULE,
+		.of_match_table = stfsm_match,
+	},
+};
+module_platform_driver(stfsm_driver);
+
+MODULE_AUTHOR("Angus Clark <angus.clark@st.com>");
+MODULE_DESCRIPTION("ST SPI FSM driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
new file mode 100644
index 0000000..df45e1a
--- /dev/null
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -0,0 +1,27 @@
+/*
+ * st_spi_fsm.c	Support for ST Serial Flash Controller
+ *
+ * Author: Angus Clark <angus.clark@st.com>
+ *
+ * Copyright (C) 2010-2013 STicroelectronics Limited
+ *
+ * JEDEC probe based on drivers/mtd/devices/m25p80.c
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef ST_SPI_FSM_H
+#define ST_SPI_FSM_H
+
+struct stfsm {
+	struct device		*dev;
+	void __iomem		*base;
+	struct resource		*region;
+	struct mtd_info		mtd;
+	struct mutex		lock;
+};
+
+#endif	/* ST_SPI_FSM_H */
-- 
1.8.3.2

Re: [PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework

[srinivas kandagatla]

On 29/11/13 12:18, Lee Jones wrote:
> This is a new driver. It's used to communicate with a special type of
> optimised Serial Flash Controller called the FSM. The FSM uses a subset
> of the SPI protocol to communicate with supported NOR-Flash devices.
> 
You might want to expand what is FSM here..(Fast sequence mode).
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mtd/devices/Kconfig      |   7 +++
>  drivers/mtd/devices/Makefile     |   1 +
>  drivers/mtd/devices/st_spi_fsm.c | 111 +++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/devices/st_spi_fsm.h |  27 ++++++++++
>  4 files changed, 146 insertions(+)
>  create mode 100644 drivers/mtd/devices/st_spi_fsm.c
>  create mode 100644 drivers/mtd/devices/st_spi_fsm.h
> 
> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> index 74ab4b7..d977281 100644
> --- a/drivers/mtd/devices/Kconfig
> +++ b/drivers/mtd/devices/Kconfig
> @@ -217,6 +217,13 @@ config MTD_DOCG3
>  	  M-Systems and now Sandisk. The support is very experimental,
>  	  and doesn't give access to any write operations.
>  
> +config MTD_ST_SPI_FSM
> +	tristate "ST Microelectronics SPI FSM Serial Flash Controller"

I think this should depend on ARCH_STI, so that it is not build for x86
or any other platforms.

> +	help
> +	  This provides an MTD device driver for the ST Microelectronics
> +	  SPI FSM Serial Flash Controller and support for a subset of
> +	  connected Serial Flash devices.
> +
>  if MTD_DOCG3
>  config BCH_CONST_M
>  	default 14
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index d83bd73..c68868f 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_NAND_OMAP_BCH)	+= elm.o
>  obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
>  obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
>  obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
> +obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
>  
>  
>  CFLAGS_docg3.o			+= -I$(src)
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> new file mode 100644
> index 0000000..1e3abde
> --- /dev/null
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -0,0 +1,111 @@
> +/*
> + * st_spi_fsm.c	Support for ST Serial Flash Controller
> + *
> + * Author: Angus Clark <angus.clark@st.com>
> + *
> + * Copyright (C) 2010-2013 STicroelectronics Limited
> + *
> + * JEDEC probe based on drivers/mtd/devices/m25p80.c
> + *
> + * This code is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +
> +#include <asm/io.h>
Are you sure about including this?
> +
> +#include "st_spi_fsm.h"
Header file can be removed totally, as this is the only file which is
using it.
> +
> +static int stfsm_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *res;
> +	struct stfsm *fsm;
> +
> +	if (!np) {
> +		dev_err(&pdev->dev, "No DT found\n");
> +		return -EINVAL;
> +	}
> +
> +	fsm = devm_kzalloc(&pdev->dev, sizeof(*fsm), GFP_KERNEL);
> +	if (!fsm)
> +		return -ENOMEM;
> +
> +	fsm->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Resource not found\n");
> +		return -ENODEV;
> +	}
> +
> +	fsm->region = devm_request_mem_region(&pdev->dev, res->start,
> +					      resource_size(res), pdev->name);
> +	if (!fsm->region) {
> +		dev_err(&pdev->dev,
> +			"Failed to reserve memory region [0x%08x-0x%08x]\n",
> +			res->start, res->end);
> +		return -EBUSY;
> +	}
> +
> +	fsm->base = devm_ioremap_nocache(&pdev->dev,
> +					 res->start, resource_size(res));
> +	if (!fsm->base) {
> +		dev_err(&pdev->dev, "Failed to ioremap [0x%08x]\n", res->start);
> +		return -EINVAL;
> +	}
> +
> +	mutex_init(&fsm->lock);
> +
> +	platform_set_drvdata(pdev, fsm);
> +
> +	fsm->mtd.dev.parent	= &pdev->dev;
> +	fsm->mtd.type		= MTD_NORFLASH;
> +	fsm->mtd.writesize	= 4;
> +	fsm->mtd.writebufsize	= fsm->mtd.writesize;
> +	fsm->mtd.flags		= MTD_CAP_NORFLASH;
> +
> +	return mtd_device_parse_register(&fsm->mtd, NULL, NULL, NULL, 0);
> +}
> +
> +static int stfsm_remove(struct platform_device *pdev)
> +{
> +	struct stfsm *fsm = platform_get_drvdata(pdev);
> +	int err;
> +
> +	err = mtd_device_unregister(&fsm->mtd);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +Any reason not to use of_match_ptr macro here?


Thanks,
srini

> +static struct of_device_id stfsm_match[] = {
> +	{ .compatible = "st,spi-fsm", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, spi_fsm_match);
> +
> +static struct platform_driver stfsm_driver = {
> +	.probe		= stfsm_probe,
> +	.remove		= stfsm_remove,
> +	.driver		= {
> +		.name	= "st-spi-fsm",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = stfsm_match,
Any reason not to use of_match_ptr macro here?


Thanks,
srini

Re: [PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework

[Brian Norris]

On Fri, Nov 29, 2013 at 12:18:50PM +0000, Lee Jones wrote:
> --- /dev/null
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -0,0 +1,111 @@
> +/*
> + * st_spi_fsm.c	Support for ST Serial Flash Controller
> + *
> + * Author: Angus Clark <angus.clark@st.com>
> + *
> + * Copyright (C) 2010-2013 STicroelectronics Limited

Should be STMicrolectronics? :)

> + *
> + * JEDEC probe based on drivers/mtd/devices/m25p80.c
> + *
> + * This code is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +
> +#include <asm/io.h>
> +
> +#include "st_spi_fsm.h"

I agree with Srinivas. Unless you think this header can be used in
another file, it should probably be part of this .c file. It also lends
itself to some strange usage of 'static' functions declared (but not
defined) in the header. But I'll comment when I get to those patches.

> +
> +static int stfsm_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct resource *res;
> +	struct stfsm *fsm;
> +
> +	if (!np) {
> +		dev_err(&pdev->dev, "No DT found\n");
> +		return -EINVAL;
> +	}
> +
> +	fsm = devm_kzalloc(&pdev->dev, sizeof(*fsm), GFP_KERNEL);
> +	if (!fsm)
> +		return -ENOMEM;
> +
> +	fsm->dev = &pdev->dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Resource not found\n");
> +		return -ENODEV;
> +	}
> +
> +	fsm->region = devm_request_mem_region(&pdev->dev, res->start,
> +					      resource_size(res), pdev->name);
> +	if (!fsm->region) {
> +		dev_err(&pdev->dev,
> +			"Failed to reserve memory region [0x%08x-0x%08x]\n",

Use the special %pr or %pR format specifier? See
Documentation/printk-formats.txt

> +			res->start, res->end);
> +		return -EBUSY;
> +	}
> +
> +	fsm->base = devm_ioremap_nocache(&pdev->dev,
> +					 res->start, resource_size(res));
> +	if (!fsm->base) {
> +		dev_err(&pdev->dev, "Failed to ioremap [0x%08x]\n", res->start);

Also %pr or %pR?

> +		return -EINVAL;
> +	}
> +
> +	mutex_init(&fsm->lock);
> +
> +	platform_set_drvdata(pdev, fsm);
> +
> +	fsm->mtd.dev.parent	= &pdev->dev;
> +	fsm->mtd.type		= MTD_NORFLASH;
> +	fsm->mtd.writesize	= 4;
> +	fsm->mtd.writebufsize	= fsm->mtd.writesize;
> +	fsm->mtd.flags		= MTD_CAP_NORFLASH;
> +
> +	return mtd_device_parse_register(&fsm->mtd, NULL, NULL, NULL, 0);
> +}
> +
> +static int stfsm_remove(struct platform_device *pdev)
> +{
> +	struct stfsm *fsm = platform_get_drvdata(pdev);
> +	int err;
> +
> +	err = mtd_device_unregister(&fsm->mtd);
> +	if (err)
> +		return err;
> +
> +	return 0;

Simplify to the following?

	return mtd_device_unregister(&fsm->mtd);

> +}

[...]

Brian

Re: [PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework

[Brian Norris]

On Fri, Nov 29, 2013 at 12:18:50PM +0000, Lee Jones wrote:
> --- /dev/null
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -0,0 +1,111 @@

[...]

> +static struct of_device_id stfsm_match[] = {
> +	{ .compatible = "st,spi-fsm", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, spi_fsm_match);

s/spi_fsm_match/stfsm_match/

This doesn't compile.

Brian

Re: [PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework

[Lee Jones]

Hi Brian,

Thank you for taking the time to review, it's very much appreciated.
You bought out some interesting points that I'm happy to go away and
rectify. Firstly however, as I inherited this code I'd like to give
Angus a chance to comment before we go off on our own tangent and
rework some of the good code which perhaps should remain unchanged.

Angus, do you have enough time to go through Brian's review comments
and perhaps reply to the ones that you feel would benefit from your
expert knowledge. To be frank, some of the questions that were asked I
wouldn't be able to answer without your guidance in any case.

Thanks both.

Kind regards,
Lee

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework

[Angus Clark]

Hi Lee and Brian,

I would also like to thank Brian; as always, very sensible comments.

I am tied up most of today and off work tomorrow (Christmas shopping!), but I
will set aside Friday to go through the comments in detail.  I should also have
some time next week if necessary, subject to any panics that might arise!

I also intend to respond to Huang's updated 'spi-nor' framework at the same
time.  At some stage, I would expect some of the device probing code in
st_spi_fsm, particularly the configuration of read/write/erase operations based
on capabilities, to be pulled into the 'spi-nor' framework.  I do not see this
an an obstacle to st_spi_fsm being integrated earlier though; it's presence in
the kernel would provide another example of a H/W Controller that 'spi-nor'
needs to accommodate.

Cheers,

Angus


On 12/11/2013 08:48 AM, Lee Jones wrote:
> Hi Brian,
> 
> Thank you for taking the time to review, it's very much appreciated.
> You bought out some interesting points that I'm happy to go away and
> rectify. Firstly however, as I inherited this code I'd like to give
> Angus a chance to comment before we go off on our own tangent and
> rework some of the good code which perhaps should remain unchanged.
> 
> Angus, do you have enough time to go through Brian's review comments
> and perhaps reply to the ones that you feel would benefit from your
> expert knowledge. To be frank, some of the questions that were asked I
> wouldn't be able to answer without your guidance in any case.
> 
> Thanks both.
> 
> Kind regards,
> Lee
> 

Re: [PATCH v3 01/36] mtd: st_spi_fsm: Allocate resources and register with MTD framework

[Brian Norris]

Hi Angus,

On Wed, Dec 11, 2013 at 09:37:28AM +0000, Angus Clark wrote:
> At some stage, I would expect some of the device probing code in
> st_spi_fsm, particularly the configuration of read/write/erase operations based
> on capabilities, to be pulled into the 'spi-nor' framework.

Yes, I think this driver's device probing is starting to improve the ID
table approach that we currently have, which is why I directed a few
comments at it. I don't think it goes far enough to being useful to
others yet, though. It employs a few hacks that tie it very closely to
its own implementation, and I think this can be improved now, before its
methods are entrenched too much.

> I do not see this
> an an obstacle to st_spi_fsm being integrated earlier though; it's presence in
> the kernel would provide another example of a H/W Controller that 'spi-nor'
> needs to accommodate.

No, I don't think a generally-useful framework will pop up overnight,
but I think this driver can be one piece of the puzzle.

I await your further responses.

Thanks,
Brian

[PATCH v3 02/36] mtd: st_spi_fsm: Supply all register address and bit logic defines

[Lee Jones]

Here we provide the FSM's register addresses, register bit names/offsets
and some commands which will prove useful as we start bulk the FMS's
driver out with functionality.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.h | 164 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index df45e1a..b420a43 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -16,6 +16,170 @@
 #ifndef ST_SPI_FSM_H
 #define ST_SPI_FSM_H
 
+/*
+ * FSM SPI Controller Registers
+ */
+#define SPI_CLOCKDIV			0x0010
+#define SPI_MODESELECT			0x0018
+#define SPI_CONFIGDATA			0x0020
+#define SPI_STA_MODE_CHANGE		0x0028
+#define SPI_FAST_SEQ_TRANSFER_SIZE	0x0100
+#define SPI_FAST_SEQ_ADD1		0x0104
+#define SPI_FAST_SEQ_ADD2		0x0108
+#define SPI_FAST_SEQ_ADD_CFG		0x010c
+#define SPI_FAST_SEQ_OPC1		0x0110
+#define SPI_FAST_SEQ_OPC2		0x0114
+#define SPI_FAST_SEQ_OPC3		0x0118
+#define SPI_FAST_SEQ_OPC4		0x011c
+#define SPI_FAST_SEQ_OPC5		0x0120
+#define SPI_MODE_BITS			0x0124
+#define SPI_DUMMY_BITS			0x0128
+#define SPI_FAST_SEQ_FLASH_STA_DATA	0x012c
+#define SPI_FAST_SEQ_1			0x0130
+#define SPI_FAST_SEQ_2			0x0134
+#define SPI_FAST_SEQ_3			0x0138
+#define SPI_FAST_SEQ_4			0x013c
+#define SPI_FAST_SEQ_CFG		0x0140
+#define SPI_FAST_SEQ_STA		0x0144
+#define SPI_QUAD_BOOT_SEQ_INIT_1	0x0148
+#define SPI_QUAD_BOOT_SEQ_INIT_2	0x014c
+#define SPI_QUAD_BOOT_READ_SEQ_1	0x0150
+#define SPI_QUAD_BOOT_READ_SEQ_2	0x0154
+#define SPI_PROGRAM_ERASE_TIME		0x0158
+#define SPI_MULT_PAGE_REPEAT_SEQ_1	0x015c
+#define SPI_MULT_PAGE_REPEAT_SEQ_2	0x0160
+#define SPI_STATUS_WR_TIME_REG		0x0164
+#define SPI_FAST_SEQ_DATA_REG		0x0300
+
+/*
+ * Register: SPI_MODESELECT
+ */
+#define SPI_MODESELECT_CONTIG		0x01
+#define SPI_MODESELECT_FASTREAD		0x02
+#define SPI_MODESELECT_DUALIO		0x04
+#define SPI_MODESELECT_FSM		0x08
+#define SPI_MODESELECT_QUADBOOT		0x10
+
+/*
+ * Register: SPI_CONFIGDATA
+ */
+#define SPI_CFG_DEVICE_ST		0x1
+#define SPI_CFG_DEVICE_ATMEL		0x4
+#define SPI_CFG_MIN_CS_HIGH(x)		(((x) & 0xfff) << 4)
+#define SPI_CFG_CS_SETUPHOLD(x)		(((x) & 0xff) << 16)
+#define SPI_CFG_DATA_HOLD(x)		(((x) & 0xff) << 24)
+
+/*
+ * Register: SPI_FAST_SEQ_TRANSFER_SIZE
+ */
+#define TRANSFER_SIZE(x)		((x) * 8)
+
+/*
+ * Register: SPI_FAST_SEQ_ADD_CFG
+ */
+#define ADR_CFG_CYCLES_ADD1(x)		((x) << 0)
+#define ADR_CFG_PADS_1_ADD1		(0x0 << 6)
+#define ADR_CFG_PADS_2_ADD1		(0x1 << 6)
+#define ADR_CFG_PADS_4_ADD1		(0x3 << 6)
+#define ADR_CFG_CSDEASSERT_ADD1		(1   << 8)
+#define ADR_CFG_CYCLES_ADD2(x)		((x) << (0+16))
+#define ADR_CFG_PADS_1_ADD2		(0x0 << (6+16))
+#define ADR_CFG_PADS_2_ADD2		(0x1 << (6+16))
+#define ADR_CFG_PADS_4_ADD2		(0x3 << (6+16))
+#define ADR_CFG_CSDEASSERT_ADD2		(1   << (8+16))
+
+/*
+ * Register: SPI_FAST_SEQ_n
+ */
+#define SEQ_OPC_OPCODE(x)		((x) << 0)
+#define SEQ_OPC_CYCLES(x)		((x) << 8)
+#define SEQ_OPC_PADS_1			(0x0 << 14)
+#define SEQ_OPC_PADS_2			(0x1 << 14)
+#define SEQ_OPC_PADS_4			(0x3 << 14)
+#define SEQ_OPC_CSDEASSERT		(1   << 16)
+
+/*
+ * Register: SPI_FAST_SEQ_CFG
+ */
+#define SEQ_CFG_STARTSEQ		(1 << 0)
+#define SEQ_CFG_SWRESET			(1 << 5)
+#define SEQ_CFG_CSDEASSERT		(1 << 6)
+#define SEQ_CFG_READNOTWRITE		(1 << 7)
+#define SEQ_CFG_ERASE			(1 << 8)
+#define SEQ_CFG_PADS_1			(0x0 << 16)
+#define SEQ_CFG_PADS_2			(0x1 << 16)
+#define SEQ_CFG_PADS_4			(0x3 << 16)
+
+/*
+ * Register: SPI_MODE_BITS
+ */
+#define MODE_DATA(x)			(x & 0xff)
+#define MODE_CYCLES(x)			((x & 0x3f) << 16)
+#define MODE_PADS_1			(0x0 << 22)
+#define MODE_PADS_2			(0x1 << 22)
+#define MODE_PADS_4			(0x3 << 22)
+#define DUMMY_CSDEASSERT		(1   << 24)
+
+/*
+ * Register: SPI_DUMMY_BITS
+ */
+#define DUMMY_CYCLES(x)			((x & 0x3f) << 16)
+#define DUMMY_PADS_1			(0x0 << 22)
+#define DUMMY_PADS_2			(0x1 << 22)
+#define DUMMY_PADS_4			(0x3 << 22)
+#define DUMMY_CSDEASSERT		(1   << 24)
+
+/*
+ * Register: SPI_FAST_SEQ_FLASH_STA_DATA
+ */
+#define STA_DATA_BYTE1(x)		((x & 0xff) << 0)
+#define STA_DATA_BYTE2(x)		((x & 0xff) << 8)
+#define STA_PADS_1			(0x0 << 16)
+#define STA_PADS_2			(0x1 << 16)
+#define STA_PADS_4			(0x3 << 16)
+#define STA_CSDEASSERT			(0x1 << 20)
+#define STA_RDNOTWR			(0x1 << 21)
+
+/*
+ * FSM SPI Instruction Opcodes
+ */
+#define STFSM_OPC_CMD			0x1
+#define STFSM_OPC_ADD			0x2
+#define STFSM_OPC_STA			0x3
+#define STFSM_OPC_MODE			0x4
+#define STFSM_OPC_DUMMY		0x5
+#define STFSM_OPC_DATA			0x6
+#define STFSM_OPC_WAIT			0x7
+#define STFSM_OPC_JUMP			0x8
+#define STFSM_OPC_GOTO			0x9
+#define STFSM_OPC_STOP			0xF
+
+/*
+ * FSM SPI Instructions (== opcode + operand).
+ */
+#define STFSM_INSTR(cmd, op)		((cmd) | ((op) << 4))
+
+#define STFSM_INST_CMD1			STFSM_INSTR(STFSM_OPC_CMD,	1)
+#define STFSM_INST_CMD2			STFSM_INSTR(STFSM_OPC_CMD,	2)
+#define STFSM_INST_CMD3			STFSM_INSTR(STFSM_OPC_CMD,	3)
+#define STFSM_INST_CMD4			STFSM_INSTR(STFSM_OPC_CMD,	4)
+#define STFSM_INST_CMD5			STFSM_INSTR(STFSM_OPC_CMD,	5)
+#define STFSM_INST_ADD1			STFSM_INSTR(STFSM_OPC_ADD,	1)
+#define STFSM_INST_ADD2			STFSM_INSTR(STFSM_OPC_ADD,	2)
+
+#define STFSM_INST_DATA_WRITE		STFSM_INSTR(STFSM_OPC_DATA,	1)
+#define STFSM_INST_DATA_READ		STFSM_INSTR(STFSM_OPC_DATA,	2)
+
+#define STFSM_INST_STA_RD1		STFSM_INSTR(STFSM_OPC_STA,	0x1)
+#define STFSM_INST_STA_WR1		STFSM_INSTR(STFSM_OPC_STA,	0x1)
+#define STFSM_INST_STA_RD2		STFSM_INSTR(STFSM_OPC_STA,	0x2)
+#define STFSM_INST_STA_WR1_2		STFSM_INSTR(STFSM_OPC_STA,	0x3)
+
+#define STFSM_INST_MODE			STFSM_INSTR(STFSM_OPC_MODE,	0)
+#define STFSM_INST_DUMMY		STFSM_INSTR(STFSM_OPC_DUMMY,	0)
+#define STFSM_INST_WAIT			STFSM_INSTR(STFSM_OPC_WAIT,	0)
+#define STFSM_INST_STOP			STFSM_INSTR(STFSM_OPC_STOP,	0)
+
 struct stfsm {
 	struct device		*dev;
 	void __iomem		*base;
-- 
1.8.3.2

[PATCH v3 03/36] mtd: st_spi_fsm: Initialise and configure the FSM for normal working conditions

[Lee Jones]

This patch uses default values to initialise a connected flash chip. This
includes; a device soft reset, setting of a safe working frequency, a
switch into Fast Sequencing Mode, configuring of timing data and a purge
of the FIFO.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 116 +++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |  11 ++++
 2 files changed, 127 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 1e3abde..fe96b47 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -25,11 +25,121 @@
 
 #include "st_spi_fsm.h"
 
+static inline uint32_t stfsm_fifo_available(struct stfsm *fsm)
+{
+	return (readl(fsm->base + SPI_FAST_SEQ_STA) >> 5) & 0x7f;
+}
+
+static void stfsm_clear_fifo(struct stfsm *fsm)
+{
+	uint32_t avail;
+
+	for (;;) {
+		avail = stfsm_fifo_available(fsm);
+		if (!avail)
+			break;
+
+		while (avail) {
+			readl(fsm->base + SPI_FAST_SEQ_DATA_REG);
+			avail--;
+		}
+	}
+}
+
+static int stfsm_set_mode(struct stfsm *fsm, uint32_t mode)
+{
+	int ret, timeout = 10;
+
+	/* Wait for controller to accept mode change */
+	while(--timeout) {
+		ret = readl(fsm->base + SPI_STA_MODE_CHANGE);
+		if (ret & 0x1)
+			break;
+		udelay(1);
+	}
+
+	if (!timeout)
+		return -EBUSY;
+
+	writel(mode, fsm->base + SPI_MODESELECT);
+
+	return 0;
+}
+
+static void stfsm_set_freq(struct stfsm *fsm, uint32_t spi_freq)
+{
+	uint32_t emi_freq;
+	uint32_t clk_div;
+
+	/* TODO: Make this dynamic */
+	emi_freq = STFSM_DEFAULT_EMI_FREQ;
+
+	/*
+	 * Calculate clk_div - values between 2 and 128
+	 * Multiple of 2, rounded up
+	 */
+	clk_div = 2*((emi_freq + (2*spi_freq - 1))/(2*spi_freq));
+	if (clk_div < 2)
+		clk_div = 2;
+	else if (clk_div > 128)
+		clk_div = 128;
+
+	/*
+	 * Determine a suitable delay for the IP to complete a change of
+	 * direction of the FIFO. The required delay is related to the clock
+	 * divider used. The following heuristics are based on empirical tests,
+	 * using a 100MHz EMI clock.
+	 */
+	if (clk_div <= 4)
+		fsm->fifo_dir_delay = 0;
+	else if (clk_div <= 10)
+		fsm->fifo_dir_delay = 1;
+	else
+		fsm->fifo_dir_delay = (clk_div + 9) / 10;
+
+	dev_dbg(fsm->dev, "emi_clk = %uHZ, spi_freq = %uHZ, clk_div = %u\n",
+		emi_freq, spi_freq, clk_div);
+
+	writel(clk_div, fsm->base + SPI_CLOCKDIV);
+}
+
+static int stfsm_init(struct stfsm *fsm)
+{
+	int ret;
+
+	/* Perform a soft reset of the FSM controller */
+	writel(SEQ_CFG_SWRESET, fsm->base + SPI_FAST_SEQ_CFG);
+	udelay(1);
+	writel(0, fsm->base + SPI_FAST_SEQ_CFG);
+
+	/* Set clock to 'safe' frequency initially */
+	stfsm_set_freq(fsm, STFSM_FLASH_SAFE_FREQ);
+
+	/* Switch to FSM */
+	ret = stfsm_set_mode(fsm, SPI_MODESELECT_FSM);
+	if (ret)
+		return ret;
+
+	/* Set timing parameters */
+	writel(SPI_CFG_DEVICE_ST            |
+	       SPI_CFG_DEFAULT_MIN_CS_HIGH  |
+	       SPI_CFG_DEFAULT_CS_SETUPHOLD |
+	       SPI_CFG_DEFAULT_DATA_HOLD,
+	       fsm->base + SPI_CONFIGDATA);
+	writel(STFSM_DEFAULT_WR_TIME, fsm->base + SPI_STATUS_WR_TIME_REG);
+
+	/* Clear FIFO, just in case */
+	stfsm_clear_fifo(fsm);
+
+	return 0;
+}
+
 static int stfsm_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct resource *res;
 	struct stfsm *fsm;
+	int ret;
 
 	if (!np) {
 		dev_err(&pdev->dev, "No DT found\n");
@@ -66,6 +176,12 @@ static int stfsm_probe(struct platform_device *pdev)
 
 	mutex_init(&fsm->lock);
 
+	ret = stfsm_init(fsm);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to initialise FSM Controller\n");
+		return ret;
+	}
+
 	platform_set_drvdata(pdev, fsm);
 
 	fsm->mtd.dev.parent	= &pdev->dev;
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index b420a43..4e92e58 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -69,6 +69,10 @@
 #define SPI_CFG_CS_SETUPHOLD(x)		(((x) & 0xff) << 16)
 #define SPI_CFG_DATA_HOLD(x)		(((x) & 0xff) << 24)
 
+#define SPI_CFG_DEFAULT_MIN_CS_HIGH	SPI_CFG_MIN_CS_HIGH(0x0AA)
+#define SPI_CFG_DEFAULT_CS_SETUPHOLD	SPI_CFG_CS_SETUPHOLD(0xA0)
+#define SPI_CFG_DEFAULT_DATA_HOLD	SPI_CFG_DATA_HOLD(0x00)
+
 /*
  * Register: SPI_FAST_SEQ_TRANSFER_SIZE
  */
@@ -180,12 +184,19 @@
 #define STFSM_INST_WAIT			STFSM_INSTR(STFSM_OPC_WAIT,	0)
 #define STFSM_INST_STOP			STFSM_INSTR(STFSM_OPC_STOP,	0)
 
+#define STFSM_DEFAULT_EMI_FREQ	100000000UL                        /* 100 MHz */
+#define STFSM_DEFAULT_WR_TIME	STFSM_DEFAULT_EMI_FREQ * (15/1000) /* 15ms */
+
+#define STFSM_FLASH_SAFE_FREQ	10000000UL                         /* 10 MHz */
+
 struct stfsm {
 	struct device		*dev;
 	void __iomem		*base;
 	struct resource		*region;
 	struct mtd_info		mtd;
 	struct mutex		lock;
+
+	uint32_t		fifo_dir_delay;
 };
 
 #endif	/* ST_SPI_FSM_H */
-- 
1.8.3.2

Re: [PATCH v3 03/36] mtd: st_spi_fsm: Initialise and configure the FSM for normal working conditions

[Brian Norris]

Hi Lee,

On Fri, Nov 29, 2013 at 12:18:52PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -25,11 +25,121 @@
>  
>  #include "st_spi_fsm.h"
>  
> +static inline uint32_t stfsm_fifo_available(struct stfsm *fsm)
> +{
> +	return (readl(fsm->base + SPI_FAST_SEQ_STA) >> 5) & 0x7f;
> +}
> +
> +static void stfsm_clear_fifo(struct stfsm *fsm)
> +{
> +	uint32_t avail;
> +
> +	for (;;) {
> +		avail = stfsm_fifo_available(fsm);
> +		if (!avail)
> +			break;
> +
> +		while (avail) {
> +			readl(fsm->base + SPI_FAST_SEQ_DATA_REG);
> +			avail--;
> +		}
> +	}
> +}
> +
> +static int stfsm_set_mode(struct stfsm *fsm, uint32_t mode)
> +{
> +	int ret, timeout = 10;
> +
> +	/* Wait for controller to accept mode change */
> +	while(--timeout) {

s/while(/while (/

You should run your series through scripts/checkpatch.pl. There are a
number of small things to fixup in a few of your patches.

> +		ret = readl(fsm->base + SPI_STA_MODE_CHANGE);
> +		if (ret & 0x1)
> +			break;
> +		udelay(1);
> +	}
> +
> +	if (!timeout)
> +		return -EBUSY;
> +
> +	writel(mode, fsm->base + SPI_MODESELECT);
> +
> +	return 0;
> +}
> +
> +static void stfsm_set_freq(struct stfsm *fsm, uint32_t spi_freq)
> +{
> +	uint32_t emi_freq;
> +	uint32_t clk_div;
> +
> +	/* TODO: Make this dynamic */
> +	emi_freq = STFSM_DEFAULT_EMI_FREQ;
> +
> +	/*
> +	 * Calculate clk_div - values between 2 and 128
> +	 * Multiple of 2, rounded up
> +	 */
> +	clk_div = 2*((emi_freq + (2*spi_freq - 1))/(2*spi_freq));

I think this can use the <linux/kerne.h> round_up() macro. Also, you're
missing some spacing. Possibly:

	clk_div = 2 * round_up(emi_freq, 2 * spi_freq);

> +	if (clk_div < 2)
> +		clk_div = 2;
> +	else if (clk_div > 128)
> +		clk_div = 128;
> +
> +	/*
> +	 * Determine a suitable delay for the IP to complete a change of
> +	 * direction of the FIFO. The required delay is related to the clock
> +	 * divider used. The following heuristics are based on empirical tests,
> +	 * using a 100MHz EMI clock.
> +	 */
> +	if (clk_div <= 4)
> +		fsm->fifo_dir_delay = 0;
> +	else if (clk_div <= 10)
> +		fsm->fifo_dir_delay = 1;
> +	else
> +		fsm->fifo_dir_delay = (clk_div + 9) / 10;

round_down()?

> +
> +	dev_dbg(fsm->dev, "emi_clk = %uHZ, spi_freq = %uHZ, clk_div = %u\n",
> +		emi_freq, spi_freq, clk_div);
> +
> +	writel(clk_div, fsm->base + SPI_CLOCKDIV);
> +}

Brian

Re: [PATCH v3 03/36] mtd: st_spi_fsm: Initialise and configure the FSM for normal working conditions

[Brian Norris]

On Tue, Dec 10, 2013 at 11:01:00AM -0800, Brian Norris wrote:
> On Fri, Nov 29, 2013 at 12:18:52PM +0000, Lee Jones wrote:
> > +	/*
> > +	 * Calculate clk_div - values between 2 and 128
> > +	 * Multiple of 2, rounded up
> > +	 */
> > +	clk_div = 2*((emi_freq + (2*spi_freq - 1))/(2*spi_freq));
> 
> I think this can use the <linux/kerne.h> round_up() macro. Also, you're
> missing some spacing. Possibly:
> 
> 	clk_div = 2 * round_up(emi_freq, 2 * spi_freq);

Sorry, I was reading one thing and writing another; this would be
DIV_ROUND_UP():

	clk_div = 2 * DIV_ROUND_UP(emi_freq, 2 * spi_freq);

> > +		fsm->fifo_dir_delay = (clk_div + 9) / 10;
> 
> round_down()?

Sorry, I again meant DIV_ROUND_UP:

		fsm->fifo_dir_delay = DIV_ROUND_UP(clk_div, 10);

Brian

[PATCH v3 04/36] mtd: st_spi_fsm: Supply framework for device requests

[Lee Jones]

The FSM hardware works by setting a predetermined sequence of register
writes. Rather than open coding them inside each functional block we're
going to define them in a series of formatted 'sequence structures'.
This patch provides the framework which shall be used for every action.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h | 14 ++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index fe96b47..8ac8cd9 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -25,6 +25,11 @@
 
 #include "st_spi_fsm.h"
 
+static inline int stfsm_is_idle(struct stfsm *fsm)
+{
+	return readl(fsm->base + SPI_FAST_SEQ_STA) & 0x10;
+}
+
 static inline uint32_t stfsm_fifo_available(struct stfsm *fsm)
 {
 	return (readl(fsm->base + SPI_FAST_SEQ_STA) >> 5) & 0x7f;
@@ -46,6 +51,36 @@ static void stfsm_clear_fifo(struct stfsm *fsm)
 	}
 }
 
+static inline void stfsm_load_seq(struct stfsm *fsm,
+				  const struct stfsm_seq *seq)
+{
+	void __iomem *dst = fsm->base + SPI_FAST_SEQ_TRANSFER_SIZE;
+	const uint32_t *src = (const uint32_t *)seq;
+	int words = STFSM_SEQ_SIZE / sizeof(uint32_t);
+
+	BUG_ON(!stfsm_is_idle(fsm));
+
+	while (words--) {
+		writel(*src, dst);
+		src++;
+		dst += 4;
+	}
+}
+
+static void stfsm_wait_seq(struct stfsm *fsm)
+{
+	unsigned long timeo = jiffies + HZ;
+
+	while (time_before(jiffies, timeo)) {
+		if (stfsm_is_idle(fsm))
+			return;
+
+		cond_resched();
+	}
+
+	dev_err(fsm->dev, "timeout on sequence completion\n");
+}
+
 static int stfsm_set_mode(struct stfsm *fsm, uint32_t mode)
 {
 	int ret, timeout = 10;
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 4e92e58..6164142 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -199,4 +199,18 @@ struct stfsm {
 	uint32_t		fifo_dir_delay;
 };
 
+struct stfsm_seq {
+	uint32_t data_size;
+	uint32_t addr1;
+	uint32_t addr2;
+	uint32_t addr_cfg;
+	uint32_t seq_opc[5];
+	uint32_t mode;
+	uint32_t dummy;
+	uint32_t status;
+	uint8_t  seq[16];
+	uint32_t seq_cfg;
+} __attribute__((__packed__, aligned(4)));
+#define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
+
 #endif	/* ST_SPI_FSM_H */
-- 
1.8.3.2

Re: [PATCH v3 04/36] mtd: st_spi_fsm: Supply framework for device requests

[Brian Norris]

On Fri, Nov 29, 2013 at 12:18:53PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -46,6 +51,36 @@ static void stfsm_clear_fifo(struct stfsm *fsm)
>  	}
>  }
>  
> +static inline void stfsm_load_seq(struct stfsm *fsm,
> +				  const struct stfsm_seq *seq)
> +{
> +	void __iomem *dst = fsm->base + SPI_FAST_SEQ_TRANSFER_SIZE;
> +	const uint32_t *src = (const uint32_t *)seq;
> +	int words = STFSM_SEQ_SIZE / sizeof(uint32_t);

I think this is clearer as:

	int words = sizeof(*seq) / sizeof(*src);

> +
> +	BUG_ON(!stfsm_is_idle(fsm));
> +
> +	while (words--) {
> +		writel(*src, dst);
> +		src++;
> +		dst += 4;
> +	}
> +}
> +
> +static void stfsm_wait_seq(struct stfsm *fsm)
> +{
> +	unsigned long timeo = jiffies + HZ;
> +
> +	while (time_before(jiffies, timeo)) {
> +		if (stfsm_is_idle(fsm))
> +			return;
> +
> +		cond_resched();
> +	}
> +
> +	dev_err(fsm->dev, "timeout on sequence completion\n");

I believe the timeout logic is incorrect. What if we wait a "long time"
to call stfsm_wait_seq() (due to scheduling, or otherwise)? Then the
while loop might not even run once (time_before(x, y) is false). Or what
if cond_resched() waits for a long time...

So you need an extra check of stfsm_is_idle() after the while loop,
before you declare a timeout.

> +}
> +
>  static int stfsm_set_mode(struct stfsm *fsm, uint32_t mode)
>  {
>  	int ret, timeout = 10;
> diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
> index 4e92e58..6164142 100644
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -199,4 +199,18 @@ struct stfsm {
>  	uint32_t		fifo_dir_delay;
>  };
>  
> +struct stfsm_seq {
> +	uint32_t data_size;
> +	uint32_t addr1;
> +	uint32_t addr2;
> +	uint32_t addr_cfg;
> +	uint32_t seq_opc[5];
> +	uint32_t mode;
> +	uint32_t dummy;
> +	uint32_t status;
> +	uint8_t  seq[16];
> +	uint32_t seq_cfg;
> +} __attribute__((__packed__, aligned(4)));

I think checkpatch.pl prefers these attributes use the kernel macros:

  struct stfsm_seq {
  ...
  } __packed __aligned(4);

> +#define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)

If you agree with my earlier comment, you won't need this macro.

> +
>  #endif	/* ST_SPI_FSM_H */

Brian

Re: [PATCH v3 04/36] mtd: st_spi_fsm: Supply framework for device requests

[Angus Clark]

On 12/10/2013 08:19 PM, Brian Norris wrote:
> On Fri, Nov 29, 2013 at 12:18:53PM +0000, Lee Jones wrote:
>> --- a/drivers/mtd/devices/st_spi_fsm.c
>> +static void stfsm_wait_seq(struct stfsm *fsm)
>> +{
>> +	unsigned long timeo = jiffies + HZ;
>> +
>> +	while (time_before(jiffies, timeo)) {
>> +		if (stfsm_is_idle(fsm))
>> +			return;
>> +
>> +		cond_resched();
>> +	}
>> +
>> +	dev_err(fsm->dev, "timeout on sequence completion\n");
> 
> I believe the timeout logic is incorrect. What if we wait a "long time"
> to call stfsm_wait_seq() (due to scheduling, or otherwise)? Then the
> while loop might not even run once (time_before(x, y) is false). Or what
> if cond_resched() waits for a long time...
> 
> So you need an extra check of stfsm_is_idle() after the while loop,
> before you declare a timeout.

Yes, good catch, this needs to updated.

Cheers,

Angus

[PATCH v3 05/36] mtd: st_spi_fsm: Supply a method to read from the FSM's FIFO

[Lee Jones]

When invoked the driver will attempt to read any available data from
the FSM's data register. Any data collected from this FIFO would have
originated from the flash chip.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 8ac8cd9..05fa834 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -81,6 +81,32 @@ static void stfsm_wait_seq(struct stfsm *fsm)
 	dev_err(fsm->dev, "timeout on sequence completion\n");
 }
 
+static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
+			    const uint32_t size)
+{
+	uint32_t remaining = size >> 2;
+	uint32_t avail;
+	uint32_t words;
+
+	dev_dbg(fsm->dev, "Reading %d bytes from FIFO\n", size);
+
+	BUG_ON((((uint32_t)buf) & 0x3) || (size & 0x3));
+
+	while (remaining) {
+		for (;;) {
+			avail = stfsm_fifo_available(fsm);
+			if (avail)
+				break;
+			udelay(1);
+		}
+		words = min(avail, remaining);
+		remaining -= words;
+
+		readsl(fsm->base + SPI_FAST_SEQ_DATA_REG, buf, words);
+		buf += words;
+	}
+}
+
 static int stfsm_set_mode(struct stfsm *fsm, uint32_t mode)
 {
 	int ret, timeout = 10;
-- 
1.8.3.2

[PATCH v3 06/36] mtd: st_spi_fsm: Supply defines for the possible flash command opcodes

[Lee Jones]

Flash chip commands are issued using a set of predefined opcodes. These
are mostly the same for all flash devices, but do differ on occasion.
This patch supplies the majority of the key ones which will be used in
this driver.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 6164142..97b4868 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -189,6 +189,44 @@
 
 #define STFSM_FLASH_SAFE_FREQ	10000000UL                         /* 10 MHz */
 
+/* Flash Commands */
+#define FLASH_CMD_WREN		0x06
+#define FLASH_CMD_WRDI		0x04
+#define FLASH_CMD_RDID		0x9f
+#define FLASH_CMD_RDSR		0x05
+#define FLASH_CMD_RDSR2		0x35
+#define FLASH_CMD_WRSR		0x01
+#define FLASH_CMD_SE_4K		0x20
+#define FLASH_CMD_SE_32K	0x52
+#define FLASH_CMD_SE		0xd8
+#define FLASH_CMD_CHIPERASE	0xc7
+#define FLASH_CMD_WRVCR		0x81
+#define FLASH_CMD_RDVCR		0x85
+
+#define FLASH_CMD_READ		0x03	/* READ */
+#define FLASH_CMD_READ_FAST	0x0b	/* FAST READ */
+#define FLASH_CMD_READ_1_1_2	0x3b	/* DUAL OUTPUT READ */
+#define FLASH_CMD_READ_1_2_2	0xbb	/* DUAL I/O READ */
+#define FLASH_CMD_READ_1_1_4	0x6b	/* QUAD OUTPUT READ */
+#define FLASH_CMD_READ_1_4_4	0xeb	/* QUAD I/O READ */
+
+#define FLASH_CMD_WRITE		0x02	/* PAGE PROGRAM */
+#define FLASH_CMD_WRITE_1_1_2	0xa2	/* DUAL INPUT PROGRAM */
+#define FLASH_CMD_WRITE_1_2_2	0xd2	/* DUAL INPUT EXT PROGRAM */
+#define FLASH_CMD_WRITE_1_1_4	0x32	/* QUAD INPUT PROGRAM */
+#define FLASH_CMD_WRITE_1_4_4	0x12	/* QUAD INPUT EXT PROGRAM */
+
+#define FLASH_CMD_EN4B_ADDR	0xb7	/* Enter 4-byte address mode */
+#define FLASH_CMD_EX4B_ADDR	0xe9	/* Exit 4-byte address mode */
+
+/* READ commands with 32-bit addressing (N25Q256 and S25FLxxxS) */
+#define FLASH_CMD_READ4		0x13
+#define FLASH_CMD_READ4_FAST	0x0c
+#define FLASH_CMD_READ4_1_1_2	0x3c
+#define FLASH_CMD_READ4_1_2_2	0xbc
+#define FLASH_CMD_READ4_1_1_4	0x6c
+#define FLASH_CMD_READ4_1_4_4	0xec
+
 struct stfsm {
 	struct device		*dev;
 	void __iomem		*base;
-- 
1.8.3.2

[PATCH v3 07/36] mtd: st_spi_fsm: Add support for JEDEC ID extraction

[Lee Jones]

Once we start supporting devices it will be handy go detect them
dynamically. This will be done using the chip's unique JEDEC ID. This
patch allows us to extract a device's JEDEC ID using the a predefined
FSM register write sequence.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 55 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 05fa834..fe19ec3 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -25,6 +25,22 @@
 
 #include "st_spi_fsm.h"
 
+static struct stfsm_seq stfsm_seq_read_jedec = {
+	.data_size = TRANSFER_SIZE(8),
+	.seq_opc[0] = (SEQ_OPC_PADS_1 |
+		       SEQ_OPC_CYCLES(8) |
+		       SEQ_OPC_OPCODE(FLASH_CMD_RDID)),
+	.seq = {
+		STFSM_INST_CMD1,
+		STFSM_INST_DATA_READ,
+		STFSM_INST_STOP,
+	},
+	.seq_cfg = (SEQ_CFG_PADS_1 |
+		    SEQ_CFG_READNOTWRITE |
+		    SEQ_CFG_CSDEASSERT |
+		    SEQ_CFG_STARTSEQ),
+};
+
 static inline int stfsm_is_idle(struct stfsm *fsm)
 {
 	return readl(fsm->base + SPI_FAST_SEQ_STA) & 0x10;
@@ -107,6 +123,42 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
 	}
 }
 
+static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
+{
+	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
+	uint32_t tmp[2];
+
+	stfsm_load_seq(fsm, seq);
+
+	stfsm_read_fifo(fsm, tmp, 8);
+
+	memcpy(jedec, tmp, 5);
+
+	stfsm_wait_seq(fsm);
+}
+
+static struct flash_info * stfsm_jedec_probe(struct stfsm *fsm)
+{
+	u16                     ext_jedec;
+	u32			jedec;
+	u8			id[5];
+
+	stfsm_read_jedec(fsm, id);
+
+	jedec     = id[0] << 16 | id[1] << 8 | id[2];
+	/*
+	 * JEDEC also defines an optional "extended device information"
+	 * string for after vendor-specific data, after the three bytes
+	 * we use here. Supporting some chips might require using it.
+	 */
+	ext_jedec = id[3] << 8  | id[4];
+
+	dev_dbg(fsm->dev, "JEDEC =  0x%08x [%02x %02x %02x %02x %02x]\n",
+		jedec, id[0], id[1], id[2], id[3], id[4]);
+
+	return NULL;
+}
+
 static int stfsm_set_mode(struct stfsm *fsm, uint32_t mode)
 {
 	int ret, timeout = 10;
@@ -243,6 +295,9 @@ static int stfsm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Detect SPI FLASH device */
+	stfsm_jedec_probe(fsm);
+
 	platform_set_drvdata(pdev, fsm);
 
 	fsm->mtd.dev.parent	= &pdev->dev;
-- 
1.8.3.2

[PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands

[Lee Jones]

JEDEC have helped to standardise a great deal of the commands which
can be issued to a Serial Flash devices. Many of the OPCODEs and all
of the Serial Flash Discoverable Parameters (SFDP) commands are
generic across devices. This patch provides a shared point where
these commands can be defined.

Suggested-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/serial_flash_cmds.h | 91 +++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.c        |  1 +
 drivers/mtd/devices/st_spi_fsm.h        |  2 +
 3 files changed, 94 insertions(+)
 create mode 100644 drivers/mtd/devices/serial_flash_cmds.h

diff --git a/drivers/mtd/devices/serial_flash_cmds.h b/drivers/mtd/devices/serial_flash_cmds.h
new file mode 100644
index 0000000..62aa420
--- /dev/null
+++ b/drivers/mtd/devices/serial_flash_cmds.h
@@ -0,0 +1,91 @@
+/*
+ * Generic/SFDP Flash Commands and Device Capabilities
+ *
+ * Copyright (C) 2013 Lee Jones <lee.jones@lianro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#ifndef _MTD_SERIAL_FLASH_CMDS_H
+#define _MTD_SERIAL_FLASH_CMDS_H
+
+/* Generic Flash Commands/OPCODEs */
+#define FLASH_CMD_WREN		0x06
+#define FLASH_CMD_WRDI		0x04
+#define FLASH_CMD_RDID		0x9f
+#define FLASH_CMD_RDSR		0x05
+#define FLASH_CMD_RDSR2		0x35
+#define FLASH_CMD_WRSR		0x01
+#define FLASH_CMD_SE_4K		0x20
+#define FLASH_CMD_SE_32K	0x52
+#define FLASH_CMD_SE		0xd8
+#define FLASH_CMD_CHIPERASE	0xc7
+#define FLASH_CMD_WRVCR		0x81
+#define FLASH_CMD_RDVCR		0x85
+
+/* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */
+#define FLASH_CMD_READ		0x03	/* READ */
+#define FLASH_CMD_READ_FAST	0x0b	/* FAST READ */
+#define FLASH_CMD_READ_1_1_2	0x3b	/* DUAL OUTPUT READ */
+#define FLASH_CMD_READ_1_2_2	0xbb	/* DUAL I/O READ */
+#define FLASH_CMD_READ_1_1_4	0x6b	/* QUAD OUTPUT READ */
+#define FLASH_CMD_READ_1_4_4	0xeb	/* QUAD I/O READ */
+
+#define FLASH_CMD_WRITE		0x02	/* PAGE PROGRAM */
+#define FLASH_CMD_WRITE_1_1_2	0xa2	/* DUAL INPUT PROGRAM */
+#define FLASH_CMD_WRITE_1_2_2	0xd2	/* DUAL INPUT EXT PROGRAM */
+#define FLASH_CMD_WRITE_1_1_4	0x32	/* QUAD INPUT PROGRAM */
+#define FLASH_CMD_WRITE_1_4_4	0x12	/* QUAD INPUT EXT PROGRAM */
+
+#define FLASH_CMD_EN4B_ADDR	0xb7	/* Enter 4-byte address mode */
+#define FLASH_CMD_EX4B_ADDR	0xe9	/* Exit 4-byte address mode */
+
+/* READ commands with 32-bit addressing */
+#define FLASH_CMD_READ4		0x13
+#define FLASH_CMD_READ4_FAST	0x0c
+#define FLASH_CMD_READ4_1_1_2	0x3c
+#define FLASH_CMD_READ4_1_2_2	0xbc
+#define FLASH_CMD_READ4_1_1_4	0x6c
+#define FLASH_CMD_READ4_1_4_4	0xec
+
+/* Configuration flags */
+#define FLASH_FLAG_SINGLE	0x000000ff
+#define FLASH_FLAG_READ_WRITE	0x00000001
+#define FLASH_FLAG_READ_FAST	0x00000002
+#define FLASH_FLAG_SE_4K	0x00000004
+#define FLASH_FLAG_SE_32K	0x00000008
+#define FLASH_FLAG_CE		0x00000010
+#define FLASH_FLAG_32BIT_ADDR	0x00000020
+#define FLASH_FLAG_RESET	0x00000040
+#define FLASH_FLAG_DYB_LOCKING	0x00000080
+
+#define FLASH_FLAG_DUAL		0x0000ff00
+#define FLASH_FLAG_READ_1_1_2	0x00000100
+#define FLASH_FLAG_READ_1_2_2	0x00000200
+#define FLASH_FLAG_READ_2_2_2	0x00000400
+#define FLASH_FLAG_WRITE_1_1_2	0x00001000
+#define FLASH_FLAG_WRITE_1_2_2	0x00002000
+#define FLASH_FLAG_WRITE_2_2_2	0x00004000
+
+#define FLASH_FLAG_QUAD		0x00ff0000
+#define FLASH_FLAG_READ_1_1_4	0x00010000
+#define FLASH_FLAG_READ_1_4_4	0x00020000
+#define FLASH_FLAG_READ_4_4_4	0x00040000
+#define FLASH_FLAG_WRITE_1_1_4	0x00100000
+#define FLASH_FLAG_WRITE_1_4_4	0x00200000
+#define FLASH_FLAG_WRITE_4_4_4	0x00400000
+
+#endif /* _MTD_SERIAL_FLASH_CMDS_H */
diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index fe19ec3..71b6188 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -24,6 +24,7 @@
 #include <asm/io.h>
 
 #include "st_spi_fsm.h"
+#include "serial_flash_cmds.h"
 
 static struct stfsm_seq stfsm_seq_read_jedec = {
 	.data_size = TRANSFER_SIZE(8),
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 97b4868..5c6c55e 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -16,6 +16,8 @@
 #ifndef ST_SPI_FSM_H
 #define ST_SPI_FSM_H
 
+#include "serial_flash_cmds.h"
+
 /*
  * FSM SPI Controller Registers
  */
-- 
1.8.3.2

Re: [PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands

[Brian Norris]

On Fri, Nov 29, 2013 at 12:18:57PM +0000, Lee Jones wrote:
> JEDEC have helped to standardise a great deal of the commands which
> can be issued to a Serial Flash devices. Many of the OPCODEs and all
> of the Serial Flash Discoverable Parameters (SFDP) commands are
> generic across devices. This patch provides a shared point where
> these commands can be defined.

I think this is probably a good start for a shared header. In the near
term, we need to settle on one header that can store commands, at least.
I think both you and Huang are submitting patches that do similar opcode
relocation. Let's make sure we get the separation right, so that this
header contains exactly as much of the opcode-related stuff that can be
easily shared and reused.

> Suggested-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mtd/devices/serial_flash_cmds.h | 91 +++++++++++++++++++++++++++++++++
>  drivers/mtd/devices/st_spi_fsm.c        |  1 +
>  drivers/mtd/devices/st_spi_fsm.h        |  2 +
>  3 files changed, 94 insertions(+)
>  create mode 100644 drivers/mtd/devices/serial_flash_cmds.h
> 
> diff --git a/drivers/mtd/devices/serial_flash_cmds.h b/drivers/mtd/devices/serial_flash_cmds.h
> new file mode 100644
> index 0000000..62aa420
> --- /dev/null
> +++ b/drivers/mtd/devices/serial_flash_cmds.h
> @@ -0,0 +1,91 @@
> +/*
> + * Generic/SFDP Flash Commands and Device Capabilities
> + *
> + * Copyright (C) 2013 Lee Jones <lee.jones@lianro.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

BTW, I think checkpatch.pl complains about putting the FSF address here.

> +/* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */
> +#define FLASH_CMD_READ		0x03	/* READ */
> +#define FLASH_CMD_READ_FAST	0x0b	/* FAST READ */
> +#define FLASH_CMD_READ_1_1_2	0x3b	/* DUAL OUTPUT READ */
> +#define FLASH_CMD_READ_1_2_2	0xbb	/* DUAL I/O READ */
> +#define FLASH_CMD_READ_1_1_4	0x6b	/* QUAD OUTPUT READ */
> +#define FLASH_CMD_READ_1_4_4	0xeb	/* QUAD I/O READ */

All of these {1,2,4}_{1,2,4}_{1,2,4} suffixes are a little cryptic; I
ended up referring to the SFDP spec to figure out what the first number
meant. Can you document them briefly at the top of this SFDP list?

> +#define FLASH_CMD_WRITE		0x02	/* PAGE PROGRAM */
> +#define FLASH_CMD_WRITE_1_1_2	0xa2	/* DUAL INPUT PROGRAM */
> +#define FLASH_CMD_WRITE_1_2_2	0xd2	/* DUAL INPUT EXT PROGRAM */
> +#define FLASH_CMD_WRITE_1_1_4	0x32	/* QUAD INPUT PROGRAM */
> +#define FLASH_CMD_WRITE_1_4_4	0x12	/* QUAD INPUT EXT PROGRAM */
> +
> +#define FLASH_CMD_EN4B_ADDR	0xb7	/* Enter 4-byte address mode */
> +#define FLASH_CMD_EX4B_ADDR	0xe9	/* Exit 4-byte address mode */
> +
> +/* READ commands with 32-bit addressing */
> +#define FLASH_CMD_READ4		0x13
> +#define FLASH_CMD_READ4_FAST	0x0c
> +#define FLASH_CMD_READ4_1_1_2	0x3c
> +#define FLASH_CMD_READ4_1_2_2	0xbc
> +#define FLASH_CMD_READ4_1_1_4	0x6c
> +#define FLASH_CMD_READ4_1_4_4	0xec

Since you list these, does SFDP even describe the 32-bit addressing
commands? It seems like it assumes the device is switched (statefully)
between 24-bit and 32-bit address modes (or kept permanently in one or
the other).

> +/* Configuration flags */
> +#define FLASH_FLAG_SINGLE	0x000000ff
> +#define FLASH_FLAG_READ_WRITE	0x00000001
> +#define FLASH_FLAG_READ_FAST	0x00000002
> +#define FLASH_FLAG_SE_4K	0x00000004
> +#define FLASH_FLAG_SE_32K	0x00000008
> +#define FLASH_FLAG_CE		0x00000010
> +#define FLASH_FLAG_32BIT_ADDR	0x00000020
> +#define FLASH_FLAG_RESET	0x00000040
> +#define FLASH_FLAG_DYB_LOCKING	0x00000080
> +
> +#define FLASH_FLAG_DUAL		0x0000ff00
> +#define FLASH_FLAG_READ_1_1_2	0x00000100
> +#define FLASH_FLAG_READ_1_2_2	0x00000200
> +#define FLASH_FLAG_READ_2_2_2	0x00000400
> +#define FLASH_FLAG_WRITE_1_1_2	0x00001000
> +#define FLASH_FLAG_WRITE_1_2_2	0x00002000
> +#define FLASH_FLAG_WRITE_2_2_2	0x00004000
> +
> +#define FLASH_FLAG_QUAD		0x00ff0000
> +#define FLASH_FLAG_READ_1_1_4	0x00010000
> +#define FLASH_FLAG_READ_1_4_4	0x00020000
> +#define FLASH_FLAG_READ_4_4_4	0x00040000
> +#define FLASH_FLAG_WRITE_1_1_4	0x00100000
> +#define FLASH_FLAG_WRITE_1_4_4	0x00200000
> +#define FLASH_FLAG_WRITE_4_4_4	0x00400000

It doesn't look to me like the dual/quad bitfields are laid out very
usefully. Can't they be divided so that their bit position is more
meaningful? Like reserve bits [8:23] for dual/quad descriptions, then
give the following:

 [8:11]  number of pins for the opcode (1=single; 2=dual; 4=quad)
 [12:15] number of pins for the address (1=single; 2=dual; ...)
 [16:19] number of pins for the data
 [20]    read or write opcode (0=read; 1=write)

(you could easily pack these differently, but you get the idea)

Then:

#define FLASH_FLAG_DUAL_MASK	0x00022200
#define FLASH_FLAG_QUAD_MASK	0x00044400

And we could do things like:

#define FLASH_FLAG_OPCODE_PINS(x)	(((x) & 0x00000f00) >> 8)
#define FLASH_FLAG_ADDR_PINS(x)		(((x) & 0x0000f000) >> 12)
#define FLASH_FLAG_DATA_PINS(x)		(((x) & 0x000f0000) >> 16)

(and maybe replace those mask/shifts with macro names)

This could eliminate some redundancy in your tables, I think, so that
you can extract (from this flag) the number of "addr_pads" and
"data_pads" needed for the opcode. And I think this would be useful to
other controllers eventually. For instance, my quad-read driver might
support 4 data pins but it can't put the opcode or address out on more
than 1 pin.

Brian

Re: [PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands

[Brian Norris]

On Tue, Dec 10, 2013 at 01:48:49PM -0800, Brian Norris wrote:
> It doesn't look to me like the dual/quad bitfields are laid out very
> usefully. Can't they be divided so that their bit position is more
> meaningful? Like reserve bits [8:23] for dual/quad descriptions, then
> give the following:
> 
>  [8:11]  number of pins for the opcode (1=single; 2=dual; 4=quad)
>  [12:15] number of pins for the address (1=single; 2=dual; ...)
>  [16:19] number of pins for the data
>  [20]    read or write opcode (0=read; 1=write)
> 
> (you could easily pack these differently, but you get the idea)
> 
> Then:
> 
> #define FLASH_FLAG_DUAL_MASK	0x00022200
> #define FLASH_FLAG_QUAD_MASK	0x00044400
> 
> And we could do things like:
> 
> #define FLASH_FLAG_OPCODE_PINS(x)	(((x) & 0x00000f00) >> 8)
> #define FLASH_FLAG_ADDR_PINS(x)		(((x) & 0x0000f000) >> 12)
> #define FLASH_FLAG_DATA_PINS(x)		(((x) & 0x000f0000) >> 16)
> 
> (and maybe replace those mask/shifts with macro names)

I realized you are packing these tight because you're using them as
combinable (bitwise OR) flags (hence the name FLASH_FLAG_*, duh!). So
while my scheme is nicer for representing a single opcode w.r.t.
controller requirements, it is not able to represent the exact opcodes
supported by a particular flash. Hence, this is not the right place for
it.

But I don't want to imitate your flags in any generic framework; we need
a method (that doesn't involve too many extra tables like in your
driver; or that supports these tables generically, to share with other
drivers) to map the "supported opcodes" flags to useful properties that
the controller/driver can examine.

Is it possible, for instance, to describe a range of opcodes supported
using patterns? Like "this flash supports opcodes for 1-4 data pins and
1-2 address pins"? Or am I being too idealistic, and most flash really
support a totally arbitrary combination of opcodes?

Brian

Re: [PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands

[Angus Clark]

On 12/10/2013 10:23 PM, Brian Norris wrote:
> On Tue, Dec 10, 2013 at 01:48:49PM -0800, Brian Norris wrote:
>> It doesn't look to me like the dual/quad bitfields are laid out very
>> usefully. Can't they be divided so that their bit position is more
>> meaningful?
[...]
> I realized you are packing these tight because you're using them as
> combinable (bitwise OR) flags (hence the name FLASH_FLAG_*, duh!). So
> while my scheme is nicer for representing a single opcode w.r.t.
> controller requirements, it is not able to represent the exact opcodes
> supported by a particular flash. Hence, this is not the right place for
> it.

There is certainly scope to compact the opcode representation...

> But I don't want to imitate your flags in any generic framework; we need
> a method (that doesn't involve too many extra tables like in your
> driver; or that supports these tables generically, to share with other
> drivers) to map the "supported opcodes" flags to useful properties that
> the controller/driver can examine.

I believe a key function of any spi-nor framework should be to determine which
fundamental modes of operation are supported by the device (e.g. READ_1_1_2,
READ_1_2_2, READ_1_1_4, READ_1_4_4 etc) and how they are driven (i.e. opcode,
number of mode bits, number of dummy cycles).  A second stage would be to
configure the best read, write, erase configurations based on the combined
capabilities of the device and the H/W controller.

However, it is not obvious how best to achieve this functionality.  Given the
amount of information that needs to be represented, a static table is not going
to be popular.  The current approach in st_spi_fsm assumes some default
configurations for supported modes, with the option to override with
device/family-specific configurations.  To be honest, it is rather ugly, and the
result of historic evolutions rather than a clean design!

> Is it possible, for instance, to describe a range of opcodes supported
> using patterns? Like "this flash supports opcodes for 1-4 data pins and
> 1-2 address pins"? Or am I being too idealistic, and most flash really
> support a totally arbitrary combination of opcodes?

Yes, I am afraid so.  For example, the mx25l12836e supports READ_1_1_2(0x3b) and
READ_1_1_4(0x6b), whereas the mx25l12845e supports READ_1_2_2(0xbb) and
READ_1_4_4(0xeb).

Cheers,

Angus

Re: [PATCH v3 08/36] mtd: devices: Provide header for shared OPCODEs and SFDP commands

[Angus Clark]

On 12/10/2013 09:48 PM, Brian Norris wrote:
> On Fri, Nov 29, 2013 at 12:18:57PM +0000, Lee Jones wrote:
>> JEDEC have helped to standardise a great deal of the commands which
>> can be issued to a Serial Flash devices. Many of the OPCODEs and all
>> of the Serial Flash Discoverable Parameters (SFDP) commands are
>> generic across devices. This patch provides a shared point where
>> these commands can be defined.

This comment isn't entirely correct.  The SFDP standard (JESD216A) does not go
as far as standardising the OPCODES, but merely a way of determining at runtime
which operations are supported (e.g. READ_1_1_4, READ_1_4_4), and the associated
opcodes; vendors are still free to use a different opcode for a particular
operation.

>> +/* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */
>> +#define FLASH_CMD_READ		0x03	/* READ */
>> +#define FLASH_CMD_READ_FAST	0x0b	/* FAST READ */
>> +#define FLASH_CMD_READ_1_1_2	0x3b	/* DUAL OUTPUT READ */
>> +#define FLASH_CMD_READ_1_2_2	0xbb	/* DUAL I/O READ */
>> +#define FLASH_CMD_READ_1_1_4	0x6b	/* QUAD OUTPUT READ */
>> +#define FLASH_CMD_READ_1_4_4	0xeb	/* QUAD I/O READ */
> 
> All of these {1,2,4}_{1,2,4}_{1,2,4} suffixes are a little cryptic; I
> ended up referring to the SFDP spec to figure out what the first number
> meant. Can you document them briefly at the top of this SFDP list?

I think the {x,y,z} descriptions offer the simplest way of representing the
myriad of operations supported by various devices -- it was one thing SFDP did
get right.  However, I agree, the nomenclature  does need explained.  We also
need a way of representing 32-bit address commands, and SDR vs DDR commands --
this has not yet been addresses by SFDP.

> Since you list these, does SFDP even describe the 32-bit addressing
> commands? 

Not yet, but I believe "JESD216B" will include a parameter table related to
32-bit address instructions.

> It seems like it assumes the device is switched (statefully)
> between 24-bit and 32-bit address modes (or kept permanently in one or
> the other).

And the complexity increases!  The st_spi_fsm H/W also supports a memory-mapped
mode for booting.  However, the boot controller is hard-coded to issue 24-bit
address cycles.  If the Serial Flash device includes a dedicated REST pin, and
this is appropriately routed to the system reset, then we have no issue -- the
device will reset to its power-on state following a warm reset.  If a reset pin
is not available, then certain precautions can be followed to minimise problems
with warm resets.   If the Serial Flash device supports a 32-bit instruction
set, then this should be used in preference, and we can avoid any statefull
transitions.  Failing that, we can minimise the window in which a reset would
fail by issuing EN4B/EX4B before/after each operation.  Of course, if we know
the system is not subject to these constraints, then we just need to issue EN4B
at the start, and stay in 4B mode.

All this knowledge needs to be in the spi-nor layer...

[PATCH v3 09/36] mtd: st_spi_fsm: Provide device look-up table

[Lee Jones]

Supply a lookup table of all the devices we intend to support. This table
is used to store device information such as; a human readable device name,
their JEDEC ID (plus the extended version), sector size and amount, a bit
store of a device's capabilities, its maximum running frequency and
possible use of a per-device configuration call-back.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.h | 137 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 137 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 5c6c55e..fd1472e 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -253,4 +253,141 @@ struct stfsm_seq {
 } __attribute__((__packed__, aligned(4)));
 #define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
 
+/* SPI Flash Device Table */
+struct flash_info {
+	char		*name;
+	/*
+	 * JEDEC id zero means "no ID" (most older chips); otherwise it has
+	 * a high byte of zero plus three data bytes: the manufacturer id,
+	 * then a two byte device id.
+	 */
+	u32		jedec_id;
+	u16             ext_id;
+	/*
+	 * The size listed here is what works with FLASH_CMD_SE, which isn't
+	 * necessarily called a "sector" by the vendor.
+	 */
+	unsigned	sector_size;
+	u16		n_sectors;
+	u32		flags;
+	/*
+	 * Note, where FAST_READ is supported, freq_max specifies the
+	 * FAST_READ frequency, not the READ frequency.
+	 */
+	u32		max_freq;
+	int		(*config)(struct stfsm *);
+};
+
+static struct flash_info flash_types[] = {
+	/*
+	 * ST Microelectronics/Numonyx --
+	 * (newer production versions may have feature updates
+	 * (eg faster operating frequency)
+	 */
+#define M25P_FLAG (FLASH_FLAG_READ_WRITE | FLASH_FLAG_READ_FAST)
+	{ "m25p40",  0x202013, 0,  64 * 1024,   8, M25P_FLAG, 25, NULL },
+	{ "m25p80",  0x202014, 0,  64 * 1024,  16, M25P_FLAG, 25, NULL },
+	{ "m25p16",  0x202015, 0,  64 * 1024,  32, M25P_FLAG, 25, NULL },
+	{ "m25p32",  0x202016, 0,  64 * 1024,  64, M25P_FLAG, 50, NULL },
+	{ "m25p64",  0x202017, 0,  64 * 1024, 128, M25P_FLAG, 50, NULL },
+	{ "m25p128", 0x202018, 0, 256 * 1024,  64, M25P_FLAG, 50, NULL },
+
+#define M25PX_FLAG (FLASH_FLAG_READ_WRITE	| \
+		    FLASH_FLAG_READ_FAST	| \
+		    FLASH_FLAG_READ_1_1_2	| \
+		    FLASH_FLAG_WRITE_1_1_2)
+	{ "m25px32", 0x207116, 0,  64 * 1024,  64, M25PX_FLAG, 75, NULL },
+	{ "m25px64", 0x207117, 0,  64 * 1024, 128, M25PX_FLAG, 75, NULL },
+
+#define MX25_FLAG (FLASH_FLAG_READ_WRITE	| \
+		   FLASH_FLAG_READ_FAST		| \
+		   FLASH_FLAG_READ_1_1_2	| \
+		   FLASH_FLAG_READ_1_2_2	| \
+		   FLASH_FLAG_READ_1_1_4	| \
+		   FLASH_FLAG_READ_1_4_4	| \
+		   FLASH_FLAG_WRITE_1_4_4	| \
+		   FLASH_FLAG_SE_4K		| \
+		   FLASH_FLAG_SE_32K)
+	{ "mx25l25635e", 0xc22019, 0, 64*1024, 512,
+	  (MX25_FLAG | FLASH_FLAG_32BIT_ADDR | FLASH_FLAG_RESET), 70, NULL },
+
+#define N25Q_FLAG (FLASH_FLAG_READ_WRITE	| \
+		   FLASH_FLAG_READ_FAST		| \
+		   FLASH_FLAG_READ_1_1_2	| \
+		   FLASH_FLAG_READ_1_2_2	| \
+		   FLASH_FLAG_READ_1_1_4	| \
+		   FLASH_FLAG_READ_1_4_4	| \
+		   FLASH_FLAG_WRITE_1_1_2	| \
+		   FLASH_FLAG_WRITE_1_2_2	| \
+		   FLASH_FLAG_WRITE_1_1_4	| \
+		   FLASH_FLAG_WRITE_1_4_4)
+	{ "n25q128", 0x20ba18, 0, 64 * 1024,  256, N25Q_FLAG, 108, NULL },
+	{ "n25q256", 0x20ba19, 0, 64 * 1024,  512,
+	  N25Q_FLAG | FLASH_FLAG_32BIT_ADDR, 108, NULL },
+
+	/*
+	 * Spansion S25FLxxxP
+	 *     - 256KiB and 64KiB sector variants (identified by ext. JEDEC)
+	 */
+#define S25FLXXXP_FLAG (FLASH_FLAG_READ_WRITE	| \
+			FLASH_FLAG_READ_1_1_2	| \
+			FLASH_FLAG_READ_1_2_2	| \
+			FLASH_FLAG_READ_1_1_4	| \
+			FLASH_FLAG_READ_1_4_4	| \
+			FLASH_FLAG_WRITE_1_1_4	| \
+			FLASH_FLAG_READ_FAST)
+	{ "s25fl129p0", 0x012018, 0x4d00, 256 * 1024,  64, S25FLXXXP_FLAG, 80,
+	  NULL },
+	{ "s25fl129p1", 0x012018, 0x4d01,  64 * 1024, 256, S25FLXXXP_FLAG, 80,
+	  NULL },
+
+	/*
+	 * Spansion S25FLxxxS
+	 *     - 256KiB and 64KiB sector variants (identified by ext. JEDEC)
+	 *     - RESET# signal supported by die but not bristled out on all
+	 *       package types.  The package type is a function of board design,
+	 *       so this information is captured in the board's flags.
+	 *     - Supports 'DYB' sector protection. Depending on variant, sectors
+	 *       may default to locked state on power-on.
+	 */
+#define S25FLXXXS_FLAG (S25FLXXXP_FLAG		| \
+			FLASH_FLAG_RESET	| \
+			FLASH_FLAG_DYB_LOCKING)
+	{ "s25fl128s0", 0x012018, 0x0300,  256 * 1024, 64, S25FLXXXS_FLAG, 80,
+	  NULL },
+	{ "s25fl128s1", 0x012018, 0x0301,  64 * 1024, 256, S25FLXXXS_FLAG, 80,
+	  NULL },
+	{ "s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128,
+	  S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, NULL },
+	{ "s25fl256s1", 0x010219, 0x4d01,  64 * 1024, 512,
+	  S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, NULL },
+
+	/* Winbond -- w25x "blocks" are 64K, "sectors" are 4KiB */
+#define W25X_FLAG (FLASH_FLAG_READ_WRITE	| \
+		   FLASH_FLAG_READ_FAST		| \
+		   FLASH_FLAG_READ_1_1_2	| \
+		   FLASH_FLAG_WRITE_1_1_2)
+	{ "w25x40",  0xef3013, 0,  64 * 1024,   8, W25X_FLAG, 75, NULL },
+	{ "w25x80",  0xef3014, 0,  64 * 1024,  16, W25X_FLAG, 75, NULL },
+	{ "w25x16",  0xef3015, 0,  64 * 1024,  32, W25X_FLAG, 75, NULL },
+	{ "w25x32",  0xef3016, 0,  64 * 1024,  64, W25X_FLAG, 75, NULL },
+	{ "w25x64",  0xef3017, 0,  64 * 1024, 128, W25X_FLAG, 75, NULL },
+
+	/* Winbond -- w25q "blocks" are 64K, "sectors" are 4KiB */
+#define W25Q_FLAG (FLASH_FLAG_READ_WRITE	| \
+		   FLASH_FLAG_READ_FAST		| \
+		   FLASH_FLAG_READ_1_1_2	| \
+		   FLASH_FLAG_READ_1_2_2	| \
+		   FLASH_FLAG_READ_1_1_4	| \
+		   FLASH_FLAG_READ_1_4_4	| \
+		   FLASH_FLAG_WRITE_1_1_4)
+	{ "w25q80",  0xef4014, 0,  64 * 1024,  16, W25Q_FLAG, 80, NULL },
+	{ "w25q16",  0xef4015, 0,  64 * 1024,  32, W25Q_FLAG, 80, NULL },
+	{ "w25q32",  0xef4016, 0,  64 * 1024,  64, W25Q_FLAG, 80, NULL },
+	{ "w25q64",  0xef4017, 0,  64 * 1024, 128, W25Q_FLAG, 80, NULL },
+
+	/* Sentinel */
+	{ NULL, 0x000000, 0, 0, 0, 0, 0, NULL },
+};
+
 #endif	/* ST_SPI_FSM_H */
-- 
1.8.3.2

Re: [PATCH v3 09/36] mtd: st_spi_fsm: Provide device look-up table

[Brian Norris]

On Fri, Nov 29, 2013 at 12:18:58PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -253,4 +253,141 @@ struct stfsm_seq {
>  } __attribute__((__packed__, aligned(4)));
>  #define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
>  
> +/* SPI Flash Device Table */
> +struct flash_info {
> +	char		*name;
> +	/*
> +	 * JEDEC id zero means "no ID" (most older chips); otherwise it has
> +	 * a high byte of zero plus three data bytes: the manufacturer id,
> +	 * then a two byte device id.
> +	 */
> +	u32		jedec_id;
> +	u16             ext_id;

Will 5 bytes of ID be enough? I think we're running into a need for 6
bytes of ID in m25p80.c right about now. Might make sense to start with
the right number of bytes.

> +	/*
> +	 * The size listed here is what works with FLASH_CMD_SE, which isn't
> +	 * necessarily called a "sector" by the vendor.
> +	 */
> +	unsigned	sector_size;
> +	u16		n_sectors;
> +	u32		flags;
> +	/*
> +	 * Note, where FAST_READ is supported, freq_max specifies the
> +	 * FAST_READ frequency, not the READ frequency.
> +	 */
> +	u32		max_freq;
> +	int		(*config)(struct stfsm *);

Do you *really* need a configuration callback? Can the callback be
represented as simply a flag for the special configuration behavior that
is needed, then your driver calls the appropriate "callback" when it
sees the flag?

BTW, your later patches have to introduce static declarations in this
header, which seems very ugly to me; you are entwining data with your
driver's implementation.

So this callback field can only be used by your driver, and it is one of
the main reasons that your table structure can't be used in other
drivers.

> +};
> +
> +static struct flash_info flash_types[] = {
> +	/*
> +	 * ST Microelectronics/Numonyx --
> +	 * (newer production versions may have feature updates
> +	 * (eg faster operating frequency)
> +	 */
> +#define M25P_FLAG (FLASH_FLAG_READ_WRITE | FLASH_FLAG_READ_FAST)

Please don't define macros in the middle of your table like this. (You
have several of these.)

> +	{ "m25p40",  0x202013, 0,  64 * 1024,   8, M25P_FLAG, 25, NULL },
> +	{ "m25p80",  0x202014, 0,  64 * 1024,  16, M25P_FLAG, 25, NULL },
> +	{ "m25p16",  0x202015, 0,  64 * 1024,  32, M25P_FLAG, 25, NULL },
> +	{ "m25p32",  0x202016, 0,  64 * 1024,  64, M25P_FLAG, 50, NULL },
> +	{ "m25p64",  0x202017, 0,  64 * 1024, 128, M25P_FLAG, 50, NULL },
> +	{ "m25p128", 0x202018, 0, 256 * 1024,  64, M25P_FLAG, 50, NULL },

[snip]

Brian

Re: [PATCH v3 09/36] mtd: st_spi_fsm: Provide device look-up table

[Angus Clark]

On 12/10/2013 10:03 PM, Brian Norris wrote:
> On Fri, Nov 29, 2013 at 12:18:58PM +0000, Lee Jones wrote:
>> --- a/drivers/mtd/devices/st_spi_fsm.h
>> +++ b/drivers/mtd/devices/st_spi_fsm.h
>> @@ -253,4 +253,141 @@ struct stfsm_seq {
>>  } __attribute__((__packed__, aligned(4)));
>>  #define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
>>  
>> +/* SPI Flash Device Table */
>> +struct flash_info {
>> +	char		*name;
>> +	/*
>> +	 * JEDEC id zero means "no ID" (most older chips); otherwise it has
>> +	 * a high byte of zero plus three data bytes: the manufacturer id,
>> +	 * then a two byte device id.
>> +	 */
>> +	u32		jedec_id;
>> +	u16             ext_id;
> 
> Will 5 bytes of ID be enough? I think we're running into a need for 6
> bytes of ID in m25p80.c right about now. Might make sense to start with
> the right number of bytes.

Yes, we will need 6 bytes of ID.  The "dirty" stm_spi_fsm driver already handles
arbitrary-length IDs.  This will need to be pulled into the "upstream" version
at some point.

>> +	int		(*config)(struct stfsm *);
> 
> Do you *really* need a configuration callback? Can the callback be
> represented as simply a flag for the special configuration behavior that
> is needed, then your driver calls the appropriate "callback" when it
> sees the flag?
> 
> BTW, your later patches have to introduce static declarations in this
> header, which seems very ugly to me; you are entwining data with your
> driver's implementation.

The config() callback is used to perform device/vendor specific initialisation
(e.g. how to set the QE bit) and configuration of the read, write, and erase
operations.  As such, it is related to the device, not to the driver or H/W
controller.

However, I take your point, it is rather ugly.  In some sense, the callback is
used to make up for the lack of information in the table that would otherwise
permit a generic configuration to be made.  One option would be to extend the
table structure to include the superset of properties required across all
devices, or perhaps an entry for extended, vendor-specific properties.

In any case, I think this discussion relates more to the spi-nor thread, rather
than the st_spi_fsm driver.

>> +static struct flash_info flash_types[] = {
>> +	/*
>> +	 * ST Microelectronics/Numonyx --
>> +	 * (newer production versions may have feature updates
>> +	 * (eg faster operating frequency)
>> +	 */
>> +#define M25P_FLAG (FLASH_FLAG_READ_WRITE | FLASH_FLAG_READ_FAST)
> 
> Please don't define macros in the middle of your table like this. (You
> have several of these.)

Personally, I think it is easier to read with the flag combinations defined near
where they are used.  I do not have strong feeling either way though, and it may
become a moot point if the table structure is changed :-)

Cheers,

Angus

Re: [PATCH v3 09/36] mtd: st_spi_fsm: Provide device look-up table

[Lee Jones]

> >> +/* SPI Flash Device Table */
> >> +struct flash_info {
> >> +	char		*name;
> >> +	/*
> >> +	 * JEDEC id zero means "no ID" (most older chips); otherwise it has
> >> +	 * a high byte of zero plus three data bytes: the manufacturer id,
> >> +	 * then a two byte device id.
> >> +	 */
> >> +	u32		jedec_id;
> >> +	u16             ext_id;
> > 
> > Will 5 bytes of ID be enough? I think we're running into a need for 6
> > bytes of ID in m25p80.c right about now. Might make sense to start with
> > the right number of bytes.
> 
> Yes, we will need 6 bytes of ID.  The "dirty" stm_spi_fsm driver already handles
> arbitrary-length IDs.  This will need to be pulled into the "upstream" version
> at some point.

Just looking at this now. May I do this subsequently? It's a pretty
big change and I don't want to disturb the patch-set too much at this
moment. The "dirty" kernel has only been updated with this change
recently. We will need it too, but we can definitely do without it for
a little while.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 10/36] mtd: st_spi_fsm: Dynamically setup flash device based on JEDEC ID

[Lee Jones]

Using previously added infrastructure we can now extract a device's JEDEC
ID, compare it to a list of known and supported devices and make assumptions
based on known characteristics of a given chip.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 26 +++++++++++++++++++++++++-
 drivers/mtd/devices/st_spi_fsm.h |  1 +
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 71b6188..ef177e5 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -140,6 +140,7 @@ static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 
 static struct flash_info * stfsm_jedec_probe(struct stfsm *fsm)
 {
+	struct flash_info	*info;
 	u16                     ext_jedec;
 	u32			jedec;
 	u8			id[5];
@@ -157,6 +158,15 @@ static struct flash_info * stfsm_jedec_probe(struct stfsm *fsm)
 	dev_dbg(fsm->dev, "JEDEC =  0x%08x [%02x %02x %02x %02x %02x]\n",
 		jedec, id[0], id[1], id[2], id[3], id[4]);
 
+	for (info = flash_types; info->name; info++) {
+		if (info->jedec_id == jedec) {
+			if (info->ext_id && info->ext_id != ext_jedec)
+				continue;
+			return info;
+		}
+	}
+	dev_err(fsm->dev, "Unrecognized JEDEC id %06x\n", jedec);
+
 	return NULL;
 }
 
@@ -251,6 +261,7 @@ static int stfsm_init(struct stfsm *fsm)
 static int stfsm_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	struct flash_info *info;
 	struct resource *res;
 	struct stfsm *fsm;
 	int ret;
@@ -297,7 +308,11 @@ static int stfsm_probe(struct platform_device *pdev)
 	}
 
 	/* Detect SPI FLASH device */
-	stfsm_jedec_probe(fsm);
+	info = stfsm_jedec_probe(fsm);
+	if (!info)
+		return -ENODEV;
+
+	fsm->info = info;
 
 	platform_set_drvdata(pdev, fsm);
 
@@ -306,6 +321,15 @@ static int stfsm_probe(struct platform_device *pdev)
 	fsm->mtd.writesize	= 4;
 	fsm->mtd.writebufsize	= fsm->mtd.writesize;
 	fsm->mtd.flags		= MTD_CAP_NORFLASH;
+	fsm->mtd.size		= info->sector_size * info->n_sectors;
+	fsm->mtd.erasesize	= info->sector_size;
+
+	dev_err(&pdev->dev,
+		"Found serial flash device: %s\n"
+		" size = %llx (%lldMiB) erasesize = 0x%08x (%uKiB)\n",
+		info->name,
+		(long long)fsm->mtd.size, (long long)(fsm->mtd.size >> 20),
+		fsm->mtd.erasesize, (fsm->mtd.erasesize >> 10));
 
 	return mtd_device_parse_register(&fsm->mtd, NULL, NULL, NULL, 0);
 }
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index fd1472e..0a6272c 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -235,6 +235,7 @@ struct stfsm {
 	struct resource		*region;
 	struct mtd_info		mtd;
 	struct mutex		lock;
+	struct flash_info 	*info;
 
 	uint32_t		fifo_dir_delay;
 };
-- 
1.8.3.2

[PATCH v3 11/36] mtd: st_spi_fsm: Search for preferred FSM message sequence configurations

[Lee Jones]

Here we provide a means to traverse though all supplied FSM message
sequence configurations and pick one based on our chip's capabilities.
The first one we match will be the preferred one, as they are
presented in order of preference.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 15 +++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index ef177e5..5f21d14 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -124,6 +124,21 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
 	}
 }
 
+/* Search for preferred configuration based on available flags */
+static struct seq_rw_config *
+stfsm_search_seq_rw_configs(struct stfsm *fsm,
+			    struct seq_rw_config cfgs[])
+{
+	struct seq_rw_config *config;
+	int flags = fsm->info->flags;
+
+	for (config = cfgs; cfgs->cmd != 0; config++)
+		if ((config->flags & flags) == config->flags)
+			return config;
+
+	return NULL;
+}
+
 static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 0a6272c..6fafa07 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -254,6 +254,18 @@ struct stfsm_seq {
 } __attribute__((__packed__, aligned(4)));
 #define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
 
+/* Parameters to configure a READ or WRITE FSM sequence */
+struct seq_rw_config {
+	uint32_t	flags;		/* flags to support config */
+	uint8_t		cmd;		/* FLASH command */
+	int		write;		/* Write Sequence */
+	uint8_t		addr_pads;	/* No. of addr pads (MODE & DUMMY) */
+	uint8_t		data_pads;	/* No. of data pads */
+	uint8_t		mode_data;	/* MODE data */
+	uint8_t		mode_cycles;	/* No. of MODE cycles */
+	uint8_t		dummy_cycles;	/* No. of DUMMY cycles */
+};
+
 /* SPI Flash Device Table */
 struct flash_info {
 	char		*name;
-- 
1.8.3.2

Re: [PATCH v3 11/36] mtd: st_spi_fsm: Search for preferred FSM message sequence configurations

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:00PM +0000, Lee Jones wrote:
> Here we provide a means to traverse though all supplied FSM message
> sequence configurations and pick one based on our chip's capabilities.
> The first one we match will be the preferred one, as they are
> presented in order of preference.

This process sounds very much like something that is needed for other
controllers: a way to match controller capabilties with the opcodes
supported by the flash. But it still isn't quite flexible enough for
others, because it doesn't try to abstract the selection criteria -- it
just uses lists that you presorted, with exceptions for a few different
types of devices (I'm referring to a later patch now, I guess, since
this patch actually has no users for stfsm_search_seq_rw_configs() yet).

> ---
>  drivers/mtd/devices/st_spi_fsm.c | 15 +++++++++++++++
>  drivers/mtd/devices/st_spi_fsm.h | 12 ++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index ef177e5..5f21d14 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -124,6 +124,21 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
>  	}
>  }
>  
> +/* Search for preferred configuration based on available flags */
> +static struct seq_rw_config *
> +stfsm_search_seq_rw_configs(struct stfsm *fsm,
> +			    struct seq_rw_config cfgs[])
> +{
> +	struct seq_rw_config *config;
> +	int flags = fsm->info->flags;
> +
> +	for (config = cfgs; cfgs->cmd != 0; config++)

This becomes an infinite loop if you can't find a matching config. You
probably meant 'config->cmd != 0':

	for (config = cfgs; config->cmd != 0; config++)

> +		if ((config->flags & flags) == config->flags)
> +			return config;
> +
> +	return NULL;
> +}
> +
>  static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
>  {
>  	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
> diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
> index 0a6272c..6fafa07 100644
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -254,6 +254,18 @@ struct stfsm_seq {
>  } __attribute__((__packed__, aligned(4)));
>  #define STFSM_SEQ_SIZE sizeof(struct stfsm_seq)
>  
> +/* Parameters to configure a READ or WRITE FSM sequence */
> +struct seq_rw_config {
> +	uint32_t	flags;		/* flags to support config */
> +	uint8_t		cmd;		/* FLASH command */
> +	int		write;		/* Write Sequence */
> +	uint8_t		addr_pads;	/* No. of addr pads (MODE & DUMMY) */
> +	uint8_t		data_pads;	/* No. of data pads */

"pads" means "pins" here, right?

> +	uint8_t		mode_data;	/* MODE data */

What does this represent? As far as I can see, all the configurations
you provide so far have this entry as 0x00.

> +	uint8_t		mode_cycles;	/* No. of MODE cycles */

What are MODE cycles? (Sorry if these are dumb questions.)

> +	uint8_t		dummy_cycles;	/* No. of DUMMY cycles */
> +};

Several pieces of this sequence struct seems suspiciously similar to
what would be needed by several other controllers, while the remaining
pieces don't seem to be so special that they cannot be configured
beneath a SPI NOR framework. I'm becoming less convinced that your
controller/driver is as unique as you say it is.

> +
>  /* SPI Flash Device Table */
>  struct flash_info {
>  	char		*name;

Brian

Re: [PATCH v3 11/36] mtd: st_spi_fsm: Search for preferred FSM message sequence configurations

[Angus Clark]

On 12/11/2013 01:06 AM, Brian Norris wrote:
> This process sounds very much like something that is needed for other
> controllers: a way to match controller capabilties with the opcodes
> supported by the flash. 

Yes, I think this should be a primary aim of any 'spi-nor' framework".

> But it still isn't quite flexible enough for
> others, because it doesn't try to abstract the selection criteria -- it
> just uses lists that you presorted

Sure, the implementation here is very specific to the st_spi_fsm controller.
However, until there is something better or more generic available, I am not
sure what else to suggest.  Selecting a configuration based on a presorted list
is probably a reasonable strategy though; READ_1_4_4 is more efficient that
READ_1_1_4, and READ_1_1_4 is more efficient than READ_1_1_2 etc.

>> +/* Search for preferred configuration based on available flags */
>> +static struct seq_rw_config *
>> +stfsm_search_seq_rw_configs(struct stfsm *fsm,
>> +			    struct seq_rw_config cfgs[])
>> +{
>> +	struct seq_rw_config *config;
>> +	int flags = fsm->info->flags;
>> +
>> +	for (config = cfgs; cfgs->cmd != 0; config++)
> 
> This becomes an infinite loop if you can't find a matching config. You
> probably meant 'config->cmd != 0':
> 

I will leave Lee to defend himself on this one ;-)

>> +/* Parameters to configure a READ or WRITE FSM sequence */
>> +struct seq_rw_config {
>> +	uint8_t		mode_data;	/* MODE data */
> 
> What does this represent? As far as I can see, all the configurations
> you provide so far have this entry as 0x00.
> 
>> +	uint8_t		mode_cycles;	/* No. of MODE cycles */
> 
> What are MODE cycles? (Sorry if these are dumb questions.)

Mode data cyles are similar to dummy cycles, except they also transmit some
extended information on how the device should perform the transfer.  For
example, Spansion devices use mode data to enter/exit a "continuous read mode"
where subsequent read operations do not need to issue the read command, just
supply an address (and further mode/dummy cycles).  In practice, we have only
used this particular mode during testing; our boot-controller dislikes any form
of statefull mode!

> 
>> +	uint8_t		dummy_cycles;	/* No. of DUMMY cycles */
>> +};
> 
> Several pieces of this sequence struct seems suspiciously similar to
> what would be needed by several other controllers, while the remaining
> pieces don't seem to be so special that they cannot be configured
> beneath a SPI NOR framework. I'm becoming less convinced that your
> controller/driver is as unique as you say it is.

Other controllers/drivers would certainly benefit from something similar.
However, I thought the point was that no such framework exists at present and
any spi-nor proposal is going to take time to mature.  In the meantime,
integrating the st_spi_fsm driver would provide another example of H/W that
needs to be supported by a spi-nor framework, and maybe give a few ideas along
the way.

Cheers,

Angus

Re: [PATCH v3 11/36] mtd: st_spi_fsm: Search for preferred FSM message sequence configurations

[Lee Jones]

> >> +/* Search for preferred configuration based on available flags */
> >> +static struct seq_rw_config *
> >> +stfsm_search_seq_rw_configs(struct stfsm *fsm,
> >> +			    struct seq_rw_config cfgs[])
> >> +{
> >> +	struct seq_rw_config *config;
> >> +	int flags = fsm->info->flags;
> >> +
> >> +	for (config = cfgs; cfgs->cmd != 0; config++)
> > 
> > This becomes an infinite loop if you can't find a matching config. You
> > probably meant 'config->cmd != 0':
> 
> I will leave Lee to defend himself on this one ;-)

No defence necessary, I think Brian's correct.

NB: You might want to take a look at the 'dirty' implementation too. ;)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 11/36] mtd: st_spi_fsm: Search for preferred FSM message sequence configurations

[Angus Clark]

On 12/17/2013 10:46 AM, Lee Jones wrote:
>> I will leave Lee to defend himself on this one ;-)
> 
> No defence necessary, I think Brian's correct.
> 
> NB: You might want to take a look at the 'dirty' implementation too. ;)
> 

Yes, just done so :-(

Angus

[PATCH v3 12/36] mtd: st_spi_fsm: Fetch platform specific configurations

[Lee Jones]

All supported platforms are able to pass specific configurations via
the Device Tree on boot. Here we add a function which is to be called
during the probing process which will extract them, or make other
assumptions based on capabilities provided.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 5f21d14..1f54983 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -273,6 +273,15 @@ static int stfsm_init(struct stfsm *fsm)
 	return 0;
 }
 
+static void stfsm_fetch_platform_configs(struct platform_device *pdev)
+{
+	struct stfsm *fsm = platform_get_drvdata(pdev);
+	struct flash_info *info = fsm->info;
+
+	if (info->sector_size * info->n_sectors > 0xFFFFFF)
+		info->flags |= FLASH_FLAG_32BIT_ADDR;
+}
+
 static int stfsm_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -331,6 +340,8 @@ static int stfsm_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, fsm);
 
+	stfsm_fetch_platform_configs(pdev);
+
 	fsm->mtd.dev.parent	= &pdev->dev;
 	fsm->mtd.type		= MTD_NORFLASH;
 	fsm->mtd.writesize	= 4;
-- 
1.8.3.2

[PATCH v3 13/36] mtd: st_spi_fsm: Prepare the read/write FSM message sequence(s)

[Lee Jones]

The FSM Serial Flash Controller is driven by issuing a standard set of
register writes we call a message sequence. This patch supplies a method
to prepare read/write FSM message sequence(s) based on chip capability
and configuration.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 69 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 1f54983..e04de62 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -139,6 +139,75 @@ stfsm_search_seq_rw_configs(struct stfsm *fsm,
 	return NULL;
 }
 
+/* Prepare a READ/WRITE sequence according to configuration parameters */
+static void stfsm_prepare_rw_seq(struct stfsm *fsm,
+				 struct stfsm_seq *seq,
+				 struct seq_rw_config *cfg)
+{
+	int addr1_cycles, addr2_cycles;
+	int i = 0;
+
+	memset(seq, 0, STFSM_SEQ_SIZE);
+
+	/* Add READ/WRITE OPC  */
+	seq->seq_opc[i++] = (SEQ_OPC_PADS_1 |
+			     SEQ_OPC_CYCLES(8) |
+			     SEQ_OPC_OPCODE(cfg->cmd));
+
+	/* Add WREN OPC for a WRITE sequence */
+	if (cfg->write)
+		seq->seq_opc[i++] = (SEQ_OPC_PADS_1 |
+				     SEQ_OPC_CYCLES(8) |
+				     SEQ_OPC_OPCODE(FLASH_CMD_WREN) |
+				     SEQ_OPC_CSDEASSERT);
+
+	/* Address configuration (24 or 32-bit addresses) */
+	addr1_cycles  = (fsm->info->flags & FLASH_FLAG_32BIT_ADDR) ? 16 : 8;
+	addr1_cycles /= cfg->addr_pads;
+	addr2_cycles  = 16 / cfg->addr_pads;
+	seq->addr_cfg = ((addr1_cycles & 0x3f) << 0 |	/* ADD1 cycles */
+			 (cfg->addr_pads - 1) << 6 |	/* ADD1 pads */
+			 (addr2_cycles & 0x3f) << 16 |	/* ADD2 cycles */
+			 ((cfg->addr_pads - 1) << 22));	/* ADD2 pads */
+
+	/* Data/Sequence configuration */
+	seq->seq_cfg = ((cfg->data_pads - 1) << 16 |
+			SEQ_CFG_STARTSEQ |
+			SEQ_CFG_CSDEASSERT);
+	if (!cfg->write)
+		seq->seq_cfg |= SEQ_CFG_READNOTWRITE;
+
+	/* Mode configuration (no. of pads taken from addr cfg) */
+	seq->mode = ((cfg->mode_data & 0xff) << 0 |	/* data */
+		     (cfg->mode_cycles & 0x3f) << 16 |	/* cycles */
+		     (cfg->addr_pads - 1) << 22);	/* pads */
+
+	/* Dummy configuration (no. of pads taken from addr cfg) */
+	seq->dummy = ((cfg->dummy_cycles & 0x3f) << 16 |	/* cycles */
+		      (cfg->addr_pads - 1) << 22);		/* pads */
+
+
+	/* Instruction sequence */
+	i = 0;
+	if (cfg->write)
+		seq->seq[i++] = STFSM_INST_CMD2;
+
+	seq->seq[i++] = STFSM_INST_CMD1;
+
+	seq->seq[i++] = STFSM_INST_ADD1;
+	seq->seq[i++] = STFSM_INST_ADD2;
+
+	if (cfg->mode_cycles)
+		seq->seq[i++] = STFSM_INST_MODE;
+
+	if (cfg->dummy_cycles)
+		seq->seq[i++] = STFSM_INST_DUMMY;
+
+	seq->seq[i++] =
+		cfg->write ? STFSM_INST_DATA_WRITE : STFSM_INST_DATA_READ;
+	seq->seq[i++] = STFSM_INST_STOP;
+}
+
 static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
-- 
1.8.3.2

[PATCH v3 14/36] mtd: st_spi_fsm: Add device-tree binding documentation

[Lee Jones]

Cc: devicetree@vger.kernel.org
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/mtd/st-fsm.txt | 26 ++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/st-fsm.txt

diff --git a/Documentation/devicetree/bindings/mtd/st-fsm.txt b/Documentation/devicetree/bindings/mtd/st-fsm.txt
new file mode 100644
index 0000000..c248939
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/st-fsm.txt
@@ -0,0 +1,26 @@
+* ST-Microelectronics SPI FSM Serial (NOR) Flash Controller
+
+Required properties:
+  - compatible : Should be "st,spi-fsm"
+  - reg        : Contains register's location and length.
+  - reg-names  : Should contain the reg names "spi-fsm"
+  - interrupts : The interrupt number
+  - pinctrl-0  : Standard Pinctrl phandle (see: pinctrl/pinctrl-bindings.txt)
+
+Optional properties:
+  - st,syscfg          : Phandle to boot-device system configuration registers
+  - st,boot-device-reg : Address of the aforementioned boot-device register(s)
+  - st,boot-device-spi : Expected boot-device value if booted via this device
+
+Example:
+	spifsm: spifsm@fe902000{
+	        compatible         = "st,spi-fsm";
+	        reg                =  <0xfe902000 0x1000>;
+	        reg-names          = "spi-fsm";
+	        pinctrl-0          = <&pinctrl_fsm>;
+		st,syscfg	   = <&syscfg_rear>;
+	        st,boot-device-reg = <0x958>;
+	        st,boot-device-spi = <0x1a>;
+		status = "okay";
+	};
+
-- 
1.8.3.2

Re: [PATCH v3 14/36] mtd: st_spi_fsm: Add device-tree binding documentation

[Linus Walleij]

On Fri, Nov 29, 2013 at 1:19 PM, Lee Jones <lee.jones@linaro.org> wrote:

Re this:

> +Optional properties:
> +  - st,syscfg          : Phandle to boot-device system configuration registers
> +  - st,boot-device-reg : Address of the aforementioned boot-device register(s)
> +  - st,boot-device-spi : Expected boot-device value if booted via this device
> +
> +Example:
> +       spifsm: spifsm@fe902000{
> +               compatible         = "st,spi-fsm";
> +               reg                =  <0xfe902000 0x1000>;
> +               reg-names          = "spi-fsm";
> +               pinctrl-0          = <&pinctrl_fsm>;
> +               st,syscfg          = <&syscfg_rear>;
> +               st,boot-device-reg = <0x958>;
> +               st,boot-device-spi = <0x1a>;

I don't think we should encode any register offsets whatsoever in device
tree but maybe that's just me. (Yes, Stephen will beat me up about
pin control single, but I consider that a special case.)

I would just put the last two things as #defines into the driver file(s)
or - if it's related to other syscfg registers and varies with SoC incarnation,
as a #define in a shared header for that syscfg thing.

The syscfg phandle is elegant though, really like that.

Yours,
Linus Walleij

Re: [PATCH v3 14/36] mtd: st_spi_fsm: Add device-tree binding documentation

[Lee Jones]

> > +Optional properties:
> > +  - st,syscfg          : Phandle to boot-device system configuration registers
> > +  - st,boot-device-reg : Address of the aforementioned boot-device register(s)
> > +  - st,boot-device-spi : Expected boot-device value if booted via this device
> > +
> > +Example:
> > +       spifsm: spifsm@fe902000{
> > +               compatible         = "st,spi-fsm";
> > +               reg                =  <0xfe902000 0x1000>;
> > +               reg-names          = "spi-fsm";
> > +               pinctrl-0          = <&pinctrl_fsm>;
> > +               st,syscfg          = <&syscfg_rear>;
> > +               st,boot-device-reg = <0x958>;
> > +               st,boot-device-spi = <0x1a>;
> 
> I don't think we should encode any register offsets whatsoever in device
> tree but maybe that's just me. (Yes, Stephen will beat me up about
> pin control single, but I consider that a special case.)
> 
> I would just put the last two things as #defines into the driver file(s)
> or - if it's related to other syscfg registers and varies with SoC incarnation,
> as a #define in a shared header for that syscfg thing.

No can do. This isn't _this_ device's register offset, this is a
syscfg register offset which a) there is no driver to apply specific
register offsets to and b) are liable to change oversubsequent SoCs.

> The syscfg phandle is elegant though, really like that.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 14/36] mtd: st_spi_fsm: Add device-tree binding documentation

[Linus Walleij]

On Mon, Dec 2, 2013 at 12:03 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> > +Optional properties:
>> > +  - st,syscfg          : Phandle to boot-device system configuration registers
>> > +  - st,boot-device-reg : Address of the aforementioned boot-device register(s)
>> > +  - st,boot-device-spi : Expected boot-device value if booted via this device
>> > +
>> > +Example:
>> > +       spifsm: spifsm@fe902000{
>> > +               compatible         = "st,spi-fsm";
>> > +               reg                =  <0xfe902000 0x1000>;
>> > +               reg-names          = "spi-fsm";
>> > +               pinctrl-0          = <&pinctrl_fsm>;
>> > +               st,syscfg          = <&syscfg_rear>;
>> > +               st,boot-device-reg = <0x958>;
>> > +               st,boot-device-spi = <0x1a>;
>>
>> I don't think we should encode any register offsets whatsoever in device
>> tree but maybe that's just me. (Yes, Stephen will beat me up about
>> pin control single, but I consider that a special case.)
>>
>> I would just put the last two things as #defines into the driver file(s)
>> or - if it's related to other syscfg registers and varies with SoC incarnation,
>> as a #define in a shared header for that syscfg thing.
>
> No can do. This isn't _this_ device's register offset, this is a
> syscfg register offset which a) there is no driver to apply specific
> register offsets to and b) are liable to change oversubsequent SoCs.

So it can be in <linux/mfd/my-sysconfig-regs.h> as
a

#define MY_SYSCON_V1_BOOT_DEV_REG 0x958
#define MY_SYSCON_V1_BOOT_DEV_SPI 0x1a

The kernel should know which SoC is in use and
act apropriately right?

But indeed, what you're doing has been done before
in drivers/regulator/anatop-regulator.c where some
register offset is read in from the attribute
"anatop-delay-reg-offset". So it may be that the DT people
need to overrule me on this one :-)

I don't think we should hard-code knowledge of specific
register locations into the device tree at all as that makes
things very hard to debug.

Yours,
Linus Walleij

Re: [PATCH v3 14/36] mtd: st_spi_fsm: Add device-tree binding documentation

[Lee Jones]

On Tue, 03 Dec 2013, Linus Walleij wrote:

> On Mon, Dec 2, 2013 at 12:03 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> > +Optional properties:
> >> > +  - st,syscfg          : Phandle to boot-device system configuration registers
> >> > +  - st,boot-device-reg : Address of the aforementioned boot-device register(s)
> >> > +  - st,boot-device-spi : Expected boot-device value if booted via this device
> >> > +
> >> > +Example:
> >> > +       spifsm: spifsm@fe902000{
> >> > +               compatible         = "st,spi-fsm";
> >> > +               reg                =  <0xfe902000 0x1000>;
> >> > +               reg-names          = "spi-fsm";
> >> > +               pinctrl-0          = <&pinctrl_fsm>;
> >> > +               st,syscfg          = <&syscfg_rear>;
> >> > +               st,boot-device-reg = <0x958>;
> >> > +               st,boot-device-spi = <0x1a>;
> >>
> >> I don't think we should encode any register offsets whatsoever in device
> >> tree but maybe that's just me. (Yes, Stephen will beat me up about
> >> pin control single, but I consider that a special case.)
> >>
> >> I would just put the last two things as #defines into the driver file(s)
> >> or - if it's related to other syscfg registers and varies with SoC incarnation,
> >> as a #define in a shared header for that syscfg thing.
> >
> > No can do. This isn't _this_ device's register offset, this is a
> > syscfg register offset which a) there is no driver to apply specific
> > register offsets to and b) are liable to change oversubsequent SoCs.
> 
> So it can be in <linux/mfd/my-sysconfig-regs.h> as
> a
> 
> #define MY_SYSCON_V1_BOOT_DEV_REG 0x958
> #define MY_SYSCON_V1_BOOT_DEV_SPI 0x1a
> 
> The kernel should know which SoC is in use and
> act apropriately right?

And this have lots of machine_is() calls everywhere?

Sounds perfect. ;)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 14/36] mtd: st_spi_fsm: Add device-tree binding documentation

[Linus Walleij]

On Tue, Dec 3, 2013 at 12:31 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 03 Dec 2013, Linus Walleij wrote:

>> So it can be in <linux/mfd/my-sysconfig-regs.h> as
>> a
>>
>> #define MY_SYSCON_V1_BOOT_DEV_REG 0x958
>> #define MY_SYSCON_V1_BOOT_DEV_SPI 0x1a
>>
>> The kernel should know which SoC is in use and
>> act apropriately right?
>
> And this have lots of machine_is() calls everywhere?

No need for Mr. Everywhere to know about the SoC.

There are other ways to achieve this. arch/arm/mach-foo
always knows which ASIC it is running on, and it can do
various things:

- Add device tree fragments/overlays (OK high-tech, but
  we're getting to this, inevitably)

- Selectively add AUXDATA, which is absolutely OK if
  we can do autoprobing of hardware IMO.

Yours,
Linus Walleij

Re: [PATCH v3 14/36] mtd: st_spi_fsm: Add device-tree binding documentation

[Lee Jones]

On Tue, 03 Dec 2013, Linus Walleij wrote:

> On Tue, Dec 3, 2013 at 12:31 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 03 Dec 2013, Linus Walleij wrote:
> 
> >> So it can be in <linux/mfd/my-sysconfig-regs.h> as
> >> a
> >>
> >> #define MY_SYSCON_V1_BOOT_DEV_REG 0x958
> >> #define MY_SYSCON_V1_BOOT_DEV_SPI 0x1a
> >>
> >> The kernel should know which SoC is in use and
> >> act apropriately right?
> >
> > And this have lots of machine_is() calls everywhere?
> 
> No need for Mr. Everywhere to know about the SoC.
> 
> There are other ways to achieve this. arch/arm/mach-foo
> always knows which ASIC it is running on, and it can do
> various things:
> 
> - Add device tree fragments/overlays (OK high-tech, but
>   we're getting to this, inevitably)
> 
> - Selectively add AUXDATA, which is absolutely OK if
>   we can do autoprobing of hardware IMO.

I prefer our solution TBH.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 15/36] mtd: st_spi_fsm: Fetch boot-device from mode pins

[Lee Jones]

It's important for us to determine which device was used to boot from in
order to make some correct decisions surrounding Power Management. On
each of the platforms which support the FSM this is communicated via
a set of mode pins held in the system configuration area. This patch
determine the boot device and stores the result.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 41 ++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |  1 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index e04de62..bb642a1 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -14,7 +14,9 @@
  */
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/platform_device.h>
+#include <linux/mfd/syscon.h>
 #include <linux/mtd/mtd.h>
 #include <linux/sched.h>
 #include <linux/delay.h>
@@ -346,9 +348,48 @@ static void stfsm_fetch_platform_configs(struct platform_device *pdev)
 {
 	struct stfsm *fsm = platform_get_drvdata(pdev);
 	struct flash_info *info = fsm->info;
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *regmap;
+	uint32_t boot_device_reg;
+	uint32_t boot_device_spi;
+	uint32_t boot_device;     /* Value we read from *boot_device_reg */
+	int ret;
 
+	/* Booting from SPI NOR Flash is the default */
+	fsm->booted_from_spi = true;
+
+	/* Use device size to determine address width */
 	if (info->sector_size * info->n_sectors > 0xFFFFFF)
 		info->flags |= FLASH_FLAG_32BIT_ADDR;
+
+	regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
+	if (!IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		goto boot_device_fail;
+	}
+
+	/* Where in the syscon the boot device information lives */
+	ret = of_property_read_u32(np, "boot-device-reg", &boot_device_reg);
+	if (ret)
+		goto boot_device_fail;
+
+	/* Boot device value when booted from SPI NOR */
+	ret = of_property_read_u32(np, "boot-device-spi", &boot_device_spi);
+	if (ret)
+		goto boot_device_fail;
+
+	ret = regmap_read(regmap, boot_device_reg, &boot_device);
+	if (ret)
+		goto boot_device_fail;
+
+	if (boot_device != boot_device_spi)
+		fsm->booted_from_spi = false;
+
+	return;
+
+boot_device_fail:
+	dev_warn(&pdev->dev,
+		 "failed to fetch boot device, assuming boot from SPI\n");
 }
 
 static int stfsm_probe(struct platform_device *pdev)
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 6fafa07..a0eafa9 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -238,6 +238,7 @@ struct stfsm {
 	struct flash_info 	*info;
 
 	uint32_t		fifo_dir_delay;
+	bool			booted_from_spi;
 };
 
 struct stfsm_seq {
-- 
1.8.3.2

Re: [PATCH v3 15/36] mtd: st_spi_fsm: Fetch boot-device from mode pins

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:04PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -346,9 +348,48 @@ static void stfsm_fetch_platform_configs(struct platform_device *pdev)
...
> +	/* Use device size to determine address width */

Since you're splitting this patch series up so much, shouldn't this
comment go in when you add the two lines below?

>  	if (info->sector_size * info->n_sectors > 0xFFFFFF)
>  		info->flags |= FLASH_FLAG_32BIT_ADDR;
...

BTW, even though it doesn't exactly make for the best (runtime)
bisectability (since your driver is not functional yet), this approach
to splitting your driver introduction into logical chunks does make it a
little easier to review. So thanks.

Brian

[PATCH v3 16/36] mtd: st_spi_fsm: Provide the erase one sector sequence

[Lee Jones]

The FSM Serial Flash Controller is driven by issuing a standard set of
register writes we call a message sequence. This patch supplies a method
to prepare the message sequence responsible for erasing a single sector.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index bb642a1..47f9da0 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -44,6 +44,28 @@ static struct stfsm_seq stfsm_seq_read_jedec = {
 		    SEQ_CFG_STARTSEQ),
 };
 
+static struct stfsm_seq stfsm_seq_erase_sector = {
+	/* 'addr_cfg' configured during initialisation */
+	.seq_opc = {
+		(SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		 SEQ_OPC_OPCODE(FLASH_CMD_WREN) | SEQ_OPC_CSDEASSERT),
+
+		(SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		 SEQ_OPC_OPCODE(FLASH_CMD_SE)),
+	},
+	.seq = {
+		STFSM_INST_CMD1,
+		STFSM_INST_CMD2,
+		STFSM_INST_ADD1,
+		STFSM_INST_ADD2,
+		STFSM_INST_STOP,
+	},
+	.seq_cfg = (SEQ_CFG_PADS_1 |
+		    SEQ_CFG_READNOTWRITE |
+		    SEQ_CFG_CSDEASSERT |
+		    SEQ_CFG_STARTSEQ),
+};
+
 static inline int stfsm_is_idle(struct stfsm *fsm)
 {
 	return readl(fsm->base + SPI_FAST_SEQ_STA) & 0x10;
@@ -126,6 +148,19 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
 	}
 }
 
+/* Configure 'addr_cfg' according to addressing mode */
+static void stfsm_prepare_erasesec_seq(struct stfsm *fsm,
+				       struct stfsm_seq *seq)
+{
+	int addr1_cycles = fsm->info->flags & FLASH_FLAG_32BIT_ADDR ? 16 : 8;
+
+	seq->addr_cfg = (ADR_CFG_CYCLES_ADD1(addr1_cycles) |
+			 ADR_CFG_PADS_1_ADD1 |
+			 ADR_CFG_CYCLES_ADD2(16) |
+			 ADR_CFG_PADS_1_ADD2 |
+			 ADR_CFG_CSDEASSERT_ADD2);
+}
+
 /* Search for preferred configuration based on available flags */
 static struct seq_rw_config *
 stfsm_search_seq_rw_configs(struct stfsm *fsm,
-- 
1.8.3.2

[PATCH v3 17/36] mtd: st_spi_fsm: Provide the sequence for enabling 32bit addressing mode

[Lee Jones]

The FSM Serial Flash Controller is driven by issuing a standard set of
register writes we call a message sequence. This patch supplies a method
to prepare the message sequence responsible for setting 32bit addressing
mode on the Flash chip.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 47f9da0..7d690c7 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -66,6 +66,28 @@ static struct stfsm_seq stfsm_seq_erase_sector = {
 		    SEQ_CFG_STARTSEQ),
 };
 
+static int stfsm_n25q_en_32bit_addr_seq(struct stfsm_seq *seq)
+{
+	seq->seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+			   SEQ_OPC_OPCODE(FLASH_CMD_EN4B_ADDR));
+	seq->seq_opc[1] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+			   SEQ_OPC_OPCODE(FLASH_CMD_WREN) |
+			   SEQ_OPC_CSDEASSERT);
+
+	seq->seq[0] = STFSM_INST_CMD2;
+	seq->seq[1] = STFSM_INST_CMD1;
+	seq->seq[2] = STFSM_INST_WAIT;
+	seq->seq[3] = STFSM_INST_STOP;
+
+	seq->seq_cfg = (SEQ_CFG_PADS_1 |
+			SEQ_CFG_ERASE |
+			SEQ_CFG_READNOTWRITE |
+			SEQ_CFG_CSDEASSERT |
+			SEQ_CFG_STARTSEQ);
+
+	return 0;
+}
+
 static inline int stfsm_is_idle(struct stfsm *fsm)
 {
 	return readl(fsm->base + SPI_FAST_SEQ_STA) & 0x10;
-- 
1.8.3.2

[PATCH v3 18/36] mtd: st_spi_fsm: Prepare read/write sequences according to configuration

[Lee Jones]

Firstly we search for our preference read/write configuration based on a
given chip's capabilities. Then we actually set up the message sequence
accordingly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 7d690c7..14891ea 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -267,6 +267,23 @@ static void stfsm_prepare_rw_seq(struct stfsm *fsm,
 	seq->seq[i++] = STFSM_INST_STOP;
 }
 
+static int stfsm_search_prepare_rw_seq(struct stfsm *fsm,
+				       struct stfsm_seq *seq,
+				       struct seq_rw_config *cfgs)
+{
+	struct seq_rw_config *config;
+
+	config = stfsm_search_seq_rw_configs(fsm, cfgs);
+	if (!config) {
+		dev_err(fsm->dev, "failed to find suitable config\n");
+		return -EINVAL;
+	}
+
+	stfsm_prepare_rw_seq(fsm, seq, config);
+
+	return 0;
+}
+
 static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
-- 
1.8.3.2

[PATCH v3 19/36] mtd: st_spi_fsm: Add a check to if the chip can handle an SoC reset

[Lee Jones]

Based on information we can obtain though platform specific data and/or
chip capabilities we are able to determine whether or not we can handle
a SoC reset or not. To find out why this is important please read the
comment provided in the patch.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 14891ea..4edd3f2 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -170,6 +170,40 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
 	}
 }
 
+/*
+ * SoC reset on 'boot-from-spi' systems
+ *
+ * Certain modes of operation cause the Flash device to enter a particular state
+ * for a period of time (e.g. 'Erase Sector', 'Quad Enable', and 'Enter 32-bit
+ * Addr' commands).  On boot-from-spi systems, it is important to consider what
+ * happens if a warm reset occurs during this period.  The SPIBoot controller
+ * assumes that Flash device is in its default reset state, 24-bit address mode,
+ * and ready to accept commands.  This can be achieved using some form of
+ * on-board logic/controller to force a device POR in response to a SoC-level
+ * reset or by making use of the device reset signal if available (limited
+ * number of devices only).
+ *
+ * Failure to take such precautions can cause problems following a warm reset.
+ * For some operations (e.g. ERASE), there is little that can be done.  For
+ * other modes of operation (e.g. 32-bit addressing), options are often
+ * available that can help minimise the window in which a reset could cause a
+ * problem.
+ *
+ */
+static bool stfsm_can_handle_soc_reset(struct stfsm *fsm)
+{
+	/* Reset signal is available on the board and supported by the device */
+	if (fsm->reset_signal && fsm->info->flags & FLASH_FLAG_RESET)
+		return true;
+
+	/* Board-level logic forces a power-on-reset */
+	if (fsm->reset_por)
+		return true;
+
+	/* Reset is not properly handled and may result in failure to reboot */
+	return false;
+}
+
 /* Configure 'addr_cfg' according to addressing mode */
 static void stfsm_prepare_erasesec_seq(struct stfsm *fsm,
 				       struct stfsm_seq *seq)
@@ -442,6 +476,10 @@ static void stfsm_fetch_platform_configs(struct platform_device *pdev)
 		goto boot_device_fail;
 	}
 
+	fsm->reset_signal = of_property_read_bool(np, "st,reset-signal");
+
+	fsm->reset_por = of_property_read_bool(np, "st,reset-por");
+
 	/* Where in the syscon the boot device information lives */
 	ret = of_property_read_u32(np, "boot-device-reg", &boot_device_reg);
 	if (ret)
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index a0eafa9..0548511 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -239,6 +239,8 @@ struct stfsm {
 
 	uint32_t		fifo_dir_delay;
 	bool			booted_from_spi;
+	bool			reset_signal;
+	bool			reset_por;
 };
 
 struct stfsm_seq {
-- 
1.8.3.2

[PATCH v3 20/36] mtd: st_spi_fsm: Provide a method to put the chip into 32bit addressing mode

[Lee Jones]

Most Serial Flash chips support 24bit addressing as a default but more
recent incarnations can support 32bit. Based on information provided
though platform specific data and capabilities we can determine whether
or not our current chip can. This patch provides a means to setup the
FSM message sequence to put the chip into 32bit mode.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 4edd3f2..325fd7a 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -28,6 +28,8 @@
 #include "st_spi_fsm.h"
 #include "serial_flash_cmds.h"
 
+static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
+
 static struct stfsm_seq stfsm_seq_read_jedec = {
 	.data_size = TRANSFER_SIZE(8),
 	.seq_opc[0] = (SEQ_OPC_PADS_1 |
@@ -170,6 +172,23 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
 	}
 }
 
+static int stfsm_enter_32bit_addr(struct stfsm *fsm, int enter)
+{
+	struct stfsm_seq *seq = &stfsm_seq_en_32bit_addr;
+	uint32_t cmd = enter ? FLASH_CMD_EN4B_ADDR : FLASH_CMD_EX4B_ADDR;
+
+	seq->seq_opc[0] = (SEQ_OPC_PADS_1 |
+			   SEQ_OPC_CYCLES(8) |
+			   SEQ_OPC_OPCODE(cmd) |
+			   SEQ_OPC_CSDEASSERT);
+
+	stfsm_load_seq(fsm, seq);
+
+	stfsm_wait_seq(fsm);
+
+	return 0;
+}
+
 /*
  * SoC reset on 'boot-from-spi' systems
  *
-- 
1.8.3.2

Re: [PATCH v3 20/36] mtd: st_spi_fsm: Provide a method to put the chip into 32bit addressing mode

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:09PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -28,6 +28,8 @@
>  #include "st_spi_fsm.h"
>  #include "serial_flash_cmds.h"
>  
> +static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */

You need a space between the semicolon and the comment.

But why isn't this struct just embedded in the struct stfsm? I don't
like static instances like this, especially when they are dynamically
configured (as your comment notes). In the crazy even that we ever have
two of these FSMs on an SoC (can this happen?), you'll be introducing a
race on needlessly-shared data.

The same comment applies to your later structs stfsm_seq_read and
stfsm_seq_write.

Brian

[PATCH v3 21/36] mtd: st_spi_fsm: Update the flash Volatile Configuration Register

[Lee Jones]

The FSM Serial Flash Controller is driven by issuing a standard set of
register writes we call a message sequence. This patch supplies a method
to prepare the message sequence responsible for updating a chip's VCR.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 325fd7a..9449005 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -68,6 +68,23 @@ static struct stfsm_seq stfsm_seq_erase_sector = {
 		    SEQ_CFG_STARTSEQ),
 };
 
+static struct stfsm_seq stfsm_seq_wrvcr = {
+	.seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		       SEQ_OPC_OPCODE(FLASH_CMD_WREN) | SEQ_OPC_CSDEASSERT),
+	.seq_opc[1] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		       SEQ_OPC_OPCODE(FLASH_CMD_WRVCR)),
+	.seq = {
+		STFSM_INST_CMD1,
+		STFSM_INST_CMD2,
+		STFSM_INST_STA_WR1,
+		STFSM_INST_STOP,
+	},
+	.seq_cfg = (SEQ_CFG_PADS_1 |
+		    SEQ_CFG_READNOTWRITE |
+		    SEQ_CFG_CSDEASSERT |
+		    SEQ_CFG_STARTSEQ),
+};
+
 static int stfsm_n25q_en_32bit_addr_seq(struct stfsm_seq *seq)
 {
 	seq->seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
@@ -189,6 +206,21 @@ static int stfsm_enter_32bit_addr(struct stfsm *fsm, int enter)
 	return 0;
 }
 
+static int stfsm_wrvcr(struct stfsm *fsm, uint8_t data)
+{
+	struct stfsm_seq *seq = &stfsm_seq_wrvcr;
+
+	dev_dbg(fsm->dev, "writing VCR 0x%02x\n", data);
+
+	seq->status = (STA_DATA_BYTE1(data) | STA_PADS_1 | STA_CSDEASSERT);
+
+	stfsm_load_seq(fsm, seq);
+
+	stfsm_wait_seq(fsm);
+
+	return 0;
+}
+
 /*
  * SoC reset on 'boot-from-spi' systems
  *
-- 
1.8.3.2

[PATCH v3 22/36] mtd: st_spi_fsm: Provide the default read/write configurations

[Lee Jones]

Message sequences can vary depending on how many pads (lines) are
required to address the chip (mode & dummy), how many data pads (lines)
are required to write out to the chip which will determine speed
amongst other things which are detailed by the SFDP specification. We
are able to use multiple configurations for each chip, but they need
to me matched to a device's capabilities. These configurations are
listed in preference order - most preferred first.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 9449005..0ecf248 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -28,6 +28,33 @@
 #include "st_spi_fsm.h"
 #include "serial_flash_cmds.h"
 
+/*
+ * FSM message sequence configurations:
+ *
+ * All configs are presented in order of preference
+ */
+
+/* Default READ configurations, in order of preference */
+static struct seq_rw_config default_read_configs[] = {
+	{FLASH_FLAG_READ_1_4_4, FLASH_CMD_READ_1_4_4,	0, 4, 4, 0x00, 2, 4},
+	{FLASH_FLAG_READ_1_1_4, FLASH_CMD_READ_1_1_4,	0, 1, 4, 0x00, 4, 0},
+	{FLASH_FLAG_READ_1_2_2, FLASH_CMD_READ_1_2_2,	0, 2, 2, 0x00, 4, 0},
+	{FLASH_FLAG_READ_1_1_2, FLASH_CMD_READ_1_1_2,	0, 1, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_FAST,	FLASH_CMD_READ_FAST,	0, 1, 1, 0x00, 0, 8},
+	{FLASH_FLAG_READ_WRITE, FLASH_CMD_READ,		0, 1, 1, 0x00, 0, 0},
+	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
+};
+
+/* Default WRITE configurations */
+static struct seq_rw_config default_write_configs[] = {
+	{FLASH_FLAG_WRITE_1_4_4, FLASH_CMD_WRITE_1_4_4, 1, 4, 4, 0x00, 0, 0},
+	{FLASH_FLAG_WRITE_1_1_4, FLASH_CMD_WRITE_1_1_4, 1, 1, 4, 0x00, 0, 0},
+	{FLASH_FLAG_WRITE_1_2_2, FLASH_CMD_WRITE_1_2_2, 1, 2, 2, 0x00, 0, 0},
+	{FLASH_FLAG_WRITE_1_1_2, FLASH_CMD_WRITE_1_1_2, 1, 1, 2, 0x00, 0, 0},
+	{FLASH_FLAG_READ_WRITE,  FLASH_CMD_WRITE,       1, 1, 1, 0x00, 0, 0},
+	{0x00,			 0,			 0, 0, 0, 0x00, 0, 0},
+};
+
 static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
 
 static struct stfsm_seq stfsm_seq_read_jedec = {
-- 
1.8.3.2

Re: [PATCH v3 22/36] mtd: st_spi_fsm: Provide the default read/write configurations

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:11PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -28,6 +28,33 @@
...
> +/* Default WRITE configurations */
> +static struct seq_rw_config default_write_configs[] = {
> +	{FLASH_FLAG_WRITE_1_4_4, FLASH_CMD_WRITE_1_4_4, 1, 4, 4, 0x00, 0, 0},
> +	{FLASH_FLAG_WRITE_1_1_4, FLASH_CMD_WRITE_1_1_4, 1, 1, 4, 0x00, 0, 0},
> +	{FLASH_FLAG_WRITE_1_2_2, FLASH_CMD_WRITE_1_2_2, 1, 2, 2, 0x00, 0, 0},
> +	{FLASH_FLAG_WRITE_1_1_2, FLASH_CMD_WRITE_1_1_2, 1, 1, 2, 0x00, 0, 0},
> +	{FLASH_FLAG_READ_WRITE,  FLASH_CMD_WRITE,       1, 1, 1, 0x00, 0, 0},
> +	{0x00,			 0,			 0, 0, 0, 0x00, 0, 0},

^^^ This line has an extra space in it, that throws off the table
alignment.

> +};

Brian

[PATCH v3 23/36] mtd: st_spi_fsm: Supply the N25Qxxx specific read configurations

[Lee Jones]

The N25Qxxx Serial Flash devices required different sequence
configurations depending on whether they're running in 24bit (3Byte)
or 32bit (4Byte) mode. We provide those here.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 43 ++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h | 10 ++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 0ecf248..3a6b233 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -55,6 +55,49 @@ static struct seq_rw_config default_write_configs[] = {
 	{0x00,			 0,			 0, 0, 0, 0x00, 0, 0},
 };
 
+/*
+ * [N25Qxxx] Configuration
+ */
+#define N25Q_VCR_DUMMY_CYCLES(x)	(((x) & 0xf) << 4)
+#define N25Q_VCR_XIP_DISABLED		((uint8_t)0x1 << 3)
+#define N25Q_VCR_WRAP_CONT		0x3
+
+/* N25Q 3-byte Address READ configurations
+ *	- 'FAST' variants configured for 8 dummy cycles.
+ *
+ * Note, the number of dummy cycles used for 'FAST' READ operations is
+ * configurable and would normally be tuned according to the READ command and
+ * operating frequency.  However, this applies universally to all 'FAST' READ
+ * commands, including those used by the SPIBoot controller, and remains in
+ * force until the device is power-cycled.  Since the SPIBoot controller is
+ * hard-wired to use 8 dummy cycles, we must configure the device to also use 8
+ * cycles.
+ */
+static struct seq_rw_config n25q_read3_configs[] = {
+	{FLASH_FLAG_READ_1_4_4, FLASH_CMD_READ_1_4_4,	0, 4, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_1_4, FLASH_CMD_READ_1_1_4,	0, 1, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_2_2, FLASH_CMD_READ_1_2_2,	0, 2, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_1_2, FLASH_CMD_READ_1_1_2,	0, 1, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_FAST,	FLASH_CMD_READ_FAST,	0, 1, 1, 0x00, 0, 8},
+	{FLASH_FLAG_READ_WRITE, FLASH_CMD_READ,	        0, 1, 1, 0x00, 0, 0},
+	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
+};
+
+/* N25Q 4-byte Address READ configurations
+ *	- use special 4-byte address READ commands (reduces overheads, and
+ *        reduces risk of hitting watchdog reset issues).
+ *	- 'FAST' variants configured for 8 dummy cycles (see note above.)
+ */
+static struct seq_rw_config n25q_read4_configs[] = {
+	{FLASH_FLAG_READ_1_4_4, FLASH_CMD_READ4_1_4_4,	0, 4, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_1_4, FLASH_CMD_READ4_1_1_4,	0, 1, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_2_2, FLASH_CMD_READ4_1_2_2,	0, 2, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_1_2, FLASH_CMD_READ4_1_1_2,	0, 1, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_FAST,	FLASH_CMD_READ4_FAST,	0, 1, 1, 0x00, 0, 8},
+	{FLASH_FLAG_READ_WRITE, FLASH_CMD_READ4,	0, 1, 1, 0x00, 0, 0},
+	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
+};
+
 static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
 
 static struct stfsm_seq stfsm_seq_read_jedec = {
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 0548511..6699174 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -229,6 +229,15 @@
 #define FLASH_CMD_READ4_1_1_4	0x6c
 #define FLASH_CMD_READ4_1_4_4	0xec
 
+/*
+ * Flags to tweak operation of default read/write/erase routines
+ */
+#define CFG_READ_TOGGLE_32BIT_ADDR	0x00000001
+#define CFG_WRITE_TOGGLE_32BIT_ADDR	0x00000002
+#define CFG_WRITE_EX_32BIT_ADDR_DELAY	0x00000004
+#define CFG_ERASESEC_TOGGLE_32BIT_ADDR	0x00000008
+#define CFG_S25FL_CHECK_ERROR_FLAGS	0x00000010
+
 struct stfsm {
 	struct device		*dev;
 	void __iomem		*base;
@@ -237,6 +246,7 @@ struct stfsm {
 	struct mutex		lock;
 	struct flash_info 	*info;
 
+	uint32_t		configuration;
 	uint32_t		fifo_dir_delay;
 	bool			booted_from_spi;
 	bool			reset_signal;
-- 
1.8.3.2

[PATCH v3 24/36] mtd: st_spi_fsm: Supply the N25Qxxx chip specific configuration call-back

[Lee Jones]

In the FSM driver we handle chip differences by providing the possibility
of calling back into a chip specific initialisation routine. In this patch
we provide one for the N25Qxxx series, which endeavours to setup things
like the read, write and erase sequences, as they differ from the
default. We also configure 32bit support and the amount of dummy cycles to
use.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 77 ++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |  7 ++--
 2 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 3a6b233..2369da1 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -98,6 +98,8 @@ static struct seq_rw_config n25q_read4_configs[] = {
 	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
 };
 
+static struct stfsm_seq stfsm_seq_read; 	/* Dynamically populated */
+static struct stfsm_seq stfsm_seq_write;	/* Dynamically populated */
 static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
 
 static struct stfsm_seq stfsm_seq_read_jedec = {
@@ -439,6 +441,71 @@ static int stfsm_search_prepare_rw_seq(struct stfsm *fsm,
 	return 0;
 }
 
+static int stfsm_n25q_config(struct stfsm *fsm)
+{
+	uint32_t flags = fsm->info->flags;
+	uint8_t vcr;
+	int ret = 0;
+	bool soc_reset;
+
+	/* Configure 'READ' sequence */
+	if (flags & FLASH_FLAG_32BIT_ADDR)
+		ret = stfsm_search_prepare_rw_seq(fsm, &stfsm_seq_read,
+						  n25q_read4_configs);
+	else
+		ret = stfsm_search_prepare_rw_seq(fsm, &stfsm_seq_read,
+						  n25q_read3_configs);
+	if (ret) {
+		dev_err(fsm->dev,
+			"failed to prepare READ sequence with flags [0x%08x]\n",
+			flags);
+		return ret;
+	}
+
+	/* Configure 'WRITE' sequence (default configs) */
+	ret = stfsm_search_prepare_rw_seq(fsm, &stfsm_seq_write,
+					  default_write_configs);
+	if (ret) {
+		dev_err(fsm->dev,
+			"preparing WRITE sequence using flags [0x%08x] failed\n",
+			flags);
+		return ret;
+	}
+
+	/* * Configure 'ERASE_SECTOR' sequence */
+	stfsm_prepare_erasesec_seq(fsm, &stfsm_seq_erase_sector);
+
+	/* Configure 32-bit address support */
+	if (flags & FLASH_FLAG_32BIT_ADDR) {
+		stfsm_n25q_en_32bit_addr_seq(&stfsm_seq_en_32bit_addr);
+
+		soc_reset = stfsm_can_handle_soc_reset(fsm);
+		if (soc_reset || !fsm->booted_from_spi) {
+			/*
+			 * If we can handle SoC resets, we enable 32-bit
+			 * address mode pervasively
+			 */
+			stfsm_enter_32bit_addr(fsm, 1);
+		} else {
+			/*
+			 * If not, enable/disable for WRITE and ERASE
+			 * operations (READ uses special commands)
+			 */
+			fsm->configuration = (CFG_WRITE_TOGGLE_32BIT_ADDR |
+					      CFG_ERASESEC_TOGGLE_32BIT_ADDR);
+		}
+	}
+
+	/*
+	 * Configure device to use 8 dummy cycles
+	 */
+	vcr = (N25Q_VCR_DUMMY_CYCLES(8) | N25Q_VCR_XIP_DISABLED |
+	       N25Q_VCR_WRAP_CONT);
+	stfsm_wrvcr(fsm, vcr);
+
+	return 0;
+}
+
 static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
@@ -681,6 +748,16 @@ static int stfsm_probe(struct platform_device *pdev)
 
 	fsm->info = info;
 
+	/*
+	 * Configure READ/WRITE/ERASE sequences according to platform and
+	 * device flags.
+	 */
+	if (info->config) {
+		ret = info->config(fsm);
+		if (ret)
+			return ret;
+	}
+
 	platform_set_drvdata(pdev, fsm);
 
 	stfsm_fetch_platform_configs(pdev);
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 6699174..b5ce07d 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -304,6 +304,8 @@ struct flash_info {
 	int		(*config)(struct stfsm *);
 };
 
+static int stfsm_n25q_config(struct stfsm *fsm);
+
 static struct flash_info flash_types[] = {
 	/*
 	 * ST Microelectronics/Numonyx --
@@ -347,9 +349,10 @@ static struct flash_info flash_types[] = {
 		   FLASH_FLAG_WRITE_1_2_2	| \
 		   FLASH_FLAG_WRITE_1_1_4	| \
 		   FLASH_FLAG_WRITE_1_4_4)
-	{ "n25q128", 0x20ba18, 0, 64 * 1024,  256, N25Q_FLAG, 108, NULL },
+	{ "n25q128", 0x20ba18, 0, 64 * 1024,  256, N25Q_FLAG, 108,
+	  stfsm_n25q_config },
 	{ "n25q256", 0x20ba19, 0, 64 * 1024,  512,
-	  N25Q_FLAG | FLASH_FLAG_32BIT_ADDR, 108, NULL },
+	  N25Q_FLAG | FLASH_FLAG_32BIT_ADDR, 108, stfsm_n25q_config },
 
 	/*
 	 * Spansion S25FLxxxP
-- 
1.8.3.2

Re: [PATCH v3 24/36] mtd: st_spi_fsm: Supply the N25Qxxx chip specific configuration call-back

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:13PM +0000, Lee Jones wrote:
> In the FSM driver we handle chip differences by providing the possibility
> of calling back into a chip specific initialisation routine. In this patch
> we provide one for the N25Qxxx series, which endeavours to setup things
> like the read, write and erase sequences, as they differ from the
> default. We also configure 32bit support and the amount of dummy cycles to
> use.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mtd/devices/st_spi_fsm.c | 77 ++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/devices/st_spi_fsm.h |  7 ++--
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index 3a6b233..2369da1 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -98,6 +98,8 @@ static struct seq_rw_config n25q_read4_configs[] = {
>  	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
>  };
>  
> +static struct stfsm_seq stfsm_seq_read; 	/* Dynamically populated */
> +static struct stfsm_seq stfsm_seq_write;	/* Dynamically populated */

These should be fields in struct stfsm, no?

>  static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
>  
>  static struct stfsm_seq stfsm_seq_read_jedec = {
> diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
> index 6699174..b5ce07d 100644
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -304,6 +304,8 @@ struct flash_info {
>  	int		(*config)(struct stfsm *);
>  };
>  
> +static int stfsm_n25q_config(struct stfsm *fsm);
> +

I really dislike putting this static declaration in the header. (And for
now, I don't think you need this header at all, since very little of
your work is shareable yet.) Your table *looks* like it could be shared,
but it won't even compile if this header is included in other files,
since they don't have the definitions of your static functions.

Anyway, I think your table should be reworked if it's going to be useful
to others.

>  static struct flash_info flash_types[] = {
>  	/*
>  	 * ST Microelectronics/Numonyx --
> @@ -347,9 +349,10 @@ static struct flash_info flash_types[] = {
>  		   FLASH_FLAG_WRITE_1_2_2	| \
>  		   FLASH_FLAG_WRITE_1_1_4	| \
>  		   FLASH_FLAG_WRITE_1_4_4)
> -	{ "n25q128", 0x20ba18, 0, 64 * 1024,  256, N25Q_FLAG, 108, NULL },
> +	{ "n25q128", 0x20ba18, 0, 64 * 1024,  256, N25Q_FLAG, 108,
> +	  stfsm_n25q_config },
>  	{ "n25q256", 0x20ba19, 0, 64 * 1024,  512,
> -	  N25Q_FLAG | FLASH_FLAG_32BIT_ADDR, 108, NULL },
> +	  N25Q_FLAG | FLASH_FLAG_32BIT_ADDR, 108, stfsm_n25q_config },
>  
>  	/*
>  	 * Spansion S25FLxxxP

Brian

[PATCH v3 25/36] mtd: st_spi_fsm: Prepare default sequences for read/write/erase

[Lee Jones]

Most chips require a predefined set of FSM message sequences for read,
write and erase operations. This patch provides a way to set them up,
which it will do so if a chip specific initialisation routine isn't
been provided.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 2369da1..ad277f1 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -441,6 +441,38 @@ static int stfsm_search_prepare_rw_seq(struct stfsm *fsm,
 	return 0;
 }
 
+/* Prepare a READ/WRITE/ERASE 'default' sequences */
+static int stfsm_prepare_rwe_seqs_default(struct stfsm *fsm)
+{
+	uint32_t flags = fsm->info->flags;
+	int ret;
+
+	/* Configure 'READ' sequence */
+	ret = stfsm_search_prepare_rw_seq(fsm, &stfsm_seq_read,
+					  default_read_configs);
+	if (ret) {
+		dev_err(fsm->dev,
+			"failed to prep READ sequence with flags [0x%08x]\n",
+			flags);
+		return ret;
+	}
+
+	/* Configure 'WRITE' sequence */
+	ret = stfsm_search_prepare_rw_seq(fsm, &stfsm_seq_write,
+					  default_write_configs);
+	if (ret) {
+		dev_err(fsm->dev,
+			"failed to prep WRITE sequence with flags [0x%08x]\n",
+			flags);
+		return ret;
+	}
+
+	/* Configure 'ERASE_SECTOR' sequence */
+	stfsm_prepare_erasesec_seq(fsm, &stfsm_seq_erase_sector);
+
+	return 0;
+}
+
 static int stfsm_n25q_config(struct stfsm *fsm)
 {
 	uint32_t flags = fsm->info->flags;
@@ -756,6 +788,10 @@ static int stfsm_probe(struct platform_device *pdev)
 		ret = info->config(fsm);
 		if (ret)
 			return ret;
+	} else {
+		ret = stfsm_prepare_rwe_seqs_default(fsm);
+		if (ret)
+			return ret;
 	}
 
 	platform_set_drvdata(pdev, fsm);
-- 
1.8.3.2

[PATCH v3 26/36] mtd: st_spi_fsm: Add the ability to read from a Serial Flash device

[Lee Jones]

When a read is issued by userspace the MFD framework calls back into
the driver to conduct the actual command issue and data extraction.
Here we provide the routines which do exactly that.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 110 +++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |   3 ++
 2 files changed, 113 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index ad277f1..c76510e 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -538,6 +538,114 @@ static int stfsm_n25q_config(struct stfsm *fsm)
 	return 0;
 }
 
+static int stfsm_read(struct stfsm *fsm, uint8_t *const buf,
+		      const uint32_t size, const uint32_t offset)
+{
+	struct stfsm_seq *seq = &stfsm_seq_read;
+	uint32_t data_pads;
+	uint32_t read_mask;
+	uint32_t size_ub;
+	uint32_t size_lb;
+	uint32_t size_mop;
+	uint32_t tmp[4];
+	uint32_t page_buf[FLASH_PAGESIZE_32];
+	uint8_t *p;
+
+	dev_dbg(fsm->dev, "reading %d bytes from 0x%08x\n", size, offset);
+
+	/* Enter 32-bit address mode, if required */
+	if (fsm->configuration & CFG_READ_TOGGLE_32BIT_ADDR)
+		stfsm_enter_32bit_addr(fsm, 1);
+
+	/* Must read in multiples of 32 cycles (or 32*pads/8 Bytes) */
+	data_pads = ((seq->seq_cfg >> 16) & 0x3) + 1;
+	read_mask = (data_pads << 2) - 1;
+
+	/* Handle non-aligned buf */
+	p = ((uint32_t)buf & 0x3) ? (uint8_t *)page_buf : buf;
+
+	/* Handle non-aligned size */
+	size_ub = (size + read_mask) & ~read_mask;
+	size_lb = size & ~read_mask;
+	size_mop = size & read_mask;
+
+	seq->data_size = TRANSFER_SIZE(size_ub);
+	seq->addr1 = (offset >> 16) & 0xffff;
+	seq->addr2 = offset & 0xffff;
+
+	stfsm_load_seq(fsm, seq);
+
+	if (size_lb)
+		stfsm_read_fifo(fsm, (uint32_t *)p, size_lb);
+
+	if (size_mop) {
+		stfsm_read_fifo(fsm, tmp, read_mask + 1);
+		memcpy(p + size_lb, &tmp, size_mop);
+	}
+
+	/* Handle non-aligned buf */
+	if ((uint32_t)buf & 0x3)
+		memcpy(buf, page_buf, size);
+
+	/* Wait for sequence to finish */
+	stfsm_wait_seq(fsm);
+
+	stfsm_clear_fifo(fsm);
+
+	/* Exit 32-bit address mode, if required */
+	if (fsm->configuration & CFG_READ_TOGGLE_32BIT_ADDR)
+		stfsm_enter_32bit_addr(fsm, 0);
+
+	return 0;
+}
+
+/*
+ * Read an address range from the flash chip. The address range
+ * may be any size provided it is within the physical boundaries.
+ */
+static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
+			  size_t *retlen, u_char *buf)
+{
+	struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent);
+	uint32_t bytes;
+
+	dev_dbg(fsm->dev, "%s from 0x%08x, len %zd\n",
+		__func__, (u32)from, len);
+
+	/* Initialise read length */
+	if (retlen)
+		*retlen = 0;
+
+	if (!len) {
+		dev_warn(fsm->dev, "Zero byte read requested\n");
+		return 0;
+	}
+
+	if (from + len > mtd->size) {
+		dev_err(fsm->dev, "Can't read past end of chip\n");
+		return -EINVAL;
+	}
+
+	mutex_lock(&fsm->lock);
+
+	while (len > 0) {
+		bytes = min(len, (size_t)FLASH_PAGESIZE);
+
+		stfsm_read(fsm, buf, bytes, from);
+
+		buf += bytes;
+		from += bytes;
+		len -= bytes;
+
+		if (retlen)
+			*retlen += bytes;
+	}
+
+	mutex_unlock(&fsm->lock);
+
+	return 0;
+}
+
 static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
@@ -806,6 +914,8 @@ static int stfsm_probe(struct platform_device *pdev)
 	fsm->mtd.size		= info->sector_size * info->n_sectors;
 	fsm->mtd.erasesize	= info->sector_size;
 
+	fsm->mtd._read  = stfsm_mtd_read;
+
 	dev_err(&pdev->dev,
 		"Found serial flash device: %s\n"
 		" size = %llx (%lldMiB) erasesize = 0x%08x (%uKiB)\n",
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index b5ce07d..e168296 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -229,6 +229,9 @@
 #define FLASH_CMD_READ4_1_1_4	0x6c
 #define FLASH_CMD_READ4_1_4_4	0xec
 
+#define FLASH_PAGESIZE		256			/* In Bytes    */
+#define FLASH_PAGESIZE_32	FLASH_PAGESIZE / 4	/* In uint32_t */
+
 /*
  * Flags to tweak operation of default read/write/erase routines
  */
-- 
1.8.3.2

Re: [PATCH v3 26/36] mtd: st_spi_fsm: Add the ability to read from a Serial Flash device

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:15PM +0000, Lee Jones wrote:
> When a read is issued by userspace the MFD framework calls back into
> the driver to conduct the actual command issue and data extraction.
> Here we provide the routines which do exactly that.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mtd/devices/st_spi_fsm.c | 110 +++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/devices/st_spi_fsm.h |   3 ++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index ad277f1..c76510e 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -538,6 +538,114 @@ static int stfsm_n25q_config(struct stfsm *fsm)
>  	return 0;
>  }
>  
> +static int stfsm_read(struct stfsm *fsm, uint8_t *const buf,

Did you really mean uint8_t *const buf? This means that buf will always
hold the same address (which is fine). But we don't often enshrine that
in the function definition.

> +		      const uint32_t size, const uint32_t offset)

Similar. Is it really important to mark size and offset const?

I won't argue, if you think this is deserving, but I don't want to
encourage this practice everywhere, since it doesn't buy us much.
(Declaring a pointer argument as, e.g., 'const int *var' is useful in an
API, because it helps a developer understand externally that the routine
will not (in the absence of unsafe casts) modify the buffer contents.
But declaring 'int *const var' is less helpful, as it is only useful for
local reasoning.)

> +{

...

> +}
> +
> +/*
> + * Read an address range from the flash chip. The address range
> + * may be any size provided it is within the physical boundaries.
> + */
> +static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
> +			  size_t *retlen, u_char *buf)
> +{
> +	struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent);
> +	uint32_t bytes;
> +
> +	dev_dbg(fsm->dev, "%s from 0x%08x, len %zd\n",
> +		__func__, (u32)from, len);
> +
> +	/* Initialise read length */
> +	if (retlen)
> +		*retlen = 0;

You're repeating the common code in mtd_read(). Kill this
initialization.

> +
> +	if (!len) {
> +		dev_warn(fsm->dev, "Zero byte read requested\n");
> +		return 0;
> +	}

Similar. You're duplicating the checks in mtd_read().

> +
> +	if (from + len > mtd->size) {
> +		dev_err(fsm->dev, "Can't read past end of chip\n");
> +		return -EINVAL;
> +	}

Ditto.

> +
> +	mutex_lock(&fsm->lock);
> +
> +	while (len > 0) {
> +		bytes = min(len, (size_t)FLASH_PAGESIZE);
> +
> +		stfsm_read(fsm, buf, bytes, from);
> +
> +		buf += bytes;
> +		from += bytes;
> +		len -= bytes;
> +
> +		if (retlen)
> +			*retlen += bytes;
> +	}
> +
> +	mutex_unlock(&fsm->lock);
> +
> +	return 0;
> +}
> +
>  static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
>  {
>  	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
> diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
> index b5ce07d..e168296 100644
> --- a/drivers/mtd/devices/st_spi_fsm.h
> +++ b/drivers/mtd/devices/st_spi_fsm.h
> @@ -229,6 +229,9 @@
>  #define FLASH_CMD_READ4_1_1_4	0x6c
>  #define FLASH_CMD_READ4_1_4_4	0xec
>  
> +#define FLASH_PAGESIZE		256			/* In Bytes    */
> +#define FLASH_PAGESIZE_32	FLASH_PAGESIZE / 4	/* In uint32_t */

checkpatch.pl warns that you need parentheses around FLASH_PAGESIZE / 4.

Brian

[PATCH v3 27/36] mtd: st_spi_fsm: Write to Flash via the FSM FIFO

[Lee Jones]

When we write data to the FIFO the FSM Controller subsequently writes
that data out to the Serial Flash chip.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index c76510e..c08805f 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -261,6 +261,20 @@ static void stfsm_read_fifo(struct stfsm *fsm, uint32_t *buf,
 	}
 }
 
+static int stfsm_write_fifo(struct stfsm *fsm,
+			    const uint32_t *buf, const uint32_t size)
+{
+	uint32_t words = size >> 2;
+
+	dev_dbg(fsm->dev, "writing %d bytes to FIFO\n", size);
+
+	BUG_ON((((uint32_t)buf) & 0x3) || (size & 0x3));
+
+	writesl(fsm->base + SPI_FAST_SEQ_DATA_REG, buf, words);
+
+	return size;
+}
+
 static int stfsm_enter_32bit_addr(struct stfsm *fsm, int enter)
 {
 	struct stfsm_seq *seq = &stfsm_seq_en_32bit_addr;
-- 
1.8.3.2

[PATCH v3 28/36] mtd: st_spi_fsm: Supply a busy wait for post-write status

[Lee Jones]

When we write data to the Serial Flash chip we'll wait a predetermined
period of time before giving up. During that period of time we poll the
status register until completion.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 52 ++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h | 10 ++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index c08805f..d8fc97b 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -118,6 +118,22 @@ static struct stfsm_seq stfsm_seq_read_jedec = {
 		    SEQ_CFG_STARTSEQ),
 };
 
+static struct stfsm_seq stfsm_seq_read_status_fifo = {
+	.data_size = TRANSFER_SIZE(4),
+	.seq_opc[0] = (SEQ_OPC_PADS_1 |
+		       SEQ_OPC_CYCLES(8) |
+		       SEQ_OPC_OPCODE(FLASH_CMD_RDSR)),
+	.seq = {
+		STFSM_INST_CMD1,
+		STFSM_INST_DATA_READ,
+		STFSM_INST_STOP,
+	},
+	.seq_cfg = (SEQ_CFG_PADS_1 |
+		    SEQ_CFG_READNOTWRITE |
+		    SEQ_CFG_CSDEASSERT |
+		    SEQ_CFG_STARTSEQ),
+};
+
 static struct stfsm_seq stfsm_seq_erase_sector = {
 	/* 'addr_cfg' configured during initialisation */
 	.seq_opc = {
@@ -292,6 +308,42 @@ static int stfsm_enter_32bit_addr(struct stfsm *fsm, int enter)
 	return 0;
 }
 
+static uint8_t stfsm_wait_busy(struct stfsm *fsm)
+{
+	struct stfsm_seq *seq = &stfsm_seq_read_status_fifo;
+	unsigned long deadline;
+	uint32_t status;
+
+	/* Use RDRS1 */
+	seq->seq_opc[0] = (SEQ_OPC_PADS_1 |
+			   SEQ_OPC_CYCLES(8) |
+			   SEQ_OPC_OPCODE(FLASH_CMD_RDSR));
+
+	/* Load read_status sequence */
+	stfsm_load_seq(fsm, seq);
+
+	/* Repeat until busy bit is deasserted, or timeout */
+	deadline = jiffies + FLASH_MAX_BUSY_WAIT;
+	do {
+		cond_resched();
+
+		stfsm_wait_seq(fsm);
+
+		stfsm_read_fifo(fsm, &status, 4);
+
+		if ((status & FLASH_STATUS_BUSY) == 0)
+			return 0;
+
+		/* Restart */
+		writel(seq->seq_cfg, fsm->base + SPI_FAST_SEQ_CFG);
+
+	} while (!time_after_eq(jiffies, deadline));
+
+	dev_err(fsm->dev, "timeout on wait_busy\n");
+
+	return -EIO;
+}
+
 static int stfsm_wrvcr(struct stfsm *fsm, uint8_t data)
 {
 	struct stfsm_seq *seq = &stfsm_seq_wrvcr;
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index e168296..5143843 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -229,8 +229,18 @@
 #define FLASH_CMD_READ4_1_1_4	0x6c
 #define FLASH_CMD_READ4_1_4_4	0xec
 
+/* Status register */
+#define FLASH_STATUS_BUSY	0x01
+#define FLASH_STATUS_WEL	0x02
+#define FLASH_STATUS_BP0	0x04
+#define FLASH_STATUS_BP1	0x08
+#define FLASH_STATUS_BP2	0x10
+#define FLASH_STATUS_SRWP0	0x80
+#define FLASH_STATUS_TIMEOUT	0xff
+
 #define FLASH_PAGESIZE		256			/* In Bytes    */
 #define FLASH_PAGESIZE_32	FLASH_PAGESIZE / 4	/* In uint32_t */
+#define FLASH_MAX_BUSY_WAIT	(300 * HZ)	/* Maximum 'CHIPERASE' time */
 
 /*
  * Flags to tweak operation of default read/write/erase routines
-- 
1.8.3.2

Re: [PATCH v3 28/36] mtd: st_spi_fsm: Supply a busy wait for post-write status

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:17PM +0000, Lee Jones wrote:
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -292,6 +308,42 @@ static int stfsm_enter_32bit_addr(struct stfsm *fsm, int enter)
>  	return 0;
>  }
>  
> +static uint8_t stfsm_wait_busy(struct stfsm *fsm)
> +{
> +	struct stfsm_seq *seq = &stfsm_seq_read_status_fifo;
> +	unsigned long deadline;
> +	uint32_t status;
> +
> +	/* Use RDRS1 */
> +	seq->seq_opc[0] = (SEQ_OPC_PADS_1 |
> +			   SEQ_OPC_CYCLES(8) |
> +			   SEQ_OPC_OPCODE(FLASH_CMD_RDSR));
> +
> +	/* Load read_status sequence */
> +	stfsm_load_seq(fsm, seq);
> +
> +	/* Repeat until busy bit is deasserted, or timeout */
> +	deadline = jiffies + FLASH_MAX_BUSY_WAIT;
> +	do {
> +		cond_resched();
> +
> +		stfsm_wait_seq(fsm);
> +
> +		stfsm_read_fifo(fsm, &status, 4);
> +
> +		if ((status & FLASH_STATUS_BUSY) == 0)
> +			return 0;
> +
> +		/* Restart */
> +		writel(seq->seq_cfg, fsm->base + SPI_FAST_SEQ_CFG);
> +
> +	} while (!time_after_eq(jiffies, deadline));

You have a timeout problem here, similar to the one I mentioned earlier,
except that in this one, your do/while at least ensures that you always
run the loop at least once... but still, what if "a long time" elapses
between stfsm_read_fifo() and checking time_after_eq()?

I think you want something like this after the while():

	stfsm_read_fifo(fsm, &status, 4);
	if ((status & FLASH_STATUS_BUSY) == 0)
		return 0;

> +	dev_err(fsm->dev, "timeout on wait_busy\n");
> +
> +	return -EIO;
> +}
> +
>  static int stfsm_wrvcr(struct stfsm *fsm, uint8_t data)
>  {
>  	struct stfsm_seq *seq = &stfsm_seq_wrvcr;

Brian

[PATCH v3 29/36] mtd: st_spi_fsm: Add the ability to write to a Serial Flash device

[Lee Jones]

When a write is requested by userspace the MFD framework calls back
into the driver to conduct the actual command issue and data send.
Here we provide the routines which do exactly that.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 150 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index d8fc97b..9b35e15 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -665,6 +665,98 @@ static int stfsm_read(struct stfsm *fsm, uint8_t *const buf,
 	return 0;
 }
 
+static int stfsm_write(struct stfsm *fsm, const uint8_t *const buf,
+		       const uint32_t size, const uint32_t offset)
+{
+	struct stfsm_seq *seq = &stfsm_seq_write;
+	uint32_t data_pads;
+	uint32_t write_mask;
+	uint32_t size_ub;
+	uint32_t size_lb;
+	uint32_t size_mop;
+	uint32_t tmp[4];
+	uint32_t page_buf[FLASH_PAGESIZE_32];
+	uint8_t *t = (uint8_t *)&tmp;
+	const uint8_t *p;
+	int ret;
+	int i;
+
+	dev_dbg(fsm->dev, "writing %d bytes to 0x%08x\n", size, offset);
+
+	/* Enter 32-bit address mode, if required */
+	if (fsm->configuration & CFG_WRITE_TOGGLE_32BIT_ADDR)
+		stfsm_enter_32bit_addr(fsm, 1);
+
+	/* Must write in multiples of 32 cycles (or 32*pads/8 bytes) */
+	data_pads = ((seq->seq_cfg >> 16) & 0x3) + 1;
+	write_mask = (data_pads << 2) - 1;
+
+	/* Handle non-aligned buf */
+	if ((uint32_t)buf & 0x3) {
+		memcpy(page_buf, buf, size);
+		p = (uint8_t *)page_buf;
+	} else {
+		p = buf;
+	}
+
+	/* Handle non-aligned size */
+	size_ub = (size + write_mask) & ~write_mask;
+	size_lb = size & ~write_mask;
+	size_mop = size & write_mask;
+
+	seq->data_size = TRANSFER_SIZE(size_ub);
+	seq->addr1 = (offset >> 16) & 0xffff;
+	seq->addr2 = offset & 0xffff;
+
+	/* Need to set FIFO to write mode, before writing data to FIFO (see
+	 * GNBvb79594)
+	 */
+	writel(0x00040000, fsm->base + SPI_FAST_SEQ_CFG);
+
+	/*
+	 * Before writing data to the FIFO, apply a small delay to allow a
+	 * potential change of FIFO direction to complete.
+	 */
+	if (fsm->fifo_dir_delay == 0)
+		readl(fsm->base + SPI_FAST_SEQ_CFG);
+	else
+		udelay(fsm->fifo_dir_delay);
+
+
+	/* Write data to FIFO, before starting sequence (see GNBvd79593) */
+	if (size_lb) {
+		stfsm_write_fifo(fsm, (uint32_t *)p, size_lb);
+		p += size_lb;
+	}
+
+	/* Handle non-aligned size */
+	if (size_mop) {
+		memset(t, 0xff, write_mask + 1);	/* fill with 0xff's */
+		for (i = 0; i < size_mop; i++)
+			t[i] = *p++;
+
+		stfsm_write_fifo(fsm, tmp, write_mask + 1);
+	}
+
+	/* Start sequence */
+	stfsm_load_seq(fsm, seq);
+
+	/* Wait for sequence to finish */
+	stfsm_wait_seq(fsm);
+
+	/* Wait for completion */
+	ret = stfsm_wait_busy(fsm);
+
+	/* Exit 32-bit address mode, if required */
+	if (fsm->configuration & CFG_WRITE_TOGGLE_32BIT_ADDR) {
+		stfsm_enter_32bit_addr(fsm, 0);
+		if (fsm->configuration & CFG_WRITE_EX_32BIT_ADDR_DELAY)
+			udelay(1);
+	}
+
+	return 0;
+}
+
 /*
  * Read an address range from the flash chip. The address range
  * may be any size provided it is within the physical boundaries.
@@ -712,6 +804,63 @@ static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
 	return 0;
 }
 
+/*
+ * Write an address range to the flash chip.  Data must be written in
+ * FLASH_PAGESIZE chunks.  The address range may be any size provided
+ * it is within the physical boundaries.
+ */
+static int stfsm_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
+			   size_t *retlen, const u_char *buf)
+{
+	struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent);
+
+	u32 page_offs;
+	u32 bytes;
+	uint8_t *b = (uint8_t *)buf;
+	int ret = 0;
+
+	dev_dbg(fsm->dev, "%s to 0x%08x, len %zd\n", __func__, (u32)to, len);
+
+	if (retlen)
+		*retlen = 0;
+
+	if (!len)
+		return 0;
+
+	if (to + len > mtd->size)
+		return -EINVAL;
+
+	/* Offset within page */
+	page_offs = to % FLASH_PAGESIZE;
+
+	mutex_lock(&fsm->lock);
+
+	while (len) {
+		/* Write up to page boundary */
+		bytes = min(FLASH_PAGESIZE - page_offs, len);
+
+		ret = stfsm_write(fsm, b, bytes, to);
+		if (ret)
+			goto out1;
+
+		b += bytes;
+		len -= bytes;
+		to += bytes;
+
+		/* We are now page-aligned */
+		page_offs = 0;
+
+		if (retlen)
+			*retlen += bytes;
+
+	}
+
+out1:
+	mutex_unlock(&fsm->lock);
+
+	return ret;
+}
+
 static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
@@ -981,6 +1130,7 @@ static int stfsm_probe(struct platform_device *pdev)
 	fsm->mtd.erasesize	= info->sector_size;
 
 	fsm->mtd._read  = stfsm_mtd_read;
+	fsm->mtd._write = stfsm_mtd_write;
 
 	dev_err(&pdev->dev,
 		"Found serial flash device: %s\n"
-- 
1.8.3.2

[PATCH v3 30/36] mtd: st_spi_fsm: Erase partly or as a whole a Serial Flash device

[Lee Jones]

When an erase is requested by userspace the MFD framework calls back
into the driver to conduct the actual command issue. Here we provide the
routines which do exactly that. We can choose to either do an entire chip
erase or by sector.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 118 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 117 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 9b35e15..ecb001d 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -156,6 +156,27 @@ static struct stfsm_seq stfsm_seq_erase_sector = {
 		    SEQ_CFG_STARTSEQ),
 };
 
+static struct stfsm_seq stfsm_seq_erase_chip = {
+	.seq_opc = {
+		(SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		 SEQ_OPC_OPCODE(FLASH_CMD_WREN) | SEQ_OPC_CSDEASSERT),
+
+		(SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		 SEQ_OPC_OPCODE(FLASH_CMD_CHIPERASE) | SEQ_OPC_CSDEASSERT),
+	},
+	.seq = {
+		STFSM_INST_CMD1,
+		STFSM_INST_CMD2,
+		STFSM_INST_WAIT,
+		STFSM_INST_STOP,
+	},
+	.seq_cfg = (SEQ_CFG_PADS_1 |
+		    SEQ_CFG_ERASE |
+		    SEQ_CFG_READNOTWRITE |
+		    SEQ_CFG_CSDEASSERT |
+		    SEQ_CFG_STARTSEQ),
+};
+
 static struct stfsm_seq stfsm_seq_wrvcr = {
 	.seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
 		       SEQ_OPC_OPCODE(FLASH_CMD_WREN) | SEQ_OPC_CSDEASSERT),
@@ -804,6 +825,47 @@ static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
 	return 0;
 }
 
+static int stfsm_erase_sector(struct stfsm *fsm, const uint32_t offset)
+{
+	struct stfsm_seq *seq = &stfsm_seq_erase_sector;
+	int ret;
+
+	dev_dbg(fsm->dev, "erasing sector at 0x%08x\n", offset);
+
+	/* Enter 32-bit address mode, if required */
+	if (fsm->configuration & CFG_ERASESEC_TOGGLE_32BIT_ADDR)
+		stfsm_enter_32bit_addr(fsm, 1);
+
+	seq->addr1 = (offset >> 16) & 0xffff;
+	seq->addr2 = offset & 0xffff;
+
+	stfsm_load_seq(fsm, seq);
+
+	stfsm_wait_seq(fsm);
+
+	/* Wait for completion */
+	ret = stfsm_wait_busy(fsm);
+
+	/* Exit 32-bit address mode, if required */
+	if (fsm->configuration & CFG_ERASESEC_TOGGLE_32BIT_ADDR)
+		stfsm_enter_32bit_addr(fsm, 0);
+
+	return ret;
+}
+
+static int stfsm_erase_chip(struct stfsm *fsm)
+{
+	const struct stfsm_seq *seq = &stfsm_seq_erase_chip;
+
+	dev_dbg(fsm->dev, "erasing chip\n");
+
+	stfsm_load_seq(fsm, seq);
+
+	stfsm_wait_seq(fsm);
+
+	return stfsm_wait_busy(fsm);
+}
+
 /*
  * Write an address range to the flash chip.  Data must be written in
  * FLASH_PAGESIZE chunks.  The address range may be any size provided
@@ -861,6 +923,60 @@ out1:
 	return ret;
 }
 
+/*
+ * Erase an address range on the flash chip. The address range may extend
+ * one or more erase sectors.  Return an error is there is a problem erasing.
+ */
+static int stfsm_mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent);
+	u32 addr, len;
+	int ret;
+
+	dev_dbg(fsm->dev, "%s at 0x%llx, len %lld\n", __func__,
+		(long long)instr->addr, (long long)instr->len);
+
+	if (instr->addr + instr->len > mtd->size)
+		return -EINVAL;
+
+	if (instr->len & (mtd->erasesize - 1))
+		return -EINVAL;
+
+	addr = instr->addr;
+	len = instr->len;
+
+	mutex_lock(&fsm->lock);
+
+	/* Whole-chip erase? */
+	if (len == mtd->size) {
+		ret = stfsm_erase_chip(fsm);
+		if (ret)
+			goto out1;
+	} else {
+		while (len) {
+			ret = stfsm_erase_sector(fsm, addr);
+			if (ret)
+				goto out1;
+
+			addr += mtd->erasesize;
+			len -= mtd->erasesize;
+		}
+	}
+
+	mutex_unlock(&fsm->lock);
+
+	instr->state = MTD_ERASE_DONE;
+	mtd_erase_callback(instr);
+
+	return 0;
+
+out1:
+	instr->state = MTD_ERASE_FAILED;
+	mutex_unlock(&fsm->lock);
+
+	return ret;
+}
+
 static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
 {
 	const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
@@ -1131,7 +1247,7 @@ static int stfsm_probe(struct platform_device *pdev)
 
 	fsm->mtd._read  = stfsm_mtd_read;
 	fsm->mtd._write = stfsm_mtd_write;
-
+	fsm->mtd._erase = stfsm_mtd_erase;
 	dev_err(&pdev->dev,
 		"Found serial flash device: %s\n"
 		" size = %llx (%lldMiB) erasesize = 0x%08x (%uKiB)\n",
-- 
1.8.3.2

Re: [PATCH v3 30/36] mtd: st_spi_fsm: Erase partly or as a whole a Serial Flash device

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:19PM +0000, Lee Jones wrote:
> When an erase is requested by userspace the MFD framework calls back

s/MFD/MTD/

;)

> into the driver to conduct the actual command issue. Here we provide the
> routines which do exactly that. We can choose to either do an entire chip
> erase or by sector.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mtd/devices/st_spi_fsm.c | 118 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 117 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index 9b35e15..ecb001d 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -861,6 +923,60 @@ out1:
>  	return ret;
>  }
>  
> +/*
> + * Erase an address range on the flash chip. The address range may extend
> + * one or more erase sectors.  Return an error is there is a problem erasing.
> + */
> +static int stfsm_mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
> +{
> +	struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent);
> +	u32 addr, len;
> +	int ret;
> +
> +	dev_dbg(fsm->dev, "%s at 0x%llx, len %lld\n", __func__,
> +		(long long)instr->addr, (long long)instr->len);
> +
> +	if (instr->addr + instr->len > mtd->size)
> +		return -EINVAL;

Duplicated from mtd_read(). Kill this.

> +
> +	if (instr->len & (mtd->erasesize - 1))
> +		return -EINVAL;

...

> @@ -1131,7 +1247,7 @@ static int stfsm_probe(struct platform_device *pdev)
>  
>  	fsm->mtd._read  = stfsm_mtd_read;
>  	fsm->mtd._write = stfsm_mtd_write;
> -
> +	fsm->mtd._erase = stfsm_mtd_erase;

Can you leave the blank line between assigning fsm->mtd.xxx callbacks
and the dev_err()?

>  	dev_err(&pdev->dev,
>  		"Found serial flash device: %s\n"
>  		" size = %llx (%lldMiB) erasesize = 0x%08x (%uKiB)\n",

(BTW, why is this dev_err()?? It's just information, so you want
dev_info().)

Brian

[PATCH v3 31/36] mtd: st_spi_fsm: Add the ability to read the FSM's status

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index ecb001d..1d6bd94 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -365,6 +365,30 @@ static uint8_t stfsm_wait_busy(struct stfsm *fsm)
 	return -EIO;
 }
 
+static int stfsm_read_status(struct stfsm *fsm, uint8_t cmd,
+			   uint8_t *status)
+{
+	struct stfsm_seq *seq = &stfsm_seq_read_status_fifo;
+	uint32_t tmp;
+
+	dev_dbg(fsm->dev, "reading STA[%s]\n",
+		(cmd == FLASH_CMD_RDSR) ? "1" : "2");
+
+	seq->seq_opc[0] = (SEQ_OPC_PADS_1 |
+			   SEQ_OPC_CYCLES(8) |
+			   SEQ_OPC_OPCODE(cmd)),
+
+	stfsm_load_seq(fsm, seq);
+
+	stfsm_read_fifo(fsm, &tmp, 4);
+
+	*status = (uint8_t)(tmp >> 24);
+
+	stfsm_wait_seq(fsm);
+
+	return 0;
+}
+
 static int stfsm_wrvcr(struct stfsm *fsm, uint8_t data)
 {
 	struct stfsm_seq *seq = &stfsm_seq_wrvcr;
-- 
1.8.3.2

[PATCH v3 32/36] mtd: st_spi_fsm: Add the ability to write to FSM's status register

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 1d6bd94..f1276e5 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -177,6 +177,23 @@ static struct stfsm_seq stfsm_seq_erase_chip = {
 		    SEQ_CFG_STARTSEQ),
 };
 
+static struct stfsm_seq stfsm_seq_write_status = {
+	.seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		       SEQ_OPC_OPCODE(FLASH_CMD_WREN) | SEQ_OPC_CSDEASSERT),
+	.seq_opc[1] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+		       SEQ_OPC_OPCODE(FLASH_CMD_WRSR)),
+	.seq = {
+		STFSM_INST_CMD1,
+		STFSM_INST_CMD2,
+		STFSM_INST_STA_WR1,
+		STFSM_INST_STOP,
+	},
+	.seq_cfg = (SEQ_CFG_PADS_1 |
+		    SEQ_CFG_READNOTWRITE |
+		    SEQ_CFG_CSDEASSERT |
+		    SEQ_CFG_STARTSEQ),
+};
+
 static struct stfsm_seq stfsm_seq_wrvcr = {
 	.seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
 		       SEQ_OPC_OPCODE(FLASH_CMD_WREN) | SEQ_OPC_CSDEASSERT),
@@ -389,6 +406,25 @@ static int stfsm_read_status(struct stfsm *fsm, uint8_t cmd,
 	return 0;
 }
 
+static int stfsm_write_status(struct stfsm *fsm, uint16_t status,
+			       int sta_bytes)
+{
+	struct stfsm_seq *seq = &stfsm_seq_write_status;
+
+	dev_dbg(fsm->dev, "writing STA[%s] 0x%04x\n",
+		(sta_bytes == 1) ? "1" : "1+2", status);
+
+	seq->status = (uint32_t)status | STA_PADS_1 | STA_CSDEASSERT;
+	seq->seq[2] = (sta_bytes == 1) ?
+		STFSM_INST_STA_WR1 : STFSM_INST_STA_WR1_2;
+
+	stfsm_load_seq(fsm, seq);
+
+	stfsm_wait_seq(fsm);
+
+	return 0;
+};
+
 static int stfsm_wrvcr(struct stfsm *fsm, uint8_t data)
 {
 	struct stfsm_seq *seq = &stfsm_seq_wrvcr;
-- 
1.8.3.2

[PATCH v3 33/36] mtd: st_spi_fsm: Supply the MX25xxx chip specific configuration call-back

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 84 ++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |  4 +-
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index f1276e5..be66a49 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -98,6 +98,31 @@ static struct seq_rw_config n25q_read4_configs[] = {
 	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
 };
 
+/*
+ * [MX25xxx] Configuration
+ */
+#define MX25_STATUS_QE			(0x1 << 6)
+
+static int stfsm_mx25_en_32bit_addr_seq(struct stfsm_seq *seq)
+{
+	seq->seq_opc[0] = (SEQ_OPC_PADS_1 |
+			   SEQ_OPC_CYCLES(8) |
+			   SEQ_OPC_OPCODE(FLASH_CMD_EN4B_ADDR) |
+			   SEQ_OPC_CSDEASSERT);
+
+	seq->seq[0] = STFSM_INST_CMD1;
+	seq->seq[1] = STFSM_INST_WAIT;
+	seq->seq[2] = STFSM_INST_STOP;
+
+	seq->seq_cfg = (SEQ_CFG_PADS_1 |
+			SEQ_CFG_ERASE |
+			SEQ_CFG_READNOTWRITE |
+			SEQ_CFG_CSDEASSERT |
+			SEQ_CFG_STARTSEQ);
+
+	return 0;
+}
+
 static struct stfsm_seq stfsm_seq_read; 	/* Dynamically populated */
 static struct stfsm_seq stfsm_seq_write;	/* Dynamically populated */
 static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
@@ -620,6 +645,65 @@ static int stfsm_prepare_rwe_seqs_default(struct stfsm *fsm)
 	return 0;
 }
 
+static int stfsm_mx25_config(struct stfsm *fsm)
+{
+	uint32_t flags = fsm->info->flags;
+	uint32_t data_pads;
+	uint8_t sta;
+	int ret;
+	bool soc_reset;
+
+	/* Disable support for 'WRITE_1_4_4' (limited to 20MHz which is of
+	 * marginal benefit on our hardware and doesn't justify implementing
+	 * different READ/WRITE frequencies).
+	 */
+	flags &= ~FLASH_FLAG_WRITE_1_4_4;
+
+	/*
+	 * Use default READ/WRITE sequences
+	 */
+	ret = stfsm_prepare_rwe_seqs_default(fsm);
+	if (ret)
+		return ret;
+
+	/*
+	 * Configure 32-bit Address Support
+	 */
+	if (flags & FLASH_FLAG_32BIT_ADDR) {
+		/* Configure 'enter_32bitaddr' FSM sequence */
+		stfsm_mx25_en_32bit_addr_seq(&stfsm_seq_en_32bit_addr);
+
+		soc_reset = stfsm_can_handle_soc_reset(fsm);
+		if (soc_reset || !fsm->booted_from_spi) {
+			/* If we can handle SoC resets, we enable 32-bit address
+			 * mode pervasively */
+			stfsm_enter_32bit_addr(fsm, 1);
+
+		} else {
+			/* Else, enable/disable 32-bit addressing before/after
+			 * each operation */
+			fsm->configuration = (CFG_READ_TOGGLE_32BIT_ADDR |
+					      CFG_WRITE_TOGGLE_32BIT_ADDR |
+					      CFG_ERASESEC_TOGGLE_32BIT_ADDR);
+			/* It seems a small delay is required after exiting
+			 * 32-bit mode following a write operation.  The issue
+			 * is under investigation.
+			 */
+			fsm->configuration |= CFG_WRITE_EX_32BIT_ADDR_DELAY;
+		}
+	}
+
+	/* For QUAD mode, set 'QE' STATUS bit */
+	data_pads = ((stfsm_seq_read.seq_cfg >> 16) & 0x3) + 1;
+	if (data_pads == 4) {
+		stfsm_read_status(fsm, FLASH_CMD_RDSR, &sta);
+		sta |= MX25_STATUS_QE;
+		stfsm_write_status(fsm, sta, 1);
+	}
+
+	return 0;
+}
+
 static int stfsm_n25q_config(struct stfsm *fsm)
 {
 	uint32_t flags = fsm->info->flags;
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 5143843..10c12ba 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -318,6 +318,7 @@ struct flash_info {
 };
 
 static int stfsm_n25q_config(struct stfsm *fsm);
+static int stfsm_mx25_config(struct stfsm *fsm);
 
 static struct flash_info flash_types[] = {
 	/*
@@ -350,7 +351,8 @@ static struct flash_info flash_types[] = {
 		   FLASH_FLAG_SE_4K		| \
 		   FLASH_FLAG_SE_32K)
 	{ "mx25l25635e", 0xc22019, 0, 64*1024, 512,
-	  (MX25_FLAG | FLASH_FLAG_32BIT_ADDR | FLASH_FLAG_RESET), 70, NULL },
+	  (MX25_FLAG | FLASH_FLAG_32BIT_ADDR | FLASH_FLAG_RESET), 70,
+	  stfsm_mx25_config },
 
 #define N25Q_FLAG (FLASH_FLAG_READ_WRITE	| \
 		   FLASH_FLAG_READ_FAST		| \
-- 
1.8.3.2

Re: [PATCH v3 33/36] mtd: st_spi_fsm: Supply the MX25xxx chip specific configuration call-back

[Brian Norris]

On Fri, Nov 29, 2013 at 12:19:22PM +0000, Lee Jones wrote:
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mtd/devices/st_spi_fsm.c | 84 ++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/devices/st_spi_fsm.h |  4 +-
>  2 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index f1276e5..be66a49 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -620,6 +645,65 @@ static int stfsm_prepare_rwe_seqs_default(struct stfsm *fsm)
>  	return 0;
>  }
>  
> +static int stfsm_mx25_config(struct stfsm *fsm)
> +{
> +	uint32_t flags = fsm->info->flags;
> +	uint32_t data_pads;
> +	uint8_t sta;
> +	int ret;
> +	bool soc_reset;
> +
> +	/* Disable support for 'WRITE_1_4_4' (limited to 20MHz which is of
> +	 * marginal benefit on our hardware and doesn't justify implementing
> +	 * different READ/WRITE frequencies).
> +	 */
> +	flags &= ~FLASH_FLAG_WRITE_1_4_4;

Huh? flags is a local variable, and you only use it for checking 32-bit
addressing mode in this function. So this flags modification is
effectively thrown away. Perhaps you meant

	fsm->info->flags &= ~FLASH_FLAG_WRITE_1_4_4;

? But then you're back to modifying static data (the device table)
through a per-instance reference. That's not good behavior. Rather,
couldn't you just remove this flag from the table entry in the first
place?

Brian

[PATCH v3 34/36] mtd: st_spi_fsm: Supply the S25FLxxx chip specific configuration call-back

[Lee Jones]

This patch allows us to prepare some of the message sequences which will
be required to talk to the  S25FLxxx family of Serial Flash devices. It
also allows us to do some required extra operations after any busy wait
failures.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 245 +++++++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h |  26 ++++-
 2 files changed, 265 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index be66a49..d31348a 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -123,6 +123,33 @@ static int stfsm_mx25_en_32bit_addr_seq(struct stfsm_seq *seq)
 	return 0;
 }
 
+/*
+ * [S25FLxxx] Configuration
+ */
+#define STFSM_S25FL_CONFIG_QE		(0x1 << 1)
+
+/*
+ * S25FLxxxS devices provide three ways of supporting 32-bit addressing: Bank
+ * Register, Extended Address Modes, and a 32-bit address command set.  The
+ * 32-bit address command set is used here, since it avoids any problems with
+ * entering a state that is incompatible with the SPIBoot Controller.
+ */
+static struct seq_rw_config stfsm_s25fl_read4_configs[] = {
+	{FLASH_FLAG_READ_1_4_4,  FLASH_CMD_READ4_1_4_4,  0, 4, 4, 0x00, 2, 4},
+	{FLASH_FLAG_READ_1_1_4,  FLASH_CMD_READ4_1_1_4,  0, 1, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_2_2,  FLASH_CMD_READ4_1_2_2,  0, 2, 2, 0x00, 4, 0},
+	{FLASH_FLAG_READ_1_1_2,  FLASH_CMD_READ4_1_1_2,  0, 1, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_FAST,   FLASH_CMD_READ4_FAST,   0, 1, 1, 0x00, 0, 8},
+	{FLASH_FLAG_READ_WRITE,  FLASH_CMD_READ4,        0, 1, 1, 0x00, 0, 0},
+	{0x00,                   0,                      0, 0, 0, 0x00, 0, 0},
+};
+
+static struct seq_rw_config stfsm_s25fl_write4_configs[] = {
+	{FLASH_FLAG_WRITE_1_1_4, S25FL_CMD_WRITE4_1_1_4, 1, 1, 4, 0x00, 0, 0},
+	{FLASH_FLAG_READ_WRITE,  S25FL_CMD_WRITE4,       1, 1, 1, 0x00, 0, 0},
+	{0x00,                   0,                      0, 0, 0, 0x00, 0, 0},
+};
+
 static struct stfsm_seq stfsm_seq_read; 	/* Dynamically populated */
 static struct stfsm_seq stfsm_seq_write;	/* Dynamically populated */
 static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
@@ -397,6 +424,11 @@ static uint8_t stfsm_wait_busy(struct stfsm *fsm)
 		if ((status & FLASH_STATUS_BUSY) == 0)
 			return 0;
 
+		if ((fsm->configuration & CFG_S25FL_CHECK_ERROR_FLAGS) &&
+		    ((status & S25FL_STATUS_P_ERR) ||
+		     (status & S25FL_STATUS_E_ERR)))
+			return (uint8_t)(status & 0xff);
+
 		/* Restart */
 		writel(seq->seq_cfg, fsm->base + SPI_FAST_SEQ_CFG);
 
@@ -769,6 +801,215 @@ static int stfsm_n25q_config(struct stfsm *fsm)
 	return 0;
 }
 
+static void stfsm_s25fl_prepare_erasesec_seq_32(struct stfsm_seq *seq)
+{
+	seq->seq_opc[1] = (SEQ_OPC_PADS_1 |
+			   SEQ_OPC_CYCLES(8) |
+			   SEQ_OPC_OPCODE(S25FL_CMD_SE4));
+
+	seq->addr_cfg = (ADR_CFG_CYCLES_ADD1(16) |
+			 ADR_CFG_PADS_1_ADD1 |
+			 ADR_CFG_CYCLES_ADD2(16) |
+			 ADR_CFG_PADS_1_ADD2 |
+			 ADR_CFG_CSDEASSERT_ADD2);
+}
+
+static void stfsm_s25fl_read_dyb(struct stfsm *fsm, uint32_t offs, uint8_t *dby)
+{
+	uint32_t tmp;
+	struct stfsm_seq seq = {
+		.data_size = TRANSFER_SIZE(4),
+		.seq_opc[0] = (SEQ_OPC_PADS_1 |
+			       SEQ_OPC_CYCLES(8) |
+			       SEQ_OPC_OPCODE(S25FL_CMD_DYBRD)),
+		.addr_cfg = (ADR_CFG_CYCLES_ADD1(16) |
+			     ADR_CFG_PADS_1_ADD1 |
+			     ADR_CFG_CYCLES_ADD2(16) |
+			     ADR_CFG_PADS_1_ADD2),
+		.addr1 = (offs >> 16) & 0xffff,
+		.addr2 = offs & 0xffff,
+		.seq = {
+			STFSM_INST_CMD1,
+			STFSM_INST_ADD1,
+			STFSM_INST_ADD2,
+			STFSM_INST_DATA_READ,
+			STFSM_INST_STOP,
+		},
+		.seq_cfg = (SEQ_CFG_PADS_1 |
+			    SEQ_CFG_READNOTWRITE |
+			    SEQ_CFG_CSDEASSERT |
+			    SEQ_CFG_STARTSEQ),
+	};
+
+	stfsm_load_seq(fsm, &seq);
+
+	stfsm_read_fifo(fsm, &tmp, 4);
+
+	*dby = (uint8_t)(tmp >> 24);
+
+	stfsm_wait_seq(fsm);
+}
+
+static void stfsm_s25fl_write_dyb(struct stfsm *fsm, uint32_t offs, uint8_t dby)
+{
+	struct stfsm_seq seq = {
+		.seq_opc[0] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+			       SEQ_OPC_OPCODE(FLASH_CMD_WREN) |
+			       SEQ_OPC_CSDEASSERT),
+		.seq_opc[1] = (SEQ_OPC_PADS_1 | SEQ_OPC_CYCLES(8) |
+			       SEQ_OPC_OPCODE(S25FL_CMD_DYBWR)),
+		.addr_cfg = (ADR_CFG_CYCLES_ADD1(16) |
+			     ADR_CFG_PADS_1_ADD1 |
+			     ADR_CFG_CYCLES_ADD2(16) |
+			     ADR_CFG_PADS_1_ADD2),
+		.status = (uint32_t)dby | STA_PADS_1 | STA_CSDEASSERT,
+		.addr1 = (offs >> 16) & 0xffff,
+		.addr2 = offs & 0xffff,
+		.seq = {
+			STFSM_INST_CMD1,
+			STFSM_INST_CMD2,
+			STFSM_INST_ADD1,
+			STFSM_INST_ADD2,
+			STFSM_INST_STA_WR1,
+			STFSM_INST_STOP,
+		},
+		.seq_cfg = (SEQ_CFG_PADS_1 |
+			    SEQ_CFG_READNOTWRITE |
+			    SEQ_CFG_CSDEASSERT |
+			    SEQ_CFG_STARTSEQ),
+	};
+
+	stfsm_load_seq(fsm, &seq);
+	stfsm_wait_seq(fsm);
+
+	stfsm_wait_busy(fsm);
+}
+
+static int stfsm_s25fl_clear_status_reg(struct stfsm *fsm)
+{
+	struct stfsm_seq seq = {
+		.seq_opc[0] = (SEQ_OPC_PADS_1 |
+			       SEQ_OPC_CYCLES(8) |
+			       SEQ_OPC_OPCODE(S25FL_CMD_CLSR) |
+			       SEQ_OPC_CSDEASSERT),
+		.seq_opc[1] = (SEQ_OPC_PADS_1 |
+			       SEQ_OPC_CYCLES(8) |
+			       SEQ_OPC_OPCODE(FLASH_CMD_WRDI) |
+			       SEQ_OPC_CSDEASSERT),
+		.seq = {
+			STFSM_INST_CMD1,
+			STFSM_INST_CMD2,
+			STFSM_INST_WAIT,
+			STFSM_INST_STOP,
+		},
+		.seq_cfg = (SEQ_CFG_PADS_1 |
+			    SEQ_CFG_ERASE |
+			    SEQ_CFG_READNOTWRITE |
+			    SEQ_CFG_CSDEASSERT |
+			    SEQ_CFG_STARTSEQ),
+	};
+
+	stfsm_load_seq(fsm, &seq);
+
+	stfsm_wait_seq(fsm);
+
+	return 0;
+}
+
+static int stfsm_s25fl_config(struct stfsm *fsm)
+{
+	struct flash_info *info = fsm->info;
+	uint32_t flags = info->flags;
+	uint32_t data_pads;
+	uint32_t offs;
+	uint16_t sta_wr;
+	uint8_t sr1, cr1, dyb;
+	int ret;
+
+	if (flags & FLASH_FLAG_32BIT_ADDR) {
+		/*
+		 * Prepare Read/Write/Erase sequences according to S25FLxxx
+		 * 32-bit address command set
+		 */
+		ret = stfsm_search_prepare_rw_seq(fsm, &stfsm_seq_read,
+						  stfsm_s25fl_read4_configs);
+		if (ret)
+			return ret;
+
+		ret = stfsm_search_prepare_rw_seq(fsm, &stfsm_seq_write,
+						  stfsm_s25fl_write4_configs);
+		if (ret)
+			return ret;
+
+		stfsm_s25fl_prepare_erasesec_seq_32(&stfsm_seq_erase_sector);
+
+	} else {
+		/* Use default configurations for 24-bit addressing */
+		ret = stfsm_prepare_rwe_seqs_default(fsm);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * For devices that support 'DYB' sector locking, check lock status and
+	 * unlock sectors if necessary (some variants power-on with sectors
+	 * locked by default)
+	 */
+	if (flags & FLASH_FLAG_DYB_LOCKING) {
+		offs = 0;
+		for (offs = 0; offs < info->sector_size * info->n_sectors;) {
+			stfsm_s25fl_read_dyb(fsm, offs, &dyb);
+			if (dyb == 0x00)
+				stfsm_s25fl_write_dyb(fsm, offs, 0xff);
+
+			/* Handle bottom/top 4KiB parameter sectors */
+			if ((offs < info->sector_size * 2) ||
+			    (offs >= (info->sector_size - info->n_sectors * 4)))
+				offs += 0x1000;
+			else
+				offs += 0x10000;
+		}
+	}
+
+	/* Check status of 'QE' bit */
+	data_pads = ((stfsm_seq_read.seq_cfg >> 16) & 0x3) + 1;
+	stfsm_read_status(fsm, FLASH_CMD_RDSR2, &cr1);
+	if (data_pads == 4) {
+		if (!(cr1 & STFSM_S25FL_CONFIG_QE)) {
+			/* Set 'QE' */
+			cr1 |= STFSM_S25FL_CONFIG_QE;
+
+			stfsm_read_status(fsm, FLASH_CMD_RDSR, &sr1);
+			sta_wr = ((uint16_t)cr1  << 8) | sr1;
+
+			stfsm_write_status(fsm, sta_wr, 2);
+
+			stfsm_wait_busy(fsm);
+		}
+	} else {
+		if ((cr1 & STFSM_S25FL_CONFIG_QE)) {
+			/* Clear 'QE' */
+			cr1 &= ~STFSM_S25FL_CONFIG_QE;
+
+			stfsm_read_status(fsm, FLASH_CMD_RDSR, &sr1);
+			sta_wr = ((uint16_t)cr1  << 8) | sr1;
+
+			stfsm_write_status(fsm, sta_wr, 2);
+
+			stfsm_wait_busy(fsm);
+		}
+
+	}
+
+	/*
+	 * S25FLxxx devices support Program and Error error flags.
+	 * Configure driver to check flags and clear if necessary.
+	 */
+	fsm->configuration |= CFG_S25FL_CHECK_ERROR_FLAGS;
+
+	return 0;
+}
+
 static int stfsm_read(struct stfsm *fsm, uint8_t *const buf,
 		      const uint32_t size, const uint32_t offset)
 {
@@ -911,6 +1152,8 @@ static int stfsm_write(struct stfsm *fsm, const uint8_t *const buf,
 
 	/* Wait for completion */
 	ret = stfsm_wait_busy(fsm);
+	if (ret && fsm->configuration & CFG_S25FL_CHECK_ERROR_FLAGS)
+		stfsm_s25fl_clear_status_reg(fsm);
 
 	/* Exit 32-bit address mode, if required */
 	if (fsm->configuration & CFG_WRITE_TOGGLE_32BIT_ADDR) {
@@ -989,6 +1232,8 @@ static int stfsm_erase_sector(struct stfsm *fsm, const uint32_t offset)
 
 	/* Wait for completion */
 	ret = stfsm_wait_busy(fsm);
+	if (ret && fsm->configuration & CFG_S25FL_CHECK_ERROR_FLAGS)
+		stfsm_s25fl_clear_status_reg(fsm);
 
 	/* Exit 32-bit address mode, if required */
 	if (fsm->configuration & CFG_ERASESEC_TOGGLE_32BIT_ADDR)
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 10c12ba..83b30f9 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -229,6 +229,16 @@
 #define FLASH_CMD_READ4_1_1_4	0x6c
 #define FLASH_CMD_READ4_1_4_4	0xec
 
+/* S25FLxxxS commands */
+#define S25FL_CMD_WRITE4_1_1_4	0x34
+#define S25FL_CMD_SE4		0xdc
+#define S25FL_CMD_CLSR		0x30
+#define S25FL_CMD_DYBWR		0xe1
+#define S25FL_CMD_DYBRD		0xe0
+#define S25FL_CMD_WRITE4	0x12	/* Note, opcode clashes with
+					 * 'FLASH_CMD_WRITE_1_4_4'
+					 * as found on N25Qxxx devices! */
+
 /* Status register */
 #define FLASH_STATUS_BUSY	0x01
 #define FLASH_STATUS_WEL	0x02
@@ -237,6 +247,9 @@
 #define FLASH_STATUS_BP2	0x10
 #define FLASH_STATUS_SRWP0	0x80
 #define FLASH_STATUS_TIMEOUT	0xff
+/* S25FL Error Flags */
+#define S25FL_STATUS_E_ERR	0x20
+#define S25FL_STATUS_P_ERR	0x40
 
 #define FLASH_PAGESIZE		256			/* In Bytes    */
 #define FLASH_PAGESIZE_32	FLASH_PAGESIZE / 4	/* In uint32_t */
@@ -319,6 +332,7 @@ struct flash_info {
 
 static int stfsm_n25q_config(struct stfsm *fsm);
 static int stfsm_mx25_config(struct stfsm *fsm);
+static int stfsm_s25fl_config(struct stfsm *fsm);
 
 static struct flash_info flash_types[] = {
 	/*
@@ -381,9 +395,9 @@ static struct flash_info flash_types[] = {
 			FLASH_FLAG_WRITE_1_1_4	| \
 			FLASH_FLAG_READ_FAST)
 	{ "s25fl129p0", 0x012018, 0x4d00, 256 * 1024,  64, S25FLXXXP_FLAG, 80,
-	  NULL },
+	  stfsm_s25fl_config },
 	{ "s25fl129p1", 0x012018, 0x4d01,  64 * 1024, 256, S25FLXXXP_FLAG, 80,
-	  NULL },
+	  stfsm_s25fl_config },
 
 	/*
 	 * Spansion S25FLxxxS
@@ -398,13 +412,13 @@ static struct flash_info flash_types[] = {
 			FLASH_FLAG_RESET	| \
 			FLASH_FLAG_DYB_LOCKING)
 	{ "s25fl128s0", 0x012018, 0x0300,  256 * 1024, 64, S25FLXXXS_FLAG, 80,
-	  NULL },
+	  stfsm_s25fl_config },
 	{ "s25fl128s1", 0x012018, 0x0301,  64 * 1024, 256, S25FLXXXS_FLAG, 80,
-	  NULL },
+	  stfsm_s25fl_config },
 	{ "s25fl256s0", 0x010219, 0x4d00, 256 * 1024, 128,
-	  S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, NULL },
+	  S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, stfsm_s25fl_config },
 	{ "s25fl256s1", 0x010219, 0x4d01,  64 * 1024, 512,
-	  S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, NULL },
+	  S25FLXXXS_FLAG | FLASH_FLAG_32BIT_ADDR, 80, stfsm_s25fl_config },
 
 	/* Winbond -- w25x "blocks" are 64K, "sectors" are 4KiB */
 #define W25X_FLAG (FLASH_FLAG_READ_WRITE	| \
-- 
1.8.3.2

[PATCH v3 35/36] mtd: st_spi_fsm: Supply the W25Qxxx chip specific configuration call-back

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mtd/devices/st_spi_fsm.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/mtd/devices/st_spi_fsm.h | 13 +++++++++----
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index d31348a..72474f1 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -150,6 +150,11 @@ static struct seq_rw_config stfsm_s25fl_write4_configs[] = {
 	{0x00,                   0,                      0, 0, 0, 0x00, 0, 0},
 };
 
+/*
+ * [W25Qxxx] Configuration
+ */
+#define W25Q_STATUS_QE			(0x1 << 9)
+
 static struct stfsm_seq stfsm_seq_read; 	/* Dynamically populated */
 static struct stfsm_seq stfsm_seq_write;	/* Dynamically populated */
 static struct stfsm_seq stfsm_seq_en_32bit_addr;/* Dynamically populated */
@@ -1010,6 +1015,35 @@ static int stfsm_s25fl_config(struct stfsm *fsm)
 	return 0;
 }
 
+static int stfsm_w25q_config(struct stfsm *fsm)
+{
+	uint32_t data_pads;
+	uint16_t sta_wr;
+	uint8_t sta1, sta2;
+	int ret;
+
+	ret = stfsm_prepare_rwe_seqs_default(fsm);
+	if (ret)
+		return ret;
+
+	/* If using QUAD mode, set QE STATUS bit */
+	data_pads = ((stfsm_seq_read.seq_cfg >> 16) & 0x3) + 1;
+	if (data_pads == 4) {
+		stfsm_read_status(fsm, FLASH_CMD_RDSR, &sta1);
+		stfsm_read_status(fsm, FLASH_CMD_RDSR2, &sta2);
+
+		sta_wr = ((uint16_t)sta2 << 8) | sta1;
+
+		sta_wr |= W25Q_STATUS_QE;
+
+		stfsm_write_status(fsm, sta_wr, 2);
+
+		stfsm_wait_busy(fsm);
+	}
+
+	return 0;
+}
+
 static int stfsm_read(struct stfsm *fsm, uint8_t *const buf,
 		      const uint32_t size, const uint32_t offset)
 {
diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h
index 83b30f9..4c8fecd 100644
--- a/drivers/mtd/devices/st_spi_fsm.h
+++ b/drivers/mtd/devices/st_spi_fsm.h
@@ -333,6 +333,7 @@ struct flash_info {
 static int stfsm_n25q_config(struct stfsm *fsm);
 static int stfsm_mx25_config(struct stfsm *fsm);
 static int stfsm_s25fl_config(struct stfsm *fsm);
+static int stfsm_w25q_config(struct stfsm *fsm);
 
 static struct flash_info flash_types[] = {
 	/*
@@ -439,10 +440,14 @@ static struct flash_info flash_types[] = {
 		   FLASH_FLAG_READ_1_1_4	| \
 		   FLASH_FLAG_READ_1_4_4	| \
 		   FLASH_FLAG_WRITE_1_1_4)
-	{ "w25q80",  0xef4014, 0,  64 * 1024,  16, W25Q_FLAG, 80, NULL },
-	{ "w25q16",  0xef4015, 0,  64 * 1024,  32, W25Q_FLAG, 80, NULL },
-	{ "w25q32",  0xef4016, 0,  64 * 1024,  64, W25Q_FLAG, 80, NULL },
-	{ "w25q64",  0xef4017, 0,  64 * 1024, 128, W25Q_FLAG, 80, NULL },
+	{ "w25q80",  0xef4014, 0,  64 * 1024,  16, W25Q_FLAG, 80,
+	  stfsm_w25q_config },
+	{ "w25q16",  0xef4015, 0,  64 * 1024,  32, W25Q_FLAG, 80,
+	  stfsm_w25q_config },
+	{ "w25q32",  0xef4016, 0,  64 * 1024,  64, W25Q_FLAG, 80,
+	  stfsm_w25q_config },
+	{ "w25q64",  0xef4017, 0,  64 * 1024, 128, W25Q_FLAG, 80,
+	  stfsm_w25q_config },
 
 	/* Sentinel */
 	{ NULL, 0x000000, 0, 0, 0, 0, 0, NULL },
-- 
1.8.3.2

[PATCH v3 36/36] ARM: STi: Add support for the FSM Serial Flash Controller

[Lee Jones]

Here we add the necessary device nodes required for successful device
probing and Pinctrl setup for the FSM.

Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/stih416-b2105.dts    | 14 ++++++++++++++
 arch/arm/boot/dts/stih416-pinctrl.dtsi | 12 ++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/stih416-b2105.dts b/arch/arm/boot/dts/stih416-b2105.dts
index e518eb0..7acdfdf 100644
--- a/arch/arm/boot/dts/stih416-b2105.dts
+++ b/arch/arm/boot/dts/stih416-b2105.dts
@@ -33,6 +33,20 @@
 			status = "okay";
 		};
 
+		/* FSM */
+		spifsm: spifsm@fe902000{
+		        compatible         = "st,spi-fsm", "simple-bus";
+		        reg                =  <0xfe902000 0x1000>;
+		        reg-names          = "spi-fsm";
+		        pinctrl-0          = <&pinctrl_fsm>;
+
+			st,syscfg	   = <&syscfg_rear>;
+		        st,boot-device-reg = <0x958>;
+		        st,boot-device-spi = <0x1a>;
+
+		        status = "okay";
+		};
+
 		leds {
 			compatible	= "gpio-leds";
 			fp_led {
diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
index 10bb4df..9a48710 100644
--- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
@@ -235,6 +235,18 @@
 				};
 			};
 
+			fsm {
+				pinctrl_fsm: fsm {
+					st,pins {
+						spi-fsm-clk     = <&PIO12 2     OUT     ALT1>;
+						spi-fsm-cs      = <&PIO12 3     OUT     ALT1>;
+						spi-fsm-mosi    = <&PIO12 4     OUT     ALT1>;
+						spi-fsm-miso    = <&PIO12 5     IN      ALT1>;
+						spi-fsm-hol     = <&PIO12 6     OUT     ALT1>;
+						spi-fsm-wp      = <&PIO12 7     OUT     ALT1>;
+					};
+				};
+			};
 		};
 
 		pin-controller-rear {
-- 
1.8.3.2

Re: [PATCH v3 36/36] ARM: STi: Add support for the FSM Serial Flash Controller

[srinivas kandagatla]

Hi Lee,
On 29/11/13 12:19, Lee Jones wrote:
> +		/* FSM */
> +		spifsm: spifsm@fe902000{
> +		        compatible         = "st,spi-fsm", "simple-bus";
"simple-bus" is redundant here.

> +		        reg                =  <0xfe902000 0x1000>;
> +		        reg-names          = "spi-fsm";
> +		        pinctrl-0          = <&pinctrl_fsm>;
> +
> +			st,syscfg	   = <&syscfg_rear>;
> +		        st,boot-device-reg = <0x958>;
> +		        st,boot-device-spi = <0x1a>;
> +
> +		        status = "okay";
> +		};


thanks,
srini

Re: [PATCH v3 36/36] ARM: STi: Add support for the FSM Serial Flash Controller

[Lee Jones]

> > +		/* FSM */
> > +		spifsm: spifsm@fe902000{
> > +		        compatible         = "st,spi-fsm", "simple-bus";
> "simple-bus" is redundant here.

Yes I agree, good spot.

I removed it from the Documentation, but missed this one.

If I remove it, can I add your Ack for the remainder of the patch?

> > +		        reg                =  <0xfe902000 0x1000>;
> > +		        reg-names          = "spi-fsm";
> > +		        pinctrl-0          = <&pinctrl_fsm>;
> > +
> > +			st,syscfg	   = <&syscfg_rear>;
> > +		        st,boot-device-reg = <0x958>;
> > +		        st,boot-device-spi = <0x1a>;
> > +
> > +		        status = "okay";
> > +		};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 36/36] ARM: STi: Add support for the FSM Serial Flash Controller

[srinivas kandagatla]

On 02/12/13 13:20, Lee Jones wrote:
>>> +		/* FSM */
>>> +		spifsm: spifsm@fe902000{
>>> +		        compatible         = "st,spi-fsm", "simple-bus";
>> "simple-bus" is redundant here.
> 
> Yes I agree, good spot.
> 
> I removed it from the Documentation, but missed this one.
> 
> If I remove it, can I add your Ack for the remainder of the patch?
Yes,

Acked-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>

You should be aware that arch/arm/boot/dts/stih416-b2105.dts is not in
mainline yet.

Thanks,
srini


> 
>>> +		        reg                =  <0xfe902000 0x1000>;
>>> +		        reg-names          = "spi-fsm";
>>> +		        pinctrl-0          = <&pinctrl_fsm>;
>>> +
>>> +			st,syscfg	   = <&syscfg_rear>;
>>> +		        st,boot-device-reg = <0x958>;
>>> +		        st,boot-device-spi = <0x1a>;
>>> +
>>> +		        status = "okay";
>>> +		};
> 

Re: [PATCH v3 36/36] ARM: STi: Add support for the FSM Serial Flash Controller

[Lee Jones]

On Mon, 02 Dec 2013, srinivas kandagatla wrote:

> On 02/12/13 13:20, Lee Jones wrote:
> >>> +		/* FSM */
> >>> +		spifsm: spifsm@fe902000{
> >>> +		        compatible         = "st,spi-fsm", "simple-bus";
> >> "simple-bus" is redundant here.
> > 
> > Yes I agree, good spot.
> > 
> > I removed it from the Documentation, but missed this one.
> > 
> > If I remove it, can I add your Ack for the remainder of the patch?
> Yes,
> 
> Acked-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> 
> You should be aware that arch/arm/boot/dts/stih416-b2105.dts is not in
> mainline yet.

Yes, I am aware of this. Are you queuing patches for it?

> >>> +		        reg                =  <0xfe902000 0x1000>;
> >>> +		        reg-names          = "spi-fsm";
> >>> +		        pinctrl-0          = <&pinctrl_fsm>;
> >>> +
> >>> +			st,syscfg	   = <&syscfg_rear>;
> >>> +		        st,boot-device-reg = <0x958>;
> >>> +		        st,boot-device-spi = <0x1a>;
> >>> +
> >>> +		        status = "okay";
> >>> +		};
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 36/36] ARM: STi: Add support for the FSM Serial Flash Controller

[srinivas kandagatla]

On 02/12/13 13:43, Lee Jones wrote:
> On Mon, 02 Dec 2013, srinivas kandagatla wrote:
> 
>> On 02/12/13 13:20, Lee Jones wrote:
>>>>> +		/* FSM */
>>>>> +		spifsm: spifsm@fe902000{
>>>>> +		        compatible         = "st,spi-fsm", "simple-bus";
>>>> "simple-bus" is redundant here.
>>>
>>> Yes I agree, good spot.
>>>
>>> I removed it from the Documentation, but missed this one.
>>>
>>> If I remove it, can I add your Ack for the remainder of the patch?
>> Yes,
>>
>> Acked-by: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>
>> You should be aware that arch/arm/boot/dts/stih416-b2105.dts is not in
>> mainline yet.
> 
> Yes, I am aware of this. Are you queuing patches for it?
Yes, I will post this soon.

thanks,
srini
> 
>>>>> +		        reg                =  <0xfe902000 0x1000>;
>>>>> +		        reg-names          = "spi-fsm";
>>>>> +		        pinctrl-0          = <&pinctrl_fsm>;
>>>>> +
>>>>> +			st,syscfg	   = <&syscfg_rear>;
>>>>> +		        st,boot-device-reg = <0x958>;
>>>>> +		        st,boot-device-spi = <0x1a>;
>>>>> +
>>>>> +		        status = "okay";
>>>>> +		};
>>>
>>
>