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 <xuejiancheng@xxxxxxxxxx>
>>
>> Add hisilicon spi-nor flash controller driver
>>
>> Signed-off-by: Binquan Peng <pengbinquan@xxxxxxxxxxxxx>
>> Signed-off-by: Jiancheng Xue <xuejiancheng@xxxxxxxxxxxxx>
>> Acked-by: Rob Herring <robh@xxxxxxxxxx>
>> Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
>> ---
[...]
>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
