[Qemu-devel] Re: [PATCH v2] lsi53c895a: fix Phase Mismatch Jump

[Paolo Bonzini <pbonzini@redhat.com>]

On 06/14/2010 07:11 PM, Paolo Bonzini wrote:
> lsi_bad_phase has a bug in the choice of pmjad1/pmjad2.  This does
> not matter with Linux guests because it uses just one routine for
> both, but it breaks Windows 64-bit guests.  This is the text
> from the spec:
>
>     "[The PMJCTL] bit controls which decision mechanism is used
>     when jumping on phase mismatch. When this bit is cleared the
>     LSI53C895A will use Phase Mismatch Jump Address 1 (PMJAD1) when
>     the WSR bit is cleared and Phase Mismatch Jump Address 2 (PMJAD2)
>     when the WSR bit is set.  When this bit is set the LSI53C895A will
>     use jump address one (PMJAD1) on data out (data out, command,
>     message out) transfers and jump address two (PMJAD2) on data in
>     (data in, status, message in) transfers."
>
> Which means:
>
>      CCNTL0.PMJCTL
>          0              SCNTL2.WSR = 0             PMJAD1
>          0              SCNTL2.WSR = 1             PMJAD2
>          1                    out                  PMJAD1
>          1                    in                   PMJAD2
>
> In qemu, what you get instead is:
>
>      CCNTL0.PMJCTL
>          0                    out                  PMJAD1
>          0                    in                   PMJAD2<<<<<
>          1                    out                  PMJAD1
>          1                    in                   PMJAD1<<<<<
>
> Considering that qemu always has SCNTL2.WSR cleared, the two marked cases
> (corresponding to phase mismatch on input) are always jumping to the
> wrong PMJAD register.  The patch implements the correct semantics.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>          >  Looks correct. But why not assigning s->pmjad[12] directly? Would
>          >  improve readability IMO.
>
>          No particular reason, hence fine by me.
>
>   hw/lsi53c895a.c |    6 +++---
>   1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
> index f5a91ba..9a37fed 100644
> --- a/hw/lsi53c895a.c
> +++ b/hw/lsi53c895a.c
> @@ -490,10 +490,10 @@ static void lsi_bad_phase(LSIState *s, int out, int new_phase)
>   {
>       /* Trigger a phase mismatch.  */
>       if (s->ccntl0&  LSI_CCNTL0_ENPMJ) {
> -        if ((s->ccntl0&  LSI_CCNTL0_PMJCTL) || out) {
> -            s->dsp = s->pmjad1;
> +        if ((s->ccntl0&  LSI_CCNTL0_PMJCTL)) {
> +            s->dsp = out ? s->pmjad1 : s->pmjad2;
>           } else {
> -            s->dsp = s->pmjad2;
> +            s->dsp = (s->scntl2&  LSI_SCNTL2_WSR ? s->pmjad2 : s->pmjad1);
>           }
>           DPRINTF("Data phase mismatch jump to %08x\n", s->dsp);
>       } else {

PING

Paolo