RE: [PATCH v2 2/2] spi: cadence: add support for Cadence XSPI controller

[Parshuram Raju Thombare]

Hi Pratyush,

Thanks for spending time to review this.

>Please specify full forms of all these acronyms. I don't know what XSPI
>or ACMD or PIO means. Does XSPI mean the JEDEC eXtended SPI protocol
>(JESD251) or something else?

Yes, here xSPI refers to JEDEC EXpanded Serial Peripheral Interface.
Some information about different work mode is included in the cover letter.

This controller basically support 3 work modes.
1. ACMD (auto command) work mode
    ACMD name is because it uses auto command engine in the controller.
    It further has 2 modes PIO and CDMA (command DMA).
    The CDMA work mode is dedicated for high-performance application
    where very low software overhead is required. In this mode the
    Command Engine is programmed by the series of linked descriptors
    stored in system memory. These descriptors provide commands to execute
    and store status information for finished commands.
    The PIO mode work mode is dedicated for single operation where
    constructing a linked list of descriptors would require too 
    much effort.

2. STIG (Software Triggered Instruction Generator) work mode 
    In STIG mode, controller sends low-level instructions to memory.
    Each instruction is 128-bit width. There is special instruction
    DataSequence which carries information about data phase.
    Driver uses Slave DMA interface to transfer data as only this
    interface can be used in STIG work mode.

3. Direct work mode
    This work mode allows sending data without invoking any command through the slave interface.

>What flashes and platforms did you test on?

This driver is tested on zynqmp zcu102 FPGA with Cadence's
xSPI controller. This FPGA board has mt35xu512 flash.

>There is also the controller driven by spi-cadence-quadspi.c. It also
>seems to support all the same modes and features as this new one. What
>is the difference between the two?

This driver is for xSPI controller IP designed to communicate with Flash Memory
Devices supporting JESD216 and JESD251 standards. This is completely
different controller from QSPI/OSPI controller.

>> +		if (!(ctrl_status & CDNS_XSPI_INIT_COMPLETED) ||
>> +		    (ctrl_status & CDNS_XSPI_INIT_FAIL)) {
>> +			if (rd_dqs_del_min != -1)
>> +				rd_dqs_del_max = rd_dqs_del - 1;
>> +		} else {
>> +			if (rd_dqs_del_min == -1)
>> +				rd_dqs_del_min = rd_dqs_del;
>> +		}
>
>How is the DQS delay detected? What data do you read?

Here DQS delay calculated based on device discovery process performed
by the controller on reset which includes reading
SFDP (Serial Flash Discoverable Parameters - JESD216 standard)
from the flash device.

>If the min delay is 60 and max delay is 70, then you would want to
>select 65, no? This would select 95 which won't work at all. I think you
>want to do:
>  rd_dqs_del = rd_dqs_del_min + ((rd_dqs_del_max - rd_dqs_del_min) / 2);

Do you mean average of two values ?
Yes, this should have been average of min and max values, I will fix this in next version.

>No need to reset the controller this time? Why did you need to reset in
>the loop above?[] 

Reset in the loop is to invoke device discovery process.

>I don't have the datasheet so I am guessing a bit here. I assume phony
>DQS would be used when the flash does not provide a DQS signal to
>estimate delays. This is generated internally in the controller. The
>external loopback DQS would then be going along the data lines to the
>flash to estimate trace delays. Do I understand this correctly? If yes,
>then shouldn't phony and external loopback DQS be exclusive to each
>other? You enable them both at the same time.

Here using 'use_phony_dqs ' bit, phony dqs or lpbk dqs can be selected
over DQS generated from flash device.

'use_lpbk_dqs' bit is to select either phony dqs or lpbk dqs.
phony dqs is generated in the internal control slice logic.  

lpbk_dqs further has option to select internal lpbk_dqs (mem_rebar_ipad)
or external lpbk_dqs (lpbk_dqs connected to the lpbk_dqs_ipad)  using
'use_ext_lpbk_dqs' bit.

Therefore, 'use_ext_lpbk_dqs' and 'use_lpbk_dqs' are valid only when phony/lpbk
dqs is selected using 'use_phony_dqs'.


>How does the controller detects an invalid command sequence or invalid
>DQS pulses? What is the judgement criteria?

Controller detects DQS_ERROR based on the absence of DQS.
Also overflow or underrun of the entry FIFO due to DQS exceeding the FIFO capacity
or not appearing at all respectively can cause DQS_ERROR.
This read FIFO is gated using programmable rd_del_sel (read delay select) signal
which delay the dfi_rddata_en signal prior to enabling the read FIFO. 
 
>You don't use the opcode, buswidths, dummy bytes, etc. from the
>spi_mem_op. Where do you get this value? I don't think the controller
>should silently ignore these fields.

Device discovery module initializes device sequence registers which selects
data rate (SDR/DDR), IO width for command, address and data phase for
erase, program and read operations.
Device discovery module can run in auto mode (attempting different protocol modes
to read SFDP) or preconfigured to selected protocol mode using bootstrap signals.
Device sequence registers are used to select protocol mode when running in
auto command modes (PIO and CDMA), which reduces software overhead
which otherwise needed in STIG mode.

>If it is not a NOR flash you always use STIG? Why do you need to know
>the flash type?
>In either case, I don't think this is the right thing to do. You can't
>just make assumptions about the driver name. What if we decide to call
>it "SPI NOR" instead of "spi-nor" in the future?

This is to allow possibility to use same driver with other memory devices,
using spi_mem_exec_op called from memory device driver.
Currently this driver is tested in STIG as well as ACMD mode, only for
NOR flash device. STIG mode is selected for accessing other types of memory devices. 

>> +	    (!op->addr.buswidth && !op->addr.nbytes && !op->addr.val)) {
>I think op->addr.nbytes should be a good enough indicator of whether the
>address phase is present or not. As long as that is 0, all other fields
>should be treated as don't care IMO.

This is basically to differentiate between memory device register access and
read, program, erase operations. Register read/write is done using STIG mode.
Here trying to detect SPI_MEM_OP_NO_ADDR condition, may be checking
just address buswidth and nbytes is sufficient. 


>spi_mem_exec_op() already locks mem->spi->controller->bus_lock_mutex and
>mem->spi->controller->io_mutex. Is that not enough to serialize access
>to the controller?[] 

Oh, ok it seems spi_mem_access_start/end does that job, then this lock can
be dropped.

Regards,
Parshuram Thombare