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

login
register
mail settings
Submitter Peter Crosthwaite
Date March 7, 2013, 3:32 a.m.
Message ID <CAEgOgz6WJ0G2tR2J_SxQ5b3KKC72f0+kptUOtBMRF6otHX2twQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/225721/
State New
Headers show

Comments

Peter Crosthwaite - March 7, 2013, 3:32 a.m.
Hi Peter, Kuo-Jung,

On Thu, Mar 7, 2013 at 12:28 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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".

There may be a complication with NAND_CMD_COPYBACKPRG1, this code may
truncate a currently in use address there although I confess I too am
confused by the control flow here. Kuo Jungs patch could be more
defensive by not trampling the address on this command. But rather
than trawl through 100 datasheets looking for all the corner cases RE
Nand addressing, Wendy (independently of this discussion) fixed this
issue in our tree with a lower impact change. I'll post a full patch
to list for review - it may be what you are looking for in that it is
a direct approach to solve the issue of the broken BLOCK_ERASE command
(which i believe was Kuo Jungs motivation to begin with) without
changing the shared address construction logic:

>
Kuo-Jung Su - March 7, 2013, 4:10 a.m.
2013/3/7 Peter Crosthwaite <peter.crosthwaite@xilinx.com>:
> Hi Peter, Kuo-Jung,
>
> On Thu, Mar 7, 2013 at 12:28 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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".
>
> There may be a complication with NAND_CMD_COPYBACKPRG1, this code may
> truncate a currently in use address there although I confess I too am
> confused by the control flow here. Kuo Jungs patch could be more
> defensive by not trampling the address on this command. But rather
> than trawl through 100 datasheets looking for all the corner cases RE
> Nand addressing, Wendy (independently of this discussion) fixed this
> issue in our tree with a lower impact change. I'll post a full patch
> to list for review - it may be what you are looking for in that it is
> a direct approach to solve the issue of the broken BLOCK_ERASE command
> (which i believe was Kuo Jungs motivation to begin with) without
> changing the shared address construction logic:
>
> --- a/hw/nand.c
> +++ b/hw/nand.c
> @@ -296,10 +296,14 @@ static void nand_command(NANDFlashState *s)
>          break;
>
>      case NAND_CMD_BLOCKERASE2:
> +       s->addr &= 0xffffff;
>
> Regards,
> Peter
>

I've verified that it works to me, too. So it's should be a
replacement of my buggy one.

>  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
>>



--
Best wishes,
Kuo-Jung Su

Patch

--- a/hw/nand.c
+++ b/hw/nand.c
@@ -296,10 +296,14 @@  static void nand_command(NANDFlashState *s)
         break;

     case NAND_CMD_BLOCKERASE2:
+	s->addr &= 0xffffff;

Regards,
Peter

 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