| Message ID | 61b9cf7d09f0f12a86f036b828665c3fc91c3ce9.1529839203.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | Misc sam460ex improvements | expand |
On 06/24/2018 01:20 PM, 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. ppc4xx support was dropped from u-boot but there is some work being done to re-add at least ppc-460x. These models should be of interest to emulate BMC like boards and, in some near future, they could even run OpenBMC. I understand that you are adding extended status support, multi transfer support, better interrupt support. Having some comments on the bit definitions and register names would help a lot. Is there a public datasheet for the I2C controller of the Sam460ex board ? and a simple boot image we could use to test ? Some comments below, > > 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(-) > > 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); #define EXTSTS_BCS_FREE 0x40 ? > 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; Don't we have a bit definition for this ? > 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) { can mdidx go above 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; MDBF sounds like a 'M ... Data Buffer Full' status > + } else if (i2c->mdidx == 0) { > + i2c->sts |= IIC_STS_MDBS; what about 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)) { That's an abort ? correct ? > + 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++) { ok. i is used for mdata, but the tct definition should not exceed 3 > + if (!i2c_bus_busy(i2c->bus)) { > + i2c->extsts = (1 << 6); please add a definition for this bit. > + 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; > + } > + } do we need to start the transfer in the loop ? The device address does not change if I am correct. We would not need to test sts against 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; > + } That's the end of the loop I suppose ? > + i2c->xfrcnt = i; what is that xfrcnt field/reg for ? > + 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: So that seems to be 'control' register of the I2C controller. > - i2c->mdcntl = value & 0xdf; > + i2c->mdcntl = value & 0x3d; Could we use a mask built from the bits instead of raw hex value ? > + if (value & IIC_MDCNTL_ESM) { > + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n", > + __func__); > + } > + if (value & IIC_MDCNTL_FMDB) { that's a flush ? > + 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); ditto for the mask. Thanks, C. > + 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; >
Hello, On Sun, 24 Jun 2018, Cédric Le Goater wrote: > On 06/24/2018 01:20 PM, 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. > > ppc4xx support was dropped from u-boot but there is some work being done > to re-add at least ppc-460x. These models should be of interest to emulate > BMC like boards and, in some near future, they could even run OpenBMC. > > I understand that you are adding extended status support, multi transfer > support, better interrupt support. Having some comments on the bit > definitions and register names would help a lot. > > Is there a public datasheet for the I2C controller of the Sam460ex board ? > and a simple boot image we could use to test ? I don't have the manual of this SoC but some of the devices including this i2c controller is similar to the 440EP for which there's a manual online. That's the best reference I know of even if not always applicable for 460ex. > Some comments below, > >> >> 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(-) >> >> 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); > > #define EXTSTS_BCS_FREE 0x40 ? I guess this could be defined but I did not bother as this is not fully emulated. If you think it's worth to add it without the other states I can do in next version. >> 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; > > Don't we have a bit definition for this ? No, like I said above not all states in extsts are emulated, just the bits needed for the guests I've tested, because I don't know how real hardware works. So extsts is mostly a placeholder if it ever needs to be emulated more closely at which points appropriate defines could also be added. >> 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) { > > can mdidx go above 3 ? No, it shouldn't, the > is just to make sure no buffer overrun is possible. >> + 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; > > MDBF sounds like a 'M ... Data Buffer Full' status Master Data Buffer Full >> + } else if (i2c->mdidx == 0) { >> + i2c->sts |= IIC_STS_MDBS; > > what about MDBS ? Master Data Buffer Status, shows if buffer has data or not. >> } >> 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)) { > > That's an abort ? correct ? HMT: Halt Master Transfer, issue stop to halt master transfer. >> + 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++) { > > ok. i is used for mdata, but the tct definition should not exceed 3 TCT: Transfer Count, mdata FIFO is 4 bytes so max index is 3, TCT has 2 bits so it should also not be higher than 3. >> + if (!i2c_bus_busy(i2c->bus)) { >> + i2c->extsts = (1 << 6); > > please add a definition for this bit. See above, this basically resets extsts to initial value which may not match what real hardware does but enough for tested guests. >> + 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; >> + } >> + } > > do we need to start the transfer in the loop ? The device address > does not change if I am correct. We would not need to test sts against > IIC_STS_ERR. I have no idea. This works with all guests I tested, that's U-Boot, AROS, AmigaOS, Linux. Don't know if it matches what real hardware does though. But there are some modes where repeated start is used so I think this needs to be within the loop to allow that. >> + 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; >> + } > > That's the end of the loop I suppose ? > >> + i2c->xfrcnt = i; > > what is that xfrcnt field/reg for ? Transfer count, number of bytes transmitted. >> + 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: > > So that seems to be 'control' register of the I2C controller. Yes. >> - i2c->mdcntl = value & 0xdf; >> + i2c->mdcntl = value & 0x3d; > > Could we use a mask built from the bits instead of raw hex value ? Probably, I don't remember the details any more. >> + if (value & IIC_MDCNTL_ESM) { >> + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n", >> + __func__); >> + } >> + if (value & IIC_MDCNTL_FMDB) { > > that's a flush ? FMDB: Flush Master Data Buffer All these names are in the documentation, you should consult that instead of using this emulation as documentation which it's not just approximate emulation of hardware to satisfy guests. Do we need a new version with more constants added or is this acceptable now? Thanks you, BALATON Zoltan >> + 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); > > ditto for the mask. > > Thanks, > > C. > >> + 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; >> > > >
On 06/24/2018 10:37 PM, BALATON Zoltan wrote: > Hello, > > On Sun, 24 Jun 2018, Cédric Le Goater wrote: >> On 06/24/2018 01:20 PM, 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. >> >> ppc4xx support was dropped from u-boot but there is some work being done >> to re-add at least ppc-460x. These models should be of interest to emulate >> BMC like boards and, in some near future, they could even run OpenBMC. >> >> I understand that you are adding extended status support, multi transfer >> support, better interrupt support. Having some comments on the bit >> definitions and register names would help a lot. >> >> Is there a public datasheet for the I2C controller of the Sam460ex board ? >> and a simple boot image we could use to test ? > > I don't have the manual of this SoC but some of the devices including this i2c controller is similar to the 440EP for which there's a manual online. That's the best reference I know of even if not always applicable for 460ex. I could not find one. Can you provide an URL ? I would to take a look at the register and bit definitions of the I2C controller of the SoC. Thanks, C. > >> Some comments below, >> >>> >>> 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(-) >>> >>> 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); >> >> #define EXTSTS_BCS_FREE 0x40 ? > > I guess this could be defined but I did not bother as this is not fully emulated. If you think it's worth to add it without the other states I can do in next version. > >>> 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; >> >> Don't we have a bit definition for this ? > > No, like I said above not all states in extsts are emulated, just the bits needed for the guests I've tested, because I don't know how real hardware works. So extsts is mostly a placeholder if it ever needs to be emulated more closely at which points appropriate defines could also be added. > >>> 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) { >> >> can mdidx go above 3 ? > > No, it shouldn't, the > is just to make sure no buffer overrun is possible. > >>> + 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; >> >> MDBF sounds like a 'M ... Data Buffer Full' status > > Master Data Buffer Full > >>> + } else if (i2c->mdidx == 0) { >>> + i2c->sts |= IIC_STS_MDBS; >> >> what about MDBS ? > > Master Data Buffer Status, shows if buffer has data or not. > >>> } >>> 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)) { >> >> That's an abort ? correct ? > > HMT: Halt Master Transfer, issue stop to halt master transfer. > >>> + 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++) { >> >> ok. i is used for mdata, but the tct definition should not exceed 3 > > TCT: Transfer Count, mdata FIFO is 4 bytes so max index is 3, TCT has 2 bits so it should also not be higher than 3. > >>> + if (!i2c_bus_busy(i2c->bus)) { >>> + i2c->extsts = (1 << 6); >> >> please add a definition for this bit. > > See above, this basically resets extsts to initial value which may not match what real hardware does but enough for tested guests. > >>> + 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; >>> + } >>> + } >> >> do we need to start the transfer in the loop ? The device address >> does not change if I am correct. We would not need to test sts against >> IIC_STS_ERR. > > I have no idea. This works with all guests I tested, that's U-Boot, AROS, AmigaOS, Linux. Don't know if it matches what real hardware does though. But there are some modes where repeated start is used so I think this needs to be within the loop to allow that. > >>> + 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; >>> + } >> >> That's the end of the loop I suppose ? >> >>> + i2c->xfrcnt = i; >> >> what is that xfrcnt field/reg for ? > > Transfer count, number of bytes transmitted. > >>> + 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: >> >> So that seems to be 'control' register of the I2C controller. > > Yes. > >>> - i2c->mdcntl = value & 0xdf; >>> + i2c->mdcntl = value & 0x3d; >> >> Could we use a mask built from the bits instead of raw hex value ? > > Probably, I don't remember the details any more. > >>> + if (value & IIC_MDCNTL_ESM) { >>> + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n", >>> + __func__); >>> + } >>> + if (value & IIC_MDCNTL_FMDB) { >> >> that's a flush ? > > FMDB: Flush Master Data Buffer > > All these names are in the documentation, you should consult that instead of using this emulation as documentation which it's not just approximate emulation of hardware to satisfy guests. > > Do we need a new version with more constants added or is this acceptable now? > > Thanks you, > BALATON Zoltan > >>> + 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); >> >> ditto for the mask. >> >> Thanks, >> >> C. >> >>> + 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; >>> >> >> >>
On 06/24/2018 10:37 PM, BALATON Zoltan wrote: > Hello, > > On Sun, 24 Jun 2018, Cédric Le Goater wrote: >> On 06/24/2018 01:20 PM, 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. >> >> ppc4xx support was dropped from u-boot but there is some work being done >> to re-add at least ppc-460x. These models should be of interest to emulate >> BMC like boards and, in some near future, they could even run OpenBMC. >> >> I understand that you are adding extended status support, multi transfer >> support, better interrupt support. Having some comments on the bit >> definitions and register names would help a lot. >> >> Is there a public datasheet for the I2C controller of the Sam460ex board ? >> and a simple boot image we could use to test ? > > I don't have the manual of this SoC but some of the devices including this i2c controller is similar to the 440EP for which there's a manual online. That's the best reference I know of even if not always applicable for 460ex. > >> Some comments below, >> >>> >>> 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(-) >>> >>> 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); >> >> #define EXTSTS_BCS_FREE 0x40 ? > > I guess this could be defined but I did not bother as this is not fully emulated. If you think it's worth to add it without the other states I can do in next version. > >>> 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; >> >> Don't we have a bit definition for this ? > > No, like I said above not all states in extsts are emulated, just the bits needed for the guests I've tested, because I don't know how real hardware works. So extsts is mostly a placeholder if it ever needs to be emulated more closely at which points appropriate defines could also be added. > >>> 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) { >> >> can mdidx go above 3 ? > > No, it shouldn't, the > is just to make sure no buffer overrun is possible. > >>> + 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; >> >> MDBF sounds like a 'M ... Data Buffer Full' status > > Master Data Buffer Full > >>> + } else if (i2c->mdidx == 0) { >>> + i2c->sts |= IIC_STS_MDBS; >> >> what about MDBS ? > > Master Data Buffer Status, shows if buffer has data or not. > >>> } >>> 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)) { >> >> That's an abort ? correct ? > > HMT: Halt Master Transfer, issue stop to halt master transfer. > >>> + 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++) { >> >> ok. i is used for mdata, but the tct definition should not exceed 3 > > TCT: Transfer Count, mdata FIFO is 4 bytes so max index is 3, TCT has 2 bits so it should also not be higher than 3. > >>> + if (!i2c_bus_busy(i2c->bus)) { >>> + i2c->extsts = (1 << 6); >> >> please add a definition for this bit. > > See above, this basically resets extsts to initial value which may not match what real hardware does but enough for tested guests. > >>> + 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; >>> + } >>> + } >> >> do we need to start the transfer in the loop ? The device address >> does not change if I am correct. We would not need to test sts against >> IIC_STS_ERR. > > I have no idea. This works with all guests I tested, that's U-Boot, AROS, AmigaOS, Linux. Don't know if it matches what real hardware does though. But there are some modes where repeated start is used so I think this needs to be within the loop to allow that. > >>> + 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; >>> + } >> >> That's the end of the loop I suppose ? >> >>> + i2c->xfrcnt = i; >> >> what is that xfrcnt field/reg for ? > > Transfer count, number of bytes transmitted. > >>> + 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: >> >> So that seems to be 'control' register of the I2C controller. > > Yes. > >>> - i2c->mdcntl = value & 0xdf; >>> + i2c->mdcntl = value & 0x3d; >> >> Could we use a mask built from the bits instead of raw hex value ? > > Probably, I don't remember the details any more. > >>> + if (value & IIC_MDCNTL_ESM) { >>> + qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n", >>> + __func__); >>> + } >>> + if (value & IIC_MDCNTL_FMDB) { >> >> that's a flush ? > > FMDB: Flush Master Data Buffer > > All these names are in the documentation, you should consult that instead of using this emulation as documentation which it's not just approximate emulation of hardware to satisfy guests. > > Do we need a new version with more constants added or is this acceptable now? I have tried an ISO from : http://aros.sourceforge.net/nightly1.php which boots and writes : U-Boot 2010.06.05 (Oct 22 2017 - 17:40:02) CPU: AMCC PowerPC 460EX Rev. B at 1150 MHz (PLB=230 OPB=115 EBC=115) No Security/Kasumi support Bootstrap Option A - Boot ROM Location EBC (8 bits) Internal PCI arbiter disabled 32 kB I-Cache 32 kB D-Cache Board: Sam460ex, PCIe 4x + SATA-2 I2C: ready DRAM: 512 MiB (ECC not enabled, 460 MHz, CL0) *** Warning - bad CRC, using default environment PCI: Bus Dev VenId DevId Class Int 00 06 126f 0501 0380 00 PCIE1: successfully set as root-complex Net: ppc_4xx_eth0 FPGA: Revision 00 (20 0-00-00) SM502: found VGA: NO CARDS and on the graphic window : FLB: no SLB found in any of the designated boot sources, returning to u-boot. the graphical bool loader menu works but I couldn't exercise much more the system. I then used the 4.5 kernel from : http://www.supertuxkart-amiga.de/amiga/sam.html#downloads with this command line : qemu-system-ppc -M sam460ex -serial mon:stdio -nodefaults -kernel ./Sam460ex-4.5.0/boot/uImage-sam460ex-4.5.0 -dtb Sam460ex-4.5.0/boot/canyonlands.dtb -append "console=ttyS0,119200" output was : [ 0.000000] Using Canyonlands machine description [ 0.000000] Linux version 4.5.0-sam460ex (root@julian-VirtualBox) (gcc version 5.3.1 20160205 (Ubuntu 5.3.1-8ubuntu2) ) #1 PREEMPT Mon Mar 14 06:27:13 AST 2016 ... [ 2.194282] i2c /dev entries driver [ 2.201326] rtc-m41t80 0-0068: rtc core: registered m41t80 as rtc0 [ 2.202357] ibm-iic 4ef600700.i2c: using standard (100 kHz) mode [ 2.203796] ibm-iic 4ef600800.i2c: using standard (100 kHz) mode ... [ 2.269934] rtc-m41t80 0-0068: setting system clock to 2018-06-28 09:49:28 UTC (1530179368) So I would say from the review of the code and the tests that we are fine. Could please document a little more the register and bit definitions and remove the raw hex values when possible ? With these addressed : Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > Thanks you, > BALATON Zoltan > >>> + 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); >> >> ditto for the mask. >> >> Thanks, >> >> C. >> >>> + 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; >>> >> >> >>
On Thu, 28 Jun 2018, Cédric Le Goater wrote: > I have tried an ISO from : > > http://aros.sourceforge.net/nightly1.php > > which boots and writes : > > U-Boot 2010.06.05 (Oct 22 2017 - 17:40:02) > > CPU: AMCC PowerPC 460EX Rev. B at 1150 MHz (PLB=230 OPB=115 EBC=115) > No Security/Kasumi support > Bootstrap Option A - Boot ROM Location EBC (8 bits) > Internal PCI arbiter disabled > 32 kB I-Cache 32 kB D-Cache > Board: Sam460ex, PCIe 4x + SATA-2 > I2C: ready > DRAM: 512 MiB (ECC not enabled, 460 MHz, CL0) > *** Warning - bad CRC, using default environment > > PCI: Bus Dev VenId DevId Class Int > 00 06 126f 0501 0380 00 > PCIE1: successfully set as root-complex > Net: ppc_4xx_eth0 > FPGA: Revision 00 (20 0-00-00) > SM502: found > VGA: NO CARDS This is the U-Boot firmware that prints this. > and on the graphic window : > > FLB: no SLB found in any of the designated boot sources, returning to u-boot. And also this is from U-Boot saying it could not find bootable image so it did not boot. (One could test i2c from u_boot with date and i2c commands as well.) > the graphical bool loader menu works but I couldn't exercise much more the > system. Have you by chance used the -cdrom shortcut? That tries to connect CDROM to secondary master but the sii3112 this board uses only has 2 SATA ports so this won't work. You need to use the long options like -drive if=none,id=cd,file=aros-sam440-ppc.iso,format=raw \ -device ide-cd,drive=cd,bus=ide.0 I'm not sure how to fix -cdrom or have it print a meaningful warning or error. > I then used the 4.5 kernel from : > > http://www.supertuxkart-amiga.de/amiga/sam.html#downloads > > with this command line : > > qemu-system-ppc -M sam460ex -serial mon:stdio -nodefaults -kernel ./Sam460ex-4.5.0/boot/uImage-sam460ex-4.5.0 -dtb Sam460ex-4.5.0/boot/canyonlands.dtb -append "console=ttyS0,119200" > > output was : > > > [ 0.000000] Using Canyonlands machine description > [ 0.000000] Linux version 4.5.0-sam460ex (root@julian-VirtualBox) (gcc version 5.3.1 20160205 (Ubuntu 5.3.1-8ubuntu2) ) #1 PREEMPT Mon Mar 14 06:27:13 AST 2016 > ... > [ 2.194282] i2c /dev entries driver > [ 2.201326] rtc-m41t80 0-0068: rtc core: registered m41t80 as rtc0 > [ 2.202357] ibm-iic 4ef600700.i2c: using standard (100 kHz) mode > [ 2.203796] ibm-iic 4ef600800.i2c: using standard (100 kHz) mode > ... > [ 2.269934] rtc-m41t80 0-0068: setting system clock to 2018-06-28 09:49:28 UTC (1530179368) > > > So I would say from the review of the code and the tests that we are fine. > > Could please document a little more the register and bit definitions and > remove the raw hex values when possible ? I'll check what can be dont but I think by now only the extsts state are not #define-d which are also not really implemented and they are not bits but 3 bit values so not sure what's the best way to define them. Other hex values are some reserved bit masks I think that probably should not need to be #defined. > With these addressed : > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > Thanks, > > C. > > Thanks for testing, BALATON Zoltan
On 06/28/2018 04:34 PM, BALATON Zoltan wrote: > On Thu, 28 Jun 2018, Cédric Le Goater wrote: >> I have tried an ISO from : >> >> http://aros.sourceforge.net/nightly1.php >> >> which boots and writes : >> >> U-Boot 2010.06.05 (Oct 22 2017 - 17:40:02) >> >> CPU: AMCC PowerPC 460EX Rev. B at 1150 MHz (PLB=230 OPB=115 EBC=115) >> No Security/Kasumi support >> Bootstrap Option A - Boot ROM Location EBC (8 bits) >> Internal PCI arbiter disabled >> 32 kB I-Cache 32 kB D-Cache >> Board: Sam460ex, PCIe 4x + SATA-2 >> I2C: ready >> DRAM: 512 MiB (ECC not enabled, 460 MHz, CL0) >> *** Warning - bad CRC, using default environment >> >> PCI: Bus Dev VenId DevId Class Int >> 00 06 126f 0501 0380 00 >> PCIE1: successfully set as root-complex >> Net: ppc_4xx_eth0 >> FPGA: Revision 00 (20 0-00-00) >> SM502: found >> VGA: NO CARDS > > This is the U-Boot firmware that prints this. > >> and on the graphic window : >> >> FLB: no SLB found in any of the designated boot sources, returning to u-boot. > > And also this is from U-Boot saying it could not find bootable image so it did not boot. (One could test i2c from u_boot with date and i2c commands as well.) > >> the graphical bool loader menu works but I couldn't exercise much more the >> system. > > Have you by chance used the -cdrom shortcut? That tries to connect CDROM to secondary master but the sii3112 this board uses only has 2 SATA ports so this won't work. You need to use the long options like > > -drive if=none,id=cd,file=aros-sam440-ppc.iso,format=raw \ > -device ide-cd,drive=cd,bus=ide.0 > > I'm not sure how to fix -cdrom or have it print a meaningful warning or error. Looks good : [BATT] i2c=011bc904 [BATT] Probing i2c RTC... [BATT] RTC found. Object=011ba234 [BATT] Dump: 35 46 15 04 28 06 18 C.
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;
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(-)