From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42416) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0rJk-0002QM-BY for qemu-devel@nongnu.org; Tue, 16 Dec 2014 07:33:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0rJe-0000yR-7u for qemu-devel@nongnu.org; Tue, 16 Dec 2014 07:33:32 -0500 Message-ID: <54902693.1000701@suse.de> Date: Tue, 16 Dec 2014 13:33:23 +0100 From: Alexander Graf MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: Amit Tomar , "qemu-devel@nongnu.org" , "qemu-ppc@nongnu.org" On 08.12.14 15:19, Amit Tomar wrote: > I am new to QEMU and tried to provide support for RTC in e500. >=20 > Please review the following set of patches that would add RTC support i= n e500 and guide me. >=20 > Tested it on both x86 and ppc host machines. =20 >=20 >=20 > Signed-off-by: Amit Singh Tomar > --- > default-configs/ppc-softmmu.mak | 2 + > default-configs/ppc64-softmmu.mak | 2 + > hw/i2c/Makefile.objs | 1 + > hw/i2c/mpc_i2c.c | 361 +++++++++++++++++++++++++++++= ++++++++ > hw/ppc/e500.c | 50 ++++- > 5 files changed, 415 insertions(+), 1 deletion(-) > create mode 100644 hw/i2c/mpc_i2c.c >=20 > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-soft= mmu.mak > index d725b23..6fdd39a 100644 > --- a/default-configs/ppc-softmmu.mak > +++ b/default-configs/ppc-softmmu.mak > @@ -9,6 +9,8 @@ CONFIG_M48T59=3Dy > CONFIG_VGA=3Dy > CONFIG_VGA_PCI=3Dy > CONFIG_SERIAL=3Dy > +CONFIG_MPC_I2C=3Dy > +CONFIG_DS1338=3Dy > CONFIG_PARALLEL=3Dy > CONFIG_I8254=3Dy > CONFIG_PCKBD=3Dy > 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=3Dy > CONFIG_VGA=3Dy > CONFIG_VGA_PCI=3Dy > CONFIG_SERIAL=3Dy > +CONFIG_MPC_I2C=3Dy > +CONFIG_DS1338=3Dy > CONFIG_PARALLEL=3Dy > CONFIG_I8254=3Dy > CONFIG_PCKBD=3Dy > 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) +=3D smbus_ich9.o > common-obj-$(CONFIG_APM) +=3D pm_smbus.o > common-obj-$(CONFIG_BITBANG_I2C) +=3D bitbang_i2c.o > common-obj-$(CONFIG_EXYNOS4) +=3D exynos4210_i2c.o > +common-obj-$(CONFIG_MPC_I2C) +=3D mpc_i2c.o > obj-$(CONFIG_OMAP) +=3D omap_i2c.o > diff --git a/hw/i2c/mpc_i2c.c b/hw/i2c/mpc_i2c.c > new file mode 100644 > index 0000000..c739d07 > --- /dev/null > +++ b/hw/i2c/mpc_i2c.c > @@ -0,0 +1,361 @@ > +/* > + * Copyright (C) 2014 Freescale Semiconductor, Inc. All rights reserve= d. > + * > + * Author: Amit Tomar, > + * > + * 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 modif= y > + * it under the terms of the GNU General Public License, version 2, as > + * published by the Free Software Foundation. Would you be incredibly opposed to making this GPLv2+? We're trying to limit the amount of code that's GPLv2 only. Of course this only works if you didn't copy code (not defines) from Linux :). > + */ > + > +#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 =3D MPC_I2C(dev); > + > + i2c->address =3D 0xFF; > + i2c->adr =3D 0x00; > + i2c->fdr =3D 0x00; > + i2c->cr =3D 0x00; > + i2c->sr =3D 0x81; > + i2c->dr =3D 0x00; > +} > + > +static void mpc_i2c_irq(MPCI2CState *s) > +{ > + bool irq_active =3D false; > + > + if (mpc_i2c_is_enabled(s) && mpc_i2c_irq_is_enabled(s) > + && mpc_i2c_irq_pending(s)) { > + irq_active =3D 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 =3D s->adr; > + mpc_i2c_reset(DEVICE(s)); > + s->adr =3D adr; > +} > + > +static void mpc_i2c_address_send(MPCI2CState *s) > +{ > + /* if returns non zero slave address is not right > + and fetches 7 bit of address and 1 bit of Direction bit */ I don't understand this comment :). > + if (i2c_start_transfer(s->bus, s->dr >> 1 , s->dr & (0x01))) { Please don't put spaces before commas. > + s->sr |=3D CSR_RXAK; > + } else { > + s->address =3D s->dr; > + s->sr &=3D ~CSR_RXAK; > + s->sr |=3D CSR_MCF; /*Byte Tranfer is completed*/ > + s->sr |=3D CSR_MIF; /*Needs to be set after Byte Transfer is = completed*/ > + 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 */ > + s->sr |=3D CSR_RXAK; > + i2c_end_transfer(s->bus); > + } else { > + s->sr &=3D ~CSR_RXAK; > + s->sr |=3D CSR_MCF; /* Byte Transfer is completed*/ > + s->sr |=3D 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 =3D i2c_recv(s->bus); > + if (ret >=3D 0) { > + s->sr |=3D CSR_MCF;/* Byte Transfer is completed*/ > + s->sr |=3D CSR_MIF;/* Needs to be set after Byte Transfer is c= ompleted*/ > + mpc_i2c_irq(s); > + } else { > + DPRINTF("read failed for device"); > + ret =3D 0xff; > + } > + s->dr =3D ret; > +} > + > +static uint64_t mpc_i2c_read(void *opaque, hwaddr addr, unsigned size) > +{ > + MPCI2CState *s =3D opaque; > + uint8_t value; > + > + switch (addr) { > + case MPC_I2C_ADR: > + value =3D s->adr; > + break; > + case MPC_I2C_FDR: > + value =3D s->fdr; > + break; > + case MPC_I2C_CR: > + value =3D s->cr; > + break; > + case MPC_I2C_SR: > + value =3D s->sr; > + break; > + case MPC_I2C_DR: > + value =3D 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 =3D 0; > + DPRINTF("ERROR: Bad read addr 0x%x\n", (unsigned int)addr); > + break; > + } > +#ifdef DEBUG_I2C > + printf("%s: addr " TARGET_FMT_plx " %02" PRIx32 "\n", __func__, > + addr, value); > +#endif Why not use DPRINTF? > + return (uint64_t)value; > +} > + > +static void mpc_i2c_write(void *opaque, hwaddr addr, > + uint64_t value, unsigned size) > +{ > + MPCI2CState *s =3D opaque; > + > +#ifdef DEBUG_I2C > + printf("%s: addr " TARGET_FMT_plx " val %08" PRIx64 "\n", __func__= , > + addr, value); > +#endif DPRINTF again? > + switch (addr) { > + case MPC_I2C_ADR: > + s->adr =3D value & CADR_MASK; > + break; > + case MPC_I2C_FDR: > + s->fdr =3D value & CFDR_MASK; > + break; > + case MPC_I2C_CR: > + if (mpc_i2c_is_enabled(s) && ((value & CCR_MEN) =3D=3D 0)) { > + mpc_i2c_soft_reset(s); I think it's fine to do a break here and indent the below 4 bytes less. > + } else { /* normal write */ > + s->cr =3D 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 |=3D CSR_MBB; > + } else { > + /* bus is not busy anymore */ > + s->sr &=3D ~CSR_MBB; > + /* Reset the address for fresh write/read cycle*/ > + if (s->address !=3D CYCLE_RESET) { > + i2c_end_transfer(s->bus); > + s->address =3D CYCLE_RESET; > + } > + } > + /* For restart end the onging transfer */ Indentation wrong? > + if (s->cr & CCR_RSTA) { > + if (s->address !=3D CYCLE_RESET) { > + s->address =3D CYCLE_RESET; > + i2c_end_transfer(s->bus); > + s->cr &=3D ~CCR_RSTA; > + } > + } > + } > + break; > + case MPC_I2C_SR: > + s->sr =3D value & CSR_MASK; > + /* > + * if the guest writes 0 to MIF/MAL then lower the interrupt > + */ > + 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 =3D value & CDR_MASK; > + if (mpc_i2c_is_master(s)) { /* master mode */ > + if (s->address =3D=3D CYCLE_RESET) { > + mpc_i2c_address_send(s); > + } else { > + mpc_i2c_data_send(s); > + } > + } > + break; > + case MPC_I2C_DFSRR: > + s->dfssr =3D value; > + break; > + default: > + DPRINTF("ERROR: Bad write addr 0x%x\n", (unsigned int)addr); > + break; > + } > +} > + > +static const MemoryRegionOps i2c_ops =3D { > + .read =3D mpc_i2c_read, > + .write =3D mpc_i2c_write, > + .valid.max_access_size =3D 1, > + .endianness =3D DEVICE_NATIVE_ENDIAN, > +}; > + > +static const VMStateDescription mpc_i2c_vmstate =3D { > + .name =3D TYPE_MPC_I2C, > + .version_id =3D 1, > + .minimum_version_id =3D 1, I think you'll need to call mpc_i2c_irq after migration finished to retrigger pending interrupts. > + .fields =3D (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 =3D SYS_BUS_DEVICE(dev); > + MPCI2CState *i2c =3D MPC_I2C(dev); > + > + sysbus_init_irq(sbd, &i2c->irq); > + memory_region_init_io(&i2c->iomem, OBJECT(i2c), &i2c_ops, i2c, > + "mpc-i2c", 0x14); > + sysbus_init_mmio(sbd, &i2c->iomem); > + i2c->bus =3D i2c_init_bus(DEVICE(dev), "i2c"); > +} > + > +static void mpc_i2c_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + > + dc->vmsd =3D &mpc_i2c_vmstate ; > + dc->reset =3D mpc_i2c_reset; > + dc->realize =3D mpc_i2c_realize; > +} > + > +static const TypeInfo mpc_i2c_type_info =3D { > + .name =3D TYPE_MPC_I2C, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .instance_size =3D sizeof(MPCI2CState), > + .class_init =3D 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" > =20 > #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) > #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 43 > +#define MPC8XXX_GPIO_IRQ 47 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. > +#define MPC8544_I2C_IRQ 43 > +#define RTC_REGS_OFFSET 0x68 > =20 > struct boot_info > { > @@ -127,6 +133,38 @@ static void dt_serial_create(void *fdt, unsigned l= ong long offset, > } > } > =20 > +static void dt_rtc_create(void *fdt, const char *i2c, const char *alia= s) > +{ > + int offset =3D RTC_REGS_OFFSET; > + > + gchar *rtc =3D g_strdup_printf("%s/rtc@%"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 =3D MPC8544_I2C_REGS_OFFSET; > + int irq0 =3D MPC8544_I2C_IRQ; > + > + gchar *i2c =3D g_strdup_printf("%s/i2c@%"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); > + 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 c= har *mpic) > { > hwaddr mmio0 =3D MPC8XXX_GPIO_OFFSET; > @@ -467,6 +505,10 @@ static int ppce500_load_device_tree(MachineState *= machine, > soc, mpic, "serial0", 0, true); > } > =20 > + dt_i2c_create(fdt, soc, mpic, "i2c"); > + > + dt_rtc_create(fdt, "i2c", "rtc"); > + > snprintf(gutil, sizeof(gutil), "%s/global-utilities@%llx", soc, > MPC8544_UTIL_OFFSET); > qemu_fdt_add_subnode(fdt, gutil); > @@ -811,6 +853,7 @@ void ppce500_init(MachineState *machine, PPCE500Par= ams *params) > MemoryRegion *ccsr_addr_space; > SysBusDevice *s; > PPCE500CCSRState *ccsr; > + I2CBus *i2c; > =20 > /* Setup CPUs */ > if (machine->cpu_model =3D=3D NULL) { > @@ -893,6 +936,11 @@ void ppce500_init(MachineState *machine, PPCE500Pa= rams *params) > serial_hds[1], DEVICE_BIG_ENDIAN); > } > =20 > + dev =3D sysbus_create_simple("mpc-i2c", MPC8544_I2C_REGS_BASE, > + mpic[MPC8544_I2C_IRQ]); > + i2c =3D (I2CBus *)qdev_get_child_bus(dev, "i2c"); > + i2c_create_slave(i2c, "ds1338", RTC_REGS_OFFSET); Is this true for a real MPC8544DS as well? If not, we'll need to conditionalize it to only spawn the i2c controller (and rtc clock) on the virt machine. Thanks a lot again for the really nice patch and sorry for the delay in review! Alex