All of lore.kernel.org
 help / color / mirror / Atom feed
* [v3 0/4] Porting ASPEED FMC/SPI memory controller driver
@ 2020-11-05 12:03 Chin-Ting Kuo
  2020-11-05 12:03 ` [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller Chin-Ting Kuo
                   ` (4 more replies)
  0 siblings, 5 replies; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-05 12:03 UTC (permalink / raw)
  To: broonie, robh+dt, joel, andrew, clg, bbrezillon, devicetree,
	linux-kernel, linux-aspeed, linux-spi
  Cc: BMC-SW

This patch series aims to porting ASPEED FMC/SPI memory controller
driver with spi-mem interface. Adjust device tree setting of SPI NOR
flash in order to fit real AST2600 EVB and new SPI memory controller
driver. Also, this patch has been verified on AST2600-A1 EVB.

v2: Fix sparse warnings reported by kernel test robot <lkp@intel.com>.
v3: Fix build warnings with x86 allmodconfig.

Chin-Ting Kuo (4):
  dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory
    controller
  ARM: dts: aspeed: ast2600: Update FMC/SPI controller setting for
    spi-aspeed.c
  ARM: dts: aspeed: ast2600-evb: Adjust SPI flash configuration
  spi: aspeed: Add ASPEED FMC/SPI memory controller driver

 .../bindings/spi/aspeed,spi-aspeed.yaml       |  66 ++
 arch/arm/boot/dts/aspeed-ast2600-evb.dts      |  26 +-
 arch/arm/boot/dts/aspeed-g6.dtsi              |  18 +-
 drivers/spi/Kconfig                           |  10 +
 drivers/spi/Makefile                          |   1 +
 drivers/spi/spi-aspeed.c                      | 969 ++++++++++++++++++
 6 files changed, 1080 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
 create mode 100644 drivers/spi/spi-aspeed.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller
  2020-11-05 12:03 [v3 0/4] Porting ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
@ 2020-11-05 12:03 ` Chin-Ting Kuo
  2020-11-05 22:39   ` Rob Herring
  2020-11-05 12:03 ` [v3 2/4] ARM: dts: aspeed: ast2600: Update FMC/SPI controller setting for spi-aspeed.c Chin-Ting Kuo
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-05 12:03 UTC (permalink / raw)
  To: broonie, robh+dt, joel, andrew, clg, bbrezillon, devicetree,
	linux-kernel, linux-aspeed, linux-spi
  Cc: BMC-SW

Create binding file with YAML syntax for ASPEED FMC/SPI memory controller.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 .../bindings/spi/aspeed,spi-aspeed.yaml       | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml

diff --git a/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml b/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
new file mode 100644
index 000000000000..41b9692c7226
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/aspeed,spi-aspeed.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SPI memory controller for ASPEED SoCs
+
+maintainers:
+  - Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
+
+description: |
+  There are three SPI memory controllers embedded in a ASPEED SoC.
+  They are usually connected to SPI NOR flashes. Each of them has
+  more than a chip select. They also support SPI single, dual and
+  quad IO modes for SPI NOR flash.
+
+allOf:
+  - $ref: /spi/spi-controller.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - enum:
+              - aspeed,ast2600-fmc
+              - aspeed,ast2600-spi
+
+  reg:
+    items:
+      - description: the control register location and length
+      - description: the flash memory mapping address and length
+
+  clocks:
+    description: AHB bus clock which will be converted to SPI bus clock
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - num-cs
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/ast2600-clock.h>
+    spi1: spi@1e630000 {
+      compatible = "aspeed,ast2600-spi";
+      reg = <0x1e630000 0xc4>, <0x30000000 0x10000000>;
+      reg-names = "spi_ctrl_reg", "spi_mmap";
+      clocks = <&syscon ASPEED_CLK_AHB>;
+      num-cs = <2>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+      flash@0 {
+        compatible = "jedec,spi-nor";
+        reg = <0>;
+        spi-max-frequency = <50000000>;
+      };
+      flash@1 {
+        compatible = "jedec,spi-nor";
+        reg = <1>;
+        spi-max-frequency = <50000000>;
+      };
+    };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [v3 2/4] ARM: dts: aspeed: ast2600: Update FMC/SPI controller setting for spi-aspeed.c
  2020-11-05 12:03 [v3 0/4] Porting ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
  2020-11-05 12:03 ` [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller Chin-Ting Kuo
@ 2020-11-05 12:03 ` Chin-Ting Kuo
  2020-11-05 12:03 ` [v3 3/4] ARM: dts: aspeed: ast2600-evb: Adjust SPI flash configuration Chin-Ting Kuo
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-05 12:03 UTC (permalink / raw)
  To: broonie, robh+dt, joel, andrew, clg, bbrezillon, devicetree,
	linux-kernel, linux-aspeed, linux-spi
  Cc: BMC-SW

- Adjust the value format of "reg" property:
  Instead of platform_get_resource(),
  platform_get_resource_byname() function can be used
  for more human-readable.
- Add "num-cs" property for FMC/SPI controller:
  Each ASPEED FMC/SPI memory controller can support more
  than a chip select. By "num-cs" property, FMC/SPI
  controller driver can know how many chip select related
  registers should be initialized at the probe stage.
  Besdies, with this property, driver can avoid accessing
  chip select which CS number is larger than the maximum
  one supported by the controller.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-g6.dtsi | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
index b58220a49cbd..8a5c798db54e 100644
--- a/arch/arm/boot/dts/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6.dtsi
@@ -89,14 +89,16 @@
 			};
 
 		fmc: spi@1e620000 {
-			reg = < 0x1e620000 0xc4
-				0x20000000 0x10000000 >;
+			reg = <0x1e620000 0xc4>,
+				<0x20000000 0x10000000>;
+			reg-names = "spi_ctrl_reg", "spi_mmap";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			compatible = "aspeed,ast2600-fmc";
 			clocks = <&syscon ASPEED_CLK_AHB>;
 			status = "disabled";
 			interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
+			num-cs = <3>;
 			flash@0 {
 				reg = < 0 >;
 				compatible = "jedec,spi-nor";
@@ -118,12 +120,14 @@
 		};
 
 		spi1: spi@1e630000 {
-			reg = < 0x1e630000 0xc4
-				0x30000000 0x10000000 >;
+			reg = <0x1e630000 0xc4>,
+				<0x30000000 0x10000000>;
+			reg-names = "spi_ctrl_reg", "spi_mmap";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			compatible = "aspeed,ast2600-spi";
 			clocks = <&syscon ASPEED_CLK_AHB>;
+			num-cs = <2>;
 			status = "disabled";
 			flash@0 {
 				reg = < 0 >;
@@ -140,12 +144,14 @@
 		};
 
 		spi2: spi@1e631000 {
-			reg = < 0x1e631000 0xc4
-				0x50000000 0x10000000 >;
+			reg = < 0x1e631000 0xc4>,
+				<0x50000000 0x10000000>;
+			reg-names = "spi_ctrl_reg", "spi_mmap";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			compatible = "aspeed,ast2600-spi";
 			clocks = <&syscon ASPEED_CLK_AHB>;
+			num-cs = <3>;
 			status = "disabled";
 			flash@0 {
 				reg = < 0 >;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [v3 3/4] ARM: dts: aspeed: ast2600-evb: Adjust SPI flash configuration
  2020-11-05 12:03 [v3 0/4] Porting ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
  2020-11-05 12:03 ` [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller Chin-Ting Kuo
  2020-11-05 12:03 ` [v3 2/4] ARM: dts: aspeed: ast2600: Update FMC/SPI controller setting for spi-aspeed.c Chin-Ting Kuo
@ 2020-11-05 12:03 ` Chin-Ting Kuo
  2020-11-05 12:03 ` [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
  2020-12-01 13:57 ` [v3 0/4] Porting " Mark Brown
  4 siblings, 0 replies; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-05 12:03 UTC (permalink / raw)
  To: broonie, robh+dt, joel, andrew, clg, bbrezillon, devicetree,
	linux-kernel, linux-aspeed, linux-spi
  Cc: BMC-SW

- Enable FMC CS1 and SPI2 CS0 SPI NOR flashes since both of
  these two flashes are mounted on AST2600 EVB by default.
- Remove spi-max-frequency setting: 50MHz is usual SPI bus
  frequency adopted on AST2600 EVB which has already been
  configured in aspeed-g6.dtsi.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-ast2600-evb.dts | 26 ++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 8d0f4656aa05..5a2e4612d155 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -96,12 +96,11 @@
 
 &fmc {
 	status = "okay";
+
 	flash@0 {
 		status = "okay";
 		m25p,fast-read;
 		label = "bmc";
-		spi-max-frequency = <50000000>;
-
 		partitions {
 			compatible = "fixed-partitions";
 			#address-cells = <1>;
@@ -133,18 +132,37 @@
 			};
 		};
 	};
+
+	flash@1 {
+		status = "okay";
+		m25p,fast-read;
+		label = "fmc0:1";
+	};
 };
 
 &spi1 {
 	status = "okay";
+
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_spi1_default>;
 
 	flash@0 {
 		status = "okay";
 		m25p,fast-read;
-		label = "pnor";
-		spi-max-frequency = <100000000>;
+		label = "spi1:0";
+	};
+};
+
+&spi2 {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_spi2_default>;
+
+	flash@0 {
+		status = "okay";
+		m25p,fast-read;
+		label = "spi2:0";
 	};
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-05 12:03 [v3 0/4] Porting ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
                   ` (2 preceding siblings ...)
  2020-11-05 12:03 ` [v3 3/4] ARM: dts: aspeed: ast2600-evb: Adjust SPI flash configuration Chin-Ting Kuo
@ 2020-11-05 12:03 ` Chin-Ting Kuo
  2020-11-05 14:09   ` Cédric Le Goater
  2020-12-01 13:57 ` [v3 0/4] Porting " Mark Brown
  4 siblings, 1 reply; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-05 12:03 UTC (permalink / raw)
  To: broonie, robh+dt, joel, andrew, clg, bbrezillon, devicetree,
	linux-kernel, linux-aspeed, linux-spi
  Cc: BMC-SW

Add driver for ASPEED BMC FMC/SPI memory controller which
supports spi-mem interface.

There are three SPI memory controllers embedded in an ASPEED SoC.
Each of them can connect to two or three SPI NOR flashes. The first
SPI memory controller is also named as Firmware Memory Controller (FMC),
which is similar to SPI memory controller. After device AC on, MCU ROM
can fetch device boot code from FMC CS 0. Thus, there exists additional
registers for boot process control in FMC.

ASPEED SPI memory controller supports single, dual and quad mode for
SPI NOR flash. It also supports two types of command mode, user mode
and command read/write mode. User mode is traditional pure SPI operations
where all transmission is controlled by CPU. Contrarily, with command
read/write mode, SPI controller can send command and address automatically
when CPU read/write related remapped address.

Besides, different wafer processes of SPI NOR flash result in different
signal response time. This phenomenon will be enlarged when SPI clock
frequency increases. ASPEED SPI memory controller provides a mechanism
for timing compensation in order to satisfy various SPI NOR flash parts
and PCB layout.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 v2: Fix sparse warnings reported by kernel test robot <lkp@intel.com>.
 v3: Fix build warnings with x86 allmodconfig.

 drivers/spi/Kconfig      |  10 +
 drivers/spi/Makefile     |   1 +
 drivers/spi/spi-aspeed.c | 969 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 980 insertions(+)
 create mode 100644 drivers/spi/spi-aspeed.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 5cff60de8e83..c848f2f7b694 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -70,6 +70,16 @@ config SPI_AR934X
 	  This enables support for the SPI controller present on the
 	  Qualcomm Atheros AR934X/QCA95XX SoCs.
 
+config SPI_ASPEED
+	tristate "ASPEED FMC/SPI Memory Controller"
+	depends on OF && HAS_IOMEM && (ARCH_ASPEED || COMPILE_TEST)
+	help
+	  Enable driver for ASPEED FMC/SPI Memory Controller.
+
+	  This driver is not a generic pure SPI driver, which
+	  is especially designed for spi-mem framework with
+	  SPI NOR flash direct read and write features.
+
 config SPI_ATH79
 	tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
 	depends on ATH79 || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 6fea5821662e..9e62c650fca0 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
 obj-$(CONFIG_SPI_ALTERA)		+= spi-altera.o
 obj-$(CONFIG_SPI_AR934X)		+= spi-ar934x.o
 obj-$(CONFIG_SPI_ARMADA_3700)		+= spi-armada-3700.o
+obj-$(CONFIG_SPI_ASPEED)		+= spi-aspeed.o
 obj-$(CONFIG_SPI_ATMEL)			+= spi-atmel.o
 obj-$(CONFIG_SPI_ATMEL_QUADSPI)		+= atmel-quadspi.o
 obj-$(CONFIG_SPI_AT91_USART)		+= spi-at91-usart.o
diff --git a/drivers/spi/spi-aspeed.c b/drivers/spi/spi-aspeed.c
new file mode 100644
index 000000000000..cfaaa0d5bac6
--- /dev/null
+++ b/drivers/spi/spi-aspeed.c
@@ -0,0 +1,969 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * ASPEED FMC/SPI Memory Controller Driver
+ *
+ * Copyright (c) 2020, ASPEED Corporation.
+ * Copyright (c) 2015-2016, IBM Corporation.
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/sizes.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+/* ASPEED FMC/SPI memory control register related */
+#define OFFSET_CE_TYPE_SETTING		0x00
+#define OFFSET_CE_ADDR_MODE_CTRL	0x04
+#define OFFSET_INTR_CTRL_STATUS		0x08
+#define OFFSET_ADDR_DATA_MASK		0x0c
+#define OFFSET_CE0_CTRL_REG		0x10
+#define OFFSET_CE0_DECODE_RANGE_REG	0x30
+#define OFFSET_DMA_CTRL			0x80
+#define OFFSET_DMA_FLASH_ADDR_REG	0x84
+#define OFFSET_DMA_RAM_ADDR_REG		0x88
+#define OFFSET_DMA_LEN_REG		0x8c
+#define OFFSET_DMA_CHECKSUM_RESULT	0x90
+#define OFFSET_CE0_TIMING_COMPENSATION	0x94
+
+#define CTRL_IO_SINGLE_DATA	0
+#define CTRL_IO_DUAL_DATA	BIT(29)
+#define CTRL_IO_QUAD_DATA	BIT(30)
+
+#define CTRL_IO_MODE_USER	GENMASK(1, 0)
+#define CTRL_IO_MODE_CMD_READ	BIT(0)
+#define CTRL_IO_MODE_CMD_WRITE	BIT(1)
+#define CTRL_STOP_ACTIVE	BIT(2)
+
+#define CALIBRATION_LEN		0x400
+#define SPI_DAM_REQUEST		BIT(31)
+#define SPI_DAM_GRANT		BIT(30)
+#define SPI_DMA_CALIB_MODE	BIT(3)
+#define SPI_DMA_CALC_CKSUM	BIT(2)
+#define SPI_DMA_ENABLE		BIT(0)
+#define SPI_DMA_STATUS		BIT(11)
+
+enum aspeed_spi_ctl_reg_value {
+	ASPEED_SPI_BASE,
+	ASPEED_SPI_READ,
+	ASPEED_SPI_WRITE,
+	ASPEED_SPI_MAX,
+};
+
+#define ASPEED_SPI_MAX_CS 5
+
+struct aspeed_spi_controller;
+struct aspeed_spi_chip;
+
+struct aspeed_spi_info {
+	uint32_t cmd_io_ctrl_mask;
+	uint32_t max_data_bus_width;
+	uint32_t min_decode_sz;
+	void (*set_4byte)(struct aspeed_spi_controller *ast_ctrl, uint32_t cs);
+	int (*calibrate)(struct aspeed_spi_controller *ast_ctrl, uint32_t cs);
+	void (*adjust_decode_sz)(uint32_t decode_sz_arr[], int len);
+	uint32_t (*segment_start)(struct aspeed_spi_controller *ast_ctrl,
+				  uint32_t reg);
+	uint32_t (*segment_end)(struct aspeed_spi_controller *ast_ctrl,
+				uint32_t reg);
+	uint32_t (*segment_reg)(struct aspeed_spi_controller *ast_ctrl,
+				uint32_t start, uint32_t end);
+};
+
+struct aspeed_spi_chip {
+	void __iomem *ahb_base;
+	phys_addr_t ahb_base_phy;
+	uint32_t ahb_window_sz;
+	uint32_t ctrl_val[ASPEED_SPI_MAX];
+	uint32_t max_clk_freq;
+};
+
+struct aspeed_spi_controller {
+	struct device *dev;
+	const struct aspeed_spi_info *info; /* controller info */
+	void __iomem *regs; /* controller registers */
+	void __iomem *ahb_base;
+	phys_addr_t ahb_base_phy; /* physical addr of AHB window */
+	uint32_t ahb_window_sz; /* AHB window size */
+	uint32_t num_cs;
+	uint64_t ahb_clk;
+	struct aspeed_spi_chip *chips; /* pointers to attached chips */
+};
+
+static uint32_t
+aspeed_2600_spi_segment_start(struct aspeed_spi_controller *ast_ctrl,
+			      uint32_t reg)
+{
+	uint32_t start_offset = (reg << 16) & 0x0ff00000;
+
+	return (uint32_t)(ast_ctrl->ahb_base_phy + start_offset);
+}
+
+static uint32_t
+aspeed_2600_spi_segment_end(struct aspeed_spi_controller *ast_ctrl,
+			    uint32_t reg)
+{
+	uint32_t end_offset = reg & 0x0ff00000;
+
+	/* no decode range, set to physical ahb base */
+	if (end_offset == 0)
+		return ast_ctrl->ahb_base_phy;
+
+	return (uint32_t)(ast_ctrl->ahb_base_phy + end_offset + 0x100000);
+}
+
+static uint32_t
+aspeed_2600_spi_segment_reg(struct aspeed_spi_controller *ast_ctrl,
+			    uint32_t start, uint32_t end)
+{
+	/* no decode range, assign zero value */
+	if (start == end)
+		return 0;
+
+	return ((start & 0x0ff00000) >> 16) | ((end - 0x100000) & 0x0ff00000);
+}
+
+static void aspeed_spi_chip_set_4byte(struct aspeed_spi_controller *ast_ctrl,
+				      uint32_t cs)
+{
+	uint32_t reg_val;
+
+	reg_val = readl(ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
+	reg_val |= 0x11 << cs;
+	writel(reg_val, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
+}
+
+static uint32_t aspeed_spi_get_io_mode(uint32_t bus_width)
+{
+	switch (bus_width) {
+	case 1:
+		return CTRL_IO_SINGLE_DATA;
+	case 2:
+		return CTRL_IO_DUAL_DATA;
+	case 4:
+		return CTRL_IO_QUAD_DATA;
+	default:
+		return CTRL_IO_SINGLE_DATA;
+	}
+}
+
+/*
+ * Check whether the data is not all 0 or 1 in order to
+ * avoid calibriate umount spi-flash.
+ */
+static bool aspeed_spi_calibriation_enable(const uint8_t *buf, uint32_t sz)
+{
+	const uint32_t *buf_32 = (const uint32_t *)buf;
+	uint32_t i;
+	uint32_t valid_count = 0;
+
+	for (i = 0; i < (sz / 4); i++) {
+		if (buf_32[i] != 0 && buf_32[i] != 0xffffffff)
+			valid_count++;
+		if (valid_count > 100)
+			return true;
+	}
+
+	return false;
+}
+
+static uint32_t
+aspeed_2600_spi_dma_checksum(struct aspeed_spi_controller *ast_ctrl,
+			     uint32_t cs, uint32_t div, uint32_t delay)
+{
+	uint32_t ctrl_val;
+	uint32_t checksum;
+
+	writel(0xaeed0000, ast_ctrl->regs + OFFSET_DMA_CTRL);
+	if (readl(ast_ctrl->regs + OFFSET_DMA_CTRL) & SPI_DAM_REQUEST) {
+		while (!(readl(ast_ctrl->regs + OFFSET_DMA_CTRL) &
+			 SPI_DAM_GRANT))
+			;
+	}
+
+	writel((uint32_t)ast_ctrl->chips[cs].ahb_base_phy,
+	       ast_ctrl->regs + OFFSET_DMA_FLASH_ADDR_REG);
+	writel(CALIBRATION_LEN, ast_ctrl->regs + OFFSET_DMA_LEN_REG);
+
+	ctrl_val = SPI_DMA_ENABLE | SPI_DMA_CALC_CKSUM | SPI_DMA_CALIB_MODE |
+		   (delay << 8) | ((div & 0xf) << 16);
+	writel(ctrl_val, ast_ctrl->regs + OFFSET_DMA_CTRL);
+	while (!(readl(ast_ctrl->regs + OFFSET_INTR_CTRL_STATUS) &
+		 SPI_DMA_STATUS))
+		;
+
+	checksum = readl(ast_ctrl->regs + OFFSET_DMA_CHECKSUM_RESULT);
+
+	writel(0xdeea0000, ast_ctrl->regs + OFFSET_DMA_CTRL);
+	writel(0x0, ast_ctrl->regs + OFFSET_DMA_CTRL);
+
+	return checksum;
+}
+
+static int get_mid_point_of_longest_one(uint8_t *buf, uint32_t len)
+{
+	int i;
+	int start = 0, mid_point = 0;
+	int max_cnt = 0, cnt = 0;
+
+	for (i = 0; i < len; i++) {
+		if (buf[i] == 1) {
+			cnt++;
+		} else {
+			cnt = 0;
+			start = i;
+		}
+
+		if (max_cnt < cnt) {
+			max_cnt = cnt;
+			mid_point = start + (cnt / 2);
+		}
+	}
+
+	/*
+	 * In order to get a stable SPI read timing,
+	 * abandon the result if the length of longest
+	 * consecutive good points is too short.
+	 */
+	if (max_cnt < 4)
+		return -1;
+
+	return mid_point;
+}
+
+/* Transfer maximum clock frequency to register setting */
+static uint32_t
+aspeed_2600_spi_clk_basic_setting(struct aspeed_spi_controller *ast_ctrl,
+				  uint32_t *max_clk)
+{
+	struct device *dev = ast_ctrl->dev;
+	uint32_t hclk_clk = ast_ctrl->ahb_clk;
+	uint32_t hclk_div = 0x400; /* default value */
+	uint32_t i, j = 0;
+	bool found = false;
+	/* HCLK/1 ..	HCLK/16 */
+	uint32_t hclk_masks[] = { 15, 7, 14, 6, 13, 5, 12, 4,
+				  11, 3, 10, 2, 9,  1, 8,  0 };
+
+	/* FMC/SPIR10[27:24] */
+	for (j = 0; j < 0xf; i++) {
+		/* FMC/SPIR10[11:8] */
+		for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
+			if (i == 0 && j == 0)
+				continue;
+
+			if (hclk_clk / (i + 1 + (j * 16)) <= *max_clk) {
+				found = 1;
+				*max_clk = hclk_clk / (i + 1 + (j * 16));
+				break;
+			}
+		}
+
+		if (found) {
+			hclk_div = ((j << 24) | hclk_masks[i] << 8);
+			break;
+		}
+	}
+
+	dev_dbg(dev, "found: %s, hclk: %d, max_clk: %d\n", found ? "yes" : "no",
+		hclk_clk, *max_clk);
+	dev_dbg(dev, "base_clk: %d, h_div: %d (mask %x), speed: %d\n", j, i + 1,
+		hclk_masks[i], hclk_clk / (i + 1 + j * 16));
+
+	return hclk_div;
+}
+
+/*
+ * If SPI frequency is too high, timing compensation is needed,
+ * otherwise, SPI controller will sample unready data. For AST2600
+ * SPI memory controller, only the first four frequency levels
+ * (HCLK/2, HCLK/3,..., HCKL/5) may need timing compensation.
+ * Here, for each frequency, we will get a sequence of reading
+ * result (pass or fail) compared to golden data. Then, getting the
+ * middle point of the maximum pass widow. Besides, if the flash's
+ * content is too monotonous, the frequency recorded in the device
+ * tree will be adopted.
+ */
+static int
+aspeed_2600_spi_timing_calibration(struct aspeed_spi_controller *ast_ctrl,
+				   uint32_t cs)
+{
+	int ret = 0;
+	struct device *dev = ast_ctrl->dev;
+	struct aspeed_spi_chip *chip = &ast_ctrl->chips[cs];
+	uint32_t max_freq = chip->max_clk_freq;
+	/* HCLK/2, ..., HCKL/5 */
+	uint32_t hclk_masks[] = { 7, 14, 6, 13 };
+	uint8_t *calib_res = NULL;
+	uint8_t *check_buf = NULL;
+	uint32_t reg_val;
+	uint32_t checksum, gold_checksum;
+	uint32_t i, hcycle, delay_ns, final_delay = 0;
+	uint32_t hclk_div;
+	bool pass;
+	int calib_point;
+
+	reg_val =
+		readl(ast_ctrl->regs + OFFSET_CE0_TIMING_COMPENSATION + cs * 4);
+	if (reg_val != 0) {
+		dev_dbg(dev, "has executed calibration.\n");
+		goto no_calib;
+	}
+
+	dev_dbg(dev, "calculate timing compensation :\n");
+	/*
+	 * use the related low frequency to get check calibration data
+	 * and get golden data.
+	 */
+	reg_val = chip->ctrl_val[ASPEED_SPI_READ] & 0xf0fff0ff;
+	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
+
+	check_buf = kzalloc(CALIBRATION_LEN, GFP_KERNEL);
+	if (!check_buf)
+		return -ENOMEM;
+
+	memcpy_fromio(check_buf, chip->ahb_base, CALIBRATION_LEN);
+	if (!aspeed_spi_calibriation_enable(check_buf, CALIBRATION_LEN)) {
+		dev_info(dev, "flash data is monotonous, skip calibration.");
+		goto no_calib;
+	}
+
+	gold_checksum = aspeed_2600_spi_dma_checksum(ast_ctrl, cs, 0, 0);
+
+	/*
+	 * allocate a space to record calibration result for
+	 * different timing compensation with fixed
+	 * HCLK division.
+	 */
+	calib_res = kzalloc(6 * 17, GFP_KERNEL);
+	if (!calib_res) {
+		ret = -ENOMEM;
+		goto no_calib;
+	}
+
+	/* From HCLK/2 to HCLK/5 */
+	for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
+		if (max_freq < (uint32_t)ast_ctrl->ahb_clk / (i + 2)) {
+			dev_dbg(dev, "skipping freq %d\n",
+				(uint32_t)ast_ctrl->ahb_clk / (i + 2));
+			continue;
+		}
+		max_freq = (uint32_t)ast_ctrl->ahb_clk / (i + 2);
+
+		checksum = aspeed_2600_spi_dma_checksum(ast_ctrl, cs,
+							hclk_masks[i], 0);
+		pass = (checksum == gold_checksum);
+		dev_dbg(dev, "HCLK/%d, no timing compensation: %s\n", i + 2,
+			pass ? "PASS" : "FAIL");
+
+		if (pass)
+			break;
+
+		memset(calib_res, 0x0, 6 * 17);
+
+		for (hcycle = 0; hcycle <= 5; hcycle++) {
+			/* increase DI delay by the step of 0.5ns */
+			dev_dbg(dev, "Delay Enable : hcycle %x\n", hcycle);
+			for (delay_ns = 0; delay_ns <= 0xf; delay_ns++) {
+				checksum = aspeed_2600_spi_dma_checksum(
+					ast_ctrl, cs, hclk_masks[i],
+					BIT(3) | hcycle | (delay_ns << 4));
+				pass = (checksum == gold_checksum);
+				calib_res[hcycle * 17 + delay_ns] = pass;
+				dev_dbg(dev,
+					"HCLK/%d, %d HCLK cycle, %d delay_ns : %s\n",
+					i + 2, hcycle, delay_ns,
+					pass ? "PASS" : "FAIL");
+			}
+		}
+
+		calib_point = get_mid_point_of_longest_one(calib_res, 6 * 17);
+		if (calib_point < 0) {
+			dev_info(dev, "cannot get good calibration point.\n");
+			continue;
+		}
+
+		hcycle = calib_point / 17;
+		delay_ns = calib_point % 17;
+		dev_dbg(dev, "final hcycle: %d, delay_ns: %d\n", hcycle,
+			delay_ns);
+
+		final_delay = (BIT(3) | hcycle | (delay_ns << 4)) << (i * 8);
+		writel(final_delay, ast_ctrl->regs +
+					    OFFSET_CE0_TIMING_COMPENSATION +
+					    cs * 4);
+		break;
+	}
+
+no_calib:
+
+	hclk_div = aspeed_2600_spi_clk_basic_setting(ast_ctrl, &max_freq);
+
+	/* configure SPI clock frequency */
+	reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
+	reg_val = (reg_val & 0xf0fff0ff) | hclk_div;
+	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
+
+	/* add clock setting info for CE ctrl setting */
+	for (i = 0; i < ASPEED_SPI_MAX; i++)
+		chip->ctrl_val[i] = (chip->ctrl_val[i] & 0xf0fff0ff) | hclk_div;
+
+	dev_info(dev, "freq: %dMHz\n", max_freq / 1000000);
+
+	kfree(check_buf);
+	kfree(calib_res);
+
+	return ret;
+}
+
+/*
+ * AST2600 SPI memory controllers support multiple chip selects.
+ * The start address of a decode range should be multiple
+ * of its related flash size. Namely, the total decoded size
+ * from flash 0 to flash N should be multiple of flash (N + 1).
+ */
+static void aspeed_2600_adjust_decode_sz(uint32_t decode_sz_arr[], int len)
+{
+	int cs, j;
+	uint32_t sz;
+
+	for (cs = len - 1; cs >= 0; cs--) {
+		sz = 0;
+		for (j = 0; j < cs; j++)
+			sz += decode_sz_arr[j];
+
+		if (sz % decode_sz_arr[cs] != 0)
+			decode_sz_arr[0] += (sz % decode_sz_arr[cs]);
+	}
+}
+
+static int
+aspeed_spi_decode_range_config(struct aspeed_spi_controller *ast_ctrl,
+			       uint32_t decode_sz_arr[])
+{
+	struct aspeed_spi_chip *chip = ast_ctrl->chips;
+	uint32_t i;
+	uint32_t cs;
+	uint32_t decode_reg_val;
+	phys_addr_t start_addr_phy, end_addr_phy, pre_end_addr_phy = 0;
+	uint32_t total_decode_sz = 0;
+
+	/* decode range sanity */
+	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
+		total_decode_sz += decode_sz_arr[cs];
+		if (ast_ctrl->ahb_window_sz < total_decode_sz) {
+			dev_err(ast_ctrl->dev, "insufficient decode size\n");
+			for (i = 0; i <= cs; i++)
+				dev_err(ast_ctrl->dev, "cs:%d %x\n", i,
+					decode_sz_arr[i]);
+			return -ENOSPC;
+		}
+	}
+
+	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
+		if (chip[cs].ahb_base)
+			devm_iounmap(ast_ctrl->dev, chip[cs].ahb_base);
+	}
+
+	/* configure each CE's decode range */
+	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
+		if (cs == 0)
+			start_addr_phy = ast_ctrl->ahb_base_phy;
+		else
+			start_addr_phy = pre_end_addr_phy;
+
+		chip[cs].ahb_base = devm_ioremap(ast_ctrl->dev, start_addr_phy,
+						 decode_sz_arr[cs]);
+		chip[cs].ahb_base_phy = start_addr_phy;
+
+		chip[cs].ahb_window_sz = decode_sz_arr[cs];
+		end_addr_phy = start_addr_phy + decode_sz_arr[cs];
+
+		decode_reg_val = ast_ctrl->info->segment_reg(
+			ast_ctrl, start_addr_phy, end_addr_phy);
+
+		writel(decode_reg_val,
+		       ast_ctrl->regs + OFFSET_CE0_DECODE_RANGE_REG + cs * 4);
+
+		pre_end_addr_phy = end_addr_phy;
+
+		dev_dbg(ast_ctrl->dev, "cs: %d, decode_reg: 0x%x\n", cs,
+			decode_reg_val);
+	}
+
+	return 0;
+}
+
+static const struct aspeed_spi_info ast2600_spi_info = {
+	.max_data_bus_width = 4,
+	.cmd_io_ctrl_mask = 0xf0ff40c3,
+	/* for ast2600, the minimum decode size for each CE is 2MB */
+	.min_decode_sz = 0x200000,
+	.set_4byte = aspeed_spi_chip_set_4byte,
+	.calibrate = aspeed_2600_spi_timing_calibration,
+	.adjust_decode_sz = aspeed_2600_adjust_decode_sz,
+	.segment_start = aspeed_2600_spi_segment_start,
+	.segment_end = aspeed_2600_spi_segment_end,
+	.segment_reg = aspeed_2600_spi_segment_reg,
+};
+
+/*
+ * If the slave device is SPI NOR flash, there are two types
+ * of command mode for ASPEED SPI memory controller used to
+ * transfer data. The first one is user mode and the other is
+ * command read/write mode. With user mode, SPI NOR flash
+ * command, address and data processes are all handled by CPU.
+ * But, when address filter is enabled to protect some flash
+ * regions from being written, user mode will be disabled.
+ * Thus, here, we use command read/write mode to issue SPI
+ * operations. After remapping flash space correctly, we can
+ * easily read/write data to flash by reading or writing
+ * related remapped address, then, SPI NOR flash command and
+ * address will be transferred to flash by controller
+ * automatically. Besides, ASPEED SPI memory controller can
+ * also block address or data bytes by configure FMC0C/SPIR0C
+ * address and data mask register in order to satisfy the
+ * following SPI flash operation sequences: (command) only,
+ * (command and address) only or (coommand and data) only.
+ */
+static int aspeed_spi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
+{
+	struct aspeed_spi_controller *ast_ctrl =
+		spi_controller_get_devdata(mem->spi->master);
+	struct device *dev = ast_ctrl->dev;
+	uint32_t cs = mem->spi->chip_select;
+	struct aspeed_spi_chip *chip = &ast_ctrl->chips[cs];
+	uint32_t ctrl_val;
+	uint32_t addr_mode_reg, addr_mode_reg_backup;
+	uint32_t addr_data_mask = 0;
+	void __iomem *op_addr;
+	const void *data_buf;
+	uint32_t data_byte = 0;
+	uint32_t dummy_data = 0;
+
+	dev_dbg(dev, "cmd:%x(%d),addr:%llx(%d),dummy:%d(%d),data_len:%x(%d)\n",
+		op->cmd.opcode, op->cmd.buswidth, op->addr.val,
+		op->addr.buswidth, op->dummy.nbytes, op->dummy.buswidth,
+		op->data.nbytes, op->data.buswidth);
+
+	addr_mode_reg = addr_mode_reg_backup =
+		readl(ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
+	addr_data_mask = readl(ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
+
+	ctrl_val = chip->ctrl_val[ASPEED_SPI_BASE];
+	ctrl_val &= ~ast_ctrl->info->cmd_io_ctrl_mask;
+
+	/* configure opcode */
+	ctrl_val |= op->cmd.opcode << 16;
+
+	/* configure operation address, address length and address mask */
+	if (op->addr.nbytes != 0) {
+		if (op->addr.nbytes == 3)
+			addr_mode_reg &= ~(0x11 << cs);
+		else
+			addr_mode_reg |= (0x11 << cs);
+
+		addr_data_mask &= 0x0f;
+		op_addr = chip->ahb_base + op->addr.val;
+	} else {
+		addr_data_mask |= 0xf0;
+		op_addr = chip->ahb_base;
+	}
+
+	if (op->dummy.nbytes != 0) {
+		ctrl_val |= ((op->dummy.nbytes & 0x3) << 6 |
+			     (op->dummy.nbytes & 0x4) << 14);
+	}
+
+	/* configure data io mode and data mask */
+	if (op->data.nbytes != 0) {
+		addr_data_mask &= 0xF0;
+		data_byte = op->data.nbytes;
+		if (op->data.dir == SPI_MEM_DATA_OUT)
+			data_buf = op->data.buf.out;
+		else
+			data_buf = op->data.buf.in;
+
+		if (op->data.buswidth)
+			ctrl_val |= aspeed_spi_get_io_mode(op->data.buswidth);
+
+	} else {
+		addr_data_mask |= 0x0f;
+		data_byte = 1;
+		data_buf = &dummy_data;
+	}
+
+	/* configure command mode */
+	if (op->data.dir == SPI_MEM_DATA_OUT)
+		ctrl_val |= CTRL_IO_MODE_CMD_WRITE;
+	else
+		ctrl_val |= CTRL_IO_MODE_CMD_READ;
+
+	/* set controller registers */
+	writel(ctrl_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
+	writel(addr_mode_reg, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
+	writel(addr_data_mask, ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
+
+	dev_dbg(dev, "ctrl: 0x%08x, addr_mode: 0x%x, mask: 0x%x, addr:%px\n",
+		ctrl_val, addr_mode_reg, addr_data_mask, op_addr);
+
+	/* trigger spi transmission or reception sequence */
+	if (op->data.dir == SPI_MEM_DATA_OUT)
+		memcpy_toio(op_addr, data_buf, data_byte);
+	else
+		memcpy_fromio((void *)data_buf, op_addr, data_byte);
+
+	/* restore controller setting */
+	writel(chip->ctrl_val[ASPEED_SPI_READ],
+	       ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
+	writel(addr_mode_reg_backup, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
+	writel(0x0, ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
+
+	return 0;
+}
+
+static ssize_t aspeed_spi_dirmap_read(struct spi_mem_dirmap_desc *desc,
+				  uint64_t offs, size_t len, void *buf)
+{
+	struct aspeed_spi_controller *ast_ctrl =
+		spi_controller_get_devdata(desc->mem->spi->master);
+	struct aspeed_spi_chip *chip =
+		&ast_ctrl->chips[desc->mem->spi->chip_select];
+	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
+
+	if (chip->ahb_window_sz < offs + len) {
+		dev_info(ast_ctrl->dev,
+			 "read range exceeds flash remapping size\n");
+		return 0;
+	}
+
+	dev_dbg(ast_ctrl->dev, "read op:0x%x, addr:0x%llx, len:0x%zx\n",
+		op_tmpl.cmd.opcode, offs, len);
+
+	memcpy_fromio(buf, chip->ahb_base + offs, len);
+
+	return len;
+}
+
+static ssize_t aspeed_spi_dirmap_write(struct spi_mem_dirmap_desc *desc,
+				   uint64_t offs, size_t len, const void *buf)
+{
+	struct aspeed_spi_controller *ast_ctrl =
+		spi_controller_get_devdata(desc->mem->spi->master);
+	struct aspeed_spi_chip *chip =
+		&ast_ctrl->chips[desc->mem->spi->chip_select];
+	uint32_t reg_val;
+	uint32_t target_cs = desc->mem->spi->chip_select;
+	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
+
+	if (chip->ahb_window_sz < offs + len) {
+		dev_info(ast_ctrl->dev,
+			 "write range exceeds flash remapping size\n");
+		return 0;
+	}
+
+	dev_dbg(ast_ctrl->dev, "write op:0x%x, addr:0x%llx, len:0x%zx\n",
+		op_tmpl.cmd.opcode, offs, len);
+
+	reg_val = ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE];
+	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs * 4);
+
+	memcpy_toio(chip->ahb_base + offs, buf, len);
+
+	reg_val = ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ];
+	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs * 4);
+
+	return len;
+}
+
+static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
+{
+	int ret = 0;
+	struct aspeed_spi_controller *ast_ctrl =
+		spi_controller_get_devdata(desc->mem->spi->master);
+	struct device *dev = ast_ctrl->dev;
+	const struct aspeed_spi_info *info = ast_ctrl->info;
+	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
+	uint32_t decode_sz_arr[5];
+	uint32_t cs, target_cs = desc->mem->spi->chip_select;
+	uint32_t reg_val;
+
+	if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN) {
+		/* record original decode size */
+		for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
+			reg_val = readl(ast_ctrl->regs +
+					OFFSET_CE0_DECODE_RANGE_REG + cs * 4);
+			decode_sz_arr[cs] =
+				info->segment_end(ast_ctrl, reg_val) -
+				info->segment_start(ast_ctrl, reg_val);
+		}
+
+		decode_sz_arr[target_cs] = desc->info.length;
+
+		if (info->adjust_decode_sz)
+			info->adjust_decode_sz(decode_sz_arr, ast_ctrl->num_cs);
+
+		for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
+			dev_dbg(dev, "cs: %d, sz: 0x%x\n", cs,
+				decode_sz_arr[cs]);
+		}
+
+		ret = aspeed_spi_decode_range_config(ast_ctrl, decode_sz_arr);
+		if (ret)
+			return ret;
+
+		reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG +
+				target_cs * 4) &
+			  (~info->cmd_io_ctrl_mask);
+		reg_val |= aspeed_spi_get_io_mode(op_tmpl.data.buswidth) |
+			   op_tmpl.cmd.opcode << 16 |
+			   ((op_tmpl.dummy.nbytes) & 0x3) << 6 |
+			   ((op_tmpl.dummy.nbytes) & 0x4) << 14 |
+			   CTRL_IO_MODE_CMD_READ;
+
+		writel(reg_val,
+		       ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs * 4);
+		ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ] = reg_val;
+		ast_ctrl->chips[target_cs].max_clk_freq =
+			desc->mem->spi->max_speed_hz;
+
+		ret = info->calibrate(ast_ctrl, target_cs);
+
+		dev_info(dev, "read bus width: %d [0x%08x]\n",
+			 op_tmpl.data.buswidth,
+			 ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ]);
+
+	} else if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_OUT) {
+		reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG +
+				target_cs * 4) &
+			  (~info->cmd_io_ctrl_mask);
+		reg_val |= aspeed_spi_get_io_mode(op_tmpl.data.buswidth) |
+			   op_tmpl.cmd.opcode << 16 | CTRL_IO_MODE_CMD_WRITE;
+		ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE] = reg_val;
+
+		dev_info(dev, "write bus width: %d [0x%08x]\n",
+			 op_tmpl.data.buswidth,
+			 ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE]);
+	}
+
+	return ret;
+}
+
+static const char *aspeed_spi_get_name(struct spi_mem *mem)
+{
+	struct device *dev = &mem->spi->master->dev;
+	const char *name;
+
+	name = devm_kasprintf(dev, GFP_KERNEL, "%s-%d", dev_name(dev),
+			      mem->spi->chip_select);
+
+	if (!name) {
+		dev_err(dev, "cannot get spi name\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	return name;
+}
+
+/*
+ * Currently, only support 1-1-1, 1-1-2 or 1-1-4
+ * SPI NOR flash operation format.
+ */
+static bool aspeed_spi_support_op(struct spi_mem *mem,
+				  const struct spi_mem_op *op)
+{
+	struct aspeed_spi_controller *ast_ctrl =
+		spi_controller_get_devdata(mem->spi->master);
+
+	if (op->cmd.buswidth > 1)
+		return false;
+
+	if (op->addr.nbytes != 0) {
+		if (op->addr.buswidth > 1 || op->addr.nbytes > 4)
+			return false;
+	}
+
+	if (op->dummy.nbytes != 0) {
+		if (op->dummy.buswidth > 1 || op->dummy.nbytes > 7)
+			return false;
+	}
+
+	if (op->data.nbytes != 0 &&
+	    ast_ctrl->info->max_data_bus_width < op->data.buswidth)
+		return false;
+
+	if (!spi_mem_default_supports_op(mem, op))
+		return false;
+
+	if (op->addr.nbytes == 4)
+		ast_ctrl->info->set_4byte(ast_ctrl, mem->spi->chip_select);
+
+	return true;
+}
+
+static const struct spi_controller_mem_ops aspeed_spi_mem_ops = {
+	.exec_op = aspeed_spi_exec_op,
+	.get_name = aspeed_spi_get_name,
+	.supports_op = aspeed_spi_support_op,
+	.dirmap_create = aspeed_spi_dirmap_create,
+	.dirmap_read = aspeed_spi_dirmap_read,
+	.dirmap_write = aspeed_spi_dirmap_write,
+};
+
+/*
+ * Initialize SPI controller for each chip select.
+ * Here, only the minimum decode range is configured
+ * in order to get device (SPI NOR flash) information
+ * at the early stage.
+ */
+static int aspeed_spi_ctrl_init(struct aspeed_spi_controller *ast_ctrl)
+{
+	int ret;
+	uint32_t cs;
+	uint32_t val;
+	uint32_t decode_sz_arr[ASPEED_SPI_MAX_CS];
+
+	/* enable write capability for all CEs */
+	val = readl(ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
+	writel(val | (GENMASK(ast_ctrl->num_cs, 0) << 16),
+	       ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
+
+	/* initial each CE's controller register */
+	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
+		val = CTRL_STOP_ACTIVE | CTRL_IO_MODE_USER;
+		writel(val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
+		ast_ctrl->chips[cs].ctrl_val[ASPEED_SPI_BASE] = val;
+	}
+
+	for (cs = 0; cs < ast_ctrl->num_cs && cs < ASPEED_SPI_MAX_CS; cs++)
+		decode_sz_arr[cs] = ast_ctrl->info->min_decode_sz;
+
+	ret = aspeed_spi_decode_range_config(ast_ctrl, decode_sz_arr);
+
+	return ret;
+}
+
+static const struct of_device_id aspeed_spi_matches[] = {
+	{ .compatible = "aspeed,ast2600-fmc", .data = &ast2600_spi_info },
+	{ .compatible = "aspeed,ast2600-spi", .data = &ast2600_spi_info },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, aspeed_spi_matches);
+
+static int aspeed_spi_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct spi_controller *spi_ctrl;
+	struct aspeed_spi_controller *ast_ctrl;
+	const struct of_device_id *match;
+	struct clk *clk;
+	struct resource *res;
+
+	spi_ctrl = spi_alloc_master(dev, sizeof(struct aspeed_spi_controller));
+	if (!spi_ctrl)
+		return -ENOMEM;
+
+	ast_ctrl = spi_controller_get_devdata(spi_ctrl);
+
+	match = of_match_device(aspeed_spi_matches, dev);
+	if (!match || !match->data) {
+		dev_err(dev, "no compatible OF match\n");
+		return -ENODEV;
+	}
+
+	ast_ctrl->info = match->data;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+					   "spi_ctrl_reg");
+	ast_ctrl->regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ast_ctrl->regs))
+		return PTR_ERR(ast_ctrl->regs);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "spi_mmap");
+	ast_ctrl->ahb_base_phy = res->start;
+	ast_ctrl->ahb_window_sz = resource_size(res);
+
+	ast_ctrl->dev = dev;
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	ast_ctrl->ahb_clk = clk_get_rate(clk);
+	devm_clk_put(&pdev->dev, clk);
+
+	if (of_property_read_u32(dev->of_node, "num-cs", &ast_ctrl->num_cs)) {
+		dev_err(dev, "fail to get chip number.\n");
+		goto end;
+	}
+
+	if (ast_ctrl->num_cs > ASPEED_SPI_MAX_CS) {
+		dev_err(dev, "chip number, %d, exceeds %d.\n", ast_ctrl->num_cs,
+			ASPEED_SPI_MAX_CS);
+		goto end;
+	}
+
+	ast_ctrl->chips =
+		devm_kzalloc(dev,
+			     sizeof(struct aspeed_spi_chip) * ast_ctrl->num_cs,
+			     GFP_KERNEL);
+
+	platform_set_drvdata(pdev, ast_ctrl);
+
+	spi_ctrl->mode_bits =
+		SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
+
+	spi_ctrl->bus_num = -1;
+	spi_ctrl->mem_ops = &aspeed_spi_mem_ops;
+	spi_ctrl->dev.of_node = dev->of_node;
+	spi_ctrl->num_chipselect = ast_ctrl->num_cs;
+
+	ret = aspeed_spi_ctrl_init(ast_ctrl);
+	if (ret)
+		goto end;
+
+	ret = devm_spi_register_master(dev, spi_ctrl);
+
+end:
+	return ret;
+}
+
+static int aspeed_spi_remove(struct platform_device *pdev)
+{
+	struct aspeed_spi_controller *ast_ctrl = platform_get_drvdata(pdev);
+	uint32_t val;
+
+	/* disable write capability for all CEs */
+	val = readl(ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
+	writel(val & ~(GENMASK(ast_ctrl->num_cs, 0) << 16),
+	       ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
+
+	return 0;
+}
+
+static struct platform_driver aspeed_spi_driver = {
+	.driver = {
+		.name = "ASPEED_FMC_SPI",
+		.bus = &platform_bus_type,
+		.of_match_table = aspeed_spi_matches,
+	},
+	.probe = aspeed_spi_probe,
+	.remove = aspeed_spi_remove,
+};
+module_platform_driver(aspeed_spi_driver);
+
+MODULE_DESCRIPTION("ASPEED FMC/SPI Memory Controller Driver");
+MODULE_AUTHOR("Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>");
+MODULE_AUTHOR("Cedric Le Goater <clg@kaod.org>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-05 12:03 ` [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
@ 2020-11-05 14:09   ` Cédric Le Goater
  2020-11-05 15:11     ` Boris Brezillon
  2020-11-06  7:38     ` Chin-Ting Kuo
  0 siblings, 2 replies; 20+ messages in thread
From: Cédric Le Goater @ 2020-11-05 14:09 UTC (permalink / raw)
  To: Chin-Ting Kuo, broonie, robh+dt, joel, andrew, bbrezillon,
	devicetree, linux-kernel, linux-aspeed, linux-spi
  Cc: BMC-SW

Hello Chin-Ting,

Thanks for this driver. It's much cleaner than the previous and we should 
try adding support for the AST2500 SoC also. I guess we can keep the old 
driver for the AST2400 which has a different register layout.

On the patchset, I think we should split this patch in three : 

 - basic support
 - AHB window calculation depending on the flash size
 - read training support  

We should avoid magic values when setting registers. This is confusing 
and defines are much better.
 
AST2500 support will be a bit challenging because HW does not allow    
to configure a 128MB AHB window, max is 120MB This is a bug and the work 
around is to use user mode for the remaining 8MB. Something to keep in
mind.

I gave it a try on QEMU. It looks good. When I can revive my EVB, I will
do the same.

More comments below, 

Thanks,

C.


On 11/5/20 1:03 PM, Chin-Ting Kuo wrote:
> Add driver for ASPEED BMC FMC/SPI memory controller which
> supports spi-mem interface.
> 
> There are three SPI memory controllers embedded in an ASPEED SoC.
> Each of them can connect to two or three SPI NOR flashes. The first
> SPI memory controller is also named as Firmware Memory Controller (FMC),
> which is similar to SPI memory controller. After device AC on, MCU ROM
> can fetch device boot code from FMC CS 0. Thus, there exists additional
> registers for boot process control in FMC.
> 
> ASPEED SPI memory controller supports single, dual and quad mode for
> SPI NOR flash. It also supports two types of command mode, user mode
> and command read/write mode. User mode is traditional pure SPI operations
> where all transmission is controlled by CPU. Contrarily, with command
> read/write mode, SPI controller can send command and address automatically
> when CPU read/write related remapped address.
> 
> Besides, different wafer processes of SPI NOR flash result in different
> signal response time. This phenomenon will be enlarged when SPI clock
> frequency increases. ASPEED SPI memory controller provides a mechanism
> for timing compensation in order to satisfy various SPI NOR flash parts
> and PCB layout.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  v2: Fix sparse warnings reported by kernel test robot <lkp@intel.com>.
>  v3: Fix build warnings with x86 allmodconfig.
> 
>  drivers/spi/Kconfig      |  10 +
>  drivers/spi/Makefile     |   1 +
>  drivers/spi/spi-aspeed.c | 969 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 980 insertions(+)
>  create mode 100644 drivers/spi/spi-aspeed.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 5cff60de8e83..c848f2f7b694 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -70,6 +70,16 @@ config SPI_AR934X
>  	  This enables support for the SPI controller present on the
>  	  Qualcomm Atheros AR934X/QCA95XX SoCs.
>  
> +config SPI_ASPEED
> +	tristate "ASPEED FMC/SPI Memory Controller"
> +	depends on OF && HAS_IOMEM && (ARCH_ASPEED || COMPILE_TEST)

We will need to do something about the other driver. For the moment,
we can select both but that won't be the case anymore when we add
AST2500 support.

> +	help
> +	  Enable driver for ASPEED FMC/SPI Memory Controller.
> +
> +	  This driver is not a generic pure SPI driver, which
> +	  is especially designed for spi-mem framework with
> +	  SPI NOR flash direct read and write features.
> +
>  config SPI_ATH79
>  	tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
>  	depends on ATH79 || COMPILE_TEST
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 6fea5821662e..9e62c650fca0 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST)		+= spi-loopback-test.o
>  obj-$(CONFIG_SPI_ALTERA)		+= spi-altera.o
>  obj-$(CONFIG_SPI_AR934X)		+= spi-ar934x.o
>  obj-$(CONFIG_SPI_ARMADA_3700)		+= spi-armada-3700.o
> +obj-$(CONFIG_SPI_ASPEED)		+= spi-aspeed.o
>  obj-$(CONFIG_SPI_ATMEL)			+= spi-atmel.o
>  obj-$(CONFIG_SPI_ATMEL_QUADSPI)		+= atmel-quadspi.o
>  obj-$(CONFIG_SPI_AT91_USART)		+= spi-at91-usart.o
> diff --git a/drivers/spi/spi-aspeed.c b/drivers/spi/spi-aspeed.c
> new file mode 100644
> index 000000000000..cfaaa0d5bac6
> --- /dev/null
> +++ b/drivers/spi/spi-aspeed.c
> @@ -0,0 +1,969 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * ASPEED FMC/SPI Memory Controller Driver
> + *
> + * Copyright (c) 2020, ASPEED Corporation.
> + * Copyright (c) 2015-2016, IBM Corporation.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/sizes.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +
> +/* ASPEED FMC/SPI memory control register related */
> +#define OFFSET_CE_TYPE_SETTING		0x00
> +#define OFFSET_CE_ADDR_MODE_CTRL	0x04
> +#define OFFSET_INTR_CTRL_STATUS		0x08
> +#define OFFSET_ADDR_DATA_MASK		0x0c
> +#define OFFSET_CE0_CTRL_REG		0x10
> +#define OFFSET_CE0_DECODE_RANGE_REG	0x30
> +#define OFFSET_DMA_CTRL			0x80
> +#define OFFSET_DMA_FLASH_ADDR_REG	0x84
> +#define OFFSET_DMA_RAM_ADDR_REG		0x88
> +#define OFFSET_DMA_LEN_REG		0x8c
> +#define OFFSET_DMA_CHECKSUM_RESULT	0x90
> +#define OFFSET_CE0_TIMING_COMPENSATION	0x94
> +
> +#define CTRL_IO_SINGLE_DATA	0
> +#define CTRL_IO_DUAL_DATA	BIT(29)
> +#define CTRL_IO_QUAD_DATA	BIT(30)
> +
> +#define CTRL_IO_MODE_USER	GENMASK(1, 0)
> +#define CTRL_IO_MODE_CMD_READ	BIT(0)
> +#define CTRL_IO_MODE_CMD_WRITE	BIT(1)
> +#define CTRL_STOP_ACTIVE	BIT(2)
> +
> +#define CALIBRATION_LEN		0x400
> +#define SPI_DAM_REQUEST		BIT(31)
> +#define SPI_DAM_GRANT		BIT(30)

What are these bits ? There is no documentation for them.

> +#define SPI_DMA_CALIB_MODE	BIT(3)
> +#define SPI_DMA_CALC_CKSUM	BIT(2)
> +#define SPI_DMA_ENABLE		BIT(0)
> +#define SPI_DMA_STATUS		BIT(11)
> +
> +enum aspeed_spi_ctl_reg_value {
> +	ASPEED_SPI_BASE,
> +	ASPEED_SPI_READ,
> +	ASPEED_SPI_WRITE,
> +	ASPEED_SPI_MAX,
> +};
> +
> +#define ASPEED_SPI_MAX_CS 5
> +
> +struct aspeed_spi_controller;
> +struct aspeed_spi_chip;
> +
> +struct aspeed_spi_info {
> +	uint32_t cmd_io_ctrl_mask;
> +	uint32_t max_data_bus_width;
> +	uint32_t min_decode_sz;
> +	void (*set_4byte)(struct aspeed_spi_controller *ast_ctrl, uint32_t cs);
> +	int (*calibrate)(struct aspeed_spi_controller *ast_ctrl, uint32_t cs);
> +	void (*adjust_decode_sz)(uint32_t decode_sz_arr[], int len);
> +	uint32_t (*segment_start)(struct aspeed_spi_controller *ast_ctrl,
> +				  uint32_t reg);
> +	uint32_t (*segment_end)(struct aspeed_spi_controller *ast_ctrl,
> +				uint32_t reg);
> +	uint32_t (*segment_reg)(struct aspeed_spi_controller *ast_ctrl,
> +				uint32_t start, uint32_t end);
> +};
> +
> +struct aspeed_spi_chip {
> +	void __iomem *ahb_base;
> +	phys_addr_t ahb_base_phy;
> +	uint32_t ahb_window_sz;
> +	uint32_t ctrl_val[ASPEED_SPI_MAX];
> +	uint32_t max_clk_freq;
> +};
> +
> +struct aspeed_spi_controller {
> +	struct device *dev;
> +	const struct aspeed_spi_info *info; /* controller info */
> +	void __iomem *regs; /* controller registers */
> +	void __iomem *ahb_base;
> +	phys_addr_t ahb_base_phy; /* physical addr of AHB window */
> +	uint32_t ahb_window_sz; /* AHB window size */
> +	uint32_t num_cs;
> +	uint64_t ahb_clk;
> +	struct aspeed_spi_chip *chips; /* pointers to attached chips */
> +};
> +
> +static uint32_t
> +aspeed_2600_spi_segment_start(struct aspeed_spi_controller *ast_ctrl,
> +			      uint32_t reg)
> +{
> +	uint32_t start_offset = (reg << 16) & 0x0ff00000;
> +
> +	return (uint32_t)(ast_ctrl->ahb_base_phy + start_offset);
> +}
> +
> +static uint32_t
> +aspeed_2600_spi_segment_end(struct aspeed_spi_controller *ast_ctrl,
> +			    uint32_t reg)
> +{
> +	uint32_t end_offset = reg & 0x0ff00000;
> +
> +	/* no decode range, set to physical ahb base */
> +	if (end_offset == 0)
> +		return ast_ctrl->ahb_base_phy;
> +
> +	return (uint32_t)(ast_ctrl->ahb_base_phy + end_offset + 0x100000);
> +}
> +
> +static uint32_t
> +aspeed_2600_spi_segment_reg(struct aspeed_spi_controller *ast_ctrl,
> +			    uint32_t start, uint32_t end)
> +{
> +	/* no decode range, assign zero value */
> +	if (start == end)
> +		return 0;
> +
> +	return ((start & 0x0ff00000) >> 16) | ((end - 0x100000) & 0x0ff00000);
> +}
> +
> +static void aspeed_spi_chip_set_4byte(struct aspeed_spi_controller *ast_ctrl,
> +				      uint32_t cs)
> +{
> +	uint32_t reg_val;
> +
> +	reg_val = readl(ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> +	reg_val |= 0x11 << cs;
> +	writel(reg_val, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> +}
> +
> +static uint32_t aspeed_spi_get_io_mode(uint32_t bus_width)
> +{
> +	switch (bus_width) {
> +	case 1:
> +		return CTRL_IO_SINGLE_DATA;
> +	case 2:
> +		return CTRL_IO_DUAL_DATA;
> +	case 4:
> +		return CTRL_IO_QUAD_DATA;
> +	default:
> +		return CTRL_IO_SINGLE_DATA;
> +	}
> +}
> +
> +/*
> + * Check whether the data is not all 0 or 1 in order to
> + * avoid calibriate umount spi-flash.
> + */
> +static bool aspeed_spi_calibriation_enable(const uint8_t *buf, uint32_t sz)
> +{
> +	const uint32_t *buf_32 = (const uint32_t *)buf;
> +	uint32_t i;
> +	uint32_t valid_count = 0;
> +
> +	for (i = 0; i < (sz / 4); i++) {
> +		if (buf_32[i] != 0 && buf_32[i] != 0xffffffff)
> +			valid_count++;
> +		if (valid_count > 100)
> +			return true;
> +	}
> +	return false;
> +}
> +
> +static uint32_t
> +aspeed_2600_spi_dma_checksum(struct aspeed_spi_controller *ast_ctrl,
> +			     uint32_t cs, uint32_t div, uint32_t delay)
> +{
> +	uint32_t ctrl_val;
> +	uint32_t checksum;
> +
> +	writel(0xaeed0000, ast_ctrl->regs + OFFSET_DMA_CTRL);

You should use define for the magic value above ^

> +	if (readl(ast_ctrl->regs + OFFSET_DMA_CTRL) & SPI_DAM_REQUEST) {
> +		while (!(readl(ast_ctrl->regs + OFFSET_DMA_CTRL) &
> +			 SPI_DAM_GRANT))
> +			;
> +	}

What are these DAM bits for ? 

I don't think the AST2500 SPI controllers supports DMA. It would be better to
use a common method.

> +
> +	writel((uint32_t)ast_ctrl->chips[cs].ahb_base_phy,
> +	       ast_ctrl->regs + OFFSET_DMA_FLASH_ADDR_REG);
> +	writel(CALIBRATION_LEN, ast_ctrl->regs + OFFSET_DMA_LEN_REG);
> +
> +	ctrl_val = SPI_DMA_ENABLE | SPI_DMA_CALC_CKSUM | SPI_DMA_CALIB_MODE |
> +		   (delay << 8) | ((div & 0xf) << 16);
> +	writel(ctrl_val, ast_ctrl->regs + OFFSET_DMA_CTRL);
> +	while (!(readl(ast_ctrl->regs + OFFSET_INTR_CTRL_STATUS) &
> +		 SPI_DMA_STATUS))
> +		;
> +
> +	checksum = readl(ast_ctrl->regs + OFFSET_DMA_CHECKSUM_RESULT);
> +
> +	writel(0xdeea0000, ast_ctrl->regs + OFFSET_DMA_CTRL);
> +	writel(0x0, ast_ctrl->regs + OFFSET_DMA_CTRL);
> +
> +	return checksum;
> +}
> +
> +static int get_mid_point_of_longest_one(uint8_t *buf, uint32_t len)
> +{
> +	int i;
> +	int start = 0, mid_point = 0;
> +	int max_cnt = 0, cnt = 0;
> +
> +	for (i = 0; i < len; i++) {
> +		if (buf[i] == 1) {
> +			cnt++;
> +		} else {
> +			cnt = 0;
> +			start = i;
> +		}
> +
> +		if (max_cnt < cnt) {
> +			max_cnt = cnt;
> +			mid_point = start + (cnt / 2);
> +		}
> +	}
> +
> +	/*
> +	 * In order to get a stable SPI read timing,
> +	 * abandon the result if the length of longest
> +	 * consecutive good points is too short.
> +	 */
> +	if (max_cnt < 4)
> +		return -1;
> +
> +	return mid_point;
> +}
> +
> +/* Transfer maximum clock frequency to register setting */
> +static uint32_t
> +aspeed_2600_spi_clk_basic_setting(struct aspeed_spi_controller *ast_ctrl,
> +				  uint32_t *max_clk)
> +{
> +	struct device *dev = ast_ctrl->dev;
> +	uint32_t hclk_clk = ast_ctrl->ahb_clk;
> +	uint32_t hclk_div = 0x400; /* default value */
> +	uint32_t i, j = 0;
> +	bool found = false;
> +	/* HCLK/1 ..	HCLK/16 */
> +	uint32_t hclk_masks[] = { 15, 7, 14, 6, 13, 5, 12, 4,
> +				  11, 3, 10, 2, 9,  1, 8,  0 };
> +
> +	/* FMC/SPIR10[27:24] */
> +	for (j = 0; j < 0xf; i++) {
> +		/* FMC/SPIR10[11:8] */
> +		for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
> +			if (i == 0 && j == 0)
> +				continue;
> +
> +			if (hclk_clk / (i + 1 + (j * 16)) <= *max_clk) {
> +				found = 1;
> +				*max_clk = hclk_clk / (i + 1 + (j * 16));
> +				break;
> +			}
> +		}
> +
> +		if (found) {
> +			hclk_div = ((j << 24) | hclk_masks[i] << 8);
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(dev, "found: %s, hclk: %d, max_clk: %d\n", found ? "yes" : "no",
> +		hclk_clk, *max_clk);
> +	dev_dbg(dev, "base_clk: %d, h_div: %d (mask %x), speed: %d\n", j, i + 1,
> +		hclk_masks[i], hclk_clk / (i + 1 + j * 16));
> +
> +	return hclk_div;
> +}
> +
> +/*
> + * If SPI frequency is too high, timing compensation is needed,
> + * otherwise, SPI controller will sample unready data. For AST2600
> + * SPI memory controller, only the first four frequency levels
> + * (HCLK/2, HCLK/3,..., HCKL/5) may need timing compensation.
> + * Here, for each frequency, we will get a sequence of reading
> + * result (pass or fail) compared to golden data. Then, getting the
> + * middle point of the maximum pass widow. Besides, if the flash's
> + * content is too monotonous, the frequency recorded in the device
> + * tree will be adopted.
> + */
> +static int
> +aspeed_2600_spi_timing_calibration(struct aspeed_spi_controller *ast_ctrl,
> +				   uint32_t cs)
> +{
> +	int ret = 0;
> +	struct device *dev = ast_ctrl->dev;
> +	struct aspeed_spi_chip *chip = &ast_ctrl->chips[cs];
> +	uint32_t max_freq = chip->max_clk_freq;
> +	/* HCLK/2, ..., HCKL/5 */
> +	uint32_t hclk_masks[] = { 7, 14, 6, 13 };
> +	uint8_t *calib_res = NULL;
> +	uint8_t *check_buf = NULL;
> +	uint32_t reg_val;
> +	uint32_t checksum, gold_checksum;
> +	uint32_t i, hcycle, delay_ns, final_delay = 0;
> +	uint32_t hclk_div;
> +	bool pass;
> +	int calib_point;
> +
> +	reg_val =
> +		readl(ast_ctrl->regs + OFFSET_CE0_TIMING_COMPENSATION + cs * 4);
> +	if (reg_val != 0) {
> +		dev_dbg(dev, "has executed calibration.\n");
> +		goto no_calib;
> +	}
> +
> +	dev_dbg(dev, "calculate timing compensation :\n");
> +	/*
> +	 * use the related low frequency to get check calibration data
> +	 * and get golden data.
> +	 */
> +	reg_val = chip->ctrl_val[ASPEED_SPI_READ] & 0xf0fff0ff;
> +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> +
> +	check_buf = kzalloc(CALIBRATION_LEN, GFP_KERNEL);
> +	if (!check_buf)
> +		return -ENOMEM;
> +
> +	memcpy_fromio(check_buf, chip->ahb_base, CALIBRATION_LEN);
> +	if (!aspeed_spi_calibriation_enable(check_buf, CALIBRATION_LEN)) {
> +		dev_info(dev, "flash data is monotonous, skip calibration.");
> +		goto no_calib;
> +	}
> +
> +	gold_checksum = aspeed_2600_spi_dma_checksum(ast_ctrl, cs, 0, 0);
> +
> +	/*
> +	 * allocate a space to record calibration result for
> +	 * different timing compensation with fixed
> +	 * HCLK division.
> +	 */
> +	calib_res = kzalloc(6 * 17, GFP_KERNEL);
> +	if (!calib_res) {
> +		ret = -ENOMEM;
> +		goto no_calib;
> +	}
> +
> +	/* From HCLK/2 to HCLK/5 */
> +	for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
> +		if (max_freq < (uint32_t)ast_ctrl->ahb_clk / (i + 2)) {
> +			dev_dbg(dev, "skipping freq %d\n",
> +				(uint32_t)ast_ctrl->ahb_clk / (i + 2));
> +			continue;
> +		}
> +		max_freq = (uint32_t)ast_ctrl->ahb_clk / (i + 2);
> +
> +		checksum = aspeed_2600_spi_dma_checksum(ast_ctrl, cs,
> +							hclk_masks[i], 0);
> +		pass = (checksum == gold_checksum);
> +		dev_dbg(dev, "HCLK/%d, no timing compensation: %s\n", i + 2,
> +			pass ? "PASS" : "FAIL");
> +
> +		if (pass)
> +			break;
> +
> +		memset(calib_res, 0x0, 6 * 17);
> +
> +		for (hcycle = 0; hcycle <= 5; hcycle++) {
> +			/* increase DI delay by the step of 0.5ns */
> +			dev_dbg(dev, "Delay Enable : hcycle %x\n", hcycle);
> +			for (delay_ns = 0; delay_ns <= 0xf; delay_ns++) {
> +				checksum = aspeed_2600_spi_dma_checksum(
> +					ast_ctrl, cs, hclk_masks[i],
> +					BIT(3) | hcycle | (delay_ns << 4));
> +				pass = (checksum == gold_checksum);
> +				calib_res[hcycle * 17 + delay_ns] = pass;
> +				dev_dbg(dev,
> +					"HCLK/%d, %d HCLK cycle, %d delay_ns : %s\n",
> +					i + 2, hcycle, delay_ns,
> +					pass ? "PASS" : "FAIL");
> +			}
> +		}
> +
> +		calib_point = get_mid_point_of_longest_one(calib_res, 6 * 17);
> +		if (calib_point < 0) {
> +			dev_info(dev, "cannot get good calibration point.\n");
> +			continue;
> +		}
> +
> +		hcycle = calib_point / 17;
> +		delay_ns = calib_point % 17;
> +		dev_dbg(dev, "final hcycle: %d, delay_ns: %d\n", hcycle,
> +			delay_ns);
> +
> +		final_delay = (BIT(3) | hcycle | (delay_ns << 4)) << (i * 8);
> +		writel(final_delay, ast_ctrl->regs +
> +					    OFFSET_CE0_TIMING_COMPENSATION +
> +					    cs * 4);
> +		break;
> +	}
> +
> +no_calib:
> +
> +	hclk_div = aspeed_2600_spi_clk_basic_setting(ast_ctrl, &max_freq);
> +
> +	/* configure SPI clock frequency */
> +	reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> +	reg_val = (reg_val & 0xf0fff0ff) | hclk_div;
> +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> +
> +	/* add clock setting info for CE ctrl setting */
> +	for (i = 0; i < ASPEED_SPI_MAX; i++)
> +		chip->ctrl_val[i] = (chip->ctrl_val[i] & 0xf0fff0ff) | hclk_div;
> +
> +	dev_info(dev, "freq: %dMHz\n", max_freq / 1000000);
> +
> +	kfree(check_buf);
> +	kfree(calib_res);
> +
> +	return ret;
> +}
> +
> +/*
> + * AST2600 SPI memory controllers support multiple chip selects.
> + * The start address of a decode range should be multiple
> + * of its related flash size. Namely, the total decoded size
> + * from flash 0 to flash N should be multiple of flash (N + 1).
> + */
> +static void aspeed_2600_adjust_decode_sz(uint32_t decode_sz_arr[], int len)
> +{
> +	int cs, j;
> +	uint32_t sz;
> +
> +	for (cs = len - 1; cs >= 0; cs--) {
> +		sz = 0;
> +		for (j = 0; j < cs; j++)
> +			sz += decode_sz_arr[j];
> +
> +		if (sz % decode_sz_arr[cs] != 0)
> +			decode_sz_arr[0] += (sz % decode_sz_arr[cs]);
> +	}
> +}
> +
> +static int
> +aspeed_spi_decode_range_config(struct aspeed_spi_controller *ast_ctrl,
> +			       uint32_t decode_sz_arr[])
> +{
> +	struct aspeed_spi_chip *chip = ast_ctrl->chips;
> +	uint32_t i;
> +	uint32_t cs;
> +	uint32_t decode_reg_val;
> +	phys_addr_t start_addr_phy, end_addr_phy, pre_end_addr_phy = 0;
> +	uint32_t total_decode_sz = 0;
> +
> +	/* decode range sanity */
> +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> +		total_decode_sz += decode_sz_arr[cs];
> +		if (ast_ctrl->ahb_window_sz < total_decode_sz) {
> +			dev_err(ast_ctrl->dev, "insufficient decode size\n");
> +			for (i = 0; i <= cs; i++)
> +				dev_err(ast_ctrl->dev, "cs:%d %x\n", i,
> +					decode_sz_arr[i]);
> +			return -ENOSPC;
> +		}
> +	}
> +
> +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> +		if (chip[cs].ahb_base)
> +			devm_iounmap(ast_ctrl->dev, chip[cs].ahb_base);
> +	}
> +
> +	/* configure each CE's decode range */
> +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> +		if (cs == 0)
> +			start_addr_phy = ast_ctrl->ahb_base_phy;
> +		else
> +			start_addr_phy = pre_end_addr_phy;
> +
> +		chip[cs].ahb_base = devm_ioremap(ast_ctrl->dev, start_addr_phy,
> +						 decode_sz_arr[cs]);
> +		chip[cs].ahb_base_phy = start_addr_phy;
> +
> +		chip[cs].ahb_window_sz = decode_sz_arr[cs];
> +		end_addr_phy = start_addr_phy + decode_sz_arr[cs];
> +
> +		decode_reg_val = ast_ctrl->info->segment_reg(
> +			ast_ctrl, start_addr_phy, end_addr_phy);
> +
> +		writel(decode_reg_val,
> +		       ast_ctrl->regs + OFFSET_CE0_DECODE_RANGE_REG + cs * 4);
> +
> +		pre_end_addr_phy = end_addr_phy;
> +
> +		dev_dbg(ast_ctrl->dev, "cs: %d, decode_reg: 0x%x\n", cs,
> +			decode_reg_val);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct aspeed_spi_info ast2600_spi_info = {
> +	.max_data_bus_width = 4,
> +	.cmd_io_ctrl_mask = 0xf0ff40c3,
> +	/* for ast2600, the minimum decode size for each CE is 2MB */
> +	.min_decode_sz = 0x200000,
> +	.set_4byte = aspeed_spi_chip_set_4byte,
> +	.calibrate = aspeed_2600_spi_timing_calibration,
> +	.adjust_decode_sz = aspeed_2600_adjust_decode_sz,
> +	.segment_start = aspeed_2600_spi_segment_start,
> +	.segment_end = aspeed_2600_spi_segment_end,
> +	.segment_reg = aspeed_2600_spi_segment_reg,
> +};
> +
> +/*
> + * If the slave device is SPI NOR flash, there are two types
> + * of command mode for ASPEED SPI memory controller used to
> + * transfer data. The first one is user mode and the other is
> + * command read/write mode. With user mode, SPI NOR flash
> + * command, address and data processes are all handled by CPU.
> + * But, when address filter is enabled to protect some flash
> + * regions from being written, user mode will be disabled.
> + * Thus, here, we use command read/write mode to issue SPI
> + * operations. After remapping flash space correctly, we can
> + * easily read/write data to flash by reading or writing
> + * related remapped address, then, SPI NOR flash command and
> + * address will be transferred to flash by controller
> + * automatically. Besides, ASPEED SPI memory controller can
> + * also block address or data bytes by configure FMC0C/SPIR0C
> + * address and data mask register in order to satisfy the
> + * following SPI flash operation sequences: (command) only,
> + * (command and address) only or (coommand and data) only.
> + */
> +static int aspeed_spi_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct aspeed_spi_controller *ast_ctrl =
> +		spi_controller_get_devdata(mem->spi->master);
> +	struct device *dev = ast_ctrl->dev;
> +	uint32_t cs = mem->spi->chip_select;
> +	struct aspeed_spi_chip *chip = &ast_ctrl->chips[cs];
> +	uint32_t ctrl_val;
> +	uint32_t addr_mode_reg, addr_mode_reg_backup;
> +	uint32_t addr_data_mask = 0;
> +	void __iomem *op_addr;
> +	const void *data_buf;
> +	uint32_t data_byte = 0;
> +	uint32_t dummy_data = 0;
> +
> +	dev_dbg(dev, "cmd:%x(%d),addr:%llx(%d),dummy:%d(%d),data_len:%x(%d)\n",
> +		op->cmd.opcode, op->cmd.buswidth, op->addr.val,
> +		op->addr.buswidth, op->dummy.nbytes, op->dummy.buswidth,
> +		op->data.nbytes, op->data.buswidth);
> +
> +	addr_mode_reg = addr_mode_reg_backup =
> +		readl(ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> +	addr_data_mask = readl(ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
> +
> +	ctrl_val = chip->ctrl_val[ASPEED_SPI_BASE];
> +	ctrl_val &= ~ast_ctrl->info->cmd_io_ctrl_mask;
> +
> +	/* configure opcode */
> +	ctrl_val |= op->cmd.opcode << 16;
> +
> +	/* configure operation address, address length and address mask */
> +	if (op->addr.nbytes != 0) {
> +		if (op->addr.nbytes == 3)
> +			addr_mode_reg &= ~(0x11 << cs);
> +		else
> +			addr_mode_reg |= (0x11 << cs);

please use define.

> +
> +		addr_data_mask &= 0x0f;
> +		op_addr = chip->ahb_base + op->addr.val;
> +	} else {
> +		addr_data_mask |= 0xf0;

Disabling the address lanes needs an explanation.

> +		op_addr = chip->ahb_base;
> +	}
> +
> +	if (op->dummy.nbytes != 0) {
> +		ctrl_val |= ((op->dummy.nbytes & 0x3) << 6 |
> +			     (op->dummy.nbytes & 0x4) << 14);
> +	}
> +
> +	/* configure data io mode and data mask */
> +	if (op->data.nbytes != 0) {
> +		addr_data_mask &= 0xF0;
> +		data_byte = op->data.nbytes;
> +		if (op->data.dir == SPI_MEM_DATA_OUT)
> +			data_buf = op->data.buf.out;
> +		else
> +			data_buf = op->data.buf.in;
> +
> +		if (op->data.buswidth)
> +			ctrl_val |= aspeed_spi_get_io_mode(op->data.buswidth);
> +
> +	} else {
> +		addr_data_mask |= 0x0f;
> +		data_byte = 1;
> +		data_buf = &dummy_data;
> +	}
> +
> +	/* configure command mode */
> +	if (op->data.dir == SPI_MEM_DATA_OUT)
> +		ctrl_val |= CTRL_IO_MODE_CMD_WRITE;
> +	else
> +		ctrl_val |= CTRL_IO_MODE_CMD_READ;
> +
> +	/* set controller registers */
> +	writel(ctrl_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> +	writel(addr_mode_reg, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> +	writel(addr_data_mask, ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
> +
> +	dev_dbg(dev, "ctrl: 0x%08x, addr_mode: 0x%x, mask: 0x%x, addr:%px\n",
> +		ctrl_val, addr_mode_reg, addr_data_mask, op_addr);
> +
> +	/* trigger spi transmission or reception sequence */
> +	if (op->data.dir == SPI_MEM_DATA_OUT)
> +		memcpy_toio(op_addr, data_buf, data_byte);
> +	else
> +		memcpy_fromio((void *)data_buf, op_addr, data_byte);
> +
> +	/* restore controller setting */
> +	writel(chip->ctrl_val[ASPEED_SPI_READ],
> +	       ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> +	writel(addr_mode_reg_backup, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> +	writel(0x0, ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
> +
> +	return 0;
> +}
> +
> +static ssize_t aspeed_spi_dirmap_read(struct spi_mem_dirmap_desc *desc,
> +				  uint64_t offs, size_t len, void *buf)
> +{
> +	struct aspeed_spi_controller *ast_ctrl =
> +		spi_controller_get_devdata(desc->mem->spi->master);
> +	struct aspeed_spi_chip *chip =
> +		&ast_ctrl->chips[desc->mem->spi->chip_select];
> +	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
> +
> +	if (chip->ahb_window_sz < offs + len) {
> +		dev_info(ast_ctrl->dev,
> +			 "read range exceeds flash remapping size\n");
> +		return 0;
> +	}
> +
> +	dev_dbg(ast_ctrl->dev, "read op:0x%x, addr:0x%llx, len:0x%zx\n",
> +		op_tmpl.cmd.opcode, offs, len);
> +
> +	memcpy_fromio(buf, chip->ahb_base + offs, len);
> +
> +	return len;
> +}
> +
> +static ssize_t aspeed_spi_dirmap_write(struct spi_mem_dirmap_desc *desc,
> +				   uint64_t offs, size_t len, const void *buf)
> +{
> +	struct aspeed_spi_controller *ast_ctrl =
> +		spi_controller_get_devdata(desc->mem->spi->master);
> +	struct aspeed_spi_chip *chip =
> +		&ast_ctrl->chips[desc->mem->spi->chip_select];
> +	uint32_t reg_val;
> +	uint32_t target_cs = desc->mem->spi->chip_select;
> +	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
> +
> +	if (chip->ahb_window_sz < offs + len) {
> +		dev_info(ast_ctrl->dev,
> +			 "write range exceeds flash remapping size\n");
> +		return 0;
> +	}
> +
> +	dev_dbg(ast_ctrl->dev, "write op:0x%x, addr:0x%llx, len:0x%zx\n",
> +		op_tmpl.cmd.opcode, offs, len);
> +
> +	reg_val = ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE];
> +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs * 4);
> +
> +	memcpy_toio(chip->ahb_base + offs, buf, len);
> +
> +	reg_val = ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ];
> +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs * 4);
> +
> +	return len;
> +}
> +
> +static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
> +{
> +	int ret = 0;
> +	struct aspeed_spi_controller *ast_ctrl =
> +		spi_controller_get_devdata(desc->mem->spi->master);
> +	struct device *dev = ast_ctrl->dev;
> +	const struct aspeed_spi_info *info = ast_ctrl->info;
> +	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
> +	uint32_t decode_sz_arr[5];
> +	uint32_t cs, target_cs = desc->mem->spi->chip_select;
> +	uint32_t reg_val;
> +
> +	if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN) {
> +		/* record original decode size */
> +		for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> +			reg_val = readl(ast_ctrl->regs +
> +					OFFSET_CE0_DECODE_RANGE_REG + cs * 4);
> +			decode_sz_arr[cs] =
> +				info->segment_end(ast_ctrl, reg_val) -
> +				info->segment_start(ast_ctrl, reg_val);
> +		}
> +
> +		decode_sz_arr[target_cs] = desc->info.length;
> +
> +		if (info->adjust_decode_sz)
> +			info->adjust_decode_sz(decode_sz_arr, ast_ctrl->num_cs);
> +
> +		for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> +			dev_dbg(dev, "cs: %d, sz: 0x%x\n", cs,
> +				decode_sz_arr[cs]);
> +		}
> +
> +		ret = aspeed_spi_decode_range_config(ast_ctrl, decode_sz_arr);
> +		if (ret)
> +			return ret;
> +
> +		reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG +
> +				target_cs * 4) &
> +			  (~info->cmd_io_ctrl_mask);
> +		reg_val |= aspeed_spi_get_io_mode(op_tmpl.data.buswidth) |
> +			   op_tmpl.cmd.opcode << 16 |
> +			   ((op_tmpl.dummy.nbytes) & 0x3) << 6 |
> +			   ((op_tmpl.dummy.nbytes) & 0x4) << 14 |
> +			   CTRL_IO_MODE_CMD_READ;
> +
> +		writel(reg_val,
> +		       ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs * 4);
> +		ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ] = reg_val;
> +		ast_ctrl->chips[target_cs].max_clk_freq =
> +			desc->mem->spi->max_speed_hz;
> +
> +		ret = info->calibrate(ast_ctrl, target_cs);
> +
> +		dev_info(dev, "read bus width: %d [0x%08x]\n",
> +			 op_tmpl.data.buswidth,
> +			 ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ]);
> +
> +	} else if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_OUT) {
> +		reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG +
> +				target_cs * 4) &
> +			  (~info->cmd_io_ctrl_mask);
> +		reg_val |= aspeed_spi_get_io_mode(op_tmpl.data.buswidth) |
> +			   op_tmpl.cmd.opcode << 16 | CTRL_IO_MODE_CMD_WRITE;
> +		ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE] = reg_val;
> +
> +		dev_info(dev, "write bus width: %d [0x%08x]\n",
> +			 op_tmpl.data.buswidth,
> +			 ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE]);
> +	}
> +
> +	return ret;
> +}
> +
> +static const char *aspeed_spi_get_name(struct spi_mem *mem)
> +{
> +	struct device *dev = &mem->spi->master->dev;
> +	const char *name;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "%s-%d", dev_name(dev),
> +			      mem->spi->chip_select);
> +
> +	if (!name) {
> +		dev_err(dev, "cannot get spi name\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	return name;
> +}
> +
> +/*
> + * Currently, only support 1-1-1, 1-1-2 or 1-1-4
> + * SPI NOR flash operation format.
> + */
> +static bool aspeed_spi_support_op(struct spi_mem *mem,
> +				  const struct spi_mem_op *op)
> +{
> +	struct aspeed_spi_controller *ast_ctrl =
> +		spi_controller_get_devdata(mem->spi->master);
> +
> +	if (op->cmd.buswidth > 1)
> +		return false;
> +
> +	if (op->addr.nbytes != 0) {
> +		if (op->addr.buswidth > 1 || op->addr.nbytes > 4)
> +			return false;
> +	}
> +
> +	if (op->dummy.nbytes != 0) {
> +		if (op->dummy.buswidth > 1 || op->dummy.nbytes > 7)
> +			return false;
> +	}
> +
> +	if (op->data.nbytes != 0 &&
> +	    ast_ctrl->info->max_data_bus_width < op->data.buswidth)
> +		return false;
> +
> +	if (!spi_mem_default_supports_op(mem, op))
> +		return false;
> +
> +	if (op->addr.nbytes == 4)
> +		ast_ctrl->info->set_4byte(ast_ctrl, mem->spi->chip_select);
> +
> +	return true;
> +}
> +
> +static const struct spi_controller_mem_ops aspeed_spi_mem_ops = {
> +	.exec_op = aspeed_spi_exec_op,
> +	.get_name = aspeed_spi_get_name,
> +	.supports_op = aspeed_spi_support_op,
> +	.dirmap_create = aspeed_spi_dirmap_create,
> +	.dirmap_read = aspeed_spi_dirmap_read,
> +	.dirmap_write = aspeed_spi_dirmap_write,
> +};
> +
> +/*
> + * Initialize SPI controller for each chip select.
> + * Here, only the minimum decode range is configured
> + * in order to get device (SPI NOR flash) information
> + * at the early stage.
> + */
> +static int aspeed_spi_ctrl_init(struct aspeed_spi_controller *ast_ctrl)
> +{
> +	int ret;
> +	uint32_t cs;
> +	uint32_t val;
> +	uint32_t decode_sz_arr[ASPEED_SPI_MAX_CS];
> +
> +	/* enable write capability for all CEs */
> +	val = readl(ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> +	writel(val | (GENMASK(ast_ctrl->num_cs, 0) << 16),
> +	       ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> +
> +	/* initial each CE's controller register */
> +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> +		val = CTRL_STOP_ACTIVE | CTRL_IO_MODE_USER;
> +		writel(val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> +		ast_ctrl->chips[cs].ctrl_val[ASPEED_SPI_BASE] = val;
> +	}
> +
> +	for (cs = 0; cs < ast_ctrl->num_cs && cs < ASPEED_SPI_MAX_CS; cs++)
> +		decode_sz_arr[cs] = ast_ctrl->info->min_decode_sz;
> +
> +	ret = aspeed_spi_decode_range_config(ast_ctrl, decode_sz_arr);
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id aspeed_spi_matches[] = {
> +	{ .compatible = "aspeed,ast2600-fmc", .data = &ast2600_spi_info },
> +	{ .compatible = "aspeed,ast2600-spi", .data = &ast2600_spi_info },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, aspeed_spi_matches);
> +
> +static int aspeed_spi_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct spi_controller *spi_ctrl;
> +	struct aspeed_spi_controller *ast_ctrl;
> +	const struct of_device_id *match;
> +	struct clk *clk;
> +	struct resource *res;
> +
> +	spi_ctrl = spi_alloc_master(dev, sizeof(struct aspeed_spi_controller));
> +	if (!spi_ctrl)
> +		return -ENOMEM;
> +
> +	ast_ctrl = spi_controller_get_devdata(spi_ctrl);
> +
> +	match = of_match_device(aspeed_spi_matches, dev);
> +	if (!match || !match->data) {
> +		dev_err(dev, "no compatible OF match\n");
> +		return -ENODEV;
> +	}
> +
> +	ast_ctrl->info = match->data;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +					   "spi_ctrl_reg");
> +	ast_ctrl->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(ast_ctrl->regs))
> +		return PTR_ERR(ast_ctrl->regs);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "spi_mmap");
> +	ast_ctrl->ahb_base_phy = res->start;
> +	ast_ctrl->ahb_window_sz = resource_size(res);
> +
> +	ast_ctrl->dev = dev;
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +	ast_ctrl->ahb_clk = clk_get_rate(clk);
> +	devm_clk_put(&pdev->dev, clk);
> +
> +	if (of_property_read_u32(dev->of_node, "num-cs", &ast_ctrl->num_cs)) {
> +		dev_err(dev, "fail to get chip number.\n");
> +		goto end;
> +	}
> +
> +	if (ast_ctrl->num_cs > ASPEED_SPI_MAX_CS) {
> +		dev_err(dev, "chip number, %d, exceeds %d.\n", ast_ctrl->num_cs,
> +			ASPEED_SPI_MAX_CS);
> +		goto end;
> +	}
> +
> +	ast_ctrl->chips =
> +		devm_kzalloc(dev,
> +			     sizeof(struct aspeed_spi_chip) * ast_ctrl->num_cs,
> +			     GFP_KERNEL);
> +
> +	platform_set_drvdata(pdev, ast_ctrl);
> +
> +	spi_ctrl->mode_bits =
> +		SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
> +
> +	spi_ctrl->bus_num = -1;
> +	spi_ctrl->mem_ops = &aspeed_spi_mem_ops;
> +	spi_ctrl->dev.of_node = dev->of_node;
> +	spi_ctrl->num_chipselect = ast_ctrl->num_cs;
> +
> +	ret = aspeed_spi_ctrl_init(ast_ctrl);
> +	if (ret)
> +		goto end;
> +
> +	ret = devm_spi_register_master(dev, spi_ctrl);
> +
> +end:
> +	return ret;
> +}
> +
> +static int aspeed_spi_remove(struct platform_device *pdev)
> +{
> +	struct aspeed_spi_controller *ast_ctrl = platform_get_drvdata(pdev);
> +	uint32_t val;
> +
> +	/* disable write capability for all CEs */
> +	val = readl(ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> +	writel(val & ~(GENMASK(ast_ctrl->num_cs, 0) << 16),
> +	       ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver aspeed_spi_driver = {
> +	.driver = {
> +		.name = "ASPEED_FMC_SPI",
> +		.bus = &platform_bus_type,
> +		.of_match_table = aspeed_spi_matches,
> +	},
> +	.probe = aspeed_spi_probe,
> +	.remove = aspeed_spi_remove,
> +};
> +module_platform_driver(aspeed_spi_driver);
> +
> +MODULE_DESCRIPTION("ASPEED FMC/SPI Memory Controller Driver");
> +MODULE_AUTHOR("Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>");
> +MODULE_AUTHOR("Cedric Le Goater <clg@kaod.org>");
> +MODULE_LICENSE("GPL v2");
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-05 14:09   ` Cédric Le Goater
@ 2020-11-05 15:11     ` Boris Brezillon
  2020-11-05 16:43       ` Mark Brown
  2020-11-06  8:58       ` Chin-Ting Kuo
  2020-11-06  7:38     ` Chin-Ting Kuo
  1 sibling, 2 replies; 20+ messages in thread
From: Boris Brezillon @ 2020-11-05 15:11 UTC (permalink / raw)
  To: Cédric Le Goater, robh+dt
  Cc: Chin-Ting Kuo, broonie, joel, andrew, bbrezillon, devicetree,
	linux-kernel, linux-aspeed, linux-spi, BMC-SW

Hi,

On Thu, 5 Nov 2020 15:09:11 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> Hello Chin-Ting,
> 
> Thanks for this driver. It's much cleaner than the previous and we should 
> try adding support for the AST2500 SoC also. I guess we can keep the old 
> driver for the AST2400 which has a different register layout.
> 
> On the patchset, I think we should split this patch in three : 
> 
>  - basic support
>  - AHB window calculation depending on the flash size
>  - read training support  

I didn't look closely at the implementation, but if the read training
tries to read a section of the NOR, I'd recommend exposing that feature
through spi-mem and letting the SPI-NOR framework trigger the training
instead of doing that at dirmap creation time (remember that spi-mem is
also used for SPI NANDs which use the dirmap API too, and this training
is unlikely to work there).

The SPI-NOR framework could pass a read op template and a reference
pattern such that all the spi-mem driver has to do is execute the
template op and compare the output to the reference buffer.


> 
> We should avoid magic values when setting registers. This is confusing 
> and defines are much better.
>  
> AST2500 support will be a bit challenging because HW does not allow    
> to configure a 128MB AHB window, max is 120MB This is a bug and the work 
> around is to use user mode for the remaining 8MB. Something to keep in
> mind.

Most SPI-MEM controllers don't have such a big dirmap window anyway, and
that shouldn't be a problem, because we don't expose the direct mapping
directly (as would be done if we were trying to support something like
XIP). That means that, assuming your controller allows controlling the
base spi-mem address the direct mapping points to, you should be able
to adjust the window at runtime and make it point where you requested.

Note that dirmap_{read,write}() are allowed to return less data than
requested thus simplifying the case where a specific access requires a
window adjustment in the middle of an read/write operation.

Regards,

Boris

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-05 15:11     ` Boris Brezillon
@ 2020-11-05 16:43       ` Mark Brown
  2020-11-06  9:01         ` Chin-Ting Kuo
  2020-11-06  8:58       ` Chin-Ting Kuo
  1 sibling, 1 reply; 20+ messages in thread
From: Mark Brown @ 2020-11-05 16:43 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Cédric Le Goater, robh+dt, Chin-Ting Kuo, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi,
	BMC-SW

[-- Attachment #1: Type: text/plain, Size: 1364 bytes --]

On Thu, Nov 05, 2020 at 04:11:32PM +0100, Boris Brezillon wrote:
> Cédric Le Goater <clg@kaod.org> wrote:

> > Thanks for this driver. It's much cleaner than the previous and we should 
> > try adding support for the AST2500 SoC also. I guess we can keep the old 
> > driver for the AST2400 which has a different register layout.
> > 
> > On the patchset, I think we should split this patch in three : 
> > 
> >  - basic support
> >  - AHB window calculation depending on the flash size
> >  - read training support  

> I didn't look closely at the implementation, but if the read training
> tries to read a section of the NOR, I'd recommend exposing that feature
> through spi-mem and letting the SPI-NOR framework trigger the training
> instead of doing that at dirmap creation time (remember that spi-mem is
> also used for SPI NANDs which use the dirmap API too, and this training
> is unlikely to work there).

> The SPI-NOR framework could pass a read op template and a reference
> pattern such that all the spi-mem driver has to do is execute the
> template op and compare the output to the reference buffer.

That seems like a good idea.

> > We should avoid magic values when setting registers. This is confusing 
> > and defines are much better.

It does depend a bit on documentation though, it's not a hard
requirement.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller
  2020-11-05 12:03 ` [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller Chin-Ting Kuo
@ 2020-11-05 22:39   ` Rob Herring
  2020-11-06  9:11     ` Chin-Ting Kuo
  0 siblings, 1 reply; 20+ messages in thread
From: Rob Herring @ 2020-11-05 22:39 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: broonie, joel, andrew, clg, bbrezillon, devicetree, linux-kernel,
	linux-aspeed, linux-spi, BMC-SW

On Thu, Nov 05, 2020 at 08:03:28PM +0800, Chin-Ting Kuo wrote:
> Create binding file with YAML syntax for ASPEED FMC/SPI memory controller.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  .../bindings/spi/aspeed,spi-aspeed.yaml       | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml b/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
> new file mode 100644
> index 000000000000..41b9692c7226
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/aspeed,spi-aspeed.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SPI memory controller for ASPEED SoCs
> +
> +maintainers:
> +  - Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> +
> +description: |
> +  There are three SPI memory controllers embedded in a ASPEED SoC.
> +  They are usually connected to SPI NOR flashes. Each of them has
> +  more than a chip select. They also support SPI single, dual and
> +  quad IO modes for SPI NOR flash.
> +
> +allOf:
> +  - $ref: /spi/spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:

You can drop oneOf (there's only 1) and items.

> +          - enum:
> +              - aspeed,ast2600-fmc
> +              - aspeed,ast2600-spi
> +
> +  reg:
> +    items:
> +      - description: the control register location and length
> +      - description: the flash memory mapping address and length
> +
> +  clocks:
> +    description: AHB bus clock which will be converted to SPI bus clock

maxItems: 1

Constraints on num-cs values? Or up to 2^32 is good?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - num-cs
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/ast2600-clock.h>
> +    spi1: spi@1e630000 {
> +      compatible = "aspeed,ast2600-spi";
> +      reg = <0x1e630000 0xc4>, <0x30000000 0x10000000>;
> +      reg-names = "spi_ctrl_reg", "spi_mmap";
> +      clocks = <&syscon ASPEED_CLK_AHB>;
> +      num-cs = <2>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      flash@0 {
> +        compatible = "jedec,spi-nor";
> +        reg = <0>;
> +        spi-max-frequency = <50000000>;
> +      };
> +      flash@1 {
> +        compatible = "jedec,spi-nor";
> +        reg = <1>;
> +        spi-max-frequency = <50000000>;
> +      };
> +    };
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-05 14:09   ` Cédric Le Goater
  2020-11-05 15:11     ` Boris Brezillon
@ 2020-11-06  7:38     ` Chin-Ting Kuo
  1 sibling, 0 replies; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-06  7:38 UTC (permalink / raw)
  To: Cédric Le Goater, broonie, robh+dt, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi
  Cc: BMC-SW

Hi C,

Thanks for your reply.

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Thursday, November 5, 2020 10:09 PM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>; broonie@kernel.org;
> robh+dt@kernel.org; joel@jms.id.au; andrew@aj.id.au; bbrezillon@kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-aspeed@lists.ozlabs.org; linux-spi@vger.kernel.org
> Cc: BMC-SW <BMC-SW@aspeedtech.com>
> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> driver
> 
> Hello Chin-Ting,
> 
> Thanks for this driver. It's much cleaner than the previous and we should try
> adding support for the AST2500 SoC also. I guess we can keep the old driver
> for the AST2400 which has a different register layout.
> 

I will also add AST2500 part into next patch version.

> On the patchset, I think we should split this patch in three :
> 
>  - basic support
>  - AHB window calculation depending on the flash size
>  - read training support

Oaky, it is more clear.

> We should avoid magic values when setting registers. This is confusing and
> defines are much better.

Okay, it will be modified at next patch.

> AST2500 support will be a bit challenging because HW does not allow
> to configure a 128MB AHB window, max is 120MB This is a bug and the work
> around is to use user mode for the remaining 8MB. Something to keep in
> mind.

This is why there is a "min_decode_sz" element in "aspeed_spi_info" struct.
Thanks for your reminder.

Some replies for the comments below.

> I gave it a try on QEMU. It looks good. When I can revive my EVB, I will do the
> same.
> 
> More comments below,
> 
> Thanks,
> 
> C.
> 
> 
> On 11/5/20 1:03 PM, Chin-Ting Kuo wrote:
> > Add driver for ASPEED BMC FMC/SPI memory controller which supports
> > spi-mem interface.
> >
> > There are three SPI memory controllers embedded in an ASPEED SoC.
> > Each of them can connect to two or three SPI NOR flashes. The first
> > SPI memory controller is also named as Firmware Memory Controller
> > (FMC), which is similar to SPI memory controller. After device AC on,
> > MCU ROM can fetch device boot code from FMC CS 0. Thus, there exists
> > additional registers for boot process control in FMC.
> >
> > ASPEED SPI memory controller supports single, dual and quad mode for
> > SPI NOR flash. It also supports two types of command mode, user mode
> > and command read/write mode. User mode is traditional pure SPI
> > operations where all transmission is controlled by CPU. Contrarily,
> > with command read/write mode, SPI controller can send command and
> > address automatically when CPU read/write related remapped address.
> >
> > Besides, different wafer processes of SPI NOR flash result in
> > different signal response time. This phenomenon will be enlarged when
> > SPI clock frequency increases. ASPEED SPI memory controller provides a
> > mechanism for timing compensation in order to satisfy various SPI NOR
> > flash parts and PCB layout.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  v2: Fix sparse warnings reported by kernel test robot <lkp@intel.com>.
> >  v3: Fix build warnings with x86 allmodconfig.
> >
> >  drivers/spi/Kconfig      |  10 +
> >  drivers/spi/Makefile     |   1 +
> >  drivers/spi/spi-aspeed.c | 969
> > +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 980 insertions(+)
> >  create mode 100644 drivers/spi/spi-aspeed.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> > 5cff60de8e83..c848f2f7b694 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -70,6 +70,16 @@ config SPI_AR934X
> >  	  This enables support for the SPI controller present on the
> >  	  Qualcomm Atheros AR934X/QCA95XX SoCs.
> >
> > +config SPI_ASPEED
> > +	tristate "ASPEED FMC/SPI Memory Controller"
> > +	depends on OF && HAS_IOMEM && (ARCH_ASPEED || COMPILE_TEST)
> 
> We will need to do something about the other driver. For the moment, we can
> select both but that won't be the case anymore when we add
> AST2500 support.
>

Yes, maybe, "!SPI_ASPEED_SMC" should be added and also add "!SPI_ASPEED" for the other driver.
Thanks for reminder.

> > +	help
> > +	  Enable driver for ASPEED FMC/SPI Memory Controller.
> > +
> > +	  This driver is not a generic pure SPI driver, which
> > +	  is especially designed for spi-mem framework with
> > +	  SPI NOR flash direct read and write features.
> > +
> >  config SPI_ATH79
> >  	tristate "Atheros AR71XX/AR724X/AR913X SPI controller driver"
> >  	depends on ATH79 || COMPILE_TEST
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
> > 6fea5821662e..9e62c650fca0 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -17,6 +17,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST)		+=
> spi-loopback-test.o
> >  obj-$(CONFIG_SPI_ALTERA)		+= spi-altera.o
> >  obj-$(CONFIG_SPI_AR934X)		+= spi-ar934x.o
> >  obj-$(CONFIG_SPI_ARMADA_3700)		+= spi-armada-3700.o
> > +obj-$(CONFIG_SPI_ASPEED)		+= spi-aspeed.o
> >  obj-$(CONFIG_SPI_ATMEL)			+= spi-atmel.o
> >  obj-$(CONFIG_SPI_ATMEL_QUADSPI)		+= atmel-quadspi.o
> >  obj-$(CONFIG_SPI_AT91_USART)		+= spi-at91-usart.o
> > diff --git a/drivers/spi/spi-aspeed.c b/drivers/spi/spi-aspeed.c new
> > file mode 100644 index 000000000000..cfaaa0d5bac6
> > --- /dev/null
> > +++ b/drivers/spi/spi-aspeed.c
> > @@ -0,0 +1,969 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +
> > +/*
> > + * ASPEED FMC/SPI Memory Controller Driver
> > + *
> > + * Copyright (c) 2020, ASPEED Corporation.
> > + * Copyright (c) 2015-2016, IBM Corporation.
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/sizes.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/spi-mem.h>
> > +
> > +/* ASPEED FMC/SPI memory control register related */
> > +#define OFFSET_CE_TYPE_SETTING		0x00
> > +#define OFFSET_CE_ADDR_MODE_CTRL	0x04
> > +#define OFFSET_INTR_CTRL_STATUS		0x08
> > +#define OFFSET_ADDR_DATA_MASK		0x0c
> > +#define OFFSET_CE0_CTRL_REG		0x10
> > +#define OFFSET_CE0_DECODE_RANGE_REG	0x30
> > +#define OFFSET_DMA_CTRL			0x80
> > +#define OFFSET_DMA_FLASH_ADDR_REG	0x84
> > +#define OFFSET_DMA_RAM_ADDR_REG		0x88
> > +#define OFFSET_DMA_LEN_REG		0x8c
> > +#define OFFSET_DMA_CHECKSUM_RESULT	0x90
> > +#define OFFSET_CE0_TIMING_COMPENSATION	0x94
> > +
> > +#define CTRL_IO_SINGLE_DATA	0
> > +#define CTRL_IO_DUAL_DATA	BIT(29)
> > +#define CTRL_IO_QUAD_DATA	BIT(30)
> > +
> > +#define CTRL_IO_MODE_USER	GENMASK(1, 0)
> > +#define CTRL_IO_MODE_CMD_READ	BIT(0)
> > +#define CTRL_IO_MODE_CMD_WRITE	BIT(1)
> > +#define CTRL_STOP_ACTIVE	BIT(2)
> > +
> > +#define CALIBRATION_LEN		0x400
> > +#define SPI_DAM_REQUEST		BIT(31)
> > +#define SPI_DAM_GRANT		BIT(30)
> 
> What are these bits ? There is no documentation for them.

They are DMA related bits for AST2600 SPI controllers, recorded in AST2600 SPI register SPIR80.
Since both SPI1 and SPI2 controllers share the same DMA engine, there exists a HW flag
in order to avoid SPI1 and SPI2 controller using that DMA engine at the same time.

> 
> > +#define SPI_DMA_CALIB_MODE	BIT(3)
> > +#define SPI_DMA_CALC_CKSUM	BIT(2)
> > +#define SPI_DMA_ENABLE		BIT(0)
> > +#define SPI_DMA_STATUS		BIT(11)
> > +
> > +enum aspeed_spi_ctl_reg_value {
> > +	ASPEED_SPI_BASE,
> > +	ASPEED_SPI_READ,
> > +	ASPEED_SPI_WRITE,
> > +	ASPEED_SPI_MAX,
> > +};
> > +
> > +#define ASPEED_SPI_MAX_CS 5
> > +
> > +struct aspeed_spi_controller;
> > +struct aspeed_spi_chip;
> > +
> > +struct aspeed_spi_info {
> > +	uint32_t cmd_io_ctrl_mask;
> > +	uint32_t max_data_bus_width;
> > +	uint32_t min_decode_sz;
> > +	void (*set_4byte)(struct aspeed_spi_controller *ast_ctrl, uint32_t cs);
> > +	int (*calibrate)(struct aspeed_spi_controller *ast_ctrl, uint32_t cs);
> > +	void (*adjust_decode_sz)(uint32_t decode_sz_arr[], int len);
> > +	uint32_t (*segment_start)(struct aspeed_spi_controller *ast_ctrl,
> > +				  uint32_t reg);
> > +	uint32_t (*segment_end)(struct aspeed_spi_controller *ast_ctrl,
> > +				uint32_t reg);
> > +	uint32_t (*segment_reg)(struct aspeed_spi_controller *ast_ctrl,
> > +				uint32_t start, uint32_t end);
> > +};
> > +
> > +struct aspeed_spi_chip {
> > +	void __iomem *ahb_base;
> > +	phys_addr_t ahb_base_phy;
> > +	uint32_t ahb_window_sz;
> > +	uint32_t ctrl_val[ASPEED_SPI_MAX];
> > +	uint32_t max_clk_freq;
> > +};
> > +
> > +struct aspeed_spi_controller {
> > +	struct device *dev;
> > +	const struct aspeed_spi_info *info; /* controller info */
> > +	void __iomem *regs; /* controller registers */
> > +	void __iomem *ahb_base;
> > +	phys_addr_t ahb_base_phy; /* physical addr of AHB window */
> > +	uint32_t ahb_window_sz; /* AHB window size */
> > +	uint32_t num_cs;
> > +	uint64_t ahb_clk;
> > +	struct aspeed_spi_chip *chips; /* pointers to attached chips */ };
> > +
> > +static uint32_t
> > +aspeed_2600_spi_segment_start(struct aspeed_spi_controller *ast_ctrl,
> > +			      uint32_t reg)
> > +{
> > +	uint32_t start_offset = (reg << 16) & 0x0ff00000;
> > +
> > +	return (uint32_t)(ast_ctrl->ahb_base_phy + start_offset); }
> > +
> > +static uint32_t
> > +aspeed_2600_spi_segment_end(struct aspeed_spi_controller *ast_ctrl,
> > +			    uint32_t reg)
> > +{
> > +	uint32_t end_offset = reg & 0x0ff00000;
> > +
> > +	/* no decode range, set to physical ahb base */
> > +	if (end_offset == 0)
> > +		return ast_ctrl->ahb_base_phy;
> > +
> > +	return (uint32_t)(ast_ctrl->ahb_base_phy + end_offset + 0x100000); }
> > +
> > +static uint32_t
> > +aspeed_2600_spi_segment_reg(struct aspeed_spi_controller *ast_ctrl,
> > +			    uint32_t start, uint32_t end)
> > +{
> > +	/* no decode range, assign zero value */
> > +	if (start == end)
> > +		return 0;
> > +
> > +	return ((start & 0x0ff00000) >> 16) | ((end - 0x100000) &
> > +0x0ff00000); }
> > +
> > +static void aspeed_spi_chip_set_4byte(struct aspeed_spi_controller
> *ast_ctrl,
> > +				      uint32_t cs)
> > +{
> > +	uint32_t reg_val;
> > +
> > +	reg_val = readl(ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> > +	reg_val |= 0x11 << cs;
> > +	writel(reg_val, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL); }
> > +
> > +static uint32_t aspeed_spi_get_io_mode(uint32_t bus_width) {
> > +	switch (bus_width) {
> > +	case 1:
> > +		return CTRL_IO_SINGLE_DATA;
> > +	case 2:
> > +		return CTRL_IO_DUAL_DATA;
> > +	case 4:
> > +		return CTRL_IO_QUAD_DATA;
> > +	default:
> > +		return CTRL_IO_SINGLE_DATA;
> > +	}
> > +}
> > +
> > +/*
> > + * Check whether the data is not all 0 or 1 in order to
> > + * avoid calibriate umount spi-flash.
> > + */
> > +static bool aspeed_spi_calibriation_enable(const uint8_t *buf,
> > +uint32_t sz) {
> > +	const uint32_t *buf_32 = (const uint32_t *)buf;
> > +	uint32_t i;
> > +	uint32_t valid_count = 0;
> > +
> > +	for (i = 0; i < (sz / 4); i++) {
> > +		if (buf_32[i] != 0 && buf_32[i] != 0xffffffff)
> > +			valid_count++;
> > +		if (valid_count > 100)
> > +			return true;
> > +	}
> > +	return false;
> > +}
> > +
> > +static uint32_t
> > +aspeed_2600_spi_dma_checksum(struct aspeed_spi_controller *ast_ctrl,
> > +			     uint32_t cs, uint32_t div, uint32_t delay) {
> > +	uint32_t ctrl_val;
> > +	uint32_t checksum;
> > +
> > +	writel(0xaeed0000, ast_ctrl->regs + OFFSET_DMA_CTRL);
> 
> You should use define for the magic value above ^

Okay, it will be updated with next patch version.

> 
> > +	if (readl(ast_ctrl->regs + OFFSET_DMA_CTRL) & SPI_DAM_REQUEST) {
> > +		while (!(readl(ast_ctrl->regs + OFFSET_DMA_CTRL) &
> > +			 SPI_DAM_GRANT))
> > +			;
> > +	}
> 
> What are these DAM bits for ?

As above explanation, there are a DMA engine shared by both SPI1 and SPI2 controller.
Before using DMA engine, they need to send a request and get the grant bit.

> 
> I don't think the AST2500 SPI controllers supports DMA. It would be better to
> use a common method.

Yes, AST2500 SPI controller does not support DMA. For AST2600, SPI DMA engine supports
calibration mode. The checksum for a specific read area can be read and calculated by SoC HW.
I think this will improve calibration performance. Thus, I plan to use different calibration function
for AST2500 and AST2600.

> > +
> > +	writel((uint32_t)ast_ctrl->chips[cs].ahb_base_phy,
> > +	       ast_ctrl->regs + OFFSET_DMA_FLASH_ADDR_REG);
> > +	writel(CALIBRATION_LEN, ast_ctrl->regs + OFFSET_DMA_LEN_REG);
> > +
> > +	ctrl_val = SPI_DMA_ENABLE | SPI_DMA_CALC_CKSUM |
> SPI_DMA_CALIB_MODE |
> > +		   (delay << 8) | ((div & 0xf) << 16);
> > +	writel(ctrl_val, ast_ctrl->regs + OFFSET_DMA_CTRL);
> > +	while (!(readl(ast_ctrl->regs + OFFSET_INTR_CTRL_STATUS) &
> > +		 SPI_DMA_STATUS))
> > +		;
> > +
> > +	checksum = readl(ast_ctrl->regs + OFFSET_DMA_CHECKSUM_RESULT);
> > +
> > +	writel(0xdeea0000, ast_ctrl->regs + OFFSET_DMA_CTRL);
> > +	writel(0x0, ast_ctrl->regs + OFFSET_DMA_CTRL);
> > +
> > +	return checksum;
> > +}
> > +
> > +static int get_mid_point_of_longest_one(uint8_t *buf, uint32_t len) {
> > +	int i;
> > +	int start = 0, mid_point = 0;
> > +	int max_cnt = 0, cnt = 0;
> > +
> > +	for (i = 0; i < len; i++) {
> > +		if (buf[i] == 1) {
> > +			cnt++;
> > +		} else {
> > +			cnt = 0;
> > +			start = i;
> > +		}
> > +
> > +		if (max_cnt < cnt) {
> > +			max_cnt = cnt;
> > +			mid_point = start + (cnt / 2);
> > +		}
> > +	}
> > +
> > +	/*
> > +	 * In order to get a stable SPI read timing,
> > +	 * abandon the result if the length of longest
> > +	 * consecutive good points is too short.
> > +	 */
> > +	if (max_cnt < 4)
> > +		return -1;
> > +
> > +	return mid_point;
> > +}
> > +
> > +/* Transfer maximum clock frequency to register setting */ static
> > +uint32_t aspeed_2600_spi_clk_basic_setting(struct
> > +aspeed_spi_controller *ast_ctrl,
> > +				  uint32_t *max_clk)
> > +{
> > +	struct device *dev = ast_ctrl->dev;
> > +	uint32_t hclk_clk = ast_ctrl->ahb_clk;
> > +	uint32_t hclk_div = 0x400; /* default value */
> > +	uint32_t i, j = 0;
> > +	bool found = false;
> > +	/* HCLK/1 ..	HCLK/16 */
> > +	uint32_t hclk_masks[] = { 15, 7, 14, 6, 13, 5, 12, 4,
> > +				  11, 3, 10, 2, 9,  1, 8,  0 };
> > +
> > +	/* FMC/SPIR10[27:24] */
> > +	for (j = 0; j < 0xf; i++) {
> > +		/* FMC/SPIR10[11:8] */
> > +		for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
> > +			if (i == 0 && j == 0)
> > +				continue;
> > +
> > +			if (hclk_clk / (i + 1 + (j * 16)) <= *max_clk) {
> > +				found = 1;
> > +				*max_clk = hclk_clk / (i + 1 + (j * 16));
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (found) {
> > +			hclk_div = ((j << 24) | hclk_masks[i] << 8);
> > +			break;
> > +		}
> > +	}
> > +
> > +	dev_dbg(dev, "found: %s, hclk: %d, max_clk: %d\n", found ? "yes" : "no",
> > +		hclk_clk, *max_clk);
> > +	dev_dbg(dev, "base_clk: %d, h_div: %d (mask %x), speed: %d\n", j, i + 1,
> > +		hclk_masks[i], hclk_clk / (i + 1 + j * 16));
> > +
> > +	return hclk_div;
> > +}
> > +
> > +/*
> > + * If SPI frequency is too high, timing compensation is needed,
> > + * otherwise, SPI controller will sample unready data. For AST2600
> > + * SPI memory controller, only the first four frequency levels
> > + * (HCLK/2, HCLK/3,..., HCKL/5) may need timing compensation.
> > + * Here, for each frequency, we will get a sequence of reading
> > + * result (pass or fail) compared to golden data. Then, getting the
> > + * middle point of the maximum pass widow. Besides, if the flash's
> > + * content is too monotonous, the frequency recorded in the device
> > + * tree will be adopted.
> > + */
> > +static int
> > +aspeed_2600_spi_timing_calibration(struct aspeed_spi_controller *ast_ctrl,
> > +				   uint32_t cs)
> > +{
> > +	int ret = 0;
> > +	struct device *dev = ast_ctrl->dev;
> > +	struct aspeed_spi_chip *chip = &ast_ctrl->chips[cs];
> > +	uint32_t max_freq = chip->max_clk_freq;
> > +	/* HCLK/2, ..., HCKL/5 */
> > +	uint32_t hclk_masks[] = { 7, 14, 6, 13 };
> > +	uint8_t *calib_res = NULL;
> > +	uint8_t *check_buf = NULL;
> > +	uint32_t reg_val;
> > +	uint32_t checksum, gold_checksum;
> > +	uint32_t i, hcycle, delay_ns, final_delay = 0;
> > +	uint32_t hclk_div;
> > +	bool pass;
> > +	int calib_point;
> > +
> > +	reg_val =
> > +		readl(ast_ctrl->regs + OFFSET_CE0_TIMING_COMPENSATION + cs *
> 4);
> > +	if (reg_val != 0) {
> > +		dev_dbg(dev, "has executed calibration.\n");
> > +		goto no_calib;
> > +	}
> > +
> > +	dev_dbg(dev, "calculate timing compensation :\n");
> > +	/*
> > +	 * use the related low frequency to get check calibration data
> > +	 * and get golden data.
> > +	 */
> > +	reg_val = chip->ctrl_val[ASPEED_SPI_READ] & 0xf0fff0ff;
> > +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> > +
> > +	check_buf = kzalloc(CALIBRATION_LEN, GFP_KERNEL);
> > +	if (!check_buf)
> > +		return -ENOMEM;
> > +
> > +	memcpy_fromio(check_buf, chip->ahb_base, CALIBRATION_LEN);
> > +	if (!aspeed_spi_calibriation_enable(check_buf, CALIBRATION_LEN)) {
> > +		dev_info(dev, "flash data is monotonous, skip calibration.");
> > +		goto no_calib;
> > +	}
> > +
> > +	gold_checksum = aspeed_2600_spi_dma_checksum(ast_ctrl, cs, 0, 0);
> > +
> > +	/*
> > +	 * allocate a space to record calibration result for
> > +	 * different timing compensation with fixed
> > +	 * HCLK division.
> > +	 */
> > +	calib_res = kzalloc(6 * 17, GFP_KERNEL);
> > +	if (!calib_res) {
> > +		ret = -ENOMEM;
> > +		goto no_calib;
> > +	}
> > +
> > +	/* From HCLK/2 to HCLK/5 */
> > +	for (i = 0; i < ARRAY_SIZE(hclk_masks); i++) {
> > +		if (max_freq < (uint32_t)ast_ctrl->ahb_clk / (i + 2)) {
> > +			dev_dbg(dev, "skipping freq %d\n",
> > +				(uint32_t)ast_ctrl->ahb_clk / (i + 2));
> > +			continue;
> > +		}
> > +		max_freq = (uint32_t)ast_ctrl->ahb_clk / (i + 2);
> > +
> > +		checksum = aspeed_2600_spi_dma_checksum(ast_ctrl, cs,
> > +							hclk_masks[i], 0);
> > +		pass = (checksum == gold_checksum);
> > +		dev_dbg(dev, "HCLK/%d, no timing compensation: %s\n", i + 2,
> > +			pass ? "PASS" : "FAIL");
> > +
> > +		if (pass)
> > +			break;
> > +
> > +		memset(calib_res, 0x0, 6 * 17);
> > +
> > +		for (hcycle = 0; hcycle <= 5; hcycle++) {
> > +			/* increase DI delay by the step of 0.5ns */
> > +			dev_dbg(dev, "Delay Enable : hcycle %x\n", hcycle);
> > +			for (delay_ns = 0; delay_ns <= 0xf; delay_ns++) {
> > +				checksum = aspeed_2600_spi_dma_checksum(
> > +					ast_ctrl, cs, hclk_masks[i],
> > +					BIT(3) | hcycle | (delay_ns << 4));
> > +				pass = (checksum == gold_checksum);
> > +				calib_res[hcycle * 17 + delay_ns] = pass;
> > +				dev_dbg(dev,
> > +					"HCLK/%d, %d HCLK cycle, %d delay_ns : %s\n",
> > +					i + 2, hcycle, delay_ns,
> > +					pass ? "PASS" : "FAIL");
> > +			}
> > +		}
> > +
> > +		calib_point = get_mid_point_of_longest_one(calib_res, 6 * 17);
> > +		if (calib_point < 0) {
> > +			dev_info(dev, "cannot get good calibration point.\n");
> > +			continue;
> > +		}
> > +
> > +		hcycle = calib_point / 17;
> > +		delay_ns = calib_point % 17;
> > +		dev_dbg(dev, "final hcycle: %d, delay_ns: %d\n", hcycle,
> > +			delay_ns);
> > +
> > +		final_delay = (BIT(3) | hcycle | (delay_ns << 4)) << (i * 8);
> > +		writel(final_delay, ast_ctrl->regs +
> > +					    OFFSET_CE0_TIMING_COMPENSATION +
> > +					    cs * 4);
> > +		break;
> > +	}
> > +
> > +no_calib:
> > +
> > +	hclk_div = aspeed_2600_spi_clk_basic_setting(ast_ctrl, &max_freq);
> > +
> > +	/* configure SPI clock frequency */
> > +	reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> > +	reg_val = (reg_val & 0xf0fff0ff) | hclk_div;
> > +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> > +
> > +	/* add clock setting info for CE ctrl setting */
> > +	for (i = 0; i < ASPEED_SPI_MAX; i++)
> > +		chip->ctrl_val[i] = (chip->ctrl_val[i] & 0xf0fff0ff) | hclk_div;
> > +
> > +	dev_info(dev, "freq: %dMHz\n", max_freq / 1000000);
> > +
> > +	kfree(check_buf);
> > +	kfree(calib_res);
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * AST2600 SPI memory controllers support multiple chip selects.
> > + * The start address of a decode range should be multiple
> > + * of its related flash size. Namely, the total decoded size
> > + * from flash 0 to flash N should be multiple of flash (N + 1).
> > + */
> > +static void aspeed_2600_adjust_decode_sz(uint32_t decode_sz_arr[],
> > +int len) {
> > +	int cs, j;
> > +	uint32_t sz;
> > +
> > +	for (cs = len - 1; cs >= 0; cs--) {
> > +		sz = 0;
> > +		for (j = 0; j < cs; j++)
> > +			sz += decode_sz_arr[j];
> > +
> > +		if (sz % decode_sz_arr[cs] != 0)
> > +			decode_sz_arr[0] += (sz % decode_sz_arr[cs]);
> > +	}
> > +}
> > +
> > +static int
> > +aspeed_spi_decode_range_config(struct aspeed_spi_controller *ast_ctrl,
> > +			       uint32_t decode_sz_arr[])
> > +{
> > +	struct aspeed_spi_chip *chip = ast_ctrl->chips;
> > +	uint32_t i;
> > +	uint32_t cs;
> > +	uint32_t decode_reg_val;
> > +	phys_addr_t start_addr_phy, end_addr_phy, pre_end_addr_phy = 0;
> > +	uint32_t total_decode_sz = 0;
> > +
> > +	/* decode range sanity */
> > +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> > +		total_decode_sz += decode_sz_arr[cs];
> > +		if (ast_ctrl->ahb_window_sz < total_decode_sz) {
> > +			dev_err(ast_ctrl->dev, "insufficient decode size\n");
> > +			for (i = 0; i <= cs; i++)
> > +				dev_err(ast_ctrl->dev, "cs:%d %x\n", i,
> > +					decode_sz_arr[i]);
> > +			return -ENOSPC;
> > +		}
> > +	}
> > +
> > +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> > +		if (chip[cs].ahb_base)
> > +			devm_iounmap(ast_ctrl->dev, chip[cs].ahb_base);
> > +	}
> > +
> > +	/* configure each CE's decode range */
> > +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> > +		if (cs == 0)
> > +			start_addr_phy = ast_ctrl->ahb_base_phy;
> > +		else
> > +			start_addr_phy = pre_end_addr_phy;
> > +
> > +		chip[cs].ahb_base = devm_ioremap(ast_ctrl->dev, start_addr_phy,
> > +						 decode_sz_arr[cs]);
> > +		chip[cs].ahb_base_phy = start_addr_phy;
> > +
> > +		chip[cs].ahb_window_sz = decode_sz_arr[cs];
> > +		end_addr_phy = start_addr_phy + decode_sz_arr[cs];
> > +
> > +		decode_reg_val = ast_ctrl->info->segment_reg(
> > +			ast_ctrl, start_addr_phy, end_addr_phy);
> > +
> > +		writel(decode_reg_val,
> > +		       ast_ctrl->regs + OFFSET_CE0_DECODE_RANGE_REG + cs *
> 4);
> > +
> > +		pre_end_addr_phy = end_addr_phy;
> > +
> > +		dev_dbg(ast_ctrl->dev, "cs: %d, decode_reg: 0x%x\n", cs,
> > +			decode_reg_val);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct aspeed_spi_info ast2600_spi_info = {
> > +	.max_data_bus_width = 4,
> > +	.cmd_io_ctrl_mask = 0xf0ff40c3,
> > +	/* for ast2600, the minimum decode size for each CE is 2MB */
> > +	.min_decode_sz = 0x200000,
> > +	.set_4byte = aspeed_spi_chip_set_4byte,
> > +	.calibrate = aspeed_2600_spi_timing_calibration,
> > +	.adjust_decode_sz = aspeed_2600_adjust_decode_sz,
> > +	.segment_start = aspeed_2600_spi_segment_start,
> > +	.segment_end = aspeed_2600_spi_segment_end,
> > +	.segment_reg = aspeed_2600_spi_segment_reg, };
> > +
> > +/*
> > + * If the slave device is SPI NOR flash, there are two types
> > + * of command mode for ASPEED SPI memory controller used to
> > + * transfer data. The first one is user mode and the other is
> > + * command read/write mode. With user mode, SPI NOR flash
> > + * command, address and data processes are all handled by CPU.
> > + * But, when address filter is enabled to protect some flash
> > + * regions from being written, user mode will be disabled.
> > + * Thus, here, we use command read/write mode to issue SPI
> > + * operations. After remapping flash space correctly, we can
> > + * easily read/write data to flash by reading or writing
> > + * related remapped address, then, SPI NOR flash command and
> > + * address will be transferred to flash by controller
> > + * automatically. Besides, ASPEED SPI memory controller can
> > + * also block address or data bytes by configure FMC0C/SPIR0C
> > + * address and data mask register in order to satisfy the
> > + * following SPI flash operation sequences: (command) only,
> > + * (command and address) only or (coommand and data) only.
> > + */
> > +static int aspeed_spi_exec_op(struct spi_mem *mem, const struct
> > +spi_mem_op *op) {
> > +	struct aspeed_spi_controller *ast_ctrl =
> > +		spi_controller_get_devdata(mem->spi->master);
> > +	struct device *dev = ast_ctrl->dev;
> > +	uint32_t cs = mem->spi->chip_select;
> > +	struct aspeed_spi_chip *chip = &ast_ctrl->chips[cs];
> > +	uint32_t ctrl_val;
> > +	uint32_t addr_mode_reg, addr_mode_reg_backup;
> > +	uint32_t addr_data_mask = 0;
> > +	void __iomem *op_addr;
> > +	const void *data_buf;
> > +	uint32_t data_byte = 0;
> > +	uint32_t dummy_data = 0;
> > +
> > +	dev_dbg(dev,
> "cmd:%x(%d),addr:%llx(%d),dummy:%d(%d),data_len:%x(%d)\n",
> > +		op->cmd.opcode, op->cmd.buswidth, op->addr.val,
> > +		op->addr.buswidth, op->dummy.nbytes, op->dummy.buswidth,
> > +		op->data.nbytes, op->data.buswidth);
> > +
> > +	addr_mode_reg = addr_mode_reg_backup =
> > +		readl(ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> > +	addr_data_mask = readl(ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
> > +
> > +	ctrl_val = chip->ctrl_val[ASPEED_SPI_BASE];
> > +	ctrl_val &= ~ast_ctrl->info->cmd_io_ctrl_mask;
> > +
> > +	/* configure opcode */
> > +	ctrl_val |= op->cmd.opcode << 16;
> > +
> > +	/* configure operation address, address length and address mask */
> > +	if (op->addr.nbytes != 0) {
> > +		if (op->addr.nbytes == 3)
> > +			addr_mode_reg &= ~(0x11 << cs);
> > +		else
> > +			addr_mode_reg |= (0x11 << cs);
> 
> please use define.

Okay, it will be updated on the next patch.

> 
> > +
> > +		addr_data_mask &= 0x0f;
> > +		op_addr = chip->ahb_base + op->addr.val;
> > +	} else {
> > +		addr_data_mask |= 0xf0;
> 
> Disabling the address lanes needs an explanation.

It will be added in the comment above this function.

> 
> > +		op_addr = chip->ahb_base;
> > +	}
> > +
> > +	if (op->dummy.nbytes != 0) {
> > +		ctrl_val |= ((op->dummy.nbytes & 0x3) << 6 |
> > +			     (op->dummy.nbytes & 0x4) << 14);
> > +	}
> > +
> > +	/* configure data io mode and data mask */
> > +	if (op->data.nbytes != 0) {
> > +		addr_data_mask &= 0xF0;
> > +		data_byte = op->data.nbytes;
> > +		if (op->data.dir == SPI_MEM_DATA_OUT)
> > +			data_buf = op->data.buf.out;
> > +		else
> > +			data_buf = op->data.buf.in;
> > +
> > +		if (op->data.buswidth)
> > +			ctrl_val |= aspeed_spi_get_io_mode(op->data.buswidth);
> > +
> > +	} else {
> > +		addr_data_mask |= 0x0f;
> > +		data_byte = 1;
> > +		data_buf = &dummy_data;
> > +	}
> > +
> > +	/* configure command mode */
> > +	if (op->data.dir == SPI_MEM_DATA_OUT)
> > +		ctrl_val |= CTRL_IO_MODE_CMD_WRITE;
> > +	else
> > +		ctrl_val |= CTRL_IO_MODE_CMD_READ;
> > +
> > +	/* set controller registers */
> > +	writel(ctrl_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> > +	writel(addr_mode_reg, ast_ctrl->regs + OFFSET_CE_ADDR_MODE_CTRL);
> > +	writel(addr_data_mask, ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
> > +
> > +	dev_dbg(dev, "ctrl: 0x%08x, addr_mode: 0x%x, mask: 0x%x, addr:%px\n",
> > +		ctrl_val, addr_mode_reg, addr_data_mask, op_addr);
> > +
> > +	/* trigger spi transmission or reception sequence */
> > +	if (op->data.dir == SPI_MEM_DATA_OUT)
> > +		memcpy_toio(op_addr, data_buf, data_byte);
> > +	else
> > +		memcpy_fromio((void *)data_buf, op_addr, data_byte);
> > +
> > +	/* restore controller setting */
> > +	writel(chip->ctrl_val[ASPEED_SPI_READ],
> > +	       ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> > +	writel(addr_mode_reg_backup, ast_ctrl->regs +
> OFFSET_CE_ADDR_MODE_CTRL);
> > +	writel(0x0, ast_ctrl->regs + OFFSET_ADDR_DATA_MASK);
> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t aspeed_spi_dirmap_read(struct spi_mem_dirmap_desc *desc,
> > +				  uint64_t offs, size_t len, void *buf) {
> > +	struct aspeed_spi_controller *ast_ctrl =
> > +		spi_controller_get_devdata(desc->mem->spi->master);
> > +	struct aspeed_spi_chip *chip =
> > +		&ast_ctrl->chips[desc->mem->spi->chip_select];
> > +	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
> > +
> > +	if (chip->ahb_window_sz < offs + len) {
> > +		dev_info(ast_ctrl->dev,
> > +			 "read range exceeds flash remapping size\n");
> > +		return 0;
> > +	}
> > +
> > +	dev_dbg(ast_ctrl->dev, "read op:0x%x, addr:0x%llx, len:0x%zx\n",
> > +		op_tmpl.cmd.opcode, offs, len);
> > +
> > +	memcpy_fromio(buf, chip->ahb_base + offs, len);
> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t aspeed_spi_dirmap_write(struct spi_mem_dirmap_desc
> *desc,
> > +				   uint64_t offs, size_t len, const void *buf) {
> > +	struct aspeed_spi_controller *ast_ctrl =
> > +		spi_controller_get_devdata(desc->mem->spi->master);
> > +	struct aspeed_spi_chip *chip =
> > +		&ast_ctrl->chips[desc->mem->spi->chip_select];
> > +	uint32_t reg_val;
> > +	uint32_t target_cs = desc->mem->spi->chip_select;
> > +	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
> > +
> > +	if (chip->ahb_window_sz < offs + len) {
> > +		dev_info(ast_ctrl->dev,
> > +			 "write range exceeds flash remapping size\n");
> > +		return 0;
> > +	}
> > +
> > +	dev_dbg(ast_ctrl->dev, "write op:0x%x, addr:0x%llx, len:0x%zx\n",
> > +		op_tmpl.cmd.opcode, offs, len);
> > +
> > +	reg_val = ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE];
> > +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs *
> > +4);
> > +
> > +	memcpy_toio(chip->ahb_base + offs, buf, len);
> > +
> > +	reg_val = ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ];
> > +	writel(reg_val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs *
> > +4);
> > +
> > +	return len;
> > +}
> > +
> > +static int aspeed_spi_dirmap_create(struct spi_mem_dirmap_desc *desc)
> > +{
> > +	int ret = 0;
> > +	struct aspeed_spi_controller *ast_ctrl =
> > +		spi_controller_get_devdata(desc->mem->spi->master);
> > +	struct device *dev = ast_ctrl->dev;
> > +	const struct aspeed_spi_info *info = ast_ctrl->info;
> > +	struct spi_mem_op op_tmpl = desc->info.op_tmpl;
> > +	uint32_t decode_sz_arr[5];
> > +	uint32_t cs, target_cs = desc->mem->spi->chip_select;
> > +	uint32_t reg_val;
> > +
> > +	if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN) {
> > +		/* record original decode size */
> > +		for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> > +			reg_val = readl(ast_ctrl->regs +
> > +					OFFSET_CE0_DECODE_RANGE_REG + cs * 4);
> > +			decode_sz_arr[cs] =
> > +				info->segment_end(ast_ctrl, reg_val) -
> > +				info->segment_start(ast_ctrl, reg_val);
> > +		}
> > +
> > +		decode_sz_arr[target_cs] = desc->info.length;
> > +
> > +		if (info->adjust_decode_sz)
> > +			info->adjust_decode_sz(decode_sz_arr, ast_ctrl->num_cs);
> > +
> > +		for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> > +			dev_dbg(dev, "cs: %d, sz: 0x%x\n", cs,
> > +				decode_sz_arr[cs]);
> > +		}
> > +
> > +		ret = aspeed_spi_decode_range_config(ast_ctrl, decode_sz_arr);
> > +		if (ret)
> > +			return ret;
> > +
> > +		reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG +
> > +				target_cs * 4) &
> > +			  (~info->cmd_io_ctrl_mask);
> > +		reg_val |= aspeed_spi_get_io_mode(op_tmpl.data.buswidth) |
> > +			   op_tmpl.cmd.opcode << 16 |
> > +			   ((op_tmpl.dummy.nbytes) & 0x3) << 6 |
> > +			   ((op_tmpl.dummy.nbytes) & 0x4) << 14 |
> > +			   CTRL_IO_MODE_CMD_READ;
> > +
> > +		writel(reg_val,
> > +		       ast_ctrl->regs + OFFSET_CE0_CTRL_REG + target_cs * 4);
> > +		ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ] = reg_val;
> > +		ast_ctrl->chips[target_cs].max_clk_freq =
> > +			desc->mem->spi->max_speed_hz;
> > +
> > +		ret = info->calibrate(ast_ctrl, target_cs);
> > +
> > +		dev_info(dev, "read bus width: %d [0x%08x]\n",
> > +			 op_tmpl.data.buswidth,
> > +			 ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_READ]);
> > +
> > +	} else if (desc->info.op_tmpl.data.dir == SPI_MEM_DATA_OUT) {
> > +		reg_val = readl(ast_ctrl->regs + OFFSET_CE0_CTRL_REG +
> > +				target_cs * 4) &
> > +			  (~info->cmd_io_ctrl_mask);
> > +		reg_val |= aspeed_spi_get_io_mode(op_tmpl.data.buswidth) |
> > +			   op_tmpl.cmd.opcode << 16 |
> CTRL_IO_MODE_CMD_WRITE;
> > +		ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE] = reg_val;
> > +
> > +		dev_info(dev, "write bus width: %d [0x%08x]\n",
> > +			 op_tmpl.data.buswidth,
> > +			 ast_ctrl->chips[target_cs].ctrl_val[ASPEED_SPI_WRITE]);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static const char *aspeed_spi_get_name(struct spi_mem *mem) {
> > +	struct device *dev = &mem->spi->master->dev;
> > +	const char *name;
> > +
> > +	name = devm_kasprintf(dev, GFP_KERNEL, "%s-%d", dev_name(dev),
> > +			      mem->spi->chip_select);
> > +
> > +	if (!name) {
> > +		dev_err(dev, "cannot get spi name\n");
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	return name;
> > +}
> > +
> > +/*
> > + * Currently, only support 1-1-1, 1-1-2 or 1-1-4
> > + * SPI NOR flash operation format.
> > + */
> > +static bool aspeed_spi_support_op(struct spi_mem *mem,
> > +				  const struct spi_mem_op *op)
> > +{
> > +	struct aspeed_spi_controller *ast_ctrl =
> > +		spi_controller_get_devdata(mem->spi->master);
> > +
> > +	if (op->cmd.buswidth > 1)
> > +		return false;
> > +
> > +	if (op->addr.nbytes != 0) {
> > +		if (op->addr.buswidth > 1 || op->addr.nbytes > 4)
> > +			return false;
> > +	}
> > +
> > +	if (op->dummy.nbytes != 0) {
> > +		if (op->dummy.buswidth > 1 || op->dummy.nbytes > 7)
> > +			return false;
> > +	}
> > +
> > +	if (op->data.nbytes != 0 &&
> > +	    ast_ctrl->info->max_data_bus_width < op->data.buswidth)
> > +		return false;
> > +
> > +	if (!spi_mem_default_supports_op(mem, op))
> > +		return false;
> > +
> > +	if (op->addr.nbytes == 4)
> > +		ast_ctrl->info->set_4byte(ast_ctrl, mem->spi->chip_select);
> > +
> > +	return true;
> > +}
> > +
> > +static const struct spi_controller_mem_ops aspeed_spi_mem_ops = {
> > +	.exec_op = aspeed_spi_exec_op,
> > +	.get_name = aspeed_spi_get_name,
> > +	.supports_op = aspeed_spi_support_op,
> > +	.dirmap_create = aspeed_spi_dirmap_create,
> > +	.dirmap_read = aspeed_spi_dirmap_read,
> > +	.dirmap_write = aspeed_spi_dirmap_write, };
> > +
> > +/*
> > + * Initialize SPI controller for each chip select.
> > + * Here, only the minimum decode range is configured
> > + * in order to get device (SPI NOR flash) information
> > + * at the early stage.
> > + */
> > +static int aspeed_spi_ctrl_init(struct aspeed_spi_controller
> > +*ast_ctrl) {
> > +	int ret;
> > +	uint32_t cs;
> > +	uint32_t val;
> > +	uint32_t decode_sz_arr[ASPEED_SPI_MAX_CS];
> > +
> > +	/* enable write capability for all CEs */
> > +	val = readl(ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> > +	writel(val | (GENMASK(ast_ctrl->num_cs, 0) << 16),
> > +	       ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> > +
> > +	/* initial each CE's controller register */
> > +	for (cs = 0; cs < ast_ctrl->num_cs; cs++) {
> > +		val = CTRL_STOP_ACTIVE | CTRL_IO_MODE_USER;
> > +		writel(val, ast_ctrl->regs + OFFSET_CE0_CTRL_REG + cs * 4);
> > +		ast_ctrl->chips[cs].ctrl_val[ASPEED_SPI_BASE] = val;
> > +	}
> > +
> > +	for (cs = 0; cs < ast_ctrl->num_cs && cs < ASPEED_SPI_MAX_CS; cs++)
> > +		decode_sz_arr[cs] = ast_ctrl->info->min_decode_sz;
> > +
> > +	ret = aspeed_spi_decode_range_config(ast_ctrl, decode_sz_arr);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct of_device_id aspeed_spi_matches[] = {
> > +	{ .compatible = "aspeed,ast2600-fmc", .data = &ast2600_spi_info },
> > +	{ .compatible = "aspeed,ast2600-spi", .data = &ast2600_spi_info },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_spi_matches);
> > +
> > +static int aspeed_spi_probe(struct platform_device *pdev) {
> > +	int ret;
> > +	struct device *dev = &pdev->dev;
> > +	struct spi_controller *spi_ctrl;
> > +	struct aspeed_spi_controller *ast_ctrl;
> > +	const struct of_device_id *match;
> > +	struct clk *clk;
> > +	struct resource *res;
> > +
> > +	spi_ctrl = spi_alloc_master(dev, sizeof(struct aspeed_spi_controller));
> > +	if (!spi_ctrl)
> > +		return -ENOMEM;
> > +
> > +	ast_ctrl = spi_controller_get_devdata(spi_ctrl);
> > +
> > +	match = of_match_device(aspeed_spi_matches, dev);
> > +	if (!match || !match->data) {
> > +		dev_err(dev, "no compatible OF match\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	ast_ctrl->info = match->data;
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +					   "spi_ctrl_reg");
> > +	ast_ctrl->regs = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(ast_ctrl->regs))
> > +		return PTR_ERR(ast_ctrl->regs);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "spi_mmap");
> > +	ast_ctrl->ahb_base_phy = res->start;
> > +	ast_ctrl->ahb_window_sz = resource_size(res);
> > +
> > +	ast_ctrl->dev = dev;
> > +
> > +	clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(clk))
> > +		return PTR_ERR(clk);
> > +	ast_ctrl->ahb_clk = clk_get_rate(clk);
> > +	devm_clk_put(&pdev->dev, clk);
> > +
> > +	if (of_property_read_u32(dev->of_node, "num-cs", &ast_ctrl->num_cs)) {
> > +		dev_err(dev, "fail to get chip number.\n");
> > +		goto end;
> > +	}
> > +
> > +	if (ast_ctrl->num_cs > ASPEED_SPI_MAX_CS) {
> > +		dev_err(dev, "chip number, %d, exceeds %d.\n", ast_ctrl->num_cs,
> > +			ASPEED_SPI_MAX_CS);
> > +		goto end;
> > +	}
> > +
> > +	ast_ctrl->chips =
> > +		devm_kzalloc(dev,
> > +			     sizeof(struct aspeed_spi_chip) * ast_ctrl->num_cs,
> > +			     GFP_KERNEL);
> > +
> > +	platform_set_drvdata(pdev, ast_ctrl);
> > +
> > +	spi_ctrl->mode_bits =
> > +		SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
> > +
> > +	spi_ctrl->bus_num = -1;
> > +	spi_ctrl->mem_ops = &aspeed_spi_mem_ops;
> > +	spi_ctrl->dev.of_node = dev->of_node;
> > +	spi_ctrl->num_chipselect = ast_ctrl->num_cs;
> > +
> > +	ret = aspeed_spi_ctrl_init(ast_ctrl);
> > +	if (ret)
> > +		goto end;
> > +
> > +	ret = devm_spi_register_master(dev, spi_ctrl);
> > +
> > +end:
> > +	return ret;
> > +}
> > +
> > +static int aspeed_spi_remove(struct platform_device *pdev) {
> > +	struct aspeed_spi_controller *ast_ctrl = platform_get_drvdata(pdev);
> > +	uint32_t val;
> > +
> > +	/* disable write capability for all CEs */
> > +	val = readl(ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> > +	writel(val & ~(GENMASK(ast_ctrl->num_cs, 0) << 16),
> > +	       ast_ctrl->regs + OFFSET_CE_TYPE_SETTING);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver aspeed_spi_driver = {
> > +	.driver = {
> > +		.name = "ASPEED_FMC_SPI",
> > +		.bus = &platform_bus_type,
> > +		.of_match_table = aspeed_spi_matches,
> > +	},
> > +	.probe = aspeed_spi_probe,
> > +	.remove = aspeed_spi_remove,
> > +};
> > +module_platform_driver(aspeed_spi_driver);
> > +
> > +MODULE_DESCRIPTION("ASPEED FMC/SPI Memory Controller Driver");
> > +MODULE_AUTHOR("Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>");
> > +MODULE_AUTHOR("Cedric Le Goater <clg@kaod.org>");
> MODULE_LICENSE("GPL
> > +v2");
> >


^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-05 15:11     ` Boris Brezillon
  2020-11-05 16:43       ` Mark Brown
@ 2020-11-06  8:58       ` Chin-Ting Kuo
  2020-11-06  9:05         ` Boris Brezillon
  1 sibling, 1 reply; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-06  8:58 UTC (permalink / raw)
  To: Boris Brezillon, Cédric Le Goater, robh+dt
  Cc: broonie, joel, andrew, bbrezillon, devicetree, linux-kernel,
	linux-aspeed, linux-spi, BMC-SW

Hi Boris,

Thanks for your quick reply.

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Sent: Thursday, November 5, 2020 11:12 PM
> To: Cédric Le Goater <clg@kaod.org>; robh+dt@kernel.org
> Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>; broonie@kernel.org;
> joel@jms.id.au; andrew@aj.id.au; bbrezillon@kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-aspeed@lists.ozlabs.org; linux-spi@vger.kernel.org; BMC-SW
> <BMC-SW@aspeedtech.com>
> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> driver
> 
> Hi,
> 
> On Thu, 5 Nov 2020 15:09:11 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > Hello Chin-Ting,
> >
> > Thanks for this driver. It's much cleaner than the previous and we
> > should try adding support for the AST2500 SoC also. I guess we can
> > keep the old driver for the AST2400 which has a different register layout.
> >
> > On the patchset, I think we should split this patch in three :
> >
> >  - basic support
> >  - AHB window calculation depending on the flash size
> >  - read training support
> 
> I didn't look closely at the implementation, but if the read training tries to read
> a section of the NOR, I'd recommend exposing that feature through spi-mem
> and letting the SPI-NOR framework trigger the training instead of doing that at
> dirmap creation time (remember that spi-mem is also used for SPI NANDs
> which use the dirmap API too, and this training is unlikely to work there).
> 
> The SPI-NOR framework could pass a read op template and a reference pattern
> such that all the spi-mem driver has to do is execute the template op and
> compare the output to the reference buffer.
> 

I agree it. Before, I were not able to find a suitable location to implement read training feature.
I think that I can add a SPI timing training function in "spi_controller_mem_ops" struct and
call it by a wrapper function called at the bottom of spi_nor_probe() in spi-nor.c.
Maybe, SPI-NOR framework does not need to pass reference buffer since calibration
method depends on each SoC itself and buffer size may be variant.
The detail calibration method may be implemented in each SoC SPI driver.

Besides, I am thinking about the possibility for adding a "spi_mem_post_init" function in
spi-mem framework sine for some SoCs, SPI controller needs to adjust some settings
after getting SPI flash information.

> 
> >
> > We should avoid magic values when setting registers. This is confusing
> > and defines are much better.
> >
> > AST2500 support will be a bit challenging because HW does not allow
> > to configure a 128MB AHB window, max is 120MB This is a bug and the
> > work around is to use user mode for the remaining 8MB. Something to
> > keep in mind.
> 
> Most SPI-MEM controllers don't have such a big dirmap window anyway, and
> that shouldn't be a problem, because we don't expose the direct mapping
> directly (as would be done if we were trying to support something like XIP).
> That means that, assuming your controller allows controlling the base spi-mem
> address the direct mapping points to, you should be able to adjust the window
> at runtime and make it point where you requested.
> 
> Note that dirmap_{read,write}() are allowed to return less data than requested
> thus simplifying the case where a specific access requires a window
> adjustment in the middle of an read/write operation.

Thanks for your remainder.


Best Wishes,
Chin-Ting

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-05 16:43       ` Mark Brown
@ 2020-11-06  9:01         ` Chin-Ting Kuo
  0 siblings, 0 replies; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-06  9:01 UTC (permalink / raw)
  To: Mark Brown, Boris Brezillon
  Cc: Cédric Le Goater, robh+dt, joel, andrew, bbrezillon,
	devicetree, linux-kernel, linux-aspeed, linux-spi, BMC-SW

Hi Mark,

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Friday, November 6, 2020 12:43 AM
> To: Boris Brezillon <boris.brezillon@collabora.com>
> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> driver
> 
> On Thu, Nov 05, 2020 at 04:11:32PM +0100, Boris Brezillon wrote:
> > Cédric Le Goater <clg@kaod.org> wrote:
> 
> > > Thanks for this driver. It's much cleaner than the previous and we
> > > should try adding support for the AST2500 SoC also. I guess we can
> > > keep the old driver for the AST2400 which has a different register layout.
> > >
> > > On the patchset, I think we should split this patch in three :
> > >
> > >  - basic support
> > >  - AHB window calculation depending on the flash size
> > >  - read training support
> 
> > I didn't look closely at the implementation, but if the read training
> > tries to read a section of the NOR, I'd recommend exposing that
> > feature through spi-mem and letting the SPI-NOR framework trigger the
> > training instead of doing that at dirmap creation time (remember that
> > spi-mem is also used for SPI NANDs which use the dirmap API too, and
> > this training is unlikely to work there).
> 
> > The SPI-NOR framework could pass a read op template and a reference
> > pattern such that all the spi-mem driver has to do is execute the
> > template op and compare the output to the reference buffer.
> 
> That seems like a good idea.

Yes, this idea will be implemented on the next patch version.

> 
> > > We should avoid magic values when setting registers. This is
> > > confusing and defines are much better.
> 
> It does depend a bit on documentation though, it's not a hard requirement.

I will update it on the next patch version.



Thanks.

Best Wishes,
Chin-Ting

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-06  8:58       ` Chin-Ting Kuo
@ 2020-11-06  9:05         ` Boris Brezillon
  2020-11-06 10:21           ` Chin-Ting Kuo
  0 siblings, 1 reply; 20+ messages in thread
From: Boris Brezillon @ 2020-11-06  9:05 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: Cédric Le Goater, robh+dt, broonie, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi,
	BMC-SW

On Fri, 6 Nov 2020 08:58:23 +0000
Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:

> Hi Boris,
> 
> Thanks for your quick reply.
> 
> > -----Original Message-----
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Sent: Thursday, November 5, 2020 11:12 PM
> > To: Cédric Le Goater <clg@kaod.org>; robh+dt@kernel.org
> > Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>; broonie@kernel.org;
> > joel@jms.id.au; andrew@aj.id.au; bbrezillon@kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> > linux-aspeed@lists.ozlabs.org; linux-spi@vger.kernel.org; BMC-SW
> > <BMC-SW@aspeedtech.com>
> > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> > driver
> > 
> > Hi,
> > 
> > On Thu, 5 Nov 2020 15:09:11 +0100
> > Cédric Le Goater <clg@kaod.org> wrote:
> >   
> > > Hello Chin-Ting,
> > >
> > > Thanks for this driver. It's much cleaner than the previous and we
> > > should try adding support for the AST2500 SoC also. I guess we can
> > > keep the old driver for the AST2400 which has a different register layout.
> > >
> > > On the patchset, I think we should split this patch in three :
> > >
> > >  - basic support
> > >  - AHB window calculation depending on the flash size
> > >  - read training support  
> > 
> > I didn't look closely at the implementation, but if the read training tries to read
> > a section of the NOR, I'd recommend exposing that feature through spi-mem
> > and letting the SPI-NOR framework trigger the training instead of doing that at
> > dirmap creation time (remember that spi-mem is also used for SPI NANDs
> > which use the dirmap API too, and this training is unlikely to work there).
> > 
> > The SPI-NOR framework could pass a read op template and a reference pattern
> > such that all the spi-mem driver has to do is execute the template op and
> > compare the output to the reference buffer.
> >   
> 
> I agree it. Before, I were not able to find a suitable location to implement read training feature.
> I think that I can add a SPI timing training function in "spi_controller_mem_ops" struct and
> call it by a wrapper function called at the bottom of spi_nor_probe() in spi-nor.c.
> Maybe, SPI-NOR framework does not need to pass reference buffer since calibration
> method depends on each SoC itself and buffer size may be variant.
> The detail calibration method may be implemented in each SoC SPI driver.

That's a real problem IMO. What makes this pattern SoC specific? I can
see why the location in flash could be *board* specific, but the
pattern should be pretty common, right? As for the spi-mem operation to
be executed, it's definitely memory specific (I can imagine some flash
vendors providing a specific command returning a fixed pattern that's
not actually stored on a visible portion of the flash).

> 
> Besides, I am thinking about the possibility for adding a "spi_mem_post_init" function in
> spi-mem framework sine for some SoCs, SPI controller needs to adjust some settings
> after getting SPI flash information.

I don't think that's a good idea. The spi-mem interface should stay
memory-type agnostic and doing that means we somehow pass NOR specific
info. What is it that you need exactly, and why?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller
  2020-11-05 22:39   ` Rob Herring
@ 2020-11-06  9:11     ` Chin-Ting Kuo
  0 siblings, 0 replies; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-06  9:11 UTC (permalink / raw)
  To: Rob Herring
  Cc: broonie, joel, andrew, clg, bbrezillon, devicetree, linux-kernel,
	linux-aspeed, linux-spi, BMC-SW

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, November 6, 2020 6:40 AM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Cc: broonie@kernel.org; joel@jms.id.au; andrew@aj.id.au; clg@kaod.org;
> bbrezillon@kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-aspeed@lists.ozlabs.org;
> linux-spi@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> Subject: Re: [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI
> memory controller
> 
> On Thu, Nov 05, 2020 at 08:03:28PM +0800, Chin-Ting Kuo wrote:
> > Create binding file with YAML syntax for ASPEED FMC/SPI memory
> controller.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  .../bindings/spi/aspeed,spi-aspeed.yaml       | 66
> +++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
> > b/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
> > new file mode 100644
> > index 000000000000..41b9692c7226
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/aspeed,spi-aspeed.yaml
> > @@ -0,0 +1,66 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/spi/aspeed,spi-aspeed.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SPI memory controller for ASPEED SoCs
> > +
> > +maintainers:
> > +  - Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > +
> > +description: |
> > +  There are three SPI memory controllers embedded in a ASPEED SoC.
> > +  They are usually connected to SPI NOR flashes. Each of them has
> > +  more than a chip select. They also support SPI single, dual and
> > +  quad IO modes for SPI NOR flash.
> > +
> > +allOf:
> > +  - $ref: /spi/spi-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> 
> You can drop oneOf (there's only 1) and items.

Okay, it will be removed on the next patch version.

> > +          - enum:
> > +              - aspeed,ast2600-fmc
> > +              - aspeed,ast2600-spi
> > +
> > +  reg:
> > +    items:
> > +      - description: the control register location and length
> > +      - description: the flash memory mapping address and length
> > +
> > +  clocks:
> > +    description: AHB bus clock which will be converted to SPI bus
> > + clock
> 
> maxItems: 1

it will be added on the next patch version.

> 
> Constraints on num-cs values? Or up to 2^32 is good?
>

The maximum is 3.
The "maximum" item will be added on the next patch version

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - num-cs
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/ast2600-clock.h>
> > +    spi1: spi@1e630000 {
> > +      compatible = "aspeed,ast2600-spi";
> > +      reg = <0x1e630000 0xc4>, <0x30000000 0x10000000>;
> > +      reg-names = "spi_ctrl_reg", "spi_mmap";
> > +      clocks = <&syscon ASPEED_CLK_AHB>;
> > +      num-cs = <2>;
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      flash@0 {
> > +        compatible = "jedec,spi-nor";
> > +        reg = <0>;
> > +        spi-max-frequency = <50000000>;
> > +      };
> > +      flash@1 {
> > +        compatible = "jedec,spi-nor";
> > +        reg = <1>;
> > +        spi-max-frequency = <50000000>;
> > +      };
> > +    };
> > --
> > 2.17.1
> >

Best Wishes,
Chin-Ting

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-06  9:05         ` Boris Brezillon
@ 2020-11-06 10:21           ` Chin-Ting Kuo
  2020-11-06 11:30             ` Boris Brezillon
  0 siblings, 1 reply; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-06 10:21 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Cédric Le Goater, robh+dt, broonie, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi,
	BMC-SW

Hi Boris,

Thanks for your comments and suggestions.

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Sent: Friday, November 6, 2020 5:06 PM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> driver
> 
> On Fri, 6 Nov 2020 08:58:23 +0000
> Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
> 
> > Hi Boris,
> >
> > Thanks for your quick reply.
> >
> > > -----Original Message-----
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Sent: Thursday, November 5, 2020 11:12 PM
> > > To: Cédric Le Goater <clg@kaod.org>; robh+dt@kernel.org
> > > Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>;
> > > broonie@kernel.org; joel@jms.id.au; andrew@aj.id.au;
> > > bbrezillon@kernel.org; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-aspeed@lists.ozlabs.org;
> > > linux-spi@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
> > > controller driver
> > >
> > > Hi,
> > >
> > > On Thu, 5 Nov 2020 15:09:11 +0100
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >
> > > > Hello Chin-Ting,
> > > >
> > > > Thanks for this driver. It's much cleaner than the previous and we
> > > > should try adding support for the AST2500 SoC also. I guess we can
> > > > keep the old driver for the AST2400 which has a different register layout.
> > > >
> > > > On the patchset, I think we should split this patch in three :
> > > >
> > > >  - basic support
> > > >  - AHB window calculation depending on the flash size
> > > >  - read training support
> > >
> > > I didn't look closely at the implementation, but if the read
> > > training tries to read a section of the NOR, I'd recommend exposing
> > > that feature through spi-mem and letting the SPI-NOR framework
> > > trigger the training instead of doing that at dirmap creation time
> > > (remember that spi-mem is also used for SPI NANDs which use the dirmap
> API too, and this training is unlikely to work there).
> > >
> > > The SPI-NOR framework could pass a read op template and a reference
> > > pattern such that all the spi-mem driver has to do is execute the
> > > template op and compare the output to the reference buffer.
> > >
> >
> > I agree it. Before, I were not able to find a suitable location to implement
> read training feature.
> > I think that I can add a SPI timing training function in
> > "spi_controller_mem_ops" struct and call it by a wrapper function called at
> the bottom of spi_nor_probe() in spi-nor.c.
> > Maybe, SPI-NOR framework does not need to pass reference buffer since
> > calibration method depends on each SoC itself and buffer size may be
> variant.
> > The detail calibration method may be implemented in each SoC SPI driver.
> 
> That's a real problem IMO. What makes this pattern SoC specific? I can see
> why the location in flash could be *board* specific, but the pattern should be
> pretty common, right? As for the spi-mem operation to be executed, it's
> definitely memory specific (I can imagine some flash vendors providing a
> specific command returning a fixed pattern that's not actually stored on a
> visible portion of the flash).

You are right, the pattern should be pretty common. The thing I was worried about is the size of
that buffer since, maybe, some controllers need to read more data than others in order to get good
training result.

> >
> > Besides, I am thinking about the possibility for adding a
> > "spi_mem_post_init" function in spi-mem framework sine for some SoCs,
> > SPI controller needs to adjust some settings after getting SPI flash
> information.
> 
> I don't think that's a good idea. The spi-mem interface should stay
> memory-type agnostic and doing that means we somehow pass NOR specific
> info. What is it that you need exactly, and why?

Yes, as you mention, the spi-mem interface should stay memory-type agnostic. Thus, currently, I just think about this, not implementation.

Why did I need this exactly?
Take ASPEED SPI controller for example, ASPEED SPI controller is designed for SPI NOR flash especially.
When ASPEED SoC powers on or reset, MCU ROM will fetch SPI NOR flash through SPI controller.
But, MCU ROM does not know the current address mode of SPI NOR flash when SoC was reset (SPI flash is not reset).
Therefore, SPI flash driver needs to set related flag to notify MCU ROM when flash is set to 4B address mode and 4B read opcode is used.

Besides, for other SoCs connected to ASPEED SoC, they can read/write SPI NOR flash connected to ASPEED SoC by a pure HW channel without any interaction of SW driver.
But, before trigger this feature, flash read/write/erase opcode, dummy cycle and other information should be filled in the related registers in advance because that HW channel
does not know accurate information about connected SPI NOR flash.


Best Wishes,
Chin-Ting





^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-06 10:21           ` Chin-Ting Kuo
@ 2020-11-06 11:30             ` Boris Brezillon
  2020-11-06 18:27               ` Chin-Ting Kuo
  0 siblings, 1 reply; 20+ messages in thread
From: Boris Brezillon @ 2020-11-06 11:30 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: Cédric Le Goater, robh+dt, broonie, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi,
	BMC-SW, Vignesh R, Tudor Ambarus

+Tudor and Vignesh

On Fri, 6 Nov 2020 10:21:06 +0000
Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:

> Hi Boris,
> 
> Thanks for your comments and suggestions.
> 
> > -----Original Message-----
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Sent: Friday, November 6, 2020 5:06 PM
> > To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> > driver
> > 
> > On Fri, 6 Nov 2020 08:58:23 +0000
> > Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
> >   
> > > Hi Boris,
> > >
> > > Thanks for your quick reply.
> > >  
> > > > -----Original Message-----
> > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Sent: Thursday, November 5, 2020 11:12 PM
> > > > To: Cédric Le Goater <clg@kaod.org>; robh+dt@kernel.org
> > > > Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>;
> > > > broonie@kernel.org; joel@jms.id.au; andrew@aj.id.au;
> > > > bbrezillon@kernel.org; devicetree@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; linux-aspeed@lists.ozlabs.org;
> > > > linux-spi@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> > > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
> > > > controller driver
> > > >
> > > > Hi,
> > > >
> > > > On Thu, 5 Nov 2020 15:09:11 +0100
> > > > Cédric Le Goater <clg@kaod.org> wrote:
> > > >  
> > > > > Hello Chin-Ting,
> > > > >
> > > > > Thanks for this driver. It's much cleaner than the previous and we
> > > > > should try adding support for the AST2500 SoC also. I guess we can
> > > > > keep the old driver for the AST2400 which has a different register layout.
> > > > >
> > > > > On the patchset, I think we should split this patch in three :
> > > > >
> > > > >  - basic support
> > > > >  - AHB window calculation depending on the flash size
> > > > >  - read training support  
> > > >
> > > > I didn't look closely at the implementation, but if the read
> > > > training tries to read a section of the NOR, I'd recommend exposing
> > > > that feature through spi-mem and letting the SPI-NOR framework
> > > > trigger the training instead of doing that at dirmap creation time
> > > > (remember that spi-mem is also used for SPI NANDs which use the dirmap  
> > API too, and this training is unlikely to work there).  
> > > >
> > > > The SPI-NOR framework could pass a read op template and a reference
> > > > pattern such that all the spi-mem driver has to do is execute the
> > > > template op and compare the output to the reference buffer.
> > > >  
> > >
> > > I agree it. Before, I were not able to find a suitable location to implement  
> > read training feature.  
> > > I think that I can add a SPI timing training function in
> > > "spi_controller_mem_ops" struct and call it by a wrapper function called at  
> > the bottom of spi_nor_probe() in spi-nor.c.  
> > > Maybe, SPI-NOR framework does not need to pass reference buffer since
> > > calibration method depends on each SoC itself and buffer size may be  
> > variant.  
> > > The detail calibration method may be implemented in each SoC SPI driver.  
> > 
> > That's a real problem IMO. What makes this pattern SoC specific? I can see
> > why the location in flash could be *board* specific, but the pattern should be
> > pretty common, right? As for the spi-mem operation to be executed, it's
> > definitely memory specific (I can imagine some flash vendors providing a
> > specific command returning a fixed pattern that's not actually stored on a
> > visible portion of the flash).  
> 
> You are right, the pattern should be pretty common. The thing I was worried about is the size of
> that buffer since, maybe, some controllers need to read more data than others in order to get good
> training result.

It would be good to see how other controllers implement that. I know
that the Cadence controller had something similar. Vignesh might be
able to share his thoughts on this.

> 
> > >
> > > Besides, I am thinking about the possibility for adding a
> > > "spi_mem_post_init" function in spi-mem framework sine for some SoCs,
> > > SPI controller needs to adjust some settings after getting SPI flash  
> > information.
> > 
> > I don't think that's a good idea. The spi-mem interface should stay
> > memory-type agnostic and doing that means we somehow pass NOR specific
> > info. What is it that you need exactly, and why?  
> 
> Yes, as you mention, the spi-mem interface should stay memory-type agnostic. Thus, currently, I just think about this, not implementation.
> 
> Why did I need this exactly?
> Take ASPEED SPI controller for example, ASPEED SPI controller is designed for SPI NOR flash especially.
> When ASPEED SoC powers on or reset, MCU ROM will fetch SPI NOR flash through SPI controller.
> But, MCU ROM does not know the current address mode of SPI NOR flash when SoC was reset (SPI flash is not reset).
> Therefore, SPI flash driver needs to set related flag to notify MCU ROM when flash is set to 4B address mode and 4B read opcode is used.

Oh, that's ugly! The SPI NOR framework tries hard to not change the
addressing mode exactly for this reason. On most NORs there
should now be READ/WRITE variants allowing you to address more than 2^24
bytes without changing the addressing mode. This being said, those
problem exists on other platform which can't even let the boot ROM know
that addressing mode changed. I don't have a proper solution for your
use case, but I definitely don't like the idea of exposing such details
to spi-mem controllers...

We usually recommend to connect the NOR reset pin to the global reset
to addressing mode gets back to known state when you reboot the board
and need to go back to the boot ROM.

> 
> Besides, for other SoCs connected to ASPEED SoC, they can read/write SPI NOR flash connected to ASPEED SoC by a pure HW channel without any interaction of SW driver.
> But, before trigger this feature, flash read/write/erase opcode, dummy cycle and other information should be filled in the related registers in advance because that HW channel
> does not know accurate information about connected SPI NOR flash.

While I can see a valid reason to allow that for READs (if we decide to
support XIP), I really don't like the idea of allowing destructive
operations (WRITE/ERASE) on the flash that don't go through the MTD
layer. This sounds like risky business to me, so I'd just forget about
that if I were you. Regarding the XIP use case, why not, but we'll need
to extend the dirmap API to support it: mappings need to stay around
and you need to return a pointer to the mapped memory region, which we
don't allow right now (because we want to let controllers move their
dirmap window if they have to).

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-06 11:30             ` Boris Brezillon
@ 2020-11-06 18:27               ` Chin-Ting Kuo
  2020-11-11  5:44                 ` Vignesh Raghavendra
  0 siblings, 1 reply; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-06 18:27 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Cédric Le Goater, robh+dt, broonie, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi,
	BMC-SW, Vignesh R, Tudor Ambarus

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Sent: Friday, November 6, 2020 7:30 PM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> driver
> 
> +Tudor and Vignesh
> 
> On Fri, 6 Nov 2020 10:21:06 +0000
> Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
> 
> > Hi Boris,
> >
> > Thanks for your comments and suggestions.
> >
> > > -----Original Message-----
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Sent: Friday, November 6, 2020 5:06 PM
> > > To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
> > > controller driver
> > >
> > > On Fri, 6 Nov 2020 08:58:23 +0000
> > > Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
> > >
> > > > Hi Boris,
> > > >
> > > > Thanks for your quick reply.
> > > >
> > > > > -----Original Message-----
> > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Sent: Thursday, November 5, 2020 11:12 PM
> > > > > To: Cédric Le Goater <clg@kaod.org>; robh+dt@kernel.org
> > > > > Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>;
> > > > > broonie@kernel.org; joel@jms.id.au; andrew@aj.id.au;
> > > > > bbrezillon@kernel.org; devicetree@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; linux-aspeed@lists.ozlabs.org;
> > > > > linux-spi@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> > > > > Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
> > > > > controller driver
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Thu, 5 Nov 2020 15:09:11 +0100 Cédric Le Goater
> > > > > <clg@kaod.org> wrote:
> > > > >
> > > > > > Hello Chin-Ting,
> > > > > >
> > > > > > Thanks for this driver. It's much cleaner than the previous
> > > > > > and we should try adding support for the AST2500 SoC also. I
> > > > > > guess we can keep the old driver for the AST2400 which has a
> different register layout.
> > > > > >
> > > > > > On the patchset, I think we should split this patch in three :
> > > > > >
> > > > > >  - basic support
> > > > > >  - AHB window calculation depending on the flash size
> > > > > >  - read training support
> > > > >
> > > > > I didn't look closely at the implementation, but if the read
> > > > > training tries to read a section of the NOR, I'd recommend
> > > > > exposing that feature through spi-mem and letting the SPI-NOR
> > > > > framework trigger the training instead of doing that at dirmap
> > > > > creation time (remember that spi-mem is also used for SPI NANDs
> > > > > which use the dirmap
> > > API too, and this training is unlikely to work there).
> > > > >
> > > > > The SPI-NOR framework could pass a read op template and a
> > > > > reference pattern such that all the spi-mem driver has to do is
> > > > > execute the template op and compare the output to the reference
> buffer.
> > > > >
> > > >
> > > > I agree it. Before, I were not able to find a suitable location to
> > > > implement
> > > read training feature.
> > > > I think that I can add a SPI timing training function in
> > > > "spi_controller_mem_ops" struct and call it by a wrapper function
> > > > called at
> > > the bottom of spi_nor_probe() in spi-nor.c.
> > > > Maybe, SPI-NOR framework does not need to pass reference buffer
> > > > since calibration method depends on each SoC itself and buffer
> > > > size may be
> > > variant.
> > > > The detail calibration method may be implemented in each SoC SPI
> driver.
> > >
> > > That's a real problem IMO. What makes this pattern SoC specific? I
> > > can see why the location in flash could be *board* specific, but the
> > > pattern should be pretty common, right? As for the spi-mem operation
> > > to be executed, it's definitely memory specific (I can imagine some
> > > flash vendors providing a specific command returning a fixed pattern
> > > that's not actually stored on a visible portion of the flash).
> >
> > You are right, the pattern should be pretty common. The thing I was
> > worried about is the size of that buffer since, maybe, some
> > controllers need to read more data than others in order to get good training
> result.
> 
> It would be good to see how other controllers implement that. I know that the
> Cadence controller had something similar. Vignesh might be able to share his
> thoughts on this.

Oh, maybe, I misunderstood your meaning and I did not describe clearly early.
As you mentioned before, for some SPI-NOR flashes, there indeed exists a command used for
read timing training with high SPI clock frequency.
When this specific command is sent, an almost common pattern with fixed length will be outputted to controller.
(This pattern is not stored on a normal read/write area.)

But, unfortunately, many flash parts we used did not support this feature. Thus, our read timing training strategy is:
Step 1: Use the lowest SPI clock frequency to read normal flash content with specific length as reference data.
Step 2: With a fixed high SPI clock frequency, adjust different timing delay cycle, then, read the same flash region for each timing delay.
Step 3: Compare each data read from step 2 to the reference data gotten from step 1. Then, we will get a suitable timing delay window.

> 
> >
> > > >
> > > > Besides, I am thinking about the possibility for adding a
> > > > "spi_mem_post_init" function in spi-mem framework sine for some
> > > > SoCs, SPI controller needs to adjust some settings after getting
> > > > SPI flash
> > > information.
> > >
> > > I don't think that's a good idea. The spi-mem interface should stay
> > > memory-type agnostic and doing that means we somehow pass NOR
> > > specific info. What is it that you need exactly, and why?
> >
> > Yes, as you mention, the spi-mem interface should stay memory-type
> agnostic. Thus, currently, I just think about this, not implementation.
> >
> > Why did I need this exactly?
> > Take ASPEED SPI controller for example, ASPEED SPI controller is designed
> for SPI NOR flash especially.
> > When ASPEED SoC powers on or reset, MCU ROM will fetch SPI NOR flash
> through SPI controller.
> > But, MCU ROM does not know the current address mode of SPI NOR flash
> when SoC was reset (SPI flash is not reset).
> > Therefore, SPI flash driver needs to set related flag to notify MCU ROM when
> flash is set to 4B address mode and 4B read opcode is used.
> 
> Oh, that's ugly! The SPI NOR framework tries hard to not change the
> addressing mode exactly for this reason. On most NORs there should now be
> READ/WRITE variants allowing you to address more than 2^24 bytes without
> changing the addressing mode. This being said, those problem exists on other
> platform which can't even let the boot ROM know that addressing mode
> changed. I don't have a proper solution for your use case, but I definitely don't
> like the idea of exposing such details to spi-mem controllers...
> 

Certainly, most of new SPI NOR flashes larger than 16MB support dedicated
4B command without change flash address mode. Originally, I want to take
all flashes into consideration. But, now, the number of flashes, larger than 16MB and
without 4B dedicated command, decreases. Perhaps, I can ignore them currently.

> We usually recommend to connect the NOR reset pin to the global reset to
> addressing mode gets back to known state when you reboot the board and
> need to go back to the boot ROM.

I agree with this.

> 
> >
> > Besides, for other SoCs connected to ASPEED SoC, they can read/write SPI
> NOR flash connected to ASPEED SoC by a pure HW channel without any
> interaction of SW driver.
> > But, before trigger this feature, flash read/write/erase opcode, dummy
> > cycle and other information should be filled in the related registers in
> advance because that HW channel does not know accurate information about
> connected SPI NOR flash.
> 
> While I can see a valid reason to allow that for READs (if we decide to support
> XIP), I really don't like the idea of allowing destructive operations
> (WRITE/ERASE) on the flash that don't go through the MTD layer. This sounds
> like risky business to me, so I'd just forget about that if I were you. Regarding
> the XIP use case, why not, but we'll need to extend the dirmap API to support it:
> mappings need to stay around and you need to return a pointer to the mapped
> memory region, which we don't allow right now (because we want to let
> controllers move their dirmap window if they have to).

Yes, for SPI(-flash) driver, I think I just needs to focus on the scenario where all flash operations go through MTD layer.
Other application may be implemented on the other driver, not here.


Best Wishes,
Chin-Ting



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-06 18:27               ` Chin-Ting Kuo
@ 2020-11-11  5:44                 ` Vignesh Raghavendra
  2020-11-13  7:30                   ` Chin-Ting Kuo
  0 siblings, 1 reply; 20+ messages in thread
From: Vignesh Raghavendra @ 2020-11-11  5:44 UTC (permalink / raw)
  To: Chin-Ting Kuo, Boris Brezillon
  Cc: Cédric Le Goater, robh+dt, broonie, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi,
	BMC-SW, Tudor Ambarus

Hi Chin-Ting,

On 11/6/20 11:57 PM, Chin-Ting Kuo wrote:
> Hi Boris,
> 
>> -----Original Message-----
>> From: Boris Brezillon <boris.brezillon@collabora.com>
>> Sent: Friday, November 6, 2020 7:30 PM
>> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
>> driver
>>
>> +Tudor and Vignesh
>>
>> On Fri, 6 Nov 2020 10:21:06 +0000
>> Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
>>
>>> Hi Boris,
>>>
>>> Thanks for your comments and suggestions.
>>>
>>>> -----Original Message-----
>>>> From: Boris Brezillon <boris.brezillon@collabora.com>
>>>> Sent: Friday, November 6, 2020 5:06 PM
>>>> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
>>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
>>>> controller driver
>>>>
>>>> On Fri, 6 Nov 2020 08:58:23 +0000
>>>> Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
>>>>
>>>>> Hi Boris,
>>>>>
>>>>> Thanks for your quick reply.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Boris Brezillon <boris.brezillon@collabora.com>
>>>>>> Sent: Thursday, November 5, 2020 11:12 PM
>>>>>> To: Cédric Le Goater <clg@kaod.org>; robh+dt@kernel.org
>>>>>> Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>;
>>>>>> broonie@kernel.org; joel@jms.id.au; andrew@aj.id.au;
>>>>>> bbrezillon@kernel.org; devicetree@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-aspeed@lists.ozlabs.org;
>>>>>> linux-spi@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
>>>>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
>>>>>> controller driver
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Thu, 5 Nov 2020 15:09:11 +0100 Cédric Le Goater
>>>>>> <clg@kaod.org> wrote:
>>>>>>
>>>>>>> Hello Chin-Ting,
>>>>>>>
>>>>>>> Thanks for this driver. It's much cleaner than the previous
>>>>>>> and we should try adding support for the AST2500 SoC also. I
>>>>>>> guess we can keep the old driver for the AST2400 which has a
>> different register layout.
>>>>>>>
>>>>>>> On the patchset, I think we should split this patch in three :
>>>>>>>
>>>>>>>  - basic support
>>>>>>>  - AHB window calculation depending on the flash size
>>>>>>>  - read training support
>>>>>>
>>>>>> I didn't look closely at the implementation, but if the read
>>>>>> training tries to read a section of the NOR, I'd recommend
>>>>>> exposing that feature through spi-mem and letting the SPI-NOR
>>>>>> framework trigger the training instead of doing that at dirmap
>>>>>> creation time (remember that spi-mem is also used for SPI NANDs
>>>>>> which use the dirmap
>>>> API too, and this training is unlikely to work there).
>>>>>>
>>>>>> The SPI-NOR framework could pass a read op template and a
>>>>>> reference pattern such that all the spi-mem driver has to do is
>>>>>> execute the template op and compare the output to the reference
>> buffer.
>>>>>>
>>>>>
>>>>> I agree it. Before, I were not able to find a suitable location to
>>>>> implement
>>>> read training feature.
>>>>> I think that I can add a SPI timing training function in
>>>>> "spi_controller_mem_ops" struct and call it by a wrapper function
>>>>> called at
>>>> the bottom of spi_nor_probe() in spi-nor.c.
>>>>> Maybe, SPI-NOR framework does not need to pass reference buffer
>>>>> since calibration method depends on each SoC itself and buffer
>>>>> size may be
>>>> variant.
>>>>> The detail calibration method may be implemented in each SoC SPI
>> driver.
>>>>
>>>> That's a real problem IMO. What makes this pattern SoC specific? I
>>>> can see why the location in flash could be *board* specific, but the
>>>> pattern should be pretty common, right? As for the spi-mem operation
>>>> to be executed, it's definitely memory specific (I can imagine some
>>>> flash vendors providing a specific command returning a fixed pattern
>>>> that's not actually stored on a visible portion of the flash).
>>>
>>> You are right, the pattern should be pretty common. The thing I was
>>> worried about is the size of that buffer since, maybe, some
>>> controllers need to read more data than others in order to get good training
>> result.
>>
>> It would be good to see how other controllers implement that. I know that the
>> Cadence controller had something similar. Vignesh might be able to share his
>> thoughts on this.
> 

Cadence controllers requires to read fixed length calibration pattern
multiple times (while tuning PHY registers) from non zero address
location. Pattern is Flash independent and SoC independent. Hence, can
be hard coded in driver (no need to read at slower speed).

> Oh, maybe, I misunderstood your meaning and I did not describe clearly early.
> As you mentioned before, for some SPI-NOR flashes, there indeed exists a command used for
> read timing training with high SPI clock frequency.
> When this specific command is sent, an almost common pattern with fixed length will be outputted to controller.
> (This pattern is not stored on a normal read/write area.)
> 
> But, unfortunately, many flash parts we used did not support this feature. Thus, our read timing training strategy is:
> Step 1: Use the lowest SPI clock frequency to read normal flash content with specific length as reference data.
> Step 2: With a fixed high SPI clock frequency, adjust different timing delay cycle, then, read the same flash region for each timing delay.
> Step 3: Compare each data read from step 2 to the reference data gotten from step 1. Then, we will get a suitable timing delay window.
> 

Using dirmap_create() to actually calibrate controller is abusing the
interface IMO.  It is not guaranteed that flash is configured to use
right number of dummy cycles values and mode for high speed operation
before call to dirmap_create(). This is true today but may change in the
future. So, there should at least be a separate callback dedicated for
calibration along the lines Boris suggested.

Max frequency that read cmd may support would not be supported by other
cmds such as write or read SFDP. This would need to be taken into
account before and post calibration probably by extending spi_mem_op to
specify freq of operation per op.

I see that calibration pattern is assumed to be at offset 0 in the
flash. This may not be the ideal position as offset 0 is typically used
to store bootloader. So, there should be a way to specify offset at
which calibration pattern is present.

Regards
Vignesh


>>
>>>
>>>>>
>>>>> Besides, I am thinking about the possibility for adding a
>>>>> "spi_mem_post_init" function in spi-mem framework sine for some
>>>>> SoCs, SPI controller needs to adjust some settings after getting
>>>>> SPI flash
>>>> information.
>>>>
>>>> I don't think that's a good idea. The spi-mem interface should stay
>>>> memory-type agnostic and doing that means we somehow pass NOR
>>>> specific info. What is it that you need exactly, and why?
>>>
>>> Yes, as you mention, the spi-mem interface should stay memory-type
>> agnostic. Thus, currently, I just think about this, not implementation.
>>>
>>> Why did I need this exactly?
>>> Take ASPEED SPI controller for example, ASPEED SPI controller is designed
>> for SPI NOR flash especially.
>>> When ASPEED SoC powers on or reset, MCU ROM will fetch SPI NOR flash
>> through SPI controller.
>>> But, MCU ROM does not know the current address mode of SPI NOR flash
>> when SoC was reset (SPI flash is not reset).
>>> Therefore, SPI flash driver needs to set related flag to notify MCU ROM when
>> flash is set to 4B address mode and 4B read opcode is used.
>>
>> Oh, that's ugly! The SPI NOR framework tries hard to not change the
>> addressing mode exactly for this reason. On most NORs there should now be
>> READ/WRITE variants allowing you to address more than 2^24 bytes without
>> changing the addressing mode. This being said, those problem exists on other
>> platform which can't even let the boot ROM know that addressing mode
>> changed. I don't have a proper solution for your use case, but I definitely don't
>> like the idea of exposing such details to spi-mem controllers...
>>
> 
> Certainly, most of new SPI NOR flashes larger than 16MB support dedicated
> 4B command without change flash address mode. Originally, I want to take
> all flashes into consideration. But, now, the number of flashes, larger than 16MB and
> without 4B dedicated command, decreases. Perhaps, I can ignore them currently.
> 
>> We usually recommend to connect the NOR reset pin to the global reset to
>> addressing mode gets back to known state when you reboot the board and
>> need to go back to the boot ROM.
> 
> I agree with this.
> 
>>
>>>
>>> Besides, for other SoCs connected to ASPEED SoC, they can read/write SPI
>> NOR flash connected to ASPEED SoC by a pure HW channel without any
>> interaction of SW driver.
>>> But, before trigger this feature, flash read/write/erase opcode, dummy
>>> cycle and other information should be filled in the related registers in
>> advance because that HW channel does not know accurate information about
>> connected SPI NOR flash.
>>
>> While I can see a valid reason to allow that for READs (if we decide to support
>> XIP), I really don't like the idea of allowing destructive operations
>> (WRITE/ERASE) on the flash that don't go through the MTD layer. This sounds
>> like risky business to me, so I'd just forget about that if I were you. Regarding
>> the XIP use case, why not, but we'll need to extend the dirmap API to support it:
>> mappings need to stay around and you need to return a pointer to the mapped
>> memory region, which we don't allow right now (because we want to let
>> controllers move their dirmap window if they have to).
> 
> Yes, for SPI(-flash) driver, I think I just needs to focus on the scenario where all flash operations go through MTD layer.
> Other application may be implemented on the other driver, not here.
> 
> 
> Best Wishes,
> Chin-Ting
> 
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
  2020-11-11  5:44                 ` Vignesh Raghavendra
@ 2020-11-13  7:30                   ` Chin-Ting Kuo
  0 siblings, 0 replies; 20+ messages in thread
From: Chin-Ting Kuo @ 2020-11-13  7:30 UTC (permalink / raw)
  To: Vignesh Raghavendra, Boris Brezillon
  Cc: Cédric Le Goater, robh+dt, broonie, joel, andrew,
	bbrezillon, devicetree, linux-kernel, linux-aspeed, linux-spi,
	BMC-SW, Tudor Ambarus

Hi Vignesh,

Thanks for your information.

> -----Original Message-----
> From: Vignesh Raghavendra <vigneshr@ti.com>
> Sent: Wednesday, November 11, 2020 1:44 PM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>; Boris Brezillon
> <boris.brezillon@collabora.com>
> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller
> driver
> 
> Hi Chin-Ting,
> 
> On 11/6/20 11:57 PM, Chin-Ting Kuo wrote:
> > Hi Boris,
> >
> >> -----Original Message-----
> >> From: Boris Brezillon <boris.brezillon@collabora.com>
> >> Sent: Friday, November 6, 2020 7:30 PM
> >> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> >> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
> >> controller driver
> >>
> >> +Tudor and Vignesh
> >>
> >> On Fri, 6 Nov 2020 10:21:06 +0000
> >> Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
> >>
> >>> Hi Boris,
> >>>
> >>> Thanks for your comments and suggestions.
> >>>
> >>>> -----Original Message-----
> >>>> From: Boris Brezillon <boris.brezillon@collabora.com>
> >>>> Sent: Friday, November 6, 2020 5:06 PM
> >>>> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> >>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
> >>>> controller driver
> >>>>
> >>>> On Fri, 6 Nov 2020 08:58:23 +0000
> >>>> Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> wrote:
> >>>>
> >>>>> Hi Boris,
> >>>>>
> >>>>> Thanks for your quick reply.
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Boris Brezillon <boris.brezillon@collabora.com>
> >>>>>> Sent: Thursday, November 5, 2020 11:12 PM
> >>>>>> To: Cédric Le Goater <clg@kaod.org>; robh+dt@kernel.org
> >>>>>> Cc: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>;
> >>>>>> broonie@kernel.org; joel@jms.id.au; andrew@aj.id.au;
> >>>>>> bbrezillon@kernel.org; devicetree@vger.kernel.org;
> >>>>>> linux-kernel@vger.kernel.org; linux-aspeed@lists.ozlabs.org;
> >>>>>> linux-spi@vger.kernel.org; BMC-SW <BMC-SW@aspeedtech.com>
> >>>>>> Subject: Re: [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory
> >>>>>> controller driver
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> On Thu, 5 Nov 2020 15:09:11 +0100 Cédric Le Goater <clg@kaod.org>
> >>>>>> wrote:
> >>>>>>
> >>>>>>> Hello Chin-Ting,
> >>>>>>>
> >>>>>>> Thanks for this driver. It's much cleaner than the previous and
> >>>>>>> we should try adding support for the AST2500 SoC also. I guess
> >>>>>>> we can keep the old driver for the AST2400 which has a
> >> different register layout.
> >>>>>>>
> >>>>>>> On the patchset, I think we should split this patch in three :
> >>>>>>>
> >>>>>>>  - basic support
> >>>>>>>  - AHB window calculation depending on the flash size
> >>>>>>>  - read training support
> >>>>>>
> >>>>>> I didn't look closely at the implementation, but if the read
> >>>>>> training tries to read a section of the NOR, I'd recommend
> >>>>>> exposing that feature through spi-mem and letting the SPI-NOR
> >>>>>> framework trigger the training instead of doing that at dirmap
> >>>>>> creation time (remember that spi-mem is also used for SPI NANDs
> >>>>>> which use the dirmap
> >>>> API too, and this training is unlikely to work there).
> >>>>>>
> >>>>>> The SPI-NOR framework could pass a read op template and a
> >>>>>> reference pattern such that all the spi-mem driver has to do is
> >>>>>> execute the template op and compare the output to the reference
> >> buffer.
> >>>>>>
> >>>>>
> >>>>> I agree it. Before, I were not able to find a suitable location to
> >>>>> implement
> >>>> read training feature.
> >>>>> I think that I can add a SPI timing training function in
> >>>>> "spi_controller_mem_ops" struct and call it by a wrapper function
> >>>>> called at
> >>>> the bottom of spi_nor_probe() in spi-nor.c.
> >>>>> Maybe, SPI-NOR framework does not need to pass reference buffer
> >>>>> since calibration method depends on each SoC itself and buffer
> >>>>> size may be
> >>>> variant.
> >>>>> The detail calibration method may be implemented in each SoC SPI
> >> driver.
> >>>>
> >>>> That's a real problem IMO. What makes this pattern SoC specific? I
> >>>> can see why the location in flash could be *board* specific, but
> >>>> the pattern should be pretty common, right? As for the spi-mem
> >>>> operation to be executed, it's definitely memory specific (I can
> >>>> imagine some flash vendors providing a specific command returning a
> >>>> fixed pattern that's not actually stored on a visible portion of the flash).
> >>>
> >>> You are right, the pattern should be pretty common. The thing I was
> >>> worried about is the size of that buffer since, maybe, some
> >>> controllers need to read more data than others in order to get good
> >>> training
> >> result.
> >>
> >> It would be good to see how other controllers implement that. I know
> >> that the Cadence controller had something similar. Vignesh might be
> >> able to share his thoughts on this.
> >
> 
> Cadence controllers requires to read fixed length calibration pattern multiple
> times (while tuning PHY registers) from non zero address location. Pattern is
> Flash independent and SoC independent. Hence, can be hard coded in driver
> (no need to read at slower speed).
> 

This method is more suitable.

> > Oh, maybe, I misunderstood your meaning and I did not describe clearly
> early.
> > As you mentioned before, for some SPI-NOR flashes, there indeed exists
> > a command used for read timing training with high SPI clock frequency.
> > When this specific command is sent, an almost common pattern with fixed
> length will be outputted to controller.
> > (This pattern is not stored on a normal read/write area.)
> >
> > But, unfortunately, many flash parts we used did not support this feature.
> Thus, our read timing training strategy is:
> > Step 1: Use the lowest SPI clock frequency to read normal flash content with
> specific length as reference data.
> > Step 2: With a fixed high SPI clock frequency, adjust different timing delay
> cycle, then, read the same flash region for each timing delay.
> > Step 3: Compare each data read from step 2 to the reference data gotten
> from step 1. Then, we will get a suitable timing delay window.
> >
> 
> Using dirmap_create() to actually calibrate controller is abusing the interface
> IMO.  It is not guaranteed that flash is configured to use right number of
> dummy cycles values and mode for high speed operation before call to
> dirmap_create(). This is true today but may change in the future. So, there
> should at least be a separate callback dedicated for calibration along the lines
> Boris suggested.
> 

Yes, it is inappropriate to locate calibrate part in dirmap_create(). I did this because there was no good place at that time.
This problem will be mitigated by using Boris suggestion.

> Max frequency that read cmd may support would not be supported by other
> cmds such as write or read SFDP. This would need to be taken into account
> before and post calibration probably by extending spi_mem_op to specify freq
> of operation per op.
> 

Yes, not all commands can support the same frequency (higher frequency).
But, it is impossible to record workable frequency for each opcode, different parts or for different flash vendors.
Maybe, only read/program commands need higher frequency for system performance improvement purpose. Other commands can use default frequency.
After flash is probed (spi_nor_scan), read/write opcode and dummy cycles are decided, we can use this information for read training for specific read opcode with its dummy cycles.
Thanks for your remainder.

Best Wishes,
Chin-Ting

> I see that calibration pattern is assumed to be at offset 0 in the flash. This may
> not be the ideal position as offset 0 is typically used to store bootloader. So,
> there should be a way to specify offset at which calibration pattern is present.
> 
> Regards
> Vignesh
> 
> 
> >>
> >>>
> >>>>>
> >>>>> Besides, I am thinking about the possibility for adding a
> >>>>> "spi_mem_post_init" function in spi-mem framework sine for some
> >>>>> SoCs, SPI controller needs to adjust some settings after getting
> >>>>> SPI flash
> >>>> information.
> >>>>
> >>>> I don't think that's a good idea. The spi-mem interface should stay
> >>>> memory-type agnostic and doing that means we somehow pass NOR
> >>>> specific info. What is it that you need exactly, and why?
> >>>
> >>> Yes, as you mention, the spi-mem interface should stay memory-type
> >> agnostic. Thus, currently, I just think about this, not implementation.
> >>>
> >>> Why did I need this exactly?
> >>> Take ASPEED SPI controller for example, ASPEED SPI controller is
> >>> designed
> >> for SPI NOR flash especially.
> >>> When ASPEED SoC powers on or reset, MCU ROM will fetch SPI NOR flash
> >> through SPI controller.
> >>> But, MCU ROM does not know the current address mode of SPI NOR flash
> >> when SoC was reset (SPI flash is not reset).
> >>> Therefore, SPI flash driver needs to set related flag to notify MCU
> >>> ROM when
> >> flash is set to 4B address mode and 4B read opcode is used.
> >>
> >> Oh, that's ugly! The SPI NOR framework tries hard to not change the
> >> addressing mode exactly for this reason. On most NORs there should
> >> now be READ/WRITE variants allowing you to address more than 2^24
> >> bytes without changing the addressing mode. This being said, those
> >> problem exists on other platform which can't even let the boot ROM
> >> know that addressing mode changed. I don't have a proper solution for
> >> your use case, but I definitely don't like the idea of exposing such details to
> spi-mem controllers...
> >>
> >
> > Certainly, most of new SPI NOR flashes larger than 16MB support
> > dedicated 4B command without change flash address mode. Originally, I
> > want to take all flashes into consideration. But, now, the number of
> > flashes, larger than 16MB and without 4B dedicated command, decreases.
> Perhaps, I can ignore them currently.
> >
> >> We usually recommend to connect the NOR reset pin to the global reset
> >> to addressing mode gets back to known state when you reboot the board
> >> and need to go back to the boot ROM.
> >
> > I agree with this.
> >
> >>
> >>>
> >>> Besides, for other SoCs connected to ASPEED SoC, they can read/write
> >>> SPI
> >> NOR flash connected to ASPEED SoC by a pure HW channel without any
> >> interaction of SW driver.
> >>> But, before trigger this feature, flash read/write/erase opcode,
> >>> dummy cycle and other information should be filled in the related
> >>> registers in
> >> advance because that HW channel does not know accurate information
> >> about connected SPI NOR flash.
> >>
> >> While I can see a valid reason to allow that for READs (if we decide
> >> to support XIP), I really don't like the idea of allowing destructive
> >> operations
> >> (WRITE/ERASE) on the flash that don't go through the MTD layer. This
> >> sounds like risky business to me, so I'd just forget about that if I
> >> were you. Regarding the XIP use case, why not, but we'll need to extend the
> dirmap API to support it:
> >> mappings need to stay around and you need to return a pointer to the
> >> mapped memory region, which we don't allow right now (because we want
> >> to let controllers move their dirmap window if they have to).
> >
> > Yes, for SPI(-flash) driver, I think I just needs to focus on the scenario where
> all flash operations go through MTD layer.
> > Other application may be implemented on the other driver, not here.
> >
> >
> > Best Wishes,
> > Chin-Ting
> >
> >

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v3 0/4] Porting ASPEED FMC/SPI memory controller driver
  2020-11-05 12:03 [v3 0/4] Porting ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
                   ` (3 preceding siblings ...)
  2020-11-05 12:03 ` [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
@ 2020-12-01 13:57 ` Mark Brown
  4 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2020-12-01 13:57 UTC (permalink / raw)
  To: bbrezillon, Chin-Ting Kuo, robh+dt, joel, linux-spi, devicetree,
	clg, linux-kernel, andrew, linux-aspeed
  Cc: BMC-SW

On Thu, 5 Nov 2020 20:03:27 +0800, Chin-Ting Kuo wrote:
> This patch series aims to porting ASPEED FMC/SPI memory controller
> driver with spi-mem interface. Adjust device tree setting of SPI NOR
> flash in order to fit real AST2600 EVB and new SPI memory controller
> driver. Also, this patch has been verified on AST2600-A1 EVB.
> 
> v2: Fix sparse warnings reported by kernel test robot <lkp@intel.com>.
> v3: Fix build warnings with x86 allmodconfig.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller
      (no commit info)
[2/2] spi: aspeed: Add ASPEED FMC/SPI memory controller driver
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-12-01 13:59 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 12:03 [v3 0/4] Porting ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
2020-11-05 12:03 ` [v3 1/4] dt-bindings: spi: Add binding file for ASPEED FMC/SPI memory controller Chin-Ting Kuo
2020-11-05 22:39   ` Rob Herring
2020-11-06  9:11     ` Chin-Ting Kuo
2020-11-05 12:03 ` [v3 2/4] ARM: dts: aspeed: ast2600: Update FMC/SPI controller setting for spi-aspeed.c Chin-Ting Kuo
2020-11-05 12:03 ` [v3 3/4] ARM: dts: aspeed: ast2600-evb: Adjust SPI flash configuration Chin-Ting Kuo
2020-11-05 12:03 ` [v3 4/4] spi: aspeed: Add ASPEED FMC/SPI memory controller driver Chin-Ting Kuo
2020-11-05 14:09   ` Cédric Le Goater
2020-11-05 15:11     ` Boris Brezillon
2020-11-05 16:43       ` Mark Brown
2020-11-06  9:01         ` Chin-Ting Kuo
2020-11-06  8:58       ` Chin-Ting Kuo
2020-11-06  9:05         ` Boris Brezillon
2020-11-06 10:21           ` Chin-Ting Kuo
2020-11-06 11:30             ` Boris Brezillon
2020-11-06 18:27               ` Chin-Ting Kuo
2020-11-11  5:44                 ` Vignesh Raghavendra
2020-11-13  7:30                   ` Chin-Ting Kuo
2020-11-06  7:38     ` Chin-Ting Kuo
2020-12-01 13:57 ` [v3 0/4] Porting " Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.