Re: [Qemu-devel] [PATCH v1 7/7] nand: Reset addressing after READSTATUS.

[Peter Maydell]

On 19 October 2012 07:40, Peter Crosthwaite
<address@hidden> wrote:
> From: Edgar E. Iglesias <address@hidden>
>
> Signed-off-by: Edgar E. Iglesias <address@hidden>
> ---
>
>  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