Re: [Qemu-devel] PPC: E500 [PATCH v2 ]: Add FSL i2c controller and integrate RTC with it.

[Alexander Graf]

On 29.12.14 11:25, Amit Tomar wrote:
> 
> Changes since v1:
>     * Move it to GPL v2+ 
>     * Replace the printf with DPRINTF
>     * Fix the coding style issues
>     * Change the subject line
> 
> This patch adds an emulation model for i2c controller found on most of the 
> FSL SoCs.
> It also integrates the RTC that sits on the i2c Bus with e500 machine model.
> 
> 
> 
> Signed-off-by: Amit Singh Tomar <address@hidden>
> ---
>  default-configs/ppc-softmmu.mak   |    2 +
>  default-configs/ppc64-softmmu.mak |    2 +
>  hw/i2c/Makefile.objs              |    1 +
>  hw/i2c/mpc_i2c.c                  |  364 
> +++++++++++++++++++++++++++++++++++++
>  hw/ppc/e500.c                     |   50 ++++-
>  5 files changed, 418 insertions(+), 1 deletion(-)
>  create mode 100644 hw/i2c/mpc_i2c.c
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index d725b23..6fdd39a 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -9,6 +9,8 @@ CONFIG_M48T59=y
>  CONFIG_VGA=y
>  CONFIG_VGA_PCI=y
>  CONFIG_SERIAL=y
> +CONFIG_MPC_I2C=y
> +CONFIG_DS1338=y
>  CONFIG_PARALLEL=y
>  CONFIG_I8254=y
>  CONFIG_PCKBD=y
> diff --git a/default-configs/ppc64-softmmu.mak 
> b/default-configs/ppc64-softmmu.mak
> index bd30d69..3ad3f58 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -9,6 +9,8 @@ CONFIG_M48T59=y
>  CONFIG_VGA=y
>  CONFIG_VGA_PCI=y
>  CONFIG_SERIAL=y
> +CONFIG_MPC_I2C=y
> +CONFIG_DS1338=y
>  CONFIG_PARALLEL=y
>  CONFIG_I8254=y
>  CONFIG_PCKBD=y
> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
> index 648278e..6e6d00d 100644
> --- a/hw/i2c/Makefile.objs
> +++ b/hw/i2c/Makefile.objs
> @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) += smbus_ich9.o
>  common-obj-$(CONFIG_APM) += pm_smbus.o
>  common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
>  common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
> +common-obj-$(CONFIG_MPC_I2C) += mpc_i2c.o
>  obj-$(CONFIG_OMAP) += omap_i2c.o
> diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c
> new file mode 100644
> index 0000000..84d13dd
> --- /dev/null
> +++ b/hw/i2c/mpc_i2c.c
> @@ -0,0 +1,364 @@
> +/*
> + * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: Amit Tomar, <address@hidden>
> + *
> + * Description:
> + * This file is derived from IMX I2C controller,
> + * by Jean-Christophe DUBOIS .
> + *
> + * Thanks to Scott Wood and Alexander Graf for their kind help on this.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2 or later,
> + * as published by the Free Software Foundation.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include "hw/sysbus.h"
> +#include "hw/i2c/i2c.h"
> +#include "qemu/bitops.h"
> +#include "hw/ptimer.h"
> +
> +/*#define DEBUG_I2C*/
> +
> +#ifdef DEBUG_I2C
> +#define DPRINTF(fmt, ...)              \
> +    do { fprintf(stderr, "mpc_i2c[%s]: " fmt, __func__, ## __VA_ARGS__); \
> +    } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define TYPE_MPC_I2C "mpc-i2c"
> +#define MPC_I2C(obj) \
> +    OBJECT_CHECK(MPCI2CState, (obj), TYPE_MPC_I2C)
> +
> +#define MPC_I2C_ADR   0x00
> +#define MPC_I2C_FDR   0x04
> +#define MPC_I2C_CR    0x08
> +#define MPC_I2C_SR    0x0c
> +#define MPC_I2C_DR    0x10
> +#define MPC_I2C_DFSRR 0x14
> +
> +#define CCR_MEN  (1<<7)
> +#define CCR_MIEN (1<<6)
> +#define CCR_MSTA (1<<5)
> +#define CCR_MTX  (1<<4)
> +#define CCR_TXAK (1<<3)
> +#define CCR_RSTA (1<<2)
> +#define CCR_BCST (1<<0)
> +
> +#define CSR_MCF  (1<<7)
> +#define CSR_MAAS (1<<6)
> +#define CSR_MBB  (1<<5)
> +#define CSR_MAL  (1<<4)
> +#define CSR_SRW  (1<<2)
> +#define CSR_MIF  (1<<1)
> +#define CSR_RXAK (1<<0)
> +
> +#define CADR_MASK 0xFE
> +#define CFDR_MASK 0x3F
> +#define CCR_MASK  0xFC
> +#define CSR_MASK  0xED
> +#define CDR_MASK  0xFF
> +
> +#define CYCLE_RESET 0xFF
> +
> +typedef struct MPCI2CState {
> +    SysBusDevice parent_obj;
> +
> +    I2CBus *bus;
> +    qemu_irq irq;
> +    MemoryRegion iomem;
> +
> +    uint8_t address;
> +    uint8_t adr;
> +    uint8_t fdr;
> +    uint8_t cr;
> +    uint8_t sr;
> +    uint8_t dr;
> +    uint8_t dfssr;
> +} MPCI2CState;
> +
> +static bool mpc_i2c_is_enabled(MPCI2CState *s)
> +{
> +    return s->cr & CCR_MEN;
> +}
> +
> +static bool mpc_i2c_is_master(MPCI2CState *s)
> +{
> +    return s->cr & CCR_MSTA;
> +}
> +
> +static bool mpc_i2c_direction_is_tx(MPCI2CState *s)
> +{
> +    return s->cr & CCR_MTX;
> +}
> +
> +static bool mpc_i2c_irq_pending(MPCI2CState *s)
> +{
> +    return s->sr & CSR_MIF;
> +}
> +
> +static bool mpc_i2c_irq_is_enabled(MPCI2CState *s)
> +{
> +    return s->cr & CCR_MIEN;
> +}
> +
> +static void mpc_i2c_reset(DeviceState *dev)
> +{
> +    MPCI2CState *i2c = MPC_I2C(dev);
> +
> +    i2c->address = 0xFF;
> +    i2c->adr = 0x00;
> +    i2c->fdr = 0x00;
> +    i2c->cr =  0x00;
> +    i2c->sr =  0x81;
> +    i2c->dr =  0x00;
> +}
> +
> +static void mpc_i2c_irq(MPCI2CState *s)
> +{
> +    bool irq_active = false;
> +
> +    if (mpc_i2c_is_enabled(s) && mpc_i2c_irq_is_enabled(s)
> +                              && mpc_i2c_irq_pending(s)) {
> +        irq_active = true;
> +    }
> +
> +    if (irq_active) {
> +        qemu_irq_raise(s->irq);
> +    } else {
> +        qemu_irq_lower(s->irq);
> +    }
> +}
> +
> +static void mpc_i2c_soft_reset(MPCI2CState *s)
> +{
> +    /* This is a soft reset. ADR is preserved during soft resets */
> +    uint8_t adr = s->adr;
> +    mpc_i2c_reset(DEVICE(s));
> +    s->adr = adr;
> +}
> +
> +static void  mpc_i2c_address_send(MPCI2CState *s)
> +{
> +    /* if returns non zero slave address is not right */
> +    if (i2c_start_transfer(s->bus, s->dr >> 1, s->dr & (0x01))) {
> +        s->sr |= CSR_RXAK;
> +    } else {
> +        s->address = s->dr;
> +        s->sr &= ~CSR_RXAK;
> +        s->sr |=  CSR_MCF; /*Byte Transfer is completed*/

In general please try to keep spaces between *s and letters:

  /* Byte transfer is completed */

> +        s->sr |=  CSR_MIF; /*Needs to be set after Byte Transfer is 
> completed*/

This line looks longer than 80 characters. Do you think you can
configure your editor to tell you when you exceed 80? It makes life a
lot easier when the editor tells you these things.

For an example .vimrc config, use

:au BufWinEnter * highlight LineTooLong cterm=bold ctermbg=yellow
guibg=LightYellow
:au BufWinEnter * let w:m2=matchadd('LineTooLong', '\%>80v.\+', -1)

> +        mpc_i2c_irq(s);
> +    }
> +}
> +
> +static void  mpc_i2c_data_send(MPCI2CState *s)
> +{
> +    if (i2c_send(s->bus, s->dr)) {
> +        /* if the target return non zero then end the transfer */

I think the comment is too verbose. "if the target returns non zero" is
already implied by the if() above. How about

  /* End of transfer */

> +        s->sr |= CSR_RXAK;
> +        i2c_end_transfer(s->bus);
> +    } else {
> +        s->sr &= ~CSR_RXAK;
> +        s->sr |=  CSR_MCF; /* Byte Transfer is completed*/

Spaces missing at the end before asterisk (*) :).

> +        s->sr |=  CSR_MIF; /* Needs to be set after Byte Transfer is 
> completed*/
> +        mpc_i2c_irq(s);
> +    }
> +}
> +
> +static void  mpc_i2c_data_recive(MPCI2CState *s)
> +{
> +    int ret;
> +    /* get the next byte */
> +    ret = i2c_recv(s->bus);
> +    if (ret >= 0) {
> +        s->sr |= CSR_MCF;/* Byte Transfer is completed*/
> +        s->sr |= CSR_MIF;/* Needs to be set after Byte Transfer is 
> completed*/
> +        mpc_i2c_irq(s);
> +    } else {
> +        DPRINTF("read failed for device");
> +        ret = 0xff;
> +    }
> +    s->dr = ret;
> +}
> +
> +static uint64_t mpc_i2c_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    MPCI2CState *s = opaque;
> +    uint8_t value;
> +
> +    switch (addr) {
> +    case MPC_I2C_ADR:
> +        value = s->adr;
> +        break;
> +    case MPC_I2C_FDR:
> +        value = s->fdr;
> +        break;
> +    case MPC_I2C_CR:
> +        value = s->cr;
> +        break;
> +    case MPC_I2C_SR:
> +        value = s->sr;
> +        break;
> +    case MPC_I2C_DR:
> +        value = s->dr;
> +        if (mpc_i2c_is_master(s)) { /* master mode */
> +            if (mpc_i2c_direction_is_tx(s)) {
> +                DPRINTF("MTX is set not in recv mode\n");
> +            } else {
> +                mpc_i2c_data_recive(s);
> +            }
> +        }
> +        break;
> +    default:
> +        value = 0;
> +        DPRINTF("ERROR: Bad read addr 0x%x\n", (unsigned int)addr);
> +        break;
> +    }
> +#ifdef DEBUG_I2C
> +    DPRINTF("%s: addr " TARGET_FMT_plx " %02" PRIx32 "\n", __func__,
> +                                         addr, value);

DPRINTF only emits code when DEBUG_I2C is defined, so why check it again?

> +#endif
> +    return (uint64_t)value;
> +}
> +
> +static void mpc_i2c_write(void *opaque, hwaddr addr,
> +                            uint64_t value, unsigned size)
> +{
> +    MPCI2CState *s = opaque;
> +
> +#ifdef DEBUG_I2C
> +    DPRINTF("%s: addr " TARGET_FMT_plx " val %08" PRIx64 "\n", __func__,
> +                                             addr, value);

Same here.

> +#endif
> +    switch (addr) {
> +    case MPC_I2C_ADR:
> +        s->adr = value & CADR_MASK;
> +        break;
> +    case MPC_I2C_FDR:
> +        s->fdr = value & CFDR_MASK;
> +        break;
> +    case MPC_I2C_CR:
> +        if (mpc_i2c_is_enabled(s) && ((value & CCR_MEN) == 0)) {
> +            mpc_i2c_soft_reset(s);
> +            break;
> +        }
> +        /* normal write */
> +        s->cr = value & CCR_MASK;
> +        if (mpc_i2c_is_master(s)) { /* master mode */
> +            /* set the bus to busy after master is set as per RM*/
> +            s->sr |= CSR_MBB;
> +        } else {
> +            /* bus is not busy anymore */
> +            s->sr &= ~CSR_MBB;
> +            /* Reset the address for fresh write/read cycle*/
> +        if (s->address != CYCLE_RESET) {
> +            i2c_end_transfer(s->bus);
> +            s->address = CYCLE_RESET;
> +            }
> +        }
> +        /* For restart end the ongoing transfer */
> +        if (s->cr & CCR_RSTA) {
> +            if (s->address != CYCLE_RESET) {
> +                s->address = CYCLE_RESET;
> +                i2c_end_transfer(s->bus);
> +                s->cr &= ~CCR_RSTA;
> +            }
> +        }
> +        break;
> +    case MPC_I2C_SR:
> +        s->sr = value & CSR_MASK;
> +        /*
> +         * if the guest writes 0 to MIF/MAL then lower the interrupt
> +        */

Why a multiline comment here? Also please go through all of your
comments and double-check whether you're describing something that's not
obvious from the code flow. A comment saying

  /* If foo do x */
  if (foo) {
    x();
  }

isn't really very useful. However, a comment saying

  /* Transfer aborted */
  if (foo) {
    x();
  }

gives actual semantic value-add. I personally prefer self-documenting
code though, so in the example case I would usually try to write

  if (foo) {
    abort_transfer();
  }

which means you don't need any comment at all.

> +        if (!(s->sr & CSR_MIF) || !(s->sr & CSR_MAL)) {
> +            mpc_i2c_irq(s);
> +        }
> +        break;
> +    case MPC_I2C_DR:
> +        /* if the device is not enabled, nothing to do */
> +        if (!mpc_i2c_is_enabled(s)) {
> +            break;
> +        }
> +        s->dr = value & CDR_MASK;
> +        if (mpc_i2c_is_master(s)) { /* master mode */
> +            if (s->address == CYCLE_RESET) {
> +                mpc_i2c_address_send(s);
> +            } else {
> +                mpc_i2c_data_send(s);
> +            }
> +        }
> +        break;
> +    case MPC_I2C_DFSRR:
> +        s->dfssr = value;
> +        break;
> +    default:
> +        DPRINTF("ERROR: Bad write addr 0x%x\n", (unsigned int)addr);
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps i2c_ops = {
> +    .read =  mpc_i2c_read,
> +    .write =  mpc_i2c_write,
> +    .valid.max_access_size = 1,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const VMStateDescription mpc_i2c_vmstate = {
> +    .name = TYPE_MPC_I2C,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(address, MPCI2CState),
> +        VMSTATE_UINT8(adr, MPCI2CState),
> +        VMSTATE_UINT8(fdr, MPCI2CState),
> +        VMSTATE_UINT8(cr, MPCI2CState),
> +        VMSTATE_UINT8(sr, MPCI2CState),
> +        VMSTATE_UINT8(dr, MPCI2CState),
> +        VMSTATE_UINT8(dfssr, MPCI2CState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void mpc_i2c_realize(DeviceState *dev, Error **errp)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    MPCI2CState  *i2c = MPC_I2C(dev);
> +
> +    sysbus_init_irq(sbd, &i2c->irq);
> +    memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c,
> +                            "mpc-i2c", 0x14);

I usually try to align the indentation with the context, so in this case
"mpc-i2c" would be aligned with &i2c-iomem. It makes it easier to grasp
what the semantic enclosure for your context is.

> +    sysbus_init_mmio(sbd, &i2c->iomem);
> +    i2c->bus = i2c_init_bus(DEVICE(dev), "i2c");
> +}
> +
> +static void mpc_i2c_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd  = &mpc_i2c_vmstate ;
> +    dc->reset = mpc_i2c_reset;
> +    dc->realize = mpc_i2c_realize;
> +}
> +
> +static const TypeInfo mpc_i2c_type_info = {
> +    .name          = TYPE_MPC_I2C,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(MPCI2CState),
> +    .class_init    = mpc_i2c_class_init,
> +};
> +
> +static void mpc_i2c_register_types(void)
> +{
> +    type_register_static(&mpc_i2c_type_info);
> +}
> +
> +type_init(mpc_i2c_register_types)
> +
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 2832fc0..bbe70b9 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -39,6 +39,7 @@
>  #include "qemu/error-report.h"
>  #include "hw/platform-bus.h"
>  #include "hw/net/fsl_etsec/etsec.h"
> +#include "hw/i2c/i2c.h"
>  
>  #define EPAPR_MAGIC                (0x45504150)
>  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
> @@ -55,6 +56,9 @@
>  #define MPC8544_CCSRBAR_SIZE       0x00100000ULL
>  #define MPC8544_MPIC_REGS_OFFSET   0x40000ULL
>  #define MPC8544_MSI_REGS_OFFSET   0x41600ULL
> +#define MPC8544_I2C_REGS_OFFSET   0x3000ULL
> +#define MPC8544_I2C_REGS_BASE     (MPC8544_CCSRBAR_BASE + \
> +                                    MPC8544_I2C_REGS_OFFSET)

I don't think you need the base, do you?

>  #define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL
>  #define MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL
>  #define MPC8544_PCI_REGS_OFFSET    0x8000ULL
> @@ -65,7 +69,9 @@
>  #define MPC8544_UTIL_OFFSET        0xe0000ULL
>  #define MPC8544_SPIN_BASE          0xEF000000ULL
>  #define MPC8XXX_GPIO_OFFSET        0x000FF000ULL
>  #define MPC8XXX_GPIO_IRQ           47
> +#define MPC8544_I2C_IRQ            43
> +#define RTC_REGS_OFFSET            0x68
>  
>  struct boot_info
>  {
> @@ -127,6 +133,38 @@ static void dt_serial_create(void *fdt, unsigned long 
> long offset,
>      }
>  }
>  
> +static void dt_rtc_create(void *fdt, const char *i2c, const char *alias)
> +{
> +    int offset = RTC_REGS_OFFSET;
> +
> +    gchar *rtc = g_strdup_printf("%s/address@hidden"PRIx32, i2c, offset);
> +    qemu_fdt_add_subnode(fdt, rtc);
> +    qemu_fdt_setprop_string(fdt, rtc, "compatible", "pericom,pt7c4338");
> +    qemu_fdt_setprop_cells(fdt, rtc, "reg", offset);
> +    qemu_fdt_setprop_string(fdt, "/aliases", alias, rtc);
> +
> +    g_free(rtc);
> +}
> +
> +static void dt_i2c_create(void *fdt, const char *soc, const char *mpic,
> +                             const char *alias)
> +{
> +    hwaddr mmio0 = MPC8544_I2C_REGS_OFFSET;
> +    int irq0 = MPC8544_I2C_IRQ;
> +
> +    gchar *i2c = g_strdup_printf("%s/address@hidden"PRIx64, soc, mmio0);
> +    qemu_fdt_add_subnode(fdt, i2c);
> +    qemu_fdt_setprop_string(fdt, i2c, "device_type", "i2c");
> +    qemu_fdt_setprop_string(fdt, i2c, "compatible", "fsl-i2c");
> +    qemu_fdt_setprop_cells(fdt, i2c, "reg", mmio0, 0x100);

Why is the region 0x100 long here, but only 0x14 in the memory region
registration?

> +    qemu_fdt_setprop_cells(fdt, i2c, "cell-index", 0);
> +    qemu_fdt_setprop_cells(fdt, i2c, "interrupts", irq0, 0x2);
> +    qemu_fdt_setprop_phandle(fdt, i2c, "interrupt-parent", mpic);
> +    qemu_fdt_setprop_string(fdt, "/aliases", alias, i2c);
> +
> +    g_free(i2c);
> +}
> +
>  static void create_dt_mpc8xxx_gpio(void *fdt, const char *soc, const char 
> *mpic)
>  {
>      hwaddr mmio0 = MPC8XXX_GPIO_OFFSET;
> @@ -467,6 +505,10 @@ static int ppce500_load_device_tree(MachineState 
> *machine,
>                           soc, mpic, "serial0", 0, true);
>      }
>  
> +    dt_i2c_create(fdt, soc, mpic, "i2c");
> +
> +    dt_rtc_create(fdt, "i2c", "rtc");
> +
>      snprintf(gutil, sizeof(gutil), "%s/address@hidden", soc,
>               MPC8544_UTIL_OFFSET);
>      qemu_fdt_add_subnode(fdt, gutil);
> @@ -811,6 +853,7 @@ void ppce500_init(MachineState *machine, PPCE500Params 
> *params)
>      MemoryRegion *ccsr_addr_space;
>      SysBusDevice *s;
>      PPCE500CCSRState *ccsr;
> +    I2CBus *i2c;
>  
>      /* Setup CPUs */
>      if (machine->cpu_model == NULL) {
> @@ -893,6 +936,11 @@ void ppce500_init(MachineState *machine, PPCE500Params 
> *params)
>                         serial_hds[1], DEVICE_BIG_ENDIAN);
>      }
>  
> +    dev = sysbus_create_simple("mpc-i2c", MPC8544_I2C_REGS_BASE,
> +                                mpic[MPC8544_I2C_IRQ]);

The i2c device is part of CCSR, no? So it should live in ccsr_addr_space
rather than have a direct map into the system address space.


Alex

> +    i2c = (I2CBus *)qdev_get_child_bus(dev, "i2c");
> +    i2c_create_slave(i2c, "ds1338", RTC_REGS_OFFSET);
> +
>      /* General Utility device */
>      dev = qdev_create(NULL, "mpc8544-guts");
>      qdev_init_nofail(dev);
>