[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 03/17] i2c: add mpc8540 i2c controller
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH 03/17] i2c: add mpc8540 i2c controller |
Date: |
Wed, 29 Nov 2017 12:32:46 +1100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Mon, Nov 27, 2017 at 01:05:11PM -0600, Michael Davidsaver wrote:
> On 11/27/2017 01:12 AM, David Gibson wrote:
> > On Sun, Nov 26, 2017 at 03:59:01PM -0600, Michael Davidsaver wrote:
> >> Signed-off-by: Michael Davidsaver <address@hidden>
> >> ---
> >> hw/i2c/Makefile.objs | 1 +
> >> hw/i2c/mpc8540_i2c.c | 307
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> hw/i2c/trace-events | 6 +
> >> 3 files changed, 314 insertions(+)
> >> create mode 100644 hw/i2c/mpc8540_i2c.c
> >>
> >> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> >> index 0594dea3ae..79af1dd901 100644
> >> --- a/hw/i2c/Makefile.objs
> >> +++ b/hw/i2c/Makefile.objs
> >> @@ -9,3 +9,4 @@ common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
> >> common-obj-$(CONFIG_ASPEED_SOC) += aspeed_i2c.o
> >> obj-$(CONFIG_OMAP) += omap_i2c.o
> >> obj-$(CONFIG_PPC4XX) += ppc4xx_i2c.o
> >> +obj-$(CONFIG_E500) += mpc8540_i2c.o
> >> diff --git a/hw/i2c/mpc8540_i2c.c b/hw/i2c/mpc8540_i2c.c
> >> new file mode 100644
> >> index 0000000000..b9f5773b35
> >> --- /dev/null
> >> +++ b/hw/i2c/mpc8540_i2c.c
> >> @@ -0,0 +1,307 @@
> >> +/*
> >> + * MPC8540 I2C bus interface
> >> + * As described in
> >> + * MPC8540 PowerQUICC III Integrated Host Processor Reference Manual,
> >> Rev. 1
> >> + * Part 2 chapter 11
> >> + *
> >> + * Compatible I2C controllers are found on other Freescale chips
> >> + * including mpc8544 and P2010.
> >> + *
> >> + * Copyright (c) 2015 Michael Davidsaver
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2. See
> >> + * the LICENSE file in the top-level directory.
> >> + */
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/log.h"
> >> +#include "hw/hw.h"
> >> +#include "hw/registerfields.h"
> >> +#include "hw/i2c/i2c.h"
> >> +#include "hw/sysbus.h"
> >> +#include "qemu/error-report.h"
> >> +
> >> +#include "trace.h"
> >> +
> >> +/* #define DEBUG_LVL 0 */
> >> +
> >> +#ifdef DEBUG_LVL
> >> +#define DPRINTK(LVL, FMT, ...) do { \
> >> + if ((LVL) <= DEBUG_LVL) {\
> >> + info_report(TYPE_MPC8540_I2C " : " FMT, ## __VA_ARGS__); \
> >> + } } while (0)
> >> +#else
> >> +#define DPRINTK(LVL, FMT, ...) do {} while (0)
> >> +#endif
> >
> > So, you have both this DPRINTK stuff, and some trace events, which
> > seems a bit odd? Why not just one or the other.
>
> My thinking is to have trace events for the commonly used points (by me at
> least)
> for troubleshooting devices on the i2c bus, but leave the others for
> troubleshooting
> the i2c controller itself as DPRINTK.
>
> So this is mainly my laziness in not seeing sufficient benefit to defining
> events for the
> remaining 5 DPRINTK s.
I sympathize - I have mixed feelings about tracepoints myself. But
they remain the preferred debugging approach in qemu.
To address they issue above, I suggest you use a consistent naming
scheme for the "common" vs. "rare" tracepoints, that way the whole of
one set or the other can be enabled/disabled with a single wildcard.
>
> >> +
> >> +#define LOG(MSK, FMT, ...) qemu_log_mask(MSK, TYPE_MPC8540_I2C \
> >> + " : " FMT "\n", ## __VA_ARGS__)
> >> +
> >> +#define TYPE_MPC8540_I2C "mpc8540-i2c"
> >> +#define MPC8540_I2C(obj) OBJECT_CHECK(MPC8540I2CState, (obj),
> >> TYPE_MPC8540_I2C)
> >> +
> >> +/* offsets relative to CCSR offset 0x3000 */
> >> +#define R_I2CADR (0)
> >> +#define R_I2CFDR (4)
> >> +#define R_I2CCR (8)
> >> +#define R_I2CSR (0xc)
> >> +#define R_I2CDR (0x10)
> >> +#define R_I2CDFSRR (0x14)
> >> +
> >> +FIELD(I2CCR, MEN, 7, 1)
> >> +FIELD(I2CCR, MIEN, 6, 1)
> >> +FIELD(I2CCR, MSTA, 5, 1)
> >> +FIELD(I2CCR, MTX, 4, 1)
> >> +FIELD(I2CCR, TXAK, 3, 1)
> >> +FIELD(I2CCR, RSTA, 2, 1)
> >> +FIELD(I2CCR, BCST, 0, 1)
> >> +
> >> +FIELD(I2CSR, MCF, 7, 1)
> >> +FIELD(I2CSR, MAAS, 6, 1)
> >> +FIELD(I2CSR, MBB, 5, 1)
> >> +FIELD(I2CSR, MAL, 4, 1)
> >> +FIELD(I2CSR, BCSTM, 3, 1)
> >> +FIELD(I2CSR, SRW, 2, 1)
> >> +FIELD(I2CSR, MIF, 1, 1)
> >> +FIELD(I2CSR, RXAK, 0, 1)
> >> +
> >> +typedef struct MPC8540I2CState {
> >> + SysBusDevice parent_obj;
> >> +
> >> + I2CBus *bus;
> >> +
> >> + uint8_t ctrl, sts;
> >> + uint8_t freq, filt;
> >> + /* Reads are pipelined, this is the next data value */
> >> + uint8_t dbuf, dbuf_valid;
> >> +
> >> + qemu_irq irq;
> >> +
> >> + MemoryRegion mmio;
> >> +} MPC8540I2CState;
> >> +
> >> +#define I2CCR(BIT) FIELD_EX32(i2c->ctrl, I2CCR, BIT)
> >> +#define I2CSR(BIT) FIELD_EX32(i2c->sts, I2CSR, BIT)
> >> +
> >> +#define I2CSR_SET(BIT, VAL) do {\
> >> + i2c->sts = FIELD_DP32(i2c->sts, I2CSR, BIT, VAL);\
> >> + } while (0)
> >> +
> >> +static
> >> +void mpc8540_update_irq(MPC8540I2CState *i2c)
> >> +{
> >> + int ena = i2c->ctrl & 0x40,
> >> + sts = i2c->sts & 0x02,
> >> + act = !!(ena && sts);
> >> +
> >> + DPRINTK(1, "IRQ %c ena %c sts %c",
> >> + act ? 'X' : '_',
> >> + ena ? 'X' : '_',
> >> + sts ? 'X' : '_');
> >> +
> >> + qemu_set_irq(i2c->irq, act);
> >> +}
> >> +
> >> +static
> >> +uint64_t mpc8540_i2c_read(void *opaque, hwaddr addr, unsigned size)
> >> +{
> >> + MPC8540I2CState *i2c = opaque;
> >> + uint32_t val;
> >> +
> >> + switch (addr) {
> >> + case R_I2CADR: /* ADDR */
> >> + val = 0;
> >> + break;
> >> + case R_I2CFDR: /* Freq Div. */
> >> + val = i2c->freq;
> >> + break;
> >> + case R_I2CCR: /* CONTROL */
> >> + val = i2c->ctrl & ~0x06;
> >> + break;
> >> + case R_I2CSR: /* STATUS */
> >> + val = i2c->sts;
> >> + break;
> >> + case R_I2CDR: /* DATA */
> >> + /* Reads are "pipelined" and so return the previous value of the
> >> + * register
> >> + */
> >> + val = i2c->dbuf;
> >> + if (I2CCR(MEN) && I2CSR(MBB)) { /* enabled and busy */
> >> + if (!i2c_bus_busy(i2c->bus) || I2CCR(MTX)) {
> >> + if (!i2c->dbuf_valid) {
> >> + LOG(LOG_GUEST_ERROR, "Read during addr or tx");
> >> + }
> >> + i2c->dbuf = 0xff;
> >> + i2c->dbuf_valid = false;
> >> + } else {
> >> + int ret = i2c_recv(i2c->bus);
> >> + i2c->dbuf = (uint8_t)ret;
> >> + i2c->dbuf_valid = true;
> >> + trace_mpc8540_i2c_read(i2c->dbuf);
> >> + I2CSR_SET(MIF, 1);
> >> + I2CSR_SET(RXAK, 0);
> >> + mpc8540_update_irq(i2c);
> >> + }
> >> + } else {
> >> + i2c->dbuf = 0xff;
> >> + i2c->dbuf_valid = false;
> >> + LOG(LOG_GUEST_ERROR, "Read when not enabled or busy");
> >> + }
> >> + break;
> >> + case R_I2CDFSRR: /* FILTER */
> >> + val = i2c->filt;
> >> + break;
> >> + default:
> >> + val = 0xff;
> >> + }
> >> +
> >> + DPRINTK(addr == 0xc ? 2 : 1, " read %08x -> %08x",
> >> + (unsigned)addr, (unsigned)val);
> >> + return val;
> >> +}
> >> +
> >> +static
> >> +void mpc8540_i2c_write(void *opaque, hwaddr addr, uint64_t val, unsigned
> >> size)
> >> +{
> >> + MPC8540I2CState *i2c = opaque;
> >> +
> >> + DPRINTK(1, " write %08x <- %08x", (unsigned)addr, (unsigned)val);
> >> +
> >> + switch (addr) {
> >> + case R_I2CADR: /* ADDR */
> >> + break;
> >> + case R_I2CFDR: /* Freq Div. */
> >> + i2c->freq = val & 0x3f;
> >> + break;
> >> + case R_I2CCR: /* CONTROL CCR */
> >> + if (!FIELD_EX32(val, I2CCR, MEN)) {
> >> + DPRINTK(0, "Not Enabled");
> >> +
> >> + } else if (!I2CCR(MSTA) && FIELD_EX32(val, I2CCR, MSTA)) {
> >> + /* MSTA 0 -> 1 is START */
> >> +
> >> + I2CSR_SET(MBB, 1);
> >> + if (I2CCR(MTX)) {
> >> + trace_mpc8540_i2c_event("START Tx");
> >> + } else {
> >> + trace_mpc8540_i2c_event("START Rx");
> >> + }
> >> + i2c_end_transfer(i2c->bus); /* paranoia */
> >> +
> >> + } else if (I2CCR(MSTA) && !FIELD_EX32(val, I2CCR, MSTA)) {
> >> + /* MSTA 1 -> 0 is STOP */
> >> +
> >> + I2CSR_SET(MBB, 0);
> >> + trace_mpc8540_i2c_event("STOP");
> >> + i2c_end_transfer(i2c->bus);
> >> +
> >> + } else if (I2CCR(MSTA) && FIELD_EX32(val, I2CCR, RSTA)) {
> >> + i2c_end_transfer(i2c->bus);
> >> + I2CSR_SET(MBB, 1);
> >> + if (I2CCR(MTX)) {
> >> + trace_mpc8540_i2c_event("REPEAT START Tx");
> >> + } else {
> >> + trace_mpc8540_i2c_event("REPEAT START Rx");
> >> + }
> >> +
> >> + }
> >> + /* RSTA always reads zero, bit 1 unusd */
> >> + val &= 0xf9;
> >> + i2c->ctrl = val;
> >> + mpc8540_update_irq(i2c);
> >> + break;
> >> + case R_I2CSR: /* STATUS CSR */
> >> + /* only MAL and MIF are writable */
> >> + val &= 0x12;
> >> + i2c->sts &= ~0x12;
> >> + i2c->sts |= val;
> >> + mpc8540_update_irq(i2c);
> >> + break;
> >> + case R_I2CDR: /* DATA CDR */
> >> + if (I2CCR(MEN) && I2CSR(MBB)) { /* enabled and busy */
> >> + if (!i2c_bus_busy(i2c->bus)) {
> >> + if (i2c_start_transfer(i2c->bus, val >> 1, val & 1)) {
> >> + LOG(LOG_GUEST_ERROR, "I2C no device %02x",
> >> + (unsigned)(val & 0xfe));
> >> + } else {
> >> + trace_mpc8540_i2c_address((unsigned)(val & 0xfe),
> >> + (val & 0x1) ? 'R' : 'T');
> >> + }
> >> + I2CSR_SET(MIF, 1);
> >> + I2CSR_SET(RXAK, 0);
> >> +
> >> + } else if (I2CCR(MTX)) {
> >> + trace_mpc8540_i2c_write((unsigned)val);
> >> + i2c_send(i2c->bus, val);
> >> + I2CSR_SET(MIF, 1);
> >> + I2CSR_SET(RXAK, 0);
> >> + } else {
> >> + LOG(LOG_GUEST_ERROR, "I2CDR Write during read");
> >> + }
> >> + mpc8540_update_irq(i2c);
> >> + } else {
> >> + LOG(LOG_GUEST_ERROR, "I2CDR Write when not enabled or busy");
> >> + }
> >> + break;
> >> + case R_I2CDFSRR: /* FILTER */
> >> + val &= 0x3f;
> >> + i2c->filt = val;
> >> + break;
> >> + }
> >> +
> >> + DPRINTK(1, "I2CCR = %02x I2SCR = %02x", i2c->ctrl, i2c->sts);
> >> +}
> >> +
> >> +static const MemoryRegionOps i2c_ops = {
> >> + .read = mpc8540_i2c_read,
> >> + .write = mpc8540_i2c_write,
> >> + .endianness = DEVICE_NATIVE_ENDIAN,
> >> + .impl = {
> >> + .min_access_size = 1,
> >> + .max_access_size = 1,
> >> + },
> >> +};
> >> +
> >> +static
> >> +void mpc8540_i2c_reset(DeviceState *dev)
> >> +{
> >> + MPC8540I2CState *i2c = MPC8540_I2C(dev);
> >> +
> >> + i2c->sts = 0x81; /* transfer complete and ack received */
> >> + i2c->dbuf_valid = false;
> >> +}
> >> +
> >> +static void mpc8540_i2c_inst_init(DeviceState *dev, Error **errp)
> >> +{
> >> + MPC8540I2CState *i2c = MPC8540_I2C(dev);
> >> +
> >> + i2c->bus = i2c_init_bus(dev, "bus");
> >> +
> >> + memory_region_init_io(&i2c->mmio, OBJECT(dev),
> >> + &i2c_ops, i2c, TYPE_MPC8540_I2C, 0x18);
> >> +
> >> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &i2c->mmio);
> >> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &i2c->irq);
> >> +}
> >
> > As someone pointed out on the earlier version, this stuff needs to go
> > into a realize() function rather than instance_init - instance_init is
> > called too early to go safely constructing and installing memory
> > regions.
>
> In fact I did (see "dc->realize" below), but apparently managed to change the
> argument list
> without thinking to re-name the function. I will do so now...
>
> >> +
> >> +static void mpc8540_i2c_class_init(ObjectClass *klass, void *data)
> >> +{
> >> + DeviceClass *dc = DEVICE_CLASS(klass);
> >> +
> >> + dc->realize = &mpc8540_i2c_inst_init;
> >> + dc->reset = &mpc8540_i2c_reset;
> >> +}
> >> +
> >> +static const TypeInfo mpc8540_i2c_type = {
> >> + .name = TYPE_MPC8540_I2C,
> >> + .parent = TYPE_SYS_BUS_DEVICE,
> >> + .instance_size = sizeof(MPC8540I2CState),
> >> + .class_size = sizeof(SysBusDeviceClass),
> >> + .class_init = mpc8540_i2c_class_init,
> >> +};
> >> +
> >> +static void mpc8540_i2c_register(void)
> >> +{
> >> + type_register_static(&mpc8540_i2c_type);
> >> +}
> >> +
> >> +type_init(mpc8540_i2c_register)
> >> diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> >> index 9284b1fbad..ac38d76984 100644
> >> --- a/hw/i2c/trace-events
> >> +++ b/hw/i2c/trace-events
> >> @@ -1 +1,7 @@
> >> # See docs/devel/tracing.txt for syntax documentation.
> >> +
> >> +# hw/i2c/mpc8540_i2c.c
> >> +mpc8540_i2c_event(const char *evt) "Bus Event %s"
> >> +mpc8540_i2c_address(uint8_t addr, const char direction) "Address device
> >> 0x%02x for %cX"
> >> +mpc8540_i2c_write(uint8_t byte) "Write 0x%02x"
> >> +mpc8540_i2c_read(uint8_t byte) "Read 0x%02x"
> >
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
- [Qemu-ppc] [PATCH 07/17] e500: fix pci host bridge class/type, (continued)
- [Qemu-ppc] [PATCH 07/17] e500: fix pci host bridge class/type, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 04/17] qtest: add e500_i2c_create(), Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 05/17] timer: generalize Dallas/Maxim RTC i2c devices, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 10/17] e500: move uarts CCSR, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 01/17] openpic: debug w/ info_report(), Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 03/17] i2c: add mpc8540 i2c controller, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 11/17] e500: derive baud from CCB clock, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 12/17] e500: add i2c controller to CCSR, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 09/17] e500: move mpic under CCSR, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 08/17] e500: additional CCSR registers, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 06/17] tests: rewrite testing for DS RTC devices, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 13/17] e500: move PCI host bridge into CCSR, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 14/17] e500: split mpc8544ds specific initialization, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 16/17] tests: run ds-rtc-i2c-test w/ ppc/mvme3100, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 17/17] tests: add mvme3100-test, Michael Davidsaver, 2017/11/26
- [Qemu-ppc] [PATCH 15/17] ppc: add mvme3100 machine, Michael Davidsaver, 2017/11/26