1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH v3 0/9] Misc sam460ex improvements
  4. View patch

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

BALATON Zoltan posted 9 patches 7 years, 8 months ago
There is a newer version of this series
[Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers
Posted by BALATON Zoltan 7 years, 8 months ago 
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 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\ufffd\ufffdois 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\ufffd\ufffdois 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 */
-- 
2.7.6
Re: [Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers
Posted by David Gibson 7 years, 8 months ago 
On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

But.. they are implemented.  Albeit as an entirely software controlled
register.

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.

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
Re: [Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers
Posted by BALATON Zoltan 7 years, 8 months ago 
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 <balaton@eik.bme.hu>
>
> 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.

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 */
>
>
Re: [Qemu-devel] [PATCH v3 1/9] ppc4xx_i2c: Remove unimplemented sdata and intr registers
Posted by David Gibson 7 years, 7 months ago 
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 <balaton@eik.bme.hu>
> > 
> > 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.