| Message ID | 1478688149-4554-1-git-send-email-clg@kaod.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Nov 9, 2016 at 9:12 PM, Cédric Le Goater <clg@kaod.org> wrote: > This driver adds mtd support for spi-nor attached to either or both of > the Firmware Memory Controller or the SPI Flash Controller (AST2400 > only). > > The SMC controllers on the Aspeed AST2500 SoC are very similar to the > ones found on the AST2400. The differences are on the number of > supported flash modules and their default mappings in the SoC address > space. > > The Aspeed AST2500 has one SPI controller for the BMC firmware and two > for the host firmware. All controllers have now the same set of > registers compatible with the AST2400 FMC controller and the legacy > 'SMC' controller is fully gone. > > Each controller has a memory range on which it maps its flash module > slaves. Each slave is assigned a memory window for its mapping that > can be changed at bootime with the Segment Address Register. > > Each SPI flash slave can then be accessed in two modes: Command and > User. When in User mode, accesses to the memory segment of the slaves > are translated in SPI transfers. When in Command mode, the HW > generates the SPI commands automatically and the memory segment is > accessed as if doing a MMIO. > > Currently, only the User mode is supported. Command mode needs a > little more work to check that the memory window on the AHB bus fits > the module size. > > Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > Tested on: > > * OpenPOWER Palmetto (AST2400) with > FMC controller : n25q256a > SPI controller : mx25l25635e and n25q512ax3 > > * Evaluation board (AST2500) with > FMC controller : w25q256 > SPI controller : w25q256 > > * OpenPOWER Witherspoon (AST2500) with > FMC controller : mx25l25635e * 2 > SPI controller : mx66l1g45g It's also in use on the Zaius and Barreleye OpenBMC systems. In that respect, I would like to see this merged once it's deemed ready by the mtd maintainers. We can follow up with the extra features in future patches. I've reviewed and tested this as Cedric and Milton have developed it, so please add: Reviewed-by: Joel Stanley <joel@jms.id.au> Cheers, Joel > > Changes since v2: > > - added a set4b ops to handle difference in the controllers > - simplified the IO routines > - prepared for fast read using dummy cycles > > Work in progress: > > - read optimization using higher SPI clock frequencies > - command mode to direct reads from AHB > - DMA support > > .../devicetree/bindings/mtd/aspeed-smc.txt | 72 ++ > drivers/mtd/spi-nor/Kconfig | 12 + > drivers/mtd/spi-nor/Makefile | 1 + > drivers/mtd/spi-nor/aspeed-smc.c | 783 +++++++++++++++++++++ > 4 files changed, 868 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt > create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c
On Wed, Nov 09, 2016 at 11:42:29AM +0100, Cédric Le Goater wrote: > This driver adds mtd support for spi-nor attached to either or both of > the Firmware Memory Controller or the SPI Flash Controller (AST2400 > only). > > The SMC controllers on the Aspeed AST2500 SoC are very similar to the > ones found on the AST2400. The differences are on the number of > supported flash modules and their default mappings in the SoC address > space. > > The Aspeed AST2500 has one SPI controller for the BMC firmware and two > for the host firmware. All controllers have now the same set of > registers compatible with the AST2400 FMC controller and the legacy > 'SMC' controller is fully gone. > > Each controller has a memory range on which it maps its flash module > slaves. Each slave is assigned a memory window for its mapping that > can be changed at bootime with the Segment Address Register. > > Each SPI flash slave can then be accessed in two modes: Command and > User. When in User mode, accesses to the memory segment of the slaves > are translated in SPI transfers. When in Command mode, the HW > generates the SPI commands automatically and the memory segment is > accessed as if doing a MMIO. > > Currently, only the User mode is supported. Command mode needs a > little more work to check that the memory window on the AHB bus fits > the module size. > > Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > Tested on: > > * OpenPOWER Palmetto (AST2400) with > FMC controller : n25q256a > SPI controller : mx25l25635e and n25q512ax3 > > * Evaluation board (AST2500) with > FMC controller : w25q256 > SPI controller : w25q256 > > * OpenPOWER Witherspoon (AST2500) with > FMC controller : mx25l25635e * 2 > SPI controller : mx66l1g45g > > Changes since v2: > > - added a set4b ops to handle difference in the controllers > - simplified the IO routines > - prepared for fast read using dummy cycles > > Work in progress: > > - read optimization using higher SPI clock frequencies > - command mode to direct reads from AHB > - DMA support > > .../devicetree/bindings/mtd/aspeed-smc.txt | 72 ++ Acked-by: Rob Herring <robh@kernel.org> > drivers/mtd/spi-nor/Kconfig | 12 + > drivers/mtd/spi-nor/Makefile | 1 + > drivers/mtd/spi-nor/aspeed-smc.c | 783 +++++++++++++++++++++ > 4 files changed, 868 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt > create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c
On 11/11/2016 05:42 AM, Joel Stanley wrote: > On Wed, Nov 9, 2016 at 9:12 PM, Cédric Le Goater <clg@kaod.org> wrote: >> This driver adds mtd support for spi-nor attached to either or both of >> the Firmware Memory Controller or the SPI Flash Controller (AST2400 >> only). >> >> The SMC controllers on the Aspeed AST2500 SoC are very similar to the >> ones found on the AST2400. The differences are on the number of >> supported flash modules and their default mappings in the SoC address >> space. >> >> The Aspeed AST2500 has one SPI controller for the BMC firmware and two >> for the host firmware. All controllers have now the same set of >> registers compatible with the AST2400 FMC controller and the legacy >> 'SMC' controller is fully gone. >> >> Each controller has a memory range on which it maps its flash module >> slaves. Each slave is assigned a memory window for its mapping that >> can be changed at bootime with the Segment Address Register. >> >> Each SPI flash slave can then be accessed in two modes: Command and >> User. When in User mode, accesses to the memory segment of the slaves >> are translated in SPI transfers. When in Command mode, the HW >> generates the SPI commands automatically and the memory segment is >> accessed as if doing a MMIO. >> >> Currently, only the User mode is supported. Command mode needs a >> little more work to check that the memory window on the AHB bus fits >> the module size. >> >> Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> Tested on: >> >> * OpenPOWER Palmetto (AST2400) with >> FMC controller : n25q256a >> SPI controller : mx25l25635e and n25q512ax3 >> >> * Evaluation board (AST2500) with >> FMC controller : w25q256 >> SPI controller : w25q256 >> >> * OpenPOWER Witherspoon (AST2500) with >> FMC controller : mx25l25635e * 2 >> SPI controller : mx66l1g45g > > It's also in use on the Zaius and Barreleye OpenBMC systems. > > In that respect, I would like to see this merged once it's deemed > ready by the mtd maintainers. We can follow up with the extra features > in future patches. > > I've reviewed and tested this as Cedric and Milton have developed it, > so please add: > > Reviewed-by: Joel Stanley <joel@jms.id.au> > Thanks! I'll review the patch ASAP (this week).
On 11/09/2016 11:42 AM, Cédric Le Goater wrote: > This driver adds mtd support for spi-nor attached to either or both of > the Firmware Memory Controller or the SPI Flash Controller (AST2400 > only). > > The SMC controllers on the Aspeed AST2500 SoC are very similar to the > ones found on the AST2400. The differences are on the number of > supported flash modules and their default mappings in the SoC address > space. > > The Aspeed AST2500 has one SPI controller for the BMC firmware and two > for the host firmware. All controllers have now the same set of > registers compatible with the AST2400 FMC controller and the legacy > 'SMC' controller is fully gone. > > Each controller has a memory range on which it maps its flash module > slaves. Each slave is assigned a memory window for its mapping that > can be changed at bootime with the Segment Address Register. > > Each SPI flash slave can then be accessed in two modes: Command and > User. When in User mode, accesses to the memory segment of the slaves > are translated in SPI transfers. When in Command mode, the HW > generates the SPI commands automatically and the memory segment is > accessed as if doing a MMIO. > > Currently, only the User mode is supported. Command mode needs a > little more work to check that the memory window on the AHB bus fits > the module size. > > Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > Tested on: > > * OpenPOWER Palmetto (AST2400) with > FMC controller : n25q256a > SPI controller : mx25l25635e and n25q512ax3 > > * Evaluation board (AST2500) with > FMC controller : w25q256 > SPI controller : w25q256 > > * OpenPOWER Witherspoon (AST2500) with > FMC controller : mx25l25635e * 2 > SPI controller : mx66l1g45g > > Changes since v2: > > - added a set4b ops to handle difference in the controllers > - simplified the IO routines > - prepared for fast read using dummy cycles > > Work in progress: > > - read optimization using higher SPI clock frequencies > - command mode to direct reads from AHB > - DMA support > > .../devicetree/bindings/mtd/aspeed-smc.txt | 72 ++ > drivers/mtd/spi-nor/Kconfig | 12 + > drivers/mtd/spi-nor/Makefile | 1 + > drivers/mtd/spi-nor/aspeed-smc.c | 783 +++++++++++++++++++++ > 4 files changed, 868 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt > create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c > > diff --git a/Documentation/devicetree/bindings/mtd/aspeed-smc.txt b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt > new file mode 100644 > index 000000000000..7516b0c01fcf > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt > @@ -0,0 +1,72 @@ > +* Aspeed Static Memory controller > +* Aspeed SPI Flash Controller > + > +The Static memory controller in the ast2400 supports 5 chip selects > +each can be attached to NAND, parallel NOR, or SPI NOR attached flash. So this controller is supported by this driver, which behaves like a SPI controller driver, yet the block can also do NAND and parallel NOR ? > +The Firmware Memory Controller in the ast2500 supports 3 chip selects, > +two of which are always in SPI-NOR mode and the third can be SPI-NOR > +or parallel flash. The SPI flash controller in the ast2400 supports > +one of 2 chip selects selected by pinmux. The two SPI flash > +controllers in the ast2500 each support two chip selects. This paragraph is confusing, it's hard to grok down how many different controllers does this driver support and what are their properties from it. It is all there, it's just hard to read. Also, please split the DT bindings into separate patch and send them to DT list for review. > +Required properties: > + - compatible : Should be one of > + "aspeed,ast2400-fmc" for the AST2400 Static Memory Controller > + "aspeed,ast2400-smc" for the AST2400 SPI Flash Controller > + "aspeed,ast2500-fmc" for the AST2500 Firmware SPI Memory Controller > + "aspeed,ast2500-smc" for the AST2500 SPI Flash Controllers > + - reg : the first contains the control register location and length, > + the second contains the memory window mapping address and length > + - #address-cells : must be 1 corresponding to chip select child binding > + - #size-cells : must be 0 corresponding to chip select child binding > + > +Optional properties: > + - interrupts : Should contain the interrupt for the dma device if an fmc > + > +The child nodes are the SPI Flash modules which must have a compatible > +property as specified in bindings/mtd/jedec,spi-nor.txt > + > +Optionally, the child node can contain properties for SPI mode (may be > +ignored): > + - spi-max-frequency - (optional) max frequency of spi bus You don't need to add the (optional) here again. > +Example: > +fmc: fmc@1e620000 { I'd suggest to keep the example minimal -- drop the partitions etc. > + compatible = "aspeed,ast2400-fmc"; > + reg = < 0x1e620000 0x94 > + 0x20000000 0x02000000 > + 0x22000000 0x02000000 >; > + #address-cells = <1>; > + #size-cells = <0>; > + interrupts = <19>; > + flash@0 { > + reg = < 0 >; > + compatible = "jedec,spi-nor" ; > + /* spi-max-frequency = <>; */ > + /* m25p,fast-read; */ > + #address-cells = <1>; > + #size-cells = <1>; > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + boot@0 { > + label = "boot-loader"; > + reg = < 0 0x8000 >; > + }; > + image@8000 { > + label = "kernel-image"; > + reg = < 0x8000 0x1f8000 >; > + }; > + }; > + }; > + flash@1 { > + reg = < 1 >; > + compatible = "jedec,spi-nor" ; > + label = "alt"; > + /* spi-max-frequency = <>; */ > + status = "fail"; > + /* m25p,fast-read; */ > + }; > +}; > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig > index 4a682ee0f632..96148600fdab 100644 > --- a/drivers/mtd/spi-nor/Kconfig > +++ b/drivers/mtd/spi-nor/Kconfig > @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI > Flash. Enable this option if you have a device with a SPIFI > controller and want to access the Flash as a mtd device. > > +config ASPEED_FLASH_SPI Should be SPI_ASPEED , see the other controllers and keep the list sorted. > + tristate "Aspeed flash controllers in SPI mode" > + depends on HAS_IOMEM && OF > + depends on ARCH_ASPEED || COMPILE_TEST > + # IO_SPACE_LIMIT must be equivalent to (~0UL) > + depends on !NEED_MACH_IO_H Why? > + help > + This enables support for the New Static Memory Controller > + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached > + to SPI nor chips, and support for the SPI Memory controller > + (SPI) for the BIOS. I think there is a naming chaos between FMC, SMC (as in Static MC) and SMC (as in SPI MC). > endif # MTD_SPI_NOR > diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile > index 121695e83542..c3174ebc45c2 100644 > --- a/drivers/mtd/spi-nor/Makefile > +++ b/drivers/mtd/spi-nor/Makefile > @@ -4,4 +4,5 @@ obj-$(CONFIG_SPI_CADENCE_QUADSPI) += cadence-quadspi.o > obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o > obj-$(CONFIG_SPI_HISI_SFC) += hisi-sfc.o > obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o > +obj-$(CONFIG_ASPEED_FLASH_SPI) += aspeed-smc.o > obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o > diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c > new file mode 100644 > index 000000000000..30662daf89ca > --- /dev/null > +++ b/drivers/mtd/spi-nor/aspeed-smc.c > @@ -0,0 +1,783 @@ > +/* > + * ASPEED Static Memory Controller driver > + * > + * Copyright (c) 2015-2016, IBM Corporation. > + * > + * 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. > + */ > + > +#include <linux/bug.h> > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/mtd/mtd.h> > +#include <linux/mtd/partitions.h> > +#include <linux/mtd/spi-nor.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/sysfs.h> > + > +#define DEVICE_NAME "aspeed-smc" > + > +/* > + * In user mode all data bytes read or written to the chip decode address > + * range are transferred to or from the SPI bus. The range is treated as a > + * fifo of arbitratry 1, 2, or 4 byte width but each write has to be aligned > + * to its size. The address within the multiple 8kB range is ignored when > + * sending bytes to the SPI bus. > + * > + * On the arm architecture, as of Linux version 4.3, memcpy_fromio and > + * memcpy_toio on little endian targets use the optimized memcpy routines > + * that were designed for well behavied memory storage. These routines > + * have a stutter if the source and destination are not both word aligned, > + * once with a duplicate access to the source after aligning to the > + * destination to a word boundary, and again with a duplicate access to > + * the source when the final byte count is not word aligned. > + * > + * When writing or reading the fifo this stutter discards data or sends > + * too much data to the fifo and can not be used by this driver. > + * > + * While the low level io string routines that implement the insl family do > + * the desired accesses and memory increments, the cross architecture io > + * macros make them essentially impossible to use on a memory mapped address > + * instead of a a token from the call to iomap of an io port. > + * > + * These fifo routines use readl and friends to a constant io port and update > + * the memory buffer pointer and count via explicit code. The final updates > + * to len are optimistically suppressed. > + */ > +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, > + size_t len) > +{ What if start of buf is unaligned ? > + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { Uh, should use boolean OR, not bitwise or. Also, if you're testing pointer for NULL, do if (!ptr) . if (!src || !buf || !len) return; while (...) > + while (len > 3) { > + *(u32 *)buf = readl(src); > + buf += 4; > + src += 4; > + len -= 4; > + } > + } > + > + while (len--) { > + *(u8 *)buf = readb(src); > + buf += 1; > + src += 1; > + } > + return 0; > +} > + > +static int aspeed_smc_write_to_ahb(void __iomem *dst, const void *buf, > + size_t len) > +{ > + if ((((unsigned long)dst | (unsigned long)buf | len) & 3) == 0) { DTTO > + while (len > 3) { > + u32 val = *(u32 *)buf; > + > + writel(val, dst); > + buf += 4; > + dst += 4; > + len -= 4; > + } > + } > + > + while (len--) { > + u8 val = *(u8 *)buf; > + > + writeb(val, dst); > + buf += 1; > + dst += 1; > + } > + return 0; > +} > + > +enum smc_flash_type { > + smc_type_nor = 0, /* controller connected to nor flash */ > + smc_type_nand = 1, /* controller connected to nand flash */ > + smc_type_spi = 2, /* controller connected to spi flash */ > +}; > + > +struct aspeed_smc_chip; > + > +struct aspeed_smc_info { > + u32 maxsize; /* maximum size of 1 chip window */ > + u8 nce; /* number of chip enables */ > + u8 maxwidth; /* max width of spi bus */ > + bool hastype; /* flash type field exists in cfg reg */ > + u8 we0; /* shift for write enable bit for ce 0 */ > + u8 ctl0; /* offset in regs of ctl for ce 0 */ > + u8 time; /* offset in regs of timing */ > + u8 misc; /* offset in regs of misc settings */ > + > + void (*set_4b)(struct aspeed_smc_chip *chip); > +}; > + > +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip); > +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip); > + > +static const struct aspeed_smc_info fmc_2400_info = { > + .maxsize = 64 * 1024 * 1024, > + .nce = 5, > + .maxwidth = 4, > + .hastype = true, Shouldn't all this be specified in DT ? > + .we0 = 16, > + .ctl0 = 0x10, > + .time = 0x94, > + .misc = 0x54, > + .set_4b = aspeed_smc_chip_set_4b, > +}; > + > +static const struct aspeed_smc_info smc_2400_info = { > + .maxsize = 64 * 1024 * 1024, > + .nce = 1, > + .maxwidth = 2, > + .hastype = false, > + .we0 = 0, > + .ctl0 = 0x04, > + .time = 0x14, > + .misc = 0x10, > + .set_4b = aspeed_smc_chip_set_4b_smc_2400, > +}; > + > +static const struct aspeed_smc_info fmc_2500_info = { > + .maxsize = 256 * 1024 * 1024, > + .nce = 3, > + .maxwidth = 2, > + .hastype = true, > + .we0 = 16, > + .ctl0 = 0x10, > + .time = 0x94, > + .misc = 0x54, > + .set_4b = aspeed_smc_chip_set_4b, > +}; > + > +static const struct aspeed_smc_info smc_2500_info = { > + .maxsize = 128 * 1024 * 1024, > + .nce = 2, > + .maxwidth = 2, > + .hastype = false, > + .we0 = 16, > + .ctl0 = 0x10, > + .time = 0x94, > + .misc = 0x54, > + .set_4b = aspeed_smc_chip_set_4b, > +}; > + > +enum smc_ctl_reg_value { > + smc_base, /* base value without mode for other commands */ > + smc_read, /* command reg for (maybe fast) reads */ > + smc_write, /* command reg for writes with timings */ > + smc_num_ctl_reg_values /* last value to get count of commands */ > +}; > + > +struct aspeed_smc_controller; > + > +struct aspeed_smc_chip { > + int cs; > + struct aspeed_smc_controller *controller; > + __le32 __iomem *ctl; /* control register */ Why do you use __le32* here and void* below ? > + void __iomem *base; /* base of chip window */ > + __le32 ctl_val[smc_num_ctl_reg_values]; /* controls with timing */ > + enum smc_flash_type type; /* what type of flash */ > + struct spi_nor nor; > +}; > + > +struct aspeed_smc_controller { > + struct device *dev; > + > + struct mutex mutex; /* controller access mutex */ > + const struct aspeed_smc_info *info; /* type info of controller */ > + void __iomem *regs; /* controller registers */ > + void __iomem *windows; /* per-chip windows resource */ > + > + struct aspeed_smc_chip *chips[0]; /* pointers to attached chips */ > +}; > + > +/* > + * SPI Flash Configuration Register (AST2400 SPI) > + */ > +#define CONFIG_REG 0x0 > +#define CONFIG_ENABLE_CE_INACTIVE BIT(1) > +#define CONFIG_WRITE BIT(0) #define[space]FOO[tab]BIT(bar) > +/* > + * SPI Flash Configuration Register (AST2500 SPI) > + * Type setting Register (AST2500 FMC and AST2400 FMC) > + */ > +#define TYPE_SETTING_REG 0x0 > +#define CONFIG_DISABLE_LEGACY BIT(31) /* 1 on AST2500 FMC */ > + > +#define CONFIG_CE2_WRITE BIT(18) > +#define CONFIG_CE1_WRITE BIT(17) > +#define CONFIG_CE0_WRITE BIT(16) > + > +#define CONFIG_CE2_TYPE BIT(4) /* FMC only */ > +#define CONFIG_CE1_TYPE BIT(2) /* FMC only */ > +#define CONFIG_CE0_TYPE BIT(0) /* FMC only */ > + > +/* > + * CE Control Register (AST2500 SPI,FMC and AST2400 FMC) > + */ > +#define CE_CONTROL_REG 0x4 > +#define CE2_ENABLE_CE_INACTIVE BIT(10) > +#define CE1_ENABLE_CE_INACTIVE BIT(9) > +#define CE0_ENABLE_CE_INACTIVE BIT(8) > +#define CE2_CONTROL_EXTENDED BIT(2) > +#define CE1_CONTROL_EXTENDED BIT(1) > +#define CE0_CONTROL_EXTENDED BIT(0) > + > +/* CE0 Control Register (depends on the controller type) */ > +#define CONTROL_SPI_AAF_MODE BIT(31) > +#define CONTROL_SPI_IO_MODE_MASK GENMASK(30, 28) > +#define CONTROL_SPI_IO_DUAL_DATA BIT(29) > +#define CONTROL_SPI_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28)) > +#define CONTROL_SPI_IO_QUAD_DATA BIT(30) > +#define CONTROL_SPI_IO_QUAD_ADDR_DATA (BIT(30) | BIT(28)) > +#define CONTROL_SPI_CE_INACTIVE_SHIFT 24 > +#define CONTROL_SPI_CE_INACTIVE_MASK GENMASK(27, CONTROL_SPI_CE_INACTIVE_SHIFT) > +/* 0 = 16T ... 15 = 1T T=HCLK */ > +#define CONTROL_SPI_COMMAND_SHIFT 16 > +#define CONTROL_SPI_DUMMY_CYCLE_COMMAND_OUTPUT BIT(15) > +#define CONTROL_SPI_IO_DUMMY_CYCLES_HI BIT(14) > +#define CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT 14 > +#define CONTROL_SPI_IO_ADDRESS_4B BIT(13) /* AST2400 SPI */ > +#define CONTROL_SPI_CLK_DIV4 BIT(13) /* others */ > +#define CONTROL_SPI_RW_MERGE BIT(12) > +#define CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT 6 > +#define CONTROL_SPI_IO_DUMMY_CYCLES_LO GENMASK(7, \ > + CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT) > +#define CONTROL_SPI_IO_DUMMY_CYCLES_MASK (CONTROL_SPI_IO_DUMMY_CYCLES_HI | \ > + CONTROL_SPI_IO_DUMMY_CYCLES_LO) > +#define CONTROL_SPI_IO_DUMMY_CYCLES_SET(dummy) \ > + (((((dummy) >> 2) & 0x1) << CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT) | \ > + (((dummy) & 0x3) << CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT)) > + > +#define CONTROL_SPI_CLOCK_FREQ_SEL_SHIFT 8 > +#define CONTROL_SPI_CLOCK_FREQ_SEL_MASK GENMASK(11, \ > + CONTROL_SPI_CLOCK_FREQ_SEL_SHIFT) > +#define CONTROL_SPI_LSB_FIRST BIT(5) > +#define CONTROL_SPI_CLOCK_MODE_3 BIT(4) > +#define CONTROL_SPI_IN_DUAL_DATA BIT(3) > +#define CONTROL_SPI_CE_STOP_ACTIVE_CONTROL BIT(2) > +#define CONTROL_SPI_COMMAND_MODE_MASK GENMASK(1, 0) > +#define CONTROL_SPI_COMMAND_MODE_NORMAL (0) > +#define CONTROL_SPI_COMMAND_MODE_FREAD (1) > +#define CONTROL_SPI_COMMAND_MODE_WRITE (2) > +#define CONTROL_SPI_COMMAND_MODE_USER (3) > + > +#define CONTROL_SPI_KEEP_MASK (CONTROL_SPI_AAF_MODE | \ > + CONTROL_SPI_CE_INACTIVE_MASK | CONTROL_SPI_CLK_DIV4 | \ > + CONTROL_SPI_IO_DUMMY_CYCLES_MASK | CONTROL_SPI_CLOCK_FREQ_SEL_MASK | \ > + CONTROL_SPI_LSB_FIRST | CONTROL_SPI_CLOCK_MODE_3) > + > +/* Segment Address Registers */ > +#define SEGMENT_ADDR_REG0 0x30 > +#define SEGMENT_ADDR_START(_r) ((((_r) >> 16) & 0xFF) << 23) > +#define SEGMENT_ADDR_END(_r) ((((_r) >> 24) & 0xFF) << 23) > + > +static u32 spi_control_fill_opcode(u8 opcode) > +{ > + return ((u32)(opcode)) << CONTROL_SPI_COMMAND_SHIFT; return opcode << CONTROL... , fix these horrible casts and parenthesis globally. > +} > + > +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip) > +{ > + return ((u32)1 << (chip->controller->info->we0 + chip->cs)); return BIT(...) I'm not sure these microfunctions are even needed. > +} > + > +static void aspeed_smc_chip_check_config(struct aspeed_smc_chip *chip) > +{ > + struct aspeed_smc_controller *controller = chip->controller; > + u32 reg; > + > + reg = readl(controller->regs + CONFIG_REG); > + > + if (!(reg & aspeed_smc_chip_write_bit(chip))) { Invert the logic and return here, ie if (reg & BIT()) return , to trim the indent. > + dev_dbg(controller->dev, > + "config write is not set ! @%p: 0x%08x\n", > + controller->regs + CONFIG_REG, reg); > + reg |= aspeed_smc_chip_write_bit(chip); > + writel(reg, controller->regs + CONFIG_REG); > + } > +} > + > +static void aspeed_smc_start_user(struct spi_nor *nor) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + u32 ctl = chip->ctl_val[smc_base]; > + > + /* > + * When the chip is controlled in user mode, we need write > + * access to send the opcodes to it. So check the config. > + */ > + aspeed_smc_chip_check_config(chip); > + > + ctl |= CONTROL_SPI_COMMAND_MODE_USER | > + CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; > + writel(ctl, chip->ctl); > + > + ctl &= ~CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; > + writel(ctl, chip->ctl); > +} > + > +static void aspeed_smc_stop_user(struct spi_nor *nor) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + u32 ctl = chip->ctl_val[smc_read]; > + u32 ctl2 = ctl | CONTROL_SPI_COMMAND_MODE_USER | > + CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; > + > + writel(ctl2, chip->ctl); /* stop user CE control */ > + writel(ctl, chip->ctl); /* default to fread or read */ > +} > + > +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); Won't this have a horrid overhead ? > + aspeed_smc_start_user(nor); > + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); > + aspeed_smc_read_from_ahb(buf, chip->base, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return 0; > +} > + > +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, > + int len) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); > + > + aspeed_smc_start_user(nor); > + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); > + aspeed_smc_write_to_ahb(chip->base, buf, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return 0; > +} > + > +static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + __be32 temp; > + u32 cmdaddr; > + > + switch (nor->addr_width) { > + default: > + WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n", > + nor->addr_width); > + /* FALLTHROUGH */ > + case 3: > + cmdaddr = addr & 0xFFFFFF; > + > + cmdaddr |= (u32)cmd << 24; Drop the cast. > + temp = cpu_to_be32(cmdaddr); > + aspeed_smc_write_to_ahb(chip->base, &temp, 4); > + break; > + case 4: > + temp = cpu_to_be32(addr); > + aspeed_smc_write_to_ahb(chip->base, &cmd, 1); > + aspeed_smc_write_to_ahb(chip->base, &temp, 4); > + break; > + } > +} > + > +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from, > + size_t len, u_char *read_buf) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); > + > + aspeed_smc_start_user(nor); > + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from); > + aspeed_smc_read_from_ahb(read_buf, chip->base, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return len; > +} > + > +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len, > + const u_char *write_buf) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); > + > + aspeed_smc_start_user(nor); > + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to); > + aspeed_smc_write_to_ahb(chip->base, write_buf, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return len; > +} > + > +static int aspeed_smc_remove(struct platform_device *dev) > +{ > + struct aspeed_smc_chip *chip; > + struct aspeed_smc_controller *controller = platform_get_drvdata(dev); > + int n; > + > + for (n = 0; n < controller->info->nce; n++) { > + chip = controller->chips[n]; > + if (chip) > + mtd_device_unregister(&chip->nor.mtd); > + } > + > + return 0; > +} > + > +static const struct of_device_id aspeed_smc_matches[] = { > + { .compatible = "aspeed,ast2400-fmc", .data = &fmc_2400_info }, > + { .compatible = "aspeed,ast2400-smc", .data = &smc_2400_info }, > + { .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info }, > + { .compatible = "aspeed,ast2500-smc", .data = &smc_2500_info }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, aspeed_smc_matches); > + > +static struct platform_device * > +of_platform_device_create_or_find(struct device_node *child, > + struct device *parent) > +{ > + struct platform_device *cdev; > + > + cdev = of_platform_device_create(child, NULL, parent); > + if (!cdev) > + cdev = of_find_device_by_node(child); > + return cdev; > +} > + > +static void __iomem *window_start(struct aspeed_smc_controller *controller, > + struct resource *r, unsigned int n) > +{ > + u32 offset = 0; > + u32 reg; > + > + if (controller->info->nce > 1) { > + reg = readl(controller->regs + SEGMENT_ADDR_REG0 + n * 4); > + > + if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg)) > + return NULL; > + > + offset = SEGMENT_ADDR_START(reg) - r->start; > + } > + > + return controller->windows + offset; > +} > + > +static void aspeed_smc_chip_enable_write(struct aspeed_smc_chip *chip) > +{ > + struct aspeed_smc_controller *controller = chip->controller; > + u32 reg; > + > + reg = readl(controller->regs + CONFIG_REG); > + > + reg |= aspeed_smc_chip_write_bit(chip); > + writel(reg, controller->regs + CONFIG_REG); > +} > + > +static void aspeed_smc_chip_set_type(struct aspeed_smc_chip *chip, int type) > +{ > + struct aspeed_smc_controller *controller = chip->controller; > + u32 reg; > + > + reg = readl(controller->regs + CONFIG_REG); > + > + chip->type = type; You can move this above the readl() to make the RMW block consistent. > + reg &= ~(3 << (chip->cs * 2)); > + reg |= chip->type << (chip->cs * 2); > + writel(reg, controller->regs + CONFIG_REG); > +} > + > +/* > + * The AST2500 FMC and AST2400 FMC flash controllers should be > + * strapped by hardware, or autodetected, but the AST2500 SPI flash > + * needs to be set. > + */ > +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip) > +{ > + struct aspeed_smc_controller *controller = chip->controller; > + u32 reg; > + > + if (chip->controller->info == &smc_2500_info) { > + reg = readl(controller->regs + CE_CONTROL_REG); > + reg |= 1 << chip->cs; > + writel(reg, controller->regs + CE_CONTROL_REG); > + } > +} > + > +/* > + * The AST2400 SPI flash controller does not have a CE Control > + * register. It uses the CE0 control register to set 4Byte mode at the > + * controller level. > + */ > +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip) > +{ > + chip->ctl_val[smc_base] |= CONTROL_SPI_IO_ADDRESS_4B; > + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_ADDRESS_4B; > +} > + > +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip, > + struct resource *r) > +{ > + struct aspeed_smc_controller *controller = chip->controller; > + const struct aspeed_smc_info *info = controller->info; > + u32 reg, base_reg; > + > + /* > + * Always turn on the write enable bit to allow opcodes to be > + * sent in user mode. > + */ > + aspeed_smc_chip_enable_write(chip); > + > + /* The driver only supports SPI type flash for the moment */ > + if (info->hastype) > + aspeed_smc_chip_set_type(chip, smc_type_spi); > + > + /* > + * Configure chip base address in memory > + */ > + chip->base = window_start(controller, r, chip->cs); > + if (!chip->base) { > + dev_warn(chip->nor.dev, "CE segment window closed.\n"); > + return -1; > + } > + > + /* > + * Read the existing control register to get basic values. > + * > + * XXX This register probably needs more sanitation. What's this comment about ? > + * Do we need support for mode 3 vs mode 0 clock phasing? > + */ > + reg = readl(chip->ctl); > + dev_dbg(controller->dev, "control register: %08x\n", reg); > + > + base_reg = reg & CONTROL_SPI_KEEP_MASK; > + if (base_reg != reg) { > + dev_info(controller->dev, > + "control register changed to: %08x\n", > + base_reg); > + } > + chip->ctl_val[smc_base] = base_reg; > + > + /* > + * Retain the prior value of the control register as the > + * default if it was normal access mode. Otherwise start with > + * the sanitized base value set to read mode. > + */ > + if ((reg & CONTROL_SPI_COMMAND_MODE_MASK) == > + CONTROL_SPI_COMMAND_MODE_NORMAL) > + chip->ctl_val[smc_read] = reg; > + else > + chip->ctl_val[smc_read] = chip->ctl_val[smc_base] | > + CONTROL_SPI_COMMAND_MODE_NORMAL; > + > + dev_dbg(controller->dev, "default control register: %08x\n", > + chip->ctl_val[smc_read]); > + return 0; > +} > + > +static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip) > +{ > + struct aspeed_smc_controller *controller = chip->controller; > + const struct aspeed_smc_info *info = controller->info; > + u32 cmd; > + > + if (chip->nor.addr_width == 4 && info->set_4b) > + info->set_4b(chip); > + > + /* > + * base mode has not been optimized yet. use it for writes. > + */ > + chip->ctl_val[smc_write] = chip->ctl_val[smc_base] | > + spi_control_fill_opcode(chip->nor.program_opcode) | > + CONTROL_SPI_COMMAND_MODE_WRITE; > + > + dev_dbg(controller->dev, "write control register: %08x\n", > + chip->ctl_val[smc_write]); > + > + /* > + * XXX TODO > + * Adjust clocks if fast read and write are supported. > + * Interpret spi-nor flags to adjust controller settings. > + * Check if resource size big enough for detected chip and > + * add support assisted (normal or fast-) read and dma. > + */ > + switch (chip->nor.flash_read) { > + case SPI_NOR_NORMAL: > + cmd = CONTROL_SPI_COMMAND_MODE_NORMAL; > + break; > + case SPI_NOR_FAST: > + cmd = CONTROL_SPI_COMMAND_MODE_FREAD; > + break; > + default: > + dev_err(chip->nor.dev, "unsupported SPI read mode\n"); > + return -EINVAL; > + } > + > + chip->ctl_val[smc_read] |= cmd | > + CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8); > + > + dev_dbg(controller->dev, "base control register: %08x\n", > + chip->ctl_val[smc_read]); > + return 0; > +} > + > +static int aspeed_smc_probe(struct platform_device *pdev) > +{ > + struct aspeed_smc_controller *controller; > + const struct of_device_id *match; > + const struct aspeed_smc_info *info; > + struct resource *r; > + struct device_node *child; > + int err = 0; > + unsigned int n; > + > + match = of_match_device(aspeed_smc_matches, &pdev->dev); > + if (!match || !match->data) > + return -ENODEV; > + info = match->data; > + > + controller = devm_kzalloc(&pdev->dev, sizeof(*controller) + > + info->nce * sizeof(controller->chips[0]), GFP_KERNEL); > + if (!controller) > + return -ENOMEM; > + controller->info = info; > + > + mutex_init(&controller->mutex); > + platform_set_drvdata(pdev, controller); > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + controller->regs = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(controller->regs)) > + return PTR_ERR(controller->regs); > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + controller->windows = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(controller->windows)) > + return PTR_ERR(controller->windows); > + > + controller->dev = &pdev->dev; > + > + /* The pinmux or bootloader will disable the legacy mode controller */ > + > + /* > + * XXX Need to add arbitration to the SMC (BIOS) controller if access > + * is shared by the host. > + */ > + for_each_available_child_of_node(controller->dev->of_node, child) { > + struct platform_device *cdev; > + struct aspeed_smc_chip *chip; Pull this into separate function, ie. like cadence_qspi.c , so we can identify the developing boilerplate easily. > + /* This version does not support nand or nor flash devices. */ > + if (!of_device_is_compatible(child, "jedec,spi-nor")) > + continue; > + > + /* > + * create a platform device from the of node. If the device > + * already was created (eg from a prior bind/unbind cycle) > + * reuse it. > + * > + * The creating the device node for the child here allows its > + * use for error reporting via dev_err below. > + */ > + cdev = of_platform_device_create_or_find(child, > + controller->dev); > + if (!cdev) > + continue; > + > + err = of_property_read_u32(child, "reg", &n); > + if (err == -EINVAL && info->nce == 1) > + n = 0; > + else if (err || n >= info->nce) > + continue; > + if (controller->chips[n]) { > + dev_err(&cdev->dev, > + "chip-id %u already in use in use by %s\n", > + n, dev_name(controller->chips[n]->nor.dev)); > + continue; > + } > + > + chip = devm_kzalloc(controller->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + continue; > + chip->controller = controller; > + chip->ctl = controller->regs + info->ctl0 + n * 4; > + chip->cs = n; > + > + chip->nor.dev = &cdev->dev; > + chip->nor.priv = chip; > + spi_nor_set_flash_node(&chip->nor, child); > + chip->nor.mtd.name = of_get_property(child, "label", NULL); > + chip->nor.read = aspeed_smc_read_user; > + chip->nor.write = aspeed_smc_write_user; > + chip->nor.read_reg = aspeed_smc_read_reg; > + chip->nor.write_reg = aspeed_smc_write_reg; > + > + err = aspeed_smc_chip_setup_init(chip, r); > + if (err) > + continue; > + > + /* > + * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach > + * when board support is present as determined by of property. > + */ > + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL); > + if (err) > + continue; > + > + err = aspeed_smc_chip_setup_finish(chip); > + if (err) > + continue; > + > + err = mtd_device_register(&chip->nor.mtd, NULL, 0); > + if (err) > + continue; What happens if some chip fails to register ? > + controller->chips[n] = chip; > + } > + > + /* Were any children registered? */ > + for (n = 0; n < info->nce; n++) > + if (controller->chips[n]) > + break; > + > + if (n == info->nce) > + return -ENODEV; > + > + return 0; > +} > + > +static struct platform_driver aspeed_smc_driver = { > + .probe = aspeed_smc_probe, > + .remove = aspeed_smc_remove, > + .driver = { > + .name = DEVICE_NAME, > + .of_match_table = aspeed_smc_matches, > + } > +}; > + > +module_platform_driver(aspeed_smc_driver); > + > +MODULE_DESCRIPTION("ASPEED Static Memory Controller Driver"); > +MODULE_AUTHOR("Milton Miller"); > +MODULE_LICENSE("GPL v2"); >
Hello Marek, Thank you for the review. I have answered a few of your questions; I'll leave the rest to Cedric. On Mon, Nov 21, 2016 at 8:13 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >> index 4a682ee0f632..96148600fdab 100644 >> --- a/drivers/mtd/spi-nor/Kconfig >> +++ b/drivers/mtd/spi-nor/Kconfig >> @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI >> Flash. Enable this option if you have a device with a SPIFI >> controller and want to access the Flash as a mtd device. >> >> +config ASPEED_FLASH_SPI > > Should be SPI_ASPEED , see the other controllers and keep the list sorted. Perhaps SPI_NOR_ASPEED so it's clear it's not a driver for a generic SPI bus? > >> + tristate "Aspeed flash controllers in SPI mode" >> + depends on HAS_IOMEM && OF >> + depends on ARCH_ASPEED || COMPILE_TEST >> + # IO_SPACE_LIMIT must be equivalent to (~0UL) >> + depends on !NEED_MACH_IO_H > > Why? > >> + help >> + This enables support for the New Static Memory Controller >> + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached >> + to SPI nor chips, and support for the SPI Memory controller >> + (SPI) for the BIOS. > > I think there is a naming chaos between FMC, SMC (as in Static MC) and > SMC (as in SPI MC). Yes, you're spot on. This naming chaos comes form the vendor's documentation. I think we could re-work this sentence to make it clearer. >> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, >> + size_t len) >> +{ > > What if start of buf is unaligned ? > >> + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { > > Uh, should use boolean OR, not bitwise or. Also, if you're testing > pointer for NULL, do if (!ptr) . > > if (!src || !buf || !len) > return; That's a different test. We're testing here that the buffers are aligned to see if we can do a word-at-a-time copy. If not, it falls through to do a byte-at-a-time copy. I think this covers your first question about buf being unaligned. Cedric, perhaps you could create a macro called IS_ALLIGNED to make it clear what this is doing? > > while (...) > >> + while (len > 3) { >> + *(u32 *)buf = readl(src); >> + buf += 4; >> + src += 4; >> + len -= 4; >> + } >> + } >> + >> + while (len--) { >> + *(u8 *)buf = readb(src); >> + buf += 1; >> + src += 1; >> + } >> + return 0; >> +} >> +/* >> + * SPI Flash Configuration Register (AST2400 SPI) >> + */ >> +#define CONFIG_REG 0x0 >> +#define CONFIG_ENABLE_CE_INACTIVE BIT(1) >> +#define CONFIG_WRITE BIT(0) > > #define[space]FOO[tab]BIT(bar) These are bits within the CONFIG_REG. It follows the same style as other spi-nor drivers, eg. nxp-spifi. I think it's somewhat clearer, but if you have a strong preference against then fair enough. > >> +/* >> + * SPI Flash Configuration Register (AST2500 SPI) >> + * Type setting Register (AST2500 FMC and AST2400 FMC) >> + */
On 11/21/2016 05:45 AM, Joel Stanley wrote: > Hello Marek, Hi! > Thank you for the review. I have answered a few of your questions; > I'll leave the rest to Cedric. > > On Mon, Nov 21, 2016 at 8:13 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >>> index 4a682ee0f632..96148600fdab 100644 >>> --- a/drivers/mtd/spi-nor/Kconfig >>> +++ b/drivers/mtd/spi-nor/Kconfig >>> @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI >>> Flash. Enable this option if you have a device with a SPIFI >>> controller and want to access the Flash as a mtd device. >>> >>> +config ASPEED_FLASH_SPI >> >> Should be SPI_ASPEED , see the other controllers and keep the list sorted. > > Perhaps SPI_NOR_ASPEED so it's clear it's not a driver for a generic SPI bus? But it's not a driver for SPI-NOR only either, it seems it's a driver for multiple distinct devices. >>> + tristate "Aspeed flash controllers in SPI mode" >>> + depends on HAS_IOMEM && OF >>> + depends on ARCH_ASPEED || COMPILE_TEST >>> + # IO_SPACE_LIMIT must be equivalent to (~0UL) >>> + depends on !NEED_MACH_IO_H >> >> Why? >> >>> + help >>> + This enables support for the New Static Memory Controller >>> + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached >>> + to SPI nor chips, and support for the SPI Memory controller >>> + (SPI) for the BIOS. >> >> I think there is a naming chaos between FMC, SMC (as in Static MC) and >> SMC (as in SPI MC). > > Yes, you're spot on. This naming chaos comes form the vendor's documentation. > > I think we could re-work this sentence to make it clearer. Please do before someone's head explodes from this :) >>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, >>> + size_t len) >>> +{ >> >> What if start of buf is unaligned ? >> >>> + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { >> >> Uh, should use boolean OR, not bitwise or. Also, if you're testing >> pointer for NULL, do if (!ptr) . >> >> if (!src || !buf || !len) >> return; > > That's a different test. We're testing here that the buffers are > aligned to see if we can do a word-at-a-time copy. > > If not, it falls through to do a byte-at-a-time copy. I think this > covers your first question about buf being unaligned. Ah, I see, thanks for clarifying. Comment in the code would be helpful for why what you're doing is OK. And I think you want to cast to uintptr_t instead to make this work on 64bit. > Cedric, perhaps you could create a macro called IS_ALLIGNED to make it > clear what this is doing? Yup, thanks! >> >> while (...) >> >>> + while (len > 3) { >>> + *(u32 *)buf = readl(src); >>> + buf += 4; >>> + src += 4; >>> + len -= 4; >>> + } >>> + } >>> + >>> + while (len--) { >>> + *(u8 *)buf = readb(src); >>> + buf += 1; >>> + src += 1; >>> + } >>> + return 0; >>> +} >>> +/* >>> + * SPI Flash Configuration Register (AST2400 SPI) >>> + */ >>> +#define CONFIG_REG 0x0 >>> +#define CONFIG_ENABLE_CE_INACTIVE BIT(1) >>> +#define CONFIG_WRITE BIT(0) >> >> #define[space]FOO[tab]BIT(bar) > > These are bits within the CONFIG_REG. It follows the same style as > other spi-nor drivers, eg. nxp-spifi. > > I think it's somewhat clearer, but if you have a strong preference > against then fair enough. It triggers my OCD, but I think it's a matter of taste, so I don't care that much.
Hello Marek, Sorry for the late answer. Here are a couple of answers to the naming problem On 11/25/2016 02:57 PM, Marek Vasut wrote: > On 11/21/2016 05:45 AM, Joel Stanley wrote: >> Hello Marek, > > Hi! > >> Thank you for the review. I have answered a few of your questions; >> I'll leave the rest to Cedric. >> >> On Mon, Nov 21, 2016 at 8:13 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >>>> index 4a682ee0f632..96148600fdab 100644 >>>> --- a/drivers/mtd/spi-nor/Kconfig >>>> +++ b/drivers/mtd/spi-nor/Kconfig >>>> @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI >>>> Flash. Enable this option if you have a device with a SPIFI >>>> controller and want to access the Flash as a mtd device. >>>> >>>> +config ASPEED_FLASH_SPI >>> >>> Should be SPI_ASPEED , see the other controllers and keep the list sorted. >> >> Perhaps SPI_NOR_ASPEED so it's clear it's not a driver for a generic SPI bus? > > But it's not a driver for SPI-NOR only either, it seems it's a driver > for multiple distinct devices. yes and I think it was a mistake to send the whole at once. We have added the support in qemu controller by controller and it was easier to understand. I need to split the patchset in the next version. >>>> + tristate "Aspeed flash controllers in SPI mode" >>>> + depends on HAS_IOMEM && OF >>>> + depends on ARCH_ASPEED || COMPILE_TEST >>>> + # IO_SPACE_LIMIT must be equivalent to (~0UL) >>>> + depends on !NEED_MACH_IO_H >>> >>> Why? >>> >>>> + help >>>> + This enables support for the New Static Memory Controller >>>> + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached >>>> + to SPI nor chips, and support for the SPI Memory controller >>>> + (SPI) for the BIOS. >>> >>> I think there is a naming chaos between FMC, SMC (as in Static MC) and >>> SMC (as in SPI MC). >> >> Yes, you're spot on. This naming chaos comes form the vendor's documentation. >> >> I think we could re-work this sentence to make it clearer. > > Please do before someone's head explodes from this :) Indeed .. :) I will give a try. Here is the status : Aspeed SoC AST2400 has a set of SMC (Static Memory Controller) controllers in which you find : - Legacy Static Memory Controller (called SMC in the spec) . base address at 0x16000000 . BMC firmware . old register set . supports NOR flash, NAND flash and SPI flash memory. All bootable. . 1 chip select pin (CE0) - New Static Memory Controller (called FMC in the spec) . base address at 0x16200000 . BMC firmware . new register set . supports NOR flash, NAND flash and SPI flash memory. . 5 chip select pins (CE0 ∼ CE4) - SPI Flash Controller (called SPI in the spec) . base address at 0x16300000 . host Firmware . exotic register set, between old and new ... . supports SPI flash memory . 1 chip select pin (CE0) Aspeed SoC AST2500 defines has a similar set of SMC (Static Memory Controller) controllers, more in the vein of the AST2400 FMC : - Legacy Static Memory Controller is gone, NOR and NAND support also - Firmware SPI Memory Controller (called FMC in the spec) . base address at 0x16200000 . BMC firmware . new register set . supports SPI flash memory. . 3 chip select pins (CE0 ~ CE2) - SPI Flash Controller (called SPI1 in the spec) first . base address at 0x16300000 . host firmware . new register set . supports SPI flash memory. . 2 chip select pins (CE0 ~ CE1) - SPI Flash Controller (called SPI2 in the spec) second . base address at 0x16310000 . host firmware . new register set . supports SPI flash memory. . 2 chip select pins (CE0 ~ CE1) So, these are the reasons behind the naming mess. Added to that the driver considers the acronym SMC to stand for SPI Memory Controller, which is wrong. I tried to reduce the confusion with some comments but that was a failure :) In qemu, we have used FMC (Firmware ...) and SPI to name the controllers and we just dropped the legacy SMC. I think using the same naming scheme is a good idea. We don't support anything else than SPI either so we can drop the other types for the moment. >>>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, >>>> + size_t len) >>>> +{ >>> >>> What if start of buf is unaligned ? >>> >>>> + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { >>> >>> Uh, should use boolean OR, not bitwise or. Also, if you're testing >>> pointer for NULL, do if (!ptr) . >>> >>> if (!src || !buf || !len) >>> return; >> >> That's a different test. We're testing here that the buffers are >> aligned to see if we can do a word-at-a-time copy. >> >> If not, it falls through to do a byte-at-a-time copy. I think this >> covers your first question about buf being unaligned. > > Ah, I see, thanks for clarifying. Comment in the code would be helpful > for why what you're doing is OK. And I think you want to cast to > uintptr_t instead to make this work on 64bit. yes >> Cedric, perhaps you could create a macro called IS_ALLIGNED to make it >> clear what this is doing? > > Yup, thanks! sure. I still need to go through Marek's comments in the initial email, I will split the pachset and fix the naming in next version. Thanks, C. >>> >>> while (...) >>> >>>> + while (len > 3) { >>>> + *(u32 *)buf = readl(src); >>>> + buf += 4; >>>> + src += 4; >>>> + len -= 4; >>>> + } >>>> + } >>>> + >>>> + while (len--) { >>>> + *(u8 *)buf = readb(src); >>>> + buf += 1; >>>> + src += 1; >>>> + } >>>> + return 0; >>>> +} >>>> +/* >>>> + * SPI Flash Configuration Register (AST2400 SPI) >>>> + */ >>>> +#define CONFIG_REG 0x0 >>>> +#define CONFIG_ENABLE_CE_INACTIVE BIT(1) >>>> +#define CONFIG_WRITE BIT(0) >>> >>> #define[space]FOO[tab]BIT(bar) >> >> These are bits within the CONFIG_REG. It follows the same style as >> other spi-nor drivers, eg. nxp-spifi. >> >> I think it's somewhat clearer, but if you have a strong preference >> against then fair enough. > > It triggers my OCD, but I think it's a matter of taste, so I don't care > that much. >
On 11/25/2016 04:01 PM, Cédric Le Goater wrote: > Hello Marek, Hi! > Sorry for the late answer. Here are a couple of answers to the naming > problem > > On 11/25/2016 02:57 PM, Marek Vasut wrote: >> On 11/21/2016 05:45 AM, Joel Stanley wrote: >>> Hello Marek, >> >> Hi! >> >>> Thank you for the review. I have answered a few of your questions; >>> I'll leave the rest to Cedric. >>> >>> On Mon, Nov 21, 2016 at 8:13 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >>>>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >>>>> index 4a682ee0f632..96148600fdab 100644 >>>>> --- a/drivers/mtd/spi-nor/Kconfig >>>>> +++ b/drivers/mtd/spi-nor/Kconfig >>>>> @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI >>>>> Flash. Enable this option if you have a device with a SPIFI >>>>> controller and want to access the Flash as a mtd device. >>>>> >>>>> +config ASPEED_FLASH_SPI >>>> >>>> Should be SPI_ASPEED , see the other controllers and keep the list sorted. >>> >>> Perhaps SPI_NOR_ASPEED so it's clear it's not a driver for a generic SPI bus? >> >> But it's not a driver for SPI-NOR only either, it seems it's a driver >> for multiple distinct devices. > > yes and I think it was a mistake to send the whole at once. We have > added the support in qemu controller by controller and it was easier > to understand. I need to split the patchset in the next version. Cool :-) >>>>> + tristate "Aspeed flash controllers in SPI mode" >>>>> + depends on HAS_IOMEM && OF >>>>> + depends on ARCH_ASPEED || COMPILE_TEST >>>>> + # IO_SPACE_LIMIT must be equivalent to (~0UL) >>>>> + depends on !NEED_MACH_IO_H >>>> >>>> Why? >>>> >>>>> + help >>>>> + This enables support for the New Static Memory Controller >>>>> + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached >>>>> + to SPI nor chips, and support for the SPI Memory controller >>>>> + (SPI) for the BIOS. >>>> >>>> I think there is a naming chaos between FMC, SMC (as in Static MC) and >>>> SMC (as in SPI MC). >>> >>> Yes, you're spot on. This naming chaos comes form the vendor's documentation. >>> >>> I think we could re-work this sentence to make it clearer. >> >> Please do before someone's head explodes from this :) > > Indeed .. :) I will give a try. Here is the status : > > Aspeed SoC AST2400 has a set of SMC (Static Memory Controller) > controllers in which you find : > > - Legacy Static Memory Controller (called SMC in the spec) > . base address at 0x16000000 > . BMC firmware > . old register set > . supports NOR flash, NAND flash and SPI flash memory. All bootable. > . 1 chip select pin (CE0) > > - New Static Memory Controller (called FMC in the spec) > . base address at 0x16200000 > . BMC firmware > . new register set > . supports NOR flash, NAND flash and SPI flash memory. > . 5 chip select pins (CE0 ∼ CE4) > > - SPI Flash Controller (called SPI in the spec) > . base address at 0x16300000 > . host Firmware > . exotic register set, between old and new ... > . supports SPI flash memory > . 1 chip select pin (CE0) This should be (except for the base address) be in some documentation, it helps. > Aspeed SoC AST2500 defines has a similar set of SMC (Static Memory > Controller) controllers, more in the vein of the AST2400 FMC : > > - Legacy Static Memory Controller is gone, NOR and NAND support also > > - Firmware SPI Memory Controller (called FMC in the spec) > . base address at 0x16200000 > . BMC firmware > . new register set > . supports SPI flash memory. > . 3 chip select pins (CE0 ~ CE2) > > - SPI Flash Controller (called SPI1 in the spec) first > . base address at 0x16300000 > . host firmware > . new register set > . supports SPI flash memory. > . 2 chip select pins (CE0 ~ CE1) > > - SPI Flash Controller (called SPI2 in the spec) second > . base address at 0x16310000 > . host firmware > . new register set > . supports SPI flash memory. > . 2 chip select pins (CE0 ~ CE1) > > > So, these are the reasons behind the naming mess. Added to that the > driver considers the acronym SMC to stand for SPI Memory Controller, > which is wrong. I tried to reduce the confusion with some comments but > that was a failure :) The explanation above is awesome though. > In qemu, we have used FMC (Firmware ...) and SPI to name the controllers > and we just dropped the legacy SMC. I think using the same naming scheme > is a good idea. We don't support anything else than SPI either so we can > drop the other types for the moment. One thing which I still ponder about is how do you support those controllers which support NOR and NAND flash and SPI, do you tap into all subsystems ? >>>>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, >>>>> + size_t len) >>>>> +{ >>>> >>>> What if start of buf is unaligned ? >>>> >>>>> + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { >>>> >>>> Uh, should use boolean OR, not bitwise or. Also, if you're testing >>>> pointer for NULL, do if (!ptr) . >>>> >>>> if (!src || !buf || !len) >>>> return; >>> >>> That's a different test. We're testing here that the buffers are >>> aligned to see if we can do a word-at-a-time copy. >>> >>> If not, it falls through to do a byte-at-a-time copy. I think this >>> covers your first question about buf being unaligned. >> >> Ah, I see, thanks for clarifying. Comment in the code would be helpful >> for why what you're doing is OK. And I think you want to cast to >> uintptr_t instead to make this work on 64bit. > > yes > >>> Cedric, perhaps you could create a macro called IS_ALLIGNED to make it >>> clear what this is doing? >> >> Yup, thanks! > > sure. I still need to go through Marek's comments in the initial email, > I will split the pachset and fix the naming in next version. Thanks!
Hello, >> Aspeed SoC AST2400 has a set of SMC (Static Memory Controller) >> controllers in which you find : >> >> - Legacy Static Memory Controller (called SMC in the spec) >> . base address at 0x16000000 >> . BMC firmware >> . old register set >> . supports NOR flash, NAND flash and SPI flash memory. All bootable. >> . 1 chip select pin (CE0) >> >> - New Static Memory Controller (called FMC in the spec) >> . base address at 0x16200000 >> . BMC firmware >> . new register set >> . supports NOR flash, NAND flash and SPI flash memory. >> . 5 chip select pins (CE0 ∼ CE4) >> >> - SPI Flash Controller (called SPI in the spec) >> . base address at 0x16300000 >> . host Firmware >> . exotic register set, between old and new ... >> . supports SPI flash memory >> . 1 chip select pin (CE0) > > This should be (except for the base address) be in some documentation, > it helps. Sure. I will add that. >> Aspeed SoC AST2500 defines has a similar set of SMC (Static Memory >> Controller) controllers, more in the vein of the AST2400 FMC : >> >> - Legacy Static Memory Controller is gone, NOR and NAND support also >> >> - Firmware SPI Memory Controller (called FMC in the spec) >> . base address at 0x16200000 >> . BMC firmware >> . new register set >> . supports SPI flash memory. >> . 3 chip select pins (CE0 ~ CE2) >> >> - SPI Flash Controller (called SPI1 in the spec) first >> . base address at 0x16300000 >> . host firmware >> . new register set >> . supports SPI flash memory. >> . 2 chip select pins (CE0 ~ CE1) >> >> - SPI Flash Controller (called SPI2 in the spec) second >> . base address at 0x16310000 >> . host firmware >> . new register set >> . supports SPI flash memory. >> . 2 chip select pins (CE0 ~ CE1) >> >> >> So, these are the reasons behind the naming mess. Added to that the >> driver considers the acronym SMC to stand for SPI Memory Controller, >> which is wrong. I tried to reduce the confusion with some comments but >> that was a failure :) > > The explanation above is awesome though. > >> In qemu, we have used FMC (Firmware ...) and SPI to name the controllers >> and we just dropped the legacy SMC. I think using the same naming scheme >> is a good idea. We don't support anything else than SPI either so we can >> drop the other types for the moment. > > One thing which I still ponder about is how do you support those > controllers which support NOR and NAND flash and SPI, do you tap > into all subsystems ? Aspeed dropped support for NAND and NOR flash in the AST2500 SoC, all controllers are SPI only and the AST2400 boards we have also only use SPI. So we did not consider the other flash flavors for the driver even though some board might use it, but I don't know that. Cheers, C.
On 11/28/2016 07:09 PM, Cédric Le Goater wrote: > Hello, > >>> Aspeed SoC AST2400 has a set of SMC (Static Memory Controller) >>> controllers in which you find : >>> >>> - Legacy Static Memory Controller (called SMC in the spec) >>> . base address at 0x16000000 >>> . BMC firmware >>> . old register set >>> . supports NOR flash, NAND flash and SPI flash memory. All bootable. >>> . 1 chip select pin (CE0) >>> >>> - New Static Memory Controller (called FMC in the spec) >>> . base address at 0x16200000 >>> . BMC firmware >>> . new register set >>> . supports NOR flash, NAND flash and SPI flash memory. >>> . 5 chip select pins (CE0 ∼ CE4) >>> >>> - SPI Flash Controller (called SPI in the spec) >>> . base address at 0x16300000 >>> . host Firmware >>> . exotic register set, between old and new ... >>> . supports SPI flash memory >>> . 1 chip select pin (CE0) >> >> This should be (except for the base address) be in some documentation, >> it helps. > > Sure. I will add that. > >>> Aspeed SoC AST2500 defines has a similar set of SMC (Static Memory >>> Controller) controllers, more in the vein of the AST2400 FMC : >>> >>> - Legacy Static Memory Controller is gone, NOR and NAND support also >>> >>> - Firmware SPI Memory Controller (called FMC in the spec) >>> . base address at 0x16200000 >>> . BMC firmware >>> . new register set >>> . supports SPI flash memory. >>> . 3 chip select pins (CE0 ~ CE2) >>> >>> - SPI Flash Controller (called SPI1 in the spec) first >>> . base address at 0x16300000 >>> . host firmware >>> . new register set >>> . supports SPI flash memory. >>> . 2 chip select pins (CE0 ~ CE1) >>> >>> - SPI Flash Controller (called SPI2 in the spec) second >>> . base address at 0x16310000 >>> . host firmware >>> . new register set >>> . supports SPI flash memory. >>> . 2 chip select pins (CE0 ~ CE1) >>> >>> >>> So, these are the reasons behind the naming mess. Added to that the >>> driver considers the acronym SMC to stand for SPI Memory Controller, >>> which is wrong. I tried to reduce the confusion with some comments but >>> that was a failure :) >> >> The explanation above is awesome though. >> >>> In qemu, we have used FMC (Firmware ...) and SPI to name the controllers >>> and we just dropped the legacy SMC. I think using the same naming scheme >>> is a good idea. We don't support anything else than SPI either so we can >>> drop the other types for the moment. >> >> One thing which I still ponder about is how do you support those >> controllers which support NOR and NAND flash and SPI, do you tap >> into all subsystems ? > > Aspeed dropped support for NAND and NOR flash in the AST2500 SoC, > all controllers are SPI only and the AST2400 boards we have also > only use SPI. So we did not consider the other flash flavors for > the driver even though some board might use it, but I don't know > that. So we cross that bridge when we come to it ? Hm, that could work. Please be sure to explicitly state you only support the SPI mode of operation in documentation. Thanks!
Hello Marek, On 11/20/2016 10:43 PM, Marek Vasut wrote: > On 11/09/2016 11:42 AM, Cédric Le Goater wrote: >> This driver adds mtd support for spi-nor attached to either or both of >> the Firmware Memory Controller or the SPI Flash Controller (AST2400 >> only). >> >> The SMC controllers on the Aspeed AST2500 SoC are very similar to the >> ones found on the AST2400. The differences are on the number of >> supported flash modules and their default mappings in the SoC address >> space. >> >> The Aspeed AST2500 has one SPI controller for the BMC firmware and two >> for the host firmware. All controllers have now the same set of >> registers compatible with the AST2400 FMC controller and the legacy >> 'SMC' controller is fully gone. >> >> Each controller has a memory range on which it maps its flash module >> slaves. Each slave is assigned a memory window for its mapping that >> can be changed at bootime with the Segment Address Register. >> >> Each SPI flash slave can then be accessed in two modes: Command and >> User. When in User mode, accesses to the memory segment of the slaves >> are translated in SPI transfers. When in Command mode, the HW >> generates the SPI commands automatically and the memory segment is >> accessed as if doing a MMIO. >> >> Currently, only the User mode is supported. Command mode needs a >> little more work to check that the memory window on the AHB bus fits >> the module size. >> >> Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> Tested on: >> >> * OpenPOWER Palmetto (AST2400) with >> FMC controller : n25q256a >> SPI controller : mx25l25635e and n25q512ax3 >> >> * Evaluation board (AST2500) with >> FMC controller : w25q256 >> SPI controller : w25q256 >> >> * OpenPOWER Witherspoon (AST2500) with >> FMC controller : mx25l25635e * 2 >> SPI controller : mx66l1g45g >> >> Changes since v2: >> >> - added a set4b ops to handle difference in the controllers >> - simplified the IO routines >> - prepared for fast read using dummy cycles >> >> Work in progress: >> >> - read optimization using higher SPI clock frequencies >> - command mode to direct reads from AHB >> - DMA support >> >> .../devicetree/bindings/mtd/aspeed-smc.txt | 72 ++ >> drivers/mtd/spi-nor/Kconfig | 12 + >> drivers/mtd/spi-nor/Makefile | 1 + >> drivers/mtd/spi-nor/aspeed-smc.c | 783 +++++++++++++++++++++ >> 4 files changed, 868 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt >> create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c >> >> diff --git a/Documentation/devicetree/bindings/mtd/aspeed-smc.txt b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt >> new file mode 100644 >> index 000000000000..7516b0c01fcf >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt >> @@ -0,0 +1,72 @@ >> +* Aspeed Static Memory controller >> +* Aspeed SPI Flash Controller >> + >> +The Static memory controller in the ast2400 supports 5 chip selects >> +each can be attached to NAND, parallel NOR, or SPI NOR attached flash. > > So this controller is supported by this driver, which behaves like a SPI > controller driver, yet the block can also do NAND and parallel NOR ? I think that was answered in a previous email. >> +The Firmware Memory Controller in the ast2500 supports 3 chip selects, >> +two of which are always in SPI-NOR mode and the third can be SPI-NOR >> +or parallel flash. The SPI flash controller in the ast2400 supports >> +one of 2 chip selects selected by pinmux. The two SPI flash >> +controllers in the ast2500 each support two chip selects. > > This paragraph is confusing, it's hard to grok down how many different > controllers does this driver support and what are their properties from > it. It is all there, it's just hard to read. I will start with the AST2500 controllers only, which are consistent. > Also, please split the DT bindings into separate patch and send them to > DT list for review. ok. >> +Required properties: >> + - compatible : Should be one of >> + "aspeed,ast2400-fmc" for the AST2400 Static Memory Controller >> + "aspeed,ast2400-smc" for the AST2400 SPI Flash Controller >> + "aspeed,ast2500-fmc" for the AST2500 Firmware SPI Memory Controller >> + "aspeed,ast2500-smc" for the AST2500 SPI Flash Controllers >> + - reg : the first contains the control register location and length, >> + the second contains the memory window mapping address and length >> + - #address-cells : must be 1 corresponding to chip select child binding >> + - #size-cells : must be 0 corresponding to chip select child binding >> + >> +Optional properties: >> + - interrupts : Should contain the interrupt for the dma device if an fmc >> + >> +The child nodes are the SPI Flash modules which must have a compatible >> +property as specified in bindings/mtd/jedec,spi-nor.txt >> + >> +Optionally, the child node can contain properties for SPI mode (may be >> +ignored): >> + - spi-max-frequency - (optional) max frequency of spi bus > > You don't need to add the (optional) here again. > >> +Example: >> +fmc: fmc@1e620000 { > > I'd suggest to keep the example minimal -- drop the partitions etc. ok >> + compatible = "aspeed,ast2400-fmc"; >> + reg = < 0x1e620000 0x94 >> + 0x20000000 0x02000000 >> + 0x22000000 0x02000000 >; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + interrupts = <19>; >> + flash@0 { >> + reg = < 0 >; >> + compatible = "jedec,spi-nor" ; >> + /* spi-max-frequency = <>; */ >> + /* m25p,fast-read; */ >> + #address-cells = <1>; >> + #size-cells = <1>; >> + partitions { >> + compatible = "fixed-partitions"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + boot@0 { >> + label = "boot-loader"; >> + reg = < 0 0x8000 >; >> + }; >> + image@8000 { >> + label = "kernel-image"; >> + reg = < 0x8000 0x1f8000 >; >> + }; >> + }; >> + }; >> + flash@1 { >> + reg = < 1 >; >> + compatible = "jedec,spi-nor" ; >> + label = "alt"; >> + /* spi-max-frequency = <>; */ >> + status = "fail"; >> + /* m25p,fast-read; */ >> + }; >> +}; >> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig >> index 4a682ee0f632..96148600fdab 100644 >> --- a/drivers/mtd/spi-nor/Kconfig >> +++ b/drivers/mtd/spi-nor/Kconfig >> @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI >> Flash. Enable this option if you have a device with a SPIFI >> controller and want to access the Flash as a mtd device. >> >> +config ASPEED_FLASH_SPI > > Should be SPI_ASPEED , see the other controllers and keep the list sorted. ok >> + tristate "Aspeed flash controllers in SPI mode" >> + depends on HAS_IOMEM && OF >> + depends on ARCH_ASPEED || COMPILE_TEST >> + # IO_SPACE_LIMIT must be equivalent to (~0UL) >> + depends on !NEED_MACH_IO_H > > Why? Some left over from the patch we have been keeping for too long (+1year) in our tree. >> + help >> + This enables support for the New Static Memory Controller >> + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached >> + to SPI nor chips, and support for the SPI Memory controller >> + (SPI) for the BIOS. > > I think there is a naming chaos between FMC, SMC (as in Static MC) and > SMC (as in SPI MC). I agree, I will try to clarify the naming in the next version. I will keep the smc suffix for the driver name though. >> endif # MTD_SPI_NOR >> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile >> index 121695e83542..c3174ebc45c2 100644 >> --- a/drivers/mtd/spi-nor/Makefile >> +++ b/drivers/mtd/spi-nor/Makefile >> @@ -4,4 +4,5 @@ obj-$(CONFIG_SPI_CADENCE_QUADSPI) += cadence-quadspi.o >> obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o >> obj-$(CONFIG_SPI_HISI_SFC) += hisi-sfc.o >> obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o >> +obj-$(CONFIG_ASPEED_FLASH_SPI) += aspeed-smc.o >> obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o >> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c >> new file mode 100644 >> index 000000000000..30662daf89ca >> --- /dev/null >> +++ b/drivers/mtd/spi-nor/aspeed-smc.c >> @@ -0,0 +1,783 @@ >> +/* >> + * ASPEED Static Memory Controller driver >> + * >> + * Copyright (c) 2015-2016, IBM Corporation. >> + * >> + * 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. >> + */ >> + >> +#include <linux/bug.h> >> +#include <linux/device.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/mtd/mtd.h> >> +#include <linux/mtd/partitions.h> >> +#include <linux/mtd/spi-nor.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/sysfs.h> >> + >> +#define DEVICE_NAME "aspeed-smc" >> + >> +/* >> + * In user mode all data bytes read or written to the chip decode address >> + * range are transferred to or from the SPI bus. The range is treated as a >> + * fifo of arbitratry 1, 2, or 4 byte width but each write has to be aligned >> + * to its size. The address within the multiple 8kB range is ignored when >> + * sending bytes to the SPI bus. >> + * >> + * On the arm architecture, as of Linux version 4.3, memcpy_fromio and >> + * memcpy_toio on little endian targets use the optimized memcpy routines >> + * that were designed for well behavied memory storage. These routines >> + * have a stutter if the source and destination are not both word aligned, >> + * once with a duplicate access to the source after aligning to the >> + * destination to a word boundary, and again with a duplicate access to >> + * the source when the final byte count is not word aligned. >> + * >> + * When writing or reading the fifo this stutter discards data or sends >> + * too much data to the fifo and can not be used by this driver. >> + * >> + * While the low level io string routines that implement the insl family do >> + * the desired accesses and memory increments, the cross architecture io >> + * macros make them essentially impossible to use on a memory mapped address >> + * instead of a a token from the call to iomap of an io port. >> + * >> + * These fifo routines use readl and friends to a constant io port and update >> + * the memory buffer pointer and count via explicit code. The final updates >> + * to len are optimistically suppressed. >> + */ >> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, >> + size_t len) >> +{ > > What if start of buf is unaligned ? > >> + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { > > Uh, should use boolean OR, not bitwise or. Also, if you're testing > pointer for NULL, do if (!ptr) . > > if (!src || !buf || !len) > return; > > while (...) I have added a bunch of IS_ALIGNED() in the next version to clarify what these tests are for. >> + while (len > 3) { >> + *(u32 *)buf = readl(src); >> + buf += 4; >> + src += 4; >> + len -= 4; >> + } >> + } >> + >> + while (len--) { >> + *(u8 *)buf = readb(src); >> + buf += 1; >> + src += 1; >> + } >> + return 0; >> +} >> + >> +static int aspeed_smc_write_to_ahb(void __iomem *dst, const void *buf, >> + size_t len) >> +{ >> + if ((((unsigned long)dst | (unsigned long)buf | len) & 3) == 0) { > > DTTO > >> + while (len > 3) { >> + u32 val = *(u32 *)buf; >> + >> + writel(val, dst); >> + buf += 4; >> + dst += 4; >> + len -= 4; >> + } >> + } >> + >> + while (len--) { >> + u8 val = *(u8 *)buf; >> + >> + writeb(val, dst); >> + buf += 1; >> + dst += 1; >> + } >> + return 0; >> +} >> + >> +enum smc_flash_type { >> + smc_type_nor = 0, /* controller connected to nor flash */ >> + smc_type_nand = 1, /* controller connected to nand flash */ >> + smc_type_spi = 2, /* controller connected to spi flash */ >> +}; >> + >> +struct aspeed_smc_chip; >> + >> +struct aspeed_smc_info { >> + u32 maxsize; /* maximum size of 1 chip window */ >> + u8 nce; /* number of chip enables */ >> + u8 maxwidth; /* max width of spi bus */ >> + bool hastype; /* flash type field exists in cfg reg */ >> + u8 we0; /* shift for write enable bit for ce 0 */ >> + u8 ctl0; /* offset in regs of ctl for ce 0 */ >> + u8 time; /* offset in regs of timing */ >> + u8 misc; /* offset in regs of misc settings */ >> + >> + void (*set_4b)(struct aspeed_smc_chip *chip); >> +}; >> + >> +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip); >> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip); >> + >> +static const struct aspeed_smc_info fmc_2400_info = { >> + .maxsize = 64 * 1024 * 1024, >> + .nce = 5, >> + .maxwidth = 4, >> + .hastype = true, > > Shouldn't all this be specified in DT ? I am not sure, these values are not configurable. I will remove a few of them in the next version as they are unused. >> + .we0 = 16, >> + .ctl0 = 0x10, >> + .time = 0x94, >> + .misc = 0x54, >> + .set_4b = aspeed_smc_chip_set_4b, >> +}; >> + >> +static const struct aspeed_smc_info smc_2400_info = { >> + .maxsize = 64 * 1024 * 1024, >> + .nce = 1, >> + .maxwidth = 2, >> + .hastype = false, >> + .we0 = 0, >> + .ctl0 = 0x04, >> + .time = 0x14, >> + .misc = 0x10, >> + .set_4b = aspeed_smc_chip_set_4b_smc_2400, >> +}; >> + >> +static const struct aspeed_smc_info fmc_2500_info = { >> + .maxsize = 256 * 1024 * 1024, >> + .nce = 3, >> + .maxwidth = 2, >> + .hastype = true, >> + .we0 = 16, >> + .ctl0 = 0x10, >> + .time = 0x94, >> + .misc = 0x54, >> + .set_4b = aspeed_smc_chip_set_4b, >> +}; >> + >> +static const struct aspeed_smc_info smc_2500_info = { >> + .maxsize = 128 * 1024 * 1024, >> + .nce = 2, >> + .maxwidth = 2, >> + .hastype = false, >> + .we0 = 16, >> + .ctl0 = 0x10, >> + .time = 0x94, >> + .misc = 0x54, >> + .set_4b = aspeed_smc_chip_set_4b, >> +}; >> + >> +enum smc_ctl_reg_value { >> + smc_base, /* base value without mode for other commands */ >> + smc_read, /* command reg for (maybe fast) reads */ >> + smc_write, /* command reg for writes with timings */ >> + smc_num_ctl_reg_values /* last value to get count of commands */ >> +}; >> + >> +struct aspeed_smc_controller; >> + >> +struct aspeed_smc_chip { >> + int cs; >> + struct aspeed_smc_controller *controller; >> + __le32 __iomem *ctl; /* control register */ > > Why do you use __le32* here and void* below ? arg. this is left over rust. > >> + void __iomem *base; /* base of chip window */ >> + __le32 ctl_val[smc_num_ctl_reg_values]; /* controls with timing */ >> + enum smc_flash_type type; /* what type of flash */ >> + struct spi_nor nor; >> +}; >> + >> +struct aspeed_smc_controller { >> + struct device *dev; >> + >> + struct mutex mutex; /* controller access mutex */ >> + const struct aspeed_smc_info *info; /* type info of controller */ >> + void __iomem *regs; /* controller registers */ >> + void __iomem *windows; /* per-chip windows resource */ >> + >> + struct aspeed_smc_chip *chips[0]; /* pointers to attached chips */ >> +}; >> + >> +/* >> + * SPI Flash Configuration Register (AST2400 SPI) >> + */ >> +#define CONFIG_REG 0x0 >> +#define CONFIG_ENABLE_CE_INACTIVE BIT(1) >> +#define CONFIG_WRITE BIT(0) > > #define[space]FOO[tab]BIT(bar) ok. I don't have a strong preference for the indent. > >> +/* >> + * SPI Flash Configuration Register (AST2500 SPI) >> + * Type setting Register (AST2500 FMC and AST2400 FMC) >> + */ >> +#define TYPE_SETTING_REG 0x0 >> +#define CONFIG_DISABLE_LEGACY BIT(31) /* 1 on AST2500 FMC */ >> + >> +#define CONFIG_CE2_WRITE BIT(18) >> +#define CONFIG_CE1_WRITE BIT(17) >> +#define CONFIG_CE0_WRITE BIT(16) >> + >> +#define CONFIG_CE2_TYPE BIT(4) /* FMC only */ >> +#define CONFIG_CE1_TYPE BIT(2) /* FMC only */ >> +#define CONFIG_CE0_TYPE BIT(0) /* FMC only */ >> + >> +/* >> + * CE Control Register (AST2500 SPI,FMC and AST2400 FMC) >> + */ >> +#define CE_CONTROL_REG 0x4 >> +#define CE2_ENABLE_CE_INACTIVE BIT(10) >> +#define CE1_ENABLE_CE_INACTIVE BIT(9) >> +#define CE0_ENABLE_CE_INACTIVE BIT(8) >> +#define CE2_CONTROL_EXTENDED BIT(2) >> +#define CE1_CONTROL_EXTENDED BIT(1) >> +#define CE0_CONTROL_EXTENDED BIT(0) >> + >> +/* CE0 Control Register (depends on the controller type) */ >> +#define CONTROL_SPI_AAF_MODE BIT(31) >> +#define CONTROL_SPI_IO_MODE_MASK GENMASK(30, 28) >> +#define CONTROL_SPI_IO_DUAL_DATA BIT(29) >> +#define CONTROL_SPI_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28)) >> +#define CONTROL_SPI_IO_QUAD_DATA BIT(30) >> +#define CONTROL_SPI_IO_QUAD_ADDR_DATA (BIT(30) | BIT(28)) >> +#define CONTROL_SPI_CE_INACTIVE_SHIFT 24 >> +#define CONTROL_SPI_CE_INACTIVE_MASK GENMASK(27, CONTROL_SPI_CE_INACTIVE_SHIFT) >> +/* 0 = 16T ... 15 = 1T T=HCLK */ >> +#define CONTROL_SPI_COMMAND_SHIFT 16 >> +#define CONTROL_SPI_DUMMY_CYCLE_COMMAND_OUTPUT BIT(15) >> +#define CONTROL_SPI_IO_DUMMY_CYCLES_HI BIT(14) >> +#define CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT 14 >> +#define CONTROL_SPI_IO_ADDRESS_4B BIT(13) /* AST2400 SPI */ >> +#define CONTROL_SPI_CLK_DIV4 BIT(13) /* others */ >> +#define CONTROL_SPI_RW_MERGE BIT(12) >> +#define CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT 6 >> +#define CONTROL_SPI_IO_DUMMY_CYCLES_LO GENMASK(7, \ >> + CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT) >> +#define CONTROL_SPI_IO_DUMMY_CYCLES_MASK (CONTROL_SPI_IO_DUMMY_CYCLES_HI | \ >> + CONTROL_SPI_IO_DUMMY_CYCLES_LO) >> +#define CONTROL_SPI_IO_DUMMY_CYCLES_SET(dummy) \ >> + (((((dummy) >> 2) & 0x1) << CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT) | \ >> + (((dummy) & 0x3) << CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT)) >> + >> +#define CONTROL_SPI_CLOCK_FREQ_SEL_SHIFT 8 >> +#define CONTROL_SPI_CLOCK_FREQ_SEL_MASK GENMASK(11, \ >> + CONTROL_SPI_CLOCK_FREQ_SEL_SHIFT) >> +#define CONTROL_SPI_LSB_FIRST BIT(5) >> +#define CONTROL_SPI_CLOCK_MODE_3 BIT(4) >> +#define CONTROL_SPI_IN_DUAL_DATA BIT(3) >> +#define CONTROL_SPI_CE_STOP_ACTIVE_CONTROL BIT(2) >> +#define CONTROL_SPI_COMMAND_MODE_MASK GENMASK(1, 0) >> +#define CONTROL_SPI_COMMAND_MODE_NORMAL (0) >> +#define CONTROL_SPI_COMMAND_MODE_FREAD (1) >> +#define CONTROL_SPI_COMMAND_MODE_WRITE (2) >> +#define CONTROL_SPI_COMMAND_MODE_USER (3) >> + >> +#define CONTROL_SPI_KEEP_MASK (CONTROL_SPI_AAF_MODE | \ >> + CONTROL_SPI_CE_INACTIVE_MASK | CONTROL_SPI_CLK_DIV4 | \ >> + CONTROL_SPI_IO_DUMMY_CYCLES_MASK | CONTROL_SPI_CLOCK_FREQ_SEL_MASK | \ >> + CONTROL_SPI_LSB_FIRST | CONTROL_SPI_CLOCK_MODE_3) >> + >> +/* Segment Address Registers */ >> +#define SEGMENT_ADDR_REG0 0x30 >> +#define SEGMENT_ADDR_START(_r) ((((_r) >> 16) & 0xFF) << 23) >> +#define SEGMENT_ADDR_END(_r) ((((_r) >> 24) & 0xFF) << 23) >> + >> +static u32 spi_control_fill_opcode(u8 opcode) >> +{ >> + return ((u32)(opcode)) << CONTROL_SPI_COMMAND_SHIFT; > > return opcode << CONTROL... , fix these horrible casts and parenthesis > globally. I killed the helper. >> +} >> + >> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip) >> +{ >> + return ((u32)1 << (chip->controller->info->we0 + chip->cs)); > > return BIT(...) > > I'm not sure these microfunctions are even needed. well, this one is used in a couple of places. >> +} >> + >> +static void aspeed_smc_chip_check_config(struct aspeed_smc_chip *chip) >> +{ >> + struct aspeed_smc_controller *controller = chip->controller; >> + u32 reg; >> + >> + reg = readl(controller->regs + CONFIG_REG); >> + >> + if (!(reg & aspeed_smc_chip_write_bit(chip))) { > > Invert the logic and return here, ie if (reg & BIT()) return , to trim > the indent. ok. >> + dev_dbg(controller->dev, >> + "config write is not set ! @%p: 0x%08x\n", >> + controller->regs + CONFIG_REG, reg); >> + reg |= aspeed_smc_chip_write_bit(chip); >> + writel(reg, controller->regs + CONFIG_REG); >> + } >> +} >> + >> +static void aspeed_smc_start_user(struct spi_nor *nor) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + u32 ctl = chip->ctl_val[smc_base]; >> + >> + /* >> + * When the chip is controlled in user mode, we need write >> + * access to send the opcodes to it. So check the config. >> + */ >> + aspeed_smc_chip_check_config(chip); >> + >> + ctl |= CONTROL_SPI_COMMAND_MODE_USER | >> + CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; >> + writel(ctl, chip->ctl); >> + >> + ctl &= ~CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; >> + writel(ctl, chip->ctl); >> +} >> + >> +static void aspeed_smc_stop_user(struct spi_nor *nor) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + u32 ctl = chip->ctl_val[smc_read]; >> + u32 ctl2 = ctl | CONTROL_SPI_COMMAND_MODE_USER | >> + CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; >> + >> + writel(ctl2, chip->ctl); /* stop user CE control */ >> + writel(ctl, chip->ctl); /* default to fread or read */ >> +} >> + >> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); > > Won't this have a horrid overhead ? Shall I use the prepare() and unprepare() ops instead ? >> + aspeed_smc_start_user(nor); >> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >> + aspeed_smc_read_from_ahb(buf, chip->base, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return 0; >> +} >> + >> +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, >> + int len) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); >> + >> + aspeed_smc_start_user(nor); >> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >> + aspeed_smc_write_to_ahb(chip->base, buf, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return 0; >> +} >> + >> +static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + __be32 temp; >> + u32 cmdaddr; >> + >> + switch (nor->addr_width) { >> + default: >> + WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n", >> + nor->addr_width); >> + /* FALLTHROUGH */ >> + case 3: >> + cmdaddr = addr & 0xFFFFFF; >> + >> + cmdaddr |= (u32)cmd << 24; > > Drop the cast. sure. > >> + temp = cpu_to_be32(cmdaddr); >> + aspeed_smc_write_to_ahb(chip->base, &temp, 4); >> + break; >> + case 4: >> + temp = cpu_to_be32(addr); >> + aspeed_smc_write_to_ahb(chip->base, &cmd, 1); >> + aspeed_smc_write_to_ahb(chip->base, &temp, 4); >> + break; >> + } >> +} >> + >> +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from, >> + size_t len, u_char *read_buf) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); >> + >> + aspeed_smc_start_user(nor); >> + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from); >> + aspeed_smc_read_from_ahb(read_buf, chip->base, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return len; >> +} >> + >> +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len, >> + const u_char *write_buf) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); >> + >> + aspeed_smc_start_user(nor); >> + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to); >> + aspeed_smc_write_to_ahb(chip->base, write_buf, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return len; >> +} >> + >> +static int aspeed_smc_remove(struct platform_device *dev) >> +{ >> + struct aspeed_smc_chip *chip; >> + struct aspeed_smc_controller *controller = platform_get_drvdata(dev); >> + int n; >> + >> + for (n = 0; n < controller->info->nce; n++) { >> + chip = controller->chips[n]; >> + if (chip) >> + mtd_device_unregister(&chip->nor.mtd); >> + } >> + >> + return 0; >> +} >> + >> +static const struct of_device_id aspeed_smc_matches[] = { >> + { .compatible = "aspeed,ast2400-fmc", .data = &fmc_2400_info }, >> + { .compatible = "aspeed,ast2400-smc", .data = &smc_2400_info }, >> + { .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info }, >> + { .compatible = "aspeed,ast2500-smc", .data = &smc_2500_info }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, aspeed_smc_matches); >> + >> +static struct platform_device * >> +of_platform_device_create_or_find(struct device_node *child, >> + struct device *parent) >> +{ >> + struct platform_device *cdev; >> + >> + cdev = of_platform_device_create(child, NULL, parent); >> + if (!cdev) >> + cdev = of_find_device_by_node(child); >> + return cdev; >> +} >> + >> +static void __iomem *window_start(struct aspeed_smc_controller *controller, >> + struct resource *r, unsigned int n) >> +{ >> + u32 offset = 0; >> + u32 reg; >> + >> + if (controller->info->nce > 1) { >> + reg = readl(controller->regs + SEGMENT_ADDR_REG0 + n * 4); >> + >> + if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg)) >> + return NULL; >> + >> + offset = SEGMENT_ADDR_START(reg) - r->start; >> + } >> + >> + return controller->windows + offset; >> +} >> + >> +static void aspeed_smc_chip_enable_write(struct aspeed_smc_chip *chip) >> +{ >> + struct aspeed_smc_controller *controller = chip->controller; >> + u32 reg; >> + >> + reg = readl(controller->regs + CONFIG_REG); >> + >> + reg |= aspeed_smc_chip_write_bit(chip); >> + writel(reg, controller->regs + CONFIG_REG); >> +} >> + >> +static void aspeed_smc_chip_set_type(struct aspeed_smc_chip *chip, int type) >> +{ >> + struct aspeed_smc_controller *controller = chip->controller; >> + u32 reg; >> + >> + reg = readl(controller->regs + CONFIG_REG); >> + >> + chip->type = type; > > You can move this above the readl() to make the RMW block consistent. ok >> + reg &= ~(3 << (chip->cs * 2)); >> + reg |= chip->type << (chip->cs * 2); >> + writel(reg, controller->regs + CONFIG_REG); >> +} >> + >> +/* >> + * The AST2500 FMC and AST2400 FMC flash controllers should be >> + * strapped by hardware, or autodetected, but the AST2500 SPI flash >> + * needs to be set. >> + */ >> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip) >> +{ >> + struct aspeed_smc_controller *controller = chip->controller; >> + u32 reg; >> + >> + if (chip->controller->info == &smc_2500_info) { >> + reg = readl(controller->regs + CE_CONTROL_REG); >> + reg |= 1 << chip->cs; >> + writel(reg, controller->regs + CE_CONTROL_REG); >> + } >> +} >> + >> +/* >> + * The AST2400 SPI flash controller does not have a CE Control >> + * register. It uses the CE0 control register to set 4Byte mode at the >> + * controller level. >> + */ >> +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip) >> +{ >> + chip->ctl_val[smc_base] |= CONTROL_SPI_IO_ADDRESS_4B; >> + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_ADDRESS_4B; >> +} >> + >> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip, >> + struct resource *r) >> +{ >> + struct aspeed_smc_controller *controller = chip->controller; >> + const struct aspeed_smc_info *info = controller->info; >> + u32 reg, base_reg; >> + >> + /* >> + * Always turn on the write enable bit to allow opcodes to be >> + * sent in user mode. >> + */ >> + aspeed_smc_chip_enable_write(chip); >> + >> + /* The driver only supports SPI type flash for the moment */ >> + if (info->hastype) >> + aspeed_smc_chip_set_type(chip, smc_type_spi); >> + >> + /* >> + * Configure chip base address in memory >> + */ >> + chip->base = window_start(controller, r, chip->cs); >> + if (!chip->base) { >> + dev_warn(chip->nor.dev, "CE segment window closed.\n"); >> + return -1; >> + } >> + >> + /* >> + * Read the existing control register to get basic values. >> + * >> + * XXX This register probably needs more sanitation. > > What's this comment about ? This is an initial comment about settings being done by U-Boot before the kernel is loaded, and some optimisations should be nice to keep, for the FMC controller. I will rephrase. >> + * Do we need support for mode 3 vs mode 0 clock phasing? >> + */ >> + reg = readl(chip->ctl); >> + dev_dbg(controller->dev, "control register: %08x\n", reg); >> + >> + base_reg = reg & CONTROL_SPI_KEEP_MASK; >> + if (base_reg != reg) { >> + dev_info(controller->dev, >> + "control register changed to: %08x\n", >> + base_reg); >> + } >> + chip->ctl_val[smc_base] = base_reg; >> + >> + /* >> + * Retain the prior value of the control register as the >> + * default if it was normal access mode. Otherwise start with >> + * the sanitized base value set to read mode. >> + */ >> + if ((reg & CONTROL_SPI_COMMAND_MODE_MASK) == >> + CONTROL_SPI_COMMAND_MODE_NORMAL) >> + chip->ctl_val[smc_read] = reg; >> + else >> + chip->ctl_val[smc_read] = chip->ctl_val[smc_base] | >> + CONTROL_SPI_COMMAND_MODE_NORMAL; >> + >> + dev_dbg(controller->dev, "default control register: %08x\n", >> + chip->ctl_val[smc_read]); >> + return 0; >> +} >> + >> +static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip) >> +{ >> + struct aspeed_smc_controller *controller = chip->controller; >> + const struct aspeed_smc_info *info = controller->info; >> + u32 cmd; >> + >> + if (chip->nor.addr_width == 4 && info->set_4b) >> + info->set_4b(chip); >> + >> + /* >> + * base mode has not been optimized yet. use it for writes. >> + */ >> + chip->ctl_val[smc_write] = chip->ctl_val[smc_base] | >> + spi_control_fill_opcode(chip->nor.program_opcode) | >> + CONTROL_SPI_COMMAND_MODE_WRITE; >> + >> + dev_dbg(controller->dev, "write control register: %08x\n", >> + chip->ctl_val[smc_write]); >> + >> + /* >> + * XXX TODO >> + * Adjust clocks if fast read and write are supported. >> + * Interpret spi-nor flags to adjust controller settings. >> + * Check if resource size big enough for detected chip and >> + * add support assisted (normal or fast-) read and dma. >> + */ >> + switch (chip->nor.flash_read) { >> + case SPI_NOR_NORMAL: >> + cmd = CONTROL_SPI_COMMAND_MODE_NORMAL; >> + break; >> + case SPI_NOR_FAST: >> + cmd = CONTROL_SPI_COMMAND_MODE_FREAD; >> + break; >> + default: >> + dev_err(chip->nor.dev, "unsupported SPI read mode\n"); >> + return -EINVAL; >> + } >> + >> + chip->ctl_val[smc_read] |= cmd | >> + CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8); >> + >> + dev_dbg(controller->dev, "base control register: %08x\n", >> + chip->ctl_val[smc_read]); >> + return 0; >> +} >> + >> +static int aspeed_smc_probe(struct platform_device *pdev) >> +{ >> + struct aspeed_smc_controller *controller; >> + const struct of_device_id *match; >> + const struct aspeed_smc_info *info; >> + struct resource *r; >> + struct device_node *child; >> + int err = 0; >> + unsigned int n; >> + >> + match = of_match_device(aspeed_smc_matches, &pdev->dev); >> + if (!match || !match->data) >> + return -ENODEV; >> + info = match->data; >> + >> + controller = devm_kzalloc(&pdev->dev, sizeof(*controller) + >> + info->nce * sizeof(controller->chips[0]), GFP_KERNEL); >> + if (!controller) >> + return -ENOMEM; >> + controller->info = info; >> + >> + mutex_init(&controller->mutex); >> + platform_set_drvdata(pdev, controller); >> + >> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + controller->regs = devm_ioremap_resource(&pdev->dev, r); >> + if (IS_ERR(controller->regs)) >> + return PTR_ERR(controller->regs); >> + >> + r = platform_get_resource(pdev, IORESOURCE_MEM, 1); >> + controller->windows = devm_ioremap_resource(&pdev->dev, r); >> + if (IS_ERR(controller->windows)) >> + return PTR_ERR(controller->windows); >> + >> + controller->dev = &pdev->dev; >> + >> + /* The pinmux or bootloader will disable the legacy mode controller */ >> + >> + /* >> + * XXX Need to add arbitration to the SMC (BIOS) controller if access >> + * is shared by the host. >> + */ >> + for_each_available_child_of_node(controller->dev->of_node, child) { >> + struct platform_device *cdev; >> + struct aspeed_smc_chip *chip; > > Pull this into separate function, ie. like cadence_qspi.c , so we can > identify the developing boilerplate easily. yes. I have reworked the code to follow the same pattern. > >> + /* This version does not support nand or nor flash devices. */ >> + if (!of_device_is_compatible(child, "jedec,spi-nor")) >> + continue; >> + >> + /* >> + * create a platform device from the of node. If the device >> + * already was created (eg from a prior bind/unbind cycle) >> + * reuse it. >> + * >> + * The creating the device node for the child here allows its >> + * use for error reporting via dev_err below. >> + */ >> + cdev = of_platform_device_create_or_find(child, >> + controller->dev); >> + if (!cdev) >> + continue; >> + >> + err = of_property_read_u32(child, "reg", &n); >> + if (err == -EINVAL && info->nce == 1) >> + n = 0; >> + else if (err || n >= info->nce) >> + continue; >> + if (controller->chips[n]) { >> + dev_err(&cdev->dev, >> + "chip-id %u already in use in use by %s\n", >> + n, dev_name(controller->chips[n]->nor.dev)); >> + continue; >> + } >> + >> + chip = devm_kzalloc(controller->dev, sizeof(*chip), GFP_KERNEL); >> + if (!chip) >> + continue; >> + chip->controller = controller; >> + chip->ctl = controller->regs + info->ctl0 + n * 4; >> + chip->cs = n; >> + >> + chip->nor.dev = &cdev->dev; >> + chip->nor.priv = chip; >> + spi_nor_set_flash_node(&chip->nor, child); >> + chip->nor.mtd.name = of_get_property(child, "label", NULL); >> + chip->nor.read = aspeed_smc_read_user; >> + chip->nor.write = aspeed_smc_write_user; >> + chip->nor.read_reg = aspeed_smc_read_reg; >> + chip->nor.write_reg = aspeed_smc_write_reg; >> + >> + err = aspeed_smc_chip_setup_init(chip, r); >> + if (err) >> + continue; >> + >> + /* >> + * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach >> + * when board support is present as determined by of property. >> + */ >> + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL); >> + if (err) >> + continue; >> + >> + err = aspeed_smc_chip_setup_finish(chip); >> + if (err) >> + continue; >> + >> + err = mtd_device_register(&chip->nor.mtd, NULL, 0); >> + if (err) >> + continue; > > What happens if some chip fails to register ? That's not correctly handled ... I have a fix for it. Thanks, C. > >> + controller->chips[n] = chip; >> + } >> + >> + /* Were any children registered? */ >> + for (n = 0; n < info->nce; n++) >> + if (controller->chips[n]) >> + break; >> + >> + if (n == info->nce) >> + return -ENODEV; >> + >> + return 0; >> +} >> + >> +static struct platform_driver aspeed_smc_driver = { >> + .probe = aspeed_smc_probe, >> + .remove = aspeed_smc_remove, >> + .driver = { >> + .name = DEVICE_NAME, >> + .of_match_table = aspeed_smc_matches, >> + } >> +}; >> + >> +module_platform_driver(aspeed_smc_driver); >> + >> +MODULE_DESCRIPTION("ASPEED Static Memory Controller Driver"); >> +MODULE_AUTHOR("Milton Miller"); >> +MODULE_LICENSE("GPL v2"); >> > >
On 12/08/2016 05:36 PM, Cédric Le Goater wrote: > Hello Marek, Hi! [...] >>> @@ -0,0 +1,72 @@ >>> +* Aspeed Static Memory controller >>> +* Aspeed SPI Flash Controller >>> + >>> +The Static memory controller in the ast2400 supports 5 chip selects >>> +each can be attached to NAND, parallel NOR, or SPI NOR attached flash. >> >> So this controller is supported by this driver, which behaves like a SPI >> controller driver, yet the block can also do NAND and parallel NOR ? > > I think that was answered in a previous email. Yep, thanks! >>> +The Firmware Memory Controller in the ast2500 supports 3 chip selects, >>> +two of which are always in SPI-NOR mode and the third can be SPI-NOR >>> +or parallel flash. The SPI flash controller in the ast2400 supports >>> +one of 2 chip selects selected by pinmux. The two SPI flash >>> +controllers in the ast2500 each support two chip selects. >> >> This paragraph is confusing, it's hard to grok down how many different >> controllers does this driver support and what are their properties from >> it. It is all there, it's just hard to read. > > I will start with the AST2500 controllers only, which are consistent. That works too :-) [...] >>> + tristate "Aspeed flash controllers in SPI mode" >>> + depends on HAS_IOMEM && OF >>> + depends on ARCH_ASPEED || COMPILE_TEST >>> + # IO_SPACE_LIMIT must be equivalent to (~0UL) >>> + depends on !NEED_MACH_IO_H >> >> Why? > > Some left over from the patch we have been keeping for too long (+1year) > in our tree. Hehe, I see, so it's fixed now. >>> + help >>> + This enables support for the New Static Memory Controller >>> + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached >>> + to SPI nor chips, and support for the SPI Memory controller >>> + (SPI) for the BIOS. >> >> I think there is a naming chaos between FMC, SMC (as in Static MC) and >> SMC (as in SPI MC). > > I agree, I will try to clarify the naming in the next version. I will keep > the smc suffix for the driver name though. Thanks! [...] >>> +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip); >>> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip); >>> + >>> +static const struct aspeed_smc_info fmc_2400_info = { >>> + .maxsize = 64 * 1024 * 1024, >>> + .nce = 5, >>> + .maxwidth = 4, >>> + .hastype = true, >> >> Shouldn't all this be specified in DT ? > > I am not sure, these values are not configurable. I will remove a few > of them in the next version as they are unused. Sooo, I had a discussion with Boris (which we didn't finish yet). Please ignore my comment for now and yes please, drop unused params. >>> + .we0 = 16, >>> + .ctl0 = 0x10, >>> + .time = 0x94, >>> + .misc = 0x54, >>> + .set_4b = aspeed_smc_chip_set_4b, >>> +}; [...] >>> +static u32 spi_control_fill_opcode(u8 opcode) >>> +{ >>> + return ((u32)(opcode)) << CONTROL_SPI_COMMAND_SHIFT; >> >> return opcode << CONTROL... , fix these horrible casts and parenthesis >> globally. > > I killed the helper. Nice, thanks for all the cleanups :) >>> +} >>> + >>> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip) >>> +{ >>> + return ((u32)1 << (chip->controller->info->we0 + chip->cs)); >> >> return BIT(...) >> >> I'm not sure these microfunctions are even needed. > > well, this one is used in a couple of places. Ah all right, then just return BIT(...) and be done with it. >>> +} [...] >>> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) >>> +{ >>> + struct aspeed_smc_chip *chip = nor->priv; >>> + >>> + mutex_lock(&chip->controller->mutex); >> >> Won't this have a horrid overhead ? > > Shall I use the prepare() and unprepare() ops instead ? I think that'd trim down the amount of locking, yes. >>> + aspeed_smc_start_user(nor); >>> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >>> + aspeed_smc_read_from_ahb(buf, chip->base, len); >>> + aspeed_smc_stop_user(nor); >>> + >>> + mutex_unlock(&chip->controller->mutex); >>> + >>> + return 0; >>> +} [...] >>> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip, >>> + struct resource *r) >>> +{ >>> + struct aspeed_smc_controller *controller = chip->controller; >>> + const struct aspeed_smc_info *info = controller->info; >>> + u32 reg, base_reg; >>> + >>> + /* >>> + * Always turn on the write enable bit to allow opcodes to be >>> + * sent in user mode. >>> + */ >>> + aspeed_smc_chip_enable_write(chip); >>> + >>> + /* The driver only supports SPI type flash for the moment */ >>> + if (info->hastype) >>> + aspeed_smc_chip_set_type(chip, smc_type_spi); >>> + >>> + /* >>> + * Configure chip base address in memory >>> + */ >>> + chip->base = window_start(controller, r, chip->cs); >>> + if (!chip->base) { >>> + dev_warn(chip->nor.dev, "CE segment window closed.\n"); >>> + return -1; >>> + } >>> + >>> + /* >>> + * Read the existing control register to get basic values. >>> + * >>> + * XXX This register probably needs more sanitation. >> >> What's this comment about ? > > This is an initial comment about settings being done by U-Boot > before the kernel is loaded, and some optimisations should be > nice to keep, for the FMC controller. I will rephrase. Shouldn't that be passed via DT instead ? We want to be bootloader agnostic in Linux. btw off-topic, but is U-Boot support for these aspeed devices ever be upstreamed ? >>> + * Do we need support for mode 3 vs mode 0 clock phasing? >>> + */ >>> + reg = readl(chip->ctl); >>> + dev_dbg(controller->dev, "control register: %08x\n", reg); >>> + >>> + base_reg = reg & CONTROL_SPI_KEEP_MASK; >>> + if (base_reg != reg) { >>> + dev_info(controller->dev, >>> + "control register changed to: %08x\n", >>> + base_reg); >>> + } [...] >>> + err = mtd_device_register(&chip->nor.mtd, NULL, 0); >>> + if (err) >>> + continue; >> >> What happens if some chip fails to register ? > > That's not correctly handled ... I have a fix for it. > > Thanks, Thanks for all the work :)
On 12/09/2016 03:29 AM, Marek Vasut wrote: > On 12/08/2016 05:36 PM, Cédric Le Goater wrote: >> Hello Marek, > > Hi! Hello Hello, [...] >>>> + /* >>>> + * Read the existing control register to get basic values. >>>> + * >>>> + * XXX This register probably needs more sanitation. >>> >>> What's this comment about ? >> >> This is an initial comment about settings being done by U-Boot >> before the kernel is loaded, and some optimisations should be >> nice to keep, for the FMC controller. I will rephrase. > > Shouldn't that be passed via DT instead ? We want to be bootloader > agnostic in Linux. Yes, clearly, Linux should do its own timing calibration and not depend on the bootloader but I am not sure how to do that correctly, yet, in the driver for all controllers. It depends on the controller type and a lot on the flash model being used, which can vary for the same board. U-Boot uses specific registers of the FMC controller to evaluate the best SPI clock frequency. So, for the moment, keeping the previous setting for : bits [11:8] SPI clock frequency selection is a nice thing to have. we can replace this setting when calibration is handled from Linux. The SPI controllers are different, they don't have the specific registers for calibration, and so the algo is bit more painful. > btw off-topic, but is U-Boot support for these aspeed devices ever > be upstreamed ? It is the plan to. This year, we have spent quite sometime porting, fixing, cleaning up the original code and getting ready to send a minimal framework, cpu and console, to mainline (flash and net drivers can come later). The code is operational on various boards but there is a major task we have not completed yet, which is to rewrite the 2/3 KLOC of assembly doing the DDR initialization :/ Once this is done, we should send. Here is the tree we use on OpenBMC : https://github.com/openbmc/u-boot/commits/v2016.07-aspeed-openbmc and a more recent branch with some extra cleanups : https://github.com/legoater/u-boot/commits/v2016.11-aspeed-openbmc Thanks, C.
Hi Cedric, Le 09/11/2016 à 11:42, Cédric Le Goater a écrit : > This driver adds mtd support for spi-nor attached to either or both of > the Firmware Memory Controller or the SPI Flash Controller (AST2400 > only). > > The SMC controllers on the Aspeed AST2500 SoC are very similar to the > ones found on the AST2400. The differences are on the number of > supported flash modules and their default mappings in the SoC address > space. > > The Aspeed AST2500 has one SPI controller for the BMC firmware and two > for the host firmware. All controllers have now the same set of > registers compatible with the AST2400 FMC controller and the legacy > 'SMC' controller is fully gone. > > Each controller has a memory range on which it maps its flash module > slaves. Each slave is assigned a memory window for its mapping that > can be changed at bootime with the Segment Address Register. > > Each SPI flash slave can then be accessed in two modes: Command and > User. When in User mode, accesses to the memory segment of the slaves > are translated in SPI transfers. When in Command mode, the HW > generates the SPI commands automatically and the memory segment is > accessed as if doing a MMIO. > > Currently, only the User mode is supported. Command mode needs a > little more work to check that the memory window on the AHB bus fits > the module size. > > Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- [...] > +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); > + > + aspeed_smc_start_user(nor); > + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); > + aspeed_smc_read_from_ahb(buf, chip->base, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return 0; > +} > + > +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, > + int len) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); > + > + aspeed_smc_start_user(nor); > + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); > + aspeed_smc_write_to_ahb(chip->base, buf, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return 0; > +} > + [...] > +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from, > + size_t len, u_char *read_buf) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); > + > + aspeed_smc_start_user(nor); > + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from); > + aspeed_smc_read_from_ahb(read_buf, chip->base, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return len; > +} > + > +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len, > + const u_char *write_buf) > +{ > + struct aspeed_smc_chip *chip = nor->priv; > + > + mutex_lock(&chip->controller->mutex); > + > + aspeed_smc_start_user(nor); > + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to); > + aspeed_smc_write_to_ahb(chip->base, write_buf, len); > + aspeed_smc_stop_user(nor); > + > + mutex_unlock(&chip->controller->mutex); > + > + return len; > +} As I've explained in review of the series v1, the chip->controller->mutex seems to be used only by aspeed_smc_read_reg(), aspeed_smc_write_reg(), aspeed_smc_read_user() and aspeed_smc_write_user(), the driver implementations of nor->read_reg(), nor->write_reg(), nor->read() and nor->write(). This driver locks the mutex at the very beginning of each of those functions and unlocks the mutex before returning. So my understanding is that you use this mutex to prevent aspeed_smc_[read|write]_[reg|user]() from being called concurrently. If so, the spi-nor framework already takes care of this issue with the couple of functions: spi_nor_lock_and_prep() / spi_nor_unlock_and_unprep(). Indeed, spi_nor_lock_and_prep() is called on entering and spi_nor_unlock_and_unprep() on exiting each of the following functions: - mtd->_read = spi_nor_read - mtd->_write = spi_nor_write / sst_write - mtd->_erase = spi_nor_erase - mtd->_lock = spi_nor_lock - mtd->_unlock = spi_nor_unlock - mtd->_is_lock = spi_nor_is_locked Except for spi_nor_scan(), which is called once for all during the probe and before registering the mtd_info structure, only the above mtd->_<handlers> call the nor->read_reg, nor->write_reg, nor->read, nor->erase and nor->write spi-nor handlers. So your spi-nor / aspeed_smc_<handlers> are always protected from concurrent access by the mutex locked in spi_nor_lock_and_prep(). So don't worry about concurrent access issue, the spi-nor framework takes care of you :) Best regards, Cyrille
On 12/09/2016 02:37 PM, Cyrille Pitchen wrote: > Hi Cedric, > > Le 09/11/2016 à 11:42, Cédric Le Goater a écrit : >> This driver adds mtd support for spi-nor attached to either or both of >> the Firmware Memory Controller or the SPI Flash Controller (AST2400 >> only). >> >> The SMC controllers on the Aspeed AST2500 SoC are very similar to the >> ones found on the AST2400. The differences are on the number of >> supported flash modules and their default mappings in the SoC address >> space. >> >> The Aspeed AST2500 has one SPI controller for the BMC firmware and two >> for the host firmware. All controllers have now the same set of >> registers compatible with the AST2400 FMC controller and the legacy >> 'SMC' controller is fully gone. >> >> Each controller has a memory range on which it maps its flash module >> slaves. Each slave is assigned a memory window for its mapping that >> can be changed at bootime with the Segment Address Register. >> >> Each SPI flash slave can then be accessed in two modes: Command and >> User. When in User mode, accesses to the memory segment of the slaves >> are translated in SPI transfers. When in Command mode, the HW >> generates the SPI commands automatically and the memory segment is >> accessed as if doing a MMIO. >> >> Currently, only the User mode is supported. Command mode needs a >> little more work to check that the memory window on the AHB bus fits >> the module size. >> >> Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- > [...] >> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); >> + >> + aspeed_smc_start_user(nor); >> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >> + aspeed_smc_read_from_ahb(buf, chip->base, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return 0; >> +} >> + >> +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, >> + int len) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); >> + >> + aspeed_smc_start_user(nor); >> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >> + aspeed_smc_write_to_ahb(chip->base, buf, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return 0; >> +} >> + > [...] >> +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from, >> + size_t len, u_char *read_buf) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); >> + >> + aspeed_smc_start_user(nor); >> + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from); >> + aspeed_smc_read_from_ahb(read_buf, chip->base, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return len; >> +} >> + >> +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len, >> + const u_char *write_buf) >> +{ >> + struct aspeed_smc_chip *chip = nor->priv; >> + >> + mutex_lock(&chip->controller->mutex); >> + >> + aspeed_smc_start_user(nor); >> + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to); >> + aspeed_smc_write_to_ahb(chip->base, write_buf, len); >> + aspeed_smc_stop_user(nor); >> + >> + mutex_unlock(&chip->controller->mutex); >> + >> + return len; >> +} > > As I've explained in review of the series v1, the chip->controller->mutex > seems to be used only by aspeed_smc_read_reg(), aspeed_smc_write_reg(), > aspeed_smc_read_user() and aspeed_smc_write_user(), the driver > implementations of nor->read_reg(), nor->write_reg(), nor->read() and > nor->write(). > > This driver locks the mutex at the very beginning of each of those > functions and unlocks the mutex before returning. > > So my understanding is that you use this mutex to prevent > aspeed_smc_[read|write]_[reg|user]() from being called concurrently. > > If so, the spi-nor framework already takes care of this issue with the > couple of functions: spi_nor_lock_and_prep() / spi_nor_unlock_and_unprep(). > > Indeed, spi_nor_lock_and_prep() is called on entering and > spi_nor_unlock_and_unprep() on exiting each of the following functions: > - mtd->_read = spi_nor_read > - mtd->_write = spi_nor_write / sst_write > - mtd->_erase = spi_nor_erase > - mtd->_lock = spi_nor_lock > - mtd->_unlock = spi_nor_unlock > - mtd->_is_lock = spi_nor_is_locked > > Except for spi_nor_scan(), which is called once for all during the probe > and before registering the mtd_info structure, only the above > mtd->_<handlers> call the nor->read_reg, nor->write_reg, nor->read, > nor->erase and nor->write spi-nor handlers. > So your spi-nor / aspeed_smc_<handlers> are always protected from > concurrent access by the mutex locked in spi_nor_lock_and_prep(). > > So don't worry about concurrent access issue, the spi-nor framework takes > care of you :) Does it take care of me even if I have multiple flashes ? I recall I had to put mutexes into prepare and unprepare myself in the CQSPI driver to prevent problems when accessing two flashes simultaneously.
Le 09/12/2016 à 15:03, Marek Vasut a écrit : > On 12/09/2016 02:37 PM, Cyrille Pitchen wrote: >> Hi Cedric, >> >> Le 09/11/2016 à 11:42, Cédric Le Goater a écrit : >>> This driver adds mtd support for spi-nor attached to either or both of >>> the Firmware Memory Controller or the SPI Flash Controller (AST2400 >>> only). >>> >>> The SMC controllers on the Aspeed AST2500 SoC are very similar to the >>> ones found on the AST2400. The differences are on the number of >>> supported flash modules and their default mappings in the SoC address >>> space. >>> >>> The Aspeed AST2500 has one SPI controller for the BMC firmware and two >>> for the host firmware. All controllers have now the same set of >>> registers compatible with the AST2400 FMC controller and the legacy >>> 'SMC' controller is fully gone. >>> >>> Each controller has a memory range on which it maps its flash module >>> slaves. Each slave is assigned a memory window for its mapping that >>> can be changed at bootime with the Segment Address Register. >>> >>> Each SPI flash slave can then be accessed in two modes: Command and >>> User. When in User mode, accesses to the memory segment of the slaves >>> are translated in SPI transfers. When in Command mode, the HW >>> generates the SPI commands automatically and the memory segment is >>> accessed as if doing a MMIO. >>> >>> Currently, only the User mode is supported. Command mode needs a >>> little more work to check that the memory window on the AHB bus fits >>> the module size. >>> >>> Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> >>> >>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>> --- >> [...] >>> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) >>> +{ >>> + struct aspeed_smc_chip *chip = nor->priv; >>> + >>> + mutex_lock(&chip->controller->mutex); >>> + >>> + aspeed_smc_start_user(nor); >>> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >>> + aspeed_smc_read_from_ahb(buf, chip->base, len); >>> + aspeed_smc_stop_user(nor); >>> + >>> + mutex_unlock(&chip->controller->mutex); >>> + >>> + return 0; >>> +} >>> + >>> +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, >>> + int len) >>> +{ >>> + struct aspeed_smc_chip *chip = nor->priv; >>> + >>> + mutex_lock(&chip->controller->mutex); >>> + >>> + aspeed_smc_start_user(nor); >>> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >>> + aspeed_smc_write_to_ahb(chip->base, buf, len); >>> + aspeed_smc_stop_user(nor); >>> + >>> + mutex_unlock(&chip->controller->mutex); >>> + >>> + return 0; >>> +} >>> + >> [...] >>> +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from, >>> + size_t len, u_char *read_buf) >>> +{ >>> + struct aspeed_smc_chip *chip = nor->priv; >>> + >>> + mutex_lock(&chip->controller->mutex); >>> + >>> + aspeed_smc_start_user(nor); >>> + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from); >>> + aspeed_smc_read_from_ahb(read_buf, chip->base, len); >>> + aspeed_smc_stop_user(nor); >>> + >>> + mutex_unlock(&chip->controller->mutex); >>> + >>> + return len; >>> +} >>> + >>> +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len, >>> + const u_char *write_buf) >>> +{ >>> + struct aspeed_smc_chip *chip = nor->priv; >>> + >>> + mutex_lock(&chip->controller->mutex); >>> + >>> + aspeed_smc_start_user(nor); >>> + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to); >>> + aspeed_smc_write_to_ahb(chip->base, write_buf, len); >>> + aspeed_smc_stop_user(nor); >>> + >>> + mutex_unlock(&chip->controller->mutex); >>> + >>> + return len; >>> +} >> >> As I've explained in review of the series v1, the chip->controller->mutex >> seems to be used only by aspeed_smc_read_reg(), aspeed_smc_write_reg(), >> aspeed_smc_read_user() and aspeed_smc_write_user(), the driver >> implementations of nor->read_reg(), nor->write_reg(), nor->read() and >> nor->write(). >> >> This driver locks the mutex at the very beginning of each of those >> functions and unlocks the mutex before returning. >> >> So my understanding is that you use this mutex to prevent >> aspeed_smc_[read|write]_[reg|user]() from being called concurrently. >> >> If so, the spi-nor framework already takes care of this issue with the >> couple of functions: spi_nor_lock_and_prep() / spi_nor_unlock_and_unprep(). >> >> Indeed, spi_nor_lock_and_prep() is called on entering and >> spi_nor_unlock_and_unprep() on exiting each of the following functions: >> - mtd->_read = spi_nor_read >> - mtd->_write = spi_nor_write / sst_write >> - mtd->_erase = spi_nor_erase >> - mtd->_lock = spi_nor_lock >> - mtd->_unlock = spi_nor_unlock >> - mtd->_is_lock = spi_nor_is_locked >> >> Except for spi_nor_scan(), which is called once for all during the probe >> and before registering the mtd_info structure, only the above >> mtd->_<handlers> call the nor->read_reg, nor->write_reg, nor->read, >> nor->erase and nor->write spi-nor handlers. >> So your spi-nor / aspeed_smc_<handlers> are always protected from >> concurrent access by the mutex locked in spi_nor_lock_and_prep(). >> >> So don't worry about concurrent access issue, the spi-nor framework takes >> care of you :) > > Does it take care of me even if I have multiple flashes ? I recall I had > to put mutexes into prepare and unprepare myself in the CQSPI driver to > prevent problems when accessing two flashes simultaneously. > > Well indeed you're right, with multiple flashes I guess the driver will need to use an additional mutex. Then it can be placed either in each read_reg/write_reg/read/write handlers like Cedric did or in prepare/unprepare handlers like Marek did in the Cadence Quad SPI drivers. Both solutions work and are fine for me. Anyway, sorry for the noise! Best regards, Cyrille
Hello Cyrille On 12/09/2016 03:13 PM, Cyrille Pitchen wrote: > Le 09/12/2016 à 15:03, Marek Vasut a écrit : >> On 12/09/2016 02:37 PM, Cyrille Pitchen wrote: >>> Hi Cedric, >>> >>> Le 09/11/2016 à 11:42, Cédric Le Goater a écrit : >>>> This driver adds mtd support for spi-nor attached to either or both of >>>> the Firmware Memory Controller or the SPI Flash Controller (AST2400 >>>> only). >>>> >>>> The SMC controllers on the Aspeed AST2500 SoC are very similar to the >>>> ones found on the AST2400. The differences are on the number of >>>> supported flash modules and their default mappings in the SoC address >>>> space. >>>> >>>> The Aspeed AST2500 has one SPI controller for the BMC firmware and two >>>> for the host firmware. All controllers have now the same set of >>>> registers compatible with the AST2400 FMC controller and the legacy >>>> 'SMC' controller is fully gone. >>>> >>>> Each controller has a memory range on which it maps its flash module >>>> slaves. Each slave is assigned a memory window for its mapping that >>>> can be changed at bootime with the Segment Address Register. >>>> >>>> Each SPI flash slave can then be accessed in two modes: Command and >>>> User. When in User mode, accesses to the memory segment of the slaves >>>> are translated in SPI transfers. When in Command mode, the HW >>>> generates the SPI commands automatically and the memory segment is >>>> accessed as if doing a MMIO. >>>> >>>> Currently, only the User mode is supported. Command mode needs a >>>> little more work to check that the memory window on the AHB bus fits >>>> the module size. >>>> >>>> Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>> [...] >>>> +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) >>>> +{ >>>> + struct aspeed_smc_chip *chip = nor->priv; >>>> + >>>> + mutex_lock(&chip->controller->mutex); >>>> + >>>> + aspeed_smc_start_user(nor); >>>> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >>>> + aspeed_smc_read_from_ahb(buf, chip->base, len); >>>> + aspeed_smc_stop_user(nor); >>>> + >>>> + mutex_unlock(&chip->controller->mutex); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, >>>> + int len) >>>> +{ >>>> + struct aspeed_smc_chip *chip = nor->priv; >>>> + >>>> + mutex_lock(&chip->controller->mutex); >>>> + >>>> + aspeed_smc_start_user(nor); >>>> + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); >>>> + aspeed_smc_write_to_ahb(chip->base, buf, len); >>>> + aspeed_smc_stop_user(nor); >>>> + >>>> + mutex_unlock(&chip->controller->mutex); >>>> + >>>> + return 0; >>>> +} >>>> + >>> [...] >>>> +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from, >>>> + size_t len, u_char *read_buf) >>>> +{ >>>> + struct aspeed_smc_chip *chip = nor->priv; >>>> + >>>> + mutex_lock(&chip->controller->mutex); >>>> + >>>> + aspeed_smc_start_user(nor); >>>> + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from); >>>> + aspeed_smc_read_from_ahb(read_buf, chip->base, len); >>>> + aspeed_smc_stop_user(nor); >>>> + >>>> + mutex_unlock(&chip->controller->mutex); >>>> + >>>> + return len; >>>> +} >>>> + >>>> +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len, >>>> + const u_char *write_buf) >>>> +{ >>>> + struct aspeed_smc_chip *chip = nor->priv; >>>> + >>>> + mutex_lock(&chip->controller->mutex); >>>> + >>>> + aspeed_smc_start_user(nor); >>>> + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to); >>>> + aspeed_smc_write_to_ahb(chip->base, write_buf, len); >>>> + aspeed_smc_stop_user(nor); >>>> + >>>> + mutex_unlock(&chip->controller->mutex); >>>> + >>>> + return len; >>>> +} >>> >>> As I've explained in review of the series v1, the chip->controller->mutex >>> seems to be used only by aspeed_smc_read_reg(), aspeed_smc_write_reg(), >>> aspeed_smc_read_user() and aspeed_smc_write_user(), the driver >>> implementations of nor->read_reg(), nor->write_reg(), nor->read() and >>> nor->write(). >>> >>> This driver locks the mutex at the very beginning of each of those >>> functions and unlocks the mutex before returning. >>> >>> So my understanding is that you use this mutex to prevent >>> aspeed_smc_[read|write]_[reg|user]() from being called concurrently. >>> >>> If so, the spi-nor framework already takes care of this issue with the >>> couple of functions: spi_nor_lock_and_prep() / spi_nor_unlock_and_unprep(). >>> >>> Indeed, spi_nor_lock_and_prep() is called on entering and >>> spi_nor_unlock_and_unprep() on exiting each of the following functions: >>> - mtd->_read = spi_nor_read >>> - mtd->_write = spi_nor_write / sst_write >>> - mtd->_erase = spi_nor_erase >>> - mtd->_lock = spi_nor_lock >>> - mtd->_unlock = spi_nor_unlock >>> - mtd->_is_lock = spi_nor_is_locked >>> >>> Except for spi_nor_scan(), which is called once for all during the probe >>> and before registering the mtd_info structure, only the above >>> mtd->_<handlers> call the nor->read_reg, nor->write_reg, nor->read, >>> nor->erase and nor->write spi-nor handlers. >>> So your spi-nor / aspeed_smc_<handlers> are always protected from >>> concurrent access by the mutex locked in spi_nor_lock_and_prep(). >>> >>> So don't worry about concurrent access issue, the spi-nor framework takes >>> care of you :) >> >> Does it take care of me even if I have multiple flashes ? I recall I had >> to put mutexes into prepare and unprepare myself in the CQSPI driver to >> prevent problems when accessing two flashes simultaneously. >> >> > > Well indeed you're right, with multiple flashes I guess the driver will > need to use an additional mutex. Then it can be placed either in each > read_reg/write_reg/read/write handlers like Cedric did or in > prepare/unprepare handlers like Marek did in the Cadence Quad SPI drivers. > Both solutions work and are fine for me. So I will go for the prepare/unprepare handler solution for now. Thanks, C.
diff --git a/Documentation/devicetree/bindings/mtd/aspeed-smc.txt b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt new file mode 100644 index 000000000000..7516b0c01fcf --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt @@ -0,0 +1,72 @@ +* Aspeed Static Memory controller +* Aspeed SPI Flash Controller + +The Static memory controller in the ast2400 supports 5 chip selects +each can be attached to NAND, parallel NOR, or SPI NOR attached flash. +The Firmware Memory Controller in the ast2500 supports 3 chip selects, +two of which are always in SPI-NOR mode and the third can be SPI-NOR +or parallel flash. The SPI flash controller in the ast2400 supports +one of 2 chip selects selected by pinmux. The two SPI flash +controllers in the ast2500 each support two chip selects. + +Required properties: + - compatible : Should be one of + "aspeed,ast2400-fmc" for the AST2400 Static Memory Controller + "aspeed,ast2400-smc" for the AST2400 SPI Flash Controller + "aspeed,ast2500-fmc" for the AST2500 Firmware SPI Memory Controller + "aspeed,ast2500-smc" for the AST2500 SPI Flash Controllers + - reg : the first contains the control register location and length, + the second contains the memory window mapping address and length + - #address-cells : must be 1 corresponding to chip select child binding + - #size-cells : must be 0 corresponding to chip select child binding + +Optional properties: + - interrupts : Should contain the interrupt for the dma device if an fmc + +The child nodes are the SPI Flash modules which must have a compatible +property as specified in bindings/mtd/jedec,spi-nor.txt + +Optionally, the child node can contain properties for SPI mode (may be +ignored): + - spi-max-frequency - (optional) max frequency of spi bus + + +Example: +fmc: fmc@1e620000 { + compatible = "aspeed,ast2400-fmc"; + reg = < 0x1e620000 0x94 + 0x20000000 0x02000000 + 0x22000000 0x02000000 >; + #address-cells = <1>; + #size-cells = <0>; + interrupts = <19>; + flash@0 { + reg = < 0 >; + compatible = "jedec,spi-nor" ; + /* spi-max-frequency = <>; */ + /* m25p,fast-read; */ + #address-cells = <1>; + #size-cells = <1>; + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + boot@0 { + label = "boot-loader"; + reg = < 0 0x8000 >; + }; + image@8000 { + label = "kernel-image"; + reg = < 0x8000 0x1f8000 >; + }; + }; + }; + flash@1 { + reg = < 1 >; + compatible = "jedec,spi-nor" ; + label = "alt"; + /* spi-max-frequency = <>; */ + status = "fail"; + /* m25p,fast-read; */ + }; +}; diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig index 4a682ee0f632..96148600fdab 100644 --- a/drivers/mtd/spi-nor/Kconfig +++ b/drivers/mtd/spi-nor/Kconfig @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI Flash. Enable this option if you have a device with a SPIFI controller and want to access the Flash as a mtd device. +config ASPEED_FLASH_SPI + tristate "Aspeed flash controllers in SPI mode" + depends on HAS_IOMEM && OF + depends on ARCH_ASPEED || COMPILE_TEST + # IO_SPACE_LIMIT must be equivalent to (~0UL) + depends on !NEED_MACH_IO_H + help + This enables support for the New Static Memory Controller + (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached + to SPI nor chips, and support for the SPI Memory controller + (SPI) for the BIOS. + endif # MTD_SPI_NOR diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile index 121695e83542..c3174ebc45c2 100644 --- a/drivers/mtd/spi-nor/Makefile +++ b/drivers/mtd/spi-nor/Makefile @@ -4,4 +4,5 @@ obj-$(CONFIG_SPI_CADENCE_QUADSPI) += cadence-quadspi.o obj-$(CONFIG_SPI_FSL_QUADSPI) += fsl-quadspi.o obj-$(CONFIG_SPI_HISI_SFC) += hisi-sfc.o obj-$(CONFIG_MTD_MT81xx_NOR) += mtk-quadspi.o +obj-$(CONFIG_ASPEED_FLASH_SPI) += aspeed-smc.o obj-$(CONFIG_SPI_NXP_SPIFI) += nxp-spifi.o diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c new file mode 100644 index 000000000000..30662daf89ca --- /dev/null +++ b/drivers/mtd/spi-nor/aspeed-smc.c @@ -0,0 +1,783 @@ +/* + * ASPEED Static Memory Controller driver + * + * Copyright (c) 2015-2016, IBM Corporation. + * + * 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. + */ + +#include <linux/bug.h> +#include <linux/device.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/mtd/mtd.h> +#include <linux/mtd/partitions.h> +#include <linux/mtd/spi-nor.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/sysfs.h> + +#define DEVICE_NAME "aspeed-smc" + +/* + * In user mode all data bytes read or written to the chip decode address + * range are transferred to or from the SPI bus. The range is treated as a + * fifo of arbitratry 1, 2, or 4 byte width but each write has to be aligned + * to its size. The address within the multiple 8kB range is ignored when + * sending bytes to the SPI bus. + * + * On the arm architecture, as of Linux version 4.3, memcpy_fromio and + * memcpy_toio on little endian targets use the optimized memcpy routines + * that were designed for well behavied memory storage. These routines + * have a stutter if the source and destination are not both word aligned, + * once with a duplicate access to the source after aligning to the + * destination to a word boundary, and again with a duplicate access to + * the source when the final byte count is not word aligned. + * + * When writing or reading the fifo this stutter discards data or sends + * too much data to the fifo and can not be used by this driver. + * + * While the low level io string routines that implement the insl family do + * the desired accesses and memory increments, the cross architecture io + * macros make them essentially impossible to use on a memory mapped address + * instead of a a token from the call to iomap of an io port. + * + * These fifo routines use readl and friends to a constant io port and update + * the memory buffer pointer and count via explicit code. The final updates + * to len are optimistically suppressed. + */ +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src, + size_t len) +{ + if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) { + while (len > 3) { + *(u32 *)buf = readl(src); + buf += 4; + src += 4; + len -= 4; + } + } + + while (len--) { + *(u8 *)buf = readb(src); + buf += 1; + src += 1; + } + return 0; +} + +static int aspeed_smc_write_to_ahb(void __iomem *dst, const void *buf, + size_t len) +{ + if ((((unsigned long)dst | (unsigned long)buf | len) & 3) == 0) { + while (len > 3) { + u32 val = *(u32 *)buf; + + writel(val, dst); + buf += 4; + dst += 4; + len -= 4; + } + } + + while (len--) { + u8 val = *(u8 *)buf; + + writeb(val, dst); + buf += 1; + dst += 1; + } + return 0; +} + +enum smc_flash_type { + smc_type_nor = 0, /* controller connected to nor flash */ + smc_type_nand = 1, /* controller connected to nand flash */ + smc_type_spi = 2, /* controller connected to spi flash */ +}; + +struct aspeed_smc_chip; + +struct aspeed_smc_info { + u32 maxsize; /* maximum size of 1 chip window */ + u8 nce; /* number of chip enables */ + u8 maxwidth; /* max width of spi bus */ + bool hastype; /* flash type field exists in cfg reg */ + u8 we0; /* shift for write enable bit for ce 0 */ + u8 ctl0; /* offset in regs of ctl for ce 0 */ + u8 time; /* offset in regs of timing */ + u8 misc; /* offset in regs of misc settings */ + + void (*set_4b)(struct aspeed_smc_chip *chip); +}; + +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip); +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip); + +static const struct aspeed_smc_info fmc_2400_info = { + .maxsize = 64 * 1024 * 1024, + .nce = 5, + .maxwidth = 4, + .hastype = true, + .we0 = 16, + .ctl0 = 0x10, + .time = 0x94, + .misc = 0x54, + .set_4b = aspeed_smc_chip_set_4b, +}; + +static const struct aspeed_smc_info smc_2400_info = { + .maxsize = 64 * 1024 * 1024, + .nce = 1, + .maxwidth = 2, + .hastype = false, + .we0 = 0, + .ctl0 = 0x04, + .time = 0x14, + .misc = 0x10, + .set_4b = aspeed_smc_chip_set_4b_smc_2400, +}; + +static const struct aspeed_smc_info fmc_2500_info = { + .maxsize = 256 * 1024 * 1024, + .nce = 3, + .maxwidth = 2, + .hastype = true, + .we0 = 16, + .ctl0 = 0x10, + .time = 0x94, + .misc = 0x54, + .set_4b = aspeed_smc_chip_set_4b, +}; + +static const struct aspeed_smc_info smc_2500_info = { + .maxsize = 128 * 1024 * 1024, + .nce = 2, + .maxwidth = 2, + .hastype = false, + .we0 = 16, + .ctl0 = 0x10, + .time = 0x94, + .misc = 0x54, + .set_4b = aspeed_smc_chip_set_4b, +}; + +enum smc_ctl_reg_value { + smc_base, /* base value without mode for other commands */ + smc_read, /* command reg for (maybe fast) reads */ + smc_write, /* command reg for writes with timings */ + smc_num_ctl_reg_values /* last value to get count of commands */ +}; + +struct aspeed_smc_controller; + +struct aspeed_smc_chip { + int cs; + struct aspeed_smc_controller *controller; + __le32 __iomem *ctl; /* control register */ + void __iomem *base; /* base of chip window */ + __le32 ctl_val[smc_num_ctl_reg_values]; /* controls with timing */ + enum smc_flash_type type; /* what type of flash */ + struct spi_nor nor; +}; + +struct aspeed_smc_controller { + struct device *dev; + + struct mutex mutex; /* controller access mutex */ + const struct aspeed_smc_info *info; /* type info of controller */ + void __iomem *regs; /* controller registers */ + void __iomem *windows; /* per-chip windows resource */ + + struct aspeed_smc_chip *chips[0]; /* pointers to attached chips */ +}; + +/* + * SPI Flash Configuration Register (AST2400 SPI) + */ +#define CONFIG_REG 0x0 +#define CONFIG_ENABLE_CE_INACTIVE BIT(1) +#define CONFIG_WRITE BIT(0) + +/* + * SPI Flash Configuration Register (AST2500 SPI) + * Type setting Register (AST2500 FMC and AST2400 FMC) + */ +#define TYPE_SETTING_REG 0x0 +#define CONFIG_DISABLE_LEGACY BIT(31) /* 1 on AST2500 FMC */ + +#define CONFIG_CE2_WRITE BIT(18) +#define CONFIG_CE1_WRITE BIT(17) +#define CONFIG_CE0_WRITE BIT(16) + +#define CONFIG_CE2_TYPE BIT(4) /* FMC only */ +#define CONFIG_CE1_TYPE BIT(2) /* FMC only */ +#define CONFIG_CE0_TYPE BIT(0) /* FMC only */ + +/* + * CE Control Register (AST2500 SPI,FMC and AST2400 FMC) + */ +#define CE_CONTROL_REG 0x4 +#define CE2_ENABLE_CE_INACTIVE BIT(10) +#define CE1_ENABLE_CE_INACTIVE BIT(9) +#define CE0_ENABLE_CE_INACTIVE BIT(8) +#define CE2_CONTROL_EXTENDED BIT(2) +#define CE1_CONTROL_EXTENDED BIT(1) +#define CE0_CONTROL_EXTENDED BIT(0) + +/* CE0 Control Register (depends on the controller type) */ +#define CONTROL_SPI_AAF_MODE BIT(31) +#define CONTROL_SPI_IO_MODE_MASK GENMASK(30, 28) +#define CONTROL_SPI_IO_DUAL_DATA BIT(29) +#define CONTROL_SPI_IO_DUAL_ADDR_DATA (BIT(29) | BIT(28)) +#define CONTROL_SPI_IO_QUAD_DATA BIT(30) +#define CONTROL_SPI_IO_QUAD_ADDR_DATA (BIT(30) | BIT(28)) +#define CONTROL_SPI_CE_INACTIVE_SHIFT 24 +#define CONTROL_SPI_CE_INACTIVE_MASK GENMASK(27, CONTROL_SPI_CE_INACTIVE_SHIFT) +/* 0 = 16T ... 15 = 1T T=HCLK */ +#define CONTROL_SPI_COMMAND_SHIFT 16 +#define CONTROL_SPI_DUMMY_CYCLE_COMMAND_OUTPUT BIT(15) +#define CONTROL_SPI_IO_DUMMY_CYCLES_HI BIT(14) +#define CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT 14 +#define CONTROL_SPI_IO_ADDRESS_4B BIT(13) /* AST2400 SPI */ +#define CONTROL_SPI_CLK_DIV4 BIT(13) /* others */ +#define CONTROL_SPI_RW_MERGE BIT(12) +#define CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT 6 +#define CONTROL_SPI_IO_DUMMY_CYCLES_LO GENMASK(7, \ + CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT) +#define CONTROL_SPI_IO_DUMMY_CYCLES_MASK (CONTROL_SPI_IO_DUMMY_CYCLES_HI | \ + CONTROL_SPI_IO_DUMMY_CYCLES_LO) +#define CONTROL_SPI_IO_DUMMY_CYCLES_SET(dummy) \ + (((((dummy) >> 2) & 0x1) << CONTROL_SPI_IO_DUMMY_CYCLES_HI_SHIFT) | \ + (((dummy) & 0x3) << CONTROL_SPI_IO_DUMMY_CYCLES_LO_SHIFT)) + +#define CONTROL_SPI_CLOCK_FREQ_SEL_SHIFT 8 +#define CONTROL_SPI_CLOCK_FREQ_SEL_MASK GENMASK(11, \ + CONTROL_SPI_CLOCK_FREQ_SEL_SHIFT) +#define CONTROL_SPI_LSB_FIRST BIT(5) +#define CONTROL_SPI_CLOCK_MODE_3 BIT(4) +#define CONTROL_SPI_IN_DUAL_DATA BIT(3) +#define CONTROL_SPI_CE_STOP_ACTIVE_CONTROL BIT(2) +#define CONTROL_SPI_COMMAND_MODE_MASK GENMASK(1, 0) +#define CONTROL_SPI_COMMAND_MODE_NORMAL (0) +#define CONTROL_SPI_COMMAND_MODE_FREAD (1) +#define CONTROL_SPI_COMMAND_MODE_WRITE (2) +#define CONTROL_SPI_COMMAND_MODE_USER (3) + +#define CONTROL_SPI_KEEP_MASK (CONTROL_SPI_AAF_MODE | \ + CONTROL_SPI_CE_INACTIVE_MASK | CONTROL_SPI_CLK_DIV4 | \ + CONTROL_SPI_IO_DUMMY_CYCLES_MASK | CONTROL_SPI_CLOCK_FREQ_SEL_MASK | \ + CONTROL_SPI_LSB_FIRST | CONTROL_SPI_CLOCK_MODE_3) + +/* Segment Address Registers */ +#define SEGMENT_ADDR_REG0 0x30 +#define SEGMENT_ADDR_START(_r) ((((_r) >> 16) & 0xFF) << 23) +#define SEGMENT_ADDR_END(_r) ((((_r) >> 24) & 0xFF) << 23) + +static u32 spi_control_fill_opcode(u8 opcode) +{ + return ((u32)(opcode)) << CONTROL_SPI_COMMAND_SHIFT; +} + +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip) +{ + return ((u32)1 << (chip->controller->info->we0 + chip->cs)); +} + +static void aspeed_smc_chip_check_config(struct aspeed_smc_chip *chip) +{ + struct aspeed_smc_controller *controller = chip->controller; + u32 reg; + + reg = readl(controller->regs + CONFIG_REG); + + if (!(reg & aspeed_smc_chip_write_bit(chip))) { + dev_dbg(controller->dev, + "config write is not set ! @%p: 0x%08x\n", + controller->regs + CONFIG_REG, reg); + reg |= aspeed_smc_chip_write_bit(chip); + writel(reg, controller->regs + CONFIG_REG); + } +} + +static void aspeed_smc_start_user(struct spi_nor *nor) +{ + struct aspeed_smc_chip *chip = nor->priv; + u32 ctl = chip->ctl_val[smc_base]; + + /* + * When the chip is controlled in user mode, we need write + * access to send the opcodes to it. So check the config. + */ + aspeed_smc_chip_check_config(chip); + + ctl |= CONTROL_SPI_COMMAND_MODE_USER | + CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; + writel(ctl, chip->ctl); + + ctl &= ~CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; + writel(ctl, chip->ctl); +} + +static void aspeed_smc_stop_user(struct spi_nor *nor) +{ + struct aspeed_smc_chip *chip = nor->priv; + + u32 ctl = chip->ctl_val[smc_read]; + u32 ctl2 = ctl | CONTROL_SPI_COMMAND_MODE_USER | + CONTROL_SPI_CE_STOP_ACTIVE_CONTROL; + + writel(ctl2, chip->ctl); /* stop user CE control */ + writel(ctl, chip->ctl); /* default to fread or read */ +} + +static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len) +{ + struct aspeed_smc_chip *chip = nor->priv; + + mutex_lock(&chip->controller->mutex); + + aspeed_smc_start_user(nor); + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); + aspeed_smc_read_from_ahb(buf, chip->base, len); + aspeed_smc_stop_user(nor); + + mutex_unlock(&chip->controller->mutex); + + return 0; +} + +static int aspeed_smc_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, + int len) +{ + struct aspeed_smc_chip *chip = nor->priv; + + mutex_lock(&chip->controller->mutex); + + aspeed_smc_start_user(nor); + aspeed_smc_write_to_ahb(chip->base, &opcode, 1); + aspeed_smc_write_to_ahb(chip->base, buf, len); + aspeed_smc_stop_user(nor); + + mutex_unlock(&chip->controller->mutex); + + return 0; +} + +static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr) +{ + struct aspeed_smc_chip *chip = nor->priv; + __be32 temp; + u32 cmdaddr; + + switch (nor->addr_width) { + default: + WARN_ONCE(1, "Unexpected address width %u, defaulting to 3\n", + nor->addr_width); + /* FALLTHROUGH */ + case 3: + cmdaddr = addr & 0xFFFFFF; + + cmdaddr |= (u32)cmd << 24; + + temp = cpu_to_be32(cmdaddr); + aspeed_smc_write_to_ahb(chip->base, &temp, 4); + break; + case 4: + temp = cpu_to_be32(addr); + aspeed_smc_write_to_ahb(chip->base, &cmd, 1); + aspeed_smc_write_to_ahb(chip->base, &temp, 4); + break; + } +} + +static ssize_t aspeed_smc_read_user(struct spi_nor *nor, loff_t from, + size_t len, u_char *read_buf) +{ + struct aspeed_smc_chip *chip = nor->priv; + + mutex_lock(&chip->controller->mutex); + + aspeed_smc_start_user(nor); + aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from); + aspeed_smc_read_from_ahb(read_buf, chip->base, len); + aspeed_smc_stop_user(nor); + + mutex_unlock(&chip->controller->mutex); + + return len; +} + +static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to, size_t len, + const u_char *write_buf) +{ + struct aspeed_smc_chip *chip = nor->priv; + + mutex_lock(&chip->controller->mutex); + + aspeed_smc_start_user(nor); + aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to); + aspeed_smc_write_to_ahb(chip->base, write_buf, len); + aspeed_smc_stop_user(nor); + + mutex_unlock(&chip->controller->mutex); + + return len; +} + +static int aspeed_smc_remove(struct platform_device *dev) +{ + struct aspeed_smc_chip *chip; + struct aspeed_smc_controller *controller = platform_get_drvdata(dev); + int n; + + for (n = 0; n < controller->info->nce; n++) { + chip = controller->chips[n]; + if (chip) + mtd_device_unregister(&chip->nor.mtd); + } + + return 0; +} + +static const struct of_device_id aspeed_smc_matches[] = { + { .compatible = "aspeed,ast2400-fmc", .data = &fmc_2400_info }, + { .compatible = "aspeed,ast2400-smc", .data = &smc_2400_info }, + { .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info }, + { .compatible = "aspeed,ast2500-smc", .data = &smc_2500_info }, + { } +}; +MODULE_DEVICE_TABLE(of, aspeed_smc_matches); + +static struct platform_device * +of_platform_device_create_or_find(struct device_node *child, + struct device *parent) +{ + struct platform_device *cdev; + + cdev = of_platform_device_create(child, NULL, parent); + if (!cdev) + cdev = of_find_device_by_node(child); + return cdev; +} + +static void __iomem *window_start(struct aspeed_smc_controller *controller, + struct resource *r, unsigned int n) +{ + u32 offset = 0; + u32 reg; + + if (controller->info->nce > 1) { + reg = readl(controller->regs + SEGMENT_ADDR_REG0 + n * 4); + + if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg)) + return NULL; + + offset = SEGMENT_ADDR_START(reg) - r->start; + } + + return controller->windows + offset; +} + +static void aspeed_smc_chip_enable_write(struct aspeed_smc_chip *chip) +{ + struct aspeed_smc_controller *controller = chip->controller; + u32 reg; + + reg = readl(controller->regs + CONFIG_REG); + + reg |= aspeed_smc_chip_write_bit(chip); + writel(reg, controller->regs + CONFIG_REG); +} + +static void aspeed_smc_chip_set_type(struct aspeed_smc_chip *chip, int type) +{ + struct aspeed_smc_controller *controller = chip->controller; + u32 reg; + + reg = readl(controller->regs + CONFIG_REG); + + chip->type = type; + + reg &= ~(3 << (chip->cs * 2)); + reg |= chip->type << (chip->cs * 2); + writel(reg, controller->regs + CONFIG_REG); +} + +/* + * The AST2500 FMC and AST2400 FMC flash controllers should be + * strapped by hardware, or autodetected, but the AST2500 SPI flash + * needs to be set. + */ +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip) +{ + struct aspeed_smc_controller *controller = chip->controller; + u32 reg; + + if (chip->controller->info == &smc_2500_info) { + reg = readl(controller->regs + CE_CONTROL_REG); + reg |= 1 << chip->cs; + writel(reg, controller->regs + CE_CONTROL_REG); + } +} + +/* + * The AST2400 SPI flash controller does not have a CE Control + * register. It uses the CE0 control register to set 4Byte mode at the + * controller level. + */ +static void aspeed_smc_chip_set_4b_smc_2400(struct aspeed_smc_chip *chip) +{ + chip->ctl_val[smc_base] |= CONTROL_SPI_IO_ADDRESS_4B; + chip->ctl_val[smc_read] |= CONTROL_SPI_IO_ADDRESS_4B; +} + +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip, + struct resource *r) +{ + struct aspeed_smc_controller *controller = chip->controller; + const struct aspeed_smc_info *info = controller->info; + u32 reg, base_reg; + + /* + * Always turn on the write enable bit to allow opcodes to be + * sent in user mode. + */ + aspeed_smc_chip_enable_write(chip); + + /* The driver only supports SPI type flash for the moment */ + if (info->hastype) + aspeed_smc_chip_set_type(chip, smc_type_spi); + + /* + * Configure chip base address in memory + */ + chip->base = window_start(controller, r, chip->cs); + if (!chip->base) { + dev_warn(chip->nor.dev, "CE segment window closed.\n"); + return -1; + } + + /* + * Read the existing control register to get basic values. + * + * XXX This register probably needs more sanitation. + * + * Do we need support for mode 3 vs mode 0 clock phasing? + */ + reg = readl(chip->ctl); + dev_dbg(controller->dev, "control register: %08x\n", reg); + + base_reg = reg & CONTROL_SPI_KEEP_MASK; + if (base_reg != reg) { + dev_info(controller->dev, + "control register changed to: %08x\n", + base_reg); + } + chip->ctl_val[smc_base] = base_reg; + + /* + * Retain the prior value of the control register as the + * default if it was normal access mode. Otherwise start with + * the sanitized base value set to read mode. + */ + if ((reg & CONTROL_SPI_COMMAND_MODE_MASK) == + CONTROL_SPI_COMMAND_MODE_NORMAL) + chip->ctl_val[smc_read] = reg; + else + chip->ctl_val[smc_read] = chip->ctl_val[smc_base] | + CONTROL_SPI_COMMAND_MODE_NORMAL; + + dev_dbg(controller->dev, "default control register: %08x\n", + chip->ctl_val[smc_read]); + return 0; +} + +static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip) +{ + struct aspeed_smc_controller *controller = chip->controller; + const struct aspeed_smc_info *info = controller->info; + u32 cmd; + + if (chip->nor.addr_width == 4 && info->set_4b) + info->set_4b(chip); + + /* + * base mode has not been optimized yet. use it for writes. + */ + chip->ctl_val[smc_write] = chip->ctl_val[smc_base] | + spi_control_fill_opcode(chip->nor.program_opcode) | + CONTROL_SPI_COMMAND_MODE_WRITE; + + dev_dbg(controller->dev, "write control register: %08x\n", + chip->ctl_val[smc_write]); + + /* + * XXX TODO + * Adjust clocks if fast read and write are supported. + * Interpret spi-nor flags to adjust controller settings. + * Check if resource size big enough for detected chip and + * add support assisted (normal or fast-) read and dma. + */ + switch (chip->nor.flash_read) { + case SPI_NOR_NORMAL: + cmd = CONTROL_SPI_COMMAND_MODE_NORMAL; + break; + case SPI_NOR_FAST: + cmd = CONTROL_SPI_COMMAND_MODE_FREAD; + break; + default: + dev_err(chip->nor.dev, "unsupported SPI read mode\n"); + return -EINVAL; + } + + chip->ctl_val[smc_read] |= cmd | + CONTROL_SPI_IO_DUMMY_CYCLES_SET(chip->nor.read_dummy / 8); + + dev_dbg(controller->dev, "base control register: %08x\n", + chip->ctl_val[smc_read]); + return 0; +} + +static int aspeed_smc_probe(struct platform_device *pdev) +{ + struct aspeed_smc_controller *controller; + const struct of_device_id *match; + const struct aspeed_smc_info *info; + struct resource *r; + struct device_node *child; + int err = 0; + unsigned int n; + + match = of_match_device(aspeed_smc_matches, &pdev->dev); + if (!match || !match->data) + return -ENODEV; + info = match->data; + + controller = devm_kzalloc(&pdev->dev, sizeof(*controller) + + info->nce * sizeof(controller->chips[0]), GFP_KERNEL); + if (!controller) + return -ENOMEM; + controller->info = info; + + mutex_init(&controller->mutex); + platform_set_drvdata(pdev, controller); + + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + controller->regs = devm_ioremap_resource(&pdev->dev, r); + if (IS_ERR(controller->regs)) + return PTR_ERR(controller->regs); + + r = platform_get_resource(pdev, IORESOURCE_MEM, 1); + controller->windows = devm_ioremap_resource(&pdev->dev, r); + if (IS_ERR(controller->windows)) + return PTR_ERR(controller->windows); + + controller->dev = &pdev->dev; + + /* The pinmux or bootloader will disable the legacy mode controller */ + + /* + * XXX Need to add arbitration to the SMC (BIOS) controller if access + * is shared by the host. + */ + for_each_available_child_of_node(controller->dev->of_node, child) { + struct platform_device *cdev; + struct aspeed_smc_chip *chip; + + /* This version does not support nand or nor flash devices. */ + if (!of_device_is_compatible(child, "jedec,spi-nor")) + continue; + + /* + * create a platform device from the of node. If the device + * already was created (eg from a prior bind/unbind cycle) + * reuse it. + * + * The creating the device node for the child here allows its + * use for error reporting via dev_err below. + */ + cdev = of_platform_device_create_or_find(child, + controller->dev); + if (!cdev) + continue; + + err = of_property_read_u32(child, "reg", &n); + if (err == -EINVAL && info->nce == 1) + n = 0; + else if (err || n >= info->nce) + continue; + if (controller->chips[n]) { + dev_err(&cdev->dev, + "chip-id %u already in use in use by %s\n", + n, dev_name(controller->chips[n]->nor.dev)); + continue; + } + + chip = devm_kzalloc(controller->dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + continue; + chip->controller = controller; + chip->ctl = controller->regs + info->ctl0 + n * 4; + chip->cs = n; + + chip->nor.dev = &cdev->dev; + chip->nor.priv = chip; + spi_nor_set_flash_node(&chip->nor, child); + chip->nor.mtd.name = of_get_property(child, "label", NULL); + chip->nor.read = aspeed_smc_read_user; + chip->nor.write = aspeed_smc_write_user; + chip->nor.read_reg = aspeed_smc_read_reg; + chip->nor.write_reg = aspeed_smc_write_reg; + + err = aspeed_smc_chip_setup_init(chip, r); + if (err) + continue; + + /* + * XXX Add support for SPI_NOR_QUAD and SPI_NOR_DUAL attach + * when board support is present as determined by of property. + */ + err = spi_nor_scan(&chip->nor, NULL, SPI_NOR_NORMAL); + if (err) + continue; + + err = aspeed_smc_chip_setup_finish(chip); + if (err) + continue; + + err = mtd_device_register(&chip->nor.mtd, NULL, 0); + if (err) + continue; + controller->chips[n] = chip; + } + + /* Were any children registered? */ + for (n = 0; n < info->nce; n++) + if (controller->chips[n]) + break; + + if (n == info->nce) + return -ENODEV; + + return 0; +} + +static struct platform_driver aspeed_smc_driver = { + .probe = aspeed_smc_probe, + .remove = aspeed_smc_remove, + .driver = { + .name = DEVICE_NAME, + .of_match_table = aspeed_smc_matches, + } +}; + +module_platform_driver(aspeed_smc_driver); + +MODULE_DESCRIPTION("ASPEED Static Memory Controller Driver"); +MODULE_AUTHOR("Milton Miller"); +MODULE_LICENSE("GPL v2");
This driver adds mtd support for spi-nor attached to either or both of the Firmware Memory Controller or the SPI Flash Controller (AST2400 only). The SMC controllers on the Aspeed AST2500 SoC are very similar to the ones found on the AST2400. The differences are on the number of supported flash modules and their default mappings in the SoC address space. The Aspeed AST2500 has one SPI controller for the BMC firmware and two for the host firmware. All controllers have now the same set of registers compatible with the AST2400 FMC controller and the legacy 'SMC' controller is fully gone. Each controller has a memory range on which it maps its flash module slaves. Each slave is assigned a memory window for its mapping that can be changed at bootime with the Segment Address Register. Each SPI flash slave can then be accessed in two modes: Command and User. When in User mode, accesses to the memory segment of the slaves are translated in SPI transfers. When in Command mode, the HW generates the SPI commands automatically and the memory segment is accessed as if doing a MMIO. Currently, only the User mode is supported. Command mode needs a little more work to check that the memory window on the AHB bus fits the module size. Based on previous work from Milton D. Miller II <miltonm@us.ibm.com> Signed-off-by: Cédric Le Goater <clg@kaod.org> --- Tested on: * OpenPOWER Palmetto (AST2400) with FMC controller : n25q256a SPI controller : mx25l25635e and n25q512ax3 * Evaluation board (AST2500) with FMC controller : w25q256 SPI controller : w25q256 * OpenPOWER Witherspoon (AST2500) with FMC controller : mx25l25635e * 2 SPI controller : mx66l1g45g Changes since v2: - added a set4b ops to handle difference in the controllers - simplified the IO routines - prepared for fast read using dummy cycles Work in progress: - read optimization using higher SPI clock frequencies - command mode to direct reads from AHB - DMA support .../devicetree/bindings/mtd/aspeed-smc.txt | 72 ++ drivers/mtd/spi-nor/Kconfig | 12 + drivers/mtd/spi-nor/Makefile | 1 + drivers/mtd/spi-nor/aspeed-smc.c | 783 +++++++++++++++++++++ 4 files changed, 868 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c