From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y1igv-0000pL-TT for qemu-devel@nongnu.org; Thu, 18 Dec 2014 16:33:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y1igq-0003W2-BL for qemu-devel@nongnu.org; Thu, 18 Dec 2014 16:33:01 -0500 From: Amit Tomar Date: Thu, 18 Dec 2014 21:32:20 +0000 Message-ID: <1418938339720.40668@freescale.com> References: In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v1] [Review Request] RTC Support in e500 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "agraf@suse.de" Cc: "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" =0A= Thank you for reviewing the patches and providing the comments.=0A= =0A= I'm able to follow most of the comments except below two.=0A= =0A= >Is this true for a real MPC8544DS as well? If not, we'll need to condition= alize it to only spawn >the i2c controller (and rtc clock) on the virt mach= ine.=0A= =0A= I could not able to parse the above comments,what I need to check here?=0A= =0A= I checked the MPC8544 dts file ,I2C is present at @3100(will double check i= t)=0A= =0A= > + case MPC_I2C_CR:=0A= > + if (mpc_i2c_is_enabled(s) && ((value & CCR_MEN) =3D=3D 0)) {=0A= > + mpc_i2c_soft_reset(s);=0A= =0A= > I think it's fine to do a break here and indent the below 4 bytes less.= =0A= =0A= Do you want to have separate function for else, if so what should I name it= ?=0A= =0A= Hope you would strengthen me on these points.=0A= =0A= Thanks,=0A= Amit.=0A= =0A= =0A= -----Original Message-----=0A= From: Alexander Graf [mailto:agraf@suse.de]=0A= Sent: Tuesday, December 16, 2014 6:03 PM=0A= To: Tomar Amit-B51888; qemu-devel@nongnu.org; qemu-ppc@nongnu.org=0A= Subject: Re: [PATCH v1] [Review Request] RTC Support in e500=0A= =0A= =0A= =0A= On 08.12.14 15:19, Amit Tomar wrote:=0A= > I am new to QEMU and tried to provide support for RTC in e500.=0A= >=0A= > Please review the following set of patches that would add RTC support in = e500 and guide me.=0A= >=0A= > Tested it on both x86 and ppc host machines.=0A= >=0A= >=0A= > Signed-off-by: Amit Singh Tomar =0A= > ---=0A= > default-configs/ppc-softmmu.mak | 2 +=0A= > default-configs/ppc64-softmmu.mak | 2 +=0A= > hw/i2c/Makefile.objs | 1 +=0A= > hw/i2c/mpc_i2c.c | 361 +++++++++++++++++++++++++++++++= ++++++=0A= > hw/ppc/e500.c | 50 ++++-=0A= > 5 files changed, 415 insertions(+), 1 deletion(-) create mode 100644=0A= > hw/i2c/mpc_i2c.c=0A= >=0A= > diff --git a/default-configs/ppc-softmmu.mak=0A= > b/default-configs/ppc-softmmu.mak index d725b23..6fdd39a 100644=0A= > --- a/default-configs/ppc-softmmu.mak=0A= > +++ b/default-configs/ppc-softmmu.mak=0A= > @@ -9,6 +9,8 @@ CONFIG_M48T59=3Dy=0A= > CONFIG_VGA=3Dy=0A= > CONFIG_VGA_PCI=3Dy=0A= > CONFIG_SERIAL=3Dy=0A= > +CONFIG_MPC_I2C=3Dy=0A= > +CONFIG_DS1338=3Dy=0A= > CONFIG_PARALLEL=3Dy=0A= > CONFIG_I8254=3Dy=0A= > CONFIG_PCKBD=3Dy=0A= > diff --git a/default-configs/ppc64-softmmu.mak=0A= > b/default-configs/ppc64-softmmu.mak=0A= > index bd30d69..3ad3f58 100644=0A= > --- a/default-configs/ppc64-softmmu.mak=0A= > +++ b/default-configs/ppc64-softmmu.mak=0A= > @@ -9,6 +9,8 @@ CONFIG_M48T59=3Dy=0A= > CONFIG_VGA=3Dy=0A= > CONFIG_VGA_PCI=3Dy=0A= > CONFIG_SERIAL=3Dy=0A= > +CONFIG_MPC_I2C=3Dy=0A= > +CONFIG_DS1338=3Dy=0A= > CONFIG_PARALLEL=3Dy=0A= > CONFIG_I8254=3Dy=0A= > CONFIG_PCKBD=3Dy=0A= > diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs index=0A= > 648278e..6e6d00d 100644=0A= > --- a/hw/i2c/Makefile.objs=0A= > +++ b/hw/i2c/Makefile.objs=0A= > @@ -4,4 +4,5 @@ common-obj-$(CONFIG_ACPI) +=3D smbus_ich9.o=0A= > common-obj-$(CONFIG_APM) +=3D pm_smbus.o=0A= > common-obj-$(CONFIG_BITBANG_I2C) +=3D bitbang_i2c.o=0A= > common-obj-$(CONFIG_EXYNOS4) +=3D exynos4210_i2c.o=0A= > +common-obj-$(CONFIG_MPC_I2C) +=3D mpc_i2c.o=0A= > obj-$(CONFIG_OMAP) +=3D omap_i2c.o=0A= > diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c new file mode 100644=0A= > index 0000000..c739d07=0A= > --- /dev/null=0A= > +++ b/hw/i2c/mpc_i2c.c=0A= > @@ -0,0 +1,361 @@=0A= > +/*=0A= > + * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserved.= =0A= > + *=0A= > + * Author: Amit Tomar, =0A= > + *=0A= > + * Description:=0A= > + * This file is derived from IMX I2C controller,=0A= > + * by Jean-Christophe DUBOIS .=0A= > + *=0A= > + * Thanks to Scott Wood and Alexander Graf for their kind help on this.= =0A= > + *=0A= > + * This program is free software; you can redistribute it and/or=0A= > +modify=0A= > + * it under the terms of the GNU General Public License, version 2,=0A= > +as=0A= > + * published by the Free Software Foundation.=0A= =0A= Would you be incredibly opposed to making this GPLv2+? We're trying to limi= t the amount of code that's GPLv2 only.=0A= =0A= Of course this only works if you didn't copy code (not defines) from Linux = :).=0A= =0A= > + */=0A= > +=0A= > +#include "hw/sysbus.h"=0A= > +#include "hw/i2c/i2c.h"=0A= > +#include "qemu/bitops.h"=0A= > +#include "hw/ptimer.h"=0A= > +=0A= > +/*#define DEBUG_I2C*/=0A= > +=0A= > +#ifdef DEBUG_I2C=0A= > +#define DPRINTF(fmt, ...) \=0A= > + do { fprintf(stderr, "mpc_i2c[%s]: " fmt, __func__, ## __VA_ARGS__);= \=0A= > + } while (0)=0A= > +#else=0A= > +#define DPRINTF(fmt, ...) do {} while (0) #endif=0A= > +=0A= > +#define TYPE_MPC_I2C "mpc-i2c"=0A= > +#define MPC_I2C(obj) \=0A= > + OBJECT_CHECK(MPCI2CState, (obj), TYPE_MPC_I2C)=0A= > +=0A= > +#define MPC_I2C_ADR 0x00=0A= > +#define MPC_I2C_FDR 0x04=0A= > +#define MPC_I2C_CR 0x08=0A= > +#define MPC_I2C_SR 0x0c=0A= > +#define MPC_I2C_DR 0x10=0A= > +#define MPC_I2C_DFSRR 0x14=0A= > +=0A= > +#define CCR_MEN (1<<7)=0A= > +#define CCR_MIEN (1<<6)=0A= > +#define CCR_MSTA (1<<5)=0A= > +#define CCR_MTX (1<<4)=0A= > +#define CCR_TXAK (1<<3)=0A= > +#define CCR_RSTA (1<<2)=0A= > +#define CCR_BCST (1<<0)=0A= > +=0A= > +#define CSR_MCF (1<<7)=0A= > +#define CSR_MAAS (1<<6)=0A= > +#define CSR_MBB (1<<5)=0A= > +#define CSR_MAL (1<<4)=0A= > +#define CSR_SRW (1<<2)=0A= > +#define CSR_MIF (1<<1)=0A= > +#define CSR_RXAK (1<<0)=0A= > +=0A= > +#define CADR_MASK 0xFE=0A= > +#define CFDR_MASK 0x3F=0A= > +#define CCR_MASK 0xFC=0A= > +#define CSR_MASK 0xED=0A= > +#define CDR_MASK 0xFF=0A= > +=0A= > +#define CYCLE_RESET 0xFF=0A= > +=0A= > +typedef struct MPCI2CState {=0A= > + SysBusDevice parent_obj;=0A= > +=0A= > + I2CBus *bus;=0A= > + qemu_irq irq;=0A= > + MemoryRegion iomem;=0A= > +=0A= > + uint8_t address;=0A= > + uint8_t adr;=0A= > + uint8_t fdr;=0A= > + uint8_t cr;=0A= > + uint8_t sr;=0A= > + uint8_t dr;=0A= > + uint8_t dfssr;=0A= > +} MPCI2CState;=0A= > +=0A= > +static bool mpc_i2c_is_enabled(MPCI2CState *s) {=0A= > + return s->cr & CCR_MEN;=0A= > +}=0A= > +=0A= > +static bool mpc_i2c_is_master(MPCI2CState *s) {=0A= > + return s->cr & CCR_MSTA;=0A= > +}=0A= > +=0A= > +static bool mpc_i2c_direction_is_tx(MPCI2CState *s) {=0A= > + return s->cr & CCR_MTX;=0A= > +}=0A= > +=0A= > +static bool mpc_i2c_irq_pending(MPCI2CState *s) {=0A= > + return s->sr & CSR_MIF;=0A= > +}=0A= > +=0A= > +static bool mpc_i2c_irq_is_enabled(MPCI2CState *s) {=0A= > + return s->cr & CCR_MIEN;=0A= > +}=0A= > +=0A= > +static void mpc_i2c_reset(DeviceState *dev) {=0A= > + MPCI2CState *i2c =3D MPC_I2C(dev);=0A= > +=0A= > + i2c->address =3D 0xFF;=0A= > + i2c->adr =3D 0x00;=0A= > + i2c->fdr =3D 0x00;=0A= > + i2c->cr =3D 0x00;=0A= > + i2c->sr =3D 0x81;=0A= > + i2c->dr =3D 0x00;=0A= > +}=0A= > +=0A= > +static void mpc_i2c_irq(MPCI2CState *s) {=0A= > + bool irq_active =3D false;=0A= > +=0A= > + if (mpc_i2c_is_enabled(s) && mpc_i2c_irq_is_enabled(s)=0A= > + && mpc_i2c_irq_pending(s)) {=0A= > + irq_active =3D true;=0A= > + }=0A= > +=0A= > + if (irq_active) {=0A= > + qemu_irq_raise(s->irq);=0A= > + } else {=0A= > + qemu_irq_lower(s->irq);=0A= > + }=0A= > +}=0A= > +=0A= > +static void mpc_i2c_soft_reset(MPCI2CState *s) {=0A= > + /* This is a soft reset. ADR is preserved during soft resets */=0A= > + uint8_t adr =3D s->adr;=0A= > + mpc_i2c_reset(DEVICE(s));=0A= > + s->adr =3D adr;=0A= > +}=0A= > +=0A= > +static void mpc_i2c_address_send(MPCI2CState *s) {=0A= > + /* if returns non zero slave address is not right=0A= > + and fetches 7 bit of address and 1 bit of Direction bit */=0A= =0A= I don't understand this comment :).=0A= =0A= > + if (i2c_start_transfer(s->bus, s->dr >> 1 , s->dr & (0x01))) {=0A= =0A= Please don't put spaces before commas.=0A= =0A= > + s->sr |=3D CSR_RXAK;=0A= > + } else {=0A= > + s->address =3D s->dr;=0A= > + s->sr &=3D ~CSR_RXAK;=0A= > + s->sr |=3D CSR_MCF; /*Byte Tranfer is completed*/=0A= > + s->sr |=3D CSR_MIF; /*Needs to be set after Byte Transfer is co= mpleted*/=0A= > + mpc_i2c_irq(s);=0A= > + }=0A= > +}=0A= > +=0A= > +static void mpc_i2c_data_send(MPCI2CState *s) {=0A= > + if (i2c_send(s->bus, s->dr)) {=0A= > + /* if the target return non zero then end the transfer */=0A= > + s->sr |=3D CSR_RXAK;=0A= > + i2c_end_transfer(s->bus);=0A= > + } else {=0A= > + s->sr &=3D ~CSR_RXAK;=0A= > + s->sr |=3D CSR_MCF; /* Byte Transfer is completed*/=0A= > + s->sr |=3D CSR_MIF; /* Needs to be set after Byte Transfer is c= ompleted*/=0A= > + mpc_i2c_irq(s);=0A= > + }=0A= > +}=0A= > +=0A= > +static void mpc_i2c_data_recive(MPCI2CState *s) {=0A= > + int ret;=0A= > + /* get the next byte */=0A= > + ret =3D i2c_recv(s->bus);=0A= > + if (ret >=3D 0) {=0A= > + s->sr |=3D CSR_MCF;/* Byte Transfer is completed*/=0A= > + s->sr |=3D CSR_MIF;/* Needs to be set after Byte Transfer is com= pleted*/=0A= > + mpc_i2c_irq(s);=0A= > + } else {=0A= > + DPRINTF("read failed for device");=0A= > + ret =3D 0xff;=0A= > + }=0A= > + s->dr =3D ret;=0A= > +}=0A= > +=0A= > +static uint64_t mpc_i2c_read(void *opaque, hwaddr addr, unsigned=0A= > +size) {=0A= > + MPCI2CState *s =3D opaque;=0A= > + uint8_t value;=0A= > +=0A= > + switch (addr) {=0A= > + case MPC_I2C_ADR:=0A= > + value =3D s->adr;=0A= > + break;=0A= > + case MPC_I2C_FDR:=0A= > + value =3D s->fdr;=0A= > + break;=0A= > + case MPC_I2C_CR:=0A= > + value =3D s->cr;=0A= > + break;=0A= > + case MPC_I2C_SR:=0A= > + value =3D s->sr;=0A= > + break;=0A= > + case MPC_I2C_DR:=0A= > + value =3D s->dr;=0A= > + if (mpc_i2c_is_master(s)) { /* master mode */=0A= > + if (mpc_i2c_direction_is_tx(s)) {=0A= > + DPRINTF("MTX is set not in recv mode\n");=0A= > + } else {=0A= > + mpc_i2c_data_recive(s);=0A= > + }=0A= > + }=0A= > + break;=0A= > + default:=0A= > + value =3D 0;=0A= > + DPRINTF("ERROR: Bad read addr 0x%x\n", (unsigned int)addr);=0A= > + break;=0A= > + }=0A= > +#ifdef DEBUG_I2C=0A= > + printf("%s: addr " TARGET_FMT_plx " %02" PRIx32 "\n", __func__,=0A= > + addr, value); #endif=0A= =0A= Why not use DPRINTF?=0A= =0A= > + return (uint64_t)value;=0A= > +}=0A= > +=0A= > +static void mpc_i2c_write(void *opaque, hwaddr addr,=0A= > + uint64_t value, unsigned size) {=0A= > + MPCI2CState *s =3D opaque;=0A= > +=0A= > +#ifdef DEBUG_I2C=0A= > + printf("%s: addr " TARGET_FMT_plx " val %08" PRIx64 "\n", __func__,= =0A= > + addr, value); #endif=0A= =0A= DPRINTF again?=0A= =0A= > + switch (addr) {=0A= > + case MPC_I2C_ADR:=0A= > + s->adr =3D value & CADR_MASK;=0A= > + break;=0A= > + case MPC_I2C_FDR:=0A= > + s->fdr =3D value & CFDR_MASK;=0A= > + break;=0A= > + case MPC_I2C_CR:=0A= > + if (mpc_i2c_is_enabled(s) && ((value & CCR_MEN) =3D=3D 0)) {=0A= > + mpc_i2c_soft_reset(s);=0A= =0A= I think it's fine to do a break here and indent the below 4 bytes less.=0A= =0A= > + } else { /* normal write */=0A= > + s->cr =3D value & CCR_MASK;=0A= > + if (mpc_i2c_is_master(s)) { /* master mode */=0A= > + /* set the bus to busy after master is set as per RM*/= =0A= > + s->sr |=3D CSR_MBB;=0A= > + } else {=0A= > + /* bus is not busy anymore */=0A= > + s->sr &=3D ~CSR_MBB;=0A= > + /* Reset the address for fresh write/read cycle*/=0A= > + if (s->address !=3D CYCLE_RESET) {=0A= > + i2c_end_transfer(s->bus);=0A= > + s->address =3D CYCLE_RESET;=0A= > + }=0A= > + }=0A= > + /* For restart end the onging transfer */=0A= =0A= Indentation wrong?=0A= =0A= > + if (s->cr & CCR_RSTA) {=0A= > + if (s->address !=3D CYCLE_RESET) {=0A= > + s->address =3D CYCLE_RESET;=0A= > + i2c_end_transfer(s->bus);=0A= > + s->cr &=3D ~CCR_RSTA;=0A= > + }=0A= > + }=0A= > + }=0A= > + break;=0A= > + case MPC_I2C_SR:=0A= > + s->sr =3D value & CSR_MASK;=0A= > + /*=0A= > + * if the guest writes 0 to MIF/MAL then lower the interrupt=0A= > + */=0A= > + if (!(s->sr & CSR_MIF) || !(s->sr & CSR_MAL)) {=0A= > + mpc_i2c_irq(s);=0A= > + }=0A= > + break;=0A= > + case MPC_I2C_DR:=0A= > + /* if the device is not enabled, nothing to do */=0A= > + if (!mpc_i2c_is_enabled(s)) {=0A= > + break;=0A= > + }=0A= > + s->dr =3D value & CDR_MASK;=0A= > + if (mpc_i2c_is_master(s)) { /* master mode */=0A= > + if (s->address =3D=3D CYCLE_RESET) {=0A= > + mpc_i2c_address_send(s);=0A= > + } else {=0A= > + mpc_i2c_data_send(s);=0A= > + }=0A= > + }=0A= > + break;=0A= > + case MPC_I2C_DFSRR:=0A= > + s->dfssr =3D value;=0A= > + break;=0A= > + default:=0A= > + DPRINTF("ERROR: Bad write addr 0x%x\n", (unsigned int)addr);=0A= > + break;=0A= > + }=0A= > +}=0A= > +=0A= > +static const MemoryRegionOps i2c_ops =3D {=0A= > + .read =3D mpc_i2c_read,=0A= > + .write =3D mpc_i2c_write,=0A= > + .valid.max_access_size =3D 1,=0A= > + .endianness =3D DEVICE_NATIVE_ENDIAN, };=0A= > +=0A= > +static const VMStateDescription mpc_i2c_vmstate =3D {=0A= > + .name =3D TYPE_MPC_I2C,=0A= > + .version_id =3D 1,=0A= > + .minimum_version_id =3D 1,=0A= =0A= I think you'll need to call mpc_i2c_irq after migration finished to retrigg= er pending interrupts.=0A= =0A= > + .fields =3D (VMStateField[]) {=0A= > + VMSTATE_UINT8(address, MPCI2CState),=0A= > + VMSTATE_UINT8(adr, MPCI2CState),=0A= > + VMSTATE_UINT8(fdr, MPCI2CState),=0A= > + VMSTATE_UINT8(cr, MPCI2CState),=0A= > + VMSTATE_UINT8(sr, MPCI2CState),=0A= > + VMSTATE_UINT8(dr, MPCI2CState),=0A= > + VMSTATE_UINT8(dfssr, MPCI2CState),=0A= > + VMSTATE_END_OF_LIST()=0A= > + }=0A= > +};=0A= > +=0A= > +static void mpc_i2c_realize(DeviceState *dev, Error **errp) {=0A= > + SysBusDevice *sbd =3D SYS_BUS_DEVICE(dev);=0A= > + MPCI2CState *i2c =3D MPC_I2C(dev);=0A= > +=0A= > + sysbus_init_irq(sbd, &i2c->irq);=0A= > + memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c,=0A= > + "mpc-i2c", 0x14);=0A= > + sysbus_init_mmio(sbd, &i2c->iomem);=0A= > + i2c->bus =3D i2c_init_bus(DEVICE(dev), "i2c"); }=0A= > +=0A= > +static void mpc_i2c_class_init(ObjectClass *klass, void *data) {=0A= > + DeviceClass *dc =3D DEVICE_CLASS(klass);=0A= > +=0A= > + dc->vmsd =3D &mpc_i2c_vmstate ;=0A= > + dc->reset =3D mpc_i2c_reset;=0A= > + dc->realize =3D mpc_i2c_realize;=0A= > +}=0A= > +=0A= > +static const TypeInfo mpc_i2c_type_info =3D {=0A= > + .name =3D TYPE_MPC_I2C,=0A= > + .parent =3D TYPE_SYS_BUS_DEVICE,=0A= > + .instance_size =3D sizeof(MPCI2CState),=0A= > + .class_init =3D mpc_i2c_class_init,=0A= > +};=0A= > +=0A= > +static void mpc_i2c_register_types(void) {=0A= > + type_register_static(&mpc_i2c_type_info);=0A= > +}=0A= > +=0A= > +type_init(mpc_i2c_register_types)=0A= > +=0A= > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index 2832fc0..bbe70b9=0A= > 100644=0A= > --- a/hw/ppc/e500.c=0A= > +++ b/hw/ppc/e500.c=0A= > @@ -39,6 +39,7 @@=0A= > #include "qemu/error-report.h"=0A= > #include "hw/platform-bus.h"=0A= > #include "hw/net/fsl_etsec/etsec.h"=0A= > +#include "hw/i2c/i2c.h"=0A= >=0A= > #define EPAPR_MAGIC (0x45504150)=0A= > #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb"=0A= > @@ -55,6 +56,9 @@=0A= > #define MPC8544_CCSRBAR_SIZE 0x00100000ULL=0A= > #define MPC8544_MPIC_REGS_OFFSET 0x40000ULL=0A= > #define MPC8544_MSI_REGS_OFFSET 0x41600ULL=0A= > +#define MPC8544_I2C_REGS_OFFSET 0x3000ULL=0A= > +#define MPC8544_I2C_REGS_BASE (MPC8544_CCSRBAR_BASE + \=0A= > + MPC8544_I2C_REGS_OFFSET)=0A= > #define MPC8544_SERIAL0_REGS_OFFSET 0x4500ULL #define=0A= > MPC8544_SERIAL1_REGS_OFFSET 0x4600ULL=0A= > #define MPC8544_PCI_REGS_OFFSET 0x8000ULL=0A= > @@ -65,7 +69,9 @@=0A= > #define MPC8544_UTIL_OFFSET 0xe0000ULL=0A= > #define MPC8544_SPIN_BASE 0xEF000000ULL=0A= > #define MPC8XXX_GPIO_OFFSET 0x000FF000ULL=0A= > -#define MPC8XXX_GPIO_IRQ 43=0A= > +#define MPC8XXX_GPIO_IRQ 47=0A= =0A= Please move this into a separate patch - it makes bisecting problems a lot = easier. Also imagine we'd have to revert the GPIO IRQ patch for some reason= , without the split we would need to revert the i2c patch as well.=0A= =0A= > +#define MPC8544_I2C_IRQ 43=0A= > +#define RTC_REGS_OFFSET 0x68=0A= >=0A= > struct boot_info=0A= > {=0A= > @@ -127,6 +133,38 @@ static void dt_serial_create(void *fdt, unsigned lon= g long offset,=0A= > }=0A= > }=0A= >=0A= > +static void dt_rtc_create(void *fdt, const char *i2c, const char=0A= > +*alias) {=0A= > + int offset =3D RTC_REGS_OFFSET;=0A= > +=0A= > + gchar *rtc =3D g_strdup_printf("%s/rtc@%"PRIx32, i2c, offset);=0A= > + qemu_fdt_add_subnode(fdt, rtc);=0A= > + qemu_fdt_setprop_string(fdt, rtc, "compatible", "pericom,pt7c4338");= =0A= > + qemu_fdt_setprop_cells(fdt, rtc, "reg", offset);=0A= > + qemu_fdt_setprop_string(fdt, "/aliases", alias, rtc);=0A= > +=0A= > + g_free(rtc);=0A= > +}=0A= > +=0A= > +static void dt_i2c_create(void *fdt, const char *soc, const char *mpic,= =0A= > + const char *alias) {=0A= > + hwaddr mmio0 =3D MPC8544_I2C_REGS_OFFSET;=0A= > + int irq0 =3D MPC8544_I2C_IRQ;=0A= > +=0A= > + gchar *i2c =3D g_strdup_printf("%s/i2c@%"PRIx64, soc, mmio0);=0A= > + qemu_fdt_add_subnode(fdt, i2c);=0A= > + qemu_fdt_setprop_string(fdt, i2c, "device_type", "i2c");=0A= > + qemu_fdt_setprop_string(fdt, i2c, "compatible", "fsl-i2c");=0A= > + qemu_fdt_setprop_cells(fdt, i2c, "reg", mmio0, 0x100);=0A= > + qemu_fdt_setprop_cells(fdt, i2c, "cell-index", 0);=0A= > + qemu_fdt_setprop_cells(fdt, i2c, "interrupts", irq0, 0x2);=0A= > + qemu_fdt_setprop_phandle(fdt, i2c, "interrupt-parent", mpic);=0A= > + qemu_fdt_setprop_string(fdt, "/aliases", alias, i2c);=0A= > +=0A= > + g_free(i2c);=0A= > +}=0A= > +=0A= > static void create_dt_mpc8xxx_gpio(void *fdt, const char *soc, const=0A= > char *mpic) {=0A= > hwaddr mmio0 =3D MPC8XXX_GPIO_OFFSET; @@ -467,6 +505,10 @@ static=0A= > int ppce500_load_device_tree(MachineState *machine,=0A= > soc, mpic, "serial0", 0, true);=0A= > }=0A= >=0A= > + dt_i2c_create(fdt, soc, mpic, "i2c");=0A= > +=0A= > + dt_rtc_create(fdt, "i2c", "rtc");=0A= > +=0A= > snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc,=0A= > MPC8544_UTIL_OFFSET);=0A= > qemu_fdt_add_subnode(fdt, gutil); @@ -811,6 +853,7 @@ void=0A= > ppce500_init(MachineState *machine, PPCE500Params *params)=0A= > MemoryRegion *ccsr_addr_space;=0A= > SysBusDevice *s;=0A= > PPCE500CCSRState *ccsr;=0A= > + I2CBus *i2c;=0A= >=0A= > /* Setup CPUs */=0A= > if (machine->cpu_model =3D=3D NULL) { @@ -893,6 +936,11 @@ void=0A= > ppce500_init(MachineState *machine, PPCE500Params *params)=0A= > serial_hds[1], DEVICE_BIG_ENDIAN);=0A= > }=0A= >=0A= > + dev =3D sysbus_create_simple("mpc-i2c", MPC8544_I2C_REGS_BASE,=0A= > + mpic[MPC8544_I2C_IRQ]);=0A= > + i2c =3D (I2CBus *)qdev_get_child_bus(dev, "i2c");=0A= > + i2c_create_slave(i2c, "ds1338", RTC_REGS_OFFSET);=0A= =0A= Is this true for a real MPC8544DS as well? If not, we'll need to conditiona= lize it to only spawn the i2c controller (and rtc clock) on the virt machin= e.=0A= =0A= =0A= Thanks a lot again for the really nice patch and sorry for the delay in rev= iew!=0A= =0A= Alex=0A=