| Message ID | 20200114130548.67721-1-wolfgang.wallner@br-automation.com |
|---|---|
| State | Accepted |
| Commit | 5e579cc0044b8660fff7fbc043982ff80ff7be7a |
| Headers | show |
| Series | [RFC] spi: ich: Drop while loop in hardware sequencing erase case | expand |
Hi Wolfgang, On Tue, 14 Jan 2020 at 06:06, Wolfgang Wallner <wolfgang.wallner at br-automation.com> wrote: > > When ich_spi_exec_op_hwseq() is called to erase a 4k block > (opcode = SPINOR_OP_BE_4K), it expects to find a length value in > op->data.nbytes, but that value is always 0. As a result, the while loop > is never executed and no erase is carried out. > > Fix this by dropping the loop code entirely, only keeping the relevant > parts of the loop body. > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com> > > --- > I tried using an SPI NOR flash on an Apollo Lake based board, but calling > 'sf erase' fails to erase the flash memory, it looks like it does nothing. > Debugging the issue I find that the function ich_spi_exec_op_hwseq() in > drivers/spi/ich.c tries to loop over the provided number of bytes, but that > value is always 0. > > The spi operation that is handled by ich_spi_exec_op_hwseq() is initially > created in the function spi_nor_erase_sector(), and handed down via the > following call stack: > > spi_nor_erase_sector() in drivers/mtd/spi/spi-nor-core.c > spi_mem_exec_op() in drivers/spi/spi-mem.c > spi_mem_exec_op() calls ops->mem_ops->exec_op() which points to > ich_spi_exec_op() > ich_spi_exec_op() in drivers/spi/ich.c > ich_spi_exec_op_hwseq() in drivers/spi/ich.c > > The number of bytes (op.data.nbytes) is always set to 0 in > spi_nor_erase_sector(), thus the loop in ich_spi_exec_op_hwseq can never be > executed. > > I tried to fix this by dropping the loop code entirely, and with this > change 'sf erase' works for me as expected. > > The code in spi_nor_erase_sector() that sets data.nbytes always to 0 was > introduced in commit f909ddb3e177 ("mtd: spi: Replace ad-hoc default > implementation with spi_mem_op"), but that commit is older then the code > in ich_spi_exec_op_hwseq() which was introduced in commit 1facebd18fd7 > ("spi: ich: Support hardware sequencing"). > > I would like to ask for feedback on the provided patch. > > drivers/spi/ich.c | 12 ++---------- > 1 file changed, 2 insertions(+), 10 deletions(-) The conversion to spi_mem broke the ich driver initially. I think it was patched up later, but perhaps erase did not work. I have tested this and repeated the problem you found. It looks to me like this is called from spi_nor_erase_sector() which only erases a single sector at a time. So I think your patch is correct, and the loop is not needed anymore. Reviewed-by: Simon Glass <sjg at chromium.org>
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 133b25b72e..a9d7715a55 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -562,16 +562,8 @@ static int ich_spi_exec_op_hwseq(struct spi_slave *slave, return 0; /* ignore */ case SPINOR_OP_BE_4K: cycle = HSFSTS_CYCLE_4K_ERASE; - while (len) { - uint xfer_len = 0x1000; - - ret = exec_sync_hwseq_xfer(regs, cycle, offset, 0); - if (ret) - return ret; - offset += xfer_len; - len -= xfer_len; - } - return 0; + ret = exec_sync_hwseq_xfer(regs, cycle, offset, 0); + return ret; default: debug("Unknown cycle %x\n", op->cmd.opcode); return -EINVAL;
When ich_spi_exec_op_hwseq() is called to erase a 4k block (opcode = SPINOR_OP_BE_4K), it expects to find a length value in op->data.nbytes, but that value is always 0. As a result, the while loop is never executed and no erase is carried out. Fix this by dropping the loop code entirely, only keeping the relevant parts of the loop body. Signed-off-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com> --- I tried using an SPI NOR flash on an Apollo Lake based board, but calling 'sf erase' fails to erase the flash memory, it looks like it does nothing. Debugging the issue I find that the function ich_spi_exec_op_hwseq() in drivers/spi/ich.c tries to loop over the provided number of bytes, but that value is always 0. The spi operation that is handled by ich_spi_exec_op_hwseq() is initially created in the function spi_nor_erase_sector(), and handed down via the following call stack: spi_nor_erase_sector() in drivers/mtd/spi/spi-nor-core.c spi_mem_exec_op() in drivers/spi/spi-mem.c spi_mem_exec_op() calls ops->mem_ops->exec_op() which points to ich_spi_exec_op() ich_spi_exec_op() in drivers/spi/ich.c ich_spi_exec_op_hwseq() in drivers/spi/ich.c The number of bytes (op.data.nbytes) is always set to 0 in spi_nor_erase_sector(), thus the loop in ich_spi_exec_op_hwseq can never be executed. I tried to fix this by dropping the loop code entirely, and with this change 'sf erase' works for me as expected. The code in spi_nor_erase_sector() that sets data.nbytes always to 0 was introduced in commit f909ddb3e177 ("mtd: spi: Replace ad-hoc default implementation with spi_mem_op"), but that commit is older then the code in ich_spi_exec_op_hwseq() which was introduced in commit 1facebd18fd7 ("spi: ich: Support hardware sequencing"). I would like to ask for feedback on the provided patch. drivers/spi/ich.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)