| Message ID | 20210716232504.182-3-a-nandan@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0395be967b067d99494113d78470574e86a02ed4 |
| Headers | show |
| Series | spi: cadence-quadspi: Fix DTR op checks and timeout in SPI NAND write operations | expand |
On 17-Jul-21 4:55 AM, Apurva Nandan wrote: > buswidth and dtr fields in spi_mem_op are only valid when the > corresponding spi_mem_op phase has a non-zero length. For example, > SPI NAND core doesn't set buswidth when using SPI_MEM_OP_NO_ADDR > phase. > > Fix the dtr checks in set_protocol() and suppports_mem_op() to > ignore empty spi_mem_op phases, as checking for dtr field in > empty phase will result in false negatives. > > Signed-off-by: Apurva Nandan <a-nandan@ti.com> > --- > drivers/spi/spi-cadence-quadspi.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index a2de23516553..1cec1c179a94 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -325,7 +325,15 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, > f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; > f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; > f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; > - f_pdata->dtr = op->data.dtr && op->cmd.dtr && op->addr.dtr; > + > + /* > + * For an op to be DTR, cmd phase along with every other non-empty > + * phase should have dtr field set to 1. If an op phase has zero > + * nbytes, ignore its dtr field; otherwise, check its dtr field. > + */ > + f_pdata->dtr = op->cmd.dtr && > + (!op->addr.nbytes || op->addr.dtr) && > + (!op->data.nbytes || op->data.dtr); > > switch (op->data.buswidth) { > case 0: > @@ -1228,8 +1236,15 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem, > { > bool all_true, all_false; > > - all_true = op->cmd.dtr && op->addr.dtr && op->dummy.dtr && > - op->data.dtr; > + /* > + * op->dummy.dtr is required for converting nbytes into ncycles. > + * Also, don't check the dtr field of the op phase having zero nbytes. > + */ > + all_true = op->cmd.dtr && > + (!op->addr.nbytes || op->addr.dtr) && > + (!op->dummy.nbytes || op->dummy.dtr) && > + (!op->data.nbytes || op->data.dtr); > + > all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && > !op->data.dtr; > > Hi Mark, Could you please have a look, I fixed the comments as you suggested. Thanks and regards, Apurva Nandan
On Wed, Jul 21, 2021 at 08:23:30PM +0530, Nandan, Apurva wrote:
> Could you please have a look, I fixed the comments as you suggested.
Please don't send content free pings and please allow a reasonable time
for review. People get busy, go on holiday, attend conferences and so
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review. If there have been
review comments then people may be waiting for those to be addressed.
Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index a2de23516553..1cec1c179a94 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -325,7 +325,15 @@ static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata, f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE; f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE; f_pdata->data_width = CQSPI_INST_TYPE_SINGLE; - f_pdata->dtr = op->data.dtr && op->cmd.dtr && op->addr.dtr; + + /* + * For an op to be DTR, cmd phase along with every other non-empty + * phase should have dtr field set to 1. If an op phase has zero + * nbytes, ignore its dtr field; otherwise, check its dtr field. + */ + f_pdata->dtr = op->cmd.dtr && + (!op->addr.nbytes || op->addr.dtr) && + (!op->data.nbytes || op->data.dtr); switch (op->data.buswidth) { case 0: @@ -1228,8 +1236,15 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem, { bool all_true, all_false; - all_true = op->cmd.dtr && op->addr.dtr && op->dummy.dtr && - op->data.dtr; + /* + * op->dummy.dtr is required for converting nbytes into ncycles. + * Also, don't check the dtr field of the op phase having zero nbytes. + */ + all_true = op->cmd.dtr && + (!op->addr.nbytes || op->addr.dtr) && + (!op->dummy.nbytes || op->dummy.dtr) && + (!op->data.nbytes || op->data.dtr); + all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr && !op->data.dtr;
buswidth and dtr fields in spi_mem_op are only valid when the corresponding spi_mem_op phase has a non-zero length. For example, SPI NAND core doesn't set buswidth when using SPI_MEM_OP_NO_ADDR phase. Fix the dtr checks in set_protocol() and suppports_mem_op() to ignore empty spi_mem_op phases, as checking for dtr field in empty phase will result in false negatives. Signed-off-by: Apurva Nandan <a-nandan@ti.com> --- drivers/spi/spi-cadence-quadspi.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)