Re: [Qemu-ppc] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely

[David Gibson]

On Tue, Jun 19, 2018 at 10:52:15AM +0200, BALATON Zoltan wrote:
> Rewrite to make it closer to how real device works so that guest OS
> drivers can access I2C devices. Previously this was only a hack to
> allow U-Boot to get past accessing SPD EEPROMs but to support other
> I2C devices and allow guests to access them we need to model real
> device more properly.
> 
> Signed-off-by: BALATON Zoltan <address@hidden>

Ugh.  This is still a large enough change that it's pretty difficult
to review, at least for someone not already familiar with the hardware.

> ---
>  hw/i2c/ppc4xx_i2c.c         | 222 
> +++++++++++++++++++++-----------------------
>  include/hw/i2c/ppc4xx_i2c.h |   3 +-
>  2 files changed, 110 insertions(+), 115 deletions(-)
> 
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index fca80d6..3ebce17 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -38,13 +38,26 @@
>  #define IIC_CNTL_READ       (1 << 1)
>  #define IIC_CNTL_CHT        (1 << 2)
>  #define IIC_CNTL_RPST       (1 << 3)
> +#define IIC_CNTL_AMD        (1 << 6)
> +#define IIC_CNTL_HMT        (1 << 7)
> +
> +#define IIC_MDCNTL_EINT     (1 << 2)
> +#define IIC_MDCNTL_ESM      (1 << 3)
> +#define IIC_MDCNTL_FMDB     (1 << 6)
>  
>  #define IIC_STS_PT          (1 << 0)
> +#define IIC_STS_IRQA        (1 << 1)
>  #define IIC_STS_ERR         (1 << 2)
> +#define IIC_STS_MDBF        (1 << 4)
>  #define IIC_STS_MDBS        (1 << 5)
>  
>  #define IIC_EXTSTS_XFRA     (1 << 0)
>  
> +#define IIC_INTRMSK_EIMTC   (1 << 0)
> +#define IIC_INTRMSK_EITA    (1 << 1)
> +#define IIC_INTRMSK_EIIC    (1 << 2)
> +#define IIC_INTRMSK_EIHE    (1 << 3)
> +
>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>  
>  #define IIC_DIRECTCNTL_SDAC (1 << 3)
> @@ -56,21 +69,13 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>  {
>      PPC4xxI2CState *i2c = PPC4xx_I2C(s);
>  
> -    /* FIXME: Should also reset bus?
> -     *if (s->address != ADDR_RESET) {
> -     *    i2c_end_transfer(s->bus);
> -     *}
> -     */
> -
> -    i2c->mdata = 0;
> -    i2c->lmadr = 0;
> -    i2c->hmadr = 0;
> +    i2c->mdidx = -1;
> +    memset(i2c->mdata, 0, ARRAY_SIZE(i2c->mdata));
> +    /* [hl][ms]addr are not affected by reset */
>      i2c->cntl = 0;
>      i2c->mdcntl = 0;
>      i2c->sts = 0;
> -    i2c->extsts = 0x8f;
> -    i2c->lsadr = 0;
> -    i2c->hsadr = 0;
> +    i2c->extsts = (1 << 6);
>      i2c->clkdiv = 0;
>      i2c->intrmsk = 0;
>      i2c->xfrcnt = 0;
> @@ -78,69 +83,29 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>      i2c->directcntl = 0xf;
>  }
>  
> -static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
> -{
> -    return true;
> -}
> -
>  static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int 
> size)
>  {
>      PPC4xxI2CState *i2c = PPC4xx_I2C(opaque);
>      uint64_t ret;
> +    int i;
>  
>      switch (addr) {
>      case 0:
> -        ret = i2c->mdata;
> -        if (ppc4xx_i2c_is_master(i2c)) {
> +        if (i2c->mdidx < 0) {
>              ret = 0xff;
> -
> -            if (!(i2c->sts & IIC_STS_MDBS)) {
> -                qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to read "
> -                              "without starting transfer\n",
> -                              TYPE_PPC4xx_I2C, __func__);
> -            } else {
> -                int pending = (i2c->cntl >> 4) & 3;
> -
> -                /* get the next byte */
> -                int byte = i2c_recv(i2c->bus);
> -
> -                if (byte < 0) {
> -                    qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
> -                                  "for device 0x%02x\n", TYPE_PPC4xx_I2C,
> -                                  __func__, i2c->lmadr);
> -                    ret = 0xff;
> -                } else {
> -                    ret = byte;
> -                    /* Raise interrupt if enabled */
> -                    /*ppc4xx_i2c_raise_interrupt(i2c)*/;
> -                }
> -
> -                if (!pending) {
> -                    i2c->sts &= ~IIC_STS_MDBS;
> -                    /*i2c_end_transfer(i2c->bus);*/
> -                /*} else if (i2c->cntl & (IIC_CNTL_RPST | IIC_CNTL_CHT)) {*/
> -                } else if (pending) {
> -                    /* current smbus implementation doesn't like
> -                       multibyte xfer repeated start */
> -                    i2c_end_transfer(i2c->bus);
> -                    if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
> -                        /* if non zero is returned, the adress is not valid 
> */
> -                        i2c->sts &= ~IIC_STS_PT;
> -                        i2c->sts |= IIC_STS_ERR;
> -                        i2c->extsts |= IIC_EXTSTS_XFRA;
> -                    } else {
> -                        /*i2c->sts |= IIC_STS_PT;*/
> -                        i2c->sts |= IIC_STS_MDBS;
> -                        i2c->sts &= ~IIC_STS_ERR;
> -                        i2c->extsts = 0;
> -                    }
> -                }
> -                pending--;
> -                i2c->cntl = (i2c->cntl & 0xcf) | (pending << 4);
> -            }
> -        } else {
> -            qemu_log_mask(LOG_UNIMP, "[%s]%s: slave mode not implemented\n",
> -                          TYPE_PPC4xx_I2C, __func__);
> +            break;
> +        }
> +        ret = i2c->mdata[0];
> +        if (i2c->mdidx == 3) {
> +            i2c->sts &= ~IIC_STS_MDBF;
> +        } else if (i2c->mdidx == 0) {
> +            i2c->sts &= ~IIC_STS_MDBS;
> +        }
> +        for (i = 0; i < i2c->mdidx; i++) {
> +            i2c->mdata[i] = i2c->mdata[i + 1];
> +        }
> +        if (i2c->mdidx >= 0) {
> +            i2c->mdidx--;
>          }
>          break;
>      case 4:
> @@ -160,6 +125,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr 
> addr, unsigned int size)
>          break;
>      case 9:
>          ret = i2c->extsts;
> +        ret |= !!i2c_bus_busy(i2c->bus) << 4;
>          break;
>      case 10:
>          ret = i2c->lsadr;
> @@ -203,70 +169,98 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr 
> addr, uint64_t value,
>  
>      switch (addr) {
>      case 0:
> -        i2c->mdata = value;
> -        if (!i2c_bus_busy(i2c->bus)) {
> -            /* assume we start a write transfer */
> -            if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 0)) {
> -                /* if non zero is returned, the adress is not valid */
> -                i2c->sts &= ~IIC_STS_PT;
> -                i2c->sts |= IIC_STS_ERR;
> -                i2c->extsts |= IIC_EXTSTS_XFRA;
> -            } else {
> -                i2c->sts |= IIC_STS_PT;
> -                i2c->sts &= ~IIC_STS_ERR;
> -                i2c->extsts = 0;
> -            }
> +        if (i2c->mdidx >= 3) {
> +            break;
>          }
> -        if (i2c_bus_busy(i2c->bus)) {
> -            if (i2c_send(i2c->bus, i2c->mdata)) {
> -                /* if the target return non zero then end the transfer */
> -                i2c->sts &= ~IIC_STS_PT;
> -                i2c->sts |= IIC_STS_ERR;
> -                i2c->extsts |= IIC_EXTSTS_XFRA;
> -                i2c_end_transfer(i2c->bus);
> -            }
> +        i2c->mdata[++i2c->mdidx] = value;
> +        if (i2c->mdidx == 3) {
> +            i2c->sts |= IIC_STS_MDBF;
> +        } else if (i2c->mdidx == 0) {
> +            i2c->sts |= IIC_STS_MDBS;
>          }
>          break;
>      case 4:
>          i2c->lmadr = value;
> -        if (i2c_bus_busy(i2c->bus)) {
> -            i2c_end_transfer(i2c->bus);
> -        }
>          break;
>      case 5:
>          i2c->hmadr = value;
>          break;
>      case 6:
> -        i2c->cntl = value;
> -        if (i2c->cntl & IIC_CNTL_PT) {
> -            if (i2c->cntl & IIC_CNTL_READ) {
> -                if (i2c_bus_busy(i2c->bus)) {
> -                    /* end previous transfer */
> -                    i2c->sts &= ~IIC_STS_PT;
> -                    i2c_end_transfer(i2c->bus);
> +        i2c->cntl = value & 0xfe;
> +        if (value & IIC_CNTL_AMD) {
> +            qemu_log_mask(LOG_UNIMP, "%s: only 7 bit addresses supported\n",
> +                          __func__);
> +        }
> +        if (value & IIC_CNTL_HMT && i2c_bus_busy(i2c->bus)) {
> +            i2c_end_transfer(i2c->bus);
> +            if (i2c->mdcntl & IIC_MDCNTL_EINT &&
> +                i2c->intrmsk & IIC_INTRMSK_EIHE) {
> +                    i2c->sts |= IIC_STS_IRQA;
> +                    qemu_irq_raise(i2c->irq);
> +            }
> +        } else if (value & IIC_CNTL_PT) {
> +            int recv = (value & IIC_CNTL_READ) >> 1;
> +            int tct = value >> 4 & 3;
> +            int i;
> +
> +            if (recv && (i2c->lmadr >> 1) >= 0x50 && (i2c->lmadr >> 1) < 
> 0x58) {
> +                /* smbus emulation does not like multi byte reads w/o 
> restart */
> +                value |= IIC_CNTL_RPST;
> +            }
> +
> +            for (i = 0; i <= tct; i++) {
> +                if (!i2c_bus_busy(i2c->bus)) {
> +                    i2c->extsts = (1 << 6);
> +                    if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, recv)) 
> {
> +                        i2c->sts |= IIC_STS_ERR;
> +                        i2c->extsts |= IIC_EXTSTS_XFRA;
> +                        break;
> +                    } else {
> +                        i2c->sts &= ~IIC_STS_ERR;
> +                    }
> +                }
> +                if (!(i2c->sts & IIC_STS_ERR) &&
> +                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
> +                        i2c->sts |= IIC_STS_ERR;
> +                        i2c->extsts |= IIC_EXTSTS_XFRA;
> +                        break;
>                  }
> -                if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
> -                    /* if non zero is returned, the adress is not valid */
> -                    i2c->sts &= ~IIC_STS_PT;
> -                    i2c->sts |= IIC_STS_ERR;
> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
> -                } else {
> -                    /*i2c->sts |= IIC_STS_PT;*/
> -                    i2c->sts |= IIC_STS_MDBS;
> -                    i2c->sts &= ~IIC_STS_ERR;
> -                    i2c->extsts = 0;
> +                if (value & IIC_CNTL_RPST || !(value & IIC_CNTL_CHT)) {
> +                    i2c_end_transfer(i2c->bus);
>                  }
> -            } else {
> -                /* we actually already did the write transfer... */
> -                i2c->sts &= ~IIC_STS_PT;
> +            }
> +            i2c->xfrcnt = i;
> +            i2c->mdidx = i - 1;
> +            if (recv && i2c->mdidx >= 0) {
> +                i2c->sts |= IIC_STS_MDBS;
> +            }
> +            if (recv && i2c->mdidx == 3) {
> +                i2c->sts |= IIC_STS_MDBF;
> +            }
> +            if (i && i2c->mdcntl & IIC_MDCNTL_EINT &&
> +                i2c->intrmsk & IIC_INTRMSK_EIMTC) {
> +                i2c->sts |= IIC_STS_IRQA;
> +                qemu_irq_raise(i2c->irq);
>              }
>          }
>          break;
>      case 7:
> -        i2c->mdcntl = value & 0xdf;
> +        i2c->mdcntl = value & 0x3d;
> +        if (value & IIC_MDCNTL_ESM) {
> +            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> +                          __func__);
> +        }
> +        if (value & IIC_MDCNTL_FMDB) {
> +            i2c->mdidx = -1;
> +            memset(i2c->mdata, 0, ARRAY_SIZE(i2c->mdata));
> +            i2c->sts &= ~(IIC_STS_MDBF | IIC_STS_MDBS);
> +        }
>          break;
>      case 8:
> -        i2c->sts &= ~(value & 0xa);
> +        i2c->sts &= ~(value & 0x0a);
> +        if (value & IIC_STS_IRQA && i2c->mdcntl & IIC_MDCNTL_EINT) {
> +            qemu_irq_lower(i2c->irq);
> +        }
>          break;
>      case 9:
>          i2c->extsts &= ~(value & 0x8f);
> @@ -287,12 +281,12 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr 
> addr, uint64_t value,
>          i2c->xfrcnt = value & 0x77;
>          break;
>      case 15:
> +        i2c->xtcntlss &= ~(value & 0xf0);
>          if (value & IIC_XTCNTLSS_SRST) {
>              /* Is it actually a full reset? U-Boot sets some regs before */
>              ppc4xx_i2c_reset(DEVICE(i2c));
>              break;
>          }
> -        i2c->xtcntlss = value;
>          break;
>      case 16:
>          i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & 
> IIC_DIRECTCNTL_SCLC);
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index ea6c8e1..0891a9c 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -46,7 +46,8 @@ typedef struct PPC4xxI2CState {
>      qemu_irq irq;
>      MemoryRegion iomem;
>      bitbang_i2c_interface *bitbang;
> -    uint8_t mdata;
> +    int mdidx;
> +    uint8_t mdata[4];
>      uint8_t lmadr;
>      uint8_t hmadr;
>      uint8_t cntl;

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature