[v1,7/7] nand: Reset addressing after READSTATUS.

From: Edgar E. Iglesias <edgar.iglesias@gmail.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---

 hw/nand.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

On 19 October 2012 07:40, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> ---
>
>  hw/nand.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/hw/nand.c b/hw/nand.c
> index 01f3ada..f931d0c 100644
> --- a/hw/nand.c
> +++ b/hw/nand.c
> @@ -478,6 +478,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
>      int i;
>      NANDFlashState *s = (NANDFlashState *) dev;
>      if (!s->ce && s->cle) {
> +        if (s->cmd == NAND_CMD_READSTATUS) {
> +            s->addr = 0;
> +            s->addrlen = 0;
> +            s->iolen = 0;
> +        }
> +

I find the NAND chip datasheets remarkably hard to interpret, but
I'm not convinced this patch is the right thing. Can you provide
some rationale/justification, please? (ideally with reference to
datasheets...)

Some of the code in the nand device definitely looks suspect though;
and I'm having trouble making a consistent mapping between how the
data sheets describe things and what our code does.  Why do
we handle some of the NAND commands directly in nand_setio but
delegate the rest to nand_command()? Should we really allow a
RANDOMREAD2 without checking that the previous command was
RANDOMREAD1? Can we safely just return after the command-handling
bit of nand_setio() or do we really need to allow the caller to
tell us to treat the data value as both a command and address
simultaneously if it wants to assert cle and ale at once? etc.

Basically I don't really understand this code, so I'm afraid
patches to it have to come with more rationale than usual attached
to them, because I can't fill in the gaps myself :-(

thanks
-- PMM

On Fri, Oct 19, 2012 at 12:59:49PM +0100, Peter Maydell wrote:
> On 19 October 2012 07:40, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
> > From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > ---
> >
> >  hw/nand.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/nand.c b/hw/nand.c
> > index 01f3ada..f931d0c 100644
> > --- a/hw/nand.c
> > +++ b/hw/nand.c
> > @@ -478,6 +478,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
> >      int i;
> >      NANDFlashState *s = (NANDFlashState *) dev;
> >      if (!s->ce && s->cle) {
> > +        if (s->cmd == NAND_CMD_READSTATUS) {
> > +            s->addr = 0;
> > +            s->addrlen = 0;
> > +            s->iolen = 0;
> > +        }
> > +
> 
> I find the NAND chip datasheets remarkably hard to interpret, but
> I'm not convinced this patch is the right thing. Can you provide
> some rationale/justification, please? (ideally with reference to
> datasheets...)

This is patch is quite old (several years). At the time modern linux kernels
stopped working with our nand model in some cases. Some patch to our
nand model broke something. I recall trying to make some sense out of
it and this was the closest I got..

I don't know what the state it is today nor do I remember the exact
circumstances on which the bug was trigged. Maybe Peter C has more
info?

Cheers,
Edgar

On Fri, Oct 19, 2012 at 10:18 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Fri, Oct 19, 2012 at 12:59:49PM +0100, Peter Maydell wrote:
>> On 19 October 2012 07:40, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>> > From: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> >
>> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> > ---
>> >
>> >  hw/nand.c |    6 ++++++
>> >  1 files changed, 6 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/hw/nand.c b/hw/nand.c
>> > index 01f3ada..f931d0c 100644
>> > --- a/hw/nand.c
>> > +++ b/hw/nand.c
>> > @@ -478,6 +478,12 @@ void nand_setio(DeviceState *dev, uint32_t value)
>> >      int i;
>> >      NANDFlashState *s = (NANDFlashState *) dev;
>> >      if (!s->ce && s->cle) {
>> > +        if (s->cmd == NAND_CMD_READSTATUS) {
>> > +            s->addr = 0;
>> > +            s->addrlen = 0;
>> > +            s->iolen = 0;
>> > +        }
>> > +
>>
>> I find the NAND chip datasheets remarkably hard to interpret, but
>> I'm not convinced this patch is the right thing. Can you provide
>> some rationale/justification, please? (ideally with reference to
>> datasheets...)
>
> This is patch is quite old (several years). At the time modern linux kernels
> stopped working with our nand model in some cases. Some patch to our
> nand model broke something. I recall trying to make some sense out of
> it and this was the closest I got..
>
> I don't know what the state it is today nor do I remember the exact
> circumstances on which the bug was trigged. Maybe Peter C has more
> info?
>

Not really. Im fairly lost as well on the data-sheet front but AFAICT
what actually happens here is an undefined behaviour. Ill have to dig
deeper on my tests to see if its a problem. Could just be a hangover
from an ancient kernel bug and this patch is unneeded.

Regards,
Peter

> Cheers,
> Edgar
>