Hi Stefan,
> -----Original Message-----
> From: Simek, Michal <michal.si...@amd.com>
> Sent: Monday, April 17, 2023 3:47 PM
> To: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-...@weidmueller.com>;
> u-boot@lists.denx.de; Soma, Ashok Reddy <ashok.reddy.s...@amd.com>
> Cc: Stefan Herbrechtsmeier <stefan.herbrechtsme...@weidmueller.com>;
> Jagan Teki <ja...@amarulasolutions.com>
> Subject: Re: [PATCH] Revert "spi: zynq_qspi: Use dummy buswidth in dummy
> byte calculation"
>
>
>
> On 3/31/23 16:44, Stefan Herbrechtsmeier wrote:
> > From: Stefan Herbrechtsmeier <stefan.herbrechtsme...@weidmueller.com>
> >
> > This reverts commit e09784728689de7949d4cdd559a9590e0bfcc702. The
> > commit wrongly divides the dummy bytes by dummy bus width to calculate
> > the dummy bytes. The framework already converts the dummy cycles to
> > the number of bytes and the controller use the SPI flash command to
> > determine the dummy cycles via the address width.
As per my understanding dummy bus width should be equal to data buswidth and
not equal to address bus width.
Please let me know if this understanding in incorrect.
Based on the above understanding we have changed in Xilinx repo, at below code
https://github.com/Xilinx/u-boot-xlnx/blob/024eb37c1e38ab811abe5408d42069fbd7901824/drivers/mtd/spi/spi-nor-core.c#L262
from
op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
to
op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
I have sent the same as RFC. Please see the attached patch.
Currently this patch is present as part of Xilinx repo, and hence based on this
below implementation dymmy_bytes are recalculated.
I have sent controller driver code to upstream but, I have not yet sent
spi-nor-core.c code, which I will be sending soon with some other changes.
Please let me know your thoughts about this patch and the changes.
Thanks,
Ashok
> >
> > Signed-off-by: Stefan Herbrechtsmeier
> > <stefan.herbrechtsme...@weidmueller.com>
> >
> > ---
> >
> > drivers/spi/zynq_qspi.c | 10 ++--------
> > 1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/spi/zynq_qspi.c b/drivers/spi/zynq_qspi.c index
> > 00e3ffcd1d..d1d4048966 100644
> > --- a/drivers/spi/zynq_qspi.c
> > +++ b/drivers/spi/zynq_qspi.c
> > @@ -676,7 +676,6 @@ static int zynq_qspi_exec_op(struct spi_slave *slave,
> > const struct spi_mem_op *op)
> > {
> > int op_len, pos = 0, ret, i;
> > - u32 dummy_bytes = 0;
> > unsigned int flag = 0;
> > const u8 *tx_buf = NULL;
> > u8 *rx_buf = NULL;
> > @@ -689,11 +688,6 @@ static int zynq_qspi_exec_op(struct spi_slave
> *slave,
> > }
> >
> > op_len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> > - if (op->dummy.nbytes) {
> > - op_len = op->cmd.nbytes + op->addr.nbytes +
> > - op->dummy.nbytes / op->dummy.buswidth;
> > - dummy_bytes = op->dummy.nbytes / op->dummy.buswidth;
> > - }
> >
> > u8 op_buf[op_len];
> >
> > @@ -707,8 +701,8 @@ static int zynq_qspi_exec_op(struct spi_slave *slave,
> > pos += op->addr.nbytes;
> > }
> >
> > - if (dummy_bytes)
> > - memset(op_buf + pos, 0xff, dummy_bytes);
> > + if (op->dummy.nbytes)
> > + memset(op_buf + pos, 0xff, op->dummy.nbytes);
> >
> > /* 1st transfer: opcode + address + dummy cycles */
> > /* Make sure to set END bit if no tx or rx data messages
> > follow */
> > --
> > 2.30.2
> >
>
> Ashok: Can you please comment on this one?
>
> Thanks,
> Michal
--- Begin Message ---
In current implementation dummy buswidth is set equal to address
buswidth. In case of quad spi (mode 1-1-4), where address width is 1
the dummy bytes will be calculated to 1(8 dummy cycles) and dummy
buswidth is set to 1. Due to this, the controller driver will introduce
8 dummy cycles on data line(D0) during read operation.
But since we are using 4 data lines in case of qspi, we need to change
this dummy bus width to 4. This will make dummy bytes to 4 inplace of 1.
This will be taken care in controller driver by dividing with dummy
buswidth again as in below code, which makes dummy cycles to 8 as
earlier.
dummy_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth;
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/spi/zynqmp_gqspi.c#L511
So with this change dummy cycles will be on all data lines(D0-D3) and it
is taken care for all the configurations(single, dual, quad and octal).
SPI experts, please advice if this change is good. What is the reason
we are using dummy bus width as address bus width so far ?
I have tested this change on all xilinx platforms with single, quad and
octal configurations. It works perfectly fine.
I would appreciate if anyone can test on your board and give feedback.
Signed-off-by: Ashok Reddy Soma <ashok.reddy.s...@xilinx.com>
---
drivers/mtd/spi/spi-nor-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index a70fbda4bb..6849da9113 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -251,7 +251,7 @@ static void spi_nor_setup_op(const struct spi_nor *nor,
op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
if (op->dummy.nbytes)
- op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
+ op->dummy.buswidth = spi_nor_get_protocol_data_nbits(proto);
if (op->data.nbytes)
op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
--
2.17.1
--- End Message ---
