Hi Brian, On 2016/4/27 23:38, Brian Norris wrote: > Hi Jiancheng, > > Cyrille hit on one of my concerns; if posible we'd like not to have you > sniffing the opcodes in read_reg()/write_reg(). But let's keep the > discussion on that thread. > OK. I'll look into this issue seriously and reply on that thread later.
> Two other comments below. > > On Tue, Apr 19, 2016 at 03:27:19PM +0800, Jiancheng Xue wrote: >> From: Jiancheng Xue <[email protected]> >> >> Add hisilicon spi-nor flash controller driver >> >> Signed-off-by: Binquan Peng <[email protected]> >> Signed-off-by: Jiancheng Xue <[email protected]> >> Acked-by: Rob Herring <[email protected]> >> Reviewed-by: Ezequiel Garcia <[email protected]> >> --- [...] >> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c >> new file mode 100644 >> index 0000000..942dafd >> --- /dev/null >> +++ b/drivers/mtd/spi-nor/hisi-sfc.c >> @@ -0,0 +1,539 @@ > > [...] > >> +static void hisi_spi_nor_dma_transfer(struct spi_nor *nor, loff_t start_off, >> + dma_addr_t dma_buf, size_t len, u8 op_type) >> +{ >> + struct hifmc_priv *priv = nor->priv; >> + struct hifmc_host *host = priv->host; >> + u8 if_type = 0, dummy = 0; >> + u8 w_cmd = 0, r_cmd = 0; >> + u32 reg; >> + >> + writel(start_off, host->regbase + FMC_ADDRL); >> + >> + if (op_type == FMC_OP_READ) { >> + if_type = get_if_type(nor->flash_read); >> + dummy = nor->read_dummy >> 3; >> + r_cmd = nor->read_opcode; >> + } else { >> + w_cmd = nor->program_opcode; >> + } >> + >> + reg = OP_CFG_FM_CS(priv->chipselect) >> + | OP_CFG_MEM_IF_TYPE(if_type) >> + | OP_CFG_ADDR_NUM(nor->addr_width) >> + | OP_CFG_DUMMY_NUM(dummy); >> + writel(reg, host->regbase + FMC_OP_CFG); >> + >> + reg = FMC_DMA_LEN_SET(len); >> + writel(reg, host->regbase + FMC_DMA_LEN); >> + writel(dma_buf, host->regbase + FMC_DMA_SADDR_D0); >> + >> + reg = OP_CTRL_RD_OPCODE(r_cmd) >> + | OP_CTRL_WR_OPCODE(w_cmd) >> + | OP_CTRL_RW_OP(op_type) >> + | OP_CTRL_DMA_OP_READY; >> + writel(0xff, host->regbase + FMC_INT_CLR); >> + writel(reg, host->regbase + FMC_OP_DMA); >> + wait_op_finish(host); > > Do you want to return the status here, so we can handle errors? > OK. I'll fix it in v11. >> +} >> + >> +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 offset; >> + >> + /* read all bytes in only one read */ >> + 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); > > Why do you have a special case for "all in one read"? This is just a > basic loop... > See below. >> + } else { >> + /* read HIFMC_DMA_MAX_LEN bytes at a time */ >> + for (offset = 0; offset < len; offset += HIFMC_DMA_MAX_LEN) { >> + hisi_spi_nor_dma_transfer(nor, from + offset, >> + host->dma_buffer, >> + HIFMC_DMA_MAX_LEN, FMC_OP_READ); >> + memcpy(read_buf + offset, >> + host->buffer, HIFMC_DMA_MAX_LEN); >> + } >> + /* read remaining bytes */ >> + offset -= HIFMC_DMA_MAX_LEN; >> + hisi_spi_nor_dma_transfer(nor, from + offset, host->dma_buffer, >> + len - offset, FMC_OP_READ); >> + memcpy(read_buf + offset, host->buffer, len - offset); >> + } > > I think the entire if/else can be rewritten as: > > for (offset = 0; offset < len; offset += HIFMC_DMA_MAX_LEN) { > size_t trans = min(HIFMC_DMA_MAX_LEN, len - offset); > > hisi_spi_nor_dma_transfer(nor, from + offset, host->dma_buffer, > trans, FMC_OP_READ); > memcpy(read_buf + offset, host->buffer, trans); > } > I had referred to the implementation of spi_nor_write. But now it seems much simpler and clearer above. I'll apply this code in v11. Thank you very much. >> + *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; >> + >> + /* len is smaller than HIFMC_DMA_MAX_LEN*/ > > Where do you get that assumption? This function must handle whatever > 'len' is passed to it, and that may be large. (I suspect your error is > inspired by the fact that mtd-utils *usually* does smaller write > transfers.) > In spi_nor_write, nor->write is called with the len not bigger than nor->page_size. I wrongly thought nor->page_size was always smaller than HIFMC_DMA_MAX_LEN. I'll rewrite this function referring to _read function. Thank you! Regards, Jiancheng
