Re: [PATCH 2/2] mtd: spi-nor: cadence-quadspi: Add support for direct access mode

[Vignesh R]

Hi Cyrille

On Wednesday 20 December 2017 08:43 PM, Cyrille Pitchen wrote:
> Hi Vignesh,
> 
> Le 07/12/2017 Ã 07:38, Vignesh R a Ãcrit :
>> Cadence QSPI controller provides direct access mode through which flash
>> can be accessed in a memory-mapped IO mode. This enables read/write to
>> flash using memcpy*() functions. This mode provides higher throughput
>> for both read/write operations when compared to current indirect mode of
>> operation.
>> 
>> This patch therefore adds support to use QSPI in direct mode. If the
>> window reserved in SoC's memory map for MMIO access is less that of
>> flash size(like on most SoCFPGA variants), then the driver falls back
>> to indirect mode of operation.
>> 
>> On TI's 66AK2G SoC, with ARM running at 600MHz and QSPI at 96MHz
>> switching to direct mode improves read throughput from 3MB/s to 8MB/s.
>> 
>> Signed-off-by: Vignesh R <vigneshr@xxxxxx>
>> ---

[...]

>> +static int cqspi_direct_read_execute(struct spi_nor *nor, u8 *rxbuf,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t from_addr, const size_t len)
>> +{
>> +ÂÂÂÂ struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +ÂÂÂÂ struct cqspi_st *cqspi = f_pdata->cqspi;
>> +ÂÂÂÂ u32 reg;
>> +
>> +ÂÂÂÂ reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +ÂÂÂÂ reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +ÂÂÂÂ writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> I guess setting the ENB_DIR_ACC_CTRL bit could be set once for all when you
> set use_direct_mode to true, couldn't it?
> 
> It may improve the read performance even more. However not expecting much
> difference for Page Program operations.
> 
> Then you could call directly call mempcy_fromio() from cqspi_read().
> Not mandatory for me, since I also like the symmetry of the 2 functions:
> cqspi_direct_read_execute() / cqspi_indirect_read_execute().
> 

Right, actually I can unconditionally set ENB_DIR_ACC_CTRL once in
cqspi_controller_init(). Indirect accesses will still be forwarded to
indirect access controller even if direct access controller is kept
enabled. So, indirect ops users are not affected.
I will make the changes in v2.

> So it's up to you :)
> 
>> +ÂÂÂÂ memcpy_fromio(rxbuf, cqspi->ahb_base + from_addr, len);
>> +
>> +ÂÂÂÂ return 0;
>> +}
>> +
>>Â static int cqspi_write_setup(struct spi_nor *nor)
>>Â {
>>ÂÂÂÂÂÂÂ unsigned int reg;
>> @@ -671,6 +689,21 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor, loff_t to_addr,
>>ÂÂÂÂÂÂÂ return ret;
>>Â }
>>Â 
>> +static int cqspi_direct_write_execute(struct spi_nor *nor, loff_t to_addr,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const u8 *txbuf, const size_t len)
>> +{
>> +ÂÂÂÂ struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +ÂÂÂÂ struct cqspi_st *cqspi = f_pdata->cqspi;
>> +ÂÂÂÂ u32 reg;
>> +
>> +ÂÂÂÂ reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>> +ÂÂÂÂ reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL;
>> +ÂÂÂÂ writel(reg, cqspi->iobase + CQSPI_REG_CONFIG);
> 
> Same comment here.
> 
>> +ÂÂÂÂ memcpy_toio(cqspi->ahb_base + to_addr, txbuf, len);
>> +
>> +ÂÂÂÂ return 0;
>> +}
>> +
>>Â static void cqspi_chipselect(struct spi_nor *nor)
>>Â {
>>ÂÂÂÂÂÂÂ struct cqspi_flash_pdata *f_pdata = nor->priv;
>> @@ -891,6 +924,7 @@ static int cqspi_set_protocol(struct spi_nor *nor, const int read)
>>Â static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t len, const u_char *buf)
>>Â {
>> +ÂÂÂÂ struct cqspi_flash_pdata *f_pdata = nor->priv;
>>ÂÂÂÂÂÂÂ int ret;
>>Â 
>>ÂÂÂÂÂÂÂ ret = cqspi_set_protocol(nor, 0);
>> @@ -901,7 +935,10 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>ÂÂÂÂÂÂÂ if (ret)
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
>>Â 
>> -ÂÂÂÂ ret = cqspi_indirect_write_execute(nor, to, buf, len);
>> +ÂÂÂÂ if (f_pdata->use_direct_mode)
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ ret = cqspi_direct_write_execute(nor, to, buf, len);
>> +ÂÂÂÂ else
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ ret = cqspi_indirect_write_execute(nor, to, buf, len);
>>ÂÂÂÂÂÂÂ if (ret)
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
>>Â 
>> @@ -911,6 +948,7 @@ static ssize_t cqspi_write(struct spi_nor *nor, loff_t to,
>>Â static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t len, u_char *buf)
>>Â {
>> +ÂÂÂÂ struct cqspi_flash_pdata *f_pdata = nor->priv;
>>ÂÂÂÂÂÂÂ int ret;
>>Â 
>>ÂÂÂÂÂÂÂ ret = cqspi_set_protocol(nor, 1);
>> @@ -921,7 +959,10 @@ static ssize_t cqspi_read(struct spi_nor *nor, loff_t from,
>>ÂÂÂÂÂÂÂ if (ret)
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
>>Â 
>> -ÂÂÂÂ ret = cqspi_indirect_read_execute(nor, buf, from, len);
>> +ÂÂÂÂ if (f_pdata->use_direct_mode)
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ ret = cqspi_direct_read_execute(nor, buf, from, len);
>> +ÂÂÂÂ else
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ ret = cqspi_indirect_read_execute(nor, buf, from, len);
>>ÂÂÂÂÂÂÂ if (ret)
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return ret;
>>Â 
>> @@ -1153,6 +1194,12 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto err;
>>Â 
>>ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ f_pdata->registered = true;
>> +
>> +ÂÂÂÂÂÂÂÂÂÂÂÂ if (mtd->size <= cqspi->ahb_size) {
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ f_pdata->use_direct_mode = true;
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_info(nor->dev, "using direct mode for %s\n",
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ mtd->name);
> 
> Please use dev_dbg() here insted of dev_info(). IMHO, this kind of output
> is not really needed by regular users.
> 

Agreed, will update that.

> Otherwise, the series looks great!
> 

Thanks for the review! Will submit v2 shortly.


-- 
Regards
Vignesh