Subject: Re: [RESEND PATCH v9] mtd: spi-nor: add hisilicon spi-nor flash
 controller driver
To: Marek Vasut <marek.vasut@gmail.com>, <linux-mtd@lists.infradead.org>
References: <1458979861-3619-1-git-send-email-xuejiancheng@huawei.com>
 <56F73BC9.5000300@gmail.com>
CC: <robh+dt@kernel.org>, <dwmw2@infradead.org>, <computersforpeace@gmail.com>,
        <boris.brezillon@free-electrons.com>, <juhosg@openwrt.org>,
        <furquan@google.com>, <suwenping@hisilicon.com>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <yanhaifeng@hisilicon.com>, <raojun@hisilicon.com>,
        <xuejiancheng@hisilicon.com>, <ml.yang@hisilicon.com>,
        <gaofei@hisilicon.com>, <yanghongwei@hisilicon.com>,
        <zhangzhenxing@hisilicon.com>
From: Jiancheng Xue <xuejiancheng@huawei.com>
Message-ID: <56F8F630.5050008@huawei.com>
Date: Mon, 28 Mar 2016 17:15:28 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101
 Thunderbird/38.5.0
MIME-Version: 1.0
In-Reply-To: <56F73BC9.5000300@gmail.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 12299
Lines: 442

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