[v2,1/8] ppc4xx_i2c: Clean up and improve error logging

Make it more readable by converting register indexes to decimal
(avoids lot of superfluous 0x0) and distinguish errors caused by
accessing non-existent vs. unimplemented registers.
No functional change.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> Make it more readable by converting register indexes to decimal
> (avoids lot of superfluous 0x0) and distinguish errors caused by
> accessing non-existent vs. unimplemented registers.
> No functional change.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
>  1 file changed, 51 insertions(+), 43 deletions(-)
> 
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index ab64d19..d1936db 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -31,7 +31,7 @@
>  #include "hw/hw.h"
>  #include "hw/i2c/ppc4xx_i2c.h"
>  
> -#define PPC4xx_I2C_MEM_SIZE 0x12
> +#define PPC4xx_I2C_MEM_SIZE 18

This looks weird, it seems all memory range sizes are in hex in other
QEMU devices.

>  
>  #define IIC_CNTL_PT         (1 << 0)
>  #define IIC_CNTL_READ       (1 << 1)
> @@ -70,7 +70,7 @@ static void ppc4xx_i2c_reset(DeviceState *s)
>      i2c->intrmsk = 0;
>      i2c->xfrcnt = 0;
>      i2c->xtcntlss = 0;
> -    i2c->directcntl = 0x0f;
> +    i2c->directcntl = 0xf;
>      i2c->intr = 0;
>  }
>  
> @@ -85,7 +85,7 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>      uint64_t ret;
>  
>      switch (addr) {
> -    case 0x00:
> +    case 0:
>          ret = i2c->mdata;
>          if (ppc4xx_i2c_is_master(i2c)) {
>              ret = 0xff;
> @@ -139,58 +139,62 @@ static uint64_t ppc4xx_i2c_readb(void *opaque, hwaddr addr, unsigned int size)
>                            TYPE_PPC4xx_I2C, __func__);
>          }
>          break;
> -    case 0x02:
> +    case 2:
>          ret = i2c->sdata;
>          break;
> -    case 0x04:
> +    case 4:
>          ret = i2c->lmadr;
>          break;
> -    case 0x05:
> +    case 5:
>          ret = i2c->hmadr;
>          break;
> -    case 0x06:
> +    case 6:
>          ret = i2c->cntl;
>          break;
> -    case 0x07:
> +    case 7:
>          ret = i2c->mdcntl;
>          break;
> -    case 0x08:
> +    case 8:
>          ret = i2c->sts;
>          break;
> -    case 0x09:
> +    case 9:
>          ret = i2c->extsts;
>          break;
> -    case 0x0A:
> +    case 10:
>          ret = i2c->lsadr;
>          break;
> -    case 0x0B:
> +    case 11:
>          ret = i2c->hsadr;
>          break;
> -    case 0x0C:
> +    case 12:
>          ret = i2c->clkdiv;
>          break;
> -    case 0x0D:
> +    case 13:
>          ret = i2c->intrmsk;
>          break;
> -    case 0x0E:
> +    case 14:
>          ret = i2c->xfrcnt;
>          break;
> -    case 0x0F:
> +    case 15:
>          ret = i2c->xtcntlss;
>          break;
> -    case 0x10:
> +    case 16:
>          ret = i2c->directcntl;
>          break;
> -    case 0x11:
> +    case 17:
>          ret = i2c->intr;
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
> -                      HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
> +        if (addr < PPC4xx_I2C_MEM_SIZE) {
> +            qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> +                          HWADDR_PRIx "\n", __func__, addr);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
> +                          HWADDR_PRIx "\n", __func__, addr);
> +        }
>          ret = 0;
>          break;
>      }
> -
>      return ret;
>  }
>  
> @@ -200,7 +204,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>      PPC4xxI2CState *i2c = opaque;
>  
>      switch (addr) {
> -    case 0x00:
> +    case 0:
>          i2c->mdata = value;
>          if (!i2c_bus_busy(i2c->bus)) {
>              /* assume we start a write transfer */
> @@ -225,19 +229,19 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>              }
>          }
>          break;
> -    case 0x02:
> +    case 2:
>          i2c->sdata = value;
>          break;
> -    case 0x04:
> +    case 4:
>          i2c->lmadr = value;
>          if (i2c_bus_busy(i2c->bus)) {
>              i2c_end_transfer(i2c->bus);
>          }
>          break;
> -    case 0x05:
> +    case 5:
>          i2c->hmadr = value;
>          break;
> -    case 0x06:
> +    case 6:
>          i2c->cntl = value;
>          if (i2c->cntl & IIC_CNTL_PT) {
>              if (i2c->cntl & IIC_CNTL_READ) {
> @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>              }
>          }
>          break;
> -    case 0x07:
> -        i2c->mdcntl = value & 0xDF;
> +    case 7:
> +        i2c->mdcntl = value & 0xdf;
>          break;
> -    case 0x08:
> -        i2c->sts &= ~(value & 0x0A);
> +    case 8:
> +        i2c->sts &= ~(value & 0xa);

'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
Matter of taste...

>          break;
> -    case 0x09:
> -        i2c->extsts &= ~(value & 0x8F);
> +    case 9:
> +        i2c->extsts &= ~(value & 0x8f);
>          break;
> -    case 0x0A:
> +    case 10:
>          i2c->lsadr = value;
> -        /*i2c_set_slave_address(i2c->bus, i2c->lsadr);*/
>          break;
> -    case 0x0B:
> +    case 11:
>          i2c->hsadr = value;
>          break;
> -    case 0x0C:
> +    case 12:
>          i2c->clkdiv = value;
>          break;
> -    case 0x0D:
> +    case 13:
>          i2c->intrmsk = value;
>          break;
> -    case 0x0E:
> +    case 14:
>          i2c->xfrcnt = value & 0x77;
>          break;
> -    case 0x0F:
> +    case 15:
>          if (value & IIC_XTCNTLSS_SRST) {
>              /* Is it actually a full reset? U-Boot sets some regs before */
>              ppc4xx_i2c_reset(DEVICE(i2c));
> @@ -296,15 +299,20 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>          }
>          i2c->xtcntlss = value;
>          break;
> -    case 0x10:
> +    case 16:
>          i2c->directcntl = value & 0x7;
>          break;
> -    case 0x11:
> +    case 17:
>          i2c->intr = value;
>          break;
>      default:
> -        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad address at offset 0x%"
> -                      HWADDR_PRIx "\n", TYPE_PPC4xx_I2C, __func__, addr);
> +        if (addr < PPC4xx_I2C_MEM_SIZE) {
> +            qemu_log_mask(LOG_UNIMP, "%s: Unimplemented register 0x%"
> +                          HWADDR_PRIx "\n", __func__, addr);
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad address 0x%"
> +                          HWADDR_PRIx "\n", __func__, addr);
> +        }
>          break;
>      }
>  }
>

On Wed, Jun 06, 2018 at 12:56:32PM -0300, Philippe Mathieu-Daudé wrote:
> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
> > Make it more readable by converting register indexes to decimal
> > (avoids lot of superfluous 0x0) and distinguish errors caused by
> > accessing non-existent vs. unimplemented registers.
> > No functional change.
> > 
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >  hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
> >  1 file changed, 51 insertions(+), 43 deletions(-)
> > 
> > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > index ab64d19..d1936db 100644
> > --- a/hw/i2c/ppc4xx_i2c.c
> > +++ b/hw/i2c/ppc4xx_i2c.c
> > @@ -31,7 +31,7 @@
> >  #include "hw/hw.h"
> >  #include "hw/i2c/ppc4xx_i2c.h"
> >  
> > -#define PPC4xx_I2C_MEM_SIZE 0x12
> > +#define PPC4xx_I2C_MEM_SIZE 18
> 
> This looks weird, it seems all memory range sizes are in hex in other
> QEMU devices.

[snip]
> > @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> >              }
> >          }
> >          break;
> > -    case 0x07:
> > -        i2c->mdcntl = value & 0xDF;
> > +    case 7:
> > +        i2c->mdcntl = value & 0xdf;
> >          break;
> > -    case 0x08:
> > -        i2c->sts &= ~(value & 0x0A);
> > +    case 8:
> > +        i2c->sts &= ~(value & 0xa);
> 
> 'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
> Matter of taste...

I tend to prefer the forms you suggest, Philippe, but not by enough to
delay this otherwise good cleanup.  Especially since Balaton is taking
on this long neglected area of the code.

On Fri, 8 Jun 2018, David Gibson wrote:
> On Wed, Jun 06, 2018 at 12:56:32PM -0300, Philippe Mathieu-Daudé wrote:
>> On 06/06/2018 10:31 AM, BALATON Zoltan wrote:
>>> Make it more readable by converting register indexes to decimal
>>> (avoids lot of superfluous 0x0) and distinguish errors caused by
>>> accessing non-existent vs. unimplemented registers.
>>> No functional change.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/i2c/ppc4xx_i2c.c | 94 +++++++++++++++++++++++++++++------------------------
>>>  1 file changed, 51 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>> index ab64d19..d1936db 100644
>>> --- a/hw/i2c/ppc4xx_i2c.c
>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>> @@ -31,7 +31,7 @@
>>>  #include "hw/hw.h"
>>>  #include "hw/i2c/ppc4xx_i2c.h"
>>>
>>> -#define PPC4xx_I2C_MEM_SIZE 0x12
>>> +#define PPC4xx_I2C_MEM_SIZE 18
>>
>> This looks weird, it seems all memory range sizes are in hex in other
>> QEMU devices.
>
> [snip]
>>> @@ -263,32 +267,31 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>              }
>>>          }
>>>          break;
>>> -    case 0x07:
>>> -        i2c->mdcntl = value & 0xDF;
>>> +    case 7:
>>> +        i2c->mdcntl = value & 0xdf;
>>>          break;
>>> -    case 0x08:
>>> -        i2c->sts &= ~(value & 0x0A);
>>> +    case 8:
>>> +        i2c->sts &= ~(value & 0xa);
>>
>> 'value & 0x0a' implicitly denotes than 'value' is a 8-bit register.
>> Matter of taste...
>
> I tend to prefer the forms you suggest, Philippe, but not by enough to
> delay this otherwise good cleanup.  Especially since Balaton is taking
> on this long neglected area of the code.

I prefer code that does not have unneeded chars so it's easier to read, 
that's why I've removed all 0x0 from this file which made it more 
comprehensible. But I'll add the 0 back to this place in next respin as 
you both seem to prefer that and since other bit masks are two digit too 
it's also consistent that way.