Hi,
This series proposes fixes for cadence-quadspi controller for the
following issues with SPI NAND flashes:
- Due to auto-HW polling without address phase, the cadence-quadspi
controller timeouts when performing any write operation on SPI NAND
flash.
- When checking for DTR spi_mem_op, cadence-quadspi doesn't ignore a
zero length phase in the SPI instruction, resulting in false negatives.
This series has been tested on TI J721e EVM with the Winbond W35N01JW
flash.
v1 series: https://lore.kernel.org/linux-spi/[email protected]/
Changes in v2:
- [PATCH v2 1/2]: Same as v1. This patch has been already applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
commit 9cb2ff111712 ("spi: cadence-quadspi: Disable Auto-HW polling")
- [PATCH v2 2/2]: Add new comments to explain the DTR check conditions
Apurva Nandan (2):
spi: cadence-quadspi: Disable Auto-HW polling
spi: cadence-quadspi: Fix check condition for DTR ops
drivers/spi/spi-cadence-quadspi.c | 48 ++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 16 deletions(-)
--
2.17.1
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 <[email protected]>
---
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;
--
2.17.1
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 <[email protected]>
> ---
> 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.
On Fri, 16 Jul 2021 23:25:01 +0000, Apurva Nandan wrote:
> This series proposes fixes for cadence-quadspi controller for the
> following issues with SPI NAND flashes:
>
> - Due to auto-HW polling without address phase, the cadence-quadspi
> controller timeouts when performing any write operation on SPI NAND
> flash.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[2/2] spi: cadence-quadspi: Fix check condition for DTR ops
commit: 0395be967b067d99494113d78470574e86a02ed4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark