Re: [PATCH v6 2/2] spi: cadence-quadpsi: Add support for the Cadence QSPI controller

From: Ramuthevar, Vadivel MuruganX
Date: Wed Jan 08 2020 - 00:44:42 EST


Hi,

On 8/1/2020 1:14 PM, Vignesh Raghavendra wrote:

On 06/01/20 4:49 pm, Ramuthevar, Vadivel MuruganX wrote:
Hi,

Thank you for the review comments.

On 6/1/2020 6:40 PM, Vignesh Raghavendra wrote:
Hi,

On 30/12/19 1:11 pm, Ramuthevar,Vadivel MuruganX wrote:
[...]
+static u32 cqspi_cmd2addr(const unsigned char *addr_buf, u32
addr_width)
+{
+ÂÂÂ unsigned int addr = 0;
+ÂÂÂ int i;
+
+ÂÂÂ /* Invalid address return zero. */
+ÂÂÂ if (addr_width > 4)
+ÂÂÂÂÂÂÂ return 0;
+
+ÂÂÂ for (i = 0; i < addr_width; i++) {
+ÂÂÂÂÂÂÂ addr = addr << 8;
+ÂÂÂÂÂÂÂ addr |= addr_buf[i];
+ÂÂÂ }
+
+ÂÂÂ return addr;
+}
+
[...]
+static int cqspi_apb_read_setup(struct struct_cqspi *cqspi,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const struct spi_mem_op *op,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const u8 *addrbuf)
+{
+ÂÂÂ void __iomem *reg_base = cqspi->iobase;
+ÂÂÂ size_t addrlen = op->addr.nbytes;
+ÂÂÂ size_t dummy_bytes = op->dummy.nbytes;
+ÂÂÂ unsigned int addr_value, dummy_clk, reg;
+
+ÂÂÂ if (addrlen) {
+ÂÂÂÂÂÂÂ addr_value = cqspi_cmd2addr(&addrbuf[0], addrlen);
+ÂÂÂÂÂÂÂ writel(addr_value, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
+ÂÂÂ }
+
Why do you need to swap the address bytes to SPI bus order?
Yes , you are right to align with spi bus order swap is done .
 You are
writing to a controller register that accepts 24 bit or 32 bit address.
32bit address.
There is no need to swap the address bytes. The current driver
(drivers/mtd/spi-nor/cadence-quadspi.c) does not swap the address to SPI
bus order, why does the new driver required to do so?
Thanks! for clarification, actually we are not swapping , just Converting address buffer into word format (MSB first).
+ÂÂÂ reg = op->cmd.opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
+ÂÂÂ reg |= (op->data.buswidth & CQSPI_REG_RD_INSTR_TYPE_DATA_MASK) <<
+ÂÂÂÂÂÂÂ CQSPI_REG_RD_INSTR_TYPE_DATA_LSB;
+
This is wrong... op->data.buswidth's range is 1 to 8 whereas
CQSPI_REG_RD_INSTR_TYPE range is 0 to 3. I wonder whether you tested
dual/quad mode with this driver?
Yes I have tested with Quad mode since Cadence-IP supports dual/quad on
my platform, used to validate
before sending the patch that's standard procedure here.
please let me know if you have any further queries.

Then I have no idea how it works on your platform..
while testing on my platform I have hardcoded to 4 instead of 8, for me it is working fine.
should be handled properly for OCTAL mode once your changes are ready

What you are
programming above overflows the assigned bit fields for bus width, right?
yes, overflow should be handled
once started working on your platform with your changes, I will squash and send it back.
Thanks for your time.

Regards
Vadivel
---
Best Regards
Vadivel
I am still unable to get this series to work on my platform. Will
continue to debug...