| Message ID | CAEgOgz6WJ0G2tR2J_SxQ5b3KKC72f0+kptUOtBMRF6otHX2twQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
--- 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