[PATCH v3 0/5] Static memory controllers for the Aspeed SoC

[PATCH v3 0/5] Static memory controllers for the Aspeed SoC

[Cédric Le Goater]

Hello,

Here is a serie introducing a new driver for the different memory
controllers of the Aspeed AST2500 and AST2400 SoCs. Each SoC has at
least a memory controller for the BMC firmware and another one for the
host firmware. The register set are mostly comptatible but there are
some slight differences on the AST2400.

The driver only support SPI type flash.

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:
 - splitted patch to distinguish AST2400 and AST2500 controllers
 - fixed controller names
 - introduced prepare/unprepare ops
 - introduced a aspeed_smc_setup_flash() routine
 - various cleanups

Changes since v1:
 - 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

Thanks,

C.

Cédric Le Goater (5):
  mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC
  mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC
  mtd: aspeed: used a label property
  mtd: spi-nor: bindings for the Aspeed memory controllers
  mtd: spi-nor: add a label property to jedec,spi-nor

 .../devicetree/bindings/mtd/aspeed-smc.txt         |  51 ++
 .../devicetree/bindings/mtd/jedec,spi-nor.txt      |   2 +
 drivers/mtd/spi-nor/Kconfig                        |  10 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/aspeed-smc.c                   | 770 +++++++++++++++++++++
 5 files changed, 834 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt
 create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c

-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Cédric Le Goater]

This driver adds mtd support for the Aspeed AST2500 SoC static memory
controllers :

 * Firmware SPI Memory Controller (FMC)
   . BMC firmware
   . 3 chip select pins (CE0 ~ CE2)
   . supports SPI type flash memory (CE0-CE1)
   . CE2 can be of NOR type flash but this is not supported by the
     driver

 * SPI Flash Controller (SPI1 and SPI2)
   . host firmware
   . 2 chip select pins (CE0 ~ CE1)
   . supports SPI type flash memory

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>
---
 drivers/mtd/spi-nor/Kconfig      |  10 +
 drivers/mtd/spi-nor/Makefile     |   1 +
 drivers/mtd/spi-nor/aspeed-smc.c | 735 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 746 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 4a682ee0f632..5c0efbd9dd89 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -29,6 +29,16 @@ config MTD_SPI_NOR_USE_4K_SECTORS
 	  Please note that some tools/drivers/filesystems may not work with
 	  4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
 
+config SPI_ASPEED
+	tristate "Aspeed flash controllers in SPI mode"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	depends on HAS_IOMEM && OF
+	help
+	  This enables support for the Firmware Memory controller (FMC)
+	  in the Aspeed AST2500 SoC when attached to SPI NOR chips,
+	  and support for the SPI flash memory controller (SPI) for
+	  the host firmware. The implementation only supports SPI NOR.
+
 config SPI_ATMEL_QUADSPI
 	tristate "Atmel Quad SPI Controller"
 	depends on ARCH_AT91 || (ARM && COMPILE_TEST)
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 121695e83542..d73d772c689c 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
+obj-$(CONFIG_SPI_ASPEED)	+= aspeed-smc.o
 obj-$(CONFIG_SPI_ATMEL_QUADSPI)	+= atmel-quadspi.o
 obj-$(CONFIG_SPI_CADENCE_QUADSPI)	+= cadence-quadspi.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.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..6f9244f07aef
--- /dev/null
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -0,0 +1,735 @@
+/*
+ * 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 (IS_ALIGNED((u32)src, sizeof(u32)) &&
+	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
+	    IS_ALIGNED(len, sizeof(u32))) {
+		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 (IS_ALIGNED((u32)dst, sizeof(u32)) &&
+	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
+	    IS_ALIGNED(len, sizeof(u32))) {
+		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;
+}
+
+/*
+ * The driver only support SPI flash
+ */
+enum aspeed_smc_flash_type {
+	smc_type_nor  = 0,
+	smc_type_nand = 1,
+	smc_type_spi  = 2,
+};
+
+struct aspeed_smc_chip;
+
+struct aspeed_smc_info {
+	u32 maxsize;		/* maximum size of chip window */
+	u8 nce;			/* number of chip enables */
+	bool hastype;		/* flash type field exists in config reg */
+	u8 we0;			/* shift for write enable bit for CE0 */
+	u8 ctl0;		/* offset in regs of ctl for CE0 */
+
+	void (*set_4b)(struct aspeed_smc_chip *chip);
+};
+
+static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
+
+static const struct aspeed_smc_info fmc_2500_info = {
+	.maxsize = 256 * 1024 * 1024,
+	.nce = 3,
+	.hastype = true,
+	.we0 = 16,
+	.ctl0 = 0x10,
+	.set_4b = aspeed_smc_chip_set_4b,
+};
+
+static const struct aspeed_smc_info spi_2500_info = {
+	.maxsize = 128 * 1024 * 1024,
+	.nce = 2,
+	.hastype = false,
+	.we0 = 16,
+	.ctl0 = 0x10,
+	.set_4b = aspeed_smc_chip_set_4b,
+};
+
+enum aspeed_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 */
+	smc_max,
+};
+
+struct aspeed_smc_controller;
+
+struct aspeed_smc_chip {
+	int cs;
+	struct aspeed_smc_controller *controller;
+	void __iomem *ctl;			/* control register */
+	void __iomem *ahb_base;			/* base of chip window */
+	u32 ctl_val[smc_max];			/* control settings */
+	enum aspeed_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 *ahb_base;			/* per-chip windows resource */
+
+	struct aspeed_smc_chip *chips[0];	/* pointers to attached chips */
+};
+
+/*
+ * SPI Flash Configuration Register (AST2500 SPI)
+ *     or
+ * Type setting Register (AST2500 FMC).
+ * CE0 and CE1 can only be of type SPI. CE2 can be of type NOR but the
+ * driver does not support it.
+ */
+#define CONFIG_REG			0x0
+#define CONFIG_DISABLE_LEGACY		BIT(31) /* 1 */
+
+#define CONFIG_CE2_WRITE		BIT(18)
+#define CONFIG_CE1_WRITE		BIT(17)
+#define CONFIG_CE0_WRITE		BIT(16)
+
+#define CONFIG_CE2_TYPE			BIT(4) /* AST2500 FMC only */
+#define CONFIG_CE1_TYPE			BIT(2) /* AST2500 FMC only */
+#define CONFIG_CE0_TYPE			BIT(0) /* AST2500 FMC only */
+
+/*
+ * CE Control Register
+ */
+#define CE_CONTROL_REG			0x4
+
+/*
+ * CEx Control Register
+ */
+#define CONTROL_AAF_MODE		BIT(31)
+#define CONTROL_IO_MODE_MASK		GENMASK(30, 28)
+#define CONTROL_IO_DUAL_DATA		BIT(29)
+#define CONTROL_IO_DUAL_ADDR_DATA	(BIT(29) | BIT(28))
+#define CONTROL_IO_QUAD_DATA		BIT(30)
+#define CONTROL_IO_QUAD_ADDR_DATA	(BIT(30) | BIT(28))
+#define CONTROL_CE_INACTIVE_SHIFT	24
+#define CONTROL_CE_INACTIVE_MASK	GENMASK(27, \
+					CONTROL_CE_INACTIVE_SHIFT)
+/* 0 = 16T ... 15 = 1T   T=HCLK */
+#define CONTROL_COMMAND_SHIFT		16
+#define CONTROL_DUMMY_COMMAND_OUT	BIT(15)
+#define CONTROL_IO_DUMMY_HI		BIT(14)
+#define CONTROL_IO_DUMMY_HI_SHIFT	14
+#define CONTROL_CLK_DIV4		BIT(13) /* others */
+#define CONTROL_RW_MERGE		BIT(12)
+#define CONTROL_IO_DUMMY_LO_SHIFT	6
+#define CONTROL_IO_DUMMY_LO		GENMASK(7, \
+						CONTROL_IO_DUMMY_LO_SHIFT)
+#define CONTROL_IO_DUMMY_MASK		(CONTROL_IO_DUMMY_HI | \
+					 CONTROL_IO_DUMMY_LO)
+#define CONTROL_IO_DUMMY_SET(dummy)				 \
+	(((((dummy) >> 2) & 0x1) << CONTROL_IO_DUMMY_HI_SHIFT) | \
+	 (((dummy) & 0x3) << CONTROL_IO_DUMMY_LO_SHIFT))
+
+#define CONTROL_CLOCK_FREQ_SEL_SHIFT	8
+#define CONTROL_CLOCK_FREQ_SEL_MASK	GENMASK(11, \
+						CONTROL_CLOCK_FREQ_SEL_SHIFT)
+#define CONTROL_LSB_FIRST		BIT(5)
+#define CONTROL_CLOCK_MODE_3		BIT(4)
+#define CONTROL_IN_DUAL_DATA		BIT(3)
+#define CONTROL_CE_STOP_ACTIVE_CONTROL	BIT(2)
+#define CONTROL_COMMAND_MODE_MASK	GENMASK(1, 0)
+#define CONTROL_COMMAND_MODE_NORMAL	(0)
+#define CONTROL_COMMAND_MODE_FREAD	(1)
+#define CONTROL_COMMAND_MODE_WRITE	(2)
+#define CONTROL_COMMAND_MODE_USER	(3)
+
+#define CONTROL_KEEP_MASK						\
+	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
+	 CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK |		\
+	 CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
+
+/*
+ * Segment Address Registers. Start and end addresses are encoded
+ * using 8MB units
+ */
+#define SEGMENT_ADDR_REG0		0x30
+#define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)
+#define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)
+
+static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip)
+{
+	return BIT(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))
+		return;
+
+	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_COMMAND_MODE_USER |
+		CONTROL_CE_STOP_ACTIVE_CONTROL;
+	writel(ctl, chip->ctl);
+
+	ctl &= ~CONTROL_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_COMMAND_MODE_USER |
+		CONTROL_CE_STOP_ACTIVE_CONTROL;
+
+	writel(ctl2, chip->ctl);	/* stop user CE control */
+	writel(ctl, chip->ctl);		/* default to fread or read mode */
+}
+
+static int aspeed_smc_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct aspeed_smc_chip *chip = nor->priv;
+
+	mutex_lock(&chip->controller->mutex);
+	return 0;
+}
+
+static void aspeed_smc_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct aspeed_smc_chip *chip = nor->priv;
+
+	mutex_unlock(&chip->controller->mutex);
+}
+
+static int aspeed_smc_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
+{
+	struct aspeed_smc_chip *chip = nor->priv;
+
+	aspeed_smc_start_user(nor);
+	aspeed_smc_write_to_ahb(chip->ahb_base, &opcode, 1);
+	aspeed_smc_read_from_ahb(buf, chip->ahb_base, len);
+	aspeed_smc_stop_user(nor);
+	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;
+
+	aspeed_smc_start_user(nor);
+	aspeed_smc_write_to_ahb(chip->ahb_base, &opcode, 1);
+	aspeed_smc_write_to_ahb(chip->ahb_base, buf, len);
+	aspeed_smc_stop_user(nor);
+	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 |= cmd << 24;
+
+		temp = cpu_to_be32(cmdaddr);
+		aspeed_smc_write_to_ahb(chip->ahb_base, &temp, 4);
+		break;
+	case 4:
+		temp = cpu_to_be32(addr);
+		aspeed_smc_write_to_ahb(chip->ahb_base, &cmd, 1);
+		aspeed_smc_write_to_ahb(chip->ahb_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;
+
+	aspeed_smc_start_user(nor);
+	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
+	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
+	aspeed_smc_stop_user(nor);
+	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;
+
+	aspeed_smc_start_user(nor);
+	aspeed_smc_send_cmd_addr(nor, nor->program_opcode, to);
+	aspeed_smc_write_to_ahb(chip->ahb_base, write_buf, len);
+	aspeed_smc_stop_user(nor);
+	return len;
+}
+
+static int aspeed_smc_unregister(struct aspeed_smc_controller *controller)
+{
+	struct aspeed_smc_chip *chip;
+	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 int aspeed_smc_remove(struct platform_device *dev)
+{
+	return aspeed_smc_unregister(platform_get_drvdata(dev));
+}
+
+static const struct of_device_id aspeed_smc_matches[] = {
+	{ .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info },
+	{ .compatible = "aspeed,ast2500-spi", .data = &spi_2500_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_smc_matches);
+
+/*
+ * Each chip has a mapping window defined by a segment address
+ * register defining a start and an end address on the AHB bus. These
+ * addresses can be configured to fit the chip size and offer a
+ * contiguous memory region across chips. For the moment, we only
+ * check that each chip segment is valid.
+ */
+static void __iomem *aspeed_smc_chip_base(struct aspeed_smc_chip *chip,
+					  struct resource *res)
+{
+	struct aspeed_smc_controller *controller = chip->controller;
+	u32 offset = 0;
+	u32 reg;
+
+	if (controller->info->nce > 1) {
+		reg = readl(controller->regs + SEGMENT_ADDR_REG0 +
+			    chip->cs * 4);
+
+		if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg))
+			return NULL;
+
+		offset = SEGMENT_ADDR_START(reg) - res->start;
+	}
+
+	return controller->ahb_base + 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;
+
+	chip->type = type;
+
+	reg = readl(controller->regs + CONFIG_REG);
+	reg &= ~(3 << (chip->cs * 2));
+	reg |= chip->type << (chip->cs * 2);
+	writel(reg, controller->regs + CONFIG_REG);
+}
+
+/*
+ * The AST2500 FMC flash controller 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 == &spi_2500_info) {
+		reg = readl(controller->regs + CE_CONTROL_REG);
+		reg |= 1 << chip->cs;
+		writel(reg, controller->regs + CE_CONTROL_REG);
+	}
+}
+
+static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
+				      struct resource *res)
+{
+	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 */
+	if (info->hastype)
+		aspeed_smc_chip_set_type(chip, smc_type_spi);
+
+	/*
+	 * Configure chip base address in memory
+	 */
+	chip->ahb_base = aspeed_smc_chip_base(chip, res);
+	if (!chip->ahb_base) {
+		dev_warn(chip->nor.dev, "CE segment window closed.\n");
+		return -1;
+	}
+
+	/*
+	 * Get value of the inherited control register. U-Boot usually
+	 * does some timing calibration on the FMC chip, so it's good
+	 * to keep them. In the future, we should handle calibration
+	 * from Linux.
+	 */
+	reg = readl(chip->ctl);
+	dev_dbg(controller->dev, "control register: %08x\n", reg);
+
+	base_reg = reg & CONTROL_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_COMMAND_MODE_MASK) ==
+	    CONTROL_COMMAND_MODE_NORMAL)
+		chip->ctl_val[smc_read] = reg;
+	else
+		chip->ctl_val[smc_read] = chip->ctl_val[smc_base] |
+			CONTROL_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] |
+		chip->nor.program_opcode << CONTROL_COMMAND_SHIFT |
+		CONTROL_COMMAND_MODE_WRITE;
+
+	dev_dbg(controller->dev, "write control register: %08x\n",
+		chip->ctl_val[smc_write]);
+
+	/*
+	 * TODO: Adjust clocks if fast read is supported and interpret
+	 * SPI-NOR flags to adjust controller settings.
+	 */
+	switch (chip->nor.flash_read) {
+	case SPI_NOR_NORMAL:
+		cmd = CONTROL_COMMAND_MODE_NORMAL;
+		break;
+	case SPI_NOR_FAST:
+		cmd = CONTROL_COMMAND_MODE_FREAD;
+		break;
+	default:
+		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
+		return -EINVAL;
+	}
+
+	chip->ctl_val[smc_read] |= cmd |
+		CONTROL_IO_DUMMY_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_setup_flash(struct aspeed_smc_controller *controller,
+				  struct device_node *np, struct resource *r)
+{
+	const struct aspeed_smc_info *info = controller->info;
+	struct device *dev = controller->dev;
+	struct device_node *child;
+	unsigned int cs;
+	int ret = -ENODEV;
+
+	for_each_available_child_of_node(np, child) {
+		struct aspeed_smc_chip *chip;
+		struct spi_nor *nor;
+		struct mtd_info *mtd;
+
+		/* This driver does not support NAND or NOR flash devices. */
+		if (!of_device_is_compatible(child, "jedec,spi-nor"))
+			continue;
+
+		ret = of_property_read_u32(child, "reg", &cs);
+		if (ret) {
+			dev_err(dev, "Couldn't not read chip select.\n");
+			break;
+		}
+
+		if (cs >= info->nce) {
+			dev_err(dev, "Chip select %d out of range.\n",
+				cs);
+			ret = -ERANGE;
+			break;
+		}
+
+		if (controller->chips[cs]) {
+			dev_err(dev, "Chip select %d already in use by %s\n",
+				cs, dev_name(controller->chips[cs]->nor.dev));
+			ret = -EBUSY;
+			break;
+		}
+
+		chip = devm_kzalloc(controller->dev, sizeof(*chip), GFP_KERNEL);
+		if (!chip) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		chip->controller = controller;
+		chip->ctl = controller->regs + info->ctl0 + cs * 4;
+		chip->cs = cs;
+
+		nor = &chip->nor;
+		mtd = &nor->mtd;
+
+		nor->dev = dev;
+		nor->priv = chip;
+		spi_nor_set_flash_node(nor, child);
+		nor->read = aspeed_smc_read_user;
+		nor->write = aspeed_smc_write_user;
+		nor->read_reg = aspeed_smc_read_reg;
+		nor->write_reg = aspeed_smc_write_reg;
+		nor->prepare = aspeed_smc_prep;
+		nor->unprepare = aspeed_smc_unprep;
+
+		ret = aspeed_smc_chip_setup_init(chip, r);
+		if (ret)
+			break;
+
+		/*
+		 * TODO: Add support for SPI_NOR_QUAD and SPI_NOR_DUAL
+		 * attach when board support is present as determined
+		 * by of property.
+		 */
+		ret = spi_nor_scan(nor, NULL, SPI_NOR_NORMAL);
+		if (ret)
+			break;
+
+		ret = aspeed_smc_chip_setup_finish(chip);
+		if (ret)
+			break;
+
+		ret = mtd_device_register(mtd, NULL, 0);
+		if (ret)
+			break;
+
+		controller->chips[cs] = chip;
+	}
+
+	if (ret)
+		aspeed_smc_unregister(controller);
+
+	return ret;
+}
+
+static int aspeed_smc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct aspeed_smc_controller *controller;
+	const struct of_device_id *match;
+	const struct aspeed_smc_info *info;
+	struct resource *res;
+	int ret;
+
+	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;
+	controller->dev = dev;
+
+	mutex_init(&controller->mutex);
+	platform_set_drvdata(pdev, controller);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	controller->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(controller->regs)) {
+		dev_err(dev, "Cannot remap controller address.\n");
+		return PTR_ERR(controller->regs);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	controller->ahb_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(controller->ahb_base)) {
+		dev_err(dev, "Cannot remap controller address.\n");
+		return PTR_ERR(controller->ahb_base);
+	}
+
+	ret = aspeed_smc_setup_flash(controller, np, res);
+	if (ret)
+		dev_err(dev, "Aspeed SMC probe failed %d\n", ret);
+
+	return ret;
+}
+
+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");
-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Marek Vasut]

On 12/09/2016 05:49 PM, Cédric Le Goater wrote:

[...]


> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
> +				    size_t len)
> +{
> +	if (IS_ALIGNED((u32)src, sizeof(u32)) &&
> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
> +	    IS_ALIGNED(len, sizeof(u32))) {

Did you try compiling this on any 64bit system ?

> +		while (len > 3) {
> +			*(u32 *)buf = readl(src);

ioread32_rep() to avoid open-coding the loop .

> +			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 (IS_ALIGNED((u32)dst, sizeof(u32)) &&
> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
> +	    IS_ALIGNED(len, sizeof(u32))) {
> +		while (len > 3) {
> +			u32 val = *(u32 *)buf;
> +
> +			writel(val, dst);

iowrite32_rep()

> +			buf += 4;
> +			dst += 4;
> +			len -= 4;
> +		}
> +	}
> +
> +	while (len--) {
> +		u8 val = *(u8 *)buf;
> +
> +		writeb(val, dst);
> +		buf += 1;
> +		dst += 1;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * The driver only support SPI flash
> + */
> +enum aspeed_smc_flash_type {
> +	smc_type_nor  = 0,
> +	smc_type_nand = 1,
> +	smc_type_spi  = 2,
> +};
> +
> +struct aspeed_smc_chip;
> +
> +struct aspeed_smc_info {
> +	u32 maxsize;		/* maximum size of chip window */
> +	u8 nce;			/* number of chip enables */
> +	bool hastype;		/* flash type field exists in config reg */
> +	u8 we0;			/* shift for write enable bit for CE0 */
> +	u8 ctl0;		/* offset in regs of ctl for CE0 */
> +
> +	void (*set_4b)(struct aspeed_smc_chip *chip);
> +};

Move all the structs to the beginning of the driver please.

> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
> +
> +static const struct aspeed_smc_info fmc_2500_info = {
> +	.maxsize = 256 * 1024 * 1024,
> +	.nce = 3,
> +	.hastype = true,
> +	.we0 = 16,
> +	.ctl0 = 0x10,
> +	.set_4b = aspeed_smc_chip_set_4b,
> +};
> +
> +static const struct aspeed_smc_info spi_2500_info = {
> +	.maxsize = 128 * 1024 * 1024,
> +	.nce = 2,
> +	.hastype = false,
> +	.we0 = 16,
> +	.ctl0 = 0x10,
> +	.set_4b = aspeed_smc_chip_set_4b,
> +};
> +
> +enum aspeed_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 */
> +	smc_max,
> +};

[...]

> +#define CONTROL_CE_STOP_ACTIVE_CONTROL	BIT(2)
> +#define CONTROL_COMMAND_MODE_MASK	GENMASK(1, 0)
> +#define CONTROL_COMMAND_MODE_NORMAL	(0)
> +#define CONTROL_COMMAND_MODE_FREAD	(1)
> +#define CONTROL_COMMAND_MODE_WRITE	(2)
> +#define CONTROL_COMMAND_MODE_USER	(3)

Drop the parenthesis around constants :)

> +#define CONTROL_KEEP_MASK						\
> +	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
> +	 CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK |		\
> +	 CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
> +
> +/*
> + * Segment Address Registers. Start and end addresses are encoded
> + * using 8MB units
> + */
> +#define SEGMENT_ADDR_REG0		0x30
> +#define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)

is that ((r) & 0xff0000) << 7 ?

> +#define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)

((r) & 0xff000000) >> 1 ?

> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip)
> +{
> +	return BIT(chip->controller->info->we0 + chip->cs);
> +}

[...]

> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
> +				      struct resource *res)
> +{
> +	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 */
> +	if (info->hastype)
> +		aspeed_smc_chip_set_type(chip, smc_type_spi);
> +
> +	/*
> +	 * Configure chip base address in memory
> +	 */
> +	chip->ahb_base = aspeed_smc_chip_base(chip, res);
> +	if (!chip->ahb_base) {
> +		dev_warn(chip->nor.dev, "CE segment window closed.\n");
> +		return -1;

return -EINVAL ? Check whether you always use errno.h macros in returns.

> +	}
> +
> +	/*
> +	 * Get value of the inherited control register. U-Boot usually
> +	 * does some timing calibration on the FMC chip, so it's good
> +	 * to keep them. In the future, we should handle calibration
> +	 * from Linux.
> +	 */
> +	reg = readl(chip->ctl);
> +	dev_dbg(controller->dev, "control register: %08x\n", reg);
> +
> +	base_reg = reg & CONTROL_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;

[...]

Mostly minor nits, looks nice otherwise, thanks :)

-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Cédric Le Goater]

Hello, 

On 12/10/2016 05:01 AM, Marek Vasut wrote:
> On 12/09/2016 05:49 PM, Cédric Le Goater wrote:
> 
> [...]
> 
> 
>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
>> +				    size_t len)
>> +{
>> +	if (IS_ALIGNED((u32)src, sizeof(u32)) &&
>> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
>> +	    IS_ALIGNED(len, sizeof(u32))) {
> 
> Did you try compiling this on any 64bit system ?

I cross compile the kernel on a 64bit host and then upload on the
target. What kind of problem are you forseeing ? 

> 
>> +		while (len > 3) {
>> +			*(u32 *)buf = readl(src);
> 
> ioread32_rep() to avoid open-coding the loop .

ok.

>> +			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 (IS_ALIGNED((u32)dst, sizeof(u32)) &&
>> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
>> +	    IS_ALIGNED(len, sizeof(u32))) {
>> +		while (len > 3) {
>> +			u32 val = *(u32 *)buf;
>> +
>> +			writel(val, dst);
> 
> iowrite32_rep()
> 
>> +			buf += 4;
>> +			dst += 4;
>> +			len -= 4;
>> +		}
>> +	}
>> +
>> +	while (len--) {
>> +		u8 val = *(u8 *)buf;
>> +
>> +		writeb(val, dst);
>> +		buf += 1;
>> +		dst += 1;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/*
>> + * The driver only support SPI flash
>> + */
>> +enum aspeed_smc_flash_type {
>> +	smc_type_nor  = 0,
>> +	smc_type_nand = 1,
>> +	smc_type_spi  = 2,
>> +};
>> +
>> +struct aspeed_smc_chip;
>> +
>> +struct aspeed_smc_info {
>> +	u32 maxsize;		/* maximum size of chip window */
>> +	u8 nce;			/* number of chip enables */
>> +	bool hastype;		/* flash type field exists in config reg */
>> +	u8 we0;			/* shift for write enable bit for CE0 */
>> +	u8 ctl0;		/* offset in regs of ctl for CE0 */
>> +
>> +	void (*set_4b)(struct aspeed_smc_chip *chip);
>> +};
> 
> Move all the structs to the beginning of the driver please.

ok.

> 
>> +static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
>> +
>> +static const struct aspeed_smc_info fmc_2500_info = {
>> +	.maxsize = 256 * 1024 * 1024,
>> +	.nce = 3,
>> +	.hastype = true,
>> +	.we0 = 16,
>> +	.ctl0 = 0x10,
>> +	.set_4b = aspeed_smc_chip_set_4b,
>> +};
>> +
>> +static const struct aspeed_smc_info spi_2500_info = {
>> +	.maxsize = 128 * 1024 * 1024,
>> +	.nce = 2,
>> +	.hastype = false,
>> +	.we0 = 16,
>> +	.ctl0 = 0x10,
>> +	.set_4b = aspeed_smc_chip_set_4b,
>> +};
>> +
>> +enum aspeed_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 */
>> +	smc_max,
>> +};
> 
> [...]
> 
>> +#define CONTROL_CE_STOP_ACTIVE_CONTROL	BIT(2)
>> +#define CONTROL_COMMAND_MODE_MASK	GENMASK(1, 0)
>> +#define CONTROL_COMMAND_MODE_NORMAL	(0)
>> +#define CONTROL_COMMAND_MODE_FREAD	(1)
>> +#define CONTROL_COMMAND_MODE_WRITE	(2)
>> +#define CONTROL_COMMAND_MODE_USER	(3)
> 
> Drop the parenthesis around constants :)

yeah

>> +#define CONTROL_KEEP_MASK						\
>> +	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
>> +	 CONTROL_IO_DUMMY_MASK | CONTROL_CLOCK_FREQ_SEL_MASK |		\
>> +	 CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
>> +
>> +/*
>> + * Segment Address Registers. Start and end addresses are encoded
>> + * using 8MB units
>> + */
>> +#define SEGMENT_ADDR_REG0		0x30
>> +#define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)
> 
> is that ((r) & 0xff0000) << 7 ?
> 
>> +#define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)
> 
> ((r) & 0xff000000) >> 1 ?

yes. 

I rather keep the initial macros though, which I found easier to 
understand.

The Segment Register uses a 8MB unit to encode the start address
and the end address of the mapping window of a flash SPI slave :

        | byte 1 | byte 2 | byte 3 | byte 4 |
        +--------+--------+--------+--------+
        |  end   |  start |   0    |   0    |



>> +static inline u32 aspeed_smc_chip_write_bit(struct aspeed_smc_chip *chip)
>> +{
>> +	return BIT(chip->controller->info->we0 + chip->cs);
>> +}
> 
> [...]
> 
>> +static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
>> +				      struct resource *res)
>> +{
>> +	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 */
>> +	if (info->hastype)
>> +		aspeed_smc_chip_set_type(chip, smc_type_spi);
>> +
>> +	/*
>> +	 * Configure chip base address in memory
>> +	 */
>> +	chip->ahb_base = aspeed_smc_chip_base(chip, res);
>> +	if (!chip->ahb_base) {
>> +		dev_warn(chip->nor.dev, "CE segment window closed.\n");
>> +		return -1;
> 
> return -EINVAL ? Check whether you always use errno.h macros in returns.

ah yes. I missed that one.

> 
>> +	}
>> +
>> +	/*
>> +	 * Get value of the inherited control register. U-Boot usually
>> +	 * does some timing calibration on the FMC chip, so it's good
>> +	 * to keep them. In the future, we should handle calibration
>> +	 * from Linux.
>> +	 */
>> +	reg = readl(chip->ctl);
>> +	dev_dbg(controller->dev, "control register: %08x\n", reg);
>> +
>> +	base_reg = reg & CONTROL_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;
> 
> [...]
> 
> Mostly minor nits, looks nice otherwise, thanks :)


Thanks for the review,

C.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Marek Vasut]

On 12/10/2016 06:34 PM, Cédric Le Goater wrote:
> Hello, 

Hi!

> On 12/10/2016 05:01 AM, Marek Vasut wrote:
>> On 12/09/2016 05:49 PM, Cédric Le Goater wrote:
>>
>> [...]
>>
>>
>>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
>>> +				    size_t len)
>>> +{
>>> +	if (IS_ALIGNED((u32)src, sizeof(u32)) &&
>>> +	    IS_ALIGNED((u32)buf, sizeof(u32)) &&
>>> +	    IS_ALIGNED(len, sizeof(u32))) {
>>
>> Did you try compiling this on any 64bit system ?
> 
> I cross compile the kernel on a 64bit host and then upload on the
> target. What kind of problem are you forseeing ? 

Something about the pointer being clipped to 4 bytes, I'd rather use
uintptr_t cast instead of u32 . I don't think it matters for this check,
but just to be extra precise ...

>>
>>> +		while (len > 3) {
>>> +			*(u32 *)buf = readl(src);


[...]

>>> +/*
>>> + * Segment Address Registers. Start and end addresses are encoded
>>> + * using 8MB units
>>> + */
>>> +#define SEGMENT_ADDR_REG0		0x30
>>> +#define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)
>>
>> is that ((r) & 0xff0000) << 7 ?
>>
>>> +#define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)
>>
>> ((r) & 0xff000000) >> 1 ?
> 
> yes. 
> 
> I rather keep the initial macros though, which I found easier to 
> understand.
> 
> The Segment Register uses a 8MB unit to encode the start address
> and the end address of the mapping window of a flash SPI slave :
> 
>         | byte 1 | byte 2 | byte 3 | byte 4 |
>         +--------+--------+--------+--------+
>         |  end   |  start |   0    |   0    |

Then the above should be in the comment , it's a good explanation :)

Thanks!

-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Joel Stanley]

On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
> This driver adds mtd support for the Aspeed AST2500 SoC static memory
> controllers :


> Based on previous work from Milton D. Miller II <miltonm-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
>
> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>

I had a read and it looks good. Once you have addressed Marek's
review, please add:

Reviewed-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>

> ---
>  drivers/mtd/spi-nor/Kconfig      |  10 +
>  drivers/mtd/spi-nor/Makefile     |   1 +
>  drivers/mtd/spi-nor/aspeed-smc.c | 735 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 746 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/aspeed-smc.c
>

> +
> +MODULE_DESCRIPTION("ASPEED Static Memory Controller Driver");
> +MODULE_AUTHOR("Milton Miller");

You can stick your name here instead, or drop the define.

> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Joel Stanley]

On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:

> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 4a682ee0f632..5c0efbd9dd89 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -29,6 +29,16 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>           Please note that some tools/drivers/filesystems may not work with
>           4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
>
> +config SPI_ASPEED
> +       tristate "Aspeed flash controllers in SPI mode"
> +       depends on ARCH_ASPEED || COMPILE_TEST
> +       depends on HAS_IOMEM && OF
> +       help
> +         This enables support for the Firmware Memory controller (FMC)
> +         in the Aspeed AST2500 SoC when attached to SPI NOR chips,
> +         and support for the SPI flash memory controller (SPI) for
> +         the host firmware. The implementation only supports SPI NOR.
> +
>  config SPI_ATMEL_QUADSPI
>         tristate "Atmel Quad SPI Controller"
>         depends on ARCH_AT91 || (ARM && COMPILE_TEST)
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 121695e83542..d73d772c689c 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_MTD_SPI_NOR)      += spi-nor.o
> +obj-$(CONFIG_SPI_ASPEED)       += aspeed-smc.o

Can we make the symbol CONFIG_SPI_ASPEED_SMC?

Brendan has proposed a generic SPI master driver for the Aspeed SoC
that would allow the controller to be used for non-flash devices. The
obvious symbol for that driver is CONFIG_SPI_ASPEED.

Cheers,

Joel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/5] mtd: spi-nor: add memory controllers for the Aspeed AST2500 SoC

[Cédric Le Goater]

On 12/12/2016 06:02 AM, Joel Stanley wrote:
> On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
> 
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 4a682ee0f632..5c0efbd9dd89 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -29,6 +29,16 @@ config MTD_SPI_NOR_USE_4K_SECTORS
>>           Please note that some tools/drivers/filesystems may not work with
>>           4096 B erase size (e.g. UBIFS requires 15 KiB as a minimum).
>>
>> +config SPI_ASPEED
>> +       tristate "Aspeed flash controllers in SPI mode"
>> +       depends on ARCH_ASPEED || COMPILE_TEST
>> +       depends on HAS_IOMEM && OF
>> +       help
>> +         This enables support for the Firmware Memory controller (FMC)
>> +         in the Aspeed AST2500 SoC when attached to SPI NOR chips,
>> +         and support for the SPI flash memory controller (SPI) for
>> +         the host firmware. The implementation only supports SPI NOR.
>> +
>>  config SPI_ATMEL_QUADSPI
>>         tristate "Atmel Quad SPI Controller"
>>         depends on ARCH_AT91 || (ARM && COMPILE_TEST)
>> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
>> index 121695e83542..d73d772c689c 100644
>> --- a/drivers/mtd/spi-nor/Makefile
>> +++ b/drivers/mtd/spi-nor/Makefile
>> @@ -1,4 +1,5 @@
>>  obj-$(CONFIG_MTD_SPI_NOR)      += spi-nor.o
>> +obj-$(CONFIG_SPI_ASPEED)       += aspeed-smc.o
> 
> Can we make the symbol CONFIG_SPI_ASPEED_SMC?

sure. I will change the config option name and leave room for a 
potential generic SPI driver.

Thanks,

C. 

> Brendan has proposed a generic SPI master driver for the Aspeed SoC
> that would allow the controller to be used for non-flash devices. The
> obvious symbol for that driver is CONFIG_SPI_ASPEED.
> 
> Cheers,
> 
> Joel
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/5] mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC

[Cédric Le Goater]

This driver adds mtd support for the Aspeed AST2400 SoC static memory
controllers:

 * New Static Memory Controller (referred as FMC)
   . BMC firmware
   . AST2500 compatible register set
   . 5 chip select pins (CE0 ∼ CE4)
   . supports NOR flash, NAND flash and SPI flash memory.

 * SPI Flash Controller (SPI)
   . host Firmware
   . slightly different register set, between AST2500 and the legacy
     controller
   . supports SPI flash memory
   . 1 chip select pin (CE0)

The legacy static memory controller (referred as SMC) is not
supported, as well as types other than SPI.

Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
---
 drivers/mtd/spi-nor/Kconfig      |  2 +-
 drivers/mtd/spi-nor/aspeed-smc.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 5c0efbd9dd89..22bea563f9bc 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -35,7 +35,7 @@ config SPI_ASPEED
 	depends on HAS_IOMEM && OF
 	help
 	  This enables support for the Firmware Memory controller (FMC)
-	  in the Aspeed AST2500 SoC when attached to SPI NOR chips,
+	  in the Aspeed AST2500/AST2400 SoCs when attached to SPI NOR chips,
 	  and support for the SPI flash memory controller (SPI) for
 	  the host firmware. The implementation only supports SPI NOR.
 
diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 6f9244f07aef..99302b0d7786 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -119,8 +119,27 @@ struct aspeed_smc_info {
 	void (*set_4b)(struct aspeed_smc_chip *chip);
 };
 
+static void aspeed_smc_chip_set_4b_spi_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,
+	.hastype = true,
+	.we0 = 16,
+	.ctl0 = 0x10,
+	.set_4b = aspeed_smc_chip_set_4b,
+};
+
+static const struct aspeed_smc_info spi_2400_info = {
+	.maxsize = 64 * 1024 * 1024,
+	.nce = 1,
+	.hastype = false,
+	.we0 = 0,
+	.ctl0 = 0x04,
+	.set_4b = aspeed_smc_chip_set_4b_spi_2400,
+};
+
 static const struct aspeed_smc_info fmc_2500_info = {
 	.maxsize = 256 * 1024 * 1024,
 	.nce = 3,
@@ -210,6 +229,7 @@ struct aspeed_smc_controller {
 #define CONTROL_IO_DUMMY_HI		BIT(14)
 #define CONTROL_IO_DUMMY_HI_SHIFT	14
 #define CONTROL_CLK_DIV4		BIT(13) /* others */
+#define CONTROL_IO_ADDRESS_4B		BIT(13) /* AST2400 SPI */
 #define CONTROL_RW_MERGE		BIT(12)
 #define CONTROL_IO_DUMMY_LO_SHIFT	6
 #define CONTROL_IO_DUMMY_LO		GENMASK(7, \
@@ -406,6 +426,8 @@ static int aspeed_smc_remove(struct platform_device *dev)
 }
 
 static const struct of_device_id aspeed_smc_matches[] = {
+	{ .compatible = "aspeed,ast2400-fmc", .data = &fmc_2400_info },
+	{ .compatible = "aspeed,ast2400-spi", .data = &spi_2400_info },
 	{ .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info },
 	{ .compatible = "aspeed,ast2500-spi", .data = &spi_2500_info },
 	{ }
@@ -479,6 +501,17 @@ static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip)
 	}
 }
 
+/*
+ * 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_spi_2400(struct aspeed_smc_chip *chip)
+{
+	chip->ctl_val[smc_base] |= CONTROL_IO_ADDRESS_4B;
+	chip->ctl_val[smc_read] |= CONTROL_IO_ADDRESS_4B;
+}
+
 static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
 				      struct resource *res)
 {
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC

[Marek Vasut]

On 12/09/2016 05:49 PM, Cédric Le Goater wrote:
> This driver adds mtd support for the Aspeed AST2400 SoC static memory
> controllers:
> 
>  * New Static Memory Controller (referred as FMC)
>    . BMC firmware
>    . AST2500 compatible register set
>    . 5 chip select pins (CE0 ∼ CE4)
>    . supports NOR flash, NAND flash and SPI flash memory.
> 
>  * SPI Flash Controller (SPI)
>    . host Firmware
>    . slightly different register set, between AST2500 and the legacy
>      controller
>    . supports SPI flash memory
>    . 1 chip select pin (CE0)
> 
> The legacy static memory controller (referred as SMC) is not
> supported, as well as types other than SPI.
> 
> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>

Well this is a nice split :-)

> ---
>  drivers/mtd/spi-nor/Kconfig      |  2 +-
>  drivers/mtd/spi-nor/aspeed-smc.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 5c0efbd9dd89..22bea563f9bc 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -35,7 +35,7 @@ config SPI_ASPEED
>  	depends on HAS_IOMEM && OF
>  	help
>  	  This enables support for the Firmware Memory controller (FMC)
> -	  in the Aspeed AST2500 SoC when attached to SPI NOR chips,
> +	  in the Aspeed AST2500/AST2400 SoCs when attached to SPI NOR chips,
>  	  and support for the SPI flash memory controller (SPI) for
>  	  the host firmware. The implementation only supports SPI NOR.
>  
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 6f9244f07aef..99302b0d7786 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -119,8 +119,27 @@ struct aspeed_smc_info {
>  	void (*set_4b)(struct aspeed_smc_chip *chip);
>  };
>  
> +static void aspeed_smc_chip_set_4b_spi_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,
> +	.hastype = true,
> +	.we0 = 16,
> +	.ctl0 = 0x10,
> +	.set_4b = aspeed_smc_chip_set_4b,
> +};
> +
> +static const struct aspeed_smc_info spi_2400_info = {
> +	.maxsize = 64 * 1024 * 1024,
> +	.nce = 1,
> +	.hastype = false,
> +	.we0 = 0,
> +	.ctl0 = 0x04,
> +	.set_4b = aspeed_smc_chip_set_4b_spi_2400,
> +};
> +
>  static const struct aspeed_smc_info fmc_2500_info = {
>  	.maxsize = 256 * 1024 * 1024,
>  	.nce = 3,
> @@ -210,6 +229,7 @@ struct aspeed_smc_controller {
>  #define CONTROL_IO_DUMMY_HI		BIT(14)
>  #define CONTROL_IO_DUMMY_HI_SHIFT	14
>  #define CONTROL_CLK_DIV4		BIT(13) /* others */
> +#define CONTROL_IO_ADDRESS_4B		BIT(13) /* AST2400 SPI */
>  #define CONTROL_RW_MERGE		BIT(12)
>  #define CONTROL_IO_DUMMY_LO_SHIFT	6
>  #define CONTROL_IO_DUMMY_LO		GENMASK(7, \
> @@ -406,6 +426,8 @@ static int aspeed_smc_remove(struct platform_device *dev)
>  }
>  
>  static const struct of_device_id aspeed_smc_matches[] = {
> +	{ .compatible = "aspeed,ast2400-fmc", .data = &fmc_2400_info },
> +	{ .compatible = "aspeed,ast2400-spi", .data = &spi_2400_info },
>  	{ .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info },
>  	{ .compatible = "aspeed,ast2500-spi", .data = &spi_2500_info },
>  	{ }
> @@ -479,6 +501,17 @@ static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip)
>  	}
>  }
>  
> +/*
> + * 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_spi_2400(struct aspeed_smc_chip *chip)
> +{
> +	chip->ctl_val[smc_base] |= CONTROL_IO_ADDRESS_4B;
> +	chip->ctl_val[smc_read] |= CONTROL_IO_ADDRESS_4B;

How do you know the SPI NOR is in 4B mode ?

> +}
> +
>  static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
>  				      struct resource *res)
>  {
> 


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC

[Cédric Le Goater]

On 12/10/2016 05:03 AM, Marek Vasut wrote:
>> +/*
>> + * 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_spi_2400(struct aspeed_smc_chip *chip)
>> +{
>> +	chip->ctl_val[smc_base] |= CONTROL_IO_ADDRESS_4B;
>> +	chip->ctl_val[smc_read] |= CONTROL_IO_ADDRESS_4B;
> How do you know the SPI NOR is in 4B mode ?

in aspeed_smc_chip_setup_finish() :

	if (chip->nor.addr_width == 4 && info->set_4b)
		info->set_4b(chip);

C.

 
>> +}
>> +
>>  static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
>>  				      struct resource *res)
>>  {
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC

[Marek Vasut]

On 12/10/2016 06:18 PM, Cédric Le Goater wrote:
> On 12/10/2016 05:03 AM, Marek Vasut wrote:
>>> +/*
>>> + * 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_spi_2400(struct aspeed_smc_chip *chip)
>>> +{
>>> +	chip->ctl_val[smc_base] |= CONTROL_IO_ADDRESS_4B;
>>> +	chip->ctl_val[smc_read] |= CONTROL_IO_ADDRESS_4B;
>> How do you know the SPI NOR is in 4B mode ?
> 
> in aspeed_smc_chip_setup_finish() :
> 
> 	if (chip->nor.addr_width == 4 && info->set_4b)
> 		info->set_4b(chip);
> 

Ahhh, great :)

-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] mtd: aspeed: add memory controllers for the Aspeed AST2400 SoC

[Joel Stanley]

On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
> This driver adds mtd support for the Aspeed AST2400 SoC static memory
> controllers:
>
> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>

Good work. It was worth spending the time splitting out the different
controllers I think.

Reviewed-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>

> ---
>  drivers/mtd/spi-nor/Kconfig      |  2 +-
>  drivers/mtd/spi-nor/aspeed-smc.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+), 1 deletion(-)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/5] mtd: aspeed: used a label property

[Cédric Le Goater]

This can be used to easily identify a chip on a system with multiple
chips.

Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 99302b0d7786..9119c0ca86b6 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -676,6 +676,8 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 		nor->prepare = aspeed_smc_prep;
 		nor->unprepare = aspeed_smc_unprep;
 
+		mtd->name = of_get_property(child, "label", NULL);
+
 		ret = aspeed_smc_chip_setup_init(chip, r);
 		if (ret)
 			break;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: aspeed: used a label property

[Marek Vasut]

On 12/09/2016 05:49 PM, Cédric Le Goater wrote:
> This can be used to easily identify a chip on a system with multiple
> chips.

I don't think I understand what this patch does.
It seems to me like this one should be wrapped into 1/5.

> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/aspeed-smc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 99302b0d7786..9119c0ca86b6 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -676,6 +676,8 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>  		nor->prepare = aspeed_smc_prep;
>  		nor->unprepare = aspeed_smc_unprep;
>  
> +		mtd->name = of_get_property(child, "label", NULL);
> +
>  		ret = aspeed_smc_chip_setup_init(chip, r);
>  		if (ret)
>  			break;
> 


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: aspeed: used a label property

[Cédric Le Goater]

On 12/10/2016 05:03 AM, Marek Vasut wrote:
> On 12/09/2016 05:49 PM, Cédric Le Goater wrote:
>> This can be used to easily identify a chip on a system with multiple
>> chips.
> 
> I don't think I understand what this patch does.
> It seems to me like this one should be wrapped into 1/5.

Yes, but this is to open the discussion on the change required in the 
jedec,spi-nor binding.

C.



>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  drivers/mtd/spi-nor/aspeed-smc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index 99302b0d7786..9119c0ca86b6 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -676,6 +676,8 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>  		nor->prepare = aspeed_smc_prep;
>>  		nor->unprepare = aspeed_smc_unprep;
>>  
>> +		mtd->name = of_get_property(child, "label", NULL);
>> +
>>  		ret = aspeed_smc_chip_setup_init(chip, r);
>>  		if (ret)
>>  			break;
>>
> 
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 3/5] mtd: aspeed: used a label property

[Joel Stanley]

On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
> This can be used to easily identify a chip on a system with multiple
> chips.
>
> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>

Our userspace benefits from having this.

Acked-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>

> ---
>  drivers/mtd/spi-nor/aspeed-smc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
> index 99302b0d7786..9119c0ca86b6 100644
> --- a/drivers/mtd/spi-nor/aspeed-smc.c
> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
> @@ -676,6 +676,8 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>                 nor->prepare = aspeed_smc_prep;
>                 nor->unprepare = aspeed_smc_unprep;
>
> +               mtd->name = of_get_property(child, "label", NULL);
> +
>                 ret = aspeed_smc_chip_setup_init(chip, r);
>                 if (ret)
>                         break;
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: aspeed: used a label property

[Marek Vasut]

On 12/12/2016 12:46 AM, Joel Stanley wrote:
> On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
>> This can be used to easily identify a chip on a system with multiple
>> chips.
>>
>> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
> 
> Our userspace benefits from having this.
> 
> Acked-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>

Shouldn't such thing be part of core code then ?

>> ---
>>  drivers/mtd/spi-nor/aspeed-smc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>> index 99302b0d7786..9119c0ca86b6 100644
>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>> @@ -676,6 +676,8 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>                 nor->prepare = aspeed_smc_prep;
>>                 nor->unprepare = aspeed_smc_unprep;
>>
>> +               mtd->name = of_get_property(child, "label", NULL);
>> +
>>                 ret = aspeed_smc_chip_setup_init(chip, r);
>>                 if (ret)
>>                         break;
>> --
>> 2.7.4
>>


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/5] mtd: aspeed: used a label property

[Cédric Le Goater]

On 12/12/2016 03:27 AM, Marek Vasut wrote:
> On 12/12/2016 12:46 AM, Joel Stanley wrote:
>> On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
>>> This can be used to easily identify a chip on a system with multiple
>>> chips.
>>>
>>> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
>>
>> Our userspace benefits from having this.
>>
>> Acked-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
> 
> Shouldn't such thing be part of core code then ?

yes. I will merge it in the next version. 

we already have the jedec,spi-nor label patch to discuss about
the binding change.

Thanks,

C. 

>>> ---
>>>  drivers/mtd/spi-nor/aspeed-smc.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
>>> index 99302b0d7786..9119c0ca86b6 100644
>>> --- a/drivers/mtd/spi-nor/aspeed-smc.c
>>> +++ b/drivers/mtd/spi-nor/aspeed-smc.c
>>> @@ -676,6 +676,8 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
>>>                 nor->prepare = aspeed_smc_prep;
>>>                 nor->unprepare = aspeed_smc_unprep;
>>>
>>> +               mtd->name = of_get_property(child, "label", NULL);
>>> +
>>>                 ret = aspeed_smc_chip_setup_init(chip, r);
>>>                 if (ret)
>>>                         break;
>>> --
>>> 2.7.4
>>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/5] mtd: spi-nor: bindings for the Aspeed memory controllers

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 .../devicetree/bindings/mtd/aspeed-smc.txt         | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt

diff --git a/Documentation/devicetree/bindings/mtd/aspeed-smc.txt b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
new file mode 100644
index 000000000000..e2c88cea38f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
@@ -0,0 +1,51 @@
+* Aspeed Firmware Memory controller
+* Aspeed SPI Flash Memory Controller
+
+The Firmware Memory Controller in the Aspeed AST2500 SoC supports
+three chip selects, two of which are always of SPI type and the third
+can be SPI or NOR type flash, but the driver only supports SPI.
+
+The two SPI flash memory controllers in the AST2500 each support two
+chip selects.
+
+Required properties:
+  - compatible : Should be one of
+	"aspeed,ast2400-fmc" for the AST2400 Firmware Memory Controller
+	"aspeed,ast2400-spi" for the AST2400 SPI Flash memory Controller
+	"aspeed,ast2500-fmc" for the AST2500 Firmware Memory Controller
+	"aspeed,ast2500-spi" for the AST2500 SPI flash memory 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 - max frequency of spi bus
+
+
+Example:
+fmc: fmc@1e620000 {
+	compatible = "aspeed,ast2500-fmc";
+	reg = < 0x1e620000 0x94
+		0x20000000 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>;
+	};
+};
-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 4/5] mtd: spi-nor: bindings for the Aspeed memory controllers

[Joel Stanley]

On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/aspeed-smc.txt         | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/aspeed-smc.txt
>
> diff --git a/Documentation/devicetree/bindings/mtd/aspeed-smc.txt b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
> new file mode 100644
> index 000000000000..e2c88cea38f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
> @@ -0,0 +1,51 @@
> +* Aspeed Firmware Memory controller
> +* Aspeed SPI Flash Memory Controller
> +
> +The Firmware Memory Controller in the Aspeed AST2500 SoC supports
> +three chip selects, two of which are always of SPI type and the third
> +can be SPI or NOR type flash, but the driver only supports SPI.

As the bindings are a description of the hardware and not the driver,
we might want to reword this. Perhaps "these bindings only describe
SPI"?

Cheers,

Joel

> +
> +The two SPI flash memory controllers in the AST2500 each support two
> +chip selects.
> +
> +Required properties:
> +  - compatible : Should be one of
> +       "aspeed,ast2400-fmc" for the AST2400 Firmware Memory Controller
> +       "aspeed,ast2400-spi" for the AST2400 SPI Flash memory Controller
> +       "aspeed,ast2500-fmc" for the AST2500 Firmware Memory Controller
> +       "aspeed,ast2500-spi" for the AST2500 SPI flash memory 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 - max frequency of spi bus
> +
> +
> +Example:
> +fmc: fmc@1e620000 {
> +       compatible = "aspeed,ast2500-fmc";
> +       reg = < 0x1e620000 0x94
> +               0x20000000 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>;
> +       };
> +};
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/5] mtd: spi-nor: add a label property to jedec,spi-nor

[Cédric Le Goater]

This can be used to easily identify a chip on a system with multiple
chips.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
index 2c91c03e7eb0..b7cd02a3ebe8 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
@@ -65,6 +65,7 @@ Optional properties:
                    all chips and support for it can not be detected at runtime.
                    Refer to your chips' datasheet to check if this is supported
                    by your chip.
+- label : name to assign to mtd. If omitted, the label is the MTD device name.
 
 Example:
 
@@ -75,4 +76,5 @@ Example:
 		reg = <0>;
 		spi-max-frequency = <40000000>;
 		m25p,fast-read;
+		label = "System-firmware";
 	};
-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v3 5/5] mtd: spi-nor: add a label property to jedec,spi-nor

[Joel Stanley]

On Sat, Dec 10, 2016 at 3:49 AM, Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org> wrote:
> This can be used to easily identify a chip on a system with multiple
> chips.
>
> Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>

This is very useful when writing userspace that wants to select the
mtd device that represents a specific flash chip.

Acked-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>

> ---
>  Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> index 2c91c03e7eb0..b7cd02a3ebe8 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> @@ -65,6 +65,7 @@ Optional properties:
>                     all chips and support for it can not be detected at runtime.
>                     Refer to your chips' datasheet to check if this is supported
>                     by your chip.
> +- label : name to assign to mtd. If omitted, the label is the MTD device name.
>
>  Example:
>
> @@ -75,4 +76,5 @@ Example:
>                 reg = <0>;
>                 spi-max-frequency = <40000000>;
>                 m25p,fast-read;
> +               label = "System-firmware";
>         };
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html