Re: [Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers

[David Gibson]

On Thu, Jun 14, 2018 at 10:18:33AM +0200, BALATON Zoltan wrote:
> On Thu, 14 Jun 2018, David Gibson wrote:
> > On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan <address@hidden>
> > 
> > But.. they are implemented.  Albeit as an entirely software controlled
> > register.
> 
> They were implemented only in that they could be written and read but did
> not implement any of the functionality they have in real hardware so in that
> sense they were not really implemented.
> 
> > I'm guessing that's not what they're supposed to do, which is why
> > you're removing them, but that needs to be explained in the commit
> > message.
> 
> Maybe adding this explanation:
> 
> We dont emulate slave mode so related registers are not needed. [lh]sadr are
> only retained to avoid too many warnings and simplify debugging but sdata is
> not even correct because device has a 4 byte FIFO instead so just remove
> this unimplemented register for now.
> 
> The intr register is also not implemented correctly, it is for diagnostics
> and normally not even visible on device without explicitly enabling it. As
> no guests are known to need this remove it as well.

That sounds reasonable.

> 
> Regards,
> BALATON Zoltan
> 
> > As a general rule cases where a one line commit message is acceptable
> > are *very* rare.
> > 
> > > ---
> > >  hw/i2c/ppc4xx_i2c.c         | 16 +---------------
> > >  include/hw/i2c/ppc4xx_i2c.h |  4 +---
> > >  2 files changed, 2 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > index d1936db..4e0aaae 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
> > > @@ -63,7 +63,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 +70,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)
> > > @@ -139,9 +137,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 +176,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%"
> > > @@ -229,9 +221,6 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr 
> > > addr, uint64_t value,
> > >              }
> > >          }
> > >          break;
> > > -    case 2:
> > > -        i2c->sdata = value;
> > > -        break;
> > >      case 4:
> > >          i2c->lmadr = value;
> > >          if (i2c_bus_busy(i2c->bus)) {
> > > @@ -302,9 +291,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%"
> > > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> > > index 3c60307..e4b6ded 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
> > > @@ -49,7 +49,6 @@ typedef struct PPC4xxI2CState {
> > >      uint8_t mdcntl;
> > >      uint8_t sts;
> > >      uint8_t extsts;
> > > -    uint8_t sdata;
> > >      uint8_t lsadr;
> > >      uint8_t hsadr;
> > >      uint8_t clkdiv;
> > > @@ -57,7 +56,6 @@ typedef struct PPC4xxI2CState {
> > >      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

signature.asc
Description: PGP signature