[PATCH v3 2/2] mtd: spi-nor: add rockchip serial flash controller driver

[Shawn Lin]

On 2016/12/5 11:40, Marek Vasut wrote:
> On 12/05/2016 03:56 AM, Shawn Lin wrote:
>> Add rockchip serial flash controller driver
>>
>> Signed-off-by: Shawn Lin <shawn.lin at rock-chips.com>
>>
>
> Looks good, a few nits below.
>
> [...]
>
>> +static int get_if_type(struct rockchip_sfc *sfc, enum read_mode flash_read)
>> +{
>> +	enum rockchip_sfc_iftype if_type;
>
> Wouldn't it be shorter if you used if-return below ?
> Example
>
> if (flash_read == SPI_NOR_QUAD)
> 	return IF_TYPE_QUAD;
>
> if (flash_read == SPI_NOR_DUAL)
> 	return IF_TYPE_DUAL;
> ...

ok, will improve.

>
> dev_err(sfc->dev, "unsupported SPI read mode\n");
> return -EINVAL;
>
>> +	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:
>> +		if_type = IF_TYPE_STD;
>> +		break;
>> +	default:
>> +		dev_err(sfc->dev, "unsupported SPI read mode\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return if_type;
>> +}
>
> [...]
>
>> +static inline void rockchip_sfc_setup_transfer(struct spi_nor *nor,
>> +					       loff_t from_to,
>> +					       size_t len, u8 op_type)
>> +{
>> +	struct rockchip_sfc_chip_priv *priv = nor->priv;
>> +	struct rockchip_sfc *sfc = priv->sfc;
>> +	u32 reg;
>> +	u8 if_type = 0;
>> +
>> +	if_type = get_if_type(sfc, nor->flash_read);
>> +	writel_relaxed((if_type << SFC_CTRL_DATA_BITS_SHIFT) |
>> +		       (if_type << SFC_CTRL_ADDR_BITS_SHIFT) |
>> +		       (if_type << SFC_CTRL_CMD_BITS_SHIFT) |
>
> Hm, looking at this, does the controller only support n-n-n mode (1-1-1,
> 2-2-2, 4-4-4) ? Or why don't you allow 1-1-n/1-n-n/2-n-n ?

No, it also could support 1-1-n, etc.
By looking at the cadence-quadspi.c,  it only allows
CQSPI_INST_TYPE_SINGLE for f_pdata->addr_width and f_pdata->inst_width,
so finally it only supports 1-1-1, 1-1-2, 1-1-4?

> I would like to hear some input from Cyrille on this one.
>
>> +		       (sfc->negative_edge ? SFC_CTRL_PHASE_SEL_NEGETIVE : 0),
>> +		       sfc->regbase + SFC_CTRL);
>> +
>> +	if (op_type == SFC_CMD_DIR_WR)
>> +		reg = nor->program_opcode << SFC_CMD_IDX_SHIFT;
>> +	else
>> +		reg = nor->read_opcode << SFC_CMD_IDX_SHIFT;
>> +
>> +	reg |= op_type << SFC_CMD_DIR_SHIFT;
>> +	reg |= (nor->addr_width == 4) ?
>> +		SFC_CMD_ADDR_32BITS : SFC_CMD_ADDR_24BITS;
>> +
>> +	reg |= priv->cs << SFC_CMD_CS_SHIFT;
>> +	reg |= len << SFC_CMD_TRAN_BYTES_SHIFT;
>> +
>> +	if (op_type == SFC_CMD_DIR_RD)
>> +		reg |= SFC_CMD_DUMMY(nor->read_dummy);
>> +
>> +	/* Should minus one as 0x0 means 1 bit flash address */
>> +	writel_relaxed(nor->addr_width * 8 - 1, sfc->regbase + SFC_ABIT);
>> +	writel_relaxed(reg, sfc->regbase + SFC_CMD);
>> +	writel_relaxed(from_to, sfc->regbase + SFC_ADDR);
>> +}
>
>
> [...]
>
>> +static int rockchip_sfc_dma_transfer(struct spi_nor *nor, loff_t from_to,
>> +				     size_t len, u_char *buf, u8 op_type)
>> +{
>> +	struct rockchip_sfc_chip_priv *priv = nor->priv;
>> +	struct rockchip_sfc *sfc = priv->sfc;
>> +	size_t offset;
>> +	int ret;
>> +	dma_addr_t dma_addr = 0;
>
> Nit, you can precalculate the DMA_TO/FROM_DEVICE and store it to
> variable here, ie.
>
> dma_dir = (op_type == SFC_CMD_DIR_RD) ? DMA_FROM_DEVICE : DMA_TO_DEVICE.
>
>> +	for (offset = 0; offset < len; offset += SFC_DMA_MAX_LEN) {
>> +		size_t trans = min_t(size_t, SFC_DMA_MAX_LEN, len - offset);
>> +
>> +		if (SFC_CMD_DIR_RD)
>
> if (op_type == is missing, but you can drop this, see above.

okay

>
>> +			dma_addr = dma_map_single(NULL, (void *)buf,
>> +						  trans, DMA_FROM_DEVICE);
>> +		else
>> +			dma_addr = dma_map_single(NULL, (void *)buf,
>> +						  trans, DMA_TO_DEVICE);
>
> You can use dma_dir here ^ and drop the condition.

sure

>
>> +		if (dma_mapping_error(sfc->dev, dma_addr)) {
>> +			/*
>> +			 * If we use pre-allocated dma_buffer, we need to
>> +			 * do a copy here.
>> +			 */
>> +			if (op_type == SFC_CMD_DIR_WR)
>> +				memcpy(sfc->buffer, buf + offset, trans);
>> +
>> +			dma_addr = 0;
>> +		}
>> +
>> +		if (op_type == SFC_CMD_DIR_WR)
>> +			/*
>> +			 * Flush the write data from write_buf to dma_addr
>> +			 * if using dynamic allocated dma buffer before dma
>> +			 * moves data from dma_addr to fifo.
>> +			 */
>> +			dma_sync_single_for_device(sfc->dev, dma_addr,
>> +						   trans, DMA_TO_DEVICE);
>> +
>> +
>> +		/* If failing to map dma, use pre-allocated area instead */
>> +		ret = rockchip_sfc_do_dma_transfer(nor, from_to + offset,
>> +						dma_addr ? dma_addr :
>> +						sfc->dma_buffer,
>> +						trans, op_type);
>> +
>> +		if (dma_addr) {
>> +			/*
>> +			 * Invalidate the read data from dma_addr if using
>> +			 * dynamic allocated dma buffer after dma moves data
>> +			 * from fifo to dma_addr.
>> +			 */
>> +			if (op_type == SFC_CMD_DIR_RD) {
>> +				dma_sync_single_for_cpu(sfc->dev, dma_addr,
>> +							trans, DMA_FROM_DEVICE);
>> +				dma_unmap_single(NULL, dma_addr,
>> +						 trans, DMA_FROM_DEVICE);
>> +			} else {
>> +				dma_unmap_single(NULL, dma_addr,
>> +						 trans, DMA_TO_DEVICE);
>
> Here as well and it'd be reduced to
>
> if (...)
>   dma_sync_single...()
> dma_unmap( ... , dma_dir);

sure.

>
>> +			}
>> +		}
>> +
>> +		if (ret) {
>> +			dev_warn(nor->dev, "DMA read timeout\n");
>> +			return ret;
>> +		}
>> +		/*
>> +		 * If we use pre-allocated dma_buffer for read, we need to
>> +		 * do a copy here.
>> +		 */
>> +		if (!dma_addr && (op_type == SFC_CMD_DIR_RD))
>> +			memcpy(buf + offset, sfc->buffer, trans);
>> +	}
>> +
>> +	return len;
>> +}
>> +
>> +static ssize_t rockchip_sfc_do_rd_wr(struct spi_nor *nor, loff_t from_to,
>> +				     size_t len, u_char *buf, u32 op_type)
>> +{
>> +	struct rockchip_sfc_chip_priv *priv = nor->priv;
>> +	struct rockchip_sfc *sfc = priv->sfc;
>> +	int ret;
>> +
>> +	if (!sfc->use_dma)
>> +		goto no_dma;
>> +
>> +	return rockchip_sfc_dma_transfer(nor, from_to, len,
>> +					 buf, op_type);
>
> if (likely(sfc->use_dma))
>   return rockchip_sfc_dma...();
>
> /* Comment saying that we fall back to PIO */
> ... pio code ...
>

sure.

>> +no_dma:
>> +	ret = rockchip_sfc_pio_transfer(nor, from_to, len,
>> +					(u_char *)buf, op_type);
>> +	if (ret) {
>> +		if (op_type == SFC_CMD_DIR_RD)
>> +			dev_warn(nor->dev, "PIO read timeout\n");
>> +		else
>> +			dev_warn(nor->dev, "PIO write timeout\n");
>> +		return ret;
>> +	}
>> +
>> +	return len;
>> +}
>
> [...]
>
>> +/**
>
> Drop this asterisk unless you document the driver in kerneldoc.

okay

>
>> + * Get spi flash device information and register it as a mtd device.
>> + */
>> +static int rockchip_sfc_register(struct device_node *np,
>> +				 struct rockchip_sfc *sfc)
>> +{
>> +	struct device *dev = sfc->dev;
>> +	struct mtd_info *mtd;
>> +	struct spi_nor *nor;
>> +	int ret;
>> +
>> +	nor = &(sfc->flash[sfc->num_chip].nor);
>
> Parenthesis not needed.

sure.

>
>> +	nor->dev = dev;
>> +	spi_nor_set_flash_node(nor, np);
>> +
>> +	ret = of_property_read_u8(np, "reg", &sfc->flash[sfc->num_chip].cs);
>> +	if (ret) {
>> +		dev_err(dev, "No reg property for %s\n",
>> +			np->full_name);
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "spi-max-frequency",
>> +			&sfc->flash[sfc->num_chip].clk_rate);
>> +	if (ret) {
>> +		dev_err(dev, "No spi-max-frequency property for %s\n",
>> +			np->full_name);
>> +		return ret;
>> +	}
>> +
>> +	sfc->flash[sfc->num_chip].sfc = sfc;
>> +	nor->priv = &(sfc->flash[sfc->num_chip]);
>> +
>> +	nor->prepare = rockchip_sfc_prep;
>> +	nor->unprepare = rockchip_sfc_unprep;
>> +	nor->read_reg = rockchip_sfc_read_reg;
>> +	nor->write_reg = rockchip_sfc_write_reg;
>> +	nor->read = rockchip_sfc_read;
>> +	nor->write = rockchip_sfc_write;
>> +	nor->erase = NULL;
>> +	ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mtd = &(nor->mtd);
>> +	mtd->name = np->name;
>> +	ret = mtd_device_register(mtd, NULL, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	sfc->num_chip++;
>> +	return 0;
>> +}
>> +
>> +static void rockchip_sfc_unregister_all(struct rockchip_sfc *sfc)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < sfc->num_chip; i++)
>> +		mtd_device_unregister(&(sfc->flash[i].nor.mtd));
>
> Inner parenthesis not needed IMO

okay.

>
>> +}
>> +
>> +static int rockchip_sfc_register_all(struct rockchip_sfc *sfc)
>> +{
>> +	struct device *dev = sfc->dev;
>> +	struct device_node *np;
>> +	int ret;
>> +
>> +	for_each_available_child_of_node(dev->of_node, np) {
>> +		ret = rockchip_sfc_register(np, sfc);
>> +		if (ret)
>> +			goto fail;
>> +
>> +		if (sfc->num_chip == SFC_MAX_CHIPSELECT_NUM) {
>> +			dev_warn(dev, "Exceeds the max cs limitation\n");
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +fail:
>> +	dev_err(dev, "Failed to register all chips\n");
>> +	/* Unregister all the _registered_ nor flash */
>> +	rockchip_sfc_unregister_all(sfc);
>> +	return ret;
>> +}
>
>
> [...]
>
>> +#ifdef CONFIG_PM
>> +int rockchip_sfc_runtime_suspend(struct device *dev)
>> +{
>> +	struct rockchip_sfc *sfc = dev_get_drvdata(dev);
>> +
>> +	clk_disable_unprepare(sfc->hclk);
>> +	return 0;
>> +}
>
> Was the suspend ever really tested with this block ? Is disabling clock
> really enough ?

It was tested and we could do more, for instance power off the genpd,
but disabling clcok should be enough.

>
>> +int rockchip_sfc_runtime_resume(struct device *dev)
>> +{
>> +	struct rockchip_sfc *sfc = dev_get_drvdata(dev);
>> +
>> +	clk_prepare_enable(sfc->hclk);
>> +	return 0;
>> +}
>> +#endif /* CONFIG_PM */
>
> [...]
>


-- 
Best Regards
Shawn Lin