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

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

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.

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

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

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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);
> 
> .
> 

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

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);
>>
>> .
>>
>

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 ?

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

+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

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

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