[RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Add hisilicon spi-nor flash controller driver

Signed-off-by: Binquan Peng <pengbinquan@huawei.com>
Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Reviewed-by: Jagan Teki <jteki@openedev.com>
---
change log
v9:
Fixed issues pointed by Jagan Teki.
v8:
Fixed issues pointed by Ezequiel Garcia and Brian Norris.
Moved dts binding file to mtd directory.
Changed the compatible string more specific.
v7:
Rebased to v4.5-rc3.
Fixed issues pointed by Ezequiel Garcia.
v6:
Based on v4.5-rc2
Fixed issues pointed by Ezequiel Garcia.
v5:
Fixed a compile error.
v4:
Rebased to v4.5-rc1
v3:
Added a compatible string "hisilicon,hi3519-sfc".
v2:
Fixed some compiling warings.

 .../bindings/mtd/hisilicon,fmc-spi-nor.txt         |  24 +
 drivers/mtd/spi-nor/Kconfig                        |   7 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/hisi-sfc.c                     | 502 +++++++++++++++++++++
 4 files changed, 534 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt
 create mode 100644 drivers/mtd/spi-nor/hisi-sfc.c

diff --git a/Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt b/Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt
new file mode 100644
index 0000000..7498152
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt
@@ -0,0 +1,24 @@
+HiSilicon SPI-NOR Flash Controller
+
+Required properties:
+- compatible : Should be "hisilicon,fmc-spi-nor" and one of the following strings:
+		"hisilicon,hi3519-spi-nor"
+- address-cells : Should be 1.
+- size-cells : Should be 0.
+- reg : Offset and length of the register set for the controller device.
+- reg-names : Must include the following two entries: "control", "memory".
+- clocks : handle to spi-nor flash controller clock.
+
+Example:
+spi-nor-controller@10000000 {
+	compatible = "hisilicon,hi3519-spi-nor", "hisilicon,fmc-spi-nor";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x10000000 0x1000>, <0x14000000 0x1000000>;
+	reg-names = "control", "memory";
+	clocks = <&clock HI3519_FMC_CLK>;
+	spi-nor@0 {
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+	};
+};
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 0dc9275..120624d 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -37,6 +37,13 @@ config SPI_FSL_QUADSPI
 	  This controller does not support generic SPI. It only supports
 	  SPI NOR.
 
+config SPI_HISI_SFC
+	tristate "Hisilicon SPI-NOR Flash Controller(SFC)"
+	depends on ARCH_HISI || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This enables support for hisilicon SPI-NOR flash controller.
+
 config SPI_NXP_SPIFI
 	tristate "NXP SPI Flash Interface (SPIFI)"
 	depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 0bf3a7f8..8a6fa69 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
+obj-$(CONFIG_SPI_HISI_SFC)	+= hisi-sfc.o
 obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
 obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
new file mode 100644
index 0000000..e974c92
--- /dev/null
+++ b/drivers/mtd/spi-nor/hisi-sfc.c
@@ -0,0 +1,502 @@
+/*
+ * HiSilicon SPI Nor Flash Controller Driver
+ *
+ * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spi-nor.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+/* Hardware register offsets and field definitions */
+#define FMC_CFG				0x00
+#define SPI_NOR_ADDR_MODE		BIT(10)
+#define FMC_GLOBAL_CFG			0x04
+#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
+#define FMC_SPI_TIMING_CFG		0x08
+#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
+#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
+#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
+#define CS_HOLD_TIME			0x6
+#define CS_SETUP_TIME			0x6
+#define CS_DESELECT_TIME		0xf
+#define FMC_INT				0x18
+#define FMC_INT_OP_DONE			BIT(0)
+#define FMC_INT_CLR			0x20
+#define FMC_CMD				0x24
+#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
+#define FMC_ADDRL			0x2c
+#define FMC_OP_CFG			0x30
+#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
+#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
+#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
+#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
+#define FMC_DATA_NUM			0x38
+#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
+#define FMC_OP				0x3c
+#define FMC_OP_DUMMY_EN			BIT(8)
+#define FMC_OP_CMD1_EN			BIT(7)
+#define FMC_OP_ADDR_EN			BIT(6)
+#define FMC_OP_WRITE_DATA_EN		BIT(5)
+#define FMC_OP_READ_DATA_EN		BIT(2)
+#define FMC_OP_READ_STATUS_EN		BIT(1)
+#define FMC_OP_REG_OP_START		BIT(0)
+#define FMC_DMA_LEN			0x40
+#define FMC_DMA_LEN_SET(_len)		((_len) & 0x0fffffff)
+#define FMC_DMA_SADDR_D0		0x4c
+#define HIFMC_DMA_MAX_LEN		(4096)
+#define HIFMC_DMA_MASK			(HIFMC_DMA_MAX_LEN - 1)
+#define FMC_OP_DMA			0x68
+#define OP_CTRL_RD_OPCODE(_code)	(((_code) & 0xff) << 16)
+#define OP_CTRL_WR_OPCODE(_code)	(((_code) & 0xff) << 8)
+#define OP_CTRL_RW_OP(_op)		((_op) << 1)
+#define OP_CTRL_DMA_OP_READY		BIT(0)
+#define FMC_OP_READ			0x0
+#define FMC_OP_WRITE			0x1
+#define FMC_WAIT_TIMEOUT		1000000
+
+enum hifmc_iftype {
+	IF_TYPE_STD,
+	IF_TYPE_DUAL,
+	IF_TYPE_DIO,
+	IF_TYPE_QUAD,
+	IF_TYPE_QIO,
+};
+
+struct hifmc_priv {
+	int chipselect;
+	u32 clkrate;
+	struct hifmc_host *host;
+};
+
+#define HIFMC_MAX_CHIP_NUM		2
+struct hifmc_host {
+	struct device *dev;
+	struct mutex lock;
+
+	void __iomem *regbase;
+	void __iomem *iobase;
+	struct clk *clk;
+	void *buffer;
+	dma_addr_t dma_buffer;
+
+	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
+	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
+	int num_chip;
+};
+
+static inline int wait_op_finish(struct hifmc_host *host)
+{
+	unsigned int reg;
+
+	return readl_poll_timeout(host->regbase + FMC_INT, reg,
+		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
+}
+
+static int get_if_type(enum read_mode flash_read)
+{
+	enum hifmc_iftype if_type;
+
+	switch (flash_read) {
+	case SPI_NOR_DUAL:
+		if_type = IF_TYPE_DUAL;
+		break;
+	case SPI_NOR_QUAD:
+		if_type = IF_TYPE_QUAD;
+		break;
+	case SPI_NOR_NORMAL:
+	case SPI_NOR_FAST:
+	default:
+		if_type = IF_TYPE_STD;
+		break;
+	}
+
+	return if_type;
+}
+
+static void hisi_spi_nor_init(struct hifmc_host *host)
+{
+	unsigned int reg;
+
+	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
+		| TIMING_CFG_TCSS(CS_SETUP_TIME)
+		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
+	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
+}
+
+static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	int ret;
+
+	mutex_lock(&host->lock);
+
+	ret = clk_set_rate(host->clk, priv->clkrate);
+	if (ret)
+		goto out;
+
+	ret = clk_prepare_enable(host->clk);
+	if (ret)
+		goto out;
+
+	return 0;
+
+out:
+	mutex_unlock(&host->lock);
+	return ret;
+}
+
+static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+
+	clk_disable_unprepare(host->clk);
+	mutex_unlock(&host->lock);
+}
+
+static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
+		u32 *opcfg)
+{
+	u32 reg;
+
+	*opcfg |= FMC_OP_CMD1_EN;
+	switch (cmd) {
+	case SPINOR_OP_RDID:
+	case SPINOR_OP_RDSR:
+	case SPINOR_OP_RDCR:
+		*opcfg |= FMC_OP_READ_DATA_EN;
+		break;
+	case SPINOR_OP_WREN:
+		reg = readl(host->regbase + FMC_GLOBAL_CFG);
+		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
+			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
+			writel(reg, host->regbase + FMC_GLOBAL_CFG);
+		}
+		break;
+	case SPINOR_OP_WRSR:
+		*opcfg |= FMC_OP_WRITE_DATA_EN;
+		break;
+	case SPINOR_OP_BE_4K:
+	case SPINOR_OP_BE_4K_PMC:
+	case SPINOR_OP_SE_4B:
+	case SPINOR_OP_SE:
+		*opcfg |= FMC_OP_ADDR_EN;
+		break;
+	case SPINOR_OP_EN4B:
+		reg = readl(host->regbase + FMC_CFG);
+		reg |= SPI_NOR_ADDR_MODE;
+		writel(reg, host->regbase + FMC_CFG);
+		break;
+	case SPINOR_OP_EX4B:
+		reg = readl(host->regbase + FMC_CFG);
+		reg &= ~SPI_NOR_ADDR_MODE;
+		writel(reg, host->regbase + FMC_CFG);
+		break;
+	case SPINOR_OP_CHIP_ERASE:
+	default:
+		break;
+	}
+}
+
+static int hisi_spi_nor_send_cmd(struct spi_nor *nor, u8 cmd, int len)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	u32 reg, op_cfg = 0;
+
+	hisi_spi_nor_cmd_prepare(host, cmd, &op_cfg);
+
+	reg = FMC_CMD_CMD1(cmd);
+	writel(reg, host->regbase + FMC_CMD);
+
+	reg = OP_CFG_FM_CS(priv->chipselect);
+	if (op_cfg & FMC_OP_ADDR_EN)
+		reg |= OP_CFG_ADDR_NUM(nor->addr_width);
+	writel(reg, host->regbase + FMC_OP_CFG);
+
+	reg = FMC_DATA_NUM_CNT(len);
+	writel(reg, host->regbase + FMC_DATA_NUM);
+
+	writel(0xff, host->regbase + FMC_INT_CLR);
+	reg = op_cfg | FMC_OP_REG_OP_START;
+	writel(reg, host->regbase + FMC_OP);
+
+	return wait_op_finish(host);
+}
+
+static int hisi_spi_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
+		int len)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	int ret;
+
+	ret = hisi_spi_nor_send_cmd(nor, opcode, len);
+	if (ret)
+		return ret;
+
+	memcpy_fromio(buf, host->iobase, len);
+
+	return 0;
+}
+
+static int hisi_spi_nor_write_reg(struct spi_nor *nor, u8 opcode,
+				u8 *buf, int len)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+
+	if (len)
+		memcpy_toio(host->iobase, buf, len);
+
+	return hisi_spi_nor_send_cmd(nor, opcode, len);
+}
+
+static void hisi_spi_nor_dma_transfer(struct spi_nor *nor, loff_t start_off,
+		dma_addr_t dma_buf, size_t len, u8 op_type)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	u8 if_type = 0, dummy = 0;
+	u8 w_cmd = 0, r_cmd = 0;
+	u32 reg;
+
+	writel(start_off, host->regbase + FMC_ADDRL);
+
+	if (op_type == FMC_OP_READ) {
+		if_type = get_if_type(nor->flash_read);
+		dummy = nor->read_dummy >> 3;
+		r_cmd = nor->read_opcode;
+	} else {
+		w_cmd = nor->program_opcode;
+	}
+
+	reg = OP_CFG_FM_CS(priv->chipselect)
+		| OP_CFG_MEM_IF_TYPE(if_type)
+		| OP_CFG_ADDR_NUM(nor->addr_width)
+		| OP_CFG_DUMMY_NUM(dummy);
+	writel(reg, host->regbase + FMC_OP_CFG);
+
+	reg = FMC_DMA_LEN_SET(len);
+	writel(reg, host->regbase + FMC_DMA_LEN);
+	writel(dma_buf, host->regbase + FMC_DMA_SADDR_D0);
+
+	reg = OP_CTRL_RD_OPCODE(r_cmd)
+		| OP_CTRL_WR_OPCODE(w_cmd)
+		| OP_CTRL_RW_OP(op_type)
+		| OP_CTRL_DMA_OP_READY;
+	writel(0xff, host->regbase + FMC_INT_CLR);
+	writel(reg, host->regbase + FMC_OP_DMA);
+	wait_op_finish(host);
+}
+
+static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
+		size_t *retlen, u_char *read_buf)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	unsigned char *ptr = read_buf;
+	size_t actual_len;
+
+	*retlen = 0;
+	while (len > 0) {
+		actual_len = (len >= HIFMC_DMA_MAX_LEN)
+			? HIFMC_DMA_MAX_LEN : len;
+		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
+				actual_len, FMC_OP_READ);
+		memcpy(ptr, host->buffer, actual_len);
+		ptr += actual_len;
+		from += actual_len;
+		len -= actual_len;
+		*retlen += actual_len;
+	}
+
+	return 0;
+}
+
+static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
+			size_t len, size_t *retlen, const u_char *write_buf)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	const unsigned char *ptr = write_buf;
+	size_t actual_len;
+
+	*retlen = 0;
+	while (len > 0) {
+		if (to & HIFMC_DMA_MASK)
+			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
+				>= len	? len
+				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
+		else
+			actual_len = (len >= HIFMC_DMA_MAX_LEN)
+				? HIFMC_DMA_MAX_LEN : len;
+		memcpy(host->buffer, ptr, actual_len);
+		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
+				FMC_OP_WRITE);
+		to += actual_len;
+		ptr += actual_len;
+		len -= actual_len;
+		*retlen += actual_len;
+	}
+}
+
+static int hisi_spi_nor_erase(struct spi_nor *nor, loff_t offs)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+
+	writel(offs, host->regbase + FMC_ADDRL);
+
+	return hisi_spi_nor_send_cmd(nor, nor->erase_opcode, 0);
+}
+
+static int hisi_spi_nor_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct hifmc_host *host;
+	struct device_node *np;
+	int ret, i = 0;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, host);
+	host->dev = dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
+	host->regbase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(host->regbase))
+		return PTR_ERR(host->regbase);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	host->iobase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(host->iobase))
+		return PTR_ERR(host->iobase);
+
+	host->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(host->clk))
+		return PTR_ERR(host->clk);
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_warn(dev, "Unable to set dma mask\n");
+		return ret;
+	}
+
+	host->buffer = dmam_alloc_coherent(dev, HIFMC_DMA_MAX_LEN,
+			&host->dma_buffer, GFP_KERNEL);
+	if (!host->buffer)
+		return -ENOMEM;
+
+	mutex_init(&host->lock);
+	clk_prepare_enable(host->clk);
+	hisi_spi_nor_init(host);
+
+	for_each_available_child_of_node(dev->of_node, np) {
+		struct spi_nor *nor = &host->nor[i];
+		struct hifmc_priv *priv = &host->priv[i];
+		struct mtd_info *mtd = &nor->mtd;
+
+		mtd->name = np->name;
+		nor->dev = dev;
+		spi_nor_set_flash_node(nor, np);
+		ret = of_property_read_u32(np, "reg", &priv->chipselect);
+		if (ret)
+			goto fail;
+		ret = of_property_read_u32(np, "spi-max-frequency",
+				&priv->clkrate);
+		if (ret)
+			goto fail;
+		priv->host = host;
+		nor->priv = priv;
+
+		nor->prepare = hisi_spi_nor_prep;
+		nor->unprepare = hisi_spi_nor_unprep;
+		nor->read_reg = hisi_spi_nor_read_reg;
+		nor->write_reg = hisi_spi_nor_write_reg;
+		nor->read = hisi_spi_nor_read;
+		nor->write = hisi_spi_nor_write;
+		nor->erase = hisi_spi_nor_erase;
+		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+		if (ret)
+			goto fail;
+
+		ret = mtd_device_register(mtd, NULL, 0);
+		if (ret)
+			goto fail;
+
+		i++;
+		host->num_chip++;
+		if (i == HIFMC_MAX_CHIP_NUM) {
+			dev_warn(dev, "Flash device number exceeds the maximum chipselect number\n");
+			break;
+		}
+	}
+
+	clk_disable_unprepare(host->clk);
+	return 0;
+
+fail:
+	for (i = 0; i < host->num_chip; i++)
+		mtd_device_unregister(&host->nor[i].mtd);
+
+	clk_disable_unprepare(host->clk);
+	mutex_destroy(&host->lock);
+
+	return ret;
+}
+
+static int hisi_spi_nor_remove(struct platform_device *pdev)
+{
+	struct hifmc_host *host = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < host->num_chip; i++)
+		mtd_device_unregister(&host->nor[i].mtd);
+
+	clk_disable_unprepare(host->clk);
+	mutex_destroy(&host->lock);
+
+	return 0;
+}
+
+static const struct of_device_id hisi_spi_nor_dt_ids[] = {
+	{ .compatible = "hisilicon,fmc-spi-nor"},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, hisi_spi_nor_dt_ids);
+
+static struct platform_driver hisi_spi_nor_driver = {
+	.driver = {
+		.name	= "hisi-sfc",
+		.of_match_table = hisi_spi_nor_dt_ids,
+	},
+	.probe	= hisi_spi_nor_probe,
+	.remove	= hisi_spi_nor_remove,
+};
+module_platform_driver(hisi_spi_nor_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("HiSilicon SPI Nor Flash Controller Driver");
-- 
1.9.1

[RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Add hisilicon spi-nor flash controller driver

Signed-off-by: Binquan Peng <pengbinquan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jiancheng Xue <xuejiancheng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reviewed-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
Reviewed-by: Jagan Teki <jteki-oRp2ZoJdM/RWk0Htik3J/w@public.gmane.org>
---
change log
v9:
Fixed issues pointed by Jagan Teki.
v8:
Fixed issues pointed by Ezequiel Garcia and Brian Norris.
Moved dts binding file to mtd directory.
Changed the compatible string more specific.
v7:
Rebased to v4.5-rc3.
Fixed issues pointed by Ezequiel Garcia.
v6:
Based on v4.5-rc2
Fixed issues pointed by Ezequiel Garcia.
v5:
Fixed a compile error.
v4:
Rebased to v4.5-rc1
v3:
Added a compatible string "hisilicon,hi3519-sfc".
v2:
Fixed some compiling warings.

 .../bindings/mtd/hisilicon,fmc-spi-nor.txt         |  24 +
 drivers/mtd/spi-nor/Kconfig                        |   7 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/hisi-sfc.c                     | 502 +++++++++++++++++++++
 4 files changed, 534 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt
 create mode 100644 drivers/mtd/spi-nor/hisi-sfc.c

diff --git a/Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt b/Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt
new file mode 100644
index 0000000..7498152
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/hisilicon,fmc-spi-nor.txt
@@ -0,0 +1,24 @@
+HiSilicon SPI-NOR Flash Controller
+
+Required properties:
+- compatible : Should be "hisilicon,fmc-spi-nor" and one of the following strings:
+		"hisilicon,hi3519-spi-nor"
+- address-cells : Should be 1.
+- size-cells : Should be 0.
+- reg : Offset and length of the register set for the controller device.
+- reg-names : Must include the following two entries: "control", "memory".
+- clocks : handle to spi-nor flash controller clock.
+
+Example:
+spi-nor-controller@10000000 {
+	compatible = "hisilicon,hi3519-spi-nor", "hisilicon,fmc-spi-nor";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x10000000 0x1000>, <0x14000000 0x1000000>;
+	reg-names = "control", "memory";
+	clocks = <&clock HI3519_FMC_CLK>;
+	spi-nor@0 {
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+	};
+};
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 0dc9275..120624d 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -37,6 +37,13 @@ config SPI_FSL_QUADSPI
 	  This controller does not support generic SPI. It only supports
 	  SPI NOR.
 
+config SPI_HISI_SFC
+	tristate "Hisilicon SPI-NOR Flash Controller(SFC)"
+	depends on ARCH_HISI || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This enables support for hisilicon SPI-NOR flash controller.
+
 config SPI_NXP_SPIFI
 	tristate "NXP SPI Flash Interface (SPIFI)"
 	depends on OF && (ARCH_LPC18XX || COMPILE_TEST)
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 0bf3a7f8..8a6fa69 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
+obj-$(CONFIG_SPI_HISI_SFC)	+= hisi-sfc.o
 obj-$(CONFIG_MTD_MT81xx_NOR)    += mtk-quadspi.o
 obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
new file mode 100644
index 0000000..e974c92
--- /dev/null
+++ b/drivers/mtd/spi-nor/hisi-sfc.c
@@ -0,0 +1,502 @@
+/*
+ * HiSilicon SPI Nor Flash Controller Driver
+ *
+ * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/spi-nor.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+/* Hardware register offsets and field definitions */
+#define FMC_CFG				0x00
+#define SPI_NOR_ADDR_MODE		BIT(10)
+#define FMC_GLOBAL_CFG			0x04
+#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
+#define FMC_SPI_TIMING_CFG		0x08
+#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
+#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
+#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
+#define CS_HOLD_TIME			0x6
+#define CS_SETUP_TIME			0x6
+#define CS_DESELECT_TIME		0xf
+#define FMC_INT				0x18
+#define FMC_INT_OP_DONE			BIT(0)
+#define FMC_INT_CLR			0x20
+#define FMC_CMD				0x24
+#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
+#define FMC_ADDRL			0x2c
+#define FMC_OP_CFG			0x30
+#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
+#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
+#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
+#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
+#define FMC_DATA_NUM			0x38
+#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
+#define FMC_OP				0x3c
+#define FMC_OP_DUMMY_EN			BIT(8)
+#define FMC_OP_CMD1_EN			BIT(7)
+#define FMC_OP_ADDR_EN			BIT(6)
+#define FMC_OP_WRITE_DATA_EN		BIT(5)
+#define FMC_OP_READ_DATA_EN		BIT(2)
+#define FMC_OP_READ_STATUS_EN		BIT(1)
+#define FMC_OP_REG_OP_START		BIT(0)
+#define FMC_DMA_LEN			0x40
+#define FMC_DMA_LEN_SET(_len)		((_len) & 0x0fffffff)
+#define FMC_DMA_SADDR_D0		0x4c
+#define HIFMC_DMA_MAX_LEN		(4096)
+#define HIFMC_DMA_MASK			(HIFMC_DMA_MAX_LEN - 1)
+#define FMC_OP_DMA			0x68
+#define OP_CTRL_RD_OPCODE(_code)	(((_code) & 0xff) << 16)
+#define OP_CTRL_WR_OPCODE(_code)	(((_code) & 0xff) << 8)
+#define OP_CTRL_RW_OP(_op)		((_op) << 1)
+#define OP_CTRL_DMA_OP_READY		BIT(0)
+#define FMC_OP_READ			0x0
+#define FMC_OP_WRITE			0x1
+#define FMC_WAIT_TIMEOUT		1000000
+
+enum hifmc_iftype {
+	IF_TYPE_STD,
+	IF_TYPE_DUAL,
+	IF_TYPE_DIO,
+	IF_TYPE_QUAD,
+	IF_TYPE_QIO,
+};
+
+struct hifmc_priv {
+	int chipselect;
+	u32 clkrate;
+	struct hifmc_host *host;
+};
+
+#define HIFMC_MAX_CHIP_NUM		2
+struct hifmc_host {
+	struct device *dev;
+	struct mutex lock;
+
+	void __iomem *regbase;
+	void __iomem *iobase;
+	struct clk *clk;
+	void *buffer;
+	dma_addr_t dma_buffer;
+
+	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
+	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
+	int num_chip;
+};
+
+static inline int wait_op_finish(struct hifmc_host *host)
+{
+	unsigned int reg;
+
+	return readl_poll_timeout(host->regbase + FMC_INT, reg,
+		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
+}
+
+static int get_if_type(enum read_mode flash_read)
+{
+	enum hifmc_iftype if_type;
+
+	switch (flash_read) {
+	case SPI_NOR_DUAL:
+		if_type = IF_TYPE_DUAL;
+		break;
+	case SPI_NOR_QUAD:
+		if_type = IF_TYPE_QUAD;
+		break;
+	case SPI_NOR_NORMAL:
+	case SPI_NOR_FAST:
+	default:
+		if_type = IF_TYPE_STD;
+		break;
+	}
+
+	return if_type;
+}
+
+static void hisi_spi_nor_init(struct hifmc_host *host)
+{
+	unsigned int reg;
+
+	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
+		| TIMING_CFG_TCSS(CS_SETUP_TIME)
+		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
+	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
+}
+
+static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	int ret;
+
+	mutex_lock(&host->lock);
+
+	ret = clk_set_rate(host->clk, priv->clkrate);
+	if (ret)
+		goto out;
+
+	ret = clk_prepare_enable(host->clk);
+	if (ret)
+		goto out;
+
+	return 0;
+
+out:
+	mutex_unlock(&host->lock);
+	return ret;
+}
+
+static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+
+	clk_disable_unprepare(host->clk);
+	mutex_unlock(&host->lock);
+}
+
+static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
+		u32 *opcfg)
+{
+	u32 reg;
+
+	*opcfg |= FMC_OP_CMD1_EN;
+	switch (cmd) {
+	case SPINOR_OP_RDID:
+	case SPINOR_OP_RDSR:
+	case SPINOR_OP_RDCR:
+		*opcfg |= FMC_OP_READ_DATA_EN;
+		break;
+	case SPINOR_OP_WREN:
+		reg = readl(host->regbase + FMC_GLOBAL_CFG);
+		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
+			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
+			writel(reg, host->regbase + FMC_GLOBAL_CFG);
+		}
+		break;
+	case SPINOR_OP_WRSR:
+		*opcfg |= FMC_OP_WRITE_DATA_EN;
+		break;
+	case SPINOR_OP_BE_4K:
+	case SPINOR_OP_BE_4K_PMC:
+	case SPINOR_OP_SE_4B:
+	case SPINOR_OP_SE:
+		*opcfg |= FMC_OP_ADDR_EN;
+		break;
+	case SPINOR_OP_EN4B:
+		reg = readl(host->regbase + FMC_CFG);
+		reg |= SPI_NOR_ADDR_MODE;
+		writel(reg, host->regbase + FMC_CFG);
+		break;
+	case SPINOR_OP_EX4B:
+		reg = readl(host->regbase + FMC_CFG);
+		reg &= ~SPI_NOR_ADDR_MODE;
+		writel(reg, host->regbase + FMC_CFG);
+		break;
+	case SPINOR_OP_CHIP_ERASE:
+	default:
+		break;
+	}
+}
+
+static int hisi_spi_nor_send_cmd(struct spi_nor *nor, u8 cmd, int len)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	u32 reg, op_cfg = 0;
+
+	hisi_spi_nor_cmd_prepare(host, cmd, &op_cfg);
+
+	reg = FMC_CMD_CMD1(cmd);
+	writel(reg, host->regbase + FMC_CMD);
+
+	reg = OP_CFG_FM_CS(priv->chipselect);
+	if (op_cfg & FMC_OP_ADDR_EN)
+		reg |= OP_CFG_ADDR_NUM(nor->addr_width);
+	writel(reg, host->regbase + FMC_OP_CFG);
+
+	reg = FMC_DATA_NUM_CNT(len);
+	writel(reg, host->regbase + FMC_DATA_NUM);
+
+	writel(0xff, host->regbase + FMC_INT_CLR);
+	reg = op_cfg | FMC_OP_REG_OP_START;
+	writel(reg, host->regbase + FMC_OP);
+
+	return wait_op_finish(host);
+}
+
+static int hisi_spi_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
+		int len)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	int ret;
+
+	ret = hisi_spi_nor_send_cmd(nor, opcode, len);
+	if (ret)
+		return ret;
+
+	memcpy_fromio(buf, host->iobase, len);
+
+	return 0;
+}
+
+static int hisi_spi_nor_write_reg(struct spi_nor *nor, u8 opcode,
+				u8 *buf, int len)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+
+	if (len)
+		memcpy_toio(host->iobase, buf, len);
+
+	return hisi_spi_nor_send_cmd(nor, opcode, len);
+}
+
+static void hisi_spi_nor_dma_transfer(struct spi_nor *nor, loff_t start_off,
+		dma_addr_t dma_buf, size_t len, u8 op_type)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	u8 if_type = 0, dummy = 0;
+	u8 w_cmd = 0, r_cmd = 0;
+	u32 reg;
+
+	writel(start_off, host->regbase + FMC_ADDRL);
+
+	if (op_type == FMC_OP_READ) {
+		if_type = get_if_type(nor->flash_read);
+		dummy = nor->read_dummy >> 3;
+		r_cmd = nor->read_opcode;
+	} else {
+		w_cmd = nor->program_opcode;
+	}
+
+	reg = OP_CFG_FM_CS(priv->chipselect)
+		| OP_CFG_MEM_IF_TYPE(if_type)
+		| OP_CFG_ADDR_NUM(nor->addr_width)
+		| OP_CFG_DUMMY_NUM(dummy);
+	writel(reg, host->regbase + FMC_OP_CFG);
+
+	reg = FMC_DMA_LEN_SET(len);
+	writel(reg, host->regbase + FMC_DMA_LEN);
+	writel(dma_buf, host->regbase + FMC_DMA_SADDR_D0);
+
+	reg = OP_CTRL_RD_OPCODE(r_cmd)
+		| OP_CTRL_WR_OPCODE(w_cmd)
+		| OP_CTRL_RW_OP(op_type)
+		| OP_CTRL_DMA_OP_READY;
+	writel(0xff, host->regbase + FMC_INT_CLR);
+	writel(reg, host->regbase + FMC_OP_DMA);
+	wait_op_finish(host);
+}
+
+static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
+		size_t *retlen, u_char *read_buf)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	unsigned char *ptr = read_buf;
+	size_t actual_len;
+
+	*retlen = 0;
+	while (len > 0) {
+		actual_len = (len >= HIFMC_DMA_MAX_LEN)
+			? HIFMC_DMA_MAX_LEN : len;
+		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
+				actual_len, FMC_OP_READ);
+		memcpy(ptr, host->buffer, actual_len);
+		ptr += actual_len;
+		from += actual_len;
+		len -= actual_len;
+		*retlen += actual_len;
+	}
+
+	return 0;
+}
+
+static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
+			size_t len, size_t *retlen, const u_char *write_buf)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+	const unsigned char *ptr = write_buf;
+	size_t actual_len;
+
+	*retlen = 0;
+	while (len > 0) {
+		if (to & HIFMC_DMA_MASK)
+			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
+				>= len	? len
+				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
+		else
+			actual_len = (len >= HIFMC_DMA_MAX_LEN)
+				? HIFMC_DMA_MAX_LEN : len;
+		memcpy(host->buffer, ptr, actual_len);
+		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
+				FMC_OP_WRITE);
+		to += actual_len;
+		ptr += actual_len;
+		len -= actual_len;
+		*retlen += actual_len;
+	}
+}
+
+static int hisi_spi_nor_erase(struct spi_nor *nor, loff_t offs)
+{
+	struct hifmc_priv *priv = nor->priv;
+	struct hifmc_host *host = priv->host;
+
+	writel(offs, host->regbase + FMC_ADDRL);
+
+	return hisi_spi_nor_send_cmd(nor, nor->erase_opcode, 0);
+}
+
+static int hisi_spi_nor_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct hifmc_host *host;
+	struct device_node *np;
+	int ret, i = 0;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, host);
+	host->dev = dev;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
+	host->regbase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(host->regbase))
+		return PTR_ERR(host->regbase);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
+	host->iobase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(host->iobase))
+		return PTR_ERR(host->iobase);
+
+	host->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(host->clk))
+		return PTR_ERR(host->clk);
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+	if (ret) {
+		dev_warn(dev, "Unable to set dma mask\n");
+		return ret;
+	}
+
+	host->buffer = dmam_alloc_coherent(dev, HIFMC_DMA_MAX_LEN,
+			&host->dma_buffer, GFP_KERNEL);
+	if (!host->buffer)
+		return -ENOMEM;
+
+	mutex_init(&host->lock);
+	clk_prepare_enable(host->clk);
+	hisi_spi_nor_init(host);
+
+	for_each_available_child_of_node(dev->of_node, np) {
+		struct spi_nor *nor = &host->nor[i];
+		struct hifmc_priv *priv = &host->priv[i];
+		struct mtd_info *mtd = &nor->mtd;
+
+		mtd->name = np->name;
+		nor->dev = dev;
+		spi_nor_set_flash_node(nor, np);
+		ret = of_property_read_u32(np, "reg", &priv->chipselect);
+		if (ret)
+			goto fail;
+		ret = of_property_read_u32(np, "spi-max-frequency",
+				&priv->clkrate);
+		if (ret)
+			goto fail;
+		priv->host = host;
+		nor->priv = priv;
+
+		nor->prepare = hisi_spi_nor_prep;
+		nor->unprepare = hisi_spi_nor_unprep;
+		nor->read_reg = hisi_spi_nor_read_reg;
+		nor->write_reg = hisi_spi_nor_write_reg;
+		nor->read = hisi_spi_nor_read;
+		nor->write = hisi_spi_nor_write;
+		nor->erase = hisi_spi_nor_erase;
+		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+		if (ret)
+			goto fail;
+
+		ret = mtd_device_register(mtd, NULL, 0);
+		if (ret)
+			goto fail;
+
+		i++;
+		host->num_chip++;
+		if (i == HIFMC_MAX_CHIP_NUM) {
+			dev_warn(dev, "Flash device number exceeds the maximum chipselect number\n");
+			break;
+		}
+	}
+
+	clk_disable_unprepare(host->clk);
+	return 0;
+
+fail:
+	for (i = 0; i < host->num_chip; i++)
+		mtd_device_unregister(&host->nor[i].mtd);
+
+	clk_disable_unprepare(host->clk);
+	mutex_destroy(&host->lock);
+
+	return ret;
+}
+
+static int hisi_spi_nor_remove(struct platform_device *pdev)
+{
+	struct hifmc_host *host = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < host->num_chip; i++)
+		mtd_device_unregister(&host->nor[i].mtd);
+
+	clk_disable_unprepare(host->clk);
+	mutex_destroy(&host->lock);
+
+	return 0;
+}
+
+static const struct of_device_id hisi_spi_nor_dt_ids[] = {
+	{ .compatible = "hisilicon,fmc-spi-nor"},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, hisi_spi_nor_dt_ids);
+
+static struct platform_driver hisi_spi_nor_driver = {
+	.driver = {
+		.name	= "hisi-sfc",
+		.of_match_table = hisi_spi_nor_dt_ids,
+	},
+	.probe	= hisi_spi_nor_probe,
+	.remove	= hisi_spi_nor_remove,
+};
+module_platform_driver(hisi_spi_nor_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("HiSilicon SPI Nor Flash Controller Driver");
-- 
1.9.1

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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Marek Vasut]

On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
> Add hisilicon spi-nor flash controller driver
> 
> Signed-off-by: Binquan Peng <pengbinquan@huawei.com>
> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Reviewed-by: Jagan Teki <jteki@openedev.com>
> ---
> change log
> v9:
> Fixed issues pointed by Jagan Teki.

It'd be really great if you could list which exact issues you fixed.

> v8:
> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
> Moved dts binding file to mtd directory.
> Changed the compatible string more specific.

[...]

> +/* Hardware register offsets and field definitions */
> +#define FMC_CFG				0x00
> +#define SPI_NOR_ADDR_MODE		BIT(10)
> +#define FMC_GLOBAL_CFG			0x04
> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
> +#define FMC_SPI_TIMING_CFG		0x08
> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
> +#define CS_HOLD_TIME			0x6
> +#define CS_SETUP_TIME			0x6
> +#define CS_DESELECT_TIME		0xf
> +#define FMC_INT				0x18
> +#define FMC_INT_OP_DONE			BIT(0)
> +#define FMC_INT_CLR			0x20
> +#define FMC_CMD				0x24
> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)

Drop the underscores in the argument names.

> +#define FMC_ADDRL			0x2c
> +#define FMC_OP_CFG			0x30
> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
> +#define FMC_DATA_NUM			0x38
> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
> +#define FMC_OP				0x3c
> +#define FMC_OP_DUMMY_EN			BIT(8)
> +#define FMC_OP_CMD1_EN			BIT(7)
> +#define FMC_OP_ADDR_EN			BIT(6)
> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
> +#define FMC_OP_READ_DATA_EN		BIT(2)
> +#define FMC_OP_READ_STATUS_EN		BIT(1)
> +#define FMC_OP_REG_OP_START		BIT(0)

[...]

> +enum hifmc_iftype {
> +	IF_TYPE_STD,
> +	IF_TYPE_DUAL,
> +	IF_TYPE_DIO,
> +	IF_TYPE_QUAD,
> +	IF_TYPE_QIO,
> +};
> +
> +struct hifmc_priv {
> +	int chipselect;

Can chipselect be set to < 0 values ?

> +	u32 clkrate;
> +	struct hifmc_host *host;
> +};
> +
> +#define HIFMC_MAX_CHIP_NUM		2

This does not scale very well, use dynamic allocation.

> +struct hifmc_host {
> +	struct device *dev;
> +	struct mutex lock;
> +
> +	void __iomem *regbase;
> +	void __iomem *iobase;
> +	struct clk *clk;
> +	void *buffer;
> +	dma_addr_t dma_buffer;
> +
> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
> +	int num_chip;
> +};
> +
> +static inline int wait_op_finish(struct hifmc_host *host)
> +{
> +	unsigned int reg;
> +
> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
> +}
> +
> +static int get_if_type(enum read_mode flash_read)
> +{
> +	enum hifmc_iftype if_type;
> +
> +	switch (flash_read) {
> +	case SPI_NOR_DUAL:
> +		if_type = IF_TYPE_DUAL;
> +		break;
> +	case SPI_NOR_QUAD:
> +		if_type = IF_TYPE_QUAD;
> +		break;
> +	case SPI_NOR_NORMAL:
> +	case SPI_NOR_FAST:
> +	default:
> +		if_type = IF_TYPE_STD;
> +		break;
> +	}
> +
> +	return if_type;
> +}
> +
> +static void hisi_spi_nor_init(struct hifmc_host *host)
> +{
> +	unsigned int reg;

Should be u32 here.

> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
> +}
> +
> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +	int ret;
> +
> +	mutex_lock(&host->lock);

Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
locks a mutex in struct spi_nor , but that's not enough if you have
multiple SPI NORs on the same bus, because concurrent access to multiple
SPI NOR flashes needs locking on the controller level, right ?

> +	ret = clk_set_rate(host->clk, priv->clkrate);
> +	if (ret)
> +		goto out;
> +
> +	ret = clk_prepare_enable(host->clk);
> +	if (ret)
> +		goto out;
> +
> +	return 0;
> +
> +out:
> +	mutex_unlock(&host->lock);
> +	return ret;
> +}
> +
> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +
> +	clk_disable_unprepare(host->clk);
> +	mutex_unlock(&host->lock);
> +}
> +
> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
> +		u32 *opcfg)
> +{
> +	u32 reg;
> +
> +	*opcfg |= FMC_OP_CMD1_EN;
> +	switch (cmd) {
> +	case SPINOR_OP_RDID:
> +	case SPINOR_OP_RDSR:
> +	case SPINOR_OP_RDCR:
> +		*opcfg |= FMC_OP_READ_DATA_EN;
> +		break;
> +	case SPINOR_OP_WREN:
> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
> +		}
> +		break;
> +	case SPINOR_OP_WRSR:
> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
> +		break;
> +	case SPINOR_OP_BE_4K:
> +	case SPINOR_OP_BE_4K_PMC:
> +	case SPINOR_OP_SE_4B:
> +	case SPINOR_OP_SE:
> +		*opcfg |= FMC_OP_ADDR_EN;
> +		break;
> +	case SPINOR_OP_EN4B:
> +		reg = readl(host->regbase + FMC_CFG);
> +		reg |= SPI_NOR_ADDR_MODE;
> +		writel(reg, host->regbase + FMC_CFG);
> +		break;
> +	case SPINOR_OP_EX4B:
> +		reg = readl(host->regbase + FMC_CFG);
> +		reg &= ~SPI_NOR_ADDR_MODE;
> +		writel(reg, host->regbase + FMC_CFG);
> +		break;
> +	case SPINOR_OP_CHIP_ERASE:
> +	default:
> +		break;
> +	}

Won't the driver fail if we add new instructions into the SPI NOR core
which are not handled by this huge switch statement ?

> +}

[...]

> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
> +			size_t len, size_t *retlen, const u_char *write_buf)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +	const unsigned char *ptr = write_buf;
> +	size_t actual_len;
> +
> +	*retlen = 0;
> +	while (len > 0) {
> +		if (to & HIFMC_DMA_MASK)
> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> +				>= len	? len
> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));

Rewrite this as something like the following snippet, for the sake of
readability:

actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
if (actual_len >= len)
   actual_len = len;

> +		else
> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
> +				? HIFMC_DMA_MAX_LEN : len;
> +		memcpy(host->buffer, ptr, actual_len);
> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
> +				FMC_OP_WRITE);
> +		to += actual_len;
> +		ptr += actual_len;
> +		len -= actual_len;
> +		*retlen += actual_len;
> +	}
> +}
> +
> +static int hisi_spi_nor_erase(struct spi_nor *nor, loff_t offs)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +
> +	writel(offs, host->regbase + FMC_ADDRL);
> +
> +	return hisi_spi_nor_send_cmd(nor, nor->erase_opcode, 0);
> +}
> +
> +static int hisi_spi_nor_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct hifmc_host *host;
> +	struct device_node *np;
> +	int ret, i = 0;
> +
> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, host);
> +	host->dev = dev;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
> +	host->regbase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(host->regbase))
> +		return PTR_ERR(host->regbase);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
> +	host->iobase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(host->iobase))
> +		return PTR_ERR(host->iobase);
> +
> +	host->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(host->clk))
> +		return PTR_ERR(host->clk);
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_warn(dev, "Unable to set dma mask\n");
> +		return ret;
> +	}
> +
> +	host->buffer = dmam_alloc_coherent(dev, HIFMC_DMA_MAX_LEN,
> +			&host->dma_buffer, GFP_KERNEL);
> +	if (!host->buffer)
> +		return -ENOMEM;
> +
> +	mutex_init(&host->lock);
> +	clk_prepare_enable(host->clk);
> +	hisi_spi_nor_init(host);
> +
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		struct spi_nor *nor = &host->nor[i];
> +		struct hifmc_priv *priv = &host->priv[i];
> +		struct mtd_info *mtd = &nor->mtd;
> +
> +		mtd->name = np->name;
> +		nor->dev = dev;
> +		spi_nor_set_flash_node(nor, np);
> +		ret = of_property_read_u32(np, "reg", &priv->chipselect);
> +		if (ret)
> +			goto fail;
> +		ret = of_property_read_u32(np, "spi-max-frequency",
> +				&priv->clkrate);
> +		if (ret)
> +			goto fail;
> +		priv->host = host;
> +		nor->priv = priv;
> +
> +		nor->prepare = hisi_spi_nor_prep;
> +		nor->unprepare = hisi_spi_nor_unprep;
> +		nor->read_reg = hisi_spi_nor_read_reg;
> +		nor->write_reg = hisi_spi_nor_write_reg;
> +		nor->read = hisi_spi_nor_read;
> +		nor->write = hisi_spi_nor_write;
> +		nor->erase = hisi_spi_nor_erase;
> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		if (ret)
> +			goto fail;
> +
> +		ret = mtd_device_register(mtd, NULL, 0);
> +		if (ret)
> +			goto fail;
> +
> +		i++;
> +		host->num_chip++;
> +		if (i == HIFMC_MAX_CHIP_NUM) {
> +			dev_warn(dev, "Flash device number exceeds the maximum chipselect number\n");
> +			break;
> +		}

Please factor this loop out into a separate function, so we can easily
locate the developing boilerplate.

> +	}
> +
> +	clk_disable_unprepare(host->clk);
> +	return 0;
> +
> +fail:
> +	for (i = 0; i < host->num_chip; i++)
> +		mtd_device_unregister(&host->nor[i].mtd);

Did you ever exercise this fail path ? I think if you call this without
registering all of the MTD devices, it will crash, but I might be wrong
on this one.

> +	clk_disable_unprepare(host->clk);
> +	mutex_destroy(&host->lock);
> +
> +	return ret;
> +}
> +
> +static int hisi_spi_nor_remove(struct platform_device *pdev)
> +{
> +	struct hifmc_host *host = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < host->num_chip; i++)
> +		mtd_device_unregister(&host->nor[i].mtd);
> +
> +	clk_disable_unprepare(host->clk);
> +	mutex_destroy(&host->lock);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id hisi_spi_nor_dt_ids[] = {
> +	{ .compatible = "hisilicon,fmc-spi-nor"},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, hisi_spi_nor_dt_ids);
> +
> +static struct platform_driver hisi_spi_nor_driver = {
> +	.driver = {
> +		.name	= "hisi-sfc",
> +		.of_match_table = hisi_spi_nor_dt_ids,
> +	},
> +	.probe	= hisi_spi_nor_probe,
> +	.remove	= hisi_spi_nor_remove,
> +};
> +module_platform_driver(hisi_spi_nor_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("HiSilicon SPI Nor Flash Controller Driver");
> 

btw I also trimmed down the crazy CC list.

-- 
Best regards,
Marek Vasut

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Marek,
    Firstly, thank you very much for your comments.

On 2016/3/27 9:47, Marek Vasut wrote:
> On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
>> Add hisilicon spi-nor flash controller driver
>>
>> Signed-off-by: Binquan Peng <pengbinquan@huawei.com>
>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> Reviewed-by: Jagan Teki <jteki@openedev.com>
>> ---
>> change log
>> v9:
>> Fixed issues pointed by Jagan Teki.
> 
> It'd be really great if you could list which exact issues you fixed.
> 

OK. I'll describe the history log more detailed in next version.

>> v8:
>> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
>> Moved dts binding file to mtd directory.
>> Changed the compatible string more specific.
> 
> [...]
> 
>> +/* Hardware register offsets and field definitions */
>> +#define FMC_CFG				0x00
>> +#define SPI_NOR_ADDR_MODE		BIT(10)
>> +#define FMC_GLOBAL_CFG			0x04
>> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
>> +#define FMC_SPI_TIMING_CFG		0x08
>> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
>> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
>> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
>> +#define CS_HOLD_TIME			0x6
>> +#define CS_SETUP_TIME			0x6
>> +#define CS_DESELECT_TIME		0xf
>> +#define FMC_INT				0x18
>> +#define FMC_INT_OP_DONE			BIT(0)
>> +#define FMC_INT_CLR			0x20
>> +#define FMC_CMD				0x24
>> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
> 
> Drop the underscores in the argument names.
> 
OK.
>> +#define FMC_ADDRL			0x2c
>> +#define FMC_OP_CFG			0x30
>> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
>> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
>> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
>> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
>> +#define FMC_DATA_NUM			0x38
>> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
>> +#define FMC_OP				0x3c
>> +#define FMC_OP_DUMMY_EN			BIT(8)
>> +#define FMC_OP_CMD1_EN			BIT(7)
>> +#define FMC_OP_ADDR_EN			BIT(6)
>> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
>> +#define FMC_OP_READ_DATA_EN		BIT(2)
>> +#define FMC_OP_READ_STATUS_EN		BIT(1)
>> +#define FMC_OP_REG_OP_START		BIT(0)
> 
> [...]
> 
>> +enum hifmc_iftype {
>> +	IF_TYPE_STD,
>> +	IF_TYPE_DUAL,
>> +	IF_TYPE_DIO,
>> +	IF_TYPE_QUAD,
>> +	IF_TYPE_QIO,
>> +};
>> +
>> +struct hifmc_priv {
>> +	int chipselect;
> 
> Can chipselect be set to < 0 values ?
> 
The type will be changed to u32.

>> +	u32 clkrate;
>> +	struct hifmc_host *host;
>> +};
>> +
>> +#define HIFMC_MAX_CHIP_NUM		2
> 
> This does not scale very well, use dynamic allocation.
> 
OK. Dynamic allocation will be used in next version.
>> +struct hifmc_host {
>> +	struct device *dev;
>> +	struct mutex lock;
>> +
>> +	void __iomem *regbase;
>> +	void __iomem *iobase;
>> +	struct clk *clk;
>> +	void *buffer;
>> +	dma_addr_t dma_buffer;
>> +
>> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
>> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
>> +	int num_chip;
>> +};
>> +
>> +static inline int wait_op_finish(struct hifmc_host *host)
>> +{
>> +	unsigned int reg;
>> +
>> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
>> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
>> +}
>> +
>> +static int get_if_type(enum read_mode flash_read)
>> +{
>> +	enum hifmc_iftype if_type;
>> +
>> +	switch (flash_read) {
>> +	case SPI_NOR_DUAL:
>> +		if_type = IF_TYPE_DUAL;
>> +		break;
>> +	case SPI_NOR_QUAD:
>> +		if_type = IF_TYPE_QUAD;
>> +		break;
>> +	case SPI_NOR_NORMAL:
>> +	case SPI_NOR_FAST:
>> +	default:
>> +		if_type = IF_TYPE_STD;
>> +		break;
>> +	}
>> +
>> +	return if_type;
>> +}
>> +
>> +static void hisi_spi_nor_init(struct hifmc_host *host)
>> +{
>> +	unsigned int reg;
> 
> Should be u32 here.
> 
OK.
>> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
>> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
>> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
>> +}
>> +
>> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +	int ret;
>> +
>> +	mutex_lock(&host->lock);
> 
> Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
> locks a mutex in struct spi_nor , but that's not enough if you have
> multiple SPI NORs on the same bus, because concurrent access to multiple
> SPI NOR flashes needs locking on the controller level, right ?
> 
Yes, you are quite right. The controller can connect with two SPI NOR flashes
on one board. This lock is used on the controller level.

>> +	ret = clk_set_rate(host->clk, priv->clkrate);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = clk_prepare_enable(host->clk);
>> +	if (ret)
>> +		goto out;
>> +
>> +	return 0;
>> +
>> +out:
>> +	mutex_unlock(&host->lock);
>> +	return ret;
>> +}
>> +
>> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +
>> +	clk_disable_unprepare(host->clk);
>> +	mutex_unlock(&host->lock);
>> +}
>> +
>> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
>> +		u32 *opcfg)
>> +{
>> +	u32 reg;
>> +
>> +	*opcfg |= FMC_OP_CMD1_EN;
>> +	switch (cmd) {
>> +	case SPINOR_OP_RDID:
>> +	case SPINOR_OP_RDSR:
>> +	case SPINOR_OP_RDCR:
>> +		*opcfg |= FMC_OP_READ_DATA_EN;
>> +		break;
>> +	case SPINOR_OP_WREN:
>> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
>> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
>> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
>> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
>> +		}
>> +		break;
>> +	case SPINOR_OP_WRSR:
>> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
>> +		break;
>> +	case SPINOR_OP_BE_4K:
>> +	case SPINOR_OP_BE_4K_PMC:
>> +	case SPINOR_OP_SE_4B:
>> +	case SPINOR_OP_SE:
>> +		*opcfg |= FMC_OP_ADDR_EN;
>> +		break;
>> +	case SPINOR_OP_EN4B:
>> +		reg = readl(host->regbase + FMC_CFG);
>> +		reg |= SPI_NOR_ADDR_MODE;
>> +		writel(reg, host->regbase + FMC_CFG);
>> +		break;
>> +	case SPINOR_OP_EX4B:
>> +		reg = readl(host->regbase + FMC_CFG);
>> +		reg &= ~SPI_NOR_ADDR_MODE;
>> +		writel(reg, host->regbase + FMC_CFG);
>> +		break;
>> +	case SPINOR_OP_CHIP_ERASE:
>> +	default:
>> +		break;
>> +	}
> 
> Won't the driver fail if we add new instructions into the SPI NOR core
> which are not handled by this huge switch statement ?
> 
Only some of commands are needed to process in this stage for the controller.
Others have no needs. So this function won't return failure.

I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.

>> +}
> 
> [...]
> 
>> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>> +			size_t len, size_t *retlen, const u_char *write_buf)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +	const unsigned char *ptr = write_buf;
>> +	size_t actual_len;
>> +
>> +	*retlen = 0;
>> +	while (len > 0) {
>> +		if (to & HIFMC_DMA_MASK)
>> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>> +				>= len	? len
>> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> 
> Rewrite this as something like the following snippet, for the sake of
> readability:
> 
> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
> if (actual_len >= len)
>    actual_len = len;
> 
OK. Thank you.
>> +		else
>> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>> +				? HIFMC_DMA_MAX_LEN : len;
>> +		memcpy(host->buffer, ptr, actual_len);
>> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>> +				FMC_OP_WRITE);
>> +		to += actual_len;
>> +		ptr += actual_len;
>> +		len -= actual_len;
>> +		*retlen += actual_len;
>> +	}
>> +}
>> +
>> +static int hisi_spi_nor_erase(struct spi_nor *nor, loff_t offs)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +
>> +	writel(offs, host->regbase + FMC_ADDRL);
>> +
>> +	return hisi_spi_nor_send_cmd(nor, nor->erase_opcode, 0);
>> +}
>> +
>> +static int hisi_spi_nor_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	struct hifmc_host *host;
>> +	struct device_node *np;
>> +	int ret, i = 0;
>> +
>> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
>> +	if (!host)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, host);
>> +	host->dev = dev;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
>> +	host->regbase = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(host->regbase))
>> +		return PTR_ERR(host->regbase);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
>> +	host->iobase = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(host->iobase))
>> +		return PTR_ERR(host->iobase);
>> +
>> +	host->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(host->clk))
>> +		return PTR_ERR(host->clk);
>> +
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> +	if (ret) {
>> +		dev_warn(dev, "Unable to set dma mask\n");
>> +		return ret;
>> +	}
>> +
>> +	host->buffer = dmam_alloc_coherent(dev, HIFMC_DMA_MAX_LEN,
>> +			&host->dma_buffer, GFP_KERNEL);
>> +	if (!host->buffer)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&host->lock);
>> +	clk_prepare_enable(host->clk);
>> +	hisi_spi_nor_init(host);
>> +
>> +	for_each_available_child_of_node(dev->of_node, np) {
>> +		struct spi_nor *nor = &host->nor[i];
>> +		struct hifmc_priv *priv = &host->priv[i];
>> +		struct mtd_info *mtd = &nor->mtd;
>> +
>> +		mtd->name = np->name;
>> +		nor->dev = dev;
>> +		spi_nor_set_flash_node(nor, np);
>> +		ret = of_property_read_u32(np, "reg", &priv->chipselect);
>> +		if (ret)
>> +			goto fail;
>> +		ret = of_property_read_u32(np, "spi-max-frequency",
>> +				&priv->clkrate);
>> +		if (ret)
>> +			goto fail;
>> +		priv->host = host;
>> +		nor->priv = priv;
>> +
>> +		nor->prepare = hisi_spi_nor_prep;
>> +		nor->unprepare = hisi_spi_nor_unprep;
>> +		nor->read_reg = hisi_spi_nor_read_reg;
>> +		nor->write_reg = hisi_spi_nor_write_reg;
>> +		nor->read = hisi_spi_nor_read;
>> +		nor->write = hisi_spi_nor_write;
>> +		nor->erase = hisi_spi_nor_erase;
>> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>> +		if (ret)
>> +			goto fail;
>> +
>> +		ret = mtd_device_register(mtd, NULL, 0);
>> +		if (ret)
>> +			goto fail;
>> +
>> +		i++;
>> +		host->num_chip++;
>> +		if (i == HIFMC_MAX_CHIP_NUM) {
>> +			dev_warn(dev, "Flash device number exceeds the maximum chipselect number\n");
>> +			break;
>> +		}
> 
> Please factor this loop out into a separate function, so we can easily
> locate the developing boilerplate.
> 
OK. I'll do it in next version.

>> +	}
>> +
>> +	clk_disable_unprepare(host->clk);
>> +	return 0;
>> +
>> +fail:
>> +	for (i = 0; i < host->num_chip; i++)
>> +		mtd_device_unregister(&host->nor[i].mtd);
> 
> Did you ever exercise this fail path ? I think if you call this without
> registering all of the MTD devices, it will crash, but I might be wrong
> on this one.
> 
Yes. Actually the host->num_chip means the number of successfully registered mtd devices.
I'll change the name to host->actual_chip_num to make it more readable.

>> +	clk_disable_unprepare(host->clk);
>> +	mutex_destroy(&host->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int hisi_spi_nor_remove(struct platform_device *pdev)
>> +{
>> +	struct hifmc_host *host = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	for (i = 0; i < host->num_chip; i++)
>> +		mtd_device_unregister(&host->nor[i].mtd);
>> +
>> +	clk_disable_unprepare(host->clk);
>> +	mutex_destroy(&host->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id hisi_spi_nor_dt_ids[] = {
>> +	{ .compatible = "hisilicon,fmc-spi-nor"},
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, hisi_spi_nor_dt_ids);
>> +
>> +static struct platform_driver hisi_spi_nor_driver = {
>> +	.driver = {
>> +		.name	= "hisi-sfc",
>> +		.of_match_table = hisi_spi_nor_dt_ids,
>> +	},
>> +	.probe	= hisi_spi_nor_probe,
>> +	.remove	= hisi_spi_nor_remove,
>> +};
>> +module_platform_driver(hisi_spi_nor_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("HiSilicon SPI Nor Flash Controller Driver");
>>
> 
> btw I also trimmed down the crazy CC list.
> 
Sorry about that and thank you again!

Regards,
Jiancheng

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Marek,
    Firstly, thank you very much for your comments.

On 2016/3/27 9:47, Marek Vasut wrote:
> On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
>> Add hisilicon spi-nor flash controller driver
>>
>> Signed-off-by: Binquan Peng <pengbinquan@huawei.com>
>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> Reviewed-by: Jagan Teki <jteki@openedev.com>
>> ---
>> change log
>> v9:
>> Fixed issues pointed by Jagan Teki.
> 
> It'd be really great if you could list which exact issues you fixed.
> 

OK. I'll describe the history log more detailed in next version.

>> v8:
>> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
>> Moved dts binding file to mtd directory.
>> Changed the compatible string more specific.
> 
> [...]
> 
>> +/* Hardware register offsets and field definitions */
>> +#define FMC_CFG				0x00
>> +#define SPI_NOR_ADDR_MODE		BIT(10)
>> +#define FMC_GLOBAL_CFG			0x04
>> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
>> +#define FMC_SPI_TIMING_CFG		0x08
>> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
>> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
>> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
>> +#define CS_HOLD_TIME			0x6
>> +#define CS_SETUP_TIME			0x6
>> +#define CS_DESELECT_TIME		0xf
>> +#define FMC_INT				0x18
>> +#define FMC_INT_OP_DONE			BIT(0)
>> +#define FMC_INT_CLR			0x20
>> +#define FMC_CMD				0x24
>> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
> 
> Drop the underscores in the argument names.
> 
OK.
>> +#define FMC_ADDRL			0x2c
>> +#define FMC_OP_CFG			0x30
>> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
>> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
>> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
>> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
>> +#define FMC_DATA_NUM			0x38
>> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
>> +#define FMC_OP				0x3c
>> +#define FMC_OP_DUMMY_EN			BIT(8)
>> +#define FMC_OP_CMD1_EN			BIT(7)
>> +#define FMC_OP_ADDR_EN			BIT(6)
>> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
>> +#define FMC_OP_READ_DATA_EN		BIT(2)
>> +#define FMC_OP_READ_STATUS_EN		BIT(1)
>> +#define FMC_OP_REG_OP_START		BIT(0)
> 
> [...]
> 
>> +enum hifmc_iftype {
>> +	IF_TYPE_STD,
>> +	IF_TYPE_DUAL,
>> +	IF_TYPE_DIO,
>> +	IF_TYPE_QUAD,
>> +	IF_TYPE_QIO,
>> +};
>> +
>> +struct hifmc_priv {
>> +	int chipselect;
> 
> Can chipselect be set to < 0 values ?
> 
The type will be changed to u32.

>> +	u32 clkrate;
>> +	struct hifmc_host *host;
>> +};
>> +
>> +#define HIFMC_MAX_CHIP_NUM		2
> 
> This does not scale very well, use dynamic allocation.
> 
OK. Dynamic allocation will be used in next version.
>> +struct hifmc_host {
>> +	struct device *dev;
>> +	struct mutex lock;
>> +
>> +	void __iomem *regbase;
>> +	void __iomem *iobase;
>> +	struct clk *clk;
>> +	void *buffer;
>> +	dma_addr_t dma_buffer;
>> +
>> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
>> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
>> +	int num_chip;
>> +};
>> +
>> +static inline int wait_op_finish(struct hifmc_host *host)
>> +{
>> +	unsigned int reg;
>> +
>> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
>> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
>> +}
>> +
>> +static int get_if_type(enum read_mode flash_read)
>> +{
>> +	enum hifmc_iftype if_type;
>> +
>> +	switch (flash_read) {
>> +	case SPI_NOR_DUAL:
>> +		if_type = IF_TYPE_DUAL;
>> +		break;
>> +	case SPI_NOR_QUAD:
>> +		if_type = IF_TYPE_QUAD;
>> +		break;
>> +	case SPI_NOR_NORMAL:
>> +	case SPI_NOR_FAST:
>> +	default:
>> +		if_type = IF_TYPE_STD;
>> +		break;
>> +	}
>> +
>> +	return if_type;
>> +}
>> +
>> +static void hisi_spi_nor_init(struct hifmc_host *host)
>> +{
>> +	unsigned int reg;
> 
> Should be u32 here.
> 
OK.
>> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
>> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
>> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
>> +}
>> +
>> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +	int ret;
>> +
>> +	mutex_lock(&host->lock);
> 
> Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
> locks a mutex in struct spi_nor , but that's not enough if you have
> multiple SPI NORs on the same bus, because concurrent access to multiple
> SPI NOR flashes needs locking on the controller level, right ?
> 
Yes, you are quite right. The controller can connect with two SPI NOR flashes
on one board. This lock is used on the controller level.

>> +	ret = clk_set_rate(host->clk, priv->clkrate);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = clk_prepare_enable(host->clk);
>> +	if (ret)
>> +		goto out;
>> +
>> +	return 0;
>> +
>> +out:
>> +	mutex_unlock(&host->lock);
>> +	return ret;
>> +}
>> +
>> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +
>> +	clk_disable_unprepare(host->clk);
>> +	mutex_unlock(&host->lock);
>> +}
>> +
>> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
>> +		u32 *opcfg)
>> +{
>> +	u32 reg;
>> +
>> +	*opcfg |= FMC_OP_CMD1_EN;
>> +	switch (cmd) {
>> +	case SPINOR_OP_RDID:
>> +	case SPINOR_OP_RDSR:
>> +	case SPINOR_OP_RDCR:
>> +		*opcfg |= FMC_OP_READ_DATA_EN;
>> +		break;
>> +	case SPINOR_OP_WREN:
>> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
>> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
>> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
>> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
>> +		}
>> +		break;
>> +	case SPINOR_OP_WRSR:
>> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
>> +		break;
>> +	case SPINOR_OP_BE_4K:
>> +	case SPINOR_OP_BE_4K_PMC:
>> +	case SPINOR_OP_SE_4B:
>> +	case SPINOR_OP_SE:
>> +		*opcfg |= FMC_OP_ADDR_EN;
>> +		break;
>> +	case SPINOR_OP_EN4B:
>> +		reg = readl(host->regbase + FMC_CFG);
>> +		reg |= SPI_NOR_ADDR_MODE;
>> +		writel(reg, host->regbase + FMC_CFG);
>> +		break;
>> +	case SPINOR_OP_EX4B:
>> +		reg = readl(host->regbase + FMC_CFG);
>> +		reg &= ~SPI_NOR_ADDR_MODE;
>> +		writel(reg, host->regbase + FMC_CFG);
>> +		break;
>> +	case SPINOR_OP_CHIP_ERASE:
>> +	default:
>> +		break;
>> +	}
> 
> Won't the driver fail if we add new instructions into the SPI NOR core
> which are not handled by this huge switch statement ?
> 
Only some of commands are needed to process in this stage for the controller.
Others have no needs. So this function won't return failure.

I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.

>> +}
> 
> [...]
> 
>> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>> +			size_t len, size_t *retlen, const u_char *write_buf)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +	const unsigned char *ptr = write_buf;
>> +	size_t actual_len;
>> +
>> +	*retlen = 0;
>> +	while (len > 0) {
>> +		if (to & HIFMC_DMA_MASK)
>> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>> +				>= len	? len
>> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> 
> Rewrite this as something like the following snippet, for the sake of
> readability:
> 
> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
> if (actual_len >= len)
>    actual_len = len;
> 
OK. Thank you.
>> +		else
>> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>> +				? HIFMC_DMA_MAX_LEN : len;
>> +		memcpy(host->buffer, ptr, actual_len);
>> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>> +				FMC_OP_WRITE);
>> +		to += actual_len;
>> +		ptr += actual_len;
>> +		len -= actual_len;
>> +		*retlen += actual_len;
>> +	}
>> +}
>> +
>> +static int hisi_spi_nor_erase(struct spi_nor *nor, loff_t offs)
>> +{
>> +	struct hifmc_priv *priv = nor->priv;
>> +	struct hifmc_host *host = priv->host;
>> +
>> +	writel(offs, host->regbase + FMC_ADDRL);
>> +
>> +	return hisi_spi_nor_send_cmd(nor, nor->erase_opcode, 0);
>> +}
>> +
>> +static int hisi_spi_nor_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	struct hifmc_host *host;
>> +	struct device_node *np;
>> +	int ret, i = 0;
>> +
>> +	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
>> +	if (!host)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, host);
>> +	host->dev = dev;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
>> +	host->regbase = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(host->regbase))
>> +		return PTR_ERR(host->regbase);
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "memory");
>> +	host->iobase = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(host->iobase))
>> +		return PTR_ERR(host->iobase);
>> +
>> +	host->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(host->clk))
>> +		return PTR_ERR(host->clk);
>> +
>> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> +	if (ret) {
>> +		dev_warn(dev, "Unable to set dma mask\n");
>> +		return ret;
>> +	}
>> +
>> +	host->buffer = dmam_alloc_coherent(dev, HIFMC_DMA_MAX_LEN,
>> +			&host->dma_buffer, GFP_KERNEL);
>> +	if (!host->buffer)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&host->lock);
>> +	clk_prepare_enable(host->clk);
>> +	hisi_spi_nor_init(host);
>> +
>> +	for_each_available_child_of_node(dev->of_node, np) {
>> +		struct spi_nor *nor = &host->nor[i];
>> +		struct hifmc_priv *priv = &host->priv[i];
>> +		struct mtd_info *mtd = &nor->mtd;
>> +
>> +		mtd->name = np->name;
>> +		nor->dev = dev;
>> +		spi_nor_set_flash_node(nor, np);
>> +		ret = of_property_read_u32(np, "reg", &priv->chipselect);
>> +		if (ret)
>> +			goto fail;
>> +		ret = of_property_read_u32(np, "spi-max-frequency",
>> +				&priv->clkrate);
>> +		if (ret)
>> +			goto fail;
>> +		priv->host = host;
>> +		nor->priv = priv;
>> +
>> +		nor->prepare = hisi_spi_nor_prep;
>> +		nor->unprepare = hisi_spi_nor_unprep;
>> +		nor->read_reg = hisi_spi_nor_read_reg;
>> +		nor->write_reg = hisi_spi_nor_write_reg;
>> +		nor->read = hisi_spi_nor_read;
>> +		nor->write = hisi_spi_nor_write;
>> +		nor->erase = hisi_spi_nor_erase;
>> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>> +		if (ret)
>> +			goto fail;
>> +
>> +		ret = mtd_device_register(mtd, NULL, 0);
>> +		if (ret)
>> +			goto fail;
>> +
>> +		i++;
>> +		host->num_chip++;
>> +		if (i == HIFMC_MAX_CHIP_NUM) {
>> +			dev_warn(dev, "Flash device number exceeds the maximum chipselect number\n");
>> +			break;
>> +		}
> 
> Please factor this loop out into a separate function, so we can easily
> locate the developing boilerplate.
> 
OK. I'll do it in next version.

>> +	}
>> +
>> +	clk_disable_unprepare(host->clk);
>> +	return 0;
>> +
>> +fail:
>> +	for (i = 0; i < host->num_chip; i++)
>> +		mtd_device_unregister(&host->nor[i].mtd);
> 
> Did you ever exercise this fail path ? I think if you call this without
> registering all of the MTD devices, it will crash, but I might be wrong
> on this one.
> 
Yes. Actually the host->num_chip means the number of successfully registered mtd devices.
I'll change the name to host->actual_chip_num to make it more readable.

>> +	clk_disable_unprepare(host->clk);
>> +	mutex_destroy(&host->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int hisi_spi_nor_remove(struct platform_device *pdev)
>> +{
>> +	struct hifmc_host *host = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	for (i = 0; i < host->num_chip; i++)
>> +		mtd_device_unregister(&host->nor[i].mtd);
>> +
>> +	clk_disable_unprepare(host->clk);
>> +	mutex_destroy(&host->lock);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id hisi_spi_nor_dt_ids[] = {
>> +	{ .compatible = "hisilicon,fmc-spi-nor"},
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, hisi_spi_nor_dt_ids);
>> +
>> +static struct platform_driver hisi_spi_nor_driver = {
>> +	.driver = {
>> +		.name	= "hisi-sfc",
>> +		.of_match_table = hisi_spi_nor_dt_ids,
>> +	},
>> +	.probe	= hisi_spi_nor_probe,
>> +	.remove	= hisi_spi_nor_remove,
>> +};
>> +module_platform_driver(hisi_spi_nor_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("HiSilicon SPI Nor Flash Controller Driver");
>>
> 
> btw I also trimmed down the crazy CC list.
> 
Sorry about that and thank you again!

Regards,
Jiancheng

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi all,
    I'll highly appreciated any of your comments.

On 2016/3/26 16:11, Jiancheng Xue wrote:
> Add hisilicon spi-nor flash controller driver
> 
[...]
> +static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
> +		size_t *retlen, u_char *read_buf)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +	unsigned char *ptr = read_buf;
> +	size_t actual_len;
> +
> +	*retlen = 0;
> +	while (len > 0) {
> +		actual_len = (len >= HIFMC_DMA_MAX_LEN)
> +			? HIFMC_DMA_MAX_LEN : len;
> +		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
> +				actual_len, FMC_OP_READ);
> +		memcpy(ptr, host->buffer, actual_len);
> +		ptr += actual_len;
> +		from += actual_len;
> +		len -= actual_len;
> +		*retlen += actual_len;
> +	}
> +
> +	return 0;
> +}
For easy understanding, the read function will be changed like below:

static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
		size_t *retlen, u_char *read_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;
	int i;

	/* read all bytes in only one time */
	if (len <= HIFMC_DMA_MAX_LEN) {
		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
				len, FMC_OP_READ);
		memcpy(read_buf, host->buffer, len);
	} else {
		/* read HIFMC_DMA_MAX_LEN bytes at a time */
		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
		}
		/* read remaining bytes */
		i -= HIFMC_DMA_MAX_LEN;
		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				len - i, FMC_OP_READ);
		memcpy(read_buf + i, host->buffer, len - i);
	}
	*retlen = len;

	return 0;
}

> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
> +			size_t len, size_t *retlen, const u_char *write_buf)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +	const unsigned char *ptr = write_buf;
> +	size_t actual_len;
> +
> +	*retlen = 0;
> +	while (len > 0) {
> +		if (to & HIFMC_DMA_MASK)
> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> +				>= len	? len
> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> +		else
> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
> +				? HIFMC_DMA_MAX_LEN : len;
> +		memcpy(host->buffer, ptr, actual_len);
> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
> +				FMC_OP_WRITE);
> +		to += actual_len;
> +		ptr += actual_len;
> +		len -= actual_len;
> +		*retlen += actual_len;
> +	}
> +}
> +
Because "len" passed from spi_nor_write is smaller than nor->page_size, and nor->page_size
is smaller than the length of host->dma_buffer. We can transfer "len" bytes data by
hisi_spi_nor_dma_transfer at one time. hisi_spi_nor_write can be simplified like below:

static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
			size_t len, size_t *retlen, const u_char *write_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;

	/* len is smaller than dma buffer length*/
	memcpy(host->buffer, write_buf, len);
	hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, len,
					FMC_OP_WRITE);

	*retlen = len;
}

Regards,
Jiancheng

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi all,
    I'll highly appreciated any of your comments.

On 2016/3/26 16:11, Jiancheng Xue wrote:
> Add hisilicon spi-nor flash controller driver
> 
[...]
> +static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
> +		size_t *retlen, u_char *read_buf)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +	unsigned char *ptr = read_buf;
> +	size_t actual_len;
> +
> +	*retlen = 0;
> +	while (len > 0) {
> +		actual_len = (len >= HIFMC_DMA_MAX_LEN)
> +			? HIFMC_DMA_MAX_LEN : len;
> +		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
> +				actual_len, FMC_OP_READ);
> +		memcpy(ptr, host->buffer, actual_len);
> +		ptr += actual_len;
> +		from += actual_len;
> +		len -= actual_len;
> +		*retlen += actual_len;
> +	}
> +
> +	return 0;
> +}
For easy understanding, the read function will be changed like below:

static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
		size_t *retlen, u_char *read_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;
	int i;

	/* read all bytes in only one time */
	if (len <= HIFMC_DMA_MAX_LEN) {
		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
				len, FMC_OP_READ);
		memcpy(read_buf, host->buffer, len);
	} else {
		/* read HIFMC_DMA_MAX_LEN bytes at a time */
		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
		}
		/* read remaining bytes */
		i -= HIFMC_DMA_MAX_LEN;
		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				len - i, FMC_OP_READ);
		memcpy(read_buf + i, host->buffer, len - i);
	}
	*retlen = len;

	return 0;
}

> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
> +			size_t len, size_t *retlen, const u_char *write_buf)
> +{
> +	struct hifmc_priv *priv = nor->priv;
> +	struct hifmc_host *host = priv->host;
> +	const unsigned char *ptr = write_buf;
> +	size_t actual_len;
> +
> +	*retlen = 0;
> +	while (len > 0) {
> +		if (to & HIFMC_DMA_MASK)
> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> +				>= len	? len
> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> +		else
> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
> +				? HIFMC_DMA_MAX_LEN : len;
> +		memcpy(host->buffer, ptr, actual_len);
> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
> +				FMC_OP_WRITE);
> +		to += actual_len;
> +		ptr += actual_len;
> +		len -= actual_len;
> +		*retlen += actual_len;
> +	}
> +}
> +
Because "len" passed from spi_nor_write is smaller than nor->page_size, and nor->page_size
is smaller than the length of host->dma_buffer. We can transfer "len" bytes data by
hisi_spi_nor_dma_transfer at one time. hisi_spi_nor_write can be simplified like below:

static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
			size_t len, size_t *retlen, const u_char *write_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;

	/* len is smaller than dma buffer length*/
	memcpy(host->buffer, write_buf, len);
	hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, len,
					FMC_OP_WRITE);

	*retlen = len;
}

Regards,
Jiancheng





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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Brian Norris]

Hi Jiancheng,

Looking good. In addition to Marek's comments, I have just a few small
comments. I'll post a small diff at the end, and a few inline comments.

On Mon, Mar 28, 2016 at 05:15:28PM +0800, Jiancheng Xue wrote:
> Hi Marek,
>     Firstly, thank you very much for your comments.
> 
> On 2016/3/27 9:47, Marek Vasut wrote:
> > On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
> >> Add hisilicon spi-nor flash controller driver
> >>
> >> Signed-off-by: Binquan Peng <pengbinquan@huawei.com>
> >> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
> >> Acked-by: Rob Herring <robh@kernel.org>
> >> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> >> Reviewed-by: Jagan Teki <jteki@openedev.com>
> >> ---
> >> change log
> >> v9:
> >> Fixed issues pointed by Jagan Teki.
> > 
> > It'd be really great if you could list which exact issues you fixed.
> > 
> 
> OK. I'll describe the history log more detailed in next version.
> 
> >> v8:
> >> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
> >> Moved dts binding file to mtd directory.
> >> Changed the compatible string more specific.
> > 
> > [...]

^^ You were using <linux/of_device.h> in here, even though you don't
need any of its contents. You just wanted <linux/of.h> and
<linux/platform_device.h>.

> > 
> >> +/* Hardware register offsets and field definitions */
> >> +#define FMC_CFG				0x00
> >> +#define SPI_NOR_ADDR_MODE		BIT(10)
> >> +#define FMC_GLOBAL_CFG			0x04
> >> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
> >> +#define FMC_SPI_TIMING_CFG		0x08
> >> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
> >> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
> >> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
> >> +#define CS_HOLD_TIME			0x6
> >> +#define CS_SETUP_TIME			0x6
> >> +#define CS_DESELECT_TIME		0xf
> >> +#define FMC_INT				0x18
> >> +#define FMC_INT_OP_DONE			BIT(0)
> >> +#define FMC_INT_CLR			0x20
> >> +#define FMC_CMD				0x24
> >> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
> > 
> > Drop the underscores in the argument names.
> > 
> OK.
> >> +#define FMC_ADDRL			0x2c
> >> +#define FMC_OP_CFG			0x30
> >> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
> >> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
> >> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
> >> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
> >> +#define FMC_DATA_NUM			0x38
> >> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
> >> +#define FMC_OP				0x3c
> >> +#define FMC_OP_DUMMY_EN			BIT(8)
> >> +#define FMC_OP_CMD1_EN			BIT(7)
> >> +#define FMC_OP_ADDR_EN			BIT(6)
> >> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
> >> +#define FMC_OP_READ_DATA_EN		BIT(2)
> >> +#define FMC_OP_READ_STATUS_EN		BIT(1)
> >> +#define FMC_OP_REG_OP_START		BIT(0)
> > 
> > [...]
> > 
> >> +enum hifmc_iftype {
> >> +	IF_TYPE_STD,
> >> +	IF_TYPE_DUAL,
> >> +	IF_TYPE_DIO,
> >> +	IF_TYPE_QUAD,
> >> +	IF_TYPE_QIO,
> >> +};
> >> +
> >> +struct hifmc_priv {
> >> +	int chipselect;
> > 
> > Can chipselect be set to < 0 values ?
> > 
> The type will be changed to u32.
> 
> >> +	u32 clkrate;
> >> +	struct hifmc_host *host;
> >> +};
> >> +
> >> +#define HIFMC_MAX_CHIP_NUM		2
> > 
> > This does not scale very well, use dynamic allocation.
> > 
> OK. Dynamic allocation will be used in next version.
> >> +struct hifmc_host {
> >> +	struct device *dev;
> >> +	struct mutex lock;
> >> +
> >> +	void __iomem *regbase;
> >> +	void __iomem *iobase;
> >> +	struct clk *clk;
> >> +	void *buffer;
> >> +	dma_addr_t dma_buffer;
> >> +
> >> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
> >> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
> >> +	int num_chip;
> >> +};
> >> +
> >> +static inline int wait_op_finish(struct hifmc_host *host)
> >> +{
> >> +	unsigned int reg;
> >> +
> >> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
> >> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
> >> +}
> >> +
> >> +static int get_if_type(enum read_mode flash_read)
> >> +{
> >> +	enum hifmc_iftype if_type;
> >> +
> >> +	switch (flash_read) {
> >> +	case SPI_NOR_DUAL:
> >> +		if_type = IF_TYPE_DUAL;
> >> +		break;
> >> +	case SPI_NOR_QUAD:
> >> +		if_type = IF_TYPE_QUAD;
> >> +		break;
> >> +	case SPI_NOR_NORMAL:
> >> +	case SPI_NOR_FAST:
> >> +	default:
> >> +		if_type = IF_TYPE_STD;
> >> +		break;
> >> +	}
> >> +
> >> +	return if_type;
> >> +}
> >> +
> >> +static void hisi_spi_nor_init(struct hifmc_host *host)
> >> +{
> >> +	unsigned int reg;
> > 
> > Should be u32 here.
> > 
> OK.
> >> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
> >> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
> >> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
> >> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
> >> +}
> >> +
> >> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> >> +{
> >> +	struct hifmc_priv *priv = nor->priv;
> >> +	struct hifmc_host *host = priv->host;
> >> +	int ret;
> >> +
> >> +	mutex_lock(&host->lock);
> > 
> > Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
> > locks a mutex in struct spi_nor , but that's not enough if you have
> > multiple SPI NORs on the same bus, because concurrent access to multiple
> > SPI NOR flashes needs locking on the controller level, right ?
> > 
> Yes, you are quite right. The controller can connect with two SPI NOR flashes
> on one board. This lock is used on the controller level.

Yeah... we should probably implement some common controller logic in the
core eventually. But the mutex is necessary for now.

> >> +	ret = clk_set_rate(host->clk, priv->clkrate);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	ret = clk_prepare_enable(host->clk);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	return 0;
> >> +
> >> +out:
> >> +	mutex_unlock(&host->lock);
> >> +	return ret;
> >> +}
> >> +
> >> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
> >> +{
> >> +	struct hifmc_priv *priv = nor->priv;
> >> +	struct hifmc_host *host = priv->host;
> >> +
> >> +	clk_disable_unprepare(host->clk);
> >> +	mutex_unlock(&host->lock);
> >> +}
> >> +
> >> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
> >> +		u32 *opcfg)
> >> +{
> >> +	u32 reg;
> >> +
> >> +	*opcfg |= FMC_OP_CMD1_EN;
> >> +	switch (cmd) {
> >> +	case SPINOR_OP_RDID:
> >> +	case SPINOR_OP_RDSR:
> >> +	case SPINOR_OP_RDCR:
> >> +		*opcfg |= FMC_OP_READ_DATA_EN;
> >> +		break;
> >> +	case SPINOR_OP_WREN:
> >> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
> >> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
> >> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
> >> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
> >> +		}
> >> +		break;
> >> +	case SPINOR_OP_WRSR:
> >> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
> >> +		break;
> >> +	case SPINOR_OP_BE_4K:
> >> +	case SPINOR_OP_BE_4K_PMC:
> >> +	case SPINOR_OP_SE_4B:
> >> +	case SPINOR_OP_SE:
> >> +		*opcfg |= FMC_OP_ADDR_EN;
> >> +		break;
> >> +	case SPINOR_OP_EN4B:
> >> +		reg = readl(host->regbase + FMC_CFG);
> >> +		reg |= SPI_NOR_ADDR_MODE;
> >> +		writel(reg, host->regbase + FMC_CFG);
> >> +		break;
> >> +	case SPINOR_OP_EX4B:
> >> +		reg = readl(host->regbase + FMC_CFG);
> >> +		reg &= ~SPI_NOR_ADDR_MODE;
> >> +		writel(reg, host->regbase + FMC_CFG);
> >> +		break;
> >> +	case SPINOR_OP_CHIP_ERASE:
> >> +	default:
> >> +		break;
> >> +	}
> > 
> > Won't the driver fail if we add new instructions into the SPI NOR core
> > which are not handled by this huge switch statement ?
> > 
> Only some of commands are needed to process in this stage for the controller.
> Others have no needs. So this function won't return failure.

It's not ideal to have this sort of function if we can avoid it (since
it's hard to figure out how to extend this for new opcodes). But it
looks necessary for now.

> I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.
> 
> >> +}
> > 
> > [...]
> > 
> >> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
> >> +			size_t len, size_t *retlen, const u_char *write_buf)
> >> +{
> >> +	struct hifmc_priv *priv = nor->priv;
> >> +	struct hifmc_host *host = priv->host;
> >> +	const unsigned char *ptr = write_buf;
> >> +	size_t actual_len;
> >> +
> >> +	*retlen = 0;
> >> +	while (len > 0) {
> >> +		if (to & HIFMC_DMA_MASK)
> >> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> >> +				>= len	? len
> >> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> > 
> > Rewrite this as something like the following snippet, for the sake of
> > readability:
> > 
> > actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
> > if (actual_len >= len)
> >    actual_len = len;
> > 
> OK. Thank you.
> >> +		else
> >> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
> >> +				? HIFMC_DMA_MAX_LEN : len;

Wait, why do you need the else case? Isn't this just equivalent to the
first case? I'd suggest consolidating these two blocks, and dropping the
?: entirely, since (like Marek said) it's hurting readability. So:

		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
			actual_len = len;
		else
			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);

> >> +		memcpy(host->buffer, ptr, actual_len);
> >> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
> >> +				FMC_OP_WRITE);
> >> +		to += actual_len;
> >> +		ptr += actual_len;
> >> +		len -= actual_len;
> >> +		*retlen += actual_len;
> >> +	}
> >> +}

[...]

Here's my diff:

diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
index e974c92a0a25..0c58fd3b8790 100644
--- a/drivers/mtd/spi-nor/hisi-sfc.c
+++ b/drivers/mtd/spi-nor/hisi-sfc.c
@@ -16,13 +16,15 @@
  * You should have received a copy of the GNU General Public License
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  */
+
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/spi-nor.h>
-#include <linux/of_platform.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 /* Hardware register offsets and field definitions */
@@ -343,13 +345,11 @@ static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
 
 	*retlen = 0;
 	while (len > 0) {
-		if (to & HIFMC_DMA_MASK)
-			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
-				>= len	? len
-				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
+		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
+		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
+			actual_len = len;
 		else
-			actual_len = (len >= HIFMC_DMA_MAX_LEN)
-				? HIFMC_DMA_MAX_LEN : len;
+			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
 		memcpy(host->buffer, ptr, actual_len);
 		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
 				FMC_OP_WRITE);

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Brian Norris]

Hi Jiancheng,

Looking good. In addition to Marek's comments, I have just a few small
comments. I'll post a small diff at the end, and a few inline comments.

On Mon, Mar 28, 2016 at 05:15:28PM +0800, Jiancheng Xue wrote:
> Hi Marek,
>     Firstly, thank you very much for your comments.
> 
> On 2016/3/27 9:47, Marek Vasut wrote:
> > On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
> >> Add hisilicon spi-nor flash controller driver
> >>
> >> Signed-off-by: Binquan Peng <pengbinquan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Jiancheng Xue <xuejiancheng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> Reviewed-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
> >> Reviewed-by: Jagan Teki <jteki-oRp2ZoJdM/RWk0Htik3J/w@public.gmane.org>
> >> ---
> >> change log
> >> v9:
> >> Fixed issues pointed by Jagan Teki.
> > 
> > It'd be really great if you could list which exact issues you fixed.
> > 
> 
> OK. I'll describe the history log more detailed in next version.
> 
> >> v8:
> >> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
> >> Moved dts binding file to mtd directory.
> >> Changed the compatible string more specific.
> > 
> > [...]

^^ You were using <linux/of_device.h> in here, even though you don't
need any of its contents. You just wanted <linux/of.h> and
<linux/platform_device.h>.

> > 
> >> +/* Hardware register offsets and field definitions */
> >> +#define FMC_CFG				0x00
> >> +#define SPI_NOR_ADDR_MODE		BIT(10)
> >> +#define FMC_GLOBAL_CFG			0x04
> >> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
> >> +#define FMC_SPI_TIMING_CFG		0x08
> >> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
> >> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
> >> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
> >> +#define CS_HOLD_TIME			0x6
> >> +#define CS_SETUP_TIME			0x6
> >> +#define CS_DESELECT_TIME		0xf
> >> +#define FMC_INT				0x18
> >> +#define FMC_INT_OP_DONE			BIT(0)
> >> +#define FMC_INT_CLR			0x20
> >> +#define FMC_CMD				0x24
> >> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
> > 
> > Drop the underscores in the argument names.
> > 
> OK.
> >> +#define FMC_ADDRL			0x2c
> >> +#define FMC_OP_CFG			0x30
> >> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
> >> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
> >> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
> >> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
> >> +#define FMC_DATA_NUM			0x38
> >> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
> >> +#define FMC_OP				0x3c
> >> +#define FMC_OP_DUMMY_EN			BIT(8)
> >> +#define FMC_OP_CMD1_EN			BIT(7)
> >> +#define FMC_OP_ADDR_EN			BIT(6)
> >> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
> >> +#define FMC_OP_READ_DATA_EN		BIT(2)
> >> +#define FMC_OP_READ_STATUS_EN		BIT(1)
> >> +#define FMC_OP_REG_OP_START		BIT(0)
> > 
> > [...]
> > 
> >> +enum hifmc_iftype {
> >> +	IF_TYPE_STD,
> >> +	IF_TYPE_DUAL,
> >> +	IF_TYPE_DIO,
> >> +	IF_TYPE_QUAD,
> >> +	IF_TYPE_QIO,
> >> +};
> >> +
> >> +struct hifmc_priv {
> >> +	int chipselect;
> > 
> > Can chipselect be set to < 0 values ?
> > 
> The type will be changed to u32.
> 
> >> +	u32 clkrate;
> >> +	struct hifmc_host *host;
> >> +};
> >> +
> >> +#define HIFMC_MAX_CHIP_NUM		2
> > 
> > This does not scale very well, use dynamic allocation.
> > 
> OK. Dynamic allocation will be used in next version.
> >> +struct hifmc_host {
> >> +	struct device *dev;
> >> +	struct mutex lock;
> >> +
> >> +	void __iomem *regbase;
> >> +	void __iomem *iobase;
> >> +	struct clk *clk;
> >> +	void *buffer;
> >> +	dma_addr_t dma_buffer;
> >> +
> >> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
> >> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
> >> +	int num_chip;
> >> +};
> >> +
> >> +static inline int wait_op_finish(struct hifmc_host *host)
> >> +{
> >> +	unsigned int reg;
> >> +
> >> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
> >> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
> >> +}
> >> +
> >> +static int get_if_type(enum read_mode flash_read)
> >> +{
> >> +	enum hifmc_iftype if_type;
> >> +
> >> +	switch (flash_read) {
> >> +	case SPI_NOR_DUAL:
> >> +		if_type = IF_TYPE_DUAL;
> >> +		break;
> >> +	case SPI_NOR_QUAD:
> >> +		if_type = IF_TYPE_QUAD;
> >> +		break;
> >> +	case SPI_NOR_NORMAL:
> >> +	case SPI_NOR_FAST:
> >> +	default:
> >> +		if_type = IF_TYPE_STD;
> >> +		break;
> >> +	}
> >> +
> >> +	return if_type;
> >> +}
> >> +
> >> +static void hisi_spi_nor_init(struct hifmc_host *host)
> >> +{
> >> +	unsigned int reg;
> > 
> > Should be u32 here.
> > 
> OK.
> >> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
> >> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
> >> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
> >> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
> >> +}
> >> +
> >> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> >> +{
> >> +	struct hifmc_priv *priv = nor->priv;
> >> +	struct hifmc_host *host = priv->host;
> >> +	int ret;
> >> +
> >> +	mutex_lock(&host->lock);
> > 
> > Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
> > locks a mutex in struct spi_nor , but that's not enough if you have
> > multiple SPI NORs on the same bus, because concurrent access to multiple
> > SPI NOR flashes needs locking on the controller level, right ?
> > 
> Yes, you are quite right. The controller can connect with two SPI NOR flashes
> on one board. This lock is used on the controller level.

Yeah... we should probably implement some common controller logic in the
core eventually. But the mutex is necessary for now.

> >> +	ret = clk_set_rate(host->clk, priv->clkrate);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	ret = clk_prepare_enable(host->clk);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	return 0;
> >> +
> >> +out:
> >> +	mutex_unlock(&host->lock);
> >> +	return ret;
> >> +}
> >> +
> >> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
> >> +{
> >> +	struct hifmc_priv *priv = nor->priv;
> >> +	struct hifmc_host *host = priv->host;
> >> +
> >> +	clk_disable_unprepare(host->clk);
> >> +	mutex_unlock(&host->lock);
> >> +}
> >> +
> >> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
> >> +		u32 *opcfg)
> >> +{
> >> +	u32 reg;
> >> +
> >> +	*opcfg |= FMC_OP_CMD1_EN;
> >> +	switch (cmd) {
> >> +	case SPINOR_OP_RDID:
> >> +	case SPINOR_OP_RDSR:
> >> +	case SPINOR_OP_RDCR:
> >> +		*opcfg |= FMC_OP_READ_DATA_EN;
> >> +		break;
> >> +	case SPINOR_OP_WREN:
> >> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
> >> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
> >> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
> >> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
> >> +		}
> >> +		break;
> >> +	case SPINOR_OP_WRSR:
> >> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
> >> +		break;
> >> +	case SPINOR_OP_BE_4K:
> >> +	case SPINOR_OP_BE_4K_PMC:
> >> +	case SPINOR_OP_SE_4B:
> >> +	case SPINOR_OP_SE:
> >> +		*opcfg |= FMC_OP_ADDR_EN;
> >> +		break;
> >> +	case SPINOR_OP_EN4B:
> >> +		reg = readl(host->regbase + FMC_CFG);
> >> +		reg |= SPI_NOR_ADDR_MODE;
> >> +		writel(reg, host->regbase + FMC_CFG);
> >> +		break;
> >> +	case SPINOR_OP_EX4B:
> >> +		reg = readl(host->regbase + FMC_CFG);
> >> +		reg &= ~SPI_NOR_ADDR_MODE;
> >> +		writel(reg, host->regbase + FMC_CFG);
> >> +		break;
> >> +	case SPINOR_OP_CHIP_ERASE:
> >> +	default:
> >> +		break;
> >> +	}
> > 
> > Won't the driver fail if we add new instructions into the SPI NOR core
> > which are not handled by this huge switch statement ?
> > 
> Only some of commands are needed to process in this stage for the controller.
> Others have no needs. So this function won't return failure.

It's not ideal to have this sort of function if we can avoid it (since
it's hard to figure out how to extend this for new opcodes). But it
looks necessary for now.

> I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.
> 
> >> +}
> > 
> > [...]
> > 
> >> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
> >> +			size_t len, size_t *retlen, const u_char *write_buf)
> >> +{
> >> +	struct hifmc_priv *priv = nor->priv;
> >> +	struct hifmc_host *host = priv->host;
> >> +	const unsigned char *ptr = write_buf;
> >> +	size_t actual_len;
> >> +
> >> +	*retlen = 0;
> >> +	while (len > 0) {
> >> +		if (to & HIFMC_DMA_MASK)
> >> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> >> +				>= len	? len
> >> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> > 
> > Rewrite this as something like the following snippet, for the sake of
> > readability:
> > 
> > actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
> > if (actual_len >= len)
> >    actual_len = len;
> > 
> OK. Thank you.
> >> +		else
> >> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
> >> +				? HIFMC_DMA_MAX_LEN : len;

Wait, why do you need the else case? Isn't this just equivalent to the
first case? I'd suggest consolidating these two blocks, and dropping the
?: entirely, since (like Marek said) it's hurting readability. So:

		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
			actual_len = len;
		else
			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);

> >> +		memcpy(host->buffer, ptr, actual_len);
> >> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
> >> +				FMC_OP_WRITE);
> >> +		to += actual_len;
> >> +		ptr += actual_len;
> >> +		len -= actual_len;
> >> +		*retlen += actual_len;
> >> +	}
> >> +}

[...]

Here's my diff:

diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
index e974c92a0a25..0c58fd3b8790 100644
--- a/drivers/mtd/spi-nor/hisi-sfc.c
+++ b/drivers/mtd/spi-nor/hisi-sfc.c
@@ -16,13 +16,15 @@
  * You should have received a copy of the GNU General Public License
  * along with this program. If not, see <http://www.gnu.org/licenses/>.
  */
+
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/spi-nor.h>
-#include <linux/of_platform.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 /* Hardware register offsets and field definitions */
@@ -343,13 +345,11 @@ static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
 
 	*retlen = 0;
 	while (len > 0) {
-		if (to & HIFMC_DMA_MASK)
-			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
-				>= len	? len
-				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
+		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
+		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
+			actual_len = len;
 		else
-			actual_len = (len >= HIFMC_DMA_MAX_LEN)
-				? HIFMC_DMA_MAX_LEN : len;
+			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
 		memcpy(host->buffer, ptr, actual_len);
 		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
 				FMC_OP_WRITE);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Brian,
   Thank you very much for your comments. I'll fix these issues in next version.
In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.

static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
		size_t *retlen, u_char *read_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;
	int i;

	/* read all bytes in only one time */
	if (len <= HIFMC_DMA_MAX_LEN) {
		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
				len, FMC_OP_READ);
		memcpy(read_buf, host->buffer, len);
	} else {
		/* read HIFMC_DMA_MAX_LEN bytes at a time */
		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
		}
		/* read remaining bytes */
		i -= HIFMC_DMA_MAX_LEN;
		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				len - i, FMC_OP_READ);
		memcpy(read_buf + i, host->buffer, len - i);
	}
	*retlen = len;

	return 0;
}

Because "len" passed from spi_nor_write is smaller than nor->page_size, and nor->page_size
is smaller than the length of host->dma_buffer. We can transfer "len" bytes data by
hisi_spi_nor_dma_transfer at one time. hisi_spi_nor_write can be simplified like below:

static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
			size_t len, size_t *retlen, const u_char *write_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;

	/* len is smaller than dma buffer length*/
	memcpy(host->buffer, write_buf, len);
	hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, len,
					FMC_OP_WRITE);

	*retlen = len;
}

Regards,
Jiancheng

On 2016/4/4 14:44, Brian Norris wrote:
> Hi Jiancheng,
> 
> Looking good. In addition to Marek's comments, I have just a few small
> comments. I'll post a small diff at the end, and a few inline comments.
> 
> On Mon, Mar 28, 2016 at 05:15:28PM +0800, Jiancheng Xue wrote:
>> Hi Marek,
>>     Firstly, thank you very much for your comments.
>>
>> On 2016/3/27 9:47, Marek Vasut wrote:
>>> On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
>>>> Add hisilicon spi-nor flash controller driver
>>>>
>>>> Signed-off-by: Binquan Peng <pengbinquan@huawei.com>
>>>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>> Reviewed-by: Jagan Teki <jteki@openedev.com>
>>>> ---
>>>> change log
>>>> v9:
>>>> Fixed issues pointed by Jagan Teki.
>>>
>>> It'd be really great if you could list which exact issues you fixed.
>>>
>>
>> OK. I'll describe the history log more detailed in next version.
>>
>>>> v8:
>>>> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
>>>> Moved dts binding file to mtd directory.
>>>> Changed the compatible string more specific.
>>>
>>> [...]
> 
> ^^ You were using <linux/of_device.h> in here, even though you don't
> need any of its contents. You just wanted <linux/of.h> and
> <linux/platform_device.h>.
> 
>>>
>>>> +/* Hardware register offsets and field definitions */
>>>> +#define FMC_CFG				0x00
>>>> +#define SPI_NOR_ADDR_MODE		BIT(10)
>>>> +#define FMC_GLOBAL_CFG			0x04
>>>> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
>>>> +#define FMC_SPI_TIMING_CFG		0x08
>>>> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
>>>> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
>>>> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
>>>> +#define CS_HOLD_TIME			0x6
>>>> +#define CS_SETUP_TIME			0x6
>>>> +#define CS_DESELECT_TIME		0xf
>>>> +#define FMC_INT				0x18
>>>> +#define FMC_INT_OP_DONE			BIT(0)
>>>> +#define FMC_INT_CLR			0x20
>>>> +#define FMC_CMD				0x24
>>>> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
>>>
>>> Drop the underscores in the argument names.
>>>
>> OK.
>>>> +#define FMC_ADDRL			0x2c
>>>> +#define FMC_OP_CFG			0x30
>>>> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
>>>> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
>>>> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
>>>> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
>>>> +#define FMC_DATA_NUM			0x38
>>>> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
>>>> +#define FMC_OP				0x3c
>>>> +#define FMC_OP_DUMMY_EN			BIT(8)
>>>> +#define FMC_OP_CMD1_EN			BIT(7)
>>>> +#define FMC_OP_ADDR_EN			BIT(6)
>>>> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
>>>> +#define FMC_OP_READ_DATA_EN		BIT(2)
>>>> +#define FMC_OP_READ_STATUS_EN		BIT(1)
>>>> +#define FMC_OP_REG_OP_START		BIT(0)
>>>
>>> [...]
>>>
>>>> +enum hifmc_iftype {
>>>> +	IF_TYPE_STD,
>>>> +	IF_TYPE_DUAL,
>>>> +	IF_TYPE_DIO,
>>>> +	IF_TYPE_QUAD,
>>>> +	IF_TYPE_QIO,
>>>> +};
>>>> +
>>>> +struct hifmc_priv {
>>>> +	int chipselect;
>>>
>>> Can chipselect be set to < 0 values ?
>>>
>> The type will be changed to u32.
>>
>>>> +	u32 clkrate;
>>>> +	struct hifmc_host *host;
>>>> +};
>>>> +
>>>> +#define HIFMC_MAX_CHIP_NUM		2
>>>
>>> This does not scale very well, use dynamic allocation.
>>>
>> OK. Dynamic allocation will be used in next version.
>>>> +struct hifmc_host {
>>>> +	struct device *dev;
>>>> +	struct mutex lock;
>>>> +
>>>> +	void __iomem *regbase;
>>>> +	void __iomem *iobase;
>>>> +	struct clk *clk;
>>>> +	void *buffer;
>>>> +	dma_addr_t dma_buffer;
>>>> +
>>>> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
>>>> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
>>>> +	int num_chip;
>>>> +};
>>>> +
>>>> +static inline int wait_op_finish(struct hifmc_host *host)
>>>> +{
>>>> +	unsigned int reg;
>>>> +
>>>> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
>>>> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
>>>> +}
>>>> +
>>>> +static int get_if_type(enum read_mode flash_read)
>>>> +{
>>>> +	enum hifmc_iftype if_type;
>>>> +
>>>> +	switch (flash_read) {
>>>> +	case SPI_NOR_DUAL:
>>>> +		if_type = IF_TYPE_DUAL;
>>>> +		break;
>>>> +	case SPI_NOR_QUAD:
>>>> +		if_type = IF_TYPE_QUAD;
>>>> +		break;
>>>> +	case SPI_NOR_NORMAL:
>>>> +	case SPI_NOR_FAST:
>>>> +	default:
>>>> +		if_type = IF_TYPE_STD;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return if_type;
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_init(struct hifmc_host *host)
>>>> +{
>>>> +	unsigned int reg;
>>>
>>> Should be u32 here.
>>>
>> OK.
>>>> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>>>> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
>>>> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
>>>> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
>>>> +}
>>>> +
>>>> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>> +{
>>>> +	struct hifmc_priv *priv = nor->priv;
>>>> +	struct hifmc_host *host = priv->host;
>>>> +	int ret;
>>>> +
>>>> +	mutex_lock(&host->lock);
>>>
>>> Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
>>> locks a mutex in struct spi_nor , but that's not enough if you have
>>> multiple SPI NORs on the same bus, because concurrent access to multiple
>>> SPI NOR flashes needs locking on the controller level, right ?
>>>
>> Yes, you are quite right. The controller can connect with two SPI NOR flashes
>> on one board. This lock is used on the controller level.
> 
> Yeah... we should probably implement some common controller logic in the
> core eventually. But the mutex is necessary for now.
> 
>>>> +	ret = clk_set_rate(host->clk, priv->clkrate);
>>>> +	if (ret)
>>>> +		goto out;
>>>> +
>>>> +	ret = clk_prepare_enable(host->clk);
>>>> +	if (ret)
>>>> +		goto out;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +out:
>>>> +	mutex_unlock(&host->lock);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>> +{
>>>> +	struct hifmc_priv *priv = nor->priv;
>>>> +	struct hifmc_host *host = priv->host;
>>>> +
>>>> +	clk_disable_unprepare(host->clk);
>>>> +	mutex_unlock(&host->lock);
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
>>>> +		u32 *opcfg)
>>>> +{
>>>> +	u32 reg;
>>>> +
>>>> +	*opcfg |= FMC_OP_CMD1_EN;
>>>> +	switch (cmd) {
>>>> +	case SPINOR_OP_RDID:
>>>> +	case SPINOR_OP_RDSR:
>>>> +	case SPINOR_OP_RDCR:
>>>> +		*opcfg |= FMC_OP_READ_DATA_EN;
>>>> +		break;
>>>> +	case SPINOR_OP_WREN:
>>>> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
>>>> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
>>>> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
>>>> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
>>>> +		}
>>>> +		break;
>>>> +	case SPINOR_OP_WRSR:
>>>> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
>>>> +		break;
>>>> +	case SPINOR_OP_BE_4K:
>>>> +	case SPINOR_OP_BE_4K_PMC:
>>>> +	case SPINOR_OP_SE_4B:
>>>> +	case SPINOR_OP_SE:
>>>> +		*opcfg |= FMC_OP_ADDR_EN;
>>>> +		break;
>>>> +	case SPINOR_OP_EN4B:
>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>> +		reg |= SPI_NOR_ADDR_MODE;
>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>> +		break;
>>>> +	case SPINOR_OP_EX4B:
>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>> +		reg &= ~SPI_NOR_ADDR_MODE;
>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>> +		break;
>>>> +	case SPINOR_OP_CHIP_ERASE:
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>
>>> Won't the driver fail if we add new instructions into the SPI NOR core
>>> which are not handled by this huge switch statement ?
>>>
>> Only some of commands are needed to process in this stage for the controller.
>> Others have no needs. So this function won't return failure.
> 
> It's not ideal to have this sort of function if we can avoid it (since
> it's hard to figure out how to extend this for new opcodes). But it
> looks necessary for now.
> 
>> I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.
>>
>>>> +}
>>>
>>> [...]
>>>
>>>> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>>>> +			size_t len, size_t *retlen, const u_char *write_buf)
>>>> +{
>>>> +	struct hifmc_priv *priv = nor->priv;
>>>> +	struct hifmc_host *host = priv->host;
>>>> +	const unsigned char *ptr = write_buf;
>>>> +	size_t actual_len;
>>>> +
>>>> +	*retlen = 0;
>>>> +	while (len > 0) {
>>>> +		if (to & HIFMC_DMA_MASK)
>>>> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>>>> +				>= len	? len
>>>> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
>>>
>>> Rewrite this as something like the following snippet, for the sake of
>>> readability:
>>>
>>> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>> if (actual_len >= len)
>>>    actual_len = len;
>>>
>> OK. Thank you.
>>>> +		else
>>>> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>>>> +				? HIFMC_DMA_MAX_LEN : len;
> 
> Wait, why do you need the else case? Isn't this just equivalent to the
> first case? I'd suggest consolidating these two blocks, and dropping the
> ?: entirely, since (like Marek said) it's hurting readability. So:
> 
> 		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
> 		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
> 			actual_len = len;
> 		else
> 			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
> 
>>>> +		memcpy(host->buffer, ptr, actual_len);
>>>> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>>>> +				FMC_OP_WRITE);
>>>> +		to += actual_len;
>>>> +		ptr += actual_len;
>>>> +		len -= actual_len;
>>>> +		*retlen += actual_len;
>>>> +	}
>>>> +}
> 
> [...]
> 
> Here's my diff:
> 
> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
> index e974c92a0a25..0c58fd3b8790 100644
> --- a/drivers/mtd/spi-nor/hisi-sfc.c
> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
> @@ -16,13 +16,15 @@
>   * You should have received a copy of the GNU General Public License
>   * along with this program. If not, see <http://www.gnu.org/licenses/>.
>   */
> +
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/spi-nor.h>
> -#include <linux/of_platform.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
>  /* Hardware register offsets and field definitions */
> @@ -343,13 +345,11 @@ static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>  
>  	*retlen = 0;
>  	while (len > 0) {
> -		if (to & HIFMC_DMA_MASK)
> -			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> -				>= len	? len
> -				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> +		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
> +		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
> +			actual_len = len;
>  		else
> -			actual_len = (len >= HIFMC_DMA_MAX_LEN)
> -				? HIFMC_DMA_MAX_LEN : len;
> +			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>  		memcpy(host->buffer, ptr, actual_len);
>  		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>  				FMC_OP_WRITE);
> 
> .
> 

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Brian,
   Thank you very much for your comments. I'll fix these issues in next version.
In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.

static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
		size_t *retlen, u_char *read_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;
	int i;

	/* read all bytes in only one time */
	if (len <= HIFMC_DMA_MAX_LEN) {
		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
				len, FMC_OP_READ);
		memcpy(read_buf, host->buffer, len);
	} else {
		/* read HIFMC_DMA_MAX_LEN bytes at a time */
		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
		}
		/* read remaining bytes */
		i -= HIFMC_DMA_MAX_LEN;
		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
				len - i, FMC_OP_READ);
		memcpy(read_buf + i, host->buffer, len - i);
	}
	*retlen = len;

	return 0;
}

Because "len" passed from spi_nor_write is smaller than nor->page_size, and nor->page_size
is smaller than the length of host->dma_buffer. We can transfer "len" bytes data by
hisi_spi_nor_dma_transfer at one time. hisi_spi_nor_write can be simplified like below:

static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
			size_t len, size_t *retlen, const u_char *write_buf)
{
	struct hifmc_priv *priv = nor->priv;
	struct hifmc_host *host = priv->host;

	/* len is smaller than dma buffer length*/
	memcpy(host->buffer, write_buf, len);
	hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, len,
					FMC_OP_WRITE);

	*retlen = len;
}

Regards,
Jiancheng

On 2016/4/4 14:44, Brian Norris wrote:
> Hi Jiancheng,
> 
> Looking good. In addition to Marek's comments, I have just a few small
> comments. I'll post a small diff at the end, and a few inline comments.
> 
> On Mon, Mar 28, 2016 at 05:15:28PM +0800, Jiancheng Xue wrote:
>> Hi Marek,
>>     Firstly, thank you very much for your comments.
>>
>> On 2016/3/27 9:47, Marek Vasut wrote:
>>> On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
>>>> Add hisilicon spi-nor flash controller driver
>>>>
>>>> Signed-off-by: Binquan Peng <pengbinquan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>> Signed-off-by: Jiancheng Xue <xuejiancheng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>> Reviewed-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
>>>> Reviewed-by: Jagan Teki <jteki-oRp2ZoJdM/RWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>> change log
>>>> v9:
>>>> Fixed issues pointed by Jagan Teki.
>>>
>>> It'd be really great if you could list which exact issues you fixed.
>>>
>>
>> OK. I'll describe the history log more detailed in next version.
>>
>>>> v8:
>>>> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
>>>> Moved dts binding file to mtd directory.
>>>> Changed the compatible string more specific.
>>>
>>> [...]
> 
> ^^ You were using <linux/of_device.h> in here, even though you don't
> need any of its contents. You just wanted <linux/of.h> and
> <linux/platform_device.h>.
> 
>>>
>>>> +/* Hardware register offsets and field definitions */
>>>> +#define FMC_CFG				0x00
>>>> +#define SPI_NOR_ADDR_MODE		BIT(10)
>>>> +#define FMC_GLOBAL_CFG			0x04
>>>> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
>>>> +#define FMC_SPI_TIMING_CFG		0x08
>>>> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
>>>> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
>>>> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
>>>> +#define CS_HOLD_TIME			0x6
>>>> +#define CS_SETUP_TIME			0x6
>>>> +#define CS_DESELECT_TIME		0xf
>>>> +#define FMC_INT				0x18
>>>> +#define FMC_INT_OP_DONE			BIT(0)
>>>> +#define FMC_INT_CLR			0x20
>>>> +#define FMC_CMD				0x24
>>>> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
>>>
>>> Drop the underscores in the argument names.
>>>
>> OK.
>>>> +#define FMC_ADDRL			0x2c
>>>> +#define FMC_OP_CFG			0x30
>>>> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
>>>> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
>>>> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
>>>> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
>>>> +#define FMC_DATA_NUM			0x38
>>>> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
>>>> +#define FMC_OP				0x3c
>>>> +#define FMC_OP_DUMMY_EN			BIT(8)
>>>> +#define FMC_OP_CMD1_EN			BIT(7)
>>>> +#define FMC_OP_ADDR_EN			BIT(6)
>>>> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
>>>> +#define FMC_OP_READ_DATA_EN		BIT(2)
>>>> +#define FMC_OP_READ_STATUS_EN		BIT(1)
>>>> +#define FMC_OP_REG_OP_START		BIT(0)
>>>
>>> [...]
>>>
>>>> +enum hifmc_iftype {
>>>> +	IF_TYPE_STD,
>>>> +	IF_TYPE_DUAL,
>>>> +	IF_TYPE_DIO,
>>>> +	IF_TYPE_QUAD,
>>>> +	IF_TYPE_QIO,
>>>> +};
>>>> +
>>>> +struct hifmc_priv {
>>>> +	int chipselect;
>>>
>>> Can chipselect be set to < 0 values ?
>>>
>> The type will be changed to u32.
>>
>>>> +	u32 clkrate;
>>>> +	struct hifmc_host *host;
>>>> +};
>>>> +
>>>> +#define HIFMC_MAX_CHIP_NUM		2
>>>
>>> This does not scale very well, use dynamic allocation.
>>>
>> OK. Dynamic allocation will be used in next version.
>>>> +struct hifmc_host {
>>>> +	struct device *dev;
>>>> +	struct mutex lock;
>>>> +
>>>> +	void __iomem *regbase;
>>>> +	void __iomem *iobase;
>>>> +	struct clk *clk;
>>>> +	void *buffer;
>>>> +	dma_addr_t dma_buffer;
>>>> +
>>>> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
>>>> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
>>>> +	int num_chip;
>>>> +};
>>>> +
>>>> +static inline int wait_op_finish(struct hifmc_host *host)
>>>> +{
>>>> +	unsigned int reg;
>>>> +
>>>> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
>>>> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
>>>> +}
>>>> +
>>>> +static int get_if_type(enum read_mode flash_read)
>>>> +{
>>>> +	enum hifmc_iftype if_type;
>>>> +
>>>> +	switch (flash_read) {
>>>> +	case SPI_NOR_DUAL:
>>>> +		if_type = IF_TYPE_DUAL;
>>>> +		break;
>>>> +	case SPI_NOR_QUAD:
>>>> +		if_type = IF_TYPE_QUAD;
>>>> +		break;
>>>> +	case SPI_NOR_NORMAL:
>>>> +	case SPI_NOR_FAST:
>>>> +	default:
>>>> +		if_type = IF_TYPE_STD;
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return if_type;
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_init(struct hifmc_host *host)
>>>> +{
>>>> +	unsigned int reg;
>>>
>>> Should be u32 here.
>>>
>> OK.
>>>> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>>>> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
>>>> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
>>>> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
>>>> +}
>>>> +
>>>> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>> +{
>>>> +	struct hifmc_priv *priv = nor->priv;
>>>> +	struct hifmc_host *host = priv->host;
>>>> +	int ret;
>>>> +
>>>> +	mutex_lock(&host->lock);
>>>
>>> Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
>>> locks a mutex in struct spi_nor , but that's not enough if you have
>>> multiple SPI NORs on the same bus, because concurrent access to multiple
>>> SPI NOR flashes needs locking on the controller level, right ?
>>>
>> Yes, you are quite right. The controller can connect with two SPI NOR flashes
>> on one board. This lock is used on the controller level.
> 
> Yeah... we should probably implement some common controller logic in the
> core eventually. But the mutex is necessary for now.
> 
>>>> +	ret = clk_set_rate(host->clk, priv->clkrate);
>>>> +	if (ret)
>>>> +		goto out;
>>>> +
>>>> +	ret = clk_prepare_enable(host->clk);
>>>> +	if (ret)
>>>> +		goto out;
>>>> +
>>>> +	return 0;
>>>> +
>>>> +out:
>>>> +	mutex_unlock(&host->lock);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>> +{
>>>> +	struct hifmc_priv *priv = nor->priv;
>>>> +	struct hifmc_host *host = priv->host;
>>>> +
>>>> +	clk_disable_unprepare(host->clk);
>>>> +	mutex_unlock(&host->lock);
>>>> +}
>>>> +
>>>> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
>>>> +		u32 *opcfg)
>>>> +{
>>>> +	u32 reg;
>>>> +
>>>> +	*opcfg |= FMC_OP_CMD1_EN;
>>>> +	switch (cmd) {
>>>> +	case SPINOR_OP_RDID:
>>>> +	case SPINOR_OP_RDSR:
>>>> +	case SPINOR_OP_RDCR:
>>>> +		*opcfg |= FMC_OP_READ_DATA_EN;
>>>> +		break;
>>>> +	case SPINOR_OP_WREN:
>>>> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
>>>> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
>>>> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
>>>> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
>>>> +		}
>>>> +		break;
>>>> +	case SPINOR_OP_WRSR:
>>>> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
>>>> +		break;
>>>> +	case SPINOR_OP_BE_4K:
>>>> +	case SPINOR_OP_BE_4K_PMC:
>>>> +	case SPINOR_OP_SE_4B:
>>>> +	case SPINOR_OP_SE:
>>>> +		*opcfg |= FMC_OP_ADDR_EN;
>>>> +		break;
>>>> +	case SPINOR_OP_EN4B:
>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>> +		reg |= SPI_NOR_ADDR_MODE;
>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>> +		break;
>>>> +	case SPINOR_OP_EX4B:
>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>> +		reg &= ~SPI_NOR_ADDR_MODE;
>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>> +		break;
>>>> +	case SPINOR_OP_CHIP_ERASE:
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>
>>> Won't the driver fail if we add new instructions into the SPI NOR core
>>> which are not handled by this huge switch statement ?
>>>
>> Only some of commands are needed to process in this stage for the controller.
>> Others have no needs. So this function won't return failure.
> 
> It's not ideal to have this sort of function if we can avoid it (since
> it's hard to figure out how to extend this for new opcodes). But it
> looks necessary for now.
> 
>> I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.
>>
>>>> +}
>>>
>>> [...]
>>>
>>>> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>>>> +			size_t len, size_t *retlen, const u_char *write_buf)
>>>> +{
>>>> +	struct hifmc_priv *priv = nor->priv;
>>>> +	struct hifmc_host *host = priv->host;
>>>> +	const unsigned char *ptr = write_buf;
>>>> +	size_t actual_len;
>>>> +
>>>> +	*retlen = 0;
>>>> +	while (len > 0) {
>>>> +		if (to & HIFMC_DMA_MASK)
>>>> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>>>> +				>= len	? len
>>>> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
>>>
>>> Rewrite this as something like the following snippet, for the sake of
>>> readability:
>>>
>>> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>> if (actual_len >= len)
>>>    actual_len = len;
>>>
>> OK. Thank you.
>>>> +		else
>>>> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>>>> +				? HIFMC_DMA_MAX_LEN : len;
> 
> Wait, why do you need the else case? Isn't this just equivalent to the
> first case? I'd suggest consolidating these two blocks, and dropping the
> ?: entirely, since (like Marek said) it's hurting readability. So:
> 
> 		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
> 		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
> 			actual_len = len;
> 		else
> 			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
> 
>>>> +		memcpy(host->buffer, ptr, actual_len);
>>>> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>>>> +				FMC_OP_WRITE);
>>>> +		to += actual_len;
>>>> +		ptr += actual_len;
>>>> +		len -= actual_len;
>>>> +		*retlen += actual_len;
>>>> +	}
>>>> +}
> 
> [...]
> 
> Here's my diff:
> 
> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
> index e974c92a0a25..0c58fd3b8790 100644
> --- a/drivers/mtd/spi-nor/hisi-sfc.c
> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
> @@ -16,13 +16,15 @@
>   * You should have received a copy of the GNU General Public License
>   * along with this program. If not, see <http://www.gnu.org/licenses/>.
>   */
> +
>  #include <linux/clk.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/spi-nor.h>
> -#include <linux/of_platform.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
>  /* Hardware register offsets and field definitions */
> @@ -343,13 +345,11 @@ static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>  
>  	*retlen = 0;
>  	while (len > 0) {
> -		if (to & HIFMC_DMA_MASK)
> -			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
> -				>= len	? len
> -				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
> +		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
> +		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
> +			actual_len = len;
>  		else
> -			actual_len = (len >= HIFMC_DMA_MAX_LEN)
> -				? HIFMC_DMA_MAX_LEN : len;
> +			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>  		memcpy(host->buffer, ptr, actual_len);
>  		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>  				FMC_OP_WRITE);
> 
> .
> 

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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Marek Vasut]

On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
> Hi Brian,
>    Thank you very much for your comments. I'll fix these issues in next version.
> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.

Would you please stop top-posting ? It rubs some people the wrong way.

> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
> 		size_t *retlen, u_char *read_buf)
> {
> 	struct hifmc_priv *priv = nor->priv;
> 	struct hifmc_host *host = priv->host;
> 	int i;
> 
> 	/* read all bytes in only one time */
> 	if (len <= HIFMC_DMA_MAX_LEN) {
> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
> 				len, FMC_OP_READ);
> 		memcpy(read_buf, host->buffer, len);

Is all the ad-hoc memcpying necessary? I think you can use
dma_map_single() and co to obtain DMAble memory for your
controller's use and then you can probably get rid of most
of this stuff.

> 	} else {
> 		/* read HIFMC_DMA_MAX_LEN bytes at a time */
> 		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
> 			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
> 				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
> 			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
> 		}
> 		/* read remaining bytes */
> 		i -= HIFMC_DMA_MAX_LEN;
> 		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
> 				len - i, FMC_OP_READ);
> 		memcpy(read_buf + i, host->buffer, len - i);
> 	}
> 	*retlen = len;
> 
> 	return 0;
> }
> 
> Because "len" passed from spi_nor_write is smaller than nor->page_size, and nor->page_size
> is smaller than the length of host->dma_buffer. We can transfer "len" bytes data by
> hisi_spi_nor_dma_transfer at one time. hisi_spi_nor_write can be simplified like below:
> 
> static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
> 			size_t len, size_t *retlen, const u_char *write_buf)
> {
> 	struct hifmc_priv *priv = nor->priv;
> 	struct hifmc_host *host = priv->host;
> 
> 	/* len is smaller than dma buffer length*/
> 	memcpy(host->buffer, write_buf, len);
> 	hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, len,
> 					FMC_OP_WRITE);
> 
> 	*retlen = len;
> }
> 
> Regards,
> Jiancheng
> 
> On 2016/4/4 14:44, Brian Norris wrote:
>> Hi Jiancheng,
>>
>> Looking good. In addition to Marek's comments, I have just a few small
>> comments. I'll post a small diff at the end, and a few inline comments.
>>
>> On Mon, Mar 28, 2016 at 05:15:28PM +0800, Jiancheng Xue wrote:
>>> Hi Marek,
>>>     Firstly, thank you very much for your comments.
>>>
>>> On 2016/3/27 9:47, Marek Vasut wrote:
>>>> On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
>>>>> Add hisilicon spi-nor flash controller driver
>>>>>
>>>>> Signed-off-by: Binquan Peng <pengbinquan@huawei.com>
>>>>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
>>>>> Acked-by: Rob Herring <robh@kernel.org>
>>>>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>>> Reviewed-by: Jagan Teki <jteki@openedev.com>
>>>>> ---
>>>>> change log
>>>>> v9:
>>>>> Fixed issues pointed by Jagan Teki.
>>>>
>>>> It'd be really great if you could list which exact issues you fixed.
>>>>
>>>
>>> OK. I'll describe the history log more detailed in next version.
>>>
>>>>> v8:
>>>>> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
>>>>> Moved dts binding file to mtd directory.
>>>>> Changed the compatible string more specific.
>>>>
>>>> [...]
>>
>> ^^ You were using <linux/of_device.h> in here, even though you don't
>> need any of its contents. You just wanted <linux/of.h> and
>> <linux/platform_device.h>.
>>
>>>>
>>>>> +/* Hardware register offsets and field definitions */
>>>>> +#define FMC_CFG				0x00
>>>>> +#define SPI_NOR_ADDR_MODE		BIT(10)
>>>>> +#define FMC_GLOBAL_CFG			0x04
>>>>> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
>>>>> +#define FMC_SPI_TIMING_CFG		0x08
>>>>> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
>>>>> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
>>>>> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
>>>>> +#define CS_HOLD_TIME			0x6
>>>>> +#define CS_SETUP_TIME			0x6
>>>>> +#define CS_DESELECT_TIME		0xf
>>>>> +#define FMC_INT				0x18
>>>>> +#define FMC_INT_OP_DONE			BIT(0)
>>>>> +#define FMC_INT_CLR			0x20
>>>>> +#define FMC_CMD				0x24
>>>>> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
>>>>
>>>> Drop the underscores in the argument names.
>>>>
>>> OK.
>>>>> +#define FMC_ADDRL			0x2c
>>>>> +#define FMC_OP_CFG			0x30
>>>>> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
>>>>> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
>>>>> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
>>>>> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
>>>>> +#define FMC_DATA_NUM			0x38
>>>>> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
>>>>> +#define FMC_OP				0x3c
>>>>> +#define FMC_OP_DUMMY_EN			BIT(8)
>>>>> +#define FMC_OP_CMD1_EN			BIT(7)
>>>>> +#define FMC_OP_ADDR_EN			BIT(6)
>>>>> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
>>>>> +#define FMC_OP_READ_DATA_EN		BIT(2)
>>>>> +#define FMC_OP_READ_STATUS_EN		BIT(1)
>>>>> +#define FMC_OP_REG_OP_START		BIT(0)
>>>>
>>>> [...]
>>>>
>>>>> +enum hifmc_iftype {
>>>>> +	IF_TYPE_STD,
>>>>> +	IF_TYPE_DUAL,
>>>>> +	IF_TYPE_DIO,
>>>>> +	IF_TYPE_QUAD,
>>>>> +	IF_TYPE_QIO,
>>>>> +};
>>>>> +
>>>>> +struct hifmc_priv {
>>>>> +	int chipselect;
>>>>
>>>> Can chipselect be set to < 0 values ?
>>>>
>>> The type will be changed to u32.
>>>
>>>>> +	u32 clkrate;
>>>>> +	struct hifmc_host *host;
>>>>> +};
>>>>> +
>>>>> +#define HIFMC_MAX_CHIP_NUM		2
>>>>
>>>> This does not scale very well, use dynamic allocation.
>>>>
>>> OK. Dynamic allocation will be used in next version.
>>>>> +struct hifmc_host {
>>>>> +	struct device *dev;
>>>>> +	struct mutex lock;
>>>>> +
>>>>> +	void __iomem *regbase;
>>>>> +	void __iomem *iobase;
>>>>> +	struct clk *clk;
>>>>> +	void *buffer;
>>>>> +	dma_addr_t dma_buffer;
>>>>> +
>>>>> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
>>>>> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
>>>>> +	int num_chip;
>>>>> +};
>>>>> +
>>>>> +static inline int wait_op_finish(struct hifmc_host *host)
>>>>> +{
>>>>> +	unsigned int reg;
>>>>> +
>>>>> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
>>>>> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
>>>>> +}
>>>>> +
>>>>> +static int get_if_type(enum read_mode flash_read)
>>>>> +{
>>>>> +	enum hifmc_iftype if_type;
>>>>> +
>>>>> +	switch (flash_read) {
>>>>> +	case SPI_NOR_DUAL:
>>>>> +		if_type = IF_TYPE_DUAL;
>>>>> +		break;
>>>>> +	case SPI_NOR_QUAD:
>>>>> +		if_type = IF_TYPE_QUAD;
>>>>> +		break;
>>>>> +	case SPI_NOR_NORMAL:
>>>>> +	case SPI_NOR_FAST:
>>>>> +	default:
>>>>> +		if_type = IF_TYPE_STD;
>>>>> +		break;
>>>>> +	}
>>>>> +
>>>>> +	return if_type;
>>>>> +}
>>>>> +
>>>>> +static void hisi_spi_nor_init(struct hifmc_host *host)
>>>>> +{
>>>>> +	unsigned int reg;
>>>>
>>>> Should be u32 here.
>>>>
>>> OK.
>>>>> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>>>>> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
>>>>> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
>>>>> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
>>>>> +}
>>>>> +
>>>>> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>>> +{
>>>>> +	struct hifmc_priv *priv = nor->priv;
>>>>> +	struct hifmc_host *host = priv->host;
>>>>> +	int ret;
>>>>> +
>>>>> +	mutex_lock(&host->lock);
>>>>
>>>> Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
>>>> locks a mutex in struct spi_nor , but that's not enough if you have
>>>> multiple SPI NORs on the same bus, because concurrent access to multiple
>>>> SPI NOR flashes needs locking on the controller level, right ?
>>>>
>>> Yes, you are quite right. The controller can connect with two SPI NOR flashes
>>> on one board. This lock is used on the controller level.
>>
>> Yeah... we should probably implement some common controller logic in the
>> core eventually. But the mutex is necessary for now.
>>
>>>>> +	ret = clk_set_rate(host->clk, priv->clkrate);
>>>>> +	if (ret)
>>>>> +		goto out;
>>>>> +
>>>>> +	ret = clk_prepare_enable(host->clk);
>>>>> +	if (ret)
>>>>> +		goto out;
>>>>> +
>>>>> +	return 0;
>>>>> +
>>>>> +out:
>>>>> +	mutex_unlock(&host->lock);
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>>> +{
>>>>> +	struct hifmc_priv *priv = nor->priv;
>>>>> +	struct hifmc_host *host = priv->host;
>>>>> +
>>>>> +	clk_disable_unprepare(host->clk);
>>>>> +	mutex_unlock(&host->lock);
>>>>> +}
>>>>> +
>>>>> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
>>>>> +		u32 *opcfg)
>>>>> +{
>>>>> +	u32 reg;
>>>>> +
>>>>> +	*opcfg |= FMC_OP_CMD1_EN;
>>>>> +	switch (cmd) {
>>>>> +	case SPINOR_OP_RDID:
>>>>> +	case SPINOR_OP_RDSR:
>>>>> +	case SPINOR_OP_RDCR:
>>>>> +		*opcfg |= FMC_OP_READ_DATA_EN;
>>>>> +		break;
>>>>> +	case SPINOR_OP_WREN:
>>>>> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
>>>>> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
>>>>> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
>>>>> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
>>>>> +		}
>>>>> +		break;
>>>>> +	case SPINOR_OP_WRSR:
>>>>> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
>>>>> +		break;
>>>>> +	case SPINOR_OP_BE_4K:
>>>>> +	case SPINOR_OP_BE_4K_PMC:
>>>>> +	case SPINOR_OP_SE_4B:
>>>>> +	case SPINOR_OP_SE:
>>>>> +		*opcfg |= FMC_OP_ADDR_EN;
>>>>> +		break;
>>>>> +	case SPINOR_OP_EN4B:
>>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>>> +		reg |= SPI_NOR_ADDR_MODE;
>>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>>> +		break;
>>>>> +	case SPINOR_OP_EX4B:
>>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>>> +		reg &= ~SPI_NOR_ADDR_MODE;
>>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>>> +		break;
>>>>> +	case SPINOR_OP_CHIP_ERASE:
>>>>> +	default:
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Won't the driver fail if we add new instructions into the SPI NOR core
>>>> which are not handled by this huge switch statement ?
>>>>
>>> Only some of commands are needed to process in this stage for the controller.
>>> Others have no needs. So this function won't return failure.
>>
>> It's not ideal to have this sort of function if we can avoid it (since
>> it's hard to figure out how to extend this for new opcodes). But it
>> looks necessary for now.
>>
>>> I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.
>>>
>>>>> +}
>>>>
>>>> [...]
>>>>
>>>>> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>>>>> +			size_t len, size_t *retlen, const u_char *write_buf)
>>>>> +{
>>>>> +	struct hifmc_priv *priv = nor->priv;
>>>>> +	struct hifmc_host *host = priv->host;
>>>>> +	const unsigned char *ptr = write_buf;
>>>>> +	size_t actual_len;
>>>>> +
>>>>> +	*retlen = 0;
>>>>> +	while (len > 0) {
>>>>> +		if (to & HIFMC_DMA_MASK)
>>>>> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>>>>> +				>= len	? len
>>>>> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
>>>>
>>>> Rewrite this as something like the following snippet, for the sake of
>>>> readability:
>>>>
>>>> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>>> if (actual_len >= len)
>>>>    actual_len = len;
>>>>
>>> OK. Thank you.
>>>>> +		else
>>>>> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>>>>> +				? HIFMC_DMA_MAX_LEN : len;
>>
>> Wait, why do you need the else case? Isn't this just equivalent to the
>> first case? I'd suggest consolidating these two blocks, and dropping the
>> ?: entirely, since (like Marek said) it's hurting readability. So:
>>
>> 		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
>> 		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
>> 			actual_len = len;
>> 		else
>> 			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>
>>>>> +		memcpy(host->buffer, ptr, actual_len);
>>>>> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>>>>> +				FMC_OP_WRITE);
>>>>> +		to += actual_len;
>>>>> +		ptr += actual_len;
>>>>> +		len -= actual_len;
>>>>> +		*retlen += actual_len;
>>>>> +	}
>>>>> +}
>>
>> [...]
>>
>> Here's my diff:
>>
>> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
>> index e974c92a0a25..0c58fd3b8790 100644
>> --- a/drivers/mtd/spi-nor/hisi-sfc.c
>> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
>> @@ -16,13 +16,15 @@
>>   * You should have received a copy of the GNU General Public License
>>   * along with this program. If not, see <http://www.gnu.org/licenses/>.
>>   */
>> +
>>  #include <linux/clk.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/module.h>
>>  #include <linux/mtd/mtd.h>
>>  #include <linux/mtd/spi-nor.h>
>> -#include <linux/of_platform.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  
>>  /* Hardware register offsets and field definitions */
>> @@ -343,13 +345,11 @@ static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>>  
>>  	*retlen = 0;
>>  	while (len > 0) {
>> -		if (to & HIFMC_DMA_MASK)
>> -			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>> -				>= len	? len
>> -				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
>> +		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
>> +		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
>> +			actual_len = len;
>>  		else
>> -			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>> -				? HIFMC_DMA_MAX_LEN : len;
>> +			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>  		memcpy(host->buffer, ptr, actual_len);
>>  		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>>  				FMC_OP_WRITE);
>>
>> .
>>
> 


-- 
Best regards,
Marek Vasut

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Marek Vasut]

On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
> Hi Brian,
>    Thank you very much for your comments. I'll fix these issues in next version.
> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.

Would you please stop top-posting ? It rubs some people the wrong way.

> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
> 		size_t *retlen, u_char *read_buf)
> {
> 	struct hifmc_priv *priv = nor->priv;
> 	struct hifmc_host *host = priv->host;
> 	int i;
> 
> 	/* read all bytes in only one time */
> 	if (len <= HIFMC_DMA_MAX_LEN) {
> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
> 				len, FMC_OP_READ);
> 		memcpy(read_buf, host->buffer, len);

Is all the ad-hoc memcpying necessary? I think you can use
dma_map_single() and co to obtain DMAble memory for your
controller's use and then you can probably get rid of most
of this stuff.

> 	} else {
> 		/* read HIFMC_DMA_MAX_LEN bytes at a time */
> 		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
> 			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
> 				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
> 			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
> 		}
> 		/* read remaining bytes */
> 		i -= HIFMC_DMA_MAX_LEN;
> 		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
> 				len - i, FMC_OP_READ);
> 		memcpy(read_buf + i, host->buffer, len - i);
> 	}
> 	*retlen = len;
> 
> 	return 0;
> }
> 
> Because "len" passed from spi_nor_write is smaller than nor->page_size, and nor->page_size
> is smaller than the length of host->dma_buffer. We can transfer "len" bytes data by
> hisi_spi_nor_dma_transfer at one time. hisi_spi_nor_write can be simplified like below:
> 
> static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
> 			size_t len, size_t *retlen, const u_char *write_buf)
> {
> 	struct hifmc_priv *priv = nor->priv;
> 	struct hifmc_host *host = priv->host;
> 
> 	/* len is smaller than dma buffer length*/
> 	memcpy(host->buffer, write_buf, len);
> 	hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, len,
> 					FMC_OP_WRITE);
> 
> 	*retlen = len;
> }
> 
> Regards,
> Jiancheng
> 
> On 2016/4/4 14:44, Brian Norris wrote:
>> Hi Jiancheng,
>>
>> Looking good. In addition to Marek's comments, I have just a few small
>> comments. I'll post a small diff at the end, and a few inline comments.
>>
>> On Mon, Mar 28, 2016 at 05:15:28PM +0800, Jiancheng Xue wrote:
>>> Hi Marek,
>>>     Firstly, thank you very much for your comments.
>>>
>>> On 2016/3/27 9:47, Marek Vasut wrote:
>>>> On 03/26/2016 09:11 AM, Jiancheng Xue wrote:
>>>>> Add hisilicon spi-nor flash controller driver
>>>>>
>>>>> Signed-off-by: Binquan Peng <pengbinquan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>>> Signed-off-by: Jiancheng Xue <xuejiancheng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>>>> Reviewed-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
>>>>> Reviewed-by: Jagan Teki <jteki-oRp2ZoJdM/RWk0Htik3J/w@public.gmane.org>
>>>>> ---
>>>>> change log
>>>>> v9:
>>>>> Fixed issues pointed by Jagan Teki.
>>>>
>>>> It'd be really great if you could list which exact issues you fixed.
>>>>
>>>
>>> OK. I'll describe the history log more detailed in next version.
>>>
>>>>> v8:
>>>>> Fixed issues pointed by Ezequiel Garcia and Brian Norris.
>>>>> Moved dts binding file to mtd directory.
>>>>> Changed the compatible string more specific.
>>>>
>>>> [...]
>>
>> ^^ You were using <linux/of_device.h> in here, even though you don't
>> need any of its contents. You just wanted <linux/of.h> and
>> <linux/platform_device.h>.
>>
>>>>
>>>>> +/* Hardware register offsets and field definitions */
>>>>> +#define FMC_CFG				0x00
>>>>> +#define SPI_NOR_ADDR_MODE		BIT(10)
>>>>> +#define FMC_GLOBAL_CFG			0x04
>>>>> +#define FMC_GLOBAL_CFG_WP_ENABLE	BIT(6)
>>>>> +#define FMC_SPI_TIMING_CFG		0x08
>>>>> +#define TIMING_CFG_TCSH(nr)		(((nr) & 0xf) << 8)
>>>>> +#define TIMING_CFG_TCSS(nr)		(((nr) & 0xf) << 4)
>>>>> +#define TIMING_CFG_TSHSL(nr)		((nr) & 0xf)
>>>>> +#define CS_HOLD_TIME			0x6
>>>>> +#define CS_SETUP_TIME			0x6
>>>>> +#define CS_DESELECT_TIME		0xf
>>>>> +#define FMC_INT				0x18
>>>>> +#define FMC_INT_OP_DONE			BIT(0)
>>>>> +#define FMC_INT_CLR			0x20
>>>>> +#define FMC_CMD				0x24
>>>>> +#define FMC_CMD_CMD1(_cmd)		((_cmd) & 0xff)
>>>>
>>>> Drop the underscores in the argument names.
>>>>
>>> OK.
>>>>> +#define FMC_ADDRL			0x2c
>>>>> +#define FMC_OP_CFG			0x30
>>>>> +#define OP_CFG_FM_CS(_cs)		((_cs) << 11)
>>>>> +#define OP_CFG_MEM_IF_TYPE(_type)	(((_type) & 0x7) << 7)
>>>>> +#define OP_CFG_ADDR_NUM(_addr)		(((_addr) & 0x7) << 4)
>>>>> +#define OP_CFG_DUMMY_NUM(_dummy)	((_dummy) & 0xf)
>>>>> +#define FMC_DATA_NUM			0x38
>>>>> +#define FMC_DATA_NUM_CNT(_n)		((_n) & 0x3fff)
>>>>> +#define FMC_OP				0x3c
>>>>> +#define FMC_OP_DUMMY_EN			BIT(8)
>>>>> +#define FMC_OP_CMD1_EN			BIT(7)
>>>>> +#define FMC_OP_ADDR_EN			BIT(6)
>>>>> +#define FMC_OP_WRITE_DATA_EN		BIT(5)
>>>>> +#define FMC_OP_READ_DATA_EN		BIT(2)
>>>>> +#define FMC_OP_READ_STATUS_EN		BIT(1)
>>>>> +#define FMC_OP_REG_OP_START		BIT(0)
>>>>
>>>> [...]
>>>>
>>>>> +enum hifmc_iftype {
>>>>> +	IF_TYPE_STD,
>>>>> +	IF_TYPE_DUAL,
>>>>> +	IF_TYPE_DIO,
>>>>> +	IF_TYPE_QUAD,
>>>>> +	IF_TYPE_QIO,
>>>>> +};
>>>>> +
>>>>> +struct hifmc_priv {
>>>>> +	int chipselect;
>>>>
>>>> Can chipselect be set to < 0 values ?
>>>>
>>> The type will be changed to u32.
>>>
>>>>> +	u32 clkrate;
>>>>> +	struct hifmc_host *host;
>>>>> +};
>>>>> +
>>>>> +#define HIFMC_MAX_CHIP_NUM		2
>>>>
>>>> This does not scale very well, use dynamic allocation.
>>>>
>>> OK. Dynamic allocation will be used in next version.
>>>>> +struct hifmc_host {
>>>>> +	struct device *dev;
>>>>> +	struct mutex lock;
>>>>> +
>>>>> +	void __iomem *regbase;
>>>>> +	void __iomem *iobase;
>>>>> +	struct clk *clk;
>>>>> +	void *buffer;
>>>>> +	dma_addr_t dma_buffer;
>>>>> +
>>>>> +	struct spi_nor	nor[HIFMC_MAX_CHIP_NUM];
>>>>> +	struct hifmc_priv priv[HIFMC_MAX_CHIP_NUM];
>>>>> +	int num_chip;
>>>>> +};
>>>>> +
>>>>> +static inline int wait_op_finish(struct hifmc_host *host)
>>>>> +{
>>>>> +	unsigned int reg;
>>>>> +
>>>>> +	return readl_poll_timeout(host->regbase + FMC_INT, reg,
>>>>> +		(reg & FMC_INT_OP_DONE), 0, FMC_WAIT_TIMEOUT);
>>>>> +}
>>>>> +
>>>>> +static int get_if_type(enum read_mode flash_read)
>>>>> +{
>>>>> +	enum hifmc_iftype if_type;
>>>>> +
>>>>> +	switch (flash_read) {
>>>>> +	case SPI_NOR_DUAL:
>>>>> +		if_type = IF_TYPE_DUAL;
>>>>> +		break;
>>>>> +	case SPI_NOR_QUAD:
>>>>> +		if_type = IF_TYPE_QUAD;
>>>>> +		break;
>>>>> +	case SPI_NOR_NORMAL:
>>>>> +	case SPI_NOR_FAST:
>>>>> +	default:
>>>>> +		if_type = IF_TYPE_STD;
>>>>> +		break;
>>>>> +	}
>>>>> +
>>>>> +	return if_type;
>>>>> +}
>>>>> +
>>>>> +static void hisi_spi_nor_init(struct hifmc_host *host)
>>>>> +{
>>>>> +	unsigned int reg;
>>>>
>>>> Should be u32 here.
>>>>
>>> OK.
>>>>> +	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>>>>> +		| TIMING_CFG_TCSS(CS_SETUP_TIME)
>>>>> +		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
>>>>> +	writel(reg, host->regbase + FMC_SPI_TIMING_CFG);
>>>>> +}
>>>>> +
>>>>> +static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>>> +{
>>>>> +	struct hifmc_priv *priv = nor->priv;
>>>>> +	struct hifmc_host *host = priv->host;
>>>>> +	int ret;
>>>>> +
>>>>> +	mutex_lock(&host->lock);
>>>>
>>>> Why do you need the mutex lock here ? Let me guess -- SPI NOR framework
>>>> locks a mutex in struct spi_nor , but that's not enough if you have
>>>> multiple SPI NORs on the same bus, because concurrent access to multiple
>>>> SPI NOR flashes needs locking on the controller level, right ?
>>>>
>>> Yes, you are quite right. The controller can connect with two SPI NOR flashes
>>> on one board. This lock is used on the controller level.
>>
>> Yeah... we should probably implement some common controller logic in the
>> core eventually. But the mutex is necessary for now.
>>
>>>>> +	ret = clk_set_rate(host->clk, priv->clkrate);
>>>>> +	if (ret)
>>>>> +		goto out;
>>>>> +
>>>>> +	ret = clk_prepare_enable(host->clk);
>>>>> +	if (ret)
>>>>> +		goto out;
>>>>> +
>>>>> +	return 0;
>>>>> +
>>>>> +out:
>>>>> +	mutex_unlock(&host->lock);
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static void hisi_spi_nor_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
>>>>> +{
>>>>> +	struct hifmc_priv *priv = nor->priv;
>>>>> +	struct hifmc_host *host = priv->host;
>>>>> +
>>>>> +	clk_disable_unprepare(host->clk);
>>>>> +	mutex_unlock(&host->lock);
>>>>> +}
>>>>> +
>>>>> +static void hisi_spi_nor_cmd_prepare(struct hifmc_host *host, u8 cmd,
>>>>> +		u32 *opcfg)
>>>>> +{
>>>>> +	u32 reg;
>>>>> +
>>>>> +	*opcfg |= FMC_OP_CMD1_EN;
>>>>> +	switch (cmd) {
>>>>> +	case SPINOR_OP_RDID:
>>>>> +	case SPINOR_OP_RDSR:
>>>>> +	case SPINOR_OP_RDCR:
>>>>> +		*opcfg |= FMC_OP_READ_DATA_EN;
>>>>> +		break;
>>>>> +	case SPINOR_OP_WREN:
>>>>> +		reg = readl(host->regbase + FMC_GLOBAL_CFG);
>>>>> +		if (reg & FMC_GLOBAL_CFG_WP_ENABLE) {
>>>>> +			reg &= ~FMC_GLOBAL_CFG_WP_ENABLE;
>>>>> +			writel(reg, host->regbase + FMC_GLOBAL_CFG);
>>>>> +		}
>>>>> +		break;
>>>>> +	case SPINOR_OP_WRSR:
>>>>> +		*opcfg |= FMC_OP_WRITE_DATA_EN;
>>>>> +		break;
>>>>> +	case SPINOR_OP_BE_4K:
>>>>> +	case SPINOR_OP_BE_4K_PMC:
>>>>> +	case SPINOR_OP_SE_4B:
>>>>> +	case SPINOR_OP_SE:
>>>>> +		*opcfg |= FMC_OP_ADDR_EN;
>>>>> +		break;
>>>>> +	case SPINOR_OP_EN4B:
>>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>>> +		reg |= SPI_NOR_ADDR_MODE;
>>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>>> +		break;
>>>>> +	case SPINOR_OP_EX4B:
>>>>> +		reg = readl(host->regbase + FMC_CFG);
>>>>> +		reg &= ~SPI_NOR_ADDR_MODE;
>>>>> +		writel(reg, host->regbase + FMC_CFG);
>>>>> +		break;
>>>>> +	case SPINOR_OP_CHIP_ERASE:
>>>>> +	default:
>>>>> +		break;
>>>>> +	}
>>>>
>>>> Won't the driver fail if we add new instructions into the SPI NOR core
>>>> which are not handled by this huge switch statement ?
>>>>
>>> Only some of commands are needed to process in this stage for the controller.
>>> Others have no needs. So this function won't return failure.
>>
>> It's not ideal to have this sort of function if we can avoid it (since
>> it's hard to figure out how to extend this for new opcodes). But it
>> looks necessary for now.
>>
>>> I'll combine SPINOR_OP_CHIP_ERASE into the default case in next version.
>>>
>>>>> +}
>>>>
>>>> [...]
>>>>
>>>>> +static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>>>>> +			size_t len, size_t *retlen, const u_char *write_buf)
>>>>> +{
>>>>> +	struct hifmc_priv *priv = nor->priv;
>>>>> +	struct hifmc_host *host = priv->host;
>>>>> +	const unsigned char *ptr = write_buf;
>>>>> +	size_t actual_len;
>>>>> +
>>>>> +	*retlen = 0;
>>>>> +	while (len > 0) {
>>>>> +		if (to & HIFMC_DMA_MASK)
>>>>> +			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>>>>> +				>= len	? len
>>>>> +				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
>>>>
>>>> Rewrite this as something like the following snippet, for the sake of
>>>> readability:
>>>>
>>>> actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>>> if (actual_len >= len)
>>>>    actual_len = len;
>>>>
>>> OK. Thank you.
>>>>> +		else
>>>>> +			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>>>>> +				? HIFMC_DMA_MAX_LEN : len;
>>
>> Wait, why do you need the else case? Isn't this just equivalent to the
>> first case? I'd suggest consolidating these two blocks, and dropping the
>> ?: entirely, since (like Marek said) it's hurting readability. So:
>>
>> 		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
>> 		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
>> 			actual_len = len;
>> 		else
>> 			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>
>>>>> +		memcpy(host->buffer, ptr, actual_len);
>>>>> +		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>>>>> +				FMC_OP_WRITE);
>>>>> +		to += actual_len;
>>>>> +		ptr += actual_len;
>>>>> +		len -= actual_len;
>>>>> +		*retlen += actual_len;
>>>>> +	}
>>>>> +}
>>
>> [...]
>>
>> Here's my diff:
>>
>> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
>> index e974c92a0a25..0c58fd3b8790 100644
>> --- a/drivers/mtd/spi-nor/hisi-sfc.c
>> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
>> @@ -16,13 +16,15 @@
>>   * You should have received a copy of the GNU General Public License
>>   * along with this program. If not, see <http://www.gnu.org/licenses/>.
>>   */
>> +
>>  #include <linux/clk.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/module.h>
>>  #include <linux/mtd/mtd.h>
>>  #include <linux/mtd/spi-nor.h>
>> -#include <linux/of_platform.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  
>>  /* Hardware register offsets and field definitions */
>> @@ -343,13 +345,11 @@ static void hisi_spi_nor_write(struct spi_nor *nor, loff_t to,
>>  
>>  	*retlen = 0;
>>  	while (len > 0) {
>> -		if (to & HIFMC_DMA_MASK)
>> -			actual_len = (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK))
>> -				>= len	? len
>> -				: (HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK));
>> +		/* Don't cross HIFMC_DMA_MAX_LEN alignment boundaries */
>> +		if ((HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK)) >= len)
>> +			actual_len = len;
>>  		else
>> -			actual_len = (len >= HIFMC_DMA_MAX_LEN)
>> -				? HIFMC_DMA_MAX_LEN : len;
>> +			actual_len = HIFMC_DMA_MAX_LEN - (to & HIFMC_DMA_MASK);
>>  		memcpy(host->buffer, ptr, actual_len);
>>  		hisi_spi_nor_dma_transfer(nor, to, host->dma_buffer, actual_len,
>>  				FMC_OP_WRITE);
>>
>> .
>>
> 


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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi,

On 2016/4/7 10:28, Marek Vasut wrote:
> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>> Hi Brian,
>>    Thank you very much for your comments. I'll fix these issues in next version.
>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
> 
> Would you please stop top-posting ? It rubs some people the wrong way.
> 
I feel very sorry about that. I have read the etiquette and won't make the same mistake again.

>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>> 		size_t *retlen, u_char *read_buf)
>> {
>> 	struct hifmc_priv *priv = nor->priv;
>> 	struct hifmc_host *host = priv->host;
>> 	int i;
>>
>> 	/* read all bytes in only one time */
>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>> 				len, FMC_OP_READ);
>> 		memcpy(read_buf, host->buffer, len);
> 
> Is all the ad-hoc memcpying necessary? I think you can use
> dma_map_single() and co to obtain DMAble memory for your
> controller's use and then you can probably get rid of most
> of this stuff.
> 
Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
and the DMA buffer allocated by the driver is still needed. But I am not sure about
this. Please let me know if I am wrong. Thank you!

Regards,
Jiancheng

>> 	} else {
>> 		/* read HIFMC_DMA_MAX_LEN bytes at a time */
>> 		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
>> 			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
>> 				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
>> 			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
>> 		}
>> 		/* read remaining bytes */
>> 		i -= HIFMC_DMA_MAX_LEN;
>> 		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
>> 				len - i, FMC_OP_READ);
>> 		memcpy(read_buf + i, host->buffer, len - i);
>> 	}
>> 	*retlen = len;
>>
>> 	return 0;
>> }
>>
[...]
>> On 2016/4/4 14:44, Brian Norris wrote:
>>> Hi Jiancheng,
>>>
>>> Looking good. In addition to Marek's comments, I have just a few small
>>> comments. I'll post a small diff at the end, and a few inline comments.
>>>
> 
> 

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi,

On 2016/4/7 10:28, Marek Vasut wrote:
> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>> Hi Brian,
>>    Thank you very much for your comments. I'll fix these issues in next version.
>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
> 
> Would you please stop top-posting ? It rubs some people the wrong way.
> 
I feel very sorry about that. I have read the etiquette and won't make the same mistake again.

>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>> 		size_t *retlen, u_char *read_buf)
>> {
>> 	struct hifmc_priv *priv = nor->priv;
>> 	struct hifmc_host *host = priv->host;
>> 	int i;
>>
>> 	/* read all bytes in only one time */
>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>> 				len, FMC_OP_READ);
>> 		memcpy(read_buf, host->buffer, len);
> 
> Is all the ad-hoc memcpying necessary? I think you can use
> dma_map_single() and co to obtain DMAble memory for your
> controller's use and then you can probably get rid of most
> of this stuff.
> 
Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
and the DMA buffer allocated by the driver is still needed. But I am not sure about
this. Please let me know if I am wrong. Thank you!

Regards,
Jiancheng

>> 	} else {
>> 		/* read HIFMC_DMA_MAX_LEN bytes at a time */
>> 		for (i = 0; i < len; i += HIFMC_DMA_MAX_LEN) {
>> 			hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
>> 				HIFMC_DMA_MAX_LEN, FMC_OP_READ);
>> 			memcpy(read_buf + i, host->buffer, HIFMC_DMA_MAX_LEN);
>> 		}
>> 		/* read remaining bytes */
>> 		i -= HIFMC_DMA_MAX_LEN;
>> 		hisi_spi_nor_dma_transfer(nor, from + i, host->dma_buffer,
>> 				len - i, FMC_OP_READ);
>> 		memcpy(read_buf + i, host->buffer, len - i);
>> 	}
>> 	*retlen = len;
>>
>> 	return 0;
>> }
>>
[...]
>> On 2016/4/4 14:44, Brian Norris wrote:
>>> Hi Jiancheng,
>>>
>>> Looking good. In addition to Marek's comments, I have just a few small
>>> comments. I'll post a small diff at the end, and a few inline comments.
>>>
> 
> 

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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Marek Vasut]

On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
> Hi,
> 
> On 2016/4/7 10:28, Marek Vasut wrote:
>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>> Hi Brian,
>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>
>> Would you please stop top-posting ? It rubs some people the wrong way.
>>
> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
> 
>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>> 		size_t *retlen, u_char *read_buf)
>>> {
>>> 	struct hifmc_priv *priv = nor->priv;
>>> 	struct hifmc_host *host = priv->host;
>>> 	int i;
>>>
>>> 	/* read all bytes in only one time */
>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>> 				len, FMC_OP_READ);
>>> 		memcpy(read_buf, host->buffer, len);
>>
>> Is all the ad-hoc memcpying necessary? I think you can use
>> dma_map_single() and co to obtain DMAble memory for your
>> controller's use and then you can probably get rid of most
>> of this stuff.
>>
> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
> and the DMA buffer allocated by the driver is still needed. But I am not sure about
> this. Please let me know if I am wrong. Thank you!

Does your controller/DMA have a limitation where it's buffers must be in
the bottom 4GiB range ? The DMA framework should be able to take care of
such platform limitations.

Best regards,
Marek Vasut

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi,

On 2016/4/8 18:04, Marek Vasut wrote:
> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>> Hi,
>>
>> On 2016/4/7 10:28, Marek Vasut wrote:
>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>> Hi Brian,
>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>
>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>
>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>
>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>> 		size_t *retlen, u_char *read_buf)
>>>> {
>>>> 	struct hifmc_priv *priv = nor->priv;
>>>> 	struct hifmc_host *host = priv->host;
>>>> 	int i;
>>>>
>>>> 	/* read all bytes in only one time */
>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>> 				len, FMC_OP_READ);
>>>> 		memcpy(read_buf, host->buffer, len);
>>>
>>> Is all the ad-hoc memcpying necessary? I think you can use
>>> dma_map_single() and co to obtain DMAble memory for your
>>> controller's use and then you can probably get rid of most
>>> of this stuff.
>>>
>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>> this. Please let me know if I am wrong. Thank you!
> 
> Does your controller/DMA have a limitation where it's buffers must be in
> the bottom 4GiB range ? The DMA framework should be able to take care of
> such platform limitations.
> 
When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
needed. Am I right?

Regards,
Jiancheng.

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi,

On 2016/4/8 18:04, Marek Vasut wrote:
> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>> Hi,
>>
>> On 2016/4/7 10:28, Marek Vasut wrote:
>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>> Hi Brian,
>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>
>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>
>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>
>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>> 		size_t *retlen, u_char *read_buf)
>>>> {
>>>> 	struct hifmc_priv *priv = nor->priv;
>>>> 	struct hifmc_host *host = priv->host;
>>>> 	int i;
>>>>
>>>> 	/* read all bytes in only one time */
>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>> 				len, FMC_OP_READ);
>>>> 		memcpy(read_buf, host->buffer, len);
>>>
>>> Is all the ad-hoc memcpying necessary? I think you can use
>>> dma_map_single() and co to obtain DMAble memory for your
>>> controller's use and then you can probably get rid of most
>>> of this stuff.
>>>
>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>> this. Please let me know if I am wrong. Thank you!
> 
> Does your controller/DMA have a limitation where it's buffers must be in
> the bottom 4GiB range ? The DMA framework should be able to take care of
> such platform limitations.
> 
When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
needed. Am I right?

Regards,
Jiancheng.

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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Marek Vasut]

On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
> Hi,
> 
> On 2016/4/8 18:04, Marek Vasut wrote:
>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>>> Hi,
>>>
>>> On 2016/4/7 10:28, Marek Vasut wrote:
>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>>> Hi Brian,
>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>>
>>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>>
>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>>
>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>> 		size_t *retlen, u_char *read_buf)
>>>>> {
>>>>> 	struct hifmc_priv *priv = nor->priv;
>>>>> 	struct hifmc_host *host = priv->host;
>>>>> 	int i;
>>>>>
>>>>> 	/* read all bytes in only one time */
>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>>> 				len, FMC_OP_READ);
>>>>> 		memcpy(read_buf, host->buffer, len);
>>>>
>>>> Is all the ad-hoc memcpying necessary? I think you can use
>>>> dma_map_single() and co to obtain DMAble memory for your
>>>> controller's use and then you can probably get rid of most
>>>> of this stuff.
>>>>
>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>>> this. Please let me know if I am wrong. Thank you!
>>
>> Does your controller/DMA have a limitation where it's buffers must be in
>> the bottom 4GiB range ? The DMA framework should be able to take care of
>> such platform limitations.
>>
> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
> needed. Am I right?

Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
solution help you in any way ?

-- 
Best regards,
Marek Vasut

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Marek Vasut]

On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
> Hi,
> 
> On 2016/4/8 18:04, Marek Vasut wrote:
>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>>> Hi,
>>>
>>> On 2016/4/7 10:28, Marek Vasut wrote:
>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>>> Hi Brian,
>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>>
>>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>>
>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>>
>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>> 		size_t *retlen, u_char *read_buf)
>>>>> {
>>>>> 	struct hifmc_priv *priv = nor->priv;
>>>>> 	struct hifmc_host *host = priv->host;
>>>>> 	int i;
>>>>>
>>>>> 	/* read all bytes in only one time */
>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>>> 				len, FMC_OP_READ);
>>>>> 		memcpy(read_buf, host->buffer, len);
>>>>
>>>> Is all the ad-hoc memcpying necessary? I think you can use
>>>> dma_map_single() and co to obtain DMAble memory for your
>>>> controller's use and then you can probably get rid of most
>>>> of this stuff.
>>>>
>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>>> this. Please let me know if I am wrong. Thank you!
>>
>> Does your controller/DMA have a limitation where it's buffers must be in
>> the bottom 4GiB range ? The DMA framework should be able to take care of
>> such platform limitations.
>>
> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
> needed. Am I right?

Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
solution help you in any way ?

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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Marek,

On 2016/4/12 3:21, Marek Vasut wrote:
> On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
>> Hi,
>>
>> On 2016/4/8 18:04, Marek Vasut wrote:
>>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>>>> Hi,
>>>>
>>>> On 2016/4/7 10:28, Marek Vasut wrote:
>>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>>>> Hi Brian,
>>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>>>
>>>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>>>
>>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>>>
>>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>> 		size_t *retlen, u_char *read_buf)
>>>>>> {
>>>>>> 	struct hifmc_priv *priv = nor->priv;
>>>>>> 	struct hifmc_host *host = priv->host;
>>>>>> 	int i;
>>>>>>
>>>>>> 	/* read all bytes in only one time */
>>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>>>> 				len, FMC_OP_READ);
>>>>>> 		memcpy(read_buf, host->buffer, len);
>>>>>
>>>>> Is all the ad-hoc memcpying necessary? I think you can use
>>>>> dma_map_single() and co to obtain DMAble memory for your
>>>>> controller's use and then you can probably get rid of most
>>>>> of this stuff.
>>>>>
>>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>>>> this. Please let me know if I am wrong. Thank you!
>>>
>>> Does your controller/DMA have a limitation where it's buffers must be in
>>> the bottom 4GiB range ? The DMA framework should be able to take care of
>>> such platform limitations.
>>>
>> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
>> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
>> needed. Am I right?
> 
> Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
> solution help you in any way ?
> 
No. I think this solution just processes the buffer within only one page.
I had referred to drivers/mtd/onenand/samsung.c and other files.
The corresponding code segment is as follows:
static int s5pc110_read_bufferram(struct mtd_info *mtd, int area,
                unsigned char *buffer, int offset, size_t count)
{
	void *buf = (void *) buffer;
        dma_addr_t dma_src, dma_dst;
	...
	/* Handle vmalloc address */
        if (buf >= high_memory) {
                struct page *page;

                if (((size_t) buf & PAGE_MASK) !=
                    ((size_t) (buf + count - 1) & PAGE_MASK))
                        goto normal;
                page = vmalloc_to_page(buf);
                if (!page)
                        goto normal;

                ...
        } else {
		...
	}

normal:
        ...
        memcpy(buffer, p, count);

	return 0;
}
I think memcpy in "normal" clause can't be removed. So I'd like to keep my original
implementation if it is also OK. What's your opinion?

Regards,
Jiancheng

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Marek,

On 2016/4/12 3:21, Marek Vasut wrote:
> On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
>> Hi,
>>
>> On 2016/4/8 18:04, Marek Vasut wrote:
>>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>>>> Hi,
>>>>
>>>> On 2016/4/7 10:28, Marek Vasut wrote:
>>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>>>> Hi Brian,
>>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>>>
>>>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>>>
>>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>>>
>>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>> 		size_t *retlen, u_char *read_buf)
>>>>>> {
>>>>>> 	struct hifmc_priv *priv = nor->priv;
>>>>>> 	struct hifmc_host *host = priv->host;
>>>>>> 	int i;
>>>>>>
>>>>>> 	/* read all bytes in only one time */
>>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>>>> 				len, FMC_OP_READ);
>>>>>> 		memcpy(read_buf, host->buffer, len);
>>>>>
>>>>> Is all the ad-hoc memcpying necessary? I think you can use
>>>>> dma_map_single() and co to obtain DMAble memory for your
>>>>> controller's use and then you can probably get rid of most
>>>>> of this stuff.
>>>>>
>>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>>>> this. Please let me know if I am wrong. Thank you!
>>>
>>> Does your controller/DMA have a limitation where it's buffers must be in
>>> the bottom 4GiB range ? The DMA framework should be able to take care of
>>> such platform limitations.
>>>
>> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
>> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
>> needed. Am I right?
> 
> Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
> solution help you in any way ?
> 
No. I think this solution just processes the buffer within only one page.
I had referred to drivers/mtd/onenand/samsung.c and other files.
The corresponding code segment is as follows:
static int s5pc110_read_bufferram(struct mtd_info *mtd, int area,
                unsigned char *buffer, int offset, size_t count)
{
	void *buf = (void *) buffer;
        dma_addr_t dma_src, dma_dst;
	...
	/* Handle vmalloc address */
        if (buf >= high_memory) {
                struct page *page;

                if (((size_t) buf & PAGE_MASK) !=
                    ((size_t) (buf + count - 1) & PAGE_MASK))
                        goto normal;
                page = vmalloc_to_page(buf);
                if (!page)
                        goto normal;

                ...
        } else {
		...
	}

normal:
        ...
        memcpy(buffer, p, count);

	return 0;
}
I think memcpy in "normal" clause can't be removed. So I'd like to keep my original
implementation if it is also OK. What's your opinion?

Regards,
Jiancheng







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

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Boris Brezillon]

+Russell

Hi Jiancheng,

On Tue, 12 Apr 2016 17:32:08 +0800
Jiancheng Xue <xuejiancheng@huawei.com> wrote:

> Hi Marek,
> 
> On 2016/4/12 3:21, Marek Vasut wrote:
> > On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
> >> Hi,
> >>
> >> On 2016/4/8 18:04, Marek Vasut wrote:
> >>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
> >>>> Hi,
> >>>>
> >>>> On 2016/4/7 10:28, Marek Vasut wrote:
> >>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
> >>>>>> Hi Brian,
> >>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
> >>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
> >>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
> >>>>>
> >>>>> Would you please stop top-posting ? It rubs some people the wrong way.
> >>>>>
> >>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
> >>>>
> >>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
> >>>>>> 		size_t *retlen, u_char *read_buf)
> >>>>>> {
> >>>>>> 	struct hifmc_priv *priv = nor->priv;
> >>>>>> 	struct hifmc_host *host = priv->host;
> >>>>>> 	int i;
> >>>>>>
> >>>>>> 	/* read all bytes in only one time */
> >>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
> >>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
> >>>>>> 				len, FMC_OP_READ);
> >>>>>> 		memcpy(read_buf, host->buffer, len);
> >>>>>
> >>>>> Is all the ad-hoc memcpying necessary? I think you can use
> >>>>> dma_map_single() and co to obtain DMAble memory for your
> >>>>> controller's use and then you can probably get rid of most
> >>>>> of this stuff.
> >>>>>
> >>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
> >>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
> >>>> this. Please let me know if I am wrong. Thank you!
> >>>
> >>> Does your controller/DMA have a limitation where it's buffers must be in
> >>> the bottom 4GiB range ? The DMA framework should be able to take care of
> >>> such platform limitations.
> >>>
> >> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
> >> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
> >> needed. Am I right?
> > 
> > Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
> > solution help you in any way ?
> > 
> No. I think this solution just processes the buffer within only one page.
> I had referred to drivers/mtd/onenand/samsung.c and other files.
> The corresponding code segment is as follows:
> static int s5pc110_read_bufferram(struct mtd_info *mtd, int area,
>                 unsigned char *buffer, int offset, size_t count)
> {
> 	void *buf = (void *) buffer;
>         dma_addr_t dma_src, dma_dst;
> 	...
> 	/* Handle vmalloc address */
>         if (buf >= high_memory) {
>                 struct page *page;
> 
>                 if (((size_t) buf & PAGE_MASK) !=
>                     ((size_t) (buf + count - 1) & PAGE_MASK))
>                         goto normal;
>                 page = vmalloc_to_page(buf);
>                 if (!page)
>                         goto normal;
> 
>                 ...
>         } else {
> 		...
> 	}
> 
> normal:
>         ...
>         memcpy(buffer, p, count);
> 
> 	return 0;
> }
> I think memcpy in "normal" clause can't be removed. So I'd like to keep my original
> implementation if it is also OK. What's your opinion?

You might want to have a look at this series [1], and particularly at
Russell's answers regarding DMA operations on non-lowmem memory.

Best Regards,

Boris

[1]http://thread.gmane.org/gmane.linux.kernel.mm/149015

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Boris Brezillon]

+Russell

Hi Jiancheng,

On Tue, 12 Apr 2016 17:32:08 +0800
Jiancheng Xue <xuejiancheng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:

> Hi Marek,
> 
> On 2016/4/12 3:21, Marek Vasut wrote:
> > On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
> >> Hi,
> >>
> >> On 2016/4/8 18:04, Marek Vasut wrote:
> >>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
> >>>> Hi,
> >>>>
> >>>> On 2016/4/7 10:28, Marek Vasut wrote:
> >>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
> >>>>>> Hi Brian,
> >>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
> >>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
> >>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
> >>>>>
> >>>>> Would you please stop top-posting ? It rubs some people the wrong way.
> >>>>>
> >>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
> >>>>
> >>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
> >>>>>> 		size_t *retlen, u_char *read_buf)
> >>>>>> {
> >>>>>> 	struct hifmc_priv *priv = nor->priv;
> >>>>>> 	struct hifmc_host *host = priv->host;
> >>>>>> 	int i;
> >>>>>>
> >>>>>> 	/* read all bytes in only one time */
> >>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
> >>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
> >>>>>> 				len, FMC_OP_READ);
> >>>>>> 		memcpy(read_buf, host->buffer, len);
> >>>>>
> >>>>> Is all the ad-hoc memcpying necessary? I think you can use
> >>>>> dma_map_single() and co to obtain DMAble memory for your
> >>>>> controller's use and then you can probably get rid of most
> >>>>> of this stuff.
> >>>>>
> >>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
> >>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
> >>>> this. Please let me know if I am wrong. Thank you!
> >>>
> >>> Does your controller/DMA have a limitation where it's buffers must be in
> >>> the bottom 4GiB range ? The DMA framework should be able to take care of
> >>> such platform limitations.
> >>>
> >> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
> >> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
> >> needed. Am I right?
> > 
> > Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
> > solution help you in any way ?
> > 
> No. I think this solution just processes the buffer within only one page.
> I had referred to drivers/mtd/onenand/samsung.c and other files.
> The corresponding code segment is as follows:
> static int s5pc110_read_bufferram(struct mtd_info *mtd, int area,
>                 unsigned char *buffer, int offset, size_t count)
> {
> 	void *buf = (void *) buffer;
>         dma_addr_t dma_src, dma_dst;
> 	...
> 	/* Handle vmalloc address */
>         if (buf >= high_memory) {
>                 struct page *page;
> 
>                 if (((size_t) buf & PAGE_MASK) !=
>                     ((size_t) (buf + count - 1) & PAGE_MASK))
>                         goto normal;
>                 page = vmalloc_to_page(buf);
>                 if (!page)
>                         goto normal;
> 
>                 ...
>         } else {
> 		...
> 	}
> 
> normal:
>         ...
>         memcpy(buffer, p, count);
> 
> 	return 0;
> }
> I think memcpy in "normal" clause can't be removed. So I'd like to keep my original
> implementation if it is also OK. What's your opinion?

You might want to have a look at this series [1], and particularly at
Russell's answers regarding DMA operations on non-lowmem memory.

Best Regards,

Boris

[1]http://thread.gmane.org/gmane.linux.kernel.mm/149015

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Boris,

On 2016/4/12 17:44, Boris Brezillon wrote:
> +Russell
> 
> Hi Jiancheng,
> 
> On Tue, 12 Apr 2016 17:32:08 +0800
> Jiancheng Xue <xuejiancheng@huawei.com> wrote:
> 
>> Hi Marek,
>>
>> On 2016/4/12 3:21, Marek Vasut wrote:
>>> On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
>>>> Hi,
>>>>
>>>> On 2016/4/8 18:04, Marek Vasut wrote:
>>>>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2016/4/7 10:28, Marek Vasut wrote:
>>>>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>>>>>> Hi Brian,
>>>>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>>>>>
>>>>>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>>>>>
>>>>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>>>>>
>>>>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>>>> 		size_t *retlen, u_char *read_buf)
>>>>>>>> {
>>>>>>>> 	struct hifmc_priv *priv = nor->priv;
>>>>>>>> 	struct hifmc_host *host = priv->host;
>>>>>>>> 	int i;
>>>>>>>>
>>>>>>>> 	/* read all bytes in only one time */
>>>>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>>>>>> 				len, FMC_OP_READ);
>>>>>>>> 		memcpy(read_buf, host->buffer, len);
>>>>>>>
>>>>>>> Is all the ad-hoc memcpying necessary? I think you can use
>>>>>>> dma_map_single() and co to obtain DMAble memory for your
>>>>>>> controller's use and then you can probably get rid of most
>>>>>>> of this stuff.
>>>>>>>
>>>>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>>>>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>>>>>> this. Please let me know if I am wrong. Thank you!
>>>>>
>>>>> Does your controller/DMA have a limitation where it's buffers must be in
>>>>> the bottom 4GiB range ? The DMA framework should be able to take care of
>>>>> such platform limitations.
>>>>>
>>>> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
>>>> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
>>>> needed. Am I right?
>>>
>>> Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
>>> solution help you in any way ?
>>>
>> No. I think this solution just processes the buffer within only one page.
>> I had referred to drivers/mtd/onenand/samsung.c and other files.
>> The corresponding code segment is as follows:
>> static int s5pc110_read_bufferram(struct mtd_info *mtd, int area,
>>                 unsigned char *buffer, int offset, size_t count)
>> {
>> 	void *buf = (void *) buffer;
>>         dma_addr_t dma_src, dma_dst;
>> 	...
>> 	/* Handle vmalloc address */
>>         if (buf >= high_memory) {
>>                 struct page *page;
>>
>>                 if (((size_t) buf & PAGE_MASK) !=
>>                     ((size_t) (buf + count - 1) & PAGE_MASK))
>>                         goto normal;
>>                 page = vmalloc_to_page(buf);
>>                 if (!page)
>>                         goto normal;
>>
>>                 ...
>>         } else {
>> 		...
>> 	}
>>
>> normal:
>>         ...
>>         memcpy(buffer, p, count);
>>
>> 	return 0;
>> }
>> I think memcpy in "normal" clause can't be removed. So I'd like to keep my original
>> implementation if it is also OK. What's your opinion?
> 
> You might want to have a look at this series [1], and particularly at
> Russell's answers regarding DMA operations on non-lowmem memory.
> 
Thank you very much for your supplied reference. Besides safety reasons described by Russell,
the dmaengine embeded in this controller doesn't support scatter-list type buffer directly.
So for this controller, I think now it's better to obtain buffer through dma_alloc_coherent
for dma operation, and then copy data to buffers supplied by mtd user. May I keep using this
implementation now?

Regards,
Jiancheng

Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash controller driver

[Jiancheng Xue]

Hi Boris,

On 2016/4/12 17:44, Boris Brezillon wrote:
> +Russell
> 
> Hi Jiancheng,
> 
> On Tue, 12 Apr 2016 17:32:08 +0800
> Jiancheng Xue <xuejiancheng-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> wrote:
> 
>> Hi Marek,
>>
>> On 2016/4/12 3:21, Marek Vasut wrote:
>>> On 04/11/2016 03:28 AM, Jiancheng Xue wrote:
>>>> Hi,
>>>>
>>>> On 2016/4/8 18:04, Marek Vasut wrote:
>>>>> On 04/08/2016 10:26 AM, Jiancheng Xue wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 2016/4/7 10:28, Marek Vasut wrote:
>>>>>>> On 04/07/2016 04:10 AM, Jiancheng Xue wrote:
>>>>>>>> Hi Brian,
>>>>>>>>    Thank you very much for your comments. I'll fix these issues in next version.
>>>>>>>> In addition, for easy understanding I'd like to rewrite hisi_spi_nor_write and
>>>>>>>> hisi_spi_nor_read. Your comments on these modifications will be highly appreciated.
>>>>>>>
>>>>>>> Would you please stop top-posting ? It rubs some people the wrong way.
>>>>>>>
>>>>>> I feel very sorry about that. I have read the etiquette and won't make the same mistake again.
>>>>>>
>>>>>>>> static int hisi_spi_nor_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>>>> 		size_t *retlen, u_char *read_buf)
>>>>>>>> {
>>>>>>>> 	struct hifmc_priv *priv = nor->priv;
>>>>>>>> 	struct hifmc_host *host = priv->host;
>>>>>>>> 	int i;
>>>>>>>>
>>>>>>>> 	/* read all bytes in only one time */
>>>>>>>> 	if (len <= HIFMC_DMA_MAX_LEN) {
>>>>>>>> 		hisi_spi_nor_dma_transfer(nor, from, host->dma_buffer,
>>>>>>>> 				len, FMC_OP_READ);
>>>>>>>> 		memcpy(read_buf, host->buffer, len);
>>>>>>>
>>>>>>> Is all the ad-hoc memcpying necessary? I think you can use
>>>>>>> dma_map_single() and co to obtain DMAble memory for your
>>>>>>> controller's use and then you can probably get rid of most
>>>>>>> of this stuff.
>>>>>>>
>>>>>> Considering read_buf >= high_mem case, I think it is also complicated to use dma_map_*
>>>>>> and the DMA buffer allocated by the driver is still needed. But I am not sure about
>>>>>> this. Please let me know if I am wrong. Thank you!
>>>>>
>>>>> Does your controller/DMA have a limitation where it's buffers must be in
>>>>> the bottom 4GiB range ? The DMA framework should be able to take care of
>>>>> such platform limitations.
>>>>>
>>>> When read_buf is allocated by vmalloc, the underlying physical memory may be not contiguous.
>>>> In this case, dma_map_single can't be used directly. I think inner DMA buffer and memcpy are still
>>>> needed. Am I right?
>>>
>>> Take a look at drivers/spi/spi-mxs.c , look for "vmalloc" , does that
>>> solution help you in any way ?
>>>
>> No. I think this solution just processes the buffer within only one page.
>> I had referred to drivers/mtd/onenand/samsung.c and other files.
>> The corresponding code segment is as follows:
>> static int s5pc110_read_bufferram(struct mtd_info *mtd, int area,
>>                 unsigned char *buffer, int offset, size_t count)
>> {
>> 	void *buf = (void *) buffer;
>>         dma_addr_t dma_src, dma_dst;
>> 	...
>> 	/* Handle vmalloc address */
>>         if (buf >= high_memory) {
>>                 struct page *page;
>>
>>                 if (((size_t) buf & PAGE_MASK) !=
>>                     ((size_t) (buf + count - 1) & PAGE_MASK))
>>                         goto normal;
>>                 page = vmalloc_to_page(buf);
>>                 if (!page)
>>                         goto normal;
>>
>>                 ...
>>         } else {
>> 		...
>> 	}
>>
>> normal:
>>         ...
>>         memcpy(buffer, p, count);
>>
>> 	return 0;
>> }
>> I think memcpy in "normal" clause can't be removed. So I'd like to keep my original
>> implementation if it is also OK. What's your opinion?
> 
> You might want to have a look at this series [1], and particularly at
> Russell's answers regarding DMA operations on non-lowmem memory.
> 
Thank you very much for your supplied reference. Besides safety reasons described by Russell,
the dmaengine embeded in this controller doesn't support scatter-list type buffer directly.
So for this controller, I think now it's better to obtain buffer through dma_alloc_coherent
for dma operation, and then copy data to buffers supplied by mtd user. May I keep using this
implementation now?

Regards,
Jiancheng

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