| Message ID | 1385727565-25794-2-git-send-email-lee.jones@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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 >
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
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 */
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