diff mbox series

[v4,03/11] ppc4xx_i2c: Rewrite to model hardware more closely

Message ID 5227f6905dc3cf9aa0ed933b591d1741acbeeac5.1529398335.git.balaton@eik.bme.hu
State New
Headers show
Series Misc sam460ex improvements | expand

Commit Message

BALATON Zoltan June 19, 2018, 8:52 a.m. UTC 
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 <balaton@eik.bme.hu>
---
 hw/i2c/ppc4xx_i2c.c         | 222 +++++++++++++++++++++-----------------------
 include/hw/i2c/ppc4xx_i2c.h |   3 +-
 2 files changed, 110 insertions(+), 115 deletions(-)

Comments

David Gibson June 20, 2018, 5:25 a.m. UTC | #1 
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 <balaton@eik.bme.hu>

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;
BALATON Zoltan June 21, 2018, 6:51 a.m. UTC | #2 
On Wed, 20 Jun 2018, David Gibson wrote:
> 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 <balaton@eik.bme.hu>
>
> 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.

Then what to do? Does it worth the effort trying to break this up more 
when without knowledge about the hardware it's not possible to review even 
if it's smaller patches? Also this touches a device which was not emulated 
before at all, then patched to make U-Boot happy (see: 
http://lists.nongnu.org/archive/html/qemu-ppc/2017-08/msg00193.html) and 
this is now the first version that really tries to emulate the device so 
that guest OSes can also use it. Considering that it's only used on some 
not really maintained or tested SoC emulations what risk is to take this 
and then fix it if problem found later? Holding on to a known wrong 
emulation just because no one knows better does not help evolve this 
device emulation.

Maybe I can try to enhance commit message to list what missing features of 
the device this tries to add if that helps. These are basically:
- mdata is a 4 byte FIFO
- device should generate interrupts

Regards,
BALATON Zoltan

>> ---
>>  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;
>
>
diff mbox series

Patch

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;