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

[Bin Meng]

On Thu, Jan 30, 2020 at 10:16 AM Simon Glass <sjg at chromium.org> wrote:
>
> 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>

applied to u-boot-x86, thanks!