Re: [Qemu-devel] [RFC 7/7] pl181.c: convert to async IO SD card interface

[Peter Maydell]

On 10 May 2013 17:10, Igor Mitsyanko <address@hidden> wrote:
> Signed-off-by: Igor Mitsyanko <address@hidden>

(not a complete review, just a couple of things I noticed on a
first pass.)

> ---
>  hw/sd/pl181.c | 302 
> +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 192 insertions(+), 110 deletions(-)
>
> diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c
> index 2caacc2..a8a3510 100644
> --- a/hw/sd/pl181.c
> +++ b/hw/sd/pl181.c
> @@ -20,7 +20,7 @@ do { printf("pl181: " fmt , ## __VA_ARGS__); } while (0)
>  #define DPRINTF(fmt, ...) do {} while(0)
>  #endif
>
> -#define PL181_FIFO_LEN 16
> +#define PL181_FIFO_LEN 64
>
>  typedef struct {
>      SysBusDevice busdev;
> @@ -39,23 +39,23 @@ typedef struct {
>      uint32_t status;
>      uint32_t mask[2];
>      int32_t fifo_pos;
> -    int32_t fifo_len;
>      /* The linux 2.6.21 driver is buggy, and misbehaves if new data arrives
>         while it is reading the FIFO.  We hack around this be defering
>         subsequent transfers until after the driver polls the status word.
>         http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4446/1
>       */
>      int32_t linux_hack;
> -    uint32_t fifo[PL181_FIFO_LEN];

This looks really odd. Why has the fifo gone away?

>      qemu_irq irq[2];
>      /* GPIO outputs for 'card is readonly' and 'card inserted' */
>      qemu_irq cardstatus[2];
> +    qemu_irq sbit_received;
> +    qemu_irq busy_deasrt;

No real need to abbreviate here, just call it 'busy_deassert'.

>  } pl181_state;

> @@ -395,7 +414,6 @@ static void pl181_write(void *opaque, hwaddr offset,
>                                "pl181: Pending commands not implemented\n");
>              } else {
>                  pl181_send_command(s);
> -                pl181_fifo_run(s);
>              }
>              /* The command has completed one way or the other.  */
>              s->cmd &= ~PL181_CMD_ENABLE;

This doesn't look right. As the comment notes, previously we could
assume (because we were strictly synchronous) that at this point the
command had completed and move the command state machine straight
back to idle. However now things are asynchronous this isn't true
and you need extra logic to change the cmd enabled bit only when we've
actually finished whatever it was we were doing.

thanks
-- PMM