[v6,12/24] hw/nand.c: bug fix to erase operation

The s->addr should be reset along with the s->addrlen,
or it might have the previous address shifted to MSB
and then causes problem to nand erase operation.

Signed-off-by: Kuo-Jung Su <dantesu@gmail.com>
---
 hw/nand.c |    1 +
 1 file changed, 1 insertion(+)

Hi Kuo Jung, Peter,

This fixes a no-boot bug in u-boot for us as well. RE PMMs comments in
v5, I realise the desire to fix this properly by rewriting that
if-else mess, but can we get a merge on this one more immediately to
get QEMU working again? Rewriting this is probably not at the top of
either mine or Kuo Jungs priority list but at the same time we would
like a working boot.

Regards,
Peter

On Wed, Mar 6, 2013 at 5:27 PM, Kuo-Jung Su <dantesu@gmail.com> wrote:
> The s->addr should be reset along with the s->addrlen,
> or it might have the previous address shifted to MSB
> and then causes problem to nand erase operation.
>
> Signed-off-by: Kuo-Jung Su <dantesu@gmail.com>

Acked-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> ---
>  hw/nand.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/nand.c b/hw/nand.c
> index 61e918f..6b2f2b8 100644
> --- a/hw/nand.c
> +++ b/hw/nand.c
> @@ -511,6 +511,7 @@ void nand_setio(DeviceState *dev, uint32_t value)
>              nand_command(s);
>
>          if (s->cmd != NAND_CMD_RANDOMREAD2) {
> +            s->addr = 0;
>              s->addrlen = 0;
>          }
>      }
> --
> 1.7.9.5
>
>

On 7 March 2013 10:18, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> This fixes a no-boot bug in u-boot for us as well. RE PMMs comments in
> v5, I realise the desire to fix this properly by rewriting that
> if-else mess, but can we get a merge on this one more immediately to
> get QEMU working again? Rewriting this is probably not at the top of
> either mine or Kuo Jungs priority list but at the same time we would
> like a working boot.

The trouble is that the control flow is so complicated that I don't
feel comfortable saying "yes, this change doesn't break operation
of any of the other commands". That's why I didn't give it a
reviewed-by tag. If you can provide a reasonably coherent explanation
of why it doesn't break anything else with reference to a decent
datasheet, you could convince me it's OK.

-- PMM

2013/3/7 Peter Maydell <peter.maydell@linaro.org>:
> On 7 March 2013 10:18, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> This fixes a no-boot bug in u-boot for us as well. RE PMMs comments in
>> v5, I realise the desire to fix this properly by rewriting that
>> if-else mess, but can we get a merge on this one more immediately to
>> get QEMU working again? Rewriting this is probably not at the top of
>> either mine or Kuo Jungs priority list but at the same time we would
>> like a working boot.
>
> The trouble is that the control flow is so complicated that I don't
> feel comfortable saying "yes, this change doesn't break operation
> of any of the other commands". That's why I didn't give it a
> reviewed-by tag. If you can provide a reasonably coherent explanation
> of why it doesn't break anything else with reference to a decent
> datasheet, you could convince me it's OK.
>
> -- PMM

I've checked the history of hw/nand.c, it might be bug-free before the patch
'hw/nand: Support large NAND devices' being applied.
Because it modifies the data type of addr from uint32_t into uint64_t,
and combined with
the problematic address clear (clear only address length), which then
leads to a bug of this.

BTW, if you're looking for the datasheet on which the nand.c based,
please refer to the link bellow:

http://www.datasheetcatalog.org/datasheet/SamsungElectronic/mXvvrxx.pdf

P.S:
The u-boot / linux suffers this bug because of the BBT, which stands for
Bad Block Table.
In other words, the u-boot / linux usually starts with a bunch of bad block scan
on block boundary (i.e. 64KB, 128KB).
While the addr is a uint32_t, right-shift 24 bits would always clear
address long enough for the incoming new address, but things would go
mad, if it's a uint64_t.

--
Best wishes,
Kuo-Jung Su