[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Cédric Le Goater]

Hello,

This series first extends the support for the Aspeed AST2500 and
AST2400 SMC driver. It adds Dual Data support and read training giving
the best read settings for a given chip. Support for the new AST2600
SoC is added at the end.

I understand that a new spi_mem framework exists and I do have an
experimental driver using it. But unfortunately, it is difficult to
integrate the read training. The Aspeed constraints are not compatible
and i haven't had the time to extend the current framework.

This patchset has been in use for some time in the OpenBMC kernel on
these systems :

 * OpenPOWER Palmetto (AST2400)
 * Evaluation board (AST2500) 
 * OpenPOWER Witherspoon (AST2500)
 * OpenPOWER Romulus (AST2500)
 * OpenPOWER Zaius (AST2500)
   and many others
 
and it is now in use on these boards with the new SoC :

 * Evaluation board (AST2600) 
 * Tacoma board (AST2600) 

Thanks,

C.

Alexander Soldatov (1):
  mtd: spi-nor: fix options for mx66l51235f

C?dric Le Goater (15):
  mtd: spi-nor: aspeed: Use command mode for reads
  mtd: spi-nor: aspeed: Add support for SPI dual IO read mode
  mtd: spi-nor: aspeed: Link controller with the ahb clock
  mtd: spi-nor: aspeed: Add read training
  mtd: spi-nor: aspeed: Limit the maximum SPI frequency
  mtd: spi-nor: aspeed: Add support for the 4B opcodes
  mtd: spi-nor: Add support for w25q512jv
  mtd: spi-nor: aspeed: Introduce a field for the AHB physical address
  mtd: spi-nor: aspeed: Introduce segment operations
  dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600
  mtd: spi-nor: aspeed: Add initial support for the AST2600
  mtd: spi-nor: aspeed: Check for disabled segments on the AST2600
  mtd: spi-nor: aspeed: Introduce training operations per platform
  mtd: spi-nor: aspeed: Introduce a HCLK mask for training
  mtd: spi-nor: aspeed: Add read training support for the AST2600

 drivers/mtd/spi-nor/aspeed-smc.c              | 593 ++++++++++++++++--
 drivers/mtd/spi-nor/spi-nor.c                 |   5 +-
 .../devicetree/bindings/mtd/aspeed-smc.txt    |   2 +
 3 files changed, 551 insertions(+), 49 deletions(-)

-- 
2.21.0

[PATCH 01/16] mtd: spi-nor: aspeed: Use command mode for reads

[Cédric Le Goater]

When reading flash contents, try to read from the AHB window
configured for the flash module. This is called the "command mode" on
Aspeed SoC SMC controllers. If the window is not big enough, because
of HW issues, fall back to the "user mode" to perform the read.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 009c1da8574c..148bbc934efc 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -398,6 +398,31 @@ static ssize_t aspeed_smc_write_user(struct spi_nor *nor, loff_t to,
 	return len;
 }
 
+static ssize_t aspeed_smc_read(struct spi_nor *nor, loff_t from, size_t len,
+			       u_char *read_buf)
+{
+	struct aspeed_smc_chip *chip = nor->priv;
+
+	/*
+	 * The AHB window configured for the chip is too small for the
+	 * read offset. Use the "User mode" of the controller to
+	 * perform the read.
+	 */
+	if (from >= chip->ahb_window_size) {
+		aspeed_smc_read_user(nor, from, len, read_buf);
+		goto out;
+	}
+
+	/*
+	 * Use the "Command mode" to do a direct read from the AHB
+	 * window configured for the chip. This should be the default.
+	 */
+	memcpy_fromio(read_buf, chip->ahb_base + from, len);
+
+out:
+	return len;
+}
+
 static int aspeed_smc_unregister(struct aspeed_smc_controller *controller)
 {
 	struct aspeed_smc_chip *chip;
@@ -739,6 +764,7 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	}
 
 	chip->ctl_val[smc_read] |= cmd |
+		chip->nor.read_opcode << CONTROL_COMMAND_SHIFT |
 		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
 
 	dev_dbg(controller->dev, "base control register: %08x\n",
@@ -805,7 +831,7 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 		nor->dev = dev;
 		nor->priv = chip;
 		spi_nor_set_flash_node(nor, child);
-		nor->read = aspeed_smc_read_user;
+		nor->read = aspeed_smc_read;
 		nor->write = aspeed_smc_write_user;
 		nor->read_reg = aspeed_smc_read_reg;
 		nor->write_reg = aspeed_smc_write_reg;
-- 
2.21.0

[PATCH 02/16] mtd: spi-nor: aspeed: Add support for SPI dual IO read mode

[Cédric Le Goater]

Implements support for the dual IO read mode on aspeed SMC/FMC
controllers which uses both MISO and MOSI lines for data during a read
to double the read bandwidth.

Based on work from Robert Lippert <roblip@gmail.com>

Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 56 +++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 148bbc934efc..c775e0612613 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -369,18 +369,49 @@ static void aspeed_smc_send_cmd_addr(struct spi_nor *nor, u8 cmd, u32 addr)
 	}
 }
 
+static int aspeed_smc_get_io_mode(struct aspeed_smc_chip *chip)
+{
+	switch (chip->nor.read_proto) {
+	case SNOR_PROTO_1_1_1:
+		return 0;
+	case SNOR_PROTO_1_1_2:
+		return CONTROL_IO_DUAL_DATA;
+	case SNOR_PROTO_1_2_2:
+		return CONTROL_IO_DUAL_ADDR_DATA;
+	default:
+		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
+		return -EINVAL;
+	}
+}
+
+static void aspeed_smc_set_io_mode(struct aspeed_smc_chip *chip, u32 io_mode)
+{
+	u32 ctl;
+
+	if (io_mode > 0) {
+		ctl = readl(chip->ctl) & ~CONTROL_IO_MODE_MASK;
+		ctl |= io_mode;
+		writel(ctl, chip->ctl);
+	}
+}
+
 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;
 	int i;
 	u8 dummy = 0xFF;
+	int io_mode = aspeed_smc_get_io_mode(chip);
 
 	aspeed_smc_start_user(nor);
 	aspeed_smc_send_cmd_addr(nor, nor->read_opcode, from);
 	for (i = 0; i < chip->nor.read_dummy / 8; i++)
 		aspeed_smc_write_to_ahb(chip->ahb_base, &dummy, sizeof(dummy));
 
+	/* Set IO mode only for data */
+	if (io_mode == CONTROL_IO_DUAL_DATA)
+		aspeed_smc_set_io_mode(chip, io_mode);
+
 	aspeed_smc_read_from_ahb(read_buf, chip->ahb_base, len);
 	aspeed_smc_stop_user(nor);
 	return len;
@@ -731,6 +762,7 @@ 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;
+	int io_mode;
 	u32 cmd;
 
 	if (chip->nor.addr_width == 4 && info->set_4b)
@@ -753,22 +785,21 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	 * TODO: Adjust clocks if fast read is supported and interpret
 	 * SPI-NOR flags to adjust controller settings.
 	 */
-	if (chip->nor.read_proto == SNOR_PROTO_1_1_1) {
-		if (chip->nor.read_dummy == 0)
-			cmd = CONTROL_COMMAND_MODE_NORMAL;
-		else
-			cmd = CONTROL_COMMAND_MODE_FREAD;
-	} else {
-		dev_err(chip->nor.dev, "unsupported SPI read mode\n");
-		return -EINVAL;
-	}
+	io_mode = aspeed_smc_get_io_mode(chip);
+	if (io_mode < 0)
+		return io_mode;
 
-	chip->ctl_val[smc_read] |= cmd |
+	if (chip->nor.read_dummy == 0)
+		cmd = CONTROL_COMMAND_MODE_NORMAL;
+	else
+		cmd = CONTROL_COMMAND_MODE_FREAD;
+
+	chip->ctl_val[smc_read] |= cmd | io_mode |
 		chip->nor.read_opcode << CONTROL_COMMAND_SHIFT |
 		CONTROL_IO_DUMMY_SET(chip->nor.read_dummy / 8);
 
-	dev_dbg(controller->dev, "base control register: %08x\n",
-		chip->ctl_val[smc_read]);
+	dev_info(controller->dev, "read control register: %08x\n",
+		 chip->ctl_val[smc_read]);
 	return 0;
 }
 
@@ -778,6 +809,7 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 	const struct spi_nor_hwcaps hwcaps = {
 		.mask = SNOR_HWCAPS_READ |
 			SNOR_HWCAPS_READ_FAST |
+			SNOR_HWCAPS_READ_1_1_2 |
 			SNOR_HWCAPS_PP,
 	};
 	const struct aspeed_smc_info *info = controller->info;
-- 
2.21.0

[PATCH 03/16] mtd: spi-nor: aspeed: Link controller with the ahb clock

[Cédric Le Goater]

We will need the AHB frequency to set the HCLK settings in the SMC
controller to perform the read training.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index c775e0612613..facd8fc16ca3 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bug.h>
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -109,6 +110,8 @@ struct aspeed_smc_controller {
 	void __iomem *ahb_base;			/* per-chip windows resource */
 	u32 ahb_window_size;			/* full mapping window size */
 
+	unsigned long	clk_frequency;
+
 	struct aspeed_smc_chip *chips[0];	/* pointers to attached chips */
 };
 
@@ -909,6 +912,7 @@ 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 clk *clk;
 	struct resource *res;
 	int ret;
 
@@ -940,6 +944,12 @@ static int aspeed_smc_probe(struct platform_device *pdev)
 
 	controller->ahb_window_size = resource_size(res);
 
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	controller->clk_frequency = clk_get_rate(clk);
+	devm_clk_put(&pdev->dev, clk);
+
 	ret = aspeed_smc_setup_flash(controller, np, res);
 	if (ret)
 		dev_err(dev, "Aspeed SMC probe failed %d\n", ret);
-- 
2.21.0

[PATCH 04/16] mtd: spi-nor: aspeed: Add read training

[Cédric Le Goater]

The read training algorithm first reads a golden buffer at low speed
and then performs reads with different clocks and delay cycles
settings to find the fastest configuration for the chip. The current
implementation is based on the OpenPOWER pflash tool.

For the moment, read training is only activated for SPI controllers as
U-Boot should have done the read training for the FMC controller using
the DMA interface. We also don't limit yet the max frequency, so it's
safer not to be too optimistic on the capabilities of the boot flash.

It can be deactivated at boot time with the kernel parameter :

	aspeed_smc.optimize_read=0

Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 204 +++++++++++++++++++++++++++++++
 1 file changed, 204 insertions(+)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index facd8fc16ca3..155c407c2bdf 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/sizes.h>
+#include <linux/slab.h>
 #include <linux/sysfs.h>
 
 #define DEVICE_NAME	"aspeed-smc"
@@ -38,12 +39,16 @@ struct aspeed_smc_info {
 	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 */
+	u8 timing;		/* offset in regs of timing */
 
 	void (*set_4b)(struct aspeed_smc_chip *chip);
+	int (*optimize_read)(struct aspeed_smc_chip *chip, u32 max_freq);
 };
 
 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 int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
+				    u32 max_freq);
 
 static const struct aspeed_smc_info fmc_2400_info = {
 	.maxsize = 64 * 1024 * 1024,
@@ -51,6 +56,7 @@ static const struct aspeed_smc_info fmc_2400_info = {
 	.hastype = true,
 	.we0 = 16,
 	.ctl0 = 0x10,
+	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
 };
 
@@ -60,7 +66,9 @@ static const struct aspeed_smc_info spi_2400_info = {
 	.hastype = false,
 	.we0 = 0,
 	.ctl0 = 0x04,
+	.timing = 0x14,
 	.set_4b = aspeed_smc_chip_set_4b_spi_2400,
+	.optimize_read = aspeed_smc_optimize_read,
 };
 
 static const struct aspeed_smc_info fmc_2500_info = {
@@ -69,6 +77,7 @@ static const struct aspeed_smc_info fmc_2500_info = {
 	.hastype = true,
 	.we0 = 16,
 	.ctl0 = 0x10,
+	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
 };
 
@@ -78,7 +87,9 @@ static const struct aspeed_smc_info spi_2500_info = {
 	.hastype = false,
 	.we0 = 16,
 	.ctl0 = 0x10,
+	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
+	.optimize_read = aspeed_smc_optimize_read,
 };
 
 enum aspeed_smc_ctl_reg_value {
@@ -200,6 +211,12 @@ struct aspeed_smc_controller {
 #define SEGMENT_ADDR_REG(controller, cs)	\
 	((controller)->regs + SEGMENT_ADDR_REG0 + (cs) * 4)
 
+/*
+ * Switch to turn off read optimisation if needed
+ */
+static bool optimize_read = true;
+module_param(optimize_read, bool, 0644);
+
 /*
  * 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
@@ -761,6 +778,187 @@ static int aspeed_smc_chip_setup_init(struct aspeed_smc_chip *chip,
 	return 0;
 }
 
+#define CALIBRATE_BUF_SIZE 16384
+
+static bool aspeed_smc_check_reads(struct aspeed_smc_chip *chip,
+				   const u8 *golden_buf, u8 *test_buf)
+{
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
+		if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0)
+			return false;
+	}
+	return true;
+}
+
+static int aspeed_smc_calibrate_reads(struct aspeed_smc_chip *chip, u32 hdiv,
+				      const u8 *golden_buf, u8 *test_buf)
+{
+	struct aspeed_smc_controller *controller = chip->controller;
+	const struct aspeed_smc_info *info = controller->info;
+	int i;
+	int good_pass = -1, pass_count = 0;
+	u32 shift = (hdiv - 1) << 2;
+	u32 mask = ~(0xfu << shift);
+	u32 fread_timing_val = 0;
+
+#define FREAD_TPASS(i)	(((i) / 2) | (((i) & 1) ? 0 : 8))
+
+	/* Try HCLK delay 0..5, each one with/without delay and look for a
+	 * good pair.
+	 */
+	for (i = 0; i < 12; i++) {
+		bool pass;
+
+		fread_timing_val &= mask;
+		fread_timing_val |= FREAD_TPASS(i) << shift;
+
+		writel(fread_timing_val, controller->regs + info->timing);
+		pass = aspeed_smc_check_reads(chip, golden_buf, test_buf);
+		dev_dbg(chip->nor.dev,
+			"  * [%08x] %d HCLK delay, %dns DI delay : %s",
+			fread_timing_val, i / 2, (i & 1) ? 0 : 4,
+			pass ? "PASS" : "FAIL");
+		if (pass) {
+			pass_count++;
+			if (pass_count == 3) {
+				good_pass = i - 1;
+				break;
+			}
+		} else {
+			pass_count = 0;
+		}
+	}
+
+	/* No good setting for this frequency */
+	if (good_pass < 0)
+		return -1;
+
+	/* We have@least one pass of margin, let's use first pass */
+	fread_timing_val &= mask;
+	fread_timing_val |= FREAD_TPASS(good_pass) << shift;
+	writel(fread_timing_val, controller->regs + info->timing);
+	dev_dbg(chip->nor.dev, " * -> good is pass %d [0x%08x]",
+		good_pass, fread_timing_val);
+	return 0;
+}
+
+static bool aspeed_smc_check_calib_data(const u8 *test_buf, u32 size)
+{
+	const u32 *tb32 = (const u32 *)test_buf;
+	u32 i, cnt = 0;
+
+	/* We check if we have enough words that are neither all 0
+	 * nor all 1's so the calibration can be considered valid.
+	 *
+	 * I use an arbitrary threshold for now of 64
+	 */
+	size >>= 2;
+	for (i = 0; i < size; i++) {
+		if (tb32[i] != 0 && tb32[i] != 0xffffffff)
+			cnt++;
+	}
+	return cnt >= 64;
+}
+
+static const u32 aspeed_smc_hclk_divs[] = {
+	0xf, /* HCLK */
+	0x7, /* HCLK/2 */
+	0xe, /* HCLK/3 */
+	0x6, /* HCLK/4 */
+	0xd, /* HCLK/5 */
+};
+
+#define ASPEED_SMC_HCLK_DIV(i) \
+	(aspeed_smc_hclk_divs[(i) - 1] << CONTROL_CLOCK_FREQ_SEL_SHIFT)
+
+static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
+{
+	/*
+	 * Keep the 4Byte address mode on the AST2400 SPI controller.
+	 * Other controllers set the 4Byte mode in the CE Control
+	 * Register
+	 */
+	u32 ctl_mask = chip->controller->info == &spi_2400_info ?
+		 CONTROL_IO_ADDRESS_4B : 0;
+
+	return (chip->ctl_val[smc_read] & ctl_mask) |
+		(0x00 << 28) | /* Single bit */
+		(0x00 << 24) | /* CE# max */
+		(0x03 << 16) | /* use normal reads */
+		(0x00 <<  8) | /* HCLK/16 */
+		(0x00 <<  6) | /* no dummy cycle */
+		(0x00);        /* normal mode */
+}
+
+static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
+				    u32 max_freq)
+{
+	u8 *golden_buf, *test_buf;
+	int i, rc, best_div = -1;
+	u32 save_read_val = chip->ctl_val[smc_read];
+	u32 ahb_freq = chip->controller->clk_frequency;
+
+	dev_dbg(chip->nor.dev, "AHB frequency: %d MHz", ahb_freq / 1000000);
+
+	test_buf = kmalloc(CALIBRATE_BUF_SIZE * 2, GFP_KERNEL);
+	golden_buf = test_buf + CALIBRATE_BUF_SIZE;
+
+	/* We start with the dumbest setting (keep 4Byte bit) and read
+	 * some data
+	 */
+	chip->ctl_val[smc_read] = aspeed_smc_default_read(chip);
+
+	writel(chip->ctl_val[smc_read], chip->ctl);
+
+	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
+
+	/* Establish our read mode with freq field set to 0 (HCLK/16) */
+	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
+
+	/* Check if calibration data is suitable */
+	if (!aspeed_smc_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
+		dev_info(chip->nor.dev,
+			 "Calibration area too uniform, using low speed");
+		writel(chip->ctl_val[smc_read], chip->ctl);
+		kfree(test_buf);
+		return 0;
+	}
+
+	/* Now we iterate the HCLK dividers until we find our breaking point */
+	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs); i > 0; i--) {
+		u32 tv, freq;
+
+		/* Compare timing to max */
+		freq = ahb_freq / i;
+		if (freq > max_freq)
+			continue;
+
+		/* Set the timing */
+		tv = chip->ctl_val[smc_read] | ASPEED_SMC_HCLK_DIV(i);
+		writel(tv, chip->ctl);
+		dev_dbg(chip->nor.dev, "Trying HCLK/%d...", i);
+		rc = aspeed_smc_calibrate_reads(chip, i, golden_buf, test_buf);
+		if (rc == 0)
+			best_div = i;
+	}
+	kfree(test_buf);
+
+	/* Nothing found ? */
+	if (best_div < 0) {
+		dev_warn(chip->nor.dev, "No good frequency, using dumb slow");
+	} else {
+		dev_dbg(chip->nor.dev, "Found good read timings at HCLK/%d",
+			best_div);
+		chip->ctl_val[smc_read] |= ASPEED_SMC_HCLK_DIV(best_div);
+	}
+
+	writel(chip->ctl_val[smc_read], chip->ctl);
+	return 0;
+}
+
 static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 {
 	struct aspeed_smc_controller *controller = chip->controller;
@@ -803,6 +1001,12 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 
 	dev_info(controller->dev, "read control register: %08x\n",
 		 chip->ctl_val[smc_read]);
+
+	/*
+	 * TODO: get max freq from chip
+	 */
+	if (optimize_read && info->optimize_read)
+		info->optimize_read(chip, 104000000);
 	return 0;
 }
 
-- 
2.21.0

[PATCH 04/16] mtd: spi-nor: aspeed: Add read training

[Boris Brezillon]

On Fri,  4 Oct 2019 13:59:07 +0200
C?dric Le Goater <clg@kaod.org> wrote:

> +#define ASPEED_SMC_HCLK_DIV(i) \
> +	(aspeed_smc_hclk_divs[(i) - 1] << CONTROL_CLOCK_FREQ_SEL_SHIFT)
> +
> +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
> +{
> +	/*
> +	 * Keep the 4Byte address mode on the AST2400 SPI controller.
> +	 * Other controllers set the 4Byte mode in the CE Control
> +	 * Register
> +	 */
> +	u32 ctl_mask = chip->controller->info == &spi_2400_info ?
> +		 CONTROL_IO_ADDRESS_4B : 0;
> +
> +	return (chip->ctl_val[smc_read] & ctl_mask) |
> +		(0x00 << 28) | /* Single bit */
> +		(0x00 << 24) | /* CE# max */
> +		(0x03 << 16) | /* use normal reads */
> +		(0x00 <<  8) | /* HCLK/16 */
> +		(0x00 <<  6) | /* no dummy cycle */
> +		(0x00);        /* normal mode */

IIUC, you're using a SPINOR_OP_READ operation to read the golden
buffer, and if I'm right, you start reading at offset 0 of the dirmap
window (offset 0 in the flash), so basically the first block in the NOR.
What happens if this block is erased? In that case your golden buf will
contain only 0xff values, and the read calibration is likely to be
useless (how can you determine if timings are good when IO pins always
stay high). Don't we have a command that return non-ff/non-0 data while
still being predictable and immutable? Do you expect users to always
flash a pattern that helps calibrating those delays?

> +}
> +
> +static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
> +				    u32 max_freq)
> +{
> +	u8 *golden_buf, *test_buf;
> +	int i, rc, best_div = -1;
> +	u32 save_read_val = chip->ctl_val[smc_read];
> +	u32 ahb_freq = chip->controller->clk_frequency;
> +
> +	dev_dbg(chip->nor.dev, "AHB frequency: %d MHz", ahb_freq / 1000000);
> +
> +	test_buf = kmalloc(CALIBRATE_BUF_SIZE * 2, GFP_KERNEL);
> +	golden_buf = test_buf + CALIBRATE_BUF_SIZE;
> +
> +	/* We start with the dumbest setting (keep 4Byte bit) and read
> +	 * some data
> +	 */
> +	chip->ctl_val[smc_read] = aspeed_smc_default_read(chip);
> +
> +	writel(chip->ctl_val[smc_read], chip->ctl);
> +
> +	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
> +
> +	/* Establish our read mode with freq field set to 0 (HCLK/16) */
> +	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
> +
> +	/* Check if calibration data is suitable */
> +	if (!aspeed_smc_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
> +		dev_info(chip->nor.dev,
> +			 "Calibration area too uniform, using low speed");
> +		writel(chip->ctl_val[smc_read], chip->ctl);
> +		kfree(test_buf);
> +		return 0;
> +	}
> +
> +	/* Now we iterate the HCLK dividers until we find our breaking point */
> +	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs); i > 0; i--) {
> +		u32 tv, freq;
> +
> +		/* Compare timing to max */
> +		freq = ahb_freq / i;
> +		if (freq > max_freq)
> +			continue;
> +
> +		/* Set the timing */
> +		tv = chip->ctl_val[smc_read] | ASPEED_SMC_HCLK_DIV(i);
> +		writel(tv, chip->ctl);
> +		dev_dbg(chip->nor.dev, "Trying HCLK/%d...", i);
> +		rc = aspeed_smc_calibrate_reads(chip, i, golden_buf, test_buf);
> +		if (rc == 0)
> +			best_div = i;
> +	}
> +	kfree(test_buf);
> +
> +	/* Nothing found ? */
> +	if (best_div < 0) {
> +		dev_warn(chip->nor.dev, "No good frequency, using dumb slow");
> +	} else {
> +		dev_dbg(chip->nor.dev, "Found good read timings at HCLK/%d",
> +			best_div);
> +		chip->ctl_val[smc_read] |= ASPEED_SMC_HCLK_DIV(best_div);
> +	}
> +
> +	writel(chip->ctl_val[smc_read], chip->ctl);
> +	return 0;
> +}

[PATCH 04/16] mtd: spi-nor: aspeed: Add read training

[Vignesh Raghavendra]

Hi Cedric,

On 11/10/19 5:58 PM, Boris Brezillon wrote:
> On Fri,  4 Oct 2019 13:59:07 +0200
> C?dric Le Goater <clg@kaod.org> wrote:
> 
>> +#define ASPEED_SMC_HCLK_DIV(i) \
>> +	(aspeed_smc_hclk_divs[(i) - 1] << CONTROL_CLOCK_FREQ_SEL_SHIFT)
>> +
>> +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
>> +{
>> +	/*
>> +	 * Keep the 4Byte address mode on the AST2400 SPI controller.
>> +	 * Other controllers set the 4Byte mode in the CE Control
>> +	 * Register
>> +	 */
>> +	u32 ctl_mask = chip->controller->info == &spi_2400_info ?
>> +		 CONTROL_IO_ADDRESS_4B : 0;
>> +
>> +	return (chip->ctl_val[smc_read] & ctl_mask) |
>> +		(0x00 << 28) | /* Single bit */
>> +		(0x00 << 24) | /* CE# max */
>> +		(0x03 << 16) | /* use normal reads */
>> +		(0x00 <<  8) | /* HCLK/16 */
>> +		(0x00 <<  6) | /* no dummy cycle */
>> +		(0x00);        /* normal mode */
> 
> IIUC, you're using a SPINOR_OP_READ operation to read the golden
> buffer, and if I'm right, you start reading at offset 0 of the dirmap
> window (offset 0 in the flash), so basically the first block in the NOR.
> What happens if this block is erased? In that case your golden buf will
> contain only 0xff values, and the read calibration is likely to be
> useless (how can you determine if timings are good when IO pins always
> stay high). Don't we have a command that return non-ff/non-0 data while
> still being predictable and immutable? Do you expect users to always
> flash a pattern that helps calibrating those delays?
> 

Yes, this is precisely my concern as well. I have been developing
training sequence for cadence-quadspi controller (requirements are
similar to what you have here) and found that its better to use read
only data such as SFDP table data to calibrate. Cadence-quadspi requires
training only in higher performance modes like Quad/Octal DTR mode and
needs 16 bytes of known data for calibration. Hence SFDP works well for
my case.
But the problem here is that, aspeed controller needs 16K of known data.
SFDP table is not that big and read beyond address space is not required
to wrap around.
Wondering if you really need to read 16K amount of data for calibration?

Regards
Vignesh

>> +}
>> +
>> +static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
>> +				    u32 max_freq)
>> +{
>> +	u8 *golden_buf, *test_buf;
>> +	int i, rc, best_div = -1;
>> +	u32 save_read_val = chip->ctl_val[smc_read];
>> +	u32 ahb_freq = chip->controller->clk_frequency;
>> +
>> +	dev_dbg(chip->nor.dev, "AHB frequency: %d MHz", ahb_freq / 1000000);
>> +
>> +	test_buf = kmalloc(CALIBRATE_BUF_SIZE * 2, GFP_KERNEL);
>> +	golden_buf = test_buf + CALIBRATE_BUF_SIZE;
>> +
>> +	/* We start with the dumbest setting (keep 4Byte bit) and read
>> +	 * some data
>> +	 */
>> +	chip->ctl_val[smc_read] = aspeed_smc_default_read(chip);
>> +
>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>> +
>> +	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
>> +
>> +	/* Establish our read mode with freq field set to 0 (HCLK/16) */
>> +	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
>> +
>> +	/* Check if calibration data is suitable */
>> +	if (!aspeed_smc_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
>> +		dev_info(chip->nor.dev,
>> +			 "Calibration area too uniform, using low speed");
>> +		writel(chip->ctl_val[smc_read], chip->ctl);
>> +		kfree(test_buf);
>> +		return 0;
>> +	}
>> +
>> +	/* Now we iterate the HCLK dividers until we find our breaking point */
>> +	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs); i > 0; i--) {
>> +		u32 tv, freq;
>> +
>> +		/* Compare timing to max */
>> +		freq = ahb_freq / i;
>> +		if (freq > max_freq)
>> +			continue;
>> +
>> +		/* Set the timing */
>> +		tv = chip->ctl_val[smc_read] | ASPEED_SMC_HCLK_DIV(i);
>> +		writel(tv, chip->ctl);
>> +		dev_dbg(chip->nor.dev, "Trying HCLK/%d...", i);
>> +		rc = aspeed_smc_calibrate_reads(chip, i, golden_buf, test_buf);
>> +		if (rc == 0)
>> +			best_div = i;
>> +	}
>> +	kfree(test_buf);
>> +
>> +	/* Nothing found ? */
>> +	if (best_div < 0) {
>> +		dev_warn(chip->nor.dev, "No good frequency, using dumb slow");
>> +	} else {
>> +		dev_dbg(chip->nor.dev, "Found good read timings at HCLK/%d",
>> +			best_div);
>> +		chip->ctl_val[smc_read] |= ASPEED_SMC_HCLK_DIV(best_div);
>> +	}
>> +
>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>> +	return 0;
>> +}
> 
> 

-- 
Regards
Vignesh

[PATCH 04/16] mtd: spi-nor: aspeed: Add read training

[Cédric Le Goater]

On 11/10/2019 15:13, Vignesh Raghavendra wrote:
> Hi Cedric,
> 
> On 11/10/19 5:58 PM, Boris Brezillon wrote:
>> On Fri,  4 Oct 2019 13:59:07 +0200
>> C?dric Le Goater <clg@kaod.org> wrote:
>>
>>> +#define ASPEED_SMC_HCLK_DIV(i) \
>>> +	(aspeed_smc_hclk_divs[(i) - 1] << CONTROL_CLOCK_FREQ_SEL_SHIFT)
>>> +
>>> +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
>>> +{
>>> +	/*
>>> +	 * Keep the 4Byte address mode on the AST2400 SPI controller.
>>> +	 * Other controllers set the 4Byte mode in the CE Control
>>> +	 * Register
>>> +	 */
>>> +	u32 ctl_mask = chip->controller->info == &spi_2400_info ?
>>> +		 CONTROL_IO_ADDRESS_4B : 0;
>>> +
>>> +	return (chip->ctl_val[smc_read] & ctl_mask) |
>>> +		(0x00 << 28) | /* Single bit */
>>> +		(0x00 << 24) | /* CE# max */
>>> +		(0x03 << 16) | /* use normal reads */
>>> +		(0x00 <<  8) | /* HCLK/16 */
>>> +		(0x00 <<  6) | /* no dummy cycle */
>>> +		(0x00);        /* normal mode */
>>
>> IIUC, you're using a SPINOR_OP_READ operation to read the golden
>> buffer, and if I'm right, you start reading at offset 0 of the dirmap
>> window (offset 0 in the flash), so basically the first block in the NOR.
>> What happens if this block is erased? In that case your golden buf will
>> contain only 0xff values, and the read calibration is likely to be
>> useless (how can you determine if timings are good when IO pins always
>> stay high). Don't we have a command that return non-ff/non-0 data while
>> still being predictable and immutable? Do you expect users to always
>> flash a pattern that helps calibrating those delays?
>>
> 
> Yes, this is precisely my concern as well. I have been developing
> training sequence for cadence-quadspi controller (requirements are
> similar to what you have here) and found that its better to use read
> only data such as SFDP table data to calibrate. Cadence-quadspi requires
> training only in higher performance modes like Quad/Octal DTR mode and
> needs 16 bytes of known data for calibration. Hence SFDP works well for
> my case.

OK. Good to know.

> But the problem here is that, aspeed controller needs 16K of known data.

It's a choice we made on the first P8 systems we had.

> SFDP table is not that big and read beyond address space is not required
> to wrap around.
> Wondering if you really need to read 16K amount of data for calibration?

May be not. I agree this is taking a bit of time as we read 10 times
and compares: 3s on boot time on an ast2400 I think. Joel could tell
better. We could reduce the amount of data read and the number of 
loops surely.  

As for the validity of the data, we check with the golden buffer with
aspeed_smc_check_calib_data().

If it's too uniform, like on a flash never written too, we will need
a first write and a reboot to have faster read speed.

Thanks,

C. 

> 
> Regards
> Vignesh
> 
>>> +}
>>> +
>>> +static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
>>> +				    u32 max_freq)
>>> +{
>>> +	u8 *golden_buf, *test_buf;
>>> +	int i, rc, best_div = -1;
>>> +	u32 save_read_val = chip->ctl_val[smc_read];
>>> +	u32 ahb_freq = chip->controller->clk_frequency;
>>> +
>>> +	dev_dbg(chip->nor.dev, "AHB frequency: %d MHz", ahb_freq / 1000000);
>>> +
>>> +	test_buf = kmalloc(CALIBRATE_BUF_SIZE * 2, GFP_KERNEL);
>>> +	golden_buf = test_buf + CALIBRATE_BUF_SIZE;
>>> +
>>> +	/* We start with the dumbest setting (keep 4Byte bit) and read
>>> +	 * some data
>>> +	 */
>>> +	chip->ctl_val[smc_read] = aspeed_smc_default_read(chip);
>>> +
>>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>>> +
>>> +	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
>>> +
>>> +	/* Establish our read mode with freq field set to 0 (HCLK/16) */
>>> +	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
>>> +
>>> +	/* Check if calibration data is suitable */
>>> +	if (!aspeed_smc_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
>>> +		dev_info(chip->nor.dev,
>>> +			 "Calibration area too uniform, using low speed");
>>> +		writel(chip->ctl_val[smc_read], chip->ctl);
>>> +		kfree(test_buf);
>>> +		return 0;
>>> +	}
>>> +
>>> +	/* Now we iterate the HCLK dividers until we find our breaking point */
>>> +	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs); i > 0; i--) {
>>> +		u32 tv, freq;
>>> +
>>> +		/* Compare timing to max */
>>> +		freq = ahb_freq / i;
>>> +		if (freq > max_freq)
>>> +			continue;
>>> +
>>> +		/* Set the timing */
>>> +		tv = chip->ctl_val[smc_read] | ASPEED_SMC_HCLK_DIV(i);
>>> +		writel(tv, chip->ctl);
>>> +		dev_dbg(chip->nor.dev, "Trying HCLK/%d...", i);
>>> +		rc = aspeed_smc_calibrate_reads(chip, i, golden_buf, test_buf);
>>> +		if (rc == 0)
>>> +			best_div = i;
>>> +	}
>>> +	kfree(test_buf);
>>> +
>>> +	/* Nothing found ? */
>>> +	if (best_div < 0) {
>>> +		dev_warn(chip->nor.dev, "No good frequency, using dumb slow");
>>> +	} else {
>>> +		dev_dbg(chip->nor.dev, "Found good read timings at HCLK/%d",
>>> +			best_div);
>>> +		chip->ctl_val[smc_read] |= ASPEED_SMC_HCLK_DIV(best_div);
>>> +	}
>>> +
>>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>>> +	return 0;
>>> +}
>>
>>
> 

[PATCH 04/16] mtd: spi-nor: aspeed: Add read training

[Cédric Le Goater]

On 11/10/2019 14:28, Boris Brezillon wrote:
> On Fri,  4 Oct 2019 13:59:07 +0200
> C?dric Le Goater <clg@kaod.org> wrote:
> 
>> +#define ASPEED_SMC_HCLK_DIV(i) \
>> +	(aspeed_smc_hclk_divs[(i) - 1] << CONTROL_CLOCK_FREQ_SEL_SHIFT)
>> +
>> +static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
>> +{
>> +	/*
>> +	 * Keep the 4Byte address mode on the AST2400 SPI controller.
>> +	 * Other controllers set the 4Byte mode in the CE Control
>> +	 * Register
>> +	 */
>> +	u32 ctl_mask = chip->controller->info == &spi_2400_info ?
>> +		 CONTROL_IO_ADDRESS_4B : 0;
>> +
>> +	return (chip->ctl_val[smc_read] & ctl_mask) |
>> +		(0x00 << 28) | /* Single bit */
>> +		(0x00 << 24) | /* CE# max */
>> +		(0x03 << 16) | /* use normal reads */
>> +		(0x00 <<  8) | /* HCLK/16 */
>> +		(0x00 <<  6) | /* no dummy cycle */
>> +		(0x00);        /* normal mode */
> 
> IIUC, you're using a SPINOR_OP_READ operation to read the golden
> buffer, and if I'm right, you start reading at offset 0 of the dirmap
> window (offset 0 in the flash), so basically the first block in the NOR.

Yes.

> What happens if this block is erased? In that case your golden buf will
> contain only 0xff values, and the read calibration is likely to be
> useless 

yes. that is why we have the aspeed_smc_check_calib_data() routine to
check that the data read makes some sense. If this is not the case, then :

	 "Calibration area too uniform, using low speed"

> (how can you determine if timings are good when IO pins always
> stay high). Don't we have a command that return non-ff/non-0 data while
> still being predictable and immutable? 

Not that I know of on these controllers.

> Do you expect users to always
> flash a pattern that helps calibrating those delays?

This is the case on the OpenBMC systems, AFAICT. 

u-boot.bin should be the data read on the FMC controller and the 
SPI controller contains the host Firmware which is as random.   

> 
>> +}
>> +
>> +static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
>> +				    u32 max_freq)
>> +{
>> +	u8 *golden_buf, *test_buf;
>> +	int i, rc, best_div = -1;
>> +	u32 save_read_val = chip->ctl_val[smc_read];
>> +	u32 ahb_freq = chip->controller->clk_frequency;
>> +
>> +	dev_dbg(chip->nor.dev, "AHB frequency: %d MHz", ahb_freq / 1000000);
>> +
>> +	test_buf = kmalloc(CALIBRATE_BUF_SIZE * 2, GFP_KERNEL);
>> +	golden_buf = test_buf + CALIBRATE_BUF_SIZE;
>> +
>> +	/* We start with the dumbest setting (keep 4Byte bit) and read
>> +	 * some data
>> +	 */
>> +	chip->ctl_val[smc_read] = aspeed_smc_default_read(chip);
>> +
>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>> +
>> +	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
>> +
>> +	/* Establish our read mode with freq field set to 0 (HCLK/16) */
>> +	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
>> +
>> +	/* Check if calibration data is suitable */
>> +	if (!aspeed_smc_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
>> +		dev_info(chip->nor.dev,
>> +			 "Calibration area too uniform, using low speed");
>> +		writel(chip->ctl_val[smc_read], chip->ctl);
>> +		kfree(test_buf);
>> +		return 0;
>> +	}
>> +
>> +	/* Now we iterate the HCLK dividers until we find our breaking point */
>> +	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs); i > 0; i--) {
>> +		u32 tv, freq;
>> +
>> +		/* Compare timing to max */
>> +		freq = ahb_freq / i;
>> +		if (freq > max_freq)
>> +			continue;
>> +
>> +		/* Set the timing */
>> +		tv = chip->ctl_val[smc_read] | ASPEED_SMC_HCLK_DIV(i);
>> +		writel(tv, chip->ctl);
>> +		dev_dbg(chip->nor.dev, "Trying HCLK/%d...", i);
>> +		rc = aspeed_smc_calibrate_reads(chip, i, golden_buf, test_buf);
>> +		if (rc == 0)
>> +			best_div = i;
>> +	}
>> +	kfree(test_buf);
>> +
>> +	/* Nothing found ? */
>> +	if (best_div < 0) {
>> +		dev_warn(chip->nor.dev, "No good frequency, using dumb slow");
>> +	} else {
>> +		dev_dbg(chip->nor.dev, "Found good read timings at HCLK/%d",
>> +			best_div);
>> +		chip->ctl_val[smc_read] |= ASPEED_SMC_HCLK_DIV(best_div);
>> +	}
>> +
>> +	writel(chip->ctl_val[smc_read], chip->ctl);
>> +	return 0;
>> +}
> 
> 

[PATCH 04/16] mtd: spi-nor: aspeed: Add read training

[Boris Brezillon]

On Fri, 11 Oct 2019 15:55:25 +0200
C?dric Le Goater <clg@kaod.org> wrote:

 
> > (how can you determine if timings are good when IO pins always
> > stay high). Don't we have a command that return non-ff/non-0 data while
> > still being predictable and immutable?   
> 
> Not that I know of on these controllers.

It's not really a controller thing, more a chip thing. The ideal
solution would be to have a loopback mode or an internal SRAM you can
write then read back, but AFAICT it doesn't exists. There's the SFDP
table as Vignesh mentioned, but we have the following problems:

1/ it might be too small (definitely < 16k)
2/ some NORs don't support SFDP (maybe not the ones we care about
   though)

[PATCH 04/16] mtd: spi-nor: aspeed: Add read training

[Cédric Le Goater]

On 11/10/2019 16:29, Boris Brezillon wrote:
> On Fri, 11 Oct 2019 15:55:25 +0200
> C?dric Le Goater <clg@kaod.org> wrote:
> 
>  
>>> (how can you determine if timings are good when IO pins always
>>> stay high). Don't we have a command that return non-ff/non-0 data while
>>> still being predictable and immutable?   
>>
>> Not that I know of on these controllers.
> 
> It's not really a controller thing, more a chip thing. The ideal
> solution would be to have a loopback mode or an internal SRAM you can
> write then read back, but AFAICT it doesn't exists. There's the SFDP
> table as Vignesh mentioned, but we have the following problems:
> 
> 1/ it might be too small (definitely < 16k)
> 2/ some NORs don't support SFDP (maybe not the ones we care about
>    though)


Yes. The approach we follow has good results, once the data is 
qualified as good enough for the training. 

We had some issues back in 2014 with some chips on early systems 
and I think we could reduce the amount of the data read and the 
number of loops now. 

Thanks,

C. 

[PATCH 05/16] mtd: spi-nor: aspeed: Limit the maximum SPI frequency

[Cédric Le Goater]

The read training algorithm can choose a 100MHz SPI frequency which
might be a bit too high for dual output IO on some chips, for the
W25Q256 on palmetto for instance. The MX66L1G45G on witherspoon should
be fine though. Also, the second chip of the FMC controller does not
get any optimize settings for reads. Only the first is configured by
U-Boot.

To fix these two issues, we introduce a "spi-max-frequency" property
in the device tree which will be used to cap the read training
algorithm. It is now considered safe to run the read training on the
FMC controller chips as well.

By default, the frequency setting is 50MHz.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 155c407c2bdf..1c1822a13407 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -58,6 +58,7 @@ static const struct aspeed_smc_info fmc_2400_info = {
 	.ctl0 = 0x10,
 	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
+	.optimize_read = aspeed_smc_optimize_read,
 };
 
 static const struct aspeed_smc_info spi_2400_info = {
@@ -79,6 +80,7 @@ static const struct aspeed_smc_info fmc_2500_info = {
 	.ctl0 = 0x10,
 	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
+	.optimize_read = aspeed_smc_optimize_read,
 };
 
 static const struct aspeed_smc_info spi_2500_info = {
@@ -110,6 +112,7 @@ struct aspeed_smc_chip {
 	u32 ctl_val[smc_max];			/* control settings */
 	enum aspeed_smc_flash_type type;	/* what type of flash */
 	struct spi_nor nor;
+	u32 clk_rate;
 };
 
 struct aspeed_smc_controller {
@@ -126,6 +129,8 @@ struct aspeed_smc_controller {
 	struct aspeed_smc_chip *chips[0];	/* pointers to attached chips */
 };
 
+#define ASPEED_SPI_DEFAULT_FREQ		50000000
+
 /*
  * SPI Flash Configuration Register (AST2500 SPI)
  *     or
@@ -1002,11 +1007,8 @@ static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 	dev_info(controller->dev, "read control register: %08x\n",
 		 chip->ctl_val[smc_read]);
 
-	/*
-	 * TODO: get max freq from chip
-	 */
 	if (optimize_read && info->optimize_read)
-		info->optimize_read(chip, 104000000);
+		info->optimize_read(chip, chip->clk_rate);
 	return 0;
 }
 
@@ -1060,6 +1062,13 @@ static int aspeed_smc_setup_flash(struct aspeed_smc_controller *controller,
 			break;
 		}
 
+		if (of_property_read_u32(child, "spi-max-frequency",
+					 &chip->clk_rate)) {
+			chip->clk_rate = ASPEED_SPI_DEFAULT_FREQ;
+		}
+		dev_info(dev, "Using %d MHz SPI frequency\n",
+			 chip->clk_rate / 1000000);
+
 		chip->controller = controller;
 		chip->ctl = controller->regs + info->ctl0 + cs * 4;
 		chip->cs = cs;
-- 
2.21.0

[PATCH 06/16] mtd: spi-nor: fix options for mx66l51235f

[Cédric Le Goater]

From: Alexander Soldatov <a.soldatov@yadro.com>

Currently in driver spi-nor there is a line for mx66l51235l.
According to Macronix site there is no such part number.
The chip detected as such is actually mx66l51235f.

According to the datasheet for mx66l51235f,
"The device default is in 24-bit address mode" (section 9-10).
Hence we removed SPI_NOR_4B_OPCODES option with this commit.

OpenBMC-Staging-Count: 7
Fixes: d342b6a973af ("mtd: spi-nor: enable 4B opcodes for mx66l51235l")
Cc: Alexander Amelkin <a.amelkin@yadro.com>
Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
Reviewed-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Lei YU <mine260309@gmail.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/spi-nor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 1d8621d43160..b1165673cd93 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2294,7 +2294,7 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "mx25v8035f",  INFO(0xc22314, 0, 64 * 1024,  16,
 			 SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
-	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
+	{ "mx66l51235f", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "mx66u51235f", INFO(0xc2253a, 0, 64 * 1024, 1024, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
 	{ "mx66l1g45g",  INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "mx66l1g55g",  INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
-- 
2.21.0

[PATCH 06/16] mtd: spi-nor: fix options for mx66l51235f

[Cédric Le Goater]

On 04/10/2019 13:59, C?dric Le Goater wrote:
> From: Alexander Soldatov <a.soldatov@yadro.com>
> 
> Currently in driver spi-nor there is a line for mx66l51235l.
> According to Macronix site there is no such part number.
> The chip detected as such is actually mx66l51235f.
> 
> According to the datasheet for mx66l51235f,
> "The device default is in 24-bit address mode" (section 9-10).
> Hence we removed SPI_NOR_4B_OPCODES option with this commit.

This patch has been discussed already on the list and it was decided 
that it should not be merged. Please drop it. 

Sorry for the noise,

C.  

> OpenBMC-Staging-Count: 7
> Fixes: d342b6a973af ("mtd: spi-nor: enable 4B opcodes for mx66l51235l")
> Cc: Alexander Amelkin <a.amelkin@yadro.com>
> Signed-off-by: Alexander Soldatov <a.soldatov@yadro.com>
> Reviewed-by: C?dric Le Goater <clg@kaod.org>
> Reviewed-by: Lei YU <mine260309@gmail.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: C?dric Le Goater <clg@kaod.org>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 1d8621d43160..b1165673cd93 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -2294,7 +2294,7 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ "mx25v8035f",  INFO(0xc22314, 0, 64 * 1024,  16,
>  			 SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
> -	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> +	{ "mx66l51235f", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "mx66u51235f", INFO(0xc2253a, 0, 64 * 1024, 1024, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>  	{ "mx66l1g45g",  INFO(0xc2201b, 0, 64 * 1024, 2048, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>  	{ "mx66l1g55g",  INFO(0xc2261b, 0, 64 * 1024, 2048, SPI_NOR_QUAD_READ) },
> 

[PATCH 07/16] mtd: spi-nor: aspeed: Add support for the 4B opcodes

[Cédric Le Goater]

Switch the default controller value to use the read mode in order to
customize the command and use SPINOR_OP_READ_4B (0x13) when the chip
supports 4B opcodes.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 1c1822a13407..add95a9aca76 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -888,14 +888,21 @@ static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
 	 */
 	u32 ctl_mask = chip->controller->info == &spi_2400_info ?
 		 CONTROL_IO_ADDRESS_4B : 0;
+	u8 cmd = chip->nor.flags & SNOR_F_4B_OPCODES ? SPINOR_OP_READ_4B :
+		SPINOR_OP_READ;
 
+	/*
+	 * Use the "read command" mode to customize the opcode. In
+	 * normal command mode, the value is necessarily READ (0x3) on
+	 * the AST2400/2500 SoCs.
+	 */
 	return (chip->ctl_val[smc_read] & ctl_mask) |
 		(0x00 << 28) | /* Single bit */
 		(0x00 << 24) | /* CE# max */
-		(0x03 << 16) | /* use normal reads */
+		(cmd  << 16) | /* use read mode to support 4B opcode */
 		(0x00 <<  8) | /* HCLK/16 */
 		(0x00 <<  6) | /* no dummy cycle */
-		(0x00);        /* normal mode */
+		(0x01);        /* read mode */
 }
 
 static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
-- 
2.21.0

[PATCH 08/16] mtd: spi-nor: Add support for w25q512jv

[Cédric Le Goater]

DUAL and QUAD are supported :

  https://www.winbond.com/resource-files/W25Q512JV%20SPI%20RevB%2006252019%20KMS.pdf

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/spi-nor.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index b1165673cd93..e8beb671ef88 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2482,6 +2482,9 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "w25q256jvm", INFO(0xef7019, 0, 64 * 1024, 512,
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{ "w25q512jv", INFO(0xef4020, 0, 64 * 1024, 1024,
+			    SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			    SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
 	{ "w25m512jv", INFO(0xef7119, 0, 64 * 1024, 1024,
 			SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_DUAL_READ) },
 
-- 
2.21.0

[PATCH 09/16] mtd: spi-nor: aspeed: Introduce a field for the AHB physical address

[Cédric Le Goater]

On the AST2600, we will use this field to compute the address of the
chip AHB window from the Segment Register value. It also removes the
need of aspeed_smc_ahb_base_phy() helper.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index add95a9aca76..c5a0c8d94371 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -121,7 +121,8 @@ struct aspeed_smc_controller {
 	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 */
+	void __iomem *ahb_base;			/* per-chip window resource */
+	u32 ahb_base_phy;			/* phys addr of AHB window  */
 	u32 ahb_window_size;			/* full mapping window size */
 
 	unsigned long	clk_frequency;
@@ -533,21 +534,13 @@ static void __iomem *aspeed_smc_chip_base(struct aspeed_smc_chip *chip,
 	return controller->ahb_base + offset;
 }
 
-static u32 aspeed_smc_ahb_base_phy(struct aspeed_smc_controller *controller)
-{
-	u32 seg0_val = readl(SEGMENT_ADDR_REG(controller, 0));
-
-	return SEGMENT_ADDR_START(seg0_val);
-}
-
 static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
 			    u32 size)
 {
 	struct aspeed_smc_controller *controller = chip->controller;
 	void __iomem *seg_reg;
-	u32 seg_oldval, seg_newval, ahb_base_phy, end;
-
-	ahb_base_phy = aspeed_smc_ahb_base_phy(controller);
+	u32 seg_oldval, seg_newval, end;
+	u32 ahb_base_phy = controller->ahb_base_phy;
 
 	seg_reg = SEGMENT_ADDR_REG(controller, cs);
 	seg_oldval = readl(seg_reg);
@@ -636,7 +629,7 @@ static u32 aspeed_smc_chip_set_segment(struct aspeed_smc_chip *chip)
 			 chip->cs, size >> 20);
 	}
 
-	ahb_base_phy = aspeed_smc_ahb_base_phy(controller);
+	ahb_base_phy = controller->ahb_base_phy;
 
 	/*
 	 * As a start address for the current segment, use the default
@@ -1158,6 +1151,7 @@ static int aspeed_smc_probe(struct platform_device *pdev)
 		return PTR_ERR(controller->regs);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	controller->ahb_base_phy = res->start;
 	controller->ahb_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(controller->ahb_base))
 		return PTR_ERR(controller->ahb_base);
-- 
2.21.0

[PATCH 10/16] mtd: spi-nor: aspeed: Introduce segment operations

[Cédric Le Goater]

AST2600 will use a different encoding for the addresses defined in the
Segment Register.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 78 ++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index c5a0c8d94371..7cdd84a2ca82 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -32,6 +32,7 @@ enum aspeed_smc_flash_type {
 };
 
 struct aspeed_smc_chip;
+struct aspeed_smc_controller;
 
 struct aspeed_smc_info {
 	u32 maxsize;		/* maximum size of chip window */
@@ -43,6 +44,10 @@ struct aspeed_smc_info {
 
 	void (*set_4b)(struct aspeed_smc_chip *chip);
 	int (*optimize_read)(struct aspeed_smc_chip *chip, u32 max_freq);
+	u32 (*segment_start)(struct aspeed_smc_controller *controller, u32 reg);
+	u32 (*segment_end)(struct aspeed_smc_controller *controller, u32 reg);
+	u32 (*segment_reg)(struct aspeed_smc_controller *controller,
+			   u32 start, u32 end);
 };
 
 static void aspeed_smc_chip_set_4b_spi_2400(struct aspeed_smc_chip *chip);
@@ -50,6 +55,13 @@ static void aspeed_smc_chip_set_4b(struct aspeed_smc_chip *chip);
 static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
 				    u32 max_freq);
 
+static u32 aspeed_smc_segment_start(struct aspeed_smc_controller *controller,
+				    u32 reg);
+static u32 aspeed_smc_segment_end(struct aspeed_smc_controller *controller,
+				  u32 reg);
+static u32 aspeed_smc_segment_reg(struct aspeed_smc_controller *controller,
+				  u32 start, u32 end);
+
 static const struct aspeed_smc_info fmc_2400_info = {
 	.maxsize = 64 * 1024 * 1024,
 	.nce = 5,
@@ -59,6 +71,9 @@ static const struct aspeed_smc_info fmc_2400_info = {
 	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
+	.segment_start = aspeed_smc_segment_start,
+	.segment_end = aspeed_smc_segment_end,
+	.segment_reg = aspeed_smc_segment_reg,
 };
 
 static const struct aspeed_smc_info spi_2400_info = {
@@ -70,6 +85,7 @@ static const struct aspeed_smc_info spi_2400_info = {
 	.timing = 0x14,
 	.set_4b = aspeed_smc_chip_set_4b_spi_2400,
 	.optimize_read = aspeed_smc_optimize_read,
+	/* No segment registers */
 };
 
 static const struct aspeed_smc_info fmc_2500_info = {
@@ -81,6 +97,9 @@ static const struct aspeed_smc_info fmc_2500_info = {
 	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
+	.segment_start = aspeed_smc_segment_start,
+	.segment_end = aspeed_smc_segment_end,
+	.segment_reg = aspeed_smc_segment_reg,
 };
 
 static const struct aspeed_smc_info spi_2500_info = {
@@ -92,6 +111,9 @@ static const struct aspeed_smc_info spi_2500_info = {
 	.timing = 0x94,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
+	.segment_start = aspeed_smc_segment_start,
+	.segment_end = aspeed_smc_segment_end,
+	.segment_reg = aspeed_smc_segment_reg,
 };
 
 enum aspeed_smc_ctl_reg_value {
@@ -201,22 +223,34 @@ struct aspeed_smc_controller {
 	(CONTROL_AAF_MODE | CONTROL_CE_INACTIVE_MASK | CONTROL_CLK_DIV4 | \
 	 CONTROL_CLOCK_FREQ_SEL_MASK | CONTROL_LSB_FIRST | CONTROL_CLOCK_MODE_3)
 
-/*
- * 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    |
- */
 #define SEGMENT_ADDR_REG0		0x30
-#define SEGMENT_ADDR_START(_r)		((((_r) >> 16) & 0xFF) << 23)
-#define SEGMENT_ADDR_END(_r)		((((_r) >> 24) & 0xFF) << 23)
-#define SEGMENT_ADDR_VALUE(start, end)					\
-	(((((start) >> 23) & 0xFF) << 16) | ((((end) >> 23) & 0xFF) << 24))
 #define SEGMENT_ADDR_REG(controller, cs)	\
 	((controller)->regs + SEGMENT_ADDR_REG0 + (cs) * 4)
 
+/*
+ * The Segment Registers of the AST2400 and AST2500 have a 8MB
+ * unit. The address range of a flash SPI slave is encoded with
+ * absolute addresses which should be part of the overall controller
+ * window.
+ */
+static u32 aspeed_smc_segment_start(struct aspeed_smc_controller *controller,
+				    u32 reg)
+{
+	return ((reg >> 16) & 0xFF) << 23;
+}
+
+static u32 aspeed_smc_segment_end(struct aspeed_smc_controller *controller,
+				  u32 reg)
+{
+	return ((reg >> 24) & 0xFF) << 23;
+}
+
+static u32 aspeed_smc_segment_reg(struct aspeed_smc_controller *controller,
+				  u32 start, u32 end)
+{
+	return (((start >> 23) & 0xFF) << 16) | (((end >> 23) & 0xFF) << 24);
+}
+
 /*
  * Switch to turn off read optimisation if needed
  */
@@ -519,16 +553,19 @@ static void __iomem *aspeed_smc_chip_base(struct aspeed_smc_chip *chip,
 					  struct resource *res)
 {
 	struct aspeed_smc_controller *controller = chip->controller;
+	const struct aspeed_smc_info *info = controller->info;
 	u32 offset = 0;
 	u32 reg;
 
-	if (controller->info->nce > 1) {
+	if (info->nce > 1) {
 		reg = readl(SEGMENT_ADDR_REG(controller, chip->cs));
 
-		if (SEGMENT_ADDR_START(reg) >= SEGMENT_ADDR_END(reg))
+		if (info->segment_start(controller, reg) >=
+		    info->segment_end(controller, reg)) {
 			return NULL;
+		}
 
-		offset = SEGMENT_ADDR_START(reg) - res->start;
+		offset = info->segment_start(controller, reg) - res->start;
 	}
 
 	return controller->ahb_base + offset;
@@ -538,6 +575,7 @@ static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
 			    u32 size)
 {
 	struct aspeed_smc_controller *controller = chip->controller;
+	const struct aspeed_smc_info *info = controller->info;
 	void __iomem *seg_reg;
 	u32 seg_oldval, seg_newval, end;
 	u32 ahb_base_phy = controller->ahb_base_phy;
@@ -551,7 +589,7 @@ static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
 	 * previous segment
 	 */
 	if (!size)
-		size = SEGMENT_ADDR_END(seg_oldval) - start;
+		size = info->segment_end(controller, seg_oldval) - start;
 
 	/*
 	 * The segment cannot exceed the maximum window size of the
@@ -564,7 +602,7 @@ static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
 	}
 
 	end = start + size;
-	seg_newval = SEGMENT_ADDR_VALUE(start, end);
+	seg_newval = info->segment_reg(controller, start, end);
 	writel(seg_newval, seg_reg);
 
 	/*
@@ -575,8 +613,8 @@ static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
 	if (seg_newval != readl(seg_reg)) {
 		dev_err(chip->nor.dev, "CE%d window invalid", cs);
 		writel(seg_oldval, seg_reg);
-		start = SEGMENT_ADDR_START(seg_oldval);
-		end = SEGMENT_ADDR_END(seg_oldval);
+		start = info->segment_start(controller, seg_oldval);
+		end = info->segment_end(controller, seg_oldval);
 		size = end - start;
 	}
 
@@ -639,7 +677,7 @@ static u32 aspeed_smc_chip_set_segment(struct aspeed_smc_chip *chip)
 	if (chip->cs) {
 		u32 prev = readl(SEGMENT_ADDR_REG(controller, chip->cs - 1));
 
-		start = SEGMENT_ADDR_END(prev);
+		start = controller->info->segment_end(controller, prev);
 	} else {
 		start = ahb_base_phy;
 	}
-- 
2.21.0

[PATCH 11/16] dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600

[Cédric Le Goater]

The SMC controllers on the AST2600 SoC are very similar to the the
AST2500. The SoC has one Firmware Memory Controller and two SPI flash
memory controllers.

Cc: Rob Herring <robh@kernel.org>
Cc: devicetree at vger.kernel.org
Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 Documentation/devicetree/bindings/mtd/aspeed-smc.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/aspeed-smc.txt b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
index 49f6528ef547..c2373d9cfd90 100644
--- a/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
+++ b/Documentation/devicetree/bindings/mtd/aspeed-smc.txt
@@ -14,6 +14,8 @@ Required properties:
 	"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
+	"aspeed,ast2600-fmc" for the AST2600 Firmware Memory Controller
+	"aspeed,ast2600-spi" for the AST2600 SPI flash memory controllers
 
   - reg : the first contains the control register location and length,
           the second contains the memory window mapping address and length
-- 
2.21.0

[PATCH 11/16] dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600

[Rob Herring]

On Fri,  4 Oct 2019 13:59:14 +0200, =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= wrote:
> The SMC controllers on the AST2600 SoC are very similar to the the
> AST2500. The SoC has one Firmware Memory Controller and two SPI flash
> memory controllers.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree at vger.kernel.org
> Signed-off-by: C?dric Le Goater <clg@kaod.org>
> ---
>  Documentation/devicetree/bindings/mtd/aspeed-smc.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH 12/16] mtd: spi-nor: aspeed: Add initial support for the AST2600

[Cédric Le Goater]

The HW interface of AST2600 SoC SMC controllers is very similar to the
the AST2500. The AST2600 Firmware Memory Controller is now SPI only.

The Segment Registers also have a different encoding. A 1MB unit is
used and the address range of a flash SPI device is encoded with
offsets in the overall controller window. The previous SoC AST2400 and
AST2500 used absolute addresses. Only bits [27:20] are relevant and
the end address is an upper bound limit.

Read training yet to come.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 68 ++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 7cdd84a2ca82..c977f8f28aef 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -116,6 +116,39 @@ static const struct aspeed_smc_info spi_2500_info = {
 	.segment_reg = aspeed_smc_segment_reg,
 };
 
+static u32 aspeed_smc_segment_start_ast2600(struct aspeed_smc_controller *ctrl,
+					    u32 reg);
+static u32 aspeed_smc_segment_end_ast2600(struct aspeed_smc_controller *ctrl,
+					  u32 reg);
+static u32 aspeed_smc_segment_reg_ast2600(struct aspeed_smc_controller *ctrl,
+					  u32 start, u32 end);
+
+static const struct aspeed_smc_info fmc_2600_info = {
+	.maxsize = 256 * 1024 * 1024,
+	.nce = 3,
+	.hastype = false, /* SPI Only */
+	.we0 = 16,
+	.ctl0 = 0x10,
+	.timing = 0x94,
+	.set_4b = aspeed_smc_chip_set_4b,
+	.segment_start = aspeed_smc_segment_start_ast2600,
+	.segment_end = aspeed_smc_segment_end_ast2600,
+	.segment_reg = aspeed_smc_segment_reg_ast2600,
+};
+
+static const struct aspeed_smc_info spi_2600_info = {
+	.maxsize = 256 * 1024 * 1024,
+	.nce = 2,
+	.hastype = false,
+	.we0 = 16,
+	.ctl0 = 0x10,
+	.timing = 0x94,
+	.set_4b = aspeed_smc_chip_set_4b,
+	.segment_start = aspeed_smc_segment_start_ast2600,
+	.segment_end = aspeed_smc_segment_end_ast2600,
+	.segment_reg = aspeed_smc_segment_reg_ast2600,
+};
+
 enum aspeed_smc_ctl_reg_value {
 	smc_base,		/* base value without mode for other commands */
 	smc_read,		/* command reg for (maybe fast) reads */
@@ -251,6 +284,39 @@ static u32 aspeed_smc_segment_reg(struct aspeed_smc_controller *controller,
 	return (((start >> 23) & 0xFF) << 16) | (((end >> 23) & 0xFF) << 24);
 }
 
+/*
+ * The Segment Registers of the AST2600 have a 1MB unit. The address
+ * range of a flash SPI slave is encoded with offsets in the overall
+ * controller window. The previous SoC AST2400 and AST2500 used
+ * absolute addresses. Only bits [27:20] are relevant and the end
+ * address is an upper bound limit.
+ */
+
+#define AST2600_SEG_ADDR_MASK 0x0ff00000
+
+static u32 aspeed_smc_segment_start_ast2600(struct aspeed_smc_controller *ctlr,
+					    u32 reg)
+{
+	u32 start_offset = (reg << 16) & AST2600_SEG_ADDR_MASK;
+
+	return ctlr->ahb_base_phy + start_offset;
+}
+
+static u32 aspeed_smc_segment_end_ast2600(struct aspeed_smc_controller *ctlr,
+					  u32 reg)
+{
+	u32 end_offset = reg & AST2600_SEG_ADDR_MASK;
+
+	return ctlr->ahb_base_phy + end_offset + 0x100000;
+}
+
+static u32 aspeed_smc_segment_reg_ast2600(struct aspeed_smc_controller *ctlr,
+					  u32 start, u32 end)
+{
+	return ((start & AST2600_SEG_ADDR_MASK) >> 16) |
+		((end - 1) & AST2600_SEG_ADDR_MASK);
+}
+
 /*
  * Switch to turn off read optimisation if needed
  */
@@ -538,6 +604,8 @@ static const struct of_device_id aspeed_smc_matches[] = {
 	{ .compatible = "aspeed,ast2400-spi", .data = &spi_2400_info },
 	{ .compatible = "aspeed,ast2500-fmc", .data = &fmc_2500_info },
 	{ .compatible = "aspeed,ast2500-spi", .data = &spi_2500_info },
+	{ .compatible = "aspeed,ast2600-fmc", .data = &fmc_2600_info },
+	{ .compatible = "aspeed,ast2600-spi", .data = &spi_2600_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, aspeed_smc_matches);
-- 
2.21.0

[PATCH 13/16] mtd: spi-nor: aspeed: Check for disabled segments on the AST2600

[Cédric Le Goater]

On the AST2600, the segment of a flash SPI device can be disabled with
zero register value. By default, the CS0 AHB window is open but the
other CS are not. This is closing the access to the flash device in
user mode and also forbids scanning. For multiple CS, we will need
firmware or a DT property to reopen the flash AHB window.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index c977f8f28aef..fad08738e534 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -307,12 +307,20 @@ static u32 aspeed_smc_segment_end_ast2600(struct aspeed_smc_controller *ctlr,
 {
 	u32 end_offset = reg & AST2600_SEG_ADDR_MASK;
 
+	/* segment is disabled */
+	if (!end_offset)
+		return ctlr->ahb_base_phy;
+
 	return ctlr->ahb_base_phy + end_offset + 0x100000;
 }
 
 static u32 aspeed_smc_segment_reg_ast2600(struct aspeed_smc_controller *ctlr,
 					  u32 start, u32 end)
 {
+	/* disable zero size segments */
+	if (start == end)
+		return 0;
+
 	return ((start & AST2600_SEG_ADDR_MASK) >> 16) |
 		((end - 1) & AST2600_SEG_ADDR_MASK);
 }
@@ -656,8 +664,15 @@ static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
 	 * size, but take into account the possible overlap with the
 	 * previous segment
 	 */
-	if (!size)
-		size = info->segment_end(controller, seg_oldval) - start;
+	if (!size) {
+		end = info->segment_end(controller, seg_oldval);
+
+		/*
+		 * Check for disabled segment (AST2600).
+		 */
+		if (end != ahb_base_phy)
+			size = end - start;
+	}
 
 	/*
 	 * The segment cannot exceed the maximum window size of the
@@ -686,8 +701,8 @@ static u32 chip_set_segment(struct aspeed_smc_chip *chip, u32 cs, u32 start,
 		size = end - start;
 	}
 
-	dev_info(chip->nor.dev, "CE%d window [ 0x%.8x - 0x%.8x ] %dMB",
-		 cs, start, end, size >> 20);
+	dev_info(chip->nor.dev, "CE%d window [ 0x%.8x - 0x%.8x ] %dMB%s",
+		 cs, start, end, size >> 20, size ? "" : " (disabled)");
 
 	return size;
 }
-- 
2.21.0

[PATCH 14/16] mtd: spi-nor: aspeed: Introduce training operations per platform

[Cédric Le Goater]

The read timing compensation register records the read delay settings
for a range of HCLK. Its encoding is different on the AST2600 and the
read training will be slightly more complex.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index fad08738e534..85b7ff3bcc91 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -41,9 +41,13 @@ struct aspeed_smc_info {
 	u8 we0;			/* shift for write enable bit for CE0 */
 	u8 ctl0;		/* offset in regs of ctl for CE0 */
 	u8 timing;		/* offset in regs of timing */
+	u32 hdiv_max;           /* Max HCLK divisor on read timing reg */
 
 	void (*set_4b)(struct aspeed_smc_chip *chip);
 	int (*optimize_read)(struct aspeed_smc_chip *chip, u32 max_freq);
+	int (*calibrate)(struct aspeed_smc_chip *chip, u32 hdiv,
+			 const u8 *golden_buf, u8 *test_buf);
+
 	u32 (*segment_start)(struct aspeed_smc_controller *controller, u32 reg);
 	u32 (*segment_end)(struct aspeed_smc_controller *controller, u32 reg);
 	u32 (*segment_reg)(struct aspeed_smc_controller *controller,
@@ -54,6 +58,8 @@ 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 int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
 				    u32 max_freq);
+static int aspeed_smc_calibrate_reads(struct aspeed_smc_chip *chip, u32 hdiv,
+				      const u8 *golden_buf, u8 *test_buf);
 
 static u32 aspeed_smc_segment_start(struct aspeed_smc_controller *controller,
 				    u32 reg);
@@ -69,8 +75,10 @@ static const struct aspeed_smc_info fmc_2400_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
+	.calibrate = aspeed_smc_calibrate_reads,
 	.segment_start = aspeed_smc_segment_start,
 	.segment_end = aspeed_smc_segment_end,
 	.segment_reg = aspeed_smc_segment_reg,
@@ -83,8 +91,10 @@ static const struct aspeed_smc_info spi_2400_info = {
 	.we0 = 0,
 	.ctl0 = 0x04,
 	.timing = 0x14,
+	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b_spi_2400,
 	.optimize_read = aspeed_smc_optimize_read,
+	.calibrate = aspeed_smc_calibrate_reads,
 	/* No segment registers */
 };
 
@@ -95,8 +105,10 @@ static const struct aspeed_smc_info fmc_2500_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
+	.calibrate = aspeed_smc_calibrate_reads,
 	.segment_start = aspeed_smc_segment_start,
 	.segment_end = aspeed_smc_segment_end,
 	.segment_reg = aspeed_smc_segment_reg,
@@ -109,8 +121,10 @@ static const struct aspeed_smc_info spi_2500_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
+	.calibrate = aspeed_smc_calibrate_reads,
 	.segment_start = aspeed_smc_segment_start,
 	.segment_end = aspeed_smc_segment_end,
 	.segment_reg = aspeed_smc_segment_reg,
@@ -1022,6 +1036,8 @@ static u32 aspeed_smc_default_read(struct aspeed_smc_chip *chip)
 static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
 				    u32 max_freq)
 {
+	struct aspeed_smc_controller *controller = chip->controller;
+	const struct aspeed_smc_info *info = controller->info;
 	u8 *golden_buf, *test_buf;
 	int i, rc, best_div = -1;
 	u32 save_read_val = chip->ctl_val[smc_read];
@@ -1054,7 +1070,8 @@ static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
 	}
 
 	/* Now we iterate the HCLK dividers until we find our breaking point */
-	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs); i > 0; i--) {
+	for (i = ARRAY_SIZE(aspeed_smc_hclk_divs);
+	     i > info->hdiv_max - 1; i--) {
 		u32 tv, freq;
 
 		/* Compare timing to max */
@@ -1065,8 +1082,8 @@ static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
 		/* Set the timing */
 		tv = chip->ctl_val[smc_read] | ASPEED_SMC_HCLK_DIV(i);
 		writel(tv, chip->ctl);
-		dev_dbg(chip->nor.dev, "Trying HCLK/%d...", i);
-		rc = aspeed_smc_calibrate_reads(chip, i, golden_buf, test_buf);
+		dev_dbg(chip->nor.dev, "Trying HCLK/%d [%08x] ...", i, tv);
+		rc = info->calibrate(chip, i, golden_buf, test_buf);
 		if (rc == 0)
 			best_div = i;
 	}
-- 
2.21.0

[PATCH 15/16] mtd: spi-nor: aspeed: Introduce a HCLK mask for training

[Cédric Le Goater]

The AST2600 handles more HCLK divisors than its predecessors.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 85b7ff3bcc91..5fa9956d183e 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -41,6 +41,7 @@ struct aspeed_smc_info {
 	u8 we0;			/* shift for write enable bit for CE0 */
 	u8 ctl0;		/* offset in regs of ctl for CE0 */
 	u8 timing;		/* offset in regs of timing */
+	u32 hclk_mask;          /* clock frequency mask in CEx Control reg */
 	u32 hdiv_max;           /* Max HCLK divisor on read timing reg */
 
 	void (*set_4b)(struct aspeed_smc_chip *chip);
@@ -75,6 +76,7 @@ static const struct aspeed_smc_info fmc_2400_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hclk_mask = 0xfffff0ff,
 	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
@@ -91,6 +93,7 @@ static const struct aspeed_smc_info spi_2400_info = {
 	.we0 = 0,
 	.ctl0 = 0x04,
 	.timing = 0x14,
+	.hclk_mask = 0xfffff0ff,
 	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b_spi_2400,
 	.optimize_read = aspeed_smc_optimize_read,
@@ -105,6 +108,7 @@ static const struct aspeed_smc_info fmc_2500_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hclk_mask = 0xfffff0ff,
 	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
@@ -121,6 +125,7 @@ static const struct aspeed_smc_info spi_2500_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hclk_mask = 0xfffff0ff,
 	.hdiv_max = 1,
 	.set_4b = aspeed_smc_chip_set_4b,
 	.optimize_read = aspeed_smc_optimize_read,
@@ -1058,7 +1063,7 @@ static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
 	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
 
 	/* Establish our read mode with freq field set to 0 (HCLK/16) */
-	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
+	chip->ctl_val[smc_read] = save_read_val & info->hclk_mask;
 
 	/* Check if calibration data is suitable */
 	if (!aspeed_smc_check_calib_data(golden_buf, CALIBRATE_BUF_SIZE)) {
-- 
2.21.0

[PATCH 16/16] mtd: spi-nor: aspeed: Add read training support for the AST2600

[Cédric Le Goater]

The read training algorithm consists of finding the appropriate read
timing delays for the HCLK dividers in range [ 2 - 5 ] and store the
results in the Read Timing Compensation register. The previous AST2500
and AST2400 SoCs were covering a broader HCLK range [ 1 - 5 ] because
the AHB frequency was lower.

The algorithm first reads a golden buffer at low speed and then
performs reads with different clocks and delay cycle settings to find
a breaking point. This selects the default clock frequency for the CEx
control register. The current settings are bit optimistic as we pick
the first delay giving good results. A safer approach would be to
determine an interval and choose the middle value. We might change the
approach depending on the results on other systems.

Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 drivers/mtd/spi-nor/aspeed-smc.c | 72 ++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/mtd/spi-nor/aspeed-smc.c b/drivers/mtd/spi-nor/aspeed-smc.c
index 5fa9956d183e..1176ad0c4fe5 100644
--- a/drivers/mtd/spi-nor/aspeed-smc.c
+++ b/drivers/mtd/spi-nor/aspeed-smc.c
@@ -141,6 +141,9 @@ static u32 aspeed_smc_segment_end_ast2600(struct aspeed_smc_controller *ctrl,
 					  u32 reg);
 static u32 aspeed_smc_segment_reg_ast2600(struct aspeed_smc_controller *ctrl,
 					  u32 start, u32 end);
+static int aspeed_smc_calibrate_reads_ast2600(struct aspeed_smc_chip *chip,
+					      u32 hdiv, const u8 *golden_buf,
+					      u8 *test_buf);
 
 static const struct aspeed_smc_info fmc_2600_info = {
 	.maxsize = 256 * 1024 * 1024,
@@ -149,7 +152,11 @@ static const struct aspeed_smc_info fmc_2600_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hclk_mask = 0xf0fff0ff,
+	.hdiv_max = 2,
 	.set_4b = aspeed_smc_chip_set_4b,
+	.optimize_read = aspeed_smc_optimize_read,
+	.calibrate = aspeed_smc_calibrate_reads_ast2600,
 	.segment_start = aspeed_smc_segment_start_ast2600,
 	.segment_end = aspeed_smc_segment_end_ast2600,
 	.segment_reg = aspeed_smc_segment_reg_ast2600,
@@ -162,7 +169,11 @@ static const struct aspeed_smc_info spi_2600_info = {
 	.we0 = 16,
 	.ctl0 = 0x10,
 	.timing = 0x94,
+	.hclk_mask = 0xf0fff0ff,
+	.hdiv_max = 2,
 	.set_4b = aspeed_smc_chip_set_4b,
+	.optimize_read = aspeed_smc_optimize_read,
+	.calibrate = aspeed_smc_calibrate_reads_ast2600,
 	.segment_start = aspeed_smc_segment_start_ast2600,
 	.segment_end = aspeed_smc_segment_end_ast2600,
 	.segment_reg = aspeed_smc_segment_reg_ast2600,
@@ -1107,6 +1118,67 @@ static int aspeed_smc_optimize_read(struct aspeed_smc_chip *chip,
 	return 0;
 }
 
+#define TIMING_DELAY_DI         BIT(3)
+#define TIMING_DELAY_HCYCLE_MAX     5
+#define TIMING_REG_AST2600(chip)					\
+	((chip)->controller->regs + (chip)->controller->info->timing +	\
+	 (chip)->cs * 4)
+
+static int aspeed_smc_calibrate_reads_ast2600(struct aspeed_smc_chip *chip,
+					      u32 hdiv, const u8 *golden_buf,
+					      u8 *test_buf)
+{
+	int hcycle;
+	u32 shift = (hdiv - 2) << 3;
+	u32 mask = ~(0xfu << shift);
+	u32 fread_timing_val = 0;
+
+	for (hcycle = 0; hcycle <= TIMING_DELAY_HCYCLE_MAX; hcycle++) {
+		int delay_ns;
+		bool pass = false;
+
+		fread_timing_val &= mask;
+		fread_timing_val |= hcycle << shift;
+
+		/* no DI input delay first  */
+		writel(fread_timing_val, TIMING_REG_AST2600(chip));
+		pass = aspeed_smc_check_reads(chip, golden_buf, test_buf);
+		dev_dbg(chip->nor.dev,
+			"  * [%08x] %d HCLK delay, DI delay none : %s",
+			fread_timing_val, hcycle, pass ? "PASS" : "FAIL");
+		if (pass)
+			return 0;
+
+		/* Add DI input delays  */
+		fread_timing_val &= mask;
+		fread_timing_val |= (TIMING_DELAY_DI | hcycle) << shift;
+
+		for (delay_ns = 0; delay_ns < 0x10; delay_ns++) {
+			fread_timing_val &= ~(0xf << (4 + shift));
+			fread_timing_val |= delay_ns << (4 + shift);
+
+			writel(fread_timing_val, TIMING_REG_AST2600(chip));
+			pass = aspeed_smc_check_reads(chip, golden_buf,
+						      test_buf);
+			dev_dbg(chip->nor.dev,
+				"  * [%08x] %d HCLK delay, DI delay %d.%dns : %s",
+				fread_timing_val, hcycle, (delay_ns + 1) / 2,
+				(delay_ns + 1) & 1 ? 5 : 5,
+				pass ? "PASS" : "FAIL");
+			/*
+			 * TODO: This is optimistic. We should look
+			 * for a working interval and save the middle
+			 * value in the read timing register.
+			 */
+			if (pass)
+				return 0;
+		}
+	}
+
+	/* No good setting for this frequency */
+	return -1;
+}
+
 static int aspeed_smc_chip_setup_finish(struct aspeed_smc_chip *chip)
 {
 	struct aspeed_smc_controller *controller = chip->controller;
-- 
2.21.0

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Boris Brezillon]

Hi Cedric,

On Fri,  4 Oct 2019 13:59:03 +0200
C?dric Le Goater <clg@kaod.org> wrote:

> Hello,
> 
> This series first extends the support for the Aspeed AST2500 and
> AST2400 SMC driver. It adds Dual Data support and read training giving
> the best read settings for a given chip. Support for the new AST2600
> SoC is added at the end.
> 
> I understand that a new spi_mem framework exists and I do have an
> experimental driver using it. But unfortunately, it is difficult to
> integrate the read training. The Aspeed constraints are not compatible
> and i haven't had the time to extend the current framework.

Hm, I don't think that's a good reason to push new features to the
existing driver, especially since I asked others to migrate their
drivers to spi-mem in the past. I do understand your concerns, and I'll
let the SPI NOR/MTD maintainers make the final call, but I think it'd
be better for the SPI MEM ecosystem to think about this link-training
API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
this kind of feature to spi-nor controller drivers.

> 
> This patchset has been in use for some time in the OpenBMC kernel on
> these systems :
> 
>  * OpenPOWER Palmetto (AST2400)
>  * Evaluation board (AST2500) 
>  * OpenPOWER Witherspoon (AST2500)
>  * OpenPOWER Romulus (AST2500)
>  * OpenPOWER Zaius (AST2500)
>    and many others
>  
> and it is now in use on these boards with the new SoC :
> 
>  * Evaluation board (AST2600) 
>  * Tacoma board (AST2600) 
> 
> Thanks,
> 
> C.
> 
> Alexander Soldatov (1):
>   mtd: spi-nor: fix options for mx66l51235f
> 
> C?dric Le Goater (15):
>   mtd: spi-nor: aspeed: Use command mode for reads
>   mtd: spi-nor: aspeed: Add support for SPI dual IO read mode
>   mtd: spi-nor: aspeed: Link controller with the ahb clock
>   mtd: spi-nor: aspeed: Add read training
>   mtd: spi-nor: aspeed: Limit the maximum SPI frequency
>   mtd: spi-nor: aspeed: Add support for the 4B opcodes
>   mtd: spi-nor: Add support for w25q512jv
>   mtd: spi-nor: aspeed: Introduce a field for the AHB physical address
>   mtd: spi-nor: aspeed: Introduce segment operations
>   dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600
>   mtd: spi-nor: aspeed: Add initial support for the AST2600
>   mtd: spi-nor: aspeed: Check for disabled segments on the AST2600
>   mtd: spi-nor: aspeed: Introduce training operations per platform
>   mtd: spi-nor: aspeed: Introduce a HCLK mask for training
>   mtd: spi-nor: aspeed: Add read training support for the AST2600
> 
>  drivers/mtd/spi-nor/aspeed-smc.c              | 593 ++++++++++++++++--
>  drivers/mtd/spi-nor/spi-nor.c                 |   5 +-
>  .../devicetree/bindings/mtd/aspeed-smc.txt    |   2 +
>  3 files changed, 551 insertions(+), 49 deletions(-)
> 

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Joel Stanley]

On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> Hi Cedric,
>
> On Fri,  4 Oct 2019 13:59:03 +0200
> C?dric Le Goater <clg@kaod.org> wrote:
>
> > Hello,
> >
> > This series first extends the support for the Aspeed AST2500 and
> > AST2400 SMC driver. It adds Dual Data support and read training giving
> > the best read settings for a given chip. Support for the new AST2600
> > SoC is added at the end.
> >
> > I understand that a new spi_mem framework exists and I do have an
> > experimental driver using it. But unfortunately, it is difficult to
> > integrate the read training. The Aspeed constraints are not compatible
> > and i haven't had the time to extend the current framework.
>
> Hm, I don't think that's a good reason to push new features to the
> existing driver, especially since I asked others to migrate their
> drivers to spi-mem in the past. I do understand your concerns, and I'll
> let the SPI NOR/MTD maintainers make the final call, but I think it'd
> be better for the SPI MEM ecosystem to think about this link-training
> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
> this kind of feature to spi-nor controller drivers.

As Cedric mentioned, the OpenBMC project has been shipping the read
training code for the ast2400/ast2400 for several years now. It would
be great to see it in mainline.

I think it's reasonable to ask for the driver to be moved to the
spi-mem subsystem once it has the required APIs.

Cheers,

Joel


>
> >
> > This patchset has been in use for some time in the OpenBMC kernel on
> > these systems :
> >
> >  * OpenPOWER Palmetto (AST2400)
> >  * Evaluation board (AST2500)
> >  * OpenPOWER Witherspoon (AST2500)
> >  * OpenPOWER Romulus (AST2500)
> >  * OpenPOWER Zaius (AST2500)
> >    and many others
> >
> > and it is now in use on these boards with the new SoC :
> >
> >  * Evaluation board (AST2600)
> >  * Tacoma board (AST2600)
> >
> > Thanks,
> >
> > C.
> >
> > Alexander Soldatov (1):
> >   mtd: spi-nor: fix options for mx66l51235f
> >
> > C?dric Le Goater (15):
> >   mtd: spi-nor: aspeed: Use command mode for reads
> >   mtd: spi-nor: aspeed: Add support for SPI dual IO read mode
> >   mtd: spi-nor: aspeed: Link controller with the ahb clock
> >   mtd: spi-nor: aspeed: Add read training
> >   mtd: spi-nor: aspeed: Limit the maximum SPI frequency
> >   mtd: spi-nor: aspeed: Add support for the 4B opcodes
> >   mtd: spi-nor: Add support for w25q512jv
> >   mtd: spi-nor: aspeed: Introduce a field for the AHB physical address
> >   mtd: spi-nor: aspeed: Introduce segment operations
> >   dt-bindings: mtd: aspeed-smc: Add new comptatible for AST2600
> >   mtd: spi-nor: aspeed: Add initial support for the AST2600
> >   mtd: spi-nor: aspeed: Check for disabled segments on the AST2600
> >   mtd: spi-nor: aspeed: Introduce training operations per platform
> >   mtd: spi-nor: aspeed: Introduce a HCLK mask for training
> >   mtd: spi-nor: aspeed: Add read training support for the AST2600
> >
> >  drivers/mtd/spi-nor/aspeed-smc.c              | 593 ++++++++++++++++--
> >  drivers/mtd/spi-nor/spi-nor.c                 |   5 +-
> >  .../devicetree/bindings/mtd/aspeed-smc.txt    |   2 +
> >  3 files changed, 551 insertions(+), 49 deletions(-)
> >
>

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Boris Brezillon]

On Thu, 10 Oct 2019 23:47:45 +0000
Joel Stanley <joel@jms.id.au> wrote:

> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > Hi Cedric,
> >
> > On Fri,  4 Oct 2019 13:59:03 +0200
> > C?dric Le Goater <clg@kaod.org> wrote:
> >  
> > > Hello,
> > >
> > > This series first extends the support for the Aspeed AST2500 and
> > > AST2400 SMC driver. It adds Dual Data support and read training giving
> > > the best read settings for a given chip. Support for the new AST2600
> > > SoC is added at the end.
> > >
> > > I understand that a new spi_mem framework exists and I do have an
> > > experimental driver using it. But unfortunately, it is difficult to
> > > integrate the read training. The Aspeed constraints are not compatible
> > > and i haven't had the time to extend the current framework.  
> >
> > Hm, I don't think that's a good reason to push new features to the
> > existing driver, especially since I asked others to migrate their
> > drivers to spi-mem in the past. I do understand your concerns, and I'll
> > let the SPI NOR/MTD maintainers make the final call, but I think it'd
> > be better for the SPI MEM ecosystem to think about this link-training
> > API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
> > this kind of feature to spi-nor controller drivers.  
> 
> As Cedric mentioned, the OpenBMC project has been shipping the read
> training code for the ast2400/ast2400 for several years now. It would
> be great to see it in mainline.
> 
> I think it's reasonable to ask for the driver to be moved to the
> spi-mem subsystem once it has the required APIs.

Except it won't have the necessary APIs unless someone works on it, and
adding this feature to existing spi-nor drivers won't help achieving
this goal.

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Cédric Le Goater]

On 11/10/2019 08:45, Boris Brezillon wrote:
> On Thu, 10 Oct 2019 23:47:45 +0000
> Joel Stanley <joel@jms.id.au> wrote:
> 
>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
>> <boris.brezillon@collabora.com> wrote:
>>>
>>> Hi Cedric,
>>>
>>> On Fri,  4 Oct 2019 13:59:03 +0200
>>> C?dric Le Goater <clg@kaod.org> wrote:
>>>  
>>>> Hello,
>>>>
>>>> This series first extends the support for the Aspeed AST2500 and
>>>> AST2400 SMC driver. It adds Dual Data support and read training giving
>>>> the best read settings for a given chip. Support for the new AST2600
>>>> SoC is added at the end.
>>>>
>>>> I understand that a new spi_mem framework exists and I do have an
>>>> experimental driver using it. But unfortunately, it is difficult to
>>>> integrate the read training. The Aspeed constraints are not compatible
>>>> and i haven't had the time to extend the current framework.  
>>>
>>> Hm, I don't think that's a good reason to push new features to the
>>> existing driver, especially since I asked others to migrate their
>>> drivers to spi-mem in the past. I do understand your concerns, and I'll
>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd
>>> be better for the SPI MEM ecosystem to think about this link-training
>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
>>> this kind of feature to spi-nor controller drivers.  
>>
>> As Cedric mentioned, the OpenBMC project has been shipping the read
>> training code for the ast2400/ast2400 for several years now. It would
>> be great to see it in mainline.
>>
>> I think it's reasonable to ask for the driver to be moved to the
>> spi-mem subsystem once it has the required APIs.
> 
> Except it won't have the necessary APIs unless someone works on it, and
> adding this feature to existing spi-nor drivers won't help achieving
> this goal.


What would you suggest ? Something like the patch below which would
call a 'train' operation at the end of spi_add_device().

Also, when doing read training, we might need to know some lowlevel 
characteristics of the chip being trained. Should we offer a way 
to grab the probed m25p80 device and give access to the underlying 
'struct spi_nor' ? 

  static struct spi_nor *spi_get_pnor(struct spi_device *spi)
  {
	struct spi_mem *spimem = spi_get_drvdata(spi);
	struct m25p *flash = spi_mem_get_drvdata(spimem);

	return flash ? &flash->spi_nor : NULL;
  }

Yeah, it's hideous. I just want to raise the issue.

Thanks,

C. 


From b34297e6b991ff051bc1e16103d14b2a05c81827 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Fri, 11 Oct 2019 11:09:33 +0200
Subject: [PATCH] spi: core: Add a device link training operation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: C?dric Le Goater <clg@kaod.org>
---
 include/linux/spi/spi.h |  4 ++++
 drivers/spi/spi.c       | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index af4f265d0f67..950b39304807 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -409,6 +409,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @fw_translate_cs: If the boot firmware uses different numbering scheme
  *	what Linux expects, this optional hook can be used to translate
  *	between the two.
+ * @train : perform device link training
  *
  * Each SPI controller can communicate with one or more @spi_device
  * children.  These make a small bus, sharing MOSI, MISO and SCK signals
@@ -604,6 +605,9 @@ struct spi_controller {
 	void			*dummy_tx;
 
 	int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs);
+
+	int			(*train)(struct spi_device *spi);
+
 };
 
 static inline void *spi_controller_get_devdata(struct spi_controller *ctlr)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 75ac046cae52..759a66d74822 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -542,6 +542,22 @@ static int spi_dev_check(struct device *dev, void *data)
 	return 0;
 }
 
+/**
+ * spi_train - link training of SPI device
+ * @spi: the device whose being trained
+ *
+ * Return: zero on success, else a negative error code.
+ */
+static int spi_train(struct spi_device *spi)
+{
+	int		status = 0;
+
+	if (spi->controller->train)
+		status = spi->controller->train(spi);
+
+	return status;
+}
+
 /**
  * spi_add_device - Add spi_device allocated with spi_alloc_device
  * @spi: spi_device to register
@@ -606,6 +622,13 @@ int spi_add_device(struct spi_device *spi)
 	else
 		dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
 
+	status = spi_train(spi);
+	if (status < 0) {
+		dev_err(dev, "can't train %s, status %d\n",
+				dev_name(&spi->dev), status);
+		goto done;
+	}
+
 done:
 	mutex_unlock(&spi_add_lock);
 	return status;
-- 
2.21.0

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Boris Brezillon]

On Fri, 11 Oct 2019 11:29:49 +0200
C?dric Le Goater <clg@kaod.org> wrote:

> On 11/10/2019 08:45, Boris Brezillon wrote:
> > On Thu, 10 Oct 2019 23:47:45 +0000
> > Joel Stanley <joel@jms.id.au> wrote:
> >   
> >> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
> >> <boris.brezillon@collabora.com> wrote:  
> >>>
> >>> Hi Cedric,
> >>>
> >>> On Fri,  4 Oct 2019 13:59:03 +0200
> >>> C?dric Le Goater <clg@kaod.org> wrote:
> >>>    
> >>>> Hello,
> >>>>
> >>>> This series first extends the support for the Aspeed AST2500 and
> >>>> AST2400 SMC driver. It adds Dual Data support and read training giving
> >>>> the best read settings for a given chip. Support for the new AST2600
> >>>> SoC is added at the end.
> >>>>
> >>>> I understand that a new spi_mem framework exists and I do have an
> >>>> experimental driver using it. But unfortunately, it is difficult to
> >>>> integrate the read training. The Aspeed constraints are not compatible
> >>>> and i haven't had the time to extend the current framework.    
> >>>
> >>> Hm, I don't think that's a good reason to push new features to the
> >>> existing driver, especially since I asked others to migrate their
> >>> drivers to spi-mem in the past. I do understand your concerns, and I'll
> >>> let the SPI NOR/MTD maintainers make the final call, but I think it'd
> >>> be better for the SPI MEM ecosystem to think about this link-training
> >>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
> >>> this kind of feature to spi-nor controller drivers.    
> >>
> >> As Cedric mentioned, the OpenBMC project has been shipping the read
> >> training code for the ast2400/ast2400 for several years now. It would
> >> be great to see it in mainline.
> >>
> >> I think it's reasonable to ask for the driver to be moved to the
> >> spi-mem subsystem once it has the required APIs.  
> > 
> > Except it won't have the necessary APIs unless someone works on it, and
> > adding this feature to existing spi-nor drivers won't help achieving
> > this goal.  
> 
> 
> What would you suggest ? Something like the patch below which would
> call a 'train' operation at the end of spi_add_device().

This has been discussed in the past with Vignesh, but I can't find the
thread where this discussion happened. My understanding was that link
training would use a command with well-known output (is there a
dedicated SPI NOR command for that?) and test different clk settings
until it finds one that works.

> 
> Also, when doing read training, we might need to know some lowlevel 
> characteristics of the chip being trained. Should we offer a way 
> to grab the probed m25p80 device and give access to the underlying 
> 'struct spi_nor' ? 
> 
>   static struct spi_nor *spi_get_pnor(struct spi_device *spi)
>   {
> 	struct spi_mem *spimem = spi_get_drvdata(spi);
> 	struct m25p *flash = spi_mem_get_drvdata(spimem);
> 
> 	return flash ? &flash->spi_nor : NULL;
>   }
> 
> Yeah, it's hideous. I just want to raise the issue.

Oh no. We definitely don't want to expose the spi_nor chip to the
spi_mem layer, but, if needed, we can add more fields to spi_mem and
let the spi_mem driver fill them. We just need to figure out what's
really needed.

> 
> Thanks,
> 
> C. 
> 
> 
> From b34297e6b991ff051bc1e16103d14b2a05c81827 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
> Date: Fri, 11 Oct 2019 11:09:33 +0200
> Subject: [PATCH] spi: core: Add a device link training operation
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Signed-off-by: C?dric Le Goater <clg@kaod.org>
> ---
>  include/linux/spi/spi.h |  4 ++++
>  drivers/spi/spi.c       | 23 +++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index af4f265d0f67..950b39304807 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -409,6 +409,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
>   * @fw_translate_cs: If the boot firmware uses different numbering scheme
>   *	what Linux expects, this optional hook can be used to translate
>   *	between the two.
> + * @train : perform device link training
>   *
>   * Each SPI controller can communicate with one or more @spi_device
>   * children.  These make a small bus, sharing MOSI, MISO and SCK signals
> @@ -604,6 +605,9 @@ struct spi_controller {
>  	void			*dummy_tx;
>  
>  	int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs);
> +
> +	int			(*train)(struct spi_device *spi);

Was more thinking of something like:

	int (*link_setup)(struct spi_mem *mem,
			  struct spi_mem_op *op_template,
			  ...);

where the op_template would potentially differ depending on the type of
memory (NOR, NAND, SRAM?). I also don't know what other params would be
needed to do the link training.

BTW, this hook should be in the spi_mem_controller_ops struct not in
spi_controller.

> +
>  };
>  
>  static inline void *spi_controller_get_devdata(struct spi_controller *ctlr)
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 75ac046cae52..759a66d74822 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -542,6 +542,22 @@ static int spi_dev_check(struct device *dev, void *data)
>  	return 0;
>  }
>  
> +/**
> + * spi_train - link training of SPI device
> + * @spi: the device whose being trained
> + *
> + * Return: zero on success, else a negative error code.
> + */
> +static int spi_train(struct spi_device *spi)
> +{
> +	int		status = 0;
> +
> +	if (spi->controller->train)
> +		status = spi->controller->train(spi);
> +
> +	return status;
> +}
> +
>  /**
>   * spi_add_device - Add spi_device allocated with spi_alloc_device
>   * @spi: spi_device to register
> @@ -606,6 +622,13 @@ int spi_add_device(struct spi_device *spi)
>  	else
>  		dev_dbg(dev, "registered child %s\n", dev_name(&spi->dev));
>  
> +	status = spi_train(spi);
> +	if (status < 0) {
> +		dev_err(dev, "can't train %s, status %d\n",
> +				dev_name(&spi->dev), status);
> +		goto done;
> +	}
> +
>  done:
>  	mutex_unlock(&spi_add_lock);
>  	return status;

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Cédric Le Goater]

On 11/10/2019 11:51, Boris Brezillon wrote:
> On Fri, 11 Oct 2019 11:29:49 +0200
> C?dric Le Goater <clg@kaod.org> wrote:
> 
>> On 11/10/2019 08:45, Boris Brezillon wrote:
>>> On Thu, 10 Oct 2019 23:47:45 +0000
>>> Joel Stanley <joel@jms.id.au> wrote:
>>>   
>>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
>>>> <boris.brezillon@collabora.com> wrote:  
>>>>>
>>>>> Hi Cedric,
>>>>>
>>>>> On Fri,  4 Oct 2019 13:59:03 +0200
>>>>> C?dric Le Goater <clg@kaod.org> wrote:
>>>>>    
>>>>>> Hello,
>>>>>>
>>>>>> This series first extends the support for the Aspeed AST2500 and
>>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving
>>>>>> the best read settings for a given chip. Support for the new AST2600
>>>>>> SoC is added at the end.
>>>>>>
>>>>>> I understand that a new spi_mem framework exists and I do have an
>>>>>> experimental driver using it. But unfortunately, it is difficult to
>>>>>> integrate the read training. The Aspeed constraints are not compatible
>>>>>> and i haven't had the time to extend the current framework.    
>>>>>
>>>>> Hm, I don't think that's a good reason to push new features to the
>>>>> existing driver, especially since I asked others to migrate their
>>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll
>>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd
>>>>> be better for the SPI MEM ecosystem to think about this link-training
>>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
>>>>> this kind of feature to spi-nor controller drivers.    
>>>>
>>>> As Cedric mentioned, the OpenBMC project has been shipping the read
>>>> training code for the ast2400/ast2400 for several years now. It would
>>>> be great to see it in mainline.
>>>>
>>>> I think it's reasonable to ask for the driver to be moved to the
>>>> spi-mem subsystem once it has the required APIs.  
>>>
>>> Except it won't have the necessary APIs unless someone works on it, and
>>> adding this feature to existing spi-nor drivers won't help achieving
>>> this goal.  
>>
>>
>> What would you suggest ? Something like the patch below which would
>> call a 'train' operation at the end of spi_add_device().
> 
> This has been discussed in the past with Vignesh, but I can't find the
> thread where this discussion happened. My understanding was that link
> training would use a command with well-known output (is there a
> dedicated SPI NOR command for that?) and test different clk settings
> until it finds one that works.

The read training on Aspeed consists of finding the appropriate read
timing delays for well-known HCLK dividers and store the results in 
the Read Timing Compensation register. 

We first read a golden buffer at low speed and then perform reads with 
different clocks and delay cycle settings to find a breaking point. This 
selects the default clock frequency for the CEx control register. 

 
>> Also, when doing read training, we might need to know some lowlevel 
>> characteristics of the chip being trained. Should we offer a way 
>> to grab the probed m25p80 device and give access to the underlying 
>> 'struct spi_nor' ? 
>>
>>   static struct spi_nor *spi_get_pnor(struct spi_device *spi)
>>   {
>> 	struct spi_mem *spimem = spi_get_drvdata(spi);
>> 	struct m25p *flash = spi_mem_get_drvdata(spimem);
>>
>> 	return flash ? &flash->spi_nor : NULL;
>>   }
>>
>> Yeah, it's hideous. I just want to raise the issue.
> 
> Oh no. We definitely don't want to expose the spi_nor chip to the
> spi_mem layer, but, if needed, we can add more fields to spi_mem and
> let the spi_mem driver fill them. We just need to figure out what's
> really needed.

We need the SPI/NOR flash characteristics for link training and its 
size to configure the controller to access the CS on the AHB window. 

[ ... ]

>>  	int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs);
>> +
>> +	int			(*train)(struct spi_device *spi);
> 
> Was more thinking of something like:
> 
> 	int (*link_setup)(struct spi_mem *mem,
> 			  struct spi_mem_op *op_template,
> 			  ...);
> 
> where the op_template would potentially differ depending on the type of
> memory (NOR, NAND, SRAM?). I also don't know what other params would be
> needed to do the link training.

The Aspeed controller needs to set read delay timings at different HCLK
settings. It's doing read operations with the default IO mode.
 
> BTW, this hook should be in the spi_mem_controller_ops struct not in
> spi_controller.

ok. Let's wait for feedback from Vinesh.

Thanks,

C. 

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Boris Brezillon]

On Fri, 11 Oct 2019 13:47:53 +0200
C?dric Le Goater <clg@kaod.org> wrote:

> On 11/10/2019 11:51, Boris Brezillon wrote:
> > On Fri, 11 Oct 2019 11:29:49 +0200
> > C?dric Le Goater <clg@kaod.org> wrote:
> >   
> >> On 11/10/2019 08:45, Boris Brezillon wrote:  
> >>> On Thu, 10 Oct 2019 23:47:45 +0000
> >>> Joel Stanley <joel@jms.id.au> wrote:
> >>>     
> >>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
> >>>> <boris.brezillon@collabora.com> wrote:    
> >>>>>
> >>>>> Hi Cedric,
> >>>>>
> >>>>> On Fri,  4 Oct 2019 13:59:03 +0200
> >>>>> C?dric Le Goater <clg@kaod.org> wrote:
> >>>>>      
> >>>>>> Hello,
> >>>>>>
> >>>>>> This series first extends the support for the Aspeed AST2500 and
> >>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving
> >>>>>> the best read settings for a given chip. Support for the new AST2600
> >>>>>> SoC is added at the end.
> >>>>>>
> >>>>>> I understand that a new spi_mem framework exists and I do have an
> >>>>>> experimental driver using it. But unfortunately, it is difficult to
> >>>>>> integrate the read training. The Aspeed constraints are not compatible
> >>>>>> and i haven't had the time to extend the current framework.      
> >>>>>
> >>>>> Hm, I don't think that's a good reason to push new features to the
> >>>>> existing driver, especially since I asked others to migrate their
> >>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll
> >>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd
> >>>>> be better for the SPI MEM ecosystem to think about this link-training
> >>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
> >>>>> this kind of feature to spi-nor controller drivers.      
> >>>>
> >>>> As Cedric mentioned, the OpenBMC project has been shipping the read
> >>>> training code for the ast2400/ast2400 for several years now. It would
> >>>> be great to see it in mainline.
> >>>>
> >>>> I think it's reasonable to ask for the driver to be moved to the
> >>>> spi-mem subsystem once it has the required APIs.    
> >>>
> >>> Except it won't have the necessary APIs unless someone works on it, and
> >>> adding this feature to existing spi-nor drivers won't help achieving
> >>> this goal.    
> >>
> >>
> >> What would you suggest ? Something like the patch below which would
> >> call a 'train' operation at the end of spi_add_device().  
> > 
> > This has been discussed in the past with Vignesh, but I can't find the
> > thread where this discussion happened. My understanding was that link
> > training would use a command with well-known output (is there a
> > dedicated SPI NOR command for that?) and test different clk settings
> > until it finds one that works.  
> 
> The read training on Aspeed consists of finding the appropriate read
> timing delays for well-known HCLK dividers and store the results in 
> the Read Timing Compensation register. 
> 
> We first read a golden buffer at low speed and then perform reads with 
> different clocks and delay cycle settings to find a breaking point.

Where does this golden buffer come from? Do you have a specific command
to access it? Is it a NOR section with well-known data?

> This 
> selects the default clock frequency for the CEx control register. 
> 
>  
> >> Also, when doing read training, we might need to know some lowlevel 
> >> characteristics of the chip being trained. Should we offer a way 
> >> to grab the probed m25p80 device and give access to the underlying 
> >> 'struct spi_nor' ? 
> >>
> >>   static struct spi_nor *spi_get_pnor(struct spi_device *spi)
> >>   {
> >> 	struct spi_mem *spimem = spi_get_drvdata(spi);
> >> 	struct m25p *flash = spi_mem_get_drvdata(spimem);
> >>
> >> 	return flash ? &flash->spi_nor : NULL;
> >>   }
> >>
> >> Yeah, it's hideous. I just want to raise the issue.  
> > 
> > Oh no. We definitely don't want to expose the spi_nor chip to the
> > spi_mem layer, but, if needed, we can add more fields to spi_mem and
> > let the spi_mem driver fill them. We just need to figure out what's
> > really needed.  
> 
> We need the SPI/NOR flash characteristics for link training and its 
> size to configure the controller to access the CS on the AHB window. 

We managed to deal with direct mapping without having to explicitly pass
the NOR size (we just pass the size of the direct mapping we want to
create). What do you need the size for? Is it just to configure the AHB
window for the link-training stuff? If that's the case, I guess this
can be part of the op_template I was talking about, or maybe passed as
extra parameters.

> 
> [ ... ]
> 
> >>  	int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs);
> >> +
> >> +	int			(*train)(struct spi_device *spi);  
> > 
> > Was more thinking of something like:
> > 
> > 	int (*link_setup)(struct spi_mem *mem,
> > 			  struct spi_mem_op *op_template,
> > 			  ...);
> > 
> > where the op_template would potentially differ depending on the type of
> > memory (NOR, NAND, SRAM?). I also don't know what other params would be
> > needed to do the link training.  
> 
> The Aspeed controller needs to set read delay timings at different HCLK
> settings. It's doing read operations with the default IO mode.

So you need information about the theoretical read delay so you can
adjust them, right. I guess that's something we can pass to the
->link_setup() hook.

>  
> > BTW, this hook should be in the spi_mem_controller_ops struct not in
> > spi_controller.  
> 
> ok. Let's wait for feedback from Vinesh.

Thanks for starting this discussion.

Boris

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Cédric Le Goater]

On 11/10/2019 14:07, Boris Brezillon wrote:
> On Fri, 11 Oct 2019 13:47:53 +0200
> C?dric Le Goater <clg@kaod.org> wrote:
> 
>> On 11/10/2019 11:51, Boris Brezillon wrote:
>>> On Fri, 11 Oct 2019 11:29:49 +0200
>>> C?dric Le Goater <clg@kaod.org> wrote:
>>>   
>>>> On 11/10/2019 08:45, Boris Brezillon wrote:  
>>>>> On Thu, 10 Oct 2019 23:47:45 +0000
>>>>> Joel Stanley <joel@jms.id.au> wrote:
>>>>>     
>>>>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
>>>>>> <boris.brezillon@collabora.com> wrote:    
>>>>>>>
>>>>>>> Hi Cedric,
>>>>>>>
>>>>>>> On Fri,  4 Oct 2019 13:59:03 +0200
>>>>>>> C?dric Le Goater <clg@kaod.org> wrote:
>>>>>>>      
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> This series first extends the support for the Aspeed AST2500 and
>>>>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving
>>>>>>>> the best read settings for a given chip. Support for the new AST2600
>>>>>>>> SoC is added at the end.
>>>>>>>>
>>>>>>>> I understand that a new spi_mem framework exists and I do have an
>>>>>>>> experimental driver using it. But unfortunately, it is difficult to
>>>>>>>> integrate the read training. The Aspeed constraints are not compatible
>>>>>>>> and i haven't had the time to extend the current framework.      
>>>>>>>
>>>>>>> Hm, I don't think that's a good reason to push new features to the
>>>>>>> existing driver, especially since I asked others to migrate their
>>>>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll
>>>>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd
>>>>>>> be better for the SPI MEM ecosystem to think about this link-training
>>>>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
>>>>>>> this kind of feature to spi-nor controller drivers.      
>>>>>>
>>>>>> As Cedric mentioned, the OpenBMC project has been shipping the read
>>>>>> training code for the ast2400/ast2400 for several years now. It would
>>>>>> be great to see it in mainline.
>>>>>>
>>>>>> I think it's reasonable to ask for the driver to be moved to the
>>>>>> spi-mem subsystem once it has the required APIs.    
>>>>>
>>>>> Except it won't have the necessary APIs unless someone works on it, and
>>>>> adding this feature to existing spi-nor drivers won't help achieving
>>>>> this goal.    
>>>>
>>>>
>>>> What would you suggest ? Something like the patch below which would
>>>> call a 'train' operation at the end of spi_add_device().  
>>>
>>> This has been discussed in the past with Vignesh, but I can't find the
>>> thread where this discussion happened. My understanding was that link
>>> training would use a command with well-known output (is there a
>>> dedicated SPI NOR command for that?) and test different clk settings
>>> until it finds one that works.  
>>
>> The read training on Aspeed consists of finding the appropriate read
>> timing delays for well-known HCLK dividers and store the results in 
>> the Read Timing Compensation register. 
>>
>> We first read a golden buffer at low speed and then perform reads with 
>> different clocks and delay cycle settings to find a breaking point.
> 
> Where does this golden buffer come from? Do you have a specific command
> to access it? Is it a NOR section with well-known data?

We read the first 16K of the flash device at low speed.  

>> This 
>> selects the default clock frequency for the CEx control register. 
>>
>>  
>>>> Also, when doing read training, we might need to know some lowlevel 
>>>> characteristics of the chip being trained. Should we offer a way 
>>>> to grab the probed m25p80 device and give access to the underlying 
>>>> 'struct spi_nor' ? 
>>>>
>>>>   static struct spi_nor *spi_get_pnor(struct spi_device *spi)
>>>>   {
>>>> 	struct spi_mem *spimem = spi_get_drvdata(spi);
>>>> 	struct m25p *flash = spi_mem_get_drvdata(spimem);
>>>>
>>>> 	return flash ? &flash->spi_nor : NULL;
>>>>   }
>>>>
>>>> Yeah, it's hideous. I just want to raise the issue.  
>>>
>>> Oh no. We definitely don't want to expose the spi_nor chip to the
>>> spi_mem layer, but, if needed, we can add more fields to spi_mem and
>>> let the spi_mem driver fill them. We just need to figure out what's
>>> really needed.  
>>
>> We need the SPI/NOR flash characteristics for link training and its 
>> size to configure the controller to access the CS on the AHB window. 
> 
> We managed to deal with direct mapping without having to explicitly pass
> the NOR size (we just pass the size of the direct mapping we want to
> create). What do you need the size for? 

because the AHB window on which are mapped all CS is segmented. 

There is one sub window for each CS and all segments need to be 
configured in the controller (in the segment registers). In case 
of a bad value or an overlap, you can loose access to the CS or 
hang the system.

To access the SPI NOR registers, you don't need a window of the 
exact size because in that case the controller operates in 'User' 
command mode and the window only needs to be partially opened. 
Reads and Writes use the start address of AHB sub-window of the CS.

If you want access to the full contents through a direct mapping,
the controller operates in 'Read' or 'Fast Read' command mode and
the window needs to be at least as wide as the flash size.

> Is it just to configure the AHB window for the link-training stuff? 

The link-training uses the direct mapping mode but not on the
whole contents, only on the first 16K. So the AHB window does not 
have to be as wide as the chip and we could use a default setting. 
But generally, we already have good knowledge on the chip before
training is started.

> If that's the case, I guess this can be part of the op_template 
> I was talking about, or maybe passed as extra parameters.
>
>> [ ... ]
>>
>>>>  	int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs);
>>>> +
>>>> +	int			(*train)(struct spi_device *spi);  
>>>
>>> Was more thinking of something like:
>>>
>>> 	int (*link_setup)(struct spi_mem *mem,
>>> 			  struct spi_mem_op *op_template,
>>> 			  ...);
>>>
>>> where the op_template would potentially differ depending on the type of
>>> memory (NOR, NAND, SRAM?). I also don't know what other params would be
>>> needed to do the link training.  
>>
>> The Aspeed controller needs to set read delay timings at different HCLK
>> settings. It's doing read operations with the default IO mode.
> 
> So you need information about the theoretical read delay so you can
> adjust them, right. I guess that's something we can pass to the
> ->link_setup() hook.

The read delay settings really depend on the controller. I was thinking 
more of a post_setup() hook called once in m25p_probe() after the chip 
is scanned. 

As for the parameter, we would need some SPI-NOR info or some generic
SPI device data that the controller would know how to interpret.
  

Cheers,

C.


>>> BTW, this hook should be in the spi_mem_controller_ops struct not in
>>> spi_controller.  
>>
>> ok. Let's wait for feedback from Vinesh.
> 
> Thanks for starting this discussion.
> 
> Boris
> 

[PATCH 00/16] mtd: spi-nor: aspeed: AST2600 support and extensions

[Boris Brezillon]

On Fri, 11 Oct 2019 15:07:18 +0200
C?dric Le Goater <clg@kaod.org> wrote:

> On 11/10/2019 14:07, Boris Brezillon wrote:
> > On Fri, 11 Oct 2019 13:47:53 +0200
> > C?dric Le Goater <clg@kaod.org> wrote:
> >   
> >> On 11/10/2019 11:51, Boris Brezillon wrote:  
> >>> On Fri, 11 Oct 2019 11:29:49 +0200
> >>> C?dric Le Goater <clg@kaod.org> wrote:
> >>>     
> >>>> On 11/10/2019 08:45, Boris Brezillon wrote:    
> >>>>> On Thu, 10 Oct 2019 23:47:45 +0000
> >>>>> Joel Stanley <joel@jms.id.au> wrote:
> >>>>>       
> >>>>>> On Wed, 9 Oct 2019 at 20:56, Boris Brezillon
> >>>>>> <boris.brezillon@collabora.com> wrote:      
> >>>>>>>
> >>>>>>> Hi Cedric,
> >>>>>>>
> >>>>>>> On Fri,  4 Oct 2019 13:59:03 +0200
> >>>>>>> C?dric Le Goater <clg@kaod.org> wrote:
> >>>>>>>        
> >>>>>>>> Hello,
> >>>>>>>>
> >>>>>>>> This series first extends the support for the Aspeed AST2500 and
> >>>>>>>> AST2400 SMC driver. It adds Dual Data support and read training giving
> >>>>>>>> the best read settings for a given chip. Support for the new AST2600
> >>>>>>>> SoC is added at the end.
> >>>>>>>>
> >>>>>>>> I understand that a new spi_mem framework exists and I do have an
> >>>>>>>> experimental driver using it. But unfortunately, it is difficult to
> >>>>>>>> integrate the read training. The Aspeed constraints are not compatible
> >>>>>>>> and i haven't had the time to extend the current framework.        
> >>>>>>>
> >>>>>>> Hm, I don't think that's a good reason to push new features to the
> >>>>>>> existing driver, especially since I asked others to migrate their
> >>>>>>> drivers to spi-mem in the past. I do understand your concerns, and I'll
> >>>>>>> let the SPI NOR/MTD maintainers make the final call, but I think it'd
> >>>>>>> be better for the SPI MEM ecosystem to think about this link-training
> >>>>>>> API (Vignesh needs it for the Cadence driver IIRC) rather than pushing
> >>>>>>> this kind of feature to spi-nor controller drivers.        
> >>>>>>
> >>>>>> As Cedric mentioned, the OpenBMC project has been shipping the read
> >>>>>> training code for the ast2400/ast2400 for several years now. It would
> >>>>>> be great to see it in mainline.
> >>>>>>
> >>>>>> I think it's reasonable to ask for the driver to be moved to the
> >>>>>> spi-mem subsystem once it has the required APIs.      
> >>>>>
> >>>>> Except it won't have the necessary APIs unless someone works on it, and
> >>>>> adding this feature to existing spi-nor drivers won't help achieving
> >>>>> this goal.      
> >>>>
> >>>>
> >>>> What would you suggest ? Something like the patch below which would
> >>>> call a 'train' operation at the end of spi_add_device().    
> >>>
> >>> This has been discussed in the past with Vignesh, but I can't find the
> >>> thread where this discussion happened. My understanding was that link
> >>> training would use a command with well-known output (is there a
> >>> dedicated SPI NOR command for that?) and test different clk settings
> >>> until it finds one that works.    
> >>
> >> The read training on Aspeed consists of finding the appropriate read
> >> timing delays for well-known HCLK dividers and store the results in 
> >> the Read Timing Compensation register. 
> >>
> >> We first read a golden buffer at low speed and then perform reads with 
> >> different clocks and delay cycle settings to find a breaking point.  
> > 
> > Where does this golden buffer come from? Do you have a specific command
> > to access it? Is it a NOR section with well-known data?  
> 
> We read the first 16K of the flash device at low speed.  

That's what I figured after reading the code.

> 
> >> This 
> >> selects the default clock frequency for the CEx control register. 
> >>
> >>    
> >>>> Also, when doing read training, we might need to know some lowlevel 
> >>>> characteristics of the chip being trained. Should we offer a way 
> >>>> to grab the probed m25p80 device and give access to the underlying 
> >>>> 'struct spi_nor' ? 
> >>>>
> >>>>   static struct spi_nor *spi_get_pnor(struct spi_device *spi)
> >>>>   {
> >>>> 	struct spi_mem *spimem = spi_get_drvdata(spi);
> >>>> 	struct m25p *flash = spi_mem_get_drvdata(spimem);
> >>>>
> >>>> 	return flash ? &flash->spi_nor : NULL;
> >>>>   }
> >>>>
> >>>> Yeah, it's hideous. I just want to raise the issue.    
> >>>
> >>> Oh no. We definitely don't want to expose the spi_nor chip to the
> >>> spi_mem layer, but, if needed, we can add more fields to spi_mem and
> >>> let the spi_mem driver fill them. We just need to figure out what's
> >>> really needed.    
> >>
> >> We need the SPI/NOR flash characteristics for link training and its 
> >> size to configure the controller to access the CS on the AHB window.   
> > 
> > We managed to deal with direct mapping without having to explicitly pass
> > the NOR size (we just pass the size of the direct mapping we want to
> > create). What do you need the size for?   
> 
> because the AHB window on which are mapped all CS is segmented. 
> 
> There is one sub window for each CS and all segments need to be 
> configured in the controller (in the segment registers). In case 
> of a bad value or an overlap, you can loose access to the CS or 
> hang the system.

The dirmap API is here for that. If you implement the
->{create,destroy}_dirmap() hooks you should be able to decide which AHB
window is assigned to which chip. The SPI NOR layer will (soon) make
use of dirmaps instead of using plain spi_mem_exec_op() for
data read/write accesses BTW.

> 
> To access the SPI NOR registers, you don't need a window of the 
> exact size because in that case the controller operates in 'User' 
> command mode and the window only needs to be partially opened. 

Sounds good.

> Reads and Writes use the start address of AHB sub-window of the CS.

This too.

> 
> If you want access to the full contents through a direct mapping,
> the controller operates in 'Read' or 'Fast Read' command mode and
> the window needs to be at least as wide as the flash size.

Well, we usually don't enforce that in other drivers (we use the concept
of sliding window when the flash is bigger than the physical AHB window,
and we adjust the start offset dynamically based on the
dirmap_read/write() request), but if you want to enforce that in your
driver it's fine.

> 
> > Is it just to configure the AHB window for the link-training stuff?   
> 
> The link-training uses the direct mapping mode but not on the
> whole contents, only on the first 16K. So the AHB window does not 
> have to be as wide as the chip and we could use a default setting.
> But generally, we already have good knowledge on the chip before
> training is started.

If you need the direct mapping to be set up for the link training, that
shouldn't be a problem.

> 
> > If that's the case, I guess this can be part of the op_template 
> > I was talking about, or maybe passed as extra parameters.
> >  
> >> [ ... ]
> >>  
> >>>>  	int (*fw_translate_cs)(struct spi_controller *ctlr, unsigned cs);
> >>>> +
> >>>> +	int			(*train)(struct spi_device *spi);    
> >>>
> >>> Was more thinking of something like:
> >>>
> >>> 	int (*link_setup)(struct spi_mem *mem,
> >>> 			  struct spi_mem_op *op_template,
> >>> 			  ...);
> >>>
> >>> where the op_template would potentially differ depending on the type of
> >>> memory (NOR, NAND, SRAM?). I also don't know what other params would be
> >>> needed to do the link training.    
> >>
> >> The Aspeed controller needs to set read delay timings at different HCLK
> >> settings. It's doing read operations with the default IO mode.  
> > 
> > So you need information about the theoretical read delay so you can
> > adjust them, right. I guess that's something we can pass to the  
> > ->link_setup() hook.  
> 
> The read delay settings really depend on the controller. I was thinking 
> more of a post_setup() hook called once in m25p_probe() after the chip 
> is scanned. 

For the record, m25p is gone (code has been merged in spi-nor.c) ;-).
Back to the original topic, I don't think link setup is something
so controller specific that we can't abstract it at the spi-nor level.
Of course the piece of HW doing the link training is still the SPI
controller, but the SPI NOR framework can abstract a few things, like
figuring out which SPI_NOR operation to use, where to read the data
from, ...

> 
> As for the parameter, we would need some SPI-NOR info or some generic
> SPI device data that the controller would know how to interpret.

Yes, that's my point. This part is memory specific. The link training
with a SPI NAND is likely to use a different operation than what's used
for a SPI NOR, and we definitely don't want SPI controllers to be aware
of that.