From: David Gibson <david@gibson.dropbear.id.au>
To: BALATON Zoltan <balaton@eik.bme.hu>
Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org,
Alexander Graf <agraf@suse.de>
Subject: Re: [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely
Date: Wed, 20 Jun 2018 15:25:32 +1000 [thread overview]
Message-ID: <20180620052532.GB24636@umbus.fritz.box> (raw)
In-Reply-To: <5227f6905dc3cf9aa0ed933b591d1741acbeeac5.1529398335.git.balaton@eik.bme.hu>
[-- Attachment #1: Type: text/plain, Size: 13345 bytes --]
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;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2018-06-20 7:12 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-19 8:52 [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 02/11] ppc4xx_i2c: Implement directcntl register BALATON Zoltan
2018-06-20 5:20 ` David Gibson
2018-06-21 7:17 ` BALATON Zoltan
2018-06-22 1:59 ` David Gibson
2018-06-22 8:00 ` BALATON Zoltan
2018-06-22 10:49 ` David Gibson
2018-11-28 10:28 ` Thomas Huth
2018-11-28 11:26 ` BALATON Zoltan
2018-11-28 11:30 ` Thomas Huth
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 08/11] sm501: Perform a full update after palette change BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 10/11] sm501: Set updated region dirty after 2D operation BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 03/11] ppc4xx_i2c: Rewrite to model hardware more closely BALATON Zoltan
2018-06-20 5:25 ` David Gibson [this message]
2018-06-21 6:51 ` BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 11/11] sm501: Fix support for non-zero frame buffer start address BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 09/11] sm501: Use values from the pitch register for 2d operations BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 07/11] sm501: Implement i2c part for reading monitor EDID BALATON Zoltan
2018-06-20 7:17 ` David Gibson
2018-06-22 12:00 ` Philippe Mathieu-Daudé
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 01/11] ppc4xx_i2c: Remove unimplemented sdata and intr registers BALATON Zoltan
2018-06-20 0:14 ` David Gibson
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 04/11] hw/timer: Add basic M41T80 emulation BALATON Zoltan
2018-06-19 8:52 ` [Qemu-devel] [PATCH v4 06/11] target/ppc: Add missing opcode for icbt on PPC440 BALATON Zoltan
2018-06-20 5:41 ` David Gibson
2018-06-19 9:12 ` [Qemu-devel] [PATCH v4 05/11] sam460ex: Add RTC device BALATON Zoltan
2018-06-20 7:25 ` [Qemu-devel] [PATCH v4 00/11] Misc sam460ex improvements David Gibson
2018-06-21 7:00 ` BALATON Zoltan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180620052532.GB24636@umbus.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=agraf@suse.de \
--cc=balaton@eik.bme.hu \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).