[RFC PATCH] spi: ich: Drop while loop in hardware sequencing erase case

[Simon Glass]

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>