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
