Re: [Qemu-devel] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 10339 bytes --]

On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

It's not clear to me why this is preferable to having the registers
embedded in the state structure.  The latter is pretty standard
practice for qemu.

> ---
>  hw/i2c/ppc4xx_i2c.c         | 75 +++++++++++++++++++++++++--------------------
>  include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
>  2 files changed, 43 insertions(+), 51 deletions(-)
> 
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index d1936db..a68b5f7 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -3,7 +3,7 @@
>   *
>   * Copyright (c) 2007 Jocelyn Mayer
>   * Copyright (c) 2012 François Revol
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2018 BALATON Zoltan
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -46,9 +46,26 @@
>  
>  #define IIC_XTCNTLSS_SRST   (1 << 0)
>  
> +typedef struct {
> +    uint8_t mdata;
> +    uint8_t lmadr;
> +    uint8_t hmadr;
> +    uint8_t cntl;
> +    uint8_t mdcntl;
> +    uint8_t sts;
> +    uint8_t extsts;
> +    uint8_t lsadr;
> +    uint8_t hsadr;
> +    uint8_t clkdiv;
> +    uint8_t intrmsk;
> +    uint8_t xfrcnt;
> +    uint8_t xtcntlss;
> +    uint8_t directcntl;
> +} PPC4xxI2CRegs;
> +
>  static void ppc4xx_i2c_reset(DeviceState *s)
>  {
> -    PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> +    PPC4xxI2CRegs *i2c = PPC4xx_I2C(s)->regs;
>  
>      /* FIXME: Should also reset bus?
>       *if (s->address != ADDR_RESET) {
> @@ -63,7 +80,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>      i2c->mdcntl = 0;
>      i2c->sts = 0;
>      i2c->extsts = 0x8f;
> -    i2c->sdata = 0;
>      i2c->lsadr = 0;
>      i2c->hsadr = 0;
>      i2c->clkdiv = 0;
> @@ -71,7 +87,6 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>      i2c->xfrcnt = 0;
>      i2c->xtcntlss = 0;
>      i2c->directcntl = 0xf;
> -    i2c->intr = 0;
>  }
>  
>  static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
> @@ -81,13 +96,14 @@ static inline bool ppc4xx_i2c_is_master(PPC4xxI2CState *i2c)
>  
>  static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>  {
> -    PPC4xxI2CState *i2c = PPC4xx_I2C(opaque);
> +    PPC4xxI2CState *s = PPC4xx_I2C(opaque);
> +    PPC4xxI2CRegs *i2c = s->regs;
>      uint64_t ret;
>  
>      switch (addr) {
>      case 0:
>          ret = i2c->mdata;
> -        if (ppc4xx_i2c_is_master(i2c)) {
> +        if (ppc4xx_i2c_is_master(s)) {
>              ret = 0xff;
>  
>              if (!(i2c->sts & IIC_STS_MDBS)) {
> @@ -98,7 +114,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>                  int pending = (i2c->cntl >> 4) & 3;
>  
>                  /* get the next byte */
> -                int byte = i2c_recv(i2c->bus);
> +                int byte = i2c_recv(s->bus);
>  
>                  if (byte < 0) {
>                      qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed "
> @@ -113,13 +129,13 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>  
>                  if (!pending) {
>                      i2c->sts &= ~IIC_STS_MDBS;
> -                    /*i2c_end_transfer(i2c->bus);*/
> +                    /*i2c_end_transfer(s->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)) {
> +                    i2c_end_transfer(s->bus);
> +                    if (i2c_start_transfer(s->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;
> @@ -139,9 +155,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>                            TYPE_PPC4xx_I2C, __func__);
>          }
>          break;
> -    case 2:
> -        ret = i2c->sdata;
> -        break;
>      case 4:
>          ret = i2c->lmadr;
>          break;
> @@ -181,9 +194,6 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>      case 16:
>          ret = i2c->directcntl;
>          break;
> -    case 17:
> -        ret = i2c->intr;
> -        break;
>      default:
>          if (addr < PPC4xx_I2C_MEM_SIZE) {
>              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> @@ -201,14 +211,15 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>  static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>                                unsigned int size)
>  {
> -    PPC4xxI2CState *i2c = opaque;
> +    PPC4xxI2CState *s = PPC4xx_I2C(opaque);
> +    PPC4xxI2CRegs *i2c = s->regs;
>  
>      switch (addr) {
>      case 0:
>          i2c->mdata = value;
> -        if (!i2c_bus_busy(i2c->bus)) {
> +        if (!i2c_bus_busy(s->bus)) {
>              /* assume we start a write transfer */
> -            if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 0)) {
> +            if (i2c_start_transfer(s->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;
> @@ -219,23 +230,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>                  i2c->extsts = 0;
>              }
>          }
> -        if (i2c_bus_busy(i2c->bus)) {
> -            if (i2c_send(i2c->bus, i2c->mdata)) {
> +        if (i2c_bus_busy(s->bus)) {
> +            if (i2c_send(s->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_end_transfer(s->bus);
>              }
>          }
>          break;
> -    case 2:
> -        i2c->sdata = value;
> -        break;
>      case 4:
>          i2c->lmadr = value;
> -        if (i2c_bus_busy(i2c->bus)) {
> -            i2c_end_transfer(i2c->bus);
> +        if (i2c_bus_busy(s->bus)) {
> +            i2c_end_transfer(s->bus);
>          }
>          break;
>      case 5:
> @@ -245,12 +253,12 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>          i2c->cntl = value;
>          if (i2c->cntl & IIC_CNTL_PT) {
>              if (i2c->cntl & IIC_CNTL_READ) {
> -                if (i2c_bus_busy(i2c->bus)) {
> +                if (i2c_bus_busy(s->bus)) {
>                      /* end previous transfer */
>                      i2c->sts &= ~IIC_STS_PT;
> -                    i2c_end_transfer(i2c->bus);
> +                    i2c_end_transfer(s->bus);
>                  }
> -                if (i2c_start_transfer(i2c->bus, i2c->lmadr >> 1, 1)) {
> +                if (i2c_start_transfer(s->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;
> @@ -294,7 +302,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>      case 15:
>          if (value & IIC_XTCNTLSS_SRST) {
>              /* Is it actually a full reset? U-Boot sets some regs before */
> -            ppc4xx_i2c_reset(DEVICE(i2c));
> +            ppc4xx_i2c_reset(DEVICE(s));
>              break;
>          }
>          i2c->xtcntlss = value;
> @@ -302,9 +310,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>      case 16:
>          i2c->directcntl = value & 0x7;
>          break;
> -    case 17:
> -        i2c->intr = value;
> -        break;
>      default:
>          if (addr < PPC4xx_I2C_MEM_SIZE) {
>              qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> @@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
>  static void ppc4xx_i2c_init(Object *o)
>  {
>      PPC4xxI2CState *s = PPC4xx_I2C(o);
> +    PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));
>  
> +    s->regs = r;
>      memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
>                            TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index 3c60307..1d5ba0c 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -3,7 +3,7 @@
>   *
>   * Copyright (c) 2007 Jocelyn Mayer
>   * Copyright (c) 2012 François Revol
> - * Copyright (c) 2016 BALATON Zoltan
> + * Copyright (c) 2016-2018 BALATON Zoltan
>   *
>   * Permission is hereby granted, free of charge, to any person obtaining a copy
>   * of this software and associated documentation files (the "Software"), to deal
> @@ -37,27 +37,12 @@
>  typedef struct PPC4xxI2CState {
>      /*< private >*/
>      SysBusDevice parent_obj;
> +    void *regs;
>  
>      /*< public >*/
>      I2CBus *bus;
>      qemu_irq irq;
>      MemoryRegion iomem;
> -    uint8_t mdata;
> -    uint8_t lmadr;
> -    uint8_t hmadr;
> -    uint8_t cntl;
> -    uint8_t mdcntl;
> -    uint8_t sts;
> -    uint8_t extsts;
> -    uint8_t sdata;
> -    uint8_t lsadr;
> -    uint8_t hsadr;
> -    uint8_t clkdiv;
> -    uint8_t intrmsk;
> -    uint8_t xfrcnt;
> -    uint8_t xtcntlss;
> -    uint8_t directcntl;
> -    uint8_t intr;
>  } PPC4xxI2CState;
>  
>  #endif /* PPC4XX_I2C_H */

-- 
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 --]